On Tue, Jan 24, 2012 at 6:43 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> Cleaned up the points noted, new patch attached in case you wish to
>> review further.
>>
>> Still has bug, so still with me to fix.
>
> Thanks! Will review further.
v3 patch contains lots of unrelated code changes like the following.
- <structfield>pid</structfield> column of the
+ <structfield>procpid</structfield> column of the
You seem to have failed to extract the patch from your repository correctly.
So I used v2 patch for the review. Sorry if I comment the things which you've
already fixed in v3 patch.
Here are the comments. They are almost not serious problems.
+/*
+ * GUC parameters
+ */
+int WalRestoreDelay = 10000;
You forget to change guc.c to define wal_restore_delay as a GUC parameter?
Or just that source code comment is incorrect?
+ elog(FATAL, "recovery_restore_command is too long");
Typo: s/recovery_restore_command/restore_command
+ InitLatch(&walrstr->WALRestoreLatch); /* initialize latch used in main loop */
That latch is shared one. OwnLatch() should be called instead of InitLatch()?
If yes, it's better to call DisownLatch() when walrestore exits. Though skipping
DisownLatch() would be harmless because the latch is never owned by new
process after walrestore exits.
+ {
+ /* use volatile pointer to prevent code rearrangement */
+ volatile WalRestoreData *walrstr = WalRstr;
+
+ nextFileTli = walrstr->nextFileTli;
The declaration of "walrstr" is not required here because it's already done
at the head of WalRestoreNextFile().
+ if (restoredFromArchive)
+ {
+ /* use volatile pointer to prevent code rearrangement */
+ volatile WalRestoreData *walrstr = WalRstr;
Same as above.
+#define TMPRECOVERYXLOG "RECOVERYXLOG"
ISTM that it's better to move this definition to an include file and we should
use it in all the places where the fixed value "RECOVERYXLOG" is still used.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
