Date: Mon, 21 Feb 2022 11:09:10 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: "Imseih (AWS), Sami" <simseih@amazon.com>
Cc: "Bossart, Nathan" <bossartn@amazon.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>,
	Justin Pryzby <pryzby@telsasoft.com>, Peter Geoghegan <pg@bowt.ie>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Add index scan progress to pg_stat_progress_vacuum
Message-ID: <20220221190910.GA3740885@nathanxps13>
References: <6D2C60C2-C09E-4117-A6B8-DCA7A0F5B182@amazon.com>
 <22C9CB0C-9576-47AF-8A6E-F9573F73CD4E@amazon.com>
 <CAD21AoCWFyvtf_EHHLWqejDjW0=apRugn_=UkpGzY5OgKhkH2g@mail.gmail.com>
 <6753CC4A-D1D8-477D-9B82-CFE12169DFF5@amazon.com>
 <7A4B3BA8-0768-4463-9B25-25DBECF56B25@amazon.com>
 <96387867-CBAF-4CCF-861E-2F4ACBCBBD68@amazon.com>
 <DC471B48-A815-4326-AC62-3672CDB9B4AB@amazon.com>
 <CC7DB450-A142-43B0-A68F-C6B45DC435F2@amazon.com>
 <441BA1FF-23AA-4CEE-9976-7FB693C30D00@amazon.com>
 <1E7CB510-FB91-4DDB-93E1-A19C05F299F3@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1E7CB510-FB91-4DDB-93E1-A19C05F299F3@amazon.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220221190910.GA3740885%40nathanxps13>
Precedence: bulk

On Mon, Feb 21, 2022 at 07:03:39PM +0000, Imseih (AWS), Sami wrote:
> Sending again with patch files renamed to ensure correct apply order.

I haven't had a chance to test this too much, but I did look through the
patch set and have a couple of small comments.

+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>indexes_total</structfield> <type>bigint</type>
+      </para>
+      <para>
+       The number of indexes to be processed in the
+       <literal>vacuuming indexes</literal> or <literal>cleaning up indexes</literal> phase
+       of the vacuum.
+      </para></entry>
+     </row>
+
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>indexes_processed</structfield> <type>bigint</type>
+      </para>
+      <para>
+       The number of indexes processed in the
+       <literal>vacuuming indexes</literal> or <literal>cleaning up indexes</literal> phase.
+       At the start of an index vacuum cycle, this value is set to <literal>0</literal>.
+      </para></entry>
+     </row>

Will these be set to 0 for failsafe vacuums and vacuums with INDEX_CLEANUP
turned off?

+typedef struct VacWorkerProgressInfo
+{
+    int                     num_vacuums;    /* number of active VACUUMS with parallel workers */
+    int                     max_vacuums;    /* max number of VACUUMS with parallel workers */
+    slock_t         mutex;
+    VacOneWorkerProgressInfo vacuums[FLEXIBLE_ARRAY_MEMBER];
+} VacWorkerProgressInfo;

max_vacuums appears to just be a local copy of MaxBackends.  Does this
information really need to be stored here?  Also, is there a strong reason
for using a spinlock instead of an LWLock?

+void
+vacuum_worker_end(int leader_pid)
+{
+    SpinLockAcquire(&vacworkerprogress->mutex);
+    for (int i = 0; i < vacworkerprogress->num_vacuums; i++)
+    {
+        VacOneWorkerProgressInfo *vac = &vacworkerprogress->vacuums[i];
+
+        if (vac->leader_pid == leader_pid)
+        {
+            *vac = vacworkerprogress->vacuums[vacworkerprogress->num_vacuums - 1];
+            vacworkerprogress->num_vacuums--;
+            SpinLockRelease(&vacworkerprogress->mutex);
+            break;
+        }
+    }
+    SpinLockRelease(&vacworkerprogress->mutex);
+}

I see this loop pattern in a couple of places, and it makes me wonder if
this information would fit more naturally in a hash table.

+        if (callback)
+            callback(values, 3);

Why does this need to be set up as a callback function?  Could we just call
the function if cmdtype == PROGRESS_COMMAND_VACUUM?  ISTM that is pretty
much all this is doing.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com