On Wed, Jul 5, 2023 at 4:32 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Wed, Jul 5, 2023 at 2:46 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Wed, Jul 5, 2023 at 9:01 AM Peter Smith <smithpb2...@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
