Re: Assertion failure in syncrep.c

Hi All,

While running some tests on REL9_2_STABLE branch, I saw an assertion
failure in syncrep.c. The stack trace looks like this:

frame #3: 0x00000001055a2da9
postgres`ExceptionalCondition(conditionName=0x000000010567b8c5,
errorType=0x00000001055ff193, fileName=0x000000010567b8f4, lineNumber=257)
+ 137 at assert.c:54
frame #4: 0x00000001053f19d2
postgres`SyncRepWaitForLSN(XactCommitLSN=XLogRecPtr at 0x00007fff5ab80d08)
+ 1410 at syncrep.c:257
frame #5: 0x00000001050f9a52 postgres`RecordTransactionCommit + 1586 at
xact.c:1221
frame #6: 0x00000001050f3e5f postgres`CommitTransaction + 303 at
xact.c:1920
frame #7: 0x00000001050f38f4 postgres`CommitTransactionCommand + 180 at
xact.c:2588
frame #8: 0x000000010543fd8e postgres`finish_xact_command + 126 at
postgres.c:2409
<snip>

The failing assertion is this:

251 /*
252 * WalSender has checked our LSN and has removed us from queue.
Clean up
253 * state and leave. It's OK to reset these shared memory fields
without
254 * holding SyncRepLock, because any walsenders will ignore us
anyway when
255 * we're not on the queue.
256 */
257 Assert(SHMQueueIsDetached(&(MyProc->syncRepLinks)));

My theory is, and I did validate by using breakpoints in gdb, the code
in SyncRepWakeQueue() sets the proc->syncRepState to SYNC_REP_WAIT_COMPLETE.

564 /*
565 * Set state to complete; see SyncRepWaitForLSN() for
discussion of
566 * the various states.
567 */
568 thisproc->syncRepState = SYNC_REP_WAIT_COMPLETE;
569
570 /*
571 * Remove thisproc from queue.
572 */
573 SHMQueueDelete(&(thisproc->syncRepLinks));

But before walsender could detach the proc from the syncRepLinks shared
queue, the backend checks for the MyProc->syncRepState and decides not to
sleep after all and goes on to check the above Assert. The assertion fails
because the proc is not yet removed from the queue.

182 syncRepState = MyProc->syncRepState;
183 if (syncRepState == SYNC_REP_WAITING)
184 {
185 LWLockAcquire(SyncRepLock, LW_SHARED);
186 syncRepState = MyProc->syncRepState;
187 LWLockRelease(SyncRepLock);
188 }
189 if (syncRepState == SYNC_REP_WAIT_COMPLETE)
190 break;
191

We could just remove the assert, but then there is another similar assert
at the start of the function and it may hit for similar reasons (though
very unlikely). Instead, we could just reverse the order of setting
syncRepState and removing proc from syncRepLinks queue. There is a very
rare possibility that the proc is removed from the Q, but before its state
is changed, walsender crashes. I wonder if that cause infinite wait for the
process.

BTW, even though I was working on 9_2_STABLE branch, looks like issue
exists in the master branch too.

Thanks,
Pavan

--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee

On Thu, Sep 18, 2014 at 12:02 PM, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com>
wrote:

> Hi All,
>
> While running some tests on REL9_2_STABLE branch, I saw an assertion
> failure in syncrep.c. The stack trace looks like this:
>
>
Any comments on this? I see it very regularly during my pgbench tests.

Thanks,
Pavan

--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee

Pavan,

* Pavan Deolasee (pavan(dot)deolasee(at)gmail(dot)com) wrote:
> On Thu, Sep 18, 2014 at 12:02 PM, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com>
> wrote:
> > While running some tests on REL9_2_STABLE branch, I saw an assertion
> > failure in syncrep.c. The stack trace looks like this:
> >
> Any comments on this? I see it very regularly during my pgbench tests.

I agree that it looks like there may be a race condition there- but
could you provide the test cases you're working with? What kind of
behavior is it that's making it show up easily for you?

Thanks!

Stephen

On Mon, Sep 22, 2014 at 6:50 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> Pavan,
>
> * Pavan Deolasee (pavan(dot)deolasee(at)gmail(dot)com) wrote:
> > On Thu, Sep 18, 2014 at 12:02 PM, Pavan Deolasee <
> pavan(dot)deolasee(at)gmail(dot)com>
> > wrote:
> > > While running some tests on REL9_2_STABLE branch, I saw an assertion
> > > failure in syncrep.c. The stack trace looks like this:
> > >
> > Any comments on this? I see it very regularly during my pgbench tests.
>
> I agree that it looks like there may be a race condition there- but
> could you provide the test cases you're working with? What kind of
> behavior is it that's making it show up easily for you?
>
>
Nothing special really. Set up a 2-node sync replication on my Mac laptop
and running pgbench with 10 clients triggers it. As I said, after looking
at the code and realising that there is a race condition, I tried with with
gdb to reproduce the race I suspect.

Anyways, the attached patch should trigger the race condition for a simple
query. I'm deliberately making backend to wait to give walsender a chance
to send outstanding WALs and then making walsender to wait so that
assertion is triggered in the backend.

Hope this helps.

Thanks,
Pavan
--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee

Pavan,

* Pavan Deolasee (pavan(dot)deolasee(at)gmail(dot)com) wrote:
> On Mon, Sep 22, 2014 at 6:50 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I agree that it looks like there may be a race condition there- but
> > could you provide the test cases you're working with? What kind of
> > behavior is it that's making it show up easily for you?
> >
> Nothing special really. Set up a 2-node sync replication on my Mac laptop
> and running pgbench with 10 clients triggers it. As I said, after looking
> at the code and realising that there is a race condition, I tried with with
> gdb to reproduce the race I suspect.

Well, that's a bit concerning.

> Anyways, the attached patch should trigger the race condition for a simple
> query. I'm deliberately making backend to wait to give walsender a chance
> to send outstanding WALs and then making walsender to wait so that
> assertion is triggered in the backend.

Great, thanks, I'll try and look into this tomorrow.

Stephen

On 18 September 2014 07:32, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com> wrote:

> 564 /*
> 565 * Set state to complete; see SyncRepWaitForLSN() for discussion
> of
> 566 * the various states.
> 567 */
> 568 thisproc->syncRepState = SYNC_REP_WAIT_COMPLETE;
> 569
> 570 /*
> 571 * Remove thisproc from queue.
> 572 */
> 573 SHMQueueDelete(&(thisproc->syncRepLinks));

Yes, looks like a bugette to me also.

Unlikely to occur except when no network involved, and even more
luckily no effect at all when Assert is not enabled. So not a priority
fix.

But fix looks trivial, just to switch around the two statements above.

I'll backpatch tomorrow.

Thanks

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