Case expression pushdown

Hi.

This patch allows pushing case expressions to foreign servers, so that
more types of updates could be executed directly.

For example, without patch:

EXPLAIN (VERBOSE, COSTS OFF)
UPDATE ft2 d SET c2 = CASE WHEN c2 > 0 THEN c2 ELSE 0 END
WHERE c1 > 1000;
QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------
Update on public.ft2 d
Remote SQL: UPDATE "S 1"."T 1" SET c2 = $2 WHERE ctid = $1
-> Foreign Scan on public.ft2 d
Output: CASE WHEN (c2 > 0) THEN c2 ELSE 0 END, ctid, d.*
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8, ctid FROM
"S 1"."T 1" WHERE (("C 1" > 1000)) FOR UPDATE

EXPLAIN (VERBOSE, COSTS OFF)
UPDATE ft2 d SET c2 = CASE WHEN c2 > 0 THEN c2 ELSE 0 END
WHERE c1 > 1000;
QUERY PLAN
----------------------------------------------------------------------------------------------------------------
Update on public.ft2 d
-> Foreign Update on public.ft2 d
Remote SQL: UPDATE "S 1"."T 1" SET c2 = (CASE WHEN (c2 > 0)
THEN c2 ELSE 0 END) WHERE (("C 1" > 1000))

--
Best regards,
Alexander Pyhalov,
Postgres Professional

Looks quite useful to me. Can you please add this to the next commitfest?

On Wed, Jun 9, 2021 at 5:25 PM Alexander Pyhalov
<a(dot)pyhalov(at)postgrespro(dot)ru> wrote:
>
> Hi.
>
> This patch allows pushing case expressions to foreign servers, so that
> more types of updates could be executed directly.
>
> For example, without patch:
>
> EXPLAIN (VERBOSE, COSTS OFF)
> UPDATE ft2 d SET c2 = CASE WHEN c2 > 0 THEN c2 ELSE 0 END
> WHERE c1 > 1000;
> QUERY PLAN
> -----------------------------------------------------------------------------------------------------------------------
> Update on public.ft2 d
> Remote SQL: UPDATE "S 1"."T 1" SET c2 = $2 WHERE ctid = $1
> -> Foreign Scan on public.ft2 d
> Output: CASE WHEN (c2 > 0) THEN c2 ELSE 0 END, ctid, d.*
> Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8, ctid FROM
> "S 1"."T 1" WHERE (("C 1" > 1000)) FOR UPDATE
>
>
> EXPLAIN (VERBOSE, COSTS OFF)
> UPDATE ft2 d SET c2 = CASE WHEN c2 > 0 THEN c2 ELSE 0 END
> WHERE c1 > 1000;
> QUERY PLAN
> ----------------------------------------------------------------------------------------------------------------
> Update on public.ft2 d
> -> Foreign Update on public.ft2 d
> Remote SQL: UPDATE "S 1"."T 1" SET c2 = (CASE WHEN (c2 > 0)
> THEN c2 ELSE 0 END) WHERE (("C 1" > 1000))
>
>
> --
> Best regards,
> Alexander Pyhalov,
> Postgres Professional

--
Best Wishes,
Ashutosh Bapat

Hi.

Ashutosh Bapat писал 2021-06-15 16:24:
> Looks quite useful to me. Can you please add this to the next
> commitfest?
>

Addded to commitfest. Here is an updated patch version.

--
Best regards,
Alexander Pyhalov,
Postgres Professional

On 2021-06-16 01:29, Alexander Pyhalov wrote:
> Hi.
>
> Ashutosh Bapat писал 2021-06-15 16:24:
>> Looks quite useful to me. Can you please add this to the next
>> commitfest?
>>
>
> Addded to commitfest. Here is an updated patch version.

Thanks for posting the patch.
I agree with this content.

> + Foreign Scan on public.ft2 (cost=156.58..165.45 rows=394 width=14)
It's not a big issue, but is there any intention behind the pattern of
outputting costs in regression tests?

Regards,

--
Yuki Seino
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Seino Yuki писал 2021-06-22 16:03:
> On 2021-06-16 01:29, Alexander Pyhalov wrote:
>> Hi.
>>
>> Ashutosh Bapat писал 2021-06-15 16:24:
>>> Looks quite useful to me. Can you please add this to the next
>>> commitfest?
>>>
>>
>> Addded to commitfest. Here is an updated patch version.
>
> Thanks for posting the patch.
> I agree with this content.
>
>> + Foreign Scan on public.ft2 (cost=156.58..165.45 rows=394 width=14)
> It's not a big issue, but is there any intention behind the pattern of
> outputting costs in regression tests?

Hi.

No, I don't think it makes much sense. Updated tests (also added case
with empty else).
--
Best regards,
Alexander Pyhalov,
Postgres Professional

Le 22/06/2021 à 15:39, Alexander Pyhalov a écrit :
> Seino Yuki писал 2021-06-22 16:03:
>> On 2021-06-16 01:29, Alexander Pyhalov wrote:
>>> Hi.
>>>
>>> Ashutosh Bapat писал 2021-06-15 16:24:
>>>> Looks quite useful to me. Can you please add this to the next
>>>> commitfest?
>>>>
>>>
>>> Addded to commitfest. Here is an updated patch version.
>>
>> Thanks for posting the patch.
>> I agree with this content.
>>
>>> + Foreign Scan on public.ft2 (cost=156.58..165.45 rows=394 width=14)
>> It's not a big issue, but is there any intention behind the pattern of
>> outputting costs in regression tests?
>
> Hi.
>
> No, I don't think it makes much sense. Updated tests (also added case
> with empty else).

The patch doesn't apply anymore to master, I join an update of your
patch update in attachment. This is your patch rebased and untouched
minus a comment in the test and renamed to v4.

I could have miss something but I don't think that additional struct
elements case_args in structs foreign_loc_cxt and deparse_expr_cxt are
necessary. They look to be useless.

The patch will also be more clear if the CaseWhen node was handled
separately in foreign_expr_walker() instead of being handled in the
T_CaseExpr case. By this way the T_CaseExpr case just need to call
recursively foreign_expr_walker(). I also think that code in
T_CaseTestExpr should just check the collation, there is nothing more to
do here like you have commented the function deparseCaseTestExpr(). This
function can be removed as it does nothing if the case_args elements are
removed.

There is a problem the regression test with nested CASE clauses:

EXPLAIN (VERBOSE, COSTS OFF)
SELECT c1,c2,c3 FROM ft2 WHERE CASE CASE WHEN c2 > 0 THEN c2 END
WHEN 100 THEN 601 WHEN c2 THEN c2 ELSE 0 END > 600 ORDER BY c1;

the original query use "WHERE CASE CASE WHEN" but the remote query is
not the same in the plan:

Remote SQL: SELECT "C 1", c2, c3 FROM "S 1"."T 1" WHERE (((CASE WHEN
((CASE WHEN (c2 > 0) THEN c2 ELSE NULL::integer END) = 100) THEN 601
WHEN ((CASE WHEN (c2 > 0) THEN c2 ELSE NULL::integer END) = c2) THEN
c2 ELSE 0 END) > 600)) ORDER BY "C 1" ASC NULLS LAST

Here this is "WHERE (((CASE WHEN ((CASE WHEN" I expected it to be
unchanged to "WHERE (((CASE (CASE WHEN".

Also I would like the following regression tests to be added. It test
that the CASE clause in aggregate and function is pushed down as well as
the aggregate function. This was the original use case that I wanted to
fix with this feature.

-- CASE in aggregate function, both must be pushed down
EXPLAIN (VERBOSE, COSTS OFF)
SELECT sum(CASE WHEN mod(c1, 4) = 0 THEN 1 ELSE 2 END) FROM ft1;
-- Same but without the ELSE clause
EXPLAIN (VERBOSE, COSTS OFF)
SELECT sum(CASE WHEN mod(c1, 4) = 0 THEN 1 END) FROM ft1;

For convenience I'm attaching a new patch v5 that change the code
following my comments above, fix the nested CASE issue and adds more
regression tests.

Best regards,

--
Gilles Darold

Hi.

Gilles Darold писал 2021-07-07 15:02:

> Le 22/06/2021 à 15:39, Alexander Pyhalov a écrit :
>
>> Seino Yuki писал 2021-06-22 16:03:
>> On 2021-06-16 01:29, Alexander Pyhalov wrote:
>> Hi.
>>
>> Ashutosh Bapat писал 2021-06-15 16:24:
>> Looks quite useful to me. Can you please add this to the next
>> commitfest?
>>
>> Addded to commitfest. Here is an updated patch version.
>
> Thanks for posting the patch.
> I agree with this content.
>
>> + Foreign Scan on public.ft2 (cost=156.58..165.45 rows=394
>> width=14)
> It's not a big issue, but is there any intention behind the pattern
> of
> outputting costs in regression tests?
>
> Hi.
>
> No, I don't think it makes much sense. Updated tests (also added case
> with empty else).
>
> The patch doesn't apply anymore to master, I join an update of your
> patch update in attachment. This is your patch rebased and untouched
> minus a comment in the test and renamed to v4.
>
> I could have miss something but I don't think that additional struct
> elements case_args in structs foreign_loc_cxt and deparse_expr_cxt are
> necessary. They look to be useless.

I thought we should compare arg collation and expression collation and
didn't suggest, that we can take CaseTestExpr's collation directly,
without deriving it from CaseExpr's arg. Your version of course looks
saner.

>
> The patch will also be more clear if the CaseWhen node was handled
> separately in foreign_expr_walker() instead of being handled in the
> T_CaseExpr case. By this way the T_CaseExpr case just need to call
> recursively foreign_expr_walker(). I also think that code in
> T_CaseTestExpr should just check the collation, there is nothing more
> to do here like you have commented the function deparseCaseTestExpr().
> This function can be removed as it does nothing if the case_args
> elements are removed.
>
> There is a problem the regression test with nested CASE clauses:
>
>> EXPLAIN (VERBOSE, COSTS OFF)
>> SELECT c1,c2,c3 FROM ft2 WHERE CASE CASE WHEN c2 > 0 THEN c2 END
>> WHEN 100 THEN 601 WHEN c2 THEN c2 ELSE 0 END > 600 ORDER BY c1;
>
> the original query use "WHERE CASE CASE WHEN" but the remote query is
> not the same in the plan:
>
>> Remote SQL: SELECT "C 1", c2, c3 FROM "S 1"."T 1" WHERE (((CASE WHEN
>> ((CASE WHEN (c2 > 0) THEN c2 ELSE NULL::integer END) = 100) THEN 601
>> WHEN ((CASE WHEN (c2 > 0) THEN c2 ELSE NULL::integer END) = c2) THEN
>> c2 ELSE 0 END) > 600)) ORDER BY "C 1" ASC NULLS LAST
>
> Here this is "WHERE (((CASE WHEN ((CASE WHEN" I expected it to be
> unchanged to "WHERE (((CASE (CASE WHEN".

I'm not sure this is an issue (as we change CASE A WHEN B ... to CASE
WHEN (A=B)),
and expressions should be free from side effects, but again your version
looks better.

Thanks for improving the patch, it looks saner now.
--
Best regards,
Alexander Pyhalov,
Postgres Professional

Le 07/07/2021 à 17:39, Alexander Pyhalov a écrit :
> Hi.
>
> Gilles Darold писал 2021-07-07 15:02:
>
>> Le 22/06/2021 à 15:39, Alexander Pyhalov a écrit :
>>
>>> Seino Yuki писал 2021-06-22 16:03:
>>> On 2021-06-16 01:29, Alexander Pyhalov wrote:
>>> Hi.
>>>
>>> Ashutosh Bapat писал 2021-06-15 16:24:
>>> Looks quite useful to me. Can you please add this to the next
>>> commitfest?
>>>
>>> Addded to commitfest. Here is an updated patch version.
>>
>> Thanks for posting the patch.
>> I agree with this content.
>>
>>> + Foreign Scan on public.ft2 (cost=156.58..165.45 rows=394
>>> width=14)
>>  It's not a big issue, but is there any intention behind the pattern
>> of
>> outputting costs in regression tests?
>>
>> Hi.
>>
>> No, I don't think it makes much sense. Updated tests (also added case
>> with empty else).
>>
>> The patch doesn't apply anymore to master, I join an update of your
>> patch update in attachment. This is your patch rebased and untouched
>> minus a comment in the test and renamed to v4.
>>
>> I could have miss something but I don't think that additional struct
>> elements case_args in structs foreign_loc_cxt and deparse_expr_cxt are
>> necessary. They look to be useless.
>
> I thought we should compare arg collation and expression collation and
> didn't suggest, that we can take CaseTestExpr's collation directly,
> without deriving it from CaseExpr's arg. Your version of course looks
> saner.
>
>>
>> The patch will also be more clear if the CaseWhen node was handled
>> separately in foreign_expr_walker() instead of being handled in the
>> T_CaseExpr case. By this way the T_CaseExpr case just need to call
>> recursively foreign_expr_walker(). I also think that code in
>> T_CaseTestExpr should just check the collation, there is nothing more
>> to do here like you have commented the function deparseCaseTestExpr().
>> This function can be removed as it does nothing if the case_args
>> elements are removed.
>>
>> There is a problem the regression test with nested CASE clauses:
>>
>>> EXPLAIN (VERBOSE, COSTS OFF)
>>> SELECT c1,c2,c3 FROM ft2 WHERE CASE CASE WHEN c2 > 0 THEN c2 END
>>> WHEN 100 THEN 601 WHEN c2 THEN c2 ELSE 0 END > 600 ORDER BY c1;
>>
>> the original query use "WHERE CASE CASE WHEN" but the remote query is
>> not the same in the plan:
>>
>>> Remote SQL: SELECT "C 1", c2, c3 FROM "S 1"."T 1" WHERE (((CASE WHEN
>>> ((CASE WHEN (c2 > 0) THEN c2 ELSE NULL::integer END) = 100) THEN 601
>>> WHEN ((CASE WHEN (c2 > 0) THEN c2 ELSE NULL::integer END) = c2) THEN
>>> c2 ELSE 0 END) > 600)) ORDER BY "C 1" ASC NULLS LAST
>>
>> Here this is "WHERE (((CASE WHEN ((CASE WHEN" I expected it to be
>> unchanged to "WHERE (((CASE (CASE WHEN".
>
> I'm not sure this is an issue (as we change CASE A WHEN B ... to CASE
> WHEN (A=B)),
> and expressions should be free from side effects, but again your version
> looks better.

Right it returns the same result but I think this is confusing to not
see the same case form in the remote query.

>
> Thanks for improving the patch, it looks saner now.

Great, I changing the state in the commitfest to "Ready for committers".

--
Gilles Darold
MigOps Inc

Le 07/07/2021 à 18:50, Gilles Darold a écrit :
>
> Great, I changing the state in the commitfest to "Ready for committers".
>
>
I'm attaching the v5 patch again as it doesn't appears in the Latest
attachment list in the commitfest.

--
Gilles Darold
MigOps Inc

Le 07/07/2021 à 18:55, Gilles Darold a écrit :
> Le 07/07/2021 à 18:50, Gilles Darold a écrit :
>>
>> Great, I changing the state in the commitfest to "Ready for committers".
>>
>>
> I'm attaching the v5 patch again as it doesn't appears in the Latest
> attachment list in the commitfest.
>
>
And the review summary:

This patch allows pushing CASE expressions to foreign servers, so that:

  - more types of updates could be executed directly
  - full foreign table scan can be avoid
  - more push down of aggregates function

The patch compile and regressions tests with assert enabled passed
successfully.
There is a compiler warning but it is not related to this patch:

        deparse.c: In function ‘foreign_expr_walker.isra.0’:
        deparse.c:891:28: warning: ‘collation’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
          891 |       outer_cxt->collation = collation;
              |       ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
        deparse.c:874:10: warning: ‘state’ may be used uninitialized in
this function [-Wmaybe-uninitialized]
          874 |  else if (state == outer_cxt->state)
              |          ^

The regression test for this feature contains the use cases where push
down of CASE clause are useful.
Nested CASE are also part of the regression tests.

The patch adds insignificant overhead by processing further than before
a case expression but overall it adds a major performance improvement
for queries on foreign tables that use a CASE WHEN clause in a predicate
or in an aggregate function.

This patch does what it claims to do without detect problem, as expected
the CASE clause is not pushed when a local table is involved in the CASE
expression of if a non default collation is used.

Ready for committers.

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.

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.

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.

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

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

Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru> writes:
> [ 0001-Allow-pushing-CASE-expression-to-foreign-server-v6.patch ]

This doesn't compile cleanly:

deparse.c: In function 'foreign_expr_walker.isra.4':
deparse.c:920:8: warning: 'collation' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (collation != outer_cxt->collation)
^
deparse.c:914:3: warning: 'state' may be used uninitialized in this function [-Wmaybe-uninitialized]
switch (state)
^~~~~~

These uninitialized variables very likely explain the fact that it fails
regression tests, both for me and for the cfbot. Even if this weren't an
outright bug, we don't tolerate code that produces warnings on common
compilers.

regards, tom lane

Tom Lane писал 2021-07-26 18:18:
> Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru> writes:
>> [ 0001-Allow-pushing-CASE-expression-to-foreign-server-v6.patch ]
>
> This doesn't compile cleanly:
>
> deparse.c: In function 'foreign_expr_walker.isra.4':
> deparse.c:920:8: warning: 'collation' may be used uninitialized in
> this function [-Wmaybe-uninitialized]
> if (collation != outer_cxt->collation)
> ^
> deparse.c:914:3: warning: 'state' may be used uninitialized in this
> function [-Wmaybe-uninitialized]
> switch (state)
> ^~~~~~
>
> These uninitialized variables very likely explain the fact that it
> fails
> regression tests, both for me and for the cfbot. Even if this weren't
> an
> outright bug, we don't tolerate code that produces warnings on common
> compilers.
>
> regards, tom lane

Hi.

Of course, this is a patch issue. Don't understand how I overlooked
this.
Rebased on master and fixed it. Tests are passing here (but they also
passed for previous patch version).

What exact tests are failing?

--
Best regards,
Alexander Pyhalov,
Postgres Professional

Le 26/07/2021 à 18:03, Alexander Pyhalov a écrit :
> Tom Lane писал 2021-07-26 18:18:
>> Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru> writes:
>>> [ 0001-Allow-pushing-CASE-expression-to-foreign-server-v6.patch ]
>>
>> This doesn't compile cleanly:
>>
>> deparse.c: In function 'foreign_expr_walker.isra.4':
>> deparse.c:920:8: warning: 'collation' may be used uninitialized in
>> this function [-Wmaybe-uninitialized]
>>      if (collation != outer_cxt->collation)
>>         ^
>> deparse.c:914:3: warning: 'state' may be used uninitialized in this
>> function [-Wmaybe-uninitialized]
>>    switch (state)
>>    ^~~~~~
>>
>> These uninitialized variables very likely explain the fact that it fails
>> regression tests, both for me and for the cfbot.  Even if this
>> weren't an
>> outright bug, we don't tolerate code that produces warnings on common
>> compilers.
>>
>>             regards, tom lane
>
> Hi.
>
> Of course, this is a patch issue. Don't understand how I overlooked this.
> Rebased on master and fixed it. Tests are passing here (but they also
> passed for previous patch version).
>
> What exact tests are failing?
>

I confirm that there is no compilation warning and all regression tests
pass successfully for the v7 patch, I have not checked previous patch
but this one doesn't fail on cfbot too.

Best regards,

--
Gilles Darold

Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru> writes:
> [ 0001-Allow-pushing-CASE-expression-to-foreign-server-v7.patch ]

I looked this over. It's better than before, but the collation
handling is still not at all correct. We have to consider that a
CASE's arg expression supplies the collation for a contained
CaseTestExpr, otherwise we'll come to the wrong conclusions about
whether "CASE foreignvar WHEN ..." is shippable, if the foreignvar
is what's determining collation of the comparisons.

This means that the CaseExpr level of recursion has to pass data down
to the CaseTestExpr level. In the attached, I did that by adding an
additional argument to foreign_expr_walker(). That's a bit invasive,
but it's not awful. I thought about instead adding fields to the
foreign_loc_cxt struct. But that seemed considerably messier in the
end, because we'd then have some fields that are information sourced
at one recursion level and some that are info sourced at another
level.

I also whacked the regression test cases around a lot. They seemed
to spend a lot of time on irrelevant combinations, while failing to
check the things that matter, namely whether collation-based pushdown
decisions are made correctly.

regards, tom lane

Tom Lane писал 2021-07-29 23:54:
> Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru> writes:
>> [ 0001-Allow-pushing-CASE-expression-to-foreign-server-v7.patch ]
>
> I looked this over. It's better than before, but the collation
> handling is still not at all correct. We have to consider that a
> CASE's arg expression supplies the collation for a contained
> CaseTestExpr, otherwise we'll come to the wrong conclusions about
> whether "CASE foreignvar WHEN ..." is shippable, if the foreignvar
> is what's determining collation of the comparisons.
>
> This means that the CaseExpr level of recursion has to pass data down
> to the CaseTestExpr level. In the attached, I did that by adding an
> additional argument to foreign_expr_walker(). That's a bit invasive,
> but it's not awful. I thought about instead adding fields to the
> foreign_loc_cxt struct. But that seemed considerably messier in the
> end, because we'd then have some fields that are information sourced
> at one recursion level and some that are info sourced at another
> level.
>
> I also whacked the regression test cases around a lot. They seemed
> to spend a lot of time on irrelevant combinations, while failing to
> check the things that matter, namely whether collation-based pushdown
> decisions are made correctly.
>
> regards, tom lane

Hi.

Overall looks good.
The only thing I'm confused about is in T_CaseTestExpr case - how can it
be that CaseTestExpr collation doesn't match case_arg_cxt->collation ?
Do we we need to inspect only case_arg_cxt->state? Can we assert that
collation == case_arg_cxt->collation?

--
Best regards,
Alexander Pyhalov,
Postgres Professional

Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru> writes:
> The only thing I'm confused about is in T_CaseTestExpr case - how can it
> be that CaseTestExpr collation doesn't match case_arg_cxt->collation ?
> Do we we need to inspect only case_arg_cxt->state? Can we assert that
> collation == case_arg_cxt->collation?

Perhaps, but:

(1) I'm disinclined to make this code look different from the otherwise-
identical coding elsewhere in foreign_expr_walker.

(2) That would create a hard assumption that foreign_expr_walker's
conclusions about the collation of a subexpression match those of
assign_query_collations. I'm not quite sure I believe that (and if
it's true, why aren't we just relying on exprCollation?). Anyway,
if we're to have an assertion that it's true, it should be in some
place that's a lot less out-of-the-way than CaseTestExpr, because
if the assumption gets violated it might be a long time till we
notice.

So I think we're best off to just write it the way I did, at least
so far as this patch is concerned. If we want to rethink the way
collation gets calculated here, that would be material for a
separate patch.

regards, tom lane

I wrote:
> Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru> writes:
>> Do we we need to inspect only case_arg_cxt->state? Can we assert that
>> collation == case_arg_cxt->collation?

> Perhaps, but:
> ...

Oh, actually there's a third point: the shakiest part of this logic
is the assumption that we've correctly matched a CaseTestExpr to
its source CaseExpr. Seeing that inlining and constant-folding can
mash things to the point where a WHEN expression doesn't look like
"CaseTestExpr = RHS", it's a little nervous-making to assume there
couldn't be another CASE in between. While there's not known problems
of this sort, if it did happen I'd prefer this code to react as
"don't push down", not as "assertion failure".

(There's been speculation in the past about whether we could find
a more bulletproof representation of this kind of CaseExpr. We've
not succeeded at that yet though.)

regards, tom lane