> On Jun 10, 2026, at 13:40, Michael Paquier <[email protected]> wrote:
>
> On Wed, Jun 10, 2026 at 05:12:26AM +0000, Hayato Kuroda (Fujitsu) wrote:
>> Dear Chao,
>>
>>> Yes, this patch rejects negative values at CREATE/ALTER SUBSCRIPTION time,
>>> so in theory the if (MySubscription->maxretention <= 0) change is not
>>> strictly
>>> necessary. I made that change for a few reasons (from strong do weak):
>>
>> I personal preference is to use Assert() for detecting cannot-happen case,
>> but it's not very strong opinion. Let's see how others say.
>
> An assertion offers less protection than an elog(ERROR) if a value is
> read from catalogs, which could be the case here? Think for example
> corrupted catalog data. (I did not read the patch in details, so I
> may have missed something, of course, but I was under the impression
> that this could apply for this case with MySubscription.)
> --
> Michael
AFAIK, Assert is mostly for catching code bugs, so I don’t think it is the
right tool here. I agree that an elog(ERROR) would catch an invalid catalog
value at runtime.
However, looking at the nearly code where MySubscription->maxretention is read:
LogicalRepApplyLoop()
```
else if (MySubscription->maxretention > 0)
wait_time = Min(wait_time, MySubscription->maxretention);
```
adjust_xid_advance_interval()
```
if (MySubscription->retentionactive && MySubscription->maxretention > 0)
rdt_data->xid_advance_interval = Min(rdt_data->xid_advance_interval,
MySubscription->maxretention);
```
Both treat the timeout as active only when maxretention > 0. So making
should_stop_conflict_info_retention() return false when maxretention <= 0 is
consistent with the existing pattern. If we add an elog(ERROR) for
MySubscription->maxretention < 0 here, then the question is why we don’t add
the same check in the other places too.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
