On Tue, Jan 25, 2011 at 1:02 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Tue, Jan 25, 2011 at 6:02 AM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
>> Hmm, perhaps the code would be more readable if instead of the
>> forcePageWrites counter that counts exclusive and non-exclusive backups, and
>> an exclusiveBackup boolean indicating if one of the in-progress backups is
>> an exclusive one, we had a counter that only counts non-exclusive backups,
>> plus a boolean indicating if an exclusive backup is in progress in addition
>> to them.
>>
>> Attached is a patch for that (against master branch, including only xlog.c).
>
> I read this patch and previous-posted one. Those look good.
>
> Comments:
>
> + * do_pg_start_backup is the workhorse of the user-visible pg_stop_backup()
> + * function.
>
> Typo: s/do_pg_start_backup/do_pg_stop_backup
>
> It's helpful to explain about this behavior in pg_basebackup.sgml or elsewhere.
When I read the patch, I found that pg_stop_backup removes the backup history
file as soon as it creates the file, if archive_mode is not enabled.
This looks like
oversight. We should prevent pg_stop_backup from removing the fresh history
file? Or we should prevent pg_stop_backup from creating the history
file from the
beginning since it's not used at all if archiving is disabled?
(If archiving is enabled, the history file can be used to clean the
archived files up).
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center