Re: Proposal: "Causal reads" mode for load balancing reads without stale data

On Mon, Mar 14, 2016 at 2:38 PM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)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