Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Simplify executor's handling of CaseTestExpr & CoerceToDomainValue.

  2. Evaluate arguments of correlated SubPlans in the referencing ExprState

  1. JIT: The nullness of casetest.value can be determined at the JIT compile time.

    Xing Guo <higuoxing@gmail.com> — 2024-08-31T08:04:40Z

    Hi hackers,

The nullness of casetest.value can be determined at the JIT compile
time. We can emit fewer codes by utilizing this property. The attached
patch is trying to fix it.

Best Regards,
Xing

  2. Re: JIT: The nullness of casetest.value can be determined at the JIT compile time.

    Andreas Karlsson <andreas@proxel.se> — 2024-09-03T12:09:04Z

    On 8/31/24 10:04 AM, Xing Guo wrote:
> The nullness of casetest.value can be determined at the JIT compile
> time. We can emit fewer codes by utilizing this property. The attached
> patch is trying to fix it.

I have not reviewed the code yet but the idea seems good.

But I wonder if we shouldn't instead simplify the code a bit by 
specializing these steps when generating them instead of doing the work 
runtime/while generating machine code. Yes, I doubt the performance 
benefits matter but I personally think the code is cleaner before my 
patch than after it.

Long term it would be nice to get rid off 
caseValue_datum/domainValue_datum as mentioned by Andres[1] but that is 
a bigger job so think that either your patch or my patch would make 
sense to apply before that.

1. 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=a7f107df2b700c859e4d9ad2ca66b07a465d6223

Andreas

  3. Re: JIT: The nullness of casetest.value can be determined at the JIT compile time.

    Xing Guo <higuoxing@gmail.com> — 2024-09-03T15:18:39Z

    On Tue, Sep 3, 2024 at 8:09 PM Andreas Karlsson <andreas@proxel.se> wrote:
>
> On 8/31/24 10:04 AM, Xing Guo wrote:
> > The nullness of casetest.value can be determined at the JIT compile
> > time. We can emit fewer codes by utilizing this property. The attached
> > patch is trying to fix it.
>
> I have not reviewed the code yet but the idea seems good.
>
> But I wonder if we shouldn't instead simplify the code a bit by
> specializing these steps when generating them instead of doing the work
> runtime/while generating machine code. Yes, I doubt the performance
> benefits matter but I personally think the code is cleaner before my
> patch than after it.

+1 to the idea.

> Long term it would be nice to get rid off
> caseValue_datum/domainValue_datum as mentioned by Andres[1] but that is
> a bigger job so think that either your patch or my patch would make
> sense to apply before that.

I think your patch makes more sense than mine! Thanks!

Best Regards,
Xing

  4. Re: JIT: The nullness of casetest.value can be determined at the JIT compile time.

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-11-15T09:38:08Z

    On 03/09/2024 18:18, Xing Guo wrote:
> On Tue, Sep 3, 2024 at 8:09 PM Andreas Karlsson <andreas@proxel.se> wrote:
>>
>> On 8/31/24 10:04 AM, Xing Guo wrote:
>>> The nullness of casetest.value can be determined at the JIT compile
>>> time. We can emit fewer codes by utilizing this property. The attached
>>> patch is trying to fix it.
>>
>> I have not reviewed the code yet but the idea seems good.
>>
>> But I wonder if we shouldn't instead simplify the code a bit by
>> specializing these steps when generating them instead of doing the work
>> runtime/while generating machine code. Yes, I doubt the performance
>> benefits matter but I personally think the code is cleaner before my
>> patch than after it.
> 
> +1 to the idea.
> 
>> Long term it would be nice to get rid off
>> caseValue_datum/domainValue_datum as mentioned by Andres[1] but that is
>> a bigger job so think that either your patch or my patch would make
>> sense to apply before that.
> 
> I think your patch makes more sense than mine! Thanks!

Huge +1 for cleaning up this abuse of caseValue_datum/domainValue_datum. 
While correct and sensible if we continue the abuse, these patches feel 
like putting lipstick on a pig.

I think the most straightforward way to clean that up is to replace 
caseValue and domainValue in ExprContext with a generic array of 
"expression params", and a new Expr node type to refer to them. 
Basically, the same as we have now, but with explicit names to indicate 
that the values are supplied externally.

In 
https://www.postgresql.org/message-id/20230302200549.l2ikytmnqzvy5a7a%40alap3.anarazel.de, 
Andres hinted at using ParamExecData for these. That'd be nice, but 
seems harder. Those params are reserved at planning time, so we'd need 
to somehow allocate more of them in ExecInitExpr, or move the 
bookkeeping to the planner. But just basically renaming "caseValue" to 
"externalValue" seems straightforward.

PARAM_EXECs are also a slightly more expensive to evaluate, because they 
support lazy evaluation of the subplan. This goes off-topic, but I 
wonder if there are cases where we could know at ExecInitExpr() time 
that the parameter must be already evaluated when it's referred, and 
skip the lazy checks?

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  5. Re: JIT: The nullness of casetest.value can be determined at the JIT compile time.

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-30T03:29:32Z

    Heikki Linnakangas <hlinnaka@iki.fi> writes:
> Huge +1 for cleaning up this abuse of caseValue_datum/domainValue_datum. 
> While correct and sensible if we continue the abuse, these patches feel 
> like putting lipstick on a pig.

Agreed.  I spent some time trying to do it better, with results
shown at [1].  If we adopt that idea, then the executor's support
for CaseTestExpr will go away, so there's little point in pursuing
that half of the patch given here.  However, I concluded that there's
insufficient reason to redesign CoerceToDomainValue, so we could
still push forward with that half of this patch.

			regards, tom lane

[1] https://www.postgresql.org/message-id/3068812.1738206654@sss.pgh.pa.us

  6. Re: JIT: The nullness of casetest.value can be determined at the JIT compile time.

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-30T18:27:32Z

    I wrote:
> Agreed.  I spent some time trying to do it better, with results
> shown at [1].  If we adopt that idea, then the executor's support
> for CaseTestExpr will go away, so there's little point in pursuing
> that half of the patch given here.  However, I concluded that there's
> insufficient reason to redesign CoerceToDomainValue, so we could
> still push forward with that half of this patch.

On further consideration, we might as well do this whole patch.
The patch I'm proposing over there is large and might well not
land in v18 (or ever), so let's get this low-hanging fruit.

I pushed Andreas's patch with some cosmetic tidying, mainly
that I felt it better not to try to combine duplicate-ish
code for the EEOP_CASE_TESTVAL and EEOP_DOMAIN_TESTVAL cases.
We're after understandability here after all, and it's still
a net code savings even with that duplication.

			regards, tom lane