Thread: pg_dump transaction's read-only mode
I'm looking at the following code in pg_dump.c /* * Start transaction-snapshot mode transaction to dump consistent data. */ ExecuteSqlStatement(fout, "BEGIN"); if (fout->remoteVersion >= 90100) { if (serializable_deferrable) ExecuteSqlStatement(fout, "SET TRANSACTION ISOLATION LEVEL " "SERIALIZABLE, READ ONLY, DEFERRABLE"); else ExecuteSqlStatement(fout, "SET TRANSACTION ISOLATION LEVEL" "REPEATABLE READ"); } else ExecuteSqlStatement(fout, "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"); Is there a reason why we do not the RR transaction as READ ONLY above ? I understand that unlike in the case of SERIALIZABLE transaction, it may not have any performance impact. But isn't it a good practice anyways to guard against any unintended database modification while taking a dump or a safe guard against any future optimizations for read-only transactions ? More so because RR seems to the default for pg_dump Thanks, Pavan
Pavan Deolasee wrote: > I'm looking at the following code in pg_dump.c > > /* > * Start transaction-snapshot mode transaction to dump > * consistent data. > */ > ExecuteSqlStatement(fout, "BEGIN"); > if (fout->remoteVersion >= 90100) > { > if (serializable_deferrable) > ExecuteSqlStatement(fout, > "SET TRANSACTION ISOLATION LEVEL " > "SERIALIZABLE, READ ONLY, DEFERRABLE"); > else > ExecuteSqlStatement(fout, > "SET TRANSACTION ISOLATION LEVEL " > "REPEATABLE READ"); > } > else > ExecuteSqlStatement(fout, > "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"); > > Is there a reason why we do not the RR transaction as READ ONLY > above ? I understand that unlike in the case of SERIALIZABLE > transaction, it may not have any performance impact. But isn't it a > good practice anyways to guard against any unintended database > modification while taking a dump or a safe guard against any future > optimizations for read-only transactions ? More so because RR seems > to the default for pg_dump That makes sense to me. The reason I didn't make that change when I added the serializable special case to pg_dump was that it seemed like a separate question; I didn't want to complicate an already big patch with unnecessary changes to non-serializable transactions. -Kevin
On Fri, Sep 7, 2012 at 6:06 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > That makes sense to me. The reason I didn't make that change when I > added the serializable special case to pg_dump was that it seemed > like a separate question; I didn't want to complicate an already big > patch with unnecessary changes to non-serializable transactions. > If we agree, should we change that now ? Thanks, Pavan
On Mon, Sep 10, 2012 at 10:00 PM, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > On Fri, Sep 7, 2012 at 6:06 PM, Kevin Grittner > <Kevin.Grittner@wicourts.gov> wrote: > > >> That makes sense to me. The reason I didn't make that change when I >> added the serializable special case to pg_dump was that it seemed >> like a separate question; I didn't want to complicate an already big >> patch with unnecessary changes to non-serializable transactions. >> > > If we agree, should we change that now ? > Sorry for posting on such an old thread. But here is a patch that fixes this. I'm also adding to the next commitfest so that we don't lose track of it again. Thanks, Pavan -- Pavan Deolasee http://www.linkedin.com/in/pavandeolasee
Attachment
* Pavan Deolasee (pavan.deolasee@gmail.com) wrote: > On Fri, Sep 7, 2012 at 6:06 PM, Kevin Grittner > > That makes sense to me. The reason I didn't make that change when I > > added the serializable special case to pg_dump was that it seemed > > like a separate question; I didn't want to complicate an already big > > patch with unnecessary changes to non-serializable transactions. > > > > If we agree, should we change that now ? This is on the next commitfest, so I figure it deserves some comment. For my part- I tend to agree that we should have it always use a read only transaction. Perhaps we should update the pg_dump documentation to mention this as well though? Pavan, do you want to put together an actual patch? Thanks, Stephen
On Sun, Dec 30, 2012 at 12:38 AM, Stephen Frost <sfrost@snowman.net> wrote: > * Pavan Deolasee (pavan.deolasee@gmail.com) wrote: >> On Fri, Sep 7, 2012 at 6:06 PM, Kevin Grittner >> > That makes sense to me. The reason I didn't make that change when I >> > added the serializable special case to pg_dump was that it seemed >> > like a separate question; I didn't want to complicate an already big >> > patch with unnecessary changes to non-serializable transactions. >> > >> >> If we agree, should we change that now ? > > This is on the next commitfest, so I figure it deserves some comment. > For my part- I tend to agree that we should have it always use a read > only transaction. Perhaps we should update the pg_dump documentation to > mention this as well though? Pavan, do you want to put together an > actual patch? > I'd posted actual patch on this thread, but probably linked wrong message-id in the commitfest page. Will check and correct. Regarding pg_dump's documentation, I don't have strong views on that. Whether pg_dump runs as a read-only transaction or not is entirely internal to its implementation, but then if we make this change, it might be worth telling users that they can trust that pg_dump will not make any changes to their database and hence a safe operation to carry out. Thanks, Pavan Pavan Deolasee http://www.linkedin.com/in/pavandeolasee
On Mon, Dec 31, 2012 at 1:38 AM, Pavan Deolasee <pavan.deolasee@gmail.com> wrote:
I have updated the commitfest submission to link to the correct patch email.
I initially thought that this patch deserves accompanying documentation because pg_dump's serializable transaction may error out because of a conflict. But the following line in the docs [1] confirms otherwise:
I'd posted actual patch on this thread, but probably linked wrongOn Sun, Dec 30, 2012 at 12:38 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Pavan Deolasee (pavan.deolasee@gmail.com) wrote:
>> On Fri, Sep 7, 2012 at 6:06 PM, Kevin Grittner
>> > That makes sense to me. The reason I didn't make that change when I
>> > added the serializable special case to pg_dump was that it seemed
>> > like a separate question; I didn't want to complicate an already big
>> > patch with unnecessary changes to non-serializable transactions.
>> >
>>
>> If we agree, should we change that now ?
>
> This is on the next commitfest, so I figure it deserves some comment.
> For my part- I tend to agree that we should have it always use a read
> only transaction. Perhaps we should update the pg_dump documentation to
> mention this as well though? Pavan, do you want to put together an
> actual patch?
>
message-id in the commitfest page. Will check and correct. Regarding
pg_dump's documentation, I don't have strong views on that. Whether
pg_dump runs as a read-only transaction or not is entirely internal to
its implementation, but then if we make this change, it might be worth
telling users that they can trust that pg_dump will not make any
changes to their database and hence a safe operation to carry out.
I have updated the commitfest submission to link to the correct patch email.
I initially thought that this patch deserves accompanying documentation because pg_dump's serializable transaction may error out because of a conflict. But the following line in the docs [1] confirms otherwise:
"read-only transactions will never have serialization conflicts"
So no doc patch necessary :)
[1] http://www.postgresql.org/docs/9.2/static/transaction-iso.html
--
On Wed, Jan 9, 2013 at 6:42 AM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:
I have updated the commitfest submission to link to the correct patch email.
Thanks Gurjeet.
I initially thought that this patch deserves accompanying documentation because pg_dump's serializable transaction may error out because of a conflict. But the following line in the docs [1] confirms otherwise:
"read-only transactions will never have serialization conflicts"
So no doc patch necessary :)
Can you please mark the patch as "Ready for committer" if you think that way ?
Thanks,
Pavan
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee
On Wed, Jan 16, 2013 at 1:21 AM, Pavan Deolasee <pavan.deolasee@gmail.com> wrote:
Done.
On Wed, Jan 9, 2013 at 6:42 AM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:
I have updated the commitfest submission to link to the correct patch email.Thanks Gurjeet.I initially thought that this patch deserves accompanying documentation because pg_dump's serializable transaction may error out because of a conflict. But the following line in the docs [1] confirms otherwise:
"read-only transactions will never have serialization conflicts"
So no doc patch necessary :)Can you please mark the patch as "Ready for committer" if you think that way ?
Done.
--
Pavan Deolasee <pavan.deolasee@gmail.com> writes: > Sorry for posting on such an old thread. But here is a patch that > fixes this. I'm also adding to the next commitfest so that we don't > lose track of it again. As submitted, this broke pg_dump for dumping from pre-8.0 servers. (7.4 didn't accept commas in SET TRANSACTION syntax, and versions before that didn't have the READ ONLY option at all.) I fixed that and committed it. regards, tom lane
On Sun, Jan 20, 2013 at 4:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Pavan Deolasee <pavan.deolasee@gmail.com> writes: >> Sorry for posting on such an old thread. But here is a patch that >> fixes this. I'm also adding to the next commitfest so that we don't >> lose track of it again. > > As submitted, this broke pg_dump for dumping from pre-8.0 servers. > (7.4 didn't accept commas in SET TRANSACTION syntax, and versions > before that didn't have the READ ONLY option at all.) My bad. I did not realize that pg_dump is still supported for pre-8.0 releases. > I fixed that > and committed it. > Thanks, Pavan -- Pavan Deolasee http://www.linkedin.com/in/pavandeolasee
Pavan Deolasee <pavan.deolasee@gmail.com> writes: > On Sun, Jan 20, 2013 at 4:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> As submitted, this broke pg_dump for dumping from pre-8.0 servers. >> (7.4 didn't accept commas in SET TRANSACTION syntax, and versions >> before that didn't have the READ ONLY option at all.) > My bad. I did not realize that pg_dump is still supported for pre-8.0 releases. It supports servers back to 7.0. At some point we ought to consciously desupport old servers and pull out the corresponding code, but that needs to be an act of intention not omission ;-) (It's entirely likely that the 7.0 server I keep around for testing this is the last one in captivity anywhere. But IIRC, we've heard fairly recent reports of people still using 7.2. We'd have to desupport before 7.3 to save any meaningful amount of pg_dump code, so I'm not convinced it's time to pull the trigger on this quite yet.) regards, tom lane
On Mon, Jan 21, 2013 at 8:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Pavan Deolasee <pavan.deolasee@gmail.com> writes: >> On Sun, Jan 20, 2013 at 4:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> As submitted, this broke pg_dump for dumping from pre-8.0 servers. >>> (7.4 didn't accept commas in SET TRANSACTION syntax, and versions >>> before that didn't have the READ ONLY option at all.) > >> My bad. I did not realize that pg_dump is still supported for pre-8.0 releases. > > It supports servers back to 7.0. At some point we ought to consciously > desupport old servers and pull out the corresponding code, but that > needs to be an act of intention not omission ;-) > > (It's entirely likely that the 7.0 server I keep around for testing this > is the last one in captivity anywhere. But IIRC, we've heard fairly > recent reports of people still using 7.2. We'd have to desupport before > 7.3 to save any meaningful amount of pg_dump code, so I'm not convinced > it's time to pull the trigger on this quite yet.) The final release of the PostgreSQL 7.3 series was stamped in January, 2008, at which point the most current release was PostgreSQL 8.2, which is now itself EOL. Also, the original release of PostgreSQL 7.3 was in November of 2012, so we've also passed the ten-year mark for that code. Both of those seem like reasonable yardsticks for saying that we don't need to support that any more. On a more practical level, if someone is trying to upgrade from PostgreSQL 7.3 or older to a modern version, it's probably going to be pretty hard for other reasons anyway. We change a few things in a backward-incompatible fashion in every release, and over time those add up. So I think anyone doing an upgrade from 7.3 probably has their work cut out for them. I would likely advise someone in that situation to consider an intermediate upgrade to 8.2 first, and then go on to 9.2 from there. All that having been said, I'm in no rush to drop the older code this release. We have enough other things to do right now. But, it might be something to consider for 9.4. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 01/21/2013 02:00 PM, Tom Lane wrote: > Pavan Deolasee <pavan.deolasee@gmail.com> writes: >> On Sun, Jan 20, 2013 at 4:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> As submitted, this broke pg_dump for dumping from pre-8.0 servers. >>> (7.4 didn't accept commas in SET TRANSACTION syntax, and versions >>> before that didn't have the READ ONLY option at all.) > >> My bad. I did not realize that pg_dump is still supported for pre-8.0 releases. > > It supports servers back to 7.0. At some point we ought to consciously > desupport old servers and pull out the corresponding code, but that > needs to be an act of intention not omission ;-) > > (It's entirely likely that the 7.0 server I keep around for testing this > is the last one in captivity anywhere. But IIRC, we've heard fairly > recent reports of people still using 7.2. We'd have to desupport before > 7.3 to save any meaningful amount of pg_dump code, so I'm not convinced > it's time to pull the trigger on this quite yet.) old versions are still alive - just yesterday we had someone on IRC trying to build 7.1.3 on a modern ubuntu installation because he has an old app depending on it., and especially 7.2 shows up regulary as well. If the effort to keep pg_dump support for those alive is not too bad, we should try to support them as long as we can. Stefan
Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> writes: > On 01/21/2013 02:00 PM, Tom Lane wrote: >> (It's entirely likely that the 7.0 server I keep around for testing this >> is the last one in captivity anywhere. But IIRC, we've heard fairly >> recent reports of people still using 7.2. We'd have to desupport before >> 7.3 to save any meaningful amount of pg_dump code, so I'm not convinced >> it's time to pull the trigger on this quite yet.) > old versions are still alive - just yesterday we had someone on IRC > trying to build 7.1.3 on a modern ubuntu installation because he has an > old app depending on it., and especially 7.2 shows up regulary as well. > If the effort to keep pg_dump support for those alive is not too bad, we > should try to support them as long as we can. Of course, the counter-argument is that people who are still using those versions are probably not going to be interested in upgrading to a modern version anyway. Or if they are, dumping with their existing version of pg_dump is likely to not be much worse than dumping with the target version's pg_dump --- as Robert mentioned upthread, if you're migrating from such an old version you're in for a lot of compatibility issues anyhow, most of which pg_dump can't save you from. Having said that, I'm not in a hurry to pull pg_dump support for old versions. But we can't keep it up forever. In particular, once that old HPUX box dies, I'm not likely to want to try to get 7.0 to compile on a newer box just so I can keep checking pg_dump compatibility. regards, tom lane
On 01/21/2013 08:45 PM, Tom Lane wrote: > Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> writes: >> On 01/21/2013 02:00 PM, Tom Lane wrote: >>> (It's entirely likely that the 7.0 server I keep around for testing this >>> is the last one in captivity anywhere. But IIRC, we've heard fairly >>> recent reports of people still using 7.2. We'd have to desupport before >>> 7.3 to save any meaningful amount of pg_dump code, so I'm not convinced >>> it's time to pull the trigger on this quite yet.) > >> old versions are still alive - just yesterday we had someone on IRC >> trying to build 7.1.3 on a modern ubuntu installation because he has an >> old app depending on it., and especially 7.2 shows up regulary as well. > >> If the effort to keep pg_dump support for those alive is not too bad, we >> should try to support them as long as we can. > > Of course, the counter-argument is that people who are still using those > versions are probably not going to be interested in upgrading to a > modern version anyway. Or if they are, dumping with their existing > version of pg_dump is likely to not be much worse than dumping with the > target version's pg_dump --- as Robert mentioned upthread, if you're > migrating from such an old version you're in for a lot of compatibility > issues anyhow, most of which pg_dump can't save you from. sure - but having at least an easy way to properly get your schema and data over from such an old version helps. That "only" leaves you with the compatibility issues in the app - having to do both would be worse. > > Having said that, I'm not in a hurry to pull pg_dump support for old > versions. But we can't keep it up forever. In particular, once that > old HPUX box dies, I'm not likely to want to try to get 7.0 to compile > on a newer box just so I can keep checking pg_dump compatibility. yeah I'm not saying we need to keep this forever, if we say drop everyting including 7.3 with 9.4 that would mean that we would "support" direct upgrades using pg_dump for over 10 years (7.3s last supported release was in january 2008 and assuming we get 9.3 out the door this year) for indirekt upgrades (ie using one intermediate step) we would be talking more like 15-20 years if we keep the current level of backwards compatibility - this seems plenty enough for me... Stefan