Re: [PATCH] Support for basic ALTER TABLE progress reporting.

Hi, thank you for the review!

On Fri, Jun 6, 2025 at 10:37 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:

> On Mon, Jun 2, 2025 at 3:35 PM Jiří Kavalík <jkavalik(at)gmail(dot)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