Re: replication slot restart_lsn initialization

On 2015-06-10 13:13:41 -0700, Gurjeet Singh wrote:
> +                     /*
> +                      * 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).

>  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.