Re: Improvements and additions to COPY progress reporting

On Sat, Feb 20, 2021 at 12:49 PM Michael Paquier <michael(at)paquier(dot)xyz>
wrote:
>
> On Sat, Feb 20, 2021 at 11:39:22AM +0530, Bharath Rupireddy wrote:
> > Actually in the code base the style of that variable declaration and
> > usage of pgstat_progress_update_multi_param is a mix. For instance, in
> > lazy_scan_heap, ReindexRelationConcurrently, the variables are
> > declared at the start of the function. And in _bt_spools_heapscan,
> > index_build, validate_index, perform_base_backup, the variables are
> > declared within a separate block.
>
> I think that we should encourage the use of
> pgstat_progress_update_multi_param() where we can, as it makes
> consistent the updates to all the parameters according to
> st_changecount. That's also usually cleaner to store all the
> parameters that are changed if these are updated multiple times like
> the REINDEX CONCURRENTLY ones. The context of the code also matters,
> of course.

Yeah. We could use pgstat_progress_update_multi_param instead of
pgstat_progress_update_param to update multiple params.

On a quick scan through the code, I found that we can do the following. If
okay, I can start a new thread so that we don't divert the main thread
here. Thoughts?

@@ -3686,12 +3686,18 @@ reindex_index(Oid indexId, bool
skip_constraint_checks, char persistence,
if (progress)
{
+ const int progress_cols[] = {
+ PROGRESS_CREATEIDX_COMMAND,
+ PROGRESS_CREATEIDX_INDEX_OID
+ };
+ const int64 progress_vals[] = {
+ PROGRESS_CREATEIDX_COMMAND_REINDEX,
+ indexId
+ };
+
pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
heapId);
- pgstat_progress_update_param(PROGRESS_CREATEIDX_COMMAND,
- PROGRESS_CREATEIDX_COMMAND_REINDEX);
- pgstat_progress_update_param(PROGRESS_CREATEIDX_INDEX_OID,
- indexId);
+ pgstat_progress_update_multi_param(2, progress_cols,
progress_vals);
}
@@ -1457,10 +1457,21 @@ DefineIndex(Oid relationId,
set_indexsafe_procflags();
/*
- * The index is now visible, so we can report the OID.
+ * The index is now visible, so we can report the OID. And also, report
+ * Phase 2 of concurrent index build.
*/
- pgstat_progress_update_param(PROGRESS_CREATEIDX_INDEX_OID,
- indexRelationId);
+ {
+ const int progress_cols[] = {
+ PROGRESS_CREATEIDX_INDEX_OID,
+ PROGRESS_CREATEIDX_PHASE
+ };
+ const int64 progress_vals[] = {
+ indexRelationId,
+ PROGRESS_CREATEIDX_PHASE_WAIT_1
+ };
+
+ pgstat_progress_update_multi_param(2, progress_cols,
progress_vals);
+ }
@@ -284,12 +284,9 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams
*params)
CHECK_FOR_INTERRUPTS();
pgstat_progress_start_command(PROGRESS_COMMAND_CLUSTER, tableOid);
- if (OidIsValid(indexOid))
- pgstat_progress_update_param(PROGRESS_CLUSTER_COMMAND,
- PROGRESS_CLUSTER_COMMAND_CLUSTER);
- else
- pgstat_progress_update_param(PROGRESS_CLUSTER_COMMAND,
- PROGRESS_CLUSTER_COMMAND_VACUUM_FULL);
+ pgstat_progress_update_param(PROGRESS_CLUSTER_COMMAND,
+ OidIsValid(indexOid) ?
PROGRESS_CLUSTER_COMMAND_CLUSTER :
+ PROGRESS_CLUSTER_COMMAND_VACUUM_FULL);

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com