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: > > 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
