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

On Thu, Jun 9, 2016 at 1:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <rhaas@postgresql.org> writes:
>> Don't generate parallel paths for rels with parallel-restricted outputs.
>
> Surely this bit is wrong on its face:
>
> @@ -971,6 +980,7 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
>             adjust_appendrel_attrs(root,
>                                    (Node *) rel->reltarget->exprs,
>                                    appinfo);
> +       childrel->reltarget_has_non_vars = rel->reltarget_has_non_vars;
>
>         /*
>          * We have to make child entries in the EquivalenceClass data
>
> The entire point of what we are doing here is that Vars might get replaced
> with things that are not Vars.  See the comment a dozen lines above.

Well, it doesn't look wrong to me (I added it, Amit's patch lacked it)
but it might be wrong all the same.  Are you saying that
adjust_appendrel_attrs() might translate Vars into non-Vars?  If so,
then yeah, this isn't right.

> More generally, if we need such a flag (which I doubt really), perhaps
> it should be part of PathTarget rather than expecting that it can
> successfully be maintained separately?

The problem with that is that it seems to be rather invasive.  We
don't really need this information for every PathTarget we build.
Currently, at least, we just need it for the default PathTargets for
join and scan relations.

> It seems like the only reason that we would need such a flag is that
> applying has_parallel_hazard() to a Var is a bit expensive thanks to
> the typeid_is_temp() test --- but if you ask me, that test is stupid
> and should be removed.  What's the argument for supposing that a temp
> table's rowtype is any more mutable intra-query than any other rowtype?

Even if has_parallel_hazard didn't involve a typeid_is_temp() test,
walking a possibly-long linked list and recursing through everything
in side of it doesn't seem like something that's so cheap as to make
it not worth avoiding.

But in response to your specific question, as the comment immediately
before that test explains, that's not the motivation at all.  If you
run the regression tests with the attached patch and
force_parallel_mode=on, you get failures like this:

*** /Users/rhaas/pgsql/src/test/regress/expected/rowtypes.out
2016-06-09 12:16:16.000000000 -0400---
/Users/rhaas/pgsql/src/test/regress/results/rowtypes.out    2016-06-09
13:21:59.000000000 -0400
***************
*** 38,48 ****
  (1 row)

  select '(Joe,)'::fullname;    -- ok, null 2nd column
!  fullname
! ----------
!  (Joe,)
! (1 row)
!
  select '(Joe)'::fullname;     -- bad
  ERROR:  malformed record literal: "(Joe)"
  LINE 1: select '(Joe)'::fullname;
--- 38,44 ----
  (1 row)

  select '(Joe,)'::fullname;    -- ok, null 2nd column
! ERROR:  cannot access temporary tables during a parallel operation
  select '(Joe)'::fullname;     -- bad
  ERROR:  malformed record literal: "(Joe)"
  LINE 1: select '(Joe)'::fullname;

That error is coming from relation_open().  It might be possible to
find a way to nerf the check in relation_open() enough to let this
case work while making the cases that we need to fail still fail, but
it wasn't obvious to me how to do that when I posted this patch and it
still isn't.  It would certainly be a good idea to improve this at
some point - perhaps by finding a way to make access to temporary
relations safe from within a parallel query - but there was no time
for that this release cycle.

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