Re: Reading recovery.conf earlier

I'm planning to read recovery.conf earlier in the startup process, so we
can make a few things more "recovery aware". It's a nice-to-have only.

This won't be part of the HS patch though.

Proposal is to split out the couple of lines in
readRecoveryCommandFile() that set important state and make it read in
an option block that can be used by caller. It would then be called by
both postmaster (earlier in startup) and again later by startup process,
as happens now. I want to do it that way so I can read file before we
create shared memory, so I don't have to worry about passing details via
shared memory itself.

It will allow some tidy up in HS patch but isn't going to be intrusive.

Not thinking about lexers and stuff though at this stage, even if it is
on the todo list.

Any vetos?

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> I'm planning to read recovery.conf earlier in the startup process, so we
> can make a few things more "recovery aware". It's a nice-to-have only.

Say what? It's read at the beginning already.

regards, tom lane

On Sat, 2009-12-05 at 15:43 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > I'm planning to read recovery.conf earlier in the startup process, so we
> > can make a few things more "recovery aware". It's a nice-to-have only.
>
> Say what? It's read at the beginning already.

Before the beginning then. :-)

Two reasons to move it earlier in the startup sequence of the server

* Some data structures are only required in HS mode. We don't know we're
in HS mode until we created shared memory and started the Startup
process. If we knew ahead of time, we could skip adding the structures.

* Some things in postgresql.conf need to be overridden in HS mode, for
example default_transaction_read_only = false. Again, we don't know
we're in HS mode until later.

So I would want to read recovery.conf before we read postgresql.conf

Also, AFAIK, it's not easily possible to have dependencies between
settings in postgresql.conf, unless the dependencies are expressed by
ordering the dependent parameters in alphabetic order. So putting the
parameters in postgresql.conf wouldn't help much.

--
Simon Riggs www.2ndQuadrant.com

On Sun, Dec 6, 2009 at 2:49 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Proposal is to split out the couple of lines in
> readRecoveryCommandFile() that set important state and make it read in
> an option block that can be used by caller. It would then be called by
> both postmaster (earlier in startup) and again later by startup process,
> as happens now. I want to do it that way so I can read file before we
> create shared memory, so I don't have to worry about passing details via
> shared memory itself.

I agree with the proposal that postmaster reads the recovery.conf.
Because this would enable all child processes to easily obtain the
parameter values in that, like GUC parameters.

But I'm not sure why recovery.conf should be read separately by
postmaster and the startup process. How about making postmaster
read all of that for the simplification?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Mon, 2009-12-07 at 13:03 +0900, Fujii Masao wrote:
> On Sun, Dec 6, 2009 at 2:49 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > Proposal is to split out the couple of lines in
> > readRecoveryCommandFile() that set important state and make it read in
> > an option block that can be used by caller. It would then be called by
> > both postmaster (earlier in startup) and again later by startup process,
> > as happens now. I want to do it that way so I can read file before we
> > create shared memory, so I don't have to worry about passing details via
> > shared memory itself.
>
> I agree with the proposal that postmaster reads the recovery.conf.
> Because this would enable all child processes to easily obtain the
> parameter values in that, like GUC parameters.

OK

> But I'm not sure why recovery.conf should be read separately by
> postmaster and the startup process. How about making postmaster
> read all of that for the simplification?

That sounds better but is more complex. We'd have to read in the file,
store in memory, then after shared memory is up put the data somewhere
so the startup process can read it. (Remember Windows).

What postgresql.conf already does is read file separately in each
process, so no data passing.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs wrote:
> What postgresql.conf already does is read file separately in each
> process, so no data passing.

No it doesn't. Postmaster reads the file once, and backends inherit the
values at fork(). In EXEC_BACKEND case, postmaster writes all the
non-default values to a separate file, which the child process reads at
startup.

Reading the file separately in each process would cause trouble with
PGC_POSTMASTER params. All backends must agree on their values.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Mon, 2009-12-07 at 10:13 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > What postgresql.conf already does is read file separately in each
> > process, so no data passing.
>
> No it doesn't. Postmaster reads the file once, and backends inherit the
> values at fork(). In EXEC_BACKEND case, postmaster writes all the
> non-default values to a separate file, which the child process reads at
> startup.

As I mentioned, the difficulty is always the Windows case. I wasn't
aware we wrote a separate file in that case, so that does potentially
effect my thinking.

> Reading the file separately in each process would cause trouble with
> PGC_POSTMASTER params. All backends must agree on their values.

Looking at the parameters in recovery.conf I don't believe they would
cause problems if read twice. So I think reading file twice would still
be simplest way forwards.

If you really think that changing the file is a possibility between
processes reading them, then I would just take a full temp copy of the
file, read it in postmaster, read it in startup, then delete temp file.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs wrote:
> On Mon, 2009-12-07 at 10:13 +0200, Heikki Linnakangas wrote:
>> Reading the file separately in each process would cause trouble with
>> PGC_POSTMASTER params. All backends must agree on their values.
>
> Looking at the parameters in recovery.conf I don't believe they would
> cause problems if read twice. So I think reading file twice would still
> be simplest way forwards.

Yeah, I don't know if that's an issue for recovery.conf, I was just
pointing out that that's one reason it's done that way for postgresql.conf.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Mon, Dec 7, 2009 at 5:28 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> If you really think that changing the file is a possibility between
> processes reading them, then I would just take a full temp copy of the
> file, read it in postmaster, read it in startup, then delete temp file.

This seems more robust because processes which are started long after
postmaster has started might use recovery.conf in the future (e.g.,
walreceiver in SR, read-only backends).

Also, in Windows, writing only non-default values into a temp file like
GUC is good for a process (like backend) which might be started many times.
Reading the full of the file every startup of such process would somewhat
harm the performance. Though, of course, this is overkill for your purpose.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Mon, 2009-12-07 at 19:26 +0900, Fujii Masao wrote:
> On Mon, Dec 7, 2009 at 5:28 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > If you really think that changing the file is a possibility between
> > processes reading them, then I would just take a full temp copy of the
> > file, read it in postmaster, read it in startup, then delete temp file.
>
> This seems more robust because processes which are started long after
> postmaster has started might use recovery.conf in the future (e.g.,
> walreceiver in SR, read-only backends).

How does this sound?

At startup we will delete recovery.conf.running, if it exists.
At startup recovery.conf will be copied to recovery.conf.running, which
will be the file read by any additional processes that read
recovery.conf during this execution. The permissions on the file will
then be set to read-only.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs wrote:
> How does this sound?
>
> At startup we will delete recovery.conf.running, if it exists.
> At startup recovery.conf will be copied to recovery.conf.running, which
> will be the file read by any additional processes that read
> recovery.conf during this execution. The permissions on the file will
> then be set to read-only.

Why not just follow the example of postresql.conf?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Mon, 2009-12-07 at 19:07 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > How does this sound?
> >
> > At startup we will delete recovery.conf.running, if it exists.
> > At startup recovery.conf will be copied to recovery.conf.running, which
> > will be the file read by any additional processes that read
> > recovery.conf during this execution. The permissions on the file will
> > then be set to read-only.
>
> Why not just follow the example of postresql.conf?

Much better idea.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Mon, 2009-12-07 at 19:07 +0200, Heikki Linnakangas wrote:
>> Why not just follow the example of postresql.conf?

> Much better idea.

Rather than reinventing all the infrastructure associated with GUCs,
maybe we should just make the recovery parameters *be* GUCs. At least
for all the ones that could be of interest outside the recovery
subprocess itself.

As an example of the kind of thing you'll find yourself coding if you
make an independent facility: how will people find out the active
values?

regards, tom lane

On Mon, 2009-12-07 at 19:21 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > On Mon, 2009-12-07 at 19:07 +0200, Heikki Linnakangas wrote:
> >> Why not just follow the example of postresql.conf?
>
> > Much better idea.
>
> Rather than reinventing all the infrastructure associated with GUCs,
> maybe we should just make the recovery parameters *be* GUCs. At least
> for all the ones that could be of interest outside the recovery
> subprocess itself.
>
> As an example of the kind of thing you'll find yourself coding if you
> make an independent facility: how will people find out the active
> values?

You're right, I was literally just writing that code.

Also, currently I have two parameters: wal_standby_info and
recovery_connections. If this was a GUC, then I could just have one
parameter: recovery_connections.

So, much agreed.

--
Simon Riggs www.2ndQuadrant.com