Hi, thank you for the review!
On Fri, Jun 6, 2025 at 10:37 AM jian he <jian.universal...@gmail.com> wrote:
> On Mon, Jun 2, 2025 at 3:35 PM Jiří Kavalík <jkava...@gmail.com> wrote:
> > What I changed:
> >
> > `commands/tablecmds.c`
> > - start and end reporting inside `ATRewriteTables()`
> > - report blocks total, blocks and tuples scanned and possibly tuples
> written in `ATRewriteTable`
> > - add at least phase info in `validateForeignKeyConstraint`, possibly
> more if the check cannot be done by left join
> >
>
> hi.
> similar to DoCopyTo, processed as uint64,
> in ATRewriteTable, numTuples should be also uint64 not int?
> + pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_TUPLES_WRITTEN,
> + numTuples);
> +
>
Yes, that makes sense, updated patch attached
> Later we may do constraint checks in ``foreach(l, tab->constraints)``.
> putting it after table_tuple_insert would be more appropriate, IMHO.
As I understand it, if we get an error from any of the constraints, the
ALTER fails anyway so there seems to be no window for mismatch. But it
might make sense to move it into the same block where `table_tuple_insert`
is anyway.
>
> static void
> ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE
> lockmode,
> AlterTableUtilityContext *context)
> {
> ListCell *ltab;
> /* Go through each table that needs to be checked or rewritten */
> foreach(ltab, *wqueue)
> {
> AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab);
> /* Relations without storage may be ignored here */
> if (!RELKIND_HAS_STORAGE(tab->relkind))
> continue;
> /* Start progress reporting */
> pgstat_progress_start_command(PROGRESS_COMMAND_CLUSTER,
> tab->relid);
> pgstat_progress_update_param(PROGRESS_CLUSTER_COMMAND,
> PROGRESS_CLUSTER_COMMAND_ALTER_TABLE);
> }
>
> Perhaps this is a bit early—we haven't completed the error checks yet.
> we have several "ereport(ERROR..." in below.
> maybe place it before ATRewriteTable, IMHO.
>
There are three different branches under it, two of them containing
`ATRewriteTable()` calls. So I picked a place before that branching instead
of starting in two separate places. It seems simpler but thats my only
argument so I am open to other opinions (for both this and where to put the
"tuples_written" update).
Best regards
jkavalik
From e5a60149438308d4d5e107f9cc36d5d2f659188d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ji=C5=99=C3=AD=20Kaval=C3=ADk?= <jkava...@gmail.com>
Date: Sun, 25 May 2025 23:23:56 +0200
Subject: [PATCH] ALTER TABLE progress support
---
doc/src/sgml/monitoring.sgml | 13 +++----
doc/src/sgml/ref/alter_table.sgml | 10 ++++++
src/backend/catalog/storage.c | 7 ++++
src/backend/catalog/system_views.sql | 2 ++
src/backend/commands/tablecmds.c | 54 ++++++++++++++++++++++++++++
src/include/commands/progress.h | 2 ++
src/test/regress/expected/rules.out | 2 ++
7 files changed, 84 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 4265a22d4de..09307c5f490 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -400,7 +400,7 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34
0:00 postgres: ser
<row>
<entry><structname>pg_stat_progress_cluster</structname><indexterm><primary>pg_stat_progress_cluster</primary></indexterm></entry>
<entry>One row for each backend running
- <command>CLUSTER</command> or <command>VACUUM FULL</command>, showing
current progress.
+ <command>CLUSTER</command>, <command>VACUUM FULL</command> or
<command>ALTER TABLE</command>, showing current progress.
See <xref linkend="cluster-progress-reporting"/>.
</entry>
</row>
@@ -5492,7 +5492,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
<productname>PostgreSQL</productname> has the ability to report the
progress of
certain commands during command execution. Currently, the only commands
which support progress reporting are <command>ANALYZE</command>,
- <command>CLUSTER</command>,
+ <command>CLUSTER</command>, <command>ALTER TABLE</command>,
<command>CREATE INDEX</command>, <command>VACUUM</command>,
<command>COPY</command>,
and <xref linkend="protocol-replication-base-backup"/> (i.e., replication
@@ -5738,8 +5738,9 @@ FROM pg_stat_get_backend_idset() AS backendid;
</indexterm>
<para>
- Whenever <command>CLUSTER</command> or <command>VACUUM FULL</command> is
- running, the <structname>pg_stat_progress_cluster</structname> view will
+ Whenever <command>CLUSTER</command>, <command>VACUUM FULL</command>
+ or <command>ALTER TABLE</command> is running,
+ the <structname>pg_stat_progress_cluster</structname> view will
contain a row for each backend that is currently running either command.
The tables below describe the information that will be reported and
provide information about how to interpret it.
@@ -5801,7 +5802,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
<structfield>command</structfield> <type>text</type>
</para>
<para>
- The command that is running. Either <literal>CLUSTER</literal> or
<literal>VACUUM FULL</literal>.
+ The command that is running. Either <literal>CLUSTER</literal>,
<literal>VACUUM FULL</literal> or <literal>ALTER TABLE</literal>.
</para></entry>
</row>
@@ -5884,7 +5885,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
</table>
<table id="cluster-phases">
- <title>CLUSTER and VACUUM FULL Phases</title>
+ <title>CLUSTER, VACUUM FULL and ALTER TABLE Phases</title>
<tgroup cols="2">
<colspec colname="col1" colwidth="1*"/>
<colspec colname="col2" colwidth="2*"/>
diff --git a/doc/src/sgml/ref/alter_table.sgml
b/doc/src/sgml/ref/alter_table.sgml
index d63f3a621ac..228f27ac5fc 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1859,6 +1859,16 @@ ALTER TABLE measurement
</para>
</refsect1>
+ <refsect1>
+ <title>Progress Reporting</title>
+ <para>
+ When an <command>ALTER TABLE</command> operation rewrites the table,
progress
+ can be monitored via the <literal>pg_stat_progress_cluster</literal>
system view,
+ similar to <command>CLUSTER</command> and <command>VACUUM FULL</command>
commands.
+ The command type will be reported as <literal>'ALTER TABLE'</literal>.
+ </para>
+ </refsect1>
+
<refsect1>
<title>See Also</title>
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 227df90f89c..79d14d86bc9 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -26,6 +26,7 @@
#include "access/xlogutils.h"
#include "catalog/storage.h"
#include "catalog/storage_xlog.h"
+#include "commands/progress.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "storage/bulk_write.h"
@@ -505,6 +506,9 @@ RelationCopyStorage(SMgrRelation src, SMgrRelation dst,
nblocks = smgrnblocks(src, forkNum);
+ /* Report expected number of block to copy */
+ pgstat_progress_update_param(PROGRESS_CLUSTER_TOTAL_HEAP_BLKS, nblocks);
+
for (blkno = 0; blkno < nblocks; blkno++)
{
BulkWriteBuffer buf;
@@ -556,6 +560,9 @@ RelationCopyStorage(SMgrRelation src, SMgrRelation dst,
* page including any unused space.
*/
smgr_bulk_write(bulkstate, blkno, buf, false);
+
+ /* Update progress report */
+
pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_BLKS_SCANNED, blkno + 1);
}
smgr_bulk_finish(bulkstate);
}
diff --git a/src/backend/catalog/system_views.sql
b/src/backend/catalog/system_views.sql
index 08f780a2e63..99a6ee0060f 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1252,6 +1252,7 @@ CREATE VIEW pg_stat_progress_cluster AS
S.relid AS relid,
CASE S.param1 WHEN 1 THEN 'CLUSTER'
WHEN 2 THEN 'VACUUM FULL'
+ WHEN 3 THEN 'ALTER TABLE'
END AS command,
CASE S.param2 WHEN 0 THEN 'initializing'
WHEN 1 THEN 'seq scanning heap'
@@ -1261,6 +1262,7 @@ CREATE VIEW pg_stat_progress_cluster AS
WHEN 5 THEN 'swapping relation files'
WHEN 6 THEN 'rebuilding index'
WHEN 7 THEN 'performing final cleanup'
+ WHEN 8 THEN 'checking foreign key constraints'
END AS phase,
CAST(S.param3 AS oid) AS cluster_index_relid,
S.param4 AS heap_tuples_scanned,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index acf11e83c04..67a3466295c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -59,6 +59,7 @@
#include "commands/comment.h"
#include "commands/defrem.h"
#include "commands/event_trigger.h"
+#include "commands/progress.h"
#include "commands/sequence.h"
#include "commands/tablecmds.h"
#include "commands/tablespace.h"
@@ -5839,6 +5840,10 @@ ATRewriteTables(AlterTableStmt *parsetree, List
**wqueue, LOCKMODE lockmode,
if (!RELKIND_HAS_STORAGE(tab->relkind))
continue;
+ /* Start progress reporting */
+ pgstat_progress_start_command(PROGRESS_COMMAND_CLUSTER,
tab->relid);
+ pgstat_progress_update_param(PROGRESS_CLUSTER_COMMAND,
PROGRESS_CLUSTER_COMMAND_ALTER_TABLE);
+
/*
* If we change column data types, the operation has to be
propagated
* to tables that use this table's rowtype as a column type.
@@ -5979,6 +5984,9 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue,
LOCKMODE lockmode,
*/
ATRewriteTable(tab, OIDNewHeap);
+ /* Report that we are now swapping relation files */
+ pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
+
PROGRESS_CLUSTER_PHASE_SWAP_REL_FILES);
/*
* Swap the physical files of the old and new heaps,
then rebuild
* indexes and discard the old heap. We can use
RecentXmin for
@@ -6090,6 +6098,10 @@ ATRewriteTables(AlterTableStmt *parsetree, List
**wqueue, LOCKMODE lockmode,
table_close(rel, NoLock);
}
+ /* Report that we are now doing clean up */
+ pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
+
PROGRESS_CLUSTER_PHASE_FINAL_CLEANUP);
+
/* Finally, run any afterStmts that were queued up */
foreach(ltab, *wqueue)
{
@@ -6104,6 +6116,8 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue,
LOCKMODE lockmode,
CommandCounterIncrement();
}
}
+
+ pgstat_progress_end_command();
}
/*
@@ -6129,6 +6143,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap)
BulkInsertState bistate;
int ti_options;
ExprState *partqualstate = NULL;
+ int64 numTuples = 0;
/*
* Open the relation(s). We have surely already locked the existing
@@ -6138,6 +6153,10 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap)
oldTupDesc = tab->oldDesc;
newTupDesc = RelationGetDescr(oldrel); /* includes all mods */
+ /* Update progress reporting - we are actually scanning and possibly
rewriting the table */
+ pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
PROGRESS_CLUSTER_PHASE_SEQ_SCAN_HEAP);
+ pgstat_progress_update_param(PROGRESS_CLUSTER_TOTAL_HEAP_BLKS,
RelationGetNumberOfBlocks(oldrel));
+
if (OidIsValid(OIDNewHeap))
{
Assert(CheckRelationOidLockedByMe(OIDNewHeap,
AccessExclusiveLock,
@@ -6245,6 +6264,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap)
TupleTableSlot *oldslot;
TupleTableSlot *newslot;
TableScanDesc scan;
+ HeapScanDesc heapScan;
MemoryContext oldCxt;
List *dropped_attrs = NIL;
ListCell *lc;
@@ -6344,6 +6364,9 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap)
*/
snapshot = RegisterSnapshot(GetLatestSnapshot());
scan = table_beginscan(oldrel, snapshot, 0, NULL);
+ heapScan = (HeapScanDesc) scan;
+ pgstat_progress_update_param(PROGRESS_CLUSTER_TOTAL_HEAP_BLKS,
+
heapScan->rs_nblocks);
/*
* Switch to per-tuple memory context and reset it for each
tuple
@@ -6354,6 +6377,13 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap)
while (table_scan_getnextslot(scan, ForwardScanDirection,
oldslot))
{
TupleTableSlot *insertslot;
+
pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_BLKS_SCANNED,
+
(heapScan->rs_cblock +
+
heapScan->rs_nblocks -
+
heapScan->rs_startblock
+
) % heapScan->rs_nblocks + 1);
+
pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_TUPLES_SCANNED,
+
++numTuples);
if (tab->rewrite > 0)
{
@@ -6402,6 +6432,9 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap)
ExecStoreVirtualTuple(newslot);
+
pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_TUPLES_WRITTEN,
+
numTuples);
+
/*
* Now, evaluate any expressions whose inputs
come from the
* new tuple. We assume these columns won't
reference each
@@ -6522,6 +6555,8 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap)
CHECK_FOR_INTERRUPTS();
}
+ pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_BLKS_SCANNED,
+
heapScan->rs_nblocks);
MemoryContextSwitchTo(oldCxt);
table_endscan(scan);
@@ -13638,10 +13673,12 @@ validateForeignKeyConstraint(char *conname,
{
TupleTableSlot *slot;
TableScanDesc scan;
+ HeapScanDesc heapScan;
Trigger trig = {0};
Snapshot snapshot;
MemoryContext oldcxt;
MemoryContext perTupCxt;
+ int64 numTuples = 0;
ereport(DEBUG1,
(errmsg_internal("validating foreign key constraint
\"%s\"", conname)));
@@ -13660,6 +13697,10 @@ validateForeignKeyConstraint(char *conname,
trig.tginitdeferred = false;
/* we needn't fill in remaining fields */
+ /* Report that we are now checking foreign keys */
+ pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
+
PROGRESS_CLUSTER_PHASE_CHECK_FKEYS);
+
/*
* See if we can do it with a single LEFT JOIN query. A false result
* indicates we must proceed with the fire-the-trigger method. We can't
do
@@ -13677,6 +13718,7 @@ validateForeignKeyConstraint(char *conname,
snapshot = RegisterSnapshot(GetLatestSnapshot());
slot = table_slot_create(rel, NULL);
scan = table_beginscan(rel, snapshot, 0, NULL);
+ heapScan = (HeapScanDesc) scan;
perTupCxt = AllocSetContextCreate(CurrentMemoryContext,
"validateForeignKeyConstraint",
@@ -13712,6 +13754,14 @@ validateForeignKeyConstraint(char *conname,
RI_FKey_check_ins(fcinfo);
MemoryContextReset(perTupCxt);
+
+ pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_BLKS_SCANNED,
+
(heapScan->rs_cblock +
+
heapScan->rs_nblocks -
+
heapScan->rs_startblock
+
) % heapScan->rs_nblocks + 1);
+
pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_TUPLES_SCANNED,
+
++numTuples);
}
MemoryContextSwitchTo(oldcxt);
@@ -16776,6 +16826,10 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace,
LOCKMODE lockmode)
*/
rel = relation_open(tableOid, lockmode);
+ /* Update progress reporting - we are copying the table */
+ pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
PROGRESS_CLUSTER_PHASE_WRITE_NEW_HEAP);
+ pgstat_progress_update_param(PROGRESS_CLUSTER_TOTAL_HEAP_BLKS,
RelationGetNumberOfBlocks(rel));
+
/* Check first if relation can be moved to new tablespace */
if (!CheckRelationTableSpaceMove(rel, newTableSpace))
{
diff --git a/src/include/commands/progress.h b/src/include/commands/progress.h
index 7c736e7b03b..56585742838 100644
--- a/src/include/commands/progress.h
+++ b/src/include/commands/progress.h
@@ -74,10 +74,12 @@
#define PROGRESS_CLUSTER_PHASE_SWAP_REL_FILES 5
#define PROGRESS_CLUSTER_PHASE_REBUILD_INDEX 6
#define PROGRESS_CLUSTER_PHASE_FINAL_CLEANUP 7
+#define PROGRESS_CLUSTER_PHASE_CHECK_FKEYS 8
/* Commands of PROGRESS_CLUSTER */
#define PROGRESS_CLUSTER_COMMAND_CLUSTER 1
#define PROGRESS_CLUSTER_COMMAND_VACUUM_FULL 2
+#define PROGRESS_CLUSTER_COMMAND_ALTER_TABLE 3 /* New command type */
/* Progress parameters for CREATE INDEX */
/* 3, 4 and 5 reserved for "waitfor" metrics */
diff --git a/src/test/regress/expected/rules.out
b/src/test/regress/expected/rules.out
index 6cf828ca8d0..3b8a0ec0756 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1982,6 +1982,7 @@ pg_stat_progress_cluster| SELECT s.pid,
CASE s.param1
WHEN 1 THEN 'CLUSTER'::text
WHEN 2 THEN 'VACUUM FULL'::text
+ WHEN 3 THEN 'ALTER TABLE'::text
ELSE NULL::text
END AS command,
CASE s.param2
@@ -1993,6 +1994,7 @@ pg_stat_progress_cluster| SELECT s.pid,
WHEN 5 THEN 'swapping relation files'::text
WHEN 6 THEN 'rebuilding index'::text
WHEN 7 THEN 'performing final cleanup'::text
+ WHEN 8 THEN 'checking foreign key constraints'::text
ELSE NULL::text
END AS phase,
(s.param3)::oid AS cluster_index_relid,
--
2.34.1
