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