lock_timeout GUC patch
in [PgSql]
|
From: Jaime Casanova on 13 Jan 2010 09:39 2010/1/13 Boszormenyi Zoltan <zb(a)cybertec.at>: >> >> well, i actually think that PANIC is too high for this... >> > > Well, it tries to lock and then open a critical system index. > Failure to open it has PANIC, it seemed appropriate to use > the same error level if the lock failure case. > if you try to open a critical system index and it doesn't exist is clearly a signal of corruption, if you can't lock it it's just a concurrency issue... don't see why they both should have the same level of message -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL AsesorÃa y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 13 Jan 2010 09:56 Boszormenyi Zoltan <zb(a)cybertec.at> writes: > Tom Lane �rta: >> If this patch is touching those parts of relcache.c, it probably needs >> rethinking. > What I did there is to check the return value of LockRelationOid() > and also elog(PANIC) if the lock wasn't available. > Does it need rethinking? Yes. What you have done is to change all the LockSomething primitives from return void to return bool and thereby require all call sites to check their results. This is a bad idea. There is no way that you can ensure that all third-party modules will make the same change, meaning that accepting this patch will certainly introduce nasty, hard to reproduce bugs. And what's the advantage? The callers are all going to throw errors anyway, so you might as well do that within the Lock function and avoid the system-wide API change. I think this is a big patch with a small patch struggling to get out. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Boszormenyi Zoltan on 13 Jan 2010 14:12 Tom Lane �rta: > Boszormenyi Zoltan <zb(a)cybertec.at> writes: > >> Tom Lane �rta: >> >>> If this patch is touching those parts of relcache.c, it probably needs >>> rethinking. >>> > > >> What I did there is to check the return value of LockRelationOid() >> and also elog(PANIC) if the lock wasn't available. >> Does it need rethinking? >> > > Yes. What you have done is to change all the LockSomething primitives > from return void to return bool and thereby require all call sites to > check their results. This is a bad idea. Okay, can you tell me how can I get the relation name out of the xid in XactLockTableWait()? There are several call site of this function, and your idea about putting the error code into the LockSomething() functions to preserve the API results strange error messages, like ERROR: could not obtain lock on transaction with ID 658 when I want to UPDATE a tuple in a session when this and another session have a FOR SHARE lock on said tuple. > There is no way that you can > ensure that all third-party modules will make the same change, meaning > that accepting this patch will certainly introduce nasty, hard to > reproduce bugs. And what's the advantage? The callers are all going > to throw errors anyway, so you might as well do that within the Lock > function and avoid the system-wide API change. > > I think this is a big patch with a small patch struggling to get out. > Your smaller patch is attached, with the above strangeness. :-) Best regards, Zolt�n B�sz�rm�nyi -- Bible has answers for everything. Proof: "But let your communication be, Yea, yea; Nay, nay: for whatsoever is more than these cometh of evil." (Matthew 5:37) - basics of digital technology. "May your kingdom come" - superficial description of plate tectonics ---------------------------------- Zolt�n B�sz�rm�nyi Cybertec Sch�nig & Sch�nig GmbH http://www.postgresql.at/
From: Boszormenyi Zoltan on 13 Jan 2010 16:26 Boszormenyi Zoltan �rta: > Tom Lane �rta: > >> Boszormenyi Zoltan <zb(a)cybertec.at> writes: >> >> >>> Tom Lane �rta: >>> >>> >>>> If this patch is touching those parts of relcache.c, it probably needs >>>> rethinking. >>>> >>>> >> >> >>> What I did there is to check the return value of LockRelationOid() >>> and also elog(PANIC) if the lock wasn't available. >>> Does it need rethinking? >>> >>> >> Yes. What you have done is to change all the LockSomething primitives >> from return void to return bool and thereby require all call sites to >> check their results. This is a bad idea. >> > > Okay, can you tell me how can I get the relation name > out of the xid in XactLockTableWait()? There are several > call site of this function, and your idea about putting the error > code into the LockSomething() functions to preserve the API > results strange error messages, like > > ERROR: could not obtain lock on transaction with ID 658 > > when I want to UPDATE a tuple in a session when > this and another session have a FOR SHARE lock > on said tuple. > > >> There is no way that you can >> ensure that all third-party modules will make the same change, meaning >> that accepting this patch will certainly introduce nasty, hard to >> reproduce bugs. And what's the advantage? The callers are all going >> to throw errors anyway, so you might as well do that within the Lock >> function and avoid the system-wide API change. >> May I change the interface of XactLockTableWait() and MultiXactIdWait()? Not the return value, only the number of parameters. E.g. with the relation name, like in the attached patch. This solves the problem of bad error messages... What do you think? >> I think this is a big patch with a small patch struggling to get out. >> >> > > Your smaller patch is attached, with the above strangeness. :-) > > Best regards, > Zolt�n B�sz�rm�nyi > > > ------------------------------------------------------------------------ > > -- Bible has answers for everything. Proof: "But let your communication be, Yea, yea; Nay, nay: for whatsoever is more than these cometh of evil." (Matthew 5:37) - basics of digital technology. "May your kingdom come" - superficial description of plate tectonics ---------------------------------- Zolt�n B�sz�rm�nyi Cybertec Sch�nig & Sch�nig GmbH http://www.postgresql.at/
From: Jaime Casanova on 14 Jan 2010 23:43
2010/1/13 Boszormenyi Zoltan <zb(a)cybertec.at>: >> >> Your smaller patch is attached, with the above strangeness. :-) >> you still had to add this parameter to the postgresql.conf.sample in the section about lock management -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL AsesorÃa y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |