|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
thanks for the continued review.
On 09/03/14 12:15, Amit Kapila wrote:
> "> 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
> CONTEXT: while attempting to lock in relation "public"."idx_t1" of database pos
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.
> " 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.
> 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"
> + 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.
> @@ -1981,7 +1981,8 @@ EvalPlanQualFetch(EState *estate, Relation
> relation, int lockmode,
> if (TransactionIdIsValid(SnapshotDirty.xmax))
> - 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.
> 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
> * 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.
> +#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
> + *
> + * 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?
> * 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.
Christian Kruse http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
|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|