From: | Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com> |
---|---|
To: | Christian Kruse <christian(at)2ndquadrant(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | Re: Patch: Show process IDs of processes holding a lock; show relation and tuple infos of a lock to acquire |
Date: | 2014-01-27 11:44:49 |
Message-ID: | BF2827DCCE55594C8D7A8F7FFD3AB7713DDBCCBF@SZXEML508-MBX.china.huawei.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 23/01/14, Christian Kruse wrote:
> > Well, is it context or detail? Those fields have reasonably well
> > defined meanings IMO.
>
> I find the distinction somewhat blurry and think both would be
> appropriate. But since I wasn't sure I changed to detail.
>
> > If we need errcontext_plural, let's add it, not adopt inferior
> > solutions just because that isn't there for lack of previous need.
>
> I would've added it if I would've been sure.
>
> > But having said that, I think this is indeed detail not context.
> > (I kinda wonder whether some of the stuff that's now in the primary
> > message shouldn't be pushed to errdetail as well. It looks like some
> > previous patches in this area have been lazy.)
>
> I agree, the primary message is not very well worded. On the other hand
> finding an appropriate alternative seems hard for me.
>
> > While I'm griping, this message isn't even trying to follow the
> > project's message style guidelines. Detail or context messages are
> > supposed to be complete sentence(s), with capitalization and
> punctuation to match.
>
> Hm, I hope I fixed it in this version of the patch.
>
> > Lastly, is this information that we want to be shipping to clients?
> > Perhaps from a security standpoint that's not such a wise idea, and
> > errdetail_log() is what should be used.
>
> Fixed. I added an errdetail_log_plural() for this, too.
I have checked the revised patch. It looks fine to me except one minor code formatting issue.
In elog.c, two tabs are missing in the definition of function "errdetail_log_plural".
Please run pgindent tool to check the same.
Also I would like to highlight one behavior here is that process ID of process trying to
acquire lock is also listed in the list of "Request queue". E.g.
session 1 with process id X: BEGIN; LOCK TABLE foo IN SHARE MODE;
session 2 with process id Y: BEGIN; LOCK TABLE foo IN EXCLUSIVE MODE;
On execution of LOCK in session-2, as part of log it will display as:
DETAIL: Process holding the lock: X. Request queue: Y.
Where Y is the process ID of same process, which was trying to acquire lock.
To me, it seems to be correct behavior as the meaning of message will be
list of all processes already holding the lock and processes waiting in queue and
position of self process in wait-list. In above example, it will indicate that
process Y in on top of wait list.
Thanks and Regards,
Kumar Rajeev Rastogi
From | Date | Subject | |
---|---|---|---|
Next Message | KONDO Mitsumasa | 2014-01-27 12:09:02 | Re: Add min and max execute statement time in pg_stat_statement |
Previous Message | Simon Riggs | 2014-01-27 11:04:49 | Re: plpgsql.warn_shadow |