From: Antonin Houska <ah@cybertec.at>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
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?
In-reply-to: <2879.1738566105@antos>
References: <202502021321.6ul3axwpsklw@alvherre.pgsql> <2879.1738566105@antos>
Comments: In-reply-to Antonin Houska <ah@cybertec.at>
   message dated "Mon, 03 Feb 2025 08:01:45 +0100."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <20611.1738588075.1@antos>
Content-Transfer-Encoding: quoted-printable
Date: Mon, 03 Feb 2025 14:07:55 +0100
Message-ID: <20612.1738588075@antos>
Archived-At: <https://www.postgresql.org/message-id/20612.1738588075%40antos>
Precedence: bulk

Antonin Houska <ah@cybertec.at> wrote:

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

> > =

> > =

> > > From bf2ec8c5d753de340140839f1b061044ec4c1149 Mon Sep 17 00:00:00 20=
01
> > > 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 Old=
Heap, 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 =3D 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 i=
s
> > this hunk you have here in copy_table_data -- you want to avoid a
> > subtransaction abort (which you use to release planner lock) clobberin=
g
> > the status.  I think this a bad idea.  It might be better to handle th=
is
> > 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 (whi=
ch
> > 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.
> =

> If this needs change, I prefer 2) because it's less invasive: 1) still a=
ffects
> the progress monitoring code. I'll look at it.

Below is what I suggest now. It resembles the use of PortalData.resowner i=
n
the sense that it's a resource owner separate from the resource owner of t=
he
transaction.

Although it's better to use a resource owner than a subtransaction here, w=
e
still need to restore the progress state in
cluster_decode_concurrent_changes() (see v07-0004-) because a subtransacti=
on
aborts that clear it can take place during the decoding.

My preference would still be to save and restore the progress state in thi=
s
case (although a new function like pgstat_progress_save_state() would be
better than memcpy()). What do you think?


@@ -950,8 +1412,48 @@ copy_table_data(Relation NewHeap, Relation OldHeap, =
Relation OldIndex, bool verb
 	 * provided, else plain seqscan.
 	 */
 	if (OldIndex !=3D NULL && OldIndex->rd_rel->relam =3D=3D BTREE_AM_OID)
+	{
+		ResourceOwner	oldowner =3D NULL;
+		ResourceOwner	resowner =3D NULL;
+
+		/*
+		 * In the CONCURRENT case, use a dedicated resource owner so we don't
+		 * leave any additional locks behind us that we cannot release easily.
+		 */
+		if (concurrent)
+		{
+			Assert(CheckRelationLockedByMe(OldHeap, ShareUpdateExclusiveLock,
+										   false));
+			Assert(CheckRelationLockedByMe(OldIndex, ShareUpdateExclusiveLock,
+										   false));
+
+			resowner =3D ResourceOwnerCreate(CurrentResourceOwner,
+										   "plan_cluster_use_sort");
+			oldowner =3D CurrentResourceOwner;
+			CurrentResourceOwner =3D resowner;
+		}
+
 		use_sort =3D plan_cluster_use_sort(RelationGetRelid(OldHeap),
 										 RelationGetRelid(OldIndex));
+
+		if (concurrent)
+		{
+			CurrentResourceOwner =3D oldowner;
+
+			/*
+			 * We are primarily concerned about locks, but if the planner
+			 * happened to allocate any other resources, we should release
+			 * them too because we're going to delete the whole resowner.
+			 */
+			ResourceOwnerRelease(resowner, RESOURCE_RELEASE_BEFORE_LOCKS,
+								 false, false);
+			ResourceOwnerRelease(resowner, RESOURCE_RELEASE_LOCKS,
+								 false, false);
+			ResourceOwnerRelease(resowner, RESOURCE_RELEASE_AFTER_LOCKS,
+								 false, false);
+			ResourceOwnerDelete(resowner);
+		}
+	}
 	else
 		use_sort =3D false;
 =


-- =

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