Andy Fan <zhihuifan1...@163.com> writes: > Álvaro Herrera <alvhe...@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
>From e043551a0d3d0af7588a02e66335b0a9c8b6ae39 Mon Sep 17 00:00:00 2001 From: Andy Fan <zhihuifan1...@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
