Date: Thu, 10 Mar 2022 14:36:55 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: "Imseih (AWS), Sami" <simseih@amazon.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>, Peter Geoghegan <pg@bowt.ie>,
	"Bossart, Nathan" <bossartn@amazon.com>,
	Justin Pryzby <pryzby@telsasoft.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Add index scan progress to pg_stat_progress_vacuum
Message-ID: <20220310223655.GA470606@nathanxps13>
References: 
 <CAD21AoA4kNdG6Qi8Qr4vFpvrGKd9MqjVPLUVZMEoBWHdCqfu9Q@mail.gmail.com>
 <F5F2CD8C-49D4-4B9A-983F-E3FF0E8CF048@amazon.com>
 <CAD21AoBW6SMJ96CNoMeu+f_BR4jmatPcfVA016FdD2hkLDsaTA@mail.gmail.com>
 <D4BBFE30-AB76-49E1-9A47-5A2851CCCB9F@amazon.com>
 <CAD21AoDf4pf89i7fgzrHDdJueNHLm4x92xHjcDeYaV+pdhem-Q@mail.gmail.com>
 <99A5CBE8-EC10-42AA-ABB4-4C812EEFD294@amazon.com>
 <20220310005237.GA307769@nathanxps13>
 <FA77DBA0-B42B-43F5-8406-E7069614CAB3@amazon.com>
 <333D1326-1EED-416F-A9AF-21C90C1C098C@amazon.com>
 <A8C3DA4F-FF8C-4E25-AC78-D94701F938BD@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <A8C3DA4F-FF8C-4E25-AC78-D94701F938BD@amazon.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220310223655.GA470606%40nathanxps13>
Precedence: bulk

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