pgsql-3854

Version:

8.2.6

Links:

bug report: http://archives.postgresql.org/pgsql-bugs/2008-01/msg00036.php

Patch: http://archives.postgresql.org/pgsql-committers/2008-01/msg00241.php

Summarize in weekly news:

- Fix ALTER INDEX RENAME so that if the index belongs to a unique or primary key constraint, the constraint is renamed as well. This avoids inconsistent situations that could confuse pg_dump (not to mention humans). We might at some point provide ALTER TABLE RENAME CONSTRAINT as a more general solution, but there seems no reason not to allow doing it this way too. Per bug #3854 and related discussions.

Symptom:

Background:

pg_dump can dump a database, the dumped file is a script that can be used to step-by-step reconstruct the dumped db. pg_restore can later read in the dumped file by pg_dump to reconstruct.

After renaming index, lead to unrestorable pg_dumps.

Specifically, if we reproduce the failure using the sequence of actions above, we notice the dump cannot be restored. The failure is that in the dump script:

ALTER TABLE ONLY mytable

    ADD CONSTRAINT mytable_pkey PRIMARY KEY (bar);

ALTER TABLE mytable CLUSTER ON mynew_pkey;

This is an unreproduceable dump, since mynew_pkey cannot be found. The correct output should be:

ALTER TABLE ONLY mytable

    ADD CONSTRAINT mynew_pkey PRIMARY KEY (bar);

ALTER TABLE mytable CLUSTER ON mynew_pkey;

How it is reproduced:

Reproduced.

How to reproduce:

1.

foo=# create table mytable(bar int primary key);

NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "mytable_pkey" for table "mytable"

CREATE TABLE

2.

foo=# alter index mytable_pkey rename to mynew_pkey;

ALTER INDEX

3.

foo=# cluster mynew_pkey on mytable;

CLUSTER

4.

foo=# \d mytable

    Table "public.mytable"

 Column |  Type   | Modifiers

--------+---------+-----------

 bar    | integer | not null

Indexes:

    "mynew_pkey" PRIMARY KEY, btree (bar) CLUSTER

5.

foo=# \q

[dyuan3@soprano bin]$ ./pg_dump -p 5555 -Fc foo > foo.dump

6.

[dyuan3@soprano bin]$ ./createdb -p 5555 newdb

CREATE DATABASE

7.

[dyuan3@soprano bin]$ ./pg_restore -p 5555 -C -d newdb ./foo.dump

pg_restore: [archiver (db)] Error while PROCESSING TOC:

pg_restore: [archiver (db)] Error from TOC entry 1598; 2606 16388 CONSTRAINT mytable_pkey dyuan3

pg_restore: [archiver (db)] could not execute query: ERROR:  index "mynew_pkey" for table "mytable" does not exist

    Command was: ALTER TABLE mytable CLUSTER ON mynew_pkey;

WARNING: errors ignored on restore: 1

Root cause:
Postgres did not rename the internal constraint after the ‘ALTER INDEX’ as expected. Thus during the pg_dump, the old ‘con->conname’ (which is mytable_pkey) was dumped.

tablecmds.c:
*************** renamerel(Oid myrelid, const char *newre
*** 1710,1715 ****
--- 1710,1726 ----
          */
         if (OidIsValid(targetrelation->rd_rel->reltype))
                 TypeRename(targetrelation->rd_rel->reltype, newrelname, namespaceId);
+
+         /*
+          * Also rename the associated constraint, if any.
+          */
+         if (relkind == RELKIND_INDEX)
+         {
+                 Oid                        constraintId = get_index_constraint(myrelid);
+
+                 if (OidIsValid(constraintId))
+                         RenameConstraintById(constraintId, newrelname);
+         }
 
         /*
          * Close rel, but keep exclusive lock!

+ /*
+  * RenameConstraintById
+  *                Rename a constraint.
+  *
+  * Note: this isn't intended to be a user-exposed function; it doesn't check
+  * permissions etc.  Currently this is only invoked when renaming an index
+  * that is associated with a constraint, but it's made a little more general
+  * than that with the expectation of someday having ALTER TABLE RENAME
+  * CONSTRAINT.
+  */
+ void
+ RenameConstraintById(Oid conId, const char *newname)
+ {
+         Relation        conDesc;
+         HeapTuple        tuple;
+         Form_pg_constraint con;
+
+         conDesc = heap_open(ConstraintRelationId, RowExclusiveLock);
+
+         tuple = SearchSysCacheCopy(CONSTROID,
+                                                            ObjectIdGetDatum(conId),
+                                                            0, 0, 0);
+         if (!HeapTupleIsValid(tuple))
+                 elog(ERROR, "cache lookup failed for constraint %u", conId);
+         con = (Form_pg_constraint) GETSTRUCT(tuple);
+
+         /*
+          * We need to check whether the name is already in use --- note that
+          * there currently is not a unique index that would catch this.
+          */
+         if (OidIsValid(con->conrelid) &&
+                 ConstraintNameIsUsed(CONSTRAINT_RELATION,
+                                                          con->conrelid,
+                                                          con->connamespace,
+                                                          newname))
+                 ereport(ERROR,
+                                 (errcode(ERRCODE_DUPLICATE_OBJECT),
+                                  errmsg("constraint \"%s\" for relation \"%s\" already exists",
+                                                 newname, get_rel_name(con->conrelid))));
+         if (OidIsValid(con->contypid) &&
+                 ConstraintNameIsUsed(CONSTRAINT_DOMAIN,
+                                                          con->contypid,
+                                                          con->connamespace,
+                                                          newname))
+                 ereport(ERROR,
+                                 (errcode(ERRCODE_DUPLICATE_OBJECT),
+                                  errmsg("constraint \"%s\" for domain \"%s\" already exists",
+                                                 newname, format_type_be(con->contypid))));
+
+         /* OK, do the rename --- tuple is a copy, so OK to scribble on it */
+         namestrcpy(&(con->conname), newname);
+
+         simple_heap_update(conDesc, &tuple->t_self, tuple);
+
+         /* update the system catalog indexes */
+         CatalogUpdateIndexes(conDesc, tuple);
+
+         heap_freetuple(tuple);
         heap_close(conDesc, RowExclusiveLock);
 }

/* Public interface */

/* Convenience function to send a query. Monitors result to handle COPY statements */

static void

ExecuteSqlCommand(ArchiveHandle *AH, const char *qry, const char *desc)

{

        PGconn           *conn = AH->connection;

        PGresult   *res;

        char                errStmt[DB_MAX_ERR_STMT];

#ifdef NOT_USED

        fprintf(stderr, "Executing: '%s'\n\n", qry);

#endif

        res = PQexec(conn, qry);

        switch (PQresultStatus(res))

        {

                case PGRES_COMMAND_OK:

                case PGRES_TUPLES_OK:

                        /* A-OK */

                        break;

                case PGRES_COPY_IN:

                        /* Assume this is an expected result */

                        AH->pgCopyIn = true;

                        break;

                default:

                        /* trouble */

                        strncpy(errStmt, qry, DB_MAX_ERR_STMT);

                        if (errStmt[DB_MAX_ERR_STMT - 1] != '\0')

                        {

                                errStmt[DB_MAX_ERR_STMT - 4] = '.';

                                errStmt[DB_MAX_ERR_STMT - 3] = '.';

                                errStmt[DB_MAX_ERR_STMT - 2] = '.';

                                errStmt[DB_MAX_ERR_STMT - 1] = '\0';

                        }

                        warn_or_die_horribly(AH, modulename, "%s: %s    Command was: %s\n",

                                                                 desc, PQerrorMessage(conn), errStmt);

                        break;

        }

        PQclear(res);

Is there any log message?:

Yes.

Where the log message is printed?

/* Public interface */

/* Convenience function to send a query. Monitors result to handle COPY statements */

static void

ExecuteSqlCommand(ArchiveHandle *AH, const char *qry, const char *desc)

{

        PGconn           *conn = AH->connection;

        PGresult   *res;

        char                errStmt[DB_MAX_ERR_STMT];

#ifdef NOT_USED

        fprintf(stderr, "Executing: '%s'\n\n", qry);

#endif

        res = PQexec(conn, qry);

        switch (PQresultStatus(res))

        {

                case PGRES_COMMAND_OK:

                case PGRES_TUPLES_OK:

                        /* A-OK */

                        break;

                case PGRES_COPY_IN:

                        /* Assume this is an expected result */

                        AH->pgCopyIn = true;

                        break;

                default:

                        /* trouble */

                        strncpy(errStmt, qry, DB_MAX_ERR_STMT);

                        if (errStmt[DB_MAX_ERR_STMT - 1] != '\0')

                        {

                                errStmt[DB_MAX_ERR_STMT - 4] = '.';

                                errStmt[DB_MAX_ERR_STMT - 3] = '.';

                                errStmt[DB_MAX_ERR_STMT - 2] = '.';

                                errStmt[DB_MAX_ERR_STMT - 1] = '\0';

                        }

                        warn_or_die_horribly(AH, modulename, "%s: %s    Command was: %s\n",

                                                                 desc, PQerrorMessage(conn), errStmt);

                        break;

        }

        PQclear(res);

}

How ErrLog automatically insert the log message?

switch-default