On Tue, Jul 7, 2015 at 4:59 AM, Andres Freund <and...@anarazel.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/
