Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 19 Jul 2024 12:17:35 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: REINDEX not updating partition progress
Message-ID: <Zpnaz_gHMyf0xsso@paquier.xyz>
References: <b72f2d89-820a-4fa2-9058-b155cf646f4f@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="ISYzhc31cg+fcTcO"
Content-Disposition: inline
In-Reply-To: <b72f2d89-820a-4fa2-9058-b155cf646f4f@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Zpnaz_gHMyf0xsso%40paquier.xyz>
Precedence: bulk


--ISYzhc31cg+fcTcO
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Jul 12, 2024 at 11:07:49PM +0100, Ilya Gladyshev wrote:
> While working on CIC for partitioned tables [1], I noticed that REINDEX f=
or
> partitioned tables is not tracking keeping progress of partitioned tables,
> so I'm creating a separate thread for this fix as suggested.

This limitation is not a bug, because we already document that
partitions_total and partitions_done are 0 during a REINDEX.  Compared
to CREATE INDEX, a REINDEX is a bit wilder as it would work on all the
indexes for all the partitions, providing this information makes
sense.

Agreed that this could be better, but what's now on HEAD is not wrong
either.

> The way REINDEX for partitioned tables works now ReindexMultipleInternal
> treats every partition as an independent table without keeping track of
> partition_done/total counts, because this is just generic code for
> processing multiple tables. The patch addresses that by passing down the
> knowledge a flag to distinguish partitions from independent tables
> in=A0ReindexMultipleInternal and its callees. I also noticed that the
> partitioned CREATE INDEX progress tracking could also benefit from
> progress_index_partition_done function that zeroizes params in addition to
> incrementing the counter, so I applied it there as well.

Still it does not do it because we know that the fields are going to
be overwritten pretty quickly anyway, no?  For now, we could just do
without progress_index_partition_done(), I guess, keeping it to the
incrementation of PROGRESS_CREATEIDX_PARTITIONS_DONE.  There's an
argument that this makes the code slightly easier to follow, with less
wrappers around the progress reporting.

+	int			progress_params[3] =3D {
+		PROGRESS_CREATEIDX_COMMAND,
+		PROGRESS_CREATEIDX_PHASE,
+		PROGRESS_CREATEIDX_PARTITIONS_TOTAL
+	};
+	int64		progress_values[3];
+	Oid			heapId =3D relid;

Rather than having new code blocks, let's use a style consistent with
DefineIndex() where we have the pgstat_progress_update_multi_param(),
with a single {} block.

Adding the counter increment at the end of the loop in
ReindexMultipleInternal() is a good idea.  It considers both the
concurrent and non-concurrent cases.

+   progress_values[2] =3D list_length(partitions);
+   pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);

Hmm.  Is setting the relid only once for pg_stat_progress_create_index
the best choice there is?  Could it be better to report the partition
OID instead?  Let's remember that indexes may have been attached with
names inconsistent with the partitioned table's index.  It is a bit
confusing to stick to the relid all the partitioned table all the
time, for all the indexes of all the partitions reindexed.  Could it
be better to actually introduce an entirely new field to the progress
table?  What I would look for is more information:
1) the partitioned table OID on which the REINDEX runs
2) the partition table being processed
3) the index OID being processed (old and new for concurrent case).

The patch sets 1) to the OID of the partitioned table, lacks 2) and
sets 3) each time an index is rebuilt.
--
Michael

--ISYzhc31cg+fcTcO
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmaZ2s8ACgkQnvQgOdby
QH0+8Q/+KWcGNPG+q3bgnhHcHoj1lNR2+I+waJX7iz5ITo6NkLTpO4xIu4W5tRwR
M+mo4CjF2gjPpVggjhog3Qyku570jRtL/OE0uwBz5yg9T4gag4y+HCiAXoP8jSFd
Hx1wualcpHECoQAGqyBfhVXHXFcn6LY2L3xRwndgT7t6CKtIDH34elUj0HBes8B9
db3yXVdFZeoWMLg3VgY6zi+VY3UkySpMk7KxOdvOLO2xC+6f5yBq5Zqj/6taHwL/
u46ReD0ujgYjuq3kYAyCzOJjO2/b4d8A1qXIESSppSFT7tExUB0JutwBl0CkqCJ9
UN9/yLbVoQ6rwAuCEYQBS5PxhDIhrQgk99JlBsil4alTjg/0jYAaC+75lEihJj9N
Z4c6/UJZe9UnEVDcIOt20ypsxwFquUH621FGkDU6AEWI9FMGOWz2fAEphuIQg+bq
l0C1huE3d8bDBmB/j6uWjJBCsdjYNF5Xg8itCnbft85GKsu8nQBfClz5Xf7/Z54H
m3tS+Y/OrGmKCxoH8g/DxkXelRpSqZ2fnXhP/ZX/vSQnJ2gtNOQXoPIrtMFFXrcf
LUmAT8YlpsNaGvG9PaCdtwFyNjM4+tMRDUn86IyHZcXN1/dN0eMDSWYHej5nOI3z
YzshGV+YZUp56SfvGRwHS/FbDza5/2vuEnwe2CzoHRdG8cGGb1s=
=jXpw
-----END PGP SIGNATURE-----

--ISYzhc31cg+fcTcO--