* Simon Riggs (si...@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? > 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
signature.asc
Description: Digital signature