> On Jan 16, 2026, at 00:43, Andres Freund <[email protected]> wrote:
>
> 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().
>
Technically, that for sure works. I raised the comment basically because it’s
uncommon in the source tree. I just did a search for “inline” across all .c
files, only found two non-static inline functions: ReadBufferExtended() and
TrackNewBufferPin(), and they are all in bufmgr.c. So, adding a new one should
be fine. I should have done the search yesterday while reviewing.
>> 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?
Ah, I see. We will never use a share-exclusive lock for fsm_search_avail?
Currently, there are two callers of fsm_search_avail, one use a share lock, the
other uses an exclusive lock. My bad.
>> 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.
>
Yeah, I could understand the comment expresses that isn’t crucial, but just
felt that’s not “explicit”. I am fine with the current comment.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
