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

On Mon, Jun 13, 2016 at 6:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Again, please have a look at the patch and see if it seems unsuitable
>> to you for some reason. I'm not confident of my ability to get all of
>> this path stuff right without a bit of help from the guy who designed
>> it.
>
> I'm kind of in a bind right now because Tomas has produced an
> FK-selectivity patch rewrite on schedule, and I already committed to
> review that before beta2, so I better go do that first. If you can wait
> awhile I will try to help out more with parallel query.

I'm happy to have you look at his patch first.

> Having said that, the main thing I noticed in your draft patch is that
> you'd removed set_grouped_rel_consider_parallel() in favor of hard-wiring
> this in create_grouping_paths():
>
> + if (input_rel->consider_parallel &&
> + !has_parallel_hazard((Node *) target->exprs, false) &&
> + !has_parallel_hazard((Node *) parse->havingQual, false))
> + grouped_rel->consider_parallel = true;
>
> 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?

> Also, I wonder whether we could reduce that test to just the
> has_parallel_hazard tests, so as to avoid the ordering dependency of
> needing to create the top scan/join rel (and set its consider_parallel
> flag) before we can create the UPPERREL_GROUP_AGG rel. This would mean
> putting more dependency on per-path parallel_safe flags to detect whether
> you can't parallelize a given step for lack of parallel-safe input, but
> I'm not sure that that's a bad thing.

It doesn't sound like an especially good thing to me. Skipping all
parallel path generation is quite a bit less work than trying each
path in turn and realizing that none of them will work, and there are
various places where we optimize on that basis. I don't understand,
in any event, why it makes any sense to create the UPPERREL_GROUP_AGG
rel before we finish scan/join planning.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company