Re: why there is not VACUUM FULL CONCURRENTLY?

On 2024-Oct-09, Antonin Houska wrote:

> diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
> index 9110938fab..f1008f5013 100644
> --- a/doc/src/sgml/ref/vacuum.sgml
> +++ b/doc/src/sgml/ref/vacuum.sgml

> @@ -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?  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.  Heck, maybe
don't even accept a partitioned table -- the user can process one
partition at a time, if they need that.


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

In 0001, the new API of make_new_heap() is somewhat bizarre regarding
the output lockmode_new_p parameter.  I didn't find any place in the
patch series where we use that to return a different lock level that the
caller gave; the only case were we do something that looks funny is when
a toast table is involved.  But I don't think I fully understand what is
going on in that case.  I'm likely missing something here, but isn't it
simpler to just state that make_new_heap will obtain a lock on the new
heap, and that the immediately following table_open needn't acquire a
lock (or, in the case of RefreshMatViewByOid, no LockRelationOid is
necessary)?

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.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/