Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
BTW, thanks to all who helped in improving this issue. Karsten -- GPG key ID E4071346 @ gpg-keyserver.de E167 67FD A291 2BEA 73BD 4537 78B9 A9F9 E407 1346 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Bruce Momjian br...@momjian.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
On Fri, Nov 22, 2013 at 01:55:10PM -0800, Kevin Grittner wrote:
Kevin Grittner kgri...@ymail.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 br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/contrib/pg_upgrade/option.c b/contrib/pg_upgrade/option.c
new file mode 100644
index 250aeb8..6bd91c5
*** a/contrib/pg_upgrade/option.c
--- b/contrib/pg_upgrade/option.c
***
*** 25,30
--- 25,31
static void usage(void);
static void check_required_directory(char **dirpath, char **configpath,
char *envVarName, char *cmdLineOption, char *description);
+ #define FIX_DEFAULT_READ_ONLY -c default_transaction_read_only=false
UserOpts user_opts;
*** parseCommandLine(int argc, char *argv[])
*** 208,213
--- 209,225
fclose(fp);
}
+ /* Turn off read-only mode; add prefix to PGOPTIONS? */
+ if (getenv(PGOPTIONS))
+ {
+ char *pgoptions = psprintf(%s %s, FIX_DEFAULT_READ_ONLY,
+ getenv(PGOPTIONS));
+ pg_putenv(PGOPTIONS, pgoptions);
+ pfree(pgoptions);
+ }
+ else
+ pg_putenv(PGOPTIONS, FIX_DEFAULT_READ_ONLY);
+
/* Get values from env if not already set */
check_required_directory(old_cluster.bindir, NULL, PGBINOLD, -b,
old cluster binaries reside);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Bruce Momjian br...@momjian.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Tom Lane t...@sss.pgh.pa.us wrote: Bruce Momjian br...@momjian.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Kevin Grittner kgri...@ymail.com writes: Tom Lane t...@sss.pgh.pa.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
On Sat, Nov 30, 2013 at 06:48:02PM -0500, Tom Lane wrote: Kevin Grittner kgri...@ymail.com writes: Tom Lane t...@sss.pgh.pa.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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Bruce Momjian br...@momjian.us wrote: On Tue, Nov 26, 2013 at 03:25:44PM -0800, Kevin Grittner wrote: Bruce Momjian br...@momjian.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.yahoomail...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
On Wed, Nov 27, 2013 at 06:05:13AM -0800, Kevin Grittner wrote: Bruce Momjian br...@momjian.us wrote: On Tue, Nov 26, 2013 at 03:25:44PM -0800, Kevin Grittner wrote: Bruce Momjian br...@momjian.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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Bruce Momjian br...@momjian.us wrote: On Wed, Nov 27, 2013 at 06:05:13AM -0800, Kevin Grittner wrote: Bruce Momjian br...@momjian.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 t...@sss.pgh.pa.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 t...@sss.pgh.pa.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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
On Wed, Nov 27, 2013 at 03:36:12PM -0800, Kevin Grittner wrote: Tom Lane t...@sss.pgh.pa.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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Bruce Momjian br...@momjian.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
On Tue, Nov 26, 2013 at 03:25:44PM -0800, Kevin Grittner wrote: Bruce Momjian br...@momjian.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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
On Sat, Nov 23, 2013 at 08:44:42AM -0800, Kevin Grittner wrote: Bruce Momjian br...@momjian.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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Bruce Momjian br...@momjian.us wrote: Kevin Grittner wrote: Bruce Momjian br...@momjian.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
On Mon, Nov 25, 2013 at 02:24:25PM -0800, Kevin Grittner wrote: Bruce Momjian br...@momjian.us wrote: Kevin Grittner wrote: Bruce Momjian br...@momjian.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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
On Fri, Nov 22, 2013 at 04:25:57PM -0800, Kevin Grittner wrote: Bruce Momjian br...@momjian.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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
On Fri, Nov 22, 2013 at 04:38:53PM -0800, Kevin Grittner wrote: Andres Freund and...@2ndquadrant.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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Bruce Momjian br...@momjian.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 21 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
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Kevin Grittner kgri...@ymail.com writes: Bruce Momjian br...@momjian.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Am Samstag, 23. November 2013, 08:44:42 schrieb Kevin Grittner: Bruce Momjian br...@momjian.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 21 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
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Bruce Momjian br...@momjian.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Kevin Grittner kgri...@ymail.com wrote: See the attached patch. Trying that again. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Companydiff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index 63a8009..199ffb0 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -2586,6 +2586,9 @@ _doSetFixedOutputState(ArchiveHandle *AH) /* Likewise for lock_timeout */ ahprintf(AH, SET lock_timeout = 0;\n); + /* Restore will need to write to the target database */ + ahprintf(AH, SET default_transaction_read_only = off;\n); + /* Select the correct character set encoding */ ahprintf(AH, SET client_encoding = '%s';\n, pg_encoding_to_char(AH-public.encoding)); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Kevin Grittner kgri...@ymail.com writes: Kevin Grittner kgri...@ymail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Tom Lane t...@sss.pgh.pa.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
On 2013-11-22 12:45:25 -0800, Kevin Grittner wrote: Tom Lane t...@sss.pgh.pa.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Andres Freund and...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
On 2013-11-22 13:07:29 -0800, Kevin Grittner wrote: Andres Freund and...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Andres Freund and...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Andres Freund and...@2ndquadrant.com wrote: On 2013-11-22 13:07:29 -0800, Kevin Grittner wrote: Andres Freund and...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Kevin Grittner kgri...@ymail.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 Companydiff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index 63a8009..199ffb0 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -2586,6 +2586,9 @@ _doSetFixedOutputState(ArchiveHandle *AH) /* Likewise for lock_timeout */ ahprintf(AH, SET lock_timeout = 0;\n); + /* Restore will need to write to the target database */ + ahprintf(AH, SET default_transaction_read_only = off;\n); + /* Select the correct character set encoding */ ahprintf(AH, SET client_encoding = '%s';\n, pg_encoding_to_char(AH-public.encoding)); diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index 336ae58..4540a19 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -452,6 +452,9 @@ main(int argc, char *argv[]) * database we're connected to at the moment is fine. */ + /* Restore will need to write to the target database */ + fprintf(OPF, SET default_transaction_read_only = off;\n); + /* Replicate encoding and std_strings in output */ fprintf(OPF, SET client_encoding = '%s';\n, pg_encoding_to_char(encoding)); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Andres Freund and...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Bruce Momjian br...@momjian.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency
Andres Freund and...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
