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

On Mon, Mar 17, 2014 at 4:20 PM, Christian Kruse
<christian(at)2ndquadrant(dot)com> wrote:
> Hi Amit,
>
> I've been ill the last few days, so sorry for my late response.

Sorry to hear and no problem for delay.

> I don't think that this fixes the translation guideline issues brought
> up by Robert. This produces differing strings for the different cases
> as well and it also passes in altering data directly to the error
> string.
>
> It also may produce error messages that are really weird. You
> initialize the string with "while attempting to ". The remaining part
> of the function is covered by if()s which may lead to error messages
> like this:
>
> "while attempting to "
> "while attempting to in relation "public"."xyz" of database "abc""
> "while attempting to in database "abc""
>
> Although this may not be very likely (because
> ItemPointerIsValid(&(info->ctid))) should in this case not return
> false).

Yes, this is good point and even I have thought of it before sending
the patch. The reason why it seems not good to duplicate the
message is that I am not aware even of single case where
tuple info (chid, relinfo) will not be available by the time it will come
to wait on tuple in new call (XactLockTableWaitExtended), do you think
there exists any such case or it's just based on apprehension.
If there exists any such, then isn't it better to use earlier version of
API for that call site as is case we have left for
SnapBuildFindSnapshot?

If it's just apprehension, then I think it's better to have a check such
that if any of the tuple info is missing then don't add context and in
future if we have any such usecase then we can have multiple
messages.

> Attached you will find a new version of this patch; it slightly
> violates the translation guidelines as well: it assembles an error
> string (but it doesn't pass in altering data like ctid or things like
> that). I simply couldn't think of a nice solution without doing so,
> and after looking through the code there are a few cases
> (e.g. CheckTableNotInUse()) where this is done, too.

I could not see any problem in the modifications done by you,
but if the function is generic enough that it doesn't assume what
caller has set info, then why to assume that opr_name will always
be filled.

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