On Thu, Sep 26, 2019 at 5:14 AM Amit Khandekar <amitdkhan...@gmail.com> wrote: > 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 ?
I agree we don't want code duplication, but I think we could reduce the code duplication to a pretty small amount with a few cleanups. I don't think wait_for_xmins() looks very well-designed. It goes to trouble of returning a value, but only 2 of the 6 call sites pay attention to the returned value. I think we should change the function so that it doesn't return anything and have the callers that want a return value call get_slot_xmins() after wait_for_xmins(). And then I think we should turn around and get rid of get_slot_xmins() altogether. Instead of: my ($xmin, $catalog_xmin) = get_slot_xmins($master_slot); is($xmin, '', "xmin null"); is($catalog_xmin, '', "catalog_xmin null"); We can write: my $slot = $node_master->slot($master_slot); is($slot->{'xmin'}, '', "xmin null"); is($slot->{'catalog_xmin'}, '', "catalog xmin null"); ...which is not really any longer or harder to read, but does eliminate the need for one function definition. Then I think we should change wait_for_xmins so that it takes three arguments rather than two: $node, $slotname, $check_expr. With that and the previous change, we can get rid of get_node_from_slotname(). At that point, the body of wait_for_xmins() would consist of a single call to $node->poll_query_until() or die(), which doesn't seem like too much code to duplicate into a new file. Looking at it at a bit more, though, I wonder why the recovery conflict scenario is even using wait_for_xmins(). It's hard-coded to check the state of the master_physical slot, which isn't otherwise manipulated by the recovery conflict tests. What's the point of testing that a slot which had xmin and catalog_xmin NULL before the test started (line 414) and which we haven't changed since still has those values at two different points during the test (lines 432, 452)? Perhaps I'm missing something here, but it seems like this is just an inadvertent entangling of these scenarios with the previous scenarios, rather than anything that necessarily needs to be connected together. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company