On Thu, Oct 27, 2022 at 2:31 AM Simon Riggs <simon.ri...@enterprisedb.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