From: Michael Paquier <michael@paquier.xyz>
To: Andres Freund <andres@anarazel.de>
Cc: Jacob Speidel <jacob@extrahop.com>
Cc: Will Mortensen <will@extrahop.com>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
Date: Fri, 1 Sep 2023 10:08:57 +0900
Message-ID: <ZPE5qXLuMs+/vx9H@paquier.xyz> (raw)
In-Reply-To: <ZNlxRmI15e1BtCR6@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>
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
Attachments:v2-0001-Fix-autovacuum-scheduler-with-dropped-databases.patch (text/x-diff, 6.1K) download
From 86acd2bda325b05f78489a038ada51268d5563b4 Mon Sep 17 00:00:00 2001
From: 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/utils/activity/pgstat_database.cindex 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;
+ /*+ * 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 = pgstat_prep_database_pending(MyDatabaseId);
/*
@@ -327,6 +334,12 @@ pgstat_prep_database_pending(Oid dboid)
{
PgStat_EntryRef *entry_ref;
+ /*+ * This should not report stats on database objects before having+ * connected to a database.+ */+ Assert(!OidIsValid(dboid) || OidIsValid(MyDatabaseId));+
entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_DATABASE, dboid, InvalidOid,
NULL);
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.cindex 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 = Template1DbOid;+ dboid = Template1DbOid;
MyDatabaseTableSpace = DEFAULTTABLESPACE_OID;
}
else if (in_dbname != 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 = (Form_pg_database) GETSTRUCT(tuple);
- MyDatabaseId = dbform->oid;- MyDatabaseTableSpace = dbform->dattablespace;- /* take database name from the caller, just for paranoia */- strlcpy(dbname, in_dbname, sizeof(dbname));+ dboid = dbform->oid;
}
- else if (OidIsValid(dboid))- {- /* caller specified database by OID */- HeapTuple tuple;- Form_pg_database dbform;-- tuple = GetDatabaseTupleByOid(dboid);- if (!HeapTupleIsValid(tuple))- ereport(FATAL,- (errcode(ERRCODE_UNDEFINED_DATABASE),- errmsg("database %u does not exist", dboid)));- dbform = (Form_pg_database) GETSTRUCT(tuple);- MyDatabaseId = dbform->oid;- MyDatabaseTableSpace = dbform->dattablespace;- Assert(MyDatabaseId == 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 = GetDatabaseTupleByOid(dboid);+ if (HeapTupleIsValid(tuple))+ datform = (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 = 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 = dboid;
/*
* Now we can mark our PGPROC entry with the database ID.
@@ -1108,35 +1134,6 @@ InitPostgres(const char *in_dbname, Oid dboid,
*/
InvalidateCatalogSnapshot();
- /*- * 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 = GetDatabaseTuple(dbname);- if (!HeapTupleIsValid(tuple) ||- MyDatabaseId != ((Form_pg_database) GETSTRUCT(tuple))->oid ||- MyDatabaseTableSpace != ((Form_pg_database) GETSTRUCT(tuple))->dattablespace)- ereport(FATAL,- (errcode(ERRCODE_UNDEFINED_DATABASE),- errmsg("database \"%s\" does not exist", dbname),- errdetail("It seems to have just been dropped or renamed.")));-- datform = (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.
--
2.40.1
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, andres@anarazel.de, jacob@extrahop.com, will@extrahop.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
In-Reply-To: <ZPE5qXLuMs+/vx9H@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox