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

On Mon, Mar 10, 2014 at 5:14 PM, Christian Kruse
<christian(at)2ndquadrant(dot)com> wrote:
> On 09/03/14 12:15, Amit Kapila wrote:
>> [...] due to which the message it displays seems to be
>> incomplete. Message it displays is as below:
>>
>> LOG: process 1800 still waiting for ShareLock on transaction 679 after 1014.000
>> ms
>> CONTEXT: while attempting to lock in relation "public"."idx_t1" of database pos
>> tgres
>
> Well, there is no tuple information available, so we have to leave it out.
>
>> Here it is not clear what it attempts *lock in*
>
> Ok, I reworded the message as you suggested below. Should make this
> clearer as well.

As per your recent changes, wherever we have tuple info, it will be used in
message, else it will just use relinfo. So the message difference will be as
below which I think is okay.

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"

Places where tuple info available

LOG: process 5788 still waiting for ShareLock on transaction 686 after 1014.000
ms
CONTEXT: while attempting to operate on tuple (0,1) with values (1, aaa, bbb) i
n relation "public"."t1" of database "postgres"

Actually, I had thought of passing tuple info from places where currently
it is not passing, but it will need some extra code which I think is not
worthy for message information.
For Example in case of _bt_doinsert(), if we have to pass index tuple
info, we might need to change prototype of XactLockTableWaitWithInfo()
such that it can accept both IndexTuple and HeapTuple and then use
inside function
accordingly. Also I think this is okay to not display Index tuple here, as it
is still not visible. For other cases where we are using DirtySnapshot,
the things can be more complex to identify if tuple can be logged. So
I think it's better to leave logging it as you have done in patch.
>
>
> Can you explain why you prefer the WithInfo naming? Just curious, it
> seemed to me the more descriptive name should have been preferred.

Actually, apart from MultiXactIdWaitWithErrorContext(), I had
thought of below options:

1. Change the prototype of MultiXactIdWait()/XactLockTableWait()
to pass rel and tuple info and make new functions XactLockTableWait_Internal()
which will do the actual work, but to achieve that we need call
internal function from some places where top level is not getting
called. Such coding convetion is used at few places (heap_beginscan_internal).

2. Name new functions as MultiXactIdWaitExtended()/XactLockTableWaitExtended()
or MultiXactIdWaitEx()/XactLockTableWaitEx().
You can find some other similar functions (ReadBufferExtended,
relation_openrv_extended)

3. MultiXactIdWaitWithInfo()/XactLockTableWaitWithInfo()

Earlier I found option 3 as a good choice, but now again thinking on
it I am leaning towards option 2.

>> 6.
>> @@ -1981,7 +1981,8 @@ EvalPlanQualFetch(EState *estate, Relation
>> relation, int lockmode,
>> if (TransactionIdIsValid(SnapshotDirty.xmax))
>> {
>> ReleaseBuffer(buffer);
>> - XactLockTableWait(SnapshotDirty.xmax);
>> + XactLockTableWaitWithInfo(relation, &tuple,
>> + SnapshotDirty.xmax);
>>
>> In function EvalPlanQualFetch(), we are using SnapshotDirty to fetch
>> the tuple, so I think there is a chance that it will log the tuple which
>> otherwise will not be visible. I don't think this is right.
>
> Hm, after checking source I tend to agree. I replaced it with NULL.

Today, again looking into it, I found that function
heap_lock_updated_tuple_rec() is using SnapshotAny to fetch the tuple
and I think for this case also it's not safe to Log the tuple.

Could you please once check (if you are comfortable doing so) wherever
this patch is passing tuple, whether it is okay to pass it based on visibility
rules, else I will again verify it once.

>> 8.
>> void
>> WaitForLockers(LOCKTAG heaplocktag, LOCKMODE lockmode)
>> {
>> - List *l;
>> + List *l;
>>
>> Extra spacing not needed.
>
> What is the policy to do here?

The simple rule I follow is don't touch the code which has no relation
to current patch.

>> [...] and recently this is used in function SnapBuildFindSnapshot().
>
> Hm, should we add the context there, too?
>
I don't think you can use it here as there is no relation or tuple
information available. This is a unique kind of usage for this API where
it is used to wait for a transaction to complete without operating on
relation/tuple, whereas all other places this is used to wait if some other
transaction is operating on a tuple.

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