Date: Wed, 27 Jan 2021 11:25:11 +0100
From: Denis Laxalde <denis.laxalde@dalibo.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Disable bgworkers during servers start in pg_upgrade
Message-ID: <20210127102511.tvhu2wmrxoz3tle4@dalibo.com>
References: <20210121152357.s6eflhqyh4g5e6dv@dalibo.com>
 <20210124003611.lkyxhd2mpdmv5xzh@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="y3a77itoykn4x2sb"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210124003611.lkyxhd2mpdmv5xzh@alap3.anarazel.de>
User-Agent: NeoMutt/20180716
Precedence: bulk


--y3a77itoykn4x2sb
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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
> > extension but the issue does not appear to related to this particular
> > code.
> 
> Well, it does imply that that backgrounder did something, as the pure
> existence of a bgworker shouldn't affect
> 
> anything. Presumably the issue is that the bgworker actually does
> transactional writes, which causes problems because the xids /
> multixacts from early during pg_upgrade won't actually be valid after we
> do pg_resetxlog etc.
> 
> 
> > As a solution, it seems that, for similar reasons that we restrict
> > socket access to prevent accidental connections (from commit
> > f763b77193), we should also prevent background workers to start at this
> > step.
> 
> I think that'd have quite the potential for negative impact - imagine
> extensions that refuse to be loaded outside of shared_preload_libraries
> (e.g. because they need to allocate shared memory) but that are required
> during the course of pg_upgrade (e.g. because it's a tableam, a PL or
> such). Those libraries will then tried to be loaded during the upgrade
> (due to the _PG_init() hook being called when functions from the
> extension are needed, e.g. the tableam or PL handler).
> 
> Nor is it clear to me that the only way this would be problematic is
> with shared_preload_libraries. A library in local_preload_libraries, or
> a demand loaded library can trigger bgworkers (or database writes in
> some other form) as well.

Thank you for those insights!

> I wonder if we could
> 
> a) set default_transaction_read_only to true, and explicitly change it
>    in the sessions that need that.
> 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.

Solution "a" appears to be enough to solve the problem described in my
initial email. See attached patch implementing this.

Cheers,
Denis

--y3a77itoykn4x2sb
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-Set-default-transactions-to-read-only-at-servers-sta.patch"

From d8b74f9220775736917b7fc08bbe397d7e2eedcd Mon Sep 17 00:00:00 2001
From: Denis Laxalde <denis.laxalde@dalibo.com>
Date: Wed, 20 Jan 2021 17:25:58 +0100
Subject: [PATCH] Set default transactions to read-only at servers start in
 pg_upgrade

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <denis.laxalde@dalibo.com>
Co-authored-by: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
---
 src/bin/pg_upgrade/server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..b17f348a5b 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -244,7 +244,7 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c default_transaction_read_only=on\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--y3a77itoykn4x2sb--