Re: [GENERAL] pg_upgrade ?deficiency

Hello all,

I am upgrading a 8.4 cluster to 9.1 and am seeing the following:

SQL command failed

CREATE TEMPORARY TABLE info_rels (reloid) AS
SELECT c.oid
FROM
pg_catalog.pg_class c
JOIN pg_catalog.pg_namespace n ON c.relnamespace = n.oid
LEFT OUTER JOIN pg_catalog.pg_index i ON c.oid = i.indexrelid
WHERE
relkind IN ('r', 'm', 'i', 'S')
AND
i.indisvalid IS DISTINCT FROM false
AND
i.indisready IS DISTINCT FROM false
AND
((n.nspname !~ '^pg_temp_'
AND
n.nspname !~ '^ pg_toast_temp_'
AND
n.nspname NOT IN ('pg_catalog', 'information_schema', 'binary_upgrade', 'pg_toast')
AND
c.oid >= 16384
)
OR
(n.nspname = 'pg_catalog'
AND
relname IN ('pg_largeobject', 'pg_largeobject_loid_pn_index')
));

ERROR: transaction is read-only

Now, this is quite understandable since one of the databases
is set to

ALTER DATABASE ... SET DEFAULT_TRANSACTION_READ_ONLY TO ON;

However, since the above setting is something which can
be expected every so often in any odd PostgreSQL cluster
(and not some weird coincidence no one really knows how
they got into in the first place) I would think pg_upgrade
really should be able to handle.

Technically that's pretty easy - make sure transactions are
set to readwrite for the pg_upgrade run by any number of
means:

- ALTER DATABASE before/after pg_upgrade
- ALTER USER running the pg_upgrade
- SET TRANSACTION READ WRITE at the appropriate times
- ...

Or at least this limitation of pg_upgrade (requiring
DB write access) should get a mention in the docs and/or
man page.

What is the informed opinion on this ?

Thanks,
Karsten
--
GPG key ID E4071346 @ gpg-keyserver.de
E167 67FD A291 2BEA 73BD 4537 78B9 A9F9 E407 1346

On Tue, Nov 19, 2013 at 11:22:47AM +0100, Karsten Hilbert wrote:
> ERROR: transaction is read-only
>
> Now, this is quite understandable since one of the databases
> is set to
>
> ALTER DATABASE ... SET DEFAULT_TRANSACTION_READ_ONLY TO ON;
>
> However, since the above setting is something which can
> be expected every so often in any odd PostgreSQL cluster
> (and not some weird coincidence no one really knows how
> they got into in the first place) I would think pg_upgrade
> really should be able to handle.
>
> Technically that's pretty easy - make sure transactions are
> set to readwrite for the pg_upgrade run by any number of
> means:
>
> - ALTER DATABASE before/after pg_upgrade
> - ALTER USER running the pg_upgrade
> - SET TRANSACTION READ WRITE at the appropriate times
> - ...
>
> Or at least this limitation of pg_upgrade (requiring
> DB write access) should get a mention in the docs and/or
> man page.
>
> What is the informed opinion on this ?

I think pg_upgrade did the right thing here by throwing an error. There
is no clean way to handle these cases without possibly causing more
problems.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

> On Tue, Nov 19, 2013 at 11:22:47AM +0100, Karsten Hilbert wrote:
> > ERROR: transaction is read-only
> >
> > Now, this is quite understandable since one of the databases
> > is set to
> >
> > ALTER DATABASE ... SET DEFAULT_TRANSACTION_READ_ONLY TO ON;
> >
> > However, since the above setting is something which can
> > be expected every so often in any odd PostgreSQL cluster
> > (and not some weird coincidence no one really knows how
> > they got into in the first place) I would think pg_upgrade
> > really should be able to handle.
> >
> > Technically that's pretty easy - make sure transactions are
> > set to readwrite for the pg_upgrade run by any number of
> > means:
> >
> > - ALTER DATABASE before/after pg_upgrade
> > - ALTER USER running the pg_upgrade
> > - SET TRANSACTION READ WRITE at the appropriate times
> > - ...
> >
> > Or at least this limitation of pg_upgrade (requiring
> > DB write access) should get a mention in the docs and/or
> > man page.
> >
> > What is the informed opinion on this ?
>
> I think pg_upgrade did the right thing here by throwing an error. There
> is no clean way to handle these cases without possibly causing more
> problems.

I am not sure this is the ideal way of looking at the problem (for one
thing it wasn't pg_upgrade throwing the error). Maybe I have not clearly
expressed myself.

Let me try to rephrase:

Fact: pg_upgrade can NOT properly upgrade clusters which contain
databases that are set to "default_transaction_read_only on"

Question: Is this intended ?

Thanks,
Karsten

Karsten Hilbert wrote:
> Let me try to rephrase:
>
> Fact: pg_upgrade can NOT properly upgrade clusters which contain
> databases that are set to "default_transaction_read_only on"
>
> Question: Is this intended ?

I am pretty sure that this is an oversight and hence a bug.

Yours,
Laurenz Albe

> Karsten Hilbert wrote:
> > Let me try to rephrase:
> >
> > Fact: pg_upgrade can NOT properly upgrade clusters which contain
> > databases that are set to "default_transaction_read_only on"
> >
> > Question: Is this intended ?
>
> I am pretty sure that this is an oversight and hence a bug.

oversight, yes ... I thought as much and was therefore a bit
cautious of calling it a bug, chose to name it "?deficiency" ;-)

Karsten

On Wed, Nov 20, 2013 at 02:36:08PM +0100, Karsten Hilbert wrote:
> > Karsten Hilbert wrote:
> > > Let me try to rephrase:
> > >
> > > Fact: pg_upgrade can NOT properly upgrade clusters which contain
> > > databases that are set to "default_transaction_read_only on"
> > >
> > > Question: Is this intended ?
> >
> > I am pretty sure that this is an oversight and hence a bug.
>
> oversight, yes ... I thought as much and was therefore a bit
> cautious of calling it a bug, chose to name it "?deficiency" ;-)

Well, pg_upgrade can't handle every possible configuration. How do we
even restore into such a database? You marked the database as
read-only, and pg_upgrade is going to honor that and not modify it. I
believe a pg_dumpall restore might fail in the same way.

You need to change the default on the old cluster before upgrading. It
is overly cumbersome to set the default_transaction_read_only for every
database connection, and there are many other settings that might also
cause failures. If it was a silent failure, I would be more concerned.

What you might be able to do is to set PGOPTIONS to "-c
default_transaction_read_only=false" and run pg_upgrade. If more people
report this problem, I could document this work-around.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

> On Wed, Nov 20, 2013 at 02:36:08PM +0100, Karsten Hilbert wrote:
> > > Karsten Hilbert wrote:
> > > > Let me try to rephrase:
> > > >
> > > > Fact: pg_upgrade can NOT properly upgrade clusters which contain
> > > > databases that are set to "default_transaction_read_only on"
> > > >
> > > > Question: Is this intended ?
> > >
> > > I am pretty sure that this is an oversight and hence a bug.
>
> Well, pg_upgrade can't handle every possible configuration.

Agreed. That would be a design decision: "no, pg_upgrade will
not support upgrading some of your databases, for example those
which are set to default_transaction_ready_only=on".

If I don't like that, fine, I can go and use other tools or
else submit a patch and hope for inclusion or apply a workaround.

That's why I tacitly suggested a hint in the docs might
help to become aware of the above limitation.

Of course, I should submit a patch to the docs just as well.

> How do we even restore into such a database?

We read the state, remember the state, change the state,
restore the data, set the initial state. But you knew that,
I assume.

> You marked the database as read-only, and pg_upgrade
> is going to honor that and not modify it.

Oh, I am extremely happy for pg_upgrade to NOT modify
ANY of my databases ! All I am wondering is whether
it is by design decision (and if so, why) that it cannot
transfer some databases from one PG version to another
one. I am more than happy if it doesn't modify the
databases in the process ;-)

> I believe a pg_dumpall restore might fail in the same way.

pg_dumpall works but a full pg_restore/psql from that dump
likely will not. I haven't tested that yet, though, and I
deliberately did not want to raise *that* question just
yet...

> You need to change the default on the old cluster before upgrading.

I know. That wasn't my question though.

> It is overly cumbersome to set the default_transaction_read_only for every
> database connection,

There is no need for that (see above).

> and there are many other settings that might also cause failures.

If so they warrant documentation as well as they become known.

> If it was a silent failure, I would be more concerned.

Absolutely, full agreement.

> What you might be able to do is to set PGOPTIONS to "-c
> default_transaction_read_only=false" and run pg_upgrade.

That is a good idea. It might have occurred to me earlier
had the pg_upgrade limitation been documented ;-)

Thanks for your work on PostgreSQL,
Karsten

On Wed, Nov 20, 2013 at 04:07:59PM +0100, Karsten Hilbert wrote:
> > On Wed, Nov 20, 2013 at 02:36:08PM +0100, Karsten Hilbert wrote:
> > > > Karsten Hilbert wrote:
> > > > > Let me try to rephrase:
> > > > >
> > > > > Fact: pg_upgrade can NOT properly upgrade clusters which contain
> > > > > databases that are set to "default_transaction_read_only on"
> > > > >
> > > > > Question: Is this intended ?
> > > >
> > > > I am pretty sure that this is an oversight and hence a bug.
> >
> > Well, pg_upgrade can't handle every possible configuration.
>
> Agreed. That would be a design decision: "no, pg_upgrade will
> not support upgrading some of your databases, for example those
> which are set to default_transaction_ready_only=on".
>
> If I don't like that, fine, I can go and use other tools or
> else submit a patch and hope for inclusion or apply a workaround.
>
> That's why I tacitly suggested a hint in the docs might
> help to become aware of the above limitation.
>
> Of course, I should submit a patch to the docs just as well.

I think the big question is whether a generic mention that there are
some database settings, like read-only, that can prevent updates, and
you might need to use PGOPTIONS to avoid that. However, you are the
first case to report this, so I am hesistant.

> > How do we even restore into such a database?
>
> We read the state, remember the state, change the state,
> restore the data, set the initial state. But you knew that,
> I assume.

Yep.

> > You marked the database as read-only, and pg_upgrade
> > is going to honor that and not modify it.
>
> Oh, I am extremely happy for pg_upgrade to NOT modify
> ANY of my databases ! All I am wondering is whether
> it is by design decision (and if so, why) that it cannot
> transfer some databases from one PG version to another
> one. I am more than happy if it doesn't modify the
> databases in the process ;-)

Yes, messing with status can often be problematic.

> > What you might be able to do is to set PGOPTIONS to "-c
> > default_transaction_read_only=false" and run pg_upgrade.
>
> That is a good idea. It might have occurred to me earlier
> had the pg_upgrade limitation been documented ;-)

True. Does anyone else see value in documenting this? I can do the
docs.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Wed, Nov 20, 2013 at 02:36:08PM +0100, Karsten Hilbert wrote:
>>> Karsten Hilbert wrote:
>>>> Let me try to rephrase:
>>>>
>>>> Fact: pg_upgrade can NOT properly upgrade clusters which
>>>>       contain databases that are set to
>>>>       "default_transaction_read_only on"
>>>> Question: Is this intended ?
>>>
>>> I am pretty sure that this is an oversight and hence a bug.
>>
>> oversight, yes ... I thought as much and was therefore a bit
>> cautious of calling it a bug, chose to name it "?deficiency" ;-)
>
> Well, pg_upgrade can't handle every possible configuration.  How
> do we even restore into such a database?  You marked the database
> as read-only, and pg_upgrade is going to honor that and not
> modify it.

That interpretation makes no sense to me.  I know of users who have
databases where 90% of their transactions don't modify data, so
they set the *default* for transactions to read only, and override
that for transactions that are read write.  The default is not, and
never has been, a restriction on what is allowed -- it is a default
that is quite easy to override.  If we have tools that don't handle
that correctly, I consider that a bug.

> I believe a pg_dumpall restore might fail in the same way.

Then it should also be fixed.

> You need to change the default on the old cluster before
> upgrading.  It is overly cumbersome to set the
> default_transaction_read_only for every database connection

Why is this any different from other settings we cover at the front
of pg_dump output?:

| SET statement_timeout = 0;
| SET lock_timeout = 0;
| SET client_encoding = 'UTF8';
| SET standard_conforming_strings = on;
| SET check_function_bodies = false;
| SET client_min_messages = warning;

> and there are many other settings that might also cause failures.

You mean, like the above?

> What you might be able to do is to set PGOPTIONS to "-c
> default_transaction_read_only=false" and run pg_upgrade.  If more
> people report this problem, I could document this work-around.

This is most likely to bite those using serializable transactions
for data integrity, because declaring transactions read only makes
a huge difference in performance in those cases.  That is where I
have seen people set the default for read only to on; they want to
explicitly set it off only when needed.

I would be happy to supply a patch to treat
default_transaction_read_only the same as statement_timeout or
standard_conforming_strings in pg_dump and related utilities.
Since it causes backup/restore failure on perfectly valid databases
I even think this is a bug which merits back-patching.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Nov 21, 2013 at 06:22:50AM -0800, Kevin Grittner wrote:

> I would be happy to supply a patch to treat
> default_transaction_read_only the same as statement_timeout or
> standard_conforming_strings in pg_dump and related utilities.
> Since it causes backup/restore failure

... (and pg_upgrade failures -- which may internally
just be dump/restore cycles ?) ...

> on perfectly valid databases I even think this is
> a bug which merits back-patching.

Thanks so much, Kevin, for offering to work
on that part. Maybe it's a small thing but
it'll make PostgreSQL once again feel
professionally consistent.

I would have needed to become proficient in C
and get acqainted with the PG source in order
to produce a patch myself.

Thanks,
Karsten
--
GPG key ID E4071346 @ gpg-keyserver.de
E167 67FD A291 2BEA 73BD 4537 78B9 A9F9 E407 1346

On Thu, Nov 21, 2013 at 06:22:50AM -0800, Kevin Grittner wrote:
> > Well, pg_upgrade can't handle every possible configuration.  How
> > do we even restore into such a database?  You marked the database
> > as read-only, and pg_upgrade is going to honor that and not
> > modify it.
>
> That interpretation makes no sense to me.  I know of users who have
> databases where 90% of their transactions don't modify data, so
> they set the *default* for transactions to read only, and override
> that for transactions that are read write.  The default is not, and
> never has been, a restriction on what is allowed -- it is a default
> that is quite easy to override.  If we have tools that don't handle
> that correctly, I consider that a bug.

OK, this is good information to hear.

> > I believe a pg_dumpall restore might fail in the same way.
>
> Then it should also be fixed.

Yes, that is easy to do.

> > You need to change the default on the old cluster before
> > upgrading.  It is overly cumbersome to set the
> > default_transaction_read_only for every database connection
>
> Why is this any different from other settings we cover at the front
> of pg_dump output?:
>
> | SET statement_timeout = 0;
> | SET lock_timeout = 0;
> | SET client_encoding = 'UTF8';
> | SET standard_conforming_strings = on;
> | SET check_function_bodies = false;
> | SET client_min_messages = warning;
>
> > and there are many other settings that might also cause failures.
>
> You mean, like the above?
>
> > What you might be able to do is to set PGOPTIONS to "-c
> > default_transaction_read_only=false" and run pg_upgrade.  If more
> > people report this problem, I could document this work-around.
>
> This is most likely to bite those using serializable transactions
> for data integrity, because declaring transactions read only makes
> a huge difference in performance in those cases.  That is where I
> have seen people set the default for read only to on; they want to
> explicitly set it off only when needed.
>
> I would be happy to supply a patch to treat
> default_transaction_read_only the same as statement_timeout or
> standard_conforming_strings in pg_dump and related utilities.
> Since it causes backup/restore failure on perfectly valid databases
> I even think this is a bug which merits back-patching.

Not sure about backpatching. default_transaction_read_only has been
around since 7.4. Setting it to true would cause pg_dump to fail unless
you changed the database setting, and pg_dumpall would fail completely
as there is no way to turn off the database setting.

The problem is that I don't remember any report of this failing in
pg_dump, pg_dumpall, or pg_upgrade, so saying it is a major issue is
hard to accept.

However, looking forward, I think we should add it to pg_dump (and hence
pg_dumpall), and we should fix pg_upgrade so it is more reliable. I am
thinking we should either document in pg_upgrade the use of PGOPTIONS to
avoid this issue, or have pg_upgrade append to PGOPTIONS in its
environment to use some of pg_dump's resets, and that will be passed to
libpq connections, psql, and all the utilities pg_upgrade calls.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Sending to hackers for comment; seems setting
default_transaction_read_only to true via ALTER database/user or
postgresql.conf can cause pg_dump, pg_dumpall, and pg_upgrade failures.
We are considering the right solution:

---------------------------------------------------------------------------

On Fri, Nov 22, 2013 at 01:32:30PM -0500, Bruce Momjian wrote:
> On Thu, Nov 21, 2013 at 06:22:50AM -0800, Kevin Grittner wrote:
> > > Well, pg_upgrade can't handle every possible configuration.  How
> > > do we even restore into such a database?  You marked the database
> > > as read-only, and pg_upgrade is going to honor that and not
> > > modify it.
> >
> > That interpretation makes no sense to me.  I know of users who have
> > databases where 90% of their transactions don't modify data, so
> > they set the *default* for transactions to read only, and override
> > that for transactions that are read write.  The default is not, and
> > never has been, a restriction on what is allowed -- it is a default
> > that is quite easy to override.  If we have tools that don't handle
> > that correctly, I consider that a bug.
>
> OK, this is good information to hear.
>
> > > I believe a pg_dumpall restore might fail in the same way.
> >
> > Then it should also be fixed.
>
> Yes, that is easy to do.
>
> > > You need to change the default on the old cluster before
> > > upgrading.  It is overly cumbersome to set the
> > > default_transaction_read_only for every database connection
> >
> > Why is this any different from other settings we cover at the front
> > of pg_dump output?:
> >
> > | SET statement_timeout = 0;
> > | SET lock_timeout = 0;
> > | SET client_encoding = 'UTF8';
> > | SET standard_conforming_strings = on;
> > | SET check_function_bodies = false;
> > | SET client_min_messages = warning;
> >
> > > and there are many other settings that might also cause failures.
> >
> > You mean, like the above?
> >
> > > What you might be able to do is to set PGOPTIONS to "-c
> > > default_transaction_read_only=false" and run pg_upgrade.  If more
> > > people report this problem, I could document this work-around.
> >
> > This is most likely to bite those using serializable transactions
> > for data integrity, because declaring transactions read only makes
> > a huge difference in performance in those cases.  That is where I
> > have seen people set the default for read only to on; they want to
> > explicitly set it off only when needed.
> >
> > I would be happy to supply a patch to treat
> > default_transaction_read_only the same as statement_timeout or
> > standard_conforming_strings in pg_dump and related utilities.
> > Since it causes backup/restore failure on perfectly valid databases
> > I even think this is a bug which merits back-patching.
>
> Not sure about backpatching. default_transaction_read_only has been
> around since 7.4. Setting it to true would cause pg_dump to fail unless
> you changed the database setting, and pg_dumpall would fail completely
> as there is no way to turn off the database setting.
>
> The problem is that I don't remember any report of this failing in
> pg_dump, pg_dumpall, or pg_upgrade, so saying it is a major issue is
> hard to accept.
>
> However, looking forward, I think we should add it to pg_dump (and hence
> pg_dumpall), and we should fix pg_upgrade so it is more reliable. I am
> thinking we should either document in pg_upgrade the use of PGOPTIONS to
> avoid this issue, or have pg_upgrade append to PGOPTIONS in its
> environment to use some of pg_dump's resets, and that will be passed to
> libpq connections, psql, and all the utilities pg_upgrade calls.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Not sure about backpatching. default_transaction_read_only has been
> around since 7.4. Setting it to true would cause pg_dump to fail unless
> you changed the database setting, and pg_dumpall would fail completely
> as there is no way to turn off the database setting.

No, neither pg_dump nor pg_dumpall would fail. What would fail is
restoring into a database that has this option already set. It's possible
that users of this option haven't noticed it because they never attempted
a restore in such a context.

> The problem is that I don't remember any report of this failing in
> pg_dump, pg_dumpall, or pg_upgrade, so saying it is a major issue is
> hard to accept.

Yeah, it's a minor issue at best, but perhaps worth fixing since
the solution is so easy.

The bigger picture here is that there are lots of ways to break
pg_upgrade via not-sane settings, and there always will be.
I don't think we should try to promise that there won't be.

regards, tom lane

Bruce Momjian <bruce(at)momjian(dot)us> wrote:

>> Not sure about backpatching.  default_transaction_read_only has been
>> around since 7.4.  Setting it to true would cause pg_dump to fail unless
>> you changed the database setting, and pg_dumpall would fail completely
>> as there is no way to turn off the database setting.

See the attached patch.  It seems to fix pg_dump and pg_dumpall.  I
don't think it will cause any problem for *dumping* earlier
versions,  Backpatching would mean that if you try to restore a
dump made by 8.4 or later software to a 7.3 or earlier database,
you would get an error; but I don't think that's supported, and I
would be amazed if that were the *only* error you got if you tried
that.

>> The problem is that I don't remember any report of this failing in
>> pg_dump, pg_dumpall, or pg_upgrade, so saying it is a major issue is
>> hard to accept.

Any time that you can appear to successfully dump a database, and
the restore attempt fails, I consider that to be a major issue.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:

> See the attached patch.

Trying that again.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Not sure about backpatching. default_transaction_read_only has been
> > around since 7.4. Setting it to true would cause pg_dump to fail unless
> > you changed the database setting, and pg_dumpall would fail completely
> > as there is no way to turn off the database setting.
>
> No, neither pg_dump nor pg_dumpall would fail. What would fail is
> restoring into a database that has this option already set. It's possible
> that users of this option haven't noticed it because they never attempted
> a restore in such a context.

I was the original poster on -users who raised this issue. Maybe I can
clarify somewhat:

I have been attempting to upgrade an 8.4 cluster to 9.1
by means of the 9.1 pg_upgrade command.

That failed due to one of the databases in the 8.4 cluster
being "ALTER DATABASE ... SET DEFAULT_TRANSACTION_READ_ONLY TO ON".

Hence my question on that list whether that was to be considered
a bug, a deficiency, or an oversight.

I knew workarounds quite well but wondered whether that
pg_upgrade behaviour was intended to stay that way.

I suggested that if it is intended to stay it might benefit
from a hint in the documentation.

> Yeah, it's a minor issue at best, but perhaps worth fixing since
> the solution is so easy.

That would be really helpful.

> The bigger picture here is that there are lots of ways to break
> pg_upgrade via not-sane settings, and there always will be.

Would setting default_transaction_read_only to on be considered
non-sane ? If so, why ?

> I don't think we should try to promise that there won't be.

That last assertion is what everyone should certainly be able
to agree with ;-)

Thanks,
Karsten

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
>> See the attached patch.

> Trying that again.

That looks sane for pg_dump, but I would rather have expected that
pg_dumpall would need to emit the same thing (possibly more than once
due to reconnections).

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> That looks sane for pg_dump, but I would rather have expected
> that pg_dumpall would need to emit the same thing (possibly more
> than once due to reconnections).

I was kinda surprised myself.  I changed it for pg_dump, tested
that, and then tested pg_dumpall to get a baseline, and the setting
was taken care of.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-11-22 12:45:25 -0800, Kevin Grittner wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> > That looks sane for pg_dump, but I would rather have expected
> > that pg_dumpall would need to emit the same thing (possibly more
> > than once due to reconnections).
>
> I was kinda surprised myself.  I changed it for pg_dump, tested
> that, and then tested pg_dumpall to get a baseline, and the setting
> was taken care of.

pg_dumpall is lazy and just executes pg_dump for every database, that's
the reason... But are you sure it also unsets default_transaction_readonly for
the roles etc. it creates?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> are you sure it also unsets default_transaction_readonly for
> the roles etc. it creates?

I'm not sure I understand.  Could you give an example of what
you're concerned about?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Nov 22, 2013 at 03:13:33PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Not sure about backpatching. default_transaction_read_only has been
> > around since 7.4. Setting it to true would cause pg_dump to fail unless
> > you changed the database setting, and pg_dumpall would fail completely
> > as there is no way to turn off the database setting.
>
> No, neither pg_dump nor pg_dumpall would fail. What would fail is
> restoring into a database that has this option already set. It's possible
> that users of this option haven't noticed it because they never attempted
> a restore in such a context.

Well, pg_dumpall is going to restore that setting before putting any
data in the database, so it will fail. I have tested that, and also
tested that PGOPTIONS fixes it.

> > The problem is that I don't remember any report of this failing in
> > pg_dump, pg_dumpall, or pg_upgrade, so saying it is a major issue is
> > hard to accept.
>
> Yeah, it's a minor issue at best, but perhaps worth fixing since
> the solution is so easy.

Yes.

> The bigger picture here is that there are lots of ways to break
> pg_upgrade via not-sane settings, and there always will be.
> I don't think we should try to promise that there won't be.

So document PGOPTIONS in pg_upgrade?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On 2013-11-22 13:07:29 -0800, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
> > are you sure it also unsets default_transaction_readonly for
> > the roles etc. it creates?
>
> I'm not sure I understand.  Could you give an example of what
> you're concerned about?

pg_dumpall first spits out global data (users, databases, tablespaces)
and then invokes pg_dump for every single database. So I'd very strongly
suspect that your patch will issue the CREATE ROLE etc. calls without
unsetting default_transaction_readonly.

E.g. output looks like:
--
-- PostgreSQL database cluster dump
--
..
CREATE ROLE andres;
ALTER ROLE andres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN
REPLICATION;
...
\connect postgres

--
-- PostgreSQL database dump
--

CREATE TABLE pgbench_accounts (
aid integer NOT NULL,
bid integer,
abalance integer,
filler character(84)
)
WITH (fillfactor=100);

...
\connect regression
...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-11-22 13:07:29 -0800, Kevin Grittner wrote:
>> I'm not sure I understand. Could you give an example of what
>> you're concerned about?

> pg_dumpall first spits out global data (users, databases, tablespaces)
> and then invokes pg_dump for every single database. So I'd very strongly
> suspect that your patch will issue the CREATE ROLE etc. calls without
> unsetting default_transaction_readonly.

Yeah, that's what I was wondering about. I don't think pg_dumpall -g
invokes pg_dump at all, so how could that case work? Maybe it would
only fail if the postgres database is read-only, though. Try it with
default-read-only set in postgresql.conf instead of as a database
property.

regards, tom lane

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-11-22 13:07:29 -0800, Kevin Grittner wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>
>>> are you sure it also unsets default_transaction_readonly for
>>> the roles etc. it creates?
>>
>> I'm not sure I understand.  Could you give an example of what
>> you're concerned about?
>
> pg_dumpall first spits out global data (users, databases, tablespaces)
> and then invokes pg_dump for every single database. So I'd very strongly
> suspect that your patch will issue the CREATE ROLE etc. calls without
> unsetting default_transaction_readonly.

I changed my postgres database to default to read only (which is
not insane, given that I've seen so many people accidentally run
DDL there rather than in the database they intended), and got these
errors when I used it for my connection to restore pg_dumpall
output:

ERROR:  cannot execute COMMENT in a read-only transaction
ERROR:  cannot execute CREATE EXTENSION in a read-only transaction
ERROR:  cannot execute COMMENT in a read-only transaction
ERROR:  cannot execute REVOKE in a read-only transaction
ERROR:  cannot execute REVOKE in a read-only transaction
ERROR:  cannot execute GRANT in a read-only transaction
ERROR:  cannot execute GRANT in a read-only transaction

Oddly, it didn't complain about creating users within a read-only
transaction.  That seems like a potential bug.

Will look at covering pg_dumpall for that initial connection.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-11-22 13:34:18 -0800, Kevin Grittner wrote:
> Oddly, it didn't complain about creating users within a read-only
> transaction.  That seems like a potential bug.

There's lots of things that escape XactReadOnly. I've thought (and I
think suggested) before that we should put in another layer of defense
by also putting a check in AssignTransactionId(). Imo the compatibility
woes (like not being able to run SELECT txid_current();) are well worth
the nearly ironclad guarantee that we're not writing.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:

This covers pg_dumpall globals.  Tested with a read-only postgres
database and with default_transaction_read_only = on in the
postgresql.conf file.

It does nothing about pg_upgrade, which is sort of a separate
issue.  My inclination is that connections to the new cluster
should set this and connections to the old should not.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-11-22 13:34:18 -0800, Kevin Grittner wrote:
> I changed my postgres database to default to read only (which is
> not insane, given that I've seen so many people accidentally run
> DDL there rather than in the database they intended)

FWIW, I am less than convinced that it is correct for pg_dump[all] to
emit SET default_transaction_readonly = off; The user might explicitly
have set that to prevent against somebody restoring into the wrong
database or somesuch. Why is it our business to override such decisions?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> FWIW, I am less than convinced that it is correct for
> pg_dump[all] to emit SET default_transaction_readonly = off;

It doesn't change the database setting, just the connection which
is issuing commands to create global object -- ones that don't
reside in the database we are connected to.  As the comment in
pg_dumpall.c says, right above where I added this:

    /*
     * We used to emit \connect postgres here, but that served no purpose
     * other than to break things for installations without a postgres
     * database.  Everything we're restoring here is a global, so whichever
     * database we're connected to at the moment is fine.
     */

> The user might explicitly have set that to prevent against
> somebody restoring into the wrong database or somesuch. Why is it
> our business to override such decisions?

Hmm.  If your argument had been that the user intended that the
database be a read-only database, then I would say that your
argument held no water.  Any user can choose to make any
transaction (or all subsequent transactions on the connection)
read-write any time they want.  The problem with pg_dumpall as it
stands is that it sets the databases to that default and then tries
to load data into them, which fails.

But you have a point regarding adding what I proposed to pg_dump.
The more I think about it, the more I'm inclined to think that
pg_dumpall should insert this right after the \connect to a
database it is going to write to, rather than affecting pg_dump
output at all. That would allow you default protection against
writing pg_dump output to a database that was flagged this way.
You could get around it by connecting interactively with psql,
issuing a SET command, and bringing in dump output with \i from a
text file.  If we go this way, would we want options on pg_restore
and/or psql to issue this set when reading in a file (or piped
input)?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Nov 22, 2013 at 01:55:10PM -0800, Kevin Grittner wrote:
> It does nothing about pg_upgrade, which is sort of a separate
> issue.  My inclination is that connections to the new cluster
> should set this and connections to the old should not.

It is going to be tricky to conditionally set/reset an environment
variable for default_transaction_read_only for old/new clusters. We
never write to the old cluster, so I am not sure setting/resetting
default_transaction_read_only is needed.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Fri, Nov 22, 2013 at 01:55:10PM -0800, Kevin Grittner wrote:
>
>> It does nothing about pg_upgrade, which is sort of a separate
>> issue.  My inclination is that connections to the new cluster
>> should set this and connections to the old should not.
>
> It is going to be tricky to conditionally set/reset an
> environment variable for default_transaction_read_only for
> old/new clusters.

Why?  If you know you have connected to the new cluster, set it to
off; if you know you have connected to the old cluster, don't touch
it.

> We never write to the old cluster, so I am not sure
> setting/resetting default_transaction_read_only is needed.

I'm sure it isn't.  That's why I said that connections to the old
cluster should not set it -- by which I meant they should not do
anything with it.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-11-22 13:34:18 -0800, Kevin Grittner wrote:
>> Oddly, it didn't complain about creating users within a read-only
>> transaction.  That seems like a potential bug.
>
> There's lots of things that escape XactReadOnly. I've thought (and I
> think suggested) before that we should put in another layer of defense
> by also putting a check in AssignTransactionId(). Imo the compatibility
> woes (like not being able to run SELECT txid_current();) are well worth
> the nearly ironclad guarantee that we're not writing.

I agree that something like that is would be a good idea; however,
I'm sure you would agree that would not be material for a
back-patch to a stable branch.

Another thing I've mused about is having some way to lock a
database to read-only, such that only the owner or a superuser
could change that.  Another setting which I know some people would
like to lock is transaction isolation level.  I haven't really
thought of a good UI for that sort of thing, though.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Nov 22, 2013 at 04:25:57PM -0800, Kevin Grittner wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > On Fri, Nov 22, 2013 at 01:55:10PM -0800, Kevin Grittner wrote:
> >
> >> It does nothing about pg_upgrade, which is sort of a separate
> >> issue.  My inclination is that connections to the new cluster
> >> should set this and connections to the old should not.
> >
> > It is going to be tricky to conditionally set/reset an
> > environment variable for default_transaction_read_only for
> > old/new clusters.
>
> Why?  If you know you have connected to the new cluster, set it to
> off; if you know you have connected to the old cluster, don't touch
> it.

Remember this is being done with a PGOPTIONS environment variable, so
you need to set/reset this on every action. I just don't see the value.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Fri, Nov 22, 2013 at 04:38:53PM -0800, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-11-22 13:34:18 -0800, Kevin Grittner wrote:
> >> Oddly, it didn't complain about creating users within a read-only
> >> transaction.  That seems like a potential bug.
> >
> > There's lots of things that escape XactReadOnly. I've thought (and I
> > think suggested) before that we should put in another layer of defense
> > by also putting a check in AssignTransactionId(). Imo the compatibility
> > woes (like not being able to run SELECT txid_current();) are well worth
> > the nearly ironclad guarantee that we're not writing.
>
> I agree that something like that is would be a good idea; however,
> I'm sure you would agree that would not be material for a
> back-patch to a stable branch.

I am not a fan of backpatching any of this. We have learned the fix is
more complex than thought, and the risk of breakage and having pg_dump
diffs change between minor releases doesn't seem justified.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> wrote:

> I am not a fan of backpatching any of this.

Here's my problem with that.  Here's setup to create what I don't
think is all that weird a setup:

initdb Debug/data
pg_ctl -D Debug/data -l Debug/data/logfile -w start
createdb test
psql test <src/test/regress/sql/matview.sql >/dev/null 2>&1
psql postgres -c "alter database test set default_transaction_read_only = on;"
psql postgres -c "alter database postgres set default_transaction_read_only = on;"

The following appears to produce a good backup, since there is no
error:

pg_dumpall >~/dumpall.sql

Let's create a brand new cluster and start it up:

pg_ctl -D Debug/data -m fast -w stop
rm -fr Debug/data/*
initdb Debug/data
pg_ctl -D Debug/data -l Debug/data/logfile -w start

Now we attempt to restore what we thought was a good backup:

psql postgres <~/dumpall.sql

What we get is:

SET
SET
ERROR:  role "kgrittn" already exists
ALTER ROLE
ALTER DATABASE
REVOKE
REVOKE
GRANT
GRANT
CREATE DATABASE
ALTER DATABASE
You are now connected to database "postgres" as user "kgrittn".
SET
SET
SET
SET
SET
SET
ERROR:  cannot execute COMMENT in a read-only transaction
ERROR:  cannot execute CREATE EXTENSION in a read-only transaction
ERROR:  cannot execute COMMENT in a read-only transaction
ERROR:  cannot execute REVOKE in a read-only transaction
ERROR:  cannot execute REVOKE in a read-only transaction
ERROR:  cannot execute GRANT in a read-only transaction
ERROR:  cannot execute GRANT in a read-only transaction
You are now connected to database "template1" as user "kgrittn".
SET
SET
SET
SET
SET
SET
COMMENT
CREATE EXTENSION
COMMENT
REVOKE
REVOKE
GRANT
GRANT
You are now connected to database "test" as user "kgrittn".
SET
SET
SET
SET
SET
SET
ERROR:  cannot execute CREATE SCHEMA in a read-only transaction
ERROR:  cannot execute ALTER SCHEMA in a read-only transaction
ERROR:  cannot execute CREATE EXTENSION in a read-only transaction
ERROR:  cannot execute COMMENT in a read-only transaction
SET
SET
SET
ERROR:  cannot execute CREATE TABLE in a read-only transaction
ERROR:  cannot execute ALTER TABLE in a read-only transaction
ERROR:  cannot execute CREATE VIEW in a read-only transaction
ERROR:  cannot execute ALTER TABLE in a read-only transaction
SET
ERROR:  relation "public.tv" does not exist
LINE 4:    FROM public.tv
                ^
ERROR:  cannot execute ALTER TABLE in a read-only transaction
SET
ERROR:  cannot execute CREATE VIEW in a read-only transaction
ERROR:  cannot execute ALTER TABLE in a read-only transaction
ERROR:  relation "tvv" does not exist
LINE 3:    FROM tvv
                ^
ERROR:  cannot execute ALTER TABLE in a read-only transaction
ERROR:  cannot execute CREATE VIEW in a read-only transaction
ERROR:  cannot execute ALTER TABLE in a read-only transaction
ERROR:  relation "tvvmv" does not exist
LINE 3:    FROM tvvmv
                ^
ERROR:  cannot execute ALTER TABLE in a read-only transaction
ERROR:  relation "t" does not exist
LINE 4:    FROM t
                ^
ERROR:  cannot execute ALTER TABLE in a read-only transaction
ERROR:  relation "tm" does not exist
LINE 3:    FROM tm
                ^
ERROR:  cannot execute ALTER TABLE in a read-only transaction
ERROR:  relation "mvschema.tvm" does not exist
LINE 3:    FROM mvschema.tvm
                ^
ERROR:  cannot execute ALTER TABLE in a read-only transaction
ERROR:  relation "t" does not exist
invalid command \.
ERROR:  syntax error at or near "1"
LINE 1: 1 x 2
        ^
ERROR:  cannot execute CREATE INDEX in a read-only transaction
ERROR:  cannot execute CREATE INDEX in a read-only transaction
ERROR:  cannot execute CREATE INDEX in a read-only transaction
ERROR:  cannot execute CREATE INDEX in a read-only transaction
SET
ERROR:  cannot execute REFRESH MATERIALIZED VIEW in a read-only transaction
SET
ERROR:  cannot execute REFRESH MATERIALIZED VIEW in a read-only transaction
ERROR:  cannot execute REFRESH MATERIALIZED VIEW in a read-only transaction
ERROR:  cannot execute REFRESH MATERIALIZED VIEW in a read-only transaction
ERROR:  cannot execute REFRESH MATERIALIZED VIEW in a read-only transaction
ERROR:  cannot execute REFRESH MATERIALIZED VIEW in a read-only transaction
ERROR:  cannot execute REVOKE in a read-only transaction
ERROR:  cannot execute REVOKE in a read-only transaction
ERROR:  cannot execute GRANT in a read-only transaction
ERROR:  cannot execute GRANT in a read-only transaction

If the dump is made with the attached patch, you get this on
restore:

SET
SET
SET
ERROR:  role "kgrittn" already exists
ALTER ROLE
ALTER DATABASE
REVOKE
REVOKE
GRANT
GRANT
CREATE DATABASE
ALTER DATABASE
You are now connected to database "postgres" as user "kgrittn".
SET
SET
SET
SET
SET
SET
SET
COMMENT
CREATE EXTENSION
COMMENT
REVOKE
REVOKE
GRANT
GRANT
You are now connected to database "template1" as user "kgrittn".
SET
SET
SET
SET
SET
SET
SET
COMMENT
CREATE EXTENSION
COMMENT
REVOKE
REVOKE
GRANT
GRANT
You are now connected to database "test" as user "kgrittn".
SET
SET
SET
SET
SET
SET
SET
CREATE SCHEMA
ALTER SCHEMA
CREATE EXTENSION
COMMENT
SET
SET
SET
CREATE TABLE
ALTER TABLE
CREATE VIEW
ALTER TABLE
SET
SELECT 0
ALTER TABLE
SET
CREATE VIEW
ALTER TABLE
SELECT 0
ALTER TABLE
CREATE VIEW
ALTER TABLE
SELECT 0
ALTER TABLE
SELECT 0
ALTER TABLE
SELECT 0
ALTER TABLE
SELECT 0
ALTER TABLE
ALTER TABLE
CREATE INDEX
CREATE INDEX
CREATE INDEX
CREATE INDEX
SET
REFRESH MATERIALIZED VIEW
SET
REFRESH MATERIALIZED VIEW
REFRESH MATERIALIZED VIEW
REFRESH MATERIALIZED VIEW
REFRESH MATERIALIZED VIEW
REFRESH MATERIALIZED VIEW
REVOKE
REVOKE
GRANT
GRANT
SET
SET
SET
ERROR:  role "kgrittn" already exists
ALTER ROLE
ALTER DATABASE
REVOKE
REVOKE
GRANT
GRANT
CREATE DATABASE
ALTER DATABASE
You are now connected to database "postgres" as user "kgrittn".
SET
SET
SET
SET
SET
SET
SET
COMMENT
CREATE EXTENSION
COMMENT
REVOKE
REVOKE
GRANT
GRANT
You are now connected to database "template1" as user "kgrittn".
SET
SET
SET
SET
SET
SET
SET
COMMENT
CREATE EXTENSION
COMMENT
REVOKE
REVOKE
GRANT
GRANT
You are now connected to database "test" as user "kgrittn".
SET
SET
SET
SET
SET
SET
SET
CREATE SCHEMA
ALTER SCHEMA
CREATE EXTENSION
COMMENT
SET
SET
SET
CREATE TABLE
ALTER TABLE
CREATE VIEW
ALTER TABLE
SET
SELECT 0
ALTER TABLE
SET
CREATE VIEW
ALTER TABLE
SELECT 0
ALTER TABLE
CREATE VIEW
ALTER TABLE
SELECT 0
ALTER TABLE
SELECT 0
ALTER TABLE
SELECT 0
ALTER TABLE
SELECT 0
ALTER TABLE
ALTER TABLE
CREATE INDEX
CREATE INDEX
CREATE INDEX
CREATE INDEX
SET
REFRESH MATERIALIZED VIEW
SET
REFRESH MATERIALIZED VIEW
REFRESH MATERIALIZED VIEW
REFRESH MATERIALIZED VIEW
REFRESH MATERIALIZED VIEW
REFRESH MATERIALIZED VIEW
REVOKE
REVOKE
GRANT
GRANT

The cluster is created in the state that was dumped, default read
only flags and all.

Are you saying that you find current behavior acceptable in back
branches?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Nov 23, 2013 at 08:44:42AM -0800, Kevin Grittner wrote:

> Here's my problem with that.  Here's setup to create what I don't
> think is all that weird a setup:

...

> The following appears to produce a good backup, since there is no
> error:

...

> Now we attempt to restore what we thought was a good backup:
>
> psql postgres <~/dumpall.sql
>
> What we get is:

> ERROR:  cannot execute COMMENT in a read-only transaction
> ERROR:  cannot execute CREATE EXTENSION in a read-only transaction
> ERROR:  cannot execute COMMENT in a read-only transaction
> ERROR:  cannot execute REVOKE in a read-only transaction
> ERROR:  cannot execute REVOKE in a read-only transaction
> ERROR:  cannot execute GRANT in a read-only transaction
> ERROR:  cannot execute GRANT in a read-only transaction
...
> ERROR:  cannot execute CREATE SCHEMA in a read-only transaction
> ERROR:  cannot execute ALTER SCHEMA in a read-only transaction
> ERROR:  cannot execute CREATE EXTENSION in a read-only transaction
> ERROR:  cannot execute COMMENT in a read-only transaction

...

> If the dump is made with the attached patch, you get this on
> restore:
...
> The cluster is created in the state that was dumped, default read
> only flags and all.

I find the patched behaviour more conformant
with the Principle Of Least Astonishment.

Maybe I am splitting hairs but setting transactions to readonly
per default does not mean the database *as such* is to be readonly.
It literally applies to the *default* state of transactions (as
opposed to the ONLY state of transactions). No more, no less.

Karsten
--
GPG key ID E4071346 @ gpg-keyserver.de
E167 67FD A291 2BEA 73BD 4537 78B9 A9F9 E407 1346

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> I am not a fan of backpatching any of this.

> Are you saying that you find current behavior acceptable in back
> branches?

I'm inclined to agree with Kevin that this behavior is wrong and
should be fixed (and back-patched), so far as pg_dumpall is concerned.
pg_dumpall's charter is to be able to recreate a database cluster's
contents in a virgin installation, but it's failing to honor that
contract if the cluster has any ALTER DATABASE SET default_read_only
settings. Similarly, I think it's reasonable to try to make pg_upgrade
cope with the case.

I also agree with *not* changing pg_dump, since it is not the charter
of pg_dump to recreate a whole cluster, and the objection about possibly
restoring into a database that was meant to be protected by this setting
seems to have some force.

regards, tom lane

On Sat, Nov 23, 2013 at 12:11:56PM -0500, Tom Lane wrote:

> I also agree with *not* changing pg_dump, since it is not the charter
> of pg_dump to recreate a whole cluster, and the objection about possibly
> restoring into a database that was meant to be protected by this setting
> seems to have some force.

This is were the suggestion comes in (which was already raised)
to add some commandline option to the effect of

--ignore-default-tx-readonly

which, however, I agree with can be worked around by
projects (like GNUmed, in which context this issue
came up at all) providing restore scripts setting
PGOPTIONS appropriately ...

Thanks for all the work,
Karsten
--
GPG key ID E4071346 @ gpg-keyserver.de
E167 67FD A291 2BEA 73BD 4537 78B9 A9F9 E407 1346

Am Samstag, 23. November 2013, 08:44:42 schrieb Kevin Grittner:
> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > I am not a fan of backpatching any of this.
>
> Here's my problem with that. Here's setup to create what I don't
> think is all that weird a setup:
>
> initdb Debug/data
> pg_ctl -D Debug/data -l Debug/data/logfile -w start
> createdb test
> psql test <src/test/regress/sql/matview.sql >/dev/null 2>&1
> psql postgres -c "alter database test set default_transaction_read_only =
> on;" psql postgres -c "alter database postgres set
> default_transaction_read_only = on;"
>
> The following appears to produce a good backup, since there is no
> error:
>
> pg_dumpall >~/dumpall.sql
>
> Let's create a brand new cluster and start it up:
>
> pg_ctl -D Debug/data -m fast -w stop
> rm -fr Debug/data/*
> initdb Debug/data
> pg_ctl -D Debug/data -l Debug/data/logfile -w start
>
> Now we attempt to restore what we thought was a good backup:
>
> psql postgres <~/dumpall.sql
>
> What we get is:
>
> SET
> SET
> ERROR: role "kgrittn" already exists
> ALTER ROLE
> ALTER DATABASE
> REVOKE
> REVOKE
> GRANT
> GRANT
> CREATE DATABASE
> ALTER DATABASE
> You are now connected to database "postgres" as user "kgrittn".
> SET
> SET
> SET
> SET
> SET
> SET
> ERROR: cannot execute COMMENT in a read-only transaction
> ERROR: cannot execute CREATE EXTENSION in a read-only transaction
> ERROR: cannot execute COMMENT in a read-only transaction
> ERROR: cannot execute REVOKE in a read-only transaction
> ERROR: cannot execute REVOKE in a read-only transaction
> ERROR: cannot execute GRANT in a read-only transaction
> ERROR: cannot execute GRANT in a read-only transaction
> You are now connected to database "template1" as user "kgrittn".
> SET
> SET
> SET
> SET
> SET
> SET
> COMMENT
> CREATE EXTENSION
> COMMENT
> REVOKE
> REVOKE
> GRANT
> GRANT
> You are now connected to database "test" as user "kgrittn".
> SET
> SET
> SET
> SET
> SET
> SET
> ERROR: cannot execute CREATE SCHEMA in a read-only transaction
> ERROR: cannot execute ALTER SCHEMA in a read-only transaction
> ERROR: cannot execute CREATE EXTENSION in a read-only transaction
> ERROR: cannot execute COMMENT in a read-only transaction
> SET
> SET
> SET
> ERROR: cannot execute CREATE TABLE in a read-only transaction
> ERROR: cannot execute ALTER TABLE in a read-only transaction
> ERROR: cannot execute CREATE VIEW in a read-only transaction
> ERROR: cannot execute ALTER TABLE in a read-only transaction
> SET
> ERROR: relation "public.tv" does not exist
> LINE 4: FROM public.tv
> ^
> ERROR: cannot execute ALTER TABLE in a read-only transaction
> SET
> ERROR: cannot execute CREATE VIEW in a read-only transaction
> ERROR: cannot execute ALTER TABLE in a read-only transaction
> ERROR: relation "tvv" does not exist
> LINE 3: FROM tvv
> ^
> ERROR: cannot execute ALTER TABLE in a read-only transaction
> ERROR: cannot execute CREATE VIEW in a read-only transaction
> ERROR: cannot execute ALTER TABLE in a read-only transaction
> ERROR: relation "tvvmv" does not exist
> LINE 3: FROM tvvmv
> ^
> ERROR: cannot execute ALTER TABLE in a read-only transaction
> ERROR: relation "t" does not exist
> LINE 4: FROM t
> ^
> ERROR: cannot execute ALTER TABLE in a read-only transaction
> ERROR: relation "tm" does not exist
> LINE 3: FROM tm
> ^
> ERROR: cannot execute ALTER TABLE in a read-only transaction
> ERROR: relation "mvschema.tvm" does not exist
> LINE 3: FROM mvschema.tvm
> ^
> ERROR: cannot execute ALTER TABLE in a read-only transaction
> ERROR: relation "t" does not exist
> invalid command \.
> ERROR: syntax error at or near "1"
> LINE 1: 1 x 2
> ^
> ERROR: cannot execute CREATE INDEX in a read-only transaction
> ERROR: cannot execute CREATE INDEX in a read-only transaction
> ERROR: cannot execute CREATE INDEX in a read-only transaction
> ERROR: cannot execute CREATE INDEX in a read-only transaction
> SET
> ERROR: cannot execute REFRESH MATERIALIZED VIEW in a read-only transaction
> SET
> ERROR: cannot execute REFRESH MATERIALIZED VIEW in a read-only transaction
> ERROR: cannot execute REFRESH MATERIALIZED VIEW in a read-only transaction
> ERROR: cannot execute REFRESH MATERIALIZED VIEW in a read-only transaction
> ERROR: cannot execute REFRESH MATERIALIZED VIEW in a read-only transaction
> ERROR: cannot execute REFRESH MATERIALIZED VIEW in a read-only transaction
> ERROR: cannot execute REVOKE in a read-only transaction
> ERROR: cannot execute REVOKE in a read-only transaction
> ERROR: cannot execute GRANT in a read-only transaction
> ERROR: cannot execute GRANT in a read-only transaction
>
> If the dump is made with the attached patch, you get this on
> restore:
>
> SET
> SET
> SET
> ERROR: role "kgrittn" already exists
> ALTER ROLE
> ALTER DATABASE
> REVOKE
> REVOKE
> GRANT
> GRANT
> CREATE DATABASE
> ALTER DATABASE
> You are now connected to database "postgres" as user "kgrittn".
> SET
> SET
> SET
> SET
> SET
> SET
> SET
> COMMENT
> CREATE EXTENSION
> COMMENT
> REVOKE
> REVOKE
> GRANT
> GRANT
> You are now connected to database "template1" as user "kgrittn".
> SET
> SET
> SET
> SET
> SET
> SET
> SET
> COMMENT
> CREATE EXTENSION
> COMMENT
> REVOKE
> REVOKE
> GRANT
> GRANT
> You are now connected to database "test" as user "kgrittn".
> SET
> SET
> SET
> SET
> SET
> SET
> SET
> CREATE SCHEMA
> ALTER SCHEMA
> CREATE EXTENSION
> COMMENT
> SET
> SET
> SET
> CREATE TABLE
> ALTER TABLE
> CREATE VIEW
> ALTER TABLE
> SET
> SELECT 0
> ALTER TABLE
> SET
> CREATE VIEW
> ALTER TABLE
> SELECT 0
> ALTER TABLE
> CREATE VIEW
> ALTER TABLE
> SELECT 0
> ALTER TABLE
> SELECT 0
> ALTER TABLE
> SELECT 0
> ALTER TABLE
> SELECT 0
> ALTER TABLE
> ALTER TABLE
> CREATE INDEX
> CREATE INDEX
> CREATE INDEX
> CREATE INDEX
> SET
> REFRESH MATERIALIZED VIEW
> SET
> REFRESH MATERIALIZED VIEW
> REFRESH MATERIALIZED VIEW
> REFRESH MATERIALIZED VIEW
> REFRESH MATERIALIZED VIEW
> REFRESH MATERIALIZED VIEW
> REVOKE
> REVOKE
> GRANT
> GRANT
> SET
> SET
> SET
> ERROR: role "kgrittn" already exists
> ALTER ROLE
> ALTER DATABASE
> REVOKE
> REVOKE
> GRANT
> GRANT
> CREATE DATABASE
> ALTER DATABASE
> You are now connected to database "postgres" as user "kgrittn".
> SET
> SET
> SET
> SET
> SET
> SET
> SET
> COMMENT
> CREATE EXTENSION
> COMMENT
> REVOKE
> REVOKE
> GRANT
> GRANT
> You are now connected to database "template1" as user "kgrittn".
> SET
> SET
> SET
> SET
> SET
> SET
> SET
> COMMENT
> CREATE EXTENSION
> COMMENT
> REVOKE
> REVOKE
> GRANT
> GRANT
> You are now connected to database "test" as user "kgrittn".
> SET
> SET
> SET
> SET
> SET
> SET
> SET
> CREATE SCHEMA
> ALTER SCHEMA
> CREATE EXTENSION
> COMMENT
> SET
> SET
> SET
> CREATE TABLE
> ALTER TABLE
> CREATE VIEW
> ALTER TABLE
> SET
> SELECT 0
> ALTER TABLE
> SET
> CREATE VIEW
> ALTER TABLE
> SELECT 0
> ALTER TABLE
> CREATE VIEW
> ALTER TABLE
> SELECT 0
> ALTER TABLE
> SELECT 0
> ALTER TABLE
> SELECT 0
> ALTER TABLE
> SELECT 0
> ALTER TABLE
> ALTER TABLE
> CREATE INDEX
> CREATE INDEX
> CREATE INDEX
> CREATE INDEX
> SET
> REFRESH MATERIALIZED VIEW
> SET
> REFRESH MATERIALIZED VIEW
> REFRESH MATERIALIZED VIEW
> REFRESH MATERIALIZED VIEW
> REFRESH MATERIALIZED VIEW
> REFRESH MATERIALIZED VIEW
> REVOKE
> REVOKE
> GRANT
> GRANT
>
> The cluster is created in the state that was dumped, default read
> only flags and all.
>
> Are you saying that you find current behavior acceptable in back
> branches?
>

Here is how this came about.

Installation of PG 8.4 (port 5432) on Windows with default settings.
Creation of a test database
Installation of PG 9.3 on Windows (port 5433) with default settings

Starting up pg_upgrade as postgres
--> fails

c:\Windows\Temp>pg_upgrade.exe --old-datadir "C:/Program Files
(x86)/PostgresPlus/8.4SS/data" --new-datadir "C:/Program Files
(x86)/PostgreSQL/9.3/data" --old-bindir "C:/Program Files
(x86)/PostgresPlus/8.4SS/bin" --new-bindir "C:/Program F
iles (x86)/PostgreSQL/9.3/bin"

SQL command failed
CREATE TEMPORARY TABLE info_rels (reloid) AS SELECT c.oid FROM
pg_catalog.pg_cla
ss c JOIN pg_catalog.pg_namespace n ON c.relnamespace = n.oid LEFT
OUTER
JOIN pg_catalog.pg_index i ON c.oid = i.indexrelid WHERE relkind IN
('r'
, 'm', 'i', 'S') AND i.indisvalid IS DISTINCT FROM false AND i.indisready IS
D
ISTINCT FROM false AND ((n.nspname !~ '^pg_temp_' AND n.nspname !~
'^pg_to
ast_temp_' AND n.nspname NOT IN ('pg_catalog', 'information_schema',
'binary_upgrade', 'pg_toast') AND
c.oid >= 16384) OR (n.nspname = 'pg_catalog' AND relname IN
('pg_largeob
ject', 'pg_largeobject_loid_pn_index') ));
ERROR: transaction is read-only

Regards,
Sebastian

On Sat, Nov 23, 2013 at 08:44:42AM -0800, Kevin Grittner wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> > I am not a fan of backpatching any of this.
>
> Here's my problem with that.  Here's setup to create what I don't
> think is all that weird a setup:
>
> The cluster is created in the state that was dumped, default read
> only flags and all.
>
> Are you saying that you find current behavior acceptable in back
> branches?

First, I don't need to see a 300-line pg_dump restore output to know it
is a bug. Second, what you didn't do is to address the rest of my
paragraph:

> I am not a fan of backpatching any of this. We have learned the fix is
> more complex than thought, and the risk of breakage and having pg_dump
> diffs change between minor releases doesn't seem justified.

We have to balance the _one_ user failure report we have received vs.
potential breakage.

Now, others seem to be fine with a backpatch, so perhaps it is safe. I
am merely pointing out that, with all backpatching, we have to balance
the fix against possible breakage and behavioral change.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Kevin Grittner wrote:
>> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>>
>>> I am not a fan of backpatching any of this.
>>
>> Here's my problem with that.  Here's setup to create what I
>> don't think is all that weird a setup:
>>
>> The cluster is created in the state that was dumped, default
>> read only flags and all.
>>
>> Are you saying that you find current behavior acceptable in back
>> branches?
>
> First, I don't need to see a 300-line pg_dump restore output to
> know it is a bug.

I was trying to save people the trouble of working up their own
test to see what happened when running pg_dumpall output from a
successful run into a freshly initdb'd cluster.  No offense
intended.

> Second, what you didn't do is to address the rest of my
> paragraph:
>
>> I am not a fan of backpatching any of this.  We have learned the
>> fix is more complex than thought,

I didn't realize that anyone thought the pg_dumpall fix would
amount to something simpler than two printf statements; but even
so, that seems pretty reasonable to me.

>> and the risk of breakage and having pg_dump diffs change between
>> minor releases doesn't seem justified.

You might have missed the part of the thread where we seemed to
have reached a consensus that pg_dump output would not change at
all; only pg_dumpall output -- to something capable of being
restored to a fresh cluster.

> We have to balance the _one_ user failure report we have received
> vs. potential breakage.

What breakage mode do you see?

> Now, others seem to be fine with a backpatch, so perhaps it is
> safe.  I am merely pointing out that, with all backpatching, we
> have to balance the fix against possible breakage and behavioral
> change.

Sure, but I think a bug in a dump utility which allows it to run
with apparent success while generating results which don't restore
is pretty clearly something to fix.  FWIW I don't feel as strongly
about the pg_upgrade aspect of this -- as long as a test run
reports that the cluster can't be upgraded without changing those
settings, there is no problem.  Of course it would be nice if it
could be fixed instead, but that's not as critical in my view.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Nov 25, 2013 at 02:24:25PM -0800, Kevin Grittner wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Kevin Grittner wrote:
> >> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >>
> >>> I am not a fan of backpatching any of this.
> >>
> >> Here's my problem with that.  Here's setup to create what I
> >> don't think is all that weird a setup:
> >>
> >> The cluster is created in the state that was dumped, default
> >> read only flags and all.
> >>
> >> Are you saying that you find current behavior acceptable in back
> >> branches?
> >
> > First, I don't need to see a 300-line pg_dump restore output to
> > know it is a bug.
>
> I was trying to save people the trouble of working up their own
> test to see what happened when running pg_dumpall output from a
> successful run into a freshly initdb'd cluster.  No offense
> intended.

Fine, but were 300 lines of output necessary?

> > Second, what you didn't do is to address the rest of my
> > paragraph:
> >
> >> I am not a fan of backpatching any of this.  We have learned the
> >> fix is more complex than thought,
>
> I didn't realize that anyone thought the pg_dumpall fix would
> amount to something simpler than two printf statements; but even
> so, that seems pretty reasonable to me.

Well, if I say "more complex than thought", it means not just the patch,
but the impact it might have. We are pushing out a replication fix in a
few weeks because of backpatches that people thought were safe.

For example, if we backpatch, we get zero user testing. If we did it
wrong somehow, it is much harder to fix later. For example, what is the
logic that a per-database setting of statement_timeout is reset in
pg_dump, but default_transaction_read_only is reset in pg_dumpall? Can
we know we have thought this all through with zero user testing?

> >> and the risk of breakage and having pg_dump diffs change between
> >> minor releases doesn't seem justified.
>
> You might have missed the part of the thread where we seemed to
> have reached a consensus that pg_dump output would not change at
> all; only pg_dumpall output -- to something capable of being
> restored to a fresh cluster.

Yes, I saw that. My point was that we have had to move around the patch
to fix the problem, meaning the fix was not obvious. The number of
lines is only part of a measure of a patch's riskiness.

> > We have to balance the _one_ user failure report we have received
> > vs. potential breakage.
>
> What breakage mode do you see?

Uh, I said "potential breakage." If I knew of the breakage, I would
have said so.

> > Now, others seem to be fine with a backpatch, so perhaps it is
> > safe.  I am merely pointing out that, with all backpatching, we
> > have to balance the fix against possible breakage and behavioral
> > change.
>
> Sure, but I think a bug in a dump utility which allows it to run
> with apparent success while generating results which don't restore
> is pretty clearly something to fix.  FWIW I don't feel as strongly

pg_dumpall certainly reduces the diff risk.

> about the pg_upgrade aspect of this -- as long as a test run
> reports that the cluster can't be upgraded without changing those
> settings, there is no problem.  Of course it would be nice if it
> could be fixed instead, but that's not as critical in my view.

How are we handling breakage of pg_dump, not pg_dumpall? doc patch?
pg_upgrade probably needs a doc patch too, or a reset like pg_dumpall.
pg_upgrade is more like pg_dumpall, so a code patch seems most logical,
again, assuming we decide that pg_dumpall is the right place for this
reset of default_transaction_read_only.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> wrote:

> How are we handling breakage of pg_dump, not pg_dumpall?

That was discussed.  Do you have something to add?

> doc patch?

Instead of the fix you mean, or with it?  I don't see what we would
change in the docs for the fix; the alternative might be to
document that pg_dumpall output will fail to restore if any
database (or the restoring user) has this property set.

> pg_upgrade probably needs a doc patch too, or a reset like
> pg_dumpall.  pg_upgrade is more like pg_dumpall, so a code patch
> seems most logical, again, assuming we decide that pg_dumpall is
> the right place for this reset of default_transaction_read_only.

I don't have much opinion on what the pg_upgrade aspect except,
like I said, that if it is going to fail, it should fail in the
check.  Passing the check but failing during the upgrade would not
be very user-friendly.  Again, I'm not sure that a doc patch is
needed to say that pg_upgrade works even when this option is set.
Why would anyone assume otherwise?  Why would we list this property
and not others?

I'm willing to do the pg_dumpall patch but would rather not take on
pg_upgrade.  If you would rather I leave the whole thing to you,
that's OK, too.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Nov 26, 2013 at 03:25:44PM -0800, Kevin Grittner wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> > How are we handling breakage of pg_dump, not pg_dumpall?
>
> That was discussed.  Do you have something to add?

I am confused what we are patching. Are we patching pg_dump,
pg_dumpall, or both? Can I see the full patch? Are we propagating
other settings from pg_dump to pg_dumpall, like statement_timeout?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Tue, Nov 26, 2013 at 03:25:44PM -0800, Kevin Grittner wrote:

> > doc patch?
>
> Instead of the fix you mean, or with it?  I don't see what we would
> change in the docs for the fix; the alternative might be to
> document that pg_dumpall output will fail to restore if any
> database (or the restoring user) has this property set.

a) since pg_dump is not planned to be changed to deal with
it a hint in the pg_dump docs would be helpful

I can fully understand the argument that if the dump
does NOT contain a "create database" the restore
should indeed honor the existing database setting.

In case it does contain a "create database" statement
the issue won't exist -- because the database will
need to be gone before the restore.

b) if pg_ugprade is not fixed then a hint in the docs
is useful (see below)

> > pg_upgrade probably needs a doc patch too, or a reset like
> > pg_dumpall.  pg_upgrade is more like pg_dumpall, so a code patch
> > seems most logical, again, assuming we decide that pg_dumpall is
> > the right place for this reset of default_transaction_read_only.
>
> I don't have much opinion on what the pg_upgrade aspect except,

The situation is this:

naive user:
- installs new PG version
- upgrades old cluster with one default_read_only database
- upgrade fails (or check fails - latter requires patch)
- user asks someone
- user unsets default_read_only
- upgrades old cluster
- upgrade succeeds
- user re-sets default_read_only

careful user:
- installs new PG version
- reads pg_upgrade docs of new version (requires doc patch)
- user unsets default_read_only
- upgrades old cluster
- upgrade succeeds
- user re-sets default_read_only

I can't for the life of it think of a scenario where
one would go: Oh, I've got a default_read_only
database -- let's NOT upgrade the cluster.

> check.  Passing the check but failing during the upgrade would not
> be very user-friendly.  Again, I'm not sure that a doc patch is
> needed to say that pg_upgrade works even when this option is set.
> Why would anyone assume otherwise?  Why would we list this property
> and not others?

A doc patch is only needed if pg_upgrade does NOT work
when some databases are default_read_only because THAT
is counterintuitive: To the naive user upgrading from
version to version is like "make a copy, perhaps rename
a file or two". It doesn't intuitively suggest a need
for WRITE access to individual databases themselves.

However, if the current behaviour is retained it would
be very useful to document that and also document one
or two ways around it (unsetting, PGOPTIONS, ...).

Karsten
--
GPG key ID E4071346 @ gpg-keyserver.de
E167 67FD A291 2BEA 73BD 4537 78B9 A9F9 E407 1346

Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Tue, Nov 26, 2013 at 03:25:44PM -0800, Kevin Grittner wrote:
>> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>>
>>> How are we handling breakage of pg_dump, not pg_dumpall?
>>
>> That was discussed.  Do you have something to add?
>
> I am confused what we are patching.  Are we patching pg_dump,
> pg_dumpall, or both?

Just pg_dumpall.c.

> Can I see the full patch?

It was attached to this post:

http://www.postgresql.org/message-id/1385225082.8248.YahooMailNeo@web162901.mail.bf1.yahoo.com

> Are we propagating other settings from pg_dump to pg_dumpall,
> like statement_timeout?

pg_dumpall output sets up the global objects (including their
properties) and then does a \connect to each database, followed by
the same output that pg_dump would generate for that database.
That includes the SET statements for statement_timeout, etc.  The
patch does nothing to change what objects or properties the
pg_dumpall output tries to set up, it just sets a property *on the
current connection* to allow those statements to run without error.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 27, 2013 at 06:05:13AM -0800, Kevin Grittner wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > On Tue, Nov 26, 2013 at 03:25:44PM -0800, Kevin Grittner wrote:
> >> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >>
> >>> How are we handling breakage of pg_dump, not pg_dumpall?
> >>
> >> That was discussed.  Do you have something to add?
> >
> > I am confused what we are patching.  Are we patching pg_dump,
> > pg_dumpall, or both?
>
> Just pg_dumpall.c.

OK, there was a pg_dump patch earlier which we are not using now.

> > Are we propagating other settings from pg_dump to pg_dumpall,
> > like statement_timeout?
>
> pg_dumpall output sets up the global objects (including their
> properties) and then does a \connect to each database, followed by
> the same output that pg_dump would generate for that database.
> That includes the SET statements for statement_timeout, etc.  The
> patch does nothing to change what objects or properties the
> pg_dumpall output tries to set up, it just sets a property *on the
> current connection* to allow those statements to run without error.

What is the logic that has us setting statement_timeout in pg_dump but
default_transaction_read_only in pg_dumpall?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Wed, Nov 27, 2013 at 06:05:13AM -0800, Kevin Grittner wrote:
>> Bruce Momjian <bruce(at)momjian(dot)us> wrote:

>>> I am confused what we are patching.  Are we patching pg_dump,
>>> pg_dumpall, or both?
>>
>> Just pg_dumpall.c.
>
> OK, there was a pg_dump patch earlier which we are not using now.

Right, that was v1, early in the discussion.  This is v3, based on
responding to the discussion on this thread.

> What is the logic that has us setting statement_timeout in
> pg_dump but default_transaction_read_only in pg_dumpall?

Of the people who posted on this thread supporting that, I think
Tom said it best:

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> I'm inclined to agree with Kevin that this behavior is wrong and
> should be fixed (and back-patched), so far as pg_dumpall is concerned.
> pg_dumpall's charter is to be able to recreate a database cluster's
> contents in a virgin installation, but it's failing to honor that
> contract if the cluster has any ALTER DATABASE SET default_read_only
> settings.  Similarly, I think it's reasonable to try to make pg_upgrade
> cope with the case.
>
> I also agree with *not* changing pg_dump, since it is not the charter
> of pg_dump to recreate a whole cluster, and the objection about possibly
> restoring into a database that was meant to be protected by this setting
> seems to have some force.

For example, I have seen dumps accidentally restored to the
postgres database on multiple occasions.  You might, for example,
flag the postgres database with this, and thereby block such
accidents.  The patch as it stands would allow pg_dumpall to
replicate such a cluster, flag and all.  Without the patch you get
many errors.

It is also much easier to work around with pg_dump output.  You
could get a psql connection to a database, set this off for the
connection, and use \i to read the pg_dump output file.  Or you
could concatenate a SET statement in front of the pg_dump output
when piping it in.  There is no correspondingly easy solution for
pg_dumpall.

--
Kevin GrittnerEDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 27, 2013 at 03:36:12PM -0800, Kevin Grittner wrote:
> Of the people who posted on this thread supporting that, I think
> Tom said it best:
>
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> > I'm inclined to agree with Kevin that this behavior is wrong and
> > should be fixed (and back-patched), so far as pg_dumpall is concerned.
> > pg_dumpall's charter is to be able to recreate a database cluster's
> > contents in a virgin installation, but it's failing to honor that
> > contract if the cluster has any ALTER DATABASE SET default_read_only
> > settings.  Similarly, I think it's reasonable to try to make pg_upgrade
> > cope with the case.
> >
> > I also agree with *not* changing pg_dump, since it is not the charter
> > of pg_dump to recreate a whole cluster, and the objection about possibly
> > restoring into a database that was meant to be protected by this setting
> > seems to have some force.
>
> For example, I have seen dumps accidentally restored to the
> postgres database on multiple occasions.  You might, for example,
> flag the postgres database with this, and thereby block such
> accidents.  The patch as it stands would allow pg_dumpall to
> replicate such a cluster, flag and all.  Without the patch you get
> many errors.
>
> It is also much easier to work around with pg_dump output.  You
> could get a psql connection to a database, set this off for the
> connection, and use \i to read the pg_dump output file.  Or you
> could concatenate a SET statement in front of the pg_dump output
> when piping it in.  There is no correspondingly easy solution for
> pg_dumpall.

Well, I can understand that, but part of the argument was that
default_transaction_read_only is not part of the database, but rather
just the transaction default:

> Karsten wrote:
> Maybe I am splitting hairs but setting transactions to readonly
> per default does not mean the database *as such* is to be readonly.
> It literally applies to the *default* state of transactions (as
> opposed to the ONLY state of transactions). No more, no less.

I ask again!

> What is the logic that has us setting statement_timeout in
> pg_dump but default_transaction_read_only in pg_dumpall?

Why can't I get an answer to that question? Is it that
statement_timeout is less likely to lead to a restore failure? Are all
the other settings output from pg_dump safe? Is only
default_transaction_read_only a problem? Whatever the answer is, the
patch should explain why we are singling out
default_transaction_read_only for pg_dumpall use and everything else is
in pg_dump.

Why does it feel like I am going around in circles here? I feel I like
am reliving the materialized view record comparison thread all over
again. :-(

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Wed, Nov 27, 2013 at 03:36:12PM -0800, Kevin Grittner wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> > I'm inclined to agree with Kevin that this behavior is wrong and
> > should be fixed (and back-patched), so far as pg_dumpall is concerned.
> > pg_dumpall's charter is to be able to recreate a database cluster's
> > contents in a virgin installation, but it's failing to honor that
> > contract if the cluster has any ALTER DATABASE SET default_read_only
> > settings.  Similarly, I think it's reasonable to try to make pg_upgrade
> > cope with the case.
> >
> > I also agree with *not* changing pg_dump, since it is not the charter
> > of pg_dump to recreate a whole cluster, and the objection about possibly
> > restoring into a database that was meant to be protected by this setting
> > seems to have some force.
>
> For example, I have seen dumps accidentally restored to the
> postgres database on multiple occasions.  You might, for example,
> flag the postgres database with this, and thereby block such
> accidents.  The patch as it stands would allow pg_dumpall to
> replicate such a cluster, flag and all.  Without the patch you get
> many errors.
>
> It is also much easier to work around with pg_dump output.  You
> could get a psql connection to a database, set this off for the
> connection, and use \i to read the pg_dump output file.  Or you
> could concatenate a SET statement in front of the pg_dump output
> when piping it in.  There is no correspondingly easy solution for
> pg_dumpall.

OK, I have read through this again, and now I see the idea is that we
are trying to have pg_dumpall override the default_transaction_read_only
value, but keep it for pg_dump restores. That is a pretty tricky
use-case, so I think we need to mention this in the C comments.

Tom and you think we should backpatch, and it seems only I am against
it, so I suppose you can move forward. We are packaging soon for a minor
release.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Wed, Nov 27, 2013 at 09:22:50PM -0500, Bruce Momjian wrote:

> Well, I can understand that, but part of the argument was that
> default_transaction_read_only is not part of the database, but rather
> just the transaction default:
>
> > Karsten wrote:
> > Maybe I am splitting hairs but setting transactions to readonly
> > per default does not mean the database *as such* is to be readonly.
> > It literally applies to the *default* state of transactions (as
> > opposed to the ONLY state of transactions). No more, no less.
>
> I ask again!
>
> > What is the logic that has us setting statement_timeout in
> > pg_dump but default_transaction_read_only in pg_dumpall?
>
> Why can't I get an answer to that question?

Bruce, I can't answer that. I am not versed enough to
know. All I can make sure (and hope to have made) is
that the failing use case is very clear.

> Is it that statement_timeout is less likely to lead to a restore failure?

That seems right -- default_read_only WILL fail the
restore while stmt_timeout CAN.

Or rather:

One of the *legitimate* settings of read_only WILL fail it.

But only *insane* settings of _timeout WILL, too (like,
extremely short ones). Saner settings CAN still.

Yes, I do agree that it seems useful to temporarily apply
a sane stmt_timeout as well.

But perhaps the idea was to think of the minimal impact
patch solving the immediate problem at hand (my use case) ?

Karsten
--
GPG key ID E4071346 @ gpg-keyserver.de
E167 67FD A291 2BEA 73BD 4537 78B9 A9F9 E407 1346

On Thu, Nov 28, 2013 at 10:20:53AM +0100, Karsten Hilbert wrote:
> > Is it that statement_timeout is less likely to lead to a restore failure?
>
> That seems right -- default_read_only WILL fail the
> restore while stmt_timeout CAN.
>
> Or rather:
>
> One of the *legitimate* settings of read_only WILL fail it.
>
> But only *insane* settings of _timeout WILL, too (like,
> extremely short ones). Saner settings CAN still.
>
> Yes, I do agree that it seems useful to temporarily apply
> a sane stmt_timeout as well.
>
> But perhaps the idea was to think of the minimal impact
> patch solving the immediate problem at hand (my use case) ?

Well, then we are actually using two different reasons for patching
pg_dumpall and not pg_dump. Your reason is based on the probability of
failure, while Tom/Kevin's reason is that default_transaction_read_only
might be used to block changes to a specific database, and they want a
pg_dump restore prevented, but a pg_dumpall restore to succeed.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Thu, Nov 28, 2013 at 10:39:18AM -0500, Bruce Momjian wrote:

> Well, then we are actually using two different reasons for patching
> pg_dumpall and not pg_dump. Your reason is based on the probability of
> failure, while Tom/Kevin's reason is that default_transaction_read_only
> might be used to block changes to a specific database, and they want a
> pg_dump restore prevented, but a pg_dumpall restore to succeed.

I can't really argue one way or another because *I* am
not likely to be able to offer an actual patch. At any
rate all I am interested in is that pg_upgrade does not
fail to upgrade in surprising ways.

Regarding restoring a pg_dump IMO the line would need to
be drawn along the -c/--clean option because using such seems
to clearly say that, yes, the user *wants* data to be deleted.

If -C/--create is used it shouldn't matter ...

However, I'm not saying that this is how it is to
be done. I am well aware that drawing such subtle
distinctions is walking quite a fine line.

Karsten
--
GPG key ID E4071346 @ gpg-keyserver.de
E167 67FD A291 2BEA 73BD 4537 78B9 A9F9 E407 1346

On Fri, Nov 29, 2013 at 12:46:01AM +0100, Karsten Hilbert wrote:
> On Thu, Nov 28, 2013 at 10:39:18AM -0500, Bruce Momjian wrote:
>
> > Well, then we are actually using two different reasons for patching
> > pg_dumpall and not pg_dump. Your reason is based on the probability of
> > failure, while Tom/Kevin's reason is that default_transaction_read_only
> > might be used to block changes to a specific database, and they want a
> > pg_dump restore prevented, but a pg_dumpall restore to succeed.
>
> I can't really argue one way or another because *I* am
> not likely to be able to offer an actual patch. At any
> rate all I am interested in is that pg_upgrade does not
> fail to upgrade in surprising ways.

Once the patch is applied, I will be patching pg_upgrade by appending to
PGOPTIONS, but that will only be for 9.4. The patch will be too risky
and there are not enough problem reports to override that and warrant
backpatching.

> Regarding restoring a pg_dump IMO the line would need to
> be drawn along the -c/--clean option because using such seems
> to clearly say that, yes, the user *wants* data to be deleted.
>
> If -C/--create is used it shouldn't matter ...
>
> However, I'm not saying that this is how it is to
> be done. I am well aware that drawing such subtle
> distinctions is walking quite a fine line.

So you are saying that default_transaction_read_only should be turned
off by pg_dump if --clean is used? Interesting.

The bottom line is that we can't handle every case and if we tried the
code would be very complex and error-prone. I am not sure where to draw
the line but it has to be drawn somewhere.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> wrote:

> Once the patch is applied, I will be patching pg_upgrade by appending to

> PGOPTIONS, but that will only be for 9.4.  The patch will be too risky
> and there are not enough problem reports to override that and warrant
> backpatching.

pg_dumpall patch applied, back to 8.4.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Nov 22, 2013 at 01:55:10PM -0800, Kevin Grittner wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
>
> This covers pg_dumpall globals.  Tested with a read-only postgres
> database and with default_transaction_read_only = on in the
> postgresql.conf file.
>
> It does nothing about pg_upgrade, which is sort of a separate
> issue.  My inclination is that connections to the new cluster
> should set this and connections to the old should not.

I have fixed pg_upgrade in git-head with the attached patch, which
prepends default_transaction_read_only=false to PGOPTIONS. I prepended
rather than appended because if PGOPTIONS had
default_transaction_read_only=true, I didn't want to override that.

While pg_dumpall would override that, that is more of an implementation
detail of having to set the value at the session level, rather than a
desired behavior.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> I have fixed pg_upgrade in git-head with the attached patch, which
> prepends default_transaction_read_only=false to PGOPTIONS.

What is the point of this, given that Kevin fixed pg_dumpall? Don't
those fixes take care of the issue?

If your argument is that you want pg_upgrade to work even if the
user already turned on default_transaction_read_only in the *new*
cluster, I would humbly disagree with that goal, for pretty much
the same reasons I didn't want pg_dump overriding it.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>
>> I have fixed pg_upgrade in git-head with the attached patch,
>> which prepends default_transaction_read_only=false to PGOPTIONS.
>
> What is the point of this, given that Kevin fixed pg_dumpall?
> Don't those fixes take care of the issue?
>
> If your argument is that you want pg_upgrade to work even if the
> user already turned on default_transaction_read_only in the *new*
> cluster, I would humbly disagree with that goal, for pretty much
> the same reasons I didn't want pg_dump overriding it.

If there were databases or users with default_transaction_read_only
set in the old cluster, the pg_dumpall run will cause that property
to be set in the new cluster, so what you are saying seems to be
that a cluster can't be upgraded to a new major release if any
database within it has that set.  My personal feeling is that it
would be good if that were not a barrier to using pg_upgrade; but
it would be OK as long as running it with the --check option
*tells* you that the cluster can't be upgraded without turning that
property off for all databases, and that the user under which it
runs can't have the property set.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> What is the point of this, given that Kevin fixed pg_dumpall?
>> Don't those fixes take care of the issue?

> If there were databases or users with default_transaction_read_only
> set in the old cluster, the pg_dumpall run will cause that property
> to be set in the new cluster, so what you are saying seems to be
> that a cluster can't be upgraded to a new major release if any
> database within it has that set.

Oh, I had forgotten that pg_upgrade will run additional commands
against the new cluster after restoring the dumpall output.
I guess we do need something like what Bruce did to cover that.

regards, tom lane

On Sat, Nov 30, 2013 at 06:48:02PM -0500, Tom Lane wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> > Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> What is the point of this, given that Kevin fixed pg_dumpall?
> >> Don't those fixes take care of the issue?
>
> > If there were databases or users with default_transaction_read_only
> > set in the old cluster, the pg_dumpall run will cause that property
> > to be set in the new cluster, so what you are saying seems to be
> > that a cluster can't be upgraded to a new major release if any
> > database within it has that set.
>
> Oh, I had forgotten that pg_upgrade will run additional commands
> against the new cluster after restoring the dumpall output.

Actually, pg_upgrade used to use pg_dumpall but since PG 9.3 is used
pg_dumpall --globals-only and pg_dump on each database, which allows
parallel operations. Also, there are other libpq sessions that modify
the new cluster, so PGOPTIONS is the best option. I was happy the patch
was so small.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Sat, Nov 30, 2013 at 03:21:08PM -0800, Kevin Grittner wrote:

> > If your argument is that you want pg_upgrade to work even if the
> > user already turned on default_transaction_read_only in the *new*
> > cluster, I would humbly disagree with that goal, for pretty much
> > the same reasons I didn't want pg_dump overriding it.
>
> If there were databases or users with default_transaction_read_only
> set in the old cluster, the pg_dumpall run will cause that property
> to be set in the new cluster, so what you are saying seems to be
> that a cluster can't be upgraded to a new major release if any
> database within it has that set.

That is *precisely* my use case which I initially asked about.

Karsten
--
GPG key ID E4071346 @ gpg-keyserver.de
E167 67FD A291 2BEA 73BD 4537 78B9 A9F9 E407 1346

On Sun, Dec 1, 2013 at 09:22:52AM +0100, Karsten Hilbert wrote:
> On Sat, Nov 30, 2013 at 03:21:08PM -0800, Kevin Grittner wrote:
>
> > > If your argument is that you want pg_upgrade to work even if the
> > > user already turned on default_transaction_read_only in the *new*
> > > cluster, I would humbly disagree with that goal, for pretty much
> > > the same reasons I didn't want pg_dump overriding it.
> >
> > If there were databases or users with default_transaction_read_only
> > set in the old cluster, the pg_dumpall run will cause that property
> > to be set in the new cluster, so what you are saying seems to be
> > that a cluster can't be upgraded to a new major release if any
> > database within it has that set.
>
> That is *precisely* my use case which I initially asked about.

The use-case would be that default_transaction_read_only is turned on in
postgresql.conf as part of installing the old and/or new cluster. The
9.4 PGOPTIONS fix for pg_upgrade _will_ allow such a cluster to be
upgraded.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Mon, Dec 02, 2013 at 11:41:10AM -0500, Bruce Momjian wrote:

> > > If there were databases or users with default_transaction_read_only
> > > set in the old cluster, the pg_dumpall run will cause that property
> > > to be set in the new cluster, so what you are saying seems to be
> > > that a cluster can't be upgraded to a new major release if any
> > > database within it has that set.
> >
> > That is *precisely* my use case which I initially asked about.
>
> The use-case would be that default_transaction_read_only is turned on in
> postgresql.conf

Are you telling me which use case I initially asked
about on this thread ?

Karsten
--
GPG key ID E4071346 @ gpg-keyserver.de
E167 67FD A291 2BEA 73BD 4537 78B9 A9F9 E407 1346

On Mon, Dec 2, 2013 at 06:57:53PM +0100, Karsten Hilbert wrote:
> On Mon, Dec 02, 2013 at 11:41:10AM -0500, Bruce Momjian wrote:
>
> > > > If there were databases or users with default_transaction_read_only
> > > > set in the old cluster, the pg_dumpall run will cause that property
> > > > to be set in the new cluster, so what you are saying seems to be
> > > > that a cluster can't be upgraded to a new major release if any
> > > > database within it has that set.
> > >
> > > That is *precisely* my use case which I initially asked about.
> >
> > The use-case would be that default_transaction_read_only is turned on in
> > postgresql.conf
>
> Are you telling me which use case I initially asked
> about on this thread ?

No, this is another use-case that is fixed the pg_upgrade patch. The
ALTER DATABASE SET is also fixed by this patch.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Mon, Dec 02, 2013 at 01:24:18PM -0500, Bruce Momjian wrote:

> > > > > If there were databases or users with default_transaction_read_only
> > > > > set in the old cluster, the pg_dumpall run will cause that property
> > > > > to be set in the new cluster, so what you are saying seems to be
> > > > > that a cluster can't be upgraded to a new major release if any
> > > > > database within it has that set.
> > > >
> > > > That is *precisely* my use case which I initially asked about.
> > >
> > > The use-case would be that default_transaction_read_only is turned on in
> > > postgresql.conf
> >
> > Are you telling me which use case I initially asked
> > about on this thread ?
>
> No, this is another use-case that is fixed the pg_upgrade patch. The
> ALTER DATABASE SET is also fixed by this patch.

I see.

Thanks,
Karsten
--
GPG key ID E4071346 @ gpg-keyserver.de
E167 67FD A291 2BEA 73BD 4537 78B9 A9F9 E407 1346