Horton

Re: race condition in pg_class

Noah Misch <noah@leadboat.com>

View thread
From: Noah Misch <noah@leadboat.com>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>, pgsql-hackers@postgresql.org, Smolkin Grigory <smallkeen@gmail.com>, Michael Paquier <michael@paquier.xyz>
Date: 2024-07-04T02:57:07Z
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 Wed, Jul 03, 2024 at 04:09:54PM -0700, Noah Misch wrote:
> On Wed, Jul 03, 2024 at 06:00:00AM +0300, Alexander Lakhin wrote:
> > 29.06.2024 05:42, Noah Misch wrote:
> > > Good point, any effort on (2) would be wasted once the fixes get certified.  I
> > > pushed (1).  I'm attaching the rebased fix patches.
> > 
> > Please look at a new anomaly, introduced by inplace110-successors-v5.patch:
> > CREATE TABLE t (i int) PARTITION BY LIST(i);
> > CREATE TABLE p1 (i int);
> > ALTER TABLE t ATTACH PARTITION p1 FOR VALUES IN (1);
> > ALTER TABLE t DETACH PARTITION p1;
> > ANALYZE t;
> > 
> > triggers unexpected
> > ERROR:  tuple to be updated was already modified by an operation triggered by the current command
> 
> Thanks.  Today, it's okay to issue heap_inplace_update() after heap_update()
> without an intervening CommandCounterIncrement().

Correction: it's not okay today.  If code does that, heap_inplace_update()
mutates a tuple that is going to become invisible at CCI.  The lack of CCI
yields a minor live bug in v14+.  Its consequences seem to be limited to
failing to update reltuples for a partitioned table having zero partitions.

> The patch makes the CCI
> required.  The ANALYZE in your example reaches this with a heap_update to set
> relhassubclass=f.  I've fixed this by just adding a CCI (and adding to the
> tests in vacuum.sql).

That's still the right fix, but I've separated it into its own patch and
expanded the test.  All the non-comment changes between v5 and v6 are now part
of the separate patch.

> The alternative would be to allow inplace updates on TM_SelfModified tuples.
> I can't think of a specific problem with allowing that, but I feel that would
> make system state interactions harder to reason about.  It might be optimal to
> allow that in back branches only, to reduce the chance of releasing a bug like
> the one you found.

Allowing a mutation of a TM_SelfModified tuple is bad, since that tuple is
going to become dead soon.  Mutating its successor could be okay.  Since we'd
expect such code to be unreachable, I'm not keen carry such code.  For that
scenario, I'd rather keep the error you encountered.  Other opinions?