Horton

Re: Oversight in reparameterize_path_by_child leading to executor crash

Alena Rybakina <lena.ribackina@yandex.ru>

View thread
From: Alena Rybakina <lena.ribackina@yandex.ru>
To: Richard Guo <guofenglinux@gmail.com>
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-10-19T18:52:00Z
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.

Hi!

Thank you for your work on the subject.


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)?

Path *
reparameterize_path_by_child(PlannerInfo *root, Path *path,
  RelOptInfo *child_rel)

...
switch (nodeTag(path))
     {
         case T_Path:
             new_path = path;
ADJUST_CHILD_ATTRS(new_path->parent->baserestrictinfo);
             if (*path*->pathtype == T_SampleScan)
             {

Is it a typo and should be new_path?


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.


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:

Returns NIL to indicate failure, so pathlist had better not be NIL.

-- 
Regards,
Alena Rybakina