Hii Wang wei, > > 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).
Yes, the sequential scan is added during make_one_rel. > If I am missing > something, please let me know. BTW, there is a typo in above comment: `by > by`. > As the comment mentions, the sequential scan could have been dominated & removed by index scan, see add_path(): > *We also remove from the rel's pathlist any old paths that are dominated * by new_path --- that is, new_path is cheaper, at least as well ordered, * generates no more rows, requires no outer rels not required by the old * path, and is no less parallel-safe. Still, I agree that the comment could be improved, which I pushed. > 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`. > > Makes sense, removed thanks. Attached v13.
v13_0001_use_index_on_subs_when_pub_rep_ident_full.patch
Description: Binary data