CountDBSubscriptions check in dropdb

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

CountDBSubscriptions check in dropdb

akapila
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 <[hidden email]>
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

rearrange_countdbsubscription_check_1.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CountDBSubscriptions check in dropdb

akapila
On Mon, Oct 21, 2019 at 11:43 AM Amit Kapila <[hidden email]> 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 <[hidden email]>
> 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].  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] - https://www.postgresql.org/message-id/CAFj8pRD75_wYzigvhk3fLcixGSkevwnYtdwE3gf%2Bb8EqRqbXSA%40mail.gmail.com

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: CountDBSubscriptions check in dropdb

Peter Eisentraut-6
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


Reply | Threaded
Open this post in threaded view
|

Re: CountDBSubscriptions check in dropdb

akapila
On Sat, Nov 9, 2019 at 3:58 PM Peter Eisentraut
<[hidden email]> 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

rearrange_countdbsubscription_check_2.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CountDBSubscriptions check in dropdb

Tom Lane-2
Amit Kapila <[hidden email]> writes:
> On Sat, Nov 9, 2019 at 3:58 PM Peter Eisentraut
> <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: CountDBSubscriptions check in dropdb

akapila
On Sat, Nov 9, 2019 at 9:38 PM Tom Lane <[hidden email]> wrote:

>
> Amit Kapila <[hidden email]> writes:
> > On Sat, Nov 9, 2019 at 3:58 PM Peter Eisentraut
> > <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: CountDBSubscriptions check in dropdb

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CountDBSubscriptions check in dropdb

akapila
On Mon, Nov 11, 2019 at 6:43 AM Michael Paquier <[hidden email]> 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