recovery.conf parsing problems

While testing a PITR recovery, I discovered that recovery.conf doesn't
seem to allow specifying ' in the command string, making it hard to
protect the restore_command against problematic filenames (whitespace
etc.). This doesn't seem to be a problem for archive_command, which
allows \' (e.g. archive_command = '/path/to/script \'%f\' \'%p\'').
Should this be fixed?

--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services

On Wed, 2006-12-13 at 04:23 +0000, Andrew - Supernews wrote:
> While testing a PITR recovery, I discovered that recovery.conf doesn't
> seem to allow specifying ' in the command string, making it hard to
> protect the restore_command against problematic filenames (whitespace
> etc.). This doesn't seem to be a problem for archive_command, which
> allows \' (e.g. archive_command = '/path/to/script \'%f\' \'%p\'').
> Should this be fixed?

Yes, I'll look into that.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

On Wed, 2006-12-13 at 19:28 +0000, Simon Riggs wrote:
> On Wed, 2006-12-13 at 04:23 +0000, Andrew - Supernews wrote:
> > While testing a PITR recovery, I discovered that recovery.conf doesn't
> > seem to allow specifying ' in the command string, making it hard to
> > protect the restore_command against problematic filenames (whitespace
> > etc.). This doesn't seem to be a problem for archive_command, which
> > allows \' (e.g. archive_command = '/path/to/script \'%f\' \'%p\'').

Immediate workaround is to use a script to encapsulate the actual
desired request.

> > Should this be fixed?
>
> Yes, I'll look into that.

OK, I would propose to extend the guc-file.l to include sufficient code
to allow the parsing of the conf files to be identical between the
postgresql.conf and the recovery.conf (it isn't the same yet).

There'll be care taken to ensure that the various options are not
settable in the wrong file.

Any requests for specific implementation details? I'll be looking to
remove code from xlog.c, if possible.

Implementation won't be immediate because of other current work.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

"Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> OK, I would propose to extend the guc-file.l to include sufficient code
> to allow the parsing of the conf files to be identical between the
> postgresql.conf and the recovery.conf (it isn't the same yet).

It would probably be far easier for long-term maintenance if you just
built an independent lexer, instead of trying to make guc-file.l
serve multiple masters.

regards, tom lane

On 2006-12-13, "Simon Riggs" <simon(at)2ndquadrant(dot)com> wrote:
> On Wed, 2006-12-13 at 19:28 +0000, Simon Riggs wrote:
>> On Wed, 2006-12-13 at 04:23 +0000, Andrew - Supernews wrote:
>> > While testing a PITR recovery, I discovered that recovery.conf doesn't
>> > seem to allow specifying ' in the command string, making it hard to
>> > protect the restore_command against problematic filenames (whitespace
>> > etc.). This doesn't seem to be a problem for archive_command, which
>> > allows \' (e.g. archive_command = '/path/to/script \'%f\' \'%p\'').
>
> Immediate workaround is to use a script to encapsulate the actual
> desired request.

That only helps if you can trust %p not to contain whitespace or $. If it
is always relative to somewhere in the data dir then this is probably ok,
but if it's an absolute path then you can't assume that.

--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services

Andrew - Supernews <andrew+nonews(at)supernews(dot)com> writes:
> That only helps if you can trust %p not to contain whitespace or $. If it
> is always relative to somewhere in the data dir then this is probably ok,
> but if it's an absolute path then you can't assume that.

It is relative, so I think this is actually not a problem. Still, we
ought to make the lexical structure the same as it is in postgresql.conf.

regards, tom lane

On Wed, 2006-12-13 at 17:02 -0500, Tom Lane wrote:
> "Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> > OK, I would propose to extend the guc-file.l to include sufficient code
> > to allow the parsing of the conf files to be identical between the
> > postgresql.conf and the recovery.conf (it isn't the same yet).
>
> It would probably be far easier for long-term maintenance if you just
> built an independent lexer, instead of trying to make guc-file.l
> serve multiple masters.

Will do.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Simon Riggs wrote:
> > It would probably be far easier for long-term maintenance if you
> > just built an independent lexer, instead of trying to make
> > guc-file.l serve multiple masters.
>
> Will do.

I'm actually not so sure that this is a good idea. The lexical
structure should be exactly the same, and some things like include
files might become useful as well, so why should all this be
replicated?

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On Thu, 2006-12-14 at 13:52 +0100, Peter Eisentraut wrote:
> Simon Riggs wrote:
> > > It would probably be far easier for long-term maintenance if you
> > > just built an independent lexer, instead of trying to make
> > > guc-file.l serve multiple masters.
> >
> > Will do.
>
> I'm actually not so sure that this is a good idea. The lexical
> structure should be exactly the same, and some things like include
> files might become useful as well, so why should all this be
> replicated?

I assumed the actual lexer would be the same, just the code that invokes
it would be different. I'm happy to do things either way.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

"Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> On Thu, 2006-12-14 at 13:52 +0100, Peter Eisentraut wrote:
>> I'm actually not so sure that this is a good idea. The lexical
>> structure should be exactly the same, and some things like include
>> files might become useful as well, so why should all this be
>> replicated?

> I assumed the actual lexer would be the same, just the code that invokes
> it would be different. I'm happy to do things either way.

Yeah, but the actual lexer is [ looks... ] less than 50 lines of flex
code. I think refactoring stuff to the point where that could be shared
would be more pain than it's worth ...

regards, tom lane

Added to TODO:

o Allow recovery.conf to allow the same syntax as
postgresql.conf, including quoting

http://archives.postgresql.org/pgsql-hackers/2006-12/msg00497.php

---------------------------------------------------------------------------

Simon Riggs wrote:
> On Wed, 2006-12-13 at 19:28 +0000, Simon Riggs wrote:
> > On Wed, 2006-12-13 at 04:23 +0000, Andrew - Supernews wrote:
> > > While testing a PITR recovery, I discovered that recovery.conf doesn't
> > > seem to allow specifying ' in the command string, making it hard to
> > > protect the restore_command against problematic filenames (whitespace
> > > etc.). This doesn't seem to be a problem for archive_command, which
> > > allows \' (e.g. archive_command = '/path/to/script \'%f\' \'%p\'').
>
> Immediate workaround is to use a script to encapsulate the actual
> desired request.
>
> > > Should this be fixed?
> >
> > Yes, I'll look into that.
>
> OK, I would propose to extend the guc-file.l to include sufficient code
> to allow the parsing of the conf files to be identical between the
> postgresql.conf and the recovery.conf (it isn't the same yet).
>
> There'll be care taken to ensure that the various options are not
> settable in the wrong file.
>
> Any requests for specific implementation details? I'll be looking to
> remove code from xlog.c, if possible.
>
> Implementation won't be immediate because of other current work.
>
> --
> Simon Riggs
> EnterpriseDB http://www.enterprisedb.com
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +