Re: Patch: show relation and tuple infos of a lock to acquire

On Sat, Feb 22, 2014 at 3:17 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-02-22 11:53:24 +0530, Amit Kapila wrote:
>
>> In general, why I am suggesting to restrict display of newly added
>> context for the case it is added to ensure that it doesn't get started
>> displaying in other cases where it might make sense or might not
>> make sense.
>
> Anything failing while inside an XactLockTableWait() et al. will benefit
> from the context. In which scenarios could that be unneccessary?

This path is quite deep, so I have not verified that whether
this new context can make sense for all error cases.
For example, in below path (error message), it doesn't seem to
make any sense (I have tried to generate it through debugger,
actual message will vary).

XactLockTableWait->SubTransGetParent->SimpleLruReadPage_ReadOnly->SimpleLruReadPage->SlruReportIOError

ERROR: could not access status of transaction 927
DETAIL: Could not open file "pg_subtrans/0000": No error.
CONTEXT: relation "public"."t1"
tuple ctid (0,2)

>> > (according to Andres we would have to look at
>> > rel->rd_node.dbNode) and since other commands dealing with names don't
>> > do so, either, I think we should leave out the database name.
>>
>> For this case as I have shown that in similar message, we already display
>> database oid, it should not be a problem to display here as well.
>> It will be more information to user.
>
> I don't think we do for any where we actually display the relation name,
> do we?

I have not checked that, but the reason I mentioned about database name
is due to display of database oid in similar message, please see the
message below. I think in below context we get it from lock tag and
I think for the patch case also, it might be better to display, but not
a mandatory thing. Consider it as a suggestion which if you also feel
good, then do it, else ignore it.

LOG: process 4656 still waiting for ExclusiveLock on tuple (0,1) of relation 57
513 of database 12045 after 1046.000 ms

>
>> > Currently I simply display the whole tuple with truncating long
>> > fields. This is plain easy, no distinction necessary. I did some code
>> > reading and it seems somewhat complex to get the PK columns and it
>> > seems that we need another lock for this, too - maybe not the best
>> > idea when we are already in locking trouble, do you disagree?
>>
>> I don't think you need to take another lock for this, it must already
>> have appropriate lock by that time. There should not be any problem
>> in calling relationHasPrimaryKey except that currently it is static, you
>> need to expose it.
>
> Other locations that deal with similar things (notably
> ExecBuildSlotValueDescription()) doesn't either. I don't think this
> patch should introduce it then.

Displaying whole tuple doesn't seem to be the most right way
to get debug information and especially in this case we are
already displaying tuple offset(ctid) which is unique identity
to identify a tuple. It seems to me that it is sufficient to display
unique value of tuple if present.
I understand that there is no clear issue here, so may be if others also
share their opinion then it will be quite easy to take a call.

PS - I will be out for work assignment for next week, so might not be
able to complete the review, so others are welcome to takeup and
complete it in the meantime.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com