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

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.

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/

2013-01-04 18:27 keltezéssel, Boszormenyi Zoltan írta:
>
> 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>
> ***************
>

I forgot to add the actual comment: this is an unnecessary whitespace change.

--
----------------------------------
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/

On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
> Hi,

> I am reviewing your patch.

Thank you very much.

> 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.

I have reffered the code just above for PG_TEMP_FILE_PREFIX in same function sendDir().
I have that not only that place, but there other places where strlen is used for PG_TEMP_FILE_PREFIX.
I think in this path, such optimization might not help much.
However if you think we should take care of this, then I can find other places where similar change can be done
to make it consistent?

> In create_conf_lock_file():

> 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.

Yes, you are right adding a new LWLock will avoid the use of sleep.
However I am just not sure if for only this purpose we should add a new LWLock?

Similar logic is used in CreateLockFile() for postmaster file but it doesn't use sleep.
How about reducing the time of sleep or removing sleep, in that user might get
error and he need to retry to get his command successful?

With Regards,
Amit Kapila.

2013-01-05 05:58 keltezéssel, Amit kapila írta:
> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>> Hi,
>> I am reviewing your patch.
> Thank you very much.
>
>> 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.
> I have reffered the code just above for PG_TEMP_FILE_PREFIX in same function sendDir().
> I have that not only that place, but there other places where strlen is used for PG_TEMP_FILE_PREFIX.
> I think in this path, such optimization might not help much.
> However if you think we should take care of this, then I can find other places where similar change can be done
> to make it consistent?
>
>> In create_conf_lock_file():
>
>> 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.
> Yes, you are right adding a new LWLock will avoid the use of sleep.
> However I am just not sure if for only this purpose we should add a new LWLock?
>
> Similar logic is used in CreateLockFile() for postmaster file but it doesn't use sleep.
> How about reducing the time of sleep or removing sleep, in that user might get
> error and he need to retry to get his command successful?

I think removing the loop entirely is better.

However, the behaviour should be discussed by a wider audience:
- the backends should wait for each other just like other statements
that fight for the same object (in which case locking is needed), or
- throw an error immediately on conflicting parallel work

I also tried to trigger one of the errors by creating the lock file manually.
You need an extra space between the "... retry" and "or ...":

+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not create lock file
\"%s\": %m ", LockFileName),
+ errhint("May be too many concurrent edit
into file happening, please wait!! and retry"
+ "or .lock is file
accidently left there please clean the file from config_dir")));

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/

2013-01-05 05:58 keltezéssel, Amit kapila írta:
> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>> Hi,
>> I am reviewing your patch.
> Thank you very much.
>
>> 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.
> I have reffered the code just above for PG_TEMP_FILE_PREFIX in same function sendDir().
> I have that not only that place, but there other places where strlen is used for PG_TEMP_FILE_PREFIX.
> I think in this path, such optimization might not help much.
> However if you think we should take care of this, then I can find other places where similar change can be done
> to make it consistent?

Yes, but it needs a separate cleanup patch.

Also, since the SET PERSISTENT patch seems to create a single lock file,
sizeof(string) (which includes the 0 byte at the end of the string, so it
matches the whole filename) can be used instead of strlen(string) or
sizeof(string)-1 that match the filename as a prefix only.

>
>> In create_conf_lock_file():
>
>> 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.
> Yes, you are right adding a new LWLock will avoid the use of sleep.
> However I am just not sure if for only this purpose we should add a new LWLock?
>
> Similar logic is used in CreateLockFile() for postmaster file but it doesn't use sleep.
> How about reducing the time of sleep or removing sleep, in that user might get
> error and he need to retry to get his command successful?
>
> With Regards,
> Amit Kapila.
>

--
----------------------------------
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/

On Sat, Jan 05, 2013 at 08:05:11AM +0100, Boszormenyi Zoltan wrote:
> 2013-01-05 05:58 keltez?ssel, Amit kapila ?rta:
>> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>>> In create_conf_lock_file():
>>
>>> 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.
>> Yes, you are right adding a new LWLock will avoid the use of sleep.
>> However I am just not sure if for only this purpose we should add a new LWLock?
>>
>> Similar logic is used in CreateLockFile() for postmaster file but it doesn't use sleep.
>> How about reducing the time of sleep or removing sleep, in that user might get
>> error and he need to retry to get his command successful?
>
> I think removing the loop entirely is better.

Agreed. A new LWLock with a narrow purpose is fine. CreateLockFile() happens
early, before shared memory is available. Its approach is not a relevant
precedent for code that runs later.

> However, the behaviour should be discussed by a wider audience:
> - the backends should wait for each other just like other statements
> that fight for the same object (in which case locking is needed), or
> - throw an error immediately on conflicting parallel work

All other things being equal, the first choice sounds better. (I don't think
the decision is essential to the utility of this feature.)

On Saturday, January 05, 2013 12:35 PM Boszormenyi Zoltan wrote:
2013-01-05 05:58 keltezéssel, Amit kapila írta:
> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>> Hi,
>> I am reviewing your patch.
> Thank you very much.
>
>> Yes, you are right adding a new LWLock will avoid the use of sleep.
>> However I am just not sure if for only this purpose we should add a new LWLock?
>
>> Similar logic is used in CreateLockFile() for postmaster file but it doesn't use sleep.
>> How about reducing the time of sleep or removing sleep, in that user might get
>> error and he need to retry to get his command successful?

> I think removing the loop entirely is better.

> However, the behaviour should be discussed by a wider audience:
> - the backends should wait for each other just like other statements
> that fight for the same object (in which case locking is needed), or
> - throw an error immediately on conflicting parallel work

Okay, I shall update the patch with first option as suggested by Noah as well.

>I also tried to trigger one of the errors by creating the lock file manually.
> You need an extra space between the "... retry" and "or ...":

> + ereport(ERROR,
> + (errcode_for_file_access(),
> + errmsg("could not create lock file
> \"%s\": %m ", LockFileName),
> + errhint("May be too many concurrent edit
> into file happening, please wait!! and retry"
> + "or .lock is file
> accidently left there please clean the file from config_dir")));

Will fix.

Thank you for review.

With Regards,
Amit Kapila.

On Sunday, January 06, 2013 10:26 AM Boszormenyi Zoltan wrote:
On Saturday, January 05, 2013 12:35 PM Boszormenyi Zoltan wrote:
2013-01-05 05:58 keltezéssel, Amit kapila írta:
> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>> Hi,
>> I am reviewing your patch.
> Thank you very much.
>

All comments are addressed as below:

>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>
>***************
Fixed the white space comment.

># 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.

Fixed.

>Can't we add a new LWLock and use it as a critical section instead
>of waiting for 10 seconds?

Added LWLock and removed sleep logic. After adding the LWLock the lock file name can be changed
as temp file. but presently the patch contains the name as lock file only. please provide the
suggestions.

>I also tried to trigger one of the errors by creating the lock file manually.
>You need an extra space between the "... retry" and "or ...":

Fixed.

>Also, since the SET PERSISTENT patch seems to create a single lock file,
>sizeof(string) (which includes the 0 byte at the end of the string, so it
>matches the whole filename) can be used instead of strlen(string) or
>sizeof(string)-1 that match the filename as a prefix only.
>#define PGAUTOCONFLOCK "postgresql.auto.conf.lock"
+ /* skip lock files used in postgresql.auto.conf edit */
+ if (strncmp(de->d_name,
+ "postgresql.auto.conf.lock",
+ strlen("postgresql.auto.conf.lock")) == 0)

Added a macro and changed the check accordingly.

With Regards,
Amit Kapila.

Hi,

2013-01-09 10:08 keltezéssel, Amit kapila írta:
> On Sunday, January 06, 2013 10:26 AM Boszormenyi Zoltan wrote:
> On Saturday, January 05, 2013 12:35 PM Boszormenyi Zoltan wrote:
> 2013-01-05 05:58 keltezéssel, Amit kapila írta:
>> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>>> Hi,
>>> I am reviewing your patch.
>> Thank you very much.
>>
> All comments are addressed as below:
>
>> 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>
>> ***************
> Fixed the white space comment.
>
>> # 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.
> Fixed.
>
>> Can't we add a new LWLock and use it as a critical section instead
>> of waiting for 10 seconds?
> Added LWLock and removed sleep logic. After adding the LWLock the lock file name can be changed
> as temp file. but presently the patch contains the name as lock file only. please provide the
> suggestions.

Current state of affairs:

a.) The whole operation is protected by the LWLock so only one backend
can do this any time. Clean operation is ensured.

b.) The code creates the lock file and fails if it the file exists. This protects
against nasties done externally. The current logic to bail out with an error
is OK, I can know that there is a stupid intruder on the system. But then
they can also remove the original .auto.conf file too and anything else and
I lost anyway.

c.) In reaper(), the code cleans up the lock file and since there can
be only one lock file, no need to search for temp files, a straightforward
unlink() call does its job.

This may be changed in two ways to make it more comfortable:

1. Simply unlink() and retry with creat() once.

Comments:
unlink() may fail under Windows if someone keeps this file open.
Then you can either give up with ereport(ERROR) just like now.

POSIX systems simply remove the file <-> inode mapping so unlink()
doesn't fail in this case.

2. Create the tempfile. There will be one less error case, the file creation
may only fail in case you are out of disk space.

Creating the tempfile is tempting. But then reaper() will need a little
more code to remove all the tempfiles.

I just noticed that you are using "%m" in format strings twice. man 3 printf says:

m (Glibc extension.) Print output of strerror(errno). No argument is required.

This doesn't work anywhere else, only on GLIBC systems, it means Linux.
I also like the brevity of this extension but PostgreSQL is a portable system.
You should use %s and strerror(errno) as argument explicitly.

>> I also tried to trigger one of the errors by creating the lock file manually.
>> You need an extra space between the "... retry" and "or ...":
> Fixed.
>
>> Also, since the SET PERSISTENT patch seems to create a single lock file,
>> sizeof(string) (which includes the 0 byte at the end of the string, so it
>> matches the whole filename) can be used instead of strlen(string) or
>> sizeof(string)-1 that match the filename as a prefix only.
>> #define PGAUTOCONFLOCK "postgresql.auto.conf.lock"
> + /* skip lock files used in postgresql.auto.conf edit */
> + if (strncmp(de->d_name,
> + "postgresql.auto.conf.lock",
> + strlen("postgresql.auto.conf.lock")) == 0)
>
> Added a macro and changed the check accordingly.

If the current logic stays or the modification in 1) will be chosen,
the comment needs tweaking:

+ /* skip lock files used in postgresql.auto.conf edit */

"skip the lock file ..."

+ if (strncmp(de->d_name,
+ PGAUTOCONFLOCK,
+ sizeof(PGAUTOCONFLOCK)) == 0)
+ continue;
+

If the 2nd suggestion is chosen, sizeof()-1 or strlen() must be uses again
to exclude all the possible tempfiles.

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/

On Friday, January 11, 2013 10:03 PM Boszormenyi Zoltan wrote:
> Hi,

2013-01-09 10:08 keltezéssel, Amit kapila írta:
> On Sunday, January 06, 2013 10:26 AM Boszormenyi Zoltan wrote:
> On Saturday, January 05, 2013 12:35 PM Boszormenyi Zoltan wrote:
> 2013-01-05 05:58 keltezéssel, Amit kapila írta:
>> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>>> Hi,
>>> I am reviewing your patch.
>> Thank you very much.
>>
> All comments are addressed as below:
>

>>> Can't we add a new LWLock and use it as a critical section instead
>>> of waiting for 10 seconds?
>> Added LWLock and removed sleep logic. After adding the LWLock the lock file name can be changed
>> as temp file. but presently the patch contains the name as lock file only. please provide the
>> suggestions.

> Current state of affairs:

> a.) The whole operation is protected by the LWLock so only one backend
> can do this any time. Clean operation is ensured.

> b.) The code creates the lock file and fails if it the file exists. This protects
> against nasties done externally. The current logic to bail out with an error
> is OK, I can know that there is a stupid intruder on the system. But then
> they can also remove the original .auto.conf file too and anything else and
> I lost anyway.

> c.) In reaper(), the code cleans up the lock file and since there can
> be only one lock file, no need to search for temp files, a straightforward
> unlink() call does its job.

> This may be changed in two ways to make it more comfortable:

> 1. Simply unlink() and retry with creat() once.

> Comments:
> unlink() may fail under Windows if someone keeps this file open.
> Then you can either give up with ereport(ERROR) just like now.

I think as this is an internal file, user is not supposed to play with file and incase he does so,
then I think current implementation is okay, means during open (O_CREAT) it gives error and the message
is also suggesting the same.

> 2. Create the tempfile. There will be one less error case, the file creation
> may only fail in case you are out of disk space.

> Creating the tempfile is tempting. But then reaper() will need a little
> more code to remove all the tempfiles.

By reaper you mean to say CATCH block?

In that case, I would prefer to keep the current implementation as it is.

Actualy I was thinking of just changing the extension from current .lock to .tmp, so in that case
the same problems can happen with this also.

> I just noticed that you are using "%m" in format strings twice. man 3 printf says:

> m (Glibc extension.) Print output of strerror(errno). No argument is required.

> This doesn't work anywhere else, only on GLIBC systems, it means Linux.
> I also like the brevity of this extension but PostgreSQL is a portable system.
> You should use %s and strerror(errno) as argument explicitly.

%m is used at other places in code as well.

Thank you for feedback.

With Regards,
Amit Kapila.

2013-01-12 06:51 keltezéssel, Amit kapila írta:
> On Friday, January 11, 2013 10:03 PM Boszormenyi Zoltan wrote:
>> Hi,
> 2013-01-09 10:08 keltezéssel, Amit kapila írta:
>> On Sunday, January 06, 2013 10:26 AM Boszormenyi Zoltan wrote:
>> On Saturday, January 05, 2013 12:35 PM Boszormenyi Zoltan wrote:
>> 2013-01-05 05:58 keltezéssel, Amit kapila írta:
>>> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>>>> Hi,
>>>> I am reviewing your patch.
>>> Thank you very much.
>>>
>> All comments are addressed as below:
>>
>
>>>> Can't we add a new LWLock and use it as a critical section instead
>>>> of waiting for 10 seconds?
>>> Added LWLock and removed sleep logic. After adding the LWLock the lock file name can be changed
>>> as temp file. but presently the patch contains the name as lock file only. please provide the
>>> suggestions.
>> Current state of affairs:
>> a.) The whole operation is protected by the LWLock so only one backend
>> can do this any time. Clean operation is ensured.
>> b.) The code creates the lock file and fails if it the file exists. This protects
>> against nasties done externally. The current logic to bail out with an error
>> is OK, I can know that there is a stupid intruder on the system. But then
>> they can also remove the original .auto.conf file too and anything else and
>> I lost anyway.
>> c.) In reaper(), the code cleans up the lock file and since there can
>> be only one lock file, no need to search for temp files, a straightforward
>> unlink() call does its job.
>
>> This may be changed in two ways to make it more comfortable:
>> 1. Simply unlink() and retry with creat() once.
>> Comments:
>> unlink() may fail under Windows if someone keeps this file open.
>> Then you can either give up with ereport(ERROR) just like now.
> I think as this is an internal file, user is not supposed to play with file and incase he does so,
> then I think current implementation is okay, means during open (O_CREAT) it gives error and the message
> is also suggesting the same.

That's what I meant in b) above.

>
>
>
>> 2. Create the tempfile. There will be one less error case, the file creation
>> may only fail in case you are out of disk space.
>> Creating the tempfile is tempting. But then reaper() will need a little
>> more code to remove all the tempfiles.
> By reaper you mean to say CATCH block?

No, I mean the reaper(SIGNAL_ARGS) function in
src/backend/postmaster/postmaster.c where your patch has this:

*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***************
*** 2552,2557 **** reaper(SIGNAL_ARGS)
--- 2552,2569 ----
continue;
}

+ /* Delete the postgresql.auto.conf.lock file if exists */
+ {
+ char LockFileName[MAXPGPATH];
+
+ strlcpy(LockFileName, ConfigFileName, sizeof(LockFileName));
+ get_parent_directory(LockFileName);
+ join_path_components(LockFileName, LockFileName,
AutoConfigLockFilename);
+ canonicalize_path(LockFileName);
+
+ unlink(LockFileName);
+ }
+
/*
* Startup succeeded, commence normal operations
*/

>
> In that case, I would prefer to keep the current implementation as it is.

I also said above the current logic is OK for me.
I just gave food for thought to provoke discussion from others.

>
> Actualy I was thinking of just changing the extension from current .lock to .tmp, so in that case
> the same problems can happen with this also.

Indeed.

>
>> I just noticed that you are using "%m" in format strings twice. man 3 printf says:
>> m (Glibc extension.) Print output of strerror(errno). No argument is required.
>> This doesn't work anywhere else, only on GLIBC systems, it means Linux.
>> I also like the brevity of this extension but PostgreSQL is a portable system.
>> You should use %s and strerror(errno) as argument explicitly.
> %m is used at other places in code as well.
>
> Thank you for feedback.

You're welcome.

>
> With Regards,
> Amit Kapila.
>
>
>

--
----------------------------------
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/

Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
> No, I mean the reaper(SIGNAL_ARGS) function in
> src/backend/postmaster/postmaster.c where your patch has this:

> *** a/src/backend/postmaster/postmaster.c
> --- b/src/backend/postmaster/postmaster.c
> ***************
> *** 2552,2557 **** reaper(SIGNAL_ARGS)
> --- 2552,2569 ----
> continue;
> }

> + /* Delete the postgresql.auto.conf.lock file if exists */
> + {
> + char LockFileName[MAXPGPATH];
> +
> + strlcpy(LockFileName, ConfigFileName, sizeof(LockFileName));
> + get_parent_directory(LockFileName);
> + join_path_components(LockFileName, LockFileName,
> AutoConfigLockFilename);
> + canonicalize_path(LockFileName);
> +
> + unlink(LockFileName);
> + }
> +
> /*
> * Startup succeeded, commence normal operations
> */

I have not been following this thread, but I happened to see this bit,
and I have to say that I am utterly dismayed that anything like this is
even on the table. This screams overdesign. We do not need a global
lock file, much less postmaster-side cleanup. All that's needed is a
suitable convention about temp file names that can be written and then
atomically renamed into place. If we're unlucky enough to crash before
a temp file can be renamed into place, it'll just sit there harmlessly.

>>> I just noticed that you are using "%m" in format strings twice. man 3 printf says:
>>> m (Glibc extension.) Print output of strerror(errno). No argument is required.
>>> This doesn't work anywhere else, only on GLIBC systems, it means Linux.
>>> I also like the brevity of this extension but PostgreSQL is a portable system.
>>> You should use %s and strerror(errno) as argument explicitly.
>> %m is used at other places in code as well.

As far as that goes, %m is appropriate in elog/ereport (which contain
special support for it), but not anywhere else.

regards, tom lane

On Sunday, January 13, 2013 12:41 AM Tom Lane wrote:
Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>> No, I mean the reaper(SIGNAL_ARGS) function in
>> src/backend/postmaster/postmaster.c where your patch has this:

>> *** a/src/backend/postmaster/postmaster.c
>> --- b/src/backend/postmaster/postmaster.c
>> ***************
>> *** 2552,2557 **** reaper(SIGNAL_ARGS)

> I have not been following this thread, but I happened to see this bit,
> and I have to say that I am utterly dismayed that anything like this is
> even on the table. This screams overdesign. We do not need a global
> lock file, much less postmaster-side cleanup. All that's needed is a
> suitable convention about temp file names that can be written and then
> atomically renamed into place. If we're unlucky enough to crash before
> a temp file can be renamed into place, it'll just sit there harmlessly.

I can think of below ways to generate tmp file
1. Generate session specific tmp file name during operation. This will be similar to what is
currently being done in OpenTemporaryFileinTablespace() to generate a tempfilename.
2. Generate global tmp file name. For this we need to maintain global counter.
3. Always generate temp file with same name "postgresql.auto.conf.tmp", as at a time only one
session is allowed to operate.

In any approach, there will be chance that temp files will remain if server crashes during this command execution
which can lead to collision of temp file name next time user tries to use SET persistent command.
An appropriate error will be raised and user manually needs to delete that file.

>>>> I just noticed that you are using "%m" in format strings twice. man 3 printf says:
>>>> m (Glibc extension.) Print output of strerror(errno). No argument is required.
>>>> This doesn't work anywhere else, only on GLIBC systems, it means Linux.
>>>> I also like the brevity of this extension but PostgreSQL is a portable system.
>>>> You should use %s and strerror(errno) as argument explicitly.
>>> %m is used at other places in code as well.

> As far as that goes, %m is appropriate in elog/ereport (which contain
> special support for it), but not anywhere else.

In the patch, it's used in ereport, so I assume it is safe and patch doesn't need any change for %m.

With Regards,
Amit Kapila.

2013-01-13 05:49 keltezéssel, Amit kapila írta:
> On Sunday, January 13, 2013 12:41 AM Tom Lane wrote:
> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>>> No, I mean the reaper(SIGNAL_ARGS) function in
>>> src/backend/postmaster/postmaster.c where your patch has this:
>>> *** a/src/backend/postmaster/postmaster.c
>>> --- b/src/backend/postmaster/postmaster.c
>>> ***************
>>> *** 2552,2557 **** reaper(SIGNAL_ARGS)
>> I have not been following this thread, but I happened to see this bit,
>> and I have to say that I am utterly dismayed that anything like this is
>> even on the table. This screams overdesign. We do not need a global
>> lock file, much less postmaster-side cleanup. All that's needed is a
>> suitable convention about temp file names that can be written and then
>> atomically renamed into place. If we're unlucky enough to crash before
>> a temp file can be renamed into place, it'll just sit there harmlessly.
> I can think of below ways to generate tmp file
> 1. Generate session specific tmp file name during operation. This will be similar to what is
> currently being done in OpenTemporaryFileinTablespace() to generate a tempfilename.
> 2. Generate global tmp file name. For this we need to maintain global counter.
> 3. Always generate temp file with same name "postgresql.auto.conf.tmp", as at a time only one
> session is allowed to operate.

What's wrong with mkstemp("config_dir/postgresql.auto.conf.XXXXXX")
that returns a file descriptor for an already created file with a unique name?

Note that the working directory of PostgreSQL is $PGDATA so "config_dir"
and files under that can be accessed with a relative path.

But a cleanup somewhere is needed to remove the stale temp files.
I think it's enough at postmaster startup. A machine that's crashing
so often that the presence of the stale temp files causes out of disk
errors would surely be noticed much earlier.

>
> In any approach, there will be chance that temp files will remain if server crashes during this command execution
> which can lead to collision of temp file name next time user tries to use SET persistent command.

mkstemp() replaces the "XXXXXX" suffix with a unique hexadecimal suffix
so there will be no collision.

> An appropriate error will be raised and user manually needs to delete that file.
>
>
>
>>>>> I just noticed that you are using "%m" in format strings twice. man 3 printf says:
>>>>> m (Glibc extension.) Print output of strerror(errno). No argument is required.
>>>>> This doesn't work anywhere else, only on GLIBC systems, it means Linux.
>>>>> I also like the brevity of this extension but PostgreSQL is a portable system.
>>>>> You should use %s and strerror(errno) as argument explicitly.
>>>> %m is used at other places in code as well.
>> As far as that goes, %m is appropriate in elog/ereport (which contain
>> special support for it), but not anywhere else.
> In the patch, it's used in ereport, so I assume it is safe and patch doesn't need any change for %m.

OK.

>
>
> With Regards,
> Amit Kapila.
>
>

--
----------------------------------
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/

On Sunday, January 13, 2013 2:45 PM Boszormenyi Zoltan wrote:
2013-01-13 05:49 keltezéssel, Amit kapila írta:
> On Sunday, January 13, 2013 12:41 AM Tom Lane wrote:
> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>>>> No, I mean the reaper(SIGNAL_ARGS) function in
>>>> src/backend/postmaster/postmaster.c where your patch has this:
>>>> *** a/src/backend/postmaster/postmaster.c
>>>> --- b/src/backend/postmaster/postmaster.c
>>>> ***************
>>>> *** 2552,2557 **** reaper(SIGNAL_ARGS)
>>> I have not been following this thread, but I happened to see this bit,
>>> and I have to say that I am utterly dismayed that anything like this is
>>> even on the table. This screams overdesign. We do not need a global
>>> lock file, much less postmaster-side cleanup. All that's needed is a
>>> suitable convention about temp file names that can be written and then
>>> atomically renamed into place. If we're unlucky enough to crash before
>>> a temp file can be renamed into place, it'll just sit there harmlessly.
>> I can think of below ways to generate tmp file
>> 1. Generate session specific tmp file name during operation. This will be similar to what is
>> currently being done in OpenTemporaryFileinTablespace() to generate a tempfilename.
>> 2. Generate global tmp file name. For this we need to maintain global counter.
>> 3. Always generate temp file with same name "postgresql.auto.conf.tmp", as at a time only one
>> session is allowed to operate.

> What's wrong with mkstemp("config_dir/postgresql.auto.conf.XXXXXX")
> that returns a file descriptor for an already created file with a unique name?

I think for Windows exactly same behavior API might not exist, we might need to use GetTempFileName.
It will generate some different kind of name, which means cleanup also need to take care of different kind of names.
So if this is right, I think it might not be very appropriate to use it.

Please point me if I am wrong as I have not used it.

> Note that the working directory of PostgreSQL is $PGDATA so "config_dir"
> and files under that can be accessed with a relative path.

> But a cleanup somewhere is needed to remove the stale temp files.
> I think it's enough at postmaster startup. A machine that's crashing
> so often that the presence of the stale temp files causes out of disk
> errors would surely be noticed much earlier.

In PostmasterMain(), we already call RemovePGTempFiles(); So I think we can delete this tmp file at
that place in PostmasterMain().
I shall do this, if nobody raise objection about it.

With Regards,
Amit Kapila.

2013-01-14 07:47 keltezéssel, Amit kapila írta:
> On Sunday, January 13, 2013 2:45 PM Boszormenyi Zoltan wrote:
> 2013-01-13 05:49 keltezéssel, Amit kapila írta:
>> On Sunday, January 13, 2013 12:41 AM Tom Lane wrote:
>> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>>>>> No, I mean the reaper(SIGNAL_ARGS) function in
>>>>> src/backend/postmaster/postmaster.c where your patch has this:
>>>>> *** a/src/backend/postmaster/postmaster.c
>>>>> --- b/src/backend/postmaster/postmaster.c
>>>>> ***************
>>>>> *** 2552,2557 **** reaper(SIGNAL_ARGS)
>>>> I have not been following this thread, but I happened to see this bit,
>>>> and I have to say that I am utterly dismayed that anything like this is
>>>> even on the table. This screams overdesign. We do not need a global
>>>> lock file, much less postmaster-side cleanup. All that's needed is a
>>>> suitable convention about temp file names that can be written and then
>>>> atomically renamed into place. If we're unlucky enough to crash before
>>>> a temp file can be renamed into place, it'll just sit there harmlessly.
>>> I can think of below ways to generate tmp file
>>> 1. Generate session specific tmp file name during operation. This will be similar to what is
>>> currently being done in OpenTemporaryFileinTablespace() to generate a tempfilename.
>>> 2. Generate global tmp file name. For this we need to maintain global counter.
>>> 3. Always generate temp file with same name "postgresql.auto.conf.tmp", as at a time only one
>>> session is allowed to operate.
>> What's wrong with mkstemp("config_dir/postgresql.auto.conf.XXXXXX")
>> that returns a file descriptor for an already created file with a unique name?
> I think for Windows exactly same behavior API might not exist, we might need to use GetTempFileName.
> It will generate some different kind of name, which means cleanup also need to take care of different kind of names.
> So if this is right, I think it might not be very appropriate to use it.

In that case (and also because the LWLock still serialize the whole procedure)
you can use GetTempFileName() on Windows and tempnam(3) for non-Windows.
GetTempFileName() and tempnam() return the constructed temporary file name
out of the directory and the filename prefix components.

The differences are:
1. For GetTempFileName(), you have to provide a buffer and it uses 3 bytes from
the prefix string. Always give you this path: dir\pfx<uuu>.TMP

2. tempnam() gives you a malloc()ed result buffer and uses at most 5 bytes from
the prefix string. According to the NOTES section in the man page, the specified
directory is only considered if $TMPDIR is not set in the environment or the
directory is not usable, like write is not permitted or the directory is missing.
In this case. If TMPDIR not only is present and usable but on a different filesystem
as the target config_dir/postgresql.auto.conf, rename() would return the EXDEV
error:

EXDEV The links named by new and old are on different file systems and the
implementation does not support links
between file systems.

Maybe mktemp(3) (not mk*s*temp!) instead of tempnam() is better here
because it always uses the specified template. You have to use you own
buffer for mktemp(). The BUGS section in man 3 mktemp says:

BUGS
Never use mktemp(). Some implementations follow 4.3BSD and replace XXXXXX by the
current process ID and a single
letter, so that at most 26 different names can be returned. Since on the one hand
the names are easy to guess, and
on the other hand there is a race between testing whether the name exists and
opening the file, every use of
mktemp() is a security risk. The race is avoided by mkstemp(3).

I think we don't really care about the security risk about the file name
being easy to guess and there is no race condition because of the LWLock.

With GetTempFileName() and mktemp(), you can have a lot of common code,
like pass the same 3-letter prefix and preallocate or statically declare the output buffer.

> Please point me if I am wrong as I have not used it.
>
>> Note that the working directory of PostgreSQL is $PGDATA so "config_dir"
>> and files under that can be accessed with a relative path.
>> But a cleanup somewhere is needed to remove the stale temp files.
>> I think it's enough at postmaster startup. A machine that's crashing
>> so often that the presence of the stale temp files causes out of disk
>> errors would surely be noticed much earlier.
> In PostmasterMain(), we already call RemovePGTempFiles(); So I think we can delete this tmp file at
> that place in PostmasterMain().
> I shall do this, if nobody raise objection about it.

No objection.

>
> With Regards,
> Amit Kapila.
>

--
----------------------------------
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/

On Monday, January 14, 2013 6:48 PM Boszormenyi Zoltan wrote:
2013-01-14 07:47 keltezéssel, Amit kapila írta:
> On Sunday, January 13, 2013 2:45 PM Boszormenyi Zoltan wrote:
> 2013-01-13 05:49 keltezéssel, Amit kapila írta:
>> On Sunday, January 13, 2013 12:41 AM Tom Lane wrote:
>> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:

>>>> I can think of below ways to generate tmp file
>>>> 1. Generate session specific tmp file name during operation. This will be similar to what is
>>>> currently being done in OpenTemporaryFileinTablespace() to generate a tempfilename.
>>>> 2. Generate global tmp file name. For this we need to maintain global counter.
>>>> 3. Always generate temp file with same name "postgresql.auto.conf.tmp", as at a time only one
>>>> session is allowed to operate.
>>> What's wrong with mkstemp("config_dir/postgresql.auto.conf.XXXXXX")
>>> that returns a file descriptor for an already created file with a unique name?
>> I think for Windows exactly same behavior API might not exist, we might need to use GetTempFileName.
>> It will generate some different kind of name, which means cleanup also need to take care of different kind of names.
>> So if this is right, I think it might not be very appropriate to use it.

> In that case (and also because the LWLock still serialize the whole procedure)
> you can use GetTempFileName() on Windows and tempnam(3) for non-Windows.
> GetTempFileName() and tempnam() return the constructed temporary file name
> out of the directory and the filename prefix components.

Okay, I shall work on this based on your suggestion.

With Regards,
Amit Kapila.