Re: why there is not VACUUM FULL CONCURRENTLY?

From: Antonin Houska <ah(at)cybertec(dot)at>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
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-03 13:07:55
Message-ID: 20612.1738588075@antos
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Antonin Houska <ah(at)cybertec(dot)at> wrote:

> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> >
> >
> > > 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.
>
> If this needs change, I prefer 2) because it's less invasive: 1) still affects
> the progress monitoring code. I'll look at it.

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

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

My preference would still be to save and restore the progress state in this
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 != NULL && OldIndex->rd_rel->relam == BTREE_AM_OID)
+ {
+ ResourceOwner oldowner = NULL;
+ ResourceOwner resowner = 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 = ResourceOwnerCreate(CurrentResourceOwner,
+ "plan_cluster_use_sort");
+ oldowner = CurrentResourceOwner;
+ CurrentResourceOwner = resowner;
+ }
+
use_sort = plan_cluster_use_sort(RelationGetRelid(OldHeap),
RelationGetRelid(OldIndex));
+
+ if (concurrent)
+ {
+ CurrentResourceOwner = 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 = false;

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message torikoshia 2025-02-03 13:35:20 Change log level for notifying hot standby is waiting non-overflowed snapshot
Previous Message Shlok Kyal 2025-02-03 13:05:43 Re: Introduce XID age and inactive timeout based replication slot invalidation