Re: race condition in pg_class

On 17/08/2024 07:07, Noah Misch wrote:
> On Fri, Aug 16, 2024 at 12:26:28PM +0300, Heikki Linnakangas wrote:
>> On 14/07/2024 20:48, Noah Misch wrote:
>>> + * ... [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 [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);
>
> ==== no systable_ wrapper for middle step, more like CatalogTupleUpdate [option 3]
>
> 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);

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

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

Ok.

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.

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

Works for me. Or perhaps the rules could just be explained more
succinctly. Something like:

-----
pg_class heap_inplace_update_scan() callers: before the call, acquire a
lock on the relation in mode ShareUpdateExclusiveLock or stricter. 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 not necessary. (This allows VACUUM to overwrite per-index
pg_class while holding a lock on the table alone.)
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 on the tuple, or a ShareUpdateExclusiveLock or stricter on the
relation.

SearchSysCacheLocked1() is one convenient way to acquire the tuple lock.
Most heap_update() callers already hold a suitable lock on the relation
for other reasons, and can skip the tuple lock. If you do acquire the
tuple lock, release it immediately after the update.

pg_database: before copying the tuple to modify, all updaters of
pg_database rows acquire LOCKTAG_TUPLE. (Few updaters acquire
LOCKTAG_OBJECT on the database OID, so it wasn't worth extending that as
a second option.)
-----

--
Heikki Linnakangas
Neon (https://neon.tech)