Обсуждение: remove wal_level archive

So we've had several rounds of discussions about simplifying replication
configuration in general and the wal_level setting in particular. [0][1]
 Let's get something going.  While we have not reached a complete
consensus yet, a few things have become clear:

- We would like to have fewer or easier to change settings.

- It looks like some folks would prefer a switchover to a completely new
and better system, but my experience in these kinds of matters is that
it's better to take smaller steps or we won't get anything changed.

- The distinction between wal_level settings "archive" and "hot_standby"
is in the way of automation or better intelligence, because the primary
cannot tell what the receiver intends to do with the WAL.

So here is a patch to get rid of the distinction.

Bike-shedding:  In this patch, I removed "archive" and kept
"hot_standby", because that's what the previous discussions suggested.
Historically and semantically, it would be more correct the other way
around.  On the other hand, keeping "hot_standby" would probably require
fewer configuration files to be changed.  Or we could keep both, but
that would be confusing (for users and in the code).

I kept the distinction between XLogIsNeeded() and
XLogStandbyInfoActive(), because it is kind of nice for documentation
(although the names are terrible).

The changed comment in xlog_redo() could probably use some review or a
bit more detailed reasoning.

There were a couple of places that I felt were overly coupled with the
wal_level settings.  The XLogArchiving*() macros shouldn't really care
what wal_level is, because that is checked elsewhere.  I replaced that
with assertions.  The check in CheckSlotRequirements() seems
unnecessary.  Why can I not add and remove slots while wal_level is minimal?

The documentation and error messages could also use more overhaul.  Some
parts say things like "archive or higher", implying that the user knows
about the ordering, other parts list all the allowed options, possibly
implying that they are mutually exclusive variants.

Maybe some of these things could be split out into separate patches.


[0]:
http://www.postgresql.org/message-id/20150203124317.GA24767@awork2.anarazel.de
[1]: http://www.postgresql.org/message-id/55D31E0D.8060801@gmx.net

On 1 September 2015 at 10:39, Peter Eisentraut <peter_e@gmx.net> wrote:
> So we've had several rounds of discussions about simplifying replication
> configuration in general and the wal_level setting in particular. [0][1]
>
> [snip]
>
> Bike-shedding:  In this patch, I removed "archive" and kept
> "hot_standby", because that's what the previous discussions suggested.
> Historically and semantically, it would be more correct the other way
> around.  On the other hand, keeping "hot_standby" would probably require
> fewer configuration files to be changed.  Or we could keep both, but
> that would be confusing (for users and in the code).

We need to keep both, IMO, with 'archive' as an obsolete synonym for
hot_standby.

Otherwise pg_upgrade will get grumpy, and so will users who migrate
their configurations.

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

On Mon, Nov 2, 2015 at 2:21 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 1 September 2015 at 10:39, Peter Eisentraut <peter_e@gmx.net> wrote:
>> So we've had several rounds of discussions about simplifying replication
>> configuration in general and the wal_level setting in particular. [0][1]
>>
>> [snip]
>>
>> Bike-shedding:  In this patch, I removed "archive" and kept
>> "hot_standby", because that's what the previous discussions suggested.
>> Historically and semantically, it would be more correct the other way
>> around.  On the other hand, keeping "hot_standby" would probably require
>> fewer configuration files to be changed.  Or we could keep both, but
>> that would be confusing (for users and in the code).
>
> We need to keep both, IMO, with 'archive' as an obsolete synonym for
> hot_standby.
>
> Otherwise pg_upgrade will get grumpy, and so will users who migrate
> their configurations.

pg_upgradecluster has some logic to switch a parameter value (see
strrepl), and pg_upgrade does not handle parameter name switches by
itself, so the price to pay would be more maintenance annoyance for
existing upgrade scripts, which happens at more or less each major
release (checkpoint_segments removed in 9.5, unix_socket_directory
renamed in 9.3, etc.).
-- 
Michael

On 2 November 2015 at 14:19, Michael Paquier <michael.paquier@gmail.com> wrote:

> pg_upgradecluster has some logic to switch a parameter value (see
> strrepl)

That's part of pg_wrapper, not core, though.

I'd quite like to see pg_wrapper become part of the PGDG RPMs, but
right now AFAIK it's a Debian-derived-only thing.

> and pg_upgrade does not handle parameter name switches by
> itself

Exactly.

> so the price to pay would be more maintenance annoyance for
> existing upgrade scripts, which happens at more or less each major
> release (checkpoint_segments removed in 9.5, unix_socket_directory
> renamed in 9.3, etc.).

Fair point. I'm not a great fan of how those changes affect users, but
I've also seen what too much backward compatibility can do to a
project. (Ref: Java, the Win32 APIs).

If users miss it then it won't explode anything in a way that's
dangerous or harmful, so I won't complain overly. I just wanted to
raise it as a possible concern.

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

On 11/2/15 12:21 AM, Craig Ringer wrote:
> On 1 September 2015 at 10:39, Peter Eisentraut <peter_e@gmx.net> wrote:
>> So we've had several rounds of discussions about simplifying replication
>> configuration in general and the wal_level setting in particular. [0][1]
>>
>> [snip]
>>
>> Bike-shedding:  In this patch, I removed "archive" and kept
>> "hot_standby", because that's what the previous discussions suggested.
>> Historically and semantically, it would be more correct the other way
>> around.  On the other hand, keeping "hot_standby" would probably require
>> fewer configuration files to be changed.  Or we could keep both, but
>> that would be confusing (for users and in the code).
>
> We need to keep both, IMO, with 'archive' as an obsolete synonym for
> hot_standby.


I would prefer to rename 'hot_standby to 'archive' and make 
'hot_standby' a deprecated synonym for the new 'archive' setting.  This 
prevents breakage in current configurations and avoids propagating a 
misleading setting.

I see a fair number of installations with backup/archiving but no hot 
standby (or any standby at all).  There is often confusion when I 
suggest setting 'wal_level' to 'hot_standby' to be better prepared for 
the future.  Admittedly these setups are becoming less common but they 
are certainly out there.

-- 
-David
david@pgmasters.net

On Mon, Nov 2, 2015 at 12:21 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> We need to keep both, IMO, with 'archive' as an obsolete synonym for
> hot_standby.
>
> Otherwise pg_upgrade will get grumpy, and so will users who migrate
> their configurations.

Removing options entirely arguably brings some worthwhile
simplification from a user perspective, but it's really unclear to me
that mapping the same set of options onto fewer underlying behaviors
buys us much.  If we don't care enough about getting rid of archive to
force people to change postgresql.conf, I doubt whether this is buying
us enough to be worthwhile.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Peter Eisentraut wrote:
> So we've had several rounds of discussions about simplifying replication
> configuration in general and the wal_level setting in particular. [0][1]
>  Let's get something going.

I looked at this patch, which I think has got enough consensus that you
should just push forward with the proposed design -- in particular, just
remove one of archive or hot_standby values, not keep it as a synonym of
the other.  If we're counting votes, I prefer keeping hot_standby over
archive.

The patch is nicely compact and applies, with only some fuzz.

I agree with changing all parts that say "XYZ or higher" to enumerate
the possible values.

It may be a good idea to have a look at Michael Paquier's recovery test
framework ( also in this commitfest: https://commitfest.postgresql.org/8/438/ )
and see how that is affected by this patch.  Maybe the tests can find a
problem in this patch, and so perhaps you'd like to commit the tests
first, then this change on top.

I'm marking this as Ready for Committer, and setting you up as such for
this patch.  If you would prefer not to commit, let me know and I can do
so.

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

On Mon, Jan 4, 2016 at 11:04 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Peter Eisentraut wrote:
>> So we've had several rounds of discussions about simplifying replication
>> configuration in general and the wal_level setting in particular. [0][1]
>>  Let's get something going.
>
> I looked at this patch, which I think has got enough consensus that you
> should just push forward with the proposed design -- in particular, just
> remove one of archive or hot_standby values, not keep it as a synonym of
> the other.  If we're counting votes, I prefer keeping hot_standby over
> archive.

I see precisely 0 votes for that alternative upthread.  I came the
closest of anyone to endorsing that proposal, I think, but my
preferred alternative is to change nothing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Mon, Jan 4, 2016 at 11:04 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > Peter Eisentraut wrote:
> >> So we've had several rounds of discussions about simplifying replication
> >> configuration in general and the wal_level setting in particular. [0][1]
> >>  Let's get something going.
> >
> > I looked at this patch, which I think has got enough consensus that you
> > should just push forward with the proposed design -- in particular, just
> > remove one of archive or hot_standby values, not keep it as a synonym of
> > the other.  If we're counting votes, I prefer keeping hot_standby over
> > archive.
> 
> I see precisely 0 votes for that alternative upthread.  I came the
> closest of anyone to endorsing that proposal, I think, but my
> preferred alternative is to change nothing.

Hm?  Perhaps the problem is that the thread stood on the shoulders of
larger threads.  Andres Freund and Magnus Hagander both expressed,
independently, their desire to see one of these modes go away:
http://www.postgresql.org/message-id/20150203124317.GA24767@awork2.anarazel.de
http://www.postgresql.org/message-id/CABUevEy15Y=sF8doKjD86eujJZL=Tq2jyUSiYVkg6EVwVt=cag@mail.gmail.com

Actually, in the first of these threads
http://www.postgresql.org/message-id/flat/20150203124317.GA24767@awork2.anarazel.de
there's a lot of discussion about getting rid of wal_level completely
instead, but it doesn't look like there's any movement at all in that
direction.  I think we should take this pretty small change that
improves things a bit in that direction, then others can continue to
propose further simplifications to our configuration in that area.

We could continue to do nothing, but then we've already been doing that
for some time and it hasn't changed things much.

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

On Mon, Jan 4, 2016 at 3:05 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Robert Haas wrote:
>> On Mon, Jan 4, 2016 at 11:04 AM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>> > Peter Eisentraut wrote:
>> >> So we've had several rounds of discussions about simplifying replication
>> >> configuration in general and the wal_level setting in particular. [0][1]
>> >>  Let's get something going.
>> >
>> > I looked at this patch, which I think has got enough consensus that you
>> > should just push forward with the proposed design -- in particular, just
>> > remove one of archive or hot_standby values, not keep it as a synonym of
>> > the other.  If we're counting votes, I prefer keeping hot_standby over
>> > archive.
>>
>> I see precisely 0 votes for that alternative upthread.  I came the
>> closest of anyone to endorsing that proposal, I think, but my
>> preferred alternative is to change nothing.
>
> Hm?  Perhaps the problem is that the thread stood on the shoulders of
> larger threads.  Andres Freund and Magnus Hagander both expressed,
> independently, their desire to see one of these modes go away:
> http://www.postgresql.org/message-id/20150203124317.GA24767@awork2.anarazel.de
> http://www.postgresql.org/message-id/CABUevEy15Y=sF8doKjD86eujJZL=Tq2jyUSiYVkg6EVwVt=cag@mail.gmail.com

OK, I see.

> Actually, in the first of these threads
> http://www.postgresql.org/message-id/flat/20150203124317.GA24767@awork2.anarazel.de
> there's a lot of discussion about getting rid of wal_level completely
> instead, but it doesn't look like there's any movement at all in that
> direction.  I think we should take this pretty small change that
> improves things a bit in that direction, then others can continue to
> propose further simplifications to our configuration in that area.
>
> We could continue to do nothing, but then we've already been doing that
> for some time and it hasn't changed things much.

True.  But it hasn't really caused much trouble either.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jan 5, 2016 at 1:04 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Peter Eisentraut wrote:
>> So we've had several rounds of discussions about simplifying replication
>> configuration in general and the wal_level setting in particular. [0][1]
>>  Let's get something going.
>
> I looked at this patch, which I think has got enough consensus that you
> should just push forward with the proposed design -- in particular, just
> remove one of archive or hot_standby values, not keep it as a synonym of
> the other.  If we're counting votes, I prefer keeping hot_standby over
> archive.

FWIW I have advocated for the simple removal of 'archive' :)

> The patch is nicely compact and applies, with only some fuzz.
>
> I agree with changing all parts that say "XYZ or higher" to enumerate
> the possible values.

Yep.

> It may be a good idea to have a look at Michael Paquier's recovery test
> framework ( also in this commitfest: https://commitfest.postgresql.org/8/438/ )
> and see how that is affected by this patch.  Maybe the tests can find a
> problem in this patch, and so perhaps you'd like to commit the tests
> first, then this change on top.

Those would need a rebase if this patch stays as is. I'll take actions
as needed.
-- 
Michael

On 26-01-2016 12:56, Simon Riggs wrote:
> Removing one of "archive" or "hot standby" will just cause confusion and
> breakage, so neither is a good choice for removal.
> 
Agree.

> What we should do is 
> 1. Map "archive" and "hot_standby" to one level with a new name that
> indicates that it can be used for both/either backup or replication.
>       (My suggested name for the new level is "replica"...)
> 2. Deprecate "archive" and "hot_standby" so that those will be removed
> in a later release.
> 
3. Add a WARNING at startup (until removal of values) saying something
like "'archive' value is deprecated and will be removed in a future
version. Use 'replica' value instead."


--   Euler Taveira                   Timbira - http://www.timbira.com.br/  PostgreSQL: Consultoria, Desenvolvimento,
Suporte24x7 e Treinamento
 

On 1/26/16 10:56 AM, Simon Riggs wrote:
> Removing one of "archive" or "hot standby" will just cause confusion and
> breakage, so neither is a good choice for removal.

I'm pretty sure nothing would break, but I do agree that it could be
confusing.

> What we should do is 
> 1. Map "archive" and "hot_standby" to one level with a new name that
> indicates that it can be used for both/either backup or replication.
>       (My suggested name for the new level is "replica"...)

I have been leaning toward making up a new name, too, but hadn't found a
good one.  I tend to like "replica", though.

> 2. Deprecate "archive" and "hot_standby" so that those will be removed
> in a later release.

If we do 1, then we might as well get rid of the old names right away.

On Thu, Jan 28, 2016 at 10:53 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 1/26/16 10:56 AM, Simon Riggs wrote:
>> What we should do is
>> 1. Map "archive" and "hot_standby" to one level with a new name that
>> indicates that it can be used for both/either backup or replication.
>>       (My suggested name for the new level is "replica"...)
>
> I have been leaning toward making up a new name, too, but hadn't found a
> good one.  I tend to like "replica", though.

"replica" sounds nice.

>> 2. Deprecate "archive" and "hot_standby" so that those will be removed
>> in a later release.
>
> If we do 1, then we might as well get rid of the old names right away.

+1.
-- 
Michael

On 1/26/16 10:56 AM, Simon Riggs wrote:
> Removing one of "archive" or "hot standby" will just cause confusion and
> breakage, so neither is a good choice for removal.
>
> What we should do is
> 1. Map "archive" and "hot_standby" to one level with a new name that
> indicates that it can be used for both/either backup or replication.
>       (My suggested name for the new level is "replica"...)
> 2. Deprecate "archive" and "hot_standby" so that those will be removed
> in a later release.

Updated patch to reflect these suggestions.

On Mon, Feb 8, 2016 at 6:47 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 1/26/16 10:56 AM, Simon Riggs wrote:
>> Removing one of "archive" or "hot standby" will just cause confusion and
>> breakage, so neither is a good choice for removal.
>>
>> What we should do is
>> 1. Map "archive" and "hot_standby" to one level with a new name that
>> indicates that it can be used for both/either backup or replication.
>>       (My suggested name for the new level is "replica"...)
>> 2. Deprecate "archive" and "hot_standby" so that those will be removed
>> in a later release.
>
> Updated patch to reflect these suggestions.

Shouldn't backup.sgml be updated as well? Here is the portion that I
am referring to:   To enable WAL archiving, set the <xref linkend="guc-wal-level">   configuration parameter to
<literal>archive</>or higher,   <xref linkend="guc-archive-mode"> to <literal>on</>,
 
        But minimal WAL does not contain enough information to reconstruct the
-        data from a base backup and the WAL logs, so <literal>archive</> or
+        data from a base backup and the WAL logs, so <literal>replica</> or        higher must be used to enable WAL
archiving       (<xref linkend="guc-archive-mode">) and streaming replication.       </para>       <para>
 
-        In <literal>hot_standby</> level, the same information is logged as
-        with <literal>archive</>, plus information needed to reconstruct
-        the status of running transactions from the WAL. To enable read-only
As the paragraph about the difference between hot_standby and archive
is removed, I think that it would be better to mention that setting
wal_level to replica allows to reconstruct data from a base backup and
the WAL logs, *and* to run read-only queries when hot_standby is
enabled.

-               if (ControlFile->wal_level < WAL_LEVEL_HOT_STANDBY)
+               if (ControlFile->wal_level < WAL_LEVEL_REPLICA)
Upthread it was mentioned that switching to an approach where enum
values are directly listed would be better. The target of an extra
patch on top of this one?

-       if (wal_level < WAL_LEVEL_ARCHIVE)
-               ereport(ERROR,
-
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                                errmsg("replication slots can only be
used if wal_level >= archive")));
We should still forbid the creation of replication slots if wal_level = minimal.
-- 
Michael

On 2/7/16 4:47 PM, Peter Eisentraut wrote:
> On 1/26/16 10:56 AM, Simon Riggs wrote:
>> Removing one of "archive" or "hot standby" will just cause confusion and
>> breakage, so neither is a good choice for removal.
>>
>> What we should do is
>> 1. Map "archive" and "hot_standby" to one level with a new name that
>> indicates that it can be used for both/either backup or replication.
>>       (My suggested name for the new level is "replica"...)
>> 2. Deprecate "archive" and "hot_standby" so that those will be removed
>> in a later release.
>
> Updated patch to reflect these suggestions.

-#define XLogIsNeeded() (wal_level >= WAL_LEVEL_ARCHIVE)
+#define XLogIsNeeded() (wal_level >= WAL_LEVEL_REPLICA)
<...>
-#define XLogStandbyInfoActive() (wal_level >= WAL_LEVEL_HOT_STANDBY)
+#define XLogStandbyInfoActive() (wal_level >= WAL_LEVEL_REPLICA)

Since these are identical now shouldn't one be removed?  I searched the
code and I couldn't find anything that looked dead (i.e. XLogIsNeeded()
&& !XLogStandbyInfoActive()) but it still seems like having both could
cause confusion.

--
-David
david@pgmasters.net

Peter Eisentraut wrote:
> On 1/26/16 10:56 AM, Simon Riggs wrote:
> > Removing one of "archive" or "hot standby" will just cause confusion and
> > breakage, so neither is a good choice for removal.
> > 
> > What we should do is 
> > 1. Map "archive" and "hot_standby" to one level with a new name that
> > indicates that it can be used for both/either backup or replication.
> >       (My suggested name for the new level is "replica"...)
> > 2. Deprecate "archive" and "hot_standby" so that those will be removed
> > in a later release.
> 
> Updated patch to reflect these suggestions.

I wonder if the "keep one / keep both" argument is running in circles as
new reviewers arrive at the thread.  Perhaps somebody could read the
whole thread(s) and figure out a way to find consensus so that we move
forward on this.

I've closed it as returned-with-feedback for now.  Please resubmit to
next CF.

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

On 2/8/16 9:36 AM, David Steele wrote:
> -#define XLogIsNeeded() (wal_level >= WAL_LEVEL_ARCHIVE)
> +#define XLogIsNeeded() (wal_level >= WAL_LEVEL_REPLICA)
> <...>
> -#define XLogStandbyInfoActive() (wal_level >= WAL_LEVEL_HOT_STANDBY)
> +#define XLogStandbyInfoActive() (wal_level >= WAL_LEVEL_REPLICA)
> 
> Since these are identical now shouldn't one be removed?  I searched the
> code and I couldn't find anything that looked dead (i.e. XLogIsNeeded()
> && !XLogStandbyInfoActive()) but it still seems like having both could
> cause confusion.

I think this should eventually be cleaned up, but it doesn't seem
necessary in the first patch.

On 2/8/16 7:34 AM, Michael Paquier wrote:
> Shouldn't backup.sgml be updated as well? Here is the portion that I
> am referring to:
>     To enable WAL archiving, set the <xref linkend="guc-wal-level">
>     configuration parameter to <literal>archive</> or higher,
>     <xref linkend="guc-archive-mode"> to <literal>on</>,
>
>          But minimal WAL does not contain enough information to reconstruct the
> -        data from a base backup and the WAL logs, so <literal>archive</> or
> +        data from a base backup and the WAL logs, so <literal>replica</> or
>          higher must be used to enable WAL archiving
>          (<xref linkend="guc-archive-mode">) and streaming replication.
>         </para>

Checked for leftovers again and fixed one.

>         <para>
> -        In <literal>hot_standby</> level, the same information is logged as
> -        with <literal>archive</>, plus information needed to reconstruct
> -        the status of running transactions from the WAL. To enable read-only
> As the paragraph about the difference between hot_standby and archive
> is removed, I think that it would be better to mention that setting
> wal_level to replica allows to reconstruct data from a base backup and
> the WAL logs, *and* to run read-only queries when hot_standby is
> enabled.

Well, I think that is really only of historical interest.  The
assumption is, as long as hot_standby = on, you can run read-only
queries.  The WAL level is taken completely out of the mental
consideration, because if you have replicate at all, it's good enough.
That is part of the point of this patch.

>
> -               if (ControlFile->wal_level < WAL_LEVEL_HOT_STANDBY)
> +               if (ControlFile->wal_level < WAL_LEVEL_REPLICA)
> Upthread it was mentioned that switching to an approach where enum
> values are directly listed would be better. The target of an extra
> patch on top of this one?

I'm not sure what is meant by that.

>
> -       if (wal_level < WAL_LEVEL_ARCHIVE)
> -               ereport(ERROR,
> -
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> -                                errmsg("replication slots can only be
> used if wal_level >= archive")));
> We should still forbid the creation of replication slots if wal_level = minimal.

I think I took this out because you actually can't get to that check,
but I put it back in because it seems better for clarity.

On Tue, Mar 1, 2016 at 10:02 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 2/8/16 7:34 AM, Michael Paquier wrote:
>> -               if (ControlFile->wal_level < WAL_LEVEL_HOT_STANDBY)
>> +               if (ControlFile->wal_level < WAL_LEVEL_REPLICA)
>> Upthread it was mentioned that switching to an approach where enum
>> values are directly listed would be better. The target of an extra
>> patch on top of this one?
>
> I'm not sure what is meant by that.

The following for example, aka using only equal comparators with the
name of wal_level used:   if (ArchiveRecoveryRequested && EnableHotStandby)   {
-       if (ControlFile->wal_level < WAL_LEVEL_HOT_STANDBY)
+       if (ControlFile->wal_level == WAL_LEVEL_ARCHIVE ||
+           ControlFile->wal_level == WAL_LEVEL_MINIMAL)           ereport(ERROR,
But that's nitpicking (this was mentioned upthread), and not something
this patch should care about.

>> -       if (wal_level < WAL_LEVEL_ARCHIVE)
>> -               ereport(ERROR,
>> -
>> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>> -                                errmsg("replication slots can only be
>> used if wal_level >= archive")));
>> We should still forbid the creation of replication slots if wal_level = minimal.
>
> I think I took this out because you actually can't get to that check,
> but I put it back in because it seems better for clarity.

It is possible to hit it, take for example the following set of
parameters in postgresql.conf:
max_replication_slots = 3
wal_level = minimal
max_wal_senders = 0
=# select pg_create_physical_replication_slot('toto');
ERROR:  55000: replication slots can only be used if wal_level >= archive
LOCATION:  CheckSlotRequirements, slot.c:766

If this patch gets committed before the one improving the checkpoint
skip logic when wal_level >= hot_standby regarding standby snapshots
(http://www.postgresql.org/message-id/CAB7nPqQAaB0v25tt4SJ_mGc3aHfZrionEG4E5cceGVZc0B6QyA@mail.gmail.com),
something to not forget is that there will be a regression: on idle
systems checkpoints won't be skipped anymore, which is now what
happens with wal_level = archive on HEAD.

Except this last concern, the patch is in a nice shape, and does what
it says it does.
-- 
Michael

On 2/8/16 2:34 PM, Alvaro Herrera wrote:
> Peter Eisentraut wrote:
>> On 1/26/16 10:56 AM, Simon Riggs wrote:
>>> Removing one of "archive" or "hot standby" will just cause confusion and
>>> breakage, so neither is a good choice for removal.
>>>
>>> What we should do is
>>> 1. Map "archive" and "hot_standby" to one level with a new name that
>>> indicates that it can be used for both/either backup or replication.
>>>       (My suggested name for the new level is "replica"...)
>>> 2. Deprecate "archive" and "hot_standby" so that those will be removed
>>> in a later release.
>>
>> Updated patch to reflect these suggestions.
>
> I wonder if the "keep one / keep both" argument is running in circles as
> new reviewers arrive at the thread.  Perhaps somebody could read the
> whole thread(s) and figure out a way to find consensus so that we move
> forward on this.

There was a lot of argument upstream about whether to keep 'hot_standby'
or 'archive' but after the proposal to change it to 'replica' came up
everybody seemed to fall in line with that.

+1 from me for using 'replica' as the WAL level to replace 'hot_standby'
and 'archive'.

+1 from me for removing the 'hot_standby' and 'archive' options entirely
in 9.6 rather than deprecating.

Unless anyone has objections I would like to mark this 'ready for
committer'.

--
-David
david@pgmasters.net

On 3/11/16 1:29 PM, David Steele wrote:

> Unless anyone has objections I would like to mark this 'ready for
> committer'.

This patch is now ready for committer.

-- 
-David
david@pgmasters.net

On Mon, Mar 14, 2016 at 12:50 PM, David Steele <david@pgmasters.net> wrote:
> On 3/11/16 1:29 PM, David Steele wrote:
>
>> Unless anyone has objections I would like to mark this 'ready for
>> committer'.
>
>
> This patch is now ready for committer.

Yes, thanks, I am cool with this version as well. I guess I should
have done what you just did at my last review to be honest.
-- 
Michael

On Mon, Mar 14, 2016 at 11:46 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Mar 14, 2016 at 12:50 PM, David Steele <david@pgmasters.net> wrote:
>> On 3/11/16 1:29 PM, David Steele wrote:
>>
>>> Unless anyone has objections I would like to mark this 'ready for
>>> committer'.
>>
>>
>> This patch is now ready for committer.
>
> Yes, thanks, I am cool with this version as well. I guess I should
> have done what you just did at my last review to be honest.

This patch has been committed as b555ed8, and maps wal_level =
"archive" to "hot_standby". As mentioned here, the condition to skip
checkpoints when a system is idle is already broken for a couple of
releases when wal_level = "hot_standby":
http://www.postgresql.org/message-id/CAB7nPqT5XdZYo0rub8hyBC9CiWxB6=TSG7ffm_QBR+Q4L8ZFGg@mail.gmail.com
So now it is broken as for "archive".

This has been already discussed on this thread:
http://www.postgresql.org/message-id/20151016203031.3019.72930@wrigleys.postgresql.org
And there is a patch as well:
https://commitfest.postgresql.org/9/398/

As the bug discussed previously is now also a regression specific to
9.6, are there objections if I add an open item?
-- 
Michael