Thread: Re: lock_timeout GUC patch - Review
Hi, I've been reviewing this patch for the last few days. Here it is : * Submission review * Is the patch in context diff format? Yes * Does it apply cleanly to the current CVS HEAD? Yes * Does it include reasonable tests, necessary doc patches, etc? Doc patches are there. There are no regression tests. Should there be ? * Usability review * Does the patch actually implement that? Yes * Do we want that? I think so. At least I'd like to have this feature :) * Do we already have it? No * Does it follow SQL spec, or the community-agreed behavior? I didn't see a clear conclusion from the -hackers thread on this (GUC vs SQL statement extension) * Does it include pg_dump support (if applicable)? Not applicable. Or should pg_dump and/or pg_restore put lock_timeout to 0 ? * Are there dangers? As it is a guc, it could be set globally, is that a danger ? * Have all the bases been covered? I honestly don't know. It touches alarm signal handling. * Apply the patch, compile it and test: * Does the feature work as advertised? I only tested it with Linux. The code is very OS-dependent. It works as advertised with Linux. I found only one corner case (see below) * Are there corner cases the author has failed to consider? The feature almost works as advertised : it fails when lock_timeout = deadlock_timeout. Then the lock_timeout isn't detected. I think this case isn't considered in handle_sig_alarm(). * Are there any assertion failures or crashes? No * Performance review * Does the patch slow down simple tests? No * If it claims to improve performance, does it? Not applicable * Does it slow down other things? No. Maybe alarm signal handling and enabling will be slower, as there is more work done there (for instance, a GetCurrentTimestamp, that was only done when log_lock_waits was activated until now. But I couldn't measure a slowdown. * Read the changes to the code in detail and consider: * Does it follow the project coding guidelines? I think so * Are there portability issues? It seems to have already been adressed, from the previous discussion in the thread. * Will it work on Windows/BSD etc? It should. I couldn't test it though. Infrastructure is there. * Are the comments sufficient and accurate? Yes * Does it do what it says, correctly? Yes * Does it produce compiler warnings? No * Can you make it crash? No * Consider the changes to the code in the context of the project as a whole: * Is everything done in a way that fits togethercoherently with other features/modules? I have a feeling that enable_sig_alarm/enable_sig_alarm_for_lock_timeout tries to solve a very specific problem, and it gets complicated because there is no infrastructure in the code to handle several timeouts at the same time with sigalarm, so each timeout has its own dedicated and intertwined code. But I'm still discovering this part of the code. * Are there interdependencies that can cause problems? I don't think so.
Hi, first, thanks for the review. > Hi, I've been reviewing this patch for the last few days. Here it is : > > * Submission review > * Is the patch in context diff format? > Yes > > * Does it apply cleanly to the current CVS HEAD? > Yes > > * Does it include reasonable tests, necessary doc patches, etc? > Doc patches are there. > There are no regression tests. Should there be ? IIRC, there was a discussion/patch about parallel psql that can hold more than one connections open. With that feature, a regression test can be added. Reading the 9.0beta3 docs, it's not there and this patch is not on the current commitfest either. Is there anyone who knows the status of this feature? > * Usability review > * Does the patch actually implement that? > Yes > > * Do we want that? > I think so. At least I'd like to have this feature :) :-) > * Do we already have it? > No > > * Does it follow SQL spec, or the community-agreed behavior? > I didn't see a clear conclusion from the -hackers thread on this (GUC > vs SQL statement extension) > > * Does it include pg_dump support (if applicable)? > Not applicable. Or should pg_dump and/or pg_restore put lock_timeout to 0 > ? > > * Are there dangers? > As it is a guc, it could be set globally, is that a danger ? It could be set globally, but it will exhibit a new global behaviour. Which is not unexpected. The problem may come from [auto]vacuum processes can get stopped. Maybe others, too. A previous version contained a checking function that refused to set it from postgresql.conf for this reason, but it was frowned upon by Tom. :-) The proper fix would be that every such processes should set this GUC to zero for them locally. > * Have all the bases been covered? > I honestly don't know. It touches alarm signal handling. > * Apply the patch, compile it and test: > * Does the feature work as advertised? > I only tested it with Linux. The code is very OS-dependent. It works > as advertised with Linux. I found only one corner case (see below) The setitimer() function is implemented in backend/port/win32/timer.c, so it's abstracted away. With that in mind, I think there's no OS-dependent in this patch. > * Are there corner cases the author has failed to consider? > The feature almost works as advertised : it fails when lock_timeout = > deadlock_timeout. Then the lock_timeout isn't detected. I think this > case isn't considered in handle_sig_alarm(). I will look into this, thanks for spotting it. > * Are there any assertion failures or crashes? > No > > > * Performance review > * Does the patch slow down simple tests? > No > > * If it claims to improve performance, does it? > Not applicable > > * Does it slow down other things? > No. Maybe alarm signal handling and enabling will be slower, as there > is more work done there (for instance, a GetCurrentTimestamp, that > was only done when log_lock_waits was activated until now. But I > couldn't measure a slowdown. > > * Read the changes to the code in detail and consider: > * Does it follow the project coding guidelines? > I think so > > * Are there portability issues? > It seems to have already been adressed, from the previous discussion > in the thread. > > * Will it work on Windows/BSD etc? > It should. I couldn't test it though. Infrastructure is there. > > * Are the comments sufficient and accurate? > Yes > > * Does it do what it says, correctly? > Yes > > * Does it produce compiler warnings? > No > > * Can you make it crash? > No > > * Consider the changes to the code in the context of the project as a > whole: > * Is everything done in a way that fits together coherently with > other features/modules? > I have a feeling that > enable_sig_alarm/enable_sig_alarm_for_lock_timeout tries to solve a > very specific problem, and it gets complicated because there is no > infrastructure in the code to handle several timeouts at the same time > with sigalarm, so each timeout has its own dedicated and intertwined > code. But I'm still discovering this part of the code. There is a problem with setitimer(): only one timer can be alive at one time with the same timer id (ITIMER_REAL). > * Are there interdependencies that can cause problems? > I don't think so. Thanks for the review, I will post a new patch sometime next week. Best regards, Zoltán Böszörményi
Hi, Marc Cousin írta: > Hi, I've been reviewing this patch for the last few days. Here it is : > ... > * Are there dangers? > As it is a guc, it could be set globally, is that a danger ? > I haven't added any new code covering supplemental (e.g. autovacuum) processes, the interactions are yet to be discovered. Setting it globally is not recommended. > * Are there corner cases the author has failed to consider? > The feature almost works as advertised : it fails when lock_timeout = > deadlock_timeout. Then the lock_timeout isn't detected. I think this > case isn't considered in handle_sig_alarm(). > I fixed this by adding CheckLockTimeout() function that works like CheckStatementTimeout() and ensuring that the same start time is used for both deadlock_timeout and lock_timeout if both are active. The preference of errors if their timeout values are equal is: statement_timeout > lock_timeout > deadlock_timeout > * Consider the changes to the code in the context of the project as a whole: > * Is everything done in a way that fits together coherently with > other features/modules? > I have a feeling that > enable_sig_alarm/enable_sig_alarm_for_lock_timeout tries to solve a > very specific problem, and it gets complicated because there is no > infrastructure in the code to handle several timeouts at the same time > with sigalarm, so each timeout has its own dedicated and intertwined > code. But I'm still discovering this part of the code. > I tried to create a framework that could potentially handle any number timeouts ordered by their fin_time but it doesn't survive "make check", it reliably stalls at the test in parallel_schedule below: # ---------- # Another group of parallel tests # ---------- test: select_into select_distinct select_distinct_on select_implicit select_having subselect union case join aggregates transactions random portals arrays btree_index hash_index update namespace prepared_xacts delete This WIP patch is also attached for reference, too. I would prefer this way, but I don't have more time to work on it and there are some interdependencies in the signal handler when e.g. disable_sig_alarm(true); means to disable ALL timers not just the statement_timeout. The specifically coded lock_timeout patch goes with the flow and doesn't change the semantics and works. If someone wants to pick up the timer framework patch and can make it work, fine. But I am not explicitely submitting it for the commitfest. The original patch with the fixes works and needs only a little more review. Best regards, Zoltán Böszörményi
Attachment
The Thursday 29 July 2010 13:55:38, Boszormenyi Zoltan wrote : > I fixed this by adding CheckLockTimeout() function that works like > CheckStatementTimeout() and ensuring that the same start time is > used for both deadlock_timeout and lock_timeout if both are active. > The preference of errors if their timeout values are equal is: > statement_timeout > lock_timeout > deadlock_timeout As soon as lock_timeout is bigger than deadlock_timeout, it doesn't work, with this new version. Keeping the deadlock_timeout to 1s, when lock_timeout >= 1001, lock_timeout doesn't trigger anymore. > > > * Consider the changes to the code in the context of the project as a whole: > > * Is everything done in a way that fits together coherently with > > other features/modules? > > I have a feeling that > > enable_sig_alarm/enable_sig_alarm_for_lock_timeout tries to solve a > > very specific problem, and it gets complicated because there is no > > infrastructure in the code to handle several timeouts at the same time > > with sigalarm, so each timeout has its own dedicated and intertwined > > code. But I'm still discovering this part of the code. > > > > This WIP patch is also attached for reference, too. I would prefer > this way, but I don't have more time to work on it and there are some > interdependencies in the signal handler when e.g. disable_sig_alarm(true); > means to disable ALL timers not just the statement_timeout. > The specifically coded lock_timeout patch goes with the flow and doesn't > change the semantics and works. If someone wants to pick up the timer > framework patch and can make it work, fine. But I am not explicitely > submitting it for the commitfest. The original patch with the fixes works > and needs only a little more review. Ok, understood. But I like the principle of this framework much more (the rest of the code seems simpler to me as a consequence of this framework). But it goes far beyond the initial intent of the patch.
Excerpts from Boszormenyi Zoltan's message of jue jul 29 07:55:38 -0400 2010: > Hi, > > Marc Cousin írta: > > Hi, I've been reviewing this patch for the last few days. Here it is : > > > ... > > * Are there dangers? > > As it is a guc, it could be set globally, is that a danger ? > > > > I haven't added any new code covering supplemental (e.g. autovacuum) > processes, > the interactions are yet to be discovered. Setting it globally is not > recommended. FWIW there is some code in autovacuum and other auxiliary processes that forcibly sets statement_timeout to 0. I think this patch should do likewise. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Marc Cousin írta: > The Thursday 29 July 2010 13:55:38, Boszormenyi Zoltan wrote : > >> I fixed this by adding CheckLockTimeout() function that works like >> CheckStatementTimeout() and ensuring that the same start time is >> used for both deadlock_timeout and lock_timeout if both are active. >> The preference of errors if their timeout values are equal is: >> statement_timeout > lock_timeout > deadlock_timeout >> > > As soon as lock_timeout is bigger than deadlock_timeout, it doesn't > work, with this new version. > > Keeping the deadlock_timeout to 1s, when lock_timeout >= 1001, > lock_timeout doesn't trigger anymore. > I missed one case when the lock_timeout_active should have been set but the timer must not have been re-set, this caused the problem. I blame the hot weather and having no air conditioning. The second is now fixed. :-) I also added one line in autovacuum.c to disable lock_timeout, in case it's globally set in postgresq.conf as per Alvaro's comment. Also, I made sure that only one or two timeout causes (one of deadlock_timeout and lock_timeout in the first case or statement_timeout plus one of the other two) can be active at a time. Previously I was able to trigger a segfault with the default 1sec deadlock_timeout and lock_timeout = 999 or 1001. Effectively, the system's clock resolution makes the lock_timeout and deadlock_timeout equal and RemoveFromWaitQueue() was called twice. This way it's a lot more robust. Best regards, Zoltán Böszörményi
Attachment
Boszormenyi Zoltan írta: > Marc Cousin írta: > >> The Thursday 29 July 2010 13:55:38, Boszormenyi Zoltan wrote : >> >> >>> I fixed this by adding CheckLockTimeout() function that works like >>> CheckStatementTimeout() and ensuring that the same start time is >>> used for both deadlock_timeout and lock_timeout if both are active. >>> The preference of errors if their timeout values are equal is: >>> statement_timeout > lock_timeout > deadlock_timeout >>> >>> >> As soon as lock_timeout is bigger than deadlock_timeout, it doesn't >> work, with this new version. >> >> Keeping the deadlock_timeout to 1s, when lock_timeout >= 1001, >> lock_timeout doesn't trigger anymore. >> >> > > I missed one case when the lock_timeout_active should have been set > but the timer must not have been re-set, this caused the problem. > I blame the hot weather and having no air conditioning. The second is > now fixed. :-) > > I also added one line in autovacuum.c to disable lock_timeout, > in case it's globally set in postgresq.conf as per Alvaro's comment. > > Also, I made sure that only one or two timeout causes (one of > deadlock_timeout > and lock_timeout in the first case or statement_timeout plus one of the > other two) > can be active at a time. A little clarification is needed. The above statement is not entirely true. There can be a case when all three timeout causes can be active, but only when deadlock_timeout is the smallest of the three. If the fin_time value for the another timeout cause is smaller than for deadlock_timeout then there's no point to make deadlock_timeout active. And in case deadlock_timeout triggers and CheckDeadLock() finds that there really is a deadlock then the possibly active lock_timeout gets disabled to avoid calling RemoveFromWaitQueue() twice. I hope the comments in the code explain it well. > Previously I was able to trigger a segfault > with the default > 1sec deadlock_timeout and lock_timeout = 999 or 1001. Effectively, the > system's > clock resolution makes the lock_timeout and deadlock_timeout equal and > RemoveFromWaitQueue() was called twice. This way it's a lot more robust. > Best regards, Zoltán Böszörményi
The Monday 02 August 2010 13:59:59, Boszormenyi Zoltan wrote : > > > > Also, I made sure that only one or two timeout causes (one of > > deadlock_timeout > > and lock_timeout in the first case or statement_timeout plus one of the > > other two) > > can be active at a time. > > A little clarification is needed. The above statement is not entirely true. > There can be a case when all three timeout causes can be active, but only > when deadlock_timeout is the smallest of the three. If the fin_time value > for the another timeout cause is smaller than for deadlock_timeout then > there's no point to make deadlock_timeout active. And in case > deadlock_timeout > triggers and CheckDeadLock() finds that there really is a deadlock then the > possibly active lock_timeout gets disabled to avoid calling > RemoveFromWaitQueue() twice. I hope the comments in the code explain it > well. > > > Previously I was able to trigger a segfault > > with the default > > 1sec deadlock_timeout and lock_timeout = 999 or 1001. Effectively, the > > system's > > clock resolution makes the lock_timeout and deadlock_timeout equal and > > RemoveFromWaitQueue() was called twice. This way it's a lot more robust. > > > Ok, I've tested this new version: This time, it's this case that doesn't work : Session 1 : lock a table Session 2 : set lock_timeout to a large value, just to make it obvious (10 seconds). It times out after 1 s (the deadlock timeout), returning 'could not obtain lock'. Of course, it should wait 10 seconds. I really feel that the timeout framework is the way to go here. I know what you said about it before, but I think that the current code is getting too complicated, with too many special cases all over. As this is only my second review and I'm sure I am missing things here, could someone with more experience have a look and give advice ? Cheers Marc
Marc Cousin <cousinmarc@gmail.com> wrote: > This time, it's this case that doesn't work : > I really feel that the timeout framework is the way to go here. Since Zoltán also seems to feel this way: http://archives.postgresql.org/message-id/4C516C3A.6090102@cybertec.at I wonder whether this patch shouldn't be rejected with a request that the timeout framework be submitted to the next CF. Does anyone feel this approach (without the framework) should be pursued further? -Kevin
Hi, Kevin Grittner írta: > Marc Cousin <cousinmarc@gmail.com> wrote: > > >> This time, it's this case that doesn't work : >> > > >> I really feel that the timeout framework is the way to go here. >> > > Since Zoltán also seems to feel this way: > > http://archives.postgresql.org/message-id/4C516C3A.6090102@cybertec.at > > I wonder whether this patch shouldn't be rejected with a request > that the timeout framework be submitted to the next CF. Does anyone > feel this approach (without the framework) should be pursued > further? > I certainly think so, the current scheme seems to be very fragile and doesn't want to be extended.
Boszormenyi Zoltan <zb@cybertec.at> wrote: > Kevin Grittner írta: >> I wonder whether this patch shouldn't be rejected with a request >> that the timeout framework be submitted to the next CF. Does >> anyone feel this approach (without the framework) should be >> pursued further? > > I certainly think so, the current scheme seems to be very fragile > and doesn't want to be extended. Sorry, I phrased that question in a rather confusing way; I'm not sure, but it sounds like you're in favor of dropping this approach and pursuing the timeout framework in the next CF -- is that right? -Kevin
On Mon, Aug 2, 2010 at 3:09 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > Marc Cousin <cousinmarc@gmail.com> wrote: > >> This time, it's this case that doesn't work : > >> I really feel that the timeout framework is the way to go here. > > Since Zoltán also seems to feel this way: > > http://archives.postgresql.org/message-id/4C516C3A.6090102@cybertec.at > > I wonder whether this patch shouldn't be rejected with a request > that the timeout framework be submitted to the next CF. Does anyone > feel this approach (without the framework) should be pursued > further? I think "Returned with Feedback" would be more appropriate than "Rejected", since we're asking for a rework, rather than saying - we just don't want this. But otherwise, +1. Generally, I'm of the opinion that patches needing significant rework should be set to "Returned with Feedback" and resubmitted for the next CF; otherwise, it just gets unmanageable. Our goal for a CF should be to review everything thoroughly, not to get everything committed. Otherwise, we end up with a never-ending train of what are effectively new patches popping up, and it becomes impossible to close out the CommitFest on time. Reviewers and committers end up getting slammed, and it's not very much fun for patch authors (who are trying to frantically do last-minute rewrites) either; nor is it good for the overall quality of code the ends up in our tree. Better to take a breather and then start fresh. (If you don't believe in committer fatigue, look at the review I gave Itagaki Takahiro on the partitioning patch in January vs. the review I gave in July. One of those reviews is a whole lot more specific, detailed, and accurate than the other one...) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> wrote: >> I wonder whether this patch shouldn't be rejected with a request >> that the timeout framework be submitted to the next CF. > I think "Returned with Feedback" would be more appropriate than > "Rejected", since we're asking for a rework, rather than saying - > we just don't want this. But otherwise, +1. Done. -Kevin