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

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.

> 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.

> 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.

> 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.

regards, tom lane