> 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);

```

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/






Reply via email to