Re: [HACKERS] Restricting maximum keep segments by repslots

On Fri, Jul 13, 2018 at 5:40 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello.
>
> At Wed, 11 Jul 2018 15:09:23 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoCFtW6+SN_eVTszDAjQeeU2sSea2VpCEx08ejNafk8H9w(at)mail(dot)gmail(dot)com>
>> On Mon, Jul 9, 2018 at 2:47 PM, Kyotaro HORIGUCHI
>> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> ..
>> Here is review comments of v4 patches.
>>
>> + if (minKeepLSN)
>> + {
>> + XLogRecPtr slotPtr = XLogGetReplicationSlotMinimumLSN();
>> + Assert(!XLogRecPtrIsInvalid(slotPtr));
>> +
>> + tailSeg = GetOldestKeepSegment(currpos, slotPtr);
>> +
>> + XLogSegNoOffsetToRecPtr(tailSeg, 0, *minKeepLSN,
>> wal_segment_size);
>> + }
>>
>> The usage of XLogSegNoOffsetToRecPtr is wrong. Since we specify the
>> destination at 4th argument the wal_segment_size will be changed in
>> the above expression. The regression tests by PostgreSQL Patch Tester
>
> I'm not sure I get this correctly, the definition of the macro is
> as follows.
>
> | #define XLogSegNoOffsetToRecPtr(segno, offset, dest, wal_segsz_bytes) \
> | (dest) = (segno) * (wal_segsz_bytes) + (offset)
>
> The destination is the *3rd* parameter and the forth is segment
> size which is not to be written.

Please see commit a22445ff0b which flipped input and output arguments.
So maybe you need to rebase the patches to current HEAD.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center