On Mon, Mar 14, 2016 at 2:38 PM, Thomas Munro <thomas.mu...@enterprisedb.com> wrote: >> What's with the extra block? > > Yeah, that's silly, thanks. Tidied up for the next version.
Some more comments on 0001: + <literal>remote_write</>, <literal> remote_apply</>, <literal>local</>, and <literal>off</>. Extra space. + * apply when the transaction eventuallly commits or aborts. Spelling. + if (synchronous_commit == SYNCHRONOUS_COMMIT_REMOTE_APPLY) + assign_synchronous_commit(SYNCHRONOUS_COMMIT_REMOTE_FLUSH, NULL); + + SyncRepWaitForLSN(gxact->prepare_end_lsn); + + if (synchronous_commit == SYNCHRONOUS_COMMIT_REMOTE_APPLY) + assign_synchronous_commit(SYNCHRONOUS_COMMIT_REMOTE_APPLY, NULL); You can't do this. Directly changing the value of a variable that is backing a GUC is verboten, and doing it through the thin veneer of calling the assign-hook will not avoid the terrible wrath of the powers that dwell in the outer dark, and/or Pittsburgh. You probably need a dance here similar to the way forcePageWrites/fullPageWrites work. /* + * Check if the caller would like to ask standbys for immediate feedback + * once this commit is applied. + */ Whitespace. + /* + * Check if the caller would like to ask standbys for immediate feedback + * once this abort is applied. + */ Whitespace again. /* + * doRequestWalReceiverReply is used by recovery code to ask the main recovery + * loop to trigger a walreceiver reply. + */ +static bool doRequestWalReceiverReply; This is the sort of comment that leads me to ask "why even bother writing a comment?". Try to say something that's not obvious from the variable name. The comment for XLogRequestWalReceiverReply has a similar issue. +static void WalRcvBlockSigUsr2(void) Style - newline after void. +static void WalRcvUnblockSigUsr2(void) And again here. + WalRcvUnblockSigUsr2(); len = walrcv_receive(NAPTIME_PER_CYCLE, &buf); + WalRcvBlockSigUsr2(); This does not seem like it will be cheap on all operating systems. I think you should try to rejigger this somehow so that it can just set the process latch and the wal receiver figures it out from looking at shared memory. Like maybe a flag in WalRcvData? An advantage of this is that it should cut down on the number of signals significantly, because it won't need to send SIGUSR1 when the latch is already set. + * Although only "on", "off", "remote_apply", "remote_write", and "local" are + * documented, we accept all the likely variants of "on" and "off". Maybe switch to listing the undocumented values. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers