MIME-Version: 1.0
References: 
 <CAA5RZ0ui6JSDPQDs2RW7eBDvrH_t_xg8bhUM8FaxawU8wDoMFg@mail.gmail.com>
 <8437F4D0-9DFB-4045-9318-CC3C5BA2E267@paquier.xyz>
In-Reply-To: <8437F4D0-9DFB-4045-9318-CC3C5BA2E267@paquier.xyz>
From: Lukas Fittl <lukas@fittl.com>
Date: Sun, 29 Mar 2026 22:21:46 -0700
Message-ID: 
 <CAP53Pkwb2f288Ji2s9ZZTL0_PEyYCZpQjBtD63X_s+yUB3YORA@mail.gmail.com>
Subject: Re: Refactor query normalization into core query jumbling
To: Michael Paquier <michael@paquier.xyz>
Cc: Sami Imseih <samimseih@gmail.com>, zengman <zengman@halodbtech.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>,
 Julien Rouhaud <rjuju123@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAP53Pkwb2f288Ji2s9ZZTL0_PEyYCZpQjBtD63X_s%2ByUB3YORA%40mail.gmail.com>
Precedence: bulk

On Fri, Mar 27, 2026 at 6:42=E2=80=AFPM Michael Paquier <michael@paquier.xy=
z> wrote:
>
> > On Mar 28, 2026, at 2:09, Sami Imseih <samimseih@gmail.com> wrote:
> > I agree that ComputeConstantLengths should be in core. This one is
> > not a gray area IMO. The query jumble already records constant location=
s,
> > but leaves the lengths unset. ComputeConstantLengths is just the
> > completion of that  work. There could be no other interpretation,
> > unlike generate_normalized_query, of what the lengths should be.
>
> This is an interesting remark. One problem with any patches presented yet=
 is that we don=E2=80=99t attach the lengths as part of the in-core query j=
umbling procedure: we plug the lengths later using the same structure as th=
e query jumbling. It seems to me that this is half-baked overall. I think t=
hat we don=E2=80=99t want to pay the extra length computation in the core q=
uery jumbling at the end, then why does it make sense to include the length=
s in the JumbleState structure at all? Shouldn=E2=80=99t we use a different=
 structure filled inside PGSS for this purpose rather than reuse the same t=
hing for PGSS and the in-core part (don=E2=80=99t have the code in front of=
 me now).

I see where you're coming from on that, but I don't think we can
remove anything here in practice:

typedef struct LocationLen
{
    int            location; /* Required */
    int            length; /* Set by _jumbleElements */
    bool        squashed; /* Set by RecordConstLocation called from
_jumbleElements */
    bool        extern_param; /* Set by RecordConstLocation called
from  _jumbleParam */
} LocationLen;

typedef struct JumbleState
{
    unsigned char *jumble; /* Required */
    Size        jumble_len; /* Required */
    LocationLen *clocations; /* Required to track constant locations */
    int            clocations_buf_size; /* Required to track constant
locations */
    int            clocations_count; /* Required to track constant location=
s */
    int            highest_extern_param_id; /* Set by _jumbleParam */
    bool        has_squashed_lists; /* Set by _jumbleElements */
    unsigned int pending_nulls; /* Required */
    Size        total_jumble_len; /* Required */
} JumbleState;

The only refactoring I could think of is to write out the
_jumbleElements information separately, then we could actually drop
length, and maybe squashed, from LocationLen. But I'm not sure that
really buys us much? It would be more clear I guess, because the mixed
locations of where length gets set is indeed a bit odd.

I still think it'd be reasonable for us to include
ComputeConstantLengths in core to complete the picture of what we're
doing with _jumbleElements and the length field already anyway. Its
basically a way to fully hydrate the partially filled out JumbleState
from the initial jumble.

Thanks,
Lukas

--=20
Lukas Fittl