public inbox for [email protected]  

help / color / mirror / Atom feed

From: Heikki Linnakangas <[email protected]>
To: Xing Guo <[email protected]>
To: Andreas Karlsson <[email protected]>
Cc: [email protected]
Subject: Re: JIT: The nullness of casetest.value can be determined at the JIT compile time.
Date: Fri, 15 Nov 2024 11:38:08 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <CACpMh+AWarmG=aKy6KUyyWVTKLedBJzZMTQG3TKPN5EGVii9TA@mail.gmail.com>
References: <CACpMh+AiBYAWn+D1aU7Rsy-V1tox06Cbc0H3qA7rwL5zdJ=anQ@mail.gmail.com>
	<[email protected]>
	<CACpMh+AWarmG=aKy6KUyyWVTKLedBJzZMTQG3TKPN5EGVii9TA@mail.gmail.com>

On 03/09/2024 18:18, Xing Guo wrote:
> On Tue, Sep 3, 2024 at 8:09 PM Andreas Karlsson <[email protected]> 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)

view thread (5+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: JIT: The nullness of casetest.value can be determined at the JIT compile time.
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox