Horton

Re: Oversight in reparameterize_path_by_child leading to executor crash

Richard Guo <guofenglinux@gmail.com>

View thread
From: Richard Guo <guofenglinux@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2023-08-21T10:39:18Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Postpone reparameterization of paths until create_plan().

  2. Apply band-aid fix for an oversight in reparameterize_path_by_child.

Attachments

On Sun, Aug 20, 2023 at 11:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I looked over the v3 patch.  I think it's going in generally
> the right direction, but there is a huge oversight:
> path_is_reparameterizable_by_child does not come close to modeling
> the success/failure conditions of reparameterize_path_by_child.
> In all the cases where reparameterize_path_by_child can recurse,
> path_is_reparameterizable_by_child has to do so also, to check
> whether the child path is reparameterizable.  (I'm somewhat
> disturbed that apparently we have no test cases that caught that
> oversight; can we add one cheaply?)


Thanks for pointing this out.  It's my oversight.  We have to check the
child path(s) recursively to tell if a path is reparameterizable or not.
I've fixed this in v4 patch, along with a test case.  In the test case
we have a MemoizePath whose subpath is TidPath, which is not
reparameterizable.  This test case would trigger Assert in v3 patch.


> BTW, I also note from the cfbot that 9e9931d2b broke this patch by
> adding more ADJUST_CHILD_ATTRS calls that need to be modified.
> I wonder if we could get away with having that macro cast to "void *"
> to avoid needing to change all its call sites.  I'm not sure whether
> pickier compilers might warn about doing it that way.


Agreed and have done that in v4 patch.

Thanks
Richard