Allow single table VACUUM in transaction block

On Thu, 27 Oct 2022 at 10:31, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

Tests, docs.

The patch tester says that a pg_upgrade test is failing on Windows,
but works for me.

t/002_pg_upgrade.pl .. ok

Anybody shed any light on that, much appreciated.

--
Simon Riggs http://www.EnterpriseDB.com/

On Thu, Oct 27, 2022 at 9:49 PM Simon Riggs
<simon.riggs@enterprisedb.com> wrote:

On Thu, 27 Oct 2022 at 10:31, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

Tests, docs.

The patch tester says that a pg_upgrade test is failing on Windows,
but works for me.

t/002_pg_upgrade.pl .. ok

Anybody shed any light on that, much appreciated.

Please see a recent thread on pg_upgrade failure -
/messages/by-id/Y04mN0ZLNzJywrad@paquier.xyz.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Thu, Oct 27, 2022 at 10:31:31AM +0100, Simon Riggs wrote:

Allows both ANALYZE and vacuum of toast tables, but not VACUUM FULL.

Maybe I misunderstood what you meant: you said "not VACUUM FULL", but
with your patch, that works:

postgres=# begin; VACUUM FULL pg_class; commit;
BEGIN
VACUUM
COMMIT

Actually, I've thought before that it was bit weird that CLUSTER can be
run within a transaction, but VACUUM FULL cannot (even though it does a
CLUSTER behind the scenes). VACUUM FULL can process multiple relations,
whereas CLUSTER can't, but it seems nice to allow vacuum full for the
case of a single relation.

I haven't checked the rest of the patch, but +1 for allowing VACUUM FULL
within a user txn.

Maybe the error message needs to be qualified "...when multiple
relations are specified".

ERROR: VACUUM cannot run inside a transaction block

--
Justin

On Thu, 27 Oct 2022 at 21:07, Justin Pryzby <pryzby@telsasoft.com> wrote:

On Thu, Oct 27, 2022 at 10:31:31AM +0100, Simon Riggs wrote:

Allows both ANALYZE and vacuum of toast tables, but not VACUUM FULL.

Maybe I misunderstood what you meant: you said "not VACUUM FULL", but
with your patch, that works:

postgres=# begin; VACUUM FULL pg_class; commit;
BEGIN
VACUUM
COMMIT

Actually, I've thought before that it was bit weird that CLUSTER can be
run within a transaction, but VACUUM FULL cannot (even though it does a
CLUSTER behind the scenes). VACUUM FULL can process multiple relations,
whereas CLUSTER can't, but it seems nice to allow vacuum full for the
case of a single relation.

I haven't checked the rest of the patch, but +1 for allowing VACUUM FULL
within a user txn.

My intention was to prevent that. I am certainly quite uneasy about
changing anything related to CLUSTER/VF, since they are old, complex
and bug prone.

So for now, I will block VF, as was my original intent.

I will be guided by what others think... so you may yet get your wish.

Maybe the error message needs to be qualified "...when multiple
relations are specified".

ERROR: VACUUM cannot run inside a transaction block

Hmm, that is standard wording based on the statement type, but I can
set a CONTEXT message also. Will update accordingly.

Thanks for your input.

--
Simon Riggs http://www.EnterpriseDB.com/

On Tue, 1 Nov 2022 at 23:56, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

I haven't checked the rest of the patch, but +1 for allowing VACUUM FULL
within a user txn.

My intention was to prevent that. I am certainly quite uneasy about
changing anything related to CLUSTER/VF, since they are old, complex
and bug prone.

So for now, I will block VF, as was my original intent.

I will be guided by what others think... so you may yet get your wish.

Maybe the error message needs to be qualified "...when multiple
relations are specified".

ERROR: VACUUM cannot run inside a transaction block

Hmm, that is standard wording based on the statement type, but I can
set a CONTEXT message also. Will update accordingly.

Thanks for your input.

New version attached, as described.

Other review comments and alternate opinions welcome.

--
Simon Riggs http://www.EnterpriseDB.com/

Hi Simon,

On Thu, Nov 3, 2022 at 3:53 PM Simon Riggs <simon.riggs@enterprisedb.com>
wrote:

On Tue, 1 Nov 2022 at 23:56, Simon Riggs <simon.riggs@enterprisedb.com>
wrote:

I haven't checked the rest of the patch, but +1 for allowing VACUUM

FULL

within a user txn.

My intention was to prevent that. I am certainly quite uneasy about
changing anything related to CLUSTER/VF, since they are old, complex
and bug prone.

So for now, I will block VF, as was my original intent.

I will be guided by what others think... so you may yet get your wish.

Maybe the error message needs to be qualified "...when multiple
relations are specified".

ERROR: VACUUM cannot run inside a transaction block

Hmm, that is standard wording based on the statement type, but I can
set a CONTEXT message also. Will update accordingly.

Thanks for your input.

New version attached, as described.

Other review comments and alternate opinions welcome.

I applied and did some basic testing on the patch, it works as described.

I would like to bring up a few points that I came across while looking into
the vacuum code.

1. As a result of this change to allow VACUUM inside a user transaction, I
think there is some chance of causing
a block/delay of concurrent VACUUMs if a VACUUM is being run under a long
running transaction.
2. Also, if a user runs VACUUM in a transaction, performance optimizations
like PROC_IN_VACUUM won't work.
3. Also, if VACUUM happens towards the end of a long running transaction,
the snapshot will be old
and xmin horizon for vacuum would be somewhat old as compared to current
lazy vacuum which
acquires a new snapshot just before scanning the table.

So, while I understand the need of the feature, I am wondering if there
should be some mention
of above caveats in documentation with the recommendation that VACUUM
should be run outside
a transaction, in general.

Thank you,
Rahila Syed

Hi Simon,

On Fri, Nov 4, 2022 at 10:15 AM Rahila Syed <rahilasyed90@gmail.com> wrote:

Hi Simon,

On Thu, Nov 3, 2022 at 3:53 PM Simon Riggs <simon.riggs@enterprisedb.com>
wrote:

On Tue, 1 Nov 2022 at 23:56, Simon Riggs <simon.riggs@enterprisedb.com>
wrote:

I haven't checked the rest of the patch, but +1 for allowing VACUUM

FULL

within a user txn.

My intention was to prevent that. I am certainly quite uneasy about
changing anything related to CLUSTER/VF, since they are old, complex
and bug prone.

So for now, I will block VF, as was my original intent.

I will be guided by what others think... so you may yet get your wish.

Maybe the error message needs to be qualified "...when multiple
relations are specified".

ERROR: VACUUM cannot run inside a transaction block

Hmm, that is standard wording based on the statement type, but I can
set a CONTEXT message also. Will update accordingly.

Thanks for your input.

New version attached, as described.

Other review comments and alternate opinions welcome.

I applied and did some basic testing on the patch, it works as described.

I would like to bring up a few points that I came across while looking
into the vacuum code.

1. As a result of this change to allow VACUUM inside a user transaction,
I think there is some chance of causing
a block/delay of concurrent VACUUMs if a VACUUM is being run under a long
running transaction.
2. Also, if a user runs VACUUM in a transaction, performance optimizations
like PROC_IN_VACUUM won't work.
3. Also, if VACUUM happens towards the end of a long running transaction,
the snapshot will be old
and xmin horizon for vacuum would be somewhat old as compared to current
lazy vacuum which
acquires a new snapshot just before scanning the table.

So, while I understand the need of the feature, I am wondering if there
should be some mention
of above caveats in documentation with the recommendation that VACUUM
should be run outside
a transaction, in general.

Sorry, I just noticed that you have already mentioned some of these in the
documentation as follows, so it seems
it is already taken care of.

+    <command>VACUUM</command> cannot be executed inside a transaction
block,
+    unless a single table is specified and <literal>FULL</literal> is not
+    specified.  When executing inside a transaction block the vacuum scan
can
+    hold back the xmin horizon and does not update the database
datfrozenxid,
+    as a result this usage is not useful for database maintenance, but is
provided
+    to allow vacuuming in special circumstances, such as temporary or
private
+    work tables.

Thank you,
Rahila Syed

Hi Rahila,

Thanks for your review.

On Fri, 4 Nov 2022 at 07:37, Rahila Syed <rahilasyed90@gmail.com> wrote:

I would like to bring up a few points that I came across while looking into the vacuum code.

1. As a result of this change to allow VACUUM inside a user transaction, I think there is some chance of causing
a block/delay of concurrent VACUUMs if a VACUUM is being run under a long running transaction.
2. Also, if a user runs VACUUM in a transaction, performance optimizations like PROC_IN_VACUUM won't work.
3. Also, if VACUUM happens towards the end of a long running transaction, the snapshot will be old
and xmin horizon for vacuum would be somewhat old as compared to current lazy vacuum which
acquires a new snapshot just before scanning the table.

So, while I understand the need of the feature, I am wondering if there should be some mention
of above caveats in documentation with the recommendation that VACUUM should be run outside
a transaction, in general.

Sorry, I just noticed that you have already mentioned some of these in the documentation as follows, so it seems
it is already taken care of.

+    <command>VACUUM</command> cannot be executed inside a transaction block,
+    unless a single table is specified and <literal>FULL</literal> is not
+    specified.  When executing inside a transaction block the vacuum scan can
+    hold back the xmin horizon and does not update the database datfrozenxid,
+    as a result this usage is not useful for database maintenance, but is provided
+    to allow vacuuming in special circumstances, such as temporary or private
+    work tables.

Yes, I wondered whether we should have a NOTICE or WARNING to remind
people of those points?

--
Simon Riggs http://www.EnterpriseDB.com/

Hi,

On Fri, Nov 4, 2022 at 2:39 PM Simon Riggs <simon.riggs@enterprisedb.com>
wrote:

Hi Rahila,

Thanks for your review.

On Fri, 4 Nov 2022 at 07:37, Rahila Syed <rahilasyed90@gmail.com> wrote:

I would like to bring up a few points that I came across while looking

into the vacuum code.

1. As a result of this change to allow VACUUM inside a user

transaction, I think there is some chance of causing

a block/delay of concurrent VACUUMs if a VACUUM is being run under a

long running transaction.

2. Also, if a user runs VACUUM in a transaction, performance

optimizations like PROC_IN_VACUUM won't work.

3. Also, if VACUUM happens towards the end of a long running

transaction, the snapshot will be old

and xmin horizon for vacuum would be somewhat old as compared to

current lazy vacuum which

acquires a new snapshot just before scanning the table.

So, while I understand the need of the feature, I am wondering if there

should be some mention

of above caveats in documentation with the recommendation that VACUUM

should be run outside

a transaction, in general.

Sorry, I just noticed that you have already mentioned some of these in

the documentation as follows, so it seems

it is already taken care of.

+ <command>VACUUM</command> cannot be executed inside a transaction

block,

+ unless a single table is specified and <literal>FULL</literal> is

not

+ specified. When executing inside a transaction block the vacuum

scan can

+ hold back the xmin horizon and does not update the database

datfrozenxid,

+ as a result this usage is not useful for database maintenance, but

is provided

+ to allow vacuuming in special circumstances, such as temporary or

private

+ work tables.

Yes, I wondered whether we should have a NOTICE or WARNING to remind
people of those points?

+1 . My vote for NOTICE over WARNING because I think
it is useful information for the user rather than any potential problem.

Thank you,
Rahila Syed

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

Peter Geoghegan <pg@bowt.ie> writes:

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.

Yeah. To be blunt, this proposal scares the sh*t out of me. I agree
that there are tons of subtle dependencies on our current assumptions
about how VACUUM works, and I strongly suspect that we won't find all of
them until after users have lost data. I cannot believe that running
VACUUM inside transactions is valuable enough to take that risk ...
especially if it's a modified limited kind of VACUUM that doesn't
eliminate the need for periodic real VACUUMs.

In general, I do not believe in encouraging users to run VACUUM
manually in the first place. We would be far better served by
spending our effort to improve autovacuum's shortcomings. I'd
like to see some sort of direct attack on its inability to deal
with temp tables, for instance. (Force the owning backend to
do it? Temporarily change the access rules so that the data
moves to shared buffers? Dunno, but we sure haven't tried hard.)
However many bugs such a thing might have initially, at least
they'd only put temporary data at hazard.

regards, tom lane

On Sun, Nov 6, 2022 at 11:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

In general, I do not believe in encouraging users to run VACUUM
manually in the first place. We would be far better served by
spending our effort to improve autovacuum's shortcomings.

I couldn't agree more. A lot of problems seem related to the idea that
VACUUM is just a command that the DBA periodically runs to get a
predictable fixed result, a little like CREATE INDEX. That conceptual
model isn't exactly wrong; it just makes it much harder to apply any
kind of context about the needs of the table over time. There is a
natural cycle to how VACUUM (really autovacuum) is run, and the
details matter.

There is a significant amount of relevant context that we can't really
use right now. That wouldn't be true if VACUUM only ran within an
autovacuum worker (by definition). The VACUUM command itself would
still be available, and support the same user interface, more or less.
Under the hood the VACUUM command would work by enqueueing a VACUUM
job, to be performed asynchronously by an autovacuum worker. Perhaps
the initial enqueue operation could be transactional, fixing Simon's complaint.

"No more VACUUMs outside of autovacuum" would enable more advanced
autovacuum.c scheduling, allowing us to apply a lot more context about
the costs and benefits, without having to treat manual VACUUM as an
independent thing. We could coalesce together redundant VACUUM jobs,
suspend and resume VACUUM operations, and have more strategies to deal
with problems as they emerge.

I'd like to see some sort of direct attack on its inability to deal
with temp tables, for instance. (Force the owning backend to
do it? Temporarily change the access rules so that the data
moves to shared buffers? Dunno, but we sure haven't tried hard.)

This is a good example of the kind of thing I have in mind. Perhaps it
could work by killing the backend that owns the temp relation when
things truly get out of hand? I think that that would be a perfectly
reasonable trade-off.

Another related idea: better behavior in the event of a manually
issued VACUUM (now just an enqueued autovacuum) that cannot do useful
work due to the presence of a long running snapshot. The VACUUM
doesn't have to dutifully report "success" when there is no practical
sense in which it was successful. There could be a back and forth
conversation between autovacuum.c and vacuumlazy.c that makes sure
that something useful happens sooner or later. The passage of time
really matters here.

As a bonus, we might be able to get rid of the autovacuum GUC
variants. Plus the current autovacuum logging would just be how we'd
log every VACUUM.

--
Peter Geoghegan

On Sun, 6 Nov 2022 at 18:50, Peter Geoghegan <pg@bowt.ie> wrote:

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

Usability is a major concern that doesn't get a high enough priority.

I'm pretty sure that there
are several deep, subtle reasons why VACUUM cannot be assumed safe to
run in a user transaction.

I expected there were, so it's good to discuss them. Thanks for the input.

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.

I'll answer that part in my reply to Tom, since there are good ideas in both.

--
Simon Riggs http://www.EnterpriseDB.com/

On Sun, 6 Nov 2022 at 20:40, Peter Geoghegan <pg@bowt.ie> wrote:

On Sun, Nov 6, 2022 at 11:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

In general, I do not believe in encouraging users to run VACUUM
manually in the first place. We would be far better served by
spending our effort to improve autovacuum's shortcomings.

I couldn't agree more. A lot of problems seem related to the idea that
VACUUM is just a command that the DBA periodically runs to get a
predictable fixed result, a little like CREATE INDEX. That conceptual
model isn't exactly wrong; it just makes it much harder to apply any
kind of context about the needs of the table over time. There is a
natural cycle to how VACUUM (really autovacuum) is run, and the
details matter.

There is a significant amount of relevant context that we can't really
use right now. That wouldn't be true if VACUUM only ran within an
autovacuum worker (by definition). The VACUUM command itself would
still be available, and support the same user interface, more or less.
Under the hood the VACUUM command would work by enqueueing a VACUUM
job, to be performed asynchronously by an autovacuum worker. Perhaps
the initial enqueue operation could be transactional, fixing Simon's complaint.

Ah, I see you got to this idea first!

Yes, what we need is for the "VACUUM command" to not fail in a script.
Not sure anyone cares where the work takes place.

Enqueuing a request for autovacuum to do that work, then blocking
until it is complete would do the job.

"No more VACUUMs outside of autovacuum" would enable more advanced
autovacuum.c scheduling, allowing us to apply a lot more context about
the costs and benefits, without having to treat manual VACUUM as an
independent thing. We could coalesce together redundant VACUUM jobs,
suspend and resume VACUUM operations, and have more strategies to deal
with problems as they emerge.

+1, but clearly this would not make temp table VACUUMs work.

I'd like to see some sort of direct attack on its inability to deal
with temp tables, for instance. (Force the owning backend to
do it? Temporarily change the access rules so that the data
moves to shared buffers? Dunno, but we sure haven't tried hard.)

This was a $DIRECT attack on making temp tables work! ;-)

Temp tables are actually easier, since we don't need any of the
concurrency features we get with lazy vacuum. So the answer is to
always run a VACUUM FULL on temp tables since this skips any issues
with indexes etc..

We would need to check a few things first.... maybe something like
this (mostly borrowed heavily from COPY)

InvalidateCatalogSnapshot();
if (!ThereAreNoPriorRegisteredSnapshots() || !ThereAreNoReadyPortals())
ereport(WARNING,
(errcode(ERRCODE_INVALID_TRANSACTION_STATE),
errmsg("vacuum of temporary table ignored because
of prior transaction activity")));
CheckTableNotInUse(rel, "VACUUM");

This is a good example of the kind of thing I have in mind. Perhaps it
could work by killing the backend that owns the temp relation when
things truly get out of hand? I think that that would be a perfectly
reasonable trade-off.

+1

Another related idea: better behavior in the event of a manually
issued VACUUM (now just an enqueued autovacuum) that cannot do useful
work due to the presence of a long running snapshot. The VACUUM
doesn't have to dutifully report "success" when there is no practical
sense in which it was successful. There could be a back and forth
conversation between autovacuum.c and vacuumlazy.c that makes sure
that something useful happens sooner or later. The passage of time
really matters here.

Regrettably, neither vacuum nor autovacuum waits for xmin to change;
perhaps it should.

--
Simon Riggs http://www.EnterpriseDB.com/

On Mon, Nov 7, 2022 at 12:20 AM Simon Riggs
<simon.riggs@enterprisedb.com> wrote:

Another related idea: better behavior in the event of a manually
issued VACUUM (now just an enqueued autovacuum) that cannot do useful
work due to the presence of a long running snapshot. The VACUUM
doesn't have to dutifully report "success" when there is no practical
sense in which it was successful. There could be a back and forth
conversation between autovacuum.c and vacuumlazy.c that makes sure
that something useful happens sooner or later. The passage of time
really matters here.

Regrettably, neither vacuum nor autovacuum waits for xmin to change;
perhaps it should.

Yes, it's very primitive right now. In fact I recently discovered that
just using the reloption version (not the GUC version) of
autovacuum_freeze_max_age in a totally straightforward way is all it
takes to utterly confuse autovacuum.c:

/messages/by-id/CAH2-Wz=DJAokY_GhKJchgpa8k9t_H_OVOvfPEn97jGNr9W=deg@mail.gmail.com

It's easy to convince autovacuum.c to launch antiwraparound
autovacuums that reliably have no chance of advancing relfrozenxid to
a degree that satisfies autovacuum.c. It will launch antiwraparound
autovacuums again and again, never realizing that VACUUM doesn't
really care about what it expects (at least not with the reloption in
use). Clearly that's just broken. It also suggests a more general
design problem, at least in my mind.

--
Peter Geoghegan

On Mon, 7 Nov 2022 at 08:20, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

Temp tables are actually easier, since we don't need any of the
concurrency features we get with lazy vacuum. So the answer is to
always run a VACUUM FULL on temp tables since this skips any issues
with indexes etc..

So I see 3 options for what to do next

1. Force the FULL option for all tables, when executed in a
transaction block. This gets round the reasonable objections to
running a concurrent vacuum in a shared xact block. As Justin points
out, CLUSTER is already supported, which uses the same code.

2. Force the FULL option for temp tables, when executed in a
transaction block. In a later patch, queue up an autovacuum run for
regular tables.

3. Return with feedback this patch. (But then what happens with temp tables?)

Thoughts?

--
Simon Riggs http://www.EnterpriseDB.com/

On Tue, 8 Nov 2022 at 03:10, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

On Mon, 7 Nov 2022 at 08:20, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

Temp tables are actually easier, since we don't need any of the
concurrency features we get with lazy vacuum.

Thoughts?

New patch, which does this, when in a xact block

1. For temp tables, only VACUUM FULL is allowed
2. For persistent tables, an AV task is created to perform the vacuum,
which eventually performs a vacuum

The patch works, but there are various aspects of the design that need
input. Thoughts?

--
Simon Riggs http://www.EnterpriseDB.com/

On Mon, 14 Nov 2022 at 19:52, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

On Tue, 8 Nov 2022 at 03:10, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

On Mon, 7 Nov 2022 at 08:20, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

Temp tables are actually easier, since we don't need any of the
concurrency features we get with lazy vacuum.

Thoughts?

New patch, which does this, when in a xact block

1. For temp tables, only VACUUM FULL is allowed
2. For persistent tables, an AV task is created to perform the vacuum,
which eventually performs a vacuum

The patch works, but there are various aspects of the design that need
input. Thoughts?

New version.

--
Simon Riggs http://www.EnterpriseDB.com/

I think the idea of being able to request an autovacuum worker for a
specific table is actually very good. I think it's what most users
actually want when they are running vacuum. In fact in previous jobs
people have built infrastructure that basically duplicates autovacuum
just so they could do this.

However I'm not a fan of commands that sometimes do one thing and
sometimes magically do something very different. I don't like the idea
that the same vacuum command would sometimes run in-process and
sometimes do this out of process request. And the rules for when it
does which are fairly complex to explain -- it runs in process unless
you're in a transaction when it runs out of process unless it's a
temporary table ...

I think this requesting autovacuum worker should be a distinct
command. Or at least an explicit option to vacuum.

Also, I was confused reading the thread above about mention of VACUUM
FULL. I don't understand why it's relevant at all. We certainly can't
force VACUUM FULL when it wasn't requested on potentially large
tables.