From: | "Drouvot, Bertrand" <bertranddrouvot(dot)pg(at)gmail(dot)com> |
---|---|
To: | "Imseih (AWS), Sami" <simseih(at)amazon(dot)com>, vignesh C <vignesh21(at)gmail(dot)com> |
Cc: | Nathan Bossart <nathandbossart(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Robert Haas <robertmhaas(at)gmail(dot)com>, "Bossart, Nathan" <bossartn(at)amazon(dot)com>, Peter Geoghegan <pg(at)bowt(dot)ie>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Add index scan progress to pg_stat_progress_vacuum |
Date: | 2023-01-04 09:50:31 |
Message-ID: | 977c555e-5d10-526f-ada0-b58d12267075@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | vignesh C | 2023-01-04 09:52:27 | Re: Pluggable toaster |
Previous Message | vignesh C | 2023-01-04 09:49:24 | Re: Parallelize correlated subqueries that execute within each worker |