Re: log_lock_waits to identify transaction's relation

On 16 January 2013 03:47, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Simon,
>
> * Simon Riggs (simon(at)2ndQuadrant(dot)com) wrote:
>> Attached patch passes through further information about the lock wait,
>> so we can display the following message instead
>> LOG: process %d acquired %s on transaction %u on relation %u of
>> database %u after %ld.%03d ms
>
> I love this idea. Please take these comments as an initial/quick review
> because I'd really like to see this get in.

It's there as a way to prove FKlocks works, or not, and to help that
get committed.

It's possible we reject this in this CF, since I see it as low priority anyway.

> 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,

> This patch does use it outside
> of lmgr.c, where XactLockTableRelid is defined. Second, do we really
> need to describeXact bool to DescribeLockTag..? Strikes me as
> unnecessary- if the information is available, include it.

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.

> Lastly, I really don't like the changes made to XactLockTableWait() and
> friends. They had a very clearly defined and simple goal previously and
> the additional parameter seems to muddy things a bit, particularly
> without any comments about what it's all about or why it's there. I
> understand that you're trying to pass the necessary information down to
> where the log is generated, but it doesn't feel quite right.

There is an API change to XactLockTableWait() to pass the relid. That
part is essential to any solution.

There are 2 possible designs for this...

(1) Simple - we remember the relid we are waiting on in a backend-only
variable so it can be used later when generating stats.

(2) Complex - record the additional information within the unused
fields of the locktag of a TRANSACTION lock, so they will be stored
and accessible by all, within the shared lock table itself. This would
require changing the hash function for transaction lock tags so that
only the key and not the additional payload items get hashed. That
seemed a much more contentious change.

> Also, what
> about VirtualXactLockTableWait()..? Should that have a similar
> treatment?

That isn't needed, IMHO. We wait on Xids whenever we see them on heap
tuples. Vxids aren't recorded anywhere, so they don't offer the same
blockage, which is basically for visibility checks on index creation.
That is more obvious. One might argue its needed as well, but I would
see it as a separate patch.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services