Обсуждение: FOR [SHARE|UPDATE] NOWAIT may still block in EvalPlanQualFetch
FOR SHARE|UPDATE NOWAIT will still block if they have to follow a ctid chain because the call to EvalPlanQualFetch doesn't take a param for noWait, so it doesn't know not to block if the updated row can't be locked. The attached patch against master includes an isolationtester spec to demonstrate this issue and a proposed fix. Builds with the fix applied pass "make check" and isolationtester "make installcheck". To reach this point you need to apply the patch in http://www.postgresql.org/message-id/51FB4305.3070600@2ndquadrant.com first, otherwise you'll get stuck there and won't touch the code changed in this patch. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
On Fri, Aug 2, 2013 at 04:00:03PM +0800, Craig Ringer wrote: > FOR SHARE|UPDATE NOWAIT will still block if they have to follow a ctid > chain because the call to EvalPlanQualFetch doesn't take a param for > noWait, so it doesn't know not to block if the updated row can't be locked. > > The attached patch against master includes an isolationtester spec to > demonstrate this issue and a proposed fix. Builds with the fix applied > pass "make check" and isolationtester "make installcheck". > > To reach this point you need to apply the patch in > http://www.postgresql.org/message-id/51FB4305.3070600@2ndquadrant.com > first, otherwise you'll get stuck there and won't touch the code changed > in this patch. The above looks like a legitimate patch that was not applied: http://www.postgresql.org/message-id/51FB6703.9090801@2ndquadrant.com The patch mentioned in the text above was applied, I think. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 02/01/2014 05:28 AM, Bruce Momjian wrote: > On Fri, Aug 2, 2013 at 04:00:03PM +0800, Craig Ringer wrote: >> FOR SHARE|UPDATE NOWAIT will still block if they have to follow a ctid >> chain because the call to EvalPlanQualFetch doesn't take a param for >> noWait, so it doesn't know not to block if the updated row can't be locked. >> >> The attached patch against master includes an isolationtester spec to >> demonstrate this issue and a proposed fix. Builds with the fix applied >> pass "make check" and isolationtester "make installcheck". >> >> To reach this point you need to apply the patch in >> http://www.postgresql.org/message-id/51FB4305.3070600@2ndquadrant.com >> first, otherwise you'll get stuck there and won't touch the code changed >> in this patch. > > The above looks like a legitimate patch that was not applied: > > http://www.postgresql.org/message-id/51FB6703.9090801@2ndquadrant.com > > The patch mentioned in the text above was applied, I think. The first patch, linked to in text, was commited as 706f9dd914c64a41e06b5fbfd62d6d6dab43eeb8. I can't see the second in the history either. It'd be good to get it committed, though the issue is obviously not causing any great outcry. It was detected when testing a high-rate queueing system in PostgreSQL. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Hi, On 2014-01-31 16:28:08 -0500, Bruce Momjian wrote: > On Fri, Aug 2, 2013 at 04:00:03PM +0800, Craig Ringer wrote: > > FOR SHARE|UPDATE NOWAIT will still block if they have to follow a ctid > > chain because the call to EvalPlanQualFetch doesn't take a param for > > noWait, so it doesn't know not to block if the updated row can't be locked. > > > > The attached patch against master includes an isolationtester spec to > > demonstrate this issue and a proposed fix. Builds with the fix applied > > pass "make check" and isolationtester "make installcheck". > > > > To reach this point you need to apply the patch in > > http://www.postgresql.org/message-id/51FB4305.3070600@2ndquadrant.com > > first, otherwise you'll get stuck there and won't touch the code changed > > in this patch. > > The above looks like a legitimate patch that was not applied: > > http://www.postgresql.org/message-id/51FB6703.9090801@2ndquadrant.com > > The patch mentioned in the text above was applied, I think. Craig: I think you should add this to the next CF. Seems like something we should fix, but which isn't super urgent. But when the skip locked stuff comes in it'll be more relevant. Might also need a rebase. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Craig Ringer wrote: > FOR SHARE|UPDATE NOWAIT will still block if they have to follow a ctid > chain because the call to EvalPlanQualFetch doesn't take a param for > noWait, so it doesn't know not to block if the updated row can't be locked. Applied with some further editorialization. In another thread, I wrote in response to Thomas Munro: > I tried Craig's patch with your test case and found that it stalls in > XactLockTableWait inside EPQFetch because it doesn't throw an error in > the noWait case before waiting. I think I will fix that and push, > including both test cases. Done. I also made the noWait case try a ConditionalXactLockTableWait before raising an error, because there's a small chance that the updating transaction aborts just before we check; in this case there is no need to raise an error. > I am wondering about backpatching Craig's fix. It looks to me like it > should be backpatched as far back as NOWAIT exists, but that was in 8.1 > and we haven't ever gotten a complaint until Craig's report AFAIK, which > I understand wasn't coming from a user finding a problem but rather some > new development. So I hesitate. On further reflection, the reason users don't complain is that it's quite difficult to notice that there is an issue. And on the other hand, since they are already specifying NOWAIT in the query, surely they must already be expecting an error to be raised; it's not like this introduces a new failure mode. My inclination would be to backpatch the fix all the way back. However, due to time constraints (because it fails to apply cleanly to older branches) and due to lack of user complaints, I only backpatched to 9.4. We can always revisit that, if there's demand. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services