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-17T04:07:48Z
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.

Thanks for reviewing.

On Fri, Aug 16, 2024 at 12:26:28PM +0300, Heikki Linnakangas wrote:
> On 14/07/2024 20:48, Noah Misch wrote:
> > I've pushed the two patches for your reports.  To placate cfbot, I'm attaching
> > the remaining patches.
> 
> inplace090-LOCKTAG_TUPLE-eoxact-v8.patch: Makes sense. A comment would be in
> order, it looks pretty random as it is. Something like:
> 
> /*
>  * Tuple locks are currently only held for short durations within a
>  * transaction. Check that we didn't forget to release one.
>  */

Will add.

> inplace110-successors-v8.patch: Makes sense.
> 
> The README changes would be better as part of the third patch, as this patch
> doesn't actually do any of the new locking described in the README, and it
> fixes the "inplace update updates wrong tuple" bug even without those tuple
> locks.

That should work.  Will confirm.

> > + * ... [any slow preparation not requiring oldtup] ...
> > + * heap_inplace_update_scan([...], &tup, &inplace_state);
> > + * if (!HeapTupleIsValid(tup))
> > + *	elog(ERROR, [...]);
> > + * ... [buffer is exclusive-locked; mutate "tup"] ...
> > + * if (dirty)
> > + *	heap_inplace_update_finish(inplace_state, tup);
> > + * else
> > + *	heap_inplace_update_cancel(inplace_state);
> 
> 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

 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

 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);

==== no systable_ wrapper for middle step, more like CatalogTupleUpdate

 systable_inplace_update_begin([...], &tup, &inplace_state);
 if (!HeapTupleIsValid(tup))
	elog(ERROR, [...]);
 ... [buffer is exclusive-locked; mutate "tup"] ...
 if (dirty)
	heap_inplace_update(relation,
						systable_inplace_old_tuple(inplace_state),
						tup,
						systable_inplace_buffer(inplace_state));
 systable_inplace_update_end(inplace_state);

> > 	/*----------
> > 	 * XXX A crash here can allow datfrozenxid() to get ahead of relfrozenxid:
> > 	 *
> > 	 * ["D" is a VACUUM (ONLY_DATABASE_STATS)]
> > 	 * ["R" is a VACUUM tbl]
> > 	 * D: vac_update_datfrozenid() -> systable_beginscan(pg_class)
> > 	* c * D: systable_getnext() returns pg_class tuple of tbl
> > 	 * R: memcpy() into pg_class tuple of tbl
> > 	 * D: raise pg_database.datfrozenxid, XLogInsert(), finish
> > 	 * [crash]
> > 	 * [recovery restores datfrozenxid w/o relfrozenxid]
> > 	 */
> 
> Hmm, that's a tight race, but feels bad to leave it unfixed. One approach
> would be to modify the tuple on the buffer only after WAL-logging it. That
> way, D cannot read the updated value before it has been WAL logged. Just
> need to make sure that the change still gets included in the WAL record.
> Maybe something like:
> 
> if (RelationNeedsWAL(relation))
> {
>     /*
>      * Make a temporary copy of the page that includes the change, in
>      * case the a full-page image is logged
>      */
>     PGAlignedBlock tmppage;
> 
>     memcpy(tmppage.data, page, BLCKSZ);
> 
>     /* copy the tuple to the temporary copy */
>     memcpy(...);
> 
>     XLogRegisterBlock(0, ..., tmppage, REGBUF_STANDARD);
>     XLogInsert();
> }
> 
> /* copy the tuple to the buffer */
> memcpy(...);

Yes, that's the essence of
inplace180-datfrozenxid-overtakes-relfrozenxid-v1.patch from
https://postgr.es/m/flat/20240620012908.92.nmisch@google.com.

> >   pg_class heap_inplace_update_scan() callers: before the call, acquire
> >   LOCKTAG_RELATION in mode ShareLock (CREATE INDEX), ShareUpdateExclusiveLock
> >   (VACUUM), or a mode with strictly more conflicts.  If the update targets a
> >   row of RELKIND_INDEX (but not RELKIND_PARTITIONED_INDEX), that lock must be
> >   on the table.  Locking the index rel is optional.  (This allows VACUUM to
> >   overwrite per-index pg_class while holding a lock on the table alone.)  We
> >   could allow weaker locks, in which case the next paragraph would simply call
> >   for stronger locks for its class of commands.  heap_inplace_update_scan()
> >   acquires and releases LOCKTAG_TUPLE in InplaceUpdateTupleLock, an alias for
> >   ExclusiveLock, on each tuple it overwrites.
> > 
> >   pg_class heap_update() callers: before copying the tuple to modify, take a
> >   lock that conflicts with at least one of those from the preceding paragraph.
> >   SearchSysCacheLocked1() is one convenient way to acquire LOCKTAG_TUPLE.
> >   After heap_update(), release any LOCKTAG_TUPLE.  Most of these callers opt
> >   to acquire just the LOCKTAG_RELATION.
> 
> These rules seem complicated. Phrasing this slightly differently, if I
> understand correctly: for a heap_update() caller, it's always sufficient to
> hold LOCKTAG_TUPLE, but if you happen to hold some other lock on the
> relation that conflicts with those mentioned in the first paragraph, then
> you can skip the LOCKTAG_TUPLE lock.

Yes.

> 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

If the count were 2, I'd say let's simplify the rule like you're exploring.
(I originally had a complicated rule for pg_database, but I abandoned that
when it helped few code sites.)  If it were 100, I'd say the complicated rule
is worth it.  A count of 23 makes both choices fair.

Long-term, I hope relfrozenxid gets reimplemented with storage outside
pg_class, removing the need for inplace updates.  So the additional 23 code
sites might change back at a future date.  That shouldn't be a big
consideration, though.

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?