Re: Oversight in reparameterize_path_by_child leading to executor crash

Richard Guo <guofenglinux@gmail.com> writes:
> On Wed, Jan 17, 2024 at 5:01 PM Richard Guo <guofenglinux@gmail.com> wrote:
>> Sure, here it is:
>> v10-0001-Avoid-reparameterizing-Paths-when-it-s-not-suitable.patch

> I forgot to mention that this patch applies on v16 not master.

I spent some time looking at this patch (which seems more urgent than
the patch for master, given that we have back-branch releases coming
up).  There are two things I'm not persuaded about:

* Why is it okay to just use pull_varnos on a tablesample expression,
when contain_references_to() does something different?

* Is contain_references_to() doing the right thing by specifying
PVC_RECURSE_PLACEHOLDERS?  That causes it to totally ignore a
PlaceHolderVar's ph_eval_at, and I'm not convinced that's correct.

Ideally it seems to me that we want to reject a PlaceHolderVar
if either its ph_eval_at or ph_lateral overlap the other join
relation; if it was coded that way then we'd not need to recurse
into the PHV's contents.   pull_varnos isn't directly amenable
to this, but I think we could use pull_var_clause with
PVC_INCLUDE_PLACEHOLDERS and then iterate through the resulting
list manually.  (If this patch were meant for HEAD, I'd think
about extending the var.c code to support this usage more directly.
But as things stand, this is a one-off so I think we should just do
what we must in reparameterize_path_by_child.)

BTW, it shouldn't be necessary to write either PVC_RECURSE_AGGREGATES
or PVC_RECURSE_WINDOWFUNCS, because neither kind of node should ever
appear in a scan-level expression.  I'd leave those out so that we
get an error if something unexpected happens.

            regards, tom lane