On Wed, Jun 19, 2019 at 10:27 AM Ian Barwick
<ian.barwick@2ndquadrant.com> wrote:
>
> n 6/18/19 12:41 AM, Stephen Frost wrote:
> > Greetings,
> >
> > * Ian Barwick (ian.barwick@2ndquadrant.com) wrote
> (...)
>
> >> I suggest explicitly documenting postgresql.auto.conf behaviour (and the circumstances
> >> where it's acceptable to modify it outside of ALTER SYSTEM calls) in the documentation
> >> (and possibly in the code), so anyone writing utilities which need to
> >> append to postgresql.auto.conf knows what the situation is.
> >
> > Yeah, I would think that, ideally, we'd have some code in the common
> > library that other utilities could leverage and which the backend would
> > also use.
>
> So maybe something along the lines of creating a stripped-down variant of
> AlterSystemSetConfigFile() (from "src/backend/utils/misc/guc.c") which can be
> called from front-end code to safely modify .auto.conf while the server is *not*
> running.
>
> I'm not terribly familiar with the GUC code, but that would presumably mean making
> parts of the GUC parsing/handling code linkable externally (ParseConfigFp() etc.)
>
Yeah, this looks a bit tricky as we can't use ereport in the frontend
code and that is used at multiple places in that code path.
> as you'd need to parse the file before rewriting it. Something like (minimal
> pseudo-code):
>
> void
> alter_system_set(char *name, char *value)
> {
> /*
> * check here that the server is *not* running
> */
> ...
> ParseConfigFp(infile, AutoConfFileName, 0, LOG, &head, &tail)
> ...
>
> /*
> * some robust portable way of ensuring another process can't
> * modify the file(s) until we're done
> */
> lock_file(AutoConfFileName);
>
> replace_auto_config_value(&head, &tail, name, value);
>
> write_auto_conf_file(AutoConfTmpFileName, head)
>
> durable_rename(AutoConfTmpFileName, AutoConfFileName, ERROR);
>
> FreeConfigVariables(head);
> unlock_file(AutoConfFileName);
> }
>
> I'm not sure how feasible it is to validate the provided parameter
> without the server running, but if not possible, that's not any worse than the
> status quo, i.e. the utility has to be trusted to write the correct parameters
> to the file anyway.
>
Right. I think even if someone has given wrong values, it will get
detected on next reload.
> The question is though - is this a change which is practical to make at this point
> in the release cycle for Pg12?
>
It depends on the solution/patch we come up with to solve this issue.
What is the alternative? If we allow Alter System to remove the
duplicate entries and call the current situation good, then we are
in-a-way allowing the room for similar or more problems in the future.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com