Re: [PATCH] Send catalog_xmin separately in hot standby feedback

On 25 October 2016 at 00:19, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:

>> Now formatted a series:
>>
>> 1. Send catalog_xmin in hot standby feedback protocol
>
>> + xmin_epoch = nextEpoch;
>> if (nextXid < xmin)
>> - nextEpoch--;
>> + xmin_epoch --;
>> + catalog_xmin_epoch = nextEpoch;
>> + if (nextXid < catalog_xmin)
>> + catalog_xmin_epoch --;
>
> Don't understand why you keep the nextEpoch here, it's not used anywhere
> that I can see, you could just as well use the xmin_epoch directly if
> that's how you want to name it.

Fixed, thanks.

>> + /*
>> + * A 10.0+ standby's walsender passes the lowest catalog xmin of any
>> + * replication slot up to the master.
>> + */
>> + feedbackCatalogXmin = pq_getmsgint(&reply_message, 4);
>> + feedbackCatalogEpoch = pq_getmsgint(&reply_message, 4);
>>
>
> I'd be more interested to know why this is sent rather than it's sent
> since version 10+ in this comment.

Removed. It's explained in a comment inside the if
(hot_standby_feedback) block in walreceiver anyway.

>> 2. Make walsender respect catalog_xmin in hot standby feedback messages
>
>> + if (TransactionIdIsNormal(feedbackCatalogXmin)
>> + && TransactionIdPrecedes(feedbackCatalogXmin, feedbackXmin))
>> + {
>> + MyPgXact->xmin = feedbackCatalogXmin;
>> + }
>> + else
>> + {
>> + MyPgXact->xmin = feedbackXmin;
>> + }
>
> This not how we usually use the {} brackets (there are some more
> instances of using them for one line of code in this particular commit).

Whoops. Thanks. I find the Pg convention pretty ghastly when dealing
with multi-line 'if' conditions followed by a single-line statement,
but it's still the convention whether I like it or not.

>> 3. Allow GetOldestXmin(...) to optionally disregard the catalog_xmin
>> By ignoring slot catalog_xmin in the GetOldestXmin() call then
>> separately calling ProcArrayGetReplicationSlotXmin() to get the
>> catalog_xmin to we might produce a catalog xmin slightly later than
>> what was in the procarray at the time we previously called
>> GetOldestXmin() to examine backend/session state. ProcArrayLock is
>> released so it can be advanced in-between the calls. That's harmless -
>> it isn't necessary for the reported catalog_xmin to be exactly
>> consistent with backend state. If it advances it's safe to report the
>> new position since we know the confirmed positions are on-disk
>> locally.
>>
>> The alternative here is to extend GetOldestXmin() to take an out-param
>> to report the slot catalog xmin. That really just duplicates the
>> functionality of ProcArrayGetReplicationSlotXmin but means we can grab
>> it within a single ProcArray lock. Variants of GetOldestXmin and
>> ProcArrayGetReplicationSlotXmin that take an already-locked flag would
>> work too, but would hold the lock across parts of GetOldestXmin() that
>> currently don't retain it. I could also convert the current boolean
>> param ignoreVacuum into a flags argument instead of adding another
>> boolean. No real preference from me.
>>
>
> I would honestly prefer the change to GetOldestXmin to return the
> catalog_xmin. It seems both cleaner and does less locking.

Fair enough. Done.

> In general it's simpler patch than I expected which is good. But it
> would be good to have some tests.

Agreed. OK, I've added basic tests for physical replication slots and
hot_standby_feedback to t/001_stream_rep.pl since it make sense to
test both along with stream_rep.pl's tests of cascading, etc and I
don't think a separate test is needed.

It's not actually practical to add tests for the catalog_xmin on
standby functionality until the next patch in the series (pending)
which enables logical decoding on standby. Currently you can't create
a slot on a standby so you can't cause the standby to hold down
catalog_xmin. But the tests show things work how they should within
the range of currently exposed functionality.

In the process I noticed how skeletal those tests still are. We have a
great framework now (thanks Michael!) and I'd like to start filling it
out with tests involving unclean shutdowns, promotions, etc. There's a
lot still to write to get solid coverage. Tests aren't hard. Who's
keen to write some? I'll happily help any volunteers out.

New patch series attached. 0001 is the new tests. The guts is patches
2-5. I'm not sure whether 2, 3, 4 and 5 should be squashed for commit
or not, but I left them separate for easier review.

For complete functionality this series will want to be coupled with
logical decoding timeline following and a pending patch to enable
logical decoding on standby.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services