Horton

Re: race condition in pg_class

Noah Misch <noah@leadboat.com>

View thread
From: Noah Misch <noah@leadboat.com>
To: Robert Haas <robertmhaas@gmail.com>, Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>, Smolkin Grigory <smallkeen@gmail.com>
Date: 2024-06-12T17:54:52Z
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 Tue, Jun 11, 2024 at 01:37:21PM +0900, Michael Paquier wrote:
> On Mon, Jun 10, 2024 at 07:19:27PM -0700, Noah Misch wrote:
> > On Fri, Jun 07, 2024 at 09:08:03AM -0400, Robert Haas wrote:
> >> I think the core code should provide an "Injection Point" wait event
> >> type and let extensions add specific wait events there, just like you
> >> did for "Extension".
> > 
> > Michael, could you accept the core code offering that, or not?  If so, I am
> > content to implement that.  If not, for injection point wait events, I have
> > just one priority.  The isolation tester already detects lmgr locks without
> > the test writer teaching it about each lock individually.  I want it to have
> > that same capability for injection points. Do you think we can find something
> > everyone can accept, having that property?  These wait events show up in tests
> > only, and I'm happy to make the cosmetics be anything compatible with that
> > detection ability.
> 
> Adding a wait event class for injection point is an interesting
> suggestion that would simplify the detection in the isolation function
> quite a bit.  Are you sure that this is something that would be fit
> for v17 material?  TBH, I am not sure.

If I were making a list of changes always welcome post-beta, it wouldn't
include adding wait event types.  But I don't hesitate to add one if it
unblocks a necessary test for a bug present in all versions.

> At the end, the test coverage has the highest priority and the bugs
> you are addressing are complex enough that isolation tests of this
> level are a necessity, so I don't object to what
> inplace050-tests-inj-v2.patch introduces with the naming dependency
> for the time being on HEAD.  I'll just adapt and live with that
> depending on what I deal with, while trying to improve HEAD later on.

Here's what I'm reading for each person's willingness to tolerate each option:

STRATEGY                        | Paquier | Misch | Haas
--------------------------------------------------------
new "Injection Point" wait type | maybe   | yes   | yes
INJECTION_POINT(...) naming     | yes     | yes   | unknown
isolation spec says event names | yes     | no    | unknown

Corrections and additional strategy lines welcome.  Robert, how do you judge
the lines where I've listed you as "unknown"?

> I'm still wondering if there is something that could be more elegant
> than a dedicated class for injection points, but I cannot think about
> something that would be better for isolation tests on top of my head.
> If there is something I can think of, I'll just go and implement it :)

I once considered changing them to use advisory lock waits instead of
ConditionVariableSleep(), but I recall that was worse from the perspective of
injection points in critical sections.