Thread: postgresql.conf error checking strategy
I just spent a rather confused half hour while testing my GUC
assign-hook patch, and when I finally figured out what was happening,
it made me wonder whether we should redesign the behavior a little bit.
The current behavior of ProcessConfigFile is that it runs through all
the "name = value" pairs extracted from the file and tries to fully
verify each value (by seeing whether set_config_option with changeVal
false likes it). Only if every one of them checks out does it actually
apply any of the settings. Now this is nice and conservative --- the
aim is to avoid applying settings from a possibly corrupted file, in
case somebody fat-fingered their edits in a big way. But there's a
little problem:
1. It's possible that not all the backends agree on whether a setting
is valid. The case I was testing involved setting client_encoding
from the config file, so whether it succeeds depends on the database
encoding (some conversions might exist and others not). This means
that some backends might apply the postgresql.conf settings and others
not. That's pretty bad in itself, if something that needs to be
consistent system-wide is changing.
2. Only the postmaster reports config file problems at elevel LOG;
backends only complain at DEBUG3, to avoid cluttering the log with
lots of duplicate messages. This means that if you do have a few
backends that fail to adopt a setting, there likely won't be anything
in the log to tell you so. (The reason I was so confused is that I'd
raised log_min_messages to DEBUG5 to try to understand what was
happening ... but my backend-under-test wasn't adopting that setting,
and wasn't logging anything to tell me so either ...)
So I'm thinking we should adopt a strategy that's less likely to result
in divergent behavior among different backends. The idea I have in mind
is to have the first "validation" pass only check that each name is a
legal GUC variable name, and not look at the values at all. If so, try
to apply all the values. Any that fail to apply we log as usual, but
still apply the others. ISTM that verifying the names should be enough
protection against broken files for practical purposes, and it should be
something that all backends will agree on even if there are individual
values that are not valid for all.
Comments?
regards, tom lane
On Wed, Apr 6, 2011 at 10:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > So I'm thinking we should adopt a strategy that's less likely to result > in divergent behavior among different backends. The idea I have in mind > is to have the first "validation" pass only check that each name is a > legal GUC variable name, and not look at the values at all. If so, try > to apply all the values. Any that fail to apply we log as usual, but > still apply the others. ISTM that verifying the names should be enough > protection against broken files for practical purposes, and it should be > something that all backends will agree on even if there are individual > values that are not valid for all. > Would it be possible to have a) a policy that GUCs should verify or fail to verify consistently for all backends and b) a way for the backends to scream loudly if they come to a different conclusion than the master when reloading the file? -- greg
On Wed, Apr 6, 2011 at 5:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I just spent a rather confused half hour while testing my GUC > assign-hook patch, and when I finally figured out what was happening, > it made me wonder whether we should redesign the behavior a little bit. > > The current behavior of ProcessConfigFile is that it runs through all > the "name = value" pairs extracted from the file and tries to fully > verify each value (by seeing whether set_config_option with changeVal > false likes it). Only if every one of them checks out does it actually > apply any of the settings. Now this is nice and conservative --- the > aim is to avoid applying settings from a possibly corrupted file, in > case somebody fat-fingered their edits in a big way. But there's a > little problem: > > 1. It's possible that not all the backends agree on whether a setting > is valid. The case I was testing involved setting client_encoding > from the config file, so whether it succeeds depends on the database > encoding (some conversions might exist and others not). This means > that some backends might apply the postgresql.conf settings and others > not. That's pretty bad in itself, if something that needs to be > consistent system-wide is changing. > > 2. Only the postmaster reports config file problems at elevel LOG; > backends only complain at DEBUG3, to avoid cluttering the log with > lots of duplicate messages. This means that if you do have a few > backends that fail to adopt a setting, there likely won't be anything > in the log to tell you so. (The reason I was so confused is that I'd > raised log_min_messages to DEBUG5 to try to understand what was > happening ... but my backend-under-test wasn't adopting that setting, > and wasn't logging anything to tell me so either ...) > > So I'm thinking we should adopt a strategy that's less likely to result > in divergent behavior among different backends. The idea I have in mind > is to have the first "validation" pass only check that each name is a > legal GUC variable name, and not look at the values at all. If so, try > to apply all the values. Any that fail to apply we log as usual, but > still apply the others. ISTM that verifying the names should be enough > protection against broken files for practical purposes, and it should be > something that all backends will agree on even if there are individual > values that are not valid for all. > > Comments? I don't think now is a good time for a major behavior change in this area, and I'm not convinced this is the best possible design. There are a number of parameters which are currently PGC_POSTMASTER rather than PGC_SIGHUP precisely because of the possibility of backends being out of step with each other. wal_level is an obvious example, and one that it would be *really* nice to be able to change without a server restart. It would be nice to have a real solution to that problem, but this isn't it, and I don't want to engineer it right now. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: > On Wed, Apr 6, 2011 at 5:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I just spent a rather confused half hour while testing my GUC > > assign-hook patch, and when I finally figured out what was happening, > > it made me wonder whether we should redesign the behavior a little bit. > > > > The current behavior of ProcessConfigFile is that it runs through all > > the "name = value" pairs extracted from the file and tries to fully > > verify each value (by seeing whether set_config_option with changeVal > > false likes it). ?Only if every one of them checks out does it actually > > apply any of the settings. ?Now this is nice and conservative --- the > > aim is to avoid applying settings from a possibly corrupted file, in > > case somebody fat-fingered their edits in a big way. ?But there's a > > little problem: > > > > 1. It's possible that not all the backends agree on whether a setting > > is valid. ?The case I was testing involved setting client_encoding > > from the config file, so whether it succeeds depends on the database > > encoding (some conversions might exist and others not). ?This means > > that some backends might apply the postgresql.conf settings and others > > not. ?That's pretty bad in itself, if something that needs to be > > consistent system-wide is changing. > > > > 2. Only the postmaster reports config file problems at elevel LOG; > > backends only complain at DEBUG3, to avoid cluttering the log with > > lots of duplicate messages. ?This means that if you do have a few > > backends that fail to adopt a setting, there likely won't be anything > > in the log to tell you so. ?(The reason I was so confused is that I'd > > raised log_min_messages to DEBUG5 to try to understand what was > > happening ... but my backend-under-test wasn't adopting that setting, > > and wasn't logging anything to tell me so either ...) > > > > So I'm thinking we should adopt a strategy that's less likely to result > > in divergent behavior among different backends. ?The idea I have in mind > > is to have the first "validation" pass only check that each name is a > > legal GUC variable name, and not look at the values at all. ?If so, try > > to apply all the values. ?Any that fail to apply we log as usual, but > > still apply the others. ?ISTM that verifying the names should be enough > > protection against broken files for practical purposes, and it should be > > something that all backends will agree on even if there are individual > > values that are not valid for all. > > > > Comments? > > I don't think now is a good time for a major behavior change in this > area, and I'm not convinced this is the best possible design. > > There are a number of parameters which are currently PGC_POSTMASTER > rather than PGC_SIGHUP precisely because of the possibility of > backends being out of step with each other. wal_level is an obvious > example, and one that it would be *really* nice to be able to change > without a server restart. It would be nice to have a real solution to > that problem, but this isn't it, and I don't want to engineer it right > now. Is this a TODO? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> writes:
> Robert Haas wrote:
>> On Wed, Apr 6, 2011 at 5:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> So I'm thinking we should adopt a strategy that's less likely to result
>>> in divergent behavior among different backends. ?The idea I have in mind
>>> is to have the first "validation" pass only check that each name is a
>>> legal GUC variable name, and not look at the values at all. ?If so, try
>>> to apply all the values. ?Any that fail to apply we log as usual, but
>>> still apply the others. ?ISTM that verifying the names should be enough
>>> protection against broken files for practical purposes, and it should be
>>> something that all backends will agree on even if there are individual
>>> values that are not valid for all.
>>>
>>> Comments?
>> I don't think now is a good time for a major behavior change in this
>> area, and I'm not convinced this is the best possible design.
>>
>> There are a number of parameters which are currently PGC_POSTMASTER
>> rather than PGC_SIGHUP precisely because of the possibility of
>> backends being out of step with each other. wal_level is an obvious
>> example, and one that it would be *really* nice to be able to change
>> without a server restart. It would be nice to have a real solution to
>> that problem, but this isn't it, and I don't want to engineer it right
>> now.
> Is this a TODO?
Yes, definitely. Perhaps summarize as "rethink how we handle partially
correct postgresql.conf files". Or maybe Robert sees it as "rethink
approach to making sure all backends share the same value of critical
settings"? Or maybe those are two different TODOs?
regards, tom lane
On Sun, May 8, 2011 at 1:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Bruce Momjian <bruce@momjian.us> writes: >> Robert Haas wrote: >>> On Wed, Apr 6, 2011 at 5:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>> So I'm thinking we should adopt a strategy that's less likely to result >>>> in divergent behavior among different backends. ?The idea I have in mind >>>> is to have the first "validation" pass only check that each name is a >>>> legal GUC variable name, and not look at the values at all. ?If so, try >>>> to apply all the values. ?Any that fail to apply we log as usual, but >>>> still apply the others. ?ISTM that verifying the names should be enough >>>> protection against broken files for practical purposes, and it should be >>>> something that all backends will agree on even if there are individual >>>> values that are not valid for all. >>>> >>>> Comments? > >>> I don't think now is a good time for a major behavior change in this >>> area, and I'm not convinced this is the best possible design. >>> >>> There are a number of parameters which are currently PGC_POSTMASTER >>> rather than PGC_SIGHUP precisely because of the possibility of >>> backends being out of step with each other. wal_level is an obvious >>> example, and one that it would be *really* nice to be able to change >>> without a server restart. It would be nice to have a real solution to >>> that problem, but this isn't it, and I don't want to engineer it right >>> now. > >> Is this a TODO? > > Yes, definitely. Perhaps summarize as "rethink how we handle partially > correct postgresql.conf files". Or maybe Robert sees it as "rethink > approach to making sure all backends share the same value of critical > settings"? Or maybe those are two different TODOs? The second is what I had in mind. I'm thinking that at least for critical GUCs we need a different mechanism for making sure everything stays in sync, like having the postmaster write a precompiled file and convincing the backends to read it in some carefully synchronized fashion. However, it's not clear to me whether something along those lines (or some other lines) would solve the problem you were complaining about; therefore it's possible, as you say, that there are two separate action items here. Or maybe not: maybe someone can come up with an approach that swats both problems in one go. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, May 8, 2011 at 1:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yes, definitely. �Perhaps summarize as "rethink how we handle partially
>> correct postgresql.conf files". �Or maybe Robert sees it as "rethink
>> approach to making sure all backends share the same value of critical
>> settings"? �Or maybe those are two different TODOs?
> The second is what I had in mind. I'm thinking that at least for
> critical GUCs we need a different mechanism for making sure everything
> stays in sync, like having the postmaster write a precompiled file and
> convincing the backends to read it in some carefully synchronized
> fashion. However, it's not clear to me whether something along those
> lines (or some other lines) would solve the problem you were
> complaining about; therefore it's possible, as you say, that there are
> two separate action items here. Or maybe not: maybe someone can come
> up with an approach that swats both problems in one go.
Well, the thing that was annoying me was that because a backend saw one
value in postgresql.conf as incorrect, it was refusing to apply any
changes at all from postgresql.conf. And worse, there was no log entry
to give any hint what was going on. This doesn't seem to me to have
much to do with the problem you're on about. I agree it's conceivable
that someone might think of a way to solve both issues at once, but
I think we'd better list them as separate TODOs.
regards, tom lane
On Mon, May 9, 2011 at 11:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Sun, May 8, 2011 at 1:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Yes, definitely. Perhaps summarize as "rethink how we handle partially >>> correct postgresql.conf files". Or maybe Robert sees it as "rethink >>> approach to making sure all backends share the same value of critical >>> settings"? Or maybe those are two different TODOs? > >> The second is what I had in mind. I'm thinking that at least for >> critical GUCs we need a different mechanism for making sure everything >> stays in sync, like having the postmaster write a precompiled file and >> convincing the backends to read it in some carefully synchronized >> fashion. However, it's not clear to me whether something along those >> lines (or some other lines) would solve the problem you were >> complaining about; therefore it's possible, as you say, that there are >> two separate action items here. Or maybe not: maybe someone can come >> up with an approach that swats both problems in one go. > > Well, the thing that was annoying me was that because a backend saw one > value in postgresql.conf as incorrect, it was refusing to apply any > changes at all from postgresql.conf. And worse, there was no log entry > to give any hint what was going on. This doesn't seem to me to have > much to do with the problem you're on about. I agree it's conceivable > that someone might think of a way to solve both issues at once, but > I think we'd better list them as separate TODOs. OK by me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: > On Mon, May 9, 2011 at 11:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > >> On Sun, May 8, 2011 at 1:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >>> Yes, definitely. ?Perhaps summarize as "rethink how we handle partially > >>> correct postgresql.conf files". ?Or maybe Robert sees it as "rethink > >>> approach to making sure all backends share the same value of critical > >>> settings"? ?Or maybe those are two different TODOs? > > > >> The second is what I had in mind. ?I'm thinking that at least for > >> critical GUCs we need a different mechanism for making sure everything > >> stays in sync, like having the postmaster write a precompiled file and > >> convincing the backends to read it in some carefully synchronized > >> fashion. ?However, it's not clear to me whether something along those > >> lines (or some other lines) would solve the problem you were > >> complaining about; therefore it's possible, as you say, that there are > >> two separate action items here. ?Or maybe not: maybe someone can come > >> up with an approach that swats both problems in one go. > > > > Well, the thing that was annoying me was that because a backend saw one > > value in postgresql.conf as incorrect, it was refusing to apply any > > changes at all from postgresql.conf. ?And worse, there was no log entry > > to give any hint what was going on. ?This doesn't seem to me to have > > much to do with the problem you're on about. ?I agree it's conceivable > > that someone might think of a way to solve both issues at once, but > > I think we'd better list them as separate TODOs. > > OK by me. Two TODOs added: Allow postgresql.conf settings to be accepted by backends even if somesettings are invalid for those backends * http://archives.postgresql.org/pgsql-hackers/2011-04/msg00330.php * http://archives.postgresql.org/pgsql-hackers/2011-05/msg00375.phpIncomplete itemAllow all backends to receive postgresql.confsettingchanges at the same time * http://archives.postgresql.org/pgsql-hackers/2011-04/msg00330.php * http://archives.postgresql.org/pgsql-hackers/2011-05/msg00375.php -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +