Re: d25ea01275 and partitionwise join

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.

* Wasn't excited about inventing makeCoalesceExpr(); the fact that it only
had two potential call sites seemed to make it not worth the trouble.
Plus, as defined, it could not handle the general case of COALESCE, which
can have N arguments; so that seemed like a recipe for confusion.

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

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

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

* 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?

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

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.

regards, tom lane