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@2ndquadrant.com> wrote:
>> On 5 September 2016 at 12:40, Craig Ringer <craig@2ndquadrant.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