Re: Case expression pushdown

Tom Lane писал 2021-07-21 19:49:
> Gilles Darold <gilles(at)migops(dot)com> writes:
>> I'm attaching the v5 patch again as it doesn't appears in the Latest
>> attachment list in the commitfest.
>
> So this has a few issues:

Hi.

>
> 1. In foreign_expr_walker, you're failing to recurse to either the
> "arg" or "defresult" subtrees of a CaseExpr, so that it would fail
> to notice unshippable constructs within those.

Fixed this.

>
> 2. You're also failing to guard against the hazard that a WHEN
> expression within a CASE-with-arg has been expanded into something
> that doesn't look like "CaseTestExpr = something". As written,
> this patch would likely dump core in that situation, and if it didn't
> it would send nonsense to the remote server. Take a look at the
> check for that situation in ruleutils.c (starting at line 8764
> as of HEAD) and adapt it to this. Probably what you want is to
> just deem the CASE un-pushable if it's been modified away from that
> structure. This is enough of a corner case that optimizing it
> isn't worth a great deal of trouble ... but crashing is not ok.
>

I think I fixed this by copying check from ruleutils.c.

> 3. A potentially uncomfortable issue for the CASE-with-arg syntax
> is that the specific equality operator being used appears nowhere
> in the decompiled expression, thus raising the question of whether
> the remote server will interpret it the same way we did. Given
> that we restrict the values-to-be-compared to be of shippable
> types, maybe this is safe in practice, but I have a bad feeling
> about it. I wonder if we'd be better off just refusing to ship
> CASE-with-arg at all, which would a-fortiori avoid point 2.

I'm not shure how 'case a when b ...' is different from 'case when a=b
...'
in this case. If type of a or b is not shippable, we will not push down
this expression in any way. And if they are of builtin types, why do
these expressions differ?

>
> 4. I'm not sure that I believe any part of the collation handling.
> There is the question of what collations will be used for the
> individual WHEN comparisons, which can probably be left for
> the recursive checks of the CaseWhen.expr subtrees to handle;
> and then there is the separate issue of whether the CASE's result
> collation (which arises from the CaseWhen.result exprs plus the
> CaseExpr.defresult expr) can be deemed to be safely derived from
> remote Vars. I haven't totally thought through how that should
> work, but I'm pretty certain that handling the CaseWhen's within
> separate recursive invocations of foreign_expr_walker cannot
> possibly get it right. However, you'll likely have to flatten
> those anyway (i.e., handle them within the loop in the CaseExpr
> case) while fixing point 2.

I've tried to account for a fact that we are interested only in
caseWhen->result collations, but still not sure that I'm right here.

>
> 5. This is a cosmetic point, but: the locations of the various
> additions in deparse.c seem to have been chosen with the aid
> of a dartboard. We do have a convention for this sort of thing,
> which is to lay out code concerned with different node types
> in the same order that the node types are declared in *nodes.h.
> I'm not sufficiently anal to want to fix the existing violations
> of that rule that I see in deparse.c; but the fact that somebody
> got this wrong before isn't license to make things worse.
>
> regards, tom lane

Fixed this.

Thanks for review.

--
Best regards,
Alexander Pyhalov,
Postgres Professional