Re: replication slot restart_lsn initialization

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