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
v38_0002_use_index_on_subs_when_pub_rep_ident_full.patch
Description: Binary data
v38_0001_use_index_on_subs_when_pub_rep_ident_full.patch
Description: Binary data
