Re: log_lock_waits to identify transaction's relation

* Simon Riggs (simon(at)2ndquadrant(dot)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?

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

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

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.

> (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.

As the lock table is itself contended for greatly, I agree that we
probably don't want to go changing it. It would be nice if we had a way
to associate additional meta-data with why a lock is being attempted
though and for logging it.

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

Alright.

Thanks,

Stephen