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

Hi Amit, all

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

Alright, that is reasonable.

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

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

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

Makes sense

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

thanks, fixed

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

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

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

Attached v38.

Thanks,
Onder KALACI