From: | Nathan Bossart <nathandbossart(at)gmail(dot)com> |
---|---|
To: | "Imseih (AWS), Sami" <simseih(at)amazon(dot)com> |
Cc: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Peter Geoghegan <pg(at)bowt(dot)ie>, "Bossart, Nathan" <bossartn(at)amazon(dot)com>, 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: | 2022-03-10 22:36:55 |
Message-ID: | 20220310223655.GA470606@nathanxps13 |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Mar 10, 2022 at 09:30:57PM +0000, Imseih (AWS), Sami wrote:
> Attached v4 which includes accounting for the hash size on startup, removal of the no longer needed comment in pgstatfuncs.c and a change in both code/docs to only reset the indexes_total to 0 when failsafe is triggered.
Thanks for the new patch set.
+/*
+ * Structs for tracking shared Progress information
+ * amongst worker ( and leader ) processes of a vacuum.
+ */
nitpick: Can we remove the extra spaces in the parentheses?
+ if (entry != NULL)
+ values[PGSTAT_NUM_PROGRESS_COMMON + PROGRESS_VACUUM_INDEXES_COMPLETED] = entry->indexes_processed;
What does it mean if there isn't an entry in the map? Is this actually
expected, or should we ERROR instead?
+ /* vacuum worker progress hash table */
+ max_table_size = GetMaxBackends();
+ size = add_size(size, hash_estimate_size(max_table_size,
+ sizeof(VacProgressEntry)));
I think the number of entries should be shared between
VacuumWorkerProgressShmemInit() and VacuumWorkerProgressShmemSize().
Otherwise, we might update one and not the other.
+ /* Call the command specific function to override datum values */
+ if (pg_strcasecmp(cmd, "VACUUM") == 0)
+ set_vaccum_worker_progress(values);
I think we should elaborate a bit more in this comment. It's difficult to
follow what this is doing without referencing the comment above
set_vacuum_worker_progress().
IMO the patches are in decent shape, and this should likely be marked as
ready-for-committer in the near future. Before doing so, I think we should
check that Sawada-san is okay with moving the deeper infrastructure changes
to a separate threaḋ.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
From | Date | Subject | |
---|---|---|---|
Next Message | Tomas Vondra | 2022-03-10 22:49:35 | Re: logical decoding and replication of sequences |
Previous Message | Tom Lane | 2022-03-10 22:36:50 | Re: ltree_gist indexes broken after pg_upgrade from 12 to 13 |