Re: [PATCH] pageinspect function to decode infomasks

On 16 August 2017 at 23:14, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Aug 15, 2017 at 4:36 PM, Tomas Vondra
> <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
> > You might say that people investigating issues in this area of code
> should
> > be aware of how HEAP_XMIN_FROZEN is defined, and perhaps you're right ...
>
> Yes, I think that's what I would say. I mean, if you happen to NOT
> know that committed|invalid == frozen, but you DO know what committed
> means and what invalid means, then you're going to be *really*
> confused when you see committed and invalid set on the same tuple.
> Showing you frozen has got to be clearer.
>
> Now, I agree with you that a test like (enumval_tup->t_data &
> HEAP_XMIN_COMMITTED) could be confusing to someone who doesn't realize
> that HEAP_XMIN_FROZEN & HEAP_XMIN_COMMITTED == HEAP_XMIN_COMMITTED,
> but I think that's just one of those things that unfortunately is
> going to require adequate knowledge for people investigating issues.
> If there's an action item there, it might be to try to come up with a
> way to make the source code clearer.
>
>
For other multi-purpose flags we have macros, and I think it'd make sense
to use them here too.

Eschew direct use of HEAP_XMIN_COMMITTED, HEAP_XMIN_INVALID and
HEAP_XMIN_FROZEN in tests. Instead, consistently use HeapXminIsFrozen(),
HeapXminIsCommitted(), and HeapXminIsInvalid() or something like that.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services