Improve REPACK (CONCURRENTLY) error messages some more

We had discussed changing the wording of messages from "cannot repack
table X" to "cannot execute REPACK on table X", so that translators
don't have to figure out how to translate REPACK as a verb in their
language.  We already do that for VACUUM and others and it's not very
nice.  Also remove extra double-quotes in a message of that form which I
mistakenly added in commit 43649b6a53e9.

While at it, add specific error messages for the cases of a table with a
deferrable primary key, and of REPLICA IDENTITY FULL; otherwise the user
gets a message that the table doesn't have an identity index and it's
not clear why that is.

Author: Baji Shaik <baji.pgdev@gmail.com>
Discussion: https://postgr.es/m/CA+fm-ROdgh0rEVuXoViBk4TVgjodrN=MTR_RYuOuKLZ9voX4YA@mail.gmail.com
Discussion: https://postgr.es/m/CABV9wwOo=wvq1hwTRK6HgBWUB=ekzsEebY30EWoc1V9UJQrrrw@mail.gmail.com

diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 94f2eaa23461202e078f92509b418de1a6a34282..ec100e3eef555c3ee64df44aff8040734d6be858 100644 (file)
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -901,7 +901,7 @@ check_concurrent_repack_requirements(Relation rel, Oid *ident_idx_p)
        if (wal_level < WAL_LEVEL_REPLICA)
                ereport(ERROR,
                                errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                               errmsg("cannot execute \"%s\" in this configuration",
+                               errmsg("cannot execute %s in this configuration",
                                           "REPACK (CONCURRENTLY)"),
                                errdetail("%s requires \"wal_level\" to be set to \"replica\" or higher.",
                                                  "REPACK (CONCURRENTLY)"));
@@ -910,8 +910,8 @@ check_concurrent_repack_requirements(Relation rel, Oid *ident_idx_p)
        if (IsCatalogRelation(rel))
                ereport(ERROR,
                                errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                               errmsg("cannot repack relation \"%s\"",
-                                          RelationGetRelationName(rel)),
+                               errmsg("cannot execute %s on relation \"%s\"",
+                                          "REPACK (CONCURRENTLY)", RelationGetRelationName(rel)),
                                errhint("%s is not supported for catalog relations.",
                                                "REPACK (CONCURRENTLY)"));
 
@@ -922,8 +922,8 @@ check_concurrent_repack_requirements(Relation rel, Oid *ident_idx_p)
        if (IsToastRelation(rel))
                ereport(ERROR,
                                errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                               errmsg("cannot repack relation \"%s\"",
-                                          RelationGetRelationName(rel)),
+                               errmsg("cannot execute %s on relation \"%s\"",
+                                          "REPACK (CONCURRENTLY)", RelationGetRelationName(rel)),
                                errhint("%s is not supported for TOAST relations.",
                                                "REPACK (CONCURRENTLY)"));
 
@@ -931,20 +931,26 @@ check_concurrent_repack_requirements(Relation rel, Oid *ident_idx_p)
        if (relpersistence != RELPERSISTENCE_PERMANENT)
                ereport(ERROR,
                                errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                               errmsg("cannot repack relation \"%s\"",
-                                          RelationGetRelationName(rel)),
+                               errmsg("cannot execute %s on relation \"%s\"",
+                                          "REPACK (CONCURRENTLY)", RelationGetRelationName(rel)),
                                errhint("%s is only allowed for permanent relations.",
                                                "REPACK (CONCURRENTLY)"));
 
-       /* With NOTHING, WAL does not contain the old tuple. */
+       /*
+        * With NOTHING, WAL does not contain the old tuple; FULL is not yet
+        * supported.
+        */
        replident = rel->rd_rel->relreplident;
-       if (replident == REPLICA_IDENTITY_NOTHING)
+       if (replident == REPLICA_IDENTITY_NOTHING ||
+               replident == REPLICA_IDENTITY_FULL)
                ereport(ERROR,
                                errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                               errmsg("cannot repack relation \"%s\"",
-                                          RelationGetRelationName(rel)),
-                               errhint("Relation \"%s\" has insufficient replication identity.",
-                                               RelationGetRelationName(rel)));
+                               errmsg("cannot execute %s on relation \"%s\"",
+                                          "REPACK (CONCURRENTLY)", RelationGetRelationName(rel)),
+                               errdetail("%s does not support tables with %s.",
+                                                 "REPACK (CONCURRENTLY)",
+                                                 replident == REPLICA_IDENTITY_NOTHING ?
+                                                 "REPLICA IDENTITY NOTHING" : "REPLICA IDENTITY FULL"));
 
        /*
         * Obtain the replica identity index -- either one that has been set
@@ -955,12 +961,25 @@ check_concurrent_repack_requirements(Relation rel, Oid *ident_idx_p)
         */
        ident_idx = GetRelationIdentityOrPK(rel);
        if (!OidIsValid(ident_idx))
+       {
+               /* This special case warrants its own error message */
+               if (OidIsValid(rel->rd_pkindex) && rel->rd_ispkdeferrable)
+                       ereport(ERROR,
+                                       errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                       errmsg("cannot execute %s on relation \"%s\"",
+                                                  "REPACK (CONCURRENTLY)",
+                                                  RelationGetRelationName(rel)),
+                                       errdetail("%s does not support deferrable primary keys.",
+                                                         "REPACK (CONCURRENTLY)"),
+                                       errhint("Use ALTER TABLE ... REPLICA IDENTITY USING INDEX to designate another index as replica identity."));
+
                ereport(ERROR,
                                errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                               errmsg("cannot process relation \"%s\"",
-                                          RelationGetRelationName(rel)),
+                               errmsg("cannot execute %s on relation \"%s\"",
+                                          "REPACK (CONCURRENTLY)", RelationGetRelationName(rel)),
                                errhint("Relation \"%s\" has no identity index.",
                                                RelationGetRelationName(rel)));
+       }
 
        *ident_idx_p = ident_idx;
 }

diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 504ac1a3131581d4b9f452a0b2f86137cbbda3b9..23f312c62a3cbb637cf062388c6d4773fd6bd76b 100644 (file)
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -801,7 +801,7 @@ ORDER BY o.relname;
 --
 -- Disallowed in catalogs
 REPACK (CONCURRENTLY) pg_class;
-ERROR:  cannot repack relation "pg_class"
+ERROR:  cannot execute REPACK (CONCURRENTLY) on relation "pg_class"
 HINT:  REPACK (CONCURRENTLY) is not supported for catalog relations.
 -- Doesn't like partitioned tables
 REPACK (CONCURRENTLY) clstrpart;
@@ -809,17 +809,17 @@ ERROR:  REPACK (CONCURRENTLY) is not supported for partitioned tables
 HINT:  Consider running the command on individual partitions.
 -- Doesn't support catalog tables
 REPACK (CONCURRENTLY) pg_class;
-ERROR:  cannot repack relation "pg_class"
+ERROR:  cannot execute REPACK (CONCURRENTLY) on relation "pg_class"
 HINT:  REPACK (CONCURRENTLY) is not supported for catalog relations.
 -- Only support permanent tables, temp and unlogged tables are not supported
 CREATE TEMP TABLE repack_conc_temp (i int PRIMARY KEY);
 REPACK (CONCURRENTLY) repack_conc_temp;
-ERROR:  cannot repack relation "repack_conc_temp"
+ERROR:  cannot execute REPACK (CONCURRENTLY) on relation "repack_conc_temp"
 HINT:  REPACK (CONCURRENTLY) is only allowed for permanent relations.
 DROP TABLE repack_conc_temp;
 CREATE UNLOGGED TABLE repack_conc_unlogged (i int PRIMARY KEY);
 REPACK (CONCURRENTLY) repack_conc_unlogged;
-ERROR:  cannot repack relation "repack_conc_unlogged"
+ERROR:  cannot execute REPACK (CONCURRENTLY) on relation "repack_conc_unlogged"
 HINT:  REPACK (CONCURRENTLY) is only allowed for permanent relations.
 DROP TABLE repack_conc_unlogged;
 -- Doesn't support TOAST tables directly
@@ -835,19 +835,20 @@ DROP TABLE repack_conc_toast;
 CREATE TABLE repack_conc_replident (i int PRIMARY KEY);
 ALTER TABLE repack_conc_replident REPLICA IDENTITY NOTHING;
 REPACK (CONCURRENTLY) repack_conc_replident;
-ERROR:  cannot repack relation "repack_conc_replident"
-HINT:  Relation "repack_conc_replident" has insufficient replication identity.
+ERROR:  cannot execute REPACK (CONCURRENTLY) on relation "repack_conc_replident"
+DETAIL:  REPACK (CONCURRENTLY) does not support tables with REPLICA IDENTITY NOTHING.
 -- Doesn't support tables without a primary key or replica identity index
 ALTER TABLE repack_conc_replident DROP CONSTRAINT repack_conc_replident_pkey;
 ALTER TABLE repack_conc_replident REPLICA IDENTITY DEFAULT;
 REPACK (CONCURRENTLY) repack_conc_replident;
-ERROR:  cannot process relation "repack_conc_replident"
+ERROR:  cannot execute REPACK (CONCURRENTLY) on relation "repack_conc_replident"
 HINT:  Relation "repack_conc_replident" has no identity index.
 -- Doesn't support tables with deferrable primary keys
 ALTER TABLE repack_conc_replident ADD PRIMARY KEY (i) DEFERRABLE;
 REPACK (CONCURRENTLY) repack_conc_replident;
-ERROR:  cannot process relation "repack_conc_replident"
-HINT:  Relation "repack_conc_replident" has no identity index.
+ERROR:  cannot execute REPACK (CONCURRENTLY) on relation "repack_conc_replident"
+DETAIL:  REPACK (CONCURRENTLY) does not support deferrable primary keys.
+HINT:  Use ALTER TABLE ... REPLICA IDENTITY USING INDEX to designate another index as replica identity.
 -- clean up
 DROP TABLE repack_conc_replident;
 DROP TABLE clustertest;