public inbox for [email protected]  

help / color / mirror / Atom feed

From: Imseih (AWS), Sami <[email protected]>
To: Masahiko Sawada <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Bossart, Nathan <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Peter Geoghegan <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Add index scan progress to pg_stat_progress_vacuum
Date: Fri, 14 Oct 2022 20:05:41 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAD21AoDc2UAqZV6JbWyFkCAhauVRxYOz6gNBB-kZ6uY87kBGWQ@mail.gmail.com>
References: <[email protected]>
	<CAD21AoD13LQ4+suYVWCyp07PHgoHVmAPZwc8neNdkTFVsSi-ww@mail.gmail.com>
	<[email protected]>
	<CAD21AoAwsaohDMissr_fR_YHp3fzq+eJ4n2=G2Bxji8aMqPAxA@mail.gmail.com>
	<[email protected]>
	<CAD21AoDAj67or-qEMZfdtz74=dgEpt1D=6ZcGJMK8TGvpR38aQ@mail.gmail.com>
	<[email protected]>
	<CAD21AoA9Phn9uPtyq_V+J9ctNhkgChnmm_hFT2sA2qR_KcKqYw@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CA+TgmoaQDGtKs8qotVBHRzr-G5OnrQNdeknAjM_GKdc0J_ideg@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CAD21AoATKO4Ltizm2CXtggE081tYeJSDWngz0w4w13wTwwCDpA@mail.gmail.com>
	<[email protected]>
	<CAD21AoDqVNhLyegNGGezXVhhkWnGHN84TJzeuRbqU077U_KRpw@mail.gmail.com>
	<CA+TgmoYuib=a1TiOn_P9n9w7gvBj_sL7jwQRBSBVpxoMVAGoUw@mail.gmail.com>
	<CAD21AoDnZrP-K6mC9oJ4cpoYNXeLOu09EET2zy564ke8u3RNrQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CAD21AoDc2UAqZV6JbWyFkCAhauVRxYOz6gNBB-kZ6uY87kBGWQ@mail.gmail.com>

Thank you for the feedback!

>    While it seems to be a good idea to have the atomic counter for the
>    number of indexes completed, I think we should not use the global
>    variable referencing the counter as follow:

>    +static pg_atomic_uint32 *index_vacuum_completed = NULL;
>    :
>    +void
>    +parallel_vacuum_progress_report(void)
>    +{
>    +   if (IsParallelWorker() || !report_parallel_vacuum_progress)
>    +       return;
>    +
>    +   pgstat_progress_update_param(PROGRESS_VACUUM_INDEX_COMPLETED,
>    +                                pg_atomic_read_u32(index_vacuum_completed));
>    +}

>    I think we can pass ParallelVacuumState (or PVIndStats) to the
>    reporting function so that it can check the counter and report the
>    progress.

Yes, that makes sense.

>    ---
>    I am not too sure that the idea of using the vacuum delay points is the best
>    plan. I think we should try to avoid piggybacking on such general
>    infrastructure if we can, and instead look for a way to tie this to
>   something that is specific to parallel vacuum.
>    ---

Adding the call to vacuum_delay_point made sense since it's
called in all major vacuum scans. While it is also called
by analyze, it will only do anything if the caller sets a flag
to report_parallel_vacuum_progress.


 >   Instead, I think we can add a boolean and the pointer for
 >   ParallelVacuumState to IndexVacuumInfo. If the boolean is true, an
 >   index AM can call the reporting function with the pointer to
 >   ParallelVacuumState while scanning index blocks, for example, for
 >   every 1GB. The boolean can be true only for the leader process.

Will doing this every 1GB be necessary? Considering the function
will not do much more than update progress from the value
stored in index_vacuum_completed?


>   Agreed, but with the following change, the leader process waits in a
>    busy loop, which should not be acceptable:

Good point.

>    Also, I think it's better to check whether idx_completed_progress
    equals to the number indexes instead.

Good point

    > 5. Went back to the idea of adding a new view called pg_stat_progress_vacuum_index
    > which is accomplished by adding a new type called VACUUM_PARALLEL in progress.h

>    Probably, we can devide the patch into two patches. One for adding the

Makes sense.

Thanks

Sami Imseih
Amazon Web Services (AWS)

view thread (96+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Add index scan progress to pg_stat_progress_vacuum
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox