Re: why there is not VACUUM FULL CONCURRENTLY?

Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> On 2024-Oct-09, Antonin Houska wrote:
> 
> > @@ -61,8 +62,12 @@ VACUUM [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] [ <re
> >    <para>
> >     Without a <replaceable class="parameter">table_and_columns</replaceable>
> >     list, <command>VACUUM</command> processes every table and materialized view
> > -   in the current database that the current user has permission to vacuum.
> > -   With a list, <command>VACUUM</command> processes only those table(s).
> > +   in the current database that the current user has permission to vacuum. If
> > +   the <literal>CONCURRENTLY</literal> is specified (see below), tables which
> > +   have not been clustered yet are silently skipped. With a
> > +   list, <command>VACUUM</command> processes only those table(s). If
> > +   the <literal>CONCURRENTLY</literal> is specified, the list may only contain
> > +   tables which have already been clustered.
> >    </para>
> 
> The idea that VACUUM CONCURRENTLY can only process tables that have been
> clustered sounds very strange to me.  I don't think such a restriction
> would really fly.  However, I think this may just be a documentation
> mistake; can you please clarify?

Right, it was a documentation problem. I think the fact that VACUUM FULL is
internally (almost) an alias for CLUSTER is what distracted me.

> I am tempted to suggest that VACUUM CONCURRENTLY should receive a table
> list; without a list, it should raise an error.  This is not supposed to be
> a routine maintenance command that you can run on all your tables, after
> all.

ok, implemented

> Heck, maybe don't even accept a partitioned table -- the user can
> process one partition at a time, if they need that.

I also thought of this but forgot. Done now.

> I don't believe in the need for the LOCK_CLUSTER_CONCURRENT define; IMO
> the code should just use ShareUpdateExclusiveLock where needed.

ok, removed

> In 0001, the new API of make_new_heap() is somewhat bizarre regarding
> the output lockmode_new_p parameter.

Oh, it was too messy. I think I was thinking of too many things at once (such
as locking the old heap, the new heap and the new heap's TOAST). Also, one
thing that might have contributed to the confusion is that make_new_heap() has
the 'lockmode' argument, which receives various values from various
callers. However, both the new heap and its TOAST relation are eventually
created by heap_create_with_catalog(), and this function always leaves the new
relation locked in AccessExclusiveMode. Maybe this needs some refactoring.

Therefore I reverted the changes arount make_new_heap() and simply pass NoLock
for lockmode in cluster.c

> Anyway, I propose some cosmetic cleanups for 0001 in attachment,
> including changing make_new_heap to assume a non-null value of
> lockmode_new_p.  I didn't go as far as making it no longer a pointer,
> but if it can be done, then I suggest we should do that.  I didn't try
> to apply the next patches in the series after this one.

Thanks, applied (except the changes related to make_new_heap(), which is not
touched in the next version of the patch.)

Next version is attached. It also tries to fix CI problems reported on
machines with -DRELCACHE_FORCE_RELEASE.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com