Re: remove wal_level archive

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