Hi Craig,

On 05/09/16 11:28, Craig Ringer wrote:
> On 5 September 2016 at 14:44, Craig Ringer <cr...@2ndquadrant.com> wrote:
>> On 5 September 2016 at 12:40, Craig Ringer <cr...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to