On Mon, Mar 17, 2014 at 4:20 PM, Christian Kruse <christ...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
