On Sat, 5 Nov 2022 at 15:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Greg Stark <stark@mit.edu> writes:
> > Simple Rebase
>
> I took a little bit of a look through these.
>
> * I find 0001 a bit scary, specifically that it's decided it's
> okay to apply extract_autovac_opts, pgstat_fetch_stat_tabentry_ext,
> and especially relation_needs_vacanalyze to another session's
> temp table. How safe is that really?
So I don't see any evidence we skip any locking on pg_class when doing
updates on rows for temporary tables. It's a bit hard to tell because
there are several ways of checking if a table is temporary. Most
places just check relpersistence but there is also an accessing macro
RelationIsPermanent() as well as a relcache field rd_islocaltemp which
could be used. I'm still looking But so far nearly all the checks I've
found just throw errors for temporary tables and none relate to any
operations on pg_class entries.
In any case we're already using the pg_class struct to look at
relpersistence itself.... So... the danger to check for is something
we would already be at risk of. Namely that the pg_class row is
updated without any locking and then vacuumed away while we hold this
struct pointer and we're looking at fields that have since been
overwritten with other data from an unrelated row. But that would
require all kinds of rules to be broken and would be posing a risk for
anyone just running select * from pg_class. So I find it hard to
believe we would be doing this.
extract_autovac_opts looks at a variable sized field so concurrent
updates would be an issue, but obviously there are only mvcc updates
to this field so I don't see how it could be a problem.
pgstat_fetch_stat_tabentry I don't even see what the possible risks
would be. The words persistence and temporary don't appear in pgstat.c
(except "temporary statistics" in some log messages).
And then there's relation_needs_vacanalyze() and it looks at
relfrozenxid and relminmxid (and relname in some debug messages).
Those fields could get updated by a concurrent vacuum or -- after this
patch -- a truncate in an inplace_update. That seems to be the only
real risk here.
But this is not related to temporary tables at all. Any pg_class entry
can get in_place_update'd by plain old vacuum to update the
relfrozenxid and relminmxid. The in_place_update would take an
exclusive lock on the buffer but I think that doesn't actually protect
us since autovacuum would only have a pin? Or does the SysCache
protect us by copying out the whole row while it's locked? This is
worth answering but it's not an issue related to this patch or
temporary tables. Is autovacuum looking at relfrozenxid and relminmxid
in a way that's safely protected against a concurrent manual vacuum
issuing an in_place_update?
--
greg
