Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Remove HeapTupleheaderSetXminCommitted/Invalid functions

  2. Convert macros to static inline functions (htup_details.h, itup.h)

  3. Change the way we mark tuples as frozen.

  4. Code review for HeapTupleHeader changes. Add version number to page headers

  5. This patch wraps all accesses to t_xmin, t_cmin, t_xmax, and t_cmax in

  1. Remove HeapTupleheaderSetXmin{Committed,Invalid} functions

    Andy Fan <zhihuifan1213@163.com> — 2025-06-25T07:47:27Z

    Hi,

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.

commit ed905d64c6b81a78627f895918d4ac723d46865c (HEAD -> heap_set_bits)
Author: Andy Fan <zhihuifan1213@163.com>
Date:   Wed Jun 25 07:35:32 2025 +0000

    Remove HeapTupleheaderSetXminCommitted/Invalid functions
    
    They are neither be used nor be safe to be used, User should use
    SetHintBits instead.

What do you think?

-- 
Best Regards
Andy Fan

  2. Re: Remove HeapTupleheaderSetXmin{Committed,Invalid} functions

    Michael Paquier <michael@paquier.xyz> — 2025-06-25T08:15:31Z

    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.
--
Michael

  3. Re: Remove HeapTupleheaderSetXmin{Committed,Invalid} functions

    Andy Fan <zhihuifan1213@163.com> — 2025-06-25T09:28:50Z

    Michael Paquier <michael@paquier.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

Just in case someone use it, it may ask for public API for SetHintBits. 

Thanks for have a look.

-- 
Best Regards
Andy Fan

  4. Re: Remove HeapTupleheaderSetXmin{Committed,Invalid} functions

    Nathan Bossart <nathandbossart@gmail.com> — 2025-06-25T15:04:19Z

    On Wed, Jun 25, 2025 at 09:28:50AM +0000, Andy Fan wrote:
> Michael Paquier <michael@paquier.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

  5. Re: Remove HeapTupleheaderSetXmin{Committed,Invalid} functions

    Andy Fan <zhihuifan1213@163.com> — 2025-06-25T23:58:29Z

    Nathan Bossart <nathandbossart@gmail.com> writes:

> On Wed, Jun 25, 2025 at 09:28:50AM +0000, Andy Fan wrote:
>> Michael Paquier <michael@paquier.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

  6. Re: Remove HeapTupleheaderSetXmin{Committed,Invalid} functions

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-06-26T12:06:34Z

    Hmm

So these functions were created from macros in commit 34694ec888d6,
which themselves had been added for the first time in commit
37484ad2aace.  However, it appears that they were added only because
they were mirroring HeapTupleHeaderSetXminFrozen(), and while the latter
was immediately used, the other two weren't and never have been.

(For a short while between June and September 2002 we had a different
macro also called HeapTupleHeaderSetXminInvalid -- added by commit
3c35face4108 and removed by commit c7a165adc64e -- and curiously enough
it was also entirely unused.)

I think Andy is right that these should be removed, not only because
they are unsafe but because they are dead code.

codesearch.debian.net shows no matches by grep, other than
htup_internals.h itself.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)

  7. Re: Remove HeapTupleheaderSetXmin{Committed,Invalid} functions

    Andy Fan <zhihuifan1213@163.com> — 2025-06-27T00:02:17Z

    Álvaro Herrera <alvherre@kurilemu.de> writes:

> Hmm
>
> So these functions were created from macros in commit 34694ec888d6,
> which themselves had been added for the first time in commit
> 37484ad2aace.  However, it appears that they were added only because
> they were mirroring HeapTupleHeaderSetXminFrozen(), and while the latter
> was immediately used, the other two weren't and never have been.
>
> (For a short while between June and September 2002 we had a different
> macro also called HeapTupleHeaderSetXminInvalid -- added by commit
> 3c35face4108 and removed by commit c7a165adc64e -- and curiously enough
> it was also entirely unused.)

Thanks for checking the history, and part of c7a165adc64e (removing
HeapTupleHeaderSetXminInvalid from htup.h) is pretty similar with the
case there.  

> I think Andy is right that these should be removed, not only because
> they are unsafe but because they are dead code.

Yes, I suggested with the two reasons. When I knew removing a public
API has potential to break some third-party extension even they are not
used in core, then the *unsafe* part encourage to do so.

Acutally no maintaince cost for the dead code is only true when no one
read/think about them, otherwise the cost is there. The
HeapTupleheaderSetXminCommitted has misleaded me to think I can do 
something there, but the fact is (1) they are never used. (2) the right 
place is SetHintBits. 

> codesearch.debian.net shows no matches by grep, other than
> htup_internals.h itself.

Thanks for sharing codesearch.decbian.net which looks a great project.

I just found even there is such code, core has already provided public
API HeapTupleSetHintBits which can be used as a replacement and it is
safe. 

-- 
Best Regards
Andy Fan

  8. Re: Remove HeapTupleheaderSetXmin{Committed,Invalid} functions

    Andy Fan <zhihuifan1213@163.com> — 2025-06-28T05:00:04Z

    Andy Fan <zhihuifan1213@163.com> writes:

> Álvaro Herrera <alvherre@kurilemu.de> writes:
>
>> Hmm
>>
>> So these functions were created from macros in commit 34694ec888d6,
>> which themselves had been added for the first time in commit
>> 37484ad2aace.  However, it appears that they were added only because
>> they were mirroring HeapTupleHeaderSetXminFrozen(), and while the latter
>> was immediately used, the other two weren't and never have been.
>>
>> (For a short while between June and September 2002 we had a different
>> macro also called HeapTupleHeaderSetXminInvalid -- added by commit
>> 3c35face4108 and removed by commit c7a165adc64e -- and curiously enough
>> it was also entirely unused.)
>
> Thanks for checking the history, and part of c7a165adc64e (removing
> HeapTupleHeaderSetXminInvalid from htup.h) is pretty similar with the
> case there.  
>
>> I think Andy is right that these should be removed, not only because
>> they are unsafe but because they are dead code.
>
> Yes, I suggested with the two reasons. When I knew removing a public
> API has potential to break some third-party extension even they are not
> used in core, then the *unsafe* part encourage to do so.
>
> Acutally no maintaince cost for the dead code is only true when no one
> read/think about them, otherwise the cost is there. The
> HeapTupleheaderSetXminCommitted has misleaded me to think I can do 
> something there, but the fact is (1) they are never used. (2) the right 
> place is SetHintBits. 
>
>> codesearch.debian.net shows no matches by grep, other than
>> htup_internals.h itself.
>
> Thanks for sharing codesearch.decbian.net which looks a great project.
>
> I just found even there is such code, core has already provided public
> API HeapTupleSetHintBits which can be used as a replacement and it is
> safe. 

The attached is the v2, I just change the commit message to address the
concern from Michael and Nathan. I also registed this into
https://commitfest.postgresql.org/patch/5870/.

Hi Michael and Nathan, has the new discussion addressed your concerns,
or do you still think we should keep them as it is?

Thanks
-- 
Best Regards
Andy Fan