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

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.