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
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 71ec740..0cf3537 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -107,6 +107,8 @@ static TransactionId MultiXactIdGetUpdateXid(TransactionId xmax,
uint16 t_infomask);
static void MultiXactIdWait(MultiXactId multi, MultiXactStatus status,
int *remaining, uint16 infomask);
+static void MultiXactIdWaitWithInfo(Relation rel, HeapTuple tup, MultiXactId multi,
+ MultiXactStatus status, int *remaining, uint16 infomask);
static bool ConditionalMultiXactIdWait(MultiXactId multi,
MultiXactStatus status, int *remaining,
uint16 infomask);
@@ -2714,8 +2716,8 @@ l1:
if (infomask & HEAP_XMAX_IS_MULTI)
{
/* wait for multixact */
- MultiXactIdWait((MultiXactId) xwait, MultiXactStatusUpdate,
- NULL, infomask);
+ MultiXactIdWaitWithInfo(relation, &tp, (MultiXactId) xwait,
+ MultiXactStatusUpdate, NULL, infomask);
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
/*
@@ -2741,7 +2743,7 @@ l1:
else
{
/* wait for regular transaction to end */
- XactLockTableWait(xwait);
+ XactLockTableWaitWithInfo(relation, &tp, xwait);
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
/*
@@ -3266,8 +3268,8 @@ l2:
int remain;
/* wait for multixact */
- MultiXactIdWait((MultiXactId) xwait, mxact_status, &remain,
- infomask);
+ MultiXactIdWaitWithInfo(relation, &oldtup,
+ (MultiXactId) xwait, mxact_status, &remain, infomask);
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
/*
@@ -3306,7 +3308,7 @@ l2:
/*
* There was no UPDATE in the MultiXact; or it aborted. No
* TransactionIdIsInProgress() call needed here, since we called
- * MultiXactIdWait() above.
+ * MultiXactIdWaitWithInfo() above.
*/
if (!TransactionIdIsValid(update_xact) ||
TransactionIdDidAbort(update_xact))
@@ -3341,7 +3343,7 @@ l2:
else
{
/* wait for regular transaction to end */
- XactLockTableWait(xwait);
+ XactLockTableWaitWithInfo(relation, &oldtup, xwait);
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
/*
@@ -4409,7 +4411,8 @@ l3:
RelationGetRelationName(relation))));
}
else
- MultiXactIdWait((MultiXactId) xwait, status, NULL, infomask);
+ MultiXactIdWaitWithInfo(relation, tuple,
+ (MultiXactId) xwait, status, NULL, infomask);
/* if there are updates, follow the update chain */
if (follow_updates &&
@@ -4464,7 +4467,7 @@ l3:
RelationGetRelationName(relation))));
}
else
- XactLockTableWait(xwait);
+ XactLockTableWaitWithInfo(relation, tuple, xwait);
/* if there are updates, follow the update chain */
if (follow_updates &&
@@ -5151,7 +5154,7 @@ l4:
if (needwait)
{
LockBuffer(buf, BUFFER_LOCK_UNLOCK);
- XactLockTableWait(members[i].xid);
+ XactLockTableWaitWithInfo(rel, &mytup, members[i].xid);
pfree(members);
goto l4;
}
@@ -5211,7 +5214,7 @@ l4:
if (needwait)
{
LockBuffer(buf, BUFFER_LOCK_UNLOCK);
- XactLockTableWait(rawxmax);
+ XactLockTableWaitWithInfo(rel, &mytup, rawxmax);
goto l4;
}
if (res != HeapTupleMayBeUpdated)
@@ -6169,6 +6172,33 @@ MultiXactIdWait(MultiXactId multi, MultiXactStatus status,
}
/*
+ * MultiXactIdWaitWithInfo
+ * Sets up the error context callback for reporting tuple
+ * information and relation for a lock to aquire
+ *
+ * Use this instead of calling MultiXactIdWait()
+ */
+static void
+MultiXactIdWaitWithInfo(Relation rel, HeapTuple tup, MultiXactId multi,
+ MultiXactStatus status, int *remaining, uint16 infomask)
+{
+ struct XactLockTableWaitLockInfo info;
+ ErrorContextCallback callback;
+
+ info.rel = rel;
+ info.tuple = tup;
+
+ callback.callback = XactLockTableWaitErrorContextCallback;
+ callback.arg = &info;
+ callback.previous = error_context_stack;
+ error_context_stack = &callback;
+
+ MultiXactIdWait(multi, status, remaining, infomask);
+
+ error_context_stack = callback.previous;
+}
+
+/*
* ConditionalMultiXactIdWait
* As above, but only lock if we can get the lock without blocking.
*
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 5f7953f..fc40c1c 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -164,7 +164,7 @@ top:
{
/* Have to wait for the other guy ... */
_bt_relbuf(rel, buf);
- XactLockTableWait(xwait);
+ XactLockTableWaitWithInfo(rel, NULL, xwait);
/* start over... */
_bt_freestack(stack);
goto top;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 877d767..49d7cd6 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2295,7 +2295,8 @@ IndexBuildHeapScan(Relation heapRelation,
* Must drop the lock on the buffer before we wait
*/
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
- XactLockTableWait(xwait);
+ XactLockTableWaitWithInfo(heapRelation,
+ heapTuple, xwait);
goto recheck;
}
}
@@ -2341,7 +2342,8 @@ IndexBuildHeapScan(Relation heapRelation,
* Must drop the lock on the buffer before we wait
*/
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
- XactLockTableWait(xwait);
+ XactLockTableWaitWithInfo(heapRelation,
+ heapTuple, xwait);
goto recheck;
}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 9e3d879..4bf4f0f 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1982,7 +1982,7 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
if (TransactionIdIsValid(SnapshotDirty.xmax))
{
ReleaseBuffer(buffer);
- XactLockTableWait(SnapshotDirty.xmax);
+ XactLockTableWaitWithInfo(relation, NULL, SnapshotDirty.xmax);
continue; /* loop back to repeat heap_fetch */
}
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 46895b2..46b64c2 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -1307,7 +1307,7 @@ retry:
if (TransactionIdIsValid(xwait))
{
index_endscan(index_scan);
- XactLockTableWait(xwait);
+ XactLockTableWaitWithInfo(heap, NULL, xwait);
goto retry;
}
diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c
index 4c61a6f..5f2cac9 100644
--- a/src/backend/storage/lmgr/lmgr.c
+++ b/src/backend/storage/lmgr/lmgr.c
@@ -18,12 +18,15 @@
#include "access/subtrans.h"
#include "access/transam.h"
#include "access/xact.h"
+#include "access/htup_details.h"
#include "catalog/catalog.h"
#include "miscadmin.h"
#include "storage/lmgr.h"
#include "storage/procarray.h"
#include "utils/inval.h"
-
+#include "utils/lsyscache.h"
+#include "mb/pg_wchar.h"
+#include "commands/dbcommands.h"
/*
* RelationInitLockInfo
@@ -534,6 +537,140 @@ ConditionalXactLockTableWait(TransactionId xid)
}
/*
+ * XactLockTableWaitErrorContextCallback
+ *
+ * error context callback set up by XactLockTableWaitWithInfo. It reports some
+ * tuple information and the relation of a lock to aquire
+ *
+ */
+void
+XactLockTableWaitErrorContextCallback(void *arg)
+{
+ StringInfoData buf,
+ error_message;
+ int i;
+ bool write_comma = false;
+ int maxfieldlen = 30;
+ struct XactLockTableWaitLockInfo *info = (struct XactLockTableWaitLockInfo *) arg;
+
+ initStringInfo(&error_message);
+ appendStringInfoString(&error_message, "while attempting to operate");
+
+ if (info->tuple != NULL)
+ {
+ /*
+ * Can't produce an error message including the tuple if we're in a
+ * possibly aborted transaction state, database access might not be
+ * safe.
+ */
+ appendStringInfo(&error_message, " on tuple (%u,%u)",
+ BlockIdGetBlockNumber(&(info->tuple->t_self.ip_blkid)),
+ info->tuple->t_self.ip_posid);
+
+ if (geterrlevel() < ERROR)
+ {
+ TupleDesc desc = RelationGetDescr(info->rel);
+ Datum *values = palloc(desc->natts * sizeof(*values));
+ bool *nulls = palloc(desc->natts * sizeof(*values));
+
+ heap_deform_tuple(info->tuple, desc, values, nulls);
+
+ initStringInfo(&buf);
+ appendStringInfoChar(&buf, '(');
+
+ for (i = 0; i < desc->natts; i++)
+ {
+ char *val;
+ int vallen;
+
+ if (desc->attrs[i]->attisdropped)
+ continue;
+
+ if (nulls[i])
+ val = "null";
+ else
+ {
+ Oid foutoid;
+ bool typisvarlena;
+
+ getTypeOutputInfo(desc->attrs[i]->atttypid,
+ &foutoid, &typisvarlena);
+
+ val = OidOutputFunctionCall(foutoid, values[i]);
+ }
+
+ if (write_comma)
+ appendStringInfoString(&buf, ", ");
+ else
+ write_comma = true;
+
+ vallen = strlen(val);
+ if (vallen <= maxfieldlen)
+ appendStringInfoString(&buf, val);
+ else
+ {
+ vallen = pg_mbcliplen(val, vallen, maxfieldlen);
+ appendBinaryStringInfo(&buf, val, vallen);
+ appendStringInfoString(&buf, "...");
+ }
+ }
+
+ appendStringInfoChar(&buf, ')');
+
+ appendStringInfo(&error_message, " with values %s",
+ buf.data);
+
+ pfree(buf.data);
+ pfree(values);
+ pfree(nulls);
+ }
+ }
+
+ if (info->rel != NULL)
+ {
+ appendStringInfo(&error_message,
+ " in relation \"%s\".\"%s\" of database \"%s\"",
+ get_namespace_name(RelationGetNamespace(info->rel)),
+ RelationGetRelationName(info->rel),
+ get_database_name(info->rel->rd_node.dbNode));
+ }
+ else if (MyDatabaseId != InvalidOid)
+ {
+ appendStringInfo(&error_message, " in database \"%s\"",
+ get_database_name(MyDatabaseId));
+ }
+
+ errcontext("%s", error_message.data);
+ pfree(error_message.data);
+}
+
+/*
+ * XactLockTableWaitWithInfo
+ * Sets up the error context callback for reporting tuple
+ * information and relation for a lock to aquire
+ *
+ * Use this instead of calling XactLockTableWait()
+ */
+void
+XactLockTableWaitWithInfo(Relation rel, HeapTuple tuple, TransactionId xid)
+{
+ struct XactLockTableWaitLockInfo info;
+ ErrorContextCallback callback;
+
+ info.rel = rel;
+ info.tuple = tuple;
+
+ callback.callback = XactLockTableWaitErrorContextCallback;
+ callback.arg = &info;
+ callback.previous = error_context_stack;
+ error_context_stack = &callback;
+
+ XactLockTableWait(xid);
+
+ error_context_stack = callback.previous;
+}
+
+/*
* WaitForLockersMultiple
* Wait until no transaction holds locks that conflict with the given
* locktags at the given lockmode.
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 8705586..6231db6 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1216,6 +1216,23 @@ geterrposition(void)
}
/*
+ * geterrlevel --- return the currently set error level
+ *
+ * This is only intended for use in error callback subroutines, since there
+ * is no other place outside elog.c where the concept is meaningful.
+ */
+int
+geterrlevel(void)
+{
+ ErrorData *edata = &errordata[errordata_stack_depth];
+
+ /* we don't bother incrementing recursion_depth */
+ CHECK_STACK_DEPTH();
+
+ return edata->elevel;
+}
+
+/*
* getinternalerrposition --- same for internal error position
*
* This is only intended for use in error callback subroutines, since there
diff --git a/src/include/storage/lmgr.h b/src/include/storage/lmgr.h
index e74ae21..954decc 100644
--- a/src/include/storage/lmgr.h
+++ b/src/include/storage/lmgr.h
@@ -18,6 +18,21 @@
#include "storage/itemptr.h"
#include "storage/lock.h"
#include "utils/rel.h"
+#include "access/htup.h"
+
+/*
+ * XactLockTableWaitWithInfo/MultiXactIdWaitWithInfo will fill this struct for
+ * XactLockTableWaitErrorContextCallback. When a log_lock_wait message is
+ * caused or the user cancels the request or something else happens,
+ * XactLockTableWaitErrorContextCallback will use it to provide some hopefully
+ * useful information. Relation rel is the relation we're operating on and
+ * HeapTuple tuple is the tuple we're operating on.
+ */
+struct XactLockTableWaitLockInfo
+{
+ Relation rel;
+ HeapTuple tuple;
+};
extern void RelationInitLockInfo(Relation relation);
@@ -57,6 +72,10 @@ extern void XactLockTableDelete(TransactionId xid);
extern void XactLockTableWait(TransactionId xid);
extern bool ConditionalXactLockTableWait(TransactionId xid);
+extern void XactLockTableWaitWithInfo(Relation rel, HeapTuple tuple,
+ TransactionId xid);
+extern void XactLockTableWaitErrorContextCallback(void *arg);
+
/* Lock VXIDs, specified by conflicting locktags */
extern void WaitForLockers(LOCKTAG heaplocktag, LOCKMODE lockmode);
extern void WaitForLockersMultiple(List *locktags, LOCKMODE lockmode);
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index d7916c2..04fd3a2 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -224,6 +224,7 @@ extern int err_generic_string(int field, const char *str);
extern int geterrcode(void);
extern int geterrposition(void);
+extern int geterrlevel(void);
extern int getinternalerrposition(void);
pgpF6z7PTRX4j.pgp
Description: PGP signature
