Re: [HACKERS] proposal: a validator for configuration files
On Jul 16, 2011, at 9:55 PM, Tom Lane wrote: I wrote: I think that it might be sensible to have the following behavior: 1. Parse the file, where parse means collect all the name = value pairs. Bail out if we find any syntax errors at that level of detail. (With this patch, we could report some or all of the syntax errors first.) 2. Tentatively apply the new custom_variable_classes setting if any. 3. Check to see whether all the names are valid. If not, report the ones that aren't and bail out. 4. Apply each value. If some of them aren't valid, report that, but continue, and apply all the ones that are valid. We can expect that the postmaster and all backends will agree on the results of steps 1 through 3. They might differ as to the validity of individual values in step 4 (as per my example of a setting that depends on database_encoding), but we should never end up with a situation where a globally correct value is not globally applied. Attached is my first attempt to implement your plan. Basically, I've reshuffled pieces of the ProcessConfigFile on top of my previous patch, dropped verification calls of set_config_option and moved the check for custom_variable_class existence right inside the loop that assigns new values to GUC variables. I'd think that removal of custom_variable_classes or setting it from the extensions could be a separate patch. I appreciate your comments and suggestions. I thought some more about this, and it occurred to me that it's not that hard to foresee a situation where different backends might have different opinions about the results of step 3, ie, different ideas about the set of valid GUC names. This could arise as a result of some of them having a particular extension module loaded and others not. Right now, whether or not you have say plpgsql loaded will not affect your ability to do SET plpgsql.junk = foobar --- as long as plpgsql is listed in custom_variable_classes, we'll accept the command and create a placeholder variable for plpgsql.junk. But it seems perfectly plausible that we might someday try to tighten that up so that once a module has done EmitWarningsOnPlaceholders(plpgsql), we'll no longer allow creation of new placeholders named plpgsql.something. If we did that, we could no longer assume that all backends agree on the set of legal GUC variable names. So that seems like an argument --- not terribly strong, but still an argument --- for doing what I suggested next: The original argument for the current behavior was to avoid applying settings from a thoroughly munged config file, but I think that the checks involved in steps 1-3 would be sufficient to reject files that had major problems. It's possible that step 1 is really sufficient to cover the issue, in which case we could drop the separate step-3 pass and just treat invalid GUC names as a reason to ignore the particular line rather than the whole file. That would make things simpler and faster, and maybe less surprising too. IOW, I'm now pretty well convinced that so long as the configuration file is syntactically valid, we should go ahead and attempt to apply each name = value setting individually, without allowing the invalidity of any one name or value to prevent others from being applied. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support pg_parser_continue_on_error_v4.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Florian Pflug f...@phlo.org writes: A variant of this would be to allow extensions (in the CREATE EXTENSION sense) to declare custom GUCs in their control file. Then we'd only need to load those files, which seems better than loading a shared library. The original patch for the extensions had that feature. It had to be removed, though. The control file value has to be stored in the catalogs and now only backends can look that up once connected to a database. This part worked well. http://git.postgresql.org/gitweb/?p=postgresql-extension.git;a=commit;h=480fa10f4ec7b495cf4f434e6f44a50b94487326 http://git.postgresql.org/gitweb/?p=postgresql-extension.git;a=commit;h=98d802aa1ee12c77c5270c7bc9ed7479360f35b9 IIRC the problem is that now, the postmaster is not seeing cvc at all, and you can have there some custom parameters to set shared memory segments, which only postmaster will take care of. An example of that is the pg_stat_statements contrib. I would love that we find a way around that, btw. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Peter Eisentraut pete...@gmx.net writes: On sön, 2011-07-17 at 00:59 -0400, Tom Lane wrote: Well, we *do* have a C API for that, of a sort. The problem is, what do you do in processes that have not loaded the relevant extension? Those processes that have the extension loaded check the parameter settings in their namespace, those that don't ignore them. Then you don't have any meaningful reporting of whether you have entered valid values --- particularly not with the policy that only the postmaster makes logfile entries about bad values. It'd work but I don't think it's tremendously user-friendly. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Josh Berkus j...@agliodbs.com writes: Hmmm. As someone who often deploys pg.conf changes as part of a production code rollout, I actually like the atomic nature of updating postgresql.conf -- that is, all your changes succeed, or they all fail. If we actually *had* that, I'd agree with you. The problem is that it appears that we have such a behavior, but it fails to work that way in corner cases. My proposal is aimed at making the corner cases less corner-y, by adopting a uniform rule that each backend adopts all the changes it can. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Robert Haas robertmh...@gmail.com writes: On Sun, Jul 17, 2011 at 12:59 AM, Tom Lane t...@sss.pgh.pa.us wrote: I agree custom_variable_classes is conceptually messy, but it's a reasonably lightweight compromise that gives some error checking without requiring a lot of possibly-irrelevant extensions to be loaded into every postgres process. Hmm. Maybe what we need is a mechanism that allows the configuration to be associated a loadable module, and whenever that module is loaded, we also load the associated configuration settings. This is probably terribly syntax, but something like: ALTER LOAD 'plpgsql' SET plpgsql.variable_conflict = 'whatever'; AFAICS, that would remove the need to set variables in postgresql.conf that can't be validated, and so we could just disallow it. No, that only fixes things for the case of setting a variable in the control file. It isn't useful for ALTER ROLE/DATABASE SET. And it still has the problem of forcing every process to load every extension, and as written it would also require the postmaster to read catalogs. OTOH, maybe that's more trouble than can be justified by the size of the problem. Yeah. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Tom, Florian, On the downside, the current behaviour prevents problems if someone changes two interrelated GUCs, but makes a mistake at one of them. For example, someone might drastically lower bgwriter_delay but might botch the matching adjustment of bgwriter_lru_maxpages. That's a fair point, but the current behavior only saves you if the botch is such that the new value is detectably invalid, as opposed to say just a factor of 100 off from what you meant. Not sure that that's all that helpful. Hmmm. As someone who often deploys pg.conf changes as part of a production code rollout, I actually like the atomic nature of updating postgresql.conf -- that is, all your changes succeed, or they all fail. If we add this feature, I'd want there to be an option which allows getting the current all-or-none behavior. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On sön, 2011-07-17 at 00:59 -0400, Tom Lane wrote: Well, we *do* have a C API for that, of a sort. The problem is, what do you do in processes that have not loaded the relevant extension? Those processes that have the extension loaded check the parameter settings in their namespace, those that don't ignore them. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Sun, Jul 17, 2011 at 12:59 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Sat, Jul 16, 2011 at 10:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Is there any way that we could get *rid* of custom_variable_classes? Well, we could just drop it and say you can set any dotted-name GUC you feel like. ...and the fact that we've made them set an extra GUC to shoot themselves in the foot hardly seems like an improvement. I was more thinking along the lines of having loadable modules register custom variable classes at load time, through some sort of C API provided for that purpose, rather than having the user declare a list that may or may not match what the .so files really care about. Well, we *do* have a C API for that, of a sort. The problem is, what do you do in processes that have not loaded the relevant extension? (And no, I don't like the answer of let's force the postmaster to load every extension we want to set any parameters for.) I agree custom_variable_classes is conceptually messy, but it's a reasonably lightweight compromise that gives some error checking without requiring a lot of possibly-irrelevant extensions to be loaded into every postgres process. Hmm. Maybe what we need is a mechanism that allows the configuration to be associated a loadable module, and whenever that module is loaded, we also load the associated configuration settings. This is probably terribly syntax, but something like: ALTER LOAD 'plpgsql' SET plpgsql.variable_conflict = 'whatever'; AFAICS, that would remove the need to set variables in postgresql.conf that can't be validated, and so we could just disallow it. OTOH, maybe that's more trouble than can be justified by the size of the problem. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jul18, 2011, at 01:29 , Robert Haas wrote: Hmm. Maybe what we need is a mechanism that allows the configuration to be associated a loadable module, and whenever that module is loaded, we also load the associated configuration settings. This is probably terribly syntax, but something like: ALTER LOAD 'plpgsql' SET plpgsql.variable_conflict = 'whatever'; A variant of this would be to allow extensions (in the CREATE EXTENSION sense) to declare custom GUCs in their control file. Then we'd only need to load those files, which seems better than loading a shared library. We do expect people to wrap their loadable modules in extensions anyway nowadays, do we? best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
2011/7/18 Florian Pflug f...@phlo.org: On Jul18, 2011, at 01:29 , Robert Haas wrote: Hmm. Maybe what we need is a mechanism that allows the configuration to be associated a loadable module, and whenever that module is loaded, we also load the associated configuration settings. This is probably terribly syntax, but something like: ALTER LOAD 'plpgsql' SET plpgsql.variable_conflict = 'whatever'; A variant of this would be to allow extensions (in the CREATE EXTENSION sense) to declare custom GUCs in their control file. Then we'd only need to load those files, which seems better than loading a shared library. +1 Pavel We do expect people to wrap their loadable modules in extensions anyway nowadays, do we? best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
I wrote: I think that it might be sensible to have the following behavior: 1. Parse the file, where parse means collect all the name = value pairs. Bail out if we find any syntax errors at that level of detail. (With this patch, we could report some or all of the syntax errors first.) 2. Tentatively apply the new custom_variable_classes setting if any. 3. Check to see whether all the names are valid. If not, report the ones that aren't and bail out. 4. Apply each value. If some of them aren't valid, report that, but continue, and apply all the ones that are valid. We can expect that the postmaster and all backends will agree on the results of steps 1 through 3. They might differ as to the validity of individual values in step 4 (as per my example of a setting that depends on database_encoding), but we should never end up with a situation where a globally correct value is not globally applied. I thought some more about this, and it occurred to me that it's not that hard to foresee a situation where different backends might have different opinions about the results of step 3, ie, different ideas about the set of valid GUC names. This could arise as a result of some of them having a particular extension module loaded and others not. Right now, whether or not you have say plpgsql loaded will not affect your ability to do SET plpgsql.junk = foobar --- as long as plpgsql is listed in custom_variable_classes, we'll accept the command and create a placeholder variable for plpgsql.junk. But it seems perfectly plausible that we might someday try to tighten that up so that once a module has done EmitWarningsOnPlaceholders(plpgsql), we'll no longer allow creation of new placeholders named plpgsql.something. If we did that, we could no longer assume that all backends agree on the set of legal GUC variable names. So that seems like an argument --- not terribly strong, but still an argument --- for doing what I suggested next: The original argument for the current behavior was to avoid applying settings from a thoroughly munged config file, but I think that the checks involved in steps 1-3 would be sufficient to reject files that had major problems. It's possible that step 1 is really sufficient to cover the issue, in which case we could drop the separate step-3 pass and just treat invalid GUC names as a reason to ignore the particular line rather than the whole file. That would make things simpler and faster, and maybe less surprising too. IOW, I'm now pretty well convinced that so long as the configuration file is syntactically valid, we should go ahead and attempt to apply each name = value setting individually, without allowing the invalidity of any one name or value to prevent others from being applied. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jul16, 2011, at 20:55 , Tom Lane wrote: The original argument for the current behavior was to avoid applying settings from a thoroughly munged config file, but I think that the checks involved in steps 1-3 would be sufficient to reject files that had major problems. It's possible that step 1 is really sufficient to cover the issue, in which case we could drop the separate step-3 pass and just treat invalid GUC names as a reason to ignore the particular line rather than the whole file. That would make things simpler and faster, and maybe less surprising too. IOW, I'm now pretty well convinced that so long as the configuration file is syntactically valid, we should go ahead and attempt to apply each name = value setting individually, without allowing the invalidity of any one name or value to prevent others from being applied. One benefit of this would be that it'd make the logic of ProcessConfigFile and its interaction with set_config_option() much simpler, and the behaviour more predictable. Given that it took me a while to work through the interactions of these two functions, I all for that. On the downside, the current behaviour prevents problems if someone changes two interrelated GUCs, but makes a mistake at one of them. For example, someone might drastically lower bgwriter_delay but might botch the matching adjustment of bgwriter_lru_maxpages. Not sure if that out-weights the benefits, but I thought I'd bring it up. best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Florian Pflug f...@phlo.org writes: On the downside, the current behaviour prevents problems if someone changes two interrelated GUCs, but makes a mistake at one of them. For example, someone might drastically lower bgwriter_delay but might botch the matching adjustment of bgwriter_lru_maxpages. That's a fair point, but the current behavior only saves you if the botch is such that the new value is detectably invalid, as opposed to say just a factor of 100 off from what you meant. Not sure that that's all that helpful. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jul16, 2011, at 21:23 , Tom Lane wrote: Florian Pflug f...@phlo.org writes: On the downside, the current behaviour prevents problems if someone changes two interrelated GUCs, but makes a mistake at one of them. For example, someone might drastically lower bgwriter_delay but might botch the matching adjustment of bgwriter_lru_maxpages. That's a fair point, but the current behavior only saves you if the botch is such that the new value is detectably invalid, as opposed to say just a factor of 100 off from what you meant. Not sure that that's all that helpful. True. And a forgotten zero or wrong unit probably is even more likely than a totally botched value. So +1 from me. Btw, if we touch that, I think we should think about providing some way to detect when a backend fails to apply a value. Showing the precise option that caused the trouble is probably hard, but could we add a flag to PGPROC that gets set once a backend fails to apply some setting on SIGUP? If we showed the state of such a flag in pg_stat_activity, it'd give an admin a quick way of verifying that all is well after a SIGUP. We might also want to record the timestamp of the last processed file so that backends which haven't yet processed the SIHUP can also be detected. best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Florian Pflug f...@phlo.org writes: Btw, if we touch that, I think we should think about providing some way to detect when a backend fails to apply a value. Hm, maybe, but keep in mind that there are valid reasons for a backend to ignore a postgresql.conf setting --- in particular, it might have a local override from some flavor of SET command. So I don't think we'd want the flag to have the semantics of this backend is actually *using* the value; and yet, if that's not what it means, people could still be confused. There might be some implementation gotchas as well. I'm not sure offhand how thoroughly the GUC code checks a value that is being overridden. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jul16, 2011, at 22:55 , Tom Lane wrote: Florian Pflug f...@phlo.org writes: Btw, if we touch that, I think we should think about providing some way to detect when a backend fails to apply a value. Hm, maybe, but keep in mind that there are valid reasons for a backend to ignore a postgresql.conf setting --- in particular, it might have a local override from some flavor of SET command. So I don't think we'd want the flag to have the semantics of this backend is actually *using* the value; Yeah, the flag would simply indicate whether a particular backend encountered an error during config file reload or not. Actually being able to inspect other backend's GUCs would be nice, but is way beyond the scope of this of course. and yet, if that's not what it means, people could still be confused. Hm, if it's called cfgfile_valid or a prettier version thereof I think the risk is small. There might be some implementation gotchas as well. I'm not sure offhand how thoroughly the GUC code checks a value that is being overridden. If it doesn't, then what happens when the overriding scope ends, and the value reverts (or attempts to revert) to its default? best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Fri, Jul 15, 2011 at 8:02 PM, Tom Lane t...@sss.pgh.pa.us wrote: 2. Tentatively apply the new custom_variable_classes setting if any. Is there any way that we could get *rid* of custom_variable_classes? The idea of using a GUC to define the set of valid GUCs seems intrinsically problematic. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Robert Haas robertmh...@gmail.com writes: On Fri, Jul 15, 2011 at 8:02 PM, Tom Lane t...@sss.pgh.pa.us wrote: 2. Tentatively apply the new custom_variable_classes setting if any. Is there any way that we could get *rid* of custom_variable_classes? The idea of using a GUC to define the set of valid GUCs seems intrinsically problematic. Well, we could just drop it and say you can set any dotted-name GUC you feel like. The only reason to have it is to have some modicum of error checking ... but I'm not sure why we should bother if there's no checking on the second half of the name. Not sure if that's going too far in the laissez-faire direction, though. I can definitely imagine people complaining I set plpqsql.variable_conflict in postgresql.conf and it didn't do anything, how come? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Sat, Jul 16, 2011 at 10:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Fri, Jul 15, 2011 at 8:02 PM, Tom Lane t...@sss.pgh.pa.us wrote: 2. Tentatively apply the new custom_variable_classes setting if any. Is there any way that we could get *rid* of custom_variable_classes? The idea of using a GUC to define the set of valid GUCs seems intrinsically problematic. Well, we could just drop it and say you can set any dotted-name GUC you feel like. The only reason to have it is to have some modicum of error checking ... but I'm not sure why we should bother if there's no checking on the second half of the name. Not sure if that's going too far in the laissez-faire direction, though. I can definitely imagine people complaining I set plpqsql.variable_conflict in postgresql.conf and it didn't do anything, how come? I'm not sure that's really making anything any better. Maybe I'm misunderstanding, but if that's going to be a problem, then presumably this will create the same problem: custom_variable_classes='plpgsql' plpgsql.variable_conflict='on' ...and the fact that we've made them set an extra GUC to shoot themselves in the foot hardly seems like an improvement. I was more thinking along the lines of having loadable modules register custom variable classes at load time, through some sort of C API provided for that purpose, rather than having the user declare a list that may or may not match what the .so files really care about. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Robert Haas robertmh...@gmail.com writes: On Sat, Jul 16, 2011 at 10:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Is there any way that we could get *rid* of custom_variable_classes? Well, we could just drop it and say you can set any dotted-name GUC you feel like. ...and the fact that we've made them set an extra GUC to shoot themselves in the foot hardly seems like an improvement. I was more thinking along the lines of having loadable modules register custom variable classes at load time, through some sort of C API provided for that purpose, rather than having the user declare a list that may or may not match what the .so files really care about. Well, we *do* have a C API for that, of a sort. The problem is, what do you do in processes that have not loaded the relevant extension? (And no, I don't like the answer of let's force the postmaster to load every extension we want to set any parameters for.) I agree custom_variable_classes is conceptually messy, but it's a reasonably lightweight compromise that gives some error checking without requiring a lot of possibly-irrelevant extensions to be loaded into every postgres process. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Excerpts from Alexey Kluykin's message of jue jul 14 09:18:15 -0400 2011: On Jul 14, 2011, at 4:38 AM, Alvaro Herrera wrote: On Jul14, 2011, at 01:38 , Alvaro Herrera wrote: This is happening because a check for total number of errors so far is happening only after coming across at least one non-recognized configuration option. What about adding one more check right after ParseConfigFile, so we can bail out early when overwhelmed with syntax errors? This would save a line in translation :). Actually I think it would make sense to do it completely the other way around: reset the number of errors to zero before starting to apply the values. The rationale is that all the settings that made it past the tokenisation are completely different lines for which the errors were reported earlier. I know I'd feel more comfortable if you (and Alexey, and Selena) gave it another look :-) I have checked it here and don't see any more problems with it. Thanks. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Alvaro Herrera alvhe...@commandprompt.com writes: Excerpts from Alexey Kluykin's message of jue jul 14 09:18:15 -0400 2011: This is happening because a check for total number of errors so far is happening only after coming across at least one non-recognized configuration option. What about adding one more check right after ParseConfigFile, so we can bail out early when overwhelmed with syntax errors? This would save a line in translation :). Actually I think it would make sense to do it completely the other way around: reset the number of errors to zero before starting to apply the values. The rationale is that all the settings that made it past the tokenisation are completely different lines for which the errors were reported earlier. I'd like to re-open the discussion from here http://archives.postgresql.org/pgsql-hackers/2011-06/msg01741.php http://archives.postgresql.org/pgsql-hackers/2011-04/msg00330.php specifically about this concern: Does that mean that some backends might currently choose to ignore an updated config file wholesale on SIGUP (because some settings are invalid) while others happily apply it? Meaning that they'll afterwards disagree even on modified settings which *would* be valid for both backends? I complained about exactly that point back in April, but at the time people (quite reasonably) didn't want to touch the behavior. Now that we are entering a new development cycle, it's time to reconsider. Particularly so if we're considering a patch that touches the behavior already. I think that it might be sensible to have the following behavior: 1. Parse the file, where parse means collect all the name = value pairs. Bail out if we find any syntax errors at that level of detail. (With this patch, we could report some or all of the syntax errors first.) 2. Tentatively apply the new custom_variable_classes setting if any. 3. Check to see whether all the names are valid. If not, report the ones that aren't and bail out. 4. Apply each value. If some of them aren't valid, report that, but continue, and apply all the ones that are valid. We can expect that the postmaster and all backends will agree on the results of steps 1 through 3. They might differ as to the validity of individual values in step 4 (as per my example of a setting that depends on database_encoding), but we should never end up with a situation where a globally correct value is not globally applied. The original argument for the current behavior was to avoid applying settings from a thoroughly munged config file, but I think that the checks involved in steps 1-3 would be sufficient to reject files that had major problems. It's possible that step 1 is really sufficient to cover the issue, in which case we could drop the separate step-3 pass and just treat invalid GUC names as a reason to ignore the particular line rather than the whole file. That would make things simpler and faster, and maybe less surprising too. Comments? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jul 14, 2011, at 4:38 AM, Alvaro Herrera wrote: Excerpts from Florian Pflug's message of mié jul 13 20:12:28 -0400 2011: On Jul14, 2011, at 01:38 , Alvaro Herrera wrote: One strange thing here is that you could get two such messages; say if a file has 100 parse errors and there are also valid lines that contain bogus settings (foo = bar). I don't find this to be too problematic, and I think fixing it would be excessively annoying. For example, a bogus run would end like this: 95 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 4, near end of line 96 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 41, near end of line 97 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 104, near end of line 98 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 156, near end of line 99 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 208, near end of line 100 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 260, near end of line 101 LOG: too many errors found, stopped processing file /pgsql/install/HEAD/data/postgresql.conf 102 LOG: unrecognized configuration parameter plperl.err 103 LOG: unrecognized configuration parameter this1 104 LOG: too many errors found, stopped processing file /pgsql/install/HEAD/data/postgresql.conf 105 FATAL: errors detected while parsing configuration files How about changing ParseConfigFile to say too many *syntax* error found instead? It'd be more precise, and we wouldn't emit exactly the same message twice. Yeah, I thought about doing it that way but refrained because it'd be one more string to translate. That's a poor reason, I admit :-) I'll change it. This is happening because a check for total number of errors so far is happening only after coming across at least one non-recognized configuration option. What about adding one more check right after ParseConfigFile, so we can bail out early when overwhelmed with syntax errors? This would save a line in translation :). Do you want me to take a closer look at your modified version of the patch before you commit, or did you post it more as a FYI, this is how it's going to look like? I know I'd feel more comfortable if you (and Alexey, and Selena) gave it another look :-) I have checked it here and don't see any more problems with it. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Excerpts from Alexey Kluykin's message of mar jun 21 07:43:02 -0400 2011: Another benefit of removing the check is that it reduces code complexity. Maybe not when measured in line counts, but it's one less outside factor that changes ProcessConfigFiles()'s behaviour and thus one thing less you need to think when you modify that part again in the future. Again, it's a small benefit, but IMHO it still outweights the benefit. While I agree that making the code potentially less bug prone is a good idea, I don't agree with the statement that since DEBUG2 output gets rarely turned on we can make it less usable, hoping that nobody would use in production. I would have sided with Florian on this issue, but Tom commented otherwise somewhere, and I think his rationale makes sense, so I left the patch as Alexey had it. I also tweaked the patch so that it really stops processing after 100 errors, say if you include file A which has 50 errors and then file B which has another 50 errors; instead of looking for 98 more errors, we just give up at that point. It makes more sense to me anyway, and it doesn't seem to add any code complexity. Also, if you have already seen 98 errors, it doesn't make sense to let another file contain 100 errors more, so with this version, that 98 is passed down so that only 2 more errors are allowed. I had to touch the other callers of ParseConfigFile but it is clean enough. I also made the code barf when bailing out of one of those loops. One strange thing here is that you could get two such messages; say if a file has 100 parse errors and there are also valid lines that contain bogus settings (foo = bar). I don't find this to be too problematic, and I think fixing it would be excessively annoying. For example, a bogus run would end like this: 95 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 4, near end of line 96 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 41, near end of line 97 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 104, near end of line 98 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 156, near end of line 99 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 208, near end of line 100 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 260, near end of line 101 LOG: too many errors found, stopped processing file /pgsql/install/HEAD/data/postgresql.conf 102 LOG: unrecognized configuration parameter plperl.err 103 LOG: unrecognized configuration parameter this1 104 LOG: too many errors found, stopped processing file /pgsql/install/HEAD/data/postgresql.conf 105 FATAL: errors detected while parsing configuration files One thing I don't like is that those unrecognized configuration parameter messages don't say what file those parameters were found in. That's material for a different patch however. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support pg_parser_continue_on_error_v3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jul14, 2011, at 01:38 , Alvaro Herrera wrote: One strange thing here is that you could get two such messages; say if a file has 100 parse errors and there are also valid lines that contain bogus settings (foo = bar). I don't find this to be too problematic, and I think fixing it would be excessively annoying. For example, a bogus run would end like this: 95 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 4, near end of line 96 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 41, near end of line 97 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 104, near end of line 98 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 156, near end of line 99 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 208, near end of line 100 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 260, near end of line 101 LOG: too many errors found, stopped processing file /pgsql/install/HEAD/data/postgresql.conf 102 LOG: unrecognized configuration parameter plperl.err 103 LOG: unrecognized configuration parameter this1 104 LOG: too many errors found, stopped processing file /pgsql/install/HEAD/data/postgresql.conf 105 FATAL: errors detected while parsing configuration files How about changing ParseConfigFile to say too many *syntax* error found instead? It'd be more precise, and we wouldn't emit exactly the same message twice. Do you want me to take a closer look at your modified version of the patch before you commit, or did you post it more as a FYI, this is how it's going to look like? best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Excerpts from Florian Pflug's message of mié jul 13 20:12:28 -0400 2011: On Jul14, 2011, at 01:38 , Alvaro Herrera wrote: One strange thing here is that you could get two such messages; say if a file has 100 parse errors and there are also valid lines that contain bogus settings (foo = bar). I don't find this to be too problematic, and I think fixing it would be excessively annoying. For example, a bogus run would end like this: 95 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 4, near end of line 96 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 41, near end of line 97 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 104, near end of line 98 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 156, near end of line 99 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 208, near end of line 100 LOG: syntax error in file /pgsql/install/HEAD/data/postgresql.conf line 260, near end of line 101 LOG: too many errors found, stopped processing file /pgsql/install/HEAD/data/postgresql.conf 102 LOG: unrecognized configuration parameter plperl.err 103 LOG: unrecognized configuration parameter this1 104 LOG: too many errors found, stopped processing file /pgsql/install/HEAD/data/postgresql.conf 105 FATAL: errors detected while parsing configuration files How about changing ParseConfigFile to say too many *syntax* error found instead? It'd be more precise, and we wouldn't emit exactly the same message twice. Yeah, I thought about doing it that way but refrained because it'd be one more string to translate. That's a poor reason, I admit :-) I'll change it. Do you want me to take a closer look at your modified version of the patch before you commit, or did you post it more as a FYI, this is how it's going to look like? I know I'd feel more comfortable if you (and Alexey, and Selena) gave it another look :-) -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jun 20, 2011, at 6:22 PM, Florian Pflug wrote: On Jun20, 2011, at 17:02 , Alexey Klyukin wrote: I don't think it has changed at all. Previously, we did goto cleanup_list (or cleanup_exit in ParseConfigFp) right after the first error, no matter whether that was a postmaster or its child. What I did in my patch is removing the goto for the postmaster's case. It was my intention to exit after the initial error for the postmaster's child, to avoid complaining about all errors both in the postmaster and in the normal backend (imagine seeing 100 errors from the postmaster and the same 100 from each of the backends if your log level is DEBUG2). I think the postmaster's child case won't cause any problems, since we do exactly what we used to do before. Hm, I think you miss-understood what I was trying to say, probably because I explained it badly. Let me try again. I fully agree that there *shouldn't* be any difference in behaviour, because it *shouldn't* matter whether we abort early or not - we won't have applied any of the settings anway. But. The code the actually implements the check settings first, apply later logic isn't easy to read. Now, assume that this code has a bug. Then, with your patch applied, we might end up with the postmaster applying a setting (because it didn't abort early) but the backend ignoring it (because they did abort early). This is obviously bad. Depending on the setting, the consequences may range from slightly confusing behaviour to outright crashes I guess... Now, the risk of that happening might be very small. But on the other hand, the benefit is pretty small also - you get a little less output for log level DEBUG2, that's it. A level that people probably don't use for the production databases anyway. This convinced me that the risk/benefit ratio isn't high enough to warrant the early abort. Another benefit of removing the check is that it reduces code complexity. Maybe not when measured in line counts, but it's one less outside factor that changes ProcessConfigFiles()'s behaviour and thus one thing less you need to think when you modify that part again in the future. Again, it's a small benefit, but IMHO it still outweights the benefit. While I agree that making the code potentially less bug prone is a good idea, I don't agree with the statement that since DEBUG2 output gets rarely turned on we can make it less usable, hoping that nobody would use in production. Having said that, this is my personal opinion and whoever will eventually commit this may very will assess the cost/benefit ratio differently. So, if after this more detailed explanations of my reasoning, you still feel that it makes sense to keep the early abort, then feel free to mark the patch Ready for Committer nevertheless. I'd say that this issue is a judgement call. Depending on a point of view, both arguments are valid. I've marked this patch as 'Ready for Committer' w/o removing the early abort stuff, but if there will be more people willing to remove them - I'll do that. Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Florian, On Jun 18, 2011, at 5:40 PM, Florian Pflug wrote: On Jun16, 2011, at 22:34 , Alexey Klyukin wrote: Attached is the v2 of the patch to show all parse errors at postgresql.conf. Changes (per review and suggestions from Florian): - do not stop on the first error during postmaster's start. - fix errors in processing files from include directives. - show only a single syntax error per line, i.e. fast forward to the EOL after coming across the first one. - additional comments/error messages, code cleanup Looks mostly good. I found one issue which I missed earlier. As it stands, the behaviour of ProcessConfigFile() changes subtly when IsUnderPostmaster because of the early abort on errors. Now, in theory the outcome should still be the same, since we don't apply any settings if there's an error in one of them. But still, there's a risk that this code isn't 100% waterproof, and then we'd end up with different settings in the postmaster compared to the backends. The benefit seems also quite small - since the backends emit their messages at DEBUG2, you usually won't see the difference anyway. I don't think it has changed at all. Previously, we did goto cleanup_list (or cleanup_exit in ParseConfigFp) right after the first error, no matter whether that was a postmaster or its child. What I did in my patch is removing the goto for the postmaster's case. It was my intention to exit after the initial error for the postmaster's child, to avoid complaining about all errors both in the postmaster and in the normal backend (imagine seeing 100 errors from the postmaster and the same 100 from each of the backends if your log level is DEBUG2). I think the postmaster's child case won't cause any problems, since we do exactly what we used to do before. The elevel setting at the start of ProcessConfigFile() also seemed needlessly complex, since we cannot have IsUnderPostmaster and context == PGCS_POSTMASTER at the same time. Agreed. I figured it'd be harder to explain the fixes I have in mind than simply do them and let the code speak. Attached you'll find an updated version of your v2 patch (called v2a) as well as an incremental patch against your v2 (called v2a_delta). The main changes are the removal of the early aborts when IsUnderPostmaster and the simplification of the elevel setting logic in ProcessConfigFile(). Attached is the new patch and the delta. It includes some of the changes from your patch, while leaving the early abort stuff for postmaster's children. Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support pg_parser_continue_on_error_v2b.patch Description: Binary data pg_parser_continue_on_error_v2b_delta.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jun20, 2011, at 17:02 , Alexey Klyukin wrote: On Jun 18, 2011, at 5:40 PM, Florian Pflug wrote: On Jun16, 2011, at 22:34 , Alexey Klyukin wrote: Attached is the v2 of the patch to show all parse errors at postgresql.conf. Changes (per review and suggestions from Florian): - do not stop on the first error during postmaster's start. - fix errors in processing files from include directives. - show only a single syntax error per line, i.e. fast forward to the EOL after coming across the first one. - additional comments/error messages, code cleanup Looks mostly good. I found one issue which I missed earlier. As it stands, the behaviour of ProcessConfigFile() changes subtly when IsUnderPostmaster because of the early abort on errors. Now, in theory the outcome should still be the same, since we don't apply any settings if there's an error in one of them. But still, there's a risk that this code isn't 100% waterproof, and then we'd end up with different settings in the postmaster compared to the backends. The benefit seems also quite small - since the backends emit their messages at DEBUG2, you usually won't see the difference anyway. I don't think it has changed at all. Previously, we did goto cleanup_list (or cleanup_exit in ParseConfigFp) right after the first error, no matter whether that was a postmaster or its child. What I did in my patch is removing the goto for the postmaster's case. It was my intention to exit after the initial error for the postmaster's child, to avoid complaining about all errors both in the postmaster and in the normal backend (imagine seeing 100 errors from the postmaster and the same 100 from each of the backends if your log level is DEBUG2). I think the postmaster's child case won't cause any problems, since we do exactly what we used to do before. Hm, I think you miss-understood what I was trying to say, probably because I explained it badly. Let me try again. I fully agree that there *shouldn't* be any difference in behaviour, because it *shouldn't* matter whether we abort early or not - we won't have applied any of the settings anway. But. The code the actually implements the check settings first, apply later logic isn't easy to read. Now, assume that this code has a bug. Then, with your patch applied, we might end up with the postmaster applying a setting (because it didn't abort early) but the backend ignoring it (because they did abort early). This is obviously bad. Depending on the setting, the consequences may range from slightly confusing behaviour to outright crashes I guess... Now, the risk of that happening might be very small. But on the other hand, the benefit is pretty small also - you get a little less output for log level DEBUG2, that's it. A level that people probably don't use for the production databases anyway. This convinced me that the risk/benefit ratio isn't high enough to warrant the early abort. Another benefit of removing the check is that it reduces code complexity. Maybe not when measured in line counts, but it's one less outside factor that changes ProcessConfigFiles()'s behaviour and thus one thing less you need to think when you modify that part again in the future. Again, it's a small benefit, but IMHO it still outweights the benefit. Having said that, this is my personal opinion and whoever will eventually commit this may very will assess the cost/benefit ratio differently. So, if after this more detailed explanations of my reasoning, you still feel that it makes sense to keep the early abort, then feel free to mark the patch Ready for Committer nevertheless. best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Florian Pflug f...@phlo.org writes: The code the actually implements the check settings first, apply later logic isn't easy to read. Now, assume that this code has a bug. Then, with your patch applied, we might end up with the postmaster applying a setting (because it didn't abort early) but the backend ignoring it (because they did abort early). This is obviously bad. Depending on the setting, the consequences may range from slightly confusing behaviour to outright crashes I guess... This is already known to happen: there are cases where the postmaster and a backend can come to different conclusions about whether a setting is valid (eg, because it depends on database encoding). Whether that's a bug or not isn't completely clear, but if this patch is critically dependent on the situation never happening, I don't think we can accept it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jun20, 2011, at 18:16 , Tom Lane wrote: Florian Pflug f...@phlo.org writes: The code the actually implements the check settings first, apply later logic isn't easy to read. Now, assume that this code has a bug. Then, with your patch applied, we might end up with the postmaster applying a setting (because it didn't abort early) but the backend ignoring it (because they did abort early). This is obviously bad. Depending on the setting, the consequences may range from slightly confusing behaviour to outright crashes I guess... This is already known to happen: there are cases where the postmaster and a backend can come to different conclusions about whether a setting is valid (eg, because it depends on database encoding). Whether that's a bug or not isn't completely clear, but if this patch is critically dependent on the situation never happening, I don't think we can accept it. Does that mean that some backends might currently choose to ignore an updated config file wholesale on SIGUP (because some settings are invalid) while others happily apply it? Meaning that they'll afterwards disagree even on modified settings which *would* be valid for both backends? Or do these kinds of setting rejections happen late enough to fall out of the all-or-nothing logic in ProcessConfigFile? Anyway, the patch *doesn't* depend on all backends's setting being in sync. The issue we were discussion was whether it's OK to add another small risk of them getting out of sync by aborting early on errors in backends but not in the postmaster. I was arguing against that, while Alexey was in favour of it, on the grounds that it reduces log traffic (but only at DEBUG2 or beyond). best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Florian Pflug f...@phlo.org writes: On Jun20, 2011, at 18:16 , Tom Lane wrote: This is already known to happen: there are cases where the postmaster and a backend can come to different conclusions about whether a setting is valid (eg, because it depends on database encoding). Whether that's a bug or not isn't completely clear, but if this patch is critically dependent on the situation never happening, I don't think we can accept it. Does that mean that some backends might currently choose to ignore an updated config file wholesale on SIGUP (because some settings are invalid) while others happily apply it? Meaning that they'll afterwards disagree even on modified settings which *would* be valid for both backends? Yes. I complained about that before: http://archives.postgresql.org/pgsql-hackers/2011-04/msg00330.php but we didn't come to any consensus about fixing it. This patch might be a good vehicle for revisiting the issue, though. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jun16, 2011, at 22:34 , Alexey Klyukin wrote: Attached is the v2 of the patch to show all parse errors at postgresql.conf. Changes (per review and suggestions from Florian): - do not stop on the first error during postmaster's start. - fix errors in processing files from include directives. - show only a single syntax error per line, i.e. fast forward to the EOL after coming across the first one. - additional comments/error messages, code cleanup Looks mostly good. I found one issue which I missed earlier. As it stands, the behaviour of ProcessConfigFile() changes subtly when IsUnderPostmaster because of the early abort on errors. Now, in theory the outcome should still be the same, since we don't apply any settings if there's an error in one of them. But still, there's a risk that this code isn't 100% waterproof, and then we'd end up with different settings in the postmaster compared to the backends. The benefit seems also quite small - since the backends emit their messages at DEBUG2, you usually won't see the difference anyway. The elevel setting at the start of ProcessConfigFile() also seemed needlessly complex, since we cannot have IsUnderPostmaster and context == PGCS_POSTMASTER at the same time. I figured it'd be harder to explain the fixes I have in mind than simply do them and let the code speak. Attached you'll find an updated version of your v2 patch (called v2a) as well as an incremental patch against your v2 (called v2a_delta). The main changes are the removal of the early aborts when IsUnderPostmaster and the simplification of the elevel setting logic in ProcessConfigFile(). The updated patch also adds a few comments. If you agree with my changes, feel free to mark this patch Ready for Committer. best regards, Florian Pflug pg_parser_continue_on_error_v2a_delta.patch Description: Binary data pg_parser_continue_on_error_v2a.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Hi On May14, 2011, at 00:49 , Alexey Klyukin wrote: The patch forces the parser to report all errors (max 100) from the ProcessConfigFile/ParseConfigFp. Currently, only the first parse error or an invalid directive is reported. Reporting all of them is crucial to automatic validation of postgres config files. This patch is based on the one submitted earlier by Selena Deckelmann: http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php It incorporates suggestions by Tom Lane for avoiding excessive bloat in logs in case there is a junk instead of postgresql.conf. http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Here's my review of your patch. The patch is in context diff format and applies cleanly to HEAD. It doesn't contain superfluous whitespace changes any more is and quite readable. First, the behaviour. The first problem I ran into when I tried to test this is that it *only* reports multiple errors during config file reload on SIHUP, not during postmaster startup. I guess it's been done that way because we ereport(ERROR,..) not ereport(LOG,...) during postmaster startup, so it's not immediatly clear how to report multiple errors. But that proplem seems solvable. What if you ereport(LOG,..)ed the individual errors during postmaster startup, and then emitted an ereport(ERROR) at the end if errors occurred? The ERROR could either repeat the first error that was encountered, or simply say config file contains errors. Now to the code. I see that you basically replaced goto cleanup... in both ParseConfigFp() and ProcessConfigFile() with ++errorcount, and arranged for ParseConfigFp() to return false, and for ProcessConfigFile() to skip the GUC updates if errorcount 0. The actual value of errorcount is never inspected. The value is also wrong in the case of include files containing more than error, since ParseConfigFp() simply increments errorcount by one for each failed ParseConfigFile() of an included file. I thus suggest that you replace errorcount with a boolean erroroccurred. You've also introduced a bug in ParseConfigFp(). Previously, if an included file contained an error, it did goto cleanup_exit() and thus didn't ereport() on it's own. With your patch applied it ereport()s a parse error at the location of the include directive, which seems wrong. Finally, I believe that ParseConfigFp() should make at least some effort to resync after hitting a parser error. I suggest that you simply fast-forward to the next GUC_EOL token after reporting a parser error. best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Florian, On Jun 16, 2011, at 2:34 PM, Florian Pflug wrote: Hi On May14, 2011, at 00:49 , Alexey Klyukin wrote: The patch forces the parser to report all errors (max 100) from the ProcessConfigFile/ParseConfigFp. Currently, only the first parse error or an invalid directive is reported. Reporting all of them is crucial to automatic validation of postgres config files. This patch is based on the one submitted earlier by Selena Deckelmann: http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php It incorporates suggestions by Tom Lane for avoiding excessive bloat in logs in case there is a junk instead of postgresql.conf. http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Here's my review of your patch. The patch is in context diff format and applies cleanly to HEAD. It doesn't contain superfluous whitespace changes any more is and quite readable. First, the behaviour. The first problem I ran into when I tried to test this is that it *only* reports multiple errors during config file reload on SIHUP, not during postmaster startup. I guess it's been done that way because we ereport(ERROR,..) not ereport(LOG,...) during postmaster startup, so it's not immediatly clear how to report multiple errors. But that proplem seems solvable. What if you ereport(LOG,..)ed the individual errors during postmaster startup, and then emitted an ereport(ERROR) at the end if errors occurred? The ERROR could either repeat the first error that was encountered, or simply say config file contains errors. Makes sense. One problem I came across is that set_config_option from guc.c sets the elevel by itself. I've changed it to emit LOG errors on PGC_S_FILE source, apparently all the callers of this function with this source are from guc-file.l, so hopefully I won't break anything with this change. Now to the code. I see that you basically replaced goto cleanup... in both ParseConfigFp() and ProcessConfigFile() with ++errorcount, and arranged for ParseConfigFp() to return false, and for ProcessConfigFile() to skip the GUC updates if errorcount 0. The actual value of errorcount is never inspected. The value is also wrong in the case of include files containing more than error, since ParseConfigFp() simply increments errorcount by one for each failed ParseConfigFile() of an included file. I thus suggest that you replace errorcount with a boolean erroroccurred. I can actually pass errorcount down from the ParseConfigFp() to report the total number of errors (w/ the include files) at the end of ProcessConfigFile if there is any interest in having the number of errors reported to a user. If not - I'll change it to boolean. You've also introduced a bug in ParseConfigFp(). Previously, if an included file contained an error, it did goto cleanup_exit() and thus didn't ereport() on it's own. With your patch applied it ereport()s a parse error at the location of the include directive, which seems wrong. Right, I noticed that I skipped switching the buffers and restoring the Lineno as well. I'll fix it in the next revision. Finally, I believe that ParseConfigFp() should make at least some effort to resync after hitting a parser error. I suggest that you simply fast-forward to the next GUC_EOL token after reporting a parser error. Makes sense. Thank you for the review. I'll post an updated patch, addressing you concerns, shortly. Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jun16, 2011, at 17:23 , Alexey Klyukin wrote: On Jun 16, 2011, at 2:34 PM, Florian Pflug wrote: The first problem I ran into when I tried to test this is that it *only* reports multiple errors during config file reload on SIHUP, not during postmaster startup. I guess it's been done that way because we ereport(ERROR,..) not ereport(LOG,...) during postmaster startup, so it's not immediatly clear how to report multiple errors. But that proplem seems solvable. What if you ereport(LOG,..)ed the individual errors during postmaster startup, and then emitted an ereport(ERROR) at the end if errors occurred? The ERROR could either repeat the first error that was encountered, or simply say config file contains errors. Makes sense. One problem I came across is that set_config_option from guc.c sets the elevel by itself. I've changed it to emit LOG errors on PGC_S_FILE source, apparently all the callers of this function with this source are from guc-file.l, so hopefully I won't break anything with this change. Hm, wouldn't a test for context == PGC_POSTMASTER be more appropriate? I see that you basically replaced goto cleanup... in both ParseConfigFp() and ProcessConfigFile() with ++errorcount, and arranged for ParseConfigFp() to return false, and for ProcessConfigFile() to skip the GUC updates if errorcount 0. The actual value of errorcount is never inspected. The value is also wrong in the case of include files containing more than error, since ParseConfigFp() simply increments errorcount by one for each failed ParseConfigFile() of an included file. I thus suggest that you replace errorcount with a boolean erroroccurred. I can actually pass errorcount down from the ParseConfigFp() to report the total number of errors (w/ the include files) at the end of ProcessConfigFile if there is any interest in having the number of errors reported to a user. If not - I'll change it to boolean. Nah, just use a boolean, unless you have concrete plans to actually use the errorcount for something other than test a la errorcount 0. best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jun 16, 2011, at 6:49 PM, Florian Pflug wrote: On Jun16, 2011, at 17:23 , Alexey Klyukin wrote: On Jun 16, 2011, at 2:34 PM, Florian Pflug wrote: The first problem I ran into when I tried to test this is that it *only* reports multiple errors during config file reload on SIHUP, not during postmaster startup. I guess it's been done that way because we ereport(ERROR,..) not ereport(LOG,...) during postmaster startup, so it's not immediatly clear how to report multiple errors. But that proplem seems solvable. What if you ereport(LOG,..)ed the individual errors during postmaster startup, and then emitted an ereport(ERROR) at the end if errors occurred? The ERROR could either repeat the first error that was encountered, or simply say config file contains errors. Makes sense. One problem I came across is that set_config_option from guc.c sets the elevel by itself. I've changed it to emit LOG errors on PGC_S_FILE source, apparently all the callers of this function with this source are from guc-file.l, so hopefully I won't break anything with this change. Hm, wouldn't a test for context == PGC_POSTMASTER be more appropriate? In such a case the errors caused by command-line arguments won't stop the postmaster. PGC_S_FILE seems to handle this correctly. I'm not sure whether it is appropriate to use there though. I see that you basically replaced goto cleanup... in both ParseConfigFp() and ProcessConfigFile() with ++errorcount, and arranged for ParseConfigFp() to return false, and for ProcessConfigFile() to skip the GUC updates if errorcount 0. The actual value of errorcount is never inspected. The value is also wrong in the case of include files containing more than error, since ParseConfigFp() simply increments errorcount by one for each failed ParseConfigFile() of an included file. I thus suggest that you replace errorcount with a boolean erroroccurred. I can actually pass errorcount down from the ParseConfigFp() to report the total number of errors (w/ the include files) at the end of ProcessConfigFile if there is any interest in having the number of errors reported to a user. If not - I'll change it to boolean. Nah, just use a boolean, unless you have concrete plans to actually use the errorcount for something other than test a la errorcount 0. I just recalled a reason for counting the total number of errors. There is a condition that checks that the total number of errors is less than 100 and bails out if it's more than that (100 is arbitrary). The reason is to avoid bloating the logs w/ something totally unrelated to postgresql.conf. That was suggested by Tom Lane here: http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jun16, 2011, at 18:46 , Alexey Klyukin wrote: On Jun 16, 2011, at 6:49 PM, Florian Pflug wrote: Hm, wouldn't a test for context == PGC_POSTMASTER be more appropriate? In such a case the errors caused by command-line arguments won't stop the postmaster. PGC_S_FILE seems to handle this correctly. I'm not sure whether it is appropriate to use there though. Ah, yeah, you're right. PGC_S_FILE sounds fine, then. I guess this means you can drop the check for context == PGC_SIGHUP though, because surely the source must be PGC_S_DEFAULT or PGC_S_FILE if context == PGC_SIGHUP, right? So the check would become if (source == PGC_S_FILE || source == PGC_S_DEFAULT) where it now says if (context == PGC_SIGHUP || source == PGC_S_DEFAULT) I see that you basically replaced goto cleanup... in both ParseConfigFp() and ProcessConfigFile() with ++errorcount, and arranged for ParseConfigFp() to return false, and for ProcessConfigFile() to skip the GUC updates if errorcount 0. The actual value of errorcount is never inspected. The value is also wrong in the case of include files containing more than error, since ParseConfigFp() simply increments errorcount by one for each failed ParseConfigFile() of an included file. I thus suggest that you replace errorcount with a boolean erroroccurred. I can actually pass errorcount down from the ParseConfigFp() to report the total number of errors (w/ the include files) at the end of ProcessConfigFile if there is any interest in having the number of errors reported to a user. If not - I'll change it to boolean. Nah, just use a boolean, unless you have concrete plans to actually use the errorcount for something other than test a la errorcount 0. I just recalled a reason for counting the total number of errors. There is a condition that checks that the total number of errors is less than 100 and bails out if it's more than that (100 is arbitrary). The reason is to avoid bloating the logs w/ something totally unrelated to postgresql.conf. That was suggested by Tom Lane here: http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Ah, right, I missed that. Guess it'll have to stay a counter, then. Still, I don't think it's worth the effort to make the count correct in case of included files, so I'd say just add a comment explaining that the count isn't totally accurate. best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jun 16, 2011, at 8:01 PM, Florian Pflug wrote: On Jun16, 2011, at 18:46 , Alexey Klyukin wrote: On Jun 16, 2011, at 6:49 PM, Florian Pflug wrote: Hm, wouldn't a test for context == PGC_POSTMASTER be more appropriate? In such a case the errors caused by command-line arguments won't stop the postmaster. PGC_S_FILE seems to handle this correctly. I'm not sure whether it is appropriate to use there though. Ah, yeah, you're right. PGC_S_FILE sounds fine, then. I guess this means you can drop the check for context == PGC_SIGHUP though, because surely the source must be PGC_S_DEFAULT or PGC_S_FILE if context == PGC_SIGHUP, right? So the check would become if (source == PGC_S_FILE || source == PGC_S_DEFAULT) where it now says if (context == PGC_SIGHUP || source == PGC_S_DEFAULT) Yes, AFAIK PGC_SIGHUP is redundant, thank you for the suggestion. I just recalled a reason for counting the total number of errors. There is a condition that checks that the total number of errors is less than 100 and bails out if it's more than that (100 is arbitrary). The reason is to avoid bloating the logs w/ something totally unrelated to postgresql.conf. That was suggested by Tom Lane here: http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Ah, right, I missed that. Guess it'll have to stay a counter, then. Still, I don't think it's worth the effort to make the count correct in case of included files, so I'd say just add a comment explaining that the count isn't totally accurate. Well, while thinking about this I decided to leave the counter for the ParseConfigFp, but drop it in ProcessConfigFile. The case we are protecting against is a single file full of junk. It's unlikely that this junk would contain include directives with valid file paths, neither it's likely to find a file with a correct syntax, but full of invalid directives. Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Jun16, 2011, at 20:14 , Alexey Klyukin wrote: On Jun 16, 2011, at 8:01 PM, Florian Pflug wrote: On Jun16, 2011, at 18:46 , Alexey Klyukin wrote: I just recalled a reason for counting the total number of errors. There is a condition that checks that the total number of errors is less than 100 and bails out if it's more than that (100 is arbitrary). The reason is to avoid bloating the logs w/ something totally unrelated to postgresql.conf. That was suggested by Tom Lane here: http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Ah, right, I missed that. Guess it'll have to stay a counter, then. Still, I don't think it's worth the effort to make the count correct in case of included files, so I'd say just add a comment explaining that the count isn't totally accurate. Well, while thinking about this I decided to leave the counter for the ParseConfigFp, but drop it in ProcessConfigFile. The case we are protecting against is a single file full of junk. It's unlikely that this junk would contain include directives with valid file paths, neither it's likely to find a file with a correct syntax, but full of invalid directives. Sounds good. best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Hi, On Jun 16, 2011, at 9:18 PM, Florian Pflug wrote: On Jun16, 2011, at 20:14 , Alexey Klyukin wrote: Well, while thinking about this I decided to leave the counter for the ParseConfigFp, but drop it in ProcessConfigFile. The case we are protecting against is a single file full of junk. It's unlikely that this junk would contain include directives with valid file paths, neither it's likely to find a file with a correct syntax, but full of invalid directives. Sounds good. Attached is the v2 of the patch to show all parse errors at postgresql.conf. Changes (per review and suggestions from Florian): - do not stop on the first error during postmaster's start. - fix errors in processing files from include directives. - show only a single syntax error per line, i.e. fast forward to the EOL after coming across the first one. - additional comments/error messages, code cleanup Questions: - Should we add a comment for the changes in guc.c? I think the existing ones are still valid, but they might be harder go grasp, given that we've removed PGC_SIGHUP from the condition. - The error message that we emit when the parsing is unsuccessful, will it cause incompatibility w/ 3rd party tools, which may, in theory, show only one error message (would it better to show the first error instead, as proposed by Florian?). I'd appreciate your comments and suggestions. Thank you, Alexey. -- Command Prompt, Inc. http://www.CommandPrompt.com PostgreSQL Replication, Consulting, Custom Development, 24x7 support parser_continue_on_errors_v2.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Hi, On Apr 14, 2011, at 9:50 PM, Robert Haas wrote: On Mon, Apr 4, 2011 at 2:03 PM, Alexey Klyukin al...@commandprompt.com wrote: Here's the update of Selena's patch, which also shows all errors in configuration parameters (as well as parser errors) during reload. You should add this here: https://commitfest.postgresql.org/action/commitfest_view/open On a quick glance, this patch appears to contain some superfluous hunks where you changed whitespace or variable names. You might want to remove those and repost before adding to the CF app. Also, some submission notes would be very helpful - when you send in the revised version, detail in the email the exact purpose of the changes so that someone can review the patch without having to read this thread and all preceding threads in their entirety. Thank you for the feedback, I've updated the patch, attached is a new version. I'll add it to the commitfest after posting this message. The patch forces the parser to report all errors (max 100) from the ProcessConfigFile/ParseConfigFp. Currently, only the first parse error or an invalid directive is reported. Reporting all of them is crucial to automatic validation of postgres config files. This patch is based on the one submitted earlier by Selena Deckelmann: http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php It incorporates suggestions by Tom Lane for avoiding excessive bloat in logs in case there is a junk instead of postgresql.conf. http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Regards, -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. parser_continue_on_errors.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Mon, Apr 4, 2011 at 2:03 PM, Alexey Klyukin al...@commandprompt.com wrote: Here's the update of Selena's patch, which also shows all errors in configuration parameters (as well as parser errors) during reload. You should add this here: https://commitfest.postgresql.org/action/commitfest_view/open On a quick glance, this patch appears to contain some superfluous hunks where you changed whitespace or variable names. You might want to remove those and repost before adding to the CF app. Also, some submission notes would be very helpful - when you send in the revised version, detail in the email the exact purpose of the changes so that someone can review the patch without having to read this thread and all preceding threads in their entirety. Thanks, -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
On Apr 1, 2011, at 12:08 AM, Alexey Klyukin wrote: Hi Selena, On Mar 30, 2011, at 11:42 PM, Selena Deckelmann wrote: Hi! On Wed, Mar 30, 2011 at 8:40 AM, Alexey Klyukin al...@commandprompt.com wrote: I did a little bit of work on this, and we discussed it here: http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Probably there's a bit of bitrot in there. Cool, I was not aware of your work in this direction. I've updated your patch to apply to the latest HEAD, implementing Tom Lane's suggestions (attached). I think I'll implement the other part (reporting all invalid parameters, as opposed to only the first one) tomorrow. Here's the update of Selena's patch, which also shows all errors in configuration parameters (as well as parser errors) during reload. When I talked to Alvaro the other day he suggested starting with a stand-alone process, which would load the postgresql.conf in a postmaster context, load other configuration files and do some of the checks I've mentioned in my initial proposal (particularly it would check that system's shared memory limit is high enough by trying to allocate a new shared memory segment). Afterwards, I'd like to implement checks from a user-callable function, and not all checks would be available from it. -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. guc-file.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Hi Selena, On Mar 30, 2011, at 11:42 PM, Selena Deckelmann wrote: Hi! On Wed, Mar 30, 2011 at 8:40 AM, Alexey Klyukin al...@commandprompt.com wrote: I did a little bit of work on this, and we discussed it here: http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Probably there's a bit of bitrot in there. Cool, I was not aware of your work in this direction. I've updated your patch to apply to the latest HEAD, implementing Tom Lane's suggestions (attached). I think I'll implement the other part (reporting all invalid parameters, as opposed to only the first one) tomorrow. The development plan consists of 2 parts. The first one is to add new code that would allow running the checks in both a stand-alone process, when postmaster is not running, and as a function call from a backend postgres process. Initially the code would simply loads configuration files, without performing any of the validation checks. The second part consists of adding specific checks. Cool! Mine was only going to work if the system started up or was reloaded. Well, I think a stand-alone check is an easy part :) As I said above, some of what you've suggested seems more like a non-postgres core thing.. maybe an extension? Or maybe offer the option to read Well, initially I'm going to start with just a check that configuration files are valid, and add other checks afterwards. I think it makes sense for them to be optional. My idea was to just check that settings were *valid* not that they met some other, more subjective criteria. Well, my definition of valid configuration is not only the one that server is able to parse and load, but also to actually apply (i.e. can bind to a listen_address or read SSL certificate files). I agree that's not always necessary (i.e. when checking configuration on a different server than the one it should be applied to), so we can add a flag to turn them off. -- Alexey Klyukin The PostgreSQL Company - Command Prompt, Inc. parser_continue_on_errors.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: a validator for configuration files
Hi! On Wed, Mar 30, 2011 at 8:40 AM, Alexey Klyukin al...@commandprompt.com wrote: I'd like to add an ability to validate the corectness of PostgreSQL configuration files, i.e. postgresql.conf, pg_hba.conf, pg_ident.conf without actually applying them. The idea is that it would be a command-line option to postgres for a stand-alone case, or a user-callable function when postmaster is running. Per the former discussion of a validator for PostgreSQL configuration files (see http://archives.postgresql.org/pgsql-hackers/2008-08/msg00048.php), here's an implementation proposal. I did a little bit of work on this, and we discussed it here: http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php Probably there's a bit of bitrot in there. The development plan consists of 2 parts. The first one is to add new code that would allow running the checks in both a stand-alone process, when postmaster is not running, and as a function call from a backend postgres process. Initially the code would simply loads configuration files, without performing any of the validation checks. The second part consists of adding specific checks. Cool! Mine was only going to work if the system started up or was reloaded. I think most of the code related to this feature should be put into the auxiliary process. The rationale is that we already have a stand-alone CheckerProcess, which nowadays only parses postgresql.conf, runs BaseInit and exists. We can easily load pg_hba.conf and pg_ident.conf and run all the necessary checks there. Moreover, the same process can be used when checks are launched from a built-in function. In that case, it would save the postgres backend from reloading postgresql.conf, pg_hba.conf and pg_ident.conf internally and restoring the previous configuration options when the function exists. Below is a more detailed description of implementation steps: 1.1. Stand-alone process (postmaster is not running): - Add a new option (--check-config) to main.c. Run AuxiliaryProcessMain with auxType= CheckerProcess if this option is specified. - In CheckerModeMain load pg_hba.conf, pg_ident.conf 1.2. Built-in function, called from a backend process. - Add a new built-in function - Add LastCheckState shared flag to denote whether the check has been successful or failed - Add a new PMSignalReason - From the function call SendPostmasterSignal with the reason added on the previous step. - In the postmaster add a check for this reason in sigusr1_handler, spawning a checker process. Set LastCheckStatus to InProgress. - Store current configuration options in AuxillaryProcessMain before reloading configuration files to avoid checking for options that haven't changed. - In AuxillaryProcessMain, modify SelectConfigFiles invocation to call it if IsUnderPostmaster = true if auxType process type is CheckerProcess. - In the CheckerModeMain, set LastCheckState to either success or failure at the end of all checks. - The built-in function would wait until LastCheckState changes its value from InProgress to either Succcess or Failure, or until some predefined timeout expires, in which case it would emit an error. 2. Add following configuration checks: postgresql.conf: Check that: 1. postgres can bind to listen address:port 2. unix_socket_directory has proper permissions (n/a on Windows). 3. unix_socket_group group exists on a target system (n/a Windows). 4. unix_socket_permissions are valid (write permission is not revoked from the owner, or a group, if group is different from the user). 5. server.crt and server.key exist in the data directory if ssl is set to true (and server is compiled with openssl) 6. krb_server_keyfile is readable by the server (if set) 7. server is not requesting more shared memory than it's available in the system. 8. shared_preload_libraries and local_preload_libraries exist. 9. synchronous_standby_names are not empty and max_wal_senders are 0 if synchronous_replication = true 10. all enable_* query planner parameters are on. 11. seq_page_cost = random_page_cost, and cpu_ costs are lower than page_ costs. 12. effective_cache_size is less than the amount of physical memory available. 13. geqo is turned on 14. default_statistics_target is = 100 15. logging collector is enabled if log destination is stderr 16. log directory either exists and writable or that the parent directory allows creating subdris 17. track counts is on if autovacuum is enabled 18. stats_temp_directory is writeable 19. default tablespace exists (if set) 20. temp_tablespace exists (if set) 21. statement_timeout is 0 (disabled) 22. dynamic_library_path exists 23. sql_inheritance is off 24. zero_damaged_pages is off Some of this seems not like things that postgres itself should be doing, but more something that an
