Use pgstat_progress_update_multi_param instead of single param update
Hi,
While reviewing progress reporting for COPY command patches [1]/messages/by-id/CALj2ACXQrFM+DSN9xr=+yRotBufnC_xgG-FQ6VXAUZRPihZAew@mail.gmail.com, a
point on using pgstat_progress_update_multi_param instead of
pgstat_progress_update_param wherever possible was suggested in [1]/messages/by-id/CALj2ACXQrFM+DSN9xr=+yRotBufnC_xgG-FQ6VXAUZRPihZAew@mail.gmail.com.
We could do multiple param updates at once with a single API than
doing each param update separately. The advantages are - 1) reducing
few function calls making the code look cleaner 2) atomically updating
multiple parameters at once within a single backend write critical
section i.e. incrementing st_changecount at once in a critical
section instead of doing it for each param separately.
Attached is a patch that replaces some subsequent multiple
update_param calls with a single update_multi_param.
Thoughts?
[1]: /messages/by-id/CALj2ACXQrFM+DSN9xr=+yRotBufnC_xgG-FQ6VXAUZRPihZAew@mail.gmail.com
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v1-0001-Update-multiple-progress-params-at-once.patchapplication/octet-stream; name=v1-0001-Update-multiple-progress-params-at-once.patchDownload
From d3470c85cc21e97dbc6db932c37c0e97b7210f78 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Sat, 20 Feb 2021 14:04:34 +0530
Subject: [PATCH v1] Update multiple progress params at once
---
src/backend/catalog/index.c | 14 ++++++++++----
src/backend/commands/cluster.c | 9 +++------
src/backend/commands/indexcmds.c | 19 ++++++++++++++-----
3 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index b4ab0b88ad..42204d3401 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -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);
}
/*
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 096a06f7b3..593b75ed8f 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -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);
/*
* We grab exclusive access to the target rel and index for the duration
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index e1bed087d7..7e7faa78b0 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -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);
+ }
/*
* Phase 2 of concurrent index build (see comments for validate_index()
@@ -1478,8 +1489,6 @@ DefineIndex(Oid relationId,
* exclusive lock on our table. The lock code will detect deadlock and
* error out properly.
*/
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
- PROGRESS_CREATEIDX_PHASE_WAIT_1);
WaitForLockers(heaplocktag, ShareLock, true);
/*
--
2.25.1
On Sun, Feb 21, 2021 at 11:30:21AM +0530, Bharath Rupireddy wrote:
Attached is a patch that replaces some subsequent multiple
update_param calls with a single update_multi_param.
Looks mostly fine to me.
- 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);
What's the point of changing this one?
--
MichaelOn Sun, Feb 21, 2021 at 4:18 PM Michael Paquier <michael@paquier.xyz> wrote:
On Sun, Feb 21, 2021 at 11:30:21AM +0530, Bharath Rupireddy wrote:
Attached is a patch that replaces some subsequent multiple
update_param calls with a single update_multi_param.Looks mostly fine to me.
- 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); What's the point of changing this one?
While we are at it, I wanted to use a single line statement instead of
if else, just like we do it in do_analyze_rel as below.
pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
inh ?
PROGRESS_ANALYZE_PHASE_ACQUIRE_SAMPLE_ROWS_INH :
PROGRESS_ANALYZE_PHASE_ACQUIRE_SAMPLE_ROWS);
We can ignore it if it doesn't seem a good way.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On Sun, Feb 21, 2021 at 04:43:23PM +0530, Bharath Rupireddy wrote:
While we are at it, I wanted to use a single line statement instead of
if else, just like we do it in do_analyze_rel as below.pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
inh ?
PROGRESS_ANALYZE_PHASE_ACQUIRE_SAMPLE_ROWS_INH :
PROGRESS_ANALYZE_PHASE_ACQUIRE_SAMPLE_ROWS);We can ignore it if it doesn't seem a good way.
What's always annoying with such things is that they create conflicts
with back-branches. So I have removed it, and applied the rest.
Thanks!
--
Michael