Re: Reject negative max_retention_duration values

> On Jun 10, 2026, at 18:31, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Jun 10, 2026 at 12:47 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>>
>>> On Jun 10, 2026, at 13:40, Michael Paquier <michael(at)paquier(dot)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/