public inbox for [email protected]  

help / color / mirror / Atom feed

From: Robert Haas <[email protected]>
To: Amit Khandekar <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: tushar <[email protected]>
Cc: Petr Jelinek <[email protected]>
Cc: Craig Ringer <[email protected]>
Cc: Petr Jelinek <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Shaun Thomas <[email protected]>
Subject: Re: Minimal logical decoding on standbys
Date: Thu, 26 Sep 2019 16:27:04 -0400
Message-ID: <CA+TgmoY-dFfB0BfH4FHWPaEsGUecMvLx4dEGvS8p0u++TTi-vA@mail.gmail.com> (raw)
In-Reply-To: <CAJ3gD9fnyrhb5cFg=+bLy_55b_iKuGadN_QvE6XXP=s8KF6OWg@mail.gmail.com>
References: <CAJ3gD9egPPK4j=MYfzFeZAQqtFfGJhk2jU8JQihZx7wD121Yeg@mail.gmail.com>
	<[email protected]>
	<CAJ3gD9euKh2ChkBY75TsHhCr=T8rqFPx9ZKRKX0vNwt11MX7PQ@mail.gmail.com>
	<CAJ3gD9cBix52=vXVtzSATQai1pkT1=z+cG89BA0v3jt029RxuQ@mail.gmail.com>
	<CA+TgmobTRwvaQAE5RhcEs9xj7hO5-31betk=y6TAD1x8ZJdbhw@mail.gmail.com>
	<CAJ3gD9fnyrhb5cFg=+bLy_55b_iKuGadN_QvE6XXP=s8KF6OWg@mail.gmail.com>

On Thu, Sep 26, 2019 at 5:14 AM Amit Khandekar <[email protected]> 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

view thread (196+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Minimal logical decoding on standbys
  In-Reply-To: <CA+TgmoY-dFfB0BfH4FHWPaEsGUecMvLx4dEGvS8p0u++TTi-vA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox