Feedback-ID: ia2694551:Fastmail
Date: Sun, 2 Feb 2025 14:21:33 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Antonin Houska <ah@cybertec.at>
Cc: Junwang Zhao <zhjwpku@gmail.com>,
	Kirill Reshke <reshkekirill@gmail.com>,
 Pavel Stehule <pavel.stehule@gmail.com>,
	Michael Paquier <michael@paquier.xyz>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: why there is not VACUUM FULL CONCURRENTLY?
Message-ID: <202502021321.6ul3axwpsklw@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <6250.1736776111@antos>
Archived-At: 
 <https://www.postgresql.org/message-id/202502021321.6ul3axwpsklw%40alvherre.pgsql>
Precedence: bulk


> From bf2ec8c5d753de340140839f1b061044ec4c1149 Mon Sep 17 00:00:00 2001
> From: Antonin Houska <ah@cybertec.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/