Re: [HACKERS] should we add a XLogRecPtr/LSN SQL type?

2014-02-24 Thread Robert Haas
On Wed, Feb 19, 2014 at 8:06 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Thu, Feb 20, 2014 at 9:43 AM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 On Thu, Feb 20, 2014 at 2:47 AM, Robert Haas robertmh...@gmail.com wrote:
 On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 On Thu, Feb 6, 2014 at 3:48 AM, Peter Eisentraut pete...@gmx.net wrote:
 On 2/5/14, 1:31 PM, Robert Haas wrote:
 On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut pete...@gmx.net wrote:
 Perhaps this type should be called pglsn, since it's an
 implementation-specific detail and not a universal concept like int,
 point, or uuid.

 If we're going to do that, I suggest pg_lsn rather than pglsn.  We
 already have pg_node_tree, so using underscores for separation would
 be more consistent.

 Yes, that's a good precedent in multiple ways.
 Here are updated patches to use pg_lsn instead of pglsn...

 OK, so I think this stuff is all committed now, with assorted changes.
  Thanks for your work on this.
 Thanks!
 Oops, it looks like I am coming after the battle (time difference does
 not help). I'll be more careful to test such patches on 32b platforms
 as well in the future.
 After re-reading the code, I found two incorrect comments in the new
 regression tests. Patch fixing them is attached.

Thanks, committed.  But I left out the whitespace change you included.

-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-24 Thread Robert Haas
On Wed, Feb 19, 2014 at 8:27 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Thu, Feb 20, 2014 at 8:55 AM, Andres Freund and...@2ndquadrant.com wrote:
 On 2014-02-19 12:47:40 -0500, Robert Haas wrote:
 On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
  Yes, that's a good precedent in multiple ways.
  Here are updated patches to use pg_lsn instead of pglsn...

 OK, so I think this stuff is all committed now, with assorted changes.
  Thanks for your work on this.

 cool, thanks you two.

 I wonder if pg_stat_replication shouldn't be updated to use it as well?
 SELECT * FROM pg_attribute WHERE attname ~ '(location|lsn)'; only shows
 that as names that are possible candidates for conversion.
 I was sure to have forgotten some views or functions in the previous
 patch... Please find attached a patch making pg_stat_replication use
 pg_lsn instead of the existing text fields.
 Regards,

Committed.  Do we want to do anything about pageinspect?

-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-24 Thread Michael Paquier
On Tue, Feb 25, 2014 at 12:41 AM, Robert Haas robertmh...@gmail.com wrote:

 On Wed, Feb 19, 2014 at 8:27 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
  On Thu, Feb 20, 2014 at 8:55 AM, Andres Freund and...@2ndquadrant.com
 wrote:
  On 2014-02-19 12:47:40 -0500, Robert Haas wrote:
  On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier
  michael.paqu...@gmail.com wrote:
   Yes, that's a good precedent in multiple ways.
   Here are updated patches to use pg_lsn instead of pglsn...
 
  OK, so I think this stuff is all committed now, with assorted changes.
   Thanks for your work on this.
 
  cool, thanks you two.
 
  I wonder if pg_stat_replication shouldn't be updated to use it as well?
  SELECT * FROM pg_attribute WHERE attname ~ '(location|lsn)'; only shows
  that as names that are possible candidates for conversion.
  I was sure to have forgotten some views or functions in the previous
  patch... Please find attached a patch making pg_stat_replication use
  pg_lsn instead of the existing text fields.
  Regards,

 Committed.  Do we want to do anything about pageinspect?

Thanks. We're dealing with that on another thread, I'll send an updated
patch there.
-- 
Michael


Re: [HACKERS] should we add a XLogRecPtr/LSN SQL type?

2014-02-19 Thread Robert Haas
On Fri, Feb 14, 2014 at 9:32 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Thu, Feb 6, 2014 at 11:26 AM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 Here are updated patches to use pg_lsn instead of pglsn...
 Should I register this patch somewhere to avoid having it lost in the void?
 Regards,

Well, I committed this, but the buildfarm's deeply unhappy with it.
Apparently the use of GET_8_BYTES() and SET_8_BYTES() is no good on
some platforms...  and I'm not sure what to do about that, right
off-hand.

-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-19 Thread Andres Freund
On 2014-02-19 09:24:03 -0500, Robert Haas wrote:
 On Fri, Feb 14, 2014 at 9:32 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
  On Thu, Feb 6, 2014 at 11:26 AM, Michael Paquier
  michael.paqu...@gmail.com wrote:
  Here are updated patches to use pg_lsn instead of pglsn...
  Should I register this patch somewhere to avoid having it lost in the void?
  Regards,
 
 Well, I committed this, but the buildfarm's deeply unhappy with it.
 Apparently the use of GET_8_BYTES() and SET_8_BYTES() is no good on
 some platforms...  and I'm not sure what to do about that, right
 off-hand.

The relevant bit probably is:

pg_lsn.c: In function 'pg_lsn_out':
pg_lsn.c:59:2: warning: implicit declaration of function 'GET_8_BYTES' 
[-Wimplicit-function-declaration]

GET_8_BYTES only exists for 64bit systems.

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] should we add a XLogRecPtr/LSN SQL type?

2014-02-19 Thread Robert Haas
On Wed, Feb 19, 2014 at 9:30 AM, Andres Freund and...@2ndquadrant.com wrote:
 On 2014-02-19 09:24:03 -0500, Robert Haas wrote:
 On Fri, Feb 14, 2014 at 9:32 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
  On Thu, Feb 6, 2014 at 11:26 AM, Michael Paquier
  michael.paqu...@gmail.com wrote:
  Here are updated patches to use pg_lsn instead of pglsn...
  Should I register this patch somewhere to avoid having it lost in the void?
  Regards,

 Well, I committed this, but the buildfarm's deeply unhappy with it.
 Apparently the use of GET_8_BYTES() and SET_8_BYTES() is no good on
 some platforms...  and I'm not sure what to do about that, right
 off-hand.

 The relevant bit probably is:

 pg_lsn.c: In function 'pg_lsn_out':
 pg_lsn.c:59:2: warning: implicit declaration of function 'GET_8_BYTES' 
 [-Wimplicit-function-declaration]

 GET_8_BYTES only exists for 64bit systems.

Right, I got that far.  So it looks like float8, int8, timestamp,
timestamptz, and money all have behavior contingent on
USE_FLOAT8_BYVAL, making that symbol a misnomer in every way.  But
since we've already marched pretty far down that path I suppose we
should keep marching.

-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-19 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Wed, Feb 19, 2014 at 9:30 AM, Andres Freund and...@2ndquadrant.com wrote:
 GET_8_BYTES only exists for 64bit systems.

 Right, I got that far.  So it looks like float8, int8, timestamp,
 timestamptz, and money all have behavior contingent on
 USE_FLOAT8_BYVAL, making that symbol a misnomer in every way.  But
 since we've already marched pretty far down that path I suppose we
 should keep marching.

You need somebody to help you with getting that working on 32-bit
platforms?  Because it needs to get fixed, or reverted, PDQ.

regards, tom lane


-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-19 Thread Robert Haas
On Wed, Feb 19, 2014 at 10:03 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Wed, Feb 19, 2014 at 9:30 AM, Andres Freund and...@2ndquadrant.com 
 wrote:
 GET_8_BYTES only exists for 64bit systems.

 Right, I got that far.  So it looks like float8, int8, timestamp,
 timestamptz, and money all have behavior contingent on
 USE_FLOAT8_BYVAL, making that symbol a misnomer in every way.  But
 since we've already marched pretty far down that path I suppose we
 should keep marching.

 You need somebody to help you with getting that working on 32-bit
 platforms?  Because it needs to get fixed, or reverted, PDQ.

Hopefully the commit I just pushed will fix it.  It now works on my
machine with and without --disable-float8-byval.

-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-19 Thread Greg Stark
On Wed, Feb 19, 2014 at 3:11 PM, Robert Haas robertmh...@gmail.com wrote:
 Hopefully the commit I just pushed will fix it.  It now works on my
 machine with and without --disable-float8-byval.

It builds and passes here on 32bits


-- 
greg


-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-19 Thread Robert Haas
On Wed, Feb 19, 2014 at 11:03 AM, Greg Stark st...@mit.edu wrote:
 On Wed, Feb 19, 2014 at 3:11 PM, Robert Haas robertmh...@gmail.com wrote:
 Hopefully the commit I just pushed will fix it.  It now works on my
 machine with and without --disable-float8-byval.

 It builds and passes here on 32bits

The buildfarm seems happy so far, too.

-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-19 Thread Robert Haas
On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Thu, Feb 6, 2014 at 3:48 AM, Peter Eisentraut pete...@gmx.net wrote:
 On 2/5/14, 1:31 PM, Robert Haas wrote:
 On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut pete...@gmx.net wrote:
 Perhaps this type should be called pglsn, since it's an
 implementation-specific detail and not a universal concept like int,
 point, or uuid.

 If we're going to do that, I suggest pg_lsn rather than pglsn.  We
 already have pg_node_tree, so using underscores for separation would
 be more consistent.

 Yes, that's a good precedent in multiple ways.
 Here are updated patches to use pg_lsn instead of pglsn...

OK, so I think this stuff is all committed now, with assorted changes.
 Thanks for your work on this.

-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-19 Thread Andres Freund
On 2014-02-19 12:47:40 -0500, Robert Haas wrote:
 On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
  Yes, that's a good precedent in multiple ways.
  Here are updated patches to use pg_lsn instead of pglsn...
 
 OK, so I think this stuff is all committed now, with assorted changes.
  Thanks for your work on this.

cool, thanks you two.

I wonder if pg_stat_replication shouldn't be updated to use it as well?
SELECT * FROM pg_attribute WHERE attname ~ '(location|lsn)'; only shows
that as names that are possible candidates for conversion.

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] should we add a XLogRecPtr/LSN SQL type?

2014-02-19 Thread Michael Paquier
On Thu, Feb 20, 2014 at 2:47 AM, Robert Haas robertmh...@gmail.com wrote:
 On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 On Thu, Feb 6, 2014 at 3:48 AM, Peter Eisentraut pete...@gmx.net wrote:
 On 2/5/14, 1:31 PM, Robert Haas wrote:
 On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut pete...@gmx.net wrote:
 Perhaps this type should be called pglsn, since it's an
 implementation-specific detail and not a universal concept like int,
 point, or uuid.

 If we're going to do that, I suggest pg_lsn rather than pglsn.  We
 already have pg_node_tree, so using underscores for separation would
 be more consistent.

 Yes, that's a good precedent in multiple ways.
 Here are updated patches to use pg_lsn instead of pglsn...

 OK, so I think this stuff is all committed now, with assorted changes.
  Thanks for your work on this.
Thanks!
Oops, it looks like I am coming after the battle (time difference does
not help). I'll be more careful to test such patches on 32b platforms
as well in the future.
Regards,
-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-19 Thread Michael Paquier
On Thu, Feb 20, 2014 at 9:43 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Thu, Feb 20, 2014 at 2:47 AM, Robert Haas robertmh...@gmail.com wrote:
 On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 On Thu, Feb 6, 2014 at 3:48 AM, Peter Eisentraut pete...@gmx.net wrote:
 On 2/5/14, 1:31 PM, Robert Haas wrote:
 On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut pete...@gmx.net wrote:
 Perhaps this type should be called pglsn, since it's an
 implementation-specific detail and not a universal concept like int,
 point, or uuid.

 If we're going to do that, I suggest pg_lsn rather than pglsn.  We
 already have pg_node_tree, so using underscores for separation would
 be more consistent.

 Yes, that's a good precedent in multiple ways.
 Here are updated patches to use pg_lsn instead of pglsn...

 OK, so I think this stuff is all committed now, with assorted changes.
  Thanks for your work on this.
 Thanks!
 Oops, it looks like I am coming after the battle (time difference does
 not help). I'll be more careful to test such patches on 32b platforms
 as well in the future.
After re-reading the code, I found two incorrect comments in the new
regression tests. Patch fixing them is attached.
Thanks,
-- 
Michael
diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index e2b528a..fae12e1 100644
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -19,7 +19,7 @@
 #include utils/pg_lsn.h
 
 #define MAXPG_LSNLEN			17
-#define MAXPG_LSNCOMPONENT	8
+#define MAXPG_LSNCOMPONENT		8
 
 /*--
  * Formatting and conversion routines.
diff --git a/src/test/regress/expected/pg_lsn.out b/src/test/regress/expected/pg_lsn.out
index 01d2983..504768c 100644
--- a/src/test/regress/expected/pg_lsn.out
+++ b/src/test/regress/expected/pg_lsn.out
@@ -52,13 +52,13 @@ SELECT '0/16AE7F8'  pg_lsn '0/16AE7F7';
  t
 (1 row)
 
-SELECT '0/16AE7F7'::pg_lsn - '0/16AE7F8'::pg_lsn; -- No negative results
+SELECT '0/16AE7F7'::pg_lsn - '0/16AE7F8'::pg_lsn;
  ?column? 
 --
-1
 (1 row)
 
-SELECT '0/16AE7F8'::pg_lsn - '0/16AE7F7'::pg_lsn; -- correct
+SELECT '0/16AE7F8'::pg_lsn - '0/16AE7F7'::pg_lsn;
  ?column? 
 --
 1
diff --git a/src/test/regress/sql/pg_lsn.sql b/src/test/regress/sql/pg_lsn.sql
index dddafb3..1634d37 100644
--- a/src/test/regress/sql/pg_lsn.sql
+++ b/src/test/regress/sql/pg_lsn.sql
@@ -21,5 +21,5 @@ SELECT '0/16AE7F8' = '0/16AE7F8'::pg_lsn;
 SELECT '0/16AE7F8'::pg_lsn != '0/16AE7F7';
 SELECT '0/16AE7F7'  '0/16AE7F8'::pg_lsn;
 SELECT '0/16AE7F8'  pg_lsn '0/16AE7F7';
-SELECT '0/16AE7F7'::pg_lsn - '0/16AE7F8'::pg_lsn; -- No negative results
-SELECT '0/16AE7F8'::pg_lsn - '0/16AE7F7'::pg_lsn; -- correct
+SELECT '0/16AE7F7'::pg_lsn - '0/16AE7F8'::pg_lsn;
+SELECT '0/16AE7F8'::pg_lsn - '0/16AE7F7'::pg_lsn;

-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-19 Thread Michael Paquier
On Thu, Feb 20, 2014 at 8:55 AM, Andres Freund and...@2ndquadrant.com wrote:
 On 2014-02-19 12:47:40 -0500, Robert Haas wrote:
 On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
  Yes, that's a good precedent in multiple ways.
  Here are updated patches to use pg_lsn instead of pglsn...

 OK, so I think this stuff is all committed now, with assorted changes.
  Thanks for your work on this.

 cool, thanks you two.

 I wonder if pg_stat_replication shouldn't be updated to use it as well?
 SELECT * FROM pg_attribute WHERE attname ~ '(location|lsn)'; only shows
 that as names that are possible candidates for conversion.
I was sure to have forgotten some views or functions in the previous
patch... Please find attached a patch making pg_stat_replication use
pg_lsn instead of the existing text fields.
Regards,
-- 
Michael
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index a37e6b6..370857a 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1490,24 +1490,24 @@ postgres: replaceableuser/ replaceabledatabase/ replaceablehost/ re
 /row
 row
  entrystructfieldsent_location//entry
- entrytypetext//entry
+ entrytypepg_lsn//entry
  entryLast transaction log position sent on this connection/entry
 /row
 row
  entrystructfieldwrite_location//entry
- entrytypetext//entry
+ entrytypepg_lsn//entry
  entryLast transaction log position written to disk by this standby
   server/entry
 /row
 row
  entrystructfieldflush_location//entry
- entrytypetext//entry
+ entrytypepg_lsn//entry
  entryLast transaction log position flushed to disk by this standby
   server/entry
 /row
 row
  entrystructfieldreplay_location//entry
- entrytypetext//entry
+ entrytypepg_lsn//entry
  entryLast transaction log position replayed into the database on this
   standby server/entry
 /row
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 06b22e2..048367a 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -67,6 +67,7 @@
 #include utils/builtins.h
 #include utils/guc.h
 #include utils/memutils.h
+#include utils/pg_lsn.h
 #include utils/ps_status.h
 #include utils/resowner.h
 #include utils/timeout.h
@@ -2137,7 +2138,6 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS)
 	{
 		/* use volatile pointer to prevent code rearrangement */
 		volatile WalSnd *walsnd = WalSndCtl-walsnds[i];
-		char		location[MAXFNAMELEN];
 		XLogRecPtr	sentPtr;
 		XLogRecPtr	write;
 		XLogRecPtr	flush;
@@ -2171,28 +2171,19 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS)
 		else
 		{
 			values[1] = CStringGetTextDatum(WalSndGetStateString(state));
-
-			snprintf(location, sizeof(location), %X/%X,
-	 (uint32) (sentPtr  32), (uint32) sentPtr);
-			values[2] = CStringGetTextDatum(location);
+			values[2] = LSNGetDatum(sentPtr);
 
 			if (write == 0)
 nulls[3] = true;
-			snprintf(location, sizeof(location), %X/%X,
-	 (uint32) (write  32), (uint32) write);
-			values[3] = CStringGetTextDatum(location);
+			values[3] = LSNGetDatum(write);
 
 			if (flush == 0)
 nulls[4] = true;
-			snprintf(location, sizeof(location), %X/%X,
-	 (uint32) (flush  32), (uint32) flush);
-			values[4] = CStringGetTextDatum(location);
+			values[4] = LSNGetDatum(flush);
 
 			if (apply == 0)
 nulls[5] = true;
-			snprintf(location, sizeof(location), %X/%X,
-	 (uint32) (apply  32), (uint32) apply);
-			values[5] = CStringGetTextDatum(location);
+			values[5] = LSNGetDatum(apply);
 
 			values[6] = Int32GetDatum(sync_priority[i]);
 
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 11c1e1a..a2cc19f 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -2634,7 +2634,7 @@ DATA(insert OID = 1936 (  pg_stat_get_backend_idset		PGNSP PGUID 12 1 100 0 0 f
 DESCR(statistics: currently active backend IDs);
 DATA(insert OID = 2022 (  pg_stat_get_activity			PGNSP PGUID 12 1 100 0 0 f f f f f t s 1 0 2249 23 {23,26,23,26,25,25,25,16,1184,1184,1184,1184,869,25,23} {i,o,o,o,o,o,o,o,o,o,o,o,o,o,o} {pid,datid,pid,usesysid,application_name,state,query,waiting,xact_start,query_start,backend_start,state_change,client_addr,client_hostname,client_port} _null_ pg_stat_get_activity _null_ _null_ _null_ ));
 DESCR(statistics: information about currently active backends);
-DATA(insert OID = 3099 (  pg_stat_get_wal_senders	PGNSP PGUID 12 1 10 0 0 f f f f f t s 0 0 2249  {23,25,25,25,25,25,23,25} {o,o,o,o,o,o,o,o} {pid,state,sent_location,write_location,flush_location,replay_location,sync_priority,sync_state} _null_ pg_stat_get_wal_senders _null_ _null_ _null_ ));
+DATA(insert OID = 3099 (  pg_stat_get_wal_senders	PGNSP PGUID 12 1 10 0 0 f f f f f t s 0 0 2249  {23,25,3220,3220,3220,3220,23,25} {o,o,o,o,o,o,o,o} 

Re: [HACKERS] should we add a XLogRecPtr/LSN SQL type?

2014-02-14 Thread Michael Paquier
On Thu, Feb 6, 2014 at 11:26 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
 Here are updated patches to use pg_lsn instead of pglsn...
Should I register this patch somewhere to avoid having it lost in the void?
Regards,
-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-05 Thread Robert Haas
On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut pete...@gmx.net wrote:
 Perhaps this type should be called pglsn, since it's an
 implementation-specific detail and not a universal concept like int,
 point, or uuid.

If we're going to do that, I suggest pg_lsn rather than pglsn.  We
already have pg_node_tree, so using underscores for separation would
be more consistent.

-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-05 Thread Peter Eisentraut
On 2/5/14, 1:31 PM, Robert Haas wrote:
 On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut pete...@gmx.net wrote:
 Perhaps this type should be called pglsn, since it's an
 implementation-specific detail and not a universal concept like int,
 point, or uuid.
 
 If we're going to do that, I suggest pg_lsn rather than pglsn.  We
 already have pg_node_tree, so using underscores for separation would
 be more consistent.

Yes, that's a good precedent in multiple ways.




-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-04 Thread Andres Freund
Hi,

On 2014-02-04 10:23:14 +0900, Michael Paquier wrote:
 On Tue, Feb 4, 2014 at 10:10 AM, Tom Lane t...@sss.pgh.pa.us wrote:
  Michael Paquier michael.paqu...@gmail.com writes:
  Please find attached a patch implementing lsn as a datatype, based on
  the one Robert wrote a couple of years ago.
 
  Patch contains regression tests as well as a bit of documentation.
  Perhaps this is too late for 9.4, so if there are no objections I'll
  simply add this patch to the next commit fest in June for 9.5.
 
  I may have lost count, but aren't a bunch of the affected functions new
  in 9.4?  If so, there's a good argument to be made that we should get
  this in now, rather than waiting and having an API change for those
  functions in 9.5.

Yes, that sounds sensible.

 + /*--
 +  *  Relational operators for LSNs
 +  *-*/

Isn't it just operators? They aren't really relational...


 *** 302,307  extern struct varlena *pg_detoast_datum_packed(struct 
 varlena * datum);
 --- 303,309 
   #define PG_RETURN_CHAR(x)return CharGetDatum(x)
   #define PG_RETURN_BOOL(x)return BoolGetDatum(x)
   #define PG_RETURN_OID(x) return ObjectIdGetDatum(x)
 + #define PG_RETURN_LSN(x) return LogSeqNumGetDatum(x)
   #define PG_RETURN_POINTER(x) return PointerGetDatum(x)
   #define PG_RETURN_CSTRING(x) return CStringGetDatum(x)
   #define PG_RETURN_NAME(x)return NameGetDatum(x)
 *** a/src/include/postgres.h
 --- b/src/include/postgres.h
 ***
 *** 484,489  typedef Datum *DatumPtr;
 --- 484,503 
   #define ObjectIdGetDatum(X) ((Datum) SET_4_BYTES(X))
   
   /*
 +  * DatumGetLogSeqNum
 +  *  Returns log sequence number of a datum.
 +  */
 + 
 + #define DatumGetLogSeqNum(X) ((XLogRecPtr) GET_8_BYTES(X))

I am not a fan of LogSegNum. I think at this point fewer people
understand that than LSN. There's also no reason to invent a third term
for LSNs. We'd have LSN, XLogRecPtr, and LogSeqNum.

 *** a/src/backend/replication/slotfuncs.c
 --- b/src/backend/replication/slotfuncs.c
 ***
 *** 141,148  pg_get_replication_slots(PG_FUNCTION_ARGS)
   boolactive;
   Oid database;
   const char *slot_name;
 - 
 - charrestart_lsn_s[MAXFNAMELEN];
   int i;
   
   SpinLockAcquire(slot-mutex);
 --- 141,146 

Unrelated change.


Looks reasonable on a first look. Thanks!

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] should we add a XLogRecPtr/LSN SQL type?

2014-02-04 Thread Michael Paquier
On Tue, Feb 4, 2014 at 6:15 PM, Andres Freund and...@2ndquadrant.com wrote:

 + /*--
 +  *  Relational operators for LSNs
 +  *-*/

 Isn't it just operators? They aren't really relational...
Operators for LSNs?

 *** 302,307  extern struct varlena *pg_detoast_datum_packed(struct 
 varlena * datum);
 --- 303,309 
   #define PG_RETURN_CHAR(x)return CharGetDatum(x)
   #define PG_RETURN_BOOL(x)return BoolGetDatum(x)
   #define PG_RETURN_OID(x) return ObjectIdGetDatum(x)
 + #define PG_RETURN_LSN(x) return LogSeqNumGetDatum(x)
   #define PG_RETURN_POINTER(x) return PointerGetDatum(x)
   #define PG_RETURN_CSTRING(x) return CStringGetDatum(x)
   #define PG_RETURN_NAME(x)return NameGetDatum(x)
 *** a/src/include/postgres.h
 --- b/src/include/postgres.h
 ***
 *** 484,489  typedef Datum *DatumPtr;
 --- 484,503 
   #define ObjectIdGetDatum(X) ((Datum) SET_4_BYTES(X))

   /*
 +  * DatumGetLogSeqNum
 +  *  Returns log sequence number of a datum.
 +  */
 +
 + #define DatumGetLogSeqNum(X) ((XLogRecPtr) GET_8_BYTES(X))

 I am not a fan of LogSegNum. I think at this point fewer people
 understand that than LSN. There's also no reason to invent a third term
 for LSNs. We'd have LSN, XLogRecPtr, and LogSeqNum.
So let's go with DatumGetLSN and LSNGetDatum instead...

 *** a/src/backend/replication/slotfuncs.c
 --- b/src/backend/replication/slotfuncs.c
 ***
 *** 141,148  pg_get_replication_slots(PG_FUNCTION_ARGS)
   boolactive;
   Oid database;
   const char *slot_name;
 -
 - charrestart_lsn_s[MAXFNAMELEN];
   int i;

   SpinLockAcquire(slot-mutex);
 --- 141,146 

 Unrelated change.
Funnily, the patch attached in my previous mail did not include all
the diffs, it is an error with filterdiff that I use to generate
context diff patches... My original branch includes the following
diffs as well in slotfuncs.c for the second patch:
diff --git a/src/backend/replication/slotfuncs.c
b/src/backend/replication/slotfuncs.c
index 98a860e..68ecdcd 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -141,8 +141,6 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
boolactive;
Oid database;
const char *slot_name;
-
-   charrestart_lsn_s[MAXFNAMELEN];
int i;

SpinLockAcquire(slot-mutex);
@@ -164,9 +162,6 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)

memset(nulls, 0, sizeof(nulls));

-   snprintf(restart_lsn_s, sizeof(restart_lsn_s), %X/%X,
-(uint32) (restart_lsn  32),
(uint32) restart_lsn);
-
i = 0;
values[i++] = CStringGetTextDatum(slot_name);
if (database == InvalidOid)
@@ -180,7 +175,7 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
else
nulls[i++] = true;
if (restart_lsn != InvalidTransactionId)
-   values[i++] = CStringGetTextDatum(restart_lsn_s);
+   values[i++] = restart_lsn;
else
nulls[i++] = true;

Anything else?
-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-04 Thread Andres Freund
On 2014-02-04 19:17:51 +0900, Michael Paquier wrote:
 On Tue, Feb 4, 2014 at 6:15 PM, Andres Freund and...@2ndquadrant.com wrote:
 
  + /*--
  +  *  Relational operators for LSNs
  +  *-*/
 
  Isn't it just operators? They aren't really relational...
 Operators for LSNs?

Fine with me.

  + #define DatumGetLogSeqNum(X) ((XLogRecPtr) GET_8_BYTES(X))
 
  I am not a fan of LogSegNum. I think at this point fewer people
  understand that than LSN. There's also no reason to invent a third term
  for LSNs. We'd have LSN, XLogRecPtr, and LogSeqNum.
 So let's go with DatumGetLSN and LSNGetDatum instead...

Sup.

  *** a/src/backend/replication/slotfuncs.c
  --- b/src/backend/replication/slotfuncs.c
  ***
  *** 141,148  pg_get_replication_slots(PG_FUNCTION_ARGS)
boolactive;
Oid database;
const char *slot_name;
  -
  - charrestart_lsn_s[MAXFNAMELEN];
int i;
 
SpinLockAcquire(slot-mutex);
  --- 141,146 
 
  Unrelated change.
 Funnily, the patch attached in my previous mail did not include all
 the diffs, it is an error with filterdiff that I use to generate
 context diff patches... My original branch includes the following

Ah, then it makes more sense.

 diffs as well in slotfuncs.c for the second patch:
 diff --git a/src/backend/replication/slotfuncs.c
 b/src/backend/replication/slotfuncs.c
 index 98a860e..68ecdcd 100644
 --- a/src/backend/replication/slotfuncs.c
 +++ b/src/backend/replication/slotfuncs.c
 @@ -141,8 +141,6 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 boolactive;
 Oid database;
 const char *slot_name;
 -
 -   charrestart_lsn_s[MAXFNAMELEN];
 int i;
 
 SpinLockAcquire(slot-mutex);
 @@ -164,9 +162,6 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 
 memset(nulls, 0, sizeof(nulls));
 
 -   snprintf(restart_lsn_s, sizeof(restart_lsn_s), %X/%X,
 -(uint32) (restart_lsn  32),
 (uint32) restart_lsn);
 -
 i = 0;
 values[i++] = CStringGetTextDatum(slot_name);
 if (database == InvalidOid)
 @@ -180,7 +175,7 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 else
 nulls[i++] = true;
 if (restart_lsn != InvalidTransactionId)
 -   values[i++] = CStringGetTextDatum(restart_lsn_s);
 +   values[i++] = restart_lsn;
 else
 nulls[i++] = true;

Isn't that missing a LSNGetDatum()? Also, isn't it lacking the
corresponding pg_proc change?

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] should we add a XLogRecPtr/LSN SQL type?

2014-02-04 Thread Michael Paquier
On Tue, Feb 4, 2014 at 7:22 PM, Andres Freund and...@2ndquadrant.com wrote:
 On 2014-02-04 19:17:51 +0900, Michael Paquier wrote:
 @@ -180,7 +175,7 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 else
 nulls[i++] = true;
 if (restart_lsn != InvalidTransactionId)
 -   values[i++] = CStringGetTextDatum(restart_lsn_s);
 +   values[i++] = restart_lsn;
 else
 nulls[i++] = true;

 Isn't that missing a LSNGetDatum()?
Oops yes. Will fix.

 Also, isn't it lacking the corresponding pg_proc change?
restart_lsn is the 6th argument of pg_get_replication_slots, and the
list of arguments of this function is already changed like that in my
patch:
{25,25,26,16,28,25} = {25,25,26,16,28,3220}
Regards,
-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-04 Thread Andres Freund
On 2014-02-04 21:04:13 +0900, Michael Paquier wrote:
 On Tue, Feb 4, 2014 at 7:22 PM, Andres Freund and...@2ndquadrant.com wrote:
  On 2014-02-04 19:17:51 +0900, Michael Paquier wrote:
  @@ -180,7 +175,7 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
  else
  nulls[i++] = true;
  if (restart_lsn != InvalidTransactionId)
  -   values[i++] = CStringGetTextDatum(restart_lsn_s);
  +   values[i++] = restart_lsn;
  else
  nulls[i++] = true;
 
  Isn't that missing a LSNGetDatum()?
 Oops yes. Will fix.
 
  Also, isn't it lacking the corresponding pg_proc change?
 restart_lsn is the 6th argument of pg_get_replication_slots, and the
 list of arguments of this function is already changed like that in my
 patch:
 {25,25,26,16,28,25} = {25,25,26,16,28,3220}
 Regards,

Ok.

I think the patch should also adapt pageinspect...

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] should we add a XLogRecPtr/LSN SQL type?

2014-02-04 Thread Peter Eisentraut
Perhaps this type should be called pglsn, since it's an
implementation-specific detail and not a universal concept like int,
point, or uuid.



-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-04 Thread Michael Paquier
On Wed, Feb 5, 2014 at 5:26 AM, Peter Eisentraut pete...@gmx.net wrote:
 Perhaps this type should be called pglsn, since it's an
 implementation-specific detail and not a universal concept like int,
 point, or uuid.
It makes sense. I'll update the patches according to that.
-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-04 Thread Michael Paquier
On Wed, Feb 5, 2014 at 9:38 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Wed, Feb 5, 2014 at 8:59 AM, Michael Paquier
 michael.paqu...@gmail.com wrote:
  I'll update the patches according to that.
 Here are the updated patches with the following changes (according to
 previous comments):
 - Datatype is renamed to pglsn, documentation, file names, regressions
 and APIs are updated as well.
 - The DatumGet* and *GetDatum APIs are renamed with PGLSN (Should be
 PgLsn? But that's a detail)
 - pg_create_physical_replication_slot uses PGLSNGetDatum for its 6th argument
 For pageinspect, only page_header is impacted and I think that this
 should be a separated patch as it makes necessary to dump it to 1.2. I
 can write it later once the core parts are decided.
I just forgot to mention that the 2nd patch does not use context diffs
but git diffs because of filterdiff not able to catch all the new
content of slotfuncs.c.
Regards,
-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-03 Thread Michael Paquier
On Sun, Feb 2, 2014 at 12:07 AM, Andres Freund and...@2ndquadrant.com wrote:
 On 2014-02-02 00:04:41 +0900, Michael Paquier wrote:
 On Fri, Dec 13, 2013 at 2:47 AM, Tom Lane t...@sss.pgh.pa.us wrote:
  Andres Freund and...@2ndquadrant.com writes:
  On 2013-12-12 11:55:51 -0500, Tom Lane wrote:
  I'm not, however, terribly thrilled with the suggestions to add implicit
  casts associated with this type.  Implicit casts are generally dangerous.
 
  It's a tradeof. Currently we have the following functions returning LSNs
  as text:
  * pg_current_xlog_location
  * pg_current_xlog_insert_location
  * pg_last_xlog_receive_location
  * pg_last_xlog_replay_location
  one view containing LSNs
  * pg_stat_replication
  and the following functions accepting LSNs as textual paramters:
  * pg_xlog_location_diff
  * pg_xlogfile_name
 
  The question is how do we deal with backward compatibility when
  introducing a LSN type? There might be some broken code around
  monitoring if we simply replace the type without implicit casts.
 
  Given the limited usage, how bad would it really be if we simply
  made all those take/return the LSN type?  As long as the type's
  I/O representation looks like the old text format, I suspect
  most queries wouldn't notice.

 I've asked around inside 2ndq and we could find one single problematic
 query, so it's really not too bad.

 Are there some plans to awaken this patch (including changing the
 output of the functions of xlogfuncs.c)? This would be useful for the
 differential backup features I am looking at these days. I imagine
 that it is too late for 9.4 though...

 I think we should definitely go ahead with the patch per-se, we just
 added another system view with lsns in it... I don't have a too strong
 opinion whether to do it in 9.4 or 9.5. It seems fairly low impact to
 me, and it's an old patch, but I personally don't have the tuits to
 refresh the patch right now.
Please find attached a patch implementing lsn as a datatype, based on
the one Robert wrote a couple of years ago. Since XLogRecPtr has been
changed to a simple uint64, this *refresh* has needed some work. In
order to have this data type plugged in more natively with other xlog
system functions, I have added as well PG_RETURN_LSN and PG_GETARG_LSN
to facilitate the interface, making easier code management if
XLogRecPtr or LSN format are changed in the future.

Patch contains regression tests as well as a bit of documentation.
Perhaps this is too late for 9.4, so if there are no objections I'll
simply add this patch to the next commit fest in June for 9.5. Having
the system functions use this data type (pg_start_backup, pg_xlog_*,
create_rep_slot, etc.)  does not look to be that difficult as all the
functions in xlogfuncs.c actually use XLogRecPtr before changing it to
text, so it would actually simplify the code. I think I'll simply code
it as I just looking at it now...
Regards,
-- 
Michael
*** a/doc/src/sgml/datatype.sgml
--- b/doc/src/sgml/datatype.sgml
***
*** 155,160 
--- 155,166 
/row
  
row
+entrytypelsn/type/entry
+entry/entry
+entryLog Sequence Number/entry
+   /row
+ 
+   row
 entrytypemacaddr/type/entry
 entry/entry
 entryMAC (Media Access Control) address/entry
***
*** 4502,4507  SELECT * FROM pg_attribute
--- 4508,4527 
 /para
/sect1
  
+   sect1 id=datatype-lsn
+titleacronymLSN/ Type/title
+ 
+indexterm zone=datatype-lsn
+ primaryLSN/primary
+/indexterm
+ 
+para
+ The typelsn/type data type can be used to store LSN (Log Sequence
+ Number) data which is a pointer to a location in the XLOG. This type is a
+ representation of XLogRecPtr.
+/para
+   /sect1
+ 
sect1 id=datatype-pseudo
 titlePseudo-Types/title
  
*** a/src/backend/utils/adt/Makefile
--- b/src/backend/utils/adt/Makefile
***
*** 20,26  OBJS = acl.o arrayfuncs.o array_selfuncs.o array_typanalyze.o \
  	cash.o char.o date.o datetime.o datum.o domains.o \
  	enum.o float.o format_type.o \
  	geo_ops.o geo_selfuncs.o int.o int8.o json.o jsonfuncs.o like.o \
! 	lockfuncs.o misc.o nabstime.o name.o numeric.o numutils.o \
  	oid.o oracle_compat.o orderedsetaggs.o \
  	pseudotypes.o rangetypes.o rangetypes_gist.o \
  	rowtypes.o regexp.o regproc.o ruleutils.o selfuncs.o \
--- 20,26 
  	cash.o char.o date.o datetime.o datum.o domains.o \
  	enum.o float.o format_type.o \
  	geo_ops.o geo_selfuncs.o int.o int8.o json.o jsonfuncs.o like.o \
! 	lockfuncs.o lsn.o misc.o nabstime.o name.o numeric.o numutils.o \
  	oid.o oracle_compat.o orderedsetaggs.o \
  	pseudotypes.o rangetypes.o rangetypes_gist.o \
  	rowtypes.o regexp.o regproc.o ruleutils.o selfuncs.o \
*** /dev/null
--- b/src/backend/utils/adt/lsn.c
***
*** 0 
--- 1,180 
+ /*-
+  *
+  * lsn.c
+  *	  Internal LSN operations
+  *

Re: [HACKERS] should we add a XLogRecPtr/LSN SQL type?

2014-02-03 Thread Tom Lane
Michael Paquier michael.paqu...@gmail.com writes:
 Please find attached a patch implementing lsn as a datatype, based on
 the one Robert wrote a couple of years ago.

 Patch contains regression tests as well as a bit of documentation.
 Perhaps this is too late for 9.4, so if there are no objections I'll
 simply add this patch to the next commit fest in June for 9.5.

I may have lost count, but aren't a bunch of the affected functions new
in 9.4?  If so, there's a good argument to be made that we should get
this in now, rather than waiting and having an API change for those
functions in 9.5.

regards, tom lane


-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-03 Thread Michael Paquier
On Tue, Feb 4, 2014 at 10:10 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Michael Paquier michael.paqu...@gmail.com writes:
 Please find attached a patch implementing lsn as a datatype, based on
 the one Robert wrote a couple of years ago.

 Patch contains regression tests as well as a bit of documentation.
 Perhaps this is too late for 9.4, so if there are no objections I'll
 simply add this patch to the next commit fest in June for 9.5.

 I may have lost count, but aren't a bunch of the affected functions new
 in 9.4?  If so, there's a good argument to be made that we should get
 this in now, rather than waiting and having an API change for those
 functions in 9.5.
Cool... I have created a second patch that updates all the system
functions to use the new lsn datatype introduced in the 1st patch
(pg_start|stop_backup, replication_slot stuff, xlog diff things, etc.)
and it is attached. This cleans up quite a bit of code in xlogfuncs.c
because we do not need anymore the LSN - cstring transformations!
I am also attaching a v2 of the first patch, I noticed that lsn_in
introduced in the first patch was using some error messages not
consistent with the ones of validate_xlog_location:xlogfuncs.c. Note
as well that validate_xlog_location is removed in the 2nd patch where
all the system functions are swicthed to the new datatype.
Regards,
-- 
Michael
*** a/doc/src/sgml/datatype.sgml
--- b/doc/src/sgml/datatype.sgml
***
*** 155,160 
--- 155,166 
/row
  
row
+entrytypelsn/type/entry
+entry/entry
+entryLog Sequence Number/entry
+   /row
+ 
+   row
 entrytypemacaddr/type/entry
 entry/entry
 entryMAC (Media Access Control) address/entry
***
*** 4502,4507  SELECT * FROM pg_attribute
--- 4508,4527 
 /para
/sect1
  
+   sect1 id=datatype-lsn
+titleacronymLSN/ Type/title
+ 
+indexterm zone=datatype-lsn
+ primaryLSN/primary
+/indexterm
+ 
+para
+ The typelsn/type data type can be used to store LSN (Log Sequence
+ Number) data which is a pointer to a location in the XLOG. This type is a
+ representation of XLogRecPtr.
+/para
+   /sect1
+ 
sect1 id=datatype-pseudo
 titlePseudo-Types/title
  
*** a/src/backend/utils/adt/Makefile
--- b/src/backend/utils/adt/Makefile
***
*** 20,26  OBJS = acl.o arrayfuncs.o array_selfuncs.o array_typanalyze.o \
  	cash.o char.o date.o datetime.o datum.o domains.o \
  	enum.o float.o format_type.o \
  	geo_ops.o geo_selfuncs.o int.o int8.o json.o jsonfuncs.o like.o \
! 	lockfuncs.o misc.o nabstime.o name.o numeric.o numutils.o \
  	oid.o oracle_compat.o orderedsetaggs.o \
  	pseudotypes.o rangetypes.o rangetypes_gist.o \
  	rowtypes.o regexp.o regproc.o ruleutils.o selfuncs.o \
--- 20,26 
  	cash.o char.o date.o datetime.o datum.o domains.o \
  	enum.o float.o format_type.o \
  	geo_ops.o geo_selfuncs.o int.o int8.o json.o jsonfuncs.o like.o \
! 	lockfuncs.o lsn.o misc.o nabstime.o name.o numeric.o numutils.o \
  	oid.o oracle_compat.o orderedsetaggs.o \
  	pseudotypes.o rangetypes.o rangetypes_gist.o \
  	rowtypes.o regexp.o regproc.o ruleutils.o selfuncs.o \
*** /dev/null
--- b/src/backend/utils/adt/lsn.c
***
*** 0 
--- 1,180 
+ /*-
+  *
+  * lsn.c
+  *	  Internal LSN operations
+  *
+  * Portions Copyright (c) 1996-2011, PostgreSQL Global Development Group
+  * Portions Copyright (c) 1994, Regents of the University of California
+  *
+  * IDENTIFICATION
+  *	  src/backend/utils/adt/lsn.c
+  *
+  *-
+  */
+ #include postgres.h
+ 
+ #include funcapi.h
+ #include libpq/pqformat.h
+ #include utils/builtins.h
+ #include utils/lsn.h
+ 
+ #define MAXLSNLEN		17
+ #define MAXLSNCOMPONENT	8
+ 
+ /*--
+  * Formatting and conversion routines.
+  *-*/
+ 
+ Datum
+ lsn_in(PG_FUNCTION_ARGS)
+ {
+ 	char	   *str = PG_GETARG_CSTRING(0);
+ 	int			len1, len2;
+ 	uint32		id, off;
+ 	XLogRecPtr	result;
+ 
+ 	/* Sanity check input format. */
+ 	len1 = strspn(str, 0123456789abcdefABCDEF);
+ 	if (len1  1 || len1  MAXLSNCOMPONENT || str[len1] != '/')
+ 		ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+  errmsg(invalid input syntax for transaction log location: \%s\, str)));
+ 	len2 = strspn(str + len1 + 1, 0123456789abcdefABCDEF);
+ 	if (len2  1 || len2  MAXLSNCOMPONENT || str[len1 + 1 + len2] != '\0')
+ 		ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+  errmsg(invalid input syntax for transaction log location: \%s\, str)));
+ 
+ 	/* Decode result. */
+ 	id = (uint32) strtoul(str, NULL, 16);
+ 	off = (uint32) strtoul(str + len1 + 1, NULL, 16);
+ 	result = (XLogRecPtr) ((uint64) id  32) | off;
+ 
+ 	PG_RETURN_LSN(result);

Re: [HACKERS] should we add a XLogRecPtr/LSN SQL type?

2014-02-01 Thread Michael Paquier
On Fri, Dec 13, 2013 at 2:47 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Andres Freund and...@2ndquadrant.com writes:
 On 2013-12-12 11:55:51 -0500, Tom Lane wrote:
 I'm not, however, terribly thrilled with the suggestions to add implicit
 casts associated with this type.  Implicit casts are generally dangerous.

 It's a tradeof. Currently we have the following functions returning LSNs
 as text:
 * pg_current_xlog_location
 * pg_current_xlog_insert_location
 * pg_last_xlog_receive_location
 * pg_last_xlog_replay_location
 one view containing LSNs
 * pg_stat_replication
 and the following functions accepting LSNs as textual paramters:
 * pg_xlog_location_diff
 * pg_xlogfile_name

 The question is how do we deal with backward compatibility when
 introducing a LSN type? There might be some broken code around
 monitoring if we simply replace the type without implicit casts.

 Given the limited usage, how bad would it really be if we simply
 made all those take/return the LSN type?  As long as the type's
 I/O representation looks like the old text format, I suspect
 most queries wouldn't notice.
Are there some plans to awaken this patch (including changing the
output of the functions of xlogfuncs.c)? This would be useful for the
differential backup features I am looking at these days. I imagine
that it is too late for 9.4 though...
Regards,
-- 
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] should we add a XLogRecPtr/LSN SQL type?

2014-02-01 Thread Andres Freund
On 2014-02-02 00:04:41 +0900, Michael Paquier wrote:
 On Fri, Dec 13, 2013 at 2:47 AM, Tom Lane t...@sss.pgh.pa.us wrote:
  Andres Freund and...@2ndquadrant.com writes:
  On 2013-12-12 11:55:51 -0500, Tom Lane wrote:
  I'm not, however, terribly thrilled with the suggestions to add implicit
  casts associated with this type.  Implicit casts are generally dangerous.
 
  It's a tradeof. Currently we have the following functions returning LSNs
  as text:
  * pg_current_xlog_location
  * pg_current_xlog_insert_location
  * pg_last_xlog_receive_location
  * pg_last_xlog_replay_location
  one view containing LSNs
  * pg_stat_replication
  and the following functions accepting LSNs as textual paramters:
  * pg_xlog_location_diff
  * pg_xlogfile_name
 
  The question is how do we deal with backward compatibility when
  introducing a LSN type? There might be some broken code around
  monitoring if we simply replace the type without implicit casts.
 
  Given the limited usage, how bad would it really be if we simply
  made all those take/return the LSN type?  As long as the type's
  I/O representation looks like the old text format, I suspect
  most queries wouldn't notice.

I've asked around inside 2ndq and we could find one single problematic
query, so it's really not too bad.

 Are there some plans to awaken this patch (including changing the
 output of the functions of xlogfuncs.c)? This would be useful for the
 differential backup features I am looking at these days. I imagine
 that it is too late for 9.4 though...

I think we should definitely go ahead with the patch per-se, we just
added another system view with lsns in it... I don't have a too strong
opinion whether to do it in 9.4 or 9.5. It seems fairly low impact to
me, and it's an old patch, but I personally don't have the tuits to
refresh the patch right now.

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] should we add a XLogRecPtr/LSN SQL type?

2013-12-12 Thread Greg Stark
Bonus points if you implement a (explicit) cast to and from timestamptz :)

-- 
greg
On 11 Dec 2013 12:41, Andres Freund and...@2ndquadrant.com wrote:

 Hi,

 There's already a couple of SQL function dealing with XLogRecPtrs and
 the logical replication work will add a couple of more. Currently each
 of those funtions taking/returning an LSN does sprintf/scanf to
 print/parse the strings. Which both is awkward and potentially
 noticeable performancewise.

 It seems relatively simple to add a proper type, with implicit casts
 from text, instead?

 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] should we add a XLogRecPtr/LSN SQL type?

2013-12-12 Thread Andres Freund
On 2013-12-11 08:13:18 -0500, Robert Haas wrote:
 On Wed, Dec 11, 2013 at 7:41 AM, Andres Freund and...@2ndquadrant.com wrote:
  There's already a couple of SQL function dealing with XLogRecPtrs and
  the logical replication work will add a couple of more. Currently each
  of those funtions taking/returning an LSN does sprintf/scanf to
  print/parse the strings. Which both is awkward and potentially
  noticeable performancewise.
 
  It seems relatively simple to add a proper type, with implicit casts
  from text, instead?
 
 I'm pretty sure that this was discussed last year, and I voted for it
 but more people
 voted against it, so it died.  I still think that was a mistake, but I
 just work here.

Ah. I missed or forgot that discussion. The primary argument seemed to
be that we were are only talking about a single function using it. I
don't think that really was true back then, but there definitely are
some more uses coming up. E.g. the changeset extraction SRF will return
the LSN of every extracted change...
I don't really buy the argument that it can wholl be replaced by
representing LSNs as numeric - those look significantly different enough
that that doesn't seem to make much sense to me. Also, they are a
pass-by-reference type...

 -- except for the implicit casts part, perhaps --

I'd like to convert some existing functions to use the new type, and
without added casts that seems too likely to break existing usages. If
we just consistently use the new type everywhere, I can't see the usual
troubles with the added casts.

On 2013-12-11 10:13:51 -0300, Euler Taveira wrote:
 On 11-12-2013 09:41, Andres Freund wrote:
 While discussing pg_xlog_location_diff function, Robert posted a lsn
 datatype [1]. At that time we wouldn't go that far (a new datatype) to
 cover only one function. If your proposal is just validation, I think
 generic validation functions is the way to follow. However, if you are
 thinking in adding operators, the lsn datatype should be implemented.

I am mostly thinking of returning many such datums - representing them
as text is just pointless overhead. But even if it just were validation
- why sprinkle validation over dozens of places if we actually have a
typesystem to handle that kind of thing?

  It seems relatively simple to add a proper type, with implicit casts
  from text, instead?
 Do you want to change the function signatures too?

Yes.

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] should we add a XLogRecPtr/LSN SQL type?

2013-12-12 Thread Simon Riggs
On 12 December 2013 12:27, Andres Freund and...@2ndquadrant.com wrote:
 On 2013-12-11 08:13:18 -0500, Robert Haas wrote:
 On Wed, Dec 11, 2013 at 7:41 AM, Andres Freund and...@2ndquadrant.com 
 wrote:
  There's already a couple of SQL function dealing with XLogRecPtrs and
  the logical replication work will add a couple of more. Currently each
  of those funtions taking/returning an LSN does sprintf/scanf to
  print/parse the strings. Which both is awkward and potentially
  noticeable performancewise.
 
  It seems relatively simple to add a proper type, with implicit casts
  from text, instead?

 I'm pretty sure that this was discussed last year, and I voted for it
 but more people
 voted against it, so it died.  I still think that was a mistake, but I
 just work here.

 Ah. I missed or forgot that discussion.

Hmm, don't recall that. Just in case I opposed it, its a good idea now.

-- 
 Simon Riggs   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] should we add a XLogRecPtr/LSN SQL type?

2013-12-12 Thread Robert Haas
On Thu, Dec 12, 2013 at 8:20 AM, Simon Riggs si...@2ndquadrant.com wrote:
 On 12 December 2013 12:27, Andres Freund and...@2ndquadrant.com wrote:
 On 2013-12-11 08:13:18 -0500, Robert Haas wrote:
 On Wed, Dec 11, 2013 at 7:41 AM, Andres Freund and...@2ndquadrant.com 
 wrote:
  There's already a couple of SQL function dealing with XLogRecPtrs and
  the logical replication work will add a couple of more. Currently each
  of those funtions taking/returning an LSN does sprintf/scanf to
  print/parse the strings. Which both is awkward and potentially
  noticeable performancewise.
 
  It seems relatively simple to add a proper type, with implicit casts
  from text, instead?

 I'm pretty sure that this was discussed last year, and I voted for it
 but more people
 voted against it, so it died.  I still think that was a mistake, but I
 just work here.

 Ah. I missed or forgot that discussion.

 Hmm, don't recall that. Just in case I opposed it, its a good idea now.

I am happy to have my old patch resurrected - could become a trend.
But someone should probably go back and check who objected and for
what reasons.

-- 
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] should we add a XLogRecPtr/LSN SQL type?

2013-12-12 Thread Alvaro Herrera
Robert Haas escribió:

 I am happy to have my old patch resurrected - could become a trend.
 But someone should probably go back and check who objected and for
 what reasons.

Here it is FWIW:

http://www.postgresql.org/message-id/flat/ca+tgmozrmnn0evesd-kxb9e-mvdmwoti6guujuvqp_8q2c5...@mail.gmail.com

-- 
Á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] should we add a XLogRecPtr/LSN SQL type?

2013-12-12 Thread Tom Lane
Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Robert Haas escribió:
 I am happy to have my old patch resurrected - could become a trend.
 But someone should probably go back and check who objected and for
 what reasons.

 Here it is FWIW:
 http://www.postgresql.org/message-id/flat/ca+tgmozrmnn0evesd-kxb9e-mvdmwoti6guujuvqp_8q2c5...@mail.gmail.com

AFAICS the objections were why bother with a datatype for just one
function.  If we've now got multiple use-cases, that loses its force.

I'm not, however, terribly thrilled with the suggestions to add implicit
casts associated with this type.  Implicit casts are generally dangerous.

regards, tom lane


-- 
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] should we add a XLogRecPtr/LSN SQL type?

2013-12-12 Thread Andres Freund
Hi,

On 2013-12-12 11:55:51 -0500, Tom Lane wrote:
 I'm not, however, terribly thrilled with the suggestions to add implicit
 casts associated with this type.  Implicit casts are generally dangerous.

It's a tradeof. Currently we have the following functions returning LSNs
as text:
* pg_current_xlog_location
* pg_current_xlog_insert_location
* pg_last_xlog_receive_location
* pg_last_xlog_replay_location
one view containing LSNs
* pg_stat_replication
and the following functions accepting LSNs as textual paramters:
* pg_xlog_location_diff
* pg_xlogfile_name

The question is how do we deal with backward compatibility when
introducing a LSN type? There might be some broken code around
monitoring if we simply replace the type without implicit casts. But
just leaving all those as-is seems quite unattractive.

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] should we add a XLogRecPtr/LSN SQL type?

2013-12-12 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 On 2013-12-12 11:55:51 -0500, Tom Lane wrote:
 I'm not, however, terribly thrilled with the suggestions to add implicit
 casts associated with this type.  Implicit casts are generally dangerous.

 It's a tradeof. Currently we have the following functions returning LSNs
 as text:
 * pg_current_xlog_location
 * pg_current_xlog_insert_location
 * pg_last_xlog_receive_location
 * pg_last_xlog_replay_location
 one view containing LSNs
 * pg_stat_replication
 and the following functions accepting LSNs as textual paramters:
 * pg_xlog_location_diff
 * pg_xlogfile_name

 The question is how do we deal with backward compatibility when
 introducing a LSN type? There might be some broken code around
 monitoring if we simply replace the type without implicit casts.

Given the limited usage, how bad would it really be if we simply
made all those take/return the LSN type?  As long as the type's
I/O representation looks like the old text format, I suspect
most queries wouldn't notice.

regards, tom lane


-- 
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] should we add a XLogRecPtr/LSN SQL type?

2013-12-11 Thread Robert Haas
On Wed, Dec 11, 2013 at 7:41 AM, Andres Freund and...@2ndquadrant.com wrote:
 There's already a couple of SQL function dealing with XLogRecPtrs and
 the logical replication work will add a couple of more. Currently each
 of those funtions taking/returning an LSN does sprintf/scanf to
 print/parse the strings. Which both is awkward and potentially
 noticeable performancewise.

 It seems relatively simple to add a proper type, with implicit casts
 from text, instead?

I'm pretty sure that this was discussed last year, and I voted for it
-- except for the implicit casts part, perhaps -- but more people
voted against it, so it died.  I still think that was a mistake, but I
just work 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] should we add a XLogRecPtr/LSN SQL type?

2013-12-11 Thread Euler Taveira
On 11-12-2013 09:41, Andres Freund wrote:
 There's already a couple of SQL function dealing with XLogRecPtrs and
 the logical replication work will add a couple of more. Currently each
 of those funtions taking/returning an LSN does sprintf/scanf to
 print/parse the strings. Which both is awkward and potentially
 noticeable performancewise.
 
While discussing pg_xlog_location_diff function, Robert posted a lsn
datatype [1]. At that time we wouldn't go that far (a new datatype) to
cover only one function. If your proposal is just validation, I think
generic validation functions is the way to follow. However, if you are
thinking in adding operators, the lsn datatype should be implemented.

 It seems relatively simple to add a proper type, with implicit casts
 from text, instead?
 
Do you want to change the function signatures too?


[1]
http://www.postgresql.org/message-id/ca+tgmozrmnn0evesd-kxb9e-mvdmwoti6guujuvqp_8q2c5...@mail.gmail.com


-- 
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


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