Hi, On 2013-05-07 08:54:54 -0400, Stephen Frost wrote: > * Andres Freund (and...@2ndquadrant.com) wrote: > > > All of which I > > > think I agree with, but I don't agree with the conclusion that this > > > larger window is somehow acceptable because there's a very small window > > > (one which can't be made any smaller, today..) which exists today. > > > > The window isn't that small currently: > > Agreed- but it also isn't currently possible to make it any smaller.
Uh. Why not? I think this is what needs to be fixed instead of making the hole marginally smaller elsewhere. You can trivially reproduce the problem with pg_dump today: S1: $ psql postgres =# CREATE DATABASE pgdumptest; =# CREATE DATABASE pgrestoretest; =# \c pgdumptest =# CREATE TABLE tbl(id serial primary key, data_a int8, data_b float8); =# INSERT INTO tbl(data_a, data_b) SELECT random()::int, random() FROM generate_series(1, 10); =# BEGIN; =# ALTER TABLE tbl RENAME COLUMN data_a TO data_swap; =# ALTER TABLE tbl RENAME COLUMN data_b TO data_a; =# ALTER TABLE tbl RENAME COLUMN data_swap TO data_b; S2: $ pg_dump pgdumptest > /tmp/pg_dump.sql S1: =# COMMIT; S2: $ psql pgrestoretest -f /tmp/pgdump.sql psql:/tmp/pgdump.sql:87: ERROR: invalid input syntax for integer: "0.944006722886115313" CONTEXT: COPY tbl, line 1, column data_a: "0.944006722886115313" A ddl upgrade script taking some seconds isn't exactly anything unusual... > > > Alright, then let's provide a function which will do that and tell > > > people to use it instead of just using pg_export_snapshot(), which > > > clearly doesn't do that. > > > > If it were clear cut what to lock and we had locks for > > everything. Maybe. But we don't have locks for everything. > > My suggestion was to lock everything that pg_dump locks, which we > clearly have locks for since pg_dump is acquiring them. Also, I don't > believe it'd be that difficult to identify what pg_dump would lock, at > least in a 'default' whole-database run. This is more of a stop-gap > than a complete solution. The problem is that locking - as shown above - doesn't really help all that much. You would have to do it like: 1) start txn 2) acquire DDL prevention lock 3) assert we do not yet have a snapshot 4) acquire snapshot 5) lock objects 6) release DDL lock 7) dump objects/data 8) commit txn Unfortunately most of these steps cannot easily/safely exposed to sql. And again, this is a very old situation, that doesn't really have to do anything with snapshot exporting. > > So we would > > need to take locks preventing any modification on any of system catalogs > > which doesn't really seem like a good thing, especially as we can't > > release them from sql during the dump were we can allow creation of > > temp tables and everything without problems. > > That's already an issue when pg_dump runs, no? Not sure why this is > different. pg_dump doesn't prevent you from running CREATE TEMPORARY TABLE? That would make it unrunnable in many situations. Especially as we cannot easily (without using several connections at once) release locks before ending a transaction. > > > I believe the main argument here is really around "you should think > > > about these issues before just throwing this in" and not "it must be > > > perfect before it goes in". Perhaps "it shouldn't make things *worse* > > > than they are now" would also be apt.. > > > > That's not how I read 8465.1367860...@sss.pgh.pa.us :( > > I believe the point that Tom is making is that we shouldn't paint > ourselves into a corner by letting users provide old snapshots to > pg_dump which haven't acquired any of the necessary locks. The goal, at > least as I read it, is to come up with a workable design (and I don't > know that we have, but still) which provides a way for the locks to be > taken at least as quickly as what pg_dump does today and which we could > modify down the road to take the locks pre-snapshot (presuming we can > figure out a way to make that work). > The proposed patch certainly doesn't make any attempt to address that > issue and would encourage users to open themselves up to this risk more > than they are exposted today w/ pg_dump. I fail to see a difference that is big enough to worry overly much about. The above problem is easy enough to encounter without any snapshot exporting and I can't remember a single report. > > I think there is no point in fixing it somewhere else. The problem is in > > pg_dump, not the snapshot import/export. > > It's really a problem for just about everything that uses transactions > and locking, isn't it? pg_dump just happens to have it worst since it > wants to go and touch every object in the database. It's certainly > possible for people to connect to the DB, look at pg_class and then try > to access some object which no longer exists (even though it's in > pg_class). Well, normal sql shouldn't need to touch pg_class and will know beforehand which locks it will need. But I have to say I more than once wished we would throw an error if an objects definition is "newer" than the one we started out with. > This will be an interesting thing to consider when > implementing MVCC for the catalog. I think using proper mvcc snapsot for catalog scans doesn't, cannot even in all case, imply having to use the user's transaction's snapshot, just one that guarantees a consistent result while a query is running. > > You did suggest how it can be fixed? You mean > > 20130506214515.gl4...@tamriel.snowman.net? > > I suggested how it might be done. :) There's undoubtably issues with an > all-database-objects lock, but it would certainly reduce the time > between transaction start and getting all the locks acquired and shrink > the window that much more. If we did implement such a beast, how could > we ensure that the locks were taken immediately after transaction start > if the snapshot is being passed to pg_dump? Basically, if we *did* > solve this issue for pg_dump in some way in the future, how would we use > it if pg_dump can accept an outside snapshot? I am not sure if the correct fix is locking and not just making sure the definition of objects hasn't changed since the snapshot started. But if we go for locking creating a function which makes sure that the source transaction has a certain strong lock wouldn't be that hard. We have all the data for it. > One other thought did occur to me- we could simply have pg_dump export > the snapshot that it gets to stdout, a file, whatever, and systems which > are trying to do this magic "everyone gets the same view" could glob > onto the snapshot created by pg_dump, after all the locks have been > acquired.. Several problems: a) exporting a snapshot to a file was discussed and deemed unacceptable risky. That's why pg_export_snapshot() exports it itself into some internal format somewhere. The user doesn't need to know where/how. b) When importing a snapshot the source transaction needs to be alive, otherwise we cannot guarantee the global xmin hasn't advanced too much. That would open up very annoying race-conditions because pg_dump would need to live long enough for the separate transaction to import data. c) Quite possibly the snapshot we need needs to meet some special criterions that pg_dump cannot guarantee. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
