Re: why there is not VACUUM FULL CONCURRENTLY?

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Antonin Houska <ah(at)cybertec(dot)at>
Cc: Junwang Zhao <zhjwpku(at)gmail(dot)com>, Kirill Reshke <reshkekirill(at)gmail(dot)com>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: why there is not VACUUM FULL CONCURRENTLY?
Date: 2025-02-02 13:21:33
Message-ID: 202502021321.6ul3axwpsklw@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


> From bf2ec8c5d753de340140839f1b061044ec4c1149 Mon Sep 17 00:00:00 2001
> From: Antonin Houska <ah(at)cybertec(dot)at>
> Date: Mon, 13 Jan 2025 14:29:54 +0100
> Subject: [PATCH 4/8] Add CONCURRENTLY option to both VACUUM FULL and CLUSTER
> commands.

> @@ -950,8 +1412,46 @@ copy_table_data(Relation NewHeap, Relation OldHeap, Relation OldIndex, bool verb

> + if (concurrent)
> + {
> + PgBackendProgress progress;
> +
> + /*
> + * Command progress reporting gets terminated at subtransaction
> + * end. Save the status so it can be eventually restored.
> + */
> + memcpy(&progress, &MyBEEntry->st_progress,
> + sizeof(PgBackendProgress));
> +
> + /* Release the locks by aborting the subtransaction. */
> + RollbackAndReleaseCurrentSubTransaction();
> +
> + /* Restore the progress reporting status. */
> + pgstat_progress_restore_state(&progress);
> +
> + CurrentResourceOwner = oldowner;
> + }

I was looking at 0002 to see if it'd make sense to commit it ahead of a
fuller review of the rest, and I find that the reason for that patch is
this hunk you have here in copy_table_data -- you want to avoid a
subtransaction abort (which you use to release planner lock) clobbering
the status. I think this a bad idea. It might be better to handle this
in a different way, for instance

1) maybe have a flag that says "do not reset progress status during
subtransaction abort"; REPACK would set that flag, so it'd be able to
continue its business without having to memcpy the current status (which
seems like quite a hack) or restoring it afterwards.

2) maybe subtransaction abort is not the best way to release the
planning locks anyway. I think it might be better to have a
ResourceOwner that owns those locks, and we do ResourceOwnerRelease()
which would release them. I think this would be a novel usage of
ResourceOwner so it needs more research. But if this works, then we
don't need the subtransaction at all, and therefore we don't need
backend progress restore at all either.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Álvaro Herrera 2025-02-02 13:24:29 Re: Proposal to CREATE FOREIGN TABLE LIKE
Previous Message Dmitry Koterov 2025-02-02 11:43:27 Increased work_mem for "logical replication tablesync worker" only?