On Wed, Jan 28, 2026 at 5:36 PM shveta malik <[email protected]> wrote:
>
> On Wed, Jan 28, 2026 at 5:33 PM shveta malik <[email protected]> wrote:
> >
> >
> > 4)
> > +# Verify that '2' is present inside the JSON structure using a regex
> > +# This matches the key/value pattern for "a": 2
> > +like($raw_json, qr/\\"a\\":2/, 'Verified that key 2 exists in the
> > local_conflicts');
> > +
>
> Oops, I missed adding my feedback for pt4 earlier, here it is:
>
> To properly validate correctness, given that local_conflicts is an
> array of multiple local tuple details here, we should also check that
> it contains the keys b:3 and c:4.
>
Few more comments on 002:
5)
+ bool log_dest_clt;
+ bool log_dest_logfile;
Do we need 'clt' in the variable name? It would be clearer to name it
log_dest_table. Having said that, neither of these names clearly
indicate a boolean value. Will it be better to name these as
logToTable and logToLogFile?
6)
+/*
+ * Helper function to extract the "raw" index key Datums and their null flags
+ * from a TupleTableSlot, given an already open index descriptor.
+ * This is the reusable core logic.
+ */
+static void
+build_index_datums_from_slot(EState *estate, Relation localrel,
+ TupleTableSlot *slot,
+ Relation indexDesc, Datum *values,
+ bool *isnull)
We should mention which ones are output variables here. Something like:
Helper function to extract...to values[] and isnull[] arrays.
<or any other way>
7)
In build_index_datums_from_slot(), shall we check/assert that 'values'
and 'isnull' are non-null before passing it to FormIndexDatum.
8)
+static TupleDesc
+build_conflict_tupledesc(void)
The name gives an impression that we are building a full
tuple-descriptor for CLT, while it is only for local_conflicts. Shall
we name it to build_tupledesc_for_local_conflicts or
build_local_conflicts_tupledesc. It aligns with its caller as well
'build_local_conflicts_json_array'.
9)
build_local_conflicts_json_array():
+ int i;
+ i = 0;
+ foreach(lc, json_datums)
+ {
+ json_datum_array[i] = (Datum) lfirst(lc);
+ i++;
+ }
I think variable 'i' and its usage is leftover from some debugging.
10)
There are few new functions such as tuple_table_slot_to_json_datum,
tuple_table_slot_to_indextup_json, build_conflict_tupledesc,
prepare_conflict_log_tuple which mention function name in
header-comment section as well, while other functions do not. We can
keep header-comment section of all functions in the same format. I
think we generally do not mention function names explicitly in the
header.
thanks
Shveta
