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