Re: Dump pageinspect to 1.2: page_header with pg_lsn datatype

Hi all,

Please find attached a patch to dump pageinspect to 1.2. This simply
changes page_header to use the new internal datatype pg_lsn instead of text.

Regards,
--
Michael

Michael Paquier escribió:
> Hi all,
>
> Please find attached a patch to dump pageinspect to 1.2. This simply
> changes page_header to use the new internal datatype pg_lsn instead of text.

Uhm. Does this crash if you pg_upgrade a server that has 1.1 installed?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Feb 23, 2014 at 6:09 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:

> Michael Paquier escribió:
> > Hi all,
> >
> > Please find attached a patch to dump pageinspect to 1.2. This simply
> > changes page_header to use the new internal datatype pg_lsn instead of
> text.
>
> Uhm. Does this crash if you pg_upgrade a server that has 1.1 installed?
>
Oops yes you're right., it is obviously broken. Just re-thinking about
that, dumping this module just to change an argument type does not seem to
be worth the code complication. Thoughts?
Regards,
--
Michael

On 2014-02-24 17:53:31 +0900, Michael Paquier wrote:
> On Sun, Feb 23, 2014 at 6:09 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:
>
> > Michael Paquier escribió:
> > > Hi all,
> > >
> > > Please find attached a patch to dump pageinspect to 1.2. This simply
> > > changes page_header to use the new internal datatype pg_lsn instead of
> > text.
> >
> > Uhm. Does this crash if you pg_upgrade a server that has 1.1 installed?
> >
> Oops yes you're right., it is obviously broken. Just re-thinking about
> that, dumping this module just to change an argument type does not seem to
> be worth the code complication. Thoughts?

It seem simple to support both, old and new type. page_header() (which
IIRC is the only thing returning an LSN) likely uses
get_call_result_type(). So you can just check what it's
expecting. Either support both types or error out if it's the old
extension version.

Greetings,

Andres Freund

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

Andres Freund escribió:
> On 2014-02-24 17:53:31 +0900, Michael Paquier wrote:
> > On Sun, Feb 23, 2014 at 6:09 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:
> >
> > > Michael Paquier escribió:
> > > > Hi all,
> > > >
> > > > Please find attached a patch to dump pageinspect to 1.2. This
> > > > simply changes page_header to use the new internal datatype
> > > > pg_lsn instead of text.
> > >
> > > Uhm. Does this crash if you pg_upgrade a server that has 1.1
> > > installed?
> > >
> > Oops yes you're right., it is obviously broken. Just re-thinking
> > about that, dumping this module just to change an argument type does
> > not seem to be worth the code complication. Thoughts?
>
> It seem simple to support both, old and new type. page_header() (which
> IIRC is the only thing returning an LSN) likely uses
> get_call_result_type(). So you can just check what it's
> expecting. Either support both types or error out if it's the old
> extension version.

Yeah, erroring out seems good enough. Particularly if you add a hint
saying "please upgrade the extension".

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

While we're messing with this, I wonder if there's any way to have
infomask and infomask2 displayed in hex format rather than plain int
without having to specify that in every query. I'm not well known for
being able to do such conversions off the top of my head ...

(Not that it's this patch' responsibility to do that.)

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Feb 24, 2014 at 11:34 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
>
> Andres Freund escribió:
> > On 2014-02-24 17:53:31 +0900, Michael Paquier wrote:
> > > On Sun, Feb 23, 2014 at 6:09 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:
> > >
> > > > Michael Paquier escribió:
> > > > > Hi all,
> > > > >
> > > > > Please find attached a patch to dump pageinspect to 1.2. This
> > > > > simply changes page_header to use the new internal datatype
> > > > > pg_lsn instead of text.
> > > >
> > > > Uhm. Does this crash if you pg_upgrade a server that has 1.1
> > > > installed?
> > > >
> > > Oops yes you're right., it is obviously broken. Just re-thinking
> > > about that, dumping this module just to change an argument type does
> > > not seem to be worth the code complication. Thoughts?
> >
> > It seem simple to support both, old and new type. page_header() (which
> > IIRC is the only thing returning an LSN) likely uses
> > get_call_result_type(). So you can just check what it's
> > expecting. Either support both types or error out if it's the old
> > extension version.
>
> Yeah, erroring out seems good enough. Particularly if you add a hint
> saying "please upgrade the extension".
Done this way by checking the type OID of TupleDesc returned by
get_call_result_type. Supporting both formats just adds complexity for
no real gain.
--
Michael

On Tue, Feb 25, 2014 at 10:02 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> While we're messing with this, I wonder if there's any way to have
> infomask and infomask2 displayed in hex format rather than plain int
> without having to specify that in every query. I'm not well known for
> being able to do such conversions off the top of my head ...
Something like calling DirectFunctionCall1 with to_hex and return the
infomask fields as text values instead of integers? This looks
straight-forward.

> (Not that it's this patch' responsibility to do that.)
Definitely a different patch.

Don't you think that this would break existing applications? I see
more flexibility to keep them as they are now, as integers, users can
always tune their queries to do this post-processing with to_hex for
them as they've (always?) been doing.
--
Michael

On 02/25/2014 06:52 AM, Michael Paquier wrote:
> On Tue, Feb 25, 2014 at 10:02 AM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
>> (Not that it's this patch' responsibility to do that.)
> Definitely a different patch.
>
> Don't you think that this would break existing applications?

I hope there are no applications relying on pageinspect. It's a very
low-level tool. Or if someone has written a nice GUI around it, I'd like
to hear about it so that I can start using it :-).

> I see more flexibility to keep them as they are now, as integers,
> users can always tune their queries to do this post-processing with
> to_hex for them as they've (always?) been doing.

Agreed. With an int4, you can more easily check for a particular bit etc.

- Heikki

On 25 Feb 2014 08:54, "Heikki Linnakangas" <hlinnakangas(at)vmware(dot)com> wrote:
>
>
> I hope there are no applications relying on pageinspect. It's a very
low-level tool. Or if someone has written a nice GUI around it, I'd like to
hear about it so that I can start using it :-).
>
>
>> I see more flexibility to keep them as they are now, as integers,
>> users can always tune their queries to do this post-processing with
>> to_hex for them as they've (always?) been doing.
>
>
> Agreed. With an int4, you can more easily check for a particular bit etc.

What about making it a bit() column?

What I would love to see is a function added to page inspect that takes
these two fields and returns a list of symbolic names or perhaps a tuple of
booleans.

On Mon, Feb 24, 2014 at 9:34 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Yeah, erroring out seems good enough. Particularly if you add a hint
> saying "please upgrade the extension".

In past instances where this has come up, we have actually made the
.so backward-compatible. See pg_stat_statements in particular. I'd
prefer to keep to that precedent here.

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

Robert Haas escribió:
> On Mon, Feb 24, 2014 at 9:34 AM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
> > Yeah, erroring out seems good enough. Particularly if you add a hint
> > saying "please upgrade the extension".
>
> In past instances where this has come up, we have actually made the
> .so backward-compatible. See pg_stat_statements in particular. I'd
> prefer to keep to that precedent here.

But pg_stat_statement is a user tool which is expected to have lots of
use, and backwards compatibility concerns because of people who might
have written tools on top of it. Not so with pageinspect. I don't
think we need to put in the same amount of effort. (Even though,
really, it's probably not all that difficult to support both cases. I
just don't see the point.)

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Feb 26, 2014 at 5:45 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Robert Haas escribió:
>> On Mon, Feb 24, 2014 at 9:34 AM, Alvaro Herrera
>> <alvherre(at)2ndquadrant(dot)com> wrote:
>> > Yeah, erroring out seems good enough. Particularly if you add a hint
>> > saying "please upgrade the extension".
>>
>> In past instances where this has come up, we have actually made the
>> .so backward-compatible. See pg_stat_statements in particular. I'd
>> prefer to keep to that precedent here.
>
> But pg_stat_statement is a user tool which is expected to have lots of
> use, and backwards compatibility concerns because of people who might
> have written tools on top of it. Not so with pageinspect. I don't
> think we need to put in the same amount of effort. (Even though,
> really, it's probably not all that difficult to support both cases. I
> just don't see the point.)
Actually a little bit of hacking I noticed that supporting both is as
complicated as supporting only pg_lsn... Here is for example how I can
get page_header to work across versions:
- snprintf(lsnchar, sizeof(lsnchar), "%X/%X",
- (uint32) (lsn >> 32), (uint32) lsn);

- values[0] = CStringGetTextDatum(lsnchar);
+ /*
+ * Do some version-related checks. pageinspect >= 1.2 uses pg_lsn
+ * instead of text when using this function for the LSN field.
+ */
+ if (tupdesc->attrs[0]->atttypid == TEXTOID)
+ {
+ char lsnchar[64];
+ snprintf(lsnchar, sizeof(lsnchar), "%X/%X",
+ (uint32) (lsn >> 32), (uint32) lsn);
+ values[0] = CStringGetTextDatum(lsnchar);
+ }
+ else
+ values[0] = LSNGetDatum(lsn);
In this case an upgraded 9.4 server using pageinspect 1.1 or older
simply goes through the text datatype path... You can find that in the
patch attached.
Regards,
--
Michael

On Thu, Feb 27, 2014 at 8:05 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Feb 26, 2014 at 5:45 AM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
>> Robert Haas escribió:
>>> On Mon, Feb 24, 2014 at 9:34 AM, Alvaro Herrera
>>> <alvherre(at)2ndquadrant(dot)com> wrote:
>>> > Yeah, erroring out seems good enough. Particularly if you add a hint
>>> > saying "please upgrade the extension".
>>>
>>> In past instances where this has come up, we have actually made the
>>> .so backward-compatible. See pg_stat_statements in particular. I'd
>>> prefer to keep to that precedent here.
>>
>> But pg_stat_statement is a user tool which is expected to have lots of
>> use, and backwards compatibility concerns because of people who might
>> have written tools on top of it. Not so with pageinspect. I don't
>> think we need to put in the same amount of effort. (Even though,
>> really, it's probably not all that difficult to support both cases. I
>> just don't see the point.)
> Actually a little bit of hacking I noticed that supporting both is as
> complicated as supporting only pg_lsn... Here is for example how I can
> get page_header to work across versions:
> - snprintf(lsnchar, sizeof(lsnchar), "%X/%X",
> - (uint32) (lsn >> 32), (uint32) lsn);
>
> - values[0] = CStringGetTextDatum(lsnchar);
> + /*
> + * Do some version-related checks. pageinspect >= 1.2 uses pg_lsn
> + * instead of text when using this function for the LSN field.
> + */
> + if (tupdesc->attrs[0]->atttypid == TEXTOID)
> + {
> + char lsnchar[64];
> + snprintf(lsnchar, sizeof(lsnchar), "%X/%X",
> + (uint32) (lsn >> 32), (uint32) lsn);
> + values[0] = CStringGetTextDatum(lsnchar);
> + }
> + else
> + values[0] = LSNGetDatum(lsn);
> In this case an upgraded 9.4 server using pageinspect 1.1 or older
> simply goes through the text datatype path... You can find that in the
> patch attached.

Thanks, committed.

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