On Wed, 18 Sep 2019 at 19:34, Robert Haas <robertmh...@gmail.com> wrote: > I took a bit of a look at > 0004-New-TAP-test-for-logical-decoding-on-standby.patch and saw some > things I don't like in terms of general code quality: > > - Not many comments. I think each set of tests should have a block > comment at the top explaining clearly what it's trying to test. Done at initial couple of test groups so that the groups would be spotted clearly. Please check.
> - print_phys_xmin and print_logical_xmin don't print anything. > - They are also identical to each other except that they each operate > on a different hard-coded slot name. > - They are also identical to wait_for_xmins except that they don't wait. Re-worked this part of the code. Now a single function get_slot_xmins(slot_name) is used to return the slot's xmins's. It figures out by the slot name, whether the slot belongs to master or slave. Also, avoided the hardcoded 'master_physical' and 'standby_logical' names. Removed 'node' parameter of wait_for_xmins(), since now we can figure out node name from slot name. > - create_logical_slots creates two slots whose names are hard-coded > using code that is cut-and-pasted. > - The same code is also cut-and-pasted into two other places in the file. Didn't remove the hardcoding for slot names, because it's not convenient to return those from create_logical_slots() and use them in check_slots_dropped(). But I have cut-pasted code in create_logical_slots() and the other two places in the file. Now I have did some of that repeated code in create_logical_slots() itself. > - Why does that cut-and-pasted code use BAIL_OUT(), which aborts the > entire test run, instead of die, which just aborts the current test > file? Oops. Didn't realize that it bails out from the complete test run. Replaced it with die(). > - cmp_ok() message in check_slots_dropped() has trailing whitespace. Remove them. > - make_slot_active() and check_slots_dropped(), at least, use global > variables; is that really necessary? I guess you are referring to $handle. Now made make_slot_active() return this handle using it's return value, and used this to pass to check_slots_dropped(). Retained node_replica global variable rather than passing it as function param, because these functions always use node_replica, and never node_master. > - In particular, $return is used only in one function and doesn't need > to survive across calls; why is it not a local variable? > - Depending on whether $return ends up true or false, the number of > executed tests will differ; so besides any actual test failures, > you'll get complaints about not executing exactly 58 tests. Right. Made it local. > - $backup_name only ever has one value, but for some reason the > variable is created at the top of the test file and then initialized > later. Just do my $backup_name = 'b1' near where it's first used, or > ditch the variable and write 'b1' in each of the three places it's > used. Declared $backup_name near it's first usage. > - Some of the calls to wait_for_xmins() save the return values into > local variables but then do nothing with those values before they are > overwritten. Either it's wrong that we're saving them into local > variables, or it's wrong that we're not doing anything with them. Yeah, at many places, it was redundant to save them into variables, so removed the function return value assignment part at those places. > - test_oldest_xid_retention() is called only once; it basically acts > as a wrapper for one group of tests. You could argue against that > approach, but I actually think it's a nice style which makes the code > more self-documenting. However, it's not used consistently; all the > other groups of tests are written directly as toplevel code. Removed the function and kept it's code at top level code. I think the test group header comments look sufficient for documenting each group of tests, so that there is no need to make a separate function for each group. > - The code in that function verifies that oldestXid is found in > pg_controldata's output, but does not check the same for NextXID. Actually, there is no need to check NextID. We want to check just oldest_xid. Removed it's usage. > - Is there a reason the code in that function prints debugging output? > Seems like a leftover. Yeah, right. Removed them. > - I think it might be an idea to move the tests for recovery > conflict/slot drop to a separate test file, so that we have one file > for the xmin-related testing and another for the recovery conflict > testing. Actually in some of the conflict-recovery testcases, I am still using wait_for_xmins() so that we could test the xmin values back after we drop the slots. So xmin-related testing is embedded in these recovery tests as well. We can move the wait_for_xmins() function to some common file and then do the split of this file, but then effectively some of the xmin-testing would go into the recovery-related test file, which did not sound sensible to me. What do you say ? Attached patch series has the test changes addressed. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
logicaldecodng_standby_v2.tar.gz
Description: GNU Zip compressed data
