Thread: pg_dump: use ALTER TABLE for PKs
I've attached a patch which enhances pg_dump to use ALTER TABLE when defining the primary keys of a table (instead of including this information in the table definition). This follows a suggestion by Christopher Kings-Lynne (good idea!). The patch is pretty simple, partly because the code was already there, just commented out ;-) I fixed a typo and added some comments. Bruce suggested that this should get some code review, since a bug in pg_dump would be bad news. Any comments would be welcome. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Attachment
On Mon, 2002-02-18 at 22:24, Bruce Momjian wrote: > Neil Conway wrote: > > Bruce suggested that this should get some code review, since a bug in > > pg_dump would be bad news. Any comments would be welcome. > > The issue was that the author wants it in 7.2.1. I'm confused: which author? Where does this information come from? > I question whether a > purely performance pg_dump patch belongs in a minor release. I will let > others decide. I agree. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
On Mon, 2002-02-18 at 22:35, Bruce Momjian wrote: > Neil Conway wrote: > > I'm confused: which author? Where does this information come from? > > I thought you were the author. I am ;-) > Oh, I thought you wanted this in 7.2.1. Miscommunication, I think -- in a private email to you, I wrote "I don't think this is appropriate for 7.2.1, in any case". ;-) > it just needs the normal review, not extra special review. :-) Okay. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
> > The issue was that the author wants it in 7.2.1. > > I'm confused: which author? Where does this information come from? I certainly didn't propose that it appear in 7.2.1 - I was thinking 7.3. > > I question whether a > > purely performance pg_dump patch belongs in a minor release. I will let > > others decide. It sure shouldn't... Chris
Neil Conway writes: > I've attached a patch which enhances pg_dump to use ALTER TABLE when > defining the primary keys of a table (instead of including this > information in the table definition). I think you've got the "7.2 or later" theme in your patch mixed up. The version number you're evaluating is the version of the database you're dumping. The feature you're adding is dependent on the version of the database you're restoring into. The version of the database you're restoring into is always >= the pg_dump version. Thus, you don't need any version checks. Plus, I think this patch is going into 7.3, no? -- Peter Eisentraut peter_e@gmx.net
I tried going patch <pg_dump_pk.patch in the root dir of latest pgsql cvs and got this: Hmm... Looks like a context diff to me... The text leading up to this was: -------------------------- |*** ./src/bin/pg_dump/pg_dump.c.orig Mon Feb 18 20:17:59 2002 |--- ./src/bin/pg_dump/pg_dump.c Mon Feb 18 21:16:37 2002 -------------------------- Patching file ./src/bin/pg_dump/pg_dump.c using Plan A... Hunk #1 failed at 4251. Hunk #2 failed at 4261. (Fascinating--this is really a new-style context diff but without the telltale extra asterisks on the *** line that usually indicate the new style...) Hunk #3 failed at 4454. 3 out of 3 hunks failed--saving rejects to ./src/bin/pg_dump/pg_dump.c.rej done How should I apply this patch? Chris > -----Original Message----- > From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org]On Behalf Of Neil Conway > Sent: Tuesday, 19 February 2002 10:26 AM > To: pgsql-hackers@postgresql.org > Subject: [HACKERS] pg_dump: use ALTER TABLE for PKs > > > I've attached a patch which enhances pg_dump to use ALTER TABLE when > defining the primary keys of a table (instead of including this > information in the table definition). > > This follows a suggestion by Christopher Kings-Lynne (good idea!). > > The patch is pretty simple, partly because the code was already there, > just commented out ;-) I fixed a typo and added some comments. > > Bruce suggested that this should get some code review, since a bug in > pg_dump would be bad news. Any comments would be welcome. > > Cheers, > > Neil > > -- > Neil Conway <neilconway@rogers.com> > PGP Key ID: DB3C29FC >
On Mon, 2002-02-18 at 22:46, Peter Eisentraut wrote: > Neil Conway writes: > > > I've attached a patch which enhances pg_dump to use ALTER TABLE when > > defining the primary keys of a table (instead of including this > > information in the table definition). > > I think you've got the "7.2 or later" theme in your patch mixed up. The > version number you're evaluating is the version of the database you're > dumping. The feature you're adding is dependent on the version of the > database you're restoring into. The version of the database you're > restoring into is always >= the pg_dump version. Thus, you don't need any > version checks. You're absolutely correct -- thanks for spotting that. A new version of the patch is attached. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Attachment
At 21:26 18/02/02 -0500, Neil Conway wrote: >The patch is pretty simple, partly because the code was already there, >just commented out ;-) I fixed a typo and added some comments. When I originally wrote the commented-out code, we did not have the ability to make 'dependant' TOC Entries, so it uses the OID of the index as the PK OID in the ArchiveEntry call. I can't really think of a case where the index OID would be less than the table OID, but it might be good to put the table OID in the deps list; this will force the PK to go after the table in all cases. Did I mention I'm paranoid? It's also a first step in the ability to dump a 'whole table' Vs. just a table definition - which we will need when all constraints can be dumped via 'ALTER TABLE'. For clarity, 'whole table' might mean the attrs, indexes, constraints, triggers etc. ---------------------------------------------------------------- Philip Warner | __---_____ Albatross Consulting Pty. Ltd. |----/ - \ (A.B.N. 75 008 659 498) | /(@) ______---_ Tel: (+61) 0500 83 82 81 | _________ \ Fax: (+61) 0500 83 82 82 | ___________ | Http://www.rhyme.com.au | / \| | --________-- PGP key available upon request, | / and from pgp5.ai.mit.edu:11371 |/
> >For clarity, 'whole table' might mean the attrs, indexes, constraints, >triggers etc. > Sorry, this should probably be limited to anything that can go into a 'CREATE TABLE' statement, so triggers & indexes would not be included - but constraints, default values etc would be there. ---------------------------------------------------------------- Philip Warner | __---_____ Albatross Consulting Pty. Ltd. |----/ - \ (A.B.N. 75 008 659 498) | /(@) ______---_ Tel: (+61) 0500 83 82 81 | _________ \ Fax: (+61) 0500 83 82 82 | ___________ | Http://www.rhyme.com.au | / \| | --________-- PGP key available upon request, | / and from pgp5.ai.mit.edu:11371 |/
Philip Warner <pjw@rhyme.com.au> writes:
> I can't really think of a case where the index OID would be less than the
> table OID, but it might be good to put the table OID in the deps list; this
> will force the PK to go after the table in all cases. Did I mention I'm
> paranoid?
After OID-counter wraparound it'd be quite easy to have such a case.
To the extent that pg_dump is depending on OID comparison to tell it
something about creation order, it's really broken and should be fixed.
The dependencies mechanism should eventually be extended to cover all
these relationships.
regards, tom lane
At 21:26 18/02/02 -0500, Neil Conway wrote: > >Bruce suggested that this should get some code review, since a bug in >pg_dump would be bad news. Any comments would be welcome. > Part of the reason I did not implement this earlier was that ALTER TABLE...PK did not handle child tables 'properly' (for some arbitrary meaning of the word 'properly'). How have you covered: CREATE TABLE PARENT(F1 INT PRIMARY KEY); CREATE TABLE CHILD(...) INHERIT PARENT this should create a PK on CHILD; what does pg-dump and the ALTER TABLE implementation do? Not sure how it should work, but ultimately we need to put the PK (and, necessarily, FK) creation at the end of the data load: CREATE TABLE PARENT(F1 INT); CREATE TABLE CHILD(...) INHERIT PARENT Load PARENT Load CHILD ALTER TABLE PARENT...PK ALTER TABLE CHILD...PK This is a non-trivial issue since we also need to cater for: CREATE TABLE PARENT(F1 INT); CREATE TABLE CHILD1(...) INHERIT PARENT ALTER TABLE PARENT...PK CREATE TABLE CHILD2(...)INHERIT PARENT Which, at least historically, resulted in CHILD1 *not* having a PK. I would have built CVS and checked myself, but I'm relocating office (and servers) at the moment. ---------------------------------------------------------------- Philip Warner | __---_____ Albatross Consulting Pty. Ltd. |----/ - \ (A.B.N. 75 008 659 498) | /(@) ______---_ Tel: (+61) 0500 83 82 81 | _________ \ Fax: (+61) 0500 83 82 82 | ___________ | Http://www.rhyme.com.au | / \| | --________-- PGP key available upon request, | / and from pgp5.ai.mit.edu:11371 |/
On Wed, 20 Feb 2002, Philip Warner wrote: > At 21:26 18/02/02 -0500, Neil Conway wrote: > > > >Bruce suggested that this should get some code review, since a bug in > >pg_dump would be bad news. Any comments would be welcome. > > > > Part of the reason I did not implement this earlier was that ALTER > TABLE...PK did not handle child tables 'properly' (for some arbitrary > meaning of the word 'properly'). How have you covered: > > CREATE TABLE PARENT(F1 INT PRIMARY KEY); > CREATE TABLE CHILD(...) INHERIT PARENT > > this should create a PK on CHILD; what does pg-dump and the ALTER TABLE > implementation do? Not sure how it should work, but ultimately we need to Unless it was changed between rc2 and release, the above type of sequence does not end up with a primary key on child(f1).
At 13:07 19/02/02 -0800, Stephan Szabo wrote: >> >> CREATE TABLE PARENT(F1 INT PRIMARY KEY); >> CREATE TABLE CHILD(...) INHERIT PARENT >> >> this should create a PK on CHILD; what does pg-dump and the ALTER TABLE >> implementation do? Not sure how it should work, but ultimately we need to > >Unless it was changed between rc2 and release, the above type of >sequence does not end up with a primary key on child(f1). Interesting, - that makes pg_dumps job easier. Are any constraints ever inherited? ---------------------------------------------------------------- Philip Warner | __---_____ Albatross Consulting Pty. Ltd. |----/ - \ (A.B.N. 75 008 659 498) | /(@) ______---_ Tel: (+61) 0500 83 82 81 | _________ \ Fax: (+61) 0500 83 82 82 | ___________ | Http://www.rhyme.com.au | / \| | --________-- PGP key available upon request, | / and from pgp5.ai.mit.edu:11371 |/
On Wed, 20 Feb 2002, Philip Warner wrote: > At 13:07 19/02/02 -0800, Stephan Szabo wrote: > >> > >> CREATE TABLE PARENT(F1 INT PRIMARY KEY); > >> CREATE TABLE CHILD(...) INHERIT PARENT > >> > >> this should create a PK on CHILD; what does pg-dump and the ALTER TABLE > >> implementation do? Not sure how it should work, but ultimately we need to > > > >Unless it was changed between rc2 and release, the above type of > >sequence does not end up with a primary key on child(f1). > > Interesting, - that makes pg_dumps job easier. Are any constraints ever > inherited? Check constraints and not null I believe are inherited by default. But I just thought of a case that won't dump and restore the same as it was originally made because we support ONLY on alter table add constraint. create table z(a int); create table z2() inherits(z); alter table only z add constraint foo1 check(a>4); will make z have a constraint on a but not z2 and the check will get dumped as part of z's definition which will restore with z2 having the check.
At 14:05 19/02/02 -0800, Stephan Szabo wrote: > >Check constraints and not null I believe are inherited by default. >But I just thought of a case that won't dump and restore the same as it >was originally made because we support ONLY on alter table add constraint. > >create table z(a int); >create table z2() inherits(z); >alter table only z add constraint foo1 check(a>4); > >will make z have a constraint on a but not z2 and the check will get >dumped as part of z's definition which will restore with z2 having >the check. This brings to mind a couple of questions: 1. Do you have plans to support inhertiance of PK constraints? 2. Does/will DROP CONSTRAINT support ONLY? ---------------------------------------------------------------- Philip Warner | __---_____ Albatross Consulting Pty. Ltd. |----/ - \ (A.B.N. 75 008 659 498) | /(@) ______---_ Tel: (+61) 0500 83 82 81 | _________ \ Fax: (+61) 0500 83 82 82 | ___________ | Http://www.rhyme.com.au | / \| | --________-- PGP key available upon request, | / and from pgp5.ai.mit.edu:11371 |/
The behaviour you illustrate below does not exist and therefore is not a problem: test=# create table parent(f1 int primary key); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index 'parent_pkey' for t able 'parent' CREATE test=# create table child(f2 int) inherits (parent); CREATE test=# \d parent Table "parent"Column | Type | Modifiers --------+---------+-----------f1 | integer | not null Primary key: parent_pkey test=# \d child Table "child"Column | Type | Modifiers --------+---------+-----------f1 | integer | not nullf2 | integer | Chris > -----Original Message----- > From: Philip Warner [mailto:pjw@rhyme.com.au] > Sent: Wednesday, 20 February 2002 4:53 AM > To: Neil Conway; pgsql-hackers@postgresql.org > Cc: Christopher Kings-Lynne > Subject: Re: [HACKERS] pg_dump: use ALTER TABLE for PKs > > > At 21:26 18/02/02 -0500, Neil Conway wrote: > > > >Bruce suggested that this should get some code review, since a bug in > >pg_dump would be bad news. Any comments would be welcome. > > > > Part of the reason I did not implement this earlier was that ALTER > TABLE...PK did not handle child tables 'properly' (for some arbitrary > meaning of the word 'properly'). How have you covered: > > CREATE TABLE PARENT(F1 INT PRIMARY KEY); > CREATE TABLE CHILD(...) INHERIT PARENT > > this should create a PK on CHILD; what does pg-dump and the ALTER TABLE > implementation do? Not sure how it should work, but ultimately we need to > put the PK (and, necessarily, FK) creation at the end of the data load: > > CREATE TABLE PARENT(F1 INT); > CREATE TABLE CHILD(...) INHERIT PARENT > > Load PARENT > Load CHILD > > ALTER TABLE PARENT...PK > ALTER TABLE CHILD...PK > > This is a non-trivial issue since we also need to cater for: > > CREATE TABLE PARENT(F1 INT); > CREATE TABLE CHILD1(...) INHERIT PARENT > ALTER TABLE PARENT...PK > CREATE TABLE CHILD2(...) INHERIT PARENT > > Which, at least historically, resulted in CHILD1 *not* having a > PK. I would > have built CVS and checked myself, but I'm relocating office (and servers) > at the moment. > > > ---------------------------------------------------------------- > Philip Warner | __---_____ > Albatross Consulting Pty. Ltd. |----/ - \ > (A.B.N. 75 008 659 498) | /(@) ______---_ > Tel: (+61) 0500 83 82 81 | _________ \ > Fax: (+61) 0500 83 82 82 | ___________ | > Http://www.rhyme.com.au | / \| > | --________-- > PGP key available upon request, | / > and from pgp5.ai.mit.edu:11371 |/ >
On Wed, 20 Feb 2002, Philip Warner wrote: > At 14:05 19/02/02 -0800, Stephan Szabo wrote: > > > >Check constraints and not null I believe are inherited by default. > >But I just thought of a case that won't dump and restore the same as it > >was originally made because we support ONLY on alter table add constraint. > > > >create table z(a int); > >create table z2() inherits(z); > >alter table only z add constraint foo1 check(a>4); > > > >will make z have a constraint on a but not z2 and the check will get > >dumped as part of z's definition which will restore with z2 having > >the check. > > This brings to mind a couple of questions: > > 1. Do you have plans to support inhertiance of PK constraints? If you mean me personally, not in the short term. I don't think that pk over an inheritance tree means a copy of the constraint for each table either, so it's not quite the same as some of the other constraints. > 2. Does/will DROP CONSTRAINT support ONLY? Pretty sure it does. I also think it'll let you drop an inherited constraint from a child table which can get you into the same state as the create/alter above. Does dropping inherited constraints from children or allowing constraints to be added to only the parent table make sense? I only wonder because unless you're using only on the queries you'll see the rows in the children that violate the constraint which seems kinda odd to me.
Your patch has been added to the PostgreSQL unapplied patches list at: http://candle.pha.pa.us/cgi-bin/pgpatches I will try to apply it within the next 48 hours. --------------------------------------------------------------------------- Neil Conway wrote: > On Mon, 2002-02-18 at 22:46, Peter Eisentraut wrote: > > Neil Conway writes: > > > > > I've attached a patch which enhances pg_dump to use ALTER TABLE when > > > defining the primary keys of a table (instead of including this > > > information in the table definition). > > > > I think you've got the "7.2 or later" theme in your patch mixed up. The > > version number you're evaluating is the version of the database you're > > dumping. The feature you're adding is dependent on the version of the > > database you're restoring into. The version of the database you're > > restoring into is always >= the pg_dump version. Thus, you don't need any > > version checks. > > You're absolutely correct -- thanks for spotting that. A new version of > the patch is attached. > > Cheers, > > Neil > > -- > Neil Conway <neilconway@rogers.com> > PGP Key ID: DB3C29FC [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 5: Have you checked our extensive FAQ? > > http://www.postgresql.org/users-lounge/docs/faq.html -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
One persons view, unfortunately probably shared by others http://www.open-mag.com/42444203327.htm Dave
> One persons view, unfortunately probably shared by others > http://www.open-mag.com/42444203327.htm Hmm. He has a few facts right imho; the last few sentences of the article describe exactly how PostgreSQL *does* get adopted. Actual adoption and use will precede trade press reports by several years (c.f. Linux adoption and severely lagging trade press recognition of such). Doesn't bother me a bit, and I am always amused by the bandwagoner journalists changing their stories once the cat is out of the bag. It'll happen here too ;) - Thomas
Dave Cramer wrote: > One persons view, unfortunately probably shared by others > > http://www.open-mag.com/42444203327.htm Yea, I shot this over to the general list. I found it an interesting read, and may be true for very large customers. He is correct that we are more popular with smaller customers. No question about that. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> Hmm. He has a few facts right imho; the last few sentences of the > article describe exactly how PostgreSQL *does* get adopted. Actual > adoption and use will precede trade press reports by several years (c.f. > Linux adoption and severely lagging trade press recognition of such). > > Doesn't bother me a bit, and I am always amused by the bandwagoner > journalists changing their stories once the cat is out of the bag. It'll > happen here too ;) Yes, Linux is an excellent parallel. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026