Обсуждение: Suggested fix for pg_dump
The problem with pg_dump and renamed primary key attrs can be fixed by using the indkey attribute of pg_index to lookup attrs in pg_class - this is what pg_dump does when it dumps indexes. If I am going to make this change, I would also like to split the PK definition out from the table definition, so we would have: Create Table fred(f1 int); Alter Table fred add constraint primary key(f1); instead of: Create Table fred(f1 int, primary key(f1)); The reason for this is that it will improve load performance, and begin to allow pg_restore to separate constraints from tables. Is this OK? Or inappropriate for beta? Needless to say, I don't really want to revisit this piece of code in 3 months time, so I'd like to do it now. ---------------------------------------------------------------- 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 Sun, 7 Jan 2001, Philip Warner wrote: > > The problem with pg_dump and renamed primary key attrs can be fixed by > using the indkey attribute of pg_index to lookup attrs in pg_class - this > is what pg_dump does when it dumps indexes. If I am going to make this > change, I would also like to split the PK definition out from the table > definition, so we would have: > > Create Table fred(f1 int); > Alter Table fred add constraint primary key(f1); > > instead of: > > Create Table fred(f1 int, primary key(f1)); > > The reason for this is that it will improve load performance, and begin to > allow pg_restore to separate constraints from tables. > > Is this OK? Or inappropriate for beta? > > Needless to say, I don't really want to revisit this piece of code in 3 > months time, so I'd like to do it now. From Tatsuo's example, it looks critical enough that it should be fixed before release, and since its a 'support program' issue, not a 'core server' issue, ramifications of fixing it aren't as big as if it was a 'core server' issue ... go for it
The Hermit Hacker <scrappy@hub.org> writes:
> On Sun, 7 Jan 2001, Philip Warner wrote:
>> Is this OK? Or inappropriate for beta?
> From Tatsuo's example, it looks critical enough that it should be fixed
> before release, and since its a 'support program' issue, not a 'core
> server' issue, ramifications of fixing it aren't as big as if it was a
> 'core server' issue ... go for it
I concur. This is not a new feature, but a bug fix, and therefore it's
appropriate to do it during beta. We don't require beta-period bug
fixes to be the smallest possible change that cures the problem. They
should be good fixes if practical.
One issue however is how confident are we of the alter table add
constraint code? I'm not sure it's been exercised enough to justify
making pg_dump rely on it ... is anyone willing to spend some time
testing that statement?
regards, tom lane
On Sun, 7 Jan 2001, Tom Lane wrote: > The Hermit Hacker <scrappy@hub.org> writes: > > On Sun, 7 Jan 2001, Philip Warner wrote: > >> Is this OK? Or inappropriate for beta? > > > From Tatsuo's example, it looks critical enough that it should be fixed > > before release, and since its a 'support program' issue, not a 'core > > server' issue, ramifications of fixing it aren't as big as if it was a > > 'core server' issue ... go for it > > I concur. This is not a new feature, but a bug fix, and therefore it's > appropriate to do it during beta. We don't require beta-period bug > fixes to be the smallest possible change that cures the problem. They > should be good fixes if practical. > > One issue however is how confident are we of the alter table add > constraint code? I'm not sure it's been exercised enough to justify > making pg_dump rely on it ... is anyone willing to spend some time > testing that statement? Since its obvious that pg_dump isn't working now, we wouldn't be breaking it any further if the constraint code has a problem with it ... and we should be able to find out relatively quickly *if* the contraint code has a problem if its used for something like this ... Essentially, worst case scenario, we are going from 'broken->broken' ...
The Hermit Hacker <scrappy@hub.org> writes:
> Essentially, worst case scenario, we are going from 'broken->broken' ...
No, I don't think so. The current pg_dump code is only broken if
you've renamed a column involved in a foreign-key dependency (if I
understood the thread correctly). But Philip is proposing to change
pg_dump to rely on alter table add constraint for *all* PRIMARY KEY
constructs. So if alter table add constraint fails, it could break
cases that had nothing to do with either foreign keys or renamed
columns.
I'm not really arguing not to make the change. I am saying there's
an area here that we'd better take care to test during beta cycle...
regards, tom lane
On Sun, 7 Jan 2001, Tom Lane wrote: > The Hermit Hacker <scrappy@hub.org> writes: > > Essentially, worst case scenario, we are going from 'broken->broken' ... > > No, I don't think so. The current pg_dump code is only broken if > you've renamed a column involved in a foreign-key dependency (if I > understood the thread correctly). But Philip is proposing to change > pg_dump to rely on alter table add constraint for *all* PRIMARY KEY > constructs. So if alter table add constraint fails, it could break > cases that had nothing to do with either foreign keys or renamed > columns. > > I'm not really arguing not to make the change. I am saying there's > an area here that we'd better take care to test during beta cycle... Agreed ... we almost need a regression test for pg_dump itself :)
> Create Table fred(f1 int); > Alter Table fred add constraint primary key(f1); Has support for the above statement (add constraint PK) been added in 7.1? If so, then what other alter table add constraints have been added? CHECK? NOT NULL? Thanks, Chris
At 13:29 7/01/01 -0500, Tom Lane wrote: >The Hermit Hacker <scrappy@hub.org> writes: >> Essentially, worst case scenario, we are going from 'broken->broken' ... > >No, I don't think so. The current pg_dump code is only broken if >you've renamed a column involved in a foreign-key dependency (if I >understood the thread correctly). But Philip is proposing to change >pg_dump to rely on alter table add constraint for *all* PRIMARY KEY >constructs. I've got a version of this working now, but it raises a further question: should I also put the CHECK constraints into ALTER TABLE statements? Eventually they should go there, but maybe not for 7.1 given the concerns over ALTER TABLE ADD CONSTRAINT. Reasons for: - It's nicer - It's more consistent with PK - Can use pg_restore to restore tables without constraints Reasons against: - ALTER TABLE ADD CONSTRAINT is untested - We're in beta, and the code ain't broke I suspect the last point says it all... ---------------------------------------------------------------- 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 |/
At 00:03 13/01/01 +1100, Philip Warner wrote: >At 13:29 7/01/01 -0500, Tom Lane wrote: >>The Hermit Hacker <scrappy@hub.org> writes: >>> Essentially, worst case scenario, we are going from 'broken->broken' ... >> >>No, I don't think so. The current pg_dump code is only broken if >>you've renamed a column involved in a foreign-key dependency (if I >>understood the thread correctly). But Philip is proposing to change >>pg_dump to rely on alter table add constraint for *all* PRIMARY KEY >>constructs. > >I've got a version of this working now, but it raises a further question: Apart from the fact that ALTER TABLE ADD CONSTRAINT name PRIMARY KEY(fields); is not supported in 7.1. Oh well. ---------------------------------------------------------------- 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 |/