Re: [HACKERS] pg_receivexlog and replication slots
On Mon, Oct 6, 2014 at 8:01 PM, Andres Freund wrote: > Pushed with these adjustments. > Thanks. The portions changed look fine to me. -- Michael
Re: [HACKERS] pg_receivexlog and replication slots
Hi,
On 2014-10-04 15:01:03 +0900, Michael Paquier wrote:
> On Fri, Oct 3, 2014 at 8:57 PM, Andres Freund wrote:
> > >
> > > +pg_receivexlog 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.
> +pg_receivexlog can perform one of the two
> +following actions in order to control physical replication slots:
> +
> +
> +
> + --create-slot
> +
> +
> +Create a new physical replication slot with the name specified in
> +--slot, then start stream.
> +
> +
> +
*, 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
On Mon, Oct 6, 2014 at 7:14 PM, Andres Freund
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 .\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
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 .\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.
I've pushed a polished up version. Fixing the above and
logicaldecoding.sgml
Note that at least gnu's getopt_long() accepts --create for
--create-slot...
Thanks,
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
On Sat, Oct 4, 2014 at 7:03 PM, Michael Paquier wrote: > On Sun, Oct 5, 2014 at 12:09 AM, Andres Freund 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
On Sun, Oct 5, 2014 at 12:09 AM, Andres Freund 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
On 2014-10-04 14:25:27 +0900, Michael Paquier wrote: > On Sat, Oct 4, 2014 at 9:24 AM, Michael Paquier > wrote: > > > > > > > On Sat, Oct 4, 2014 at 6:48 AM, Andres Freund > > wrote: > > > >> On 2014-10-03 14:02:08 -0400, Robert Haas wrote: > >> > On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund > >> 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
On 2014-10-04 09:24:07 +0900, Michael Paquier wrote: > On Sat, Oct 4, 2014 at 6:48 AM, Andres Freund > wrote: > > > On 2014-10-03 14:02:08 -0400, Robert Haas wrote: > > > On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund > > 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
On Fri, Oct 3, 2014 at 8:57 PM, Andres Freund wrote:
> >
> > +pg_receivexlog 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
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 pg_receivexlog cannot become the
synchronous standby through an incautious setting of
; it does not flush
- data frequently enough for this to work correctly.
+ data frequently enough for this to work correctly. In
+ --create-slot mode, create the slot with this name.
+ In --drop-slot mode, delete the slot with this name.
@@ -263,6 +265,33 @@ PostgreSQL documentation
+pg_receivexlog can perform one of the two
+following actions in order to control physical replication slots:
+
+
+
+ --create-slot
+
+
+Create a new physical replication slot with the name specified in
+--slot, then start stream.
+
+
+
+
+
+ --drop-slot
+
+
+Drop the replication slot with the name specified in
+--slot, then exit.
+
+
+
+
+
+
+
Other options are also available:
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(_(" --create-slot create a new replication slot (for the slot's name see --slot)\n"));
+ printf(_(" --drop-slotdrop the replication slot (for the slot's name see --slot)\n"));
printf(_("\nReport bugs to .\n"));
}
@@ -118,6 +125,44 @@ stop_streaming(XLogRecPtr xlogpos, uint32 timeline, bool segment_finished)
return false;
}
+
+/*
+ * Get destinati
Re: [HACKERS] pg_receivexlog and replication slots
On Sat, Oct 4, 2014 at 9:24 AM, Michael Paquier
wrote:
>
>
> On Sat, Oct 4, 2014 at 6:48 AM, Andres Freund
> wrote:
>
>> On 2014-10-03 14:02:08 -0400, Robert Haas wrote:
>> > On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund
>> 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
- --create
+ --create-slot
Create a new logical replication slot with the name specified in
@@ -82,7 +82,7 @@ PostgreSQL documentation
- --drop
+ --drop-slot
Drop the replication slot with the name specified
@@ -266,8 +266,8 @@ PostgreSQL documentation
In --start mode, use the existing logical replication slot named
-slot_name. In --create mode, create the
-slot with this name. In --drop mode, delete the slot with this name.
+slot_name. In --create-slot mode, create the
+slot with this name. In --drop-slot mode, delete the slot with this name.
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 .\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 make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_receivexlog and replication slots
On Sat, Oct 4, 2014 at 6:48 AM, Andres Freund wrote: > On 2014-10-03 14:02:08 -0400, Robert Haas wrote: > > On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund > 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
On 2014-10-03 14:02:08 -0400, Robert Haas wrote: > On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund 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
On Fri, Oct 3, 2014 at 7:57 AM, Andres Freund 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
On 2014-10-03 10:30:19 +0900, Michael Paquier wrote:
> On Thu, Oct 2, 2014 at 12:44 AM, Andres Freund
> 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.
>
> +pg_receivexlog 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
On Thu, Oct 2, 2014 at 12:44 AM, Andres Freund
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
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
Options
+pg_receivexlog can run in one of two following
+modes, which control physical replication slot:
+
+
+
+
+ --create
+
+
+Create a new physical replication slot with the name specified in
+--slot.
+
+
+
+
+
+ --drop
+
+
+Drop the replication slot with the name specified in
+--slot, then exit.
+
+
+
+
+
+
+
+
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 .\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 = readdir(dir)) != NULL)
{
@@ -219,12 +258,7 @@ FindStreamingStart(uint32 *tli)
disconnect_and_exit(1);
}
- if (closedir(dir))
- {
- fprintf(stderr, _("%s: could not close directory \"%s\": %s\n"),
-progname, basedir, strerror(errno));
- disconnect_and_exit(1);
- }
+ c
Re: [HACKERS] pg_receivexlog and replication slots
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
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
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
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 -- 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
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 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
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 > 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
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 > > 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
Andres Freund wrote: > From d667f7a63cd62733d88ec5b7228dfd5d7136b9d7 Mon Sep 17 00:00:00 2001 > From: Michael Paquier > 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
On 2014-10-01 21:54:56 +0900, Michael Paquier wrote: > Thanks! > > On Wed, Oct 1, 2014 at 8:38 PM, Andres Freund > 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
Thanks! On Wed, Oct 1, 2014 at 8:38 PM, Andres Freund 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
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
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 2>&1
$(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
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
On Mon, Sep 29, 2014 at 9:42 PM, Andres Freund
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
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'
Re: [HACKERS] pg_receivexlog and replication slots
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
On Mon, Sep 22, 2014 at 2:25 PM, Amit Kapila wrote: > On Sat, Sep 20, 2014 at 10:06 PM, Michael Paquier > wrote: >> On Sat, Sep 20, 2014 at 7:09 AM, Amit Kapila >> 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 sam
Re: [HACKERS] pg_receivexlog and replication slots
On Sat, Sep 20, 2014 at 10:06 PM, Michael Paquier
wrote:
> On Sat, Sep 20, 2014 at 7:09 AM, Amit Kapila
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
On Sat, Sep 20, 2014 at 7:09 AM, Amit Kapila 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
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, "ID
Re: [HACKERS] pg_receivexlog and replication slots
On Wed, Sep 10, 2014 at 10:09 AM, Michael Paquier
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
On Mon, Sep 8, 2014 at 8:50 PM, Michael Paquier
wrote:
> On Wed, Sep 3, 2014 at 11:40 PM, Robert Haas wrote:
>> On Sun, Aug 31, 2014 at 9:45 AM, Magnus Hagander 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
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
On Wed, Sep 3, 2014 at 11:40 PM, Robert Haas wrote: > On Sun, Aug 31, 2014 at 9:45 AM, Magnus Hagander 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
On Sun, Aug 31, 2014 at 9:45 AM, Magnus Hagander 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
On 2014-09-01 20:58:29 +0900, Michael Paquier wrote: > On Sun, Aug 31, 2014 at 10:45 PM, Magnus Hagander 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
On Sun, Aug 31, 2014 at 10:45 PM, Magnus Hagander 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
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
- */
- c
Re: [HACKERS] pg_receivexlog and replication slots
On Tue, Aug 26, 2014 at 9:43 AM, Michael Paquier wrote: > On Tue, Aug 19, 2014 at 2:49 PM, Michael Paquier > wrote: >> On Mon, Aug 18, 2014 at 4:01 PM, Michael Paquier >> wrote: >>> On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao wrote: On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier 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
On Tue, Aug 19, 2014 at 2:49 PM, Michael Paquier
wrote:
> On Mon, Aug 18, 2014 at 4:01 PM, Michael Paquier
> wrote:
>> On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao wrote:
>>> On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
>>> 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
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
Options
+pg_receivexlog can run in one of two following
+modes, which control physical replication slot:
+
+
+
+
+ --create
+
+
+Create a new physical replication slot with the name specified in
+--slot, then exit.
+
+
+
+
+
+ --drop
+
+
+Drop the replication slot with the name specified
+in --slot, then exit.
+
+
+
+
+
+
+
+
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 drop the replication slot (for the slot's name see --slot)\n"));
printf(_("\nReport bugs to .\n"));
}
@@ -253,21 +258,10 @@ FindStreamingStart(uint32 *tli)
static void
StreamLog(void)
{
- PGresult *res;
XLogRecPtr startpos;
- uint32 starttli;
+ TimeLineID starttli;
XLogRecPtr serverpos;
- uint32 ser
Re: [HACKERS] pg_receivexlog and replication slots
On Tue, Aug 19, 2014 at 6:03 PM, Andres Freund wrote: > On 2014-08-19 18:02:32 +0900, Fujii Masao wrote: >> On Tue, Aug 19, 2014 at 5:52 PM, Andres Freund >> 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
On 2014-08-19 18:02:32 +0900, Fujii Masao wrote: > On Tue, Aug 19, 2014 at 5:52 PM, Andres Freund 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
On Tue, Aug 19, 2014 at 5:52 PM, Andres Freund 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
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
On Mon, Aug 18, 2014 at 4:01 PM, Michael Paquier wrote: > On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao wrote: >> On Mon, Aug 18, 2014 at 2:38 PM, 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) >>> 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
On Mon, Aug 18, 2014 at 4:01 PM, Michael Paquier
wrote:
> On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao wrote:
>> On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
>> 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
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
Options
+pg_receivexlog can run in one of two following
+modes, which control physical replication slot:
+
+
+
+
+ --create
+
+
+Create a new physical replication slot with the name specified in
+--slot, then exit.
+
+
+
+
+
+ --drop
+
+
+Drop the replication slot with the name specified
+in --slot, then exit.
+
+
+
+
+
+
+
+
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 .\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))
Re: [HACKERS] pg_receivexlog and replication slots
On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao wrote: > On Mon, Aug 18, 2014 at 2:38 PM, 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) >> 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
On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao wrote: > On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier > wrote: >> On Fri, Aug 15, 2014 at 5:17 PM, Michael Paquier >> wrote: >>> Thanks for your review. >>> >>> On Fri, Aug 15, 2014 at 12:56 AM, 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
On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
wrote:
> On Fri, Aug 15, 2014 at 5:17 PM, Michael Paquier
> wrote:
>> Thanks for your review.
>>
>> On Fri, Aug 15, 2014 at 12:56 AM, 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
On Fri, Aug 15, 2014 at 5:17 PM, Michael Paquier
wrote:
> Thanks for your review.
>
> On Fri, Aug 15, 2014 at 12:56 AM, 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
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
Options
+pg_receivexlog can run in one of two following
+modes, which control physical replication slot:
+
+
+
+
+ --create
+
+
+Create a new physical replication slot with the name specified in
+--slot, then exit.
+
+
+
+
+
+ --drop
+
+
+Drop the replication slot with the name specified
+in --slot, then exit.
+
+
+
+
+
+
+
+
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 .\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 (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:
Re: [HACKERS] pg_receivexlog and replication slots
Thanks for your review.
On Fri, Aug 15, 2014 at 12:56 AM, 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
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
Options
+pg_receivexlog can run in one of two following
+modes, which control physical replication slot:
+
+
+
+
+ --create
+
+
+Create a new physical replication slot with the name specified in
+--slot, then exit.
+
+
+
+
+
+ --drop
+
+
+Drop the replication slot with the name specified
+in --slot, then exit.
+
+
+
+
+
+
+
+
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 .\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 directory specified\n"), progname);
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
@@ -492,6 +515,38 @@ main(int argc, char **argv)
pqsignal(SIGINT, sigint_handler);
#endif
+ /* Obtain a connection before doing anything */
+ conn = Get
Re: [HACKERS] pg_receivexlog and replication slots
> 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
On Fri, Jul 11, 2014 at 6:23 PM, Andres Freund 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
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
Options
+pg_receivexlog can run in one of two following
+modes, which control physical replication slot:
+
+
+
+
+ --create
+
+
+Create a new physical replication slot with the name specified in
+--slot, then exit.
+
+
+
+
+
+ --drop
+
+
+Drop the replication slot with the name specified
+in --slot, then exit.
+
+
+
+
+
+
+
+
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 .\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 directory specified\n"), progname);
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
@@ -492,6 +515,38 @@ main(int argc, char **argv)
pqsignal(SIGINT,
Re: [HACKERS] pg_receivexlog and replication slots
On 2014-07-11 11:18:58 +0200, Magnus Hagander wrote: > On Fri, Jul 11, 2014 at 11:14 AM, Andres Freund > 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
Re: [HACKERS] pg_receivexlog and replication slots
On Fri, Jul 11, 2014 at 11:14 AM, Andres Freund 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
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
