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.
