Not deleted mentions of the cleared path

On Mon, Oct 30, 2023 at 5:01 PM Alena Rybakina
<a.rybakina@postgrespro.ru> wrote:

Hi, hackers!

I have already written about the problem of InvalidPath [0] appearing. I investigated this and found an error in the add_path() function, when we reject a path, we free up the memory of the path, but do not delete various mentions of it (for example, in the ancestor of relation, as in the example below).

Thus, we may face the problem of accessing the freed memory.

I demonstrated this below using gdb when I call a query after running a test in test/regression/sql/create_misc.sql:

Query:

:-- That ALTER TABLE should have added TOAST tables.

SELECT relname, reltoastrelid <> 0 AS has_toast_table
FROM pg_class
WHERE oid::regclass IN ('a_star', 'c_star')
ORDER BY 1;

--UPDATE b_star*
-- SET a = text 'gazpacho'
-- WHERE aa > 4;

SELECT class, aa, a FROM a_star*;

gdb:

0x00007ff3f7325fda in epoll_wait (epfd=5, events=0x55bf9ee75c38, maxevents=1, timeout=-1) at ../sysdeps/unix/sysv/linux/epoll_wait.c:30
30 ../sysdeps/unix/sysv/linux/epoll_wait.c: No such file or directory.
(gdb) b /home/alena/postgrespro_3/src/backend/optimizer/util/pathnode.c:620
Breakpoint 1 at 0x55bf9cfe4c65: file pathnode.c, line 621.
(gdb) c
Continuing.

Breakpoint 1, add_path (parent_rel=0x55bf9ef7f5c0, new_path=0x55bf9ef7f4e0) at pathnode.c:621
621 if (!IsA(new_path, IndexPath))
(gdb) n
622 pfree(new_path);
(gdb) n
624 }
(gdb) p *new_path
$1 = {type = T_Invalid, pathtype = T_Invalid, parent = 0x7f7f7f7f7f7f7f7f, pathtarget = 0x7f7f7f7f7f7f7f7f,
param_info = 0x7f7f7f7f7f7f7f7f, parallel_aware = 127, parallel_safe = 127, parallel_workers = 2139062143,
rows = 1.3824172084878715e+306, startup_cost = 1.3824172084878715e+306, total_cost = 1.3824172084878715e+306,
pathkeys = 0x7f7f7f7f7f7f7f7f}
(gdb) p new_path
$2 = (Path *) 0x55bf9ef7f4e0

At this point the new_path has not been added to the parent_rel. We do
not set the cheapest* paths while paths are being added. The stack
trace will give you an idea where this is happening.

(gdb) p ((ProjectionPath *)((SortPath*)parent_rel->pathlist->elements [0].ptr_value)->subpath)->path->parent->cheapest_startup_path
$20 = (struct Path *) 0x55bf9ef7f4e0

This looks familiar though. There was some nearby thread where Tom
Lane, if my memory serves well, provided a case where a path from
lower rel was added to an upper rel without copying or changing its
parent. This very much looks like that case.

--
Best Wishes,
Ashutosh Bapat

On Mon, Oct 30, 2023 at 7:31 PM Alena Rybakina <a.rybakina@postgrespro.ru>
wrote:

I have already written about the problem of InvalidPath [0] appearing. I
investigated this and found an error in the add_path() function, when we
reject a path, we free up the memory of the path, but do not delete various
mentions of it (for example, in the ancestor of relation, as in the example
below).

I agree that what you observed is true - add_path() may free a path
while it's still referenced from some lower rels. For instance, when
creating ordered paths, we may use the input path unchanged without
copying if it's already well ordered, and it might be freed afterwards
if it fails when competing in add_path().

But this doesn't seem to be a problem in practice. We will not access
these references from the lower rels.

I'm not sure if this is an issue that we need to fix, or we need to live
with. But I do think it deserves some explanation in the comment of
add_path().

Thanks
Richard

Hi! Thank you for the interest to this issue.

On 31.10.2023 06:25, Richard Guo wrote:

On Mon, Oct 30, 2023 at 7:31 PM Alena Rybakina <a.rybakina@postgrespro.ru> wrote:

I have already written about the problem of InvalidPath [0]
appearing. I investigated this and found an error in the add_path()
function, when we reject a path, we free up the memory of the path,
but do not delete various mentions of it (for example, in the
ancestor of relation, as in the example below).

I agree that what you observed is true - add_path() may free a path
while it's still referenced from some lower rels.  For instance, when
creating ordered paths, we may use the input path unchanged without
copying if it's already well ordered, and it might be freed afterwards
if it fails when competing in add_path().
But this doesn't seem to be a problem in practice.  We will not access
these references from the lower rels.
I'm not sure if this is an issue that we need to fix, or we need to live
with.  But I do think it deserves some explanation in the comment of
add_path().

I agree that the code looks like an error, but without a real request,
it is still difficult to identify it as a bug. I'll try to reproduce it.
And yes, at least a comment is required here, and to be honest, I have
already faced this problem myself.

On 30.10.2023 17:36, Ashutosh Bapat wrote:

On Mon, Oct 30, 2023 at 5:01 PM Alena Rybakina
<a.rybakina@postgrespro.ru> wrote:

Hi, hackers!

I have already written about the problem of InvalidPath [0] appearing. I investigated this and found an error in the add_path() function, when we reject a path, we free up the memory of the path, but do not delete various mentions of it (for example, in the ancestor of relation, as in the example below).

Thus, we may face the problem of accessing the freed memory.

I demonstrated this below using gdb when I call a query after running a test in test/regression/sql/create_misc.sql:

Query:

:-- That ALTER TABLE should have added TOAST tables.

SELECT relname, reltoastrelid <> 0 AS has_toast_table
FROM pg_class
WHERE oid::regclass IN ('a_star', 'c_star')
ORDER BY 1;

--UPDATE b_star*
-- SET a = text 'gazpacho'
-- WHERE aa > 4;

SELECT class, aa, a FROM a_star*;

gdb:

0x00007ff3f7325fda in epoll_wait (epfd=5, events=0x55bf9ee75c38, maxevents=1, timeout=-1) at ../sysdeps/unix/sysv/linux/epoll_wait.c:30
30 ../sysdeps/unix/sysv/linux/epoll_wait.c: No such file or directory.
(gdb) b /home/alena/postgrespro_3/src/backend/optimizer/util/pathnode.c:620
Breakpoint 1 at 0x55bf9cfe4c65: file pathnode.c, line 621.
(gdb) c
Continuing.

Breakpoint 1, add_path (parent_rel=0x55bf9ef7f5c0, new_path=0x55bf9ef7f4e0) at pathnode.c:621
621 if (!IsA(new_path, IndexPath))
(gdb) n
622 pfree(new_path);
(gdb) n
624 }
(gdb) p *new_path
$1 = {type = T_Invalid, pathtype = T_Invalid, parent = 0x7f7f7f7f7f7f7f7f, pathtarget = 0x7f7f7f7f7f7f7f7f,
param_info = 0x7f7f7f7f7f7f7f7f, parallel_aware = 127, parallel_safe = 127, parallel_workers = 2139062143,
rows = 1.3824172084878715e+306, startup_cost = 1.3824172084878715e+306, total_cost = 1.3824172084878715e+306,
pathkeys = 0x7f7f7f7f7f7f7f7f}
(gdb) p new_path
$2 = (Path *) 0x55bf9ef7f4e0

At this point the new_path has not been added to the parent_rel. We do
not set the cheapest* paths while paths are being added. The stack
trace will give you an idea where this is happening.

(gdb) p ((ProjectionPath *)((SortPath*)parent_rel->pathlist->elements [0].ptr_value)->subpath)->path->parent->cheapest_startup_path
$20 = (struct Path *) 0x55bf9ef7f4e0

This looks familiar though. There was some nearby thread where Tom
Lane, if my memory serves well, provided a case where a path from
lower rel was added to an upper rel without copying or changing its
parent. This very much looks like that case.

Thank you, I think this might help me to find a query to reproduce it.