Date: Fri, 28 Jan 2022 22:18:28 +0800
From: Julien Rouhaud <rjuju123@gmail.com>
To: Denis Laxalde <denis.laxalde@dalibo.com>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@lists.postgresql.org,
	Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Subject: Re: [PATCH] Disable bgworkers during servers start in pg_upgrade*
Message-ID: <20220128141828.sl7alufb2odg26c3@jrouhaud>
References: <20210121152357.s6eflhqyh4g5e6dv@dalibo.com>
 <20210124003611.lkyxhd2mpdmv5xzh@alap3.anarazel.de>
 <20210127102511.tvhu2wmrxoz3tle4@dalibo.com>
 <20210127144132.215fe4bb@firost>
 <20210312082325.weltivlokfpc3gt2@nol>
 <54f0d813-a38a-6b72-269a-7d6dd02bcc5b@dalibo.com>
 <20220128135636.ls2zjbajirbdmrzb@jrouhaud>
 <f135679a-9590-d81c-5d76-f9f4c3825d37@dalibo.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <f135679a-9590-d81c-5d76-f9f4c3825d37@dalibo.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220128141828.sl7alufb2odg26c3%40jrouhaud>
Precedence: bulk

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 only the one executed by
> > pg_upgrade can succeed.
> > 
> > I guess it could be an undocumented option, similar to postgres' -b, which
> > would only be allowed iff --all and --freeze is also passed to be extra safe.
> 
> The help text in pg_dump's man page states:
> 
>        --binary-upgrade
>            This option is for use by in-place upgrade
>            utilities. Its use for other purposes is not
>            recommended or supported. The behavior of
>            the option may change in future releases
>            without notice.
> 
> Is it enough? Or do we actually want to hide it for vacuumdb?

I think it should be hidden, with a comment about it like postmaster.c getopt
call:

			case 'b':
				/* Undocumented flag used for binary upgrades */

> > > vacuumdb: error: processing of database "postgres" failed: PANIC: cannot
> > > make new WAL entries during recovery
> > 
> > Should we tweak that message when IsBinaryUpgrade is true?
> 
> Yes, indeed, I had in mind to simply make the message more generic as:
> "cannot insert new WAL entries".

-1, it's good to have a clear reason why the error happened, especially since
it's supposed to be "should not happen" situation.