On Mon, Mar 31, 2025 at 1:28 PM Lukas Fittl <lu...@fittl.com> wrote:
>
> On Tue, Mar 25, 2025 at 8:12 PM Sami Imseih <samims...@gmail.com> wrote:
>>>
>>> So this comes down to forking the Postgres code to do the job. What I
>>> had in mind was a slightly different flow, where we would be able to
>>> push custom node attributes between the header parsing and the
>>> generation of the extension code. If we do that, there would be no
>>> need to fork the Postgres code: extensions could force the definitions
>>> they want to the nodes they want to handle, as long as these do not
>>> need to touch the in-core query jumble logic, of course.
>>
>>
>> Allowing extensions to generate code for custom node attributes could be
>> taken up in 19. What about for 18 we think about exposing AppendJumble,
>> JUMBLE_FIELD, JUMBLE_FIELD_SINGLE and JUMBLE_STRING?
>
>
> +1, I think exposing the node jumbling logic + ability to jumble values for
> extensions would make a big difference to get into 18, specifically for
> allowing extensions to do plan ID calculations efficiently
I think getting these APIs into 18 is super important, especially with
the optimizations made in
ad9a23bc4; I will reiterate that extension developers should not have
to re-invent these
wheels :)
> I've also intentionally reduced the API surface area and not allowed
> initializing a jumble state that records constant locations / not exported
> RecordConstLocations.
> I think the utility of that seems less clear (possibly out-of-core queryid
> customization with a future extension hook in jumbleNode) and requires more
> refinement.
I agree with that primarily because I don't know of the use case at
this time in which
an extension will need to record a constant location.
I reviewed the patch and I only have one comment. I still think
we need an Assert inside RecordConstantLocation to make sure clocations
is allocated if the caller intends to record constant locations.
RecordConstLocation(JumbleState *jstate, int location, bool squashed)
{
+ Assert(jstate->clocations);
+
--
Sami Imseih
Amazon Web Services (AWS)
