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

On Tues, Sep 20, 2022 at 18:30 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> Thanks for the reviews, attached v12.

Thanks for your patch. Here is a question and a comment:

1. In the function GetCheapestReplicaIdentityFullPath.
+    if (rel->pathlist == NIL)
+    {
+        /*
+         * A sequential scan could have been dominated by by an index scan
+         * during make_one_rel(). We should always have a sequential scan
+         * before set_cheapest().
+         */
+        Path       *seqScanPath = create_seqscan_path(root, rel, NULL, 0);
+
+        add_path(rel, seqScanPath);
+    }

This is a question I'm not sure about:
Do we need this part to add sequential scan?

I think in our case, the sequential scan seems to have been added by the
function make_one_rel (see function set_plain_rel_pathlist). If I am missing
something, please let me know. BTW, there is a typo in above comment: `by by`.

2. In the file execReplication.c.
+#ifdef USE_ASSERT_CHECKING
+#include "catalog/index.h"
+#endif
 #include "commands/trigger.h"
 #include "executor/executor.h"
 #include "executor/nodeModifyTable.h"
 #include "nodes/nodeFuncs.h"
 #include "parser/parse_relation.h"
 #include "parser/parsetree.h"
+#ifdef USE_ASSERT_CHECKING
+#include "replication/logicalrelation.h"
+#endif

I think it's fine to only add `logicalrelation.h` here, because `index.h` has
been added by `logicalrelation.h`.

Regards,
Wang wei