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

On Wed, Jul 5, 2023 at 4:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> 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.
>

TBH, this IsIndexOnlyOnExpression() function seemed redundant to me,
otherwise, there can be some indexes that are firstly considered
"useable" but then fail the subsequent leftmost check. It does not
seem right.

> > > 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".

IIUC the subscriber-side table can have more columns than the
publisher-side table, so just describing in the docs that the leftmost
INDEX field must be a column may not be quite enough; it also needs to
say that column has to exist on the publisher-table, doesn't it?

Also, after you document this 'leftmost field restriction' that
already implies there *must* be a non-expression in the INDEX. So I
thought we can just omit the "(i.e. cannot consist of only
expressions)" part.

Anyway, I will wait to see the wording of the updated patch before
commenting further.

------
Kind Regards,
Peter Smith.
Fujitsu Australia