Re: refactoring relation extension and BufferAlloc(), faster COPY

On 11/02/2023 23:36, Andres Freund wrote:
> On 2023-02-11 23:03:56 +0200, Heikki Linnakangas wrote:
>> * I don't understand this comment:
>>
>>> /*
>>> * Clear out the buffer's tag and flags and usagecount. We must do
>>> * this to ensure that linear scans of the buffer array don't think
>>> * the buffer is valid.
>>> *
>>> * XXX: This is a pre-existing comment I just moved, but isn't it
>>> * entirely bogus with regard to the tag? We can't do anything with
>>> * the buffer without taking BM_VALID / BM_TAG_VALID into
>>> * account. Likely doesn't matter because we're already dirtying the
>>> * cacheline, but still.
>>> *
>>> */
>>> ClearBufferTag(&buf_hdr->tag);
>>> buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK);
>>> UnlockBufHdr(buf_hdr, buf_state);
>>
>> What exactly is wrong with clearing the tag? What does dirtying the
>> cacheline have to do with the correctness here?
>
> There's nothing wrong with clearing out the tag, but I don't think it's a hard
> requirement today, and certainly not for the reason stated above.
>
> Validity of the buffer isn't determined by the tag, it's determined by
> BM_VALID (or, if you interpret valid more widely, BM_TAG_VALID).
>
> Without either having pinned the buffer, or holding the buffer header
> spinlock, the tag can change at any time. And code like DropDatabaseBuffers()
> knows that, and re-checks the the tag after locking the buffer header
> spinlock.
>
> Afaict, there'd be no correctness issue with removing the
> ClearBufferTag(). There would be an efficiency issue though, because when
> encountering an invalid buffer, we'd unnecessarily enter InvalidateBuffer(),
> which'd find that BM_[TAG_]VALID isn't set, and not to anything.

Okay, gotcha.

> Even though it's not a correctness issue, it seems to me that
> DropRelationsAllBuffers() etc ought to check if the buffer is BM_TAG_VALID,
> before doing anything further. Particularly in DropRelationsAllBuffers(), the
> check we do for each buffer isn't cheap. Doing it for buffers that don't even
> have a tag seems .. not smart.

Depends on what percentage of buffers are valid, I guess. If all buffers
are valid, checking BM_TAG_VALID first would lose. In practice, I doubt
it makes any measurable difference either way.

Since we're micro-optimizing, I noticed that
BufTagMatchesRelFileLocator() compares the fields in order "spcOid,
dbOid, relNumber". Before commit 82ac34db20, we used
RelFileLocatorEqual(), which has this comment:

/*
* Note: RelFileLocatorEquals and RelFileLocatorBackendEquals compare
relNumber
* first since that is most likely to be different in two unequal
* RelFileLocators. It is probably redundant to compare spcOid if the
other
* fields are found equal, but do it anyway to be sure. Likewise for
checking
* the backend ID in RelFileLocatorBackendEquals.
*/

So we lost that micro-optimization. Should we reorder the checks in
BufTagMatchesRelFileLocator()?

- Heikki