Re: Oversight in reparameterize_path_by_child leading to executor crash

On Fri, Oct 20, 2023 at 2:52 AM Alena Rybakina <lena(dot)ribackina(at)yandex(dot)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