2012-04-23 15:08 keltezéssel, Marc Cousin írta:
On Mon, 2012-04-23 at 10:53 +0200, Boszormenyi Zoltan wrote:
2012-04-10 09:02 keltezéssel, Boszormenyi Zoltan írta:
2012-04-06 14:47 keltezéssel, Cousin Marc írta:
On 05/04/12 08:02, Boszormenyi Zoltan wrote:
2012-04-04 21:30 keltezéssel, Alvaro Herrera írta:
I think this patch is doing two things: first touching infrastructure
stuff and then adding lock_timeout on top of that. Would it work to
split the patch in two pieces?
Sure. Attached is the split version.
Best regards,
Zoltán Böszörményi
Hi,
I've started looking at and testing both patches.
Technically speaking, I think the source looks much better than the
first version of lock timeout, and may help adding other timeouts in the
future. I haven't tested it in depth though, because I encountered the
following problem:
While testing the patch, I found a way to crash PG. But what's weird is
that it crashes also with an unpatched git version.
Here is the way to reproduce it (I have done it with a pgbench schema):
- Set a small statement_timeout (just to save time during the tests)
Session1:
=#BEGIN;
=#lock TABLE pgbench_accounts ;
Session 2:
=#BEGIN;
=#lock TABLE pgbench_accounts ;
ERROR: canceling statement due to statement timeout
=# lock TABLE pgbench_accounts ;
I'm using \set ON_ERROR_ROLLBACK INTERACTIVE by the way. It can also be
done with a rollback to savepoint of course.
Session 2 crashes with this : TRAP : FailedAssertion(«
!(locallock->holdsStrongLockCount == 0) », fichier : « lock.c », ligne :
749).
It can also be done without a statement_timeout, and a control-C on the
second lock table.
I didn't touch anything but this. It occurs everytime, when asserts are
activated.
I tried it on 9.1.3, and I couldn't make it crash with the same sequence
of events. So maybe it's something introduced since ? Or is the assert
still valid ?
Cheers
Attached are the new patches. I rebased them to current GIT and
they are expected to be applied after Robert Haas' patch in the
"bug in fast-path locking" thread.
Now it survives the above scenario.
Best regards,
Zoltán Böszörményi
New patch attached, rebased to today's GIT.
Best regards,
Zoltán Böszörményi
Ok, I've done what was missing from the review (from when I had a bug in
locking the other day), so here is the full review. By the way, this
patch doesn't belong to current commitfest, but to the next one.
It was added to 2012-Next when I posted it, 2012-01 was already
closed for new additions.
Is the patch in context diff format?
Yes
Does it apply cleanly to the current git master?
Yes
Does it include reasonable tests, necessary doc patches, etc?
The new lock_timeout GUC is documented. There are regression tests.
Read what the patch is supposed to do, and consider:
Does the patch actually implement that?
Yes
Do we want that?
I do. Mostly for administrative jobs which could lock the whole
application. It would be much easier to run reindexes, vacuum full, etc…
without worrying about bringing application down because of lock
contention.
Do we already have it?
No.
Does it follow SQL spec, or the community-agreed behavior?
I don't know if there is a consensus on this new GUC. statement_timeout
is obviously not in the SQL spec.
Does it include pg_dump support (if applicable)?
Not applicable
Are there dangers?
Yes, as it rewrites all the timeout code. I feel it is much cleaner this
way, as there is a generic set of function managing all sigalarm code,
but it heavily touches this part.
Have all the bases been covered?
I tried all sql statements I could think of (select, insert, update,
delete, truncate, drop, create index, adding constraint, lock.
I tried having statement_timeout, lock_timeout and deadlock_timeout at
very short and close or equal values. It worked too.
Rollback to savepoint while holding locks dont crash PostgreSQL anymore.
Other timeouts such as archive_timeout and checkpoint_timeout still
work.
Does the feature work as advertised?
Yes
Are there corner cases the author has failed to consider?
I didn't find any.
Are there any assertion failures or crashes?
No.
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
Does it follow the project coding guidelines?
I think so
Are there portability issues?
No, all the portable code (acquiring locks and manipulating sigalarm) is
the same as before.
Will it work on Windows/BSD etc?
It should. I couldn't test it though.
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?
Not anymore
Is everything done in a way that fits together coherently with other
features/modules?
Yes, I think so. The new way of handling sigalarm seems more robust to
me.
Are there interdependencies that can cause problems?
I don't see any.
Regards,
Marc
Thanks for the review.
Best regards,
Zoltán Böszörményi
--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig& Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
