Horton

Thread

Commits

  1. Use pgstat_progress_update_multi_param() where possible

  1. Use pgstat_progress_update_multi_param instead of single param update

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-21T06:00:21Z

    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

  2. Re: Use pgstat_progress_update_multi_param instead of single param update

    Michael Paquier <michael@paquier.xyz> — 2021-02-21T10:47:54Z

    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

  3. Re: Use pgstat_progress_update_multi_param instead of single param update

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-21T11:13:23Z

    On 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

  4. Re: Use pgstat_progress_update_multi_param instead of single param update

    Michael Paquier <michael@paquier.xyz> — 2021-02-22T05:22:46Z

    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