Re: Reject negative max_retention_duration values

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.