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(dot)mshk(at)gmail(dot)com> wrote:
>
> 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.
>

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