Horton

Thread

Commits

  1. Fix copy-paste error in errhint() introduced in 691d79a07933.

  2. Lower error level from PANIC to FATAL when restoring slots at startup

  3. Fix error message typo introduced 691d79a07933.

  4. Disallow starting server with insufficient wal_level for existing slot.

  5. Allow replication slots to be dropped in single-user mode

  1. replication_slots usability issue

    Joshua D. Drake <jd@commandprompt.com> — 2018-10-29T17:31:56Z

    -Hackers,


Working on 9.6 today (unsure if fixed in newer versions). Had an issue 
where the wal was 280G despite max_wal_size being 8G. Found out there 
were stale replication slots from a recent base backup. I went to drop 
the replication slots and found that since the wal_level was set to 
minimal vs replica or higher, I couldn't drop the replication slot. 
Clearly that makes sense for creating a replication slot but it seems 
like an artificial limitation for dropping them.


JD

-- 
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
***  A fault and talent of mine is to tell it exactly how it is.  ***
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://postgresconf.org
*****     Unless otherwise stated, opinions are my own.   *****

  2. Re: replication_slots usability issue

    Andres Freund <andres@anarazel.de> — 2018-10-29T18:26:56Z

    
On October 29, 2018 1:31:56 PM EDT, "Joshua D. Drake" <jd@commandprompt.com> wrote:
>-Hackers,
>
>
>Working on 9.6 today (unsure if fixed in newer versions). Had an issue 
>where the wal was 280G despite max_wal_size being 8G. Found out there 
>were stale replication slots from a recent base backup. I went to drop 
>the replication slots and found that since the wal_level was set to 
>minimal vs replica or higher, I couldn't drop the replication slot. 
>Clearly that makes sense for creating a replication slot but it seems 
>like an artificial limitation for dropping them.

Uh, huh? How did you manage to start a server with existing slots with that configuration? It should have errored out at start...

Andres

Andres

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  3. Re: replication_slots usability issue

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-10-29T19:02:18Z

    On 2018-Oct-29, Joshua D. Drake wrote:

> -Hackers,
> 
> 
> Working on 9.6 today (unsure if fixed in newer versions). Had an issue where
> the wal was 280G despite max_wal_size being 8G. Found out there were stale
> replication slots from a recent base backup. I went to drop the replication
> slots and found that since the wal_level was set to minimal vs replica or
> higher, I couldn't drop the replication slot. Clearly that makes sense for
> creating a replication slot but it seems like an artificial limitation for
> dropping them.

This sounds closely related to
https://www.postgresql.org/message-id/20180508143725.mn3ivlyvgpul6ovr%40alvherre.pgsql
(commit a1f680d962ff) wherein we made it possible to drop a slot in
single-user mode.

Seems worth fixing.  Send a patch?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  4. Re: replication_slots usability issue

    Andres Freund <andres@anarazel.de> — 2018-10-29T19:13:04Z

    On 2018-10-29 16:02:18 -0300, Alvaro Herrera wrote:
> On 2018-Oct-29, Joshua D. Drake wrote:
> 
> > -Hackers,
> > 
> > 
> > Working on 9.6 today (unsure if fixed in newer versions). Had an issue where
> > the wal was 280G despite max_wal_size being 8G. Found out there were stale
> > replication slots from a recent base backup. I went to drop the replication
> > slots and found that since the wal_level was set to minimal vs replica or
> > higher, I couldn't drop the replication slot. Clearly that makes sense for
> > creating a replication slot but it seems like an artificial limitation for
> > dropping them.
> 
> This sounds closely related to
> https://www.postgresql.org/message-id/20180508143725.mn3ivlyvgpul6ovr%40alvherre.pgsql
> (commit a1f680d962ff) wherein we made it possible to drop a slot in
> single-user mode.
> 
> Seems worth fixing.  Send a patch?

I don't think this quite is the problem. ISTM the issue is rather that
StartupReplicationSlots() *needs* to check whether wal_level > minimal,
and doesn't. So you can create a slot, shutdown, change wal_level,
startup. A slot exists but won't work correctly.

Greetings,

Andres Freund

  5. Re: replication_slots usability issue

    Joshua D. Drake <jd@commandprompt.com> — 2018-10-29T22:13:07Z

    On 10/29/18 11:26 AM, Andres Freund wrote:
>
> On October 29, 2018 1:31:56 PM EDT, "Joshua D. Drake" <jd@commandprompt.com> wrote:
>> -Hackers,
>>
>>
>> Working on 9.6 today (unsure if fixed in newer versions). Had an issue
>> where the wal was 280G despite max_wal_size being 8G. Found out there
>> were stale replication slots from a recent base backup. I went to drop
>> the replication slots and found that since the wal_level was set to
>> minimal vs replica or higher, I couldn't drop the replication slot.
>> Clearly that makes sense for creating a replication slot but it seems
>> like an artificial limitation for dropping them.
> Uh, huh? How did you manage to start a server with existing slots with that configuration? It should have errored out at start...

Well, this is the recovery.conf:

standby_mode = 'on'
recovery_target = 'immediate'
primary_slot_name = 'testing_db01'
primary_conninfo = 'user=replication 
passfile=/var/lib/postgresql/.pgpass host=db01 port=5432 sslmode=prefer 
sslcompression=1 krbsrvname=postgres target_session_attrs=any'

recovery_target_action = 'promote'

The machine came up clean and the only reason I noticed the problem is 
that it ran out of disk space. I cleared enough disk space to get it to 
come up again and noticed that there were replication slots that were 
identical to the primary.


JD


-- 
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
***  A fault and talent of mine is to tell it exactly how it is.  ***
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://postgresconf.org
*****     Unless otherwise stated, opinions are my own.   *****

  6. Re: replication_slots usability issue

    Michael Paquier <michael@paquier.xyz> — 2018-10-30T02:51:09Z

    On Mon, Oct 29, 2018 at 12:13:04PM -0700, Andres Freund wrote:
> I don't think this quite is the problem. ISTM the issue is rather that
> StartupReplicationSlots() *needs* to check whether wal_level > minimal,
> and doesn't. So you can create a slot, shutdown, change wal_level,
> startup. A slot exists but won't work correctly.

It seems to me that what we are looking for is just to complain at
startup if we find any slot data and if trying to start up with
wal_level = minimal.

Er...  At the same time, shouldn't RestoreSlotFromDisk() *not* use PANIC
if more slots are found in pg_replslot than max_replication_slots can
handle.  A FATAL is fine at startup, PANIC blows up a core file, which
is clearly overdoing it if the goal is to give a recommendation at the
end.
--
Michael

  7. Re: replication_slots usability issue

    Andres Freund <andres@anarazel.de> — 2018-10-30T17:52:54Z

    On 2018-10-30 11:51:09 +0900, Michael Paquier wrote:
> On Mon, Oct 29, 2018 at 12:13:04PM -0700, Andres Freund wrote:
> > I don't think this quite is the problem. ISTM the issue is rather that
> > StartupReplicationSlots() *needs* to check whether wal_level > minimal,
> > and doesn't. So you can create a slot, shutdown, change wal_level,
> > startup. A slot exists but won't work correctly.
> 
> It seems to me that what we are looking for is just to complain at
> startup if we find any slot data and if trying to start up with
> wal_level = minimal.

Right, we really should just call CheckSlotRequirements() before doing
so. I'll make it so, once I'm actually awake and had some coffee.


> Er...  At the same time, shouldn't RestoreSlotFromDisk() *not* use PANIC
> if more slots are found in pg_replslot than max_replication_slots can
> handle.  A FATAL is fine at startup, PANIC blows up a core file, which
> is clearly overdoing it if the goal is to give a recommendation at the
> end.

I can't get particularly excited about this. I guess we can change it,
but I'd only do so in master.


Greetings,

Andres Freund

  8. Re: replication_slots usability issue

    Joshua D. Drake <jd@commandprompt.com> — 2018-10-30T18:02:04Z

    On 10/30/18 10:52 AM, Andres Freund wrote:
> On 2018-10-30 11:51:09 +0900, Michael Paquier wrote:
>> On Mon, Oct 29, 2018 at 12:13:04PM -0700, Andres Freund wrote:
>>> I don't think this quite is the problem. ISTM the issue is rather that
>>> StartupReplicationSlots() *needs* to check whether wal_level > minimal,
>>> and doesn't. So you can create a slot, shutdown, change wal_level,
>>> startup. A slot exists but won't work correctly.
>> It seems to me that what we are looking for is just to complain at
>> startup if we find any slot data and if trying to start up with
>> wal_level = minimal.
> Right, we really should just call CheckSlotRequirements() before doing
> so. I'll make it so, once I'm actually awake and had some coffee.

Why not just disable the slot and report an INFO: line?


JD


>
>
>> Er...  At the same time, shouldn't RestoreSlotFromDisk() *not* use PANIC
>> if more slots are found in pg_replslot than max_replication_slots can
>> handle.  A FATAL is fine at startup, PANIC blows up a core file, which
>> is clearly overdoing it if the goal is to give a recommendation at the
>> end.
> I can't get particularly excited about this. I guess we can change it,
> but I'd only do so in master.
>
>
> Greetings,
>
> Andres Freund
>

-- 
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
***  A fault and talent of mine is to tell it exactly how it is.  ***
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://postgresconf.org
*****     Unless otherwise stated, opinions are my own.   *****

  9. Re: replication_slots usability issue

    Andres Freund <andres@anarazel.de> — 2018-10-30T18:08:55Z

    On 2018-10-30 11:02:04 -0700, Joshua D. Drake wrote:
> On 10/30/18 10:52 AM, Andres Freund wrote:
> > On 2018-10-30 11:51:09 +0900, Michael Paquier wrote:
> > > On Mon, Oct 29, 2018 at 12:13:04PM -0700, Andres Freund wrote:
> > > > I don't think this quite is the problem. ISTM the issue is rather that
> > > > StartupReplicationSlots() *needs* to check whether wal_level > minimal,
> > > > and doesn't. So you can create a slot, shutdown, change wal_level,
> > > > startup. A slot exists but won't work correctly.
> > > It seems to me that what we are looking for is just to complain at
> > > startup if we find any slot data and if trying to start up with
> > > wal_level = minimal.
> > Right, we really should just call CheckSlotRequirements() before doing
> > so. I'll make it so, once I'm actually awake and had some coffee.
>
> Why not just disable the slot and report an INFO: line?

Because afterwards there'd be a slot with corrupted contents. Especially
bad for logical slots. Which might look ok, but crash in weird ways.

- Andres

  10. Re: replication_slots usability issue

    Michael Paquier <michael@paquier.xyz> — 2018-10-31T02:10:23Z

    On Tue, Oct 30, 2018 at 10:52:54AM -0700, Andres Freund wrote:
> On 2018-10-30 11:51:09 +0900, Michael Paquier wrote:
>> Er...  At the same time, shouldn't RestoreSlotFromDisk() *not* use PANIC
>> if more slots are found in pg_replslot than max_replication_slots can
>> handle.  A FATAL is fine at startup, PANIC blows up a core file, which
>> is clearly overdoing it if the goal is to give a recommendation at the
>> end.
> 
> I can't get particularly excited about this.

This gets only used by the startup process to restore slot information,
so for example if you have an instance with two slots, shut it down,
reduce max_replication_slots to 1, and restart then you generate a core
dump, which is a bit overdoing it to simply let the user know that he
just needs to bump up one parameter and to give him a recommendation to
do so :)

For example:
2018-10-31 11:04:08 JST [9014]: [11-1] db=,user=,app=,client= DEBUG:
starting up replication slots
2018-10-31 11:04:08 JST [9014]: [12-1] db=,user=,app=,client= DEBUG:
restoring replication slot from "pg_replslot/popo3/state"
2018-10-31 11:04:08 JST [9014]: [13-1] db=,user=,app=,client= DEBUG:
restoring replication slot from "pg_replslot/popo2/state"
2018-10-31 11:04:08 JST [9014]: [14-1] db=,user=,app=,client= PANIC:
too many replication slots active before shutdown
2018-10-31 11:04:08 JST [9014]: [15-1] db=,user=,app=,client= HINT:
Increase max_replication_slots and try again.

> I guess we can change it, but I'd only do so in master.

I won't fight hard for a back-patch, now I think that we should
back-patch a fix.  Generating a core file if a state is unexpected is
fine because you expect the system to react so, being able to trigger
one based on a parameter switch just to give a recommendation is not in
my opinion.

For example with the attached the system reacts as follows:
2018-10-31 11:07:23 JST [10063]: [14-1] db=,user=,app=,client= FATAL:
too many replication slots active before shutdown
2018-10-31 11:07:23 JST [10063]: [15-1] db=,user=,app=,client= HINT:
Increase max_replication_slots and try again.
2018-10-31 11:07:23 JST [10061]: [6-1] db=,user=,app=,client= LOG:
startup process (PID 10063) exited with exit code 1
2018-10-31 11:07:23 JST [10061]: [7-1] db=,user=,app=,client= LOG:
aborting startup due to startup process failure
--
Michael

  11. Re: replication_slots usability issue

    Andres Freund <andres@anarazel.de> — 2018-10-31T22:48:02Z

    On 2018-10-30 10:52:54 -0700, Andres Freund wrote:
> On 2018-10-30 11:51:09 +0900, Michael Paquier wrote:
> > On Mon, Oct 29, 2018 at 12:13:04PM -0700, Andres Freund wrote:
> > > I don't think this quite is the problem. ISTM the issue is rather that
> > > StartupReplicationSlots() *needs* to check whether wal_level > minimal,
> > > and doesn't. So you can create a slot, shutdown, change wal_level,
> > > startup. A slot exists but won't work correctly.
> > 
> > It seems to me that what we are looking for is just to complain at
> > startup if we find any slot data and if trying to start up with
> > wal_level = minimal.
> 
> Right, we really should just call CheckSlotRequirements() before doing
> so. I'll make it so, once I'm actually awake and had some coffee.

And done.  Thanks for the report JD.

- Andres

  12. Re: replication_slots usability issue

    Michael Paquier <michael@paquier.xyz> — 2018-11-01T00:34:05Z

    HI Andres,

On Wed, Oct 31, 2018 at 03:48:02PM -0700, Andres Freund wrote:
> And done.  Thanks for the report JD.

Shouldn't we also switch the PANIC to a FATAL in RestoreSlotFromDisk()?
I don't mind doing so myself if you agree with the change, only on
HEAD as you seemed to disagree about changing that on back-branches.

Also, from 691d79a which you just committed:
+       ereport(FATAL,
+               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                errmsg("logical replication slots \"%s\" exists, but wal_level < logical",
+                       NameStr(cp.slotdata.name)),
I can see one grammar mistake here, as you refer to only one slot here.
The error messages should read:
"logical replication slot \"%s\" exists, but wal_level < logical"
and:
"physical replication slot \"%s\" exists, but wal_level < replica"

Thanks,
--
Michael

  13. Re: replication_slots usability issue

    Andres Freund <andres@anarazel.de> — 2018-11-01T17:54:23Z

    On 2018-11-01 09:34:05 +0900, Michael Paquier wrote:
> HI Andres,
> 
> On Wed, Oct 31, 2018 at 03:48:02PM -0700, Andres Freund wrote:
> > And done.  Thanks for the report JD.
> 
> Shouldn't we also switch the PANIC to a FATAL in
> RestoreSlotFromDisk()?

That has absolutely nothing to do with the issue at hand though, so I
don't think it'd have made much sense to do it at the same time. Nor do
I think it's particularly important.


> I don't mind doing so myself if you agree with the change, only on
> HEAD as you seemed to disagree about changing that on back-branches.

Cool. And yes, I don't think a cosmetic log level adjustment that could
affect people's scripts should be backpatched without need. Even if not
particularly likely to break something.


> Also, from 691d79a which you just committed:
> +       ereport(FATAL,
> +               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> +                errmsg("logical replication slots \"%s\" exists, but wal_level < logical",
> +                       NameStr(cp.slotdata.name)),
> I can see one grammar mistake here, as you refer to only one slot here.
> The error messages should read:
> "logical replication slot \"%s\" exists, but wal_level < logical"
> and:
> "physical replication slot \"%s\" exists, but wal_level < replica"

Darnit. Fixed. Thanks.

Greetings,

Andres Freund

  14. Re: replication_slots usability issue

    Michael Paquier <michael@paquier.xyz> — 2018-11-01T23:01:28Z

    On Thu, Nov 01, 2018 at 10:54:23AM -0700, Andres Freund wrote:
> On 2018-11-01 09:34:05 +0900, Michael Paquier wrote:
> That has absolutely nothing to do with the issue at hand though, so I
> don't think it'd have made much sense to do it at the same time. Nor do
> I think it's particularly important.

Thanks for the confirmation.

>> I don't mind doing so myself if you agree with the change, only on
>> HEAD as you seemed to disagree about changing that on back-branches.
> 
> Cool. And yes, I don't think a cosmetic log level adjustment that could
> affect people's scripts should be backpatched without need. Even if not
> particularly likely to break something.

No issues with your arguments.  I did the change this way.

>> Also, from 691d79a which you just committed:
>> +       ereport(FATAL,
>> +               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>> +                errmsg("logical replication slots \"%s\" exists, but wal_level < logical",
>> +                       NameStr(cp.slotdata.name)),
>> I can see one grammar mistake here, as you refer to only one slot here.
>> The error messages should read:
>> "logical replication slot \"%s\" exists, but wal_level < logical"
>> and:
>> "physical replication slot \"%s\" exists, but wal_level < replica"
> 
> Darnit. Fixed. Thanks.

And thanks for fixing that.
--
Michael

  15. Re: replication_slots usability issue

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2018-11-02T14:51:34Z

    On 01/11/2018 18:54, Andres Freund wrote:>
>> Also, from 691d79a which you just committed:
>> +       ereport(FATAL,
>> +               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>> +                errmsg("logical replication slots \"%s\" exists, but wal_level < logical",
>> +                       NameStr(cp.slotdata.name)),
>> I can see one grammar mistake here, as you refer to only one slot here.
>> The error messages should read:
>> "logical replication slot \"%s\" exists, but wal_level < logical"
>> and:
>> "physical replication slot \"%s\" exists, but wal_level < replica"
> 
> Darnit. Fixed. Thanks.
> 

Since we are fixing this message, shouldn't the hint for logical slot
say "Change wal_level to be logical or higher" rather than "replica or
higher" :)

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  16. Re: replication_slots usability issue

    Andres Freund <andres@anarazel.de> — 2018-11-05T20:10:44Z

    On 2018-11-02 15:51:34 +0100, Petr Jelinek wrote:
> On 01/11/2018 18:54, Andres Freund wrote:>
> >> Also, from 691d79a which you just committed:
> >> +       ereport(FATAL,
> >> +               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> >> +                errmsg("logical replication slots \"%s\" exists, but wal_level < logical",
> >> +                       NameStr(cp.slotdata.name)),
> >> I can see one grammar mistake here, as you refer to only one slot here.
> >> The error messages should read:
> >> "logical replication slot \"%s\" exists, but wal_level < logical"
> >> and:
> >> "physical replication slot \"%s\" exists, but wal_level < replica"
> > 
> > Darnit. Fixed. Thanks.
> > 
> 
> Since we are fixing this message, shouldn't the hint for logical slot
> say "Change wal_level to be logical or higher" rather than "replica or
> higher" :)

Gah. I really wasn't firing on all cylinders here. Darned jetlag (let's
just assume that's what it was).

Greetings,

Andres Freund