Re: Remove HeapTupleheaderSetXmin{Committed,Invalid} functions

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.

--
nathan