Re: Case expression pushdown - Mailing list pgsql-hackers

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