From: | Gilles Darold <gilles(at)migops(dot)com> |
---|---|
To: | Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru> |
Cc: | Seino Yuki <seinoyu(at)oss(dot)nttdata(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Case expression pushdown |
Date: | 2021-07-07 16:50:37 |
Message-ID: | 06a1834a-d6ba-473b-a5b7-6e5042ab6646@migops.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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
From | Date | Subject | |
---|---|---|---|
Next Message | Gilles Darold | 2021-07-07 16:55:51 | Re: Case expression pushdown |
Previous Message | Tom Lane | 2021-07-07 16:05:34 | Re: PostgreSQL-13.3 parser.y with positional references by named references |