Re: Remove HeapTupleheaderSetXmin{Committed,Invalid} functions

Nathan Bossart <nathandbossart(at)gmail(dot)com> writes:

> On Wed, Jun 25, 2025 at 09:28:50AM +0000, Andy Fan wrote:
>> Michael Paquier <michael(at)paquier(dot)xyz> writes:
>>> On Wed, Jun 25, 2025 at 07:47:27AM +0000, Andy Fan wrote:
>>>> When I am reading the code, I first thought I can do something in
>>>> HeapTupleheaderSetXminCommitted, then I realized we have SetHintBits.
>>>> After some research, I find HeapTupleHeaderSetXminCommitted is never
>>>> used and it looks not safe to use after comparing with SetHintBits. So
>>>> to avoid future confusion or misuse, I'd suggest to remove it. I think
>>>> HeapTupleHeaderSetXminInvalid should be the same. So here is the patch.
>>>
>>> There is a cost in removing such code, even if not used in core:
>>> extension code outside of core may use it, and they would fail to
>>> compile. This can break code, and keeping them around has no
>>> maintenance cost.
>>
>> I did thought about the impact on third-party extension, but I suggested
>> the patch not only because it is not used but also it is not safe to set
>> the flags directly. What do you think about this?
>>
>> From comments of SetHintBits:
>>
>> * It is only safe to set a transaction-committed hint bit if we know the
>> * transaction's commit record is guaranteed to be flushed to disk before the
>> * buffer, or if the table is temporary or unlogged and will be obliterated by
>> * a crash anyway. We cannot change the LSN of the page here, because we may
>
> That doesn't mean they're fundamentally unsafe in all cases. And besides,
> there are many other ways to do unsafe things in extensions, including
> using the HEAP_XMIN_* macros directly instead of via these inline
> functions, so I don't think removing them gains us much.

Sure we can't avoid them totally, so I agree with your "removing them
not gains us much*, but that doesn't mean core should provide convience
for such purpose. In my opinion, if we find such things, we can try to
avoid them if that would not cost us much.

All of my opinion are based on the following fact for set a hintbits is
not well konwn by most user. But if you are more confident it is well
konwn by most people, then I'm agree with we could keep them.

* It is only safe to set a transaction-committed hint bit if we know the
* transaction's commit record is guaranteed to be flushed to disk before the
* buffer, or if the table is temporary or unlogged and will be obliterated by
* a crash anyway.

--
Best Regards
Andy Fan