pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
24+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2023-09-03 2b8e5273e9 c397c98ae6 5a9325fdde Fix handling of shared statistics with dropped databases
* BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-06-13 23:49 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 24+ messages in thread
From: PG Bug reporting form @ 2023-06-13 23:49 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: will@extrahop.com
The following bug has been logged on the website:
Bug reference: 17973
Logged by: Will Mortensen
Email address: will@extrahop.com
PostgreSQL version: 15.3
Operating system: Ubuntu 22.04
Description:
My colleague Jacob Speidel (jacob@extrahop.com) and I have diagnosed a race
condition that, under certain conditions, can cause the autovacuum daemon to
stop launching autovacuum workers until the autovacuum daemon (or the whole
server) is restarted. This obviously causes serious problems for the
server.
We've reproduced this on both 15.2 and REL_15_STABLE (git commit
bd590d1fea1ba9245c791d589eea94d2dbad5a2b). We've never seen a similar issue
on 14 despite very similar conditions occurring frequently. We haven't yet
tried on 16.
In Jacob's repro, the problem begins when AutoVacWorkerMain() is in
InitPostgres() and some other backend drops the database. Specifically, the
autovacuum worker's InitPostgres() is just about to obtain the lock at
https://github.com/postgres/postgres/blob/bd590d1fea1ba9245c791d589eea94d2dbad5a2b/src/backend/utils...
. The backend dropping the DB marks the DB's pgstats entry as dropped but
can’t free it because its refcount is nonzero, so
AtEOXact_PgStat_DroppedStats() calls pgstat_request_entry_refs_gc(). The
autovacuum worker's InitPostgres() proceeds to call GetDatabaseTuple() and
notices the database has been dropped, at some point calling
pgstat_gc_entry_refs() to release its reference to the DB's pgstats entry,
and the worker decides to exit with a fatal error. But while exiting, it
calls pgstat_report_stat(), which calls pgstat_update_dbstats() for the
dropped DB, which calls pgstat_prep_database_pending(), which calls
pgstat_prep_pending_entry(), which calls pgstat_get_entry_ref() with create
== true, which calls pgstat_reinit_entry() against the DB's pgstats entry.
This sets dropped to false on that entry. Finally, the autovacuum worker
exits.
The fact that a dropped database now indefinitely has a pgstats entry with
dropped == false seemingly violates some assumptions and confuses the
autovacuum daemon. In particular, rebuild_database_list() will forever
include it in DatabaseList and take it into account when computing the
adl_next_worker for each DB, but do_start_worker() won’t consider the DB
because it's never returned by get_database_list(). In our repros, this
mismatch causes do_start_worker() to get stuck never processing any DB: in
particular, it always sees that for each non-dropped database
adl_next_worker is in the future but within the next autovacuum_naptime,
i.e. skipit = true. This causes do_start_worker() to call
rebuild_database_list() at the end, which again miscomputes adl_next_worker
and pushes it further into the future, so that the situation repeats on the
next call to do_start_worker(), and so on indefinitely.
That’s the crux of our issue. Please let us know if any clarification or
more detailed repro steps are needed. Our repro patch just sleeps before and
after the LockSharedObject() call in InitPostgres() (to widen the race
windows) and adds a lot of logging. (Jacob did >90% of the debugging here; I
merely determined how the pgstats entry lost its dropped flag.)
We assume that one fix would be to somehow ensure that the dropped flag
remains true on a dropped database’s pgstats entry until it’s freed, but
also, it seems a bit fragile for autovacuum’s do_start_worker() to sometimes
call rebuild_database_list() and delay all the adl_next_worker times.
Without thinking about it too hard, we wonder if there would still be a
pattern of ongoing DB creates and drops that could cause it to misbehave in
a similar way, never deciding to autovacuum any database even if one lives
long enough that it should be autovacuumed.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-06-14 21:37 Andres Freund <andres@anarazel.de>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 24+ messages in thread
From: Andres Freund @ 2023-06-14 21:37 UTC (permalink / raw)
To: will@extrahop.com; pgsql-bugs@lists.postgresql.org
Hi,
Good catch!
On 2023-06-13 23:49:27 +0000, PG Bug reporting form wrote:
> We've reproduced this on both 15.2 and REL_15_STABLE (git commit
> bd590d1fea1ba9245c791d589eea94d2dbad5a2b). We've never seen a similar issue
> on 14 despite very similar conditions occurring frequently. We haven't yet
> tried on 16.
The relevant code is new for 15, so that makes sense.
> In Jacob's repro, the problem begins when AutoVacWorkerMain() is in
> InitPostgres() and some other backend drops the database. Specifically, the
> autovacuum worker's InitPostgres() is just about to obtain the lock at
> https://github.com/postgres/postgres/blob/bd590d1fea1ba9245c791d589eea94d2dbad5a2b/src/backend/utils...
> . The backend dropping the DB marks the DB's pgstats entry as dropped but
> can’t free it because its refcount is nonzero, so
> AtEOXact_PgStat_DroppedStats() calls pgstat_request_entry_refs_gc(). The
> autovacuum worker's InitPostgres() proceeds to call GetDatabaseTuple() and
> notices the database has been dropped, at some point calling
> pgstat_gc_entry_refs() to release its reference to the DB's pgstats entry,
> and the worker decides to exit with a fatal error. But while exiting, it
> calls pgstat_report_stat(), which calls pgstat_update_dbstats() for the
> dropped DB, which calls pgstat_prep_database_pending(), which calls
> pgstat_prep_pending_entry(), which calls pgstat_get_entry_ref() with create
> == true, which calls pgstat_reinit_entry() against the DB's pgstats entry.
> This sets dropped to false on that entry. Finally, the autovacuum worker
> exits.
Hm. It's pretty nasty that we end up in the proc_exit() hooks with
MyDatabaseId set, even though we aren't actually validly connected to that
database.
ISTM that at the very least we should make "Recheck pg_database to make sure
the target database hasn't gone away." in InitPostgres() unset
MyDatabaseId, MyProc->databaseId.
We unfortunately can't just defer setting up MyProc->databaseId, I
think. Otherwise a concurrent vacuum could decide to remove database contents
we think we still can see.
If we had something like that it should be fairly easy to skip relevant parts
of pgstat_report_stat(), based on MyDatabaseId being invalid.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-06-14 21:57 Andres Freund <andres@anarazel.de>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 24+ messages in thread
From: Andres Freund @ 2023-06-14 21:57 UTC (permalink / raw)
To: will@extrahop.com; pgsql-bugs@lists.postgresql.org
Hi,
On 2023-06-14 14:37:24 -0700, Andres Freund wrote:
> On 2023-06-13 23:49:27 +0000, PG Bug reporting form wrote:
> > In Jacob's repro, the problem begins when AutoVacWorkerMain() is in
> > InitPostgres() and some other backend drops the database. Specifically, the
> > autovacuum worker's InitPostgres() is just about to obtain the lock at
> > https://github.com/postgres/postgres/blob/bd590d1fea1ba9245c791d589eea94d2dbad5a2b/src/backend/utils...
> > . The backend dropping the DB marks the DB's pgstats entry as dropped but
> > can’t free it because its refcount is nonzero, so
> > AtEOXact_PgStat_DroppedStats() calls pgstat_request_entry_refs_gc(). The
> > autovacuum worker's InitPostgres() proceeds to call GetDatabaseTuple() and
> > notices the database has been dropped, at some point calling
> > pgstat_gc_entry_refs() to release its reference to the DB's pgstats entry,
> > and the worker decides to exit with a fatal error. But while exiting, it
> > calls pgstat_report_stat(), which calls pgstat_update_dbstats() for the
> > dropped DB, which calls pgstat_prep_database_pending(), which calls
> > pgstat_prep_pending_entry(), which calls pgstat_get_entry_ref() with create
> > == true, which calls pgstat_reinit_entry() against the DB's pgstats entry.
> > This sets dropped to false on that entry. Finally, the autovacuum worker
> > exits.
>
> Hm. It's pretty nasty that we end up in the proc_exit() hooks with
> MyDatabaseId set, even though we aren't actually validly connected to that
> database.
>
> ISTM that at the very least we should make "Recheck pg_database to make sure
> the target database hasn't gone away." in InitPostgres() unset
> MyDatabaseId, MyProc->databaseId.
>
> We unfortunately can't just defer setting up MyProc->databaseId, I
> think. Otherwise a concurrent vacuum could decide to remove database contents
> we think we still can see.
Hm. On second thought, I don't see any reason to do
/*
* Now we can mark our PGPROC entry with the database ID.
*
* We assume this is an atomic store so no lock is needed; though actually
* things would work fine even if it weren't atomic. Anyone searching the
* ProcArray for this database's ID should hold the database lock, so they
* would not be executing concurrently with this store. A process looking
* for another database's ID could in theory see a chance match if it read
* a partially-updated databaseId value; but as long as all such searches
* wait and retry, as in CountOtherDBBackends(), they will certainly see
* the correct value on their next try.
*/
MyProc->databaseId = MyDatabaseId;
/*
* We established a catalog snapshot while reading pg_authid and/or
* pg_database; but until we have set up MyDatabaseId, we won't react to
* incoming sinval messages for unshared catalogs, so we won't realize it
* if the snapshot has been invalidated. Assume it's no good anymore.
*/
InvalidateCatalogSnapshot();
before
/*
* 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)
{
as pg_database is a shared catalog. So we can safely access it at that
point. Or am I missing something?
If I am not, then we we should defer setting MyDatabaseId until after "Recheck
pg_database", e.g. by setting 'dboid' for the in_dbname path.
I think that would likely end up with *less* code than before, because we'd
need less redundant code.
It's a bit odd that we do the second lookup with dbname, rather the oid we
previously looked up.
Afaics there's no reason to look up MyDatabaseTableSpace before the recheck.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-06-14 22:30 Andres Freund <andres@anarazel.de>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 24+ messages in thread
From: Andres Freund @ 2023-06-14 22:30 UTC (permalink / raw)
To: will@extrahop.com; pgsql-bugs@lists.postgresql.org
Hi,
On 2023-06-14 14:57:44 -0700, Andres Freund wrote:
> If I am not, then we we should defer setting MyDatabaseId until after "Recheck
> pg_database", e.g. by setting 'dboid' for the in_dbname path.
>
> I think that would likely end up with *less* code than before, because we'd
> need less redundant code.
>
>
> It's a bit odd that we do the second lookup with dbname, rather the oid we
> previously looked up.
>
>
> Afaics there's no reason to look up MyDatabaseTableSpace before the recheck.
Attached is a rough prototype implementing this idea. Could you check if that
fixes the issue?
Greetings,
Andres Freund
Attachments:
pgstat-mydatabase-fix-prototype.diff (text/x-diff, 4.9K) download
diff --git i/src/backend/utils/activity/pgstat_database.c w/src/backend/utils/activity/pgstat_database.c
index 7149f22f729..3efcc9bb7c3 100644
--- i/src/backend/utils/activity/pgstat_database.c
+++ w/src/backend/utils/activity/pgstat_database.c
@@ -271,6 +271,14 @@ pgstat_update_dbstats(TimestampTz ts)
{
PgStat_StatDBEntry *dbentry;
+ /*
+ * If we haven't 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 +335,12 @@ pgstat_prep_database_pending(Oid dboid)
{
PgStat_EntryRef *entry_ref;
+ /*
+ * Shouldn't report stats on database objects before having connected to
+ * database.
+ */
+ Assert(!OidIsValid(dboid) || OidIsValid(MyDatabaseId));
+
entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_DATABASE, dboid, InvalidOid,
NULL);
diff --git i/src/backend/utils/init/postinit.c w/src/backend/utils/init/postinit.c
index 561bd13ed24..a40f70503a4 100644
--- i/src/backend/utils/init/postinit.c
+++ w/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,47 @@ 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 dbform;
+
+ tuple = GetDatabaseTupleByOid(dboid);
+ if (HeapTupleIsValid(tuple))
+ dbform = (Form_pg_database) GETSTRUCT(tuple);
+
+ if (!HeapTupleIsValid(tuple) ||
+ (in_dbname && namestrcmp(&dbform->datname, in_dbname)))
+ {
+ if (in_dbname)
+ ereport(PANIC,
+ (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)));
+ }
+
+ MyDatabaseTableSpace = dbform->dattablespace;
+ strlcpy(dbname, NameStr(dbform->datname), sizeof(dbname));
+ if (out_dbname)
+ strcpy(out_dbname, dbname);
+ }
+
+ /*
+ * Now that we rechecked, we can be certain we're connected to a database
+ * and thus can set MyDatabaseId.
+ */
+ MyDatabaseId = dboid;
/*
* Now we can mark our PGPROC entry with the database ID.
@@ -1108,25 +1124,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;
-
- 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.")));
- }
-
/*
* Now we should be able to access the database directory safely. Verify
* it's there and looks reasonable.
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-06-15 20:35 Will Mortensen <will@extrahop.com>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 24+ messages in thread
From: Will Mortensen @ 2023-06-15 20:35 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-bugs@lists.postgresql.org, Jacob Speidel <jacob@extrahop.com>
Thanks for the quick patch Andres! Jacob tested and that appears to
fix our scenario.
On Wed, Jun 14, 2023 at 3:30 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2023-06-14 14:57:44 -0700, Andres Freund wrote:
> > If I am not, then we we should defer setting MyDatabaseId until after "Recheck
> > pg_database", e.g. by setting 'dboid' for the in_dbname path.
> >
> > I think that would likely end up with *less* code than before, because we'd
> > need less redundant code.
> >
> >
> > It's a bit odd that we do the second lookup with dbname, rather the oid we
> > previously looked up.
> >
> >
> > Afaics there's no reason to look up MyDatabaseTableSpace before the recheck.
>
> Attached is a rough prototype implementing this idea. Could you check if that
> fixes the issue?
>
> Greetings,
>
> Andres Freund
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-06-16 01:01 Michael Paquier <michael@paquier.xyz>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 2 replies; 24+ messages in thread
From: Michael Paquier @ 2023-06-16 01:01 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: will@extrahop.com; pgsql-bugs@lists.postgresql.org
On Wed, Jun 14, 2023 at 03:30:12PM -0700, Andres Freund wrote:
> Attached is a rough prototype implementing this idea. Could you check if that
> fixes the issue?
It requires a few manual steps, but I have been able to stuck the
autovacuum launcher schedule. Nice investigation from the reporters.
I may be missing something here, but finishing with an inconsistent
database list (generated based on the pgstat database entries) in the
autovacuum launcher is not something that can happen only because of a
worker, right? A normal backend would call pgstat_update_dbstats()
once it exists, re-creating a fresh entry with the dropped database
OID. Is that right?
+ /*
+ * If we haven't 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;
Hmm. pgstat_report_stat() is called by standby_redo() for a
XLOG_RUNNING_XACTS record so this would prevent the startup process
from doing any stats updates for the shared db state, no?
Looking at the most recent things in this area, like bc49d93 or
ac23b71, I don't immediately see why your new ordering suggestion
would not be OK with MyDatabaseId getting set before acquiring the
shared lock of the database, so as the stat entries don't get messed
up. The result feels cleaner in the initialization sequence,
additionally. So, nice.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-06-16 01:36 Will Mortensen <will@extrahop.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 24+ messages in thread
From: Will Mortensen @ 2023-06-16 01:36 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Andres Freund <andres@anarazel.de>; pgsql-bugs@lists.postgresql.org, Jacob Speidel <jacob@extrahop.com>
On Thu, Jun 15, 2023 at 6:01 PM Michael Paquier <michael@paquier.xyz> wrote:
> It requires a few manual steps, but I have been able to stuck the
> autovacuum launcher schedule. Nice investigation from the reporters.
>
> I may be missing something here, but finishing with an inconsistent
> database list (generated based on the pgstat database entries) in the
> autovacuum launcher is not something that can happen only because of a
> worker, right? A normal backend would call pgstat_update_dbstats()
> once it exists, re-creating a fresh entry with the dropped database
> OID. Is that right?
Yes, sorry, Jacob was able to repro with a normal backend just now. We
probably should have tried that earlier. :-)
I'm also unsure if reiniting the pgstats entry (as opposed to creating
a new one) is actually necessary or just what we happened to observe.
We're definitely not very familiar with these internals. :-)
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-06-16 01:58 Michael Paquier <michael@paquier.xyz>
parent: Will Mortensen <will@extrahop.com>
0 siblings, 0 replies; 24+ messages in thread
From: Michael Paquier @ 2023-06-16 01:58 UTC (permalink / raw)
To: Will Mortensen <will@extrahop.com>; +Cc: Andres Freund <andres@anarazel.de>; pgsql-bugs@lists.postgresql.org, Jacob Speidel <jacob@extrahop.com>
On Thu, Jun 15, 2023 at 06:36:41PM -0700, Will Mortensen wrote:
> On Thu, Jun 15, 2023 at 6:01 PM Michael Paquier <michael@paquier.xyz> wrote:
>> It requires a few manual steps, but I have been able to stuck the
>> autovacuum launcher schedule. Nice investigation from the reporters.
>>
>> I may be missing something here, but finishing with an inconsistent
>> database list (generated based on the pgstat database entries) in the
>> autovacuum launcher is not something that can happen only because of a
>> worker, right? A normal backend would call pgstat_update_dbstats()
>> once it exists, re-creating a fresh entry with the dropped database
>> OID. Is that right?
>
> Yes, sorry, Jacob was able to repro with a normal backend just now. We
> probably should have tried that earlier. :-)
Okay, thanks for confirming. Yes, I was able to stuck the scheduler
for both. FWIW, I have initially hardcoded a stop point with an
on-disk file around InitPostgres() before we take the shared lock.
But thinking harder, it is possible to be sneaky with the following
steps to get these incorrect stat entries:
1) LOCK pg_database in a first session, on a database different than
the one to drop.
2) Connect with a second session to the database to drop, stuck
because of the previous LOCK when scanning pg_database to find the
tuple OID, during connection startup, just before taking the shared
lock.
3) Third session executing DROP DATABASE, with a session that
connected before the pg_database lock.
4) Commit transaction of first session to release pg_database lock,
the database is dropped and the second session fails to connect with
the database renamed.
> I'm also unsure if reiniting the pgstats entry (as opposed to creating
> a new one) is actually necessary or just what we happened to observe.
> We're definitely not very familiar with these internals. :-)
The proposed patch does better than that, actually, and takes the
approach of not touching the stats for the database dropped/renamed
by delaying MyDatabaseId which is what the backend uses to update the
database-level pgstat entries.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-06-19 17:45 Andres Freund <andres@anarazel.de>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 24+ messages in thread
From: Andres Freund @ 2023-06-19 17:45 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: will@extrahop.com; pgsql-bugs@lists.postgresql.org
Hi,
On 2023-06-16 10:01:33 +0900, Michael Paquier wrote:
> + /*
> + * If we haven't 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;
>
> Hmm. pgstat_report_stat() is called by standby_redo() for a
> XLOG_RUNNING_XACTS record so this would prevent the startup process
> from doing any stats updates for the shared db state, no?
Hm. Effectively the startup process isn't doing that in a useful way
today. Commit/Rollback and session stats don't make sense, and block
read/write time are wrongly attributed.
I think it'd take a fair amount of work to track these stats in a more useful
manner for the startup process, by virtue of it effectively being connected to
multiple databases. We'd need to track
pgStatBlockReadTime/pgStatBlockWriteTime on a per-database level, which
wouldn't be easy to do without increasing overhead.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-06-20 06:41 Michael Paquier <michael@paquier.xyz>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 24+ messages in thread
From: Michael Paquier @ 2023-06-20 06:41 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: will@extrahop.com; pgsql-bugs@lists.postgresql.org
On Mon, Jun 19, 2023 at 10:45:23AM -0700, Andres Freund wrote:
> I think it'd take a fair amount of work to track these stats in a more useful
> manner for the startup process, by virtue of it effectively being connected to
> multiple databases. We'd need to track
> pgStatBlockReadTime/pgStatBlockWriteTime on a per-database level, which
> wouldn't be easy to do without increasing overhead.
Is it really necessary to do this much amount of work for the scope of
this issue, though? Relying on MyDatabaseId to control if these
updates should happen does not look like the right move to me, to be
honest, because this can be used to update shared stats. In the
pgstat shutdown callback, shouldn't we try to check if the database
entry exists and/or has been dropped and just do nothing in this case?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-06-21 03:59 Andres Freund <andres@anarazel.de>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 24+ messages in thread
From: Andres Freund @ 2023-06-21 03:59 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: will@extrahop.com; pgsql-bugs@lists.postgresql.org
Hi,
On 2023-06-20 15:41:22 +0900, Michael Paquier wrote:
> On Mon, Jun 19, 2023 at 10:45:23AM -0700, Andres Freund wrote:
> > I think it'd take a fair amount of work to track these stats in a more useful
> > manner for the startup process, by virtue of it effectively being connected to
> > multiple databases. We'd need to track
> > pgStatBlockReadTime/pgStatBlockWriteTime on a per-database level, which
> > wouldn't be easy to do without increasing overhead.
>
> Is it really necessary to do this much amount of work for the scope of
> this issue, though?
No! That'd just be for actually making db-level stats for the startup process
work. In other words, I don't think there's anything we could break, because
it already doesn't work :)
> Relying on MyDatabaseId to control if these updates should happen does not
> look like the right move to me, to be honest, because this can be used to
> update shared stats.
Shared stats only make sense when you know what non-shared stats are. At the
moment that afaict only works (and afaict only ever has worked) for database
connected backends.
I guess we could start reporting everything to the shared entry if we aren't
connected to a database, but that doesn't really seem like an improvement.
> In the pgstat shutdown callback, shouldn't we try to check if the database
> entry exists and/or has been dropped and just do nothing in this case?
I don't think that'd be a good approach.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-08-11 21:15 Jacob Speidel <jacob@extrahop.com>
parent: Will Mortensen <will@extrahop.com>
0 siblings, 1 reply; 24+ messages in thread
From: Jacob Speidel @ 2023-08-11 21:15 UTC (permalink / raw)
To: Will Mortensen <will@extrahop.com>; +Cc: Andres Freund <andres@anarazel.de>; pgsql-bugs@lists.postgresql.org
On Thu, Jun 15, 2023 at 1:35 PM Will Mortensen <will@extrahop.com> wrote:
> Thanks for the quick patch Andres! Jacob tested and that appears to
> fix our scenario.
>
>
> On Wed, Jun 14, 2023 at 3:30 PM Andres Freund <andres@anarazel.de> wrote:
> >
> > Hi,
> >
> > On 2023-06-14 14:57:44 -0700, Andres Freund wrote:
> > > If I am not, then we we should defer setting MyDatabaseId until after
> "Recheck
> > > pg_database", e.g. by setting 'dboid' for the in_dbname path.
> > >
> > > I think that would likely end up with *less* code than before, because
> we'd
> > > need less redundant code.
> > >
> > >
> > > It's a bit odd that we do the second lookup with dbname, rather the
> oid we
> > > previously looked up.
> > >
> > >
> > > Afaics there's no reason to look up MyDatabaseTableSpace before the
> recheck.
> >
> > Attached is a rough prototype implementing this idea. Could you check
> if that
> > fixes the issue?
> >
> > Greetings,
> >
> > Andres Freund
>
Hello,
I don't think this is a surprise to anyone but this is just an update that
the bug still exists in 15.4 and the patch from Andres still fixes the
problem.
Thanks,
Jacob
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-08-12 00:23 Michael Paquier <michael@paquier.xyz>
parent: Jacob Speidel <jacob@extrahop.com>
0 siblings, 1 reply; 24+ messages in thread
From: Michael Paquier @ 2023-08-12 00:23 UTC (permalink / raw)
To: Jacob Speidel <jacob@extrahop.com>; +Cc: Will Mortensen <will@extrahop.com>; Andres Freund <andres@anarazel.de>; pgsql-bugs@lists.postgresql.org
On Fri, Aug 11, 2023 at 02:15:34PM -0700, Jacob Speidel wrote:
> I don't think this is a surprise to anyone but this is just an update that
> the bug still exists in 15.4 and the patch from Andres still fixes the
> problem.
Yes, this one has been sitting in my inbox for a few weeks now. The
part I am not quite sure of is what we should do for the stats
gathered by startup process, but that's separate from this autovacuum
scheduling problem at the end.
Andres, were you planning to do something here or did you need some
help?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-08-12 00:48 Andres Freund <andres@anarazel.de>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 24+ messages in thread
From: Andres Freund @ 2023-08-12 00:48 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Jacob Speidel <jacob@extrahop.com>; Will Mortensen <will@extrahop.com>; pgsql-bugs@lists.postgresql.org
Hi,
On 2023-08-12 09:23:35 +0900, Michael Paquier wrote:
> On Fri, Aug 11, 2023 at 02:15:34PM -0700, Jacob Speidel wrote:
> > I don't think this is a surprise to anyone but this is just an update that
> > the bug still exists in 15.4 and the patch from Andres still fixes the
> > problem.
>
> Yes, this one has been sitting in my inbox for a few weeks now. The
> part I am not quite sure of is what we should do for the stats
> gathered by startup process, but that's separate from this autovacuum
> scheduling problem at the end.
>
> Andres, were you planning to do something here or did you need some
> help?
I was basically still wondering whether I defused your startup process
concerns or not.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-08-14 00:11 Michael Paquier <michael@paquier.xyz>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 24+ messages in thread
From: Michael Paquier @ 2023-08-14 00:11 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Jacob Speidel <jacob@extrahop.com>; Will Mortensen <will@extrahop.com>; pgsql-bugs@lists.postgresql.org
On Fri, Aug 11, 2023 at 05:48:12PM -0700, Andres Freund wrote:
> I was basically still wondering whether I defused your startup process
> concerns or not.
My apologies for dropping the ball for so long here.
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?
+ /*
+ * If we haven't connected to a database yet, don't attribute time to
+ * "shared state" (InvalidOid is used to track stats for shared relations
+ * etc).
+ */
At least this commit is incorrect because pgstat_update_dbstats() can
be called via pgstat_report_stat() with an invalid MyDatabaseId on
purpose? And I assume that standby.c should perhaps document a bit
better that this does not affect database-level stats.
Perhaps the top-comment of pgstat_report_stat() should document the
startup process part, but I am not completely sure how helpful such an
addition would be on its own.
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" ;)
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-09-01 01:08 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 24+ messages in thread
From: Michael Paquier @ 2023-09-01 01:08 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Jacob Speidel <jacob@extrahop.com>; Will Mortensen <will@extrahop.com>; pgsql-bugs@lists.postgresql.org
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.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;
+ /*
+ * 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.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 = 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
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-09-01 02:56 Nathan Bossart <nathandbossart@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 24+ messages in thread
From: Nathan Bossart @ 2023-09-01 02:56 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Andres Freund <andres@anarazel.de>; Jacob Speidel <jacob@extrahop.com>; Will Mortensen <will@extrahop.com>; pgsql-bugs@lists.postgresql.org
On Fri, Sep 01, 2023 at 10:08:57AM +0900, Michael Paquier wrote:
> On Mon, Aug 14, 2023 at 09:11:50AM +0900, Michael Paquier wrote:
>> 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.
I've spent much less time staring at the patch than you, but it looks
logically sound to me. IMO the changes to InitPostgres() are arguably
worthwhile on their own.
> + /*
> + * Now that we rechecked, we are certain to be connected to a database and
> + * thus can set MyDatabaseId.
> + */
> + MyDatabaseId = dboid;
Perhaps we should expand this comment to make it clear that some code
depends on this to avoid races with concurrent database drops/renames.
One other thing that comes to mind is whether this patch creates the
opposite problem for some other code, i.e., it depends on MyDatabaseId
being set before the lock is taken. But that seems pretty unlikely, and
I'm not aware of anything like that off the top of my head.
Also, IIUC we could simply move the recheck to just after the lock
aquisition and then check MyProc->databaseId in pgstat_update_dbstats(),
but as I mentioned above, the changes to InitPostgres() are kind of nice
independently. Plus, it's nice to be able to depend on MyDatabaseId in the
same way.
In short, LGTM.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-09-01 03:31 Michael Paquier <michael@paquier.xyz>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 24+ messages in thread
From: Michael Paquier @ 2023-09-01 03:31 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; Jacob Speidel <jacob@extrahop.com>; Will Mortensen <will@extrahop.com>; pgsql-bugs@lists.postgresql.org
On Thu, Aug 31, 2023 at 07:56:13PM -0700, Nathan Bossart wrote:
> On Fri, Sep 01, 2023 at 10:08:57AM +0900, Michael Paquier wrote:
>> + /*
>> + * Now that we rechecked, we are certain to be connected to a database and
>> + * thus can set MyDatabaseId.
>> + */
>> + MyDatabaseId = dboid;
>
> Perhaps we should expand this comment to make it clear that some code
> depends on this to avoid races with concurrent database drops/renames.
Yeah, this could be a bit more detailed, mentioning the dependency
with the pgstat shutdown callback. How about appending the following
sentence in a second paragraph, say:
"Setting MyDatabaseId after rechecking the existence of the database
matters for instance with the shutdown callback of the shared
statistics, as pgstat_report_stat() will update database statistics
depending on MyDatabaseId."
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-09-01 15:20 Nathan Bossart <nathandbossart@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 24+ messages in thread
From: Nathan Bossart @ 2023-09-01 15:20 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Andres Freund <andres@anarazel.de>; Jacob Speidel <jacob@extrahop.com>; Will Mortensen <will@extrahop.com>; pgsql-bugs@lists.postgresql.org
On Fri, Sep 01, 2023 at 12:31:09PM +0900, Michael Paquier wrote:
> On Thu, Aug 31, 2023 at 07:56:13PM -0700, Nathan Bossart wrote:
>> On Fri, Sep 01, 2023 at 10:08:57AM +0900, Michael Paquier wrote:
>>> + /*
>>> + * Now that we rechecked, we are certain to be connected to a database and
>>> + * thus can set MyDatabaseId.
>>> + */
>>> + MyDatabaseId = dboid;
>>
>> Perhaps we should expand this comment to make it clear that some code
>> depends on this to avoid races with concurrent database drops/renames.
>
> Yeah, this could be a bit more detailed, mentioning the dependency
> with the pgstat shutdown callback. How about appending the following
> sentence in a second paragraph, say:
> "Setting MyDatabaseId after rechecking the existence of the database
> matters for instance with the shutdown callback of the shared
> statistics, as pgstat_report_stat() will update database statistics
> depending on MyDatabaseId."
Something along those lines seems fine to me. FWIW this is what I had in
my head:
It is important that MyDatabaseId only be set once we are sure that the
target database can no longer be concurrently dropped or renamed. Some
code depends on this. For example, without this guarantee,
pgstat_update_dbstats() could create entries for databases that were
just dropped, which will lead to problems down the road.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-09-02 00:08 Michael Paquier <michael@paquier.xyz>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 24+ messages in thread
From: Michael Paquier @ 2023-09-02 00:08 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; Jacob Speidel <jacob@extrahop.com>; Will Mortensen <will@extrahop.com>; pgsql-bugs@lists.postgresql.org
On Fri, Sep 01, 2023 at 08:20:24AM -0700, Nathan Bossart wrote:
> Something along those lines seems fine to me. FWIW this is what I had in
> my head:
>
> It is important that MyDatabaseId only be set once we are sure that the
> target database can no longer be concurrently dropped or renamed. Some
> code depends on this. For example, without this guarantee,
> pgstat_update_dbstats() could create entries for databases that were
> just dropped, which will lead to problems down the road.
I'm mostly OK with this formulation as well, but without the second
sentence and adding as extra that pgstat_update_dbstats() could be
specifically called in the shutdown callback of the shared statistics.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-09-02 15:21 Nathan Bossart <nathandbossart@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 24+ messages in thread
From: Nathan Bossart @ 2023-09-02 15:21 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Andres Freund <andres@anarazel.de>; Jacob Speidel <jacob@extrahop.com>; Will Mortensen <will@extrahop.com>; pgsql-bugs@lists.postgresql.org
On Sat, Sep 02, 2023 at 09:08:22AM +0900, Michael Paquier wrote:
> On Fri, Sep 01, 2023 at 08:20:24AM -0700, Nathan Bossart wrote:
>> Something along those lines seems fine to me. FWIW this is what I had in
>> my head:
>>
>> It is important that MyDatabaseId only be set once we are sure that the
>> target database can no longer be concurrently dropped or renamed. Some
>> code depends on this. For example, without this guarantee,
>> pgstat_update_dbstats() could create entries for databases that were
>> just dropped, which will lead to problems down the road.
>
> I'm mostly OK with this formulation as well, but without the second
> sentence and adding as extra that pgstat_update_dbstats() could be
> specifically called in the shutdown callback of the shared statistics.
WFM
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-09-03 23:09 Michael Paquier <michael@paquier.xyz>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 2 replies; 24+ messages in thread
From: Michael Paquier @ 2023-09-03 23:09 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; Jacob Speidel <jacob@extrahop.com>; Will Mortensen <will@extrahop.com>; pgsql-bugs@lists.postgresql.org
On Sat, Sep 02, 2023 at 08:21:34AM -0700, Nathan Bossart wrote:
> WFM
Okay, applied with something slightly-reworded to 15~.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-09-04 01:55 Will Mortensen <will@extrahop.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 24+ messages in thread
From: Will Mortensen @ 2023-09-04 01:55 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; Andres Freund <andres@anarazel.de>; Jacob Speidel <jacob@extrahop.com>; pgsql-bugs@lists.postgresql.org
Thanks all for the fix!
On Sun, Sep 3, 2023 at 4:09 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Sat, Sep 02, 2023 at 08:21:34AM -0700, Nathan Bossart wrote:
> > WFM
>
> Okay, applied with something slightly-reworded to 15~.
> --
> Michael
>
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon
@ 2023-09-12 02:11 Andres Freund <andres@anarazel.de>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 24+ messages in thread
From: Andres Freund @ 2023-09-12 02:11 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; Jacob Speidel <jacob@extrahop.com>; Will Mortensen <will@extrahop.com>; pgsql-bugs@lists.postgresql.org
On 2023-09-04 08:09:41 +0900, Michael Paquier wrote:
> On Sat, Sep 02, 2023 at 08:21:34AM -0700, Nathan Bossart wrote:
> > WFM
>
> Okay, applied with something slightly-reworded to 15~.
Thanks!
^ permalink raw reply [nested|flat] 24+ messages in thread
end of thread, other threads:[~2023-09-12 02:11 UTC | newest]
Thread overview: 24+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-06-13 23:49 BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon PG Bug reporting form <noreply@postgresql.org>
2023-06-14 21:37 ` Andres Freund <andres@anarazel.de>
2023-06-14 21:57 ` Andres Freund <andres@anarazel.de>
2023-06-14 22:30 ` Andres Freund <andres@anarazel.de>
2023-06-15 20:35 ` Will Mortensen <will@extrahop.com>
2023-08-11 21:15 ` Jacob Speidel <jacob@extrahop.com>
2023-08-12 00:23 ` Michael Paquier <michael@paquier.xyz>
2023-08-12 00:48 ` Andres Freund <andres@anarazel.de>
2023-08-14 00:11 ` Michael Paquier <michael@paquier.xyz>
2023-09-01 01:08 ` Michael Paquier <michael@paquier.xyz>
2023-09-01 02:56 ` Nathan Bossart <nathandbossart@gmail.com>
2023-09-01 03:31 ` Michael Paquier <michael@paquier.xyz>
2023-09-01 15:20 ` Nathan Bossart <nathandbossart@gmail.com>
2023-09-02 00:08 ` Michael Paquier <michael@paquier.xyz>
2023-09-02 15:21 ` Nathan Bossart <nathandbossart@gmail.com>
2023-09-03 23:09 ` Michael Paquier <michael@paquier.xyz>
2023-09-04 01:55 ` Will Mortensen <will@extrahop.com>
2023-09-12 02:11 ` Andres Freund <andres@anarazel.de>
2023-06-16 01:01 ` Michael Paquier <michael@paquier.xyz>
2023-06-16 01:36 ` Will Mortensen <will@extrahop.com>
2023-06-16 01:58 ` Michael Paquier <michael@paquier.xyz>
2023-06-19 17:45 ` Andres Freund <andres@anarazel.de>
2023-06-20 06:41 ` Michael Paquier <michael@paquier.xyz>
2023-06-21 03:59 ` Andres Freund <andres@anarazel.de>
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