>
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
