Use pgstat_progress_update_multi_param instead of single param update

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Use pgstat_progress_update_multi_param instead of single param update

Bharath Rupireddy
Hi,

While reviewing progress reporting for COPY command patches [1], a
point on using pgstat_progress_update_multi_param instead of
pgstat_progress_update_param wherever possible was suggested in [1].
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] - https://www.postgresql.org/message-id/CALj2ACXQrFM%2BDSN9xr%3D%2ByRotBufnC_xgG-FQ6VXAUZRPihZAew%40mail.gmail.com

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

v1-0001-Update-multiple-progress-params-at-once.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Use pgstat_progress_update_multi_param instead of single param update

Michael Paquier-2
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?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Use pgstat_progress_update_multi_param instead of single param update

Bharath Rupireddy
On Sun, Feb 21, 2021 at 4:18 PM Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Use pgstat_progress_update_multi_param instead of single param update

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment