On 16 January 2013 16:12, Stephen Frost <sfrost@snowman.net> wrote:
> * Simon Riggs (simon@2ndquadrant.com) wrote:
>> > A couple quick notes regarding the patch- what does
>> > GetXactLockTableRelid really provide..?
>>
>> The ability to access a static variable in a different module. It
>> doesn't provide anything other than that,
>
> It isn't actually necessary for that currently though, is it? All calls
> to it appeared to be inside the same module. Do you anticipate that
> needing to change in the future?
OK, will change.
>> The information isn't available, which is why I didn't include it.
>> Comments explain that the additional information is only available
>> within the backend that was waiting. That's sufficient for stats, but
>> not enough for an extended multi-backend deadlock report.
>
> Right, sorry, to be clear, it seemed that we could simply detect if the
> information is available and print it out if it is- and not print it if
> it isn't. The additional boolean variable looked to be unnecessary, or
> is there a point where OidIsValid(GetXactLockTableRelid()) will be true
> but the additional information shouldn't be printed or isn't actually
> available?
You might be reporting a deadlock in another backend when you are
waiting on a transaction yourself. If we don't address that we'll end
up reporting the wrong info in a way that makes it look real and
probably never even notice.
>> There is an API change to XactLockTableWait() to pass the relid. That
>> part is essential to any solution.
>
> I'm wondering if we could pass in something both more generic and with
> more information- perhaps a simple string which is constructed when we
> have more information and is then included in the log message which is
> generated? Perhaps something which includes both the relname and the
> OID? Could that be used across more than just these kinds of locks? Of
> course, on the flip side, I understand that we don't want to spend a lot
> of time building strings and whatnot during all of these code paths;
> would be nice if we could defer that until we're about to log.
Keeping it as an Oid is better, since we don't always need the string.
> Regardless, I do think it'd be good to update the comments to indicate
> that the relid is being passed in solely for the purpose of being
> available to be included in the log, if necessary.
OK, will add.
-- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services