Re: Reject negative max_retention_duration values

> On Jun 10, 2026, at 18:31, Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Wed, Jun 10, 2026 at 12:47 PM Chao Li <li.evan.chao@gmail.com> wrote:
>> 
>>> On Jun 10, 2026, at 13:40, Michael Paquier <michael@paquier.xyz> 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.
>> 
> 
> You have made some good points for this defensive coding and I do see
> value in those but I still feel we can leave the original code as-is.
> The new people who try to understand this code area could question why
> this < is required here similar to Kuroda-san, we can try to address
> that via a comment but I feel there is no strong appeal for the same.
> If you still want to make the case, you can extract that part of the
> code and we can discuss it separately, if we reach consensus on the
> same, I would be happy to commit the same.
> 
> -- 
> With Regards,
> Amit Kapila.

I don’t have a strong opinion there. So I reverted the change from worker.c. Other parts are unchanged from v1.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/