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.
Ah, sure, a rebase issue, fixed in v34
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.
Hmm, probably an artifact of the initial versions of the patch where we needed some
costing functionality.
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"?
yes, fixed
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?
right, probably a copy & paste typo, thanks for spotting.
I'll attach v34 with the next e-mail given the comments here only touch small parts
