Re: pg_dump transaction's read-only mode

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(dot)Grittner(at)wicourts(dot)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(dot)deolasee(at)gmail(dot)com> wrote:
> On Fri, Sep 7, 2012 at 6:06 PM, Kevin Grittner
> <Kevin(dot)Grittner(at)wicourts(dot)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

* Pavan Deolasee (pavan(dot)deolasee(at)gmail(dot)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(at)snowman(dot)net> wrote:
> * Pavan Deolasee (pavan(dot)deolasee(at)gmail(dot)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(dot)deolasee(at)gmail(dot)com>wrote:

> On Sun, Dec 30, 2012 at 12:38 AM, Stephen Frost <sfrost(at)snowman(dot)net>
> wrote:
> > * Pavan Deolasee (pavan(dot)deolasee(at)gmail(dot)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.
>

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
--
Gurjeet Singh

http://gurjeet.singh.im/

On Wed, Jan 9, 2013 at 6:42 AM, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)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(dot)deolasee(at)gmail(dot)com>wrote:

>
>
> On Wed, Jan 9, 2013 at 6:42 AM, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)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.

--
Gurjeet Singh

http://gurjeet.singh.im/

Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)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(dot)deolasee(at)gmail(dot)com> writes:
> On Sun, Jan 20, 2013 at 4:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com> writes:
>> On Sun, Jan 20, 2013 at 4:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(dot)deolasee(at)gmail(dot)com> writes:
>> On Sun, Jan 20, 2013 at 4:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)kaltenbrunner(dot)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(at)kaltenbrunner(dot)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