public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alvaro Herrera <[email protected]>
To: Antonin Houska <[email protected]>
Cc: Junwang Zhao <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: Pavel Stehule <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: why there is not VACUUM FULL CONCURRENTLY?
Date: Sun, 2 Feb 2025 14:21:33 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <6250.1736776111@antos>
> From bf2ec8c5d753de340140839f1b061044ec4c1149 Mon Sep 17 00:00:00 2001
> From: Antonin Houska <[email protected]>
> 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/
view thread (7+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: why there is not VACUUM FULL CONCURRENTLY?
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox