Thread: CountDBSubscriptions check in dropdb
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
Thoughts?
[1] - https://commitfest.postgresql.org/25/2055/
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachment
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]. 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
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
Attachment
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
Attachment
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