Thread: pg_reload_conf() synchronously

In an internal conversation it was seen that for some tests that want
to enforce a behaviour, a behaviour that is controlled by a GUC, we
_need_ to perform a sleep for an arbitrary amount of time.
Alternatively, executing the rest of the test on a new connection also
helps to get the expected behaviour. Following is a sample snippet of
such a test.

ALTER SYSTEM SET param TO value;
SELECT pg_reload_conf();
-- Either pg_sleep(0.1) or \connect here for next command to behave as expected.
ALTER ROLE ... PASSWORD '...';

This is because of the fact that the SIGHUP, sent to Postmaster by
this backend, takes some time to get back to the issuing backend.

Although a pg_sleep() call works to alleviate the pain in most cases,
it does not provide any certainty. Following the Principle Of Least
Astonishment, we want a command that loads the configuration
_synchronously_, so that the subsequent commands from the client can
be confident that the requisite parameter changes have taken effect.

The attached patch makes the pg_reload_conf() function set
ConfigReloadPending to true, which will force the postgres main
command-processing loop to process and apply config changes _before_
executing the next command.

The only downside I can think of this approach is that it _might_
cause the issuing backend to process the config file twice; once after
it has processed the current command, and another time when the SIGHUP
signal comes from the Postmaster. If the SIGHUP signal from the
Postmaster comes before the end of current command, then the issuing
backend will process the config only once, as before the patch. In any
case, this extra processing of the config seems harmless, and the
benefit outweighs the extra processing done.

The alternate methods that I considered (see branch reload_my_conf at
[2])  were to either implement the SQL command RELOAD CONFIGURATION [
FOR ALL ], or to create an overloaded version of function
pg_reload_conf(). But both those approaches had much more significant
downsides, like additional GRANTs, etc.

There have been issues identified in the past (see [1]) that possibly
may be alleviated by this approach of applying config changes
synchronously.

[1]: https://www.postgresql.org/message-id/2138662.1623460441%40sss.pgh.pa.us
[2]: https://github.com/gurjeet/postgres/tree/reload_my_conf

Best regards,
Gurjeet
http://Gurje.et

Hi,

On 2022-11-04 10:26:38 -0700, Gurjeet Singh wrote:
> The attached patch makes the pg_reload_conf() function set
> ConfigReloadPending to true, which will force the postgres main
> command-processing loop to process and apply config changes _before_
> executing the next command.

Worth noting that this doesn't necessarily suffice to avoid race conditions in
tests, if the test depends on *other* backends having seen the configuration
changes.

It might be worth to use the global barrier mechanism to count which backends
have reloaded configuration and to provide a function / option to pg_sleep
that waits for that.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> Worth noting that this doesn't necessarily suffice to avoid race conditions in
> tests, if the test depends on *other* backends having seen the configuration
> changes.

True, but do we have any such cases?

> It might be worth to use the global barrier mechanism to count which backends
> have reloaded configuration and to provide a function / option to pg_sleep
> that waits for that.

That ... seems like a lot of mechanism.  And it could easily result
in undetected deadlocks, if any other session is blocked on you.
I seriously doubt that we should go there.

            regards, tom lane

Hi,

On 2022-11-04 23:35:21 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Worth noting that this doesn't necessarily suffice to avoid race conditions in
> > tests, if the test depends on *other* backends having seen the configuration
> > changes.
> 
> True, but do we have any such cases?

I know I hit it twice and gave up on the tests.


> > It might be worth to use the global barrier mechanism to count which backends
> > have reloaded configuration and to provide a function / option to pg_sleep
> > that waits for that.
> 
> That ... seems like a lot of mechanism.  And it could easily result
> in undetected deadlocks, if any other session is blocked on you.
> I seriously doubt that we should go there.

Yea, it's not great. Probably ok enough for a test, but ...

Greetings,

Andres Freund

On Fri, Nov 04, 2022 at 09:51:08PM -0700, Andres Freund wrote:
> On 2022-11-04 23:35:21 -0400, Tom Lane wrote:
>> True, but do we have any such cases?
>
> I know I hit it twice and gave up on the tests.

Still, is there any need for a full-blown facility for the case of an
individual backend?  Using a new function to track that all the
changes are in effect is useful for isolation tests, less for single
sessions where a function to wait for all the backends is no different
than a \c to enforce a reload because both tests would need an extra
step (on top of making parallel tests longer if something does a long
transaction?).

As far as I know, Gurjeet has been annoyed only with non-user-settable
GUCs for one connection (correct me of course), there was nothing
fancy with isolation tests, yet.  Not saying that this is useless for
isolation tests, this would have its cases for assumptions where
multiple GUCs need to be synced across multiple sessions, but it seems
to me that we have two different cases in need of two different
solutions.
--
Michael

Hi,

On 2022-11-05 14:26:44 +0900, Michael Paquier wrote:
> On Fri, Nov 04, 2022 at 09:51:08PM -0700, Andres Freund wrote:
> > On 2022-11-04 23:35:21 -0400, Tom Lane wrote:
> >> True, but do we have any such cases?
> > 
> > I know I hit it twice and gave up on the tests.
> 
> Still, is there any need for a full-blown facility for the case of an
> individual backend?

No, and I didn't claim it was.


> Using a new function to track that all the changes are in effect is useful
> for isolation tests

I hit it in tap tests, fwiw.


> As far as I know, Gurjeet has been annoyed only with non-user-settable
> GUCs for one connection (correct me of course), there was nothing
> fancy with isolation tests, yet.  Not saying that this is useless for
> isolation tests, this would have its cases for assumptions where
> multiple GUCs need to be synced across multiple sessions, but it seems
> to me that we have two different cases in need of two different
> solutions.

I didn't say we need to go for something more complete. Just that it's worth
thinking about.

Greetings,

Andres Freund

On Sat, Nov 5, 2022 at 11:23 AM Andres Freund <andres@anarazel.de> wrote:
> > As far as I know, Gurjeet has been annoyed only with non-user-settable
> > GUCs for one connection (correct me of course), there was nothing
> > fancy with isolation tests, yet.  Not saying that this is useless for
> > isolation tests, this would have its cases for assumptions where
> > multiple GUCs need to be synced across multiple sessions, but it seems
> > to me that we have two different cases in need of two different
> > solutions.
>
> I didn't say we need to go for something more complete. Just that it's worth
> thinking about.

FWIW, I have considered a few different approaches, but all of them
were not only more work, they were fragile, and diffcult to prove
correctness of. For example, I thought of implementing DSM based
synchronisation between processes, so that the invoking backend can be
sure that _all_ children of Postmaster have processed the reload. But
that will run into problems as different backends get created, and as
they exit.

The invoking client may be interested in just client-facing backends
honoring the reload before moving on, so it would have to wait until
all the other backends finish their current command and return to the
main loop; but that may never happen, because one of the backends may
be processing a long-running query. Or, for some reason, the the
caller may be interested in only the autovacuum proccesses honoring
its reload request. So I thought about creating _classes_ of backends:
client-facing, other always-on children of Postmaster, BGWorkers, etc.
But that just makes the problem more difficult to solve.

I hadn't considered the possibilty of deadlocks that Tom raised, so
that's another downside of the other approaches.

The proposed patch solves a specific problem, that of a single backend
reloading conf before the next command, without adding any complexity
for any other cases. It sure is worth solving the case where a backend
waits for another backed(s) to process the conf files, but it will
have to be a separate solution, becuase it's much more difficult to
get it right.

Best regards,
Gurjeet
http://Gurje.et

Hi,

On 2022-11-05 12:03:49 -0700, Gurjeet Singh wrote:
> For example, I thought of implementing DSM based synchronisation between
> processes, so that the invoking backend can be sure that _all_ children of
> Postmaster have processed the reload. But that will run into problems as
> different backends get created, and as they exit.

If you just want something like that you really can just use the global
barrier mechanism.  The hard part is how to deal with situations like two
backends waiting at the same time. Possibly the best way would be to not
actually offer a blocking API but just a way to ask whether a change has been
processed everywhere, and require explicit polling on the client side.


> The proposed patch solves a specific problem, that of a single backend
> reloading conf before the next command, without adding any complexity
> for any other cases.

I'm not sure that's true btw - I seem to recall that there's code somewhere
noting that it relies on postmaster being the first to process a config
change. Which wouldn't be true with this change anymore. I remember not being
convinced by that logic, because successive config changes can still lead to
backends processing the config file first.

Greetings,

Andres Freund