Re: Patch: show relation and tuple infos of a lock to acquire - Mailing list pgsql-hackers

On Thu, Jan 2, 2014 at 4:38 PM, Christian Kruse
<christian@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.000ms
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.000ms
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.000ms
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