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(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Jul 5, 2023 at 9:01 AM Peter Smith <smithpb2250(at)gmail(dot)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