Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 1 Sep 2023 10:08:57 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Andres Freund <andres@anarazel.de>
Cc: Jacob Speidel <jacob@extrahop.com>, Will Mortensen <will@extrahop.com>,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17973: Reinit of pgstats entry for dropped DB can break
 autovacuum daemon
Message-ID: <ZPE5qXLuMs+/vx9H@paquier.xyz>
References: <17973-bca1f7d5c14f601e@postgresql.org>
 <20230614213724.gweo666oxqrugm44@awork3.anarazel.de>
 <20230614215744.ddeuii7jcojbp3yv@awork3.anarazel.de>
 <20230614223012.cfyzxyfnt3ggqkt4@awork3.anarazel.de>
 <CAMpnoC5=njD8tLG9q8XQZKi=1SfLehjVPmLJC+pktzGdfZEgoA@mail.gmail.com>
 <CA+pD2MKw6FwU21efw4+kAziFXWB+cnfxiE1824COmp=0x+jwBQ@mail.gmail.com>
 <ZNbRByjoQlUtTkTb@paquier.xyz>
 <20230812004812.4yev47iili57u4hc@awork3.anarazel.de>
 <ZNlxRmI15e1BtCR6@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="IkUUlbhYVuS5LOQr"
Content-Disposition: inline
In-Reply-To: <ZNlxRmI15e1BtCR6@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/ZPE5qXLuMs%2B/vx9H%40paquier.xyz>
Precedence: bulk


--IkUUlbhYVuS5LOQr
Content-Type: multipart/mixed; boundary="L4jk6RFCkd2k/Hdd"
Content-Disposition: inline


--L4jk6RFCkd2k/Hdd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Aug 14, 2023 at 09:11:50AM +0900, Michael Paquier wrote:
> FWIW, I am still confused about what the long-term plan should be for
> the startup process for these database-level stats, and how
> significant it would be to get access to them (mostly for pg_stat_io,
> right?  Still what does it mean for the startup process?).  I
> understand that we should not undo what e3cb1a58 has tried to improve
> post-feature freeze, still the POC patch of this thread does not touch
> standby.c.  So would it be better to just undo e3cb1a58?  Or rework
> the stat structures and track pgStatBlockWriteTime for each database,
> studying cheaper methods to achieve that?  If we do so, what's there
> to gain in terms of user experience?

After studying more this one, improving the set of static
PgStat_Counters for the startup process so as these can be tracked for
each database is what would make the most sense in the long term.  The
extra overhead when dealing with a lot of databases is something that
we'd need measurements for, but not touching that now won't make
things worse than they are currently.  So I've let that aside,
focusing on the core issue.

> At the end, after looking again at the patch and the thread, I'm OK
> with pgstat_update_dbstats() doing nothing if we don't have
> MyDatabaseId, because there is nothing to do in this case based on the
> current structure of the database-level stats, but the expectations
> surrounding pgstat_update_dbstats() need some clarifications.  Take it
> as a "I'm OK with the logic of the patch, but it can be improved" ;)

I have spent quite a few eye hours on the patch, and I would like to
apply the attached down to v15 at the beginning of next week, likely
Monday, to take care of the autovacuum scheduler issue.

If there are any comments or opinions, please feel free.
--
Michael

--L4jk6RFCkd2k/Hdd
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-Fix-autovacuum-scheduler-with-dropped-databases.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 86acd2bda325b05f78489a038ada51268d5563b4 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Fri, 1 Sep 2023 10:04:40 +0900
Subject: [PATCH v2] Fix autovacuum scheduler with dropped databases

Blah.

Reported-by: Will Mortensen, Jacob Speidel
Analyzed-by: Will Mortensen, Jacob Speidel
Author: Andres Freund
Discussion: https://postgr.es/m/17973-bca1f7d5c14f601e@postgresql.org
---
 src/backend/utils/activity/pgstat_database.c |  13 +++
 src/backend/utils/init/postinit.c            | 111 +++++++++----------
 2 files changed, 67 insertions(+), 57 deletions(-)

diff --git a/src/backend/utils/activity/pgstat_database.c b/src/backend/uti=
ls/activity/pgstat_database.c
index 7149f22f72..d04426f53f 100644
--- a/src/backend/utils/activity/pgstat_database.c
+++ b/src/backend/utils/activity/pgstat_database.c
@@ -271,6 +271,13 @@ pgstat_update_dbstats(TimestampTz ts)
 {
 	PgStat_StatDBEntry *dbentry;
=20
+	/*
+	 * If not connected to a database yet, don't attribute time to "shared
+	 * state" (InvalidOid is used to track stats for shared relations, etc.).
+	 */
+	if (!OidIsValid(MyDatabaseId))
+		return;
+
 	dbentry =3D pgstat_prep_database_pending(MyDatabaseId);
=20
 	/*
@@ -327,6 +334,12 @@ pgstat_prep_database_pending(Oid dboid)
 {
 	PgStat_EntryRef *entry_ref;
=20
+	/*
+	 * This should not report stats on database objects before having
+	 * connected to a database.
+	 */
+	Assert(!OidIsValid(dboid) || OidIsValid(MyDatabaseId));
+
 	entry_ref =3D pgstat_prep_pending_entry(PGSTAT_KIND_DATABASE, dboid, Inva=
lidOid,
 										  NULL);
=20
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/pos=
tinit.c
index f31b85c013..38c8f378d4 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -1006,7 +1006,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
 	 */
 	if (bootstrap)
 	{
-		MyDatabaseId =3D Template1DbOid;
+		dboid =3D Template1DbOid;
 		MyDatabaseTableSpace =3D DEFAULTTABLESPACE_OID;
 	}
 	else if (in_dbname !=3D NULL)
@@ -1020,32 +1020,9 @@ InitPostgres(const char *in_dbname, Oid dboid,
 					(errcode(ERRCODE_UNDEFINED_DATABASE),
 					 errmsg("database \"%s\" does not exist", in_dbname)));
 		dbform =3D (Form_pg_database) GETSTRUCT(tuple);
-		MyDatabaseId =3D dbform->oid;
-		MyDatabaseTableSpace =3D dbform->dattablespace;
-		/* take database name from the caller, just for paranoia */
-		strlcpy(dbname, in_dbname, sizeof(dbname));
+		dboid =3D dbform->oid;
 	}
-	else if (OidIsValid(dboid))
-	{
-		/* caller specified database by OID */
-		HeapTuple	tuple;
-		Form_pg_database dbform;
-
-		tuple =3D GetDatabaseTupleByOid(dboid);
-		if (!HeapTupleIsValid(tuple))
-			ereport(FATAL,
-					(errcode(ERRCODE_UNDEFINED_DATABASE),
-					 errmsg("database %u does not exist", dboid)));
-		dbform =3D (Form_pg_database) GETSTRUCT(tuple);
-		MyDatabaseId =3D dbform->oid;
-		MyDatabaseTableSpace =3D dbform->dattablespace;
-		Assert(MyDatabaseId =3D=3D dboid);
-		strlcpy(dbname, NameStr(dbform->datname), sizeof(dbname));
-		/* pass the database name back to the caller */
-		if (out_dbname)
-			strcpy(out_dbname, dbname);
-	}
-	else
+	else if (!OidIsValid(dboid))
 	{
 		/*
 		 * If this is a background worker not bound to any particular
@@ -1083,8 +1060,57 @@ InitPostgres(const char *in_dbname, Oid dboid,
 	 * CREATE DATABASE.
 	 */
 	if (!bootstrap)
-		LockSharedObject(DatabaseRelationId, MyDatabaseId, 0,
-						 RowExclusiveLock);
+		LockSharedObject(DatabaseRelationId, dboid, 0, RowExclusiveLock);
+
+	/*
+	 * Recheck pg_database to make sure the target database hasn't gone away.
+	 * If there was a concurrent DROP DATABASE, this ensures we will die
+	 * cleanly without creating a mess.
+	 */
+	if (!bootstrap)
+	{
+		HeapTuple	tuple;
+		Form_pg_database datform;
+
+		tuple =3D GetDatabaseTupleByOid(dboid);
+		if (HeapTupleIsValid(tuple))
+			datform =3D (Form_pg_database) GETSTRUCT(tuple);
+
+		if (!HeapTupleIsValid(tuple) ||
+			(in_dbname && namestrcmp(&datform->datname, in_dbname)))
+		{
+			if (in_dbname)
+				ereport(FATAL,
+						(errcode(ERRCODE_UNDEFINED_DATABASE),
+						 errmsg("database \"%s\" does not exist", in_dbname),
+						 errdetail("It seems to have just been dropped or renamed.")));
+			else
+				ereport(FATAL,
+						(errcode(ERRCODE_UNDEFINED_DATABASE),
+						 errmsg("database %u does not exist", dboid)));
+		}
+
+		strlcpy(dbname, NameStr(datform->datname), sizeof(dbname));
+
+		if (database_is_invalid_form(datform))
+		{
+			ereport(FATAL,
+					errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+					errmsg("cannot connect to invalid database \"%s\"", dbname),
+					errhint("Use DROP DATABASE to drop invalid databases."));
+		}
+
+		MyDatabaseTableSpace =3D datform->dattablespace;
+		/* pass the database name back to the caller */
+		if (out_dbname)
+			strcpy(out_dbname, dbname);
+	}
+
+	/*
+	 * Now that we rechecked, we are certain to be connected to a database and
+	 * thus can set MyDatabaseId.
+	 */
+	MyDatabaseId =3D dboid;
=20
 	/*
 	 * Now we can mark our PGPROC entry with the database ID.
@@ -1108,35 +1134,6 @@ InitPostgres(const char *in_dbname, Oid dboid,
 	 */
 	InvalidateCatalogSnapshot();
=20
-	/*
-	 * Recheck pg_database to make sure the target database hasn't gone away.
-	 * If there was a concurrent DROP DATABASE, this ensures we will die
-	 * cleanly without creating a mess.
-	 */
-	if (!bootstrap)
-	{
-		HeapTuple	tuple;
-		Form_pg_database datform;
-
-		tuple =3D GetDatabaseTuple(dbname);
-		if (!HeapTupleIsValid(tuple) ||
-			MyDatabaseId !=3D ((Form_pg_database) GETSTRUCT(tuple))->oid ||
-			MyDatabaseTableSpace !=3D ((Form_pg_database) GETSTRUCT(tuple))->dattab=
lespace)
-			ereport(FATAL,
-					(errcode(ERRCODE_UNDEFINED_DATABASE),
-					 errmsg("database \"%s\" does not exist", dbname),
-					 errdetail("It seems to have just been dropped or renamed.")));
-
-		datform =3D (Form_pg_database) GETSTRUCT(tuple);
-		if (database_is_invalid_form(datform))
-		{
-			ereport(FATAL,
-					errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-					errmsg("cannot connect to invalid database \"%s\"", dbname),
-					errhint("Use DROP DATABASE to drop invalid databases."));
-		}
-	}
-
 	/*
 	 * Now we should be able to access the database directory safely. Verify
 	 * it's there and looks reasonable.
--=20
2.40.1


--L4jk6RFCkd2k/Hdd--

--IkUUlbhYVuS5LOQr
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmTxOakACgkQnvQgOdby
QH11RxAAnbauav9Kf2TAofZZTjFbVfiIvOo1ZN2YA8sTktYT+Thwj8OAenuA9X1E
PeN9iOoQeI9CBoOq9tuwUaULjuKtm9rz+YEwFu7pJb7sev6FAQ+WNxNgE6GrgEQU
CD5tzG+kmZiGs6XEpVkjMjFDMSTQNvBXFedAOU2j0k68xFArmYSyOe2EmNvXupMo
qqOOAtChHryzFL5qYJ61ElQ05hO5TDaLkL7fX68xOBOapk0ff8Q2NUHaRXgxGYoJ
UMeSMq1j1CIj6bKj9FJq0vhNZauHUwyZumM/jwfUoLkHAteCgXM8hpHgG0bht6kU
TMkbqyi9/4SN1A6RxHvNoVqUh14RnoeKkbwNxPy15S8V2aauc5fFnBZQmYcB+lA2
i9LPUGnYm9KDWoMRqS71bAXacl1VOFVZbUTqkqr4BeR8kLr63Z+ICGF+Rlw76O/1
EFFXJP4OVxpgcyt3rZ1zr+vW9j+yrIBkkNqN1oDe8VLsB2wKJyv58bHMwbJ/Vp3F
S667E726pRI9Sepx4kqDpVFt3Z0Gkd2ESdruuyPDSqtrVitc6Nq877Uko0A6Fh0V
A3rNwuokwBLcg4Q3//OGTgGLV5CE8igkYg+TfEWCtBS2MJddJTZ9IL/2QIKkkcGJ
zDs54CfKUjBCFs9ou6dmbV40PN+bPZc1yQbwnI/+bTL+D5PFcw0=
=pVuW
-----END PGP SIGNATURE-----

--IkUUlbhYVuS5LOQr--