On Wed, Jan 7, 2026 at 3:15 AM Chao Li <[email protected]> wrote: > > I believe the reason why we add Assert(TransactionIdIsValid(dead_after)) > under HEAPTUPLE_RECENTLY_DEAD is to ensure that when > HeapTupleSatisfiesVacuumHorizon() returns HEAPTUPLE_RECENTLY_DEAD, dead_after > must be set. So the goal of the assert is to catch bugs of > HeapTupleSatisfiesVacuumHorizon(). > > From this perspective, I now feel dead_after should be initialized to > InvalidTransactionId. Otherwise, say HeapTupleSatisfiesVacuumHorizon() has a > bug and miss to set dead_after, then the assert mostly like won’t be fired, > because it holds a random value, most likely not be 0.
Actually, thinking about it more, I decided to remove the assertions on dead_after from those patches entirely. I don't use dead_after and only pass it in because HeapTupleSatisfiesVacuumHorizon requires it. In fact, I don't care if the function correctly sets dead_after since I don't use it. > + /* set if the query doesn't modify the rel */ > + SO_HINT_REL_READ_ONLY = 1 << 10, > ``` > > Nit: I think it’s better to replace “rel” to “relation”. For a function > comment, if there is a parameter named “rel”, then we can use it to refer to > the parameter, without such a context, I guess here a while word is better. k I'm currently working on a new version that incorporates Andres' review feedback and will post soon. - Melanie
