From: | Christian Kruse <christian(at)2ndquadrant(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, 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 |
Date: | 2014-03-10 11:44:09 |
Message-ID: | 20140310114409.GA9524@defunct.ch |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
thanks for the continued review.
On 09/03/14 12:15, Amit Kapila wrote:
> 1.
> "> ISTM that you should be printing just the value and the unique index
> > there, and not any information about the tuple proper.
>
> Good point, I will have a look at this."
>
> This point is still not handled in patch, […]
There have been some complaints about this by Andres, so I left it.
> […] due to which the message it displays seems to be
> incomplete. Message it displays is as below:
>
> LOG: process 1800 still waiting for ShareLock on transaction 679 after 1014.000
> ms
> CONTEXT: while attempting to lock in relation "public"."idx_t1" of database pos
> tgres
Well, there is no tuple information available, so we have to leave it out.
> Here it is not clear what it attempts *lock in*
Ok, I reworded the message as you suggested below. Should make this
clearer as well.
> 2. In function XactLockTableWaitErrorContextCallback(), ignore dropped
> columns in tuple, else it will lead to following failure:
> […]
> Problem is in Session-2 (cache lookup failed), when it tries to print values
> for dropped attribute.
Urghs. I really thought I had done this in the patch before. Thanks
for pointing out, fixed in attached patch.
> 3.
> " in relation \"%s\".\"%s\" of database %s",
> Why to display only relation name in quotes?
> I think database name should also be displayed in quotes.
Didn't think that it would make sense; the quoting makes sense for the
schema and relation name, but I did not see any need to quote the
database name. However, fixed in attached patch.
> 4.
> Context message:
> "while attempting to lock tuple (0,2) ".
>
> I think it will be better to rephrase it (may be by using 'operate on'
> instead of 'lock').
> The reason is that actually we trying to lock on transaction, so it doesn't make
> good sense to use "lock on tuple"
Fixed.
> 5.
> + XactLockTableWaitWithInfo(relation, &tp, xwait);
>
> + MultiXactIdWaitWithErrorContext(relation, &tp, (MultiXactId) xwait,
> + MultiXactStatusUpdate, NULL, infomask);
>
> I think we should make the name of MultiXactIdWaitWithErrorContext()
> similar to XactLockTableWaitWithInfo.
Fixed as well.
Can you explain why you prefer the WithInfo naming? Just curious, it
seemed to me the more descriptive name should have been preferred.
> 6.
> @@ -1981,7 +1981,8 @@ EvalPlanQualFetch(EState *estate, Relation
> relation, int lockmode,
> if (TransactionIdIsValid(SnapshotDirty.xmax))
> {
> ReleaseBuffer(buffer);
> - XactLockTableWait(SnapshotDirty.xmax);
> + XactLockTableWaitWithInfo(relation, &tuple,
> + SnapshotDirty.xmax);
>
> In function EvalPlanQualFetch(), we are using SnapshotDirty to fetch
> the tuple, so I think there is a chance that it will log the tuple which
> otherwise will not be visible. I don't think this is right.
Hm, after checking source I tend to agree. I replaced it with NULL.
> 8.
> void
> WaitForLockers(LOCKTAG heaplocktag, LOCKMODE lockmode)
> {
> - List *l;
> + List *l;
>
> Extra spacing not needed.
What is the policy to do here? The extra spaces have been added by
pg_indent, and there have been more changes in a couple of other files
which I had to drop manually as well. How should this been handled
generally?
> 9.
> /*
> * XactLockTableWaitErrorContextCallback
> * error context callback set up by
> * XactLockTableWaitWithInfo. It reports some
> * tuple information and the relation of a lock to aquire
> *
> */
> Please improve indentation of above function header
Heh. Seems that Emacs' fill-paragraph messed this up. Thanks, fixed.
> 10.
> +#include "access/htup.h"
> +
> +struct XactLockTableWaitLockInfo
> +{
> + Relation rel;
> + HeapTuple tuple;
> +};
>
> I think it is better to add comments for this structure.
> You can refer comments for struct HeapUpdateFailureData
Fixed.
>
> 11.
> + *
> + * Use this instead of calling XactTableLockWait()
>
> In above comment, function name used is wrong and I am not sure this
> is right statement to use because still we are using XactLockTableWait()
> at few places like in function Do_MultiXactIdWait() […]
Fixed function name, but I'd like to keep the wording;
Do_MultiXactIdWait() is wrapped by MultiXactIdWaitWithInfo() and
therefore sets up the context as well.
> […] and recently this is used in function SnapBuildFindSnapshot().
Hm, should we add the context there, too?
> 12.
> heap_update()
> {
> ..
> ..
> /*
> * There was no UPDATE in the MultiXact; or it aborted. No
> * TransactionIdIsInProgress() call needed here, since we called
> * MultiXactIdWait() above.
>
> Change the function name in above comment.
Fixed.
Best regards,
--
Christian Kruse http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachment | Content-Type | Size |
---|---|---|
show_table_name_and_tuple_in_lock_log_v7.patch | text/x-diff | 12.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2014-03-10 12:06:53 | Standby node using replication slot not visible in pg_stat_replication while catching up |
Previous Message | Alexander Korotkov | 2014-03-10 11:19:36 | Re: jsonb and nested hstore |