Re: pg_receivexlog and replication slots - Mailing list pgsql-hackers

Hi,

On 2014-10-04 15:01:03 +0900, Michael Paquier wrote:
> On Fri, Oct 3, 2014 at 8:57 PM, Andres Freund <andres@2ndquadrant.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