Subject: Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
To: Stephen Frost <sfrost@snowman.net>
Cc: Amit Kapila <amit.kapila16@gmail.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
References: <aed6cc9f-98f3-2693-ac81-52bb0052307e@2ndquadrant.com>
 <20190614160835.GF2480@tamriel.snowman.net>
 <CAA4eK1JW9UfLOtMEJq5b_YcogbWdxzAGUzNi72F2Xx4r1_h6QA@mail.gmail.com>
 <20190616171610.GO2480@tamriel.snowman.net>
 <27dc9f8d-8d8f-9dd1-0494-5d8bd2f49a45@2ndquadrant.com>
 <20190617154133.GF2480@tamriel.snowman.net>
From: Ian Barwick <ian.barwick@2ndquadrant.com>
Message-ID: <2e9f82a8-27e8-60cf-7df3-616d6707eede@2ndquadrant.com>
Date: Wed, 19 Jun 2019 13:57:08 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <20190617154133.GF2480@tamriel.snowman.net>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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

The question is though - is this a change which is practical to make at this point
in the release cycle for Pg12?


Regards

Ian Barwick


-- 
  Ian Barwick                   https://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services