Remove HeapTupleheaderSetXmin{Committed,Invalid} functions
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
Attachments:
v1-0001-Remove-HeapTupleheaderSetXminCommitted-Invalid-fu.patchtext/x-diffDownload
From 343c867aff8838af4906c7a3084b33efb850f351 Mon Sep 17 00:00:00 2001
From: Andy Fan <zhihuifan1213@163.com>
Date: Wed, 25 Jun 2025 07:35:32 +0000
Subject: [PATCH v1 1/1] Remove HeapTupleheaderSetXminCommitted/Invalid
functions
They are neither be used nor be safe to be used, User should use
SetHintBits instead.
---
src/include/access/htup_details.h | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index aa957cf3b01..bc6094b001f 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -357,20 +357,6 @@ HeapTupleHeaderXminFrozen(const HeapTupleHeaderData *tup)
return (tup->t_infomask & HEAP_XMIN_FROZEN) == HEAP_XMIN_FROZEN;
}
-static inline void
-HeapTupleHeaderSetXminCommitted(HeapTupleHeaderData *tup)
-{
- Assert(!HeapTupleHeaderXminInvalid(tup));
- tup->t_infomask |= HEAP_XMIN_COMMITTED;
-}
-
-static inline void
-HeapTupleHeaderSetXminInvalid(HeapTupleHeaderData *tup)
-{
- Assert(!HeapTupleHeaderXminCommitted(tup));
- tup->t_infomask |= HEAP_XMIN_INVALID;
-}
-
static inline void
HeapTupleHeaderSetXminFrozen(HeapTupleHeaderData *tup)
{
--
2.45.1
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
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
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
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 mayThat 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
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)
Á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
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
Attachments:
v2-0001-Remove-HeapTupleheaderSetXminCommitted-Invalid-fu.patchtext/x-diffDownload
From e043551a0d3d0af7588a02e66335b0a9c8b6ae39 Mon Sep 17 00:00:00 2001
From: Andy Fan <zhihuifan1213@163.com>
Date: Wed, 25 Jun 2025 07:35:32 +0000
Subject: [PATCH v2 1/1] Remove HeapTupleheaderSetXminCommitted/Invalid
functions
Reasons include: (1). They are not used in core or any known
third-party extension in codesearch.debian.net. (2). They are not safe
to use when comparing with HeapTupleSetHintBits which is a public API
as well for third party. (3). These functions do has some maintenance
cost when people try to understanding them, including identifying they
are not used at all and the right one is SetHintBits.
Discussion:
https://www.postgresql.org/message-id/87frfmgigm.fsf%40163.com
---
src/include/access/htup_details.h | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index aa957cf3b01..bc6094b001f 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -357,20 +357,6 @@ HeapTupleHeaderXminFrozen(const HeapTupleHeaderData *tup)
return (tup->t_infomask & HEAP_XMIN_FROZEN) == HEAP_XMIN_FROZEN;
}
-static inline void
-HeapTupleHeaderSetXminCommitted(HeapTupleHeaderData *tup)
-{
- Assert(!HeapTupleHeaderXminInvalid(tup));
- tup->t_infomask |= HEAP_XMIN_COMMITTED;
-}
-
-static inline void
-HeapTupleHeaderSetXminInvalid(HeapTupleHeaderData *tup)
-{
- Assert(!HeapTupleHeaderXminCommitted(tup));
- tup->t_infomask |= HEAP_XMIN_INVALID;
-}
-
static inline void
HeapTupleHeaderSetXminFrozen(HeapTupleHeaderData *tup)
{
--
2.45.1