CountDBSubscriptions check in dropdb
While reviewing Pavel's patch for a new option in Drop Database
command [1]https://commitfest.postgresql.org/25/2055/, I noticed that the check for CountDBSubscriptions in
dropdb() is done after we kill the autovac workers and allowed other
backends to exit via CountOtherDBBackends. Now, if there are already
active subscritions due to which we can't drop database, then it is
better to fail before we do CountOtherDBBackends. It is also
indicated in a comment (
check this after other error conditions) that CountOtherDBBackends has
to be done after error checks.
So, I feel we should rearrange the code to move the subscriptions
check before CountOtherDBBackends as is done in the attached patch.
This has been introduced by below commit:
commit 665d1fad99e7b11678b0d5fa24d2898424243cd6
Author: Peter Eisentraut <peter_e@gmx.net>
Date: Thu Jan 19 12:00:00 2017 -0500
Logical replication
Thoughts?
[1]: https://commitfest.postgresql.org/25/2055/
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
rearrange_countdbsubscription_check_1.patchapplication/octet-stream; name=rearrange_countdbsubscription_check_1.patchDownload
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 01d66212e9..4ad62e6bf8 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -896,19 +896,6 @@ dropdb(const char *dbname, bool missing_ok)
nslots_active, nslots_active)));
}
- /*
- * Check for other backends in the target database. (Because we hold the
- * database lock, no new ones can start after this.)
- *
- * As in CREATE DATABASE, check this after other error conditions.
- */
- if (CountOtherDBBackends(db_id, ¬herbackends, &npreparedxacts))
- ereport(ERROR,
- (errcode(ERRCODE_OBJECT_IN_USE),
- errmsg("database \"%s\" is being accessed by other users",
- dbname),
- errdetail_busy_db(notherbackends, npreparedxacts)));
-
/*
* Check if there are subscriptions defined in the target database.
*
@@ -924,6 +911,19 @@ dropdb(const char *dbname, bool missing_ok)
"There are %d subscriptions.",
nsubscriptions, nsubscriptions)));
+ /*
+ * Check for other backends in the target database. (Because we hold the
+ * database lock, no new ones can start after this.)
+ *
+ * As in CREATE DATABASE, check this after other error conditions.
+ */
+ if (CountOtherDBBackends(db_id, ¬herbackends, &npreparedxacts))
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_IN_USE),
+ errmsg("database \"%s\" is being accessed by other users",
+ dbname),
+ errdetail_busy_db(notherbackends, npreparedxacts)));
+
/*
* Remove the database's tuple from pg_database.
*/
On Mon, Oct 21, 2019 at 11:43 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
While reviewing Pavel's patch for a new option in Drop Database
command [1], I noticed that the check for CountDBSubscriptions in
dropdb() is done after we kill the autovac workers and allowed other
backends to exit via CountOtherDBBackends. Now, if there are already
active subscritions due to which we can't drop database, then it is
better to fail before we do CountOtherDBBackends. It is also
indicated in a comment (
check this after other error conditions) that CountOtherDBBackends has
to be done after error checks.So, I feel we should rearrange the code to move the subscriptions
check before CountOtherDBBackends as is done in the attached patch.This has been introduced by below commit:
commit 665d1fad99e7b11678b0d5fa24d2898424243cd6
Author: Peter Eisentraut <peter_e@gmx.net>
Date: Thu Jan 19 12:00:00 2017 -0500Logical replication
I am planning to commit and backpatch this till PG10 where it was
introduced on Monday morning (IST). Pavel agreed that this is a good
change in the other thread where we need it [1]/messages/by-id/CAFj8pRD75_wYzigvhk3fLcixGSkevwnYtdwE3gf+b8EqRqbXSA@mail.gmail.com. It is not an urgent
thing, so I can wait if we think this is not a good time to commit
this. Let me know if anyone has objections?
[1]: /messages/by-id/CAFj8pRD75_wYzigvhk3fLcixGSkevwnYtdwE3gf+b8EqRqbXSA@mail.gmail.com
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On 2019-11-08 14:38, Amit Kapila wrote:
I am planning to commit and backpatch this till PG10 where it was
introduced on Monday morning (IST). Pavel agreed that this is a good
change in the other thread where we need it [1]. It is not an urgent
thing, so I can wait if we think this is not a good time to commit
this. Let me know if anyone has objections?
I think the change makes sense for master, but I don't think it should
be backpatched.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Nov 9, 2019 at 3:58 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 2019-11-08 14:38, Amit Kapila wrote:
I am planning to commit and backpatch this till PG10 where it was
introduced on Monday morning (IST). Pavel agreed that this is a good
change in the other thread where we need it [1]. It is not an urgent
thing, so I can wait if we think this is not a good time to commit
this. Let me know if anyone has objections?I think the change makes sense for master, but I don't think it should
be backpatched.
Fair enough. Attached patch with a proposed commit message.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
rearrange_countdbsubscription_check_2.patchapplication/octet-stream; name=rearrange_countdbsubscription_check_2.patchDownload
From 991b29dadbb82ccd9afc65b8f1f8a6d48afd1a6d Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Sat, 9 Nov 2019 17:28:27 +0530
Subject: [PATCH] Rearrange dropdb() to avoid errors after allowing other
sessions to exit.
During Drop Database, it is better to error out before allowing other
sessions to exit and forcefully terminating autovacuum workers. All the
other errors except for checking subscriptions are done before.
Author: Amit Kapila
Discussion: https://postgr.es/m/CAA4eK1+qhLkCYG2oy9xug9ur_j=G2wQNRYAyd+-kZfZ1z42pLw@mail.gmail.com
---
src/backend/commands/dbcommands.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 01d6621..4ad62e6 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -897,19 +897,6 @@ dropdb(const char *dbname, bool missing_ok)
}
/*
- * Check for other backends in the target database. (Because we hold the
- * database lock, no new ones can start after this.)
- *
- * As in CREATE DATABASE, check this after other error conditions.
- */
- if (CountOtherDBBackends(db_id, ¬herbackends, &npreparedxacts))
- ereport(ERROR,
- (errcode(ERRCODE_OBJECT_IN_USE),
- errmsg("database \"%s\" is being accessed by other users",
- dbname),
- errdetail_busy_db(notherbackends, npreparedxacts)));
-
- /*
* Check if there are subscriptions defined in the target database.
*
* We can't drop them automatically because they might be holding
@@ -925,6 +912,19 @@ dropdb(const char *dbname, bool missing_ok)
nsubscriptions, nsubscriptions)));
/*
+ * Check for other backends in the target database. (Because we hold the
+ * database lock, no new ones can start after this.)
+ *
+ * As in CREATE DATABASE, check this after other error conditions.
+ */
+ if (CountOtherDBBackends(db_id, ¬herbackends, &npreparedxacts))
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_IN_USE),
+ errmsg("database \"%s\" is being accessed by other users",
+ dbname),
+ errdetail_busy_db(notherbackends, npreparedxacts)));
+
+ /*
* Remove the database's tuple from pg_database.
*/
tup = SearchSysCache1(DATABASEOID, ObjectIdGetDatum(db_id));
--
1.8.3.1
Amit Kapila <amit.kapila16@gmail.com> writes:
On Sat, Nov 9, 2019 at 3:58 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:On 2019-11-08 14:38, Amit Kapila wrote:
I am planning to commit and backpatch this till PG10 where it was
introduced on Monday morning (IST). Pavel agreed that this is a good
change in the other thread where we need it [1]. It is not an urgent
thing, so I can wait if we think this is not a good time to commit
this. Let me know if anyone has objections?
I think the change makes sense for master, but I don't think it should
be backpatched.
Fair enough. Attached patch with a proposed commit message.
I don't have an opinion on whether it's appropriate to back-patch
this, but I do have an opinion that Monday morning is the worst
possible schedule for committing such a thing. We are already
past the point where we can expect to get reports from the slowest
buildfarm critters (e.g. Valgrind builds) before Monday's
back-branch wraps. Anything that is even slightly inessential
should be postponed until after those releases are tagged.
If it's HEAD-only, of course, it's business as usual.
regards, tom lane
On Sat, Nov 9, 2019 at 9:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Amit Kapila <amit.kapila16@gmail.com> writes:
On Sat, Nov 9, 2019 at 3:58 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:On 2019-11-08 14:38, Amit Kapila wrote:
I am planning to commit and backpatch this till PG10 where it was
introduced on Monday morning (IST). Pavel agreed that this is a good
change in the other thread where we need it [1]. It is not an urgent
thing, so I can wait if we think this is not a good time to commit
this. Let me know if anyone has objections?I think the change makes sense for master, but I don't think it should
be backpatched.Fair enough. Attached patch with a proposed commit message.
I don't have an opinion on whether it's appropriate to back-patch
this, but I do have an opinion that Monday morning is the worst
possible schedule for committing such a thing. We are already
past the point where we can expect to get reports from the slowest
buildfarm critters (e.g. Valgrind builds) before Monday's
back-branch wraps. Anything that is even slightly inessential
should be postponed until after those releases are tagged.If it's HEAD-only, of course, it's business as usual.
I am planning to go with Peter's suggestion and will push in
HEAD-only. So, I think that should be fine.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Sun, Nov 10, 2019 at 08:48:27AM +0530, Amit Kapila wrote:
I am planning to go with Peter's suggestion and will push in
HEAD-only. So, I think that should be fine.
I was just looking at this thread, and my take would be to just apply
that on HEAD. Good catch by the way.
--
Michael
On Mon, Nov 11, 2019 at 6:43 AM Michael Paquier <michael@paquier.xyz> wrote:
On Sun, Nov 10, 2019 at 08:48:27AM +0530, Amit Kapila wrote:
I am planning to go with Peter's suggestion and will push in
HEAD-only. So, I think that should be fine.I was just looking at this thread, and my take would be to just apply
that on HEAD. Good catch by the way.
Okay, thanks for looking into it. Pushed.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com