RE: Collect statistics about conflicts in logical replication

On Monday, August 26, 2024 3:30 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> ======
> src/include/replication/conflict.h
>
> nit - defined 'NUM_CONFLICT_TYPES' inside the enum (I think this way is
> often used in other PG source enums)

I think we have recently tended to avoid doing that, as it has been commented
that this style is somewhat deceptive and can cause confusion. See a previous
similar comment[1]. The current style follows the other existing examples like:

#define IOOBJECT_NUM_TYPES (IOOBJECT_TEMP_RELATION + 1)
#define IOCONTEXT_NUM_TYPES (IOCONTEXT_VACUUM + 1)
#define IOOP_NUM_TYPES (IOOP_WRITEBACK + 1)
#define BACKEND_NUM_TYPES (B_LOGGER + 1)
...

> ======
> src/test/subscription/t/026_stats.pl
>
> 1.
> + # Delete data from the test table on the publisher. This delete
> + operation # should be skipped on the subscriber since the table is already
> empty.
> + $node_publisher->safe_psql($db, qq(DELETE FROM $table_name;));
> +
> + # Wait for the subscriber to report tuple missing conflict.
> + $node_subscriber->poll_query_until(
> + $db,
> + qq[
> + SELECT update_missing_count > 0 AND delete_missing_count > 0 FROM
> + pg_stat_subscription_stats WHERE subname = '$sub_name'
> + ])
> + or die
> + qq(Timed out while waiting for tuple missing conflict for
> subscription '$sub_name');
>
> Can you write a comment to explain why the replicated DELETE is
> expected to increment both the 'update_missing_count' and the
> 'delete_missing_count'?

I think the comments several lines above the wait explained the reason[2]. I
slightly modified the comments to make it clear.

Other changes look good to me and have been merged, thanks!

Here is the V2 patch.

[1] https://www.postgresql.org/message-id/202201130922.izanq4hkkqnx%40alvherre.pgsql

[2]
..
# Truncate test table to ensure the upcoming update operation is skipped
# and the test can continue.
$node_subscriber->safe_psql($db, qq(TRUNCATE $table_name));

Best Regards,
Hou zj