> 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
doweak):
* The new check is equivalent to if (!(MySubscription->maxretention > 0)), meaning MySubscription->maxretention is not
meaningfulfor 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
ispositive, 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
understandbeta1 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/
