On Tue, Nov 3, 2026 at 5:38 PM Narayanan Venkateswaran <[email protected]> wrote: > > Hi, > > I looked at v89 of the patch, I had just one small comment (different > from the comments posted already), > > In insert_conflict_log_tuple(): we have the following lines > > + if (remote_commit_ts > 0) > + values[attno++] = TimestampTzGetDatum(remote_commit_ts); > + else > + nulls[attno++] = true; > > I think "if (remote_commit_ts != 0)" would be better for the following > reasons, > > 1. build_local_conflicts_json_array() just above checks "if > (conflicttuple->ts)" > 2. The patch also has the line, > > + TimestampTz commit_ts; /* remote commit ts; 0 if unknown */ > > we explicitly define 0 as the sentinel when the remote commit > timestamp is unknown
Yeah make sense, and also for consistency with conflicttuple->ts we can check, if (remote_commit_ts != 0) or if (remote_commit_ts) -- Regards, Dilip Kumar Google
