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

Hi,

attached you will find a new version of the patch, removing the ctid
text but leaving the ctid itself in the message.

On 23/02/14 11:14, 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)

To be honest, I don't like the idea of setting up this error context
only for log_lock_wait messages. This sounds unnecessary complex to me
and I think that in the few cases where this message doesn't add a
value (and thus is useless) don't justify such complexity.

> 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

After thinking a bit about this I added the database name to the
context message, see attached patch.

> >> > 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.

That would be nice. I didn't change it, yet.

Best regards,

--
Christian Kruse http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services