Re: Regression on PostgreSQL 10 ORDER/GROUP BY expression not found in targetlist - Mailing list pgsql-hackers

I wrote:
> "Regina Obe" <lr@pcorp.us> writes:
>> Here is a trivial example to exercise that doesn't involve PostGIS.
>> CREATE TABLE edge AS SELECT 1 AS edge_id;
>> SELECT 't3280', 'L1b' || generate_series(1,10)
>> FROM edge
>> ORDER BY 1;

> That's odd.  It works fine in HEAD but not the v10 branch.  I suspect
> that the fix David mentioned failed to cover some case related to the
> issue we saw last week ... but why the cross-version difference?

So the proximate cause of this problem is that we're applying a pathtarget
that contains the Const 't3280', marked with sortgroupref 1, to a seqscan
path.  We later conclude that we don't really care about the Const at the
seqscan level (it'll get generated in ProjectSet instead), so during
createplan.c, use_physical_tlist returns TRUE for the seqscan node.
That's fine ... but then create_scan_plan does this:

            /*
             * Transfer any sortgroupref data to the replacement tlist, unless
             * we don't care because the gating Result will handle it.
             */
            if (!gating_clauses)
                apply_pathtarget_labeling_to_tlist(tlist, best_path->pathtarget);

and apply_pathtarget_labeling_to_tlist chokes because there is noplace
to transfer the Const's sortgroupref label to.  So this is just wrong;
it should read more like the similar code in create_projection_plan:

        if ((flags & CP_LABEL_TLIST) != 0)
            apply_pathtarget_labeling_to_tlist(tlist, best_path->pathtarget);

But that's been wrong since this code was introduced in 9.6.  Why isn't
HEAD failing similarly?  Apparently, the planner.c code restructuring in
11cf92f6e, or possibly its predecessor e2f1eb0ee, changed the path tree
that's produced for this case.  Since the commit log entries for those
patches make no mention of intention to change the behavior for non
parallel and non partitioned cases, I would imagine that that's a bug in
itself.

I also think that there's some horribly unsafe coding in
apply_scanjoin_target_to_paths: it clobbers a RelOptInfo's reltarget,
with no thought for whether that might affect things elsewhere,
and it also clobbers individual paths' PathTargets like so:

       if (tlist_same_exprs)
           subpath->pathtarget->sortgrouprefs =
               scanjoin_target->sortgrouprefs;

with similarly little regard for whether those PathTarget structures are
shared with anything else.

I've run out of time to poke at this for today, though, so I do not have
test cases that prove this code is broken.

            regards, tom lane