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

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

> =

> =

> > 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 C=
LUSTER
> >  commands.
> =

> > @@ -950,8 +1412,46 @@ copy_table_data(Relation NewHeap, Relation OldHe=
ap, 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 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 aff=
ects
the progress monitoring code. I'll look at it.

-- =

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