GUC patch for Win32

Bruce Momjian <pgman@candle.pha.pa.us> writes:

The patch basically writes all modified GUC variables to a binary file
that can be read in by newly created exec'ed backends.

Where exactly is the interlock to ensure that the new backend will end up
with the correct settings if someone is changing the values at about
the time of the fork?

The relcache code has this problem solved, but I'm unconvinced that GUC
does.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

The patch basically writes all modified GUC variables to a binary file
that can be read in by newly created exec'ed backends.

Where exactly is the interlock to ensure that the new backend will end up
with the correct settings if someone is changing the values at about
the time of the fork?

Postmaster creates a new file, then does rename() to move it to the name
used by the backends. It can't move it until the file is not in use.

The relcache code has this problem solved, but I'm unconvinced that GUC
does.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

The other nice thing is that it only creates the file on startup and on
sighup, so it obeys the normal Unix behavior.

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

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

The patch basically writes all modified GUC variables to a binary file
that can be read in by newly created exec'ed backends.

Where exactly is the interlock to ensure that the new backend will end up
with the correct settings if someone is changing the values at about
the time of the fork?

The relcache code has this problem solved, but I'm unconvinced that GUC
does.

regards, tom lane

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Where exactly is the interlock to ensure that the new backend will end up
with the correct settings if someone is changing the values at about
the time of the fork?

Postmaster creates a new file, then does rename() to move it to the name
used by the backends. It can't move it until the file is not in use.

And?

How exactly does that guarantee that the new backend will see an update
occurring at about the same time? I'm pretty sure that GUC is fired up
before backends start listening to signals (and that's assuming the
Windows port has a Unixy idea of signal response, which I seem to recall
you telling me wasn't the case).

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Where exactly is the interlock to ensure that the new backend will end up
with the correct settings if someone is changing the values at about
the time of the fork?

Postmaster creates a new file, then does rename() to move it to the name
used by the backends. It can't move it until the file is not in use.

And?

How exactly does that guarantee that the new backend will see an update
occurring at about the same time? I'm pretty sure that GUC is fired up
before backends start listening to signals (and that's assuming the
Windows port has a Unixy idea of signal response, which I seem to recall
you telling me wasn't the case).

Oh, I am not sure. I haven't gotten the signal stuff done yet.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Applied. I have added a note to check for file changes before signal
handler is installed in child.

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

Bruce Momjian wrote:

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Where exactly is the interlock to ensure that the new backend will end up
with the correct settings if someone is changing the values at about
the time of the fork?

Postmaster creates a new file, then does rename() to move it to the name
used by the backends. It can't move it until the file is not in use.

And?

How exactly does that guarantee that the new backend will see an update
occurring at about the same time? I'm pretty sure that GUC is fired up
before backends start listening to signals (and that's assuming the
Windows port has a Unixy idea of signal response, which I seem to recall
you telling me wasn't the case).

Oh, I am not sure. I haven't gotten the signal stuff done yet.

-- 
Bruce Momjian                        |  http://candle.pha.pa.us
pgman@candle.pha.pa.us               |  (610) 359-1001
+  If your life is a hard drive,     |  13 Roberts Road
+  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

What about all the other global variables? Will there be a separate
mechanism for each kind?

Bruce Momjian writes:

Here is a patch that saves the postmaster GUC state into a file to be
read in by exec'ed backends. This is a start toward a fork/exec option
for Unix (for testing) and a CreateProcess option for Win32.

The patch basically writes all modified GUC variables to a binary file
that can be read in by newly created exec'ed backends.

--
Peter Eisentraut peter_e@gmx.net

Peter Eisentraut wrote:

What about all the other global variables? Will there be a separate
mechanism for each kind?

Good question. I am not sure yet. I figured I would hit the GUC ones
first because they are easy and all in one place, then see what others
exist in the Peer Direct patch.

Did I hit all the GUC structure members that need to be dumped --- name,
value, source?

Bruce Momjian writes:

Here is a patch that saves the postmaster GUC state into a file to be
read in by exec'ed backends. This is a start toward a fork/exec option
for Unix (for testing) and a CreateProcess option for Win32.

The patch basically writes all modified GUC variables to a binary file
that can be read in by newly created exec'ed backends.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian writes:

Peter Eisentraut wrote:

What about all the other global variables? Will there be a separate
mechanism for each kind?

Good question. I am not sure yet. I figured I would hit the GUC ones
first because they are easy and all in one place, then see what others
exist in the Peer Direct patch.

Well, the question of what to do with all the global variables seems to be
a central point in the port, given that no fork() function exists. So
I think that should be solved before piecewise solutions for some
variables are installed that will become obsolete later on.

--
Peter Eisentraut peter_e@gmx.net

Peter Eisentraut wrote:

Bruce Momjian writes:

Peter Eisentraut wrote:

What about all the other global variables? Will there be a separate
mechanism for each kind?

Good question. I am not sure yet. I figured I would hit the GUC ones
first because they are easy and all in one place, then see what others
exist in the Peer Direct patch.

Well, the question of what to do with all the global variables seems to be
a central point in the port, given that no fork() function exists. So
I think that should be solved before piecewise solutions for some
variables are installed that will become obsolete later on.

Well, I am pretty far along and haven't seen tons of problems with
globals yet --- most of the globals don't pass from postmaster to
backend, I guess.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Where exactly is the interlock to ensure that the new backend will end up
with the correct settings if someone is changing the values at about
the time of the fork?

Postmaster creates a new file, then does rename() to move it to the name
used by the backends. It can't move it until the file is not in use.

And?

How exactly does that guarantee that the new backend will see an update
occurring at about the same time? I'm pretty sure that GUC is fired up
before backends start listening to signals (and that's assuming the
Windows port has a Unixy idea of signal response, which I seem to recall
you telling me wasn't the case).

So far the postmaster is not multi-threaded, so it will not create a new
file and start a backend at the same time. Also, the rename() call is
supposed to be atomic. So there is allways a file, and it's either the
old or the new one, never something in between.

Jan

--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #

Jan Wieck <JanWieck@Yahoo.com> writes:

So far the postmaster is not multi-threaded, so it will not create a new
file and start a backend at the same time. Also, the rename() call is
supposed to be atomic. So there is allways a file, and it's either the
old or the new one, never something in between.

(a) rename isn't atomic on Windows, I thought.

(b) I'm still not convinced that there's no race condition in SIGHUP.
(It doesn't help that SIGHUP_handler does SignalChildren() *before*
reading the file for itself --- that looks wrong.)

It occurs to me though that there need be no problem because the only
GUC variables that the postmaster really needs to send to children
are the ones it gets from its command line and environment. Updates
coming from postgresql.conf are not a problem because the children
can get them for themselves (as already-launched backends surely must).

Thus, write_nondefault_variables is misdesigned: it should be called
*once*, not during SIGHUP_handler, and should only be responsible for
writing out values that came from PGC_S_ENV_VAR or PGC_S_ARGV sources
(maybe also PGC_S_OVERRIDE, not sure if postmaster startup uses that).
That way the file never changes after postmaster start and there can
be no race condition. Children will instead have to read
postgresql.conf for themselves during their launch (after they read the
nondefault_variables file), but that's an easy one-line addition.

regards, tom lane

I said:

That way the file never changes after postmaster start and there can
be no race condition. Children will instead have to read
postgresql.conf for themselves during their launch (after they read the
nondefault_variables file), but that's an easy one-line addition.

On second thought, that just moves the race condition upstream (to the
person editing postgresql.conf) ... so never mind that idea. We don't
want any part of Postgres reading postgresql.conf except shortly after
someone has SIGHUP'd the postmaster.

But I'm still wondering if the order of operations in SIGHUP_handler is
wrong.

regards, tom lane

Tom Lane wrote:

I said:

That way the file never changes after postmaster start and there can
be no race condition. Children will instead have to read
postgresql.conf for themselves during their launch (after they read the
nondefault_variables file), but that's an easy one-line addition.

On second thought, that just moves the race condition upstream (to the
person editing postgresql.conf) ... so never mind that idea. We don't
want any part of Postgres reading postgresql.conf except shortly after
someone has SIGHUP'd the postmaster.

I didn't actually look at that part of the code yet ... why would any
backend read postgresql.conf? I thought only the postmaster does that
and that he SIGHUP's all backends after writing the new file. That way
there is not race condition other that hupping the PM while w'ing the
file.

Jan

--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #

Jan Wieck <JanWieck@Yahoo.com> writes:

Tom Lane wrote:

On second thought, that just moves the race condition upstream (to the
person editing postgresql.conf) ... so never mind that idea.

I didn't actually look at that part of the code yet ... why would any
backend read postgresql.conf?

At SIGHUP, the postmaster re-reads postgresql.conf, and each backend
must do so too --- there's no other way for pre-existing backends to
learn about changed values. The assumption is that this happens over a
short enough timespan that it's okay from the perspective of the person
editing postgresql.conf. But if newly started backends were to read
postgresql.conf when they start, that would be bad news for someone
trying to edit postgresql.conf, because there'd be no way to know when
it would happen.

regards, tom lane

Tom Lane wrote:

Jan Wieck <JanWieck@Yahoo.com> writes:

Tom Lane wrote:

On second thought, that just moves the race condition upstream (to the
person editing postgresql.conf) ... so never mind that idea.

I didn't actually look at that part of the code yet ... why would any
backend read postgresql.conf?

At SIGHUP, the postmaster re-reads postgresql.conf, and each backend
must do so too --- there's no other way for pre-existing backends to
learn about changed values. The assumption is that this happens over a
short enough timespan that it's okay from the perspective of the person
editing postgresql.conf. But if newly started backends were to read
postgresql.conf when they start, that would be bad news for someone
trying to edit postgresql.conf, because there'd be no way to know when
it would happen.

Sure is there ... the way I did it originally.

The postmaster reads the config file and overrides with commandline
options. Then he dumps ALL options into a separate file.

A backend does not need to read the config file or parse commandline at
all. It just jeads the postmaster provided file.

If the user changes the config and HUP's the postmaster, postmaster
rereads the config and merges only those changes, that are changable at
runtime into it's status. From that status it creates the new file,
renames, HUP's the backends and they reread that file.

Jan

--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #

Jan Wieck <JanWieck@Yahoo.com> writes:

If the user changes the config and HUP's the postmaster, postmaster
rereads the config and merges only those changes, that are changable at
runtime into it's status. From that status it creates the new file,
renames, HUP's the backends and they reread that file.

That just moves any potential race conditions to another place, doesn't it?
How's reading this file any safer than reading postgresql.conf? If the
PM gets a second SIGHUP in quick succession, it could be rewriting the
intermediate file while backends are trying to read it.

regards, tom lane

Tom Lane wrote:

Jan Wieck <JanWieck@Yahoo.com> writes:

If the user changes the config and HUP's the postmaster, postmaster
rereads the config and merges only those changes, that are changable at
runtime into it's status. From that status it creates the new file,
renames, HUP's the backends and they reread that file.

That just moves any potential race conditions to another place, doesn't it?
How's reading this file any safer than reading postgresql.conf? If the
PM gets a second SIGHUP in quick succession, it could be rewriting the
intermediate file while backends are trying to read it.

I have applied the following patch to improve the race condition. With
the old code, the nondefault setting file would be written after telling
the children to processing the nondefault setting file. Now, the
nondefaults file is written before sending the children the SIGHUP.
This leaves the only race condition as when a new child is reading the
the nondefaults file for the first time. I will make sure WIN32 doesn't
lose signals during startup time and processes the new version of the
file as well.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073