Re: Patch: show relation and tuple infos of a lock to acquire

On Thu, Mar 13, 2014 at 12:16 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Mar 11, 2014 at 3:53 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> Places where tuple info not available
>>
>> LOG:  process 5788 still waiting for ShareLock on transaction 679 after 1014.000
>>  ms
>> CONTEXT:  while attempting to operate in relation "public"."idx_t1" of database
>> "postgres"
>
> The way the context message is assembled piecemeal in
> XactLockTableWaitErrorContextCallback violates translation guidelines.
>  You need to have completely separate strings for each variant.
>
> While attempting to "operate in"?  That seems like unhelpful
> weasel-wording.  I wonder if we ought to have separate messages for
> each possibility, like "delete tuple (X,Y)" when called from
> heap_delete(), "update tuple (X,Y)", "check exclusion constraint on
> tuple (X,Y)" when called from check_exclusion_constraint, etc.  That
> seems like it would be handy information to have.

Okay, below are the distinct places from where we need to pass
such information.

heap_delete - "delete tuple (X,Y)"
heap_update - "update tuple (X,Y)"
heap_lock_tuple - "lock tuple (X,Y)"
heap_lock_updated_tuple_rec - "lock updated tuple (X,Y)"
_bt_doinsert - "insert index tuple (X,Y)" (here it will refer to index tuple
location)
IndexBuildHeapScan - "scan tuple (X,Y)"
EvalPlanQualFetch - "fetch tuple (X,Y)"
check_exclusion_constraint - "check exclusion constraint on tuple (X,Y)"

I think it might not be a big deal to update the patch to pass such info.
Won't it effect the translatability guidelines as we need to have different
translation message for each op?

> Why can't check_exclusion_constraint, for example, pass the TID, so
> that at least that much information is available?

I don't think there is as such any problem in passing TID, rather I think
if we can pass TID from all places, it will be a better way.

The other option could be we need to ensure which places are safe to
pass tuple so that we can display whole tuple instead of just TID,
for example the tuple we are passing from heap_lock_tuple() has been
fetched using Dirty Snapshot (refer EvalPlanQualFetch() caller of
heap_lock_tuple()), but still we can use it in error as it has been decided
that it is live tuple and transaction can update it by the time it reaches
XactLockTableWaitWithInfo(), so it is safe. I think we need to discuss
and validate all places where ever we use Dirty/Any Snapshot to ensure
that we can pass tuple from such a call, may be at end the result is
we can pass tuple from most of locations, but still it needs to be done
carefully.

> I'm not very happy with the idea of including the tuple details only
> when the level is less than ERROR.  For one thing, to do that in a way
> that respects translatability guidelines will require two versions of
> every string that would otherwise require only one.  For another
> thing, it seems like it's punting a pretty important case.  If we're
> gonna add context detail to lots of cases (instead only the "still
> waiting" case that people probably mostly care about) then we should
> actually print the details more-or-less consistently in all of those
> cases, not pretend like a solution that only works in the narrow case
> is more general than it really is.  I think we should really try hard
> to make the amount of detail provided as uniform as possible across
> all the cases, even if that means removing information from some cases
> where it might have been available.

Agreed and if we use TID, then it will address your concern.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com