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:
> > How about this alternative then? The tuple length check
> > and the elog(ERROR) gets its own function. Something like
> > heap_inplace_update_validate or
> > heap_inplace_update_validate_tuple_length. So in that case, it would
> > look like this :
> >
> > genam.c:systable_inplace_update_finish
> > heapam.c:heap_inplace_update_validate/heap_inplace_update_precheck
> > PreInplace_Inval
> > START_CRIT_SECTION
> > heapam.c:heap_inplace_update
> > BUFFER_LOCK_UNLOCK
> > AtInplace_Inval
> > END_CRIT_SECTION
> > UnlockTuple
> > AcceptInvalidationMessages
> >
> > This is starting to get complicated though so I don't have any issues
> > with just renaming the heap_inplace_update to
> > heap_inplace_update_and_unlock.
>
> Complexity aside, I don't see the _precheck design qualifying as a modularity
> improvement.
>
> > 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?
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.
> On Thu, Aug 22, 2024 at 12:32:00AM -0700, Noah Misch wrote:
> > On Tue, Aug 20, 2024 at 11:59:45AM +0300, Heikki Linnakangas wrote:
> > > How many of those for RELKIND_INDEX vs tables? I'm thinking if we should
> > > always require a tuple lock on indexes, if that would make a difference.
> >
> > Three sites. See attached inplace125 patch. Is it a net improvement? If so,
> > I'll squash it into inplace120.
>
> If nobody has an opinion, I'll discard inplace125. I feel it's not a net
> improvement, but either way is fine with me.
Seems moderately simpler to me. But there is still special handling
for the RELKIND_INDEX. Just that instead of doing it in
systable_inplace_update_begin, we have a special case in heap_update.
So overall it's only a small improvement and I'm fine either way.
Thanks & Regards
Nitin Motiani
Google
