Re: ERROR: ORDER/GROUP BY expression not found in targetlist

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Jun 13, 2016 at 6:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I think this is bad because it forces any external creators of
>> UPPERREL_GROUP_AGG to duplicate that code --- and heaven help everybody
>> if anyone is out of sync on whether to set the flag. So I'd rather keep
>> set_grouped_rel_consider_parallel(), even if all it does is the above.
>> And make it global not static. Ditto for the other upperrels.

> I'm slightly mystified by this because we really shouldn't be setting
> that flag more than once. We don't want to do that work repeatedly,
> just once, and prior to adding any paths to the rel. Are you
> imagining that somebody would try to created grouped paths before we
> finish scan/join planning?

Exactly. The original pathification design contemplated that FDWs and
other extensions could inject Paths for the upperrels whenever they felt
like it, specifically during relation path building. Even if you think
that's silly, it *must* be possible to do it during GetForeignUpperPaths,
else that hook is completely useless. Therefore, adding any data other
than new Paths to the upperrel during create_grouping_paths is a broken
design unless there is some easy, reliable, and not too expensive way for
an FDW to make the same thing happen a bit earlier. See the commentary in
optimizer/README starting at line 922.

I generally don't like rel->consider_parallel at all for basically this
reason, but it may be too late to undo that aspect of the parallel join
planning design. I will settle for having an API call that allows FDWs
to get the flag set correctly. Another possibility is to set the flag
before calling GetForeignUpperPaths, but that might be messy too.

regards, tom lane