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

On Thu, 2 Mar 2023 at 20:53, Önder Kalacı <onderkalaci(at)gmail(dot)com> wrote:
>
> Hi,
>
>>
>> Is it just a matter of testing other
>> index types
>
>
> Yes, there are more to it. build_replindex_scan_key()
> only works for btree indexes, as it does BTEqualStrategyNumber.
>
> I might expect a few more limitations like that. I added comment
> in the code (see FindUsableIndexForReplicaIdentityFull)
>
>> or there is something more to it, if so, we should add
>> comments so that they can be supported in the future if it is feasible
>> to do so.
>
>
> I really don't see any fundamental issues regarding expanding the
> support for more index types, it is just some more coding & testing.
>
> And, I can (and willing to) work on that as a follow-up. I explicitly
> try to keep this patch as small as possible.
>
>>
>> >
>> > Attached are both patches: the main patch, and the patch that
>> > optionally disables the index scans.
>> >
>>
>> Both the patches are numbered 0001. It would be better to number them
>> as 0001 and 0002.
>>
>
> Alright, attached v27_0001_use_index_on_subs_when_pub_rep_ident_full.patch and
> v27_0002_use_index_on_subs_when_pub_rep_ident_full.patch.
>
> I also added one more test which Andres asked me on a private chat
> (Testcase start: SUBSCRIPTION USES INDEX WITH PUB/SUB different data).

Thanks for the patch. Few comments:
1) We are currently calling RelationGetIndexList twice, once in
FindUsableIndexForReplicaIdentityFull function and in the caller too,
we could avoid one of the calls by passing the indexlist to the
function or removing the check here, index list check can be handled
in FindUsableIndexForReplicaIdentityFull.
+ if (remoterel->replident == REPLICA_IDENTITY_FULL &&
+ RelationGetIndexList(localrel) != NIL)
+ {
+ /*
+ * If we had a primary key or relation identity with a
unique index,
+ * we would have already found and returned that oid.
At this point,
+ * the remote relation has replica identity full and
we have at least
+ * one local index defined.
+ *
+ * We are looking for one more opportunity for using
an index. If
+ * there are any indexes defined on the local
relation, try to pick
+ * a suitable index.
+ *
+ * The index selection safely assumes that all the
columns are going
+ * to be available for the index scan given that
remote relation has
+ * replica identity full.
+ */
+ return FindUsableIndexForReplicaIdentityFull(localrel);
+ }
+

2) Copyright year should be mentioned as 2023
diff --git a/src/test/subscription/t/032_subscribe_use_index.pl
b/src/test/subscription/t/032_subscribe_use_index.pl
new file mode 100644
index 0000000000..db0a7ea2a0
--- /dev/null
+++ b/src/test/subscription/t/032_subscribe_use_index.pl
@@ -0,0 +1,861 @@
+# Copyright (c) 2021-2022, PostgreSQL Global Development Group
+
+# Test logical replication behavior with subscriber uses available index
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+

3) Many of the tests are using the same tables, we need not
drop/create publication/subscription for each of the team, we could
just drop and create required indexes and verify the update/delete
statements.
+# ====================================================================
+# Testcase start: SUBSCRIPTION USES INDEX
+#
+# Basic test where the subscriber uses index
+# and only updates 1 row and deletes
+# 1 other row
+#
+
+# create tables pub and sub
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE test_replica_id_full (x int)");
+$node_publisher->safe_psql('postgres',
+ "ALTER TABLE test_replica_id_full REPLICA IDENTITY FULL;");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE test_replica_id_full (x int)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE INDEX test_replica_id_full_idx ON test_replica_id_full(x)");

+# ====================================================================
+# Testcase start: SUBSCRIPTION CREATE/DROP INDEX WORKS WITHOUT ISSUES
+#
+# This test ensures that after CREATE INDEX, the subscriber can automatically
+# use one of the indexes (provided that it fulfils the requirements).
+# Similarly, after DROP index, the subscriber can automatically switch to
+# sequential scan
+
+# create tables pub and sub
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE test_replica_id_full (x int NOT NULL, y int)");
+$node_publisher->safe_psql('postgres',
+ "ALTER TABLE test_replica_id_full REPLICA IDENTITY FULL;");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE test_replica_id_full (x int NOT NULL, y int)");

4) These additional blank lines can be removed to keep it consistent:
4.a)
+# Testcase end: SUBSCRIPTION DOES NOT USE PARTIAL INDEX
+# ====================================================================
+
+
+# ====================================================================
+# Testcase start: SUBSCRIPTION DOES NOT USE INDEXES WITH ONLY EXPRESSIONS

4.b)
+# Testcase end: Unique index that is not primary key or replica identity
+# ====================================================================
+
+
+
+# ====================================================================
+# Testcase start: SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN

Regards,
Vignesh