Re: [HACKERS] 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 br...@momjian.ushttp://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 - 1.109 --- doc/src/sgml/ref/pg_dump.sgml 17 Feb 2009 01:57:10 - *** *** 827,832 --- 827,837 editing of the dump file might be required. /para + para +applicationpg_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 - 1.75 --- doc/src/sgml/ref/pg_dumpall.sgml 17 Feb 2009 01:57:10 - *** *** 489,494 --- 489,499 locations. /para + para +applicationpg_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 - 1.521 --- src/bin/pg_dump/pg_dump.c 17 Feb 2009 01:57:10 - *** *** 99,104 --- 99,106 /* default, if no inclusion switches
Re: [HACKERS] pg_migrator and handling dropped columns
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + 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 - 1.109 --- doc/src/sgml/ref/pg_dump.sgml 17 Feb 2009 01:57:10 - *** *** 827,832 --- 827,837 editing of the dump file might be required. /para + para +applicationpg_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 - 1.75 --- doc/src/sgml/ref/pg_dumpall.sgml 17 Feb 2009 01:57:10 - *** *** 489,494 --- 489,499 locations. /para + para +applicationpg_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 - 1.521 --- src/bin/pg_dump/pg_dump.c 17 Feb 2009 01:57:10 - *** *** 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
Re: [HACKERS] pg_migrator and handling dropped columns
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_migrator and handling dropped columns
bruce wrote: Peter Eisentraut wrote: Bruce Momjian wrote: Now that pg_migrator is BSD licensed, and already in C, I am going to spend my time trying to improve pg_migrator for 8.4: http://pgfoundry.org/projects/pg-migrator/ What is the plan now? Get pg_upgrade working, get pg_migrator working, ship pg_migrator in core or separately? Is there any essential functionality that we need to get into the server code before release? Should we try to get dropped columns working? It's quite late to be I have thought about how to handle dumped columns and would like to get some feedback on this. It is easy to find the dropped columns with 'pg_attribute.attisdropped = true'. The basic problem is that dropped columns do not appear in the pg_dump output schema, but still exist in the data files. While the missing data is not a problem, the dropped column's existence affects all subsequent columns, increasing their attno values and their placement in the data files. I can think of three possible solutions, all involve recreating and dropping the dropped column in the new schema: 1 modify the pg_dumpall --schema-only output file before loading to add the dropped column 2 drop/recreate the table after loading to add the dropped column 3 modify the system tables directly to add the dropped column, perhaps using pg_depend information #1 seems like the best option, though it requires parsing the pg_dump file to some extent. #2 is a problem because dropping/recreating the table might be difficult because of foreign key relationships, even for empty tables. #3 seems prone to maintenance requirements every time we change system object relationships. Once the dropped column is created in the new server, it can be dropped to match the incoming data files. Comments? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_migrator and handling dropped columns
Bruce Momjian br...@momjian.us writes: I can think of three possible solutions, all involve recreating and dropping the dropped column in the new schema: (4) add a switch to pg_dump to include dropped columns in its schema output and then drop them. This seems far more maintainable than writing separate code that tries to parse the output. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_migrator and handling dropped columns
Tom Lane wrote: Bruce Momjian br...@momjian.us writes: I can think of three possible solutions, all involve recreating and dropping the dropped column in the new schema: (4) add a switch to pg_dump to include dropped columns in its schema output and then drop them. This seems far more maintainable than writing separate code that tries to parse the output. That would certainly be the easiest. I was going to have trouble generating the exact column creation string anyway in pg_migrator. I assume I would also drop the column in the pg_dump output. Is this acceptable to everyone? We could name the option -u/--upgrade-compatible. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_migrator and handling dropped columns
Bruce Momjian br...@momjian.us writes: Tom Lane wrote: (4) add a switch to pg_dump to include dropped columns in its schema output and then drop them. This seems far more maintainable than writing separate code that tries to parse the output. I assume I would also drop the column in the pg_dump output. Right, that's what I meant --- do all the work within pg_dump. 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. And probably not even list it in the user documentation. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_migrator and handling dropped columns
On Thu, 2009-02-12 at 13:39 -0500, Tom Lane wrote: Right, that's what I meant --- do all the work within pg_dump. 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. And probably not even list it in the user documentation. Why wouldn't we want to list it? Joshua D. Drake regards, tom lane -- PostgreSQL - XMPP: jdr...@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_migrator and handling dropped columns
Tom Lane wrote: Bruce Momjian br...@momjian.us writes: Tom Lane wrote: (4) add a switch to pg_dump to include dropped columns in its schema output and then drop them. This seems far more maintainable than writing separate code that tries to parse the output. I assume I would also drop the column in the pg_dump output. Right, that's what I meant --- do all the work within pg_dump. 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. And probably not even list it in the user documentation. OK, works for me; any objections from anyone? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_migrator and handling dropped columns
Joshua D. Drake wrote: On Thu, 2009-02-12 at 13:39 -0500, Tom Lane wrote: Right, that's what I meant --- do all the work within pg_dump. 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. And probably not even list it in the user documentation. Why wouldn't we want to list it? Because it is for internal use by upgrade utilities, not for user use. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_migrator and handling dropped columns
Joshua D. Drake j...@commandprompt.com writes: On Thu, 2009-02-12 at 13:39 -0500, Tom Lane wrote: a long form only. And probably not even list it in the user documentation. Why wouldn't we want to list it? Because it's for internal use only. Although the effect we're discussing here is relatively harmless, it seems possible that further down the road we might find a need for hacks that would render the output entirely unfit for ordinary dump purposes. I don't see a need to encourage people to play with fire. It's hardly unprecedented for us to have undocumented internal options --- there are some in postgres.c for example. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_migrator and handling dropped columns
Tom Lane wrote: Joshua D. Drake j...@commandprompt.com writes: On Thu, 2009-02-12 at 13:39 -0500, Tom Lane wrote: a long form only. And probably not even list it in the user documentation. Why wouldn't we want to list it? Because it's for internal use only. Although the effect we're discussing here is relatively harmless, it seems possible that further down the road we might find a need for hacks that would render the output entirely unfit for ordinary dump purposes. I don't see a need to encourage people to play with fire. It's hardly unprecedented for us to have undocumented internal options --- there are some in postgres.c for example. The important point is that we add comments in the source code about why it is undocumented. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
