Horton

Re: race condition in pg_class

Noah Misch <noah@leadboat.com>

View thread
From: Noah Misch <noah@leadboat.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: 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-08-22T07:32:00Z
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.

Attachments

On Tue, Aug 20, 2024 at 11:59:45AM +0300, Heikki Linnakangas wrote:
> On 17/08/2024 07:07, Noah Misch wrote:
> > On Fri, Aug 16, 2024 at 12:26:28PM +0300, Heikki Linnakangas wrote:
> > > I wonder if the functions should be called "systable_*" and placed in
> > > genam.c rather than in heapam.c. The interface looks more like the existing
> > > systable functions. It feels like a modularity violation for a function in
> > > heapam.c to take an argument like "indexId", and call back into systable_*
> > > functions.
> > 
> > Yes, _scan() and _cancel() especially are wrappers around systable.  Some API
> > options follow.  Any preference or other ideas?
> > 
> > ==== direct s/heap_/systable_/ rename [option 1]
> > 
> >   systable_inplace_update_scan([...], &tup, &inplace_state);
> >   if (!HeapTupleIsValid(tup))
> > 	elog(ERROR, [...]);
> >   ... [buffer is exclusive-locked; mutate "tup"] ...
> >   if (dirty)
> > 	systable_inplace_update_finish(inplace_state, tup);
> >   else
> > 	systable_inplace_update_cancel(inplace_state);
> > 
> > ==== make the first and last steps more systable-like [option 2]
> > 
> >   systable_inplace_update_begin([...], &tup, &inplace_state);
> >   if (!HeapTupleIsValid(tup))
> > 	elog(ERROR, [...]);
> >   ... [buffer is exclusive-locked; mutate "tup"] ...
> >   if (dirty)
> > 	systable_inplace_update(inplace_state, tup);
> >   systable_inplace_update_end(inplace_state);

> My order of preference is: 2, 1, 3.

I kept tuple locking responsibility in heapam.c.  That's simpler and better
for modularity, but it does mean we release+acquire after any xmax wait.
Before, we avoided that if the next genam.c scan found the same TID.  (If the
next scan finds the same TID, the xmax probably aborted.)  I think DDL aborts
are rare enough to justify simplifying as this version does.  I don't expect
anyone to notice the starvation outside of tests built to show it.  (With
previous versions, one can show it with a purpose-built test that commits
instead of aborting, like the "001_pgbench_grant@9" test.)

This move also loses the optimization of unpinning before XactLockTableWait().
heap_update() doesn't optimize that way, so that's fine.

The move ended up more like (1), though I did do
s/systable_inplace_update_scan/systable_inplace_update_begin/ like in (2).  I
felt that worked better than (2) to achieve lock release before
CacheInvalidateHeapTuple().  Alternatives that could be fine:

- In the cancel case, call both systable_inplace_update_cancel and
  systable_inplace_update_end.  _finish or _cancel would own unlock, while
  _end would own systable_endscan().

- Hoist the CacheInvalidateHeapTuple() up to the genam.c layer.  While
  tolerable now, this gets less attractive after the inplace160 patch from
  https://postgr.es/m/flat/20240523000548.58.nmisch@google.com

I made the other changes we discussed, also.

> > > Could we just stipulate that you must always hold LOCKTAG_TUPLE when you
> > > call heap_update() on pg_class or pg_database? That'd make the rule simple.
> > 
> > We could.  That would change more code sites.  Rough estimate:
> > 
> > $ git grep -E CatalogTupleUpd'.*(class|relrelation|relationRelation)' | wc -l
> > 23

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

> > Another option here would be to preface that README section with a simplified
> > view, something like, "If a warning brought you here, take a tuple lock.  The
> > rest of this section is just for people needing to understand the conditions
> > for --enable-casserts emitting that warning."  How about that instead of
> > simplifying the rules?
> 
> Works for me. Or perhaps the rules could just be explained more succinctly.
> Something like:
> 
> -----

I largely used your text instead.


While doing these updates, I found an intra-grant-inplace.spec permutation
being flaky on inplace110 but stable on inplace120.  That turned out not to be
v9-specific.  As of patch v1, I now see it was already flaky (~5% failure
here).  I've now added to inplace110 a minimal tweak to stabilize that spec,
which inplace120 removes.

Thanks,
nm