Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: Proposal for Allow postgresql.conf values to be changed via SQL [review])

Amit Kapila escribió:

> I have changed the file name to postgresql.auto.conf and I have changed the
> name of SetPersistentLock to AutoFileLock.
>
> Zoltan,
>
> Could you please once check the attached Patch if you have time, as now all
> the things are resolved except for small doubt in syntax extensibility
> which can be changed based on final decision.

There are a bunch of whitespace problems, as you would notice with "git
diff --check" or "git show --color". Could you please clean that up?
Also, some of the indentation in various places is not right; perhaps
you could fix that by running pgindent over the source files you
modified (this is easily visible by eyeballing the git diff output;
stuff is misaligned in pretty obvious ways.)

Random minor other comments:

+ use of <xref linkend="SQL-ALTER SYSTEM">

this SGML link cannot possibly work. Please run "make check" in the
doc/src/sgml directory.

Examples in alter_system.sgml have a typo "SYTEM". Same file has "or or".

Also in that file,
The name of an configuration parameter that exist in <filename>postgresql.conf</filename>.
the parameter needn't exist in that file to be settable, right?

<refnamediv>
<refname>ALTER SYSTEM</refname>
<refpurpose>change a server configuration parameter</refpurpose>
</refnamediv>

Perhaps "permanently change .."?

Also, some parameters require a restart, not a reload; maybe we should
direct the user somewhere else to learn how to freshen up the
configuration after calling the command.

+ /* skip auto conf temporary file */
+ if (strncmp(de->d_name,
+ PG_AUTOCONF_FILENAME ".",
+ sizeof(PG_AUTOCONF_FILENAME)) == 0)
+ continue;

What's the dot doing there?

+ if ((stat(AutoConfFileName, &st) == -1))
+ ereport(elevel,
+ (errmsg("configuration parameters changed with ALTER SYSTEM command before restart of server "
+ "will not be effective as \"%s\" file is not accessible.", PG_AUTOCONF_FILENAME)));
+ else
+ ereport(elevel,
+ (errmsg("configuration parameters changed with ALTER SYSTEM command before restart of server "
+ "will not be effective as default include directive for \"%s\" folder is not present in postgresql.conf", PG_AUTOCONF_DIR)));

These messages should be split. Perhaps have the "will not be
effective" in the errmsg() and the reason as part of errdetail()? Also,
I'm not really sure about doing another stat() on the file after parsing
failed; I think the parse routine should fill some failure context
information, instead of having this code trying to reproduce the failure
to know what to report. Maybe something like the SlruErrorCause enum,
not sure.

Similarly,

+ /*
+ * record if the file currently being parsed is postgresql.auto.conf,
+ * so that it can be later used to give warning if it doesn't parse
+ * it.
+ */
+ snprintf(Filename,sizeof(Filename),"%s/%s", PG_AUTOCONF_DIR, PG_AUTOCONF_FILENAME);
+ ConfigAutoFileName = AbsoluteConfigLocation(Filename, ConfigFileName);
+ if (depth == 1 && strcmp(ConfigAutoFileName, config_file) == 0)
+ *is_config_dir_parsed = true;

this seems very odd. The whole "is_config_dir_parsed" mechanism smells
strange to me, really. I think the caller should be in charge of
keeping track of this, but I'm not sure. ParseConfigFp() would have no
way of knowing, and in one place we're calling that routine precisely to
parse the auto file.

Thanks,

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services