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

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.

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

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

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

> > * 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 "?

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

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

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

Greetings,

Andres Freund

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