Re: Wrong results from a parameterized Append

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> I ran into a wrong-result issue on master (also reproducible on v16
> and later, I believe).

FWIW, this example reproduces for me on v17 and up, but not v16.
It might be worth determining why. A quick bisect run says that it
broke at

9f133763961e280d8ba692bcad0b061b861e9138 is the first bad commit
commit 9f133763961e280d8ba692bcad0b061b861e9138
Author: Alexander Korotkov <akorotkov(at)postgresql(dot)org>
Date: Thu Feb 15 12:05:52 2024 +0200

Pull up ANY-SUBLINK with the necessary lateral support.

but I've not dug further than that.

> I believe the same happens with any path that inherits its subpath's
> ParamPathInfo: Memoize, Projection, Sort, IncrementalSort, Unique, Agg
> and GroupingSets, although I don't have proof. Attached is a patch
> that makes get_param_path_clause_serials() look through those paths,
> and uses it in get_memoize_path() too, which reads ppi_serials
> directly.

I think you're on to something, but this patch has a hasty look to
it. AFAICS, the root of the problem is that the wrapper-path-making
functions in pathnode.c just do

pathnode->path.param_info = subpath->param_info;

and we now see that that isn't good enough. I wonder if we should try
harder. At the very least I think it'd be advisable to make all of
those look like, say,

pathnode->path.param_info = get_wrapper_parampathinfo(subpath);

Even if get_wrapper_parampathinfo doesn't do anything beyond handing
back the subpath's param_info, it would provide a place for a comment
discussing what it's doing and why.

However, I feel like we should consider a redesign rather than just
a minimal fix. In particular, it took me a bit to reconstruct why
the code looks like it does now. I think it's this:

(1) Joins need special handling in get_param_path_clause_serials
because different join input pairs for the same result join may have
different joinrestrictinfo lists and thus enforce different sets of
rinfo serials; so we can't compute a ppi_serials value in advance.

(2) AppendPath needs special handling in case there are joins under
it, which would only occur for partitionwise joining. Otherwise
we could have precomputed the correct ppi_serials to begin with.
(I didn't check the code history, but I wonder if somebody rewrote
this for partitionwise joining and did not bother to add a comment
explaining that.)

But if that is the situation, then what about a wrapper path that
wraps a join? Seems like we need to be able to recurse and
compute a context-dependent set of ppi_serials for that too.

Now we could just recurse always, which is what your patch does, but
that feels annoyingly inefficient. I am wondering about a redesign
that would allow marking ParamPathInfos as context-dependent (if they
are for a join or something with a join under it) or not (everything
else). Then we only need to recurse when the hard cases occur,
and otherwise we can just believe the ppi_serials field.

Also, as a more nitpicky objection, I'd be inclined to refactor
get_param_path_clause_serials so that the whole thing is one big
switch over nodeTag(path), rather than the messy structure your
v1 patch would leave behind.

regards, tom lane