|From:||Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>|
|To:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>|
|Cc:||Gilles Darold <gilles(at)migops(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org|
|Subject:||Re: Case expression pushdown|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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:
> 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.
> 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
Thanks for review.
|Next Message||Davide Fasolo||2021-07-22 09:23:34||Re: BUG #15293: Stored Procedure Triggered by Logical Replication is Unable to use Notification Events|
|Previous Message||Artur Zakirov||2021-07-22 09:04:07||Re: BUG #15293: Stored Procedure Triggered by Logical Replication is Unable to use Notification Events|