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

On Thursday, January 24, 2013 10:33 PM Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
> >> Say again? Surely the temp file is being written by whichever
> backend
> >> is executing SET PERSISTENT, and there could be more than one.
>
> > Sure, but the patch acquires SetPersistentLock exlusively beforehand
> > which seems fine to me.
>
> Why should we have such a lock? Seems like that will probably
> introduce
> as many problems as it fixes. Deadlock risk, blockages, etc. It is
> not
> necessary for atomicity, since rename() would be atomic already.
>
> > Any opinion whether its acceptable to allow SET PERSISTENT in
> functions?
> > It seems absurd to me to allow it, but Amit seems to be of another
> > opinion.
>
> Well, it's really a definitional question I think: do you expect that
> subsequent failure of the transaction should cause such a SET to roll
> back?
>
> I think it would be entirely self-consistent to define SET PERSISTENT
> as
> a nontransactional operation. Then the implementation would just be to
> write the file immediately when the command is executed, and there's no
> particular reason why it can't be allowed inside a transaction block.
>
> If you want it to be transactional, then the point of disallowing it in
> transaction blocks (or functions) would be to not have a very large
> window between writing the file and committing. But it's still
> possible
> that the transaction would fail somewhere in there, leading to the
> inconsistent outcome that the transaction reports failing but we
> applied
> the SET anyway. I do agree that it would be nonsensical to allow SET
> PERSISTENT in functions but not transaction blocks.

As we have agreed on before that this command has to be non-transactional,
so
lets stick on that and about allowing it in functions and not in transaction
blocks,
I had some reasoning behind it as below, but as it is not making much sense
we can disallow it for both transaction blocks as well as functions:

Reasoning for disallowing in transaction blocks and allowing in functions:
----------------------------------------------------------------------------
--
" 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."

> Say what? I thought the plan was one setting per file, so that we don't
get involved > in having to parse-and-edit the file contents.
> What was all that argument about a new directory, if we're only using one
file?

A new directory (named as auto.conf.d, initially in the patch named as
config_dir) is for having automatically generated configuration files.
Also it was suggested previously that it is better to have this file in
directory instead of standalone file.

> If we are using just one file, then I agree a lock would be needed to
synchronize
> updates. But that seems to add a lot of complication elsewhere.

There were 2 ways to protect the file write,
a. one is by having lock SetPersistentLock
b. another is by opening file in X mode and if open fails wait and retry for
sometime
and then give error.
As suggested in this thread I have implemented as Approach-1.
What is the complication it can add, can we resolve that?

With Regards,
Amit Kapila.