Robert Haas <robertmh...@gmail.com> writes:
> I dug into this a bit and found more problems. I wondered why Tom's
> patch did this:
> ! if (has_parallel_hazard((Node *) rte->subquery,
> false))
> ! return;
> ! break;
> Instead of just doing this:
> - return;
> + break;
> After all, the code that built subquery paths ought to be sufficient
> to find any parallel hazards during subquery planning; there seems to
> be no especially-good reason why we should walk the whole query tree
> searching for the again.
Yeah, I think you are right. I modeled my patch on the nearby handling of
function RTEs, wherein function_rte_parallel_ok checks the RTE contents
for safety. But we must do that because create_functionscan_path relies
entirely on the rel's consider_parallel flag. In contrast,
create_subqueryscan_path looks at both rel->consider_parallel and
subpath->parallel_safe, so we can rely on the subpath(s)' markings as an
indication of the subquery's safety. (rel->consider_parallel then tells
us just about the restriction and projection expressions on the subquery
rel itself, and we'll end up with the right choice if those are unsafe.)
BTW, looking elsewhere in set_rel_consider_parallel, isn't there an extra
"return;" in the tablesample stanza, allpaths.c:541 as of HEAD? Looks to
me like we're failing to ever treat tablesampling as parallel-safe.
I'm rather dubious about whether any of our tablesample methods actually
are parallel-safe, but that should be down to the method to say. If this
code's been like this all along, we've never tested them.
> I think it's hard to avoid the conclusion that
> set_rel_consider_parallel() needs to run on other member rels as well
> as baserels. We might be able to do that only for cases where the
> parent is a subquery RTE, since if the parent is a baserel then I
> think we must have just a standard inheritance hierarchy and things
> might be OK, but even then, I fear there might be problems with RLS.
> Anyway, the attached patch solves the problem in a fairly "brute
> force" fashion.
I agree, and I also agree that this way is pretty brute-force.
I think a cleaner way is to have set_append_rel_size() invoke
set_rel_consider_parallel() on the child rels and then propagate their
parallel-unsafety up to the parent. That seems fairly analogous to
the way we already deal with their sizes: in particular, set_rel_size
is invoked on an appendrel child from there, not from an extra pass in
set_base_rel_sizes. Then set_append_rel_pathlist can propagate unsafety
down again, as you've done here.
> (Official status update: I'm not prepared to commit this without some
> review. So I intend to wait for a while and see whether I get some
> review. I realize these status updates are supposed to contain a date
> by which further action will be taken, but I don't know how to
> meaningfully do that in this type of case.)
You mentioned that you'll be on vacation for much of July. If you like,
I will take this open item off your hands, since I'll be around and can
deal with any bugs that pop up in it.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
