Re: [HACKERS] Restricting maximum keep segments by repslots

On Mon, Jul 9, 2018 at 2:47 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello. Sawada-san.
>
> Thank you for the comments.
>

Thank you for updating the patch!

> At Thu, 5 Jul 2018 15:43:56 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoDiiA4qHj0thqw80Bt=vefSQ9yGpZnr0kuLTXszbrV9iQ(at)mail(dot)gmail(dot)com>
>> On Wed, Jul 4, 2018 at 5:28 PM, Kyotaro HORIGUCHI
>> ---
>> + SpinLockAcquire(&XLogCtl->info_lck);
>> + oldestSeg = XLogCtl->lastRemovedSegNo;
>> + SpinLockRelease(&XLogCtl->info_lck);
>>
>> We can use XLogGetLastRemovedSegno() instead.
>
> It is because I thought that it is for external usage,
> spcifically by slot.c since CheckXLogRemoved() is reading it
> directly. I leave it alone and they would have to be fixed at
> once if we decide to use it internally.

Agreed. I noticed that after commented.

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
seem passed but I got the following assertion failure in
recovery/t/010_logical_decoding_timelines.pl

TRAP: FailedAssertion("!(XLogRecPtrToBytePos(*StartPos) ==
startbytepos)", File: "xlog.c", Line: 1277)
----
+ XLByteToSeg(restartLSN, restartSeg, wal_segment_size);
+
+
+ if (minKeepLSN)

There is an extra empty line.

----
+ /* but, keep larger than wal_segment_size if any*/
+ if (wal_keep_segments > 0 && keepSegs < wal_keep_segments)
+ keepSegs = wal_keep_segments;

You meant wal_keep_segments in the above comment rather than
wal_segment_size? Also, the above comment need a whitespace just after
"any".

----
+bool
+IsLsnStillAvaiable(XLogRecPtr restartLSN, XLogRecPtr *minKeepLSN)
+{

I think restartLSN is a word used for replication slots. Since this
function is defined in xlog.c it would be better to change the
argument name to more generic name, for example recptr.

----
+ /*
+ * Calcualte keep segments by slots first. The second term of the
+ * condition is just a sanity check.
+ */

s/calcualte/calculate/

----
+ /* get minimum segment ignorig timeline ID */

s/ignorig/ignoring/

----
min_keep_lsn in pg_replication_slots currently shows the same value in
every slots but I felt that the value seems not easy to understand
intuitively for users because users will have to confirm that value
and to compare the current LSN in order to check if replication slots
will become the "lost" status. So how about showing values that
indicate how far away from the point where we become "lost" for
individual slots?

Regards,

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