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.
