Re: max_slot_wal_keep_size and wal_keep_segments

On 2020/07/13 16:01, Kyotaro Horiguchi wrote:
> At Mon, 13 Jul 2020 14:14:30 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote in
>>
>>
>> On 2020/07/09 13:47, Kyotaro Horiguchi wrote:
>>> At Thu, 9 Jul 2020 00:37:57 +0900, Fujii Masao
>>> <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote in
>>>>
>>>>
>>>> On 2020/07/02 2:18, David Steele wrote:
>>>>> On 7/1/20 10:54 AM, Alvaro Herrera wrote:
>>>>>> On 2020-Jul-01, Fujii Masao wrote:
>>>>>>
>>>>>>> On 2020/07/01 12:26, Alvaro Herrera wrote:
>>>>>>>> On 2020-Jun-30, Fujii Masao wrote:
>>>>>>>>
>>>>>>>>> When I talked about max_slot_wal_keep_size as new feature in v13
>>>>>>>>> at the conference, I received the question like "Why are the units of
>>>>>>>>> setting values in max_slot_wal_keep_size and wal_keep_segments
>>>>>>>>> different?"
>>>>>>>>> from audience. That difference looks confusing for users and
>>>>>>>>> IMO it's better to use the same unit for them. Thought?
>>>>>>>>
>>>>>>>> Do we still need wal_keep_segments for anything?
>>>>>>>
>>>>>>> Yeah, personally I like wal_keep_segments because its setting is very
>>>>>>> simple and no extra operations on replication slots are necessary.
>>>>>>
>>>>>> Okay.  In that case I +1 the idea of renaming to wal_keep_size.
>>>>> +1 for renaming to wal_keep_size.
>>>>
>>>> I attached the patch that renames wal_keep_segments to wal_keep_size.
>>> It fails on 019_replslot_limit.pl for uncertain reason to me..
>>
>> I could not reproduce this...
>
> Sorry for the ambiguity. The patch didn't applied on the file, and I
> noticed that the reason is the wording change from master to
> primary. So no problem in the latest patch.
>
>>> @@ -11323,7 +11329,7 @@ do_pg_stop_backup(char *labelfile, bool
>>> waitforarchive, TimeLineID *stoptli_p)
>>> * If archiving is enabled, wait for all the required WAL files to be
>>> * archived before returning. If archiving isn't enabled, the required
>>> * WAL
>>> * needs to be transported via streaming replication (hopefully with
>>> - * wal_keep_segments set high enough), or some more exotic mechanism like
>>> + * wal_keep_size set high enough), or some more exotic mechanism like
>>> * polling and copying files from pg_wal with script. We have no
>>> * knowledge
>>> Isn't this time a good chance to mention replication slots?
>>
>> +1 to do that. But I found there are other places where replication
>> slots
>> need to be mentioned. So I think it's better to do this as separate
>> patch.
>
> Agreed.
>
>>> - "ALTER SYSTEM SET wal_keep_segments to 8; SELECT pg_reload_conf();");
>>> + "ALTER SYSTEM SET wal_keep_size to '128MB'; SELECT
>>> pg_reload_conf();");
>>> wal_segment_size to 1MB here so, that conversion is not correct.
>>> (However, that test works as long as it is more than
>>> max_slot_wal_keep_size so it's practically no problem.)
>>
>> So I changed 128MB to 8MB. Is this OK?
>> I attached the updated version of the patch upthread.
>
> That change looks find by me.

Thanks for the review!

The patch was no longer applied cleanly because of recent commit.
So I updated the patch. Attached.

Barring any objection, I will commit this patch.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION