Re: race condition in pg_class

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:

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.