Обсуждение: 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