> On Dec 23, 2025, at 01:57, Melanie Plageman <[email protected]> wrote:
>
> On Mon, Dec 22, 2025 at 2:20 AM Chao Li <[email protected]> wrote:
>>
>> A few more comments on v29:
>
> Thanks for the continued review! I've attached v30.
>
>> 1 - 0002 - Looks like since 0002, visibilitymap_set()’s return value is no
>> longer used, so do we need to update the function and change return type to
>> void? I remember in some patches, to address Coverity alerts, people had to
>> do “(void) function_with_a_return_value()”.
>
> I was torn about whether or not to change the return value. Coverity
> doesn't always warn about unused return values. Usually it warns if it
> perceives the return value as needed for error checking or if it
> thinks not using the return value is incorrect. It may still warn in
> this case, but it's not obvious to me which way it would go.
>
> I have changed the function signature as you suggested in v30.
>
> My hesitation is that visibilitymap_set() is in a header file and
> could be used by extensions/forks, etc. Adding more information by
> changing a return value from void to non-void doesn't have any
> negative effect on those potential callers. But taking away a return
> value is more likely to affect them in a potentially negative way.
>
> However, I'm significantly changing the signature in this release, so
> everybody that used it will have to change their code completely
> anyway. Also, I just added a return value for visibilitymap_set() in
> the previous release (18). Historically, it returned void. So, I've
> gone with your suggestion.
From a previous patch, I learned from Peter Eisentraut that “We don't care
about ABI changes in major releases.”, see:
https://www.postgresql.org/message-id/70913dbd-dadf-4560-9f81-c0df72bf6578%40eisentraut.org
>> - * HEAP_PAGE_PRUNE_FREEZE indicates that we will also freeze tuples,
>> and
>> - * will return 'all_visible', 'all_frozen' flags to the caller.
>> + * HEAP_PAGE_PRUNE_FREEZE indicates that we will also freeze tuples
>>
>> Nit: a tailing dot is needed in the end of the comment line.
>
> I've changed it. One interesting thing is that our "policy" for
> periods in comments is that we don't put periods at the end of
> one-line comments and we do put them at the end of mult-line comment
> sentences. This is a one-line comment inside a comment block, so I
> wasn't sure what to do. If you noticed it, and it bothered you, it's
> easy enough to change, though.
If this is a one-line comment, I would have not been caring about the tailing
period.
The problem is this is a paragraph of a block comment, and the above and below
paragraphs all have tailing periods. So, for consistency, I raised the comment.
```
* HEAP_PAGE_PRUNE_MARK_UNUSED_NOW indicates that dead items can be set
* LP_UNUSED during pruning. <=== Has a tailing period
*
- * HEAP_PAGE_PRUNE_FREEZE indicates that we will also freeze tuples, and
- * will return 'all_visible', 'all_frozen' flags to the caller.
+ * HEAP_PAGE_PRUNE_FREEZE indicates that we will also freeze tuples
<=== Not a tailing period
*
* HEAP_PAGE_PRUNE_UPDATE_VM indicates that we will set the page's
status
* in the VM. <=== Has a tailing period
```
>
>> 9 - 0006
>>
>> @@ -3537,6 +3537,7 @@ heap_page_would_be_all_visible(Relation rel, Buffer
>> buf,
>> {
>> ItemId itemid;
>> HeapTupleData tuple;
>> + TransactionId dead_after = InvalidTransactionId;
>> ```
>>
>> This initialization seems to not needed, as
>> HeapTupleSatisfiesVacuumHorizon() will always set a value to it.
>
> I think this is a comment for a later patch in the set (you originally
> said it was from 0006), but I've changed dead_after to not be
> initialized like this.
My bad. This comment was actually for 0009. In v31, I see you have removed the
initialization to dead_after.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
