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

Hi Craig,

On 05/09/16 11:28, Craig Ringer wrote:
> 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.
>

Here is my review of them.

> 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.

> + /*
> + * 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.

> 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).

> 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.

> 4. Send catalog_xmin separately in hot_standby_feedback messages
>

This looks okay (provided the change above).

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

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