Re: Refactor query normalization into core query jumbling

On Fri, Mar 27, 2026 at 6:42 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> > On Mar 28, 2026, at 2:09, Sami Imseih <samimseih(at)gmail(dot)com> wrote:
> > I agree that ComputeConstantLengths should be in core. This one is
> > not a gray area IMO. The query jumble already records constant locations,
> > 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’t attach the lengths as part of the in-core query jumbling procedure: we plug the lengths later using the same structure as the query jumbling. It seems to me that this is half-baked overall. I think that we don’t want to pay the extra length computation in the core query jumbling at the end, then why does it make sense to include the lengths in the JumbleState structure at all? Shouldn’t we use a different structure filled inside PGSS for this purpose rather than reuse the same thing for PGSS and the in-core part (don’t 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 locations */
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

--
Lukas Fittl