Re: Proposal: Conflict log history table for Logical Replication

Hi Dilip.

Some comments about the v20-0002 (test code only)

======
src/test/subscription/t/035_conflicts.pl

1.
+my $subid = $node_subscriber->safe_psql('postgres',
+ "SELECT oid FROM pg_subscription WHERE subname = 'sub_tab';");
+my $conflict_table = "pg_conflict.pg_conflict_$subid";

1a.
Maybe there should be a comment somewhere near here to describe that
the remainder of this test part is for verifying the contents of the
CLT.

~

1b.
Consider renaming the variable '$conflict_table' as '$clt', so it
cannot be confused with "the table that has the conflict".

~~~

2.
+# Wait for the conflict to be logged
+my $log_check = $node_subscriber->poll_query_until(
+ 'postgres',
+ "SELECT count(*) > 0 FROM $conflict_table;"
+);

/to be logged/to be logged in the CLT/

~~~

3.
+# Verify that '2' is present in the resulting string
+like($all_keys, qr/2/, 'Verified that key 2 exists in the
local_conflicts log');

The check just for '2' seemed vague. IIUC, this would also match 12,
25, 102, or anything else with a 2 in it. Is it possible for this test
to compare a longer string so it is guaranteed to be a more unique
match for what you are expecting?

~~~

4.
+# Wait for the conflict to be logged
+$log_check = $node_subscriber->poll_query_until(
+ 'postgres',
+ "SELECT count(*) > 0 FROM $conflict_table;"
+);
+
+$conflict_check = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM $conflict_table WHERE conflict_type =
'multiple_unique_conflicts';");
+is($conflict_check, 1, 'Verified multiple_unique_conflicts logged
into internal table');
+
+$json_query = qq[
+ SELECT string_agg((unnested.j::json)->'key'->>'a', ',')
+ FROM (
+ SELECT unnest(local_conflicts) AS j
+ FROM $conflict_table
+ ) AS unnested;
+];
+
+$all_keys = $node_subscriber->safe_psql('postgres', $json_query);
+
+# Verify that '6' is present in the resulting string
+like($all_keys, qr/6/, 'Verified that key 6 exists in the
local_conflicts log');
+

All the same review comments above apply here, too.

- Comment to say all this is for verifying the CLT
- /to be logged/to be logged in the CLT/
- The check just for '6' seemed vague.

======
Kind Regards,
Peter Smith.
Fujitsu Australia