From: | Simon Riggs <simon(at)2ndQuadrant(dot)com> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
Cc: | Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, pgsql-hackers(at)postgresql(dot)org, Daniel Farina <daniel(at)heroku(dot)com> |
Subject: | Re: Sync Rep v17 |
Date: | 2011-02-28 18:40:24 |
Message-ID: | 1298918424.12992.1715.camel@ebony |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, 2011-02-24 at 22:08 +0900, Fujii Masao wrote:
> On Tue, Feb 22, 2011 at 2:38 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Simon Riggs | 2011-02-28 18:40:33 | Re: Sync Rep v17 |
Previous Message | Simon Riggs | 2011-02-28 18:39:48 | Re: Sync Rep v17 |