> On Jun 9, 2026, at 22:44, Hayato Kuroda (Fujitsu) <[email protected]>
> 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/
