RE: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher - Mailing list pgsql-hackers

On Monay, Mar 6, 2023 7:19 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> 
> Yeah, seems easier to follow to me as well. Reflected it in the comment as well.  
> 

Thanks for updating the patch. Here are some comments on v33-0001 patch.

1.
+    if (RelationReplicaIdentityFullIndexScanEnabled(localrel) &&
+        remoterel->replident == REPLICA_IDENTITY_FULL)

RelationReplicaIdentityFullIndexScanEnabled() is introduced in 0002 patch, so
the call to it should be moved to 0002 patch I think.

2.
+#include "optimizer/cost.h"

Do we need this in the latest patch? I tried and it looks it could be removed
from src/backend/replication/logical/relation.c.

3.
+# now, create a unique index and set the replica
+$node_publisher->safe_psql('postgres',
+    "CREATE UNIQUE INDEX test_replica_id_full_unique ON test_replica_id_full(x);");
+$node_subscriber->safe_psql('postgres',
+    "CREATE UNIQUE INDEX test_replica_id_full_unique ON test_replica_id_full(x);");
+

Should the comment be "now, create a unique index and set the replica identity"?

4.
+$node_publisher->safe_psql('postgres',
+    "ALTER TABLE test_replica_id_full REPLICA IDENTITY USING INDEX test_replica_id_full_unique;");
+$node_subscriber->safe_psql('postgres',
+    "ALTER TABLE test_replica_id_full REPLICA IDENTITY USING INDEX test_replica_id_full_unique;");
+
+# wait for the synchronization to finish
+$node_subscriber->wait_for_subscription_sync;

There's no new tables to need to be synchronized here, should we remove the call
to wait_for_subscription_sync?

Regards,
Shi Yu