Re: replication slot restart_lsn initialization

On Tue, Jul 7, 2015 at 4:59 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:

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

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.

[1]: I am particularly annoyed by these:

note: remove extraneous parentheses around the comparison to silence this
warning
note: use '=' to turn this equality comparison into an assignment

Eg. : if (((opaque)->btpo_next == 0))

I'll see what I can do about these.

> I mean physical slots can (and normally are)
> persistent as well? Instead check whether it's a database specifics lot.
>

Agreed, the slot being database-specific is the right indicator.

>
> 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'?
>

I still like 'activate, but okay. How about 'reserve_immediately' instead?

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

>
> Other than that it's looking good to me.
>

Will send a new patch after your feedback on the 'activate' renaming.

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/