Re: [HACKERS] Dump pageinspect to 1.2: page_header with pg_lsn datatype

2014-03-03 Thread Robert Haas
On Thu, Feb 27, 2014 at 8:05 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Wed, Feb 26, 2014 at 5:45 AM, Alvaro Herrera
 alvhe...@2ndquadrant.com wrote:
 Robert Haas escribió:
 On Mon, Feb 24, 2014 at 9:34 AM, Alvaro Herrera
 alvhe...@2ndquadrant.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)
 +   {
 +   charlsnchar[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


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


Re: [HACKERS] Dump pageinspect to 1.2: page_header with pg_lsn datatype

2014-02-27 Thread Michael Paquier
On Wed, Feb 26, 2014 at 5:45 AM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 Robert Haas escribió:
 On Mon, Feb 24, 2014 at 9:34 AM, Alvaro Herrera
 alvhe...@2ndquadrant.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)
+   {
+   charlsnchar[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
diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index 0e267eb..ee78cb2 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -4,8 +4,8 @@ MODULE_big	= pageinspect
 OBJS		= rawpage.o heapfuncs.o btreefuncs.o fsmfuncs.o
 
 EXTENSION = pageinspect
-DATA = pageinspect--1.1.sql pageinspect--1.0--1.1.sql \
-	pageinspect--unpackaged--1.0.sql
+DATA = pageinspect--1.2.sql pageinspect--1.0--1.1.sql \
+	pageinspect--1.1--1.2.sql pageinspect--unpackaged--1.0.sql
 
 ifdef USE_PGXS
 PG_CONFIG = pg_config
diff --git a/contrib/pageinspect/pageinspect--1.1--1.2.sql b/contrib/pageinspect/pageinspect--1.1--1.2.sql
new file mode 100644
index 000..5e23ca4
--- /dev/null
+++ b/contrib/pageinspect/pageinspect--1.1--1.2.sql
@@ -0,0 +1,18 @@
+/* contrib/pageinspect/pageinspect--1.1--1.2.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use ALTER EXTENSION pageinspect UPDATE TO 1.2 to load this file. \quit
+
+DROP FUNCTION page_header(bytea);
+CREATE FUNCTION page_header(IN page bytea,
+OUT lsn pg_lsn,
+OUT checksum smallint,
+OUT flags smallint,
+OUT lower smallint,
+OUT upper smallint,
+OUT special smallint,
+OUT pagesize smallint,
+OUT version smallint,
+OUT prune_xid xid)
+AS 'MODULE_PATHNAME', 'page_header'
+LANGUAGE C STRICT;
diff --git a/contrib/pageinspect/pageinspect--1.1.sql b/contrib/pageinspect/pageinspect--1.1.sql
deleted file mode 100644
index 22a47d5..000
--- a/contrib/pageinspect/pageinspect--1.1.sql
+++ /dev/null
@@ -1,107 +0,0 @@
-/* contrib/pageinspect/pageinspect--1.1.sql */
-
--- complain if script is sourced in psql, rather than via CREATE EXTENSION
-\echo Use CREATE EXTENSION pageinspect to load this file. \quit
-
---
--- get_raw_page()
---
-CREATE FUNCTION get_raw_page(text, int4)
-RETURNS bytea
-AS 'MODULE_PATHNAME', 'get_raw_page'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION get_raw_page(text, text, int4)
-RETURNS bytea
-AS 'MODULE_PATHNAME', 'get_raw_page_fork'
-LANGUAGE C STRICT;
-
---
--- page_header()
---
-CREATE FUNCTION page_header(IN page bytea,
-OUT lsn text,
-OUT checksum smallint,
-OUT flags smallint,
-OUT lower smallint,
-OUT upper smallint,
-OUT special smallint,
-OUT pagesize smallint,
-OUT version smallint,
-OUT prune_xid xid)
-AS 'MODULE_PATHNAME', 'page_header'
-LANGUAGE C STRICT;
-
---
--- heap_page_items()
---
-CREATE FUNCTION heap_page_items(IN page bytea,
-OUT lp smallint,
-OUT lp_off smallint,
-OUT lp_flags smallint,
-OUT lp_len smallint,
-OUT t_xmin xid,
-OUT t_xmax xid,
-OUT t_field3 int4,
-OUT t_ctid tid,
-OUT t_infomask2 integer,
-OUT t_infomask integer,
-OUT t_hoff smallint,
-OUT t_bits text,
-OUT t_oid oid)
-RETURNS SETOF record
-AS 'MODULE_PATHNAME', 'heap_page_items'
-LANGUAGE C STRICT;
-
---
--- bt_metap()
---
-CREATE FUNCTION bt_metap(IN relname text,
-OUT magic int4,
-OUT version int4,
-OUT root int4,
-OUT level int4,
-OUT fastroot int4,
-OUT fastlevel int4)
-AS 'MODULE_PATHNAME', 

Re: [HACKERS] Dump pageinspect to 1.2: page_header with pg_lsn datatype

2014-02-25 Thread Heikki Linnakangas

On 02/25/2014 06:52 AM, Michael Paquier wrote:

On Tue, Feb 25, 2014 at 10:02 AM, Alvaro Herrera
alvhe...@2ndquadrant.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


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


Re: [HACKERS] Dump pageinspect to 1.2: page_header with pg_lsn datatype

2014-02-25 Thread Greg Stark
On 25 Feb 2014 08:54, Heikki Linnakangas hlinnakan...@vmware.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.


Re: [HACKERS] Dump pageinspect to 1.2: page_header with pg_lsn datatype

2014-02-25 Thread Robert Haas
On Mon, Feb 24, 2014 at 9:34 AM, Alvaro Herrera
alvhe...@2ndquadrant.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


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


Re: [HACKERS] Dump pageinspect to 1.2: page_header with pg_lsn datatype

2014-02-25 Thread Alvaro Herrera
Robert Haas escribió:
 On Mon, Feb 24, 2014 at 9:34 AM, Alvaro Herrera
 alvhe...@2ndquadrant.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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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


Re: [HACKERS] Dump pageinspect to 1.2: page_header with pg_lsn datatype

2014-02-24 Thread Michael Paquier
On Sun, Feb 23, 2014 at 6:09 AM, Alvaro Herrera alvhe...@2ndquadrant.comwrote:

 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


Re: [HACKERS] Dump pageinspect to 1.2: page_header with pg_lsn datatype

2014-02-24 Thread Andres Freund
On 2014-02-24 17:53:31 +0900, Michael Paquier wrote:
 On Sun, Feb 23, 2014 at 6:09 AM, Alvaro Herrera 
 alvhe...@2ndquadrant.comwrote:
 
  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


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


Re: [HACKERS] Dump pageinspect to 1.2: page_header with pg_lsn datatype

2014-02-24 Thread Alvaro Herrera
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 
  alvhe...@2ndquadrant.comwrote:
  
   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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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


Re: [HACKERS] Dump pageinspect to 1.2: page_header with pg_lsn datatype

2014-02-24 Thread Alvaro Herrera
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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


Re: [HACKERS] Dump pageinspect to 1.2: page_header with pg_lsn datatype

2014-02-24 Thread Michael Paquier
On Mon, Feb 24, 2014 at 11:34 PM, Alvaro Herrera
alvhe...@2ndquadrant.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 
   alvhe...@2ndquadrant.comwrote:
  
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
diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index 0e267eb..ee78cb2 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -4,8 +4,8 @@ MODULE_big	= pageinspect
 OBJS		= rawpage.o heapfuncs.o btreefuncs.o fsmfuncs.o
 
 EXTENSION = pageinspect
-DATA = pageinspect--1.1.sql pageinspect--1.0--1.1.sql \
-	pageinspect--unpackaged--1.0.sql
+DATA = pageinspect--1.2.sql pageinspect--1.0--1.1.sql \
+	pageinspect--1.1--1.2.sql pageinspect--unpackaged--1.0.sql
 
 ifdef USE_PGXS
 PG_CONFIG = pg_config
diff --git a/contrib/pageinspect/pageinspect--1.1--1.2.sql b/contrib/pageinspect/pageinspect--1.1--1.2.sql
new file mode 100644
index 000..5e23ca4
--- /dev/null
+++ b/contrib/pageinspect/pageinspect--1.1--1.2.sql
@@ -0,0 +1,18 @@
+/* contrib/pageinspect/pageinspect--1.1--1.2.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use ALTER EXTENSION pageinspect UPDATE TO 1.2 to load this file. \quit
+
+DROP FUNCTION page_header(bytea);
+CREATE FUNCTION page_header(IN page bytea,
+OUT lsn pg_lsn,
+OUT checksum smallint,
+OUT flags smallint,
+OUT lower smallint,
+OUT upper smallint,
+OUT special smallint,
+OUT pagesize smallint,
+OUT version smallint,
+OUT prune_xid xid)
+AS 'MODULE_PATHNAME', 'page_header'
+LANGUAGE C STRICT;
diff --git a/contrib/pageinspect/pageinspect--1.1.sql b/contrib/pageinspect/pageinspect--1.1.sql
deleted file mode 100644
index 22a47d5..000
--- a/contrib/pageinspect/pageinspect--1.1.sql
+++ /dev/null
@@ -1,107 +0,0 @@
-/* contrib/pageinspect/pageinspect--1.1.sql */
-
--- complain if script is sourced in psql, rather than via CREATE EXTENSION
-\echo Use CREATE EXTENSION pageinspect to load this file. \quit
-
---
--- get_raw_page()
---
-CREATE FUNCTION get_raw_page(text, int4)
-RETURNS bytea
-AS 'MODULE_PATHNAME', 'get_raw_page'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION get_raw_page(text, text, int4)
-RETURNS bytea
-AS 'MODULE_PATHNAME', 'get_raw_page_fork'
-LANGUAGE C STRICT;
-
---
--- page_header()
---
-CREATE FUNCTION page_header(IN page bytea,
-OUT lsn text,
-OUT checksum smallint,
-OUT flags smallint,
-OUT lower smallint,
-OUT upper smallint,
-OUT special smallint,
-OUT pagesize smallint,
-OUT version smallint,
-OUT prune_xid xid)
-AS 'MODULE_PATHNAME', 'page_header'
-LANGUAGE C STRICT;
-
---
--- heap_page_items()
---
-CREATE FUNCTION heap_page_items(IN page bytea,
-OUT lp smallint,
-OUT lp_off smallint,
-OUT lp_flags smallint,
-OUT lp_len smallint,
-OUT t_xmin xid,
-OUT t_xmax xid,
-OUT t_field3 int4,
-OUT t_ctid tid,
-OUT t_infomask2 integer,
-OUT t_infomask integer,
-OUT t_hoff smallint,
-OUT t_bits text,
-OUT t_oid oid)
-RETURNS SETOF record
-AS 'MODULE_PATHNAME', 'heap_page_items'
-LANGUAGE C STRICT;
-
---
--- bt_metap()
---
-CREATE FUNCTION bt_metap(IN relname text,
-OUT magic int4,
-OUT version int4,
-OUT root int4,
-OUT level int4,
-OUT fastroot int4,
-OUT fastlevel int4)
-AS 'MODULE_PATHNAME', 'bt_metap'
-LANGUAGE C STRICT;
-
---
--- bt_page_stats()
---
-CREATE FUNCTION bt_page_stats(IN relname text, IN blkno int4,
-OUT blkno int4,
-OUT type char,
-OUT live_items int4,
-OUT dead_items int4,
-OUT avg_item_size int4,
-OUT page_size int4,
-OUT free_size int4,
-OUT btpo_prev int4,
-OUT btpo_next int4,
-OUT btpo int4,
-OUT btpo_flags int4)
-AS 'MODULE_PATHNAME', 'bt_page_stats'
-LANGUAGE C STRICT;
-
---
--- bt_page_items()
---
-CREATE FUNCTION bt_page_items(IN relname text, IN blkno int4,
-OUT itemoffset smallint,
-OUT ctid tid,
-

Re: [HACKERS] Dump pageinspect to 1.2: page_header with pg_lsn datatype

2014-02-24 Thread Michael Paquier
On Tue, Feb 25, 2014 at 10:02 AM, Alvaro Herrera
alvhe...@2ndquadrant.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


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


Re: [HACKERS] Dump pageinspect to 1.2: page_header with pg_lsn datatype

2014-02-22 Thread Alvaro Herrera
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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