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

On Thursday, January 24, 2013 6:51 PM Andres Freund wrote:
> 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.

I am working on to provide the exact reasoning of split and if some variable
is not appropriate,
I shall do the appropriate movement.

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

Okay, as I told you that I have also proposed initially, so as now you have
pointed it specifically,
I shall do it that way unless somebody will have another strong point of not
doing it this way.

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

I have no specific reason, so I shall do it the way you have suggested.

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

Okay, I will check this and let you know if I have any doubts.

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

I think the core point of this command behavior is that it is not related to
transaction boundary.
So now the question is in such case if we don't allow this command inside
transaction block, why to allow inside
Function?
When user starts transaction and runs this command and then few other
commands and now if he rollback, this command cannot be rolledback.
OTOH in function there is no explicit way to issue rollback, only an error
can cause rollback which is same as when running outside block.

With Regards,
Amit Kapila.