Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Wed, Jul 5, 2023 at 2:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Jul 5, 2023 at 9:01 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Hi. Here are some review comments for this patch.
> >
> > +1 for the patch idea.
> >
> > ------
> >
> > I wasn't sure about the code comment adjustments suggested by Amit [1]:
> > "Accordingly, the comments atop build_replindex_scan_key(),
> > FindUsableIndexForReplicaIdentityFull(), IsIndexOnlyOnExpression()
> > should also be adjusted."

As for IsIndexOnlyOnExpression(), what part do you think we need to
adjust? It says:

/*
 * Returns true if the given index consists only of expressions such as:
 *  CREATE INDEX idx ON table(foo(col));
 *
 * Returns false even if there is one column reference:
 *   CREATE INDEX idx ON table(foo(col), col_2);
 */

and it seems to me that the function doesn't check if the leftmost
index column is a non-expression.

> > doc/src/sgml/logical-replication.sgml
> >
> > 1.
> >     the key.  When replica identity <literal>FULL</literal> is specified,
> >     indexes can be used on the subscriber side for searching the rows.
> > Candidate
> >     indexes must be btree, non-partial, and have at least one column reference
> > -   (i.e. cannot consist of only expressions).  These restrictions
> > -   on the non-unique index properties adhere to some of the restrictions that
> > -   are enforced for primary keys.  If there are no such suitable indexes,
> > +   at the leftmost column indexes (i.e. cannot consist of only
> > expressions).  These
> > +   restrictions on the non-unique index properties adhere to some of
> > the restrictions
> > +   that are enforced for primary keys.  If there are no such suitable indexes,
> >     the search on the subscriber side can be very inefficient, therefore
> >     replica identity <literal>FULL</literal> should only be used as a
> >     fallback if no other solution is possible.  If a replica identity other
> >
> > Isn't this using the word "indexes" with different meanings in the
> > same sentence? e.g. IIUC "leftmost column indexes" is referring to the
> > ordinal number of the index fields.

That was my mistake, it should be " at the leftmost column".

>
> I don't know if this suggestion is what the code is actually doing. In
> function RemoteRelContainsLeftMostColumnOnIdx(), we have the following
> checks:
> ==========
> keycol = indexInfo->ii_IndexAttrNumbers[0];
> if (!AttributeNumberIsValid(keycol))
> return false;
>
> if (attrmap->maplen <= AttrNumberGetAttrOffset(keycol))
> return false;
>
> return attrmap->attnums[AttrNumberGetAttrOffset(keycol)] >= 0;
> ==========
>
> The first of these checks indicates that the leftmost column of the
> index should be non-expression, second and third indicates what you
> suggest in your wording. We can also think that what you wrote in a
> way is a superset of "leftmost index column is a non-expression" and
> "leftmost index column should be present in remote rel" but I feel it
> would be better to explicit about the first part as it is easy to
> understand for users at least in docs.

+1

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com