Re: Sync Rep v19 - Mailing list pgsql-hackers

On Fri, 2011-03-04 at 00:02 +0900, Fujii Masao wrote: 
> > * synchronous_standby_names = "*" matches all standby names
> 
> Using '*' as the default seems to lead the performance degradation by
> being connected from unexpected synchronous standby.

You can configure it however you wish. It seemed better to have an out
of the box setting that was useful.

> > * pg_stat_replication now shows standby priority - this is an ordinal
> > number so "1" means 1st, "2" means 2nd etc, though 0 means "not a sync
> > standby".
> 
> monitoring.sgml should be updated.

Didn't think it needed to be, but I've added a few lines to explain.

> Though I've not read whole of the patch yet, here is the current comment:
> 
> Using MyProc->lwWaiting and lwWaitLink for backends to wait for replication
> looks fragile. Since they are used also by lwlock, the value of them can be
> changed unexpectedly. Instead, how about defining dedicated variables for
> replication?

Yes, I think the queue stuff needs a rewrite now.

> +        else if (WaitingForSyncRep)
> +        {
> +            /*
> +             * This must NOT be a FATAL message. We want the state of the
> +             * transaction being aborted to be indeterminate to ensure that
> +             * the transaction completion guarantee is never broken.
> +             */
> 
> The backend can reach this code path after returning the commit to the client.
> Instead, how about doing this in EndCommand, to close the connection before
> returning the commit?

OK, will look.

> +        LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);
> +        sync_priority = walsnd->sync_standby_priority;
> +        LWLockRelease(SyncRepLock);
> 
> LW_SHARE can be used here, instead.

Seemed easier to keep it simple and have all lockers use LW_EXCLUSIVE.
But I've changed it for you.

> +            /*
> +             * Wait no longer if we have already reached our LSN
> +             */
> +            if (XLByteLE(XactCommitLSN, queue->lsn))
> +            {
> +                /* No need to wait */
> +                LWLockRelease(SyncRepLock);
> +                return;
> +            }
> 
> It might take long to acquire SyncRepLock, so how about comparing
> our LSN with WalSnd->flush before here?

If we're not the sync standby and we need to takeover the role of sync
standby we may need to issue a wakeup even though our standby reached
that LSN some time before. So we need to check each time.

> replication_timeout_client depends on GetCurrentTransactionStopTimestamp().
> In COMMIT case, it's OK. But In PREPARE TRANSACTION, COMMIT PREPARED
> and ROLLBACK PREPARED cases, it seems problematic because they don't call
> SetCurrentTransactionStopTimestamp().

Shame on them!

Seems reasonable that they should call
SetCurrentTransactionStopTimestamp().

I don't want to make a special case there for prepared transactions.

> In SyncRepWaitOnQueue, the backend can theoretically call WaitLatch() again
> after the wake-up from the latch. In this case, the "timeout" should
> be calculated
> again. Otherwise, it would take unexpectedly very long to cause the timeout.

That was originally modelled on on the way the statement_timeout timer
works. If it gets nudged and wakes up too early it puts itself back to
sleep to wakeup at the same time again.

I've renamed the variables to make that clearer and edited slightly.

-- Simon Riggs           http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services