On Tue, Aug 15, 2017 at 4:36 PM, Tomas Vondra <tomas.vondra@2ndquadrant.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.
