Message-ID: <977c555e-5d10-526f-ada0-b58d12267075@gmail.com>
Date: Wed, 4 Jan 2023 10:50:31 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: Add index scan progress to pg_stat_progress_vacuum
To: "Imseih (AWS), Sami" <simseih@amazon.com>, vignesh C <vignesh21@gmail.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>,
 Masahiko Sawada <sawada.mshk@gmail.com>, Andres Freund <andres@anarazel.de>,
 Robert Haas <robertmhaas@gmail.com>, "Bossart, Nathan"
 <bossartn@amazon.com>, Peter Geoghegan <pg@bowt.ie>,
 Justin Pryzby <pryzby@telsasoft.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
References: <D460F8EF-D61F-46F3-93FE-E6B5EB2F687A@amazon.com>
 <AC87124B-ABCA-4C26-8E08-36DCB5485419@amazon.com>
 <20221109030030.5wmvr76xj256cccw@awork3.anarazel.de>
 <7C22F915-5BB8-4729-8FF1-98B774CD6C85@amazon.com>
 <CAD21AoC-npaR1xsCn1JWAds1CEwF-ccqK-W+Lowa_rqym1qBwA@mail.gmail.com>
 <3FC1BD50-1640-45DC-A354-B6183B0966F9@amazon.com>
 <CAD21AoCmXtJKvFFA_TMT-4iVoAZuh=FJcz5oysD5nzmswi+7LA@mail.gmail.com>
 <7CCF18B6-9CB6-4DC6-AE9B-756F388C541F@amazon.com>
 <CAD21AoAjSYmTWgWvuJhDa7Dwi0wZLJBh+1irybjCLRVh6jvJKg@mail.gmail.com>
 <20F7ED07-4D32-4231-A81C-432EB111F0B2@amazon.com>
 <20221230183955.GB437226@nathanxps13>
 <3D992C93-AA5D-4719-8712-6ECD3D864966@amazon.com>
 <CALDaNm3OxrvWHftY9Asc4VKamApgPQ7smtP0ahCRwacMOTojdQ@mail.gmail.com>
 <7A191516-3D65-4819-BF19-E116B53E783C@amazon.com>
Content-Language: en-US
From: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
In-Reply-To: <7A191516-3D65-4819-BF19-E116B53E783C@amazon.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/977c555e-5d10-526f-ada0-b58d12267075%40gmail.com>
Precedence: bulk

Hi,

On 1/3/23 5:46 PM, Imseih (AWS), Sami wrote:
>> cirrus-ci.com/task/4557389261701120
> 
> I earlier compiled without building with --enable-cassert,
> which is why the compilation errors did not produce on my
> buid.
> 
> Fixed in v19.
> 
> Thanks
> 

Thanks for the updated patch!

Some comments about it:

+
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>indexes_total</structfield> <type>bigint</type>
+      </para>
+      <para>
+       Number of indexes that wil be vacuumed. This value will be

Typo: wil

+       /* report number of indexes to vacuum, if we are told to cleanup indexes */
+       if (vacrel->do_index_cleanup)
+               pgstat_progress_update_param(PROGRESS_VACUUM_INDEX_TOTAL, vacrel->nindexes);

"Report" instead? (to looks like the surrounding code)

+                       /*
+                        * Done vacuuming an index. Increment the indexes completed
+                        */
+                       pgstat_progress_update_param(PROGRESS_VACUUM_INDEX_COMPLETED,
+                                                                                idx + 1);

"Increment the indexes completed." (dot at the end) instead?

-        * Increase and report the number of index scans.
+        * Reset and report the number of indexes scanned.
+        * Also, increase and report the number of index
+        * scans.

What about "Reset and report zero as the number of indexes scanned."? (just to make clear we
don't want to report the value as it was prior to the reset)

-               /* Disable index vacuuming, index cleanup, and heap rel truncation */
+               /*
+                * Disable index vacuuming, index cleanup, and heap rel truncation
+                *

The new "Disable index vacuuming, index cleanup, and heap rel truncation" needs a dot at the end.

+                * Also, report to progress.h that we are no longer tracking
+                * index vacuum/cleanup.
+                */

"Also, report that we are" instead?

+                       /*
+                        * Done cleaning an index. Increment the indexes completed
+                        */

Needs a dot at the end.

-       /* Reset the parallel index processing counter */
+       /* Reset the parallel index processing counter ( index progress counter also ) */

"Reset the parallel index processing and progress counters" instead?

+       /* Update the number of indexes completed. */
+       pg_atomic_add_fetch_u32(&(pvs->shared->nindexes_completed), 1);

Remove the dot at the end? (to looks like the surrounding code)

+
+/*
+ * Read pvs->shared->nindexes_completed and update progress.h
+ * with indexes vacuumed so far. This function is called periodically

"Read pvs->shared->nindexes_completed and report the number of indexes vacuumed so far" instead?

+ * Note: This function should be used by the leader process only,

"called" instead of "used"?

                 case WAIT_EVENT_XACT_GROUP_UPDATE:
                         event_name = "XactGroupUpdate";
                         break;
+               case WAIT_EVENT_PARALLEL_VACUUM_FINISH:
+                       event_name = "ParallelVacuumFinish";
+                       break;
                         /* no default case, so that compiler will warn */

It seems to me that the case ordering should follow the alphabetical order (that's how it is done currently without the patch).

+#define REPORT_PARALLEL_VACUUM_EVERY_PAGES ((BlockNumber) (((uint64) 1024 * 1024 * 1024) / BLCKSZ))

It seems to me that "#define REPORT_PARALLEL_VACUUM_EVERY_PAGES ((BlockNumber) (1024 * 1024 * 1024 / BLCKSZ))" would be fine too.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com