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

On Thu, Mar 9, 2023 at 3:26 PM Önder Kalacı <onderkalaci(at)gmail(dot)com> wrote:
>
>>
>> 4.
>> +# Testcase start: SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN
>>
>> 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.
>

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

> 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)?
>

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.

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

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.