On Thu, Jan 22, 2026 at 6:30 PM Chao Li <[email protected]> wrote: > > > On Jan 21, 2026, at 15:10, Hayato Kuroda (Fujitsu) > > <[email protected]> wrote: > > > > Dear Shveta, > > > > Thanks for ideas, I prefer first one. Also, pgindent told me that Line blank > > should be before the code comment. PSA the new version. > > > > Best regards, > > Hayato Kuroda > > FUJITSU LIMITED > >> Shveta > > <v2-0001-Add-Assert-for-UPDATE-DELETE-_ORIGIN_DIFFERS.patch> > > Hi Hayato-san, > > Thanks for the patch. Though the change is simple, I see some problems: > > ``` > + /* > + * We reach this point only if track_commit_timestamp > is enabled. > + * Therefore, localts must contain a valid timestamp. > + */ > + Assert(localts); > ``` > > 1. The same code appears twice, so kinda redundant. > 2. The comment is unclear. It asserts localts, but the comment talks about > GUC track_commit_timestamp. > 3. Assert(localts) technically works, because C treat un-zero integer as > true, but as we are check localts is valid, it’s better to be explicit as > Assert(localts != 0). > > So, I would suggest add the assert in the very beginning of the function as: > ``` > /* > * UPDATE_ORIGIN_DIFFERS and DELETE_ORIGIN_DIFFERS conflicts are only > * reported when track_commit_timestamp is enabled, and a valid local > * commit timestamp is available for the conflicting row. > */ > Assert(type != CT_UPDATE_ORIGIN_DIFFERS && type != CT_DELETE_ORIGIN_DIFFERS > || localts != 0); >
We can follow your suggestion to reduce minor code duplicacy but OTOH it leads to type specific checks (Assert in this case) which can make code difficult to follow if we continue such a practice. But your idea to have a bit more explicit assert like Assert(localts != 0) sounds okay to me. -- With Regards, Amit Kapila.
