On 2014-02-22 11:53:24 +0530, Amit Kapila wrote: > On Fri, Feb 21, 2014 at 4:55 PM, Christian Kruse > <christ...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers