Re: GetBufferDescriptor() being called for local buffers from MarkBufferDirtyHint()

On Fri, Jul 3, 2026 at 4:51 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Thu, Jul 02, 2026 at 10:29:47AM +0530, Ashutosh Bapat wrote:
> > We usually do not backport changes to function definitions or new
> > assertions, unless they are critical to the fix. That is not the case
> > here. The functions have been in this state for a couple of releases
> > and we haven't heard any complaints. Overall HEAD-only is fine with
> > me.
> >
> > If required we can always backport, the functions are static so they
> > won't cause an ABI break. The Assertions are applicable in those
> > branches as well.
>
> I've never mentioned a backpatch, FWIW.
>

Since you mentioned "HEAD-only" in your question "Any thoughts or
objections regarding a HEAD-only change?", it felt like you are fine
making this change but are debating whether to accept it only on HEAD
or backbranches as well. Sorry for the misunderstanding.

> Anyway, while putting my eyes into all that in details, I saw one
> problem and one potential gap:
> - The problem. The change for local buffers is actually incorrect,
> where this patch decides to set NLocBuffer earlier. If the hash
> allocation fails on ERROR, we would track an incorrect state in
> memory, most likely crashing later if attempting a local buffer
> lookup.

Every code fragment, except the ones listed below, that uses
NLocBuffer is gated, eventually, by
if (LocalBufHash == NULL)
InitLocalBuffers();

So we will not access NLocBuffer unless LocalBufHash is initialized.
If we are accessing it in a way that leads to a crash, we may have a
subtle bug lurking somewhere.

An actual subtle problem does exist in check_temp_buffers. If we set
NLocBuffer to a non-zero value when LocalBufHash is not initialized,
it's going to prohibit a change in the GUC. If LocalBufHash allocation
fails, probably user would like to try again by reducing NLocBuffer.
The prohibition makes that impossible.

The other such usage in hashbuild() is fine, since it will allocate
more space if at all.

CheckForLocalBufferLeaks() walks the local buffer descriptor array,
but it's already initialized when NLocBuffer is set.

Given that it's better not to move the assignment above just for the
sake of an assertion. However, I am worried if there is a crash which
can result because of that movement.

--
Best Wishes,
Ashutosh Bapat