Re: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL
On Saturday, December 01, 2012 1:30 AM Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On Wed, Nov 28, 2012 at 9:47 AM, Amit kapila amit.kap...@huawei.com wrote: 5. PERSISTENT Keyword is added to the reserved keyword list. As it was giving some errors given below while parsing gram.y 15 shift/reduce conflicts . Allow me to be the first to say that any syntax for this feature that involves reserving new keywords is a bad syntax. Let me put that a little more strongly: syntax that requires reserving words that aren't reserved in the SQL standard is unacceptable. Even if the new word is reserved according to SQL, we'll frequently try pretty hard to avoid making it reserved in Postgres, so as not to break existing applications. But if it's not in the standard then you're breaking applications that can reasonably expect not to get broken. But having said that, it's not apparent to me why inventing SET PERSISTENT should require reserving PERSISTENT. In the existing syntaxes SET LOCAL and SET SESSION, there's not been a need to reserve LOCAL or SESSION. Maybe you're just trying to be a bit too cute in the grammar productions? Frequently there's more than one way to do it and not all require the same level of keyword reservedness. The problem is due to RESET PERSISTENT configuration_variable Syntax. I think the reason is that configuration_variable name can also be persistent, so its not able to resolve. I have tried quite a few ways. I shall try some more and send you result of all. If you have any idea or any hint where similar syntax is used, please point me I will refer it. Any other Suggestions? With Regards, Amit Kapila. -- 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] Hot Standby Feedback should default to on in 9.3+
Magnus Hagander wrote: On 30.11.2012 21:02, Andres Freund wrote: There are workloads where its detrimental, but in general having it default to on improver experience tremendously because getting conflicts because of vacuum is rather confusing. In the workloads where it might not be a good idea (very long queries on the standby, many dead tuples on the primary) you need to think very carefuly about the strategy of avoiding conflicts anyway, and explicit configuration is required as well. Does anybody have an argument against changing the default value? -1. By default, I would expect a standby server to not have any meaningful impact on the performance of the master. With hot standby feedback, you can bloat the master very badly if you're not careful. I'm with Heikki on the -1 on this. It's certainly unexpected to have the slave affect the master by default - people will expect the master to be independent. I agree. +1. Having your reporting query time out *shows you* the problem. Having the master bloat for you won't show the problem until later - when it's much bigger, and it's much more pain to recover from. I couldn't agree more. There are different requirements, and there will always be people who need to change the defaults, but the way it is is the safest in my opinion. Yours, Laurenz Albe -- 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] WIP: index support for regexp search
On Fri, November 30, 2012 12:22, Alexander Korotkov wrote: Hi! On Thu, Nov 29, 2012 at 12:58 PM, er e...@xs4all.nl wrote: On Mon, November 26, 2012 20:49, Alexander Korotkov wrote: I ran the simple-minded tests against generated data (similar to the ones I did in January 2012). The problems of that older version seem pretty much all removed. (although I didn't do much work on it -- just reran these tests). Thanks a lot for testing! Could you repeat for 0.7 version of patch which has new overflow handling? I've attached a similar test re-run that compares HEAD with patch versions 0.6, and 0.7. Erik Rijkers trgm_compare.txt.gz Description: GNU Zip compressed data -- 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] proposal: fix corner use case of variadic fuctions usage
Hello Hi Pavel. I am trying to review this patch and on my work computer everything compiles and tests perfectly. However, on my laptop, the regression tests don't pass with cache lookup failed for type XYZ where XYZ is some number that does not appear to be any type oid. I don't really know where to go from here. I am asking that other people try this patch to see if they get errors as well. yes, I checked it on .x86_64 and I had a same problems probably there was more than one issue - I had to fix a creating a unpacked params and I had a issue with gcc optimalization when I used a stack variable for fcinfo. Now I fixed these issues and I hope so it will work on all platforms Regards Pavel Stehule Vik PS: I won't be able to answer this thread until Tuesday. variadic_argument_for_variadic_any_function_20121201.diff Description: Binary data -- 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] --single-transaction hack to pg_upgrade does not work
On 11/30/2012 11:10 PM, Tom Lane wrote:
Some of the buildfarm members are failing the pg_upgrade regression test
since commit 12ee6ec71f8754ff3573711032b9b4d5a764ba84. I can duplicate
it here, and the symptom is:
pg_restore: creating TYPE float8range
pg_restore: creating TYPE insenum
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 978; 1247 16584 TYPE insenum
tgl
pg_restore: [archiver (db)] could not execute query: ERROR: ALTER TYPE ... ADD
cannot run inside a transaction block
Command was:
-- For binary upgrade, must preserve pg_type oid
SELECT binary_upgrade.set_next_pg_type_oid('16584'::pg_catalog.oid);
I have not investigated why it apparently passes some places; this looks
to me like a guaranteed failure.
Testing pg_upgrade has only been in buildfarm releases since September
28, and even then is optional, although enabled by default in the sample
config file. Looks like even I need to upgrade a few of my animals to do
it. It probably needs to improve its error logging though.
Seems odd not to have run make check before committing, though.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Tablespaces in the data directory
Someone just reported a problem when they had created a new tablespace inside the old data directory. I'm sure there can be other issues caused by this as well, but this is mainly a confusing scenario for people now. As there isn't (as far as I know at least) any actual *point* in creating a tablespace inside the main data directory, should we perhaps disallow this in CREATE TABLESPACE? Or at least throw a WARNING if one does it? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- 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] Tablespaces in the data directory
On 1 December 2012 13:45, Magnus Hagander mag...@hagander.net wrote: Someone just reported a problem when they had created a new tablespace inside the old data directory. I'm sure there can be other issues caused by this as well, but this is mainly a confusing scenario for people now. As there isn't (as far as I know at least) any actual *point* in creating a tablespace inside the main data directory, should we perhaps disallow this in CREATE TABLESPACE? Or at least throw a WARNING if one does it? +1 -- Simon Riggs 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
[HACKERS] ALTER TABLE ... NOREWRITE option
It's hard to know whether your tables will be locked for long periods when implementing DDL changes. The NOREWRITE option would cause an ERROR if the table would be rewritten by the command. This would allow testing to highlight long running statements before code hits production. -- Simon Riggs 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] --single-transaction hack to pg_upgrade does not work
On Sat, Dec 1, 2012 at 07:43:17AM -0500, Andrew Dunstan wrote:
On 11/30/2012 11:10 PM, Tom Lane wrote:
Some of the buildfarm members are failing the pg_upgrade regression test
since commit 12ee6ec71f8754ff3573711032b9b4d5a764ba84. I can duplicate
it here, and the symptom is:
pg_restore: creating TYPE float8range
pg_restore: creating TYPE insenum
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 978; 1247 16584 TYPE
insenum tgl
pg_restore: [archiver (db)] could not execute query: ERROR: ALTER TYPE ...
ADD cannot run inside a transaction block
Command was:
-- For binary upgrade, must preserve pg_type oid
SELECT binary_upgrade.set_next_pg_type_oid('16584'::pg_catalog.oid);
I have not investigated why it apparently passes some places; this looks
to me like a guaranteed failure.
I see now. Sorry. I was so focused on performance testing and never
thought this cause pg_upgrade to fail. I did not run my full tests this
time.
It seems the problem is that we bundling the pg_upgrade oid set function
into the same code block as ALTER TYPE, to preserve the type oid. Let
me see how to fix this.
Should I do something temporarily to get the buildfarm green again?
Just revert the entire thing?
Testing pg_upgrade has only been in buildfarm releases since
September 28, and even then is optional, although enabled by default
in the sample config file. Looks like even I need to upgrade a few
of my animals to do it. It probably needs to improve its error
logging though.
Seems odd not to have run make check before committing, though.
I was not aware the pg_upgrade testing was in our git tree; I thought
it was only in the buildfarm code. I am glad it is in our tree and it
seem to do my full tests in a more automated manner. I will use it in
the future.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
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] --single-transaction hack to pg_upgrade does not work
On Sat, Dec 1, 2012 at 10:25:10AM -0500, Bruce Momjian wrote:
On Sat, Dec 1, 2012 at 07:43:17AM -0500, Andrew Dunstan wrote:
On 11/30/2012 11:10 PM, Tom Lane wrote:
Some of the buildfarm members are failing the pg_upgrade regression test
since commit 12ee6ec71f8754ff3573711032b9b4d5a764ba84. I can duplicate
it here, and the symptom is:
pg_restore: creating TYPE float8range
pg_restore: creating TYPE insenum
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 978; 1247 16584 TYPE
insenum tgl
pg_restore: [archiver (db)] could not execute query: ERROR: ALTER TYPE
... ADD cannot run inside a transaction block
Command was:
-- For binary upgrade, must preserve pg_type oid
SELECT binary_upgrade.set_next_pg_type_oid('16584'::pg_catalog.oid);
I have not investigated why it apparently passes some places; this looks
to me like a guaranteed failure.
I see now. Sorry. I was so focused on performance testing and never
thought this cause pg_upgrade to fail. I did not run my full tests this
time.
It seems the problem is that we bundling the pg_upgrade oid set function
into the same code block as ALTER TYPE, to preserve the type oid. Let
me see how to fix this.
Should I do something temporarily to get the buildfarm green again?
Just revert the entire thing?
OK, I found the problem, and it isn't good. Our manual clearly says:
ALTER TYPE ... ADD VALUE (the form that adds a new value
to an enum type) cannot be executed inside a transaction block.
This also means it can't be passed inside an implicit transaction block,
which happens when you pass:
SELECT 1; SELECT 2;
as a string, and I think this is what pg_restore is doing. So, not only
is --single-transction causing the failure, but even without
--single-transction, pg_restore just passes the multi-statement string
to the backend, and you get the error:
pg_restore: [archiver (db)] could not execute query: ERROR: ALTER TYPE
... ADD cannot run inside a transaction block
Command was:
-- For binary upgrade, must preserve pg_type oid
SELECT binary_upgrade.set_next_pg_type_oid('16584'::pg_catalog.oid);
psql dutifully splits up the string into separate commands, which is why
the previous pg_dumpall | psql coding worked. One simple fix would be
to revert to plain output format, and return to using psql. Of course,
we lose a lot of performance with that. The pending AtOEXAct patch gets
us most of the performance back:
#tbls git -1AtOEXAct both
1 11.06 13.06 10.99 13.20
1000 21.71 22.92 22.20 22.51
2000 32.86 31.09 32.51 31.62
4000 55.22 49.96 52.50 49.99
8000 105.34 82.10 95.32 82.94
16000 223.67 164.27 187.40 159.53
32000 543.93 324.63 366.44 317.93
640001697.14 791.82 767.32 752.57
so maybe that's how we have to go, or modify pg_dump to emit the
binary-upgrade function call as a separate pg_dump entry, rather than
lumping it in with ALTER TYPE ... ADD VALUE.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
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] --single-transaction hack to pg_upgrade does not work
On Sat, Dec 1, 2012 at 10:41:06AM -0500, Bruce Momjian wrote:
OK, I found the problem, and it isn't good. Our manual clearly says:
ALTER TYPE ... ADD VALUE (the form that adds a new value
to an enum type) cannot be executed inside a transaction block.
This also means it can't be passed inside an implicit transaction block,
which happens when you pass:
SELECT 1; SELECT 2;
as a string, and I think this is what pg_restore is doing. So, not only
is --single-transction causing the failure, but even without
--single-transction, pg_restore just passes the multi-statement string
to the backend, and you get the error:
pg_restore: [archiver (db)] could not execute query: ERROR: ALTER TYPE
... ADD cannot run inside a transaction block
Command was:
-- For binary upgrade, must preserve pg_type oid
SELECT binary_upgrade.set_next_pg_type_oid('16584'::pg_catalog.oid);
psql dutifully splits up the string into separate commands, which is why
the previous pg_dumpall | psql coding worked. One simple fix would be
to revert to plain output format, and return to using psql. Of course,
we lose a lot of performance with that. The pending AtOEXAct patch gets
us most of the performance back:
#tbls git -1AtOEXAct both
1 11.06 13.06 10.99 13.20
1000 21.71 22.92 22.20 22.51
2000 32.86 31.09 32.51 31.62
4000 55.22 49.96 52.50 49.99
8000 105.34 82.10 95.32 82.94
16000 223.67 164.27 187.40 159.53
32000 543.93 324.63 366.44 317.93
640001697.14 791.82 767.32 752.57
so maybe that's how we have to go, or modify pg_dump to emit the
binary-upgrade function call as a separate pg_dump entry, rather than
lumping it in with ALTER TYPE ... ADD VALUE.
Scratch that idea. By definition, no matter how we modify pg_dump or
pg_restore, ALTER TYPE ... ADD VALUE is never going to be able to be run
in a multi-statement transaction, so we have to certainly remove
--single-transction, and then we can decide if we want to continue using
pg_restore with an improved pg_dump, or just fall back to pg_dump and
psql.
I am thinking at this point I should just switch to pg_dump text format
and psql to get the build farm green again, but not lose the other
changes that give us per-database dumps.
This does make me wonder why pg_restore supports --single-transaction if
it has known failure cases (that are not documented in the pg_restore
manual page, only in the ALTER TYPE manual page). Are users really
going to know if their database has objects that are not supported by
--single-transaction?
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
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] --single-transaction hack to pg_upgrade does not work
On 2012-12-01 10:55:09 -0500, Bruce Momjian wrote: On Sat, Dec 1, 2012 at 10:41:06AM -0500, Bruce Momjian wrote: OK, I found the problem, and it isn't good. Our manual clearly says: ALTER TYPE ... ADD VALUE (the form that adds a new value to an enum type) cannot be executed inside a transaction block. so maybe that's how we have to go, or modify pg_dump to emit the binary-upgrade function call as a separate pg_dump entry, rather than lumping it in with ALTER TYPE ... ADD VALUE. Scratch that idea. By definition, no matter how we modify pg_dump or pg_restore, ALTER TYPE ... ADD VALUE is never going to be able to be run in a multi-statement transaction, so we have to certainly remove --single-transction, and then we can decide if we want to continue using pg_restore with an improved pg_dump, or just fall back to pg_dump and psql. I am thinking at this point I should just switch to pg_dump text format and psql to get the build farm green again, but not lose the other changes that give us per-database dumps. This does make me wonder why pg_restore supports --single-transaction if it has known failure cases (that are not documented in the pg_restore manual page, only in the ALTER TYPE manual page). Are users really going to know if their database has objects that are not supported by --single-transaction? Could we possibly allow adding enum values to a type which was just created in this transaction? That shouldn't be too hard. At least easier than providing the capability to pre-assign the next N oids... 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] --single-transaction hack to pg_upgrade does not work
On 2012-12-01 10:55:09 -0500, Bruce Momjian wrote: This does make me wonder why pg_restore supports --single-transaction if it has known failure cases (that are not documented in the pg_restore manual page, only in the ALTER TYPE manual page). Are users really going to know if their database has objects that are not supported by --single-transaction? That problem only exists in binary upgrade mode, in plain mode the enum is created with all values in one CREATE TYPE ... AS ENUM(...) statement. So the problem simply doesn't exist there. 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] --single-transaction hack to pg_upgrade does not work
Bruce Momjian br...@momjian.us writes: This does make me wonder why pg_restore supports --single-transaction if it has known failure cases (that are not documented in the pg_restore manual page, only in the ALTER TYPE manual page). AFAIR, the ADD VALUE path is only taken with --binary-upgrade, which is just about entirely undocumented anyway. 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] --single-transaction hack to pg_upgrade does not work
On Sat, Dec 1, 2012 at 10:55:09AM -0500, Bruce Momjian wrote: Scratch that idea. By definition, no matter how we modify pg_dump or pg_restore, ALTER TYPE ... ADD VALUE is never going to be able to be run in a multi-statement transaction, so we have to certainly remove --single-transction, and then we can decide if we want to continue using pg_restore with an improved pg_dump, or just fall back to pg_dump and psql. I am thinking at this point I should just switch to pg_dump text format and psql to get the build farm green again, but not lose the other changes that give us per-database dumps. This does make me wonder why pg_restore supports --single-transaction if it has known failure cases (that are not documented in the pg_restore manual page, only in the ALTER TYPE manual page). Are users really going to know if their database has objects that are not supported by --single-transaction? OK, Andrew has accurately told me via IM that ALTER TYPE ... ADD VALUE is only emitted by pg_dump in binary-upgrade mode. Seems you can run it manually, but pg_dump doesn't use it except for binary-upgrade mode, and I now see that in the code. So, that removes my concern about pg_restore --single-transaction in general. So, we have to decide if we should improve pg_dump to split up the function call and ALTER TYPE ... ADD VALUE command, or fall back to text dump mode and psql. That removes the optimization of using custom format, and the optimization of using pg_restore. However, I don't see how I can guarantee that the pg_upgrade oid setting function will be called just _before_ the ALTER TYPE ... ADD VALUE command without having them in the same command string package. Shame --- pg_upgrade performance was improving so steadily, I was hoping to see negative duration times soon. ;-) -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] --single-transaction hack to pg_upgrade does not work
On Sat, Dec 1, 2012 at 11:11:31AM -0500, Bruce Momjian wrote: Shame --- pg_upgrade performance was improving so steadily, I was hoping to see negative duration times soon. ;-) Is that the definition of optimism? :-) -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Proposal for Allow postgresql.conf values to be changed via SQL
Amit Kapila amit.kap...@huawei.com writes: On Saturday, December 01, 2012 1:30 AM Tom Lane wrote: But having said that, it's not apparent to me why inventing SET PERSISTENT should require reserving PERSISTENT. The problem is due to RESET PERSISTENT configuration_variable Syntax. I think the reason is that configuration_variable name can also be persistent, so its not able to resolve. Well, that certainly looks like it should not be very difficult. The secret to getting bison to do what you want is to not ask it to make a shift/reduce decision too early. In this case I imagine you are trying to do something like RESET opt_persistent var_name which cannot work if persistent could be a var_name, because bison has to decide whether to reduce opt_persistent to PERSISTENT or empty before it can see if there's anything after the var_name. So the fix is to not use an opt_persistent production, but spell out both alternatives: RESET var_name RESET PERSISTENT var_name Now bison doesn't have to choose what to reduce until it can see the end of the statement; that is, once it's scanned RESET and PERSISTENT, the choice of whether to treat PERSISTENT as a var_name can be conditional on whether the next token is a name or EOL. But even if we can't make that work, it's not grounds for reserving PERSISTENT. Instead I'd be inclined to forget about RESET PERSISTENT syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that. (BTW, I wonder what behavior that syntax has now in your patch.) 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] --single-transaction hack to pg_upgrade does not work
On 2012-12-01 17:03:03 +0100, Andres Freund wrote: Could we possibly allow adding enum values to a type which was just created in this transaction? That shouldn't be too hard. At least easier than providing the capability to pre-assign the next N oids... The attached patch does just that. Its *not* ready yet though, as it will be apparent for everyone who reads it ;) To really make that work in a reliable manner we would probably need an rd_createSubid for typcache entries instead of testing xmin as I have done here? 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] --single-transaction hack to pg_upgrade does not work
On 2012-12-01 17:36:20 +0100, Andres Freund wrote:
On 2012-12-01 17:03:03 +0100, Andres Freund wrote:
Could we possibly allow adding enum values to a type which was just created
in
this transaction? That shouldn't be too hard. At least easier than providing
the capability to pre-assign the next N oids...
The attached patch does just that. Its *not* ready yet though, as it
will be apparent for everyone who reads it ;)
To really make that work in a reliable manner we would probably need
an rd_createSubid for typcache entries instead of testing xmin as I have
done here?
And the patch...
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
From 2839c7037d4ca8903a322aba5c399f2e54f2d63b Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Sat, 1 Dec 2012 17:37:57 +0100
Subject: [PATCH] Allow ALTER TYPE ... ADD VALUE inside transactions if the
type was created in the same transaction
---
src/backend/commands/typecmds.c | 16 ++--
src/backend/tcop/utility.c |4 ++--
src/include/commands/typecmds.h |2 +-
3 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 8418096..d419ed0 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1169,11 +1169,14 @@ DefineEnum(CreateEnumStmt *stmt)
* Adds a new label to an existing enum.
*/
void
-AlterEnum(AlterEnumStmt *stmt)
+AlterEnum(AlterEnumStmt *stmt, bool toplevel)
{
Oid enum_type_oid;
TypeName *typename;
HeapTuple tup;
+ bool in_transaction;
+
+ in_transaction = IsTransactionBlock() || IsSubTransaction() || !toplevel;
/* Make a TypeName so we can use standard type lookup machinery */
typename = makeTypeNameFromNameList(stmt-typeName);
@@ -1183,12 +1186,21 @@ AlterEnum(AlterEnumStmt *stmt)
if (!HeapTupleIsValid(tup))
elog(ERROR, cache lookup failed for type %u, enum_type_oid);
+ if (in_transaction)
+ {
+ TransactionId xmin = HeapTupleHeaderGetXmin(tup-t_data);
+ if (!TransactionIdIsCurrentTransactionId(xmin))
+ PreventTransactionChain(toplevel, ALTER TYPE ... ADD2);
+ }
+ else
+ PreventTransactionChain(toplevel, ALTER TYPE ... ADD);
+
/* Check it's an enum and check user has permission to ALTER the enum */
checkEnumOwner(tup);
/* Add the new label */
AddEnumLabel(enum_type_oid, stmt-newVal,
- stmt-newValNeighbor, stmt-newValIsAfter,
+ stmt-newValNeighbor, stmt-newValIsAfter,
stmt-skipIfExists);
ReleaseSysCache(tup);
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 491bd29..bf2a0e3 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -977,9 +977,9 @@ standard_ProcessUtility(Node *parsetree,
* We disallow this in transaction blocks, because we can't cope
* with enum OID values getting into indexes and then having their
* defining pg_enum entries go away.
+ * XXX
*/
- PreventTransactionChain(isTopLevel, ALTER TYPE ... ADD);
- AlterEnum((AlterEnumStmt *) parsetree);
+ AlterEnum((AlterEnumStmt *) parsetree, isTopLevel);
break;
case T_ViewStmt: /* CREATE VIEW */
diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h
index 2351024..792b146 100644
--- a/src/include/commands/typecmds.h
+++ b/src/include/commands/typecmds.h
@@ -26,7 +26,7 @@ extern void RemoveTypeById(Oid typeOid);
extern void DefineDomain(CreateDomainStmt *stmt);
extern void DefineEnum(CreateEnumStmt *stmt);
extern void DefineRange(CreateRangeStmt *stmt);
-extern void AlterEnum(AlterEnumStmt *stmt);
+extern void AlterEnum(AlterEnumStmt *stmt, bool toplevel);
extern Oid DefineCompositeType(RangeVar *typevar, List *coldeflist);
extern Oid AssignTypeArrayOid(void);
--
1.7.10.4
--
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] ALTER TABLE ... NOREWRITE option
Simon Riggs si...@2ndquadrant.com writes: It's hard to know whether your tables will be locked for long periods when implementing DDL changes. The NOREWRITE option would cause an ERROR if the table would be rewritten by the command. This would allow testing to highlight long running statements before code hits production. I'm not thrilled about inventing YA keyword for this. If you have a problem with that sort of scenario, why aren't you testing your DDL on a test server before you do it on production? Or even more to the point, you can always cancel the statement once you realize it's taking too long. Also, I don't really like the idea of exposing syntax knobs for what ought to be purely an internal optimization. If someday the optimization becomes unnecessary or radically different in behavior, you're stuck with dead syntax. Sometimes the knob is sufficiently important to take that risk, but it doesn't seem to be so here. 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] Proposal for Allow postgresql.conf values to be changed via SQL
I wrote: But even if we can't make that work, it's not grounds for reserving PERSISTENT. Instead I'd be inclined to forget about RESET PERSISTENT syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that. (BTW, I wonder what behavior that syntax has now in your patch.) In fact, rereading this, I wonder why you think RESET PERSISTENT is a good idea even if there were no bison issues with it. We don't write RESET LOCAL or RESET SESSION, so it seems asymmetric to have RESET PERSISTENT. 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] --single-transaction hack to pg_upgrade does not work
Andres Freund and...@2ndquadrant.com writes: The attached patch does just that. Its *not* ready yet though, as it will be apparent for everyone who reads it ;) ISTM this sort of thing ought to be safe enough, though you probably need to insist both that the pg_type row's xmin be current XID and that it not be HEAP_UPDATED. To really make that work in a reliable manner we would probably need an rd_createSubid for typcache entries instead of testing xmin as I have done here? What's more reliable about that? For one thing, cache entries can get flushed. The relcache goes to some lengths to hang onto rd_createSubid anyway, but I don't want to put equivalent logic into typcache. 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] --single-transaction hack to pg_upgrade does not work
On 12/01/2012 11:38 AM, Andres Freund wrote: On 2012-12-01 17:36:20 +0100, Andres Freund wrote: On 2012-12-01 17:03:03 +0100, Andres Freund wrote: Could we possibly allow adding enum values to a type which was just created in this transaction? That shouldn't be too hard. At least easier than providing the capability to pre-assign the next N oids... The attached patch does just that. Its *not* ready yet though, as it will be apparent for everyone who reads it ;) To really make that work in a reliable manner we would probably need an rd_createSubid for typcache entries instead of testing xmin as I have done here? Does this actually get you over the problem identified in the comment?: * We disallow this in transaction blocks, because we can't cope * with enum OID values getting into indexes and then having their * defining pg_enum entries go away. cheers andrew -- 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] --single-transaction hack to pg_upgrade does not work
On Sat, Dec 1, 2012 at 05:36:20PM +0100, Andres Freund wrote: On 2012-12-01 17:03:03 +0100, Andres Freund wrote: Could we possibly allow adding enum values to a type which was just created in this transaction? That shouldn't be too hard. At least easier than providing the capability to pre-assign the next N oids... The attached patch does just that. Its *not* ready yet though, as it will be apparent for everyone who reads it ;) To really make that work in a reliable manner we would probably need an rd_createSubid for typcache entries instead of testing xmin as I have done here? I can confirm that this patch allows pg_upgrade's test.sh to pass. :-) -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] --single-transaction hack to pg_upgrade does not work
Andrew Dunstan and...@dunslane.net writes: Does this actually get you over the problem identified in the comment?: * We disallow this in transaction blocks, because we can't cope * with enum OID values getting into indexes and then having their * defining pg_enum entries go away. Why wouldn't it? If the enum type was created in the current xact, then surely any table columns of the type, or a fortiori indexes on the type, were also created in the current xact and they'd all go away on abort. 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] --single-transaction hack to pg_upgrade does not work
On 12/01/2012 12:06 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: Does this actually get you over the problem identified in the comment?: * We disallow this in transaction blocks, because we can't cope * with enum OID values getting into indexes and then having their * defining pg_enum entries go away. Why wouldn't it? If the enum type was created in the current xact, then surely any table columns of the type, or a fortiori indexes on the type, were also created in the current xact and they'd all go away on abort. OK, I understand. So this seems like a Good Thing to do. cheers andrew -- 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] --single-transaction hack to pg_upgrade does not work
On 2012-12-01 12:00:46 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: The attached patch does just that. Its *not* ready yet though, as it will be apparent for everyone who reads it ;) ISTM this sort of thing ought to be safe enough, though you probably need to insist both that the pg_type row's xmin be current XID and that it not be HEAP_UPDATED. To really make that work in a reliable manner we would probably need an rd_createSubid for typcache entries instead of testing xmin as I have done here? What's more reliable about that? For one thing, cache entries can get flushed. The relcache goes to some lengths to hang onto rd_createSubid anyway, but I don't want to put equivalent logic into typcache. I was concerned about updated rows but forgot about HEAP_UPDATED. So I thought that it would be possible to alter the type in some generic fashion (e.g. change owner) and then add new values. The typecache variant would also have some hope of allowing some intermediate changes to the type (like changing the type as above) in the same transaction while still allowing to add new values. But then, all that is not necessary for pg_upgrade. Let me provide something a littlebit more mature. 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] --single-transaction hack to pg_upgrade does not work
On 2012-12-01 12:01:17 -0500, Andrew Dunstan wrote: On 12/01/2012 11:38 AM, Andres Freund wrote: On 2012-12-01 17:36:20 +0100, Andres Freund wrote: On 2012-12-01 17:03:03 +0100, Andres Freund wrote: Could we possibly allow adding enum values to a type which was just created in this transaction? That shouldn't be too hard. At least easier than providing the capability to pre-assign the next N oids... The attached patch does just that. Its *not* ready yet though, as it will be apparent for everyone who reads it ;) To really make that work in a reliable manner we would probably need an rd_createSubid for typcache entries instead of testing xmin as I have done here? Does this actually get you over the problem identified in the comment?: * We disallow this in transaction blocks, because we can't cope * with enum OID values getting into indexes and then having their * defining pg_enum entries go away. I don't see why not at least. No index that can contain values from the enum will survive a transaction abort or can be seen from the outside before it committed. So I don't see a problem. What made you concerned? 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] --single-transaction hack to pg_upgrade does not work
Andres Freund and...@2ndquadrant.com writes: On 2012-12-01 12:00:46 -0500, Tom Lane wrote: ISTM this sort of thing ought to be safe enough, though you probably need to insist both that the pg_type row's xmin be current XID and that it not be HEAP_UPDATED. I was concerned about updated rows but forgot about HEAP_UPDATED. So I thought that it would be possible to alter the type in some generic fashion (e.g. change owner) and then add new values. Yeah, I was just thinking about that: we'd have to fail if pg_dump emitted CREATE TYPE, ALTER TYPE OWNER, and then tried to add more values. Fortunately it doesn't do that; the ADD VALUE business is just a multi-statement expansion of CREATE TYPE AS ENUM, and any other ALTERs will come afterwards. Let me provide something a littlebit more mature. It could do with some comments ;-) 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] Tablespaces in the data directory
Magnus Hagander mag...@hagander.net writes: Someone just reported a problem when they had created a new tablespace inside the old data directory. I'm sure there can be other issues caused by this as well, but this is mainly a confusing scenario for people now. As there isn't (as far as I know at least) any actual *point* in creating a tablespace inside the main data directory, should we perhaps disallow this in CREATE TABLESPACE? Or at least throw a WARNING if one does it? It could be pretty hard to detect that in general (think symlinks and such). I guess if we're just trying to print a helpful warning, we don't have to worry about extreme corner cases. But what exactly do you have in mind --- complain about any relative path? Complain about absolute paths that have a prefix matching the DataDir? 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] ALTER TABLE ... NOREWRITE option
On 1 December 2012 16:38, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: It's hard to know whether your tables will be locked for long periods when implementing DDL changes. The NOREWRITE option would cause an ERROR if the table would be rewritten by the command. This would allow testing to highlight long running statements before code hits production. I'm not thrilled about inventing YA keyword for this. If you have a problem with that sort of scenario, why aren't you testing your DDL on a test server before you do it on production? That's the point. You run it on a test server first, and you can conclusively see that it will/will not run for a long time on production server. Greg Sabine Mullane wrote an interesting blog about a way of solving the problem in userspace. Or even more to the point, you can always cancel the statement once you realize it's taking too long. Which means you have to watch it, which is not always possible. Also, I don't really like the idea of exposing syntax knobs for what ought to be purely an internal optimization. If someday the optimization becomes unnecessary or radically different in behavior, you're stuck with dead syntax. Sometimes the knob is sufficiently important to take that risk, but it doesn't seem to be so here. I think it was an interesting idea, but I agree with comments about weird syntax. We need something better and more general for impact assessment. -- Simon Riggs 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] --single-transaction hack to pg_upgrade does not work
On 2012-12-01 12:14:37 -0500, Tom Lane wrote:
Andres Freund and...@2ndquadrant.com writes:
On 2012-12-01 12:00:46 -0500, Tom Lane wrote:
ISTM this sort of thing ought to be safe enough, though you probably
need to insist both that the pg_type row's xmin be current XID and
that it not be HEAP_UPDATED.
I was concerned about updated rows but forgot about HEAP_UPDATED. So I
thought that it would be possible to alter the type in some generic
fashion (e.g. change owner) and then add new values.
Yeah, I was just thinking about that: we'd have to fail if pg_dump
emitted CREATE TYPE, ALTER TYPE OWNER, and then tried to add more
values. Fortunately it doesn't do that; the ADD VALUE business is
just a multi-statement expansion of CREATE TYPE AS ENUM, and any
other ALTERs will come afterwards.
Well, there's a binary_upgrade.set_next_pg_enum_oid() inbetween, but thats
luckily just fine.
Let me provide something a littlebit more mature.
It could do with some comments ;-)
Hehe, yes. Hopefully this version has enough of that.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
From 7288d2cfdd7300bc665ecbfa43640814e665dad1 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Sat, 1 Dec 2012 17:37:57 +0100
Subject: [PATCH] Allow adding new labels to enums inside a transaction if
enum was created in the same txn
Normally it is not safe to do so because the enum values could appear in
indexes even though the transaction aborted but if the enum was originally
created in the same transaction thats not a problem because all indexes
containing the new label won't survive that anyway.
The check employed for testing whether the enum was created in the same txn can
miss some valid cases but it should never miss a case where it would be invalid
to allow this case.
The reason to allow this somewhat strange looking, after all why alter an enum
created in the same txn, case is that pg_dump --binary-upgrade emits CREATE
TYPE typename AS ENUM(); separately from ALTER TYPE ... ADD VALUE to be able to
set the oids of the individual enum labels. Being able to employ
--single-transaction mode during restore speeds up pg_upgrade.
Don't document the relaxation of this restriction in user visible
documentation, it has a too limited scope to be generally interesting.
---
src/backend/access/heap/rewriteheap.c |2 +-
src/backend/commands/typecmds.c | 36 +++--
src/backend/tcop/utility.c| 14 -
src/include/access/htup_details.h |5 +
src/include/commands/typecmds.h |2 +-
src/test/regress/expected/enum.out| 24 ++
src/test/regress/sql/enum.sql | 28 +
7 files changed, 102 insertions(+), 9 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 0f67a80..ae42b2d 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -426,7 +426,7 @@ rewrite_heap_tuple(RewriteState state,
* previous tuple's xmax would equal this one's xmin, so it's
* RECENTLY_DEAD if and only if the xmin is not before OldestXmin.
*/
- if ((new_tuple-t_data-t_infomask HEAP_UPDATED)
+ if (HeapTupleHeaderIsUpdate(new_tuple-t_data)
!TransactionIdPrecedes(HeapTupleHeaderGetXmin(new_tuple-t_data),
state-rs_oldest_xmin))
{
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 8418096..c26800d 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1169,11 +1169,22 @@ DefineEnum(CreateEnumStmt *stmt)
* Adds a new label to an existing enum.
*/
void
-AlterEnum(AlterEnumStmt *stmt)
+AlterEnum(AlterEnumStmt *stmt, bool toplevel)
{
Oid enum_type_oid;
TypeName *typename;
HeapTuple tup;
+ boolin_transaction;
+
+ /*
+ * When executed inside a transaction we need to run some extra checks to
+ * make sure its safe to alter the enum. It is only so if we can be sure
+ * the new value will not end up in an index thats still there after an
+ * abort of this transaction. The only easily detectable case of this is
+ * that the type were adding a value to was also created in this
+ * transaction.
+ */
+ in_transaction = !toplevel || IsTransactionBlock() || IsSubTransaction();
/* Make a TypeName so we can use standard type lookup machinery */
typename = makeTypeNameFromNameList(stmt-typeName);
@@ -1183,12 +1194,33 @@ AlterEnum(AlterEnumStmt *stmt)
if (!HeapTupleIsValid(tup))
elog(ERROR, cache lookup failed for type %u, enum_type_oid);
+ /*
+ * We check whether the type was created in the same transaction by
+ * examining its xmin and checking the tuple was freshly inserted and not
+ * updated. This disallows some valid sequences like CREATE TYPE ... AS
+ * ENUM ...; ALTER
Re: [HACKERS] ALTER TABLE ... NOREWRITE option
On 2012-12-01 18:27:08 +, Simon Riggs wrote: On 1 December 2012 16:38, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: It's hard to know whether your tables will be locked for long periods when implementing DDL changes. The NOREWRITE option would cause an ERROR if the table would be rewritten by the command. This would allow testing to highlight long running statements before code hits production. I'm not thrilled about inventing YA keyword for this. If you have a problem with that sort of scenario, why aren't you testing your DDL on a test server before you do it on production? That's the point. You run it on a test server first, and you can conclusively see that it will/will not run for a long time on production server. Greg Sabine Mullane wrote an interesting blog about a way of solving the problem in userspace. Or even more to the point, you can always cancel the statement once you realize it's taking too long. Which means you have to watch it, which is not always possible. Also, I don't really like the idea of exposing syntax knobs for what ought to be purely an internal optimization. If someday the optimization becomes unnecessary or radically different in behavior, you're stuck with dead syntax. Sometimes the knob is sufficiently important to take that risk, but it doesn't seem to be so here. I think it was an interesting idea, but I agree with comments about weird syntax. We need something better and more general for impact assessment. My first thought is to add more detailed EXPLAIN support for DDL... Although that unfortunately broadens the scope of this a tiny bit. 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] --single-transaction hack to pg_upgrade does not work
On Sat, Dec 1, 2012 at 07:32:48PM +0100, Andres Freund wrote: On 2012-12-01 12:14:37 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2012-12-01 12:00:46 -0500, Tom Lane wrote: ISTM this sort of thing ought to be safe enough, though you probably need to insist both that the pg_type row's xmin be current XID and that it not be HEAP_UPDATED. I was concerned about updated rows but forgot about HEAP_UPDATED. So I thought that it would be possible to alter the type in some generic fashion (e.g. change owner) and then add new values. Yeah, I was just thinking about that: we'd have to fail if pg_dump emitted CREATE TYPE, ALTER TYPE OWNER, and then tried to add more values. Fortunately it doesn't do that; the ADD VALUE business is just a multi-statement expansion of CREATE TYPE AS ENUM, and any other ALTERs will come afterwards. Well, there's a binary_upgrade.set_next_pg_enum_oid() inbetween, but thats luckily just fine. Do we need a comment in pg_dump.c to make sure that doesn't change? Let me provide something a littlebit more mature. It could do with some comments ;-) Hehe, yes. Hopefully this version has enough of that. I believe this text in alter_type.sgml need updating: commandALTER TYPE ... ADD VALUE/ (the form that adds a new value to an enum type) cannot be executed inside a transaction block. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] --single-transaction hack to pg_upgrade does not work
On 2012-12-01 13:43:44 -0500, Bruce Momjian wrote: On Sat, Dec 1, 2012 at 07:32:48PM +0100, Andres Freund wrote: On 2012-12-01 12:14:37 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2012-12-01 12:00:46 -0500, Tom Lane wrote: ISTM this sort of thing ought to be safe enough, though you probably need to insist both that the pg_type row's xmin be current XID and that it not be HEAP_UPDATED. I was concerned about updated rows but forgot about HEAP_UPDATED. So I thought that it would be possible to alter the type in some generic fashion (e.g. change owner) and then add new values. Yeah, I was just thinking about that: we'd have to fail if pg_dump emitted CREATE TYPE, ALTER TYPE OWNER, and then tried to add more values. Fortunately it doesn't do that; the ADD VALUE business is just a multi-statement expansion of CREATE TYPE AS ENUM, and any other ALTERs will come afterwards. Well, there's a binary_upgrade.set_next_pg_enum_oid() inbetween, but thats luckily just fine. Do we need a comment in pg_dump.c to make sure that doesn't change? We could, but I don't really see it likely that somethig problematic will be added there the regression tests should catch any problem there (right?). Let me provide something a littlebit more mature. It could do with some comments ;-) Hehe, yes. Hopefully this version has enough of that. I believe this text in alter_type.sgml need updating: commandALTER TYPE ... ADD VALUE/ (the form that adds a new value to an enum type) cannot be executed inside a transaction block. I purposefully didn't change that because the new support is rather minimalistic. E.g. BEGIN; CREATE TYPE foo AS ENUM(); ALTER TYPE foo RENAME TO bar; ALTER TYPE bar ADD VALUE 'blub'; COMMIT; is not going to work. So it seems best not to make it something official but keep it as an extension for pg_upgrade support. (btw, the commit message inside the git am'able patch contained that explanation...) 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] --single-transaction hack to pg_upgrade does not work
Andres Freund and...@2ndquadrant.com writes: On 2012-12-01 13:43:44 -0500, Bruce Momjian wrote: I believe this text in alter_type.sgml need updating: commandALTER TYPE ... ADD VALUE/ (the form that adds a new value to an enum type) cannot be executed inside a transaction block. I purposefully didn't change that because the new support is rather minimalistic. Yeah, I tend to agree. There are a lot of cases that people might think should work that won't, and anyway it's not clear what the use-case is for this beyond pg_dump's very specific usage. 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] --single-transaction hack to pg_upgrade does not work
Andres Freund and...@2ndquadrant.com writes: On 2012-12-01 12:14:37 -0500, Tom Lane wrote: It could do with some comments ;-) Hehe, yes. Hopefully this version has enough of that. Hm, maybe too many --- I don't really think it's necessary for utility.c to provide a redundant explanation of what's happening. Committed with adjustments --- mainly, the TransactionIdIsCurrentTransactionId test was flat out wrong, because it would accept a parent transaction ID as well as a subcommitted subtransaction ID. We could safely allow the latter, but I don't think it's worth the trouble to add another xact.c test function. 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] --single-transaction hack to pg_upgrade does not work
On Sat, Dec 1, 2012 at 02:31:03PM -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2012-12-01 12:14:37 -0500, Tom Lane wrote: It could do with some comments ;-) Hehe, yes. Hopefully this version has enough of that. Hm, maybe too many --- I don't really think it's necessary for utility.c to provide a redundant explanation of what's happening. Committed with adjustments --- mainly, the TransactionIdIsCurrentTransactionId test was flat out wrong, because it would accept a parent transaction ID as well as a subcommitted subtransaction ID. We could safely allow the latter, but I don't think it's worth the trouble to add another xact.c test function. Thanks everyone. I can confirm that pg_upgrades make check now passes, so this should green the buildfarm. Again, I aplogize for the fire drill. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] --single-transaction hack to pg_upgrade does not work
On 2012-12-01 14:31:03 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2012-12-01 12:14:37 -0500, Tom Lane wrote: It could do with some comments ;-) Hehe, yes. Hopefully this version has enough of that. Hm, maybe too many --- I don't really think it's necessary for utility.c to provide a redundant explanation of what's happening. Yea, was in doubt about that. Added it because it felt a bit strange to pass down isTopLevel. Committed with adjustments --- mainly, the TransactionIdIsCurrentTransactionId test was flat out wrong, because it would accept a parent transaction ID as well as a subcommitted subtransaction ID. We could safely allow the latter, but I don't think it's worth the trouble to add another xact.c test function. Yea, I plainly oversaw that it would be 'dangerous' for a toplevel txn if a subtransaction aborts. I don't really see a usecase for supporting subtxns either, so the current GetCurrentTransactionId() seems sensible. Thanks. Andres -- 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] ALTER TABLE ... NOREWRITE option
I'm not thrilled about inventing YA keyword for this. If you have a
problem with that sort of scenario, why aren't you testing your DDL
on a test server before you do it on production?
*I* do test my DDL. However, there are literally hundreds of thousands
of Rails, Django and Hibernate developers who test their DDL by
running it using a 5-row dataset on their laptops before pushing it to
production. As far as these folks are concerned, rewrite if there's a
default is a completely unintuitive booby-trap.
I agree that adding NOREWRITE is a bad solution, though. Personally,
I'd rather have a system function which tests whether a series of DDL
statements involves a rewrite anywhere. e.g.:
SELECT pg_test_for_rewrite('ALTER TABLE josh ADD COLUMN haircolor')
H. Actually, that wouldn't work with migrations tools, especially
Rails. Better, what about a GUC?
SET message_on_rewrite=WARNING;
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
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] ALTER TABLE ... NOREWRITE option
On 2012-12-01 12:24:57 -0800, Josh Berkus wrote:
I'm not thrilled about inventing YA keyword for this. If you have a
problem with that sort of scenario, why aren't you testing your DDL
on a test server before you do it on production?
*I* do test my DDL. However, there are literally hundreds of thousands
of Rails, Django and Hibernate developers who test their DDL by
running it using a 5-row dataset on their laptops before pushing it to
production. As far as these folks are concerned, rewrite if there's a
default is a completely unintuitive booby-trap.
I agree that adding NOREWRITE is a bad solution, though. Personally,
I'd rather have a system function which tests whether a series of DDL
statements involves a rewrite anywhere. e.g.:
SELECT pg_test_for_rewrite('ALTER TABLE josh ADD COLUMN haircolor')
H. Actually, that wouldn't work with migrations tools, especially
Rails. Better, what about a GUC?
SET message_on_rewrite=WARNING;
If you have a framework and you want to test for this you can just
compare relfilenodes before/after.
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] ALTER TABLE ... NOREWRITE option
If you have a framework and you want to test for this you can just compare relfilenodes before/after. You're targeting the wrong users. This feature isn't for helping you or me. It's for helping the Rails users. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- 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] ALTER TABLE ... NOREWRITE option
On 2012-12-01 12:35:15 -0800, Josh Berkus wrote: If you have a framework and you want to test for this you can just compare relfilenodes before/after. You're targeting the wrong users. This feature isn't for helping you or me. It's for helping the Rails users. I am not targeting anything. All I am saying is that if some framework (not the framework's users!) wants to build something like that today its not all that complicated. 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] --single-transaction hack to pg_upgrade does not work
On 12/01/2012 02:34 PM, Bruce Momjian wrote: On Sat, Dec 1, 2012 at 02:31:03PM -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2012-12-01 12:14:37 -0500, Tom Lane wrote: It could do with some comments ;-) Hehe, yes. Hopefully this version has enough of that. Hm, maybe too many --- I don't really think it's necessary for utility.c to provide a redundant explanation of what's happening. Committed with adjustments --- mainly, the TransactionIdIsCurrentTransactionId test was flat out wrong, because it would accept a parent transaction ID as well as a subcommitted subtransaction ID. We could safely allow the latter, but I don't think it's worth the trouble to add another xact.c test function. Thanks everyone. I can confirm that pg_upgrades make check now passes, so this should green the buildfarm. Again, I aplogize for the fire drill. I've added better logging of pg_upgrade testing to the buildfarm module: https://github.com/PGBuildFarm/client-code/commit/83834cceaea95ba42c03a1079a8c768782e32a6b example is at http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=crakedt=2012-12-01%2017%3A44%3A03 This will be in the next buildfarm client release. cheers andrew -- 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] ALTER TABLE ... NOREWRITE option
-Original Message- From: pgsql-hackers-ow...@postgresql.org [mailto:pgsql-hackers- ow...@postgresql.org] On Behalf Of Andres Freund Sent: 01 December 2012 21:40 To: Josh Berkus Cc: pgsql-hackers@postgresql.org Subject: Re: [HACKERS] ALTER TABLE ... NOREWRITE option On 2012-12-01 12:35:15 -0800, Josh Berkus wrote: If you have a framework and you want to test for this you can just compare relfilenodes before/after. You're targeting the wrong users. This feature isn't for helping you or me. It's for helping the Rails users. I am not targeting anything. All I am saying is that if some framework (not the framework's users!) wants to build something like that today its not all that complicated. I would prefer to be able to detect this *before* the rewrite happens though when writing tooling. Regards Petr Jelinek -- 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] [WIP] pg_ping utility
On Tue, Nov 27, 2012 at 9:43 AM, Phil Sorber p...@omniti.com wrote: On Tue, Nov 27, 2012 at 8:45 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Tue, Nov 27, 2012 at 7:35 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Peter Eisentraut pete...@gmx.net writes: Sure, PQping is useful for this very specific use case of seeing whether the server has finished starting up. If someone came with a plausible Rename the utility to pg_isready? +1, the current version of the patch is already fitted for that and would not need extra options like the number of packages sent. I am 100% fine with this. I can make this change tomorrow. -- Michael Paquier http://michael.otacoo.com Sorry I haven't jumped in on this thread more, I have limited connectivity where I am. Here is the updated patch. I renamed it, but using v5 to stay consistent. pg_isready_bin_v5.diff Description: Binary data pg_isready_docs_v5.diff Description: Binary data -- 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] [PATCH] make -jN check fails / unset MAKEFLAGS in pg_regress.c
Andres Freund and...@2ndquadrant.com writes: Trivially updated patch attached. Applied, thanks. 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] WIP: store additional info in GIN index
On 18.11.2012 22:54, Alexander Korotkov wrote:
Hackers,
Patch completely changes storage in posting lists and leaf pages of
posting trees. It uses varbyte encoding for BlockNumber and
OffsetNumber. BlockNumber are stored incremental in page. Additionally
one bit of OffsetNumber is reserved for additional information NULL
flag. To be able to find position in leaf data page quickly patch
introduces small index in the end of page.
Hi,
I've tried to apply the patch with the current HEAD, but I'm getting
segfaults whenever VACUUM runs (either called directly or from autovac
workers).
The patch applied cleanly against 9b3ac49e and needed a minor fix when
applied on HEAD (because of an assert added to ginRedoCreatePTree), but
that shouldn't be a problem.
The backtrace always looks like this:
Program received signal SIGSEGV, Segmentation fault.
0x004dea3b in processPendingPage (accum=0x7fff15ab8aa0,
ka=0x7fff15ab8a70, page=0x7f88774a7ea0 , startoff=1) at ginfast.c:785
785 addInfo = index_getattr(itup, 2,
accum-ginstate-tupdesc[curattnum - 1], addInfoIsNull);
(gdb) bt
#0 0x004dea3b in processPendingPage (accum=0x7fff15ab8aa0,
ka=0x7fff15ab8a70, page=0x7f88774a7ea0 , startoff=1) at ginfast.c:785
#1 0x004df3c6 in ginInsertCleanup (ginstate=0x7fff15ab97c0,
vac_delay=1 '\001', stats=0xfb0050) at ginfast.c:909
#2 0x004dbe8c in ginbulkdelete (fcinfo=0x7fff15abbfb0) at
ginvacuum.c:747
Reproducing the issue is quite simple:
1) create table messages (id int, txt text, ts tsvector);
2) insert into messages select i, substr(md5(i::text), 0, 4),
to_tsvector('english', substr(md5(i::text), 0, 4))
from generate_series(1,10) s(i);
3) vacuum messages
4) ... segfault :-(
regards
Tomas
--
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] ALTER TABLE ... NOREWRITE option
On Sat, Dec 01, 2012 at 07:34:51PM +0100, Andres Freund wrote: On 2012-12-01 18:27:08 +, Simon Riggs wrote: On 1 December 2012 16:38, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: It's hard to know whether your tables will be locked for long periods when implementing DDL changes. The NOREWRITE option would cause an ERROR if the table would be rewritten by the command. This would allow testing to highlight long running statements before code hits production. I'm not thrilled about inventing YA keyword for this. If you have a problem with that sort of scenario, why aren't you testing your DDL on a test server before you do it on production? That's the point. You run it on a test server first, and you can conclusively see that it will/will not run for a long time on production server. Acquiring the lock could still take an unpredictable amount of time. Greg Sabine Mullane wrote an interesting blog about a way of solving the problem in userspace. I currently recommend using the DEBUG1 messages for this purpose: [local] test=# set client_min_messages = debug1; SET [local] test=# create table t (c int8 primary key, c1 text); DEBUG: building index pg_toast_109381_index on table pg_toast_109381 DEBUG: CREATE TABLE / PRIMARY KEY will create implicit index t_pkey for table t DEBUG: building index t_pkey on table t CREATE TABLE [local] test=# alter table t alter c type int4; DEBUG: building index pg_toast_109391_index on table pg_toast_109391 DEBUG: rewriting table t DEBUG: building index t_pkey on table t ALTER TABLE [local] test=# alter table t alter c type oid; DEBUG: building index t_pkey on table t ALTER TABLE Observe that some changes rewrite the table and all indexes, while others skip rewriting the table but rebuild one or more indexes. I've threatened to optimize type changes like (base type) - (domain with CHECK constraint) by merely scanning the table for violations. If we do add syntax such as you have proposed, I recommend using a different name and defining it to reject any operation with complexity O(n) or worse relative to table size. That being said, I share Tom's doubts. The DEBUG1 messages are a sorry excuse for a UI, but I'm not seeing a clear improvement in NOREWRITE. Or even more to the point, you can always cancel the statement once you realize it's taking too long. Which means you have to watch it, which is not always possible. There's statement_timeout. My first thought is to add more detailed EXPLAIN support for DDL... Although that unfortunately broadens the scope of this a tiny bit. That would be ideal. Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] proposal: separate databases for contrib module testing
I'd like to change the way we set the CONTRIB_TESTDB name for contrib modules. so that each module doesn't wipe out the previous module's test db. The reason is that this will let us test upgrading them using pg_upgrade much more easily. Not testing this is a significant hole in the pg_upgrade testing regime. This can be achieved by a fairly simple change in Makefile.global.in along these lines: ifneq ($(MODULE_big),) CONTRIB_TESTDB = contrib_regression_$(MODULE_big) else ifneq ($(MODULES),) CONTRIB_TESTDB = contrib_regression_$(MODULES) else CONTRIB_TESTDB = contrib_regression endif endif plus some changes in the dblink tests / results that rely on the database name. The downside is that this involves in increase in space of 6.5Mb to 7.5Mb per module. That doesn't seem huge in these days when a standard commodity hard drive is 500Gb and up. Thoughts? cheers andrew -- 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] Proposal for Allow postgresql.conf values to be changed via SQL
On Saturday, December 01, 2012 10:00 PM Tom Lane wrote: Amit Kapila amit.kap...@huawei.com writes : On Saturday, December 01, 2012 1:30 AM Tom Lane wrote: But having said that, it's not apparent to me why inventing SET PERSISTENT should require reserving PERSISTENT. The problem is due to RESET PERSISTENT configuration_variable Syntax. I think the reason is that configuration_variable name can also be persistent, so its not able to resolve. But even if we can't make that work, it's not grounds for reserving PERSISTENT. Instead I'd be inclined to forget about RESET PERSISTENT syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that. (BTW, I wonder what behavior that syntax has now in your patch.) The current behavior is 1. RESET PERSISTENT ... will delete the entry from postgresql.auto.conf 2. SET PERSISTENT... TO DEFAULT will update the entry value in postgresql.auto.conf to default value The discussion for this is at below link: http://archives.postgresql.org/pgsql-hackers/2012-11/msg01276.php I am not too keen to have RESET Persistent, but the only point of keeping it was that it can give user flexibility that it can have the values to take effect from postgresql.conf even if user has executed SET Persistent. Apart from this also user can do that by putting the configuration variable after include_dir in postgresql.conf. With Regards, Amit Kapila. -- 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] Proposal for Allow postgresql.conf values to be changed via SQL
On Saturday, December 01, 2012 10:15 PM Tom Lane wrote: I wrote: But even if we can't make that work, it's not grounds for reserving PERSISTENT. Instead I'd be inclined to forget about RESET PERSISTENT syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that. (BTW, I wonder what behavior that syntax has now in your patch.) In fact, rereading this, I wonder why you think RESET PERSISTENT is a good idea even if there were no bison issues with it. We don't write RESET LOCAL or RESET SESSION, so it seems asymmetric to have RESET PERSISTENT. Currently RESET will reset both local and session specific parameter value on commit. I am not sure if we can change the persistent value with current RESET command. However as you said if we introduce PERSISTENT it will be asymmetric as per current specification of RESET command, so we can even let RESET behavior as it is and user will have mechanism to change default value only with SET PERSISTENT var_name TO DEFAULT. With Regards, Amit Kapila. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
