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+27-16
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