Date: Wed, 15 May 2019 11:17:51 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers@lists.postgresql.org
Subject: Re: Inconsistent error message wording for REINDEX CONCURRENTLY
Message-ID: <20190515021609.GD1440@paquier.xyz>
References: <20190509041154.GC14573@paquier.xyz>
 <20190514153252.GA22168@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="QDd5rp1wjxlDmy9q"
Content-Disposition: inline
In-Reply-To: <20190514153252.GA22168@alvherre.pgsql>
Precedence: bulk


--QDd5rp1wjxlDmy9q
Content-Type: multipart/mixed; boundary="ulDeV4rPMk/y39in"
Content-Disposition: inline


--ulDeV4rPMk/y39in
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, May 14, 2019 at 11:32:52AM -0400, Alvaro Herrera wrote:
> I do :-)

And actually I am happy to see somebody raising that point.  The
current log messages are quite inconsistent for a couple of years now
but I did not bother changing anything other than the new strings per
the feedback I got until, well, yesterday.

> It seems strange to have some cases say "cannot do foo" and other cases
> say "foo is not supported".  However, I think having
> ReindexMultipleTables say "cannot reindex a system catalog" would be
> slightly wrong (since we're not reindexing one but many) -- so it would
> have to be "cannot reindex system catalogs".  And in order to avoid
> having two messages that are essentially identical except in number, I
> propose to change the others to use the plural too.  So the one you just
> committed
>=20
> would become "cannot reindex system catalogs concurrently", identical to
> the one in ReindexMultipleTables.

There are also a couple of similar, much older, error messages in
index_create() for concurrent creation.  Do you think that these
should be changed?  I can see benefits for translators to unify things
a bit more, but these do not directly apply to REINDEX, and all
messages are a bit different depending on the context.  One argument
to change them is that they don't comply with the project style as
they use full sentences.

Perhaps something like the attached for the REINDEX portion would make
the world a better place?  What do you think?
--
Michael

--ulDeV4rPMk/y39in
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="reindex-error-strings.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexc=
mds.c
index 7e7c03ef12..8501b29b0a 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2485,7 +2485,7 @@ ReindexMultipleTables(const char *objectName, Reindex=
ObjectType objectKind,
 	if (objectKind =3D=3D REINDEX_OBJECT_SYSTEM && concurrent)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("concurrent reindex of system catalogs is not supported")));
+				 errmsg("cannot reindex system catalogs concurrently")));
=20
 	/*
 	 * Get OID of object to reindex, being the database currently being used
@@ -2599,7 +2599,7 @@ ReindexMultipleTables(const char *objectName, Reindex=
ObjectType objectKind,
 			if (!concurrent_warning)
 				ereport(WARNING,
 						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-						 errmsg("concurrent reindex is not supported for catalog relations, =
skipping all")));
+						 errmsg("cannot reindex system catalogs concurrently, skipping all")=
));
 			concurrent_warning =3D true;
 			continue;
 		}
@@ -2747,7 +2747,7 @@ ReindexRelationConcurrently(Oid relationOid, int opti=
ons)
 				if (IsCatalogRelationOid(relationOid))
 					ereport(ERROR,
 							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-							 errmsg("cannot reindex a system catalog concurrently")));
+							 errmsg("cannot reindex system catalogs concurrently")));
=20
 				/* Open relation to get its indexes */
 				heapRelation =3D table_open(relationOid, ShareUpdateExclusiveLock);
@@ -2841,7 +2841,7 @@ ReindexRelationConcurrently(Oid relationOid, int opti=
ons)
 				if (IsCatalogRelationOid(heapId))
 					ereport(ERROR,
 							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-							 errmsg("cannot reindex a system catalog concurrently")));
+							 errmsg("cannot reindex system catalogs concurrently")));
=20
 				/* Save the list of relation OIDs in private context */
 				oldcontext =3D MemoryContextSwitchTo(private_context);
@@ -2862,7 +2862,7 @@ ReindexRelationConcurrently(Oid relationOid, int opti=
ons)
 			/* see reindex_relation() */
 			ereport(WARNING,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("REINDEX of partitioned tables is not yet implemented, skippi=
ng \"%s\"",
+					 errmsg("cannot reindex partitioned tables, skipping \"%s\"",
 							get_rel_name(relationOid))));
 			return false;
 		default:
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/=
expected/create_index.out
index c8bc6be061..fda9878f25 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2045,7 +2045,7 @@ REINDEX TABLE concur_reindex_part_10;
 WARNING:  REINDEX of partitioned tables is not yet implemented, skipping "=
concur_reindex_part_10"
 NOTICE:  table "concur_reindex_part_10" has no indexes
 REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
-WARNING:  REINDEX of partitioned tables is not yet implemented, skipping "=
concur_reindex_part_10"
+WARNING:  cannot reindex partitioned tables, skipping "concur_reindex_part=
_10"
 NOTICE:  table "concur_reindex_part_10" has no indexes
 SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_pa=
rt_index')
   ORDER BY relid, level;
@@ -2093,19 +2093,19 @@ REINDEX TABLE CONCURRENTLY concur_reindex_tab;
 ERROR:  REINDEX CONCURRENTLY cannot run inside a transaction block
 COMMIT;
 REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relation
-ERROR:  cannot reindex a system catalog concurrently
+ERROR:  cannot reindex system catalogs concurrently
 REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index
-ERROR:  cannot reindex a system catalog concurrently
+ERROR:  cannot reindex system catalogs concurrently
 -- These are the toast table and index of pg_authid.
 REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast tab=
le
-ERROR:  cannot reindex a system catalog concurrently
+ERROR:  cannot reindex system catalogs concurrently
 REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toa=
st index
-ERROR:  cannot reindex a system catalog concurrently
+ERROR:  cannot reindex system catalogs concurrently
 REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
-ERROR:  concurrent reindex of system catalogs is not supported
+ERROR:  cannot reindex system catalogs concurrently
 -- Warns about catalog relations
 REINDEX SCHEMA CONCURRENTLY pg_catalog;
-WARNING:  concurrent reindex is not supported for catalog relations, skipp=
ing all
+WARNING:  cannot reindex system catalogs concurrently, skipping all
 -- Check the relation status, there should not be invalid indexes
 \d concur_reindex_tab
          Table "public.concur_reindex_tab"

--ulDeV4rPMk/y39in--

--QDd5rp1wjxlDmy9q
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlzbds8ACgkQnvQgOdby
QH01mA/+OFEtil6yclKNwxNCLOoLcPOaxBFvJvXr9pOApd9czz6htjU5Ld6xphi8
y0MLOeGQS+HHU88mbNq2SP4EWPAzq6xtwoh+qZ2FAY2TQVEQ1wRWoutHDeXPoXtk
8FJb4MtBipqfR7t4pqaVp3a8LLmgYC3bGwJF3K5VNQPkK6PB62b2wUJi46Ne2nR9
OM9sNwCPmsrGiOOaGHOhxKKOyHHHSDt1ySLPeiErHCrKLXeIrIDbNbNF8fqueCc8
r/JIGA9VwOl+xwJRZJ0zL5Wux53PuNlFf0uQBlYjcV+hb5cL71bgoyVNP/l2mZo/
/FPDf2iSoExVk7iQYqCUElq2eNfP3cEJ2AcmVK+ORdnZ9uriuMoYCgK9IPHPqHy8
jUA36W83K/+QIE16oRuaq78HrE4G1nltenmBfKI6JJJ3W2ibOEqb0dudpA71ur02
EXtHvsbh0ZKhYKMXFpaFA3I9ZRmNV+eYOhUJjGB0fMyhjj3M5F2wWO/udL2sh6U2
gp0J5R7RociP9MJz2M0KbILs6/xPe3bguLxpH/TtIoTGorM0+83RU0Ca/XAppOQg
Pr3f5clNSyWDxlBGvtJj7lTyZNxV25k25aQ93DThCtDM82Pq10jeMqxNODnRZCeO
+POk7U8zw0I5Id6QSkTEr4tznbKmtF3jSxm6UJaVUQZa8zEYtHo=
=1f8O
-----END PGP SIGNATURE-----

--QDd5rp1wjxlDmy9q--