NotificationsYou must be signed in to change notification settings
Fork28
Star153

Commit3420851

committed

Fix busted logic for parallel lock grouping in TopoSort().

A "break" statement erroneously left behind by commita1c1af2caused TopoSort to do the wrong thing if a lock's wait listcontained multiple members of the same locking group.Because parallel workers don't normally need any locks not alreadytaken by their leader, this is very hard --- maybe impossible ---to hit in production. Still, if it did happen, the queries involvedin an otherwise-resolvable deadlock would block until canceled.In addition to removing the bogus "break", add an Assert showingthat the conflicting uses of the beforeConstraints[] array (for bothcounts and flags) don't overlap, and add some commentary explainingwhy not; because it's not obvious without explanation, IMHO.Original report and patch from Rui Hai Jiang; additional assertand commentary by me. Back-patch to 9.6 where the bug came in.Discussion:https://postgr.es/m/CAEri+mLd3bpHLyW+a9pSe1y=aEkeuJpwBSwvo-+m4n7-ceRmXw@mail.gmail.com

1 parent1e2fddf commit3420851Copy full SHA for 3420851

File tree

1 file changed

-1

lines changed

src/backend/storage/lmgr
- deadlock.c

1 file changed

-1

lines changed

`‎src/backend/storage/lmgr/deadlock.c‎`

Lines changed: 7 additions & 1 deletion

Original file line number	Diff line number	Diff line change
`@@ -922,6 +922,12 @@ TopoSort(LOCK *lock,`
`922`	`922`	`* in the same lock group on the queue, set their number of`
`923`	`923`	`* beforeConstraints to -1 to indicate that they should be emitted`
`924`	`924`	`* with their groupmates rather than considered separately.`
	`925`	`+ *`
	`926`	`+ * In this loop and the similar one just below, it's critical that we`
	`927`	`+ * consistently select the same representative member of any one lock`
	`928`	`+ * group, so that all the constraints are associated with the same`
	`929`	`+ * proc, and the -1's are only associated with not-representative`
	`930`	`+ * members. We select the last one in the topoProcs array.`
`925`	`931`	`*/`
`926`	`932`	`proc=constraints[i].waiter;`
`927`	`933`	`Assert(proc!=NULL);`
`@@ -940,7 +946,6 @@ TopoSort(LOCK *lock,`
`940`	`946`	`Assert(beforeConstraints[j] <=0);`
`941`	`947`	`beforeConstraints[j]=-1;`
`942`	`948`	`}`
`943`		`-break;`
`944`	`949`	`}`
`945`	`950`	`}`
`946`	`951`
`@@ -977,6 +982,7 @@ TopoSort(LOCK *lock,`
`977`	`982`	`if (kk<0)`
`978`	`983`	`continue;`
`979`	`984`
	`985`	`+Assert(beforeConstraints[jj] >=0);`
`980`	`986`	`beforeConstraints[jj]++;/* waiter must come before */`
`981`	`987`	`/* add this constraint to list of after-constraints for blocker */`
`982`	`988`	`constraints[i].pred=jj;`

0 commit comments

Comments

(0)

Movatterモバイル変換

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Commit3420851

File tree

1 file changed

1 file changed

`‎src/backend/storage/lmgr/deadlock.c‎`

0 commit comments