pg_dump: use ALTER TABLE for PKs

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

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

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

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

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

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 null
f2 | integer |

Chris

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, Pennsylvania 19026

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, Pennsylvania 19026

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, Pennsylvania 19026