|From:||Christian Kruse <christian(at)2ndquadrant(dot)com>|
|To:||Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>|
|Cc:||Andres Freund <andres(at)2ndquadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Simon Riggs <simon(at)2ndquadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Patch: show relation and tuple infos of a lock to acquire|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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).
> 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.
Christian Kruse http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
|Next Message||Shigeru Hanada||2014-02-24 15:41:07||Re: Custom Scan APIs (Re: Custom Plan node)|
|Previous Message||Andres Freund||2014-02-24 15:11:31||Re: Changeset Extraction v7.7|