From: Imseih (AWS), Sami <simseih@amazon.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Bossart, Nathan <bossartn@amazon.com>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: Add index scan progress to pg_stat_progress_vacuum
Date: Fri, 20 Jan 2023 14:38:33 +0000
Message-ID: <08FEC8D2-B5BA-4E0F-80EB-5428E7C980EC@amazon.com> (raw)
In-Reply-To: <CAD21AoAnqAM3Eagdat_b9gMDyBRZT4=48FHd=FbZ20JndZjn2w@mail.gmail.com>
References: <20221230183955.GB437226@nathanxps13>
<3D992C93-AA5D-4719-8712-6ECD3D864966@amazon.com>
<CALDaNm3OxrvWHftY9Asc4VKamApgPQ7smtP0ahCRwacMOTojdQ@mail.gmail.com>
<7A191516-3D65-4819-BF19-E116B53E783C@amazon.com>
<977c555e-5d10-526f-ada0-b58d12267075@gmail.com>
<2397CA8A-DAD7-46C7-AF69-97365DA47AF8@amazon.com>
<CAD21AoAL=0rp0Km=wSgGAnonAK5B_+efi+VGnquXFuCiW_ijHw@mail.gmail.com>
<84DF5E2A-3AB5-4744-92B3-DA63F9C4B3AE@amazon.com>
<CAD21AoDUuNrb_gL7-SvKRSFfpJnpMBZpY=e5Scu_4NxuoBgBJQ@mail.gmail.com>
<39C09108-1A6A-4105-9E16-5F0F68503658@amazon.com>
<20230110015247.yspx3hkz2sk6p6u3@awork3.anarazel.de>
<72CD33F6-C2B5-45E4-A78F-85EC923DCF0F@amazon.com>
<CAD21AoAnqAM3Eagdat_b9gMDyBRZT4=48FHd=FbZ20JndZjn2w@mail.gmail.com>
> Number of indexes that will be vacuumed or cleaned up. This counter only
> advances when the phase is vacuuming indexes or cleaning up indexes.
I agree, this reads better.
---
- /* Report that we are now vacuuming indexes */
- pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
-
PROGRESS_VACUUM_PHASE_VACUUM_INDEX);
+ /*
+ * Report that we are now vacuuming indexes
+ * and the number of indexes to vacuum.
+ */
+ progress_start_val[0] = PROGRESS_VACUUM_PHASE_VACUUM_INDEX;
+ progress_start_val[1] = vacrel->nindexes;
+ pgstat_progress_update_multi_param(2, progress_start_index,
progress_start_val);
> According to our code style guideline[1], we limit line lengths so
> that the code is readable in an 80-column window. Some comments
> updated in this patch seem too short.
I will correct this.
> I think it's better to define "void *arg".
Agree
> ---
> + /*
> + * A Leader process that receives this message
> + * must be ready to update progress.
> + */
> + Assert(pcxt->parallel_progress_callback);
> + Assert(pcxt->parallel_progress_callback_arg);
> +
> + /* Report progress */
> +
> pcxt->parallel_progress_callback(pcxt->parallel_progress_callback_arg);
> I think the parallel query infra should not require
> parallel_progress_callback_arg to always be set. I think it can be
> NULL.
This assertion is inside the new 'P' message type handling.
If a leader is consuming this message, they must have a
progress callback set. Right now we only set the callback
in the parallel vacuum case only, so not all leaders will be prepared
to handle this case.
Would you agree this is needed for safety?
case 'P': /* Parallel progress reporting */
{
/*
* A Leader process that receives this message
* must be ready to update progress.
*/
Assert(pcxt->parallel_progress_callback);
Assert(pcxt->parallel_progress_callback_arg);
---
> +void
> +parallel_vacuum_update_progress(void *arg)
> +{
> + ParallelVacuumState *pvs = (ParallelVacuumState *)arg;
> +
> + Assert(!IsParallelWorker());
> +
> + if (pvs)
> + pgstat_progress_update_param(PROGRESS_VACUUM_INDEX_COMPLETED,
> +
> pg_atomic_add_fetch_u32(&(pvs->shared->nindexes_completed), 1));
> +}
> Since parallel vacuum always sets the arg, I think we don't need to check it.
The arg is only set during parallel vacuum. During non-parallel vacuum,
It's NULL. This check can be removed, but I realize now that we do need
an Assert(pvs). Do you agree?
--
Regards,
Sami Imseih
Amazon Web Services (AWS)
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: pgsql-hackers@postgresql.org
Cc: simseih@amazon.com, sawada.mshk@gmail.com, andres@anarazel.de, bertranddrouvot.pg@gmail.com, vignesh21@gmail.com, nathandbossart@gmail.com, robertmhaas@gmail.com, bossartn@amazon.com, pg@bowt.ie, pryzby@telsasoft.com
Subject: Re: Add index scan progress to pg_stat_progress_vacuum
In-Reply-To: <08FEC8D2-B5BA-4E0F-80EB-5428E7C980EC@amazon.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox