Thread: replication slot restart_lsn initialization

<div style="direction: ltr;font-family: Tahoma;color: #000000;font-size: 10pt;">Hello, <br /><br /> I am looking to
betterunderstand the thought behind a replication slot's restart_lsn initialization. Currently in 9.4 and master, a
replicationslot's restart_lsn is set to InvalidXLogRecPtr and will only start tracking restart_lsn once a walreceiver
hasconfirmed receipt of an lsn.<br /><br /> Was there any consideration for initializing restart_lsn to the latest WAL
writepointer when a slot is created? Or for allowing an optional parameter in
pg_create_(physical|logical)_replication_slot()for specifying the restart_lsn at slot creation?<br /><br /> I believe
thereare valid usage patterns where the user would like to start holding transaction logs from being removed/recycled
duringthe time that the standby is being restored from base backup. Currently this can be worked around by using
pg_receivexlogimmediately after creating the replication slot but it feels kind of hacky.<br /><br /> It is also
strangethat the return type for pg_create_(physical|logical)_replication_slot includes xlog_position but as far as I
cantell, it will never contain a value. Is this intended for something in the future?<br /><br /> Thanks,<br />
Danilo<br/></div> 

Hi,

On 2015-05-06 00:42:14 +0000, Duran, Danilo wrote:
> I am looking to better understand the thought behind a replication
> slot's restart_lsn initialization. Currently in 9.4 and master, a
> replication slot's restart_lsn is set to InvalidXLogRecPtr and will
> only start tracking restart_lsn once a walreceiver has confirmed
> receipt of an lsn.

Right, for physical slots that's true.

> Was there any consideration for initializing restart_lsn to the latest
> WAL write pointer when a slot is created? Or for allowing an optional
> parameter in pg_create_(physical|logical)_replication_slot() for
> specifying the restart_lsn at slot creation?

I've been wondering about allowing for the latter alternative. I could
have used it a couple times. The former doesn't make much sense to me,
it could be too far *ahead* in many cases actually.  A patch for this
would be fairly trivial.

It doesn't make sense for logical slots (as the computation of the
initial lsn is more complicated; it's also also already set when you
create one.

> I believe there are valid usage patterns where the user would like to
> start holding transaction logs from being removed/recycled during the
> time that the standby is being restored from base backup. Currently
> this can be worked around by using pg_receivexlog immediately after
> creating the replication slot but it feels kind of hacky.

Yea, that's what I've done as well. I'd much rather have a proper option
for it.

> It is also strange that the return type for
> pg_create_(physical|logical)_replication_slot includes xlog_position
> but as far as I can tell, it will never contain a value. Is this
> intended for something in the future?

It'll contain something for logical slots. I wanted the physical version
to be analogous, with the thought of adding a different signature at
some point.

Greetings,

Andres Freund

On 2015-06-10 08:00:28 -0700, Gurjeet Singh wrote:
> Attached is the patch that takes the former approach (initialize
> restart_lsn when the slot is created).

If it's an option that's imo a sane approach.

> pg_create_logical_replication_slot() prevents LSN from being
> recycled that by looping (worst case 2 times) until there's no
> conflict with the checkpointer recycling the segment. So I have used
> the same approach.

There's no need to change anything for logical slots? Or do you just
mean that you moved the existing code?
>  
>  /*
> + * Grab and save an LSN value to prevent WAL recycling past that point.
> + */
> +void
> +ReplicationSlotRegisterRestartLSN()
> +{
> +    ReplicationSlot *slot = MyReplicationSlot;
> +
> +    Assert(slot != NULL);
> +    Assert(slot->data.restart_lsn == InvalidXLogRecPtr);
> +
> +    /*
> +     * The replication slot mechanism is used to prevent removal of required
> +     * WAL. As there is no interlock between this and checkpoints required, WAL
> +     * segment could be removed before ReplicationSlotsComputeRequiredLSN() has
> +     * been called to prevent that. In the very unlikely case that this happens
> +     * we'll just retry.
> +     */
> +    while (true)
> +    {
> +        XLogSegNo    segno;
> +
> +        /*
> +         * Let's start with enough information if we can, so log a standby
> +         * snapshot and start decoding at exactly that position.
> +         */
> +        if (!RecoveryInProgress())
> +        {
> +            XLogRecPtr    flushptr;
> +
> +            /* start at current insert position */
> +            slot->data.restart_lsn = GetXLogInsertRecPtr();
> +
> +            /* make sure we have enough information to start */
> +            flushptr = LogStandbySnapshot();
> +
> +            /* and make sure it's fsynced to disk */
> +            XLogFlush(flushptr);
> +        }
> +        else
> +            slot->data.restart_lsn = GetRedoRecPtr();
> +
> +        /* prevent WAL removal as fast as possible */
> +        ReplicationSlotsComputeRequiredLSN();
> +
> +        /*
> +         * If all required WAL is still there, great, otherwise retry. The
> +         * slot should prevent further removal of WAL, unless there's a
> +         * concurrent ReplicationSlotsComputeRequiredLSN() after we've written
> +         * the new restart_lsn above, so normally we should never need to loop
> +         * more than twice.
> +         */
> +        XLByteToSeg(slot->data.restart_lsn, segno);
> +        if (XLogGetLastRemovedSegno() < segno)
> +            break;
> +    }
> +}

That doesn't look right to me. Why is this code logging a standby
snapshot for physical slots?

Greetings,

Andres Freund

On 2015-06-10 08:24:23 -0700, Gurjeet Singh wrote:
> On Wed, Jun 10, 2015 at 8:07 AM, Andres Freund <andres@anarazel.de> wrote:
> > That doesn't look right to me. Why is this code logging a standby
> > snapshot for physical slots?
> >
> 
> This is the new function I referred to above. The logging of the snapshot
> is in 'not RecoveryInProgress()' case, meaning it's running in primary and
> not in a standby.

It's still done uselessly for physical slots?

On 2015-06-10 13:13:41 -0700, Gurjeet Singh wrote:
>  /*
> + * Grab and save an LSN value to prevent WAL recycling past that point.
> + */
> +void
> +ReplicationSlotRegisterRestartLSN()
> +{
> +    ReplicationSlot *slot = MyReplicationSlot;
> +
> +    Assert(slot != NULL);
> +    Assert(slot->data.restart_lsn == InvalidXLogRecPtr);
> +
> +    /*
> +     * The replication slot mechanism is used to prevent removal of required
> +     * WAL. As there is no interlock between this and checkpoints required, WAL
> +     * segment could be removed before ReplicationSlotsComputeRequiredLSN() has
> +     * been called to prevent that. In the very unlikely case that this happens
> +     * we'll just retry.
> +     */
> +    while (true)
> +    {
> +        XLogSegNo    segno;
> +
> +        /*
> +         * Let's start with enough information if we can, so log a standby
> +         * snapshot and start decoding at exactly that position.
> +         */
> +        if (!RecoveryInProgress())
> +        {
> +            XLogRecPtr    flushptr;
> +
> +            /* start at current insert position */
> +            slot->data.restart_lsn = GetXLogInsertRecPtr();
> +
> +            /*
> +             * Log an xid snapshot for logical replication. It's not needed for
> +             * physical slots as it is done in BGWriter on a regular basis.
> +             */
> +            if (!slot->data.persistency == RS_PERSISTENT)
> +            {
> +                /* make sure we have enough information to start */
> +                flushptr = LogStandbySnapshot();
> +
> +                /* and make sure it's fsynced to disk */
> +                XLogFlush(flushptr);
> +            }

Huh? The slot->data.persistency == RS_PERSISTENT check seems pretty much
entirely random to me. I mean physical slots can (and normally are)
persistent as well?  Instead check whether it's a database specifics lot.

The reasoning why it's not helpful for physical slots is wrong. The
point is that a standby snapshot at that location will simply not help
physical replication, it's only going to read ones after the location at
which the base backup starts (i.e. the location from the backup label).

>  /* ----
> - * Manipulation of ondisk state of replication slots
> + * Manipulation of on-disk state of replication slots
>   *
>   * NB: none of the routines below should take any notice whether a slot is the
>   * current one or not, that's all handled a layer above.
> diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
> index 9a2793f..bd526fa 100644
> --- a/src/backend/replication/slotfuncs.c
> +++ b/src/backend/replication/slotfuncs.c
> @@ -40,6 +40,7 @@ Datum
>  pg_create_physical_replication_slot(PG_FUNCTION_ARGS)
>  {
>      Name        name = PG_GETARG_NAME(0);
> +    bool         activate = PG_GETARG_BOOL(1);

Don't like 'activate' much as a name. How about 'immediately_reserve'?

Other than that it's looking good to me.

Regards,

Andres

On 2015-07-07 06:41:55 -0700, Gurjeet Singh wrote:
> There seems to be a misplaced not operator  ! in that if statement, as
> well. That sucks :( The MacOS gcc binary is actually clang, and its output
> is too noisy [1], which is probably why I might have missed warning if any.

Which version of clang is it? With newer ones you can individually
disable nearly all of the warnings. I regularly use clang, and most of
the time it compiles master without warnings.

> > >  pg_create_physical_replication_slot(PG_FUNCTION_ARGS)
> > >  {
> > >       Name            name = PG_GETARG_NAME(0);
> > > +     bool            activate = PG_GETARG_BOOL(1);
> >
> > Don't like 'activate' much as a name. How about 'immediately_reserve'?
> >
> 
> I still like 'activate, but okay. How about 'reserve_immediately' instead?

Activate is just utterly wrong. A slot isn't inactive before. And
'active' already is used for slots that are currently in use
(i.e. connected to).

> Also, do you want this name change just in the C code, or in the pg_proc
> and docs as well?

All.

- Andres

On 2015-07-07 09:42:54 -0700, Gurjeet Singh wrote:
> On a side note, I see that the pg_create_*_replication_slot() functions do
> not behave transactionally; that is, rolling back a transaction does not
> undo the slot creation.

It can't, because otherwise you couldn't run them on a standby.

> Should we prevent these, and corresponding drop functions, from being
> called inside an explicit transaction?  PreventTransactionChain() is
> geared towards serving just the utility commands. Do we protect
> against calling such functions in a transaction block, or from user
> functions? How?

We discussed that when slots where introduced. There seems little
benefit in doing so, and it'll make some legitimate use cases harder.

On 2015-07-07 09:42:54 -0700, Gurjeet Singh wrote:
> --- a/src/backend/replication/slot.c
> +++ b/src/backend/replication/slot.c
> @@ -40,10 +40,10 @@
>  #include <sys/stat.h>
>
>  #include "access/transam.h"
> +#include "access/xlog_internal.h"
>  #include "common/string.h"
>  #include "miscadmin.h"
>  #include "replication/slot.h"
> -#include "storage/fd.h"
>  #include "storage/proc.h"
>  #include "storage/procarray.h"

Why did you remove fd.h? The file definitely uses infrastructure from
there. We're not terribly consistent about that but I'd rather not rely
on it being included via xlog_internal.h -> xlog.h.

>  /*
> + * Grab and save an LSN value to prevent WAL recycling past that point.
> + */
> +void
> +ReplicationSlotRegisterRestartLSN()
> +{

Didn't like that description and function name very much. What does
'grabbing' mean here? Should probably mention that it works on the
currently active slot and modifies it.

It's now ReplicationSlotReserveWal()

> +    ReplicationSlot *slot = MyReplicationSlot;
> +
> +    Assert(slot != NULL);
> +    Assert(slot->data.restart_lsn == InvalidXLogRecPtr);
> +
> +    /*
> +     * The replication slot mechanism is used to prevent removal of required
> +     * WAL. As there is no interlock between this and checkpoints required, WAL
> +     * segment could be removed before ReplicationSlotsComputeRequiredLSN() has
> +     * been called to prevent that. In the very unlikely case that this happens
> +     * we'll just retry.
> +     */

You removed some punctuation in that sentence converting a sentence in
bad english into one without the original meaning ;). See the attached
for a new version.

> +    while (true)
> +    {
> +        XLogSegNo    segno;
> +
> +        /*
> +         * Let's start with enough information if we can, so log a standby
> +         * snapshot and start logical decoding at exactly that position.
> +         */
> +        if (!RecoveryInProgress())
> +        {
> +            XLogRecPtr    flushptr;
> +
> +            /* start at current insert position */
> +            slot->data.restart_lsn = GetXLogInsertRecPtr();
> +
> +            /*
> +             * Log an xid snapshot for logical replication. This snapshot is not
> +             * needed for physical replication, as it relies on the snapshot
> +             * created by checkpoint when the base backup starts.
> +             */
> +            if (slot->data.database != InvalidOid)
> +            {
> +                /* make sure we have enough information to start */
> +                flushptr = LogStandbySnapshot();
> +
> +                /* and make sure it's fsynced to disk */
> +                XLogFlush(flushptr);
> +            }
> +        }
> +        else
> +            slot->data.restart_lsn = GetRedoRecPtr();
> +
> +        /* prevent WAL removal as fast as possible */
> +        ReplicationSlotsComputeRequiredLSN();
> +
> +        /*
> +         * If all required WAL is still there, great, otherwise retry. The
> +         * slot should prevent further removal of WAL, unless there's a
> +         * concurrent ReplicationSlotsComputeRequiredLSN() after we've written
> +         * the new restart_lsn above, so normally we should never need to loop
> +         * more than twice.
> +         */
> +        XLByteToSeg(slot->data.restart_lsn, segno);
> +        if (XLogGetLastRemovedSegno() < segno)
> +            break;
> +    }
> +}

The way you added the check for logical vs. physical slots in there
looks wrong to me. For a physical slot created !InRecovy we'll possibly
return a xlog position from the future (it's the insert position *and*
not flushed to disk), which then cannot be received.

> +/*
>   * Flush all replication slots to disk.
>   *
>   * This needn't actually be part of a checkpoint, but it's a convenient
> @@ -876,7 +942,7 @@ StartupReplicationSlots(void)
>  }
>
>  /* ----
> - * Manipulation of ondisk state of replication slots
> + * Manipulation of on-disk state of replication slots
>   *
>   * NB: none of the routines below should take any notice whether a slot is the
>   * current one or not, that's all handled a layer above.
> diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
> index 9a2793f..01b376a 100644
> --- a/src/backend/replication/slotfuncs.c
> +++ b/src/backend/replication/slotfuncs.c
> @@ -40,6 +40,7 @@ Datum
>  pg_create_physical_replication_slot(PG_FUNCTION_ARGS)
>  {
>      Name        name = PG_GETARG_NAME(0);
> +    bool         immediately_reserve = PG_GETARG_BOOL(1);
>      Datum        values[2];
>      bool        nulls[2];
>      TupleDesc    tupdesc;
> @@ -58,10 +59,28 @@ pg_create_physical_replication_slot(PG_FUNCTION_ARGS)
>      /* acquire replication slot, this will check for conflicting names */
>      ReplicationSlotCreate(NameStr(*name), false, RS_PERSISTENT);
>
> -    values[0] = NameGetDatum(&MyReplicationSlot->data.name);
> +    if (immediately_reserve)
> +    {
> +        /* Allocate restart-LSN, if the user asked for it */
> +        ReplicationSlotRegisterRestartLSN();
> +
> +        /* Write this slot to disk */
> +        ReplicationSlotMarkDirty();
> +        ReplicationSlotSave();
>
> -    nulls[0] = false;
> -    nulls[1] = true;
> +        values[0] = NameGetDatum(&MyReplicationSlot->data.name);
> +        values[1] = LSNGetDatum(MyReplicationSlot->data.restart_lsn);
> +
> +        nulls[0] = false;
> +        nulls[1] = false;
> +    }
> +    else
> +    {
> +        values[0] = NameGetDatum(&MyReplicationSlot->data.name);
> +
> +        nulls[0] = false;
> +        nulls[1] = true;
> +    }

I moved
values[0] = NameGetDatum(&MyReplicationSlot->data.name);
nulls[0] = false;
to outside the conditional block, there seems no reason to have it in
there?

I also removed a bunch of unrelated minor cleanups that I plan to commit
& backpatch separately.

What do you think?

Andres

On 2015-08-11 15:59:59 -0700, Gurjeet Singh wrote:
> In your version, I don't see a comment that refers to the fact that it
> works on the currently active (global variable) slot.

Hm?

/** Reserve WAL for the currently active slot.** Compute and set restart_lsn in a manner that's appropriate for the
typeof* the slot and concurrency safe.*/
 
> > I moved
> > values[0] = NameGetDatum(&MyReplicationSlot->data.name);
> > nulls[0] = false;
> > to outside the conditional block, there seems no reason to have it in
> > there?
> >
> 
> The assignment to values[0] is being done twice. We can do away with the
> one in the else part of the if condition.

Ugh, that's a mistake.

> [1]: I altered you to it in a personal email, but probably it fell through
> the cracks.

I usually just check the lists for newer patch versions, sorry...

Greetings,

Andres Freund

On Wed, Aug 12, 2015 at 8:20 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-08-11 15:59:59 -0700, Gurjeet Singh wrote:
>> In your version, I don't see a comment that refers to the fact that it
>> works on the currently active (global variable) slot.
>
> Hm?
>
> /*
>  * Reserve WAL for the currently active slot.
>  *
>  * Compute and set restart_lsn in a manner that's appropriate for the type of
>  * the slot and concurrency safe.
>  */
>> > I moved
>> > values[0] = NameGetDatum(&MyReplicationSlot->data.name);
>> > nulls[0] = false;
>> > to outside the conditional block, there seems no reason to have it in
>> > there?
>> >
>>
>> The assignment to values[0] is being done twice. We can do away with the
>> one in the else part of the if condition.
>
> Ugh, that's a mistake.
>
>> [1]: I altered you to it in a personal email, but probably it fell through
>> the cracks.
>
> I usually just check the lists for newer patch versions, sorry...

Commit 6fcd8851, which is the result of this thread, is not touching
the replication protocol at all. This looks like an oversight to me:
we should be a maximum consistent between the SQL interface and the
replication protocol if possible, and it looks useful to me to be able
to set restart_lsn when creating the slot as well when using a
replication connection. Most of the work has visibly been done with
the refactoring that created ReplicationSlotReserveWal, hence how
would we expose this option in CREATE_REPLICATION_SLOT?

For now we can do that:
CREATE_REPLICATION_SLOT IDENT K_PHYSICAL
We could append a keyword like RESERVE on this query. Or go through
more fancy things like (slot_options) where slot_options is a list of
option items, reserve = on/off.
Thoughts?
-- 
Michael

On 2015-08-14 16:44:44 +0900, Michael Paquier wrote:
> Commit 6fcd8851, which is the result of this thread, is not touching
> the replication protocol at all. This looks like an oversight to me:
> we should be a maximum consistent between the SQL interface and the
> replication protocol if possible, and it looks useful to me to be able
> to set restart_lsn when creating the slot as well when using a
> replication connection.

It wasn't, at least not from my side. You can relatively easily do
nearly the same just by connecting to the slot and sending a feedback
message. The complaint upthread (and/or a related thread) was that it's
not possible to do the same from SQL.

It'd be a good additional to offer the same facility to the replication
protocol.

> For now we can do that: CREATE_REPLICATION_SLOT IDENT K_PHYSICAL We
> could append a keyword like RESERVE on this query. Or go through more
> fancy things like (slot_options) where slot_options is a list of
> option items, reserve = on/off.  Thoughts?  -- Michael

I'd name it RESERVE_WAL. My feeling is that the options for the logical
case are geared towards the output plugin, not the walsender. I think
it'd be confusing to use (slot_options) differently for physical slots.

Greetings,

Andres Freund

On 2015-08-14 11:09:38 +0200, Shulgin, Oleksandr wrote:
> Yes, but the options list you pass to START_REPLICATION ... LOGICAL, not to
> CREATE_REPLICATION_SLOT.

I know, but we might want to extend that at some point.

- Andres

On Fri, Aug 14, 2015 at 4:54 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-08-14 16:44:44 +0900, Michael Paquier wrote:
>> Commit 6fcd8851, which is the result of this thread, is not touching
>> the replication protocol at all. This looks like an oversight to me:
>> we should be a maximum consistent between the SQL interface and the
>> replication protocol if possible, and it looks useful to me to be able
>> to set restart_lsn when creating the slot as well when using a
>> replication connection.
>
> It wasn't, at least not from my side. You can relatively easily do
> nearly the same just by connecting to the slot and sending a feedback
> message. The complaint upthread (and/or a related thread) was that it's
> not possible to do the same from SQL.
>
> It'd be a good additional to offer the same facility to the replication
> protocol.
> [...]
> I'd name it RESERVE_WAL. My feeling is that the options for the logical
> case are geared towards the output plugin, not the walsender. I think
> it'd be confusing to use (slot_options) differently for physical slots.

Well, this has taken less time than I thought:
=# CREATE_REPLICATION_SLOT toto PHYSICAL;
 slot_name | consistent_point | snapshot_name | output_plugin
-----------+------------------+---------------+---------------
 toto      | 0/0              | null          | null
(1 row)
=# CREATE_REPLICATION_SLOT toto2 PHYSICAL RESERVE_WAL;
 slot_name | consistent_point | snapshot_name | output_plugin
-----------+------------------+---------------+---------------
 toto2     | 0/0              | null          | null
(1 row)
=# \q
$ psql -c 'select slot_name,restart_lsn from pg_replication_slots'
 slot_name | restart_lsn
-----------+-------------
 toto      |
 toto2     | 0/1738850
(2 rows)

What do you want to do with that? Do you wish to have a look at it or
should I register it in the next CF? I am fine with both, though this
is tightly linked with the feature already committed.
Regards,
--
Michael

On 8/14/15 3:54 AM, Andres Freund wrote:
> I'd name it RESERVE_WAL. My feeling is that the options for the logical
> case are geared towards the output plugin, not the walsender. I think
> it'd be confusing to use (slot_options) differently for physical slots.

Why "reserve"?  Isn't it "preserve"?

On 2015-08-17 15:22:44 -0400, Peter Eisentraut wrote:
> On 8/14/15 3:54 AM, Andres Freund wrote:
> > I'd name it RESERVE_WAL.

> Why "reserve"? > Isn't it "preserve"?

Hm. I honestly do not know. I was thinking of ticket reservations...

On Tue, Aug 18, 2015 at 4:46 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-08-17 15:22:44 -0400, Peter Eisentraut wrote:
>> On 8/14/15 3:54 AM, Andres Freund wrote:
>> > I'd name it RESERVE_WAL.
>
>> Why "reserve"? > Isn't it "preserve"?
>
> Hm. I honestly do not know. I was thinking of ticket reservations...

Or "retain"?
-- 
Michael

On 2015-08-15 21:16:11 +0900, Michael Paquier wrote:
> Well, this has taken less time than I thought:
> =# CREATE_REPLICATION_SLOT toto PHYSICAL;
>  slot_name | consistent_point | snapshot_name | output_plugin
> -----------+------------------+---------------+---------------
>  toto      | 0/0              | null          | null
> (1 row)
> =# CREATE_REPLICATION_SLOT toto2 PHYSICAL RESERVE_WAL;
>  slot_name | consistent_point | snapshot_name | output_plugin
> -----------+------------------+---------------+---------------
>  toto2     | 0/0              | null          | null
> (1 row)

Independently I wonder if it's worth splitting the returned columns for
physical/logical slots.

> +      <varlistentry>
> +       <term><literal>RESERVE_WAL</></term>
> +       <listitem>
> +        <para>
> +         Specify that the <acronym>LSN</> for this physical replication
> +         slot is reserved immediately; otherwise the <acronym>LSN</> is
> +         reserved on first connection from a streaming replication client.
> +        </para>
> +       </listitem>

I replaced LSN with WAL here - it's not LSNs that are reserved.

> +opt_reserve_wal:
> +            K_RESERVE_WAL
> +                { $$ = TRUE }
> +            | /* EMPTY */
> +                { $$ = FALSE }
> +

I felt free to add semicolons here ;)


Committed, thanks for the patch.