Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]

Hi,

I am reviewing your patch.

2012-12-10 13:58 keltezéssel, Amit kapila írta:
>
> On Thu, 6 Dec 2012 10:12:31 +0530 Amit Kapila wrote:
> On Tuesday, December 04, 2012 8:37 AM Amit Kapila wrote:
> > On Monday, December 03, 2012 8:59 PM Tom Lane wrote:
> > > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > > > On Sat, Dec 1, 2012 at 11:45 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> > Neither of you have responded to the point about what "SET PERSISTENT
> >> > var_name TO DEFAULT" will do, and whether it is or should be different
> >> > from RESET PERSISTENT, and if not why we should put the latter into
> >> > the grammar as well.
> >
> >
> >> The current behavior is
> >> 1. "RESET PERSISTENT ..." will delete the entry from
> >> postgresql.auto.conf
> >> 2. "SET PERSISTENT... TO DEFAULT" will update the entry value in
> >> postgresql.auto.conf to default value
> >
> >> However we can even change "SET PERSISTENT... TO DEFAULT" to delete the
> >> entry and then we can avoid "RESET PERSISTENT ..."
>
> > As per my understanding from the points raised by you, the behavior could be
> > defined as follows:
>
> > 1. No need to have "RESET PERSISTENT ..." syntax.
> > 2. It is better if we provide a way to delete entry which could be done for
> > syntax:
> > "SET PERSISTENT... TO DEFAULT"
>
> Updated patch to handle above 2 points.
>
> With Regards,
>
> Amit Kapila.
>

* Is the patch in context diff format <http://en.wikipedia.org/wiki/Diff#Context_format>?

Yes. (But with PostgreSQL using GIT, and most developers are
now comfortable with the unified diff format, this question should
not be in the wiki any more...)

* Does it apply cleanly to the current git master?

Not quite cleanly, it gives some offset warnings:

[zozo(at)localhost postgresql.1]$ cat ../set_persistent_v3.patch | patch -p1
patching file doc/src/sgml/ref/set.sgml
patching file src/backend/nodes/copyfuncs.c
patching file src/backend/nodes/equalfuncs.c
patching file src/backend/parser/gram.y
patching file src/backend/postmaster/postmaster.c
Hunk #1 succeeded at 2552 (offset -4 lines).
patching file src/backend/replication/basebackup.c
Hunk #1 succeeded at 736 (offset 155 lines).
patching file src/backend/utils/misc/guc-file.l
patching file src/backend/utils/misc/guc.c
Hunk #1 succeeded at 3348 (offset -13 lines).
Hunk #2 succeeded at 4125 (offset -13 lines).
Hunk #3 succeeded at 5108 (offset -13 lines).
Hunk #4 succeeded at 6090 (offset -13 lines).
Hunk #5 succeeded at 6657 (offset -13 lines).
Hunk #6 succeeded at 6742 (offset -13 lines).
patching file src/backend/utils/misc/postgresql.conf.sample
patching file src/bin/initdb/initdb.c
patching file src/include/nodes/parsenodes.h
patching file src/include/parser/kwlist.h
patching file src/include/utils/guc.h
patching file src/include/utils/guc_tables.h
patching file src/test/regress/expected/persistent.out
patching file src/test/regress/parallel_schedule
patching file src/test/regress/serial_schedule
patching file src/test/regress/sql/persistent.sql

There are no "fuzz" warnings though, so rebase is not needed.

* Does it include reasonable tests, necessary doc patches, etc?

Yes, it contains documentation, but it will surely need proofreading.
I noticed some typos in it already but the wording seems to be clear.

It also contains an extensive set of regression tests.

One specific comment about the documentation part of the patch:

***************
*** 86,92 **** SET [ SESSION | LOCAL ] TIME ZONE { <replaceable
class="PARAMETER">timezone</rep
<application>PL/pgSQL</application> exception block. This behavior
has been changed because it was deemed unintuitive.
</para>
! </note>
</refsect1>

<refsect1>
--- 95,101 ----
<application>PL/pgSQL</application> exception block. This behavior
has been changed because it was deemed unintuitive.
</para>
! </note>
</refsect1>

<refsect1>
***************

* Does the patch actually implement that?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

No such SQL spec. It was implemented according to the discussion.

It uses a single file in an extra configuration directory added to
postgresql.conf as:

# This includes the default configuration directory included to support
# SET PERSISTENT statement.
#
# WARNNING: Any configuration parameter values specified after this line
# will override the values set by SET PERSISTENT statement.
include_dir = 'config_dir'

Note the typo in the above message: WARNNING, there are too many Ns.

The extra configuration file ($PGDATA/config_dir/postgresql.auto.conf)
is loaded automatically. I tested it by setting shared_buffers and work_mem.
Executing "SELECT pg_reload_conf();" changed work_mem and left these
messages in the server log:

LOG: parameter "shared_buffers" cannot be changed without restarting the server
LOG: configuration file "/home/zozo/pgc93dev-setpersistent/data/postgresql.conf" contains
errors; unaffected changes were applied

Just as if I manually edited postgresql.conf. It works as it should.

According to the discussion, SET PERSISTENT cannot be executed inside
a transaction block, it's implemented and covered by the regression test.

* Does it include pg_dump support (if applicable)?

Not applicable. The $PGDATA/config_dir (and subsequently the
postgresql.auto.conf file) is included in binary dumps via pg_basebackup.

* Are there dangers?

The patch is about modifying the server configuration so there is
some danger in creating a configuration that won't start up the server

on a given machine, but you can do such a thing manually too today.
Also, SET PERSISTENT requires database superuser. If an attacker can

gain superuser privileges, you lost anyway. You can destroy a database

easily with creative uses of COPY TO.

* Have all the bases been covered?

It seems so. The regression test contains a lot of tests
for error cases, but there are no test cases for executing
SET PERSISTENT for an unknown GUC (fails as it should) and
executing SET PERSISTENT in a function inside a transaction
(also fails as it should).

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

I don't think so, the mentioned corner cases are also handled.

* Are there any assertion failures or crashes?

No. (I compiled the server with --enable-cassert)

* Does the patch slow down simple tests?

No.

* If it claims to improve performance, does it?

It's not a performance patch.

* Does it slow down other things?

No.

* Does it follow the project coding guidelines
<http://developer.postgresql.org/pgdocs/postgres/source.html>?

Yes.

* Are there portability issues?

No. I have seen an added chmod() call in setup_config()

in initdb.c but is's used already there so it should be ok.

* Will it work on Windows/BSD etc?

It should.

* Are the comments sufficient and accurate?

I think so.

* Does it do what it says, correctly?

I think so.

* Does it produce compiler warnings?

No.

* Can you make it crash?

No.

* Is everything done in a way that fits together coherently with other features/modules?

Yes.

* Are there interdependencies that can cause problems?

No.

Some more comments:

*** a/src/backend/replication/basebackup.c
--- b/src/backend/replication/basebackup.c
***************
*** 581,586 **** sendDir(char *path, int basepathlen, bool sizeonly)
--- 581,592 ----
strlen(PG_TEMP_FILE_PREFIX)) == 0)
continue;

+ /* skip lock files used in postgresql.auto.conf edit */
+ if (strncmp(de->d_name,
+ "postgresql.auto.conf.lock",
+ strlen("postgresql.auto.conf.lock")) == 0)
+ continue;
+
/*
* If there's a backup_label file, it belongs to a backup started by
* the user with pg_start_backup(). It is *not* correct for this
***************

Since you are using a constant string, it would be a little faster
to use "sizeof(string)-1" as it can be computed at compile-time
and not run the strlen() all the time this code is reached.
Something like this below:

#define PGAUTOCONFLOCK "postgresql.auto.conf.lock"

+ /* skip lock files used in postgresql.auto.conf edit */
+ if (strncmp(de->d_name,
+ PGAUTOCONFLOCK,
+ sizeof(PGAUTOCONFLOCK)-1) == 0)

In create_conf_lock_file():

+ static int
+ create_conf_lock_file(const char *LockFileName)
+ {
+ int fd;
+ int ntries;
+
+ /*
+ * We need a loop here because of race conditions. Retry for 100 times.
+ */
+ for (ntries = 0;; ntries++)
+ {
+ fd = open(LockFileName, O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
+ if (fd >= 0)
+ break;
+
+ /*
+ * Couldn't create the lock file. Probably it already exists. If so
+ * wait for some time
+ */
+ if ((errno != EEXIST && errno != EACCES) || ntries > 100)
+ {
...
+ }
+ else
+ {
+ pg_usleep(100000); /* in total wait for 10sec */
+ }
+ }
+
+ return fd;
+ }

Can't we add a new LWLock and use it as a critical section instead
of waiting for 10 seconds? It would be quite surprising to wait
10 seconds when accidentally executing two SET PERSISTENT
statements in parallel. Something like the attached patch against
set_persistent_v3.patch. The pg_usleep(15000000) in the patch
is intentional (as the comment says) to allow enough time enter
and run two or more SET PERSISTENT statements.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web:http://www.postgresql-support.de
http://www.postgresql.at/