Thread: [PATCH] Support for basic ALTER TABLE progress reporting.
Hi,
while testing int->bigint migrations for our db at work I really missed ALTER TABLE progress reporting and during the waits I checked the code.
It seems to me that ALTERs can be mostly categorized as
1) trivial ones - metadata rewrites, fast adding/removing columns, trying to change column type to one already present etc. not much to report here
2) scanning ones - adding constraints - it imho gives enough info to report blocks total and scanned and tuples scanned
3) rewrites - actually changing data or types - add number of written blocks/tuples
3b) index rewrites - report number of indexes processed
From that it seems to me that the basic info is very similar to already present CLUSTER/VACUUM-FULL reporting so I tried to tap into that and just add a support for a new command.
I identified a handful of places where to add the reporting for ALTERs and it seems to work,
What I changed:
`commands/progress.h`
- new cluster reporting command + new phase for FK checks
`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
`catalog/system_views.sql`
- output for the new command and phase
`catalog/storage.c`
- number of blocks processed in `RelationCopyStorage()` for the case table is moved between tablespaces by direct copying
+ some basic documentation updates
What I did not have to change - index rebuilds used by CLUSTER reported their progress already, it just was not shown without a valid command configured.
I ran some manual tests locally + ran regression tests and it seems to work fine.
The reporting may be a bit crude and may be missing some phases but it covers the IO-heavy operations with some reasonable numbers. (well, not the FK check by left anti-join, but I don't want to mess with that + maybe number of FKs checked might be shown?)
Thanks
Best regards
jkavalik
Attachment
On Mon, Jun 2, 2025 at 3:35 PM Jiří Kavalík <jkavalik@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);
+
Later we may do constraint checks in ``foreach(l, tab->constraints)``.
putting it after table_tuple_insert would be more appropriate, IMHO.
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.
Hi, thank you for the review!
On Fri, Jun 6, 2025 at 10:37 AM jian he <jian.universality@gmail.com> wrote:
On Mon, Jun 2, 2025 at 3:35 PM Jiří Kavalík <jkavalik@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