Hi,
On 2026-01-15 14:22:09 +0800, Chao Li wrote:
> 3 - 0005 - bufmgr.c
> ```
> +inline void
> +MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
> ```
>
> It’s quite uncommon to extern an inline function. I think usually if we want
> to make an inline function accessible from external, we define it “static
> inline” in a header file. So, I guess “inline” is a typo here.
It works just fine to put an inline into the function definition, even if it's
an external function. That hints the compiler to inline it inside the
translation unit. Which is useful here, because it leads to
MarkBufferDirtyHint() being inlined into BufferFinishSetHintBits().
> 4 - 0005 - bufmgr.c
> ```
> /*
> * MarkBufferDirtyHint
> *
> * Mark a buffer dirty for non-critical changes.
> *
> * This is essentially the same as MarkBufferDirty, except:
> *
> * 1. The caller does not write WAL; so if checksums are enabled, we may need
> * to write an XLOG_FPI_FOR_HINT WAL record to protect against torn pages.
> * 2. The caller might have only a share-exclusive-lock instead of an
> * exclusive-lock on the buffer's content lock.
> ```
>
> For point 2, do we need to mention “For shared buffers”. Because this
> function also handles local buffer that doesn’t require a lock.
That's an old comment, just slightly rephrased. And I don't think it matters
that temp buffers don't need to be locked.
> 8 - 0005 - fsmpage.c
> ```
> * needs to be updated. exclusive_lock_held should be set to true if the
> * caller is already holding an exclusive lock, to avoid extra work.
> ```
>
> The function comment of fsm_search_avail() needs to be
> updated. exclusive_lock_held should be set to true if the caller is already
> holding a **share-exclusive or** exclusive lock.
Why?
> 9 - 0005 - fsmpage.c
> ```
> + if (!exclusive_lock_held)
> + BufferFinishSetHintBits(buf, false, true);
> ```
>
> Nit: just curious why set the third parameter as “true”? When the second
> (mark_dirty) is false, the third parameter is not used at all.
That's wrong, indeed. Not because of the mark_dirty, but because they aren't
standard pages.
> 10 - 0005 - freespace.c
> ```
> - * Reset the next slot pointer. This encourages the use of low-numbered
> - * pages, increasing the chances that a later vacuum can truncate the
> - * relation. We don't bother with marking the page dirty if it wasn't
> - * already, since this is just a hint.
> + * Try to reset the next slot pointer. This encourages the use of
> + * low-numbered pages, increasing the chances that a later vacuum can
> + * truncate the relation. We don't bother with marking the page dirty if
> + * it wasn't already, since this is just a hint.
> */
> LockBuffer(buf, BUFFER_LOCK_SHARE);
> - ((FSMPage) PageGetContents(page))->fp_next_slot = 0;
> + if (BufferBeginSetHintBits(buf))
> + {
> + ((FSMPage) PageGetContents(page))->fp_next_slot = 0;
> + BufferFinishSetHintBits(buf, false, false);
> + }
> ```
>
> Before this patch, we unconditionally set fp_next_slot, now the setting
> might be skipped. You have add “Try to” in the comment that has explained
> the possibility of skipping setting fp_next_slot. Would it be better to add
> a brief statement for what will result in when skipping setting
> fp_next_slot? Something like “Skipping the update only affects reuse, not
> correctness”.
I don't see the point. The whole paragraph is about how this isn't crucial.
> 11 - 0005 Maybe not a problem. In nbtree.h:
> ```
> /*
> * We need to be able to tell the difference between read and write
> * requests for pages, in order to do locking correctly.
> */
> #define BT_READ BUFFER_LOCK_SHARE
> #define BT_WRITE BUFFER_LOCK_EXCLUSIVE
> ```
>
> Can the new lock type BUFFER_LOCK_SHARE_EXCLUSIVE be used by nbt?
I don't think at the moment. If we introduce a user, we can add the
define. Orthogonally, I think stuff like BT_READ/BT_WRITE should eventually be
removed, the only thing it does is to make it harder to search the code.
> Maybe implicitly upgrading from BT_READ to BUFFER_LOCK_SHARE_EXCLUSIVE is
> good enough?
I don't see where that would trivially be safe.
> 12 - 0005 - heapam_visibility.c
>
> After this commit, tuple hint bits may remain unset if we can’t obtain
> share-exclusive permission. That’s fine because hint bits are advisory and
> optional, but this is a behavior change. Would it make sense to mention this
> explicitly and briefly in the SetHintBitsExt() comment?
That's not new though, we already could skip setting hint bits due to async
commits.
Greetings,
Andres Freund
