On Fri, Sep 6, 2024 at 3:34 AM Noah Misch <n...@leadboat.com> wrote: > > On Thu, Sep 05, 2024 at 07:10:04PM +0530, Nitin Motiani wrote: > > On Thu, Sep 5, 2024 at 1:27 AM Noah Misch <n...@leadboat.com> wrote: > > > On Wed, Sep 04, 2024 at 09:00:32PM +0530, Nitin Motiani wrote: > > > > Assert(rel->ri_needsLockTagTuple == > > > > IsInplaceUpdateRelation(rel->relationDesc) > > > > > > > > This can safeguard against users of ResultRelInfo missing this field. > > > > > > v10 does the rename and adds that assertion. This question remains open: > > > > Looks good. A couple of minor comments : > > 1. In the inplace110 commit message, there are still references to > > heap_inplace_update. Should it be clarified that the function has been > > renamed? > > PGXN has only one caller of this function, so I think that wouldn't help > readers enough. If someone gets a compiler error about the old name, they'll > figure it out without commit log guidance. If a person doesn't get a compiler > error, they didn't need to read about the fact of the rename. > > > 2. Should there be a comment above the ri_needLockTag definition in > > execNodes.h that we are caching this value to avoid function calls to > > IsInPlaceUpdateRelation for every tuple? Similar to how the comment > > above ri_TrigFunctions mentions that it is cached lookup info. > > Current comment: > > /* updates do LockTuple() before oldtup read; see README.tuplock */ > bool ri_needLockTagTuple; > > Once the comment doesn't fit in one line, pgindent rules make it take a > minimum of four lines. I don't think words about avoiding function calls > would add enough value to justify the vertical space, because a person > starting to remove it would see where it's called. That's not to say the > addition would be negligent. If someone else were writing the patch and had > included that, I wouldn't be deleting the material.
Thanks. I have no other comments.
