Re: d25ea01275 and partitionwise join

On Mon, Apr 6, 2020 at 7:29 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> > Okay, I tried that in the updated patch. I didn't try to come up with
> > examples that might break it though.
>
> I looked through this.

Thank you.

> * I think your logic for building the coalesce combinations was just
> wrong. We need combinations of left-hand inputs with right-hand inputs,
> not left-hand with left-hand or right-hand with right-hand. Also the
> nesting should already have happened in the inputs, we don't need to
> try to generate it here. The looping code was pretty messy as well.

It didn't occur to me that that many Coalesce combinations would be
necessary given the component rel combinations possible.

> * I don't think using partopcintype is necessarily right; that could be
> a polymorphic type, for instance. Safer to copy the type of the input
> expressions. Likely we could have got away with using partcollation,
> but for consistency I copied that as well.

Ah, seeing set_baserel_partition_key_exprs(), I suppose they will come
from parttypid and parttypcoll of the base partitioned tables, which
seems fine.

> * You really need to update the data structure definitional comments
> when you make a change like this.

Sorry, I should have.

> * I did not like putting a test case that requires
> enable_partitionwise_aggregate in the partition_join test; that seems
> misplaced. But it's not necessary to the test, is it?

Earlier in the discussion (which turned into a separate discussion),
there were test cases where partition-level grouping would fail with
errors in setrefs.c, but I think that was fixed last year by
529ebb20aaa5. Agree that it has nothing to do with the problem being
solved here.

> * I did not follow the point of your second test case. The WHERE
> constraint on p1.a allows the planner to strength-reduce the joins,
> which is why there's no full join in that explain result, but then
> we aren't going to get to this code at all.

Oops, I thought I copy-pasted 4-way full join test not this one, but
evidently didn't.

> I repaired the above in the attached.
>
> I'm actually sort of pleasantly surprised that this worked; I was
> not sure that building COALESCEs like this would provide the result
> we needed. But it seems okay -- it does fix the behavior in this
> 3-way test case, as well as the 4-way join you showed at the top
> of the thread. It's fairly dependent on the fact that the planner
> won't rearrange full joins; otherwise, the COALESCE structures we
> build here might not match those made at parse time. But that's
> not likely to change anytime soon; and this is hardly the only
> place that would break, so I'm not sweating about it. (I have
> some vague ideas about getting rid of the COALESCEs as part of
> the Var redefinition I've been muttering about, anyway; there
> might be a cleaner fix for this if that happens.)
>
> Anyway, I think this is probably OK for now. Given that the
> nullable_partexprs lists are only used in one place, it's pretty
> hard to see how it would break anything.

Makes sense.

--
Thank you,

Amit Langote
EnterpriseDB: http://www.enterprisedb.com