Horton

Re: race condition in pg_class

Nitin Motiani <nitinmotiani@google.com>

View thread
From: Nitin Motiani <nitinmotiani@google.com>
To: Noah Misch <noah@leadboat.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, Robert Haas <robertmhaas@gmail.com>, pgsql-hackers@postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>, Smolkin Grigory <smallkeen@gmail.com>, Michael Paquier <michael@paquier.xyz>, Alexander Lakhin <exclusion@gmail.com>
Date: 2024-09-05T13:40:04Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Replace tests of ALTER DATABASE RESET TABLESPACE.

  2. meson: Flush stdout in testwrap

  3. Fix catcache invalidation of a list entry that's being built

  4. Stop reading uninitialized memory in heap_inplace_lock().

  5. Fix use of uninitialized value in previous commit.

  6. Back-patch "Refactor code in tablecmds.c to check and process tablespace moves"

  7. Fix data loss at inplace update after heap_update().

  8. For inplace update durability, make heap_update() callers wait.

  9. Warn if LOCKTAG_TUPLE is held at commit, under debug_assertions.

  10. Don't lose partitioned table reltuples=0 after relhassubclass=f.

  11. Fix new assertion for MERGE view_name ... DO NOTHING.

  12. Remove configuration-dependent output from new inplace-inval test.

  13. AccessExclusiveLock new relations just after assigning the OID.

  14. Cope with inplace update making catcache stale during TOAST fetch.

  15. Expand comments and add an assertion in nodeModifyTable.c.

  16. Improve test coverage for changes to inplace-updated catalogs.

  17. Lock before setting relhassubclass on RELKIND_PARTITIONED_INDEX.

  18. Lock owned sequences during ALTER TABLE SET { LOGGED | UNLOGGED }.

  19. Make TAP todo_start effects the same under Meson and prove_check.

  20. Add an injection_points isolation test suite.

  21. Add wait event type "InjectionPoint", a custom type like "Extension".

  22. Create waitfuncs.c for pg_isolation_test_session_is_blocked().

  23. Rework planning and execution of UPDATE and DELETE.

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