Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

Hi Peter,

> 1.
> In my previous review [1] (comment #1) I wrote that only some of the
> "should" were misleading and gave examples where to change. But I
> didn't say that *every* usage of that word was wrong, so your global
> replace of "should" to "must" has modified a couple of places in
> unexpected ways.
>
> Details are in subsequent review comments below -- see #2b, #3, #5.
>

Ah, that was my mistake. Thanks for thorough review on this.

>
> ======
> doc/src/sgml/logical-replication.sgml
>
> 2.
> A published table must have a “replica identity” configured in order
> to be able to replicate UPDATE and DELETE operations, so that
> appropriate rows to update or delete can be identified on the
> subscriber side. By default, this is the primary key, if there is one.
> Another unique index (with certain additional requirements) can also
> be set to be the replica identity. If the table does not have any
> suitable key, then it can be set to replica identity “full”, which
> means the entire row becomes the key. When replica identity “full” 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 adheres to some
> of the restrictions that are enforced for primary keys. Internally, we
> follow a similar approach for supporting index scans within logical
> replication scope. If there are no such suitable indexes, the search
> on the subscriber side can be very inefficient, therefore replica
> identity “full” must only be used as a fallback if no other solution
> is possible. If a replica identity other than “full” is set on the
> publisher side, a replica identity comprising the same or fewer
> columns must also be set on the subscriber side. See REPLICA IDENTITY
> for details on how to set the replica identity. If a table without a
> replica identity is added to a publication that replicates UPDATE or
> DELETE operations then subsequent UPDATE or DELETE operations will
> cause an error on the publisher. INSERT operations can proceed
> regardless of any replica identity.
>
> ~~
>
> 2a.
> My previous review [1] (comment #2) was not fixed quite as suggested.
>
> Please change:
> "adheres to" --> "adhere to"
>
>
Oops, it seems I only got the "to" part of your suggestion and missed "s".

Done now.

> ~~
>
> 2b. should/must
>
> This should/must change was OK as it was before, because here it is only
> advice.
>
> Please change this back how it was:
> "must only be used as a fallback" --> "should only be used as a fallback"
>

Thanks, changed.

>
> ======
> src/backend/executor/execReplication.c
>
> 3. build_replindex_scan_key
>
> /*
> * Setup a ScanKey for a search in the relation 'rel' for a tuple 'key'
> that
> * is setup to match 'rel' (*NOT* idxrel!).
> *
> - * Returns whether any column contains NULLs.
> + * Returns how many columns must be used for the index scan.
> + *
>
> ~
>
> This should/must change does not seem quite right.
>
> SUGGESTION (reworded)
> Returns how many columns to use for the index scan.
>

Fixed.

(I wish we had a simpler process to incorporate such
comments.)

>
> ~~~
>
> 4. build_replindex_scan_key
>
> >
> > Based on the discussions below, I kept as-is. I really don't want to do
> unrelated
> > changes in this patch, as I also got several feedback for not doing it,
> >
>
> Hmm, although this code pre-existed I don’t consider this one as
> "unrelated changes" because the patch introduced the new "if
> (!AttributeNumberIsValid(table_attno))" which changed things. As I
> wrote to Amit yesterday [2] IMO it would be better to do the 'opttype'
> assignment *after* the potential 'continue' otherwise there is every
> chance that the assignment is just redundant. And if you move the
> assignment where it belongs, then you might as well declare the
> variable in the more appropriate place at the same time – i.e. with
> 'opfamily' declaration. Anyway, I've given my reason a couple of times
> now, so if you don't want to change it I won't about it debate
> anymore.
>

Alright, given both you and Amit [1] agree on this, I'll follow that.

>
> ======
> src/backend/replication/logical/relation.c
>
> 5. FindUsableIndexForReplicaIdentityFull
>
> + * XXX: There are no fundamental problems for supporting non-btree
> indexes.
> + * We mostly need to relax the limitations in
> RelationFindReplTupleByIndex().
> + * For partial indexes, the required changes are likely to be larger. If
> + * none of the tuples satisfy the expression for the index scan, we must
> + * fall-back to sequential execution, which might not be a good idea in
> some
> + * cases.
>
> The should/must change (the one in the XXX comment) does not seem quite
> right.
>
> SUGGESTION
> "we must fall-back to sequential execution" --> "we fallback to
> sequential execution"
>

fixed, thanks.

>
> ======
> .../subscription/t/032_subscribe_use_index.pl
>
> FYI, I get TAP test in error (Note - this is when only patch 0001 is
> appied)
>
> t/031_column_list.pl ............... ok
> t/032_subscribe_use_index.pl ....... 19/?
> # Failed test 'ensure subscriber has not used index with
> enable_indexscan=false'
> # at t/032_subscribe_use_index.pl line 806.
> # got: '1'
> # expected: '0'
> t/032_subscribe_use_index.pl ....... 21/? # Looks like you failed 1 test
> of 22.
> t/032_subscribe_use_index.pl ....... Dubious, test returned 1 (wstat 256,
> 0x100)
> Failed 1/22 subtests
> t/100_bugs.pl ...................... ok
>
> AFAICT there is a test case that is testing the patch 0002
> functionality even when patch 0002 is not applied yet.
>
>
Oops, I somehow managed to make the same rebase mistake. I fixed this,
and for next time I'll make sure that each commit passes the CI separately.
Sorry for the noise.

I'll attach the changes on v38 in the next e-mail.

Thanks,
Onder KALACI

[1]
https://www.postgresql.org/message-id/CAA4eK1LDcZgkbOBr1O0cN%3DCaXT-TKf-86fb2XuKbcbOzPXRk4w%40mail.gmail.com