On Fri, Sep 6, 2024 at 3:34 AM Noah Misch <noah@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 <noah@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.
