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

On 2014-02-22 11:53:24 +0530, Amit Kapila wrote:
> On Fri, Feb 21, 2014 at 4:55 PM, Christian Kruse
> <christian(at)2ndquadrant(dot)com> wrote:
> > Hi,
> >> 1. New context added by this patch is display at wrong place
> >> [...]
> >> Do this patch expect to print the context at cancel request
> >> as well?
> >
> > To be honest, I don't see a problem here. If you cancel the request in
> > most cases it is because it doesn't finish in an acceptable time. So
> > displaying the context seems logical to me.
>
> Isn't it a matter of consistency, we might not be able to display it
> on all long running updates, rather only when it goes for update on tuple
> on which some other transaction is operating.

We'll display it when we are waiting for a lock. Which quite possibly is
the reason it's cancelled, so logging the locking information is highly
pertinent.

> Is it possible that we set callback to error_context_stack, only
> when we go for displaying this message. The way to do could be that
> rather than forming callback in local variable in fuction
> XactLockTableWaitWithErrorCallback(), store it in global variable
> and then use that at appropriate place to set error_context_stack.

That seems like unneccessary complexity.

> In general, why I am suggesting to restrict display of newly added
> context for the case it is added to ensure that it doesn't get started
> displaying in other cases where it might make sense or might not
> make sense.

Anything failing while inside an XactLockTableWait() et al. will benefit
from the context. In which scenarios could that be unneccessary?

> > (according to Andres we would have to look at
> > rel->rd_node.dbNode) and since other commands dealing with names don't
> > do so, either, I think we should leave out the database name.
>
> For this case as I have shown that in similar message, we already display
> database oid, it should not be a problem to display here as well.
> It will be more information to user.

I don't think we do for any where we actually display the relation name,
do we?

> > Currently I simply display the whole tuple with truncating long
> > fields. This is plain easy, no distinction necessary. I did some code
> > reading and it seems somewhat complex to get the PK columns and it
> > seems that we need another lock for this, too - maybe not the best
> > idea when we are already in locking trouble, do you disagree?
>
> I don't think you need to take another lock for this, it must already
> have appropriate lock by that time. There should not be any problem
> in calling relationHasPrimaryKey except that currently it is static, you
> need to expose it.

Other locations that deal with similar things (notably
ExecBuildSlotValueDescription()) doesn't either. I don't think this
patch should introduce it then.

Greetings,

Andres Freund