Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Fri, Jan 28, 2022 at 9:51 PM Julien Rouhaud wrote: > On Fri, Jan 28, 2022 at 10:20:07AM -0800, Andres Freund wrote: > > On 2022-01-28 21:56:36 +0800, Julien Rouhaud wrote: > > > I think having a new option for vacuumdb is the right move. > > > > Can't we pass the option via the connection strin

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Hi, On Fri, Jan 28, 2022 at 10:20:07AM -0800, Andres Freund wrote: > > On 2022-01-28 21:56:36 +0800, Julien Rouhaud wrote: > > I think having a new option for vacuumdb is the right move. > > Can't we pass the option via the connection string, e.g. something > PGOPTIONS='-c binary_upgrade_mode=tr

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Hi, On 2022-01-28 21:56:36 +0800, Julien Rouhaud wrote: > I think having a new option for vacuumdb is the right move. Can't we pass the option via the connection string, e.g. something PGOPTIONS='-c binary_upgrade_mode=true'? That seems to scale better than to add it gradually to multiple tools.

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade*

On Fri, Jan 28, 2022 at 03:06:57PM +0100, Denis Laxalde wrote: > Julien Rouhaud a écrit : > > I think having a new option for vacuumdb is the right move. > > > > It seems unlikely that any cron or similar on the host will try to run some > > concurrent vacuumdb, but we still have to enforce that o

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Julien Rouhaud a écrit : I think having a new option for vacuumdb is the right move. It seems unlikely that any cron or similar on the host will try to run some concurrent vacuumdb, but we still have to enforce that only the one executed by pg_upgrade can succeed. I guess it could be an undocum

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Hi, On Fri, Jan 28, 2022 at 11:02:46AM +0100, Denis Laxalde wrote: > > I tried that simple change first: > > diff --git a/src/backend/access/transam/xlog.c > b/src/backend/access/transam/xlog.c > index dfe2a0bcce..8feab0cb96 100644 > --- a/src/backend/access/transam/xlog.c > +++ b/src/backend/ac

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Hi, Julien Rouhaud a écrit : On Wed, 27 Jan 2021 11:25:11 +0100 Denis Laxalde wrote: Andres Freund a écrit : b) when in binary upgrade mode / -b, error out on all wal writes in sessions that don't explicitly set a session-level GUC to allow writes. It should be enough to add an ad

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Hi, On Thu, Jan 13, 2022 at 06:44:12PM -0800, Andres Freund wrote: > > The point is that we need the check for WAL writes / xid assignments / etc > *either* way. There are ways extensions could trigger problems like e.g. xid > assigned, besides bgworker doing stuff. Or postgres components doing s

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Hi, On 2022-01-12 17:54:31 +0800, Julien Rouhaud wrote: > On Sat, Aug 28, 2021 at 10:40:42AM +0900, Michael Paquier wrote: > > On Fri, Aug 27, 2021 at 12:28:42PM -0700, Andres Freund wrote: > > > Isn't that just going to end up with extension code erroring out and/or > > > blocking waiting for a b

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Sat, Aug 28, 2021 at 10:40:42AM +0900, Michael Paquier wrote: > On Fri, Aug 27, 2021 at 12:28:42PM -0700, Andres Freund wrote: > > Isn't that just going to end up with extension code erroring out and/or > > blocking waiting for a bgworker to start? > > Well, that's the point to block things dur

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Sat, Aug 28, 2021 at 9:40 AM Michael Paquier wrote: > > On Fri, Aug 27, 2021 at 12:28:42PM -0700, Andres Freund wrote: > > Isn't that just going to end up with extension code erroring out and/or > > blocking waiting for a bgworker to start? > > Well, that's the point to block things during an u

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Sat, Aug 28, 2021 at 3:28 AM Andres Freund wrote: > > > On Fri, Aug 27, 2021 at 7:31 AM Michael Paquier wrote: > > > > > Wouldn't it be better to block things at the source, as of > > > RegisterBackgroundWorker()? And that would keep track of the control > > > we have on bgworkers in a single

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Fri, Aug 27, 2021 at 12:28:42PM -0700, Andres Freund wrote: > Isn't that just going to end up with extension code erroring out and/or > blocking waiting for a bgworker to start? Well, that's the point to block things during an upgrade. Do you have a list of requirements you'd like to see satis

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Hi, On 2021-08-27 09:34:24 +0800, Julien Rouhaud wrote: > On Fri, Aug 27, 2021 at 7:31 AM Michael Paquier wrote: > > > > Indeed, there is some history here with autovacuum. I have not been > > careful enough to check that. Still, putting a check on > > IsBinaryUpgrade in bgworker_should_start_n

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Fri, Aug 27, 2021 at 12:41 PM Michael Paquier wrote: > > Perhaps. That feels like a topic different than what's discussed > here, though, because we don't really need to check if a bgworker has > been launched or not. We just need to make sure that it never runs in > the context of a binary u

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Fri, Aug 27, 2021 at 11:25:19AM +0800, Julien Rouhaud wrote: > Right now AFAICT there's no official API to check if a call to > RegisterBackgroundWorker() succeeded or not, but an extension could > check by itself using BackgroundWorkerList in bgworker_internals.h, > and error out or something i

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Fri, Aug 27, 2021 at 10:02 AM Michael Paquier wrote: > > On Fri, Aug 27, 2021 at 09:34:24AM +0800, Julien Rouhaud wrote: > > That shouldn't lead to any problem right? > > Well, bgworker_should_start_now() does not exist for that, and > RegisterBackgroundWorker() is the one doing the classificat

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Fri, Aug 27, 2021 at 09:34:24AM +0800, Julien Rouhaud wrote: > That shouldn't lead to any problem right? Well, bgworker_should_start_now() does not exist for that, and RegisterBackgroundWorker() is the one doing the classification job, so it would be more consistent to keep everything under con

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Fri, Aug 27, 2021 at 7:31 AM Michael Paquier wrote: > > Indeed, there is some history here with autovacuum. I have not been > careful enough to check that. Still, putting a check on > IsBinaryUpgrade in bgworker_should_start_now() would mean that we > still keep track of the set of bgworkers

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Thu, Aug 26, 2021 at 10:34:48AM -0400, Bruce Momjian wrote: > On Thu, Aug 26, 2021 at 03:59:49PM +0200, Daniel Gustafsson wrote: >> Agreed, in this particular case I think there is merit to the idea of >> enforcing >> it in the backend. > > OK, works for me Indeed, there is some history here

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Thu, Aug 26, 2021 at 03:59:49PM +0200, Daniel Gustafsson wrote: > > On 26 Aug 2021, at 15:43, Julien Rouhaud wrote: > > > > Le jeu. 26 août 2021 à 21:38, Daniel Gustafsson > > a écrit : > > > On 26 Aug 2021, at 15:09, Bruce Momjian > > > wrote

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Bruce Momjian a écrit : On Thu, Aug 26, 2021 at 03:38:23PM +0200, Daniel Gustafsson wrote: On 26 Aug 2021, at 15:09, Bruce Momjian wrote: On Thu, Aug 26, 2021 at 03:24:33PM +0800, Julien Rouhaud wrote: .. I still think that changing bgworker_should_start_now() is a better option. I am not sur

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

> On 26 Aug 2021, at 15:43, Julien Rouhaud wrote: > > Le jeu. 26 août 2021 à 21:38, Daniel Gustafsson > a écrit : > > On 26 Aug 2021, at 15:09, Bruce Momjian > > wrote: > > > Basically, pg_upgrade has avoided any backend changes that could be >

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Le jeu. 26 août 2021 à 21:38, Daniel Gustafsson a écrit : > > On 26 Aug 2021, at 15:09, Bruce Momjian wrote: > > > Basically, pg_upgrade has avoided any backend changes that could be > > controlled by GUCs and I am not sure we want to start adding such > > changes for just this. > > In principle

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Thu, Aug 26, 2021 at 03:38:23PM +0200, Daniel Gustafsson wrote: > > On 26 Aug 2021, at 15:09, Bruce Momjian wrote: > > On Thu, Aug 26, 2021 at 03:24:33PM +0800, Julien Rouhaud wrote: > > >> .. I still think that > >> changing bgworker_should_start_now() is a better option. > > > > I am not su

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

> On 26 Aug 2021, at 15:09, Bruce Momjian wrote: > On Thu, Aug 26, 2021 at 03:24:33PM +0800, Julien Rouhaud wrote: >> .. I still think that >> changing bgworker_should_start_now() is a better option. > > I am not sure. We have a lot of pg_upgrade code that turns off things > like autovacuum and

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Thu, Aug 26, 2021 at 03:24:33PM +0800, Julien Rouhaud wrote: > On Thu, Aug 26, 2021 at 3:15 PM Denis Laxalde > wrote: > > > > Michael Paquier a écrit : > > >> @@ -5862,6 +5862,9 @@ do_start_bgworker(RegisteredBgWorker *rw) > > >> static bool > > >> bgworker_should_start_now(BgWorkerStartTi

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Thu, Aug 26, 2021 at 3:15 PM Denis Laxalde wrote: > > Michael Paquier a écrit : > >> @@ -5862,6 +5862,9 @@ do_start_bgworker(RegisteredBgWorker *rw) > >> static bool > >> bgworker_should_start_now(BgWorkerStartTime start_time) > >> { > >> +if (IsBinaryUpgrade) > >> +return

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Michael Paquier a écrit : @@ -5862,6 +5862,9 @@ do_start_bgworker(RegisteredBgWorker *rw) static bool bgworker_should_start_now(BgWorkerStartTime start_time) { + if (IsBinaryUpgrade) + return false; + Using -c max_worker_processes=0 would just have the same effect, no?

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Tue, Aug 24, 2021 at 04:40:02PM +0200, Denis Laxalde wrote: > Julien Rouhaud a écrit : >> I don't think that it would add that much complexity or overhead as there's >> already all the infrastructure to prevent WAL writes in certain condition. >> It >> should be enough to add an additional tes

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Wed, Jan 27, 2021 at 03:06:46PM +0100, Jehan-Guillaume de Rorthais wrote: > Maybe it might worth adding some final safety checks in pg_upgrade itself? > Eg. checking controldata and mxid files coherency between old and new > cluster would have catch the inconsistency here. Yeah, I agree that th

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

> On 24 Aug 2021, at 16:40, Denis Laxalde wrote: > Please find attached another patch implementing this suggestion (as a > complement to the previous patch setting default_transaction_read_only). Please add this to the upcoming commitfest to make sure it's not missed: https://commitfes

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Julien Rouhaud a écrit : On Wed, Jan 27, 2021 at 02:41:32PM +0100, Jehan-Guillaume de Rorthais wrote: On Wed, 27 Jan 2021 11:25:11 +0100 Denis Laxalde wrote: Andres Freund a écrit : I wonder if we could a) set default_transaction_read_only to true, and explicitly change it in the ses

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

On Wed, Jan 27, 2021 at 02:41:32PM +0100, Jehan-Guillaume de Rorthais wrote: > > On Wed, 27 Jan 2021 11:25:11 +0100 > Denis Laxalde wrote: > > > Andres Freund a écrit : > > > > I wonder if we could > > > > > > a) set default_transaction_read_only to true, and explicitly change it > > >in t

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Oh, I forgot another point before sending my previous email. Maybe it might worth adding some final safety checks in pg_upgrade itself? Eg. checking controldata and mxid files coherency between old and new cluster would have catch the inconsistency here.

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Hi, On Wed, 27 Jan 2021 11:25:11 +0100 Denis Laxalde wrote: > Andres Freund a écrit : > > On 2021-01-21 16:23:58 +0100, Denis Laxalde wrote: > > > We found an issue in pg_upgrade on a cluster with a third-party > > > background worker. The upgrade goes fine, but the new cluster is then in > >

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Hi, Andres Freund a écrit : > On 2021-01-21 16:23:58 +0100, Denis Laxalde wrote: > > We found an issue in pg_upgrade on a cluster with a third-party > > background worker. The upgrade goes fine, but the new cluster is then in > > an inconsistent state. The background worker comes from the PoWA > >

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

Hi, On 2021-01-21 16:23:58 +0100, Denis Laxalde wrote: > We found an issue in pg_upgrade on a cluster with a third-party > background worker. The upgrade goes fine, but the new cluster is then in > an inconsistent state. The background worker comes from the PoWA > extension but the issue does not