Horton

Re: race condition in pg_class

Nitin Motiani <nitinmotiani@google.com>

View thread
From: Nitin Motiani <nitinmotiani@google.com>
To: 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-08-29T15:38:43Z
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, Aug 29, 2024 at 8:11 PM Noah Misch <noah@leadboat.com> wrote:
>
> On Tue, Aug 20, 2024 at 11:59:45AM +0300, Heikki Linnakangas wrote:
> > 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:
>
>From a consistency point of view, I find it cleaner if we can have all
the heap_inplace_lock and heap_inplace_unlock in the same set of
functions. So here those would be the systable_inplace_* functions.

> - 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().
>
What happens to CacheInvalidateHeapTuple() in this approach?  I think
it will still need to be brought to the genam.c layer if we are
releasing the lock in systable_inplace_update_finish.

> - 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 skimmed through the inplace160 patch. It wasn't clear to me why this
becomes less attractive with the patch. I see there is a new
CacheInvalidateHeapTupleInPlace but that looks like it would be called
while holding the lock. And then there is an
AcceptInvalidationMessages which can perhaps be moved to the genam.c
layer too. Is the concern that one invalidation call will be in the
heapam layer and the other will be in the genam layer?

Also I have a small question from inplace120.

I see that all the places we check resultRelInfo->ri_needLockTagTuple,
we can just call
IsInplaceUpdateRelation(resultRelInfo->ri_RelationDesc). Is there a
big advantage of storing a separate bool field? Also there is another
write to ri_RelationDesc in CatalogOpenIndexes in
src/backlog/catalog/indexing.c. I think ri_needLockTagTuple needs to
be set there also to keep it consistent with ri_RelationDesc. Please
let me know if I am missing something about the usage of the new
field.

Thanks & Regards,
Nitin Motiani
Google