Re: fixing subplan/subquery confusion

On Sun, Jul 3, 2016 at 1:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 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.

You are correct.

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

I was reluctant to do it that way because of the relatively stupid way
that we have to go about finding the inheritance children to visit,
but maybe I shouldn't let that dominate my thinking. We could always
replace that with some more efficient data structure at some point
down the road.

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

That would be much appreciated.

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