CountDBSubscriptions check in dropdb

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 -0500

Logical 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

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