On Tue, Jun 26, 2018 at 11:40 AM, Amit Khandekar <amitdkhan...@gmail.com> wrote: > On 25 June 2018 at 17:20, Amit Kapila <amit.kapil...@gmail.com> wrote: >> On Mon, Jun 25, 2018 at 3:06 PM, Amit Khandekar <amitdkhan...@gmail.com> >> wrote: >>> On 23 June 2018 at 15:46, Amit Kapila <amit.kapil...@gmail.com> wrote: >>>>> >>>> >>>> Why do you need to update when newslot is NULL? Already *epqslot is >>>> initialized as NULL in the caller (ExecUpdate). I think we only want >>>> to update it when trigtuple is not null. >>> >>> But GetTupleForTrigger() updates the epqslot to NULL even when it >>> returns NULL. So to be consistent with it, we want to do the same >>> thing for ExecBRDeleteTriggers() : Update the epqslot even when >>> GetTupleForTrigger() returns NULL. >>> >>> I think the reason we are doing "*newSlot = NULL" as the very first >>> thing in the GetTupleForTrigger() code, is so that callers don't have >>> to initialise newSlot to NULL before calling GetTupleForTrigger. And >>> so ExecBRUpdateTriggers() does not initialize newSlot with NULL. We >>> can follow the same approach while calling ExecDelete(). >>> >> >> Yeah, we can do that if it is required. > > It is not required as such, and there is no correctness issue. > >> I see your point, but I feel that is making code bit less readable. > > I did it that way only to be consistent with the existing trigger.c > code, namely ExecBRUpdateTriggers() where it sets newSlot to NULL > immediately. If you find some appropriate comments to make that > snippet more readable, that can work. > > Or else, we can go ahead with the way you did it in your patch; and > additionally mention in the ExecBRDeleteTriggers() header comments > that epqslot value is undefined if there was no concurrently updated > tuple passed. To me, explaining this last part seems confusing. >
Yeah, so let's leave it as it is in the patch. I think now we should wait and see what Alvaro has to say about the overall patch. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
