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

Hi,

thanks for the continued review.

On 09/03/14 12:15, Amit Kapila wrote:
> 1.
> "> ISTM that you should be printing just the value and the unique index
> > there, and not any information about the tuple proper.
>
> Good point, I will have a look at this."
>
> This point is still not handled in patch, […]

There have been some complaints about this by Andres, so I left it.

> […] 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.

> 2. In function XactLockTableWaitErrorContextCallback(), ignore dropped
> columns in tuple, else it will lead to following failure:
> […]
> Problem is in Session-2 (cache lookup failed), when it tries to print values
> for dropped attribute.

Urghs. I really thought I had done this in the patch before. Thanks
for pointing out, fixed in attached patch.

> 3.
> " in relation \"%s\".\"%s\" of database %s",
> Why to display only relation name in quotes?
> I think database name should also be displayed in quotes.

Didn't think that it would make sense; the quoting makes sense for the
schema and relation name, but I did not see any need to quote the
database name. However, fixed in attached patch.

> 4.
> Context message:
> "while attempting to lock tuple (0,2) ".
>
> I think it will be better to rephrase it (may be by using 'operate on'
> instead of 'lock').
> The reason is that actually we trying to lock on transaction, so it doesn't make
> good sense to use "lock on tuple"

Fixed.

> 5.
> + XactLockTableWaitWithInfo(relation, &tp, xwait);
>
> + MultiXactIdWaitWithErrorContext(relation, &tp, (MultiXactId) xwait,
> +  MultiXactStatusUpdate, NULL, infomask);
>
> I think we should make the name of MultiXactIdWaitWithErrorContext()
> similar to XactLockTableWaitWithInfo.

Fixed as well.

Can you explain why you prefer the WithInfo naming? Just curious, it
seemed to me the more descriptive name should have been preferred.

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

> 8.
>  void
>  WaitForLockers(LOCKTAG heaplocktag, LOCKMODE lockmode)
>  {
> - List   *l;
> + List   *l;
>
> Extra spacing not needed.

What is the policy to do here? The extra spaces have been added by
pg_indent, and there have been more changes in a couple of other files
which I had to drop manually as well. How should this been handled
generally?

> 9.
> /*
>  * XactLockTableWaitErrorContextCallback
>  * error context callback set up by
>  * XactLockTableWaitWithInfo. It reports some
>  * tuple information and the relation of a lock to aquire
>  *
>  */
> Please improve indentation of above function header

Heh. Seems that Emacs' fill-paragraph messed this up. Thanks, fixed.

> 10.
> +#include "access/htup.h"
> +
> +struct XactLockTableWaitLockInfo
> +{
> + Relation rel;
> + HeapTuple tuple;
> +};
>
> I think it is better to add comments for this structure.
> You can refer comments for struct HeapUpdateFailureData

Fixed.

>
> 11.
> + *
> + * Use this instead of calling XactTableLockWait()
>
> In above comment, function name used is wrong and I am not sure this
> is right statement to use because still we are using XactLockTableWait()
> at few places like in function Do_MultiXactIdWait() […]

Fixed function name, but I'd like to keep the wording;
Do_MultiXactIdWait() is wrapped by MultiXactIdWaitWithInfo() and
therefore sets up the context as well.

> […] and recently this is used in function SnapBuildFindSnapshot().

Hm, should we add the context there, too?

> 12.
> heap_update()
> {
> ..
> ..
> /*
>  * There was no UPDATE in the MultiXact; or it aborted. No
>  * TransactionIdIsInProgress() call needed here, since we called
>  * MultiXactIdWait() above.
>
> Change the function name in above comment.

Fixed.

Best regards,

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