Re: Patch: show relation and tuple infos of a lock to acquire

Robert Haas escribió:
> On Tue, Mar 18, 2014 at 12:00 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >> Therefore I think the only case worth considering here is when
> >> database/relation/TID are all defined. The other cases where there is
> >> partial information are dead code; and the case where there is nothing
> >> defined (such as the one in SnapBuildFindSnapshot) is already handled by
> >> simply not setting up a context at all.
> >
> > Right. So I think we should just keep one version of message.
>
> Well, if we're back to one version of the message, and I'm glad we
> are, can we go back to saying:
>
> CONTEXT: while updating tuple (0,2) in relation "public"."foo" of
> database "postgres"

That isn't easy. The callers would need to pass the whole message in
order for it to be translatable; and generating a format string in one
module and having the arguments be stapled on top in a separate module
doesn't seem a very good idea to me. Currently we have this:

/* wait for regular transaction to end */
XactLockTableWait(xwait, relation, &tp.t_data->t_ctid,
/* translator: string is XactLockTableWait operation name */
"deleting tuple");

And if we go with what you propose, we would have this:

/* wait for regular transaction to end */
XactLockTableWait(xwait, relation, &tp.t_data->t_ctid,
"while updating tuple (%u,%u) in relation \"%s\"")

which doesn't seem an improvement to me.

Now another idea would be to pass a code or something; so callers would
do
XactLockTableWait(xwait, relation, TID, XLTW_OPER_UPDATE);

and the callback would select the appropriate message. Not really
excited about that, though.

In the current version of the patch, attached, I've reduced the callback
to this:

if (ItemPointerIsValid(info->ctid) && RelationIsValid(info->rel))
{
errcontext("while operating on tuple (%u,%u) in relation \"%s\": %s",
ItemPointerGetBlockNumber(info->ctid),
ItemPointerGetOffsetNumber(info->ctid),
RelationGetRelationName(info->rel),
_(info->opr_name));
}

Note that:
1. the database name is gone, as discussed
2. the schema name is gone too, because it requires a syscache lookup

Now we can go back to having a schema name, but I think we would have to
add an IsTransactionState() check first or something like that. Or save
the schema name when setting up the callback, but this doesn't sound so
good (particularly considering that lock waits might end without
actually waiting at all, so this'd be wasted effort.)

If you have a better idea, I'm all ears.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services