Horton

Thread

Commits

  1. Rearrange dropdb() to avoid errors after allowing other sessions to exit.

  2. Logical replication

  1. CountDBSubscriptions check in dropdb

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-21T06:13:56Z

    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

  2. Re: CountDBSubscriptions check in dropdb

    Amit Kapila <amit.kapila16@gmail.com> — 2019-11-08T13:38:34Z

    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

  3. Re: CountDBSubscriptions check in dropdb

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-11-09T10:27:58Z

    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

  4. Re: CountDBSubscriptions check in dropdb

    Amit Kapila <amit.kapila16@gmail.com> — 2019-11-09T12:07:27Z

    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

  5. Re: CountDBSubscriptions check in dropdb

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-09T16:08:14Z

    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

  6. Re: CountDBSubscriptions check in dropdb

    Amit Kapila <amit.kapila16@gmail.com> — 2019-11-10T03:18:27Z

    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

  7. Re: CountDBSubscriptions check in dropdb

    Michael Paquier <michael@paquier.xyz> — 2019-11-11T01:13:11Z

    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

  8. Re: CountDBSubscriptions check in dropdb

    Amit Kapila <amit.kapila16@gmail.com> — 2019-11-11T03:16:54Z

    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