[PATCH] pageinspect function to decode infomasks

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
81 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

[PATCH] pageinspect function to decode infomasks

Craig Ringer-3
Hi

Whenever I'm debugging some kind of corruption incident, possible visibility bug, etc, I always land up staring at integer infomasks or using a SQL helper function to decode them.

That's silly, so here's a patch to teach pageinspect how to decode infomasks to a human readable array of flag names.

Example:

SELECT t_infomask, t_infomask2, flags
FROM heap_page_items(get_raw_page('test1', 0)),
     LATERAL heap_infomask_flags(t_infomask, t_infomask2, true) m(flags);
 t_infomask | t_infomask2 |                                   flags                                    
------------+-------------+----------------------------------------------------------------------------
       2816 |           2 | {HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID,HEAP_XMIN_FROZEN}
(1 row)


To decode individual mask integers you can just call it directly. It's strict, so pass 0 for the other mask if you don't have both, e.g.

SELECT heap_infomask_flags(2816, 0);

The patch backports easily to older pageinspect versions for when you're debugging something old.

BTW, I used text[] not enums. That costs a fair bit of memory, but it doesn't seem worth worrying too much about in this context. 

For convenience it also tests and reports HEAP_LOCKED_UPGRADED and HEAP_XMAX_IS_LOCKED_ONLY as pseudo-flags.

I decided not to filter out HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID when HEAP_XMIN_FROZEN is set; that doesn't make sense when we examine HEAP_XMAX_IS_LOCKED_ONLY or HEAP_LOCKED_UPGRADED, and filtering them out could be just as confusing as leaving them in.

The infomask2 natts mask is ignored. You can bitwise-and it out in SQL pretty easily if needed. I could output it here as a constructed text datum, but it seems mostly pointless.

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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

v1-0001-Introduce-heap_infomask_flags-to-decode-infomask-.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Craig Ringer-3
On 20 July 2017 at 11:33, Craig Ringer <[hidden email]> wrote:
Hi

Whenever I'm debugging some kind of corruption incident, possible visibility bug, etc, I always land up staring at integer infomasks or using a SQL helper function to decode them.

That's silly, so here's a patch to teach pageinspect how to decode infomasks to a human readable array of flag names.

Example:

SELECT t_infomask, t_infomask2, flags
FROM heap_page_items(get_raw_page('test1', 0)),
     LATERAL heap_infomask_flags(t_infomask, t_infomask2, true) m(flags);
 t_infomask | t_infomask2 |                                   flags                                    
------------+-------------+----------------------------------------------------------------------------
       2816 |           2 | {HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID,HEAP_XMIN_FROZEN}
(1 row)


To decode individual mask integers you can just call it directly. It's strict, so pass 0 for the other mask if you don't have both, e.g.

SELECT heap_infomask_flags(2816, 0);

The patch backports easily to older pageinspect versions for when you're debugging something old.

BTW, I used text[] not enums. That costs a fair bit of memory, but it doesn't seem worth worrying too much about in this context. 

For convenience it also tests and reports HEAP_LOCKED_UPGRADED and HEAP_XMAX_IS_LOCKED_ONLY as pseudo-flags.

I decided not to filter out HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID when HEAP_XMIN_FROZEN is set

Er, decided not to filter out  HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID. Obviously wouldn't filter out HEAP_XMAX_INVALID, that was a copy-paste'o.

I wonder if it's worth dropping the HEAP_ prefix. Meh, anyway, usable as-is.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Peter Geoghegan-4
In reply to this post by Craig Ringer-3
On Wed, Jul 19, 2017 at 8:33 PM, Craig Ringer <[hidden email]> wrote:

> That's silly, so here's a patch to teach pageinspect how to decode infomasks
> to a human readable array of flag names.
>
> Example:
>
> SELECT t_infomask, t_infomask2, flags
> FROM heap_page_items(get_raw_page('test1', 0)),
>      LATERAL heap_infomask_flags(t_infomask, t_infomask2, true) m(flags);
>  t_infomask | t_infomask2 |                                   flags
> ------------+-------------+----------------------------------------------------------------------------
>        2816 |           2 |
> {HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID,HEAP_XMIN_FROZEN}
> (1 row)

Seems like a good idea to me.

--
Peter Geoghegan


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Julien Rouhaud
On Thu, Jul 20, 2017 at 5:44 AM, Peter Geoghegan <[hidden email]> wrote:

> On Wed, Jul 19, 2017 at 8:33 PM, Craig Ringer <[hidden email]> wrote:
>> That's silly, so here's a patch to teach pageinspect how to decode infomasks
>> to a human readable array of flag names.
>>
>> Example:
>>
>> SELECT t_infomask, t_infomask2, flags
>> FROM heap_page_items(get_raw_page('test1', 0)),
>>      LATERAL heap_infomask_flags(t_infomask, t_infomask2, true) m(flags);
>>  t_infomask | t_infomask2 |                                   flags
>> ------------+-------------+----------------------------------------------------------------------------
>>        2816 |           2 |
>> {HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID,HEAP_XMIN_FROZEN}
>> (1 row)
>
> Seems like a good idea to me.
>

+1, it'll be really helpful.


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Masahiko Sawada
On Thu, Jul 20, 2017 at 3:13 PM, Julien Rouhaud <[hidden email]> wrote:

> On Thu, Jul 20, 2017 at 5:44 AM, Peter Geoghegan <[hidden email]> wrote:
>> On Wed, Jul 19, 2017 at 8:33 PM, Craig Ringer <[hidden email]> wrote:
>>> That's silly, so here's a patch to teach pageinspect how to decode infomasks
>>> to a human readable array of flag names.
>>>
>>> Example:
>>>
>>> SELECT t_infomask, t_infomask2, flags
>>> FROM heap_page_items(get_raw_page('test1', 0)),
>>>      LATERAL heap_infomask_flags(t_infomask, t_infomask2, true) m(flags);
>>>  t_infomask | t_infomask2 |                                   flags
>>> ------------+-------------+----------------------------------------------------------------------------
>>>        2816 |           2 |
>>> {HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID,HEAP_XMIN_FROZEN}
>>> (1 row)
>>
>> Seems like a good idea to me.
>>
>
> +1, it'll be really helpful.
>

+1.
When I investigated data corruption incident I also wrote a plpgsql
function for the same purpose, and it was very useful. I think we can
have the similar thing for lp_flags as well.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Ashutosh Sharma
I had a quick look into this patch and also tested it and following
are my observations.

1) I am seeing a server crash when passing any non meaningful value
for t_infomask2 to heap_infomask_flags().

postgres=# SELECT heap_infomask_flags(2816, 3);
server closed the connection unexpectedly
    This probably means the server terminated abnormally
    before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!> \q

Following is the backtrace,

(gdb) bt
#0  0x0000000000d9c55b in pg_detoast_datum (datum=0x0) at fmgr.c:1833
#1  0x0000000000b87374 in construct_md_array (elems=0x2ad74c0,
nulls=0x0, ndims=1, dims=0x7ffc0b0bbcd0, lbs=0x7ffc0b0bbcc0,
elmtype=25, elmlen=-1,
    elmbyval=0 '\000', elmalign=105 'i') at arrayfuncs.c:3382
#2  0x0000000000b8709f in construct_array (elems=0x2ad74c0, nelems=10,
elmtype=25, elmlen=-1, elmbyval=0 '\000', elmalign=105 'i') at
arrayfuncs.c:3316
#3  0x00007fb8001603a5 in heap_infomask_flags (fcinfo=0x2ad3b88) at
heapfuncs.c:597
#4  0x000000000082f4cd in ExecInterpExpr (state=0x2ad3aa0,
econtext=0x2ad3750, isnull=0x7ffc0b0bbf67 "") at execExprInterp.c:672
#5  0x000000000088b832 in ExecEvalExprSwitchContext (state=0x2ad3aa0,
econtext=0x2ad3750, isNull=0x7ffc0b0bbf67 "")
    at ../../../src/include/executor/executor.h:290
#6  0x000000000088b8e3 in ExecProject (projInfo=0x2ad3a98) at
../../../src/include/executor/executor.h:324
#7  0x000000000088bb89 in ExecResult (node=0x2ad36b8) at nodeResult.c:132
#8  0x00000000008494fe in ExecProcNode (node=0x2ad36b8) at execProcnode.c:416
#9  0x000000000084125d in ExecutePlan (estate=0x2ad34a0,
planstate=0x2ad36b8, use_parallel_mode=0 '\000', operation=CMD_SELECT,
sendTuples=1 '\001',
    numberTuples=0, direction=ForwardScanDirection, dest=0x2ac0ae0,
execute_once=1 '\001') at execMain.c:1693
#10 0x000000000083d54b in standard_ExecutorRun (queryDesc=0x2a42880,
direction=ForwardScanDirection, count=0, execute_once=1 '\001') at
execMain.c:362
#11 0x000000000083d253 in ExecutorRun (queryDesc=0x2a42880,
direction=ForwardScanDirection, count=0, execute_once=1 '\001') at
execMain.c:305
#12 0x0000000000b3dd8f in PortalRunSelect (portal=0x2ad1490, forward=1
'\001', count=0, dest=0x2ac0ae0) at pquery.c:932
#13 0x0000000000b3d7e7 in PortalRun (portal=0x2ad1490,
count=9223372036854775807, isTopLevel=1 '\001', run_once=1 '\001',
dest=0x2ac0ae0, altdest=0x2ac0ae0,
    completionTag=0x7ffc0b0bc2c0 "") at pquery.c:773
#14 0x0000000000b31fe4 in exec_simple_query (query_string=0x2a9d9a0
"SELECT heap_infomask_flags(11008, 1111, true);") at postgres.c:1099
#15 0x0000000000b3a727 in PostgresMain (argc=1, argv=0x2a49eb0,
dbname=0x2a1d480 "postgres", username=0x2a49d18 "ashu") at
postgres.c:4090
#16 0x0000000000a2cb3f in BackendRun (port=0x2a3e700) at postmaster.c:4357
#17 0x0000000000a2bc63 in BackendStartup (port=0x2a3e700) at postmaster.c:4029
#18 0x0000000000a248ab in ServerLoop () at postmaster.c:1753
#19 0x0000000000a236a9 in PostmasterMain (argc=3, argv=0x2a1b2b0) at
postmaster.c:1361
#20 0x00000000008d8054 in main (argc=3, argv=0x2a1b2b0) at main.c:228

2) I can see the documentation for heap_infomask(). But, I do not see
it being defined or used anywhere in the patch.

+     <para>
+      The <function>heap_infomask</function> function can be used to unpack the
+      recognised bits of the infomasks of heap tuples.
+     </para>

3) If show_combined flag is set to it's default value and a tuple is
frozen then may i know the reason for not showing it as frozen tuple
when t_infomask2
is passed as zero.

postgres=# SELECT heap_infomask_flags(2816, 0);
                    heap_infomask_flags
-----------------------------------------------------------
 {HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID}
(1 row)

postgres=# SELECT heap_infomask_flags(2816, 1);
                            heap_infomask_flags
----------------------------------------------------------------------------
 {HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID,HEAP_XMIN_FROZEN}
(1 row)


4) I think, it would be better to use the same argument name for the
newly added function i.e heap_infomask_flags() in both documentation
and sql file. I am basically refering to 'include_combined' argument.
IF you see the function definition, the argument name used is
'include_combined' whereas in documentation you have mentioned
'show_combined'.

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

On Thu, Jul 20, 2017 at 11:56 AM, Masahiko Sawada <[hidden email]> wrote:

> On Thu, Jul 20, 2017 at 3:13 PM, Julien Rouhaud <[hidden email]> wrote:
>> On Thu, Jul 20, 2017 at 5:44 AM, Peter Geoghegan <[hidden email]> wrote:
>>> On Wed, Jul 19, 2017 at 8:33 PM, Craig Ringer <[hidden email]> wrote:
>>>> That's silly, so here's a patch to teach pageinspect how to decode infomasks
>>>> to a human readable array of flag names.
>>>>
>>>> Example:
>>>>
>>>> SELECT t_infomask, t_infomask2, flags
>>>> FROM heap_page_items(get_raw_page('test1', 0)),
>>>>      LATERAL heap_infomask_flags(t_infomask, t_infomask2, true) m(flags);
>>>>  t_infomask | t_infomask2 |                                   flags
>>>> ------------+-------------+----------------------------------------------------------------------------
>>>>        2816 |           2 |
>>>> {HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID,HEAP_XMIN_FROZEN}
>>>> (1 row)
>>>
>>> Seems like a good idea to me.
>>>
>>
>> +1, it'll be really helpful.
>>
>
> +1.
> When I investigated data corruption incident I also wrote a plpgsql
> function for the same purpose, and it was very useful. I think we can
> have the similar thing for lp_flags as well.
>
> Regards,
>
> --
> Masahiko Sawada
> NIPPON TELEGRAPH AND TELEPHONE CORPORATION
> NTT Open Source Software Center
>
>
> --
> Sent via pgsql-hackers mailing list ([hidden email])
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Craig Ringer-3


On 20 Jul. 2017 19:09, "Ashutosh Sharma" <[hidden email]> wrote:
I had a quick look into this patch and also tested it and following
are my observations.

Thanks very much.

I'll expand the tests to cover various normal and nonsensical masks and combinations and fix the identified issues.

This was a quick morning's work in amongst other things so not surprised I missed a few details. The check is appreciated.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Craig Ringer-3
In reply to this post by Ashutosh Sharma
On 20 July 2017 at 19:09, Ashutosh Sharma <[hidden email]> wrote:
I had a quick look into this patch and also tested it and following
are my observations.

1) I am seeing a server crash when passing any non meaningful value
for t_infomask2 to heap_infomask_flags().

postgres=# SELECT heap_infomask_flags(2816, 3);
server closed the connection unexpectedly
    This probably means the server terminated abnormally
    before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!> \q

Following is the backtrace,

(gdb) bt
#0  0x0000000000d9c55b in pg_detoast_datum (datum=0x0) at fmgr.c:1833
#1  0x0000000000b87374 in construct_md_array (elems=0x2ad74c0,
nulls=0x0, ndims=1, dims=0x7ffc0b0bbcd0, lbs=0x7ffc0b0bbcc0,
elmtype=25, elmlen=-1,
    elmbyval=0 '\000', elmalign=105 'i') at arrayfuncs.c:3382
#2  0x0000000000b8709f in construct_array (elems=0x2ad74c0, nelems=10,
elmtype=25, elmlen=-1, elmbyval=0 '\000', elmalign=105 'i') at
arrayfuncs.c:3316
#3  0x00007fb8001603a5 in heap_infomask_flags (fcinfo=0x2ad3b88) at
heapfuncs.c:597


Fixed.
 
2) I can see the documentation for heap_infomask(). But, I do not see
it being defined or used anywhere in the patch.

+     <para>
+      The <function>heap_infomask</function> function can be used to unpack the
+      recognised bits of the infomasks of heap tuples.
+     </para>

Fixed. Renamed the function, missed a spot.
 
3) If show_combined flag is set to it's default value and a tuple is
frozen then may i know the reason for not showing it as frozen tuple
when t_infomask2
is passed as zero.

It was a consequence of (1). Fixed.


4) I think, it would be better to use the same argument name for the
newly added function i.e heap_infomask_flags() in both documentation
and sql file. I am basically refering to 'include_combined' argument.
IF you see the function definition, the argument name used is
'include_combined' whereas in documentation you have mentioned
'show_combined'.

Fixed, thanks.

I want to find time to expand the tests on this some more and look more closely, but here it is for now.

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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

v2-0001-Introduce-heap_infomask_flags-to-decode-infomask-.patch (22K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Moon, Insung
In reply to this post by Craig Ringer-3

Dear Craig Ringer

 

Frist, thank you for implementing the necessary function.

 

but, i have some question.

 

question 1) vacuum freeze hint bits

If run a vacuum freeze, bits in the infomask will be 0x0300.

in this case, if output the value of informsk in the run to you modified,

HEAP_XMIN_COMMITTED(0x0100), HEAP_XMIN_INVALID(0x0200), HEAP_XMIN_FROZEN(0x0300)

all outputs to hint bits.

 

is it normal to output values?

 

if look at htup_details.h code,

 

#define HeapTupleHeaderXminInvalid(tup) \

( \

              ((tup)->t_infomask & (HEAP_XMIN_COMMITTED|HEAP_XMIN_INVALID)) == \

                            HEAP_XMIN_INVALID \

)

#define HeapTupleHeaderSetXminCommitted(tup) \

( \

              AssertMacro(!HeapTupleHeaderXminInvalid(tup)), \

              ((tup)->t_infomask |= HEAP_XMIN_COMMITTED) \

)

 

HEAP_XMIN_INVALID and HEAP_XMIN_COMMITTED can not be write simultaneously.

 

So I think the value of 0x0300 is to HEAP_XMIN_COMMITTED, HEAP_XMIN_FROZEN

Only output needs to be values.

 

 

question 2) xmax lock hint bits

similar to the vacuum freezeze question..

Assume that the infomask has a bit of 0x0050

 

In this case, if run on the code that you modified,

HEAP_XMAX_KEYSHR_LOCK(0x0010), HEAP_XMAX_EXCL_LOCK(0x0040), HEAP_XMAX_IS_LOCKED_ONLY

three hint bits are the output.

 

if look at htup_details.h code,

 

#define HEAP_XMAX_IS_SHR_LOCKED(infomask) \

              (((infomask) & HEAP_LOCK_MASK) == HEAP_XMAX_SHR_LOCK)

#define HEAP_XMAX_IS_EXCL_LOCKED(infomask) \

              (((infomask) & HEAP_LOCK_MASK) == HEAP_XMAX_EXCL_LOCK)

#define HEAP_XMAX_IS_KEYSHR_LOCKED(infomask) \

              (((infomask) & HEAP_LOCK_MASK) == HEAP_XMAX_KEYSHR_LOCK)

 

It is divided into to hint bits.

so I think this part needs to fix.

 

If my opinion may be wrong. So plz check one more time.

 

Regards.

Moon

 

From: [hidden email] [mailto:[hidden email]] On Behalf Of Craig Ringer
Sent: Thursday, July 20, 2017 8:53 PM
To: Ashutosh Sharma
Cc: PostgreSQL Hackers; Julien Rouhaud; Pavan Deolasee; Álvaro Herrera; Peter Eisentraut; Masahiko Sawada; abhijit Menon-Sen; Peter Geoghegan
Subject: Re: [HACKERS] [PATCH] pageinspect function to decode infomasks

 

 

 

On 20 Jul. 2017 19:09, "Ashutosh Sharma" <[hidden email]> wrote:

I had a quick look into this patch and also tested it and following
are my observations.

 

Thanks very much.

 

I'll expand the tests to cover various normal and nonsensical masks and combinations and fix the identified issues.

 

This was a quick morning's work in amongst other things so not surprised I missed a few details. The check is appreciated.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Craig Ringer-3
On 15 August 2017 at 09:11, Moon Insung <[hidden email]> wrote:

Dear Craig Ringer

 

Frist, thank you for implementing the necessary function.

 

but, i have some question.

 

question 1) vacuum freeze hint bits

If run a vacuum freeze, bits in the infomask will be 0x0300.

in this case, if output the value of informsk in the run to you modified,

HEAP_XMIN_COMMITTED(0x0100), HEAP_XMIN_INVALID(0x0200), HEAP_XMIN_FROZEN(0x0300)

all outputs to hint bits.

 

is it normal to output values?

 

if look at htup_details.h code,

 

#define HeapTupleHeaderXminInvalid(tup) \

( \

              ((tup)->t_infomask & (HEAP_XMIN_COMMITTED|HEAP_XMIN_INVALID)) == \

                            HEAP_XMIN_INVALID \

)

#define HeapTupleHeaderSetXminCommitted(tup) \

( \

              AssertMacro(!HeapTupleHeaderXminInvalid(tup)), \

              ((tup)->t_infomask |= HEAP_XMIN_COMMITTED) \

)

 

HEAP_XMIN_INVALID and HEAP_XMIN_COMMITTED can not be write simultaneously.


The bits are set, those macros just test to exclude the special meaning of both bits being set at once to mean "frozen".

I was reluctant to filter out  HEAP_XMIN_COMMITTED and HEAP_XMIN_INVALID when we detect that it's frozen, because that could well be misleading when debugging.

If you think that is useful, then I suggest you add an option so that when it's outputting the interpreted mask not the raw mask, it suppresses output of HEAP_XMIN_COMMITTED and HEAP_XMIN_INVALID if HEAP_XMIN_FROZEN.

question 2) xmax lock hint bits

similar to the vacuum freezeze question..

Assume that the infomask has a bit of 0x0050

 

In this case, if run on the code that you modified,

HEAP_XMAX_KEYSHR_LOCK(0x0010), HEAP_XMAX_EXCL_LOCK(0x0040), HEAP_XMAX_IS_LOCKED_ONLY

three hint bits are the output.

 

if look at htup_details.h code,

 

#define HEAP_XMAX_IS_SHR_LOCKED(infomask) \

              (((infomask) & HEAP_LOCK_MASK) == HEAP_XMAX_SHR_LOCK)

#define HEAP_XMAX_IS_EXCL_LOCKED(infomask) \

              (((infomask) & HEAP_LOCK_MASK) == HEAP_XMAX_EXCL_LOCK)

#define HEAP_XMAX_IS_KEYSHR_LOCKED(infomask) \

              (((infomask) & HEAP_LOCK_MASK) == HEAP_XMAX_KEYSHR_LOCK)

 

It is divided into to hint bits.

so I think this part needs to fix.


It's the same issue as above, with the same answer IMO.

If we're showing the raw mask bits we should show the raw mask bits only.

But if we're showing combined bits and masks too, I guess we should filter out the raw bits when matched by some mask.

I'm not entirely convinced by that, since I think hiding information could create more confusion than it fixes. I welcome others' views here.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Robert Haas
On Mon, Aug 14, 2017 at 9:59 PM, Craig Ringer <[hidden email]> wrote:
> The bits are set, those macros just test to exclude the special meaning of
> both bits being set at once to mean "frozen".
>
> I was reluctant to filter out  HEAP_XMIN_COMMITTED and HEAP_XMIN_INVALID
> when we detect that it's frozen, because that could well be misleading when
> debugging.

I don't think so -- the "committed" and "invalid" meanings are
effectively canceled when the "frozen" mask is present.

I mean, "committed" and "invalid" contradict each other...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Tomas Vondra-4


On 08/15/2017 03:24 PM, Robert Haas wrote:

> On Mon, Aug 14, 2017 at 9:59 PM, Craig Ringer <[hidden email]> wrote:
>> The bits are set, those macros just test to exclude the special meaning of
>> both bits being set at once to mean "frozen".
>>
>> I was reluctant to filter out  HEAP_XMIN_COMMITTED and HEAP_XMIN_INVALID
>> when we detect that it's frozen, because that could well be misleading when
>> debugging.
>
> I don't think so -- the "committed" and "invalid" meanings are
> effectively canceled when the "frozen" mask is present.
>
> I mean, "committed" and "invalid" contradict each other...
>

FWIW I agree with Craig - the functions should output the masks raw,
without any filtering. The reason is that when you're investigating data
corruption or unexpected behavior, all this is very useful when
reasoning about what might (not) have happened.

Or at least make the filtering optional.

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Masahiko Sawada
On Tue, Aug 15, 2017 at 10:59 PM, Tomas Vondra
<[hidden email]> wrote:

>
>
> On 08/15/2017 03:24 PM, Robert Haas wrote:
>>
>> On Mon, Aug 14, 2017 at 9:59 PM, Craig Ringer <[hidden email]>
>> wrote:
>>>
>>> The bits are set, those macros just test to exclude the special meaning
>>> of
>>> both bits being set at once to mean "frozen".
>>>
>>> I was reluctant to filter out  HEAP_XMIN_COMMITTED and HEAP_XMIN_INVALID
>>> when we detect that it's frozen, because that could well be misleading
>>> when
>>> debugging.
>>
>>
>> I don't think so -- the "committed" and "invalid" meanings are
>> effectively canceled when the "frozen" mask is present.
>>
>> I mean, "committed" and "invalid" contradict each other...
>>
>
> FWIW I agree with Craig - the functions should output the masks raw, without
> any filtering. The reason is that when you're investigating data corruption
> or unexpected behavior, all this is very useful when reasoning about what
> might (not) have happened.
>
> Or at least make the filtering optional.
>

I'd vote for having both and making one optional (perhaps filtering?).
Both are useful to me for the debugging and study purpose.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Robert Haas
In reply to this post by Tomas Vondra-4
On Tue, Aug 15, 2017 at 9:59 AM, Tomas Vondra
<[hidden email]> wrote:

>> I don't think so -- the "committed" and "invalid" meanings are
>> effectively canceled when the "frozen" mask is present.
>>
>> I mean, "committed" and "invalid" contradict each other...
>
> FWIW I agree with Craig - the functions should output the masks raw, without
> any filtering. The reason is that when you're investigating data corruption
> or unexpected behavior, all this is very useful when reasoning about what
> might (not) have happened.
>
> Or at least make the filtering optional.

I don't think "filtering" is the right way to think about it.  It's
just labeling each combination of bits with the meaning appropriate to
that combination of bits.

I mean, if you were displaying the contents of a CLOG entry, would you
want the value 3 to be displayed as COMMITTED ABORTED SUBCOMMITTED
because TRANSACTION_STATUS_COMMITTED|TRANSACTION_STATUS_ABORTED ==
TRANSACTION_STATUS_SUB_COMMITTED?

I realize that you may be used to thinking of the HEAP_XMIN_COMMITTED
and HEAP_XMAX_COMMITTED bits as two separate bits, but that's not
really true any more.  They're a 2-bit field that can have one of four
values: committed, aborted, frozen, or none of the above.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Tomas Vondra-4


On 08/15/2017 07:54 PM, Robert Haas wrote:

> On Tue, Aug 15, 2017 at 9:59 AM, Tomas Vondra
> <[hidden email]> wrote:
>>> I don't think so -- the "committed" and "invalid" meanings are
>>> effectively canceled when the "frozen" mask is present.
>>>
>>> I mean, "committed" and "invalid" contradict each other...
>>
>> FWIW I agree with Craig - the functions should output the masks raw, without
>> any filtering. The reason is that when you're investigating data corruption
>> or unexpected behavior, all this is very useful when reasoning about what
>> might (not) have happened.
>>
>> Or at least make the filtering optional.
>
> I don't think "filtering" is the right way to think about it.  It's
> just labeling each combination of bits with the meaning appropriate to
> that combination of bits.
>
> I mean, if you were displaying the contents of a CLOG entry, would you
> want the value 3 to be displayed as COMMITTED ABORTED SUBCOMMITTED
> because TRANSACTION_STATUS_COMMITTED|TRANSACTION_STATUS_ABORTED ==
> TRANSACTION_STATUS_SUB_COMMITTED?
>
> I realize that you may be used to thinking of the HEAP_XMIN_COMMITTED
> and HEAP_XMAX_COMMITTED bits as two separate bits, but that's not
> really true any more.  They're a 2-bit field that can have one of four
> values: committed, aborted, frozen, or none of the above.
>

All I'm saying is that having the complete information (knowing which
bits are actually set in the bitmask) is valuable when reasoning about
how you might have gotten to the current state. Which I think is what
Craig is after.

What I think we should not do is interpret the bitmasks (omitting some
of the information) assuming all the bits were set correctly.

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Robert Haas
On Tue, Aug 15, 2017 at 3:42 PM, Tomas Vondra
<[hidden email]> wrote:
> What I think we should not do is interpret the bitmasks (omitting some of
> the information) assuming all the bits were set correctly.

I'm still confused.  HEAP_XMIN_COMMITTED|HEAP_XMIN_ABORTED ==
HEAP_XMIN_FROZEN.  Nobody is proposing to omit anything; to the
contrary, what's being proposed is not to display the same thing twice
(and in a misleading fashion, to boot).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Tomas Vondra-4


On 08/15/2017 09:55 PM, Robert Haas wrote:

> On Tue, Aug 15, 2017 at 3:42 PM, Tomas Vondra
> <[hidden email]> wrote:
>> What I think we should not do is interpret the bitmasks (omitting some of
>> the information) assuming all the bits were set correctly.
>
> I'm still confused. HEAP_XMIN_COMMITTED|HEAP_XMIN_ABORTED ==
> HEAP_XMIN_FROZEN. Nobody is proposing to omit anything; to the
> contrary, what's being proposed is not to display the same thing
> twice (and in a misleading fashion, to boot).
>

I understand your point. Assume you're looking at this bit of code:

     if (HeapTupleHeaderXminCommitted(enumval_tup->t_data))
         return;

which is essentially

     if (enumval_tup->t_data & HEAP_XMIN_COMMITTED)
         return;

If the function only gives you HEAP_XMIN_FROZEN, how likely is it you
miss this actually evaluates as true?

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 ...

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Moon, Insung
I checked for code related to infomask.
(add flag state -- HEAP_XMIN_COMMITTED, HEAP_XMIN_INVALID, HEAP_XMIN_FROZEN)

first i'm still beginner level about postgresql, so my opinion may be wrong.

if the "HEAP_XMIN_COMMITTED" flag is added, check the function of "HeapTupleHeaderXminInvalid"
if the "HEAP_XMIN_INVALID" flag is added, check the function of "HeapTupleHeaderXminCommitted"
if the "HEAP_XMIN_FROZEN" flag is added, use the "HeapTupleHeaderSetXminFrozen" function or
use the code as
--------------------------------------
xid = HeapTupleHeaderGetXmin(tuple);
if (TransactionIdIsNormal(xid))
{
    if (TransactionIdPrecedes(xid, cutoff_xid))
    {
        frz->t_infomask |= HEAP_XMIN_FROZEN;
        changed = true;
    }
    else
        totally_frozen = false;
}
--------------------------------------
to add the flag.

so as a result, HEAP_XMIN_INVALID and HEAP_XMIN_COMMITTED is cannot coexist.
unfortunately, i don't know if HEAP_XMIN_COMMITTED and HEAP_XMIN_FROZEN flags can coexist.

so i think it's also a good idea to output the raw masks, without any filtering.
however, i think the information that is presented to the user should inform us which flags was entered.

Regards.
Moon

> -----Original Message-----
> From: [hidden email]
> [mailto:[hidden email]] On Behalf Of Tomas Vondra
> Sent: Wednesday, August 16, 2017 5:36 AM
> To: Robert Haas
> Cc: [hidden email]
> Subject: Re: [HACKERS] [PATCH] pageinspect function to decode infomasks
>
>
>
> On 08/15/2017 09:55 PM, Robert Haas wrote:
> > On Tue, Aug 15, 2017 at 3:42 PM, Tomas Vondra
> > <[hidden email]> wrote:
> >> What I think we should not do is interpret the bitmasks (omitting
> >> some of the information) assuming all the bits were set correctly.
> >
> > I'm still confused. HEAP_XMIN_COMMITTED|HEAP_XMIN_ABORTED ==
> > HEAP_XMIN_FROZEN. Nobody is proposing to omit anything; to the
> > contrary, what's being proposed is not to display the same thing twice
> > (and in a misleading fashion, to boot).
> >
>
> I understand your point. Assume you're looking at this bit of code:
>
>      if (HeapTupleHeaderXminCommitted(enumval_tup->t_data))
>          return;
>
> which is essentially
>
>      if (enumval_tup->t_data & HEAP_XMIN_COMMITTED)
>          return;
>
> If the function only gives you HEAP_XMIN_FROZEN, how likely is it you miss
> this actually evaluates as true?
>
> 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 ...
>
> regards
>
> --
> Tomas Vondra                  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list ([hidden email]) To make
> changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers




--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Craig Ringer-3
In reply to this post by Tomas Vondra-4
On 16 August 2017 at 03:42, Tomas Vondra <[hidden email]> wrote:


On 08/15/2017 07:54 PM, Robert Haas wrote:
On Tue, Aug 15, 2017 at 9:59 AM, Tomas Vondra
<[hidden email]> wrote:
I don't think so -- the "committed" and "invalid" meanings are
effectively canceled when the "frozen" mask is present.

I mean, "committed" and "invalid" contradict each other...

FWIW I agree with Craig - the functions should output the masks raw, without
any filtering. The reason is that when you're investigating data corruption
or unexpected behavior, all this is very useful when reasoning about what
might (not) have happened.

Or at least make the filtering optional.

I don't think "filtering" is the right way to think about it.  It's
just labeling each combination of bits with the meaning appropriate to
that combination of bits.

I mean, if you were displaying the contents of a CLOG entry, would you
want the value 3 to be displayed as COMMITTED ABORTED SUBCOMMITTED
because TRANSACTION_STATUS_COMMITTED|TRANSACTION_STATUS_ABORTED ==
TRANSACTION_STATUS_SUB_COMMITTED?

I realize that you may be used to thinking of the HEAP_XMIN_COMMITTED
and HEAP_XMAX_COMMITTED bits as two separate bits, but that's not
really true any more.  They're a 2-bit field that can have one of four
values: committed, aborted, frozen, or none of the above.


All I'm saying is that having the complete information (knowing which bits are actually set in the bitmask) is valuable when reasoning about how you might have gotten to the current state. Which I think is what Craig is after.

What I think we should not do is interpret the bitmasks (omitting some of the information) assuming all the bits were set correctly.

I agree, and the patch already does half of this: it can output just the raw bit flags, or it can interpret them to show HEAP_XMIN_FROZEN etc.

So the required change, which seems to have broad agreement, is to have the "interpret the bits" mode show only HEAP_XMIN_FROZEN when it sees HEAP_XMIN_COMMITTED|HEAP_XMIN_INVALID, etc. We can retain raw-flags output as-is for when seriously bogus state is suspected.

Any takers?
 
--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pageinspect function to decode infomasks

Robert Haas
In reply to this post by Tomas Vondra-4
On Tue, Aug 15, 2017 at 4:36 PM, Tomas Vondra
<[hidden email]> 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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
12345