Re: [COMMITTERS] pgsql: Don't generate parallel paths for rels with parallel-restricted

On Thu, Jun 9, 2016 at 2:08 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Thu, Jun 9, 2016 at 1:44 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Well, yeah, you could remove it. It's inappropriate. The right place
>>> for such an error check is an attempt to actually access any data within
>>> a temp table, ie someplace in localbuf.c. There is no reason a worker
>>> shouldn't be allowed to look at the catalog entries for a temp table;
>>> they're just like any other catalog entries.
>
>> That's a possibility. Do you think it's a good idea to go making such
>> changes right now, with beta2 just around the corner? Do you want to
>> work on it? Are you asking me to work on it?
>
> I'll do it, if you don't want to. The rowtype test in has_parallel_hazard
> has made me acutely uncomfortable since I first saw it, because I don't
> think it's either maintainable or adequate for its alleged purpose.
> Never mind that it makes has_parallel_hazard probably several times slower
> than it needs to be.

It's been on my list of things to look into for a long time, but I
don't think it's likely to make it to the top in the next week. So if
you feel motivated to have a whack at it, that's great. I have not
been convinced that it is entirely straightforward, but maybe it is.

>> There's one other related thing I'm concerned about, which is that the
>> code in namespace.c that manages pg_temp doesn't know anything about
>> parallelism. So it will interpret pg_temp to mean the pg_temp_NNN
>> schema for its own backend ID rather than the leader's backend ID.
>> I'm not sure that's a problem, but I haven't thought deeply about it.
>
> Hmmm. I'll take a look.

Thanks.

>> Could you answer my question about whether adjust_appendrel_attrs()
>> might translate Vars into non-Vars?
>
> Yes, absolutely. It may be that this code accidentally fails to fail
> because nothing is actually looking at the flag for a childrel ...
> but that's obviously not something to rely on long-term.

Yes, I think that's not likely to hold up very long at all.

<ponders>

Hmm. I wonder if the right thing to do is to clear the child's flag
if it is currently set, but leave it clear if it already is clear.
Let me go look at how that translated_vars mapping is constructed...

>> The code comment in that function
>> header doesn't seem to me to very clear about it. I'm guessing that
>> the answer is yes, so maybe the line of code you're complaining about
>> should just say:
>> childrel->reltarget_has_non_vars = true;
>> ...but that seems like it might suck.
>
> [ shrug... ] I'm still of the opinion that we should just drop
> reltarget_has_non_vars; the most charitable thing I can say about it
> is that it's premature optimization.

I'm surprised by that.

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