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

On 2013-01-24 18:37:29 +0530, Amit Kapila wrote:
> On Thursday, January 24, 2013 5:25 PM Andres Freund wrote:
> > On 2013-01-24 16:45:42 +0530, Amit Kapila wrote:
> > > > * The gram.y changes arround set_rest_(more|common) seem pretty
> > > > confused
> > > > to me.
> > >
> > > >E.g. its not possible anymore to set the timezone for a function.
> > >
> > > What do you exactly mean by this part of comment.
> >
> > The set_rest_more production is used in FunctionSetResetClause and
> > youve
> > removed capabilities from it.
>
> set_rest_more has set_reset_common, so there should be no problem.
>
> set_rest_more: /* Generic SET syntaxes: */
> set_rest_common
> | var_name FROM CURRENT_P
> {
> VariableSetStmt *n = makeNode(VariableSetStmt);
> n->kind = VAR_SET_CURRENT;
> n->name = $1;
> $$ = n;
> }

True. I still think that the split youve made isn't right as-is.

> > > > * Writing the temporary file to .$pid seems like a bad idea, better use
> > > > one file for that, SET PERSISTENT is protected by an exclusive lock
> > > > anyway.
> > >
> > > I think we can use one temporary file, infact that was one of the ways I
> > > have asked in one of the previous mails.
> > > However Tom and Zoltan felt this is better way to do it.

> > The have? I didn't read it like that. The file can only ever written by
> > a running postmaster and we already have code that ensures that.
> > There's
> > absolutely no need for the tempfile to have a nondeterministic
> > name. That makes cleanup way easier as well.
>
> Sure, I understand that cleaning will be easier. So IIUC you are suggesting,
> we should just keep
> name as postgresql.auto.conf.tmp
>
> In general, please read the below mail where it has been suggested to use
> .$pid
> http://www.postgresql.org/message-id/28379.1358541135@sss.pgh.pa.us

That was in the context of your use of mkstemp() as far as I read
it. We use constantly named temp files in other locations as well.

> > Why on earth should somebody have the tempfile open? There's an
> > exclusive lock around writing the file in your code and if anybody
> > interferes like that with postgres' temporary data you have far bigger
> > problems than SET PERSISTENT erroring out.
>
> I am also not sure, but may be some people do for test purpose.

That seems like an completely pointless reasoning.

> > > > * the write sequence should be:
> > > > * fsync(tempfile)
> > > > * fsync(directory)
> > > > * rename(tempfile, configfile)
> > > > * fsync(configfile)
> > > > * fsync(directory)
> > >
> > > Why do we need fsync(directory) and fsync(configfile)?
> >
> > So they don't vanish /get corrupted after a crash? The above is the
> > canonically safe way to safely write a file without an invalid file
> > ever
> > being visible.
>
> Do you think there will be problem if we just use as below:
> * fsync(tempfile)
> * rename(tempfile, configfile)
>
> I have seen such kind of use elsewhere also in code (writeTimeLineHistory())

Yea, there are some places where the code isn't entirely safe. No reason
to introduce more of those.
> > > > * the check that prevents persistent SETs in a transaction should
> > > > rather
> > > > be in utility.c and use PreventTransactionChain like most of the
> > > > others that need to do that (c.f. T_CreatedbStmt).
> > >
> > > We can move the check in utility.c, but if we use
> > PreventTransactionChain,
> > > then it will be disallowed in functions as well. But the original
> > idea was to not support in
> > > transaction blocks only.
> > > So I feel use of current function IsTransactionBlock() should be
> > okay.
> >
> > I don't think its even remotely safe to allow doing this from a
> > function. Doing it there explicitly allows doing it in a transaction.
>
> As SET command is allowed inside a function, so I don't think without any
> reason we should disallow SET PERSISTENT in function.
> The reason why it's not allowed in transaction is that we have to delete the
> temporary file in transaction end or at rollback which could have made the
> logic much more complex.

Yea, and allowing use in functions doesn't help you with that at all:

Consider the following plpgsql function:

$$
BEGIN
SET PERSISTENT foo = 'bar';
RAISE ERROR 'blub';
END;
$$

Now you have a SET PERSISTENT that succeeded although the transaction
failed.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services