Re: Allow single table VACUUM in transaction block

On Thu, Oct 27, 2022 at 2:31 AM Simon Riggs
<simon(dot)riggs(at)enterprisedb(dot)com> wrote:
> Fix, so that this works without issue:
>
> BEGIN;
> ....
> VACUUM (ANALYZE) vactst;
> ....
> COMMIT;
>
> Allows both ANALYZE and vacuum of toast tables, but not VACUUM FULL.
>
> When in a xact block, we do not set PROC_IN_VACUUM,
> nor update datfrozenxid.

It doesn't seem like a good idea to add various new special cases to
VACUUM just to make scripts like this work. I'm pretty sure that there
are several deep, subtle reasons why VACUUM cannot be assumed safe to
run in a user transaction.

For example, the whole way that index page deletion is decoupled from
recycling in access methods like nbtree (see "Placing deleted pages in
the FSM" from the nbtree README) rests upon delicate assumptions about
whether or not there could be an "in-flight" B-tree descent that is
at risk of landing on a deleted page as it is concurrently recycled.
In general the deleted page has to remain in place as a tombstone,
until that is definitely not possible anymore. This relies on the backend
that runs VACUUM having no references to the page pending deletion.
(Commit 9dd963ae25 added an optimization that heavily leaned on the
idea that the state within the backend running VACUUM couldn't
possibly have a live page reference that is at risk of being broken by
the optimization, though I'm pretty sure that you'd have problems even
without that commit/optimization in place.)

My guess is that there are more things like that. Possibly even things
that were never directly considered. VACUUM evolved in a world where
we absolutely took not running in a transaction for granted. Changing
that now is a pretty big deal. Maybe it would all be worth it if the end
result was a super compelling feature. But I for one don't think that
it will be.

If we absolutely have to do this, then the least worst approach might
be to make VACUUM into a no-op rather than throwing an ERROR -- demote
the ERROR into a WARNING. You could argue that we're just arbitrarily
deciding to not do a VACUUM just to be able to avoid throwing an error
if we do that. But isn't that already true with the patch that we
have? Is it really a "true VACUUM" if the operation can never advance
datfrozenxid? At least selectively demoting the ERROR to a WARNING is
"transparent" about it.

--
Peter Geoghegan