On Thu, Jan 22, 2026 at 6:30 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
> > On Jan 21, 2026, at 15:10, Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> 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’sbetter 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.
