Hi Shi Yu, all
> 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 of the patch. Thanks, Onder KALACI