Re: [HACKERS] pg_receivexlog and replication slots

2014-10-06 Thread Michael Paquier
On Mon, Oct 6, 2014 at 7:14 PM, Andres Freund and...@2ndquadrant.com
wrote:

 On 2014-10-04 14:25:27 +0900, Michael Paquier wrote:
  And as I am on it, attached is a patch that can be applied to master and
  REL9_4_STABLE to rename the --create and --drop to --create-slot and
  --drop-slot.

 Thanks.

  diff --git a/src/bin/pg_basebackup/pg_recvlogical.c
 b/src/bin/pg_basebackup/pg_recvlogical.c
  index c48cecc..585d7b0 100644
  --- a/src/bin/pg_basebackup/pg_recvlogical.c
  +++ b/src/bin/pg_basebackup/pg_recvlogical.c
  @@ -91,8 +91,8 @@ usage(void)
  time between status
 packets sent to server (default: %d)\n), (standby_message_timeout / 1000));
printf(_(  -S, --slot=SLOTname of the logical replication
 slot\n));
printf(_(\nAction to be performed:\n));
  - printf(_(  --create   create a new replication slot
 (for the slot's name see --slot)\n));
  - printf(_(  --startstart streaming in a
 replication slot (for the slot's name see --slot)\n));
  + printf(_(  --create-slot  create a new replication slot
 (for the slot's name see --slot)\n));
  + printf(_(  --start-slot   start streaming in a
 replication slot (for the slot's name see --slot)\n));
printf(_(  --drop drop the replication slot (for
 the slot's name see --slot)\n));
printf(_(\nReport bugs to pgsql-b...@postgresql.org.\n));
   }
  @@ -618,9 +618,9 @@ main(int argc, char **argv)
{status-interval, required_argument, NULL, 's'},
{slot, required_argument, NULL, 'S'},
   /* action */
  - {create, no_argument, NULL, 1},
  + {create-slot, no_argument, NULL, 1},
{start, no_argument, NULL, 2},
  - {drop, no_argument, NULL, 3},
  + {drop-slot, no_argument, NULL, 3},
{NULL, 0, NULL, 0}

 Uh? You're documenting --start-slot here and a couple other places, but
 you implement --drop-slot. And --start-slot doesn't seem to make much
 sense to me.

+-+, yes. It was aimed to be --create-slot :) Perhaps that was caused
because of the lack of caffeine. Thanks for noticing and fixing.
-- 
Michael


Re: [HACKERS] pg_receivexlog and replication slots

2014-10-06 Thread Andres Freund
Hi,

On 2014-10-04 15:01:03 +0900, Michael Paquier wrote:
 On Fri, Oct 3, 2014 at 8:57 PM, Andres Freund and...@2ndquadrant.com wrote:
   para
   +applicationpg_receivexlog/application can run in one of two 
   following
   +modes, which control physical replication slot:
 
  I don't think that's good enough. There's also the important mode where
  it's not doing --create/--drop at all.
 Well, yes, however the third mode is not explicitly present, and I
 don't see much point in adding a --start mode thinking
 backward-compatibility. Now, I refactored a bit the documentation to
 mention that pg_receivexlog can perform additional actions to control
 replication slots. I added as well in the portion of option --slot how
 it interacts with --create-slot and --drop-slot.

That's better.

   + if (db_name)
   + {
   + fprintf(stderr,
   + _(%s: database defined for replication 
   connection \%s\\n),
   + progname, replication_slot);
   + disconnect_and_exit(1);
   + }
 
  I don't like 'defined' here. 'replication connection unexpectedly is
  database specific' or something would be better.
 
 Sure, IMO the error message should as well mention the replication
 slot being used, so I reformulated as such:
 replication connection using slot foo is unexpectedly database specific

I don't see why the slot should be there. If this has gone wrong it's
not related to the slot.

 +applicationpg_receivexlog/application can perform one of the two
 +following actions in order to control physical replication slots:
 +
 +variablelist
 + varlistentry
 +  termoption--create-slot/option/term
 +  listitem
 +   para
 +Create a new physical replication slot with the name specified in
 +option--slot/option, then start stream.
 +   /para
 +  /listitem
 + /varlistentry

*, then start to stream WAL.

 + if (replication_slot == NULL  (do_drop_slot || do_create_slot))
 + {
 + fprintf(stderr, _(%s: replication slot needed with action 
 --create-slot or --drop-slot\n), progname);
 + fprintf(stderr, _(Try \%s --help\ for more information.\n),
 + progname);
 + exit(1);
 + }

I changed this to refer to --slot.

 + /*
 +  * Run IDENTIFY_SYSTEM so we can get the timeline and current xlog
 +  * position.
 +  */
 + if (!RunIdentifySystem(conn, NULL, NULL, NULL, db_name))
 + disconnect_and_exit(1);

Comment is wrongly copied. Obviously we're neither looking at the
timeline nor the WAL position.

Pushed with these adjustments.

Amit, Fujii: Sorry for not mentioning you as reviewers of the
patchset. I hadn't followed the earlier development and thus somehow
missed that you'd both done a couple rounds of reivew.

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] pg_receivexlog and replication slots

2014-10-06 Thread Michael Paquier
On Mon, Oct 6, 2014 at 8:01 PM, Andres Freund and...@2ndquadrant.com
wrote:

 Pushed with these adjustments.

Thanks. The portions changed look fine to me.
--
Michael


Re: [HACKERS] pg_receivexlog and replication slots

2014-10-04 Thread Michael Paquier
On Fri, Oct 3, 2014 at 8:57 PM, Andres Freund and...@2ndquadrant.com wrote:
  para
  +applicationpg_receivexlog/application can run in one of two 
  following
  +modes, which control physical replication slot:

 I don't think that's good enough. There's also the important mode where
 it's not doing --create/--drop at all.
Well, yes, however the third mode is not explicitly present, and I
don't see much point in adding a --start mode thinking
backward-compatibility. Now, I refactored a bit the documentation to
mention that pg_receivexlog can perform additional actions to control
replication slots. I added as well in the portion of option --slot how
it interacts with --create-slot and --drop-slot.

  + if (db_name)
  + {
  + fprintf(stderr,
  + _(%s: database defined for replication 
  connection \%s\\n),
  + progname, replication_slot);
  + disconnect_and_exit(1);
  + }

 I don't like 'defined' here. 'replication connection unexpectedly is
 database specific' or something would be better.

Sure, IMO the error message should as well mention the replication
slot being used, so I reformulated as such:
replication connection using slot foo is unexpectedly database specific


 I do wonder whether --create/--drop aren't somewhat weird for
 pg_receivexlog. It's not that clear what it means. It'd be ugly, but we
 could rename them --create-slot/drop-slot.

In line with the other patch sent earlier, options are renamed to
--create-slot and --drop-slot.
Regards,
-- 
Michael
From aba831b62795303ec666b8c18810f404458d8acd Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Mon, 1 Sep 2014 20:53:45 +0900
Subject: [PATCH] Support for replslot creation and drop in pg_receivexlog

Using the new actions --create-slot and --drop-slot that are similarly
present in pg_recvlogical, a user can respectively create and drop a
replication slot that can be used afterwards when fetching WALs.
---
 doc/src/sgml/ref/pg_receivexlog.sgml   |  31 ++-
 src/bin/pg_basebackup/pg_receivexlog.c | 154 +
 2 files changed, 169 insertions(+), 16 deletions(-)

diff --git a/doc/src/sgml/ref/pg_receivexlog.sgml b/doc/src/sgml/ref/pg_receivexlog.sgml
index 5916b8f..72290e5 100644
--- a/doc/src/sgml/ref/pg_receivexlog.sgml
+++ b/doc/src/sgml/ref/pg_receivexlog.sgml
@@ -255,7 +255,9 @@ PostgreSQL documentation
  to make sure that applicationpg_receivexlog/ cannot become the
  synchronous standby through an incautious setting of
  xref linkend=guc-synchronous-standby-names; it does not flush
- data frequently enough for this to work correctly.
+ data frequently enough for this to work correctly. In
+ option--create-slot/option mode, create the slot with this name.
+ In option--drop-slot/option mode, delete the slot with this name.
 /para
   /listitem
  /varlistentry
@@ -263,6 +265,33 @@ PostgreSQL documentation
/para
 
para
+applicationpg_receivexlog/application can perform one of the two
+following actions in order to control physical replication slots:
+
+variablelist
+ varlistentry
+  termoption--create-slot/option/term
+  listitem
+   para
+Create a new physical replication slot with the name specified in
+option--slot/option, then start stream.
+   /para
+  /listitem
+ /varlistentry
+
+ varlistentry
+  termoption--drop-slot/option/term
+  listitem
+   para
+Drop the replication slot with the name specified in
+option--slot/option, then exit.
+   /para
+  /listitem
+ /varlistentry
+/variablelist
+   /para
+
+   para
 Other options are also available:
 
 variablelist
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index 171cf43..5b71f85 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -38,11 +38,15 @@ static int	noloop = 0;
 static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
 static int	fsync_interval = 0; /* 0 = default */
 static volatile bool time_to_abort = false;
+static bool do_create_slot = false;
+static bool do_drop_slot = false;
 
 
 static void usage(void);
+static DIR* get_destination_dir(char *dest_folder);
+static void close_destination_dir(DIR *dest_dir, char *dest_folder);
 static XLogRecPtr FindStreamingStart(uint32 *tli);
-static void StreamLog();
+static void StreamLog(void);
 static bool stop_streaming(XLogRecPtr segendpos, uint32 timeline,
 			   bool segment_finished);
 
@@ -78,6 +82,9 @@ usage(void)
 	printf(_(  -w, --no-password  never prompt for password\n));
 	printf(_(  -W, --password force password prompt (should happen automatically)\n));
 	printf(_(  -S, --slot=SLOTNAMEreplication slot to use\n));
+	printf(_(\nOptional actions:\n));
+	printf(_(   

Re: [HACKERS] pg_receivexlog and replication slots

2014-10-04 Thread Andres Freund
On 2014-10-04 09:24:07 +0900, Michael Paquier wrote:
 On Sat, Oct 4, 2014 at 6:48 AM, Andres Freund and...@2ndquadrant.com
 wrote:
 
  On 2014-10-03 14:02:08 -0400, Robert Haas wrote:
   On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund and...@2ndquadrant.com
  wrote:
I do wonder whether --create/--drop aren't somewhat wierd for
pg_receivexlog. It's not that clear what it means. It'd be ugly, but we
could rename them --create-slot/drop-slot.
  
   +1 on doing it, -1 on it being ugly.
 
  The reason I'm calling it uglyu is that it's different from
  pg_recvlogical. We could change it there, too? A bit late, but probably
  better than having a discrepancy forever
 
 I'm on board to make things as consistent as possible between both
 utilities, the only reason why --create/--drop are used in my patch is for
 the sake of consistency btw. 9.4 ship has not sailed yet, and IMO it is
 important from the user prospective if options are a maximum consistent
 between pg_receivexlog and pg_recvlogical. That would be even better if
 change is done before 9.4beta3 shows up, and I doubt that there are many
 users using the --create/--drop options already.

Any opinion on whether whe should accept both --create and --create-slot
or only the latter? Accepting both would get rid of problems due to
potential usages of the old syntax - and it's easier to type...

I don't really care.

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] pg_receivexlog and replication slots

2014-10-04 Thread Andres Freund
On 2014-10-04 14:25:27 +0900, Michael Paquier wrote:
 On Sat, Oct 4, 2014 at 9:24 AM, Michael Paquier michael.paqu...@gmail.com
 wrote:
 
 
 
  On Sat, Oct 4, 2014 at 6:48 AM, Andres Freund and...@2ndquadrant.com
  wrote:
 
  On 2014-10-03 14:02:08 -0400, Robert Haas wrote:
   On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund and...@2ndquadrant.com
  wrote:
I do wonder whether --create/--drop aren't somewhat wierd for
pg_receivexlog. It's not that clear what it means. It'd be ugly, but
  we
could rename them --create-slot/drop-slot.
  
   +1 on doing it, -1 on it being ugly.
 
  The reason I'm calling it uglyu is that it's different from
  pg_recvlogical. We could change it there, too? A bit late, but probably
  better than having a discrepancy forever
 
  I'm on board to make things as consistent as possible between both
  utilities, the only reason why --create/--drop are used in my patch is for
  the sake of consistency btw. 9.4 ship has not sailed yet, and IMO it is
  important from the user prospective if options are a maximum consistent
  between pg_receivexlog and pg_recvlogical. That would be even better if
  change is done before 9.4beta3 shows up, and I doubt that there are many
  users using the --create/--drop options already.
 
 And as I am on it, attached is a patch that can be applied to master and
 REL9_4_STABLE to rename the --create and --drop to --create-slot and
 --drop-slot.

Misses logicaldecoding.sgml... I'll fix that up, once there's agreement
whether we should continue accept the old form. No need to send a new
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] pg_receivexlog and replication slots

2014-10-04 Thread Michael Paquier
On Sun, Oct 5, 2014 at 12:09 AM, Andres Freund and...@2ndquadrant.com wrote:
 Any opinion on whether we should accept both --create and --create-slot
 or only the latter? Accepting both would get rid of problems due to
 potential usages of the old syntax - and it's easier to type...
My vote goes for only --create-slot and --drop-slot.
-- 
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] pg_receivexlog and replication slots

2014-10-04 Thread Robert Haas
On Sat, Oct 4, 2014 at 7:03 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Sun, Oct 5, 2014 at 12:09 AM, Andres Freund and...@2ndquadrant.com wrote:
 Any opinion on whether we should accept both --create and --create-slot
 or only the latter? Accepting both would get rid of problems due to
 potential usages of the old syntax - and it's easier to type...
 My vote goes for only --create-slot and --drop-slot.

Ditto.

-- 
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] pg_receivexlog and replication slots

2014-10-03 Thread Andres Freund
On 2014-10-03 10:30:19 +0900, Michael Paquier wrote:
 On Thu, Oct 2, 2014 at 12:44 AM, Andres Freund and...@2ndquadrant.com
 wrote:
 
  I pushed the first part.
 
 Thanks. Attached is a rebased version of patch 2, implementing the actual
 feature. One thing I noticed with more testing is that if --create is used
 and that the destination folder does not exist, pg_receivexlog was creating
 the slot, and left with an error. This does not look user-friendly so I
 changed the logic a bit to check for the destination folder before creating
 any slot. This results in a bit of refactoring, but this way behavior is
 more intuitive.

Ok.

 para
 +applicationpg_receivexlog/application can run in one of two following
 +modes, which control physical replication slot:

I don't think that's good enough. There's also the important mode where
it's not doing --create/--drop at all.


 + /*
 +  * Run IDENTIFY_SYSTEM so we can get the timeline and current xlog
 +  * position.
 +  */
 + if (!RunIdentifySystem(conn, NULL, NULL, NULL, db_name))
 + disconnect_and_exit(1);
 +
 + /*
 +  * Check that there is a database associated with connection, none
 +  * should be defined in this context.
 +  */
 + if (db_name)
 + {
 + fprintf(stderr,
 + _(%s: database defined for replication 
 connection \%s\\n),
 + progname, replication_slot);
 + disconnect_and_exit(1);
 + }

I don't like 'defined' here. 'replication connection unexpectedly is
database specific' or something would be better.

I do wonder whether --create/--drop aren't somewhat wierd for
pg_receivexlog. It's not that clear what it means. It'd be ugly, but we
could rename them --create-slot/drop-slot.

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] pg_receivexlog and replication slots

2014-10-03 Thread Robert Haas
On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund and...@2ndquadrant.com wrote:
 I do wonder whether --create/--drop aren't somewhat wierd for
 pg_receivexlog. It's not that clear what it means. It'd be ugly, but we
 could rename them --create-slot/drop-slot.

+1 on doing it, -1 on it being ugly.

-- 
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] pg_receivexlog and replication slots

2014-10-03 Thread Andres Freund
On 2014-10-03 14:02:08 -0400, Robert Haas wrote:
 On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund and...@2ndquadrant.com wrote:
  I do wonder whether --create/--drop aren't somewhat wierd for
  pg_receivexlog. It's not that clear what it means. It'd be ugly, but we
  could rename them --create-slot/drop-slot.
 
 +1 on doing it, -1 on it being ugly.

The reason I'm calling it uglyu is that it's different from
pg_recvlogical. We could change it there, too? A bit late, but probably
better than having a discrepancy forever?

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] pg_receivexlog and replication slots

2014-10-03 Thread Michael Paquier
On Sat, Oct 4, 2014 at 6:48 AM, Andres Freund and...@2ndquadrant.com
wrote:

 On 2014-10-03 14:02:08 -0400, Robert Haas wrote:
  On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund and...@2ndquadrant.com
 wrote:
   I do wonder whether --create/--drop aren't somewhat wierd for
   pg_receivexlog. It's not that clear what it means. It'd be ugly, but we
   could rename them --create-slot/drop-slot.
 
  +1 on doing it, -1 on it being ugly.

 The reason I'm calling it uglyu is that it's different from
 pg_recvlogical. We could change it there, too? A bit late, but probably
 better than having a discrepancy forever

I'm on board to make things as consistent as possible between both
utilities, the only reason why --create/--drop are used in my patch is for
the sake of consistency btw. 9.4 ship has not sailed yet, and IMO it is
important from the user prospective if options are a maximum consistent
between pg_receivexlog and pg_recvlogical. That would be even better if
change is done before 9.4beta3 shows up, and I doubt that there are many
users using the --create/--drop options already.
-- 
Michael


Re: [HACKERS] pg_receivexlog and replication slots

2014-10-03 Thread Michael Paquier
On Sat, Oct 4, 2014 at 9:24 AM, Michael Paquier michael.paqu...@gmail.com
wrote:



 On Sat, Oct 4, 2014 at 6:48 AM, Andres Freund and...@2ndquadrant.com
 wrote:

 On 2014-10-03 14:02:08 -0400, Robert Haas wrote:
  On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund and...@2ndquadrant.com
 wrote:
   I do wonder whether --create/--drop aren't somewhat wierd for
   pg_receivexlog. It's not that clear what it means. It'd be ugly, but
 we
   could rename them --create-slot/drop-slot.
 
  +1 on doing it, -1 on it being ugly.

 The reason I'm calling it uglyu is that it's different from
 pg_recvlogical. We could change it there, too? A bit late, but probably
 better than having a discrepancy forever

 I'm on board to make things as consistent as possible between both
 utilities, the only reason why --create/--drop are used in my patch is for
 the sake of consistency btw. 9.4 ship has not sailed yet, and IMO it is
 important from the user prospective if options are a maximum consistent
 between pg_receivexlog and pg_recvlogical. That would be even better if
 change is done before 9.4beta3 shows up, and I doubt that there are many
 users using the --create/--drop options already.

And as I am on it, attached is a patch that can be applied to master and
REL9_4_STABLE to rename the --create and --drop to --create-slot and
--drop-slot.
Regards,
-- 
Michael
diff --git a/doc/src/sgml/ref/pg_recvlogical.sgml b/doc/src/sgml/ref/pg_recvlogical.sgml
index 76240fe..ec28ff4 100644
--- a/doc/src/sgml/ref/pg_recvlogical.sgml
+++ b/doc/src/sgml/ref/pg_recvlogical.sgml
@@ -53,7 +53,7 @@ PostgreSQL documentation
 variablelist
 
  varlistentry
-  termoption--create/option/term
+  termoption--create-slot/option/term
   listitem
para
 Create a new logical replication slot with the name specified in
@@ -82,7 +82,7 @@ PostgreSQL documentation
  /varlistentry
 
  varlistentry
-  termoption--drop/option/term
+  termoption--drop-slot/option/term
   listitem
para
 Drop the replication slot with the name specified
@@ -266,8 +266,8 @@ PostgreSQL documentation
   listitem
para
 In option--start/option mode, use the existing logical replication slot named
-replaceableslot_name/replaceable. In option--create/option mode, create the
-slot with this name. In option--drop/option mode, delete the slot with this name.
+replaceableslot_name/replaceable. In option--create-slot/option mode, create the
+slot with this name. In option--drop-slot/option mode, delete the slot with this name.
/para
   /listitem
  /varlistentry
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index c48cecc..585d7b0 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -91,8 +91,8 @@ usage(void)
 			  time between status packets sent to server (default: %d)\n), (standby_message_timeout / 1000));
 	printf(_(  -S, --slot=SLOTname of the logical replication slot\n));
 	printf(_(\nAction to be performed:\n));
-	printf(_(  --create   create a new replication slot (for the slot's name see --slot)\n));
-	printf(_(  --startstart streaming in a replication slot (for the slot's name see --slot)\n));
+	printf(_(  --create-slot  create a new replication slot (for the slot's name see --slot)\n));
+	printf(_(  --start-slot   start streaming in a replication slot (for the slot's name see --slot)\n));
 	printf(_(  --drop drop the replication slot (for the slot's name see --slot)\n));
 	printf(_(\nReport bugs to pgsql-b...@postgresql.org.\n));
 }
@@ -618,9 +618,9 @@ main(int argc, char **argv)
 		{status-interval, required_argument, NULL, 's'},
 		{slot, required_argument, NULL, 'S'},
 /* action */
-		{create, no_argument, NULL, 1},
+		{create-slot, no_argument, NULL, 1},
 		{start, no_argument, NULL, 2},
-		{drop, no_argument, NULL, 3},
+		{drop-slot, no_argument, NULL, 3},
 		{NULL, 0, NULL, 0}
 	};
 	int			c;
@@ -814,7 +814,7 @@ main(int argc, char **argv)
 
 	if (do_drop_slot  (do_create_slot || do_start_slot))
 	{
-		fprintf(stderr, _(%s: cannot use --create or --start together with --drop\n), progname);
+		fprintf(stderr, _(%s: cannot use --create-slot or --start-slot together with --drop\n), progname);
 		fprintf(stderr, _(Try \%s --help\ for more information.\n),
 progname);
 		exit(1);
@@ -822,7 +822,7 @@ main(int argc, char **argv)
 
 	if (startpos != InvalidXLogRecPtr  (do_create_slot || do_drop_slot))
 	{
-		fprintf(stderr, _(%s: cannot use --create or --drop together with --startpos\n), progname);
+		fprintf(stderr, _(%s: cannot use --create-slot or --drop-slot together with --startpos\n), progname);
 		fprintf(stderr, _(Try \%s --help\ for more information.\n),
 progname);
 		exit(1);

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To 

Re: [HACKERS] pg_receivexlog and replication slots

2014-10-02 Thread Michael Paquier
On Thu, Oct 2, 2014 at 12:44 AM, Andres Freund and...@2ndquadrant.com
wrote:

 I pushed the first part.

Thanks. Attached is a rebased version of patch 2, implementing the actual
feature. One thing I noticed with more testing is that if --create is used
and that the destination folder does not exist, pg_receivexlog was creating
the slot, and left with an error. This does not look user-friendly so I
changed the logic a bit to check for the destination folder before creating
any slot. This results in a bit of refactoring, but this way behavior is
more intuitive.
Regards,
-- 
Michael
From e3ee4b918b8122d414c03207fdd50f2472dc292e Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Mon, 1 Sep 2014 20:53:45 +0900
Subject: [PATCH] Support for replslot creation and drop in pg_receivexlog

Using the new actions --create and --drop that are similarly present
in pg_recvlogical, a user can respectively create and drop a replication
slot that can be used afterwards when fetching WALs.
---
 doc/src/sgml/ref/pg_receivexlog.sgml   |  29 +++
 src/bin/pg_basebackup/pg_receivexlog.c | 154 +
 2 files changed, 168 insertions(+), 15 deletions(-)

diff --git a/doc/src/sgml/ref/pg_receivexlog.sgml b/doc/src/sgml/ref/pg_receivexlog.sgml
index 5916b8f..69aa0af 100644
--- a/doc/src/sgml/ref/pg_receivexlog.sgml
+++ b/doc/src/sgml/ref/pg_receivexlog.sgml
@@ -72,6 +72,35 @@ PostgreSQL documentation
   titleOptions/title
 
para
+applicationpg_receivexlog/application can run in one of two following
+modes, which control physical replication slot:
+
+variablelist
+
+ varlistentry
+  termoption--create/option/term
+  listitem
+   para
+Create a new physical replication slot with the name specified in
+option--slot/option.
+   /para
+  /listitem
+ /varlistentry
+
+ varlistentry
+  termoption--drop/option/term
+  listitem
+   para
+Drop the replication slot with the name specified in
+option--slot/option, then exit.
+   /para
+  /listitem
+ /varlistentry
+/variablelist
+
+   /para
+
+   para
 The following command-line options control the location and format of the
 output.
 
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index 171cf43..8c4f752 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -38,11 +38,15 @@ static int	noloop = 0;
 static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
 static int	fsync_interval = 0; /* 0 = default */
 static volatile bool time_to_abort = false;
+static bool do_create_slot = false;
+static bool do_drop_slot = false;
 
 
 static void usage(void);
+static DIR* get_destination_dir(char *dest_folder);
+static void close_destination_dir(DIR *dest_dir, char *dest_folder);
 static XLogRecPtr FindStreamingStart(uint32 *tli);
-static void StreamLog();
+static void StreamLog(void);
 static bool stop_streaming(XLogRecPtr segendpos, uint32 timeline,
 			   bool segment_finished);
 
@@ -78,6 +82,9 @@ usage(void)
 	printf(_(  -w, --no-password  never prompt for password\n));
 	printf(_(  -W, --password force password prompt (should happen automatically)\n));
 	printf(_(  -S, --slot=SLOTNAMEreplication slot to use\n));
+	printf(_(\nOptional actions:\n));
+	printf(_(  --create   create a new replication slot (for the slot's name see --slot)\n));
+	printf(_(  --drop drop the replication slot (for the slot's name see --slot)\n));
 	printf(_(\nReport bugs to pgsql-b...@postgresql.org.\n));
 }
 
@@ -118,6 +125,44 @@ stop_streaming(XLogRecPtr xlogpos, uint32 timeline, bool segment_finished)
 	return false;
 }
 
+
+/*
+ * Get destination directory.
+ */
+static DIR*
+get_destination_dir(char *dest_folder)
+{
+	DIR *dir;
+
+	Assert(dest_folder != NULL);
+	dir = opendir(dest_folder);
+	if (dir == NULL)
+	{
+		fprintf(stderr, _(%s: could not open directory \%s\: %s\n),
+progname, basedir, strerror(errno));
+		disconnect_and_exit(1);
+	}
+
+	return dir;
+}
+
+
+/*
+ * Close existing directory.
+ */
+static void
+close_destination_dir(DIR *dest_dir, char *dest_folder)
+{
+	Assert(dest_dir != NULL  dest_folder != NULL);
+	if (closedir(dest_dir))
+	{
+		fprintf(stderr, _(%s: could not close directory \%s\: %s\n),
+progname, dest_folder, strerror(errno));
+		disconnect_and_exit(1);
+	}
+}
+
+
 /*
  * Determine starting location for streaming, based on any existing xlog
  * segments in the directory. We start at the end of the last one that is
@@ -134,13 +179,7 @@ FindStreamingStart(uint32 *tli)
 	uint32		high_tli = 0;
 	bool		high_ispartial = false;
 
-	dir = opendir(basedir);
-	if (dir == NULL)
-	{
-		fprintf(stderr, _(%s: could not open directory \%s\: %s\n),
-progname, basedir, strerror(errno));
-		disconnect_and_exit(1);
-	}
+	dir = get_destination_dir(basedir);
 
 	while (errno = 0, (dirent = 

Re: [HACKERS] pg_receivexlog and replication slots

2014-10-01 Thread Andres Freund
Hi,

I've split and edited patch 0001:
0001) Old WIP patch for pg_recvlogical tests. Useful for easier development.
0002) Copy editing that should be in 9.4
0003) Rebased patches of yours
0004) My changes to 0003 besides the rebase. This'll be squashed, but I
  thought it might be interesting for you.

I haven't tested my edits besides running 0001...

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services
From 45582de61f670967f72835cd14d1c520c3d04fce Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Wed, 1 Oct 2014 12:57:15 +0200
Subject: [PATCH 1/4] WIP: pg_recvlogical tests

---
 src/bin/pg_basebackup/Makefile|  3 ++
 src/bin/pg_basebackup/t/030_pg_recvlogical.pl | 45 +++
 2 files changed, 48 insertions(+)
 create mode 100644 src/bin/pg_basebackup/t/030_pg_recvlogical.pl

diff --git a/src/bin/pg_basebackup/Makefile b/src/bin/pg_basebackup/Makefile
index 90d19e7..3c0a14e 100644
--- a/src/bin/pg_basebackup/Makefile
+++ b/src/bin/pg_basebackup/Makefile
@@ -51,7 +51,10 @@ clean distclean maintainer-clean:
 	rm -rf tmp_check
 
 check: all
+# Needs test_decoding to run the pg_recvlogical tests
+	$(MAKE) -C $(top_builddir)/contrib/test_decoding DESTDIR='$(CURDIR)'/tmp_check/install install '$(CURDIR)'/tmp_check/log/install.log 21
 	$(prove_check)
 
 installcheck:
+# XXX: We rely on test_decoding already being installed here
 	$(prove_installcheck)
diff --git a/src/bin/pg_basebackup/t/030_pg_recvlogical.pl b/src/bin/pg_basebackup/t/030_pg_recvlogical.pl
new file mode 100644
index 000..1741319
--- /dev/null
+++ b/src/bin/pg_basebackup/t/030_pg_recvlogical.pl
@@ -0,0 +1,45 @@
+use strict;
+use warnings;
+use TestLib;
+use Test::More tests = 9;
+
+program_help_ok('pg_recvlogical');
+program_version_ok('pg_recvlogical');
+program_options_handling_ok('pg_recvlogical');
+
+
+my $tempdir = tempdir;
+start_test_server $tempdir;
+
+open HBA, $tempdir/pgdata/pg_hba.conf;
+print HBA local replication all trust\n;
+close HBA;
+system_or_bail 'pg_ctl', '-s', '-D', $tempdir/pgdata, 'reload';
+
+open HBA, $tempdir/pgdata/pg_hba.conf;
+print HBA local replication all trust\n;
+close HBA;
+
+open CONF, $tempdir/pgdata/postgresql.conf;
+print CONF max_wal_senders = 1\n;
+print CONF max_replication_slots = 2\n;
+print CONF wal_level = logical\n;
+close CONF;
+
+restart_test_server;
+
+command_fails([ 'pg_recvlogical', '-d', 'postgres', '--slot', 'regress', '--plugin', 'barf', '--create'],
+	   'pg recvlogical cannot create slot with nonexistant plugin');
+command_ok([ 'pg_recvlogical', '-d', 'postgres', '--slot', 'regress', '--create'],
+	   'pg recvlogical can create a slot');
+command_fails([ 'pg_recvlogical', '-d', 'postgres', '--slot', 'regress', '--create'],
+	   'pg recvlogical cannot create slot with a already used name');
+command_fails([ 'pg_recvlogical', '-d', 'postgres', '--slot', 'regress', '--create'],
+	   'pg recvlogical cannot create slot when all slots are in use');
+command_ok([ 'pg_recvlogical', '-d', 'postgres', '--slot', 'regress', '--drop'],
+	   'pg recvlogical can drop slot');
+command_fails([ 'pg_recvlogical', '-d', 'postgres', '--slot', 'regress', '--start'],
+	   'pg recvlogical cannot stream from nonexistant slot');
+
+# cannot easily test streaming actual changes because that goes on
+# forever.
-- 
1.8.3.251.g1462b67

From b9f73cef1a9dc1aa96ad1807893e78c924f690f6 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Mon, 29 Sep 2014 15:35:40 +0200
Subject: [PATCH 2/4] Message and comment policing of pg_recvlogical.c.

Several comments still referred to 'initiating', 'freeing', 'stopping'
replication slots. These were terms used during different phases of
the development of logical decoding, but are no long accurate.

Author: Michael Paquier

Backpatch to 9.4 where pg_recvlogical was introduced.
---
 src/bin/pg_basebackup/pg_recvlogical.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index f3b0f34..4144688 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -868,7 +868,7 @@ main(int argc, char **argv)
 
 
 	/*
-	 * stop a replication slot
+	 * drop a replication slot
 	 */
 	if (do_drop_slot)
 	{
@@ -876,7 +876,7 @@ main(int argc, char **argv)
 
 		if (verbose)
 			fprintf(stderr,
-	_(%s: freeing replication slot \%s\\n),
+	_(%s: dropping replication slot \%s\\n),
 	progname, replication_slot);
 
 		snprintf(query, sizeof(query), DROP_REPLICATION_SLOT \%s\,
@@ -892,8 +892,8 @@ main(int argc, char **argv)
 		if (PQntuples(res) != 0 || PQnfields(res) != 0)
 		{
 			fprintf(stderr,
-	_(%s: could not stop logical replication: got %d rows and %d fields, expected %d rows and %d fields\n),
-	progname, PQntuples(res), PQnfields(res), 0, 0);
+	

Re: [HACKERS] pg_receivexlog and replication slots

2014-10-01 Thread Michael Paquier
Thanks!

On Wed, Oct 1, 2014 at 8:38 PM, Andres Freund and...@2ndquadrant.com
wrote:

 0001) Old WIP patch for pg_recvlogical tests. Useful for easier
 development.

From where is this patch? Is it some rest from the time when pg_recvlogical
was developed?


 0002) Copy editing that should be in 9.4

Definitely makes sense for a backpatch.


 0003) Rebased patches of yours

0004) My changes to 0003 besides the rebase. This'll be squashed, but I
   thought it might be interesting for you.

(thanks for caring)
-/* Drop a replication slot */
+/* Drop a replication slot. */
I don't recall that comments on a single line have a dot even if this
single line is a complete sentence.

-static void StreamLog();
+static void StreamLogicalLog();
Not related at all to those patches, but for correctness it is better to
add a declaration (void).
Except those small things the changes look good to me.

Regards,
-- 
Michael


Re: [HACKERS] pg_receivexlog and replication slots

2014-10-01 Thread Andres Freund
On 2014-10-01 21:54:56 +0900, Michael Paquier wrote:
 Thanks!
 
 On Wed, Oct 1, 2014 at 8:38 PM, Andres Freund and...@2ndquadrant.com
 wrote:
 
  0001) Old WIP patch for pg_recvlogical tests. Useful for easier
  development.
 
 From where is this patch? Is it some rest from the time when pg_recvlogical
 was developed?
 
 
  0002) Copy editing that should be in 9.4
 
 Definitely makes sense for a backpatch.
 
 
  0003) Rebased patches of yours
 
 0004) My changes to 0003 besides the rebase. This'll be squashed, but I
thought it might be interesting for you.
 
 (thanks for caring)
 -/* Drop a replication slot */
 +/* Drop a replication slot. */
 I don't recall that comments on a single line have a dot even if this
 single line is a complete sentence.

Then it shouldn't start with a uppercase - which you changed...

 -static void StreamLog();
 +static void StreamLogicalLog();
 Not related at all to those patches, but for correctness it is better to
 add a declaration (void).

Agreed.

 Except those small things the changes look good to me.

Cool. Will push and then, sometime this week, review the next patch.

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] pg_receivexlog and replication slots

2014-10-01 Thread Alvaro Herrera
Andres Freund wrote:

 From d667f7a63cd62733d88ec5b7228dfd5d7136b9d7 Mon Sep 17 00:00:00 2001
 From: Michael Paquier mich...@otacoo.com
 Date: Mon, 1 Sep 2014 20:48:43 +0900
 Subject: [PATCH 3/4] Refactoring of pg_basebackup utilities
 
 Code duplication is reduced with the introduction of new APIs for each
 individual replication command:
 - IDENTIFY_SYSTEM
 - CREATE_REPLICATION_SLOT
 - DROP_REPLICATION_SLOT
 A couple of variables used to identify a timeline ID are changed as well
 to be more consistent with core code.
 ---
  src/bin/pg_basebackup/pg_basebackup.c  |  21 +
  src/bin/pg_basebackup/pg_receivexlog.c |  38 ++--
  src/bin/pg_basebackup/pg_recvlogical.c | 116 ++--
  src/bin/pg_basebackup/streamutil.c | 159 
 +
  src/bin/pg_basebackup/streamutil.h |  10 +++
  5 files changed, 207 insertions(+), 137 deletions(-)

Not objecting to any of this, but isn't it a bit funny that a patch that
aims to reduce duplication ends up with more lines than there were
originally?

-- 
Á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] pg_receivexlog and replication slots

2014-10-01 Thread Andres Freund
On 2014-10-01 11:21:12 -0300, Alvaro Herrera wrote:
 Andres Freund wrote:
 
  From d667f7a63cd62733d88ec5b7228dfd5d7136b9d7 Mon Sep 17 00:00:00 2001
  From: Michael Paquier mich...@otacoo.com
  Date: Mon, 1 Sep 2014 20:48:43 +0900
  Subject: [PATCH 3/4] Refactoring of pg_basebackup utilities
  
  Code duplication is reduced with the introduction of new APIs for each
  individual replication command:
  - IDENTIFY_SYSTEM
  - CREATE_REPLICATION_SLOT
  - DROP_REPLICATION_SLOT
  A couple of variables used to identify a timeline ID are changed as well
  to be more consistent with core code.
  ---
   src/bin/pg_basebackup/pg_basebackup.c  |  21 +
   src/bin/pg_basebackup/pg_receivexlog.c |  38 ++--
   src/bin/pg_basebackup/pg_recvlogical.c | 116 ++--
   src/bin/pg_basebackup/streamutil.c | 159 
  +
   src/bin/pg_basebackup/streamutil.h |  10 +++
   5 files changed, 207 insertions(+), 137 deletions(-)
 
 Not objecting to any of this, but isn't it a bit funny that a patch that
 aims to reduce duplication ends up with more lines than there were
 originally?

The reduction is there in combination with a later patch - which needs
most of the code moved to streamutil.c.

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] pg_receivexlog and replication slots

2014-10-01 Thread Andres Freund
Hi,

I pushed the first part.

On 2014-10-01 21:54:56 +0900, Michael Paquier wrote:
 On Wed, Oct 1, 2014 at 8:38 PM, Andres Freund and...@2ndquadrant.com
 wrote:
  0001) Old WIP patch for pg_recvlogical tests. Useful for easier
  development.
 From where is this patch? Is it some rest from the time when pg_recvlogical
 was developed?

IIRC I wrote it when looking at Peter's prove patches. There's a bit of
a problem with it because it requires test_decoding, a contrib module,
to be installed. That's easily solvable for 'make check', but less
clearly so for 'make installcheck'.

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] pg_receivexlog and replication slots

2014-10-01 Thread Jan Wieck

On 10/01/2014 08:59 AM, Andres Freund wrote:

On 2014-10-01 21:54:56 +0900, Michael Paquier wrote:

Thanks!

On Wed, Oct 1, 2014 at 8:38 PM, Andres Freund and...@2ndquadrant.com
wrote:

 0001) Old WIP patch for pg_recvlogical tests. Useful for easier
 development.

From where is this patch? Is it some rest from the time when pg_recvlogical
was developed?


 0002) Copy editing that should be in 9.4

Definitely makes sense for a backpatch.


 0003) Rebased patches of yours

0004) My changes to 0003 besides the rebase. This'll be squashed, but I
   thought it might be interesting for you.

(thanks for caring)
-/* Drop a replication slot */
+/* Drop a replication slot. */
I don't recall that comments on a single line have a dot even if this
single line is a complete sentence.


Then it shouldn't start with a uppercase - which you changed...


-static void StreamLog();
+static void StreamLogicalLog();
Not related at all to those patches, but for correctness it is better to
add a declaration (void).


Agreed.


Except those small things the changes look good to me.


Cool. Will push and then, sometime this week, review the next patch.

Greetings,

Andres Freund




You might want to do that function renaming in pg_receivexlog.c as well.


Jan

--
Jan Wieck
Senior Software Engineer
http://slony.info


--
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] pg_receivexlog and replication slots

2014-10-01 Thread Jan Wieck

On 10/01/2014 01:19 PM, Andres Freund wrote:

On 2014-10-01 13:17:17 -0400, Jan Wieck wrote:

-static void StreamLog();
+static void StreamLogicalLog();
Not related at all to those patches, but for correctness it is better to
add a declaration (void).

Agreed.

Except those small things the changes look good to me.

Cool. Will push and then, sometime this week, review the next patch.



You might want to do that function renaming in pg_receivexlog.c as well.


Why? You mean to StreamPhysicalLog()?

Greetings,

Andres Freund



Like that.

Plus, there is one more occurrence of StreamLog() in pg_recvlogical.c.


Jan

--
Jan Wieck
Senior Software Engineer
http://slony.info


--
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] pg_receivexlog and replication slots

2014-10-01 Thread Andres Freund
On 2014-10-01 13:22:53 -0400, Jan Wieck wrote:
 On 10/01/2014 01:19 PM, Andres Freund wrote:
 On 2014-10-01 13:17:17 -0400, Jan Wieck wrote:
 -static void StreamLog();
 +static void StreamLogicalLog();
 Not related at all to those patches, but for correctness it is better to
 add a declaration (void).
 
 Agreed.
 
 Except those small things the changes look good to me.
 
 Cool. Will push and then, sometime this week, review the next patch.
 
 You might want to do that function renaming in pg_receivexlog.c as well.
 
 Why? You mean to StreamPhysicalLog()?
 
 Like that.

Don't see that much point - it just seemed wrong to have StreamLog do
two somewhat different things. And StreamLog() in receivexlog is much older...

 Plus, there is one more occurrence of StreamLog() in pg_recvlogical.c.

Gah. Wrongly split commit :(. Fixed in 9.4, it's already fixed by the
subsequent commit in master.

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] pg_receivexlog and replication slots

2014-10-01 Thread Jan Wieck

On 10/01/2014 01:32 PM, Andres Freund wrote:

On 2014-10-01 13:22:53 -0400, Jan Wieck wrote:

On 10/01/2014 01:19 PM, Andres Freund wrote:
On 2014-10-01 13:17:17 -0400, Jan Wieck wrote:
-static void StreamLog();
+static void StreamLogicalLog();
Not related at all to those patches, but for correctness it is better to
add a declaration (void).

Agreed.

Except those small things the changes look good to me.

Cool. Will push and then, sometime this week, review the next patch.

You might want to do that function renaming in pg_receivexlog.c as well.

Why? You mean to StreamPhysicalLog()?

Like that.


Don't see that much point - it just seemed wrong to have StreamLog do
two somewhat different things. And StreamLog() in receivexlog is much older...


The point is that StreamLog is semantically a superset of StreamWhateverLog.


Jan





Plus, there is one more occurrence of StreamLog() in pg_recvlogical.c.


Gah. Wrongly split commit :(. Fixed in 9.4, it's already fixed by the
subsequent commit in master.

Greetings,

Andres Freund




--
Jan Wieck
Senior Software Engineer
http://slony.info


--
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] pg_receivexlog and replication slots

2014-09-29 Thread Andres Freund
On 2014-09-22 15:40:41 +0900, Michael Paquier wrote:
 Updated patches attached.

These are patches about pg_dump. I don't think these were the ones you
wanted to attach... :)

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] pg_receivexlog and replication slots

2014-09-29 Thread Michael Paquier
On Mon, Sep 29, 2014 at 9:42 PM, Andres Freund and...@2ndquadrant.com
wrote:

 On 2014-09-22 15:40:41 +0900, Michael Paquier wrote:
  Updated patches attached.

 These are patches about pg_dump. I don't think these were the ones you
 wanted to attach... :)

Wah, sorry (bakabakashii...). Here they are.
-- 
Michael
From 6f237d4ad74c1e9c699e6e4302afb4f75b8e16b9 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Mon, 1 Sep 2014 20:48:43 +0900
Subject: [PATCH 1/2] Refactoring of pg_basebackup utilities

Code duplication is reduced with the introduction of new APIs for each
individual replication command:
- IDENTIFY_SYSTEM
- CREATE_REPLICATION_SLOT
- DROP_REPLICATION_SLOT
A couple of variables used to identify a timeline ID are changed as well
to be more consistent with core code.
---
 src/bin/pg_basebackup/pg_basebackup.c  |  21 +
 src/bin/pg_basebackup/pg_receivexlog.c |  38 ++--
 src/bin/pg_basebackup/pg_recvlogical.c | 120 +++--
 src/bin/pg_basebackup/streamutil.c | 159 +
 src/bin/pg_basebackup/streamutil.h |  10 +++
 5 files changed, 209 insertions(+), 139 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8b9acea..0ebda9a 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1569,8 +1569,8 @@ BaseBackup(void)
 {
 	PGresult   *res;
 	char	   *sysidentifier;
-	uint32		latesttli;
-	uint32		starttli;
+	TimeLineID	latesttli;
+	TimeLineID	starttli;
 	char	   *basebkp;
 	char		escaped_label[MAXPGPATH];
 	char	   *maxrate_clause = NULL;
@@ -1624,23 +1624,8 @@ BaseBackup(void)
 	/*
 	 * Run IDENTIFY_SYSTEM so we can get the timeline
 	 */
-	res = PQexec(conn, IDENTIFY_SYSTEM);
-	if (PQresultStatus(res) != PGRES_TUPLES_OK)
-	{
-		fprintf(stderr, _(%s: could not send replication command \%s\: %s),
-progname, IDENTIFY_SYSTEM, PQerrorMessage(conn));
+	if (!RunIdentifySystem(conn, sysidentifier, latesttli, NULL, NULL))
 		disconnect_and_exit(1);
-	}
-	if (PQntuples(res) != 1 || PQnfields(res)  3)
-	{
-		fprintf(stderr,
-_(%s: could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields\n),
-progname, PQntuples(res), PQnfields(res), 1, 3);
-		disconnect_and_exit(1);
-	}
-	sysidentifier = pg_strdup(PQgetvalue(res, 0, 0));
-	latesttli = atoi(PQgetvalue(res, 0, 1));
-	PQclear(res);
 
 	/*
 	 * Start the actual backup
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index a8b9ad3..171cf43 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -253,13 +253,8 @@ FindStreamingStart(uint32 *tli)
 static void
 StreamLog(void)
 {
-	PGresult   *res;
-	XLogRecPtr	startpos;
-	uint32		starttli;
-	XLogRecPtr	serverpos;
-	uint32		servertli;
-	uint32		hi,
-lo;
+	XLogRecPtr	startpos, serverpos;
+	TimeLineID	starttli, servertli;
 
 	/*
 	 * Connect in replication mode to the server
@@ -280,33 +275,12 @@ StreamLog(void)
 	}
 
 	/*
-	 * Run IDENTIFY_SYSTEM so we can get the timeline and current xlog
-	 * position.
+	 * Identify server, obtaining start LSN position and current timeline ID
+	 * at the same time, necessary if not valid data can be found in the
+	 * existing output directory.
 	 */
-	res = PQexec(conn, IDENTIFY_SYSTEM);
-	if (PQresultStatus(res) != PGRES_TUPLES_OK)
-	{
-		fprintf(stderr, _(%s: could not send replication command \%s\: %s),
-progname, IDENTIFY_SYSTEM, PQerrorMessage(conn));
-		disconnect_and_exit(1);
-	}
-	if (PQntuples(res) != 1 || PQnfields(res)  3)
-	{
-		fprintf(stderr,
-_(%s: could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields\n),
-progname, PQntuples(res), PQnfields(res), 1, 3);
+	if (!RunIdentifySystem(conn, NULL, servertli, serverpos, NULL))
 		disconnect_and_exit(1);
-	}
-	servertli = atoi(PQgetvalue(res, 0, 1));
-	if (sscanf(PQgetvalue(res, 0, 2), %X/%X, hi, lo) != 2)
-	{
-		fprintf(stderr,
-_(%s: could not parse transaction log location \%s\\n),
-progname, PQgetvalue(res, 0, 2));
-		disconnect_and_exit(1);
-	}
-	serverpos = ((uint64) hi)  32 | lo;
-	PQclear(res);
 
 	/*
 	 * Figure out where to start streaming.
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index f3b0f34..23ae225 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -596,7 +596,6 @@ sighup_handler(int signum)
 int
 main(int argc, char **argv)
 {
-	PGresult   *res;
 	static struct option long_options[] = {
 /* general options */
 		{file, required_argument, NULL, 'f'},
@@ -628,6 +627,7 @@ main(int argc, char **argv)
 	int			option_index;
 	uint32		hi,
 lo;
+	char	   *db_name;
 
 	progname = get_progname(argv[0]);
 	set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN(pg_recvlogical));
@@ -834,121 +834,63 @@ main(int argc, char **argv)
 #endif
 
 	/*
-	 * don't 

Re: [HACKERS] pg_receivexlog and replication slots

2014-09-22 Thread Michael Paquier
On Mon, Sep 22, 2014 at 2:25 PM, Amit Kapila amit.kapil...@gmail.com wrote:
 On Sat, Sep 20, 2014 at 10:06 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 On Sat, Sep 20, 2014 at 7:09 AM, Amit Kapila amit.kapil...@gmail.com
 wrote:
  3.
  I find existing comments okay, is there a need to change
  in this case?  Part of the new comment mentions
  obtaining start LSN position, actually the start position is
  identified as part of next function call FindStreamingStart(),
  only incase it return InvalidXLogRecPtr, the value returned
  by IDENTIFY_SYSTEM will be used.
 Hopefully I am following you correctly here: comment has been updated
 to mention that the start LSN and TLI fetched from IDENTIFY_SYSTEM are
 always fetched but used only if no valid position is found in output
 folder of pg_receivexlog.

 Updated comment is consistent with code, however my main point
 was that in this case, I don't see much need to change existing
 comment.  I think this point is more related to each individual's
 perspective, so if you think there is a need to update the existing
 comment, then retain as it is in your patch and let Committer
 take a call about it.
Well, let's use your suggestion then and switch back to the former comment then.

  4.
  + /* Obtain a connection before doing anything */
  + conn = GetConnection();
  + if (!conn)
  + /* Error message already written in GetConnection() */
  Is there a reason for moving this function out of StreamLog(),
  there is no harm in moving it, but there doesn't seem to be any
  problem even if it is called inside StreamLog().
 For pg_recvlogical, this move is done to reduce code redundancy, and
 actually it makes sense to just grab one connection in the main() code
 path before performing any replication commands. For pg_receivexlog,
 the move is done because it makes the code more consistent with
 pg_recvlogical, and also it is a preparatory work for the second patch
 that introduces the create/drop slot. Even if 2nd patch is not
 accepted I figured out that it would not hurt to simply grab one
 connection in the main code path before doing any action.

 In pg_receivexlog, StreamLog() calls PQfinish() to close a connection
 to the backend and StreamLog() is getting called in while(true) loop,
 so if you just grab the connection once in main loop, the current
 logic won't work.  For same reasons, the current coding related to
 GetConnection() in pg_receivelogical seems to be right, so it is better
 not to change that as well.  For the second patch (that introduces the
 create/drop slot),  I think it is better to do in a way similar to what
 current pg_receivelogical does.
OK let's do so then. I changed back the GetConnection stuff back to
what is done on master. In the second patch, I added an extra call to
GetConnection before the create/drop commands.

  6.
  + /* Identify system, obtaining start LSN position at the same time */
  + if (!RunIdentifySystem(conn,
  NULL, NULL, startpos, plugin_name))
  + disconnect_and_exit(1);
  a.
  Generally IdentifySystem is called as first API, but I think you
  have changed its location after CreateReplicationSlot as you want
  to double check the value of plugin, not sure if that is necessary or
  important enough that we start calling it later.
 Funny part here is that even the current code on master and
 REL9_4_STABLE of pg_recvlogical.c is fetching a start position when
 creating a replication slot that is not used as two different actions
 cannot be used at the same time. That's a matter of removing this line
 in code though:
 startpos = ((uint64) hi)  32 | lo;

 User is not allowed to give startpos with drop or create of replication
 slot.  It is prevented by check:
 if (startpos != InvalidXLogRecPtr  (do_create_slot || do_drop_slot))
 So it seems you cannot remove the startpos assignment in code.
Ah yes true, it is actually possible to start the replication process
after creating the slot, so better not to remove it... I have updated
CreateReplicationSlot to add startpos in the fields that can be
requested from results.

 As that's only cosmetic for 9.4, and this patch makes things more
 correct I guess that's fine to do nothing and just try to get this
 patch in.

 In what sense do you think that this part of patch is better than
 current code?
I was trying to make the code a maximum consistent between the two
utilities, but your approach makes more sense.

 I think calling Identify_System as a first command makes sense
 (as it is in current code) because if that fails we should not
 proceed with execution of other command's.
Yes looking at that again you are right.

 Another point about refactoring patch is that fourth column in
 Identify_System is dbname and in patch, you are referring it as
 plugin which seems to be inconsistent.
Oops. Thanks for checking, I changed the check to have something for
the database name.

At the same time, I noticed an unnecessary limitation in the second
patch: we should be able to 

Re: [HACKERS] pg_receivexlog and replication slots

2014-09-21 Thread Amit Kapila
On Sat, Sep 20, 2014 at 10:06 PM, Michael Paquier michael.paqu...@gmail.com
wrote:
 On Sat, Sep 20, 2014 at 7:09 AM, Amit Kapila amit.kapil...@gmail.com
wrote:
  3.
  I find existing comments okay, is there a need to change
  in this case?  Part of the new comment mentions
  obtaining start LSN position, actually the start position is
  identified as part of next function call FindStreamingStart(),
  only incase it return InvalidXLogRecPtr, the value returned
  by IDENTIFY_SYSTEM will be used.
 Hopefully I am following you correctly here: comment has been updated
 to mention that the start LSN and TLI fetched from IDENTIFY_SYSTEM are
 always fetched but used only if no valid position is found in output
 folder of pg_receivexlog.

Updated comment is consistent with code, however my main point
was that in this case, I don't see much need to change existing
comment.  I think this point is more related to each individual's
perspective, so if you think there is a need to update the existing
comment, then retain as it is in your patch and let Committer
take a call about it.

  4.
  + /* Obtain a connection before doing anything */
  + conn = GetConnection();
  + if (!conn)
  + /* Error message already written in GetConnection() */
  Is there a reason for moving this function out of StreamLog(),
  there is no harm in moving it, but there doesn't seem to be any
  problem even if it is called inside StreamLog().
 For pg_recvlogical, this move is done to reduce code redundancy, and
 actually it makes sense to just grab one connection in the main() code
 path before performing any replication commands. For pg_receivexlog,
 the move is done because it makes the code more consistent with
 pg_recvlogical, and also it is a preparatory work for the second patch
 that introduces the create/drop slot. Even if 2nd patch is not
 accepted I figured out that it would not hurt to simply grab one
 connection in the main code path before doing any action.

In pg_receivexlog, StreamLog() calls PQfinish() to close a connection
to the backend and StreamLog() is getting called in while(true) loop,
so if you just grab the connection once in main loop, the current
logic won't work.  For same reasons, the current coding related to
GetConnection() in pg_receivelogical seems to be right, so it is better
not to change that as well.  For the second patch (that introduces the
create/drop slot),  I think it is better to do in a way similar to what
current pg_receivelogical does.


  6.
  + /* Identify system, obtaining start LSN position at the same time */
  + if (!RunIdentifySystem(conn,
  NULL, NULL, startpos, plugin_name))
  + disconnect_and_exit(1);
  a.
  Generally IdentifySystem is called as first API, but I think you
  have changed its location after CreateReplicationSlot as you want
  to double check the value of plugin, not sure if that is necessary or
  important enough that we start calling it later.
 Funny part here is that even the current code on master and
 REL9_4_STABLE of pg_recvlogical.c is fetching a start position when
 creating a replication slot that is not used as two different actions
 cannot be used at the same time. That's a matter of removing this line
 in code though:
 startpos = ((uint64) hi)  32 | lo;

User is not allowed to give startpos with drop or create of replication
slot.  It is prevented by check:
if (startpos != InvalidXLogRecPtr  (do_create_slot || do_drop_slot))
{
fprintf(stderr, _
(%s: cannot use --create or --drop together with --startpos\n), progname);
fprintf(stderr, _
(Try \%s --help\ for more information.\n),
progname);
exit(1);
}
So it seems you cannot remove the startpos assignment in code.

 As that's only cosmetic for 9.4, and this patch makes things more
 correct I guess that's fine to do nothing and just try to get this
 patch in.

In what sense do you think that this part of patch is better than
current code?
I think calling Identify_System as a first command makes sense
(as it is in current code) because if that fails we should not
proceed with execution of other command's.

Another point about refactoring patch is that fourth column in
Identify_System is dbname and in patch, you are referring it as
plugin which seems to be inconsistent.
Refer below code in patch:

RunIdentifySystem()
{
..
+ /* Get decoder plugin name, only available in 9.4 and newer versions */
+ if  (plugin != NULL)
+
*plugin = PQnfields(res)  3  !PQgetisnull(res, 0, 3) ?
+ pg_strdup(PQgetvalue
(res, 0, 3)) : (char *) NULL;
..
}

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Re: [HACKERS] pg_receivexlog and replication slots

2014-09-20 Thread Amit Kapila
On Wed, Sep 10, 2014 at 10:09 AM, Michael Paquier michael.paqu...@gmail.com
wrote:


 New patches taking into account all those comments are attached.

I have looked into refactoring related patch and would like
to share my observations with you:

1.
+ * Run IDENTIFY_SYSTEM through a given connection and give back to caller
+ * some result information if
requested:
+ * - Start LSN position
+ * - Current timeline ID
+ * - system identifier

This API also gets plugin if requested, so I think it is better
to mention the same in function header as well.

2.
+ /* Get LSN start position if necessary */
+ if (sscanf(PQgetvalue(res, 0, 2), %X/%X, hi, lo)
!= 2)
+ {
+ fprintf(stderr,
+ _(%s: could not parse transaction
log location \%s\\n),
+ progname, PQgetvalue(res, 0, 2));
+
return false;
+ }
+ if (startpos != NULL)
+ *startpos = ((uint64) hi)  32 | lo;

Isn't it better if we try to get the LSN position only incase
startpos!=NULL (as BaseBackup don't need this)

3.
/*
- * Run IDENTIFY_SYSTEM so we can get the timeline and current xlog
- * position.
+ * Identify
server, obtaining start LSN position and current timeline ID
+ * at the same time.
  */

I find existing comments okay, is there a need to change
in this case?  Part of the new comment mentions
obtaining start LSN position, actually the start position is
identified as part of next function call FindStreamingStart(),
only incase it return InvalidXLogRecPtr, the value returned
by IDENTIFY_SYSTEM will be used.

4.
  /*
  * Figure out where to start streaming.
@@ -492,6 +459,12 @@ main(int argc, char **argv)

pqsignal(SIGINT, sigint_handler);
 #endif

+ /* Obtain a connection before doing anything */
+ conn
= GetConnection();
+ if (!conn)
+ /* Error message already written in GetConnection() */
+
exit(1);
+

Is there a reason for moving this function out of StreamLog(),
there is no harm in moving it, but there doesn't seem to be any
problem even if it is called inside StreamLog().

5.
+bool
+RunIdentifySystem(PGconn *conn, char **sysid, TimeLineID *starttli,
+
XLogRecPtr *startpos, char **plugin)
+{
+ PGresult   *res;
+ uint32 hi, lo;
+
+ /* Leave if
no connection present */
+ if (conn == NULL)
+ return false;

Shouldn't there be Assert instead of if (conn == NULL), as
RunIdentifySystem() is not expected to be called without connection.

6.
main()
{
..

+ /* Identify system, obtaining start LSN position at the same time */
+ if (!RunIdentifySystem(conn,
NULL, NULL, startpos, plugin_name))
+ disconnect_and_exit(1);
+
+ /*
+ * Check that there
is a plugin name, a logical slot needs to have
+ * one absolutely.
+ */
+ if (!plugin_name)
+ {
+
fprintf(stderr,
+ _(%s: no plugin found for replication slot \%s
\\n),
+ progname, replication_slot);
+ disconnect_and_exit(1);
+
}
+
+ /* Stream loop */

a.
Generally IdentifySystem is called as first API, but I think you
have changed its location after CreateReplicationSlot as you want
to double check the value of plugin, not sure if that is necessary or
important enough that we start calling it later.
b.
Above call is trying to get startpos, won't it overwrite the value
passed by user (case 'I':) for startpos?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Re: [HACKERS] pg_receivexlog and replication slots

2014-09-20 Thread Michael Paquier
On Sat, Sep 20, 2014 at 7:09 AM, Amit Kapila amit.kapil...@gmail.com wrote:
 I have looked into refactoring related patch and would like
 to share my observations with you:
Thanks! Useful input is always welcome.

 1.
 + * Run IDENTIFY_SYSTEM through a given connection and give back to caller
 This API also gets plugin if requested, so I think it is better
 to mention the same in function header as well.
True.

 2.
 + /* Get LSN start position if necessary */
 Isn't it better if we try to get the LSN position only incase
 startpos!=NULL (as BaseBackup don't need this)
OK. Let's do that for consistency with the other fields.

 3.
 I find existing comments okay, is there a need to change
 in this case?  Part of the new comment mentions
 obtaining start LSN position, actually the start position is
 identified as part of next function call FindStreamingStart(),
 only incase it return InvalidXLogRecPtr, the value returned
 by IDENTIFY_SYSTEM will be used.
Hopefully I am following you correctly here: comment has been updated
to mention that the start LSN and TLI fetched from IDENTIFY_SYSTEM are
always fetched but used only if no valid position is found in output
folder of pg_receivexlog.

 4.
 + /* Obtain a connection before doing anything */
 + conn = GetConnection();
 + if (!conn)
 + /* Error message already written in GetConnection() */
 Is there a reason for moving this function out of StreamLog(),
 there is no harm in moving it, but there doesn't seem to be any
 problem even if it is called inside StreamLog().
For pg_recvlogical, this move is done to reduce code redundancy, and
actually it makes sense to just grab one connection in the main() code
path before performing any replication commands. For pg_receivexlog,
the move is done because it makes the code more consistent with
pg_recvlogical, and also it is a preparatory work for the second patch
that introduces the create/drop slot. Even if 2nd patch is not
accepted I figured out that it would not hurt to simply grab one
connection in the main code path before doing any action.

 5.
 Shouldn't there be Assert instead of if (conn == NULL), as
 RunIdentifySystem() is not expected to be called without connection.
Fine for me. That's indeed more consistent with the rest this way.

 6.
 + /* Identify system, obtaining start LSN position at the same time */
 + if (!RunIdentifySystem(conn,
 NULL, NULL, startpos, plugin_name))
 + disconnect_and_exit(1);
 a.
 Generally IdentifySystem is called as first API, but I think you
 have changed its location after CreateReplicationSlot as you want
 to double check the value of plugin, not sure if that is necessary or
 important enough that we start calling it later.
Funny part here is that even the current code on master and
REL9_4_STABLE of pg_recvlogical.c is fetching a start position when
creating a replication slot that is not used as two different actions
cannot be used at the same time. That's a matter of removing this line
in code though:
startpos = ((uint64) hi)  32 | lo;
As that's only cosmetic for 9.4, and this patch makes things more
correct I guess that's fine to do nothing and just try to get this
patch in.

 b.
 Above call is trying to get startpos, won't it overwrite the value
 passed by user (case 'I':) for startpos?
Yep, that's a bug. The value that user could give would have been
overwritten all the time. Current code does not even care about
checking startpos in IDENTIFY_SYSTEM so we're fine by just removing
this part.

Updated patches are attached.
-- 
Michael
From a7743619dc93870c815920872c3b5a6971c25714 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Mon, 1 Sep 2014 20:48:43 +0900
Subject: [PATCH 1/2] Refactoring of pg_basebackup utilities

Code duplication is reduced with the introduction of new APIs for each
individual replication command:
- IDENTIFY_SYSTEM
- CREATE_REPLICATION_SLOT
- DROP_REPLICATION_SLOT
A couple of variables used to identify a timeline ID are changed as well
to be more consistent with core code.
---
 src/bin/pg_basebackup/pg_basebackup.c  |  21 +
 src/bin/pg_basebackup/pg_receivexlog.c |  52 +++
 src/bin/pg_basebackup/pg_recvlogical.c | 132 +++-
 src/bin/pg_basebackup/streamutil.c | 155 +
 src/bin/pg_basebackup/streamutil.h |  10 +++
 5 files changed, 211 insertions(+), 159 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8b9acea..0ebda9a 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1569,8 +1569,8 @@ BaseBackup(void)
 {
 	PGresult   *res;
 	char	   *sysidentifier;
-	uint32		latesttli;
-	uint32		starttli;
+	TimeLineID	latesttli;
+	TimeLineID	starttli;
 	char	   *basebkp;
 	char		escaped_label[MAXPGPATH];
 	char	   *maxrate_clause = NULL;
@@ -1624,23 +1624,8 @@ BaseBackup(void)
 	/*
 	 * Run IDENTIFY_SYSTEM so we can get the timeline
 	 */
-	res = PQexec(conn, 

Re: [HACKERS] pg_receivexlog and replication slots

2014-09-09 Thread Michael Paquier
On Mon, Sep 8, 2014 at 8:50 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Wed, Sep 3, 2014 at 11:40 PM, Robert Haas robertmh...@gmail.com wrote:
 On Sun, Aug 31, 2014 at 9:45 AM, Magnus Hagander mag...@hagander.net wrote:
 Do we really want those Asserts? There is not a single Assert in
 bin/pg_basebackup today - as is the case for most things in bin/. We
 typically use regular if statements for things that can happen, and
 just ignore the others I think - since the callers are fairly simple
 to trace.

 I have no opinion on whether we want these particular Assert() calls,
 but I note that using Assert() in front-end code only became possible
 in February of 2013, as a result of commit
 e1d25de35a2b1f809e8f8d7b182ce0af004f3ec9.  So the lack of assertions
 there may not be so much because people thought it was a bad idea as
 that it didn't use to work.  Generally, I favor the use of Assert() in
 front-end code in the same scenarios in which we use it in back-end
 code: for checks that shouldn't burden production builds but are
 useful during development.

 Well that was exactly why they have been added first. The assertions
 have been placed in some functions to check for incompatible
 combinations of argument values when those functions are called. I
 don't mind re-adding them if people agree that they make sense. IMO
 they do and they help as well for the development of future utilities
 using replication protocol in src/bin/pg_basebackup as much as the
 refactoring work done on this thread.
Let's keep them then.

 We probably need to adjust the error message as well
 in that case, because it's no longer what's expected, it's what's
 required?
 OK, changed this way.
 The reason for the formulation of the current error message is that it's
 the same across all callsites emitting it to make it easier for
 translators. It used to be more specific at some point and then was
 changed. Since these aren't expected to be hit much I don't really see a
 need to be very detailed.
Re-changed back to that.

 Hm. I'd vote to simplify the code a bit based on the argument that the
 current API only looks at the 3 first columns and does not care about
 the 4th which is the plugin name.
 Why not return all four columns from RunIdentifySystem(), setting plugin
 to NULL if not available. Then the caller can error out.
This seems the cleaner approach, do did so.

New patches taking into account all those comments are attached.
Thanks for the feedback.
Regards,
-- 
Michael
From 6a864d1ef18a01e6680fceab181b78d266063200 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Mon, 1 Sep 2014 20:48:43 +0900
Subject: [PATCH 1/2] Refactoring of pg_basebackup utilities

Code duplication is reduced with the introduction of new APIs for each
individual replication command:
- IDENTIFY_SYSTEM
- CREATE_REPLICATION_SLOT
- DROP_REPLICATION_SLOT
A couple of variables used to identify a timeline ID are changed as well
to be more consistent with core code.
---
 src/bin/pg_basebackup/pg_basebackup.c  |  21 +
 src/bin/pg_basebackup/pg_receivexlog.c |  49 +++
 src/bin/pg_basebackup/pg_recvlogical.c | 132 +++-
 src/bin/pg_basebackup/streamutil.c | 153 +
 src/bin/pg_basebackup/streamutil.h |  10 +++
 5 files changed, 208 insertions(+), 157 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8b9acea..0ebda9a 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1569,8 +1569,8 @@ BaseBackup(void)
 {
 	PGresult   *res;
 	char	   *sysidentifier;
-	uint32		latesttli;
-	uint32		starttli;
+	TimeLineID	latesttli;
+	TimeLineID	starttli;
 	char	   *basebkp;
 	char		escaped_label[MAXPGPATH];
 	char	   *maxrate_clause = NULL;
@@ -1624,23 +1624,8 @@ BaseBackup(void)
 	/*
 	 * Run IDENTIFY_SYSTEM so we can get the timeline
 	 */
-	res = PQexec(conn, IDENTIFY_SYSTEM);
-	if (PQresultStatus(res) != PGRES_TUPLES_OK)
-	{
-		fprintf(stderr, _(%s: could not send replication command \%s\: %s),
-progname, IDENTIFY_SYSTEM, PQerrorMessage(conn));
+	if (!RunIdentifySystem(conn, sysidentifier, latesttli, NULL, NULL))
 		disconnect_and_exit(1);
-	}
-	if (PQntuples(res) != 1 || PQnfields(res)  3)
-	{
-		fprintf(stderr,
-_(%s: could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields\n),
-progname, PQntuples(res), PQnfields(res), 1, 3);
-		disconnect_and_exit(1);
-	}
-	sysidentifier = pg_strdup(PQgetvalue(res, 0, 0));
-	latesttli = atoi(PQgetvalue(res, 0, 1));
-	PQclear(res);
 
 	/*
 	 * Start the actual backup
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index a8b9ad3..cd7a41b 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -253,21 +253,10 @@ FindStreamingStart(uint32 *tli)
 static void
 StreamLog(void)
 {
-	PGresult   *res;
 	

Re: [HACKERS] pg_receivexlog and replication slots

2014-09-08 Thread Michael Paquier
On Wed, Sep 3, 2014 at 11:40 PM, Robert Haas robertmh...@gmail.com wrote:
 On Sun, Aug 31, 2014 at 9:45 AM, Magnus Hagander mag...@hagander.net wrote:
 Do we really want those Asserts? There is not a single Assert in
 bin/pg_basebackup today - as is the case for most things in bin/. We
 typically use regular if statements for things that can happen, and
 just ignore the others I think - since the callers are fairly simple
 to trace.

 I have no opinion on whether we want these particular Assert() calls,
 but I note that using Assert() in front-end code only became possible
 in February of 2013, as a result of commit
 e1d25de35a2b1f809e8f8d7b182ce0af004f3ec9.  So the lack of assertions
 there may not be so much because people thought it was a bad idea as
 that it didn't use to work.  Generally, I favor the use of Assert() in
 front-end code in the same scenarios in which we use it in back-end
 code: for checks that shouldn't burden production builds but are
 useful during development.

Well that was exactly why they have been added first. The assertions
have been placed in some functions to check for incompatible
combinations of argument values when those functions are called. I
don't mind re-adding them if people agree that they make sense. IMO
they do and they help as well for the development of future utilities
using replication protocol in src/bin/pg_basebackup as much as the
refactoring work done on this thread.
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] pg_receivexlog and replication slots

2014-09-03 Thread Robert Haas
On Sun, Aug 31, 2014 at 9:45 AM, Magnus Hagander mag...@hagander.net wrote:
 Do we really want those Asserts? There is not a single Assert in
 bin/pg_basebackup today - as is the case for most things in bin/. We
 typically use regular if statements for things that can happen, and
 just ignore the others I think - since the callers are fairly simple
 to trace.

I have no opinion on whether we want these particular Assert() calls,
but I note that using Assert() in front-end code only became possible
in February of 2013, as a result of commit
e1d25de35a2b1f809e8f8d7b182ce0af004f3ec9.  So the lack of assertions
there may not be so much because people thought it was a bad idea as
that it didn't use to work.  Generally, I favor the use of Assert() in
front-end code in the same scenarios in which we use it in back-end
code: for checks that shouldn't burden production builds but are
useful during development.

-- 
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] pg_receivexlog and replication slots

2014-09-01 Thread Michael Paquier
On Sun, Aug 31, 2014 at 10:45 PM, Magnus Hagander mag...@hagander.net wrote:
 As this is a number of patches rolled into one - do you happen to keep
 them separate in your local repo? If so can you send them as separate
 ones (refactor identify_system should be quite unrelated to supporting
 replication slots, right?), for easier review? (if not, I'll just
 split them apart mentally, but it's easier to review separately)
Thanks for your review!

OK, here are 2 patches, the 2nd needing the 1st one:
1) Refactor IDENTIFY_SYSTEM and replslot create/drop APIs
2) Support for --create and --drop in pg_receivexlog

 On the identify_system part - my understanding of the code is that
 what you pass in as num_cols is the number of columns required for it
 to work, right?
The argument is I would say cross-version compatibility and
consistency with the existing 9.4 code, but... (see below for the rest
of the story).

 We probably need to adjust the error message as well
 in that case, because it's no longer what's expected, it's what's
 required?
OK, changed this way.

 And we might want to include a hint about the reason (wrong version)?
I am not sure about that, a simple error message looks fine IMO, and
there is no notion of error hinting in the other client utilities as
well.

 There's also a note get LSN start position if necessary, but it
 tries to do it unconditionally. What is the if necessary supposed to
 refer to?
That's remnant of some old code, so I removed it. Thanks for spotting that.

 Actually - why do we even care about the 3 vs 4 in RunIdentifySystem,
 as it never actually looks at the 4th column anyway? If we do
 specifically want it to fail in the case of pg_recvlogical, we really
 need to think up a better error message for it, and perhaps a
 different way of specifying it?
Hm. I'd vote to simplify the code a bit based on the argument that the
current API only looks at the 3 first columns and does not care about
the 4th which is the plugin name.

 Do we really want those Asserts? There is not a single Assert in
 bin/pg_basebackup today - as is the case for most things in bin/. We
 typically use regular if statements for things that can happen, and
 just ignore the others I think - since the callers are fairly simple
 to trace.
OK, removed.

Regards,
-- 
Michael
From fdca8988480cac602157c3ae24ae61311bdaf960 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Mon, 1 Sep 2014 20:48:43 +0900
Subject: [PATCH 1/2] Refactoring of pg_basebackup utilities

Code duplication is reduced with the introduction of new APIs for each
individual replication command:
- IDENTIFY_SYSTEM
- CREATE_REPLICATION_SLOT
- DROP_REPLICATION_SLOT
A couple of variables used to identify a timeline ID are changed as well
to be more consistent with core code.
---
 src/bin/pg_basebackup/pg_basebackup.c  |  21 +
 src/bin/pg_basebackup/pg_receivexlog.c |  49 +++-
 src/bin/pg_basebackup/pg_recvlogical.c | 119 +--
 src/bin/pg_basebackup/streamutil.c | 142 +
 src/bin/pg_basebackup/streamutil.h |   9 +++
 5 files changed, 183 insertions(+), 157 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8b9acea..49675cf 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1569,8 +1569,8 @@ BaseBackup(void)
 {
 	PGresult   *res;
 	char	   *sysidentifier;
-	uint32		latesttli;
-	uint32		starttli;
+	TimeLineID	latesttli;
+	TimeLineID	starttli;
 	char	   *basebkp;
 	char		escaped_label[MAXPGPATH];
 	char	   *maxrate_clause = NULL;
@@ -1624,23 +1624,8 @@ BaseBackup(void)
 	/*
 	 * Run IDENTIFY_SYSTEM so we can get the timeline
 	 */
-	res = PQexec(conn, IDENTIFY_SYSTEM);
-	if (PQresultStatus(res) != PGRES_TUPLES_OK)
-	{
-		fprintf(stderr, _(%s: could not send replication command \%s\: %s),
-progname, IDENTIFY_SYSTEM, PQerrorMessage(conn));
+	if (!RunIdentifySystem(conn, sysidentifier, latesttli, NULL))
 		disconnect_and_exit(1);
-	}
-	if (PQntuples(res) != 1 || PQnfields(res)  3)
-	{
-		fprintf(stderr,
-_(%s: could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields\n),
-progname, PQntuples(res), PQnfields(res), 1, 3);
-		disconnect_and_exit(1);
-	}
-	sysidentifier = pg_strdup(PQgetvalue(res, 0, 0));
-	latesttli = atoi(PQgetvalue(res, 0, 1));
-	PQclear(res);
 
 	/*
 	 * Start the actual backup
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index a8b9ad3..f722374 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -253,21 +253,10 @@ FindStreamingStart(uint32 *tli)
 static void
 StreamLog(void)
 {
-	PGresult   *res;
 	XLogRecPtr	startpos;
-	uint32		starttli;
+	TimeLineID	starttli;
 	XLogRecPtr	serverpos;
-	uint32		servertli;
-	uint32		hi,
-lo;
-
-	/*
-	 * Connect in replication mode to the server
-	 */
-	conn = 

Re: [HACKERS] pg_receivexlog and replication slots

2014-09-01 Thread Andres Freund
On 2014-09-01 20:58:29 +0900, Michael Paquier wrote:
 On Sun, Aug 31, 2014 at 10:45 PM, Magnus Hagander mag...@hagander.net wrote:
  As this is a number of patches rolled into one - do you happen to keep
  them separate in your local repo? If so can you send them as separate
  ones (refactor identify_system should be quite unrelated to supporting
  replication slots, right?), for easier review? (if not, I'll just
  split them apart mentally, but it's easier to review separately)
 Thanks for your review!
 
 OK, here are 2 patches, the 2nd needing the 1st one:
 1) Refactor IDENTIFY_SYSTEM and replslot create/drop APIs
 2) Support for --create and --drop in pg_receivexlog
 
  On the identify_system part - my understanding of the code is that
  what you pass in as num_cols is the number of columns required for it
  to work, right?
 The argument is I would say cross-version compatibility and
 consistency with the existing 9.4 code, but... (see below for the rest
 of the story).

I don't really see a need to that for slot specific code. The locations
where we more lenient are ones where  9.4 is ok, or a better message is
following shortly afterwards.

  We probably need to adjust the error message as well
  in that case, because it's no longer what's expected, it's what's
  required?
 OK, changed this way.

The reason for the formulation of the current error message is that it's
the same across all callsites emitting it to make it easier for
translators. It used to be more specific at some point and then was
changed. Since these aren't expected to be hit much I don't really see a
need to be very detailed.

  And we might want to include a hint about the reason (wrong version)?
 I am not sure about that, a simple error message looks fine IMO, and
 there is no notion of error hinting in the other client utilities as
 well.

Agreed.

  Actually - why do we even care about the 3 vs 4 in RunIdentifySystem,
  as it never actually looks at the 4th column anyway? If we do
  specifically want it to fail in the case of pg_recvlogical, we really
  need to think up a better error message for it, and perhaps a
  different way of specifying it?

 Hm. I'd vote to simplify the code a bit based on the argument that the
 current API only looks at the 3 first columns and does not care about
 the 4th which is the plugin name.

Why not return all four columns from RunIdentifySystem(), setting plugin
to NULL if not available. Then the caller can error out.

Greetings,

Andres Freund


-- 
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] pg_receivexlog and replication slots

2014-08-31 Thread Magnus Hagander
On Tue, Aug 26, 2014 at 9:43 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Tue, Aug 19, 2014 at 2:49 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 On Mon, Aug 18, 2014 at 4:01 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 And now looking at your patch there is additional refactoring possible
 with IDENTIFY_SYSTEM and pg_basebackup as well...
 And attached is a rebased patch doing so.
 I reworked this patch a bit to take into account the fact that the
 number of columns to check after running IDENTIFY_SYSTEM is not always
 4 as pointed out by Andres:
 - pg_basebackup = 3
 - pg_receivexlog = 3
 - pg_recvlogical = 4

As this is a number of patches rolled into one - do you happen to keep
them separate in your local repo? If so can you send them as separate
ones (refactor identify_system should be quite unrelated to supporting
replication slots, right?), for easier review? (if not, I'll just
split them apart mentally, but it's easier to review separately)

On the identify_system part - my understanding of the code is that
what you pass in as num_cols is the number of columns required for it
to work, right? We probably need to adjust the error message as well
in that case, because it's no longer what's expected, it's what's
required? And we might want to include a hint about the reason
(wrong version)?

There's also a note get LSN start position if necessary, but it
tries to do it unconditionally. What is the if necessary supposed to
refer to?

Actually - why do we even care about the 3 vs 4 in RunIdentifySystem,
as it never actually looks at the 4th column anyway? If we do
specifically want it to fail in the case of pg_recvlogical, we really
need to think up a better error message for it, and perhaps a
different way of specifying it?

Do we really want those Asserts? There is not a single Assert in
bin/pg_basebackup today - as is the case for most things in bin/. We
typically use regular if statements for things that can happen, and
just ignore the others I think - since the callers are fairly simple
to trace.


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/


-- 
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] pg_receivexlog and replication slots

2014-08-26 Thread Michael Paquier
On Tue, Aug 19, 2014 at 2:49 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Mon, Aug 18, 2014 at 4:01 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 And now looking at your patch there is additional refactoring possible
 with IDENTIFY_SYSTEM and pg_basebackup as well...
 And attached is a rebased patch doing so.
I reworked this patch a bit to take into account the fact that the
number of columns to check after running IDENTIFY_SYSTEM is not always
4 as pointed out by Andres:
- pg_basebackup = 3
- pg_receivexlog = 3
- pg_recvlogical = 4

Regards,
-- 
Michael
From c54c987f34fbdc7d7cdf59af114a247029be532e Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Mon, 18 Aug 2014 14:32:44 +0900
Subject: [PATCH] Add support for physical slot creation/deletion in
 pg_receivexlog

Physical slot creation can be done with --create and drop with --drop.
In both cases --slot is needed. Code for replication slot creation and
drop is refactored with what was available in pg_recvlogical, the same
applied with IDENTIFY_SYSTEM to simplify the whole set.
---
 doc/src/sgml/ref/pg_receivexlog.sgml   |  29 +++
 src/bin/pg_basebackup/pg_basebackup.c  |  21 +
 src/bin/pg_basebackup/pg_receivexlog.c | 108 +++-
 src/bin/pg_basebackup/pg_recvlogical.c | 119 --
 src/bin/pg_basebackup/streamutil.c | 150 +
 src/bin/pg_basebackup/streamutil.h |  10 +++
 6 files changed, 279 insertions(+), 158 deletions(-)

diff --git a/doc/src/sgml/ref/pg_receivexlog.sgml b/doc/src/sgml/ref/pg_receivexlog.sgml
index 5916b8f..7e46005 100644
--- a/doc/src/sgml/ref/pg_receivexlog.sgml
+++ b/doc/src/sgml/ref/pg_receivexlog.sgml
@@ -72,6 +72,35 @@ PostgreSQL documentation
   titleOptions/title
 
para
+applicationpg_receivexlog/application can run in one of two following
+modes, which control physical replication slot:
+
+variablelist
+
+ varlistentry
+  termoption--create/option/term
+  listitem
+   para
+Create a new physical replication slot with the name specified in
+option--slot/option, then exit.
+   /para
+  /listitem
+ /varlistentry
+
+ varlistentry
+  termoption--drop/option/term
+  listitem
+   para
+Drop the replication slot with the name specified
+in option--slot/option, then exit.
+   /para
+  /listitem
+ /varlistentry
+/variablelist
+
+   /para
+
+   para
 The following command-line options control the location and format of the
 output.
 
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8b9acea..c5d9ec2 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1569,8 +1569,8 @@ BaseBackup(void)
 {
 	PGresult   *res;
 	char	   *sysidentifier;
-	uint32		latesttli;
-	uint32		starttli;
+	TimeLineID	latesttli;
+	TimeLineID	starttli;
 	char	   *basebkp;
 	char		escaped_label[MAXPGPATH];
 	char	   *maxrate_clause = NULL;
@@ -1624,23 +1624,8 @@ BaseBackup(void)
 	/*
 	 * Run IDENTIFY_SYSTEM so we can get the timeline
 	 */
-	res = PQexec(conn, IDENTIFY_SYSTEM);
-	if (PQresultStatus(res) != PGRES_TUPLES_OK)
-	{
-		fprintf(stderr, _(%s: could not send replication command \%s\: %s),
-progname, IDENTIFY_SYSTEM, PQerrorMessage(conn));
+	if (!RunIdentifySystem(conn, sysidentifier, latesttli, NULL, 3))
 		disconnect_and_exit(1);
-	}
-	if (PQntuples(res) != 1 || PQnfields(res)  3)
-	{
-		fprintf(stderr,
-_(%s: could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields\n),
-progname, PQntuples(res), PQnfields(res), 1, 3);
-		disconnect_and_exit(1);
-	}
-	sysidentifier = pg_strdup(PQgetvalue(res, 0, 0));
-	latesttli = atoi(PQgetvalue(res, 0, 1));
-	PQclear(res);
 
 	/*
 	 * Start the actual backup
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index a8b9ad3..a191c29 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -38,6 +38,8 @@ static int	noloop = 0;
 static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
 static int	fsync_interval = 0; /* 0 = default */
 static volatile bool time_to_abort = false;
+static bool do_create_slot = false;
+static bool do_drop_slot = false;
 
 
 static void usage(void);
@@ -78,6 +80,9 @@ usage(void)
 	printf(_(  -w, --no-password  never prompt for password\n));
 	printf(_(  -W, --password force password prompt (should happen automatically)\n));
 	printf(_(  -S, --slot=SLOTNAMEreplication slot to use\n));
+	printf(_(\nOptional actions:\n));
+	printf(_(  --create   create a new replication slot (for the slot's name see --slot)\n));
+	printf(_(  --drop 

Re: [HACKERS] pg_receivexlog and replication slots

2014-08-19 Thread Fujii Masao
On Mon, Aug 18, 2014 at 4:01 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
 check was done but in 9.4 this command returns 4 fields:
 (PQntuples(res) != 1 || PQnfields(res)  3)
 That's not directly related to this patch, but making some corrections
 is not going to hurt..

 Good catch! I found that libpqwalreceiver.c, etc have the same problem.
 It's better to fix this separately. Patch attached.
 Patch looks good to me.

Okay, applied!

 Once you push that I'll rebase the stuff on
 this thread once again, that's going to conflict for sure. And now
 looking at your patch there is additional refactoring possible with
 IDENTIFY_SYSTEM and pg_basebackup as well...

Yep, that's possible. But since the patch needs to be back-patch to 9.4,
I didn't do the refactoring.

Regards,

-- 
Fujii Masao


-- 
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] pg_receivexlog and replication slots

2014-08-19 Thread Andres Freund
On 2014-08-18 14:38:06 +0900, Michael Paquier wrote:
 - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
 check was done but in 9.4 this command returns 4 fields:
 (PQntuples(res) != 1 || PQnfields(res)  3)

Which is correct. We don't want to error out in the case where 3 columns
are returned because that'd unnecessarily break compatibility with 
9.4. Previously that check was != 3...

This isn't a bug.

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] pg_receivexlog and replication slots

2014-08-19 Thread Fujii Masao
On Tue, Aug 19, 2014 at 5:52 PM, Andres Freund and...@2ndquadrant.com wrote:
 On 2014-08-18 14:38:06 +0900, Michael Paquier wrote:
 - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
 check was done but in 9.4 this command returns 4 fields:
 (PQntuples(res) != 1 || PQnfields(res)  3)

 Which is correct. We don't want to error out in the case where 3 columns
 are returned because that'd unnecessarily break compatibility with 
 9.4. Previously that check was != 3...

 This isn't a bug.

Okay, I understood why you didn't update those codes.

Since we don't allow replication between different major versions,
it's better to apply this change at least into libpqwalreceiver.c. Thought?

Regards,

-- 
Fujii Masao


-- 
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] pg_receivexlog and replication slots

2014-08-19 Thread Andres Freund
On 2014-08-19 18:02:32 +0900, Fujii Masao wrote:
 On Tue, Aug 19, 2014 at 5:52 PM, Andres Freund and...@2ndquadrant.com wrote:
  On 2014-08-18 14:38:06 +0900, Michael Paquier wrote:
  - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
  check was done but in 9.4 this command returns 4 fields:
  (PQntuples(res) != 1 || PQnfields(res)  3)
 
  Which is correct. We don't want to error out in the case where 3 columns
  are returned because that'd unnecessarily break compatibility with 
  9.4. Previously that check was != 3...
 
  This isn't a bug.
 
 Okay, I understood why you didn't update those codes.
 
 Since we don't allow replication between different major versions,
 it's better to apply this change at least into libpqwalreceiver.c. Thought?

We'd discussed that we'd rather keep it consistent. It also results in a
more explanatory error message lateron.

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] pg_receivexlog and replication slots

2014-08-19 Thread Fujii Masao
On Tue, Aug 19, 2014 at 6:03 PM, Andres Freund and...@2ndquadrant.com wrote:
 On 2014-08-19 18:02:32 +0900, Fujii Masao wrote:
 On Tue, Aug 19, 2014 at 5:52 PM, Andres Freund and...@2ndquadrant.com 
 wrote:
  On 2014-08-18 14:38:06 +0900, Michael Paquier wrote:
  - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
  check was done but in 9.4 this command returns 4 fields:
  (PQntuples(res) != 1 || PQnfields(res)  3)
 
  Which is correct. We don't want to error out in the case where 3 columns
  are returned because that'd unnecessarily break compatibility with 
  9.4. Previously that check was != 3...
 
  This isn't a bug.

 Okay, I understood why you didn't update those codes.

 Since we don't allow replication between different major versions,
 it's better to apply this change at least into libpqwalreceiver.c. Thought?

 We'd discussed that we'd rather keep it consistent. It also results in a
 more explanatory error message lateron.

Hmm... okay, will revert the commit.

Regards,

-- 
Fujii Masao


-- 
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] pg_receivexlog and replication slots

2014-08-18 Thread Fujii Masao
On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Fri, Aug 15, 2014 at 5:17 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 Thanks for your review.

 On Fri, Aug 15, 2014 at 12:56 AM,  furu...@pm.nttdata.co.jp wrote:
 At consistency with pg_recvlogical, do you think about --start?
 I did not add that for the sake of backward-compatibility as in
 pg_recvlogical an action is mandatory. It is not the case now of
 pg_receivexlog.

 [postgres postgresql-6aa6158]$ make  /dev/null
 streamutil.c: In function 'CreateReplicationSlot':
 streamutil.c:244: warning: suggest parentheses around '' within '||'
 I see. Here is a rebased patch.

 Looking more at the code, IDENTIFY_SYSTEM management can be unified
 into a single function. So I have written a newer version of the patch
 grouping IDENTIFY_SYSTEM call into a single function for both
 utilities, fixing at the same time a couple of other issues:
 - correct use of TimelineID in code
 - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
 check was done but in 9.4 this command returns 4 fields:
 (PQntuples(res) != 1 || PQnfields(res)  3)
 That's not directly related to this patch, but making some corrections
 is not going to hurt..

Good catch! I found that libpqwalreceiver.c, etc have the same problem.
It's better to fix this separately. Patch attached.

Regards,

-- 
Fujii Masao
*** a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
--- b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
***
*** 131,137  libpqrcv_identify_system(TimeLineID *primary_tli)
  		the primary server: %s,
  		PQerrorMessage(streamConn;
  	}
! 	if (PQnfields(res)  3 || PQntuples(res) != 1)
  	{
  		int			ntuples = PQntuples(res);
  		int			nfields = PQnfields(res);
--- 131,137 
  		the primary server: %s,
  		PQerrorMessage(streamConn;
  	}
! 	if (PQnfields(res)  4 || PQntuples(res) != 1)
  	{
  		int			ntuples = PQntuples(res);
  		int			nfields = PQnfields(res);
***
*** 140,146  libpqrcv_identify_system(TimeLineID *primary_tli)
  		ereport(ERROR,
  (errmsg(invalid response from primary server),
   errdetail(Could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields.,
! 		   ntuples, nfields, 3, 1)));
  	}
  	primary_sysid = PQgetvalue(res, 0, 0);
  	*primary_tli = pg_atoi(PQgetvalue(res, 0, 1), 4, 0);
--- 140,146 
  		ereport(ERROR,
  (errmsg(invalid response from primary server),
   errdetail(Could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields.,
! 		   ntuples, nfields, 4, 1)));
  	}
  	primary_sysid = PQgetvalue(res, 0, 0);
  	*primary_tli = pg_atoi(PQgetvalue(res, 0, 1), 4, 0);
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
***
*** 1644,1654  BaseBackup(void)
  progname, IDENTIFY_SYSTEM, PQerrorMessage(conn));
  		disconnect_and_exit(1);
  	}
! 	if (PQntuples(res) != 1 || PQnfields(res)  3)
  	{
  		fprintf(stderr,
  _(%s: could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields\n),
! progname, PQntuples(res), PQnfields(res), 1, 3);
  		disconnect_and_exit(1);
  	}
  	sysidentifier = pg_strdup(PQgetvalue(res, 0, 0));
--- 1644,1654 
  progname, IDENTIFY_SYSTEM, PQerrorMessage(conn));
  		disconnect_and_exit(1);
  	}
! 	if (PQntuples(res) != 1 || PQnfields(res)  4)
  	{
  		fprintf(stderr,
  _(%s: could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields\n),
! progname, PQntuples(res), PQnfields(res), 1, 4);
  		disconnect_and_exit(1);
  	}
  	sysidentifier = pg_strdup(PQgetvalue(res, 0, 0));
*** a/src/bin/pg_basebackup/pg_receivexlog.c
--- b/src/bin/pg_basebackup/pg_receivexlog.c
***
*** 290,300  StreamLog(void)
  progname, IDENTIFY_SYSTEM, PQerrorMessage(conn));
  		disconnect_and_exit(1);
  	}
! 	if (PQntuples(res) != 1 || PQnfields(res)  3)
  	{
  		fprintf(stderr,
  _(%s: could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields\n),
! progname, PQntuples(res), PQnfields(res), 1, 3);
  		disconnect_and_exit(1);
  	}
  	servertli = atoi(PQgetvalue(res, 0, 1));
--- 290,300 
  progname, IDENTIFY_SYSTEM, PQerrorMessage(conn));
  		disconnect_and_exit(1);
  	}
! 	if (PQntuples(res) != 1 || PQnfields(res)  4)
  	{
  		fprintf(stderr,
  _(%s: could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields\n),
! progname, PQntuples(res), PQnfields(res), 1, 4);
  		disconnect_and_exit(1);
  	}
  	servertli = atoi(PQgetvalue(res, 0, 1));
*** a/src/bin/pg_basebackup/receivelog.c
--- b/src/bin/pg_basebackup/receivelog.c
***
*** 499,509  ReceiveXlogStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline,
  			PQclear(res);
  			return false;
  		}
! 		if (PQntuples(res) != 1 || 

Re: [HACKERS] pg_receivexlog and replication slots

2014-08-18 Thread Fujii Masao
On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 On Fri, Aug 15, 2014 at 5:17 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 Thanks for your review.

 On Fri, Aug 15, 2014 at 12:56 AM,  furu...@pm.nttdata.co.jp wrote:
 At consistency with pg_recvlogical, do you think about --start?
 I did not add that for the sake of backward-compatibility as in
 pg_recvlogical an action is mandatory. It is not the case now of
 pg_receivexlog.

 [postgres postgresql-6aa6158]$ make  /dev/null
 streamutil.c: In function 'CreateReplicationSlot':
 streamutil.c:244: warning: suggest parentheses around '' within '||'
 I see. Here is a rebased patch.

 Looking more at the code, IDENTIFY_SYSTEM management can be unified
 into a single function. So I have written a newer version of the patch
 grouping IDENTIFY_SYSTEM call into a single function for both
 utilities, fixing at the same time a couple of other issues:
 - correct use of TimelineID in code
 - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
 check was done but in 9.4 this command returns 4 fields:
 (PQntuples(res) != 1 || PQnfields(res)  3)
 That's not directly related to this patch, but making some corrections
 is not going to hurt..

 Good catch! I found that libpqwalreceiver.c, etc have the same problem.
 It's better to fix this separately. Patch attached.

BTW, the name of the forth column in IDENTIFY_SYSETM is dbname. Maybe I don't
like this name because we usually use datname as the name of column which
identify the database name. For example, pg_database, pg_stat_activity, etc use
datname.

Regards,

-- 
Fujii Masao


-- 
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] pg_receivexlog and replication slots

2014-08-18 Thread Michael Paquier
On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 - IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
 check was done but in 9.4 this command returns 4 fields:
 (PQntuples(res) != 1 || PQnfields(res)  3)
 That's not directly related to this patch, but making some corrections
 is not going to hurt..

 Good catch! I found that libpqwalreceiver.c, etc have the same problem.
 It's better to fix this separately. Patch attached.
Patch looks good to me. Once you push that I'll rebase the stuff on
this thread once again, that's going to conflict for sure. And now
looking at your patch there is additional refactoring possible with
IDENTIFY_SYSTEM and pg_basebackup as well...
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] pg_receivexlog and replication slots

2014-08-18 Thread Michael Paquier
On Mon, Aug 18, 2014 at 4:01 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 And now looking at your patch there is additional refactoring possible
 with IDENTIFY_SYSTEM and pg_basebackup as well...
And attached is a rebased patch doing so.
Regards,
-- 
Michael
From a75def1dc554023d94611887410926d779596749 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Mon, 18 Aug 2014 14:32:44 +0900
Subject: [PATCH] Add support for physical slot creation/deletion in
 pg_receivexlog

Physical slot creation can be done with --create and drop with --drop.
In both cases --slot is needed. Code for replication slot creation and
drop is refactored with what was available in pg_recvlogical, the same
applies with IDENTIFY_SYSTEM to simplify the whole set, including
pg_basebackup in bonus.
---
 doc/src/sgml/ref/pg_receivexlog.sgml   |  29 +++
 src/bin/pg_basebackup/pg_basebackup.c  |  21 +
 src/bin/pg_basebackup/pg_receivexlog.c | 108 +++-
 src/bin/pg_basebackup/pg_recvlogical.c | 119 --
 src/bin/pg_basebackup/streamutil.c | 148 +
 src/bin/pg_basebackup/streamutil.h |   9 ++
 6 files changed, 276 insertions(+), 158 deletions(-)

diff --git a/doc/src/sgml/ref/pg_receivexlog.sgml b/doc/src/sgml/ref/pg_receivexlog.sgml
index 5916b8f..7e46005 100644
--- a/doc/src/sgml/ref/pg_receivexlog.sgml
+++ b/doc/src/sgml/ref/pg_receivexlog.sgml
@@ -72,6 +72,35 @@ PostgreSQL documentation
   titleOptions/title
 
para
+applicationpg_receivexlog/application can run in one of two following
+modes, which control physical replication slot:
+
+variablelist
+
+ varlistentry
+  termoption--create/option/term
+  listitem
+   para
+Create a new physical replication slot with the name specified in
+option--slot/option, then exit.
+   /para
+  /listitem
+ /varlistentry
+
+ varlistentry
+  termoption--drop/option/term
+  listitem
+   para
+Drop the replication slot with the name specified
+in option--slot/option, then exit.
+   /para
+  /listitem
+ /varlistentry
+/variablelist
+
+   /para
+
+   para
 The following command-line options control the location and format of the
 output.
 
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 3d26e22..66c851b 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1582,8 +1582,8 @@ BaseBackup(void)
 {
 	PGresult   *res;
 	char	   *sysidentifier;
-	uint32		latesttli;
-	uint32		starttli;
+	TimeLineID	latesttli;
+	TimeLineID	starttli;
 	char	   *basebkp;
 	char		escaped_label[MAXPGPATH];
 	char	   *maxrate_clause = NULL;
@@ -1637,23 +1637,8 @@ BaseBackup(void)
 	/*
 	 * Run IDENTIFY_SYSTEM so we can get the timeline
 	 */
-	res = PQexec(conn, IDENTIFY_SYSTEM);
-	if (PQresultStatus(res) != PGRES_TUPLES_OK)
-	{
-		fprintf(stderr, _(%s: could not send replication command \%s\: %s),
-progname, IDENTIFY_SYSTEM, PQerrorMessage(conn));
+	if (!RunIdentifySystem(conn, sysidentifier, latesttli, NULL))
 		disconnect_and_exit(1);
-	}
-	if (PQntuples(res) != 1 || PQnfields(res)  3)
-	{
-		fprintf(stderr,
-_(%s: could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields\n),
-progname, PQntuples(res), PQnfields(res), 1, 3);
-		disconnect_and_exit(1);
-	}
-	sysidentifier = pg_strdup(PQgetvalue(res, 0, 0));
-	latesttli = atoi(PQgetvalue(res, 0, 1));
-	PQclear(res);
 
 	/*
 	 * Start the actual backup
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index a8b9ad3..e87839a 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -38,6 +38,8 @@ static int	noloop = 0;
 static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
 static int	fsync_interval = 0; /* 0 = default */
 static volatile bool time_to_abort = false;
+static bool do_create_slot = false;
+static bool do_drop_slot = false;
 
 
 static void usage(void);
@@ -78,6 +80,9 @@ usage(void)
 	printf(_(  -w, --no-password  never prompt for password\n));
 	printf(_(  -W, --password force password prompt (should happen automatically)\n));
 	printf(_(  -S, --slot=SLOTNAMEreplication slot to use\n));
+	printf(_(\nOptional actions:\n));
+	printf(_(  --create   create a new replication slot (for the slot's name see --slot)\n));
+	printf(_(  --drop drop the replication slot (for the slot's name see --slot)\n));
 	printf(_(\nReport bugs to pgsql-b...@postgresql.org.\n));
 }
 
@@ -253,21 +258,10 @@ FindStreamingStart(uint32 *tli)
 static void
 StreamLog(void)
 {
-	PGresult   *res;
 	XLogRecPtr	startpos;
-	uint32		starttli;
+	

Re: [HACKERS] pg_receivexlog and replication slots

2014-08-17 Thread Michael Paquier
On Fri, Aug 15, 2014 at 5:17 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 Thanks for your review.

 On Fri, Aug 15, 2014 at 12:56 AM,  furu...@pm.nttdata.co.jp wrote:
 At consistency with pg_recvlogical, do you think about --start?
 I did not add that for the sake of backward-compatibility as in
 pg_recvlogical an action is mandatory. It is not the case now of
 pg_receivexlog.

 [postgres postgresql-6aa6158]$ make  /dev/null
 streamutil.c: In function 'CreateReplicationSlot':
 streamutil.c:244: warning: suggest parentheses around '' within '||'
 I see. Here is a rebased patch.

Looking more at the code, IDENTIFY_SYSTEM management can be unified
into a single function. So I have written a newer version of the patch
grouping IDENTIFY_SYSTEM call into a single function for both
utilities, fixing at the same time a couple of other issues:
- correct use of TimelineID in code
- IDENTIFY_SYSTEM checks were incorrect (even in HEAD). The following
check was done but in 9.4 this command returns 4 fields:
(PQntuples(res) != 1 || PQnfields(res)  3)
That's not directly related to this patch, but making some corrections
is not going to hurt..

Regards,
-- 
Michael
From 160b00e595dab739aa4da7d18a81aff38d0a837f Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Mon, 18 Aug 2014 14:32:44 +0900
Subject: [PATCH] Add support for physical slot creation/deletion in
 pg_receivexlog

Physical slot creation can be done with --create and drop with --drop.
In both cases --slot is needed. Code for replication slot creation and
drop is refactored with what was available in pg_recvlogical, the same
applied with IDENTIFY_SYSTEM to simplify the whole set.
---
 doc/src/sgml/ref/pg_receivexlog.sgml   |  29 +++
 src/bin/pg_basebackup/pg_receivexlog.c | 108 -
 src/bin/pg_basebackup/pg_recvlogical.c | 119 +---
 src/bin/pg_basebackup/streamutil.c | 140 +
 src/bin/pg_basebackup/streamutil.h |   9 +++
 5 files changed, 265 insertions(+), 140 deletions(-)

diff --git a/doc/src/sgml/ref/pg_receivexlog.sgml b/doc/src/sgml/ref/pg_receivexlog.sgml
index 5916b8f..7e46005 100644
--- a/doc/src/sgml/ref/pg_receivexlog.sgml
+++ b/doc/src/sgml/ref/pg_receivexlog.sgml
@@ -72,6 +72,35 @@ PostgreSQL documentation
   titleOptions/title
 
para
+applicationpg_receivexlog/application can run in one of two following
+modes, which control physical replication slot:
+
+variablelist
+
+ varlistentry
+  termoption--create/option/term
+  listitem
+   para
+Create a new physical replication slot with the name specified in
+option--slot/option, then exit.
+   /para
+  /listitem
+ /varlistentry
+
+ varlistentry
+  termoption--drop/option/term
+  listitem
+   para
+Drop the replication slot with the name specified
+in option--slot/option, then exit.
+   /para
+  /listitem
+ /varlistentry
+/variablelist
+
+   /para
+
+   para
 The following command-line options control the location and format of the
 output.
 
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index a8b9ad3..a56cd9e 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -38,6 +38,8 @@ static int	noloop = 0;
 static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
 static int	fsync_interval = 0; /* 0 = default */
 static volatile bool time_to_abort = false;
+static bool do_create_slot = false;
+static bool do_drop_slot = false;
 
 
 static void usage(void);
@@ -78,6 +80,9 @@ usage(void)
 	printf(_(  -w, --no-password  never prompt for password\n));
 	printf(_(  -W, --password force password prompt (should happen automatically)\n));
 	printf(_(  -S, --slot=SLOTNAMEreplication slot to use\n));
+	printf(_(\nOptional actions:\n));
+	printf(_(  --create   create a new replication slot (for the slot's name see --slot)\n));
+	printf(_(  --drop drop the replication slot (for the slot's name see --slot)\n));
 	printf(_(\nReport bugs to pgsql-b...@postgresql.org.\n));
 }
 
@@ -253,21 +258,10 @@ FindStreamingStart(uint32 *tli)
 static void
 StreamLog(void)
 {
-	PGresult   *res;
 	XLogRecPtr	startpos;
-	uint32		starttli;
+	TimeLineID	starttli;
 	XLogRecPtr	serverpos;
-	uint32		servertli;
-	uint32		hi,
-lo;
-
-	/*
-	 * Connect in replication mode to the server
-	 */
-	conn = GetConnection();
-	if (!conn)
-		/* Error message already written in GetConnection() */
-		return;
+	TimeLineID	servertli;
 
 	if (!CheckServerVersionForStreaming(conn))
 	{
@@ -280,33 +274,11 @@ StreamLog(void)
 	}
 
 	/*
-	 * Run IDENTIFY_SYSTEM so we can get the timeline and current xlog
-	 * position.
+	 * Identify server, obtaining start LSN position and current timeline ID
+	 * at the same time.
 	 */
-	res = PQexec(conn, IDENTIFY_SYSTEM);
-	if 

Re: [HACKERS] pg_receivexlog and replication slots

2014-08-15 Thread furuyao
 Actually I came up with the same need as Magnus, but a bit later, so...
 Attached is a patch to support physical slot creation and drop in
 pg_receivexlog with the addition of new options --create and --drop. It
 would be nice to have that in 9.4, but I will not the one deciding that
 at the end :) Code has been refactored with what is already available
 in pg_recvlogical for the slot creation and drop.

I think that create/drop options of a slot is unnecessary. 
It is not different from performing pg_create_physical_replication_slot() by 
psql. 

At consistency with pg_recvlogical, do you think about --start?

Initial review.

The patch was not able to be applied to the source file at this morning time. 

commit-id:5ff5bfb5f0d83a538766903275b230499fa9ebe1

[postgres postgresql-5ff5bfb]$ patch -p1  
20140812_physical_slot_receivexlog.patch
patching file doc/src/sgml/ref/pg_receivexlog.sgml
patching file src/bin/pg_basebackup/pg_receivexlog.c
Hunk #2 FAILED at 80.
1 out of 7 hunks FAILED -- saving rejects to file 
src/bin/pg_basebackup/pg_receivexlog.c.rej
patching file src/bin/pg_basebackup/pg_recvlogical.c
patching file src/bin/pg_basebackup/streamutil.c
patching file src/bin/pg_basebackup/streamutil.h

The patch was applied to the source file before August 12. 
warning comes out then make.

commit-id:6aa61580e08d58909b2a8845a4087b7699335ee0

[postgres postgresql-6aa6158]$ make  /dev/null
streamutil.c: In function ‘CreateReplicationSlot’:
streamutil.c:244: warning: suggest parentheses around ‘’ within ‘||’

Regards,

--
Furuya Osamu


-- 
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] pg_receivexlog and replication slots

2014-08-15 Thread Michael Paquier
Thanks for your review.

On Fri, Aug 15, 2014 at 12:56 AM,  furu...@pm.nttdata.co.jp wrote:
 At consistency with pg_recvlogical, do you think about --start?
I did not add that for the sake of backward-compatibility as in
pg_recvlogical an action is mandatory. It is not the case now of
pg_receivexlog.

 [postgres postgresql-6aa6158]$ make  /dev/null
 streamutil.c: In function 'CreateReplicationSlot':
 streamutil.c:244: warning: suggest parentheses around '' within '||'
I see. Here is a rebased patch.
Regards,
-- 
Michael
From 18e754569c1fd97f4346be935341b27e228775a4 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Fri, 15 Aug 2014 17:15:31 +0900
Subject: [PATCH] Add support for physical slot creation/deletion in
 pg_receivexlog

Physical slot creation can be done with --create and drop with --drop.
In both cases --slot is needed. Code for replication slot creation and
drop is refactored with what was available in pg_recvlogical.
---
 doc/src/sgml/ref/pg_receivexlog.sgml   | 29 ++
 src/bin/pg_basebackup/pg_receivexlog.c | 73 +
 src/bin/pg_basebackup/pg_recvlogical.c | 66 +++
 src/bin/pg_basebackup/streamutil.c | 97 ++
 src/bin/pg_basebackup/streamutil.h |  7 +++
 5 files changed, 203 insertions(+), 69 deletions(-)

diff --git a/doc/src/sgml/ref/pg_receivexlog.sgml b/doc/src/sgml/ref/pg_receivexlog.sgml
index 5916b8f..7e46005 100644
--- a/doc/src/sgml/ref/pg_receivexlog.sgml
+++ b/doc/src/sgml/ref/pg_receivexlog.sgml
@@ -72,6 +72,35 @@ PostgreSQL documentation
   titleOptions/title
 
para
+applicationpg_receivexlog/application can run in one of two following
+modes, which control physical replication slot:
+
+variablelist
+
+ varlistentry
+  termoption--create/option/term
+  listitem
+   para
+Create a new physical replication slot with the name specified in
+option--slot/option, then exit.
+   /para
+  /listitem
+ /varlistentry
+
+ varlistentry
+  termoption--drop/option/term
+  listitem
+   para
+Drop the replication slot with the name specified
+in option--slot/option, then exit.
+   /para
+  /listitem
+ /varlistentry
+/variablelist
+
+   /para
+
+   para
 The following command-line options control the location and format of the
 output.
 
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index a8b9ad3..98d034a 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -38,6 +38,8 @@ static int	noloop = 0;
 static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
 static int	fsync_interval = 0; /* 0 = default */
 static volatile bool time_to_abort = false;
+static bool do_create_slot = false;
+static bool do_drop_slot = false;
 
 
 static void usage(void);
@@ -78,6 +80,9 @@ usage(void)
 	printf(_(  -w, --no-password  never prompt for password\n));
 	printf(_(  -W, --password force password prompt (should happen automatically)\n));
 	printf(_(  -S, --slot=SLOTNAMEreplication slot to use\n));
+	printf(_(\nOptional actions:\n));
+	printf(_(  --create   create a new replication slot (for the slot's name see --slot)\n));
+	printf(_(  --drop drop the replication slot (for the slot's name see --slot)\n));
 	printf(_(\nReport bugs to pgsql-b...@postgresql.org.\n));
 }
 
@@ -261,14 +266,6 @@ StreamLog(void)
 	uint32		hi,
 lo;
 
-	/*
-	 * Connect in replication mode to the server
-	 */
-	conn = GetConnection();
-	if (!conn)
-		/* Error message already written in GetConnection() */
-		return;
-
 	if (!CheckServerVersionForStreaming(conn))
 	{
 		/*
@@ -370,6 +367,9 @@ main(int argc, char **argv)
 		{status-interval, required_argument, NULL, 's'},
 		{slot, required_argument, NULL, 'S'},
 		{verbose, no_argument, NULL, 'v'},
+/* action */
+		{create, no_argument, NULL, 1},
+		{drop, no_argument, NULL, 2},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -453,6 +453,13 @@ main(int argc, char **argv)
 			case 'v':
 verbose++;
 break;
+/* action */
+			case 1:
+do_create_slot = true;
+break;
+			case 2:
+do_drop_slot = true;
+break;
 			default:
 
 /*
@@ -477,10 +484,26 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
+	if (replication_slot == NULL  (do_drop_slot || do_create_slot))
+	{
+		fprintf(stderr, _(%s: replication slot needed with action --create or --drop\n), progname);
+		fprintf(stderr, _(Try \%s --help\ for more information.\n),
+progname);
+		exit(1);
+	}
+
+	if (do_drop_slot  do_create_slot)
+	{
+		fprintf(stderr, _(%s: cannot use --create together with --drop\n), progname);
+		fprintf(stderr, _(Try \%s --help\ for more information.\n),
+progname);
+		exit(1);
+	}
+
 	/*
 	 * Required arguments
 	 */
-	if (basedir == NULL)
+	if (basedir == NULL  !do_create_slot  !do_drop_slot)
 	{
 		fprintf(stderr, _(%s: no target 

Re: [HACKERS] pg_receivexlog and replication slots

2014-08-12 Thread Michael Paquier
On Fri, Jul 11, 2014 at 6:23 PM, Andres Freund and...@2ndquadrant.com wrote:
 Ok. Do you plan to take care of it? If, I'd be fine with backpatching
 it. I'm not likely to get to it right now :(

Actually I came up with the same need as Magnus, but a bit later,
so... Attached is a patch to support physical slot creation and drop
in pg_receivexlog with the addition of new options --create and
--drop. It would be nice to have that in 9.4, but I will not the one
deciding that at the end :) Code has been refactored with what is
already available in pg_recvlogical for the slot creation and drop.

Regards,
-- 
Michael
From 62ef9bd097453648e4f313f1aedf91ba2b4a3f1e Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Tue, 12 Aug 2014 21:54:13 +0900
Subject: [PATCH] Add support for physical slot creation/deletion in
 pg_receivexlog

Physical slot creation can be done with --create and drop with --drop.
In both cases --slot is needed. Code for replication slot creation and
drop is refactored with what was available in pg_recvlogical.
---
 doc/src/sgml/ref/pg_receivexlog.sgml   | 29 ++
 src/bin/pg_basebackup/pg_receivexlog.c | 73 +
 src/bin/pg_basebackup/pg_recvlogical.c | 66 +++
 src/bin/pg_basebackup/streamutil.c | 97 ++
 src/bin/pg_basebackup/streamutil.h |  7 +++
 5 files changed, 203 insertions(+), 69 deletions(-)

diff --git a/doc/src/sgml/ref/pg_receivexlog.sgml b/doc/src/sgml/ref/pg_receivexlog.sgml
index c15776f..9251f85 100644
--- a/doc/src/sgml/ref/pg_receivexlog.sgml
+++ b/doc/src/sgml/ref/pg_receivexlog.sgml
@@ -72,6 +72,35 @@ PostgreSQL documentation
   titleOptions/title
 
para
+applicationpg_receivexlog/application can run in one of two following
+modes, which control physical replication slot:
+
+variablelist
+
+ varlistentry
+  termoption--create/option/term
+  listitem
+   para
+Create a new physical replication slot with the name specified in
+option--slot/option, then exit.
+   /para
+  /listitem
+ /varlistentry
+
+ varlistentry
+  termoption--drop/option/term
+  listitem
+   para
+Drop the replication slot with the name specified
+in option--slot/option, then exit.
+   /para
+  /listitem
+ /varlistentry
+/variablelist
+
+   /para
+
+   para
 The following command-line options control the location and format of the
 output.
 
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index 0b7af54..67c56cb 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -38,6 +38,8 @@ static int	noloop = 0;
 static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
 static int	fsync_interval = 0; /* 0 = default */
 static volatile bool time_to_abort = false;
+static bool do_create_slot = false;
+static bool do_drop_slot = false;
 
 
 static void usage(void);
@@ -78,6 +80,9 @@ usage(void)
 	printf(_(  -w, --no-password  never prompt for password\n));
 	printf(_(  -W, --password force password prompt (should happen automatically)\n));
 	printf(_(  --slot=SLOTNAMEreplication slot to use\n));
+	printf(_(\nOptional actions:\n));
+	printf(_(  --create   create a new replication slot (for the slot's name see --slot)\n));
+	printf(_(  --drop drop the replication slot (for the slot's name see --slot)\n));
 	printf(_(\nReport bugs to pgsql-b...@postgresql.org.\n));
 }
 
@@ -261,14 +266,6 @@ StreamLog(void)
 	uint32		hi,
 lo;
 
-	/*
-	 * Connect in replication mode to the server
-	 */
-	conn = GetConnection();
-	if (!conn)
-		/* Error message already written in GetConnection() */
-		return;
-
 	if (!CheckServerVersionForStreaming(conn))
 	{
 		/*
@@ -370,6 +367,9 @@ main(int argc, char **argv)
 		{status-interval, required_argument, NULL, 's'},
 		{slot, required_argument, NULL, 'S'},
 		{verbose, no_argument, NULL, 'v'},
+/* action */
+		{create, no_argument, NULL, 1},
+		{drop, no_argument, NULL, 2},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -453,6 +453,13 @@ main(int argc, char **argv)
 			case 'v':
 verbose++;
 break;
+/* action */
+			case 1:
+do_create_slot = true;
+break;
+			case 2:
+do_drop_slot = true;
+break;
 			default:
 
 /*
@@ -477,10 +484,26 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
+	if (replication_slot == NULL  (do_drop_slot || do_create_slot))
+	{
+		fprintf(stderr, _(%s: replication slot needed with action --create or --drop\n), progname);
+		fprintf(stderr, _(Try \%s --help\ for more information.\n),
+progname);
+		exit(1);
+	}
+
+	if (do_drop_slot  do_create_slot)
+	{
+		fprintf(stderr, _(%s: cannot use --create together with --drop\n), progname);
+		fprintf(stderr, _(Try \%s --help\ for more information.\n),
+progname);
+		exit(1);
+	}
+
 	/*
 	 * Required arguments
 	 */
-	if (basedir == NULL)
+	

Re: [HACKERS] pg_receivexlog and replication slots

2014-07-11 Thread Andres Freund
On 2014-07-11 11:08:48 +0200, Magnus Hagander wrote:
 Is there a particular reason why pg_receivexlog only supports using
 manually created slots but pg_recvlogical supports creating and
 dropping them? Wouldn't it be good for consistency there?

I've added it to recvlogical because logical decoding isn't usable
without slots. I'm not sure what you'd want to create/drop a slot from
receivexlog for, but I'm not adverse to adding the capability.

 I'm guessing it's related to not being exposed over the replication
 protocol?

It's exposed:
create_replication_slot:
/* CREATE_REPLICATION_SLOT slot PHYSICAL */
K_CREATE_REPLICATION_SLOT IDENT K_PHYSICAL

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] pg_receivexlog and replication slots

2014-07-11 Thread Magnus Hagander
On Fri, Jul 11, 2014 at 11:14 AM, Andres Freund and...@2ndquadrant.com wrote:
 On 2014-07-11 11:08:48 +0200, Magnus Hagander wrote:
 Is there a particular reason why pg_receivexlog only supports using
 manually created slots but pg_recvlogical supports creating and
 dropping them? Wouldn't it be good for consistency there?

 I've added it to recvlogical because logical decoding isn't usable
 without slots. I'm not sure what you'd want to create/drop a slot from
 receivexlog for, but I'm not adverse to adding the capability.

I was mostly thinking for consistency, really. Using slots with
pg_receivexlog makes quite a bit of sense, even though it's useful
without it. But having the ability to create and drop (with compatible
commandline arguments of course) them directly when you set it up
would certainly be more convenient.


 I'm guessing it's related to not being exposed over the replication
 protocol?

 It's exposed:
 create_replication_slot:
 /* CREATE_REPLICATION_SLOT slot PHYSICAL */
 K_CREATE_REPLICATION_SLOT IDENT K_PHYSICAL

Hmm. You know, it would help if I actually looked at a 9.4 version of
the file when I check for functions of this kind :) Thanks!

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/


-- 
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] pg_receivexlog and replication slots

2014-07-11 Thread Andres Freund
On 2014-07-11 11:18:58 +0200, Magnus Hagander wrote:
 On Fri, Jul 11, 2014 at 11:14 AM, Andres Freund and...@2ndquadrant.com 
 wrote:
  On 2014-07-11 11:08:48 +0200, Magnus Hagander wrote:
  Is there a particular reason why pg_receivexlog only supports using
  manually created slots but pg_recvlogical supports creating and
  dropping them? Wouldn't it be good for consistency there?
 
  I've added it to recvlogical because logical decoding isn't usable
  without slots. I'm not sure what you'd want to create/drop a slot from
  receivexlog for, but I'm not adverse to adding the capability.
 
 I was mostly thinking for consistency, really. Using slots with
 pg_receivexlog makes quite a bit of sense, even though it's useful
 without it. But having the ability to create and drop (with compatible
 commandline arguments of course) them directly when you set it up
 would certainly be more convenient.

Ok. Do you plan to take care of it? If, I'd be fine with backpatching
it. I'm not likely to get to it 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