Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

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