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

On 5 September 2016 at 14:44, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 5 September 2016 at 12:40, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> Hi all
>>
>> Currently hot standby feedback sends GetOldestXmin()'s result to the
>> upstream as the required xmin. GetOldestXmin() returns a slot's
>> catalog_xmin if that's the lowest xmin on the system.
>
>
> Note that this patch changes the API to GetOldestXmin(), adding a new
> boolean to allow it to disregard the catalog_xmin of slots.
>
> Per Simon's feedback I'm going to split that out into a separate
> patch, so will post a follow-up split one soon as the series.

Now formatted a series:

1. Send catalog_xmin in hot standby feedback protocol
2. Make walsender respect catalog_xmin in hot standby feedback messages
3. Allow GetOldestXmin(...) to optionally disregard the catalog_xmin
4. Send catalog_xmin separately in hot_standby_feedback messages

Descriptions are in the patch headers.

1 adds the protocol field only. The value is at this point always sent
as 0 by walreceiver and ignored by walsender. There's need to handle
backward compatibility in the addition to the hot standby protocol
message here as only the same major version Pg has any business
sending us hot standby feedback. pg_receivexlog, pg_basebackup etc
don't use hs feedback. Includes protocol docs change.

2 makes walsender now pay attention to the sent catalog_xmin.
walreceiver doesn't set it yet and has no way to get it separately.

3 Provides a way to get the global xmin without considering the
catalog_xmin so walreceiver can use it.

4 makes walsender use the modified GetOldestXmin()

(3) needs additional attention:

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 cut out some comment changes to be submitted separately; otherwise
this series is much the same as the original patch upthread.

Also available at
https://github.com/2ndQuadrant/postgres/tree/dev/feedback-catalog-xmin
(and tagged dev/feedback-catalog-xmin). Branch subject to rebasing.

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