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

From: Önder Kalacı <[email-masked]>
Date: Wed, 22 Feb 2023 17:24:03 +0300

Hi Peter, Amit, Shi Yu and all,

(I'm replying multiple reviews in this single reply, hope that's fine)

On Fri, Feb 17, 2023 at 5:57 PM Peter Smith <[email-masked]> wrote:
>
> FYI, I accidentally left this (v23) patch's TAP test
> t/032_subscribe_use_index.pl still lurking even after removing all
> other parts of this patch.
>
> In this scenario, the t/032 test gets stuck (build of latest HEAD)
>
> IIUC the patch is only meant to affect performance, so I expected this
> 032 test to work regardless of whether the rest of the patch is
> applied.
>
> Anyway,  it hangs every time for me. I didn't dig looking for the
> cause, but if it requires patched code for this new test to pass, I
> thought it indicates something wrong either with the test or something
> more sinister the new test has exposed. Maybe I am mistaken
>

>
> Sorry, probably the above was a false alarm. After a long time
> (minutes) the stuck test did eventually timeout with:
> t/032_subscribe_use_index.pl ....... # poll_query_until timed out
> executing this query:
> # select (idx_scan = 1) from pg_stat_all_indexes where indexrelname =
> 'test_replica_id_full_idx';
> # expecting this output:
> # t
> # last actual query output:
> # f
> # with stderr:
> t/032_subscribe_use_index.pl ....... Dubious, test returned 29 (wstat
> 7424, 0x1d00)
>
>
I can tell that this is the expected behavior. Majority of the tests do the
following:
- update/delete row on the source
- check pg_stat_all_indexes on the target

So, given that HEAD does not use any indexes, it is expected that the tests
would
wait until poll_query_until timeout.  That's why, I do not see/expect any
problems on
HEAD. I run the test file by removing the poll_query_until for the index
scan counts,
and all finished properly.

I think such a case exists. I tried the following cases based on v23 patch.

As I noted in the earlier reply, I created another patch, which optionally
gives the ability to
disable index scans on the subscription level for the replica identity
full case.

That is the second patch attached in this mail,
named v25_0001_optionally_disable_index.patch.

Here are some review comments for patch v23.

Thanks Peter, see the following reply:

======
> General

> 1.
> IIUC the previous logic for checking "cost" comparisons and selecting
> the "cheapest" strategy is no longer present in the latest patch.

In that case, I think there are some leftover stale comments that need

changing. For example,

1a. Commit message:
> "let the planner sub-modules compare the costs of index versus
> sequential scan and choose the cheapest."
> 1b. Commit message:
> "Finally, pick the cheapest `Path` among."
> 1c. FindLogicalRepUsableIndex function:
> + * We are looking for one more opportunity for using an index. If
> + * there are any indexes defined on the local relation, try to pick
> + * the cheapest index.

Makes sense, the commit message and function messages should reflect
the new logic. I went over the patch with a detailed look for this.

======
> doc/src/sgml/logical-replication.sgml
> If replica identity "full" is used, indexes can be used on the
> subscriber side for seaching the rows. The index should be btree,
> non-partial and have at least one column reference (e.g., should not
> consists of only expressions). If there are no suitable indexes, the
> search on the subscriber side is very inefficient and should only be
> used as a fallback if no other solution is possible

2a.
> Fixed typo "seaching", and minor rewording.

SUGGESTION
> When replica identity "full" is specified, indexes can be used on the
> subscriber side for searching the rows. These indexes should be btree,
> non-partial and have at least one column reference (e.g., should not
> consist of only expressions). If there are no such suitable indexes,
> the search on the subscriber side can be very inefficient, therefore
> replica identity "full" should only be used as a fallback if no other
> solution is possible.

I like your suggestion, updated

2b.
> I know you are just following some existing text here, but IMO this
> should probably refer to replica identity <literal>FULL</literal>
> instead of "full".

I guess that works, I don't have any preference / knowledge on this.

>
> ======
> src/backend/executor/execReplication.c
> 3. IdxIsRelationIdentityOrPK
> +/*
> + * Given a relation and OID of an index, returns true if
> + * the index is relation's primary key's index or
> + * relaton's replica identity index.
> + *
> + * Returns false otherwise.
> + */
> +static bool
> +IdxIsRelationIdentityOrPK(Relation rel, Oid idxoid)
> +{
> + Assert(OidIsValid(idxoid));
> +
> + if (RelationGetReplicaIndex(rel) == idxoid ||
> + RelationGetPrimaryKeyIndex(rel) == idxoid)
> + return true;
> +
> + return false;
> 3a.
> Comment typo "relaton"

fixed

3b.
> Code could be written using single like below if you wish (but see #2c)
> return RelationGetReplicaIndex(rel) == idxoid ||
> RelationGetPrimaryKeyIndex(rel) == idxoid;
> ~
> 3c.
> Actually, RelationGetReplicaIndex and RelationGetPrimaryKeyIndex
> implementations are very similar so it seemed inefficient to be
> calling both of them. IMO it might be better to just make a new
> relcache function IdxIsRelationIdentityOrPK(Relation rel, Oid idxoid).
> This implementation will be similar to those others, but now you need
> only to call the workhorse RelationGetIndexList *one* time.
> ~~~

Regarding (3c), RelationGetIndexList is only called once, when
!relation->rd_indexvalid.
So, there seems not to be a necessary case for merging two functions to me.
Also, I'd rather keep both functions, as I know that some extensions rely
on these functions separately.

Regarding (3b), it makes sense, applied.

> 4. RelationFindReplTupleByIndex
>   bool found;
> + TypeCacheEntry **eq = NULL; /* only used when the index is not repl.
> ident
> + * or pkey */
> + bool idxIsRelationIdentityOrPK;
> If you change the comment to say "RI" instead of "repl. Ident" then it
> can all fit on one line, which would be an improvement.

Done, also changed pkey to PK as this seems to be used throughout the code.

======
> src/backend/replication/logical/relation.c
> 5.
>  #include "replication/logicalrelation.h"
>  #include "replication/worker_internal.h"
> +#include "optimizer/cost.h"
>  #include "utils/inval.h"
> Can that #include be added in alphabetical order like the others or not?

Sure, it seems like I intended to do it, but made a small mistake :)

6. logicalrep_partition_open
> + /*
> + * Finding a usable index is an infrequent task. It occurs when an
> + * operation is first performed on the relation, or after invalidation of
> + * of the relation cache entry (such as ANALYZE or CREATE/DROP index on
> + * the relation).
> + */
> + entry->usableIndexOid = FindLogicalRepUsableIndex(partrel, remoterel);
> +
> Typo "of of the relation"

Fixed

7. FindUsableIndexForReplicaIdentityFull
> +static Oid
> +FindUsableIndexForReplicaIdentityFull(Relation localrel)
> +{
> + MemoryContext usableIndexContext;
> + MemoryContext oldctx;
> + Oid usableIndex;
> + Oid idxoid;
> + List *indexlist;
> + ListCell   *lc;
> + Relation        indexRelation;
> + IndexInfo  *indexInfo;
> + bool is_btree;
> + bool is_partial;
> + bool is_only_on_expression;
> It looks like some of these variables are only used within the scope
> of the foreach loop, so I think that is where they should be declared.

makes sense, done

8.
> + usableIndex = InvalidOid;
> Might as well do that assignment at the declaration.

done

9. FindLogicalRepUsableIndex
> + /*
> + * Simple case, we already have a primary key or a replica identity index.
> + *
> + * Note that we do not use index scans below when enable_indexscan is
> + * false. Allowing primary key or replica identity even when index scan is
> + * disabled is the legacy behaviour. So we hesitate to move the below
> + * enable_indexscan check to be done earlier in this function.
> + */
> + idxoid = GetRelationIdentityOrPK(localrel);
> + if (OidIsValid(idxoid))
> + return idxoid;
> +
> + /* If index scans are disabled, use a sequential scan */
> + if (!enable_indexscan)
> + return InvalidOid;
> IMO that "Note" really belongs with the if (!enable)indexscan) more like
> this:
> SUGGESTION
> /*
> * Simple case, we already have a primary key or a replica identity index.
> */
> idxoid = GetRelationIdentityOrPK(localrel);
> if (OidIsValid(idxoid))
> return idxoid;
> /*
> * If index scans are disabled, use a sequential scan.
> *
> * Note we hesitate to move this check to earlier in this function
> * because allowing primary key or replica identity even when index scan
> * is disabled is the legacy behaviour.
> */
> if (!enable_indexscan)
> return InvalidOid;

makes sense, moved

> ======
> src/backend/replication/logical/worker.c
> 10. get_usable_indexoid
> +/*
> + * Decide whether we can pick an index for the relinfo (e.g., the
> relation)
> + * we're actually deleting/updating from. If it is a child partition of
> + * edata->targetRelInfo, find the index on the partition.
> + *
> + * Note that if the corresponding relmapentry has invalid usableIndexOid,
> + * the function returns InvalidOid.
> + */
> "(e.g., the relation)" --> "(i.e. the relation)"

fixed

Thanks for your patch. Here are some comments.

Thanks Shi Yu, see my reply below

1.
> I noticed that get_usable_indexoid() is called in
> apply_handle_update_internal()
> and apply_handle_delete_internal() to get the usable index. Could
> usableIndexOid
> be a parameter of these two functions? Because we have got the
> LogicalRepRelMapEntry when calling them and if we do so, we can get
> usableIndexOid without get_usable_indexoid(). Otherwise for partitioned
> tables,
> logicalrep_partition_open() is called in get_usable_indexoid() and
> searching
> the entry via hash_search() will increase cost.

I think I cannot easily follow this comment. We call
logicalrep_partition_open()
because if an update/delete is on a partitioned table, we should find the
corresponding local index on the partition itself. edata->targetRel points
to the
partitioned table, and we map it to the partition inside
get_usable_indexoid().

Overall, I cannot see how we can avoid the call
to logicalrep_partition_open().
Can you please explain a little further?

Note that logicalrep_partition_open() is cheap for the cases where there is
no
invalidations (which is probably most of the time)

2.
> +                        * This attribute is an expression, and
> +                        * SuitableIndexPathsForRepIdentFull() was called
> earlier when the
> +                        * index for subscriber was selected. There, the
> indexes
> +                        * comprising *only* expressions have already been
> eliminated.

The comment looks need to be updated:
> SuitableIndexPathsForRepIdentFull
> ->
> FindUsableIndexForReplicaIdentityFull

Yes, updated.

3.
>         /* Build scankey for every attribute in the index. */
> -       for (attoff = 0; attoff <
> IndexRelationGetNumberOfKeyAttributes(idxrel); attoff++)
> +       for (index_attoff = 0; index_attoff <
> IndexRelationGetNumberOfKeyAttributes(idxrel);
> +                index_attoff++)
>         {
> Should the comment be changed? Because we skip the attributes that are
> expressions.

makes sense

4.
> +                       Assert(RelationGetReplicaIndex(rel) !=
> RelationGetRelid(idxrel) &&
> +                                  RelationGetPrimaryKeyIndex(rel) !=
> RelationGetRelid(idxrel));
> Maybe we can call the new function idxIsRelationIdentityOrPK()?

Makes sense, becomes easier to understand.

Here are some comments on the test cases.

1. in test case "SUBSCRIPTION RE-CALCULATES INDEX AFTER CREATE/DROP INDEX"
> +# now, ingest more data and create index on column y which has higher
> cardinality
> +# so that the future commands use the index on column y
> +$node_publisher->safe_psql('postgres',
> +       "INSERT INTO test_replica_id_full SELECT 50, i FROM
> generate_series(0,3100)i;");
> +$node_subscriber->safe_psql('postgres',
> +       "CREATE INDEX test_replica_id_full_idy ON
> test_replica_id_full(y)");
> We don't pick the cheapest index in the current patch, so should we modify
> this
> part of the test?

I think I already changed that test. I kept the test so that we still make
sure that even if we
create/drop indexes, we do not mess anything. I agree that the wording /
comments were
stale.

Can you check if it looks better now?

> BTW, the following comment in FindLogicalRepUsableIndex() need to be
> changed,
> too.
> +                * We are looking for one more opportunity for using an
> index. If
> +                * there are any indexes defined on the local relation,
> try to pick
> +                * the cheapest index.

makes sense, Peter also had a similar comment, fixed.

2. Is there any reasons why we need the test case "SUBSCRIPTION USES INDEX
> WITH
> DROPPED COLUMNS"? Has there been a problem related to dropped columns
> before?

Not really, but dropped columns are tricky in general. As far as I know,
those columns
continue to exist in pg_attribute, which might cause some edge cases. So, I
wanted to
have coverage for that.

> 3. in test case "SUBSCRIPTION USES INDEX ON PARTITIONED TABLES"
> +# deletes rows and moves between partitions
> +$node_publisher->safe_psql('postgres',
> +       "DELETE FROM users_table_part WHERE user_id = 1 and value_1 = 1;");
> +$node_publisher->safe_psql('postgres',
> +       "DELETE FROM users_table_part WHERE user_id = 12 and value_1 =
> 12;");
> "moves between partitions" in the comment seems wrong.

Yes, probably copy & paste error from the UPDATE test

4. in test case "SUBSCRIPTION DOES NOT USE INDEXES WITH ONLY EXPRESSIONS"
> +# update 2 rows
> +$node_publisher->safe_psql('postgres',
> +       "UPDATE people SET firstname = 'Nan' WHERE firstname =
> 'first_name_1';");
> +$node_publisher->safe_psql('postgres',
> +       "UPDATE people SET firstname = 'Nan' WHERE firstname =
> 'first_name_2' AND lastname = 'last_name_2';");
> +
> +# make sure the index is not used on the subscriber
> +$result = $node_subscriber->safe_psql('postgres',
> +       "select idx_scan from pg_stat_all_indexes where indexrelname =
> 'people_names'");
> +is($result, qq(0), 'ensure subscriber tap_sub_rep_full updates two rows
> via seq. scan with index on expressions');
> +
> I think it would be better to call wait_for_catchup() before the check
> because
> we want to check the index is NOT used. Otherwise the check may pass
> because the
> rows have not yet been updated on subscriber.

that's right, added

5. in test case "SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN"
> +# show that index is not used even when enable_indexscan=false
> +$result = $node_subscriber->safe_psql('postgres',
> +       "select idx_scan from pg_stat_all_indexes where indexrelname =
> 'test_replica_id_full_idx'");
> +is($result, qq(0), 'ensure subscriber has not used index with
> enable_indexscan=false');
> Should we remove the word "even" in the comment?

done

6.
> In each test case we re-create publications, subscriptions, and tables.
> Could we
> create only one publication and one subscription at the beginning, and use
> them
> in all test cases? I think this can save some time running the test file.

I'd rather keep as-is for (a) simplicity (b) other test files seem to have
similar patterns.

Do you think strongly that we should change the test file? It could make
debugging
the tests harder as well.

Tom, Andres, or others, do you have any suggestions on how to move
> forward with this patch?

Yes, happy to hear any feedback for the attached patch(es).

Thanks,
Onder KALACI

Önder Kalacı <[email-masked]>, 21 Şub 2023 Sal, 17:25 tarihinde şunu
yazdı:

> Hi Amit, all
>
> Amit Kapila <[email-masked]>, 15 Şub 2023 Çar, 07:37 tarihinde
> şunu yazdı:
>
>> On Wed, Feb 15, 2023 at 9:23 AM [email-masked]
>> <[email-masked]> wrote:
>> >
>> > On Sat, Feb 4, 2023 7:24 PM Amit Kapila <[email-masked]>
>> wrote:
>> > >
>> > > On Thu, Feb 2, 2023 at 2:03 PM Önder Kalacı <[email-masked]>
>> wrote:
>> > > >
>> > > > On v23, I dropped the planner support for picking the index.
>> Instead, it simply
>> > > > iterates over the indexes and picks the first one that is suitable.
>> > > >
>> > > > I'm currently thinking on how to enable users to override this
>> decision.
>> > > > One option I'm leaning towards is to add a syntax like the
>> following:
>> > > >
>> > > > ALTER SUBSCRIPTION .. ALTER TABLE ... SET INDEX ...
>> > > >
>> > > > Though, that should probably be a seperate patch. I'm going to work
>> > > > on that, but still wanted to share v23 given picking the index
>> sounds
>> > > > complementary, not strictly required at this point.
>> > > >
>> > >
>> > > I agree that it could be a separate patch. However, do you think we
>> > > need some way to disable picking the index scan? This is to avoid
>> > > cases where sequence scan could be better or do we think there won't
>> > > exist such a case?
>> > >
>> >
>> > I think such a case exists. I tried the following cases based on v23
>> patch.
>> >
>> ...
>> > # Result
>> > The time executing update (the average of 3 runs is taken, the unit is
>> > milliseconds):
>> >
>> > +--------+---------+---------+
>> > |        | patched |  master |
>> > +--------+---------+---------+
>> > | case 1 | 3933.68 | 1298.32 |
>> > | case 2 | 1803.46 | 1294.42 |
>> > | case 3 | 1380.82 | 1299.90 |
>> > | case 4 | 1042.60 | 1300.20 |
>> > | case 5 |  691.69 | 1297.51 |
>> > | case 6 |  578.50 | 1300.69 |
>> > | case 7 |  566.45 | 1302.17 |
>> > +--------+---------+---------+
>> >
>> > In case 1~3, there's an overhead after applying the patch. In other
>> cases, the
>> > patch improved the performance. As more duplicate values, the greater
>> the
>> > overhead after applying the patch.
>> >
>>
>> I think this overhead seems to be mostly due to the need to perform
>> tuples_equal multiple times for duplicate values. I don't know if
>> there is any simple way to avoid this without using the planner stuff
>> as was used in the previous approach. So, this brings us to the
>> question of whether just providing a way to disable/enable the use of
>> index scan for such cases is sufficient or if we need any other way.
>>
>> Tom, Andres, or others, do you have any suggestions on how to move
>> forward with this patch?
>>
>>
> Thanks for the feedback and testing. Due to personal circumstances,
> I could not reply the thread in the last 2 weeks, but I'll be more active
> going forward.
>
>  I also agree that we should have a way to control the behavior.
>
> I created another patch (v24_0001_optionally_disable_index.patch) which
> can be applied
> on top of v23_0001_use_index_on_subs_when_pub_rep_ident_full.patch.
>
> The new patch adds a new *subscription_parameter* for both CREATE and
> ALTER subscription
> named: *enable_index_scan*. The setting is valid only when REPLICA
> IDENTITY is full.
>
> What do you think about such a patch to control the behavior? It does not
> give a per-relation
> level of control, but still useful for many cases.
>
> (Note that I'll be working on the other feedback in the email thread,
> wanted to send this earlier
> to hear some early thoughts on v24_0001_optionally_disable_index.patch).
>
>
>

---

From: Amit Kapila <[email-masked]>
Date: Wed, 1 Mar 2023 17:16:42 +0530

On Wed, Feb 22, 2023 at 7:54 PM Önder Kalacı <[email-masked]> wrote:

Few comments:
===============
1.
+   identity.  When replica identity <literal>FULL</literal> is specified,
+   indexes can be used on the subscriber side for searching the rows. These
+   indexes should be btree,

Why only btree and not others like a hash index? Also, there should be
some comments in FindUsableIndexForReplicaIdentityFull() to explain
the choices.

2.
- * This is not generic routine, it expects the idxrel to be replication
- * identity of a rel and meet all limitations associated with that.
+ * This is not a generic routine - it expects the idxrel to be an index
+ * that planner would choose if the searchslot includes all the columns
+ * (e.g., REPLICA IDENTITY FULL on the source).
  */
-static bool
+static int

This comment is not clear to me. Which change here makes the
expectation like that? Which planner function/functionality are you
referring to here?

3.
+/*
+ * Given a relation and OID of an index, returns true if
+ * the index is relation's primary key's index or
+ * relation's replica identity index.

It seems the line length is a bit off in the above comments. There
could be a similar mismatch in other places. You might want to run
pgindent.

4.
+}
+
+
+/*
+ * Returns an index oid if there is an index that can be used

Spurious empty line.

5.
- /*
- * We are in error mode so it's fine this is somewhat slow. It's better to
- * give user correct error.
- */
- if (OidIsValid(GetRelationIdentityOrPK(rel->localrel)))
+ /* Give user more precise error if possible. */
+ if (OidIsValid(rel->usableIndexOid))
  {
  ereport(ERROR,
  (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),

Is this change valid? I mean this could lead to the error "publisher
did not send replica identity column expected by the logical
replication target relation" when it should have given an error:
"logical replication target relation \"%s.%s\" has neither REPLICA
IDENTITY index nor PRIMARY ...

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Thu, 2 Mar 2023 12:30:20 +0300

Hi,

> Few comments:

I updated the comment(s).

For a more technical reference, we have these restrictions, because we rely
on
RelationFindReplTupleByIndex() which is designed to handle PK/RI. And,
RelationFindReplTupleByIndex() is written in a way that only expects
indexes with these limitations.

In order to keep the changes as small as possible, I refrained from
relaxing this
limitation for now. I'm definitely up to working on this for relaxing these
limitations, and practically allowing more cases for non-unique indexes.

> expectation like that? Which planner function/functionality are you

Ops, planner related comments are definitely stale. As you might
remember, in the earlier iterations of this patch, we had some
planner functions to pick  indexes for us.

Anyway, I think even for that version of the patch, this comment was
wrong. Updated now, does that look better?

> could be a similar mismatch in other places. You might want to run

Makes sense, run pgindent. But it didn't fix this specific instance
automatically,
I changed that manually.

fixed

> IDENTITY index nor PRIMARY ...
 Hmm, that's right, we'd get a wrong error message.

I spent quite a bit of time trying to understand whether we'd
need anything additional after this patch regarding this function,
but my current understanding is that we should just leave the
check as-is. It is mainly because when REPLICA IDENTITY is
full, there is no need to check anything further (other than the
check that is at the bottom of the function)

Attached are both patches: the main patch, and the patch that
optionally disables the index scans. Let's discuss the necessity
for the second patch in the lights of the data we collect with
some more tests.

Thanks,
Onder

---

From: Amit Kapila <[email-masked]>
Date: Thu, 2 Mar 2023 17:44:44 +0530

On Thu, Mar 2, 2023 at 3:00 PM Önder Kalacı <[email-masked]> wrote:
> For a more technical reference, we have these restrictions, because we rely on
> In order to keep the changes as small as possible, I refrained from relaxing this

See, I think I understand why partial/expression indexes can't be
supported. It seems to me that because the required tuple may not
satisfy the expression and that won't work for our case. But what are
other limitations you see due to which we can't support other index
types for non-unique indexes? Is it just a matter of testing other
index types or there is something more to it, if so, we should add
comments so that they can be supported in the future if it is feasible
to do so.

> optionally disables the index scans.
>

Both the patches are numbered 0001. It would be better to number them
as 0001 and 0002.

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Thu, 2 Mar 2023 18:22:44 +0300

Hi,

>  Is it just a matter of testing other
> index types
>

Yes, there are more to it. build_replindex_scan_key()
only works for btree indexes, as it does BTEqualStrategyNumber.

I might expect a few more limitations like that. I added comment
in the code (see FindUsableIndexForReplicaIdentityFull)

or there is something more to it, if so, we should add
> comments so that they can be supported in the future if it is feasible

I really don't see any fundamental issues regarding expanding the
support for more index types, it is just some more coding & testing.

And, I can (and willing to) work on that as a follow-up. I explicitly
try to keep this patch as small as possible.

> Both the patches are numbered 0001. It would be better to number them
Alright, attached v27_0001_use_index_on_subs_when_pub_rep_ident_full.patch
and
v27_0002_use_index_on_subs_when_pub_rep_ident_full.patch.

I also added one more test which Andres asked me on a private chat
(Testcase start: SUBSCRIPTION USES INDEX WITH PUB/SUB different data).

Thanks,
Onder

---

From: Peter Smith <[email-masked]>
Date: Fri, 3 Mar 2023 13:10:47 +1100

FYI,

After applying only the 0001 patch I received a TAP test error.

t/032_subscribe_use_index.pl ....... 1/? # Tests were run but no plan
was declared and done_testing() was not seen.
t/032_subscribe_use_index.pl ....... Dubious, test returned 29 (wstat
7424, 0x1d00)
All 1 subtests passed
t/100_bugs.pl ...................... ok

More details:

2023-03-03 12:45:45.382 AEDT [9931] 032_subscribe_use_index.pl LOG:
statement: CREATE INDEX test_replica_id_full_idx ON
test_replica_id_full(x)
2023-03-03 12:45:45.423 AEDT [9937] 032_subscribe_use_index.pl LOG:
statement: CREATE SUBSCRIPTION tap_sub_rep_full CONNECTION 'port=56538
host=/tmp/zWyRQnOa1a dbname=postgres application_name=tap_sub'
PUBLICATION tap_pub_rep_full WITH (enable_index_scan = false)
2023-03-03 12:45:45.423 AEDT [9937] 032_subscribe_use_index.pl ERROR:
unrecognized subscription parameter: "enable_index_scan"
2023-03-03 12:45:45.423 AEDT [9937] 032_subscribe_use_index.pl
STATEMENT:  CREATE SUBSCRIPTION tap_sub_rep_full CONNECTION
'port=56538 host=/tmp/zWyRQnOa1a dbname=postgres
application_name=tap_sub' PUBLICATION tap_pub_rep_full WITH
(enable_index_scan = false)
2023-03-03 12:45:45.532 AEDT [9834] LOG:  received immediate shutdown request
2023-03-03 12:45:45.533 AEDT [9834] LOG:  database system is shut down

~~

The patches 0001 and 0002 seem to have accidentally blended together
because AFAICT the error is because patch 0001 is testing something
that is not available until 0002.

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

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 3 Mar 2023 03:40:31 +0000

On Thursday, March 2, 2023 11:23 PM Önder Kalacı <[email-masked]>  wrote:

> Alright, attached v27_0001_use_index_on_subs_when_pub_rep_ident_full.patch and 

Thanks for updating the patch. I think this patch can bring noticeable
performance improvements in some use cases.

And here are few comments after reading the patch.

1.
+	usableIndexContext = AllocSetContextCreate(CurrentMemoryContext,
+											   "usableIndexContext",
+											   ALLOCSET_DEFAULT_SIZES);
+	oldctx = MemoryContextSwitchTo(usableIndexContext);
+
+	/* Get index list of the local relation */
+	indexlist = RelationGetIndexList(localrel);
+	Assert(indexlist != NIL);
+
+	foreach(lc, indexlist)

Is it necessary to create a memory context here ? I thought the memory will be
freed after this apply action soon.

2.

+			/*
+			 * Furthermore, because primary key and unique key indexes can't
+			 * include expressions we also sanity check the index is neither
+			 * of those kinds.
+			 */
+			Assert(!IdxIsRelationIdentityOrPK(rel, idxrel->rd_id));

It seems you mean "replica identity key" instead of "unique key" in the comments.

3.
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
...
+extern bool IsIndexOnlyOnExpression(IndexInfo *indexInfo);

The definition function seems better to be placed in execReplication.c

4.

+extern Oid GetRelationIdentityOrPK(Relation rel);

The function is only used in relation.c, so we can make it a static
function.

5.

+	/*
+	 * If index scans are disabled, use a sequential scan.
+	 *

Since the document of enable_indexscan says "Enables or disables the query
planner's use of index-scan plan types. The default is on.", and we don't use
planner here, so I am not sure should we allow/disallow index scan in apply
worker based on this GUC.

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Fri, 3 Mar 2023 11:49:21 +0530

> v27_0002_use_index_on_subs_when_pub_rep_ident_full.patch.

Few comments on 0001
====================
1.
+   such suitable indexes, the search on the subscriber s ide can be
very inefficient,

unnecessary space in 'side'

2.
-   identity.  If the table does not have any suitable key, then it can be set
-   to replica identity <quote>full</quote>, which means the entire row becomes
-   the key.  This, however, is very inefficient and should only be used as a
-   fallback if no other solution is possible.  If a replica identity other
+   identity. When replica identity <literal>FULL</literal> is specified,
+   indexes can be used on the subscriber side for searching the rows.

I think it is better to retain the first sentence (If the table does
not ... entire row becomes the key.) as that says what will be part of
the key.

3.
-   comprising the same or fewer columns must also be set on the subscriber
-   side.  See <xref linkend="sql-altertable-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 <command>UPDATE</command>
+   comprising the same or fewer columns must also be set on the
subscriber side.
+   See <xref linkend="sql-altertable-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 <command>UPDATE</command>

I don't see any change in this except line length. If so, I don't
think we should change it as part of this patch.

4.
 /*
  * 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 should be used for the index scan.
+ *
+ * This is not generic routine, it expects the idxrel to be
+ * a btree, non-partial and have at least one column
+ * reference (e.g., should not consist of only expressions).
  *
- * This is not generic routine, it expects the idxrel to be replication
- * identity of a rel and meet all limitations associated with that.
+ * By definition, replication identity of a rel meets all
+ * limitations associated with that. Note that any other
+ * index could also meet these limitations.

The comment changes look quite asymmetric to me. Normally, we break
the line if the line length goes beyond 80 cols. Please check and
change other places in the patch if they have a similar symptom.

5.
+ * There are no fundamental problems for supporting non-btree
+ * and/or partial indexes.

Can we mention partial indexes in the above comment? It seems to me
that because the required tuple may not satisfy the expression (in the
case of partial indexes) it may not be easy to support it.

6.
build_replindex_scan_key()
{
...
+ for (index_attoff = 0; index_attoff <
IndexRelationGetNumberOfKeyAttributes(idxrel);
+ index_attoff++)
...
...
+#ifdef USE_ASSERT_CHECKING
+ IndexInfo  *indexInfo = BuildIndexInfo(idxrel);
+
+ Assert(!IsIndexOnlyOnExpression(indexInfo));
+#endif
...
}

We can avoid building index info multiple times. This can be either
checked at the beginning of the function outside attribute offset loop
or we can probably cache it. I understand this is for assert builds
but seems easy to avoid it doing multiple times and it also looks odd
to do it multiple times for the same index.

7.
- /* Build scankey for every attribute in the index. */
- for (attoff = 0; attoff <
IndexRelationGetNumberOfKeyAttributes(idxrel); attoff++)
+ /* Build scankey for every non-expression attribute in the index. */
+ for (index_attoff = 0; index_attoff <
IndexRelationGetNumberOfKeyAttributes(idxrel);
+ index_attoff++)
  {
  Oid operator;
  Oid opfamily;
+ Oid optype = get_opclass_input_type(opclass->values[index_attoff]);
  RegProcedure regop;
- int pkattno = attoff + 1;
- int mainattno = indkey->values[attoff];
- Oid optype = get_opclass_input_type(opclass->values[attoff]);
+ int table_attno = indkey->values[index_attoff];

I don't think here we need to change variable names if we retain
mainattno as it is instead of changing it to table_attno. The current
naming doesn't seem bad for the current usage of the patch.

8.
+ TypeCacheEntry **eq = NULL; /* only used when the index is not RI or PK */

Normally, we don't add such comments as the usage is quite obvious by
looking at the code.

-- 
With Regards,
Amit Kapila.

---

From: vignesh C <[email-masked]>
Date: Fri, 3 Mar 2023 12:10:35 +0530

> (Testcase start: SUBSCRIPTION USES INDEX WITH PUB/SUB different data).

Thanks for the patch. Few comments:
1) We are currently calling RelationGetIndexList twice, once in
FindUsableIndexForReplicaIdentityFull function and in the caller too,
we could avoid one of the calls by passing the indexlist to the
function or removing the check here, index list check can be handled
in FindUsableIndexForReplicaIdentityFull.
+       if (remoterel->replident == REPLICA_IDENTITY_FULL &&
+               RelationGetIndexList(localrel) != NIL)
+       {
+               /*
+                * If we had a primary key or relation identity with a
unique index,
+                * we would have already found and returned that oid.
At this point,
+                * the remote relation has replica identity full and
we have at least
+                * one local index defined.
+                *
+                * We are looking for one more opportunity for using
an index. If
+                * there are any indexes defined on the local
relation, try to pick
+                * a suitable index.
+                *
+                * The index selection safely assumes that all the
columns are going
+                * to be available for the index scan given that
remote relation has
+                * replica identity full.
+                */
+               return FindUsableIndexForReplicaIdentityFull(localrel);
+       }
+

2) Copyright year should be mentioned as 2023
diff --git a/src/test/subscription/t/032_subscribe_use_index.pl
b/src/test/subscription/t/032_subscribe_use_index.pl
new file mode 100644
index 0000000000..db0a7ea2a0
--- /dev/null
+++ b/src/test/subscription/t/032_subscribe_use_index.pl
@@ -0,0 +1,861 @@
+# Copyright (c) 2021-2022, PostgreSQL Global Development Group
+
+# Test logical replication behavior with subscriber uses available index
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+

3) Many of the tests are using the same tables, we need not
drop/create publication/subscription for each of the team, we could
just drop and create required indexes and verify the update/delete
statements.
+# ====================================================================
+# Testcase start: SUBSCRIPTION USES INDEX
+#
+# Basic test where the subscriber uses index
+# and only updates 1 row and deletes
+# 1 other row
+#
+
+# create tables pub and sub
+$node_publisher->safe_psql('postgres',
+       "CREATE TABLE test_replica_id_full (x int)");
+$node_publisher->safe_psql('postgres',
+       "ALTER TABLE test_replica_id_full REPLICA IDENTITY FULL;");
+$node_subscriber->safe_psql('postgres',
+       "CREATE TABLE test_replica_id_full (x int)");
+$node_subscriber->safe_psql('postgres',
+       "CREATE INDEX test_replica_id_full_idx ON test_replica_id_full(x)");

+# ====================================================================
+# Testcase start: SUBSCRIPTION CREATE/DROP INDEX WORKS WITHOUT ISSUES
+#
+# This test ensures that after CREATE INDEX, the subscriber can automatically
+# use one of the indexes (provided that it fulfils the requirements).
+# Similarly, after DROP index, the subscriber can automatically switch to
+# sequential scan
+
+# create tables pub and sub
+$node_publisher->safe_psql('postgres',
+       "CREATE TABLE test_replica_id_full (x int NOT NULL, y int)");
+$node_publisher->safe_psql('postgres',
+       "ALTER TABLE test_replica_id_full REPLICA IDENTITY FULL;");
+$node_subscriber->safe_psql('postgres',
+       "CREATE TABLE test_replica_id_full (x int NOT NULL, y int)");

4) These additional blank lines can be removed to keep it consistent:
4.a)
+# Testcase end: SUBSCRIPTION DOES NOT USE PARTIAL INDEX
+# ====================================================================
+
+
+# ====================================================================
+# Testcase start: SUBSCRIPTION DOES NOT USE INDEXES WITH ONLY EXPRESSIONS

4.b)
+# Testcase end: Unique index that is not primary key or replica identity
+# ====================================================================
+
+
+
+# ====================================================================
+# Testcase start: SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN

Regards,
Vignesh

---

From: Önder Kalacı <[email-masked]>
Date: Fri, 3 Mar 2023 10:39:43 +0300

Hi Hou zj, all

>                 "usableIndexContext",
>                 ALLOCSET_DEFAULT_SIZES);

Yeah, probably not useful anymore, removed.

In the earlier versions of this patch, this code block was relying on some
planner functions. In that case, it felt safer to use a memory context. Now,
it seems useless.

> It seems you mean "replica identity key" instead of "unique key" in the

Right, I fixed this comment. Though, are you mentioning multiple comment*s*?
I couldn't
see any other in the patch. Let me know if you see.

> The definition function seems better to be placed in execReplication.c

Hmm, why do you think so? IsIndexOnlyOnExpression() is used in
logical/relaton.c, and used for an assertion on execReplication.c

I think it is better suited for relaton.c, but let me know about
your perspective as well.

> The function is only used in relation.c, so we can make it a static
In the recent iteration of the patch (I think v27), we also use this
function in check_relation_updatable() in logical/worker.c.

One could argue that we can move the definition back to worker.c,
but it feels better suited for in relation.c, as the perimeter of the
function
is a *Rel*, and the function is looking for a property of a relation.

Let me know if you think otherwise, I don't have strong opinions
on this.

> worker based on this GUC.
Given Amit's suggestion on [1], I'm planning to drop this check altogether,
and
rely on table storage parameters.

(I'll incorporate these changes with a patch that I'm going to reply
to Peter's e-mail).

Thanks,
Onder

[1]:
https://www.postgresql.org/message-id/CAA4eK1KP-sV4aER51J-2mELjNzq_zVSLf1%2BW90Vu0feo-thVNA%40mail.gmail.com

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 3 Mar 2023 08:46:20 +0000

Dear Önder,

Thanks for updating the patch!
I played with your patch and I confirmed that parallel apply worker and tablesync worker
could pick the index in typical case.

Followings are comments for v27-0001. Please ignore if it is fixed in newer one.

execReplication.c

```
```

I think that single line comments should not terminated by ".".

```
+       /* There should always be at least one attribute for the index scan. */
```

Same as above.

```
```

I may misunderstand, but the condition of usable index has alteady been checked
when the oid was set but anyway the you confirmed the condition again before
really using that, right?
So is it OK not to check another assumption that the index is b-tree, non-partial,
and one column reference?

IIUC we can do that by adding new function like IsIndexUsableForReplicaIdentityFull()
that checks these condition, and then call at RelationFindReplTupleByIndex() if
idxIsRelationIdentityOrPK is false.

032_subscribe_use_index.pl

```
+# Testcase start: SUBSCRIPTION CREATE/DROP INDEX WORKS WITHOUT ISSUES
...
+# Testcase end: SUBSCRIPTION RE-CALCULATES INDEX AFTER CREATE/DROP INDEX
```

There is still non-consistent case.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Önder Kalacı <[email-masked]>
Date: Fri, 3 Mar 2023 12:32:39 +0300

Hi Amit, all

> Few comments on 0001

Fixed in v28

> not ... entire row becomes the key.) as that says what will be part of
right, that sentence looks useful, added back.

> think we should change it as part of this patch.
Yes, fixed. But the first line (starting with See <xref ..) still shows as
if it is changed,
probably because its line has changed.  I couldn't make that line show as
it had not
changed.

> change other places in the patch if they have a similar symptom.

Went over the patch, and expanded each line ~80 chars.

I'm guessing 80 is not the hard limit, in some cases I went over 81-82.

> case of partial indexes) it may not be easy to support it.

Expanded the comment and explained the differences a little further.

> to do it multiple times for the same index.

Applied your suggestions. Although I do not have strong opinions, I think
that
it was easier to follow with building the indexInfo for each iteration.

> naming doesn't seem bad for the current usage of the patch.

Hmm, I'm actually not convinced that the variable naming on HEAD is good for
the current patch. The main difference is that now we allow indexes like:
   * CREATE INDEX idx ON table(foo(col), col_2)*

(See # Testcase start: SUBSCRIPTION CAN USE INDEXES WITH
EXPRESSIONS AND COLUMNS)

In such indexes, we could skip the attributes of the index. So, skey_attoff
is not
equal to  index_attoff anymore. So, calling out this explicitly via the
variable names
seems more robust to me. Plus, mainattno sounded vague to me when I first
read
this function.

So, unless you have strong objections, I'm leaning towards having variable
names more explicit. I'm also open for suggestions if you think the names
I picked is not clear enough.

> Normally, we don't add such comments as the usage is quite obvious by
Sure, I also don't see much value for it, removed.

Attached v29 for this review. Note that I'll be working on the disable
index scan changes after
I reply to some of the other pending reviews.

Thanks,
Onder

---

From: Amit Kapila <[email-masked]>
Date: Fri, 3 Mar 2023 16:04:56 +0530

On Fri, Mar 3, 2023 at 3:02 PM Önder Kalacı <[email-masked]> wrote:
>     CREATE INDEX idx ON table(foo(col), col_2)
>
> In such indexes, we could skip the attributes of the index. So, skey_attoff is not
> equal to  index_attoff anymore. So, calling out this explicitly via the variable names
> seems more robust to me. Plus, mainattno sounded vague to me when I first read
> this function.
>

Yeah, I understand this part. By looking at the diff, it appeared to
me that this was an unnecessary change. Anyway, I see your point, so
if you want to keep the naming as you proposed at least don't change
the ordering for get_opclass_input_type() call because that looks odd
to me.
>
> Attached v29 for this review. Note that I'll be working on the disable index scan changes after
>

Okay, thanks!

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Fri, 3 Mar 2023 16:10:33 +0300

Hi Vignesh,

Thanks for the review

makes sense, done

> +++ b/src/test/subscription/t/032_subscribe_use_index.pl

I changed it to #Copyright (c) 2022-2023, but I'm not sure if it should be
only 2023 or
like this.

> just drop and create required indexes and verify the update/delete

Well, not all the tables are exactly the same, there are 4-5 different
tables. Mostly the table names are the same.

Plus, the overhead does not seem to be large enough to complicate
the test. Many of the src/test/subscription/t files follow this pattern.

Do you have strong opinions on changing this?

> +# Testcase end: SUBSCRIPTION DOES NOT USE PARTIAL INDEX

Thanks, fixed.

Attached v30

---

From: Önder Kalacı <[email-masked]>
Date: Fri, 3 Mar 2023 17:15:49 +0300

Hi Hayato, Amit, all

> I think that single line comments should not terminated by ".".

Hmm, looking into execReplication.c, many of the single line comments
terminated by ".".  Also, On the HEAD, the same comment has single
line comment. So, I'd rather stick to that?

> +       /* There should always be at least one attribute for the index

Same as above :)

> and one column reference?

IIUC we can do that by adding new function like
> IsIndexUsableForReplicaIdentityFull()

I think adding a function like IsIndexUsableForReplicaIdentityFull is
useful. I can use it inside
FindUsableIndexForReplicaIdentityFull() and assert here. Also good for
readability.

So, I mainly moved this assert to a more generic place with a more generic
check
to RelationFindReplTupleByIndex

> There is still non-consistent case.
Fixed, thanks

Anyway, I see your point, so
> the ordering for get_opclass_input_type() call because that looks odd

(A small comment from Amit's previous e-mail)

Sure, applied now.

Attaching v31.

Thanks for the reviews!
Onder KALACI

---

From: Amit Kapila <[email-masked]>
Date: Mon, 6 Mar 2023 15:10:24 +0530

>> +               return FindUsableIndexForReplicaIdentityFull(localrel);

Today, I was looking at this comment and the fix for it. It seems to
me that it would be better to not add the check (indexlist != NIL)
here and rather get the indexlist in
FindUsableIndexForReplicaIdentityFull(). It will anyway return
InvalidOid, if there is no index and that way code will look a bit
cleaner.

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Mon, 6 Mar 2023 14:18:39 +0300

Hi Amit, all

Amit Kapila <[email-masked]>, 6 Mar 2023 Pzt, 12:40 tarihinde şunu

> InvalidOid, if there is no index and that way code will look a bit
Yeah, seems easier to follow to me as well. Reflected it in the comment as
well.

Thanks,
Onder

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 7 Mar 2023 03:46:48 +0000

On Monay, Mar 6, 2023 7:19 PM Önder Kalacı <[email-masked]> wrote:
> 
> Yeah, seems easier to follow to me as well. Reflected it in the comment as well.  
> 

Thanks for updating the patch. Here are some comments on v33-0001 patch.

1.
+	if (RelationReplicaIdentityFullIndexScanEnabled(localrel) &&
+		remoterel->replident == REPLICA_IDENTITY_FULL)

RelationReplicaIdentityFullIndexScanEnabled() is introduced in 0002 patch, so
the call to it should be moved to 0002 patch I think.

2.

Do we need this in the latest patch? I tried and it looks it could be removed
from src/backend/replication/logical/relation.c.

3.
+# now, create a unique index and set the replica
+$node_publisher->safe_psql('postgres',
+	"CREATE UNIQUE INDEX test_replica_id_full_unique ON test_replica_id_full(x);");
+$node_subscriber->safe_psql('postgres',
+	"CREATE UNIQUE INDEX test_replica_id_full_unique ON test_replica_id_full(x);");
+

Should the comment be "now, create a unique index and set the replica identity"?

4.
+$node_publisher->safe_psql('postgres',
+	"ALTER TABLE test_replica_id_full REPLICA IDENTITY USING INDEX test_replica_id_full_unique;");
+$node_subscriber->safe_psql('postgres',
+	"ALTER TABLE test_replica_id_full REPLICA IDENTITY USING INDEX test_replica_id_full_unique;");
+
+# wait for the synchronization to finish
+$node_subscriber->wait_for_subscription_sync;

There's no new tables to need to be synchronized here, should we remove the call
to wait_for_subscription_sync?

Regards,
Shi Yu

---

From: Amit Kapila <[email-masked]>
Date: Tue, 7 Mar 2023 12:27:58 +0530

> > Yeah, seems easier to follow to me as well. Reflected it in the comment as well.

Few comments:
=============
1.
+get_usable_indexoid(ApplyExecutionData *edata, ResultRelInfo *relinfo)
{
...
+ if (targetrelkind == RELKIND_PARTITIONED_TABLE)
+ {
+ /* Target is a partitioned table, so find relmapentry of the partition */
+ TupleConversionMap *map = ExecGetRootToChildMap(relinfo, edata->estate);
+ AttrMap    *attrmap = map ? map->attrMap : NULL;
+
+ relmapentry =
+ logicalrep_partition_open(relmapentry, relinfo->ri_RelationDesc,
+   attrmap);

When will we hit this part of the code? As per my understanding, for
partitioned tables, we always follow apply_handle_tuple_routing()
which should call logicalrep_partition_open(), and do the necessary
work for us.

2. In logicalrep_partition_open(), it would be better to set
localrelvalid after finding the required index. The entry should be
marked valid after initializing/updating all the required members. I
have changed this in the attached.

3.
@@ -31,6 +32,7 @@ typedef struct LogicalRepRelMapEntry
  Relation localrel; /* relcache entry (NULL when closed) */
  AttrMap    *attrmap; /* map of local attributes to remote ones */
  bool updatable; /* Can apply updates/deletes? */
+ Oid usableIndexOid; /* which index to use, or InvalidOid if none */

Would it be better to name this new variable as localindexoid to match
it with the existing variable localreloid? Also, the camel case for
this variable appears odd.

4. If you agree with the above, then we should probably change the
name of functions get_usable_indexoid() and
FindLogicalRepUsableIndex() accordingly.

5.
+ {
+ /*
+ * If we had a primary key or relation identity with a unique index,
+ * we would have already found and returned that oid. At this point,
+ * the remote relation has replica identity full.

These comments are not required as this just states what the code just
above is doing.

Apart from the above, I have made some modifications in the other
comments. If you are convinced with those, then kindly include them in
the next version.

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Tue, 7 Mar 2023 13:59:12 +0300

Hi Shi Yu, all

> the call to it should be moved to 0002 patch I think.

Ah, sure, a rebase issue, fixed in v34

> from src/backend/replication/logical/relation.c.

Hmm, probably an artifact of the initial versions of the patch where we
needed some
costing functionality.

> Should the comment be "now, create a unique index and set the replica

yes, fixed

> to wait_for_subscription_sync?

right, probably a copy & paste typo, thanks for spotting.

I'll attach v34 with the next e-mail given the comments here only touch
small parts
of the patch.

Thanks,
Onder KALACI

---

From: Önder Kalacı <[email-masked]>
Date: Tue, 7 Mar 2023 13:59:18 +0300

Hi Amit, all

> Few comments:
Looking closer, there is really no need for that. I changed the
code such that we pass usableLocalIndexOid. It looks simpler
to me. Do you agree?

makes sense

> this variable appears odd.

yes, both makes sense

> FindLogicalRepUsableIndex() accordingly.

I dropped get_usable_indexoid() as noted in (1).

Changed FindLogicalRepUsableIndex->FindLogicalRepLocalIndex

> These comments are not required as this just states what the code just

I don't have any strong opinions on adding this comment, applied your
suggestion.

> comments. If you are convinced with those, then kindly include them in
Sure, they all look good. I think I have lost (and caused the reviewers to
lose)
quite a bit of time on the comment reviews. Next time, I'll try to be more
prepared
for the comments.

Attached v34

Thanks,
Onder KALACI

---

From: vignesh C <[email-masked]>
Date: Wed, 8 Mar 2023 18:39:29 +0530

On Fri, 3 Mar 2023 at 18:40, Önder Kalacı <[email-masked]> wrote:
> I changed it to #Copyright (c) 2022-2023, but I'm not sure if it should be only 2023 or
> like this.
>
>>
>>

I felt that once you remove the create publication/subscription/wait
for sync steps, the test execution might become faster and save some
time in the local execution, cfbot and the various machines in
buildfarm. If the execution time will not reduce, then no need to
change.

Regards,
Vignesh

---

From: Önder Kalacı <[email-masked]>
Date: Wed, 8 Mar 2023 18:14:39 +0300

Hi,

> buildfarm. If the execution time will not reduce, then no need to
So, as I noted earlier, there are different schemas. As far as I count,
there are at least
7 different table definitions. I think all tables having the same name are
maybe confusing?

Even if I try to group the same table definitions, and avoid create
publication/subscription/wait
for sync steps, the total execution time of the test drops only ~5%. As far
as I test, that does not
seem to be the bottleneck for the tests.

Well, I'm really not sure if it is really worth doing that. I think having
each test independent of each
other is really much easier to follow.

Though, while looking into the execution times, I realized that in some
tests, I used quite a lot
of unnecessary tuples such as:

-       "INSERT INTO test_replica_id_full SELECT i, i FROM
generate_series(0,2100)i;");
+       "INSERT INTO test_replica_id_full SELECT i, i FROM
generate_series(0,21)i;");

In the next iteration of the patch, I'm going to decrease the number of
tuples. That seems to
save 5-10% of the execution time on my local machine.

Thanks,
Onder KALACI

---

From: Amit Kapila <[email-masked]>
Date: Thu, 9 Mar 2023 10:19:02 +0530

On Wed, Mar 8, 2023 at 8:44 PM Önder Kalacı <[email-masked]> wrote:
> So, as I noted earlier, there are different schemas. As far as I count, there are at least
> 7 different table definitions. I think all tables having the same name are maybe confusing?
>
> Even if I try to group the same table definitions, and avoid create publication/subscription/wait
> for sync steps, the total execution time of the test drops only ~5%. As far as I test, that does not
> Well, I'm really not sure if it is really worth doing that. I think having each test independent of each

This new test takes ~9s on my machine whereas most other tests in
subscription/t take roughly 2-5s. I feel we should try to reduce the
test timing without sacrificing much of the functionality or code
coverage.  I think if possible we should try to reduce setup/teardown
cost for each separate test by combining them where possible. I have a
few comments on tests which also might help to optimize these tests.

1.
+# Testcase start: SUBSCRIPTION USES INDEX
+#
+# Basic test where the subscriber uses index
+# and only updates 1 row and deletes
+# 1 other row
...
...
+# Testcase start: SUBSCRIPTION USES INDEX UPDATEs MULTIPLE ROWS
+#
+# Basic test where the subscriber uses index
+# and updates 50 rows

...
+# Testcase start: SUBSCRIPTION USES INDEX WITH MULTIPLE COLUMNS
+#
+# Basic test where the subscriber uses index
+# and deletes 200 rows

I think to a good extent these tests overlap each other. I think we
can have just one test for the index with multiple columns that
updates multiple rows and have both updates and deletes.

2.
+# Testcase start: SUBSCRIPTION DOES NOT USE PARTIAL INDEX
...
...
+# Testcase start: SUBSCRIPTION DOES NOT USE INDEXES WITH ONLY EXPRESSIONS

Instead of having separate tests where we do all setups again, I think
it would be better if we create both the indexes in one test and show
that none of them is used.

3.
+# now, the update could either use the test_replica_id_full_idy or
test_replica_id_full_idy index
+# it is not possible for user to control which index to use

The name of the second index is wrong in the above comment.

4.

As we have removed enable_indexscan check, you should remove this test.

5. In general, the line length seems to vary a lot for different
multi-line comments. Though we are not strict in that it will look
better if there is consistency in that (let's have ~80 cols line
length for each comment in a single line).

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Thu, 9 Mar 2023 16:07:30 +1100

> few comments on tests which also might help to optimize these tests.

To avoid culling useful tests just because they take too long to run I
have often thought we should separate some of the useful (but costly)
subscription tests from the mainstream other tests. Then they won't
cost any extra time for the build-farm, but at least we can still run
them on-demand using PG_TEST_EXTRA [1] approach if we really want to.

------
[1] https://www.postgresql.org/docs/devel/regress-run.html

Kind Regards,
Peter Smith.
Fujitsu Austrlia

---

From: Amit Kapila <[email-masked]>
Date: Thu, 9 Mar 2023 11:35:21 +0530

> them on-demand using PG_TEST_EXTRA [1] approach if we really want to.

I don't think that is relevant here. It is mostly about removing
duplicate work we are doing in tests. I don't see anything in the
tests that should require a long time to complete.

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Thu, 9 Mar 2023 12:56:00 +0300

Hi Amit, all

> coverage.

Alright, that is reasonable.

> I think if possible we should try to reduce setup/teardown

Alright, dropped *SUBSCRIPTION USES INDEX*, expanded
*SUBSCRIPTION USES INDEX WITH MULTIPLE COLUMNS* with an UPDATE
that touches multiple rows

> that none of them is used.

Makes sense

> The name of the second index is wrong in the above comment.

thanks, fixed

> As we have removed enable_indexscan check, you should remove this test.

Hmm, I think my rebase problems are causing confusion here, which v38 fixes.

In the first commit, we have ENABLE_INDEXSCAN checks. In the second commit,
I changed the same test to use enable_replica_identity_full_index_scan.

If we are going to only consider the first patch to get into the master
branch,
I can probably drop the test. In that case, I'm not sure what is our
perspective
on ENABLE_INDEXSCAN GUC. Do we want to keep that guard in the code
(hence the test)?

> length for each comment in a single line).
Went over the tests, and made ~80 cols. There is one exception, in the first
commit, the test for enable_indexcan is still shorter, but I failed to make
that
properly. I'll try to fix that as well, but I didn't want to block the
progress due to
that.

Also, you have not noted, but I think* SUBSCRIPTION USES INDEX WITH
MULTIPLE COLUMNS*
already covers  *SUBSCRIPTION USES INDEX UPDATEs MULTIPLE ROWS.*

So, I changed the first one to *SUBSCRIPTION USES INDEX WITH MULTIPLE ROWS
AND COLUMNS*
and dropped the second one. Let me know if it does not make sense to you.
If I try, there are few more
opportunities to squeeze in some more tests together, but those would start
to complicate readability.

Attached v38.

Thanks,
Onder KALACI

---

From: Amit Kapila <[email-masked]>
Date: Thu, 9 Mar 2023 16:50:37 +0530

> Hmm, I think my rebase problems are causing confusion here, which v38 fixes.

I think it is still not fixed in v38 as the test is still present in 0001.

> If we are going to only consider the first patch to get into the master branch,
> I can probably drop the test. In that case, I'm not sure what is our perspective

I am not sure what we are going to do on this because I feel we need
some storage option as you have in 0002 patch but you and Andres
thinks that is not required. So, we can discuss that a bit more after
0001 is committed but if there is no agreement then we need to
probably drop it. Even if drop it, I don't think using enable_index
makes sense. I think for now you don't need to send 0002 patch, let's
first try to get 0001 patch and then we can discuss about 0002.

> commit, the test for enable_indexcan is still shorter, but I failed to make that
> properly. I'll try to fix that as well, but I didn't want to block the progress due to
> that.
>
> Also, you have not noted, but I think SUBSCRIPTION USES INDEX WITH MULTIPLE COLUMNS
> already covers  SUBSCRIPTION USES INDEX UPDATEs MULTIPLE ROWS.
>
> So, I changed the first one to SUBSCRIPTION USES INDEX WITH MULTIPLE ROWS AND COLUMNS
> and dropped the second one. Let me know if it does not make sense to you. If I try, there are few more
> opportunities to squeeze in some more tests together, but those would start to complicate readability.
>

I still want to reduce the test time and will think about it. Which of
the other tests do you think can be combined?

BTW, did you consider updating the patch based on my yesterday's email [1]?

One minor comment:
+# now, create index and see that the index is used
+$node_subscriber->safe_psql('postgres',
+ "CREATE INDEX test_replica_id_full_idx ON test_replica_id_full(x)");
+
+# wait until the index is created
+$node_subscriber->poll_query_until(
+ 'postgres', q{select count(*)=1 from pg_stat_all_indexes where
indexrelname = 'test_replica_id_full_idx';}
+) or die "Timed out while waiting for creating index test_replica_id_full_idx";
+
+$node_publisher->safe_psql('postgres',
+ "UPDATE test_replica_id_full SET x = x + 1 WHERE x = 15;");
+$node_publisher->wait_for_catchup($appname);
+
+
+# wait until the index is used on the subscriber
+$node_subscriber->poll_query_until(
+ 'postgres', q{select (idx_scan = 1) from pg_stat_all_indexes where
indexrelname = 'test_replica_id_full_idx';}
+) or die "Timed out while waiting for check subscriber
tap_sub_rep_full updates one row via index";
+
+
+# now, create index on column y as well
+$node_subscriber->safe_psql('postgres',
+ "CREATE INDEX test_replica_id_full_idy ON test_replica_id_full(y)");
+
+# wait until the index is created
+$node_subscriber->poll_query_until(
+ 'postgres', q{select count(*)=1 from pg_stat_all_indexes where
indexrelname = 'test_replica_id_full_idy';}
+) or die "Timed out while waiting for creating index test_replica_id_full_idy";

It appears you are using inconsistent spacing. It may be better to use
a single empty line wherever required.

[1] - https://www.postgresql.org/message-id/CAA4eK1%2BoM_v-b_WDHZmqCyVHU2oD4j3vF9YcH9xVHj%3DzAfy4og%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 9 Mar 2023 17:43:04 +0530

> the other tests do you think can be combined?

Some of the ideas I can think of are as follows:

1. Combine "SUBSCRIPTION USES INDEX WITH MULTIPLE ROWS AND COLUMNS"
and "SUBSCRIPTION USES INDEX WITH DROPPED COLUMNS" such that after
verifying updates and deletes of the first test, we can drop some of
the columns on both publisher and subscriber, then use alter
subscription ... refresh publication command and then do the steps of
the second test. Note that we don't add tables after initial setup,
only changing schema.

2. We can also combine "Some NULL values" and "PUBLICATION LACKS THE
COLUMN ON THE SUBS INDEX" as both use the same schema. After the first
test, we need to drop the existing index and create a new index on the
subscriber node.

3. General comment
+# updates 200 rows
+$node_publisher->safe_psql('postgres',
+ "UPDATE test_replica_id_full SET x = x + 1 WHERE x IN (5, 6);");

I think here you are updating 20 rows not 200. So, the comment seems
wrong to me.

Please think more and see if we can combine some other tests like
"Unique index that is not primary key or replica identity" and the
test we will have after comment#2 above.

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Thu, 9 Mar 2023 15:18:28 +0300

Hi Amit,

> I think it is still not fixed in v38 as the test is still present in 0001.

Ah, yes, sorry again for the noise. v39 will drop that.

> first try to get 0001 patch and then we can discuss about 0002.

sounds good, when needed I'll rebase 0002.

> the other tests do you think can be combined?
I'll follow your suggestion in the next e-mail [2], and focus on further
improvements.

Yes, replied to that one just now with some wip commits [1]

> a single empty line wherever required.
Sure, let me fix those.

attached v39.

[1]
https://www.postgresql.org/message-id/CACawEhXGnk6v7UOHrxuJjjybHvvq33Zv666ouy4UzjPfJM6tBw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAA4eK1LSYWrthA3xjbrZvZVmwuha10HtM3-QRrVMD7YBt4t3pg%40mail.gmail.com

---

From: Peter Smith <[email-masked]>
Date: Fri, 10 Mar 2023 15:04:35 +1100

Here are some review comments for patch v39-0001 (mostly the test code).

======

1. FindUsableIndexForReplicaIdentityFull

+static Oid
+FindUsableIndexForReplicaIdentityFull(Relation localrel)
+{
+ List    *indexlist = RelationGetIndexList(localrel);
+ Oid usableIndex = InvalidOid;
+ ListCell   *lc;
+
+ foreach(lc, indexlist)
+ {
+ Oid idxoid = lfirst_oid(lc);
+ bool isUsableIndex;
+ Relation indexRelation = index_open(idxoid, AccessShareLock);
+ IndexInfo  *indexInfo = BuildIndexInfo(indexRelation);
+
+ isUsableIndex = IsIndexUsableForReplicaIdentityFull(indexInfo);
+
+ index_close(indexRelation, AccessShareLock);
+
+ if (isUsableIndex)
+ {
+ /* we found one eligible index, don't need to continue */
+ usableIndex = idxoid;
+ break;
+ }
+ }
+
+ return usableIndex;
+}

This comment is not functional -- if you prefer the code as-is, then
ignore this comment.

But, personally I would:
- Move some of that code from the declarations. I feel it would be
better if the index_open/index_close were both in the code-body
instead of half in declarations and half not.
- Remove the 'usableIndex' variable, and just return directly.
- Shorten all the long names (and use consistent 'idx' instead of
sometimes 'idx' and sometimes 'index')

SUGGESTION (YMMV)

static Oid
FindUsableIndexForReplicaIdentityFull(Relation localrel)
{
List    *idxlist = RelationGetIndexList(localrel);
ListCell   *lc;

foreach(lc, idxlist)
{
Oid idxoid = lfirst_oid(lc);
bool isUsableIdx;
Relation idxRel;
IndexInfo  *idxInfo;

idxRel = index_open(idxoid, AccessShareLock);
idxInfo = BuildIndexInfo(idxRel);
isUsableIdx = IsIndexUsableForReplicaIdentityFull(idxInfo);
index_close(idxRel, AccessShareLock);

/* Return the first eligible index found */
if (isUsableIdx)
return idxoid;
}

return InvalidOid;
}

======
.../subscription/t/032_subscribe_use_index.pl

2. SUBSCRIPTION CREATE/DROP INDEX WORKS WITHOUT ISSUES

2a.
# Testcase start: SUBSCRIPTION CREATE/DROP INDEX WORKS WITHOUT ISSUES
#
# This test ensures that after CREATE INDEX, the subscriber can automatically
# use one of the indexes (provided that it fulfils the requirements).
# Similarly, after DROP index, the subscriber can automatically switch to
# sequential scan

The last sentence is missing full-stop.

~

2b.
# now, create index and see that the index is used
$node_subscriber->safe_psql('postgres',
    "CREATE INDEX test_replica_id_full_idx ON test_replica_id_full(x)");

Don't say "and see that the index is used". Yes, that is what this
whole test is doing, but it is not what the psql following this
comment is doing.

~

2c.
$node_publisher->safe_psql('postgres',
    "UPDATE test_replica_id_full SET x = x + 1 WHERE x = 15;");
$node_publisher->wait_for_catchup($appname);

# wait until the index is used on the subscriber

The double blank lines here should be single.

~

2d.
# now, the update could either use the test_replica_id_full_idx or
# test_replica_id_full_idy index it is not possible for user to control
# which index to use

This sentence should break at "it".

Aso "user" --> "the user"

SUGGESTION
# now, the update could either use the test_replica_id_full_idx or
# test_replica_id_full_idy index; it is not possible for the user to control
# which index to use

~

2e.
# let's also test dropping test_replica_id_full_idy and
# hence use test_replica_id_full_idx

I think you ought to have dropped the other (first) index because we
already know that the first index had been used (from earlier), but we
are not 100% sure if the 'y' index has been chosen yet.

~~~~

3. SUBSCRIPTION USES INDEX WITH MULTIPLE ROWS AND COLUMNS

3a.
# deletes 20 rows
$node_publisher->safe_psql('postgres',
    "DELETE FROM test_replica_id_full WHERE x IN (5, 6);");

# updates 20 rows
$node_publisher->safe_psql('postgres',
    "UPDATE test_replica_id_full SET x = 100, y = '200' WHERE x IN (1, 2);");

"deletes" --> "delete"

"updates" --> "update"

~~~

4. SUBSCRIPTION USES INDEX WITH DROPPED COLUMNS

# updates 200 rows
$node_publisher->safe_psql('postgres',
    "UPDATE test_replica_id_full SET x = x + 1 WHERE x IN (5, 6);");

"updates" --> "update"

"200 rows" ??? is that right --  20 maybe ???

~~~

5. SUBSCRIPTION USES INDEX ON PARTITIONED TABLES

5a.
# updates rows and moves between partitions
$node_publisher->safe_psql('postgres',
    "UPDATE users_table_part SET value_1 = 0 WHERE user_id = 4;");

"updates rows and moves between partitions" --> "update rows, moving
them to other partitions"

~

5b.
# deletes rows from different partitions

"deletes" --> "delete"

~~~

6. SUBSCRIPTION DOES NOT USE INDEXES WITH ONLY EXPRESSIONS OR PARTIAL INDEX

6a.
# update 2 rows
$node_publisher->safe_psql('postgres',
    "UPDATE people SET firstname = 'Nan' WHERE firstname = 'first_name_1';");
$node_publisher->safe_psql('postgres',
    "UPDATE people SET firstname = 'Nan' WHERE firstname =
'first_name_2' AND lastname = 'last_name_2';");

IMO 'Nan' seemed a curious name to assign as a test value, because it
seems like it has a special meaning but in reality, I don't think it
does. Even 'xxx' would be better.

~

6b.
# make sure none of the indexes is not used on the subscriber
$result = $node_subscriber->safe_psql('postgres',
    "select sum(idx_scan) from pg_stat_all_indexes where indexrelname
IN ('people_names_expr_only', 'people_names_partial')");
is($result, qq(0), 'ensure subscriber tap_sub_rep_full updates two
rows via seq. scan with index on expressions');

~

Looks like a typo in this comment: "none of the indexes is not used" ???

~~~

7. SUBSCRIPTION CAN USE INDEXES WITH EXPRESSIONS AND COLUMNS

7a.
# update 2 rows
$node_publisher->safe_psql('postgres',
    "UPDATE people SET firstname = 'Nan' WHERE firstname = 'first_name_1';");
$node_publisher->safe_psql('postgres',
    "UPDATE people SET firstname = 'Nan' WHERE firstname =
'first_name_3' AND lastname = 'last_name_3';");

Same as #6a, 'Nan' seems like a strange test value to assign to a name.

~~~

8. Some NULL values

$node_publisher->safe_psql('postgres',
    "INSERT INTO test_replica_id_full VALUES (1), (2), (3);");
$node_publisher->safe_psql('postgres',
    "UPDATE test_replica_id_full SET x = x + 1 WHERE x = 1;");
$node_publisher->safe_psql('postgres',
    "UPDATE test_replica_id_full SET x = x + 1 WHERE x = 3;");

~

8a.
For some reason, this insert/update psql was not commented.

~

8b.
Maybe this test data could be more obvious by explicitly inserting the NULLs?

~~~

9. Unique index that is not primary key or replica identity

9a.
Why are other "Testcase start" comments all uppercase but not this one?

~~~

10. SUBSCRIPTION USES INDEX WITH PUB/SUB different data

Why is there a mixed case in this "Test case:" comment?

~~~

11. PUBLICATION LACKS THE COLUMN ON THE SUBS INDEX

11a.
# The subsriber has an additional column compared to publisher,
# and the index is on that column. We still pick the index scan
# on the subscriber even though it is practically similar to
# sequential scan

Typo "subsriber"

Missing full-stop on last sentence.

~

11b.
# make sure that the subscriber has the correct data
# we only deleted 1 row
$result = $node_subscriber->safe_psql('postgres',
    "SELECT sum(x) FROM test_replica_id_full");
is($result, qq(232), 'ensure subscriber has the correct data at the
end of the test');

Why does that say "deleted 1 row", when the previous operation was not a DELETE?

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

---

From: Önder Kalacı <[email-masked]>
Date: Fri, 10 Mar 2023 12:54:48 +0300

Hi Amit, all

> the second test. Note that we don't add tables after initial setup,

Done with an important caveat. I think this reorganization of the test
helped
us to find one edge case regarding dropped columns.

I realized that the dropped columns also get into the tuples_equal()
function. And,
the remote sends NULL to for the dropped columns(i.e., remoteslot), but
index_getnext_slot() (or table_scan_getnextslot) indeed fills the dropped
columns on the outslot. So, the dropped columns are not NULL in the outslot.

This triggers tuples_equal to fail. To fix that, I improved the tuples_equal
such that it skips the dropped columns.

I also spend quite a bit of time understanding how/why this impacts
HEAD. See steps below on HEAD, where REPLICA IDENTITY FULL
fails to replica the data properly:

-- pub
CREATE TABLE test (drop_1 jsonb, x int, drop_2 numeric, y text, drop_3
timestamptz);
ALTER TABLE test REPLICA IDENTITY FULL;
INSERT INTO test SELECT NULL, i, i, (i)::text, now() FROM
generate_series(0,1)i;
CREATE PUBLICATION pub FOR ALL TABLES;

-- sub
CREATE TABLE test (drop_1 jsonb, x int, drop_2 numeric, y text, drop_3
timestamptz);
CREATE SUBSCRIPTION sub CONNECTION 'host=localhost port=5432
dbname=postgres' PUBLICATION pub;

-- show that before dropping the columns, the data in the source and
-- target are deleted properly
DELETE FROM test WHERE x = 0;

-- both on the source and target
SELECT count(*) FROM test WHERE x = 0;
┌───────┐
│ count │
├───────┤
│     0 │
└───────┘
(1 row)

-- drop columns on both the the source
ALTER TABLE test DROP COLUMN drop_1;
ALTER TABLE test DROP COLUMN drop_2;
ALTER TABLE test DROP COLUMN drop_3;

-- drop columns on both the the target
ALTER TABLE test DROP COLUMN drop_1;
ALTER TABLE test DROP COLUMN drop_2;
ALTER TABLE test DROP COLUMN drop_3;

-- on the target
ALTER SUBSCRIPTION sub REFRESH PUBLICATION;

-- after dropping the columns
DELETE FROM test WHERE x = 1;

-- source
SELECT count(*) FROM test WHERE x = 1;
┌───────┐
│ count │
├───────┤
│     0 │
└───────┘
(1 row)

*-- target, OOPS wrong result!!!!*SELECT count(*) FROM test WHERE x = 1;

┌───────┐
│ count │
├───────┤
│     1 │
└───────┘
(1 row)

Should we have a separate patch for the tuples_equal change so that we
might want to backport? Attached as v40_0001 on the patch.

Note that I need to have that commit as 0001 so that 0002 patch
passes the tests.

done

> I think here you are updating 20 rows not 200. So, the comment seems

I think I have fixed that in an earlier version because I cannot see this
comment anymore.

> test we will have after comment#2 above.
I'll look for more opportunities and reply to the thread. I wanted to send
this mail so that you can have a look at (1) earlier.

Thanks,
Onder

---

From: Önder Kalacı <[email-masked]>
Date: Fri, 10 Mar 2023 13:58:25 +0300

Hi Peter, all

> /* Return the first eligible index found */

applied your suggestion. I think it made it slightly easier to follow.

fixed

> whole test is doing, but it is not what the psql following this

 true

> The double blank lines here should be single.

fixed,

SUGGESTION
> # test_replica_id_full_idy index; it is not possible for the user to
looks good, thanks

> are not 100% sure if the 'y' index has been chosen yet.

 make sense. Though in general it is hard to check pg_stat_all_indexes
for any of the indexes on this test, as we don't know the exact number
of tuples for each. Just wanted to explicitly note

>     "UPDATE test_replica_id_full SET x = 100, y = '200' WHERE x IN (1,

Well, I thought the command *deletes* but I guess delete looks better

> "200 rows" ??? is that right --  20 maybe ???
I guess this is from an earlier version of the patch, I fixed these types
of errors.

> them to other partitions"
fixed, thanks

> # deletes rows from different partitions
fixed, and searched for similar errors but couldn't see any more

~~~

> does. Even 'xxx' would be better.

changed to "no-name" as "xxx" also looks not so good

> dropped "not"

> Same as #6a, 'Nan' seems like a strange test value to assign to a name.
similarly, changed to no-name

> For some reason, this insert/update psql was not commented.
added some

> Maybe this test data could be more obvious by explicitly inserting the
Well, that's a bit hard. We merged a few tests for perf reasons. And, I
merged this
test with "missing column" test. Now, the NULL values are triggered due to
missing column on the source.

> Why are other "Testcase start" comments all uppercase but not this one?
fixed, there was one more

> Why is there a mixed case in this "Test case:" comment?

no specific reason, fixed

> # on the subscriber even though it is practically similar to

I guess I fixed this in a recent iteration, I cannot find it.

> Missing full-stop on last sentence.

Similarly, probably merged into another test.

Still went over the all such explanations in the test, and ensured
we have the full stop

> Why does that say "deleted 1 row", when the previous operation was not a
Probably due to merging multiple tests into one. Fixed now.

Again, thanks for thorough review. Attached v41.

See the reason for 0001 patch in [1].

Onder KALACI.

[1]
https://www.postgresql.org/message-id/CACawEhUu6S8E4Oo7%2Bs5iaq%3DyLRZJb6uOZeEQSGJj-7NVkDzSaw%40mail.gmail.com

---

From: Önder Kalacı <[email-masked]>
Date: Fri, 10 Mar 2023 17:28:33 +0300

Hi Amit, all

> I merged SUBSCRIPTION CREATE/DROP INDEX WORKS WITHOUT ISSUES
into SUBSCRIPTION CAN USE INDEXES WITH EXPRESSIONS AND COLUMNS.

Also, merged SUBSCRIPTION USES INDEX WITH PUB/SUB DIFFERENT DATA and
 A UNIQUE INDEX THAT IS NOT PRIMARY KEY OR REPLICA IDENTITY

So, we have 6 test cases left. I start to feel that trying to merge further
is going to start making
the readability get worse. Do you have any further easy test case merge
suggestions?

I think one option could be to drop some cases altogether, but not sure
we'd want that.

As a semi-related question: Are you aware of any setting that'd
make pg_stat_all_indexes
reflect the changes sooner? It is hard to debug what is the bottleneck in
the tests, but
I have a suspicion that there might be several poll_query_until() calls on
pg_stat_all_indexes, which might be the reason?

Attaching v43.

---

From: Amit Kapila <[email-masked]>
Date: Sat, 11 Mar 2023 09:00:49 +0530

On Fri, Mar 10, 2023 at 3:25 PM Önder Kalacı <[email-masked]> wrote:
>
>>
>
> Done with an important caveat. I think this reorganization of the test helped
> I realized that the dropped columns also get into the tuples_equal() function. And,

Good catch. By any chance, have you tried with generated columns? See
logicalrep_write_tuple()/logicalrep_write_attrs() where we neither
send anything for dropped columns nor for generated columns.
Similarly, on receiving side, in logicalrep_rel_open() and
slot_store_data(), we seem to be using NULL for such columns.

> CREATE TABLE test (drop_1 jsonb, x int, drop_2 numeric, y text, drop_3 timestamptz);
> INSERT INTO test SELECT NULL, i, i, (i)::text, now() FROM generate_series(0,1)i;
> CREATE TABLE test (drop_1 jsonb, x int, drop_2 numeric, y text, drop_3 timestamptz);
> CREATE SUBSCRIPTION sub CONNECTION 'host=localhost port=5432 dbname=postgres' PUBLICATION pub;
>
> -- target, OOPS wrong result!!!!
> might want to backport?

Yes, it would be better to report and discuss this in a separate thread,

> Attached as v40_0001 on the patch.
>

I think we can add such a test (which relies on existing buggy
behavior) later after fixing the existing bug. For now, it would be
better to remove that test and add it after we fix dropped columns
issue in HEAD.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Sat, 11 Mar 2023 12:35:13 +0530

On Fri, Mar 10, 2023 at 7:58 PM Önder Kalacı <[email-masked]> wrote:
>
>
> I think one option could be to drop some cases altogether, but not sure we'd want that.
>
> As a semi-related question: Are you aware of any setting that'd make pg_stat_all_indexes
> reflect the changes sooner? It is hard to debug what is the bottleneck in the tests, but

Yeah, I also think poll_query_until() calls on pg_stat_all_indexes is
the main reason for these tests taking more time. When I commented
those polls, it drastically reduces the test time. On looking at
pgstat_report_stat(), it seems we don't report stats sooner than 1s
and as most of this patch's test relies on stats, it leads to taking
more time. I don't have a better idea to verify this patch without
checking whether the index scan is really used by referring to
pg_stat_all_indexes. I think trying to reduce the poll calls may help
in reducing the test timings further. Some ideas on those lines are as
follows:
1.
+# Testcase start: SUBSCRIPTION USES INDEX WITH PUB/SUB DIFFERENT DATA VIA
+# A UNIQUE INDEX THAT IS NOT PRIMARY KEY OR REPLICA IDENTITY

No need to use Delete test separate for this.

2.
+$node_publisher->safe_psql('postgres',
+ "UPDATE test_replica_id_full SET x = x + 1 WHERE x = 1;");
+$node_publisher->safe_psql('postgres',
+ "UPDATE test_replica_id_full SET x = x + 1 WHERE x = 3;");
+
+# check if the index is used even when the index has NULL values
+$node_subscriber->poll_query_until(
+ 'postgres', q{select idx_scan=2 from pg_stat_all_indexes where
indexrelname = 'test_replica_id_full_idx';}
+) or die "Timed out while waiting for check subscriber
tap_sub_rep_full updates test_replica_id_full table";

Here, I think only one update is sufficient.

3.
+$node_subscriber->safe_psql('postgres',
+ "CREATE INDEX people_last_names ON people(lastname)");
+
+# wait until the index is created
+$node_subscriber->poll_query_until(
+ 'postgres', q{select count(*)=1 from pg_stat_all_indexes where
indexrelname = 'people_last_names';}
+) or die "Timed out while waiting for creating index people_last_names";

I don't think we need this poll.

4.
+# update 2 rows
+$node_publisher->safe_psql('postgres',
+ "UPDATE people SET firstname = 'no-name' WHERE firstname = 'first_name_1';");
+$node_publisher->safe_psql('postgres',
+ "UPDATE people SET firstname = 'no-name' WHERE firstname =
'first_name_3' AND lastname = 'last_name_3';");
+
+# wait until the index is used on the subscriber
+$node_subscriber->poll_query_until(
+ 'postgres', q{select idx_scan=2 from pg_stat_all_indexes where
indexrelname = 'people_names';}
+) or die "Timed out while waiting for check subscriber
tap_sub_rep_full updates two rows via index scan with index on
expressions and columns";
+
+$node_publisher->safe_psql('postgres',
+ "DELETE FROM people WHERE firstname = 'no-name';");
+
+# wait until the index is used on the subscriber
+$node_subscriber->poll_query_until(
+ 'postgres', q{select idx_scan=4 from pg_stat_all_indexes where
indexrelname = 'people_names';}
+) or die "Timed out while waiting for check subscriber
tap_sub_rep_full deletes two rows via index scan with index on
expressions and columns";

I think having one update or delete should be sufficient.

5.
+# update rows, moving them to other partitions
+$node_publisher->safe_psql('postgres',
+ "UPDATE users_table_part SET value_1 = 0 WHERE user_id = 4;");
+
+# wait until the index is used on the subscriber
+$node_subscriber->poll_query_until(
+ 'postgres', q{select sum(idx_scan)=1 from pg_stat_all_indexes where
indexrelname ilike 'users_table_part_%';}
+) or die "Timed out while waiting for updates on partitioned table with index";
+
+# delete rows from different partitions
+$node_publisher->safe_psql('postgres',
+ "DELETE FROM users_table_part WHERE user_id = 1 and value_1 = 1;");
+$node_publisher->safe_psql('postgres',
+ "DELETE FROM users_table_part WHERE user_id = 12 and value_1 = 12;");
+
+# wait until the index is used on the subscriber
+$node_subscriber->poll_query_until(
+ 'postgres', q{select sum(idx_scan)=3 from pg_stat_all_indexes where
indexrelname ilike 'users_table_part_%';}
+) or die "Timed out while waiting for check subscriber
tap_sub_rep_full updates partitioned table";
+

Can we combine these two polls?

6.
+# Testcase start: SUBSCRIPTION USES INDEX WITH MULTIPLE ROWS AND COLUMNS, ALSO
+# DROPS COLUMN

In this test, let's try to update/delete 2-3 rows instead of 20. And
after drop columns, let's keep just one of the update or delete.

7. Apart from the above, I think it is better to use
wait_for_catchup() consistently before trying to verify the data on
the subscriber. We always use it in other tests. I guess here you are
relying on the poll for index scans to ensure that data is replicated
but I feel it may still be better to use wait_for_catchup().
Similarly, wait_for_subscription_sync uses the publisher name and
appname in other tests, so it is better to be consistent. It can avoid
random failures by ensuring data is synced.

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Sat, 11 Mar 2023 23:04:26 +0300

Hi Amit, all

>  By any chance, have you tried with generated columns?

Yes, it shows the same behavior.

> See

> slot_store_data(), we seem to be using NULL for such columns.
Thanks for the explanation, it helps a lot.

> Yes, it would be better to report and discuss this in a separate thread,

Done via [1]

> better to remove that test and add it after we fix dropped columns

Alright, when I push the next version (hopefully tomorrow), I'll follow
this suggestion.

Thanks,
Onder KALACI

[1]
https://www.postgresql.org/message-id/CACawEhVQC9WoofunvXg12aXtbqKnEgWxoRx3%2Bv8q32AWYsdpGg%40mail.gmail.com

---

From: Amit Kapila <[email-masked]>
Date: Sun, 12 Mar 2023 11:28:33 +0530

On Sun, Mar 12, 2023 at 1:34 AM Önder Kalacı <[email-masked]> wrote:
> Alright, when I push the next version (hopefully tomorrow), I'll follow this suggestion.
>

Okay, thanks. See, if you can also include your changes in the patch
wip_for_optimize_index_column_match (after discussed modification).
Few other minor comments:

1.
+   are enforced for primary keys.  Internally, we follow a similar approach for
+   supporting index scans within logical replication scope.  If there are no

I think we can remove the above line: "Internally, we follow a similar
approach for supporting index scans within logical replication scope."
This didn't seem useful for users.

2.
diff --git a/src/backend/executor/execReplication.c
b/src/backend/executor/execReplication.c
index bc6409f695..646e608eb7 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -83,11 +83,8 @@ build_replindex_scan_key(ScanKey skey, Relation
rel, Relation idxrel,
                if (!AttributeNumberIsValid(table_attno))
                {
                        /*
-                        * XXX: For a non-primary/unique index with an
additional
-                        * expression, we do not have to continue at
this point. However,
-                        * the below code assumes the index scan is
only done for simple
-                        * column references. If we can relax the
assumption in the below
-                        * code-block, we can also remove the continue.
+                        * XXX: Currently, we don't support
expressions in the scan key,
+                        * see code below.
                         */

I have tried to simplify the above comment. See, if that makes sense to you.

3.
/*
+ * We only need to allocate once. This is allocated within per
+ * tuple context -- ApplyMessageContext -- hence no need to
+ * explicitly pfree().
+ */

We normally don't write why we don't need to explicitly pfree. It is
good during the review but not sure if it is a good idea to keep it in
the final code.

4. I have modified the proposed commit message as follows, see if that
makes sense to you, and let me know if I missed anything especially
the review/author credit.

Allow the use of indexes other than PK and REPLICA IDENTITY on the subscriber.

Using REPLICA IDENTITY FULL on the publisher can lead to a full table
scan per tuple change on the subscription when REPLICA IDENTITY or PK
index is not available. This makes REPLICA IDENTITY FULL impractical
to use apart from some small number of use cases.

This patch allows using indexes other than PRIMARY KEY or REPLICA
IDENTITY on the subscriber during apply of update/delete. The index
that can be used must be a btree index, not a partial index, and it
must have at least one column reference (i.e. cannot consist of only
expressions). We can uplift these restrictions in the future. There is
no smart mechanism to pick the index. If there is more than one index
that
satisfies these requirements, we just pick the first one. We discussed
using some of the optimizer's low-level APIs for this but ruled it out
as that can be a maintenance burden in the long run.

This patch improves the performance in the vast majority of cases and
the improvement is proportional to the amount of data in the table.
However, there could be some regression in a small number of cases
where the indexes have a lot of duplicate and dead rows. It was
discussed that those are mostly impractical cases but we can provide a
table or subscription level option to disable this feature if
required.

Author: Onder Kalaci
Reviewed-by: Peter Smith, Shi yu, Hou Zhijie, Vignesh C, Kuroda
Hayato, Amit Kapila
Discussion: https://postgr.es/m/CACawEhVLqmAAyPXdHEPv1ssU2c=[email-masked]

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Sun, 12 Mar 2023 18:07:17 +0300

Hi Amit, all

> No need to use Delete test separate for this.

Yeah, there is really no difference between update/delete for this patch,
so it makes sense. I initially added it for completeness for the coverage,
but as it has the perf overhead for the tests, I agree that we could
drop some of those.

> Here, I think only one update is sufficient.

done. I guess you requested this change so that we would wait
for idx_scan=1 not idx_scan=2, which could help.

> I don't think we need this poll.

 true, not sure why I have this. none of the tests has this anyway.

> I think having one update or delete should be sufficient.

So, I dropped the 2nd update, but kept 1 update and 1 delete.
The latter deletes the tuple updated by the former. Seems like
an interesting test to keep.

Still, I dropped one of the extra poll_query_until, which is probably
good enough for this one? Let me know if you think otherwise.

> Can we combine these two polls?

Looking at it closely, the first one seems like an unnecessary poll anyway.
We can simply check the idxscan at the end of the test, I don't see
value in checking earlier.

> after drop columns, let's keep just one of the update or delete.

changed to 3 rows

> but I feel it may still be better to use wait_for_catchup().

Yes, that was my understanding & expectation. I'm not convinced that
 wait_for_catchup() is strictly needed, as without catching up, how could
pg_stat_all_indexes be updated? Still, it is good to be consistent
with the test suite. So, applied your suggestion.

> random failures by ensuring data is synced.

makes sense.

I'll attach a new patch in the next e-mail, along with your
other comments.

Thanks,
Onder KALACI

---

From: Önder Kalacı <[email-masked]>
Date: Sun, 12 Mar 2023 18:07:21 +0300

Hi Amit, all

> Few other minor comments:

Sure, done. Please check RemoteRelContainsLeftMostColumnOnIdx() function.

Note that we already have a test for that on SOME NULL VALUES AND MISSING
COLUMN.
Previously we'd check if test_replica_id_full_idy is used. Now, we don't
because it is not
used anymore :) I initially used poll_query_until with idx_scan=0, but that
also seems
confusing to read in the test. It looks like it could be prone to race
conditions as
poll_query_until with idxcan=0 does not guarantee anything.

> This didn't seem useful for users.
removed

Makes sense

> good during the review but not sure if it is a good idea to keep it in
Sounds good, applied

> Discussion:
> https://postgr.es/m/CACawEhVLqmAAyPXdHEPv1ssU2c=[email-masked]
>
>
I also see 2 mails/reviews from Wang wei, but I'm not sure what qualifies
as "reviewer" for this group. Should we
add that name as well? I think you can guide us on this.

Other than that, I only fixed one extra new line between 'that' and
"satisfies'. Other than that, it looks pretty good!

Thanks,
Onder

---

From: Peter Smith <[email-masked]>
Date: Mon, 13 Mar 2023 08:14:03 +1100

> in reducing the test timings further. Some ideas on those lines are as

If the reason for the stats polling was only to know if some index is
chosen or not, I was wondering if you can just convey the same
information to the TAP test via some conveniently placed (DEBUG?)
logging.

This way the TAP test can do a 'wait_for_log' instead of the
'poll_query_until'. It will probably generate lots of extra logging
but it still might be lots faster that current code because it won't
incur the 1s overheads of the stats.

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

---

From: Amit Kapila <[email-masked]>
Date: Mon, 13 Mar 2023 17:18:13 +0530

> information to the TAP test via some conveniently placed (DEBUG?)

I had thought about it but didn't convince myself that it would be a
better approach because it would LOG a lot of messages for bulk
updates/deletes. Note for each row update on the publisher a new
index/sequence scan will happen. So, instead, I tried to further
change the test cases to remove unnecessary parts. I have changed
below tests:

1.
+# subscriber gets the missing table information
+$node_subscriber->safe_psql('postgres',
+ "ALTER SUBSCRIPTION tap_sub_rep_full REFRESH PUBLICATION");

This and the follow-on test was not required after we have removed
Dropped columns test.

2. Reduce the number of updates/deletes in the first test to two rows.

3. Removed the cases for dropping the index. This ensures that after
dropping the index on the table we switch to either an index scan (if
a new index is created) or to a sequence scan. It doesn't seem like a
very interesting case to me.

Apart from the above, I have removed the explicit setting of
'wal_retrieve_retry_interval = 1ms' as the same is not done for any
other subscription tests. I know setting wal_retrieve_retry_interval
avoids the launcher sometimes taking more time to launch apply worker
but it is better to be consistent. See the changes in
changes_amit_1.patch, if you agree with the same then please include
them in the next version.

After doing the above, the test time on my machine is closer to what
other tests take which is ~5s.

--
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Mon, 13 Mar 2023 15:43:54 +0300

Hi Amit, Peter, all

> updates/deletes.

I'm also hesitant to add any log messages for testing purposes, especially
something like this one, where a single UPDATE on the source code
leads to an unbounded number of logs.

> This and the follow-on test was not required after we have removed
Right, I kept it with the idea that we might get the dropped column changes
earlier, so that I can rebase and add the drop column ones.

But, sure, we can add that later to other tests.

> 2. Reduce the number of updates/deletes in the first test to two rows.

We don't have any particular reasons to have more tuples. Given the
time constraints, I don't have any objections to change this.

> very interesting case to me.

For that test, my goal was to ensure/show that the invalidation callback
is triggered after `DROP / CREATE INDEX` commands.

Can we always assume that this would never change? Because if this
behavior ever changes, the users would stuck with the wrong/old
index until VACUUM happens.

> but it is better to be consistent

Hmm, I cannot remember why I added that. It was probably to make
poll_query_until/wait_for_catchup to happen faster.

But, running the test w/wout this setting, I cannot observe any noticeable
difference. So, probably fine to remove.

> . See the changes in

included all, but I'm not very sure to remove (3). If you think we have
coverage for that in other cases, I'm fine with that.

> Yes, same for me.

Thanks, attaching v46

---

From: Amit Kapila <[email-masked]>
Date: Mon, 13 Mar 2023 18:41:30 +0530

> is triggered after `DROP / CREATE INDEX` commands.

Fair point. I suggest in that case just keep one of the tests for Drop
Index such that after that it will pick up a sequence scan. However,
just do the poll for the number of index scans stat once. I think that
will cover the case you are worried about without having a noticeable
impact on test timing.

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Mon, 13 Mar 2023 17:16:23 +0300

Hi Amit, all

> will cover the case you are worried about without having a noticeable
So, after dropping the index, it is not possible to poll for the idxscan.

But, I think, after the drop index, it is enough to check if the
modification
is applied properly on the target (wait_for_catchup + safe_psql).
If it were to cache the indexOid, the update/delete would fail anyway.

Attaching v47.

Thanks,
Onder KALACI

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 14 Mar 2023 06:01:11 +0000

> Attaching v47.

Thanks for updating the patch. Here are some comments.

1.
in RemoteRelContainsLeftMostColumnOnIdx():

+	if (indexInfo->ii_NumIndexAttrs < 1)
+		return false;

Did you see any cases that the condition is true? I think there is at least one
column in the index. If so, we can use an Assert().

+	if (attrmap->maplen <= AttrNumberGetAttrOffset(keycol))
+		return false;

Similarly, I think `attrmap->maplen` is the number of columns and it is always
greater than keycol. If you agree, we can check it with an Assert(). Besides, It
seems we don't need AttrNumberGetAttrOffset().

2.
+# make sure that the subscriber has the correct data after the update UPDATE

"update UPDATE" seems to be a typo.

3.
+# now, drop the index with the expression, and re-create index on column lastname

The comment says "re-create index on column lastname" but it seems we didn't do
that, should it be modified to something like: 
# now, drop the index with the expression, we will use sequential scan

Besides these, the patch LGTM.

Regards,
Shi Yu

---

From: Amit Kapila <[email-masked]>
Date: Tue, 14 Mar 2023 12:20:14 +0530

> Attaching v47.

I have made the following changes in the attached patch (a) removed
the function IsIdxSafeToSkipDuplicates() and used the check directly
in the caller; (b) changed a few comments in the patch; (c) the test
file was inconsistently using ';' while executing statement with
safe_psql, changed it to remove ';'.

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Tue, 14 Mar 2023 10:18:08 +0300

Hi Amit, all

Amit Kapila <[email-masked]>, 14 Mar 2023 Sal, 09:50 tarihinde
şunu yazdı:

> in the caller

Should be fine, we can re-introduce this function when I work on the
non-pkey/RI unique index improvement as a follow up to this.

> ; (b) changed a few comments in the patch;

Thanks, looks good.

> (c) the test
Alright, thanks.

And as a self-review, when I write regression tests next time, I'll spend a
lot
more time on the style/consistency/comments etc. During this review,
the reviewers had to spend many cycles on that area, which is something
I should have done better.

Attaching v49 with some minor changes Shi Yu noted earlier.

Thanks,
Onder KALACI