Skip site navigation (1) Skip section navigation (2)

Re: Sync Rep v17

From: Yeb Havinga <yebhavinga(at)gmail(dot)com>
To: Jaime Casanova <jaime(at)2ndquadrant(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, 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-25 15:41:56
Message-ID: 4D67CDC4.3010203@gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
On 2011-02-22 20:43, Jaime Casanova wrote:
>
> you can make this happen more easily, i just run "pgbench -n -c10 -j10
> test" and qot that warning and sometimes a segmentation fault and
> sometimes a failed assertion
>
> and the problematic code starts at
> src/backend/replication/syncrep.c:277, here my suggestions on that
> code.
> still i get a failed assertion because of the second Assert (i think
> we should just remove that one)
>
> *************** SyncRepRemoveFromQueue(void)
> *** 288,299 ****
>
>                          if (proc->lwWaitLink == NULL)
>                                  elog(WARNING, "could not locate
> ourselves on wait queue");
> !                       proc = proc->lwWaitLink;
>                  }
>
>                  if (proc->lwWaitLink == NULL)   /* At tail */
>                  {
> !                       Assert(proc == MyProc);
>                          /* Remove ourselves from tail of queue */
>                          Assert(queue->tail == MyProc);
>                          queue->tail = proc;
> --- 288,300 ----
>
>                          if (proc->lwWaitLink == NULL)
>                                  elog(WARNING, "could not locate
> ourselves on wait queue");
> !                       else
> !                               proc = proc->lwWaitLink;
>                  }
>
>                  if (proc->lwWaitLink == NULL)   /* At tail */
>                  {
> !                       Assert(proc != MyProc);
>                          /* Remove ourselves from tail of queue */
>                          Assert(queue->tail == MyProc);
>                          queue->tail = proc;
>
I also did some initial testing on this patch and got the queue related 
errors with > 1 clients. With the code change from Jaime above I still 
got a lot of 'not on queue warnings'.

I tried to understand how the queue was supposed to work - resulting in 
the changes below that also incorporates a suggestion from Fujii 
upthread, to early exit when myproc was found.

With the changes below all seems to work without warnings. I now see 
that the note about the list invariant is too short, better was: "if 
queue length = 1 then head = tail"

--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -274,6 +274,8 @@ SyncRepRemoveFromQueue(void)
         }
         else
         {
+               bool found = false;
+
                 while (proc->lwWaitLink != NULL)
                 {
                         /* Are we the next proc in our traversal of the 
queue? */
@@ -284,17 +286,19 @@ SyncRepRemoveFromQueue(void)
                                  * No need to touch head or tail.
                                  */
                                 proc->lwWaitLink = MyProc->lwWaitLink;
+                               found = true;
+                               break;
                         }

-                       if (proc->lwWaitLink == NULL)
-                               elog(WARNING, "could not locate 
ourselves on wait queue");
                         proc = proc->lwWaitLink;
                 }
+               if (!found)
+                       elog(WARNING, "could not locate ourselves on 
wait queue");

-               if (proc->lwWaitLink == NULL)   /* At tail */
+               /* If MyProc was removed from the tail, maintain list 
invariant head==tail */
+               if (proc->lwWaitLink == NULL)
                 {
-                       Assert(proc == MyProc);
-                       /* Remove ourselves from tail of queue */
+                       Assert(proc != MyProc); /* impossible since that 
is the head=MyProc branch above */
                         Assert(queue->tail == MyProc);
                         queue->tail = proc;
                         proc->lwWaitLink = NULL;

I needed to add this to make the documentation compile

--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2010,6 +2010,9 @@ SET ENABLE_SEQSCAN TO OFF;
          You should also consider setting <varname>hot_standby_feedback</>
          as an alternative to using this parameter.
</para>
+ </listitem>
+ </varlistentry>
+ </variablelist></sect2>

<sect2 id="runtime-config-sync-rep">

regards,
Yeb Havinga


In response to

Responses

pgsql-hackers by date

Next:From: Tom LaneDate: 2011-02-25 15:46:57
Subject: Re: wCTE: why not finish sub-updates at the end, not the beginning?
Previous:From: Kevin GrittnerDate: 2011-02-25 15:30:42
Subject: Re: wCTE: why not finish sub-updates at the end, not the beginning?

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group