On Thu, Dec 29, 2016 at 10:41 AM, Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> I have done one more pass of the review today. I have few comments.
>
> + if (nextidx != INVALID_PGPROCNO)
> + {
> + /* Sleep until the leader updates our XID status. */
> + for (;;)
> + {
> + /* acts as a read barrier */
> + PGSemaphoreLock(&proc->sem);
> + if (!proc->clogGroupMember)
> + break;
> + extraWaits++;
> + }
> +
> + Assert(pg_atomic_read_u32(&proc->clogGroupNext) == INVALID_PGPROCNO);
> +
> + /* Fix semaphore count for any absorbed wakeups */
> + while (extraWaits-- > 0)
> + PGSemaphoreUnlock(&proc->sem);
> + return true;
> + }
>
> 1. extraWaits is used only locally in this block so I guess we can
> declare inside this block only.
>Agreed and changed accordingly. > 2. It seems that we have missed one unlock in case of absorbed > wakeups. You have initialised extraWaits with -1 and if there is one > extra wake up then extraWaits will become 0 (it means we have made one > extra call to PGSemaphoreLock and it's our responsibility to fix it as > the leader will Unlock only once). But it appear in such case we will > not make any call to PGSemaphoreUnlock. > Good catch! I have fixed it by initialising extraWaits to 0. This same issue exists from Group clear xid for which I will send a patch separately. Apart from above, the patch needs to be adjusted for commit be7b2848 which has changed the definition of PGSemaphore. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
group_update_clog_v10.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
