Re: Case expression pushdown - Mailing list pgsql-hackers

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