Re: pg_migrator and handling dropped columns

Applied.

---------------------------------------------------------------------------

Bruce Momjian wrote:
> Peter Eisentraut wrote:
> > Tom Lane wrote:
> > >> Is this acceptable to everyone? We could name the option
> > >> -u/--upgrade-compatible.
> > >
> > > If the switch is specifically for pg_upgrade support (enabling this as
> > > well as any other hacks we find necessary), which seems like a good
> > > idea, then don't chew up a short option letter for it. There should be
> > > a long form only.
> >
> > Note that pg_dump's output is already upgrade compatible. That's what
> > pg_dump is often used for after all. I believe what we are after here
> > is something like "in-place upgrade compatible" or "upgrade binary
> > compatible".
> >
> > > And probably not even list it in the user documentation.
> >
> > I think we should still list it somewhere and say it is for use by
> > in-place upgrade utilities. It will only confuse people if it is not
> > documented at all.
>
> OK, I have completed the patch; attached.
>
> I ran into a little problem, as documented by this comment in
> catalog/heap.c:
>
> /*
> * Set the type OID to invalid. A dropped attribute's type link
> * cannot be relied on (once the attribute is dropped, the type might
> * be too). Fortunately we do not need the type row --- the only
> * really essential information is the type's typlen and typalign,
> * which are preserved in the attribute's attlen and attalign. We set
> * atttypid to zero here as a means of catching code that incorrectly
> * expects it to be valid.
> */
>
> Basically, drop column zeros pg_attribute.atttypid, and there doesn't
> seem to be enough information left in pg_attribute to guess the typid
> that, combined with atttypmod, would restore the proper values for
> pg_attribute.atttypid and pg_attribute.attalign. Therefore, I just
> brute-forced an UPDATE into dump to set the values properly after
> dropping the fake TEXT column.
>
> I did a minimal documentation addition by adding something to the
> "Notes" section of the manual pages.
>
> Here is what a dump of a table with dropped columns looks like:
>
> --
> -- Name: test; Type: TABLE; Schema: public; Owner: postgres; Tablespace:
> --
>
> CREATE TABLE test (
> x integer,
> "........pg.dropped.2........" TEXT
> );
> ALTER TABLE ONLY test DROP COLUMN "........pg.dropped.2........";
>
> -- For binary upgrade, recreate dropped column's length and alignment.
> UPDATE pg_attribute
> SET attlen = -1, attalign = 'i'
> WHERE attname = '........pg.dropped.2........'
> AND attrelid =
> (
> SELECT oid
> FROM pg_class
> WHERE relnamespace = (SELECT oid FROM pg_namespace WHERE nspname = CURRENT_SCHEMA)
> AND relname = 'test'
> );
>
> ALTER TABLE public.test OWNER TO postgres;
>
> --
> Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
> EnterpriseDB http://enterprisedb.com
>
> + If your life is a hard drive, Christ can be your backup. +

[ text/x-diff is unsupported, treating like TEXT/PLAIN ]

> Index: doc/src/sgml/ref/pg_dump.sgml
> ===================================================================
> RCS file: /cvsroot/pgsql/doc/src/sgml/ref/pg_dump.sgml,v
> retrieving revision 1.109
> diff -c -c -r1.109 pg_dump.sgml
> *** doc/src/sgml/ref/pg_dump.sgml 10 Feb 2009 00:55:21 -0000 1.109
> --- doc/src/sgml/ref/pg_dump.sgml 17 Feb 2009 01:57:10 -0000
> ***************
> *** 827,832 ****
> --- 827,837 ----
> editing of the dump file might be required.
> </para>
>
> + <para>
> + <application>pg_dump</application> also supports a
> + <literal>--binary-upgrade</> option for upgrade utility usage.
> + </para>
> +
> </refsect1>
>
> <refsect1 id="pg-dump-examples">
> Index: doc/src/sgml/ref/pg_dumpall.sgml
> ===================================================================
> RCS file: /cvsroot/pgsql/doc/src/sgml/ref/pg_dumpall.sgml,v
> retrieving revision 1.75
> diff -c -c -r1.75 pg_dumpall.sgml
> *** doc/src/sgml/ref/pg_dumpall.sgml 7 Feb 2009 14:31:30 -0000 1.75
> --- doc/src/sgml/ref/pg_dumpall.sgml 17 Feb 2009 01:57:10 -0000
> ***************
> *** 489,494 ****
> --- 489,499 ----
> locations.
> </para>
>
> + <para>
> + <application>pg_dump</application> also supports a
> + <literal>--binary-upgrade</> option for upgrade utility usage.
> + </para>
> +
> </refsect1>
>
>
> Index: src/bin/pg_dump/pg_dump.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/bin/pg_dump/pg_dump.c,v
> retrieving revision 1.521
> diff -c -c -r1.521 pg_dump.c
> *** src/bin/pg_dump/pg_dump.c 16 Feb 2009 23:06:55 -0000 1.521
> --- src/bin/pg_dump/pg_dump.c 17 Feb 2009 01:57:10 -0000
> ***************
> *** 99,104 ****
> --- 99,106 ----
> /* default, if no "inclusion" switches appear, is to dump everything */
> static bool include_everything = true;
>
> + static int binary_upgrade = 0;
> +
> char g_opaque_type[10]; /* name for the opaque type */
>
> /* placeholders for the delimiters for comments */
> ***************
> *** 236,242 ****
> static int outputNoTablespaces = 0;
> static int use_setsessauth = 0;
>
> ! static struct option long_options[] = {
> {"data-only", no_argument, NULL, 'a'},
> {"blobs", no_argument, NULL, 'b'},
> {"clean", no_argument, NULL, 'c'},
> --- 238,245 ----
> static int outputNoTablespaces = 0;
> static int use_setsessauth = 0;
>
> ! struct option long_options[] = {
> ! {"binary-upgrade", no_argument, &binary_upgrade, 1}, /* not documented */
> {"data-only", no_argument, NULL, 'a'},
> {"blobs", no_argument, NULL, 'b'},
> {"clean", no_argument, NULL, 'c'},
> ***************
> *** 4611,4616 ****
> --- 4614,4621 ----
> int i_attnotnull;
> int i_atthasdef;
> int i_attisdropped;
> + int i_attlen;
> + int i_attalign;
> int i_attislocal;
> PGresult *res;
> int ntups;
> ***************
> *** 4655,4661 ****
> appendPQExpBuffer(q, "SELECT a.attnum, a.attname, a.atttypmod, "
> "a.attstattarget, a.attstorage, t.typstorage, "
> "a.attnotnull, a.atthasdef, a.attisdropped, "
> ! "a.attislocal, "
> "pg_catalog.format_type(t.oid,a.atttypmod) AS atttypname "
> "FROM pg_catalog.pg_attribute a LEFT JOIN pg_catalog.pg_type t "
> "ON a.atttypid = t.oid "
> --- 4660,4666 ----
> appendPQExpBuffer(q, "SELECT a.attnum, a.attname, a.atttypmod, "
> "a.attstattarget, a.attstorage, t.typstorage, "
> "a.attnotnull, a.atthasdef, a.attisdropped, "
> ! "a.attlen, a.attalign, a.attislocal, "
> "pg_catalog.format_type(t.oid,a.atttypmod) AS atttypname "
> "FROM pg_catalog.pg_attribute a LEFT JOIN pg_catalog.pg_type t "
> "ON a.atttypid = t.oid "
> ***************
> *** 4674,4680 ****
> appendPQExpBuffer(q, "SELECT a.attnum, a.attname, "
> "a.atttypmod, -1 AS attstattarget, a.attstorage, "
> "t.typstorage, a.attnotnull, a.atthasdef, "
> ! "false AS attisdropped, false AS attislocal, "
> "format_type(t.oid,a.atttypmod) AS atttypname "
> "FROM pg_attribute a LEFT JOIN pg_type t "
> "ON a.atttypid = t.oid "
> --- 4679,4686 ----
> appendPQExpBuffer(q, "SELECT a.attnum, a.attname, "
> "a.atttypmod, -1 AS attstattarget, a.attstorage, "
> "t.typstorage, a.attnotnull, a.atthasdef, "
> ! "false AS attisdropped, 0 AS attlen, "
> ! "' ' AS attalign, false AS attislocal, "
> "format_type(t.oid,a.atttypmod) AS atttypname "
> "FROM pg_attribute a LEFT JOIN pg_type t "
> "ON a.atttypid = t.oid "
> ***************
> *** 4690,4696 ****
> "-1 AS attstattarget, attstorage, "
> "attstorage AS typstorage, "
> "attnotnull, atthasdef, false AS attisdropped, "
> ! "false AS attislocal, "
> "(SELECT typname FROM pg_type WHERE oid = atttypid) AS atttypname "
> "FROM pg_attribute a "
> "WHERE attrelid = '%u'::oid "
> --- 4696,4703 ----
> "-1 AS attstattarget, attstorage, "
> "attstorage AS typstorage, "
> "attnotnull, atthasdef, false AS attisdropped, "
> ! "0 AS attlen, ' ' AS attalign, "
> ! "false AS attislocal, "
> "(SELECT typname FROM pg_type WHERE oid = atttypid) AS atttypname "
> "FROM pg_attribute a "
> "WHERE attrelid = '%u'::oid "
> ***************
> *** 4714,4719 ****
> --- 4721,4728 ----
> i_attnotnull = PQfnumber(res, "attnotnull");
> i_atthasdef = PQfnumber(res, "atthasdef");
> i_attisdropped = PQfnumber(res, "attisdropped");
> + i_attlen = PQfnumber(res, "attlen");
> + i_attalign = PQfnumber(res, "attalign");
> i_attislocal = PQfnumber(res, "attislocal");
>
> tbinfo->numatts = ntups;
> ***************
> *** 4724,4729 ****
> --- 4733,4740 ----
> tbinfo->attstorage = (char *) malloc(ntups * sizeof(char));
> tbinfo->typstorage = (char *) malloc(ntups * sizeof(char));
> tbinfo->attisdropped = (bool *) malloc(ntups * sizeof(bool));
> + tbinfo->attlen = (int *) malloc(ntups * sizeof(int));
> + tbinfo->attalign = (char *) malloc(ntups * sizeof(char));
> tbinfo->attislocal = (bool *) malloc(ntups * sizeof(bool));
> tbinfo->notnull = (bool *) malloc(ntups * sizeof(bool));
> tbinfo->attrdefs = (AttrDefInfo **) malloc(ntups * sizeof(AttrDefInfo *));
> ***************
> *** 4747,4752 ****
> --- 4758,4765 ----
> tbinfo->attstorage[j] = *(PQgetvalue(res, j, i_attstorage));
> tbinfo->typstorage[j] = *(PQgetvalue(res, j, i_typstorage));
> tbinfo->attisdropped[j] = (PQgetvalue(res, j, i_attisdropped)[0] == 't');
> + tbinfo->attlen[j] = atoi(PQgetvalue(res, j, i_attlen));
> + tbinfo->attalign[j] = *(PQgetvalue(res, j, i_attalign));
> tbinfo->attislocal[j] = (PQgetvalue(res, j, i_attislocal)[0] == 't');
> tbinfo->notnull[j] = (PQgetvalue(res, j, i_attnotnull)[0] == 't');
> tbinfo->attrdefs[j] = NULL; /* fix below */
> ***************
> *** 4760,4765 ****
> --- 4773,4793 ----
>
> PQclear(res);
>
> +
> + /*
> + * ALTER TABLE DROP COLUMN clears pg_attribute.atttypid, so we
> + * set the column data type to 'TEXT; we will later drop the
> + * column.
> + */
> + if (binary_upgrade)
> + {
> + for (j = 0; j < ntups; j++)
> + {
> + if (tbinfo->attisdropped[j])
> + tbinfo->atttypnames[j] = strdup("TEXT");
> + }
> + }
> +
> /*
> * Get info about column defaults
> */
> ***************
> *** 9680,9686 ****
> for (j = 0; j < tbinfo->numatts; j++)
> {
> /* Is this one of the table's own attrs, and not dropped ? */
> ! if (!tbinfo->inhAttrs[j] && !tbinfo->attisdropped[j])
> {
> /* Format properly if not first attr */
> if (actual_atts > 0)
> --- 9708,9715 ----
> for (j = 0; j < tbinfo->numatts; j++)
> {
> /* Is this one of the table's own attrs, and not dropped ? */
> ! if (!tbinfo->inhAttrs[j] &&
> ! (!tbinfo->attisdropped[j] || binary_upgrade))
> {
> /* Format properly if not first attr */
> if (actual_atts > 0)
> ***************
> *** 9786,9791 ****
> --- 9815,9867 ----
>
> appendPQExpBuffer(q, ";\n");
>
> + /*
> + * For binary-compatible heap files, we create dropped columns
> + * above and drop them here.
> + */
> + if (binary_upgrade)
> + {
> + for (j = 0; j < tbinfo->numatts; j++)
> + {
> + if (tbinfo->attisdropped[j])
> + {
> + appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
> + fmtId(tbinfo->dobj.name));
> + appendPQExpBuffer(q, "DROP COLUMN %s;\n",
> + fmtId(tbinfo->attnames[j]));
> +
> + /*
> + * ALTER TABLE DROP COLUMN clears pg_attribute.atttypid,
> + * so we have to set pg_attribute.attlen and
> + * pg_attribute.attalign values because that is what
> + * is used to skip over dropped columns in the heap tuples.
> + * We have atttypmod, but it seems impossible to know the
> + * correct data type that will yield pg_attribute values
> + * that match the old installation.
> + * See comment in backend/catalog/heap.c::RemoveAttributeById()
> + */
> + appendPQExpBuffer(q, "\n-- For binary upgrade, recreate dropped column's length and alignment.\n");
> + appendPQExpBuffer(q, "UPDATE pg_attribute\n"
> + "SET attlen = %d, "
> + "attalign = '%c'\n"
> + "WHERE attname = '%s'\n"
> + " AND attrelid = \n"
> + " (\n"
> + " SELECT oid\n"
> + " FROM pg_class\n"
> + " WHERE relnamespace = "
> + "(SELECT oid FROM pg_namespace "
> + "WHERE nspname = CURRENT_SCHEMA)\n"
> + " AND relname = '%s'\n"
> + " );",
> + tbinfo->attlen[j],
> + tbinfo->attalign[j],
> + tbinfo->attnames[j],
> + tbinfo->dobj.name);
> + }
> + }
> + }
> +
> /* Loop dumping statistics and storage statements */
> for (j = 0; j < tbinfo->numatts; j++)
> {
> Index: src/bin/pg_dump/pg_dump.h
> ===================================================================
> RCS file: /cvsroot/pgsql/src/bin/pg_dump/pg_dump.h,v
> retrieving revision 1.150
> diff -c -c -r1.150 pg_dump.h
> *** src/bin/pg_dump/pg_dump.h 2 Feb 2009 19:31:39 -0000 1.150
> --- src/bin/pg_dump/pg_dump.h 17 Feb 2009 01:57:10 -0000
> ***************
> *** 245,250 ****
> --- 245,252 ----
> char *attstorage; /* attribute storage scheme */
> char *typstorage; /* type storage scheme */
> bool *attisdropped; /* true if attr is dropped; don't dump it */
> + int *attlen; /* attribute length, used by binary_upgrade */
> + char *attalign; /* attribute align, used by binary_upgrade */
> bool *attislocal; /* true if attr has local definition */
>
> /*
> Index: src/bin/pg_dump/pg_dumpall.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/bin/pg_dump/pg_dumpall.c,v
> retrieving revision 1.113
> diff -c -c -r1.113 pg_dumpall.c
> *** src/bin/pg_dump/pg_dumpall.c 22 Jan 2009 20:16:08 -0000 1.113
> --- src/bin/pg_dump/pg_dumpall.c 17 Feb 2009 01:57:10 -0000
> ***************
> *** 90,97 ****
> const char *std_strings;
> int c,
> ret;
>
> ! static struct option long_options[] = {
> {"data-only", no_argument, NULL, 'a'},
> {"clean", no_argument, NULL, 'c'},
> {"inserts", no_argument, NULL, 'd'},
> --- 90,99 ----
> const char *std_strings;
> int c,
> ret;
> + int binary_upgrade = 0;
>
> ! struct option long_options[] = {
> ! {"binary-upgrade", no_argument, &binary_upgrade, 1}, /* not documented */
> {"data-only", no_argument, NULL, 'a'},
> {"clean", no_argument, NULL, 'c'},
> {"inserts", no_argument, NULL, 'd'},
> ***************
> *** 310,315 ****
> --- 312,319 ----
> }
>
> /* Add long options to the pg_dump argument list */
> + if (binary_upgrade)
> + appendPQExpBuffer(pgdumpopts, " --binary-upgrade");
> if (disable_dollar_quoting)
> appendPQExpBuffer(pgdumpopts, " --disable-dollar-quoting");
> if (disable_triggers)

>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +