Re: Sync Rep v17

On Sat, Feb 19, 2011 at 9:06 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>
> Well, good news all round.
>
> v17 implements what I believe to be the final set of features for sync
> rep. This one I'm actually fairly happy with. It can be enjoyed best at
> DEBUG3.
>
> The patch is very lite touch on a few areas of code, plus a chunk of
> specific code, all on master-side. Pretty straight really. I'm sure
> problems will be found, its not long since I completed this; thanks to
> Daniel Farina for your help with patch assembly.
>
> Which is just as well, because the other news is that I'm off on holiday
> for a few days, which is most inconvenient. I won't be committing this
> for at least a week and absent from the list. OTOH, I think its ready
> for a final review and commit, so I'm OK if you commit or OK if you
> leave it for me.

Thanks for the patch!

Here are the comments:


PREPARE TRANSACTION and ROLLBACK PREPARED should wait for
replication as well as COMMIT PREPARED?

What if fast shutdown is requested while RecordTransactionCommit
is waiting in SyncRepWaitForLSN? ISTM fast shutdown cannot complete
until replication has been successfully done (i.e., until at least one
synchronous standby has connected to the master especially if
allow_standalone_primary is disabled). Is this OK?

We should emit WARNING when the synchronous standby with
wal_receiver_status_interval = 0 connects to the master. Because,
in that case, a transaction unexpectedly would wait for replication
infinitely.

+    /* Need a modifiable copy of string */
+    rawstring = pstrdup(SyncRepStandbyNames);
+
+    /* Parse string into list of identifiers */
+    if (!SplitIdentifierString(rawstring, ',', &elemlist))

pfree(rawstring) and list_free(elemlist) should be called also if
SplitIdentifierString returns TRUE. Otherwise, memory-leak would
happen.

+        ereport(FATAL,
+                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+           errmsg("invalid list syntax for parameter
\"synchronous_standby_names\"")));
+        return false;

"return false" is not required here though that might be harmless.


I've read about a tenth of the patch, so I'll submit another comments
about the rest later.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center