Re: [PATCH] pageinspect function to decode infomasks

Hi Craig,

On Thu, Aug 17, 2017 at 5:50 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 16 August 2017 at 23:14, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> 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.
>
> --

Are you planning to work on the review comments from Robert, Moon
Insung and supply the new patch. I just had a quick glance into this
mail thread (after a long time) and could understand Robert's concern
till some extent. I think, he is trying to say that if a tuple is
frozen (committed|invalid) then it shouldn't be shown as COMMITTED and
INVALID together in fact it should just be displayed as FROZEN tuple.

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com