Horton

Re: Patch pg_is_in_backup()

Gabriele Bartolini <gabriele.bartolini@2ndquadrant.it>

View thread
From: Gabriele Bartolini <gabriele.bartolini@2ndQuadrant.it>
To: Gilles Darold <gilles.darold@dalibo.com>
Cc: pgsql-hackers@postgresql.org, Magnus Hagander <magnus@hagander.net>, Fujii Masao <masao.fujii@gmail.com>, Bernd Helmle <mailings@oopsware.de>, Euler Taveira de Oliveira <euler@timbira.com>, Marti Raudsepp <marti@juffo.org>
Date: 2012-05-02T17:53:21Z
Lists: pgsql-hackers 
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.bartolini@2ndQuadrant.it | www.2ndQuadrant.it