Hi Gilles,

   Sorry for the delay.

Il 03/04/12 14:21, Gilles Darold ha scritto:
+1, this is also my point of view.

I have looked at the patch that contains both pg_is_in_backup() and pg_backup_start_time().

From a functional point of view it looks fine to me. I was thinking of adding the BackupInProgress() at the beginning of pg_backup_start_time(), but the AllocateFile() function already make sure the file exists.

I have performed some basic testing of both functions and tried to inject invalid characters in the start time field of the backup_label file and it is handled (with an exception) by the server. Cool.

I spotted though some formatting issues, in particular indentation and multi-line comments. Some rows are longer than 80 chars.

Please resubmit with these cosmetic changes and it is fine with me. Thank you.

Cheers,
Gabriele

--
 Gabriele Bartolini - 2ndQuadrant Italia
 PostgreSQL Training, Services and Support
 gabriele.bartol...@2ndquadrant.it | www.2ndQuadrant.it


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to