On Tue, Jan 13, 2026 at 3:25 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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.
Done
> ~
>
> 1b.
> Consider renaming the variable '$conflict_table' as '$clt', so it
> cannot be confused with "the table that has the conflict".
Done
> ~~~
>
> 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/
Done
> ~~~
> 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?
How about matching specifically 2 using like($all_keys, qr/\b2\b/,
'Verified that key 2 exists in the local_conflicts log'); ?
> ~~~
>
> 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.
Done same treatment as above.
I am not sure does it makes sense to check the tuple from clt for all
the test cases in 035_conflict.pl or just checking in a few of them is
sufficient? Thoughts?
--
Regards,
Dilip Kumar
Google
