On Fri, Sep 13, 2019 at 7:20 AM Amit Khandekar <amitdkhan...@gmail.com> wrote: > > Thanks for notifying about this. Will work this week on rebasing this > > patchset and putting it into the 2019-11 commit fest. > > Rebased patch set attached. > > Added in the Nov commitfest : https://commitfest.postgresql.org/25/2283/
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. - 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. - 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. - 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? - cmp_ok() message in check_slots_dropped() has trailing whitespace. - make_slot_active() and check_slots_dropped(), at least, use global variables; is that really necessary? - 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. - $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. - 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. - 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. - The code in that function verifies that oldestXid is found in pg_controldata's output, but does not check the same for NextXID. - Is there a reason the code in that function prints debugging output? Seems like a leftover. - 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company