Thread: Conflicting updates of command progress

While working on [1] I realized that some field of pg_stat_progress_cluste has
weird value. On closer look I found out that cluster_rel() indirectly calls
index_build() and that overwrites the progress parameter. Following are the
parameter values in the master branch:

/* Phases of cluster (as advertised via PROGRESS_CLUSTER_PHASE) */
#define PROGRESS_CLUSTER_PHASE_SEQ_SCAN_HEAP    1
#define PROGRESS_CLUSTER_PHASE_INDEX_SCAN_HEAP    2
#define PROGRESS_CLUSTER_PHASE_SORT_TUPLES        3
#define PROGRESS_CLUSTER_PHASE_WRITE_NEW_HEAP    4
#define PROGRESS_CLUSTER_PHASE_SWAP_REL_FILES    5
#define PROGRESS_CLUSTER_PHASE_REBUILD_INDEX    6
#define PROGRESS_CLUSTER_PHASE_FINAL_CLEANUP    7

/* Progress parameters for CREATE INDEX */
/* 3, 4 and 5 reserved for "waitfor" metrics */
#define PROGRESS_CREATEIDX_COMMAND                0
#define PROGRESS_CREATEIDX_INDEX_OID            6
#define PROGRESS_CREATEIDX_ACCESS_METHOD_OID    8
#define PROGRESS_CREATEIDX_PHASE                9    /* AM-agnostic phase # */
#define PROGRESS_CREATEIDX_SUBPHASE                10    /* phase # filled by AM */
#define PROGRESS_CREATEIDX_TUPLES_TOTAL            11
#define PROGRESS_CREATEIDX_TUPLES_DONE            12
#define PROGRESS_CREATEIDX_PARTITIONS_TOTAL        13
#define PROGRESS_CREATEIDX_PARTITIONS_DONE        14
/* 15 and 16 reserved for "block number" metrics */

The only conflicting parameters I see here are
PROGRESS_CLUSTER_PHASE_REBUILD_INDEX vs PROGRESS_CREATEIDX_INDEX_OID (number
6). Fortunately, index_build() does not set PROGRESS_CREATEIDX_INDEX_OID so
there's no live bug. However [1] adds some PROGRESS_CLUSTER_ parameters, thus
making conflicts realistic.

AFAICS the current design does not consider that one progress-reporting
command can be called by another one. Not sure what the correct fix is. We can
either ignore update requests from the "nested" commands, or display the
progress of both. The latter approach is probably more invasive - is that
worth the effort?

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

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

> While working on [1] I realized that some field of pg_stat_progress_cluste has
> weird value.

Is there a repro that you can share that shows the weird values? It sounds like
the repro is on top of [1]. Is that right?

> AFAICS the current design does not consider that one progress-reporting
> command can be called by another one.

pgstat_progress_start_command should only be called once by the entry
point for the
command. In theory, we could end up in a situation where start_command
is called multiple times during the same top-level command;

> Not sure what the correct fix is. We can
> either ignore update requests from the "nested" commands, or display the

There is a pattern where we do

```
if (progress)
      pgstat_progress_update_param
```

cluster_rel can pass down a flag to index_build or others that update progress
to not report progress. Therefore, only the top level command is
updating progress.
what do you think?


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

--
Sami Imseih
Amazon Web Services (AWS)

On 2025/04/15 2:13, Sami Imseih wrote:
>> While working on [1] I realized that some field of pg_stat_progress_cluste has
>> weird value.

I ran into the same issue while working on [2], and eventually had to
withdraw that patch because of it.


> Is there a repro that you can share that shows the weird values? It sounds like
> the repro is on top of [1]. Is that right?

You can reproduce the similar problem by creating a trigger function that
runs a progress-reporting command like COPY, and then COPY data into
a table that uses that trigger.

Regards,

[2] https://commitfest.postgresql.org/patch/5282/

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION