Re: Minimal logical decoding on standbys

On Wed, 18 Sep 2019 at 19:34, Robert Haas <robertmhaas@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