Re: Oversight in reparameterize_path_by_child leading to executor crash

I spent some time reviewing the v4 patch. I noted that
path_is_reparameterizable_by_child still wasn't modeling the pass/fail
behavior of reparameterize_path_by_child very well, because that
function checks this at every recursion level:

/*
* If the path is not parameterized by the parent of the given relation,
* it doesn't need reparameterization.
*/
if (!path->param_info ||
!bms_overlap(PATH_REQ_OUTER(path), child_rel->top_parent_relids))
return path;

So we might have a sub-path (eg a join input rel) that is not one of
the supported kinds, and yet we can succeed because parameterization
appears only in other sub-paths. The patch as written will not crash
in such a case, but it might refuse to use a path that we previously
would have allowed. So I think we need to put the same test into
path_is_reparameterizable_by_child, which requires adding child_rel
to its param list but is otherwise simple enough.

I also realized that this test is equivalent to
PATH_PARAM_BY_PARENT(), which makes it really unnecessary for
createplan.c to test PATH_PARAM_BY_PARENT, so we don't need to
expose those macros globally after all.

(On the same logic, we could skip PATH_PARAM_BY_PARENT at the
call sites of path_is_reparameterizable_by_child. I didn't
do that in the attached, mainly because it seems to make it
harder to understand/explain what is being tested.)

Another change I made is to put the path_is_reparameterizable_by_child
tests before the initial_cost_nestloop/add_path_precheck steps, on
the grounds that they're now cheap enough that we might as well do
them first. The existing ordering of these steps was sensible when
we were doing the expensive reparameterization, but it seems a bit
unnatural IMO.

Lastly, although I'd asked for a test case demonstrating detection of
an unparameterizable sub-path, I ended up not using that, because
it seemed pretty fragile. If somebody decides that
reparameterize_path_by_child ought to cover TidPaths, the test won't
prove anything any more.

So that led me to the attached v5, which seemed committable to me so I
set about back-patching it ... and it fell over immediately in v15, as
shown in the attached regression diffs from v15. It looks to me like
we are now failing to recognize that reparameterized quals are
redundant with not-reparameterized ones, so this patch is somehow
dependent on restructuring that happened during the v16 cycle.
I don't have time to dig deeper than that, and I'm not sure that that
is an area we'd want to mess with in a back-patched bug fix.

What I'm thinking we ought to do instead for the back branches
is just refuse to generate a reparameterized path for tablesample
scans. A minimal fix like that could be as little as

case T_Path:
+ if (path->pathtype == T_SampleScan)
+ return NULL;
FLAT_COPY_PATH(new_path, path, Path);
break;

This rejects more than it absolutely has to, because the
parameterization (that we know exists) might be in the path's
regular quals or tlist rather than in the tablesample.
So we could add something to see if the tablesample is
parameter-free, but I'm quite unsure that it's worth the
trouble. There must be just about nobody using such cases,
or we'd have heard of this bug long ago.

(BTW, I did look at Ashutosh's idea of merging the
reparameterize_path_by_child and path_is_reparameterizable_by_child
functions, but I didn't think that would be an improvement,
because we'd have to clutter reparameterize_path_by_child with
a lot of should-I-skip-this-step tests. Some of that could be
hidden in the macros, but a lot would not be. Another issue
is that I do not think we can change reparameterize_path_by_child's
API contract in the back branches, because we advertise it as
something that FDWs and custom scan providers can use.)

Thoughts?

regards, tom lane