On Thu, Jan 2, 2014 at 4:38 PM, Christian Kruse <christ...@2ndquadrant.com> wrote: > Hi, > > On 02/01/14 10:02, Andres Freund wrote: >> > Christian's idea of a context line seems plausible to me. I don't >> > care for this implementation too much --- a global variable? Ick. >> >> Yea, the data should be stored in ErrorContextCallback.arg instead. > > Fixed.
I have looked into this patch. Below are some points which I think should be improved in this patch: 1. New context added by this patch is display at wrong place Session-1 Create table foo(c1 int); insert into foo values(generate_series(1,10); Begin; update foo set c1 =11; Session-2 postgres=# begin; BEGIN postgres=# update foo set val1 = 13 where val1=3; Cancel request sent ERROR: canceling statement due to user request CONTEXT: relation name: foo (OID 57496) tuple ctid (0,7) Do this patch expect to print the context at cancel request as well? I don't find it useful. I think the reason is that after setup of context, if any other error happens, it will display the newly setup context. 2a. About the message: LOG: process 1936 still waiting for ShareLock on transaction 842 after 1014.000ms CONTEXT: relation name: foo (OID 57496) tuple (ctid (0,1)): (1, 2, abc ) Isn't it better to display database name as well, as if we see in one of related messages as below, it displays database name as well. "LOG: process 3012 still waiting for ExclusiveLock on tuple (0,1) of relation 57 499 of database 12045 after 1014.000 ms" 2b. I think there is no need to display *ctid* before tuple offset, it seems to be obvious as well as consistent with other similar message. 3. About callback I think rather than calling setup/tear down functions from multiple places, it will be better to have wrapper functions for XactTableLockWait() and MultiXactIdWait(). Just check in plpgsql_exec_function(), how the errorcallback is set, can we do something similar without to avoid palloc? I think we still need to see how to avoid issue mentioned in point-1. 4. About the values of tuple I think it might be useful to display some unique part of tuple for debugging part, but what will we do incase there is no primary key on table, so may be we can display initial few columns (2 or 3) or just display tuple offset as is done in other similar message shown above. More to the point, I have observed that in few cases displaying values of tuple can be confusing as well. For Example: Session-1 Create table foo(c1 int); postgres=# insert into foo values(generate_series(1,3)); INSERT 0 3 postgres=# begin; BEGIN postgres=# update foo set c1 = 4; UPDATE 3 Session-2 postgres=# update foo set c1=6; LOG: process 1936 still waiting for ShareLock on transaction 884 after 1014.000 ms CONTEXT: relation name: foo (OID 57499) tuple (ctid (0,1)): (1) Session-3 postgres=# begin; BEGIN postgres=# update foo set c1=5 where c1 =3; LOG: process 3012 still waiting for ShareLock on transaction 884 after 1015.000 ms CONTEXT: relation name: foo (OID 57499) tuple (ctid (0,3)): (3) Session-1 Commit; Session-2 LOG: process 1936 acquired ShareLock on transaction 884 after 25294.000 ms CONTEXT: relation name: foo (OID 57499) tuple (ctid (0,1)): (1) UPDATE 3 Session-3 LOG: process 3012 acquired ShareLock on transaction 884 after 13217.000 ms CONTEXT: relation name: foo (OID 57499) tuple (ctid (0,3)): (3) LOG: process 3012 still waiting for ShareLock on transaction 885 after 1014.000 ms CONTEXT: relation name: foo (OID 57499) tuple (ctid (0,6)): (4) Now here it will be bit confusing to display values with tuple, as in session-3 statement has asked to update value (3), but we have started waiting for value (4). Although it is right, but might not make much sense. Also after session-2 commits the transaction, session-3 will say acquired lock, however still it will not be able to update it. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
