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: Alena Rybakina <lena.ribackina@yandex.ru>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Andrei Lepikhov <a.lepikhov@postgrespro.ru>
Date: 2023-12-06T06:51:27Z
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.

On Fri, Oct 20, 2023 at 2:52 AM Alena Rybakina <lena.ribackina@yandex.ru>
wrote:

> Thank you for your work on the subject.
>
Thanks for taking an interest in this patch.


> During review your patch I didn't understand why are you checking that the
> variable is path and not new_path of type T_SamplePath (I highlighted it)?
>
> Is it a typo and should be new_path?
>
I don't think there is any difference: path and new_path are the same
pointer in this case.


> Besides, it may not be important, but reviewing your code all the time
> stumbled on the statement of the comments while reading it (I highlighted
> it too). This:
>
> * path_is_reparameterizable_by_child
>  *         Given a path parameterized by the parent of the given child
> relation,
>  *         see if it can be translated to be parameterized by the child
> relation.
>  *
>  * This must return true if *and only if *reparameterize_path_by_child()
>  * would succeed on this path.
>
> Maybe is it better to rewrite it simplier:
>
>  * This must return true *only if *reparameterize_path_by_child()
>  * would succeed on this path.
>
I don't think so.  "if and only if" is more accurate to me.


> And can we add assert in reparameterize_pathlist_by_child function that
> pathlist is not a NIL, because according to the comment it needs to be
> added there:
>
Hmm, I'm not sure, as in REPARAMETERIZE_CHILD_PATH_LIST we have already
explicitly checked that the pathlist is not NIL.

Thanks
Richard