> On Jun 10, 2026, at 18:31, Amit Kapila <[email protected]> wrote: > > On Wed, Jun 10, 2026 at 12:47 PM Chao Li <[email protected]> wrote: >> >>> On Jun 10, 2026, at 13:40, Michael Paquier <[email protected]> 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/
v2-0001-Reject-negative-max_retention_duration-values.patch
Description: Binary data
