Re: 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 <andres(at)2ndquadrant(dot)com> wrote:
> > > <para>
> > > + <application>pg_receivexlog</application> can run in one of two following
> > > + modes, which control physical replication slot:
> >
> > I don't think that's good enough. There's also the important mode where
> > it's not doing --create/--drop at all.
> Well, yes, however the third mode is not explicitly present, and I
> don't see much point in adding a --start mode thinking
> backward-compatibility. Now, I refactored a bit the documentation to
> mention that pg_receivexlog can perform additional actions to control
> replication slots. I added as well in the portion of option --slot how
> it interacts with --create-slot and --drop-slot.

That's better.

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

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

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

*, then start to stream WAL.

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

I changed this to refer to --slot.

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

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

Pushed with these adjustments.

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

Greetings,

Andres Freund

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