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

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_nameFROM CURRENT_P                                {                                        VariableSetStmt *n =
 
makeNode(VariableSetStmt);                                        n->kind = VAR_SET_CURRENT;
           n->name = $1;                                        $$ = n;                                } 
 

> > > And why is it possible to persistently set the search path,
> > >   but not client encoding? Why is FROM CURRENT in set_rest_more?
> >
> > I think the main reason was some of the commands can work only in
> > transaction  and as SET Persistent cannot work inside transaction
> block, so I had kept
> > some seggregation.
> 
> Yes, I can see reasons for doing this, I just think the split isn't
> correct as youve done it.
> 
> > > * 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


> >   I can think of one reason where it will be better to have .$pid,
> and that
> > is if some one has opened the
> >   file manually, then all other sessions can fail (in WINDOWS).
> Infact this
> > is one of test Zoltan had performed.
> 
> 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. 


> > > * 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())

> > > * write_auto_conf_file should probably escape quoted values?
> >   Can you please elaborate more, I am not able to understand your
> >   point?
> 
> You do something like (don't have the code right here)
> if (quoted)
>    appendStringInfo("= \"%s\"", value).
> what happens if value contains a "?

I think this case is missed. I shall take care to handle the case when value
contains such value.


> > > * coding style should be adhered to more closesly, there are many
> > >   if (pointer) which should be if (pointer != NULL),
> >
> > Are you pointing in function  validate_conf_option(const char *name,
> char  *value)
> >   for below usage:
> >   +                                 if (value)
> 
> For example, yes.

Okay, I shall fix these, but I have seen such code at other places
(set_config_option), that's why I have kept it like how it is currently.

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

> Should we find out its safe, fine, relaxing restrictions lateron is no
> problem, imposing ones after introducing a feature is.

I have discussed most of these restrictions in this thread. But there can be
few which I could have missed.

With Regards,
Amit Kapila.