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

On Wed, Feb 22, 2023 at 7:54 PM Önder Kalacı <onderkalaci(at)gmail(dot)com> wrote:
>

Few comments:
===============
1.
+ identity. When replica identity <literal>FULL</literal> is specified,
+ indexes can be used on the subscriber side for searching the rows. These
+ indexes should be btree,

Why only btree and not others like a hash index? Also, there should be
some comments in FindUsableIndexForReplicaIdentityFull() to explain
the choices.

2.
- * This is not generic routine, it expects the idxrel to be replication
- * identity of a rel and meet all limitations associated with that.
+ * This is not a generic routine - it expects the idxrel to be an index
+ * that planner would choose if the searchslot includes all the columns
+ * (e.g., REPLICA IDENTITY FULL on the source).
*/
-static bool
+static int

This comment is not clear to me. Which change here makes the
expectation like that? Which planner function/functionality are you
referring to here?

3.
+/*
+ * Given a relation and OID of an index, returns true if
+ * the index is relation's primary key's index or
+ * relation's replica identity index.

It seems the line length is a bit off in the above comments. There
could be a similar mismatch in other places. You might want to run
pgindent.

4.
+}
+
+
+/*
+ * Returns an index oid if there is an index that can be used

Spurious empty line.

5.
- /*
- * We are in error mode so it's fine this is somewhat slow. It's better to
- * give user correct error.
- */
- if (OidIsValid(GetRelationIdentityOrPK(rel->localrel)))
+ /* Give user more precise error if possible. */
+ if (OidIsValid(rel->usableIndexOid))
{
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),

Is this change valid? I mean this could lead to the error "publisher
did not send replica identity column expected by the logical
replication target relation" when it should have given an error:
"logical replication target relation \"%s.%s\" has neither REPLICA
IDENTITY index nor PRIMARY ...

--
With Regards,
Amit Kapila.