RE: Query ID Calculation Fix for DISTINCT / ORDER BY and LIMIT / OFFSET

From: Bykov Ivan <i(dot)bykov(at)modernsys(dot)ru>
To: Michael Paquier <michael(at)paquier(dot)xyz>, David Rowley <dgrowleyml(at)gmail(dot)com>
Cc: Sami Imseih <samimseih(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: RE: Query ID Calculation Fix for DISTINCT / ORDER BY and LIMIT / OFFSET
Date: 2025-03-26 09:31:16
Message-ID: 99fe527000ca4d3ba630e08c6a9ef381@localhost.localdomain
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hello, David!

> I see you opted to only jumble the low-order byte of the pending null
> counter. I used the full 4 bytes as I don't think the extra 3 bytes is
> a problem. Witrh v7 the memcpy to copy the pending_nulls into the
> buffer is inlined into a single instruction. It's semi-conceivable
> that you could have more than 255 NULLs given that a List can contain
> Nodes. I don't know if we ever have any NULL items in Lists at the
> moment, but I don't think that that's disallowed. I'd feel much safer
> taking the full 4 bytes of the counter to help prevent future
> surprises.

Yes _jumbleList could theoretically handle cases with over 256 Node elements in same list,
but most (or all) of them is non-NULL (as I know only PlannedStmt->subplans accepts NULL elements),
Most of the foreach cycles over query lists don't have any NULL safety checks;
we just assume that the lists contain no NULL pointers.
I still believe storing just one byte is sufficient.

> I'm happy to proceed with the v7 version. I'm also happy to credit you
> As the primary author of it, given that you came up with the v2b
> patch. It might be best to extract the performance stuff that's in v7
> and apply that separately from the bug fix. If you're still interested
> and have time in the next 7 hours to do it, would you be able to split
> the v7 as described, making v8-0001 as the bug fix and v8-0002 as the
> performance stuff? If so, could you also add the total_jumble_len to
> v8-0001 like Michael's last patch had, but adjust so it's only
> maintained for Assert builds. Michael is quite keen on having more
> assurance that custom jumble functions don't decide to forego any
> jumbling.

As I can see, you have already split the patches. I apologize for the delay; I don't have
much time to work on this issue. Thank you for your proposal to credit me as the patch author.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Kirill Reshke 2025-03-26 09:36:39 Re: support create index on virtual generated column.
Previous Message Peter Eisentraut 2025-03-26 09:18:53 Re: [PATCH] Add a new pattern for zero-based months for Date/Time Formatting