On Wed, Feb 22, 2023 at 7:54 PM Önder Kalacı <onderkal...@gmail.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.
