On Wed, Nov 21, 2012 at 5:48 AM, Karl O. Pinc <k...@meme.com> wrote: >> On Fri, Sep 21, 2012 at 8:54 AM, Karl O. Pinc <k...@meme.com> wrote: >> > On 09/20/2012 12:24:49 PM, Karl O. Pinc wrote:
> OT: > After looking at the code I found a number of "conflicting" > option combinations are not tested for or rejected. I can't > recall what they are now. The right way to fix these would be > to send in a separate patch for each "conflict" all attached > to one email/commitfest item? Or one patch that just gets > adjusted until it's right? Typically I think it's easiest for the reviewer+committer to consider a bunch of such similar changes altogether in one patch, rather than in a handful of separate patches, though that's just my own preference. >> > ---- >> > >> > More serious objections were raised regarding semantics. >> > >> > What if, instead, the initial structure looked like: >> > >> > CREATE TABLE schemaA.foo >> > (id PRIMARY KEY, data INT); >> > >> > CREATE TABLE schemaB.bar >> > (id INT CONSTRAINT "bar_on_foo" REFERENCES foo >> > , moredata INT); >> > >> > With a case like this, in most real-world situations, you'd >> > have to use pg_restore with --disable-triggers if you wanted >> > to use --data-only and --truncate-tables. The possibility of >> > foreign key referential integrity corruption is obvious. >> >> Why would --disable-triggers be used in this example? I don't think >> you could use --truncate-tables to restore only table "foo", because >> you would get: >> >> ERROR: cannot truncate a table referenced in a foreign key >> constraint >> >> (At least, I think so, not having tried with the actual patch.) You >> could use --disable-triggers when restoring "bar". > > I tried it and you're quite right. (I thought I'd tried this > before, but clearly I did not -- proving the utility of the review > process. :-) My assumption was that since triggers > were turned off that constraints, being triggers, would be off as > well and therefore tables with foreign keys could be truncated. > Obviously not, since the I get the above error. > > I just tried it. --disable-triggers does not disable constraints. Just to be clear, I believe the problem in this example is that --disable-triggers does not disable any foreign key constraints of other tables when you are restoring a single table. So with: pg_restore -1 --truncate-tables --disable-triggers --table=foo \ --data-only my.dump ... you would get the complaint ERROR: cannot truncate a table referenced in a foreign key constraint which is talking about bar's referencing foo, and there was no ALTER TABLE bar DISABLE TRIGGER ALL done, since "bar" isn't being restored. I don't have a quibble with this existing behavior -- you are instructing pg_restore to only mess with "bar", after all. See below, though, for a comparison of --clean and --truncate-tables when restoring "foo" and "bar" together. >> For your first example, --truncate-tables seems to have some use, so >> that the admin isn't forced to recreate various views which may be >> dependent on the table. (Though it's not too difficult to work around >> this case today.) > > As an aside: I never have an issue with this, having planned ahead. > I'm curious what the not-too-difficult work-around is that you have > in mind. I'm not coming up with a tidy way to, e.g, pull a lot > of views out of a dump. Well, for the first example, there was one table and only one view which depended on the table, so manually editing the list file like so: pg_restore --list --file=my.dump > output.list # manually edit file "output.list", select only entries pertaining # to the table and dependent view pg_restore -1 --clean --use-list=output.list ... isn't too arduous, though it would become so if you had more dependent views to worry about. I'm willing to count this use-case as a usability win for --truncate-tables, since with that option the restore can be boiled down to a short and sweet: pg_restore -1 --data-only --truncate-tables --table=mytable my.dump ... Though note this may not prove practical for large tables, since --data-only leaves constraints and indexes intact during the restore, and can be massively slower compared to adding the constraints only after the data has been COPYed in, as pg_restore otherwise does. >> For the second example involving FKs, I'm a little bit more hesitant >> about endorsing the use of --truncate-tables combined with >> --disable-triggers to potentially allow bogus FKs. I know this is >> possible to some extent today using the --disable-triggers option, >> but >> it makes me nervous to be adding a mode to pg_restore if it's >> primarily designed to work together with --disable-triggers as a >> larger foot-gun. > > This is the crux of the issue, and where I was thinking of > taking this patch. I very often am of the mindset that > foreign keys are no more or less special than other > more complex data integrity rules enforced with triggers. > (I suppose others might say the same regards to integrity > enforced at the application level.) This naturally > inclines me to think that one more way, beyond > --disable-triggers, to break integrity is no big deal. > > But I quite see your point. Is it possible to get > resolution on this issue before either of us do any > more work in the direction of foreign keys? I think the patch has some promise with at least one use-case (view(s) dependent on table which needs to be reloaded, as discussed above). With the other use-case we have been discussing, of reloading a table referenced by other table(s)'s FKs, whether --truncate-tables is helpful is less clear to me, at least in the patch's current state. (See also bottom.) > An additional foot-gun, --disable-constraints, > seems like the natural progression in this direction. > Constraints, unlike triggers (?), can, in theory, > be fired at any time to check data content so perhaps > providing a way to test existing constraints against > db content would be a way to mitigate the foot-gun-ness > and drive an after-the-fact data cleanup process. > > --disable-constraints seems like an entirely separate > patch so maybe we can stop the FK related issue right > here? (Although I would appreciate feedback regards > whether such an option might be accepted, at minimum > I'd like to get this out of my brain.) I'm not sure I follow exactly how you envision --disable-constraints would work, but it does seem a separate issue from the --truncate-tables option at hand. > One thing you'll want to pay attention to is the point > in the restore process at which the truncation is done. > In the current version each table is truncated immediately > before being copied. It might or might not be better to do > all the truncation up-front, in the fashion of --clean. > I would appreciate some guidance on this. IMO --truncate-tables is, at a minimum, going to need to truncate tables up-front and in the appropriate order to avoid the annoying "ERROR: cannot truncate a table referenced in a foreign key constraint", at least when avoiding that error is possible. Let's go back to your example of two tables, with "bar" referencing "foo". This existing --clean functionality works fine: # edit list to only include lines mentioning "foo" and "bar" pg_restore -1 --clean --use-list=output.list ... But this won't work, since pg_restore attempts to truncate and restore foo separately from bar: # edit list to only include lines mentioning "foo" and "bar" pg_restore --truncate-tables --data-only -1 --use-list=output.list ... i.e. will run into "ERROR: cannot truncate a table referenced in a foreign key constraint". > In case it's helpful I'm attaching two files > I used to test the foreign key issue. Thanks, I do appreciate seeing testcases scripts like this, since it can neatly demonstrate the intended use-case of the feature, and help bring to light anything that's missing. I tried your testcase, and got: pg_restore: [archiver (db)] could not execute query: ERROR: cannot truncate a table referenced in a foreign key constraint DETAIL: Table "bar" references "foo". as discussed above. If you'd like to advertise this feature as being handy for reloading a table referenced by other FKs, I'd be interested to see a testcase demonstrating this use, along with any changes to the patch (e.g. moving TRUNCATEs to the start) which might be needed. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
