Обсуждение: [PATCH] Support for basic ALTER TABLE progress reporting.

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.
within ATRewriteTable we have:
pgstat_progress_update_param(PROGRESS_CLUSTER_TOTAL_HEAP_BLKS,
RelationGetNumberOfBlocks(oldrel));
        pgstat_progress_update_param(PROGRESS_CLUSTER_TOTAL_HEAP_BLKS,
                                             heapScan->rs_nblocks);

PROGRESS_CLUSTER_TOTAL_HEAP_BLKS
value is fixed, we only need to call pgstat_progress_update_param once here?


another patch [1] is expected to refactor pg_stat_progress_cluster a lot, so I'm
unsure whether it's a good idea to put CLUSTER, VACUUM FULL, or ALTER TABLE into
pg_stat_progress_cluster.
alternatively, we could introduce a separate progress report specifically for
ALTER TABLE, allowing us to distinguish between table rewrite and table
scan.

[1] https://commitfest.postgresql.org/patch/5117

On 2025-Jul-21, Jiří Kavalík wrote:

> I noticed that but not sure if it is targeting v19?

It is -- I'm working on that patch.

> I hoped to make the change as small as possible, but if it would
> collide with the refactoring then it makes sense to separate the
> functionality.

I think from the users' point of view it makes little sense to report
the progress of ALTER TABLE rewriting in the same view as CLUSTER.  This
is irrespective of the implementation difficulty.  I mean, as a user
trying to monitor an operation, why would I be querying
pg_stat_progress_cluster?  That's just weird.  On the other hand, ALTER
TABLE has a need to report whether it's rewriting the table or just
reading it to recheck or validate constraints.  If you try to cram that
bit of state in the CLUSTER report, it's going to look strange, because
it doesn't make sense for CLUSTER.  I think the distinction is rather
important: for example if you're just checking constraints, you don't
need to rebuild indexes.  This is information that the user would be
really happy to know.

In any case, having a separate progress.h definition isn't really all
that much extra work: it's just one more view and a few #defines to give
symbolic names to each counter.

Another point that might be worth keeping in mind is ALTER TABLE's
recursion mechanism for inheritance and partitioning.  As a user I
imagine I would like to know how far we are not just in processing the
current relation, but also how many children are done and how many are
still left.

Also worth keeping in mind: some AT subcommands use the "standard"
recursion mechanism that simply adds extra subcommands to the queue for
each child relation, whereas other AT subcommands implement recursion at
execution time.  Those work very differently.

What about multi-command ALTER TABLE?  I think this is easy because we
preprocess the command to collect a list of actions to execute, and then
rnu a single check/rewrite scan on each table.  So you may not need to
do anything special.  But it's worth thinking about.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"People get annoyed when you try to debug them."  (Larry Wall)