Re: Reject negative max_retention_duration values

> On Jun 9, 2026, at 22:44, Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> wrote:
> 
> Dear Chao,
> 
> +1 the idea to reject the negative value.
> 
> ```
> @@ -4796,7 +4796,7 @@ should_stop_conflict_info_retention(RetainDeadTuplesData *rdt_data)
> Assert(rdt_data->phase == RDT_WAIT_FOR_PUBLISHER_STATUS ||
>   rdt_data->phase == RDT_WAIT_FOR_LOCAL_FLUSH);
> 
> - if (!MySubscription->maxretention)
> + if (MySubscription->maxretention <= 0)
> return false;
> ```
> 
> IIUC, the negative value can be rejected at CREATE/ALTER SUBSCRIPTION phase, right?
> Why do we have to update even here? For the clarification purpose?
> 

Thanks for your review.

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):

* The new check is equivalent to if (!(MySubscription->maxretention > 0)), meaning MySubscription->maxretention is not meaningful for this function unless it is positive. So semantically, I think it is correct, or at least it does not hurt anything.
* The new check makes the purpose more explicit in this function: only positive values are meaningful here. 
* It also keeps the worker logic self-contained: this function only performs timeout checks when max_retention_duration is positive, instead of relying entirely on option parsing to ensure that.
* Since v19beta1 has been released, there is a very small possibility that some databases, most likely test deployments, may already have negative max_retention_duration values, so this also acts as defensive coding. I understand beta1 can never be a production deployment, so this is just a weak reason.

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