Re: Sync Rep v17

On Thu, 2011-02-24 at 22:08 +0900, Fujii Masao wrote:
> On Tue, Feb 22, 2011 at 2:38 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> > I've read about two-tenths of the patch, so I'll submit another comments
> > about the rest later. Sorry for the slow reviewing...
> 
> Here are another comments:

Thanks for your comments
Code available at git://github.com/simon2ndQuadrant/postgres.git

> +		{"synchronous_standby_names", PGC_SIGHUP, WAL_REPLICATION,
> +			gettext_noop("List of potential standby names to synchronise with."),
> +			NULL,
> +			GUC_LIST_INPUT | GUC_IS_NAME
> 
> Why did you add GUC_IS_NAME here? I don't think that it's reasonable
> to limit the length of this parameter to 63. Because dozens of standby
> names might be specified in the parameter.

OK, misunderstanding by me causing bug. Fixed

> SyncRepQueue->qlock should be initialized by calling SpinLockInit?

Fixed

> + * Portions Copyright (c) 2010-2010, PostgreSQL Global Development Group
>
> Typo: s/2010/2011

Fixed

> sync_replication_timeout_client would mess up the "wait-forever" option.
> So, when allow_standalone_primary is disabled, ISTM that
> sync_replication_timeout_client should have no effect.

Agreed, done.

> Please check max_wal_senders before calling SyncRepWaitForLSN for
> non-replication case.

SyncRepWaitForLSN() handles this

> SyncRepRemoveFromQueue seems not to be as short-term as we can
> use the spinlock. Instead, LW lock should be used there.
> 
> +			old_status = get_ps_display(&len);
> +			new_status = (char *) palloc(len + 21 + 1);
> +			memcpy(new_status, old_status, len);
> +			strcpy(new_status + len, " waiting for sync rep");
> +			set_ps_display(new_status, false);
> +			new_status[len] = '\0'; /* truncate off " waiting" */
> 
> Updating the PS display should be skipped if update_process_title is false.

Fixed.

> +		/*
> +		 * XXX extra code needed here to maintain sorted invariant.
> 
> Yeah, such a code is required. I think that we can shorten the time
> it takes to find an insert position by searching the list backwards.
> Because the given LSN is expected to be relatively new in the queue.

Sure, just skipped that because of time pressure. Will add.

> +		 * Our approach should be same as racing car - slow in, fast out.
> +		 */
> 
> Really? Even when removing the entry from the queue, we need
> to search the queue as well as we do in the add-entry case.
> Why don't you make walsenders remove the entry from the queue,
> instead?

This models wakeup behaviour of LWlocks

> +	long		timeout = SyncRepGetWaitTimeout();
> <snip>
> +			bool timeout = false;
> <snip>
> +			else if (timeout > 0 &&
> +				TimestampDifferenceExceeds(GetCurrentTransactionStopTimestamp(),
> +											now, timeout))
> +			{
> +				release = true;
> +				timeout = true;
> +			}
> 
> You seem to mix up two "timeout" variables.

Yes, good catch. Fixed.

> +			if (proc->lwWaitLink == MyProc)
> +			{
> +				/*
> +				 * Remove ourselves from middle of queue.
> +				 * No need to touch head or tail.
> +				 */
> +				proc->lwWaitLink = MyProc->lwWaitLink;
> +			}
> 
> When we find ourselves, we should break out of the loop soon,
> instead of continuing the loop to the end?

Incorporated in Yeb's patch

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