On Thu, Jun 9, 2016 at 1:02 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Robert Haas <rh...@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
break-things-by-removing-temp-check.patch
Description: binary/octet-stream
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
