Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe Reply-To:
On 21 March 2014 23:36, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: On 21 March 2014 20:58, Noah Misch n...@leadboat.com wrote: It's not the behavior I would choose for a new product, but I can't see benefits sufficient to overturn previous decisions to keep it. Speechless The key argument for not fixing this is that it would break existing pg_dump files. That's a pretty hard argument to overcome, unfortunately, even if you're willing to blow off the possibility that client applications might contain similar shortcuts. We still do our best to read dump files from the 7.0 era (see ConvertTriggerToFK() for one example of going above and beyond for that); and every so often we do hear of people trying to get data out of such ancient servers. So even if you went and fixed pg_dump tomorrow, it'd probably be ten or fifteen years before people would let you stop reading dumps from existing versions. Noah had already convinced me, but thank you for explaining the issue behind that. -- 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] ALTER TABLE lock strength reduction patch is unsafe Reply-To:
On 21 March 2014 16:11, Simon Riggs si...@2ndquadrant.com wrote: + * Be careful to ensure this function is called for Tables and Indexes only. + * It is not currently safe to be called for Views because security_barrier + * is listed as an option and so would be allowed to be set at a level lower + * than AccessExclusiveLock, which would not be correct. This statement is accepted and takes only ShareUpdateExclusiveLock: alter table information_schema.triggers set (security_barrier = true); I suggest adding a LOCKMODE field to relopt_gen and adding a reloptions_locklevel() function to determine the required level from an options list. That puts control of the lock level near the code that understands the implications for each option. You can then revert the addition of AlterViewInternal() and some of the utility.c changes. Sure, that's how we code it, but I'm not sure we should introduce that feature. The above weirdness is not itself justification. OK, will follow this path. It's a good idea since it encourages authors of new options to consider properly the lock level that will be required. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On 2/26/14, 9:15 AM, Simon Riggs wrote: On 26 February 2014 13:38, Andres Freundand...@2ndquadrant.com wrote: Hi, On 2014-02-26 07:32:45 +, Simon Riggs wrote: * This definitely should include isolationtester tests actually performing concurrent ALTER TABLEs. All that's currently there is tests that the locklevel isn't too high, but not that it actually works. There is no concurrent behaviour here, hence no code that would be exercised by concurrent tests. Huh? There's most definitely new concurrent behaviour. Previously no other backends could have a relation open (and locked) while it got altered (which then sends out relcache invalidations). That's something that should be tested. It has been. High volume concurrent testing has been performed, per Tom's original discussion upthread, but that's not part of the test suite. For other tests I have no guide as to how to write a set of automated regression tests. Anything could cause a failure, so I'd need to write an infinite set of tests to prove there is no bug*somewhere*. How many tests are required? 0, 1, 3, 30? ISTM that we don't want hand-written tests here, but rather generated tests that actually hit all potential cases. Obviously we'd never run that as part of normal reqression, but farm animals certainly could. -- Jim C. Nasby, Data Architect j...@nasby.net 512.569.9461 (cell) http://jim.nasby.net -- 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 lock strength reduction patch is unsafe Reply-To:
On 21 March 2014 03:45, Noah Misch n...@leadboat.com wrote: On Sat, Mar 08, 2014 at 11:14:30AM +, Simon Riggs wrote: On 7 March 2014 09:04, Simon Riggs si...@2ndquadrant.com wrote: The right thing to do here is to not push to the extremes. If we mess too much with the ruleutil stuff it will just be buggy. A more considered analysis in a later release is required for a full and complete approach. As I indicated earlier, an 80/20 solution is better for this release. Slimming down the patch, I've removed changes to lock levels for almost all variants. The only lock levels now reduced are those for VALIDATE, plus setting of relation and attribute level options. Good call. Thanks for the review. I'll respond to each point on a later email but looks nothing much major, apart from the point raised on separate thread. + * Be careful to ensure this function is called for Tables and Indexes only. + * It is not currently safe to be called for Views because security_barrier + * is listed as an option and so would be allowed to be set at a level lower + * than AccessExclusiveLock, which would not be correct. This statement is accepted and takes only ShareUpdateExclusiveLock: alter table information_schema.triggers set (security_barrier = true); I find it hard to justify why we accept such a statement. Surely its a bug when the named table turns out to be a view? Presumably ALTER SEQUENCE and ALTER other stuff has checks for the correct object type? OMG. I suggest adding a LOCKMODE field to relopt_gen and adding a reloptions_locklevel() function to determine the required level from an options list. That puts control of the lock level near the code that understands the implications for each option. You can then revert the addition of AlterViewInternal() and some of the utility.c changes. Sure, that's how we code it, but I'm not sure we should introduce that feature. The above weirdness is not itself justification. -- 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] ALTER TABLE lock strength reduction patch is unsafe Reply-To:
On Fri, Mar 21, 2014 at 04:11:12PM +, Simon Riggs wrote: On 21 March 2014 03:45, Noah Misch n...@leadboat.com wrote: On Sat, Mar 08, 2014 at 11:14:30AM +, Simon Riggs wrote: Thanks for the review. I'll respond to each point on a later email but looks nothing much major, apart from the point raised on separate thread. Yep. + * Be careful to ensure this function is called for Tables and Indexes only. + * It is not currently safe to be called for Views because security_barrier + * is listed as an option and so would be allowed to be set at a level lower + * than AccessExclusiveLock, which would not be correct. This statement is accepted and takes only ShareUpdateExclusiveLock: alter table information_schema.triggers set (security_barrier = true); I find it hard to justify why we accept such a statement. Surely its a bug when the named table turns out to be a view? Presumably ALTER SEQUENCE and ALTER other stuff has checks for the correct object type? OMG. We've framed ALTER TABLE's relkind leniency as a historic artifact. As a move toward stricter checks, ALTER TABLE refused to operate on foreign tables in 9.1 and 9.2. 9.3 reversed that course, though. For better or worse, ALTER TABLE is nearly a union of the relation ALTER possibilities. That choice is well-entrenched. -- Noah Misch EnterpriseDB http://www.enterprisedb.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 lock strength reduction patch is unsafe Reply-To:
On 21 March 2014 17:49, Noah Misch n...@leadboat.com wrote: + * Be careful to ensure this function is called for Tables and Indexes only. + * It is not currently safe to be called for Views because security_barrier + * is listed as an option and so would be allowed to be set at a level lower + * than AccessExclusiveLock, which would not be correct. This statement is accepted and takes only ShareUpdateExclusiveLock: alter table information_schema.triggers set (security_barrier = true); I find it hard to justify why we accept such a statement. Surely its a bug when the named table turns out to be a view? Presumably ALTER SEQUENCE and ALTER other stuff has checks for the correct object type? OMG. We've framed ALTER TABLE's relkind leniency as a historic artifact. As a move toward stricter checks, ALTER TABLE refused to operate on foreign tables in 9.1 and 9.2. 9.3 reversed that course, though. For better or worse, ALTER TABLE is nearly a union of the relation ALTER possibilities. That choice is well-entrenched. By well entrenched, I think you mean undocumented, untested, unintentional? Do we think anyone *relies* on being able to say the word TABLE when in fact they mean VIEW or SEQUENCE? How is that artefact anything but a bug? i.e. is anyone going to stop me fixing it? -- 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] ALTER TABLE lock strength reduction patch is unsafe Reply-To:
On 21 March 2014 03:45, Noah Misch n...@leadboat.com wrote: + * Note that Hot Standby only knows about AccessExclusiveLocks on the master + * so any changes that might affect SELECTs running on standbys need to use + * AccessExclusiveLocks even if you think a lesser lock would do, unless you + * have a solution for that also. Out of curiosity, do SELECTs on hot standbys impose known challenges in this area not shared with local SELECTs? No, but locks less than AccessExclusiveLock won't happen at all, so its a difference that if improperly handled could cause a bug. Plus I wanted to indicate I'd thought about it. - * 2. Relcache needs to be internally consistent, so unless we lock the - * definition during reads we have no way to guarantee that. I looked for hazards like this, but I found none in the ALTER forms covered by this patch. None of them modify multiple catalog rows affecting the same relcache entry. However, thinking about that did lead me to ponder another class of hazards. When backends can use one or more relations concurrently with a DDL operation affecting those relations, those backends can find themselves running with a subset of the catalog changes made within a particular DDL operation. Consider VALIDATE CONSTRAINT against an inherited constraint of an inheritance parent. It validates child table constraints, modifying one catalog row per table. At COMMIT time, we queue sinval messages for all affected tables. We add to the queue in atomic groups of WRITE_QUANTUM (64) messages. Between two such groups joining the queue, another backend may process the first group of messages. If the original DDL used AccessExclusiveLock, this is always harmless. The DDL-issuing backend still holds its lock, which means the inval-accepting backend must not have the relation open. If the inval-accepting backend later opens the affected relation, it will first acquire some lock and process the rest of the invalidations from the DDL operation. When doing DDL under a weaker lock, the inval-accepting backend might apply half the invalidations and immediately use them in the context of an open relation. For VALIDATE CONSTRAINT, this means a backend might briefly recognize only a subset of the inheritance tree becoming valid. (I did not actually build a test case to confirm this.) Considering that constraint exclusion is the sole consumer of convalidated/ccvalid that can run in parallel with VALIDATE CONSTRAINT, I think this is harmless. I did not find problems of this nature in any ALTER TABLE forms affected by the patch. Let's just keep it in mind during future lock level changes. I'll document pg_get_constraintdef_mvcc() still does syscache lookups by way of decompile_column_index_array(), get_constraint_index(), and deparse_expression_pretty(). It uses MVCC for things that matter for pg_dump vs. reduced lock levels, but not comprehensively. I recommend not adding a new function and instead changing pg_get_constraintdef() to use the transaction snapshot unconditionally. OK -- 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] ALTER TABLE lock strength reduction patch is unsafe Reply-To:
On Fri, Mar 21, 2014 at 06:53:27PM +, Simon Riggs wrote: On 21 March 2014 17:49, Noah Misch n...@leadboat.com wrote: alter table information_schema.triggers set (security_barrier = true); I find it hard to justify why we accept such a statement. Surely its a bug when the named table turns out to be a view? Presumably ALTER SEQUENCE and ALTER other stuff has checks for the correct object type? OMG. We've framed ALTER TABLE's relkind leniency as a historic artifact. As a move toward stricter checks, ALTER TABLE refused to operate on foreign tables in 9.1 and 9.2. 9.3 reversed that course, though. For better or worse, ALTER TABLE is nearly a union of the relation ALTER possibilities. That choice is well-entrenched. By well entrenched, I think you mean undocumented, untested, unintentional? It's deliberate; a -hackers discussion revisits it perhaps once a year. The ALTER VIEW documentation says: For historical reasons, ALTER TABLE can be used with views too; but the only variants of ALTER TABLE that are allowed with views are equivalent to the ones shown above. ALTER INDEX and ALTER SEQUENCE say something similar. Do we think anyone *relies* on being able to say the word TABLE when in fact they mean VIEW or SEQUENCE? pg_dump emits statements that exercise it: psql -c 'create view v as select 1 as c; alter view v alter c set default 0;' pg_dump --table v | grep ALTER How is that artefact anything but a bug? i.e. is anyone going to stop me fixing it? It's not the behavior I would choose for a new product, but I can't see benefits sufficient to overturn previous decisions to keep it. -- Noah Misch EnterpriseDB http://www.enterprisedb.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 lock strength reduction patch is unsafe Reply-To:
On 21 March 2014 20:58, Noah Misch n...@leadboat.com wrote: On Fri, Mar 21, 2014 at 06:53:27PM +, Simon Riggs wrote: On 21 March 2014 17:49, Noah Misch n...@leadboat.com wrote: alter table information_schema.triggers set (security_barrier = true); I find it hard to justify why we accept such a statement. Surely its a bug when the named table turns out to be a view? Presumably ALTER SEQUENCE and ALTER other stuff has checks for the correct object type? OMG. We've framed ALTER TABLE's relkind leniency as a historic artifact. As a move toward stricter checks, ALTER TABLE refused to operate on foreign tables in 9.1 and 9.2. 9.3 reversed that course, though. For better or worse, ALTER TABLE is nearly a union of the relation ALTER possibilities. That choice is well-entrenched. By well entrenched, I think you mean undocumented, untested, unintentional? It's deliberate; a -hackers discussion revisits it perhaps once a year. The ALTER VIEW documentation says: For historical reasons, ALTER TABLE can be used with views too; but the only variants of ALTER TABLE that are allowed with views are equivalent to the ones shown above. ALTER INDEX and ALTER SEQUENCE say something similar. Do we think anyone *relies* on being able to say the word TABLE when in fact they mean VIEW or SEQUENCE? pg_dump emits statements that exercise it: psql -c 'create view v as select 1 as c; alter view v alter c set default 0;' pg_dump --table v | grep ALTER How is that artefact anything but a bug? i.e. is anyone going to stop me fixing it? It's not the behavior I would choose for a new product, but I can't see benefits sufficient to overturn previous decisions to keep it. Speechless -- 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] ALTER TABLE lock strength reduction patch is unsafe Reply-To:
Simon Riggs si...@2ndquadrant.com writes: On 21 March 2014 20:58, Noah Misch n...@leadboat.com wrote: It's not the behavior I would choose for a new product, but I can't see benefits sufficient to overturn previous decisions to keep it. Speechless The key argument for not fixing this is that it would break existing pg_dump files. That's a pretty hard argument to overcome, unfortunately, even if you're willing to blow off the possibility that client applications might contain similar shortcuts. We still do our best to read dump files from the 7.0 era (see ConvertTriggerToFK() for one example of going above and beyond for that); and every so often we do hear of people trying to get data out of such ancient servers. So even if you went and fixed pg_dump tomorrow, it'd probably be ten or fifteen years before people would let you stop reading dumps from existing versions. 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 lock strength reduction patch is unsafe Reply-To:
On 03/18/2014 11:39 AM, Simon Riggs wrote:
On 8 March 2014 11:14, Simon Riggs si...@2ndquadrant.com wrote:
On 7 March 2014 09:04, Simon Riggs si...@2ndquadrant.com wrote:
The right thing to do here is to not push to the extremes. If we mess
too much with the ruleutil stuff it will just be buggy. A more
considered analysis in a later release is required for a full and
complete approach. As I indicated earlier, an 80/20 solution is better
for this release.
Slimming down the patch, I've removed changes to lock levels for
almost all variants. The only lock levels now reduced are those for
VALIDATE, plus setting of relation and attribute level options.
VALIDATE is implemented by calling pg_get_constraintdef_mvcc(), a
slightly modified variant of pg_get_constraintdef that uses the
transaction snapshot. I propose this rather than Noah's solution
solely because this will allow any user to request the MVCC data,
rather than implement a hack that only works for pg_dump. I will post
the patch later today.
Implemented in attached patch, v22
The following commands (only) are allowed with
ShareUpdateExclusiveLock, patch includes doc changes.
ALTER TABLE ... VALIDATE CONSTRAINT constraint_name
covered by isolation test, plus verified manually with pg_dump
ALTER TABLE ... ALTER COLUMN ... SET STATISTICS
ALTER TABLE ... ALTER COLUMN ... SET (...)
ALTER TABLE ... ALTER COLUMN ... RESET (...)
ALTER TABLE ... CLUSTER ON ...
ALTER TABLE ... SET WITHOUT CLUSTER
ALTER TABLE ... SET (...)
covered by isolation test
ALTER TABLE ... RESET (...)
ALTER INDEX ... SET (...)
ALTER INDEX ... RESET (...)
All other ALTER commands take AccessExclusiveLock
I commend this patch to you for final review; I would like to commit
this in a few days.
I'm planning to commit this today at 1500UTC barring objections or
negative reviews.
At my current level of competence, this patch looks good to me. I'm
looking forward to reading Noah's review to see what I may have missed.
The attached patch fixes two typos in the code comments.
--
Vik
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 2939,2945 AlterTableGetLockLevel(List *cmds)
/*
* These subcommands affect implicit row type conversion. They
! * have affects similar to CREATE/DROP CAST on queries.
* don't provide for invalidating parse trees as a result of
* such changes, so we keep these at AccessExclusiveLock.
*/
--- 2939,2945
/*
* These subcommands affect implicit row type conversion. They
! * have affects similar to CREATE/DROP CAST on queries. We
* don't provide for invalidating parse trees as a result of
* such changes, so we keep these at AccessExclusiveLock.
*/
*** a/src/backend/utils/cache/relcache.c
--- b/src/backend/utils/cache/relcache.c
***
*** 1889,1895 RelationDestroyRelation(Relation relation, bool remember_tupdesc)
if (--relation-rd_att-tdrefcount == 0)
{
/*
! * If we Rebuilt a relcache entry during a transaction then its
* possible we did that because the TupDesc changed as the result
* of an ALTER TABLE that ran at less than AccessExclusiveLock.
* It's possible someone copied that TupDesc, in which case the
--- 1889,1895
if (--relation-rd_att-tdrefcount == 0)
{
/*
! * If we Rebuilt a relcache entry during a transaction then it's
* possible we did that because the TupDesc changed as the result
* of an ALTER TABLE that ran at less than AccessExclusiveLock.
* It's possible someone copied that TupDesc, in which case the
--
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 lock strength reduction patch is unsafe Reply-To:
On Sat, Mar 08, 2014 at 11:14:30AM +, Simon Riggs wrote:
On 7 March 2014 09:04, Simon Riggs si...@2ndquadrant.com wrote:
The right thing to do here is to not push to the extremes. If we mess
too much with the ruleutil stuff it will just be buggy. A more
considered analysis in a later release is required for a full and
complete approach. As I indicated earlier, an 80/20 solution is better
for this release.
Slimming down the patch, I've removed changes to lock levels for
almost all variants. The only lock levels now reduced are those for
VALIDATE, plus setting of relation and attribute level options.
Good call.
The following commands (only) are allowed with
ShareUpdateExclusiveLock, patch includes doc changes.
ALTER TABLE ... VALIDATE CONSTRAINT constraint_name
covered by isolation test, plus verified manually with pg_dump
I found a pre-existing bug aggravated by this, which I will shortly report on
a new thread.
ALTER TABLE ... ALTER COLUMN ... SET STATISTICS
ALTER TABLE ... ALTER COLUMN ... SET (...)
ALTER TABLE ... ALTER COLUMN ... RESET (...)
ALTER TABLE ... CLUSTER ON ...
ALTER TABLE ... SET WITHOUT CLUSTER
A comment at check_index_is_clusterable() still mentions exclusive lock.
ALTER TABLE ... SET (...)
covered by isolation test
ALTER TABLE ... RESET (...)
ALTER INDEX ... SET (...)
ALTER INDEX ... RESET (...)
See discussion below.
All other ALTER commands take AccessExclusiveLock
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -865,7 +865,8 @@ ERROR: could not serialize access due to read/write
dependencies among transact
para
Acquired by commandVACUUM/command (without
optionFULL/option),
commandANALYZE/, commandCREATE INDEX CONCURRENTLY/, and
- some forms of commandALTER TABLE/command.
+ commandALTER TABLE VALIDATE/command and other
+ commandALTER TABLE/command variants that set options.
ALTER TABLE's documentation covers the details, so the old text sufficed for
here. I find variants that set options too vague, considering the nuances
of the actual list of affected forms.
/para
/listitem
/varlistentry
@@ -951,7 +952,7 @@ ERROR: could not serialize access due to read/write
dependencies among transact
/para
para
- Acquired by the commandALTER TABLE/, commandDROP TABLE/,
+ Acquired by the commandALTER TABLE/ for rewriting,
commandDROP TABLE/,
commandTRUNCATE/command, commandREINDEX/command,
commandCLUSTER/command, and commandVACUUM FULL/command
commands.
Forms that rewrite the table are just one class of examples. I would write
Acquired by DROP TABLE, ..., VACUUM FULL, and some forms of ALTER TABLE.
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -420,6 +439,9 @@ ALTER TABLE [ IF EXISTS ] replaceable
class=PARAMETERname/replaceable
index specification from the table. This affects
future cluster operations that don't specify an index.
/para
+ para
+ Changing cluster options uses a literalSHARE UPDATE
EXCLUSIVE/literal lock.
+ /para
/listitem
/varlistentry
@@ -467,6 +489,10 @@ ALTER TABLE [ IF EXISTS ] replaceable
class=PARAMETERname/replaceable
FULL/, xref linkend=SQL-CLUSTER or one of the forms
of commandALTER TABLE/ that forces a table rewrite.
/para
+ para
+ Changing storage parameters requires only a
+ literalSHARE UPDATE EXCLUSIVE/literal lock.
+ /para
Some places say requires only, while others say uses. Please adopt one of
those consistently. I somewhat prefer the latter.
@@ -1075,6 +1105,14 @@ ALTER TABLE distributors ADD CONSTRAINT distfk FOREIGN
KEY (address) REFERENCES
/para
para
+ To add a foreign key constraint to a table minimising impact on other
work:
Our documentation has used the minimize spelling exclusively.
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -52,7 +53,7 @@ static bool needs_toast_table(Relation rel);
* to end with CommandCounterIncrement if it makes any changes.
*/
void
-AlterTableCreateToastTable(Oid relOid, Datum reloptions)
+AlterTableCreateToastTable(Oid relOid, Datum reloptions, LOCKMODE lockmode)
{
Relationrel;
@@ -63,10 +64,10 @@ AlterTableCreateToastTable(Oid relOid, Datum reloptions)
* concurrent readers of the pg_class tuple won't have visibility
issues,
* so let's be safe.
*/
The comment ending right here is falsified by the change.
- rel = heap_open(relOid, AccessExclusiveLock);
+ rel = heap_open(relOid, lockmode);
We now request whatever lock our caller has already taken. If that is
AccessExclusiveLock, create_toast_table() could actually add a toast table.
Otherwise, it will either deduce that no change is required or raise an error.
@@
Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe Reply-To:
On 18 March 2014 21:21, Noah Misch n...@leadboat.com wrote: On Tue, Mar 18, 2014 at 10:39:03AM +, Simon Riggs wrote: On 8 March 2014 11:14, Simon Riggs si...@2ndquadrant.com wrote: Implemented in attached patch, v22 I commend this patch to you for final review; I would like to commit this in a few days. I'm planning to commit this today at 1500UTC barring objections or negative reviews. Not an objection, but FYI, I'm currently in the midst of reviewing this. Thanks, I'll holdoff until I hear from you. -- 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] ALTER TABLE lock strength reduction patch is unsafe Reply-To:
On 8 March 2014 11:14, Simon Riggs si...@2ndquadrant.com wrote: On 7 March 2014 09:04, Simon Riggs si...@2ndquadrant.com wrote: The right thing to do here is to not push to the extremes. If we mess too much with the ruleutil stuff it will just be buggy. A more considered analysis in a later release is required for a full and complete approach. As I indicated earlier, an 80/20 solution is better for this release. Slimming down the patch, I've removed changes to lock levels for almost all variants. The only lock levels now reduced are those for VALIDATE, plus setting of relation and attribute level options. VALIDATE is implemented by calling pg_get_constraintdef_mvcc(), a slightly modified variant of pg_get_constraintdef that uses the transaction snapshot. I propose this rather than Noah's solution solely because this will allow any user to request the MVCC data, rather than implement a hack that only works for pg_dump. I will post the patch later today. Implemented in attached patch, v22 The following commands (only) are allowed with ShareUpdateExclusiveLock, patch includes doc changes. ALTER TABLE ... VALIDATE CONSTRAINT constraint_name covered by isolation test, plus verified manually with pg_dump ALTER TABLE ... ALTER COLUMN ... SET STATISTICS ALTER TABLE ... ALTER COLUMN ... SET (...) ALTER TABLE ... ALTER COLUMN ... RESET (...) ALTER TABLE ... CLUSTER ON ... ALTER TABLE ... SET WITHOUT CLUSTER ALTER TABLE ... SET (...) covered by isolation test ALTER TABLE ... RESET (...) ALTER INDEX ... SET (...) ALTER INDEX ... RESET (...) All other ALTER commands take AccessExclusiveLock I commend this patch to you for final review; I would like to commit this in a few days. I'm planning to commit this today at 1500UTC barring objections or negative reviews. -- 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] ALTER TABLE lock strength reduction patch is unsafe Reply-To:
On Tue, Mar 18, 2014 at 10:39:03AM +, Simon Riggs wrote: On 8 March 2014 11:14, Simon Riggs si...@2ndquadrant.com wrote: Implemented in attached patch, v22 I commend this patch to you for final review; I would like to commit this in a few days. I'm planning to commit this today at 1500UTC barring objections or negative reviews. Not an objection, but FYI, I'm currently in the midst of reviewing this. -- Noah Misch EnterpriseDB http://www.enterprisedb.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 lock strength reduction patch is unsafe
On 03/06/2014 10:47 AM, Simon Riggs wrote: On 5 March 2014 09:28, Simon Riggs si...@2ndquadrant.com wrote: So that returns us to solving the catalog consistency problem in pg_dump and similar applications. No answer, guys, and time is ticking away here. Sorry, I've accumulated several days of backlog (and it'll only get worse in the coming week) so I haven't had time to look at all this in the detail I wanted to. I'd like to get a communal solution to this rather than just punting the whole patch. If we have to strip it down to the bar essentials, so be it. For me, the biggest need here is to make VALIDATE CONSTRAINT take only a ShareUpdateExclusiveLock while it runs. Almost everything else needs a full AccessExclusiveLock anyway, or doesn't run for very long so isn't a critical problem. (Perhaps we can then wrap ADD CONSTRAINT ... NOT VALID and VALIDATE into a single command using the CONCURRENTLY keyword so it runs two transactions to complete the task). Validating FKs on big tables can take hours and it really isn't acceptable for us to lock out access while we do that. FKs are *supposed* to be a major reason people use RDBMS, so keeping them in a state where they are effectively unusable is a major debilitating point against adoption of PostgreSQL. If there are issues with pg_dump we can just document them. Guide me with your thoughts. I think committing VALIDATE CONSTRAINT is essential for 9.4; the rest can be delayed until 9.5. None of the discussion in this thread has been about that subcommand, and I don't personally see a problem with it. I don't care much about ADD CONSTRAINT CONCURRENTLY. If it's there, fine. If it's not, that's fine, too. My personal use case for this, and I even started writing a patch before I realized I was re-writing this one, is adding a CHECK constraint NOT VALID so that future commits respect it, then UPDATEing the existing rows to fix them, and then VALIDATE CONSTRAINTing it. There is zero need for an AccessExclusiveLock on that last step. My original idea was to concurrently create a partial index on the bad rows, and then validate the constraint using that index. The AEL would only be held long enough to check if the index is empty or not. Obviously, reducing the lock level is a cleaner solution, so I'd like to see that happen. -- Vik -- 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 lock strength reduction patch is unsafe Reply-To:
On 6 March 2014 22:43, Noah Misch n...@leadboat.com wrote: On Tue, Mar 04, 2014 at 06:50:17PM +0100, Andres Freund wrote: On 2014-03-04 11:40:10 -0500, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: I think this is all too late for 9.4, though. I agree with the feeling that a meaningful fix for pg_dump isn't going to get done for 9.4. So that leaves us with the alternatives of (1) put off the lock-strength-reduction patch for another year; (2) push it anyway and accept a reduction in pg_dump reliability. I don't care for (2). I'd like to have lock strength reduction as much as anybody, but it can't come at the price of reduction of reliability. I am sorry, but I think this is vastly overstating the scope of the pg_dump problem. CREATE INDEX *already* doesn't require a AEL, and the amount of problems that has caused in the past is surprisingly low. If such a frequently used command doesn't cause problems, why are you assuming other commands to be that problematic? And I think it's hard to argue that the proposed changes are more likely to cause problems. Let's try to go at this a bit more methodically. The commands that - afaics - change their locklevel due to latest patch (v21) are: [snip] Good analysis. The hazards arise when pg_dump uses one of the ruleutils.c deparse worker functions. As a cross-check to your study, I looked briefly at the use of those functions in pg_dump and how this patch might affect them: -- pg_get_constraintdef() pg_dump reads the constraint OID with its transaction snapshot, so we will never see a too-new constraint. Dropping a constraint still requires AccessExclusiveLock. Agreed Concerning VALIDATE CONSTRAINT, pg_dump reads convalidated with its transaction snapshot and uses that to decide whether to dump the CHECK constraint as part of the CREATE TABLE or as a separate ALTER TABLE ADD CONSTRAINT following the data load. However, pg_get_constraintdef() reads the latest convalidated to decide whether to emit NOT VALID. Consequently, one can get a dump in which the dumped table data did not yet conform to the constraint, and the ALTER TABLE ADD CONSTRAINT (w/o NOT VALID) fails. (Suppose you deleted the last invalid rows just before executing the VALIDATE CONSTRAINT. I tested this by committing the DELETE + VALIDATE CONSTRAINT with pg_dump stopped at getTableAttrs().) One hacky, but maintainable and effective, solution to the VALIDATE CONSTRAINT problem is to have pg_dump tack on a NOT VALID if pg_get_constraintdef() did not do so. It's, conveniently, the last part of the definition. I would tend to choose this. We could also just decide this isn't bad enough to worry about. The consequence is that an ALTER TABLE ADD CONSTRAINT fails. Assuming no --single-transaction for the original restoral, you just add NOT VALID to the command and rerun. Like most of the potential new pg_dump problems, this can already happen today if the relevant database changes happen between taking the pg_dump transaction snapshot and locking the tables. Too hacky for me, but some good thinking. My proposed solution is below. -- pg_get_expr() for default expressions pg_dump reads pg_attrdef.adbin using its transaction snapshot, so it will never see a too-new default. This does allow us to read a dropped default. That's not a problem directly. However, suppose the default references a function dropped at the same time as the default. pg_dump could fail in pg_get_expr(). -- pg_get_indexdef() As you explained elsewhere, new indexes are no problem. DROP INDEX still requires AccessExclusiveLock. Overall, no problems here. -- pg_get_ruledef() The patch changes lock requirements for enabling and disabling of rules, but that is all separate from the rule expression handling. No problems. -- pg_get_triggerdef() The patch reduces CREATE TRIGGER and DROP TRIGGER to ShareUpdateExclusiveLock. The implications for pg_dump are similar to those for pg_get_expr(). These are certainly concerning. What surprises me the most is that pg_dump has been so happy to randomly mix SQL using the transaction snapshot with sys cache access code using a different snapshot. If that was intention there is no documentation in code or in the docs to explain that. -- pg_get_viewdef() Untamed: pg_dump does not lock views at all. OMG, its really a wonder pg_dump works at all. One thing not to forget is that you can always get the old mutual exclusion back by issuing LOCK TABLE just before a DDL operation. If some unlucky user regularly gets pg_dump failures due to concurrent DROP TRIGGER, he has a workaround. There's no comparable way for someone who would not experience that problem to weaken the now-hardcoded AccessExclusiveLock. Many consequences of insufficient locking are too severe for that workaround to bring comfort, but the pg_dump failure scenarios around pg_get_expr() and pg_get_triggerdef()
Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe
On 5 March 2014 09:28, Simon Riggs si...@2ndquadrant.com wrote: So that returns us to solving the catalog consistency problem in pg_dump and similar applications. No answer, guys, and time is ticking away here. I'd like to get a communal solution to this rather than just punting the whole patch. If we have to strip it down to the bar essentials, so be it. For me, the biggest need here is to make VALIDATE CONSTRAINT take only a ShareUpdateExclusiveLock while it runs. Almost everything else needs a full AccessExclusiveLock anyway, or doesn't run for very long so isn't a critical problem. (Perhaps we can then wrap ADD CONSTRAINT ... NOT VALID and VALIDATE into a single command using the CONCURRENTLY keyword so it runs two transactions to complete the task). Validating FKs on big tables can take hours and it really isn't acceptable for us to lock out access while we do that. FKs are *supposed* to be a major reason people use RDBMS, so keeping them in a state where they are effectively unusable is a major debilitating point against adoption of PostgreSQL. If there are issues with pg_dump we can just document them. Guide me with your thoughts. -- 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] ALTER TABLE lock strength reduction patch is unsafe Reply-To:
On Tue, Mar 04, 2014 at 06:50:17PM +0100, Andres Freund wrote: On 2014-03-04 11:40:10 -0500, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: I think this is all too late for 9.4, though. I agree with the feeling that a meaningful fix for pg_dump isn't going to get done for 9.4. So that leaves us with the alternatives of (1) put off the lock-strength-reduction patch for another year; (2) push it anyway and accept a reduction in pg_dump reliability. I don't care for (2). I'd like to have lock strength reduction as much as anybody, but it can't come at the price of reduction of reliability. I am sorry, but I think this is vastly overstating the scope of the pg_dump problem. CREATE INDEX *already* doesn't require a AEL, and the amount of problems that has caused in the past is surprisingly low. If such a frequently used command doesn't cause problems, why are you assuming other commands to be that problematic? And I think it's hard to argue that the proposed changes are more likely to cause problems. Let's try to go at this a bit more methodically. The commands that - afaics - change their locklevel due to latest patch (v21) are: [snip] Good analysis. The hazards arise when pg_dump uses one of the ruleutils.c deparse worker functions. As a cross-check to your study, I looked briefly at the use of those functions in pg_dump and how this patch might affect them: -- pg_get_constraintdef() pg_dump reads the constraint OID with its transaction snapshot, so we will never see a too-new constraint. Dropping a constraint still requires AccessExclusiveLock. Concerning VALIDATE CONSTRAINT, pg_dump reads convalidated with its transaction snapshot and uses that to decide whether to dump the CHECK constraint as part of the CREATE TABLE or as a separate ALTER TABLE ADD CONSTRAINT following the data load. However, pg_get_constraintdef() reads the latest convalidated to decide whether to emit NOT VALID. Consequently, one can get a dump in which the dumped table data did not yet conform to the constraint, and the ALTER TABLE ADD CONSTRAINT (w/o NOT VALID) fails. (Suppose you deleted the last invalid rows just before executing the VALIDATE CONSTRAINT. I tested this by committing the DELETE + VALIDATE CONSTRAINT with pg_dump stopped at getTableAttrs().) One hacky, but maintainable and effective, solution to the VALIDATE CONSTRAINT problem is to have pg_dump tack on a NOT VALID if pg_get_constraintdef() did not do so. It's, conveniently, the last part of the definition. I would tend to choose this. We could also just decide this isn't bad enough to worry about. The consequence is that an ALTER TABLE ADD CONSTRAINT fails. Assuming no --single-transaction for the original restoral, you just add NOT VALID to the command and rerun. Like most of the potential new pg_dump problems, this can already happen today if the relevant database changes happen between taking the pg_dump transaction snapshot and locking the tables. -- pg_get_expr() for default expressions pg_dump reads pg_attrdef.adbin using its transaction snapshot, so it will never see a too-new default. This does allow us to read a dropped default. That's not a problem directly. However, suppose the default references a function dropped at the same time as the default. pg_dump could fail in pg_get_expr(). -- pg_get_indexdef() As you explained elsewhere, new indexes are no problem. DROP INDEX still requires AccessExclusiveLock. Overall, no problems here. -- pg_get_ruledef() The patch changes lock requirements for enabling and disabling of rules, but that is all separate from the rule expression handling. No problems. -- pg_get_triggerdef() The patch reduces CREATE TRIGGER and DROP TRIGGER to ShareUpdateExclusiveLock. The implications for pg_dump are similar to those for pg_get_expr(). -- pg_get_viewdef() Untamed: pg_dump does not lock views at all. One thing not to forget is that you can always get the old mutual exclusion back by issuing LOCK TABLE just before a DDL operation. If some unlucky user regularly gets pg_dump failures due to concurrent DROP TRIGGER, he has a workaround. There's no comparable way for someone who would not experience that problem to weaken the now-hardcoded AccessExclusiveLock. Many consequences of insufficient locking are too severe for that workaround to bring comfort, but the pg_dump failure scenarios around pg_get_expr() and pg_get_triggerdef() seem mild enough. Restore-time failures are more serious, hence my recommendation to put a hack in pg_dump around VALIDATE CONSTRAINT. Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.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 lock strength reduction patch is unsafe Reply-To:
On 6 March 2014 22:43, Noah Misch n...@leadboat.com wrote: Good analysis. The hazards arise when pg_dump uses one of the ruleutils.c deparse worker functions. Ah, good. We're thinking along the same lines. I was already working on this analysis also. I'll complete mine and then compare notes. One thing not to forget is that you can always get the old mutual exclusion back by issuing LOCK TABLE just before a DDL operation. If some unlucky user regularly gets pg_dump failures due to concurrent DROP TRIGGER, he has a workaround. There's no comparable way for someone who would not experience that problem to weaken the now-hardcoded AccessExclusiveLock. Good point. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On 4 March 2014 21:37, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Mar 4, 2014 at 2:39 PM, Simon Riggs si...@2ndquadrant.com wrote: Your earlier claim that the dump is inconsistent just isn't accurate. We now have MVCC catalogs, so any dump is going to see a perfectly consistent set of data plus DDL. OK the catalogs may change AFTER the snapshot was taken for the dump, but then so can the data change - that's just MVCC. Unfortunately, this isn't correct. The MVCC snapshots taken for catalog scans are instantaneous; that is, we take a new, current snapshot for each catalog scan. If all of the ruleutils.c stuff were using the transaction snapshot rather than instantaneous snapshots, this would be right. But as has been previously discussed, that's not the case. Yeah. And that's *necessary* for catalog lookups in a normally functioning backend, because we have to see latest data (eg, it wouldn't do for a backend to fail to enforce a just-added CHECK constraint because it was committed after the backend's transaction started). OK, thanks for explaining. A valuable point to note for us all. However, it seems possible that we could have a mode in which a read-only session did all its catalog fetches according to the transaction snapshot. That would get us to a situation where the backend-internal lookups that ruleutils relies on would give the same answers as queries done by pg_dump. Robert's work on getting rid of SnapshotNow has probably moved that much closer than it was before, but it's still not exactly a trivial patch. Meanwhile, Andres claimed upthread that none of the currently-proposed reduced-lock ALTER commands affect data that pg_dump is using ruleutils to fetch. If that's the case, then maybe this is a problem that we can punt till later. I've not gone through the list to verify it though. So that returns us to solving the catalog consistency problem in pg_dump and similar applications. We could (1) change the lock taken by pg_dump to be ShareUpdateExclusive. As discussed, this would be optional. (Trivial implementation) The catalog accesses are all in a rather isolated piece of code in pg_dump and run for a short period. That allows us to consider locking *always* at ShareUpdateExclusive but only for the period of catalog access and then release the higher level lock before transaction end. Since pg_dump is a client program any action we take to resolve this would need to be done in a user accessible way. That is acceptable since there may be other user programs that wish/need to read a consistent view of the definition of a table. This can be implemented in a few ways: (2) Implement a server function that allows you to lock a table for a short duration. e.g. pg_lock_catalog(Oid) and pg_unlock_catalog(Oid). We can already do this in server-side code, so this is simply a matter of exposing that same functionality for users. (3) A new variant of the LOCK command: LOCK CATALOG FOR tablename IN lock mode MODE NOWAIT, which then would have a matching UNLOCK CATALOG FOR tablename command. This is just a sugar coated version of (2). (4) Implement functions to suspend invalidation message handling for a short period. That's basically the same as (2) in profile. My feeling is that sounds rather dangerous and not something I'd want to go near now in in the future. We tried to avoid locking the catalog some years back, which is how we went off down this MVCC catalog access, which now seems to have been something of a red-shifted herring. ISTM that the user would need to specifically request a consistent catalog. Using (2) in pg_dump is pretty easy - patch attached. So we can solve this problem completely in about another 1 hour of work, so I suggest we implement (2) and be done. Happy to document this in a new subsection of docs to describe how to dump a consistent view of a database object from a user application. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services lock_catalog_for_pgdump.v1.patch 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] ALTER TABLE lock strength reduction patch is unsafe
On 4 March 2014 01:07, Andres Freund and...@2ndquadrant.com wrote: On 2014-03-03 19:15:27 -0500, Tom Lane wrote: Noah Misch n...@leadboat.com writes: Just to be clear, that list is not a commentary on the particular patch at hand. Those are merely the kinds of regressions to look for in a patch affecting this area of the code. A complaint on pgsql-bugs just now reminded me of a specific area that needs to be looked at hard: how bad are the implications for pg_dump? Up to now, pg_dump could be reasonably confident that once it had AccessShareLock on every table it intended to dump, there would be no schema changes happening on those tables until it got done. The guarantee wasn't actually that strong. It already was quite possible that indexes got created/dropped during that time, which probably is the by far most frequent DDL run in production. Good points. In most cases, DDL is applied manually after careful thought, so people seldom dump at the same time they upgrade the database. This is especially true for pg_dump since it captures the logical definition of tables. So most people will be happy with the default locking, but we could make the lock level optional. Currently we use AccessShareLock. Locking out all DDL, even with this patch applied would only require ShareUpdateExclusiveLock. Looking at the code, it will take about an hour to add an option to pg_dump that specifies the lock level used when dumping. I would be happy to include that as part of this patch. -- 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] ALTER TABLE lock strength reduction patch is unsafe
Good points. In most cases, DDL is applied manually after careful thought, so people seldom dump at the same time they upgrade the database. This is especially true for pg_dump since it captures the logical definition of tables. So most people will be happy with the default locking, but we could make the lock level optional. Currently we use AccessShareLock. Locking out all DDL, even with this patch applied would only require ShareUpdateExclusiveLock. Looking at the code, it will take about an hour to add an option to pg_dump that specifies the lock level used when dumping. I would be happy to include that as part of this patch. I think the use case for specifying multiple locks is pretty slim given that a ShareUpdateExclusiveLock is good enough mostly for everybody. If its not the case, the user should be more careful about when he is scheduling backups to so that they dont conflict with DDL changes. I am not too comfortable with exposing the locking type to the user. That may be just me though. Regards, Atri -- Regards, Atri *l'apprenant*
Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe
On 4 March 2014 08:39, Atri Sharma atri.j...@gmail.com wrote: Good points. In most cases, DDL is applied manually after careful thought, so people seldom dump at the same time they upgrade the database. This is especially true for pg_dump since it captures the logical definition of tables. So most people will be happy with the default locking, but we could make the lock level optional. Currently we use AccessShareLock. Locking out all DDL, even with this patch applied would only require ShareUpdateExclusiveLock. Looking at the code, it will take about an hour to add an option to pg_dump that specifies the lock level used when dumping. I would be happy to include that as part of this patch. I think the use case for specifying multiple locks is pretty slim given that a ShareUpdateExclusiveLock is good enough mostly for everybody. Increasing the lock strength would be a change in behaviour that might adversely affect existing users. If its not the case, the user should be more careful about when he is scheduling backups to so that they dont conflict with DDL changes. That is most certainly the wise choice. I am not too comfortable with exposing the locking type to the user. That may be just me though. Why would that be a problem? Hard reasons, please. -- 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] ALTER TABLE lock strength reduction patch is unsafe
If its not the case, the user should be more careful about when he is scheduling backups to so that they dont conflict with DDL changes. That is most certainly the wise choice. I am not too comfortable with exposing the locking type to the user. That may be just me though. Why would that be a problem? Hard reasons, please. Should we genuinely depend on the user's good judgement to decide the locking types? -- Regards, Atri *l'apprenant*
Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe
On 4 March 2014 09:31, Simon Riggs si...@2ndquadrant.com wrote: On 4 March 2014 08:39, Atri Sharma atri.j...@gmail.com wrote: Good points. In most cases, DDL is applied manually after careful thought, so people seldom dump at the same time they upgrade the database. This is especially true for pg_dump since it captures the logical definition of tables. So most people will be happy with the default locking, but we could make the lock level optional. Currently we use AccessShareLock. Locking out all DDL, even with this patch applied would only require ShareUpdateExclusiveLock. Looking at the code, it will take about an hour to add an option to pg_dump that specifies the lock level used when dumping. I would be happy to include that as part of this patch. I think the use case for specifying multiple locks is pretty slim given that a ShareUpdateExclusiveLock is good enough mostly for everybody. Increasing the lock strength would be a change in behaviour that might adversely affect existing users. The main impact I see is that this would block VACUUM while pg_dump runs. But then, while pg_dump runs VACUUM is ineffective anyway so perhaps that is no bad thing. Autovacuum requests VACOPT_NOWAIT so would skip the relations being dumped rather than waiting. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On Tue, Mar 4, 2014 at 6:57 AM, Simon Riggs si...@2ndquadrant.com wrote: The main impact I see is that this would block VACUUM while pg_dump runs. But then, while pg_dump runs VACUUM is ineffective anyway so perhaps that is no bad thing. Well, a vacuum that's already running when pg_dump starts up may be doing a lot of good, so it would be a shame to see pg_dump kill them all off. Also, this would put us in the surprising situation that you can't run two simultaneous dumps of overlapping sets of tables, which doesn't strike me as a great thing. I'd really like to see us find a way to apply some version of this patch. I was in favor of the concept 3 years ago when we did this the first time, and I've subsequently done quite a bit of work (viz., MVCC catalog snapshots) to eliminate the main objection that was raised at that time. But it's really hard to reason about what might happen with lowered lock levels, and convince yourself that there's absolutely nothing that can ever go wrong. I don't know what to do about that tension, but I think even modest improvements in this area stand to benefit an awful lot of users. -- Robert Haas EnterpriseDB: 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] ALTER TABLE lock strength reduction patch is unsafe
I'd really like to see us find a way to apply some version of this patch. I was in favor of the concept 3 years ago when we did this the first time, and I've subsequently done quite a bit of work (viz., MVCC catalog snapshots) to eliminate the main objection that was raised at that time. But it's really hard to reason about what might happen with lowered lock levels, and convince yourself that there's absolutely nothing that can ever go wrong. I don't know what to do about that tension, but I think even modest improvements in this area stand to benefit an awful lot of users. Wouldnt MVCC's strictness rules pose harder restrictions on pg_dump instead of relaxing them? Regards, Atri -- Regards, Atri *l'apprenant*
Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe
On 4 March 2014 12:18, Robert Haas robertmh...@gmail.com wrote: On Tue, Mar 4, 2014 at 6:57 AM, Simon Riggs si...@2ndquadrant.com wrote: The main impact I see is that this would block VACUUM while pg_dump runs. But then, while pg_dump runs VACUUM is ineffective anyway so perhaps that is no bad thing. Well, a vacuum that's already running when pg_dump starts up may be doing a lot of good, so it would be a shame to see pg_dump kill them all off. Also, this would put us in the surprising situation that you can't run two simultaneous dumps of overlapping sets of tables, which doesn't strike me as a great thing. These changes in concurrency are the most serious objections and a definite change in previous behaviour. So we cannot pick a single lock level that suits all goals the user may have. I'd really like to see us find a way to apply some version of this patch. I was in favor of the concept 3 years ago when we did this the first time, and I've subsequently done quite a bit of work (viz., MVCC catalog snapshots) to eliminate the main objection that was raised at that time. But it's really hard to reason about what might happen with lowered lock levels, and convince yourself that there's absolutely nothing that can ever go wrong. I don't know what to do about that tension, but I think even modest improvements in this area stand to benefit an awful lot of users. Agreed. The question is, which subset? The issue just raised would affect whichever subset we choose, so reducing the scope of the patch does nothing to the impact of the pg_dump issue. I will add the option to change lock level for pg_dump. It's simple to use, clear as to why it would be needed and effective at removing this as an obstacle. -- 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] ALTER TABLE lock strength reduction patch is unsafe
* Atri Sharma (atri.j...@gmail.com) wrote: If its not the case, the user should be more careful about when he is scheduling backups to so that they dont conflict with DDL changes. I'm not following this as closely as I'd like to, but I wanted to voice my opinion that this is just not acceptable as a general answer. There are a good many applications out there which do DDL as part of ongoing activity (part of ETL, or something else) and still need to be able to get a pg_dump done. It's not a design I'd recommend, but I don't think we get to just write it off either. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe
Stephen Frost escribió: * Atri Sharma (atri.j...@gmail.com) wrote: If its not the case, the user should be more careful about when he is scheduling backups to so that they dont conflict with DDL changes. I'm not following this as closely as I'd like to, but I wanted to voice my opinion that this is just not acceptable as a general answer. There are a good many applications out there which do DDL as part of ongoing activity (part of ETL, or something else) and still need to be able to get a pg_dump done. It's not a design I'd recommend, but I don't think we get to just write it off either. Agreed -- user caution is a recipe for trouble, because these things cannot always be planned in minute detail (or such planning creates an excessive cost.) One concern is schema changes that make a dump unrestorable, for instance if there's a foreign key relationship between tables A and B, such that pg_dump dumps the FK for table A but by the time it dumps table B the unique index has gone and thus restoring the FK fails. If this is a realistic failure scenario, then we need some mechanism to avoid it. One possible idea would be to create a new lock level which conflicts with DDL changes but not with regular operation including dumps; so it wouldn't self-conflict but it would conflict with ShareUpdateExclusive. pg_dump would acquire a lock of that level instead of AccessShare; thus two pg_dumps would be able to run on the same table simultaneously, but it would block and be blocked by DDL changes that grab SUE. The big hole in this is that pg_dump would still block vacuum, which is a problem. I hesitate two suggest two extra levels, one for dumps (which wouldn't conflict with SUE) and one for non-exclusive DDL changes (which would.) -- Álvaro Herrerahttp://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 lock strength reduction patch is unsafe
On Tue, Mar 4, 2014 at 8:19 PM, Stephen Frost sfr...@snowman.net wrote: * Atri Sharma (atri.j...@gmail.com) wrote: If its not the case, the user should be more careful about when he is scheduling backups to so that they dont conflict with DDL changes. I'm not following this as closely as I'd like to, but I wanted to voice my opinion that this is just not acceptable as a general answer. There are a good many applications out there which do DDL as part of ongoing activity (part of ETL, or something else) and still need to be able to get a pg_dump done. It's not a design I'd recommend, but I don't think we get to just write it off either. Well, that will require something like MVCC or stricter locking in general. That is not in line with the aim of this patch, hence I raised this point. Regards, Atri Regards, Atri *l'apprenant*
Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe
On Tue, Mar 4, 2014 at 10:17 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: One possible idea would be to create a new lock level which conflicts with DDL changes but not with regular operation including dumps; so it wouldn't self-conflict but it would conflict with ShareUpdateExclusive. pg_dump would acquire a lock of that level instead of AccessShare; thus two pg_dumps would be able to run on the same table simultaneously, but it would block and be blocked by DDL changes that grab SUE. The big hole in this is that pg_dump would still block vacuum, which is a problem. I hesitate two suggest two extra levels, one for dumps (which wouldn't conflict with SUE) and one for non-exclusive DDL changes (which would.) AFAIK, the only reason why vacuum takes ShareUpdateExclusive lock is because it can't run at the same time as another vacuum. I tend to think (and have thought for some time) that we really ought to have vacuum take AccessShareLock on the relation plus some other lock that is specific to vacuum (say, a relation vacuum lock, just as we have relation extension locks). Your idea of a lock strong enough to conflict with DDL but not self-conflicting is interesting, too, but I can't claim to have thought through it all that carefully just yet. I think this is all too late for 9.4, though. -- Robert Haas EnterpriseDB: 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] ALTER TABLE lock strength reduction patch is unsafe
Alvaro Herrera alvhe...@2ndquadrant.com writes: One concern is schema changes that make a dump unrestorable, for instance if there's a foreign key relationship between tables A and B, Yeah. Ideally, what pg_dump would produce would be a consistent snapshot of the database state as of its transaction snapshot time. We have always had that guarantee so far as user data was concerned, but it's been shaky (and getting worse) so far as the database schema is concerned. What bothers me about the current patch is that it's going to make it a whole lot more worse. Also, I don't have any love at all for proposals that we increase the lock level that pg_dump holds. pg_dump tends to run for a long time. I've not been paying all that much attention to the logical-decoding patches, but wasn't there something in there about being able to see the catalog state as it was at some point in the past? If so, maybe we could leverage that to allow a backend to enter a pg_dump state wherein its view of the catalogs was frozen at its transaction start snapshot. We'd have to restrict it to read-only operation for safety, but that's surely no problem for pg_dump. If we had that, then this whole problem of server-side computations producing inconsistent results would go away. There might still be a window wherein tables visible at transaction start could be dropped before AccessShareLock could be acquired, but I think we could let pg_dump error out in that case. 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 lock strength reduction patch is unsafe
* Tom Lane (t...@sss.pgh.pa.us) wrote: Yeah. Ideally, what pg_dump would produce would be a consistent snapshot of the database state as of its transaction snapshot time. We have always had that guarantee so far as user data was concerned, but it's been shaky (and getting worse) so far as the database schema is concerned. What bothers me about the current patch is that it's going to make it a whole lot more worse. Also, I don't have any love at all for proposals that we increase the lock level that pg_dump holds. pg_dump tends to run for a long time. Agreed. I've not been paying all that much attention to the logical-decoding patches, but wasn't there something in there about being able to see the catalog state as it was at some point in the past? If so, maybe we could leverage that to allow a backend to enter a pg_dump state wherein its view of the catalogs was frozen at its transaction start snapshot. We'd have to restrict it to read-only operation for safety, but that's surely no problem for pg_dump. If we had that, then this whole problem of server-side computations producing inconsistent results would go away. That certainly sounds like a tempting idea. There might still be a window wherein tables visible at transaction start could be dropped before AccessShareLock could be acquired, but I think we could let pg_dump error out in that case. I don't have too much of an issue with the above, but I would like to have us figure out a solution to the deadlock problem with parallel pg_dump. The issue arises when pg_dump gets an AccessShareLock and then another process attempts to acquire an AccessExclusiveLock, which then blocks, and then the pg_dump worker process tries to get its AccessShareLock- we end up not being able to make any progress on anything at that point. One suggestion that was discussed at PGConf.EU was having processes which share the same snapshot (the pg_dump master and worker processes) able to either share the same locks or at least be able to jump the lock queue (that is, the worker process wouldn't have to wait being the AEL to get an ASL, since the ASL was already aquired for the snapshot which was exported and shared with it). Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe
Robert Haas robertmh...@gmail.com writes: I think this is all too late for 9.4, though. I agree with the feeling that a meaningful fix for pg_dump isn't going to get done for 9.4. So that leaves us with the alternatives of (1) put off the lock-strength-reduction patch for another year; (2) push it anyway and accept a reduction in pg_dump reliability. I don't care for (2). I'd like to have lock strength reduction as much as anybody, but it can't come at the price of reduction of reliability. The bigger picture here is that it seems like anytime I've thought for more than five minutes about the lock strength reduction patch, I've come up with some fundamental problem. That doesn't leave me with a warm feeling that we're getting close to having something committable. 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 lock strength reduction patch is unsafe
Stephen Frost sfr...@snowman.net writes: I don't have too much of an issue with the above, but I would like to have us figure out a solution to the deadlock problem with parallel pg_dump. The issue arises when pg_dump gets an AccessShareLock and then another process attempts to acquire an AccessExclusiveLock, which then blocks, and then the pg_dump worker process tries to get its AccessShareLock- we end up not being able to make any progress on anything at that point. The original ASL was acquired by the pg_dump master, right? One suggestion that was discussed at PGConf.EU was having processes which share the same snapshot (the pg_dump master and worker processes) able to either share the same locks or at least be able to jump the lock queue (that is, the worker process wouldn't have to wait being the AEL to get an ASL, since the ASL was already aquired for the snapshot which was exported and shared with it). Yeah, it seems like we need lock export not only snapshot export to make this work nicely. But that sounds orthogonal to the issues being discussed in this thread. 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 lock strength reduction patch is unsafe
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: I don't have too much of an issue with the above, but I would like to have us figure out a solution to the deadlock problem with parallel pg_dump. The issue arises when pg_dump gets an AccessShareLock and then another process attempts to acquire an AccessExclusiveLock, which then blocks, and then the pg_dump worker process tries to get its AccessShareLock- we end up not being able to make any progress on anything at that point. The original ASL was acquired by the pg_dump master, right? Yup. It goes through and gets ASLs on everything first. One suggestion that was discussed at PGConf.EU was having processes which share the same snapshot (the pg_dump master and worker processes) able to either share the same locks or at least be able to jump the lock queue (that is, the worker process wouldn't have to wait being the AEL to get an ASL, since the ASL was already aquired for the snapshot which was exported and shared with it). Yeah, it seems like we need lock export not only snapshot export to make this work nicely. But that sounds orthogonal to the issues being discussed in this thread. Indeed, just figured I'd mention it since we're talking about pg_dump-related locking. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe
Tom Lane escribió: I'd like to have lock strength reduction as much as anybody, but it can't come at the price of reduction of reliability. Can we have at least a cut-down version of it? If we can just reduce the lock level required for ALTER TABLE / VALIDATE, that would be an enormous improvement already. -- Álvaro Herrerahttp://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 lock strength reduction patch is unsafe
On Tue, Mar 4, 2014 at 10:20 PM, Stephen Frost sfr...@snowman.net wrote: * Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: I don't have too much of an issue with the above, but I would like to have us figure out a solution to the deadlock problem with parallel pg_dump. The issue arises when pg_dump gets an AccessShareLock and then another process attempts to acquire an AccessExclusiveLock, which then blocks, and then the pg_dump worker process tries to get its AccessShareLock- we end up not being able to make any progress on anything at that point. The original ASL was acquired by the pg_dump master, right? Yup. It goes through and gets ASLs on everything first. One suggestion that was discussed at PGConf.EU was having processes which share the same snapshot (the pg_dump master and worker processes) able to either share the same locks or at least be able to jump the lock queue (that is, the worker process wouldn't have to wait being the AEL to get an ASL, since the ASL was already aquired for the snapshot which was exported and shared with it). Yeah, it seems like we need lock export not only snapshot export to make this work nicely. But that sounds orthogonal to the issues being discussed in this thread. Indeed, just figured I'd mention it since we're talking about pg_dump-related locking. What happens for foreign key constraints? For pg_dump, do we lock the tables referenced by the table which is being dumped right now? If that is the case, wouldnt MVCC based approach be the best way for this? Please ignore if I said anything silly. I am just trying to understand how it works here. Regards, Atri
Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe Reply-To:
On 2014-03-04 11:40:10 -0500, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: I think this is all too late for 9.4, though. I agree with the feeling that a meaningful fix for pg_dump isn't going to get done for 9.4. So that leaves us with the alternatives of (1) put off the lock-strength-reduction patch for another year; (2) push it anyway and accept a reduction in pg_dump reliability. I don't care for (2). I'd like to have lock strength reduction as much as anybody, but it can't come at the price of reduction of reliability. I am sorry, but I think this is vastly overstating the scope of the pg_dump problem. CREATE INDEX *already* doesn't require a AEL, and the amount of problems that has caused in the past is surprisingly low. If such a frequently used command doesn't cause problems, why are you assuming other commands to be that problematic? And I think it's hard to argue that the proposed changes are more likely to cause problems. Let's try to go at this a bit more methodically. The commands that - afaics - change their locklevel due to latest patch (v21) are: 01) ALTER TABLE .. ADD CONSTRAINT 02) ALTER TABLE .. ADD CONSTRAINT ... USING 03) ALTER TABLE .. ENABLE | DISABLE [ REPLICA | ALWAYS ] TRIGGER [ ALL ] 04) ALTER TABLE .. ALTER CONSTRAINT 05) ALTER TABLE .. REPLICA IDENTITY 06) ALTER TABLE .. ALTER COLUMN .. SET NOT NULL (*not* DROP NULL) cmd_lockmode = ShareRowExclusiveLock; 07) ALTER TABLE ... ALTER COLUMN ... SET STATISTICS 08) ALTER TABLE ... CLUSTER ON ... 09) ALTER TABLE ... SET WITHOUT CLUSTER 10) ALTER TABLE ... SET (...) 11) ALTER TABLE ... RESET (...) 12) ALTER TABLE ... ALTER COLUMN ... SET (...) 13) ALTER TABLE ... ALTER COLUMN ... RESET (...) 14) ALTER TABLE ... VALIDATE CONSTRAINT constraint_name cmd_lockmode = ShareUpdateExclusiveLock; I have my reservations about ADD CONSTRAINT (including SET NOT NULL) being unproblematic (mostly because I haven't thought through possible consquences for the planner making different choices with added constraints). From the perspective of pg_dump consistency, except ADD CONSTRAINT, none of those seem to have graver possible consequences than CREATE INDEX (and DROP INDEX CONCURRENTLY) already being unsafe. In fact all of those should actually end up being *safer*, even ending up always being dumped consistently since they are all reconstructed clientside by pg_dump. You argue elsewhere that that's a fragile coincidence. But so what, even if it changes, the consequences still are going to be *far* less significant than missing various index, trigger, and whatnot commands. I think the set of problems you mention are going to be really important when we someday get around to make stuff like ALTER TABLE ... ADD/DROP COLUMN require lower lock levels, but that's not what's proposed. 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 lock strength reduction patch is unsafe
On 4 March 2014 16:27, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: One concern is schema changes that make a dump unrestorable, for instance if there's a foreign key relationship between tables A and B, Yeah. Ideally, what pg_dump would produce would be a consistent snapshot of the database state as of its transaction snapshot time. We have always had that guarantee so far as user data was concerned, but it's been shaky (and getting worse) so far as the database schema is concerned. What bothers me about the current patch is that it's going to make it a whole lot more worse. While thinking this through it occurs to me that there is no problem at all. Your earlier claim that the dump is inconsistent just isn't accurate. We now have MVCC catalogs, so any dump is going to see a perfectly consistent set of data plus DDL. OK the catalogs may change AFTER the snapshot was taken for the dump, but then so can the data change - that's just MVCC. I was going to add an option to increase lock level, but I can't see why you'd want it even. The dumps are consistent... -- 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] ALTER TABLE lock strength reduction patch is unsafe
On March 4, 2014 8:39:55 PM CET, Simon Riggs si...@2ndquadrant.com wrote: On 4 March 2014 16:27, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: One concern is schema changes that make a dump unrestorable, for instance if there's a foreign key relationship between tables A and B, Yeah. Ideally, what pg_dump would produce would be a consistent snapshot of the database state as of its transaction snapshot time. We have always had that guarantee so far as user data was concerned, but it's been shaky (and getting worse) so far as the database schema is concerned. What bothers me about the current patch is that it's going to make it a whole lot more worse. While thinking this through it occurs to me that there is no problem at all. Your earlier claim that the dump is inconsistent just isn't accurate. We now have MVCC catalogs, so any dump is going to see a perfectly consistent set of data plus DDL. OK the catalogs may change AFTER the snapshot was taken for the dump, but then so can the data change - that's just MVCC. I was going to add an option to increase lock level, but I can't see why you'd want it even. The dumps are consistent... Mvcc scans only guarantee that individual scans are consistent, not that separate scans are. Each individual scan takes a new snapshot if there's been ddl. Andres -- Please excuse brevity and formatting - I am writing this on my mobile phone. 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 lock strength reduction patch is unsafe Reply-To:
Andres Freund and...@2ndquadrant.com writes: On 2014-03-04 11:40:10 -0500, Tom Lane wrote: I don't care for (2). I'd like to have lock strength reduction as much as anybody, but it can't come at the price of reduction of reliability. I am sorry, but I think this is vastly overstating the scope of the pg_dump problem. CREATE INDEX *already* doesn't require a AEL, and the amount of problems that has caused in the past is surprisingly low. CREATE INDEX happens to be okay because pg_dump won't try to dump indexes it doesn't see in its snapshot, ie the list of indexes to dump is created client-side. CREATE INDEX CONCURRENTLY, otoh, already did break pg_dump, and we had to hack things to fix it; see commit 683abc73dff549e94555d4020dae8d02f32ed78b. 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 lock strength reduction patch is unsafe
On Tue, Mar 4, 2014 at 2:39 PM, Simon Riggs si...@2ndquadrant.com wrote: On 4 March 2014 16:27, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: One concern is schema changes that make a dump unrestorable, for instance if there's a foreign key relationship between tables A and B, Yeah. Ideally, what pg_dump would produce would be a consistent snapshot of the database state as of its transaction snapshot time. We have always had that guarantee so far as user data was concerned, but it's been shaky (and getting worse) so far as the database schema is concerned. What bothers me about the current patch is that it's going to make it a whole lot more worse. While thinking this through it occurs to me that there is no problem at all. Your earlier claim that the dump is inconsistent just isn't accurate. We now have MVCC catalogs, so any dump is going to see a perfectly consistent set of data plus DDL. OK the catalogs may change AFTER the snapshot was taken for the dump, but then so can the data change - that's just MVCC. Unfortunately, this isn't correct. The MVCC snapshots taken for catalog scans are instantaneous; that is, we take a new, current snapshot for each catalog scan. If all of the ruleutils.c stuff were using the transaction snapshot rather than instantaneous snapshots, this would be right. But as has been previously discussed, that's not the case. -- Robert Haas EnterpriseDB: 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] ALTER TABLE lock strength reduction patch is unsafe
Robert Haas robertmh...@gmail.com writes: On Tue, Mar 4, 2014 at 2:39 PM, Simon Riggs si...@2ndquadrant.com wrote: Your earlier claim that the dump is inconsistent just isn't accurate. We now have MVCC catalogs, so any dump is going to see a perfectly consistent set of data plus DDL. OK the catalogs may change AFTER the snapshot was taken for the dump, but then so can the data change - that's just MVCC. Unfortunately, this isn't correct. The MVCC snapshots taken for catalog scans are instantaneous; that is, we take a new, current snapshot for each catalog scan. If all of the ruleutils.c stuff were using the transaction snapshot rather than instantaneous snapshots, this would be right. But as has been previously discussed, that's not the case. Yeah. And that's *necessary* for catalog lookups in a normally functioning backend, because we have to see latest data (eg, it wouldn't do for a backend to fail to enforce a just-added CHECK constraint because it was committed after the backend's transaction started). However, it seems possible that we could have a mode in which a read-only session did all its catalog fetches according to the transaction snapshot. That would get us to a situation where the backend-internal lookups that ruleutils relies on would give the same answers as queries done by pg_dump. Robert's work on getting rid of SnapshotNow has probably moved that much closer than it was before, but it's still not exactly a trivial patch. Meanwhile, Andres claimed upthread that none of the currently-proposed reduced-lock ALTER commands affect data that pg_dump is using ruleutils to fetch. If that's the case, then maybe this is a problem that we can punt till later. I've not gone through the list to verify it though. 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 lock strength reduction patch is unsafe
On 03/04/2014 11:43 AM, Andres Freund wrote: On March 4, 2014 8:39:55 PM CET, Simon Riggs si...@2ndquadrant.com wrote: I was going to add an option to increase lock level, but I can't see why you'd want it even. The dumps are consistent... Mvcc scans only guarantee that individual scans are consistent, not that separate scans are. Each individual scan takes a new snapshot if there's been ddl. Andres I thought that we were sharing the same snapshot, for parallel dump? -- 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 lock strength reduction patch is unsafe
On 2014-03-04 14:29:31 -0800, Josh Berkus wrote: On 03/04/2014 11:43 AM, Andres Freund wrote: On March 4, 2014 8:39:55 PM CET, Simon Riggs si...@2ndquadrant.com wrote: I was going to add an option to increase lock level, but I can't see why you'd want it even. The dumps are consistent... Mvcc scans only guarantee that individual scans are consistent, not that separate scans are. Each individual scan takes a new snapshot if there's been ddl. I thought that we were sharing the same snapshot, for parallel dump? That snapshot is about data, not the catalog. And no, we can't easily reuse one for the other, see elsewhere in this thread for some of the reasons. 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 lock strength reduction patch is unsafe Reply-To:
On Tue, Mar 4, 2014 at 8:08 PM, Tom Lane t...@sss.pgh.pa.us wrote: CREATE INDEX CONCURRENTLY, otoh, already did break pg_dump, and we had to hack things to fix it; see commit 683abc73dff549e94555d4020dae8d02f32ed78b. Well pg_dump was only broken in that there was a new catalog state to deal with. But the commit you linked to was fixing pg_upgrade which was broken because the on-disk schema was then out of sync with what pg_dump would generate. But that should only matter for creating or deleting whole relations. -- greg -- 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 lock strength reduction patch is unsafe
On 2014-03-04 16:37:48 -0500, Tom Lane wrote: However, it seems possible that we could have a mode in which a read-only session did all its catalog fetches according to the transaction snapshot. That would get us to a situation where the backend-internal lookups that ruleutils relies on would give the same answers as queries done by pg_dump. Robert's work on getting rid of SnapshotNow has probably moved that much closer than it was before, but it's still not exactly a trivial patch. The most interesting bit seems to be the cache invalidation handling. It would need to be something like PushCatalogSnapshot() which disables applying invals, including catchup interrupts and all. If the sinval queue hasn't overflown while that snapshot was up, everything is fine we just need to apply all pending invalidations, if it has, we need to do a InvalidateSystemCaches(). Meanwhile, Andres claimed upthread that none of the currently-proposed reduced-lock ALTER commands affect data that pg_dump is using ruleutils to fetch. If that's the case, then maybe this is a problem that we can punt till later. I've not gone through the list to verify it though. I think it's true for all but T_AddConstraint, but I am wary about that one anyway. But somebody else should definitely check the list. I wonder if AddConstraintUsing would possibly be safe... 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 lock strength reduction patch is unsafe Reply-To:
Greg Stark st...@mit.edu writes: On Tue, Mar 4, 2014 at 8:08 PM, Tom Lane t...@sss.pgh.pa.us wrote: CREATE INDEX CONCURRENTLY, otoh, already did break pg_dump, and we had to hack things to fix it; see commit 683abc73dff549e94555d4020dae8d02f32ed78b. Well pg_dump was only broken in that there was a new catalog state to deal with. But the commit you linked to was fixing pg_upgrade which was broken because the on-disk schema was then out of sync with what pg_dump would generate. No, it was fixing cases that would cause problems with or without pg_upgrade. Arguably that patch made it worse for pg_upgrade, which needed a followon patch (203d8ae2d). 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 lock strength reduction patch is unsafe
Andres Freund and...@2ndquadrant.com writes: On 2014-03-04 16:37:48 -0500, Tom Lane wrote: However, it seems possible that we could have a mode in which a read-only session did all its catalog fetches according to the transaction snapshot. That would get us to a situation where the backend-internal lookups that ruleutils relies on would give the same answers as queries done by pg_dump. Robert's work on getting rid of SnapshotNow has probably moved that much closer than it was before, but it's still not exactly a trivial patch. The most interesting bit seems to be the cache invalidation handling. It would need to be something like PushCatalogSnapshot() which disables applying invals, including catchup interrupts and all. If the sinval queue hasn't overflown while that snapshot was up, everything is fine we just need to apply all pending invalidations, if it has, we need to do a InvalidateSystemCaches(). Yeah, at least within the transaction we would simply ignore invals. To avoid causing sinval queue overrun (which would hurt everyone system-wide), my inclination would be to just drop invals on the floor all the time when in this mode, and forcibly do InvalidateSystemCaches at transaction end. For pg_dump, at least, there is no value in working any harder than that, since it's going to quit at transaction end anyhow. 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 lock strength reduction patch is unsafe
On Thu, Feb 27, 2014 at 3:12 AM, Simon Riggs si...@2ndquadrant.com wrote: Removing SELECT privilege while running a SELECT would be a different matter. This is all a matter of definition; we can make up any rules we like. Doing so is IMHO a separate patch and not something to hold up the main patch. So I think this is an interesting point. There are various things that could go wrong as a result of using the wrong lock level. Worst would be that the server crashes or corrupts data. A little less bad would be that sessions error out with inexplicable error conditions, as in SnapshotNow days. Alternatively, we could just have arguably wrong behavior, like basing query results on the old version of the table's metadata even after it's been changed. I don't really care about that second category of behavior. If somebody changes some property of a table and existing sessions continue to use the old value until eoxact, well, we can argue about that, but at least until we have concrete reports of really undesirable behavior, I don't think it's the primary issue. What I'm really concerned about is whether there are other things like the SnapshotNow issues that can cause stuff to halt and catch fire. I don't know whether there are or are not, but that's my concern. -- Robert Haas EnterpriseDB: 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] ALTER TABLE lock strength reduction patch is unsafe
On 3 March 2014 15:19, Robert Haas robertmh...@gmail.com wrote: On Thu, Feb 27, 2014 at 3:12 AM, Simon Riggs si...@2ndquadrant.com wrote: What I'm really concerned about is whether there are other things like the SnapshotNow issues that can cause stuff to halt and catch fire. I don't know whether there are or are not, but that's my concern. Of course its a concern, I feel it also. But that's why we have beta period to handle the unknowns. The question is are there any specific areas of concern here? If not, then we commit because we've done a lot of work on it and at the moment the balance is high benefit to users against a non-specific feeling of risk. @Noah - Last call... -- 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] ALTER TABLE lock strength reduction patch is unsafe
Simon Riggs si...@2ndquadrant.com writes: On 3 March 2014 15:19, Robert Haas robertmh...@gmail.com wrote: What I'm really concerned about is whether there are other things like the SnapshotNow issues that can cause stuff to halt and catch fire. I don't know whether there are or are not, but that's my concern. Of course its a concern, I feel it also. But that's why we have beta period to handle the unknowns. I have exactly zero faith that beta testing would catch low-probability problems in this area. What's needed, and hasn't happened AFAIK, is detailed study of the patch by assorted senior hackers. The question is are there any specific areas of concern here? If not, then we commit because we've done a lot of work on it and at the moment the balance is high benefit to users against a non-specific feeling of risk. This is backwards. The default decision around here has never been to commit when in doubt. 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 lock strength reduction patch is unsafe
On Sun, Mar 2, 2014 at 4:50 AM, Simon Riggs si...@2ndquadrant.com wrote: v20 includes slightly re-ordered checks in GetLockLevel, plus more detailed comments on each group of subcommands. Also corrects grammar as noted by Vik. Plus adds an example of usage to the docs. This patch contains a one line change to src/bin/pg_dump/pg_backup_archiver.c which seems not to belong. This hunk in ATRewriteCatalogs() looks scary: + /* +* If we think we might need to add/re-add toast tables then +* we currently need to hold an AccessExclusiveLock. +*/ + if (lockmode AccessExclusiveLock) + return; It would make sense to me to add an Assert() or elog() check inside the subsequent loop to verify that the lock level is adequate ... but just returning silently seems like a bad idea. I have my doubts about whether it's safe to do AT_AddInherit, AT_DropInherit, AT_AddOf, or AT_DropOf with a full lock. All of those can change the tuple descriptor, and we discussed, back when we did this the first time, the fact that the executor may get *very* unhappy if the tuple descriptor changes in mid-execution. I strongly suspect these are unsafe with less than a full AccessExclusiveLock. -- Robert Haas EnterpriseDB: 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] ALTER TABLE lock strength reduction patch is unsafe
On 3 March 2014 15:53, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: On 3 March 2014 15:19, Robert Haas robertmh...@gmail.com wrote: What I'm really concerned about is whether there are other things like the SnapshotNow issues that can cause stuff to halt and catch fire. I don't know whether there are or are not, but that's my concern. Of course its a concern, I feel it also. But that's why we have beta period to handle the unknowns. I have exactly zero faith that beta testing would catch low-probability problems in this area. What's needed, and hasn't happened AFAIK, is detailed study of the patch by assorted senior hackers. The question is are there any specific areas of concern here? If not, then we commit because we've done a lot of work on it and at the moment the balance is high benefit to users against a non-specific feeling of risk. This is backwards. The default decision around here has never been to commit when in doubt. I'm not in doubt. If anybody can give me some more pointers of things to look at, I will. But I've looked and I can't see anything more. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On 3 March 2014 16:06, Robert Haas robertmh...@gmail.com wrote: On Sun, Mar 2, 2014 at 4:50 AM, Simon Riggs si...@2ndquadrant.com wrote: v20 includes slightly re-ordered checks in GetLockLevel, plus more detailed comments on each group of subcommands. Also corrects grammar as noted by Vik. Plus adds an example of usage to the docs. This patch contains a one line change to src/bin/pg_dump/pg_backup_archiver.c which seems not to belong. This hunk in ATRewriteCatalogs() looks scary: + /* +* If we think we might need to add/re-add toast tables then +* we currently need to hold an AccessExclusiveLock. +*/ + if (lockmode AccessExclusiveLock) + return; It would make sense to me to add an Assert() or elog() check inside the subsequent loop to verify that the lock level is adequate ... but just returning silently seems like a bad idea. OK, I will check elog. I have my doubts about whether it's safe to do AT_AddInherit, AT_DropInherit, AT_AddOf, or AT_DropOf with a full lock. All of those can change the tuple descriptor, and we discussed, back when we did this the first time, the fact that the executor may get *very* unhappy if the tuple descriptor changes in mid-execution. I strongly suspect these are unsafe with less than a full AccessExclusiveLock. I'm happy to change those if you feel there is insufficient evidence. I don't personally feel that it would matter to usability to keep locks for those at AccessExclusiveLock, especially since they are otherwise fast. Some others might be kept higher also. I'm merely trying to balance between requests to reduce to minimal theoretical level and fears that anything less than AccessExclusiveLock is a problem. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On Mon, Mar 3, 2014 at 11:29 AM, Simon Riggs si...@2ndquadrant.com wrote: On 3 March 2014 16:06, Robert Haas robertmh...@gmail.com wrote: On Sun, Mar 2, 2014 at 4:50 AM, Simon Riggs si...@2ndquadrant.com wrote: v20 includes slightly re-ordered checks in GetLockLevel, plus more detailed comments on each group of subcommands. Also corrects grammar as noted by Vik. Plus adds an example of usage to the docs. This patch contains a one line change to src/bin/pg_dump/pg_backup_archiver.c which seems not to belong. This hunk in ATRewriteCatalogs() looks scary: + /* +* If we think we might need to add/re-add toast tables then +* we currently need to hold an AccessExclusiveLock. +*/ + if (lockmode AccessExclusiveLock) + return; It would make sense to me to add an Assert() or elog() check inside the subsequent loop to verify that the lock level is adequate ... but just returning silently seems like a bad idea. OK, I will check elog. I have my doubts about whether it's safe to do AT_AddInherit, AT_DropInherit, AT_AddOf, or AT_DropOf with a full lock. All of those can change the tuple descriptor, and we discussed, back when we did this the first time, the fact that the executor may get *very* unhappy if the tuple descriptor changes in mid-execution. I strongly suspect these are unsafe with less than a full AccessExclusiveLock. I'm happy to change those if you feel there is insufficient evidence. Not sure what that means, but yes, I think the lock levels on those need to be increased. -- Robert Haas EnterpriseDB: 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] ALTER TABLE lock strength reduction patch is unsafe
On Mon, Mar 03, 2014 at 10:19:55AM -0500, Robert Haas wrote: On Thu, Feb 27, 2014 at 3:12 AM, Simon Riggs si...@2ndquadrant.com wrote: Removing SELECT privilege while running a SELECT would be a different matter. This is all a matter of definition; we can make up any rules we like. Doing so is IMHO a separate patch and not something to hold up the main patch. So I think this is an interesting point. There are various things that could go wrong as a result of using the wrong lock level. Worst would be that the server crashes or corrupts data. A little less bad would be that sessions error out with inexplicable error conditions, as in SnapshotNow days. Alternatively, we could just have arguably wrong behavior, like basing query results on the old version of the table's metadata even after it's been changed. I would order the concerns like this: 1. Data corruption 2. Transient, clearly-wrong answers without an error 3. Server crash 4. Catalog logical inconsistency 5. Inexplicable, transient errors 6. Valid behavior capable of surprising more than zero upgraders I don't really care about that second category of behavior. If somebody changes some property of a table and existing sessions continue to use the old value until eoxact, well, we can argue about that, but at least until we have concrete reports of really undesirable behavior, I don't think it's the primary issue. What I'm really concerned about is whether there are other things like the SnapshotNow issues that can cause stuff to halt and catch fire. I don't know whether there are or are not, but that's my concern. Since we can't know whether something qualifies as (2) or (6) without analyzing it, I don't find waiting for user complaints to be a good strategy here. An ownership change not immediately affecting ACL checks does fall under (6), for me. (However, changing ownership without AccessExclusiveLock might also create hazards in category (4) for concurrent DDL that performs owner checks.) -- Noah Misch EnterpriseDB http://www.enterprisedb.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 lock strength reduction patch is unsafe
On Mon, Mar 03, 2014 at 03:43:46PM +, Simon Riggs wrote: The question is are there any specific areas of concern here? If not, then we commit because we've done a lot of work on it and at the moment the balance is high benefit to users against a non-specific feeling of risk. @Noah - Last call... I am not specifically aware of any outstanding problems. I have planned to give this a close look, but it will be at least two weeks before I dig out far enough to do so. If that makes it a post-commit review, so be it. -- Noah Misch EnterpriseDB http://www.enterprisedb.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 lock strength reduction patch is unsafe
On 3 March 2014 18:57, Noah Misch n...@leadboat.com wrote: On Mon, Mar 03, 2014 at 10:19:55AM -0500, Robert Haas wrote: On Thu, Feb 27, 2014 at 3:12 AM, Simon Riggs si...@2ndquadrant.com wrote: Removing SELECT privilege while running a SELECT would be a different matter. This is all a matter of definition; we can make up any rules we like. Doing so is IMHO a separate patch and not something to hold up the main patch. So I think this is an interesting point. There are various things that could go wrong as a result of using the wrong lock level. Worst would be that the server crashes or corrupts data. A little less bad would be that sessions error out with inexplicable error conditions, as in SnapshotNow days. Alternatively, we could just have arguably wrong behavior, like basing query results on the old version of the table's metadata even after it's been changed. I would order the concerns like this: 1. Data corruption 2. Transient, clearly-wrong answers without an error 3. Server crash 4. Catalog logical inconsistency 5. Inexplicable, transient errors 6. Valid behavior capable of surprising more than zero upgraders I like your model for risk assessment. How can we apply it in detail in a way that helps us decide? Or do we just go on gut feel? My experience with mentioning such topics is that without structure it results in an assessment of unacceptable risk just simply because somebody has mentioned some scary words. I don't really care about that second category of behavior. If somebody changes some property of a table and existing sessions continue to use the old value until eoxact, well, we can argue about that, but at least until we have concrete reports of really undesirable behavior, I don't think it's the primary issue. What I'm really concerned about is whether there are other things like the SnapshotNow issues that can cause stuff to halt and catch fire. I don't know whether there are or are not, but that's my concern. Since we can't know whether something qualifies as (2) or (6) without analyzing it, I don't find waiting for user complaints to be a good strategy here. An ownership change not immediately affecting ACL checks does fall under (6), for me. (However, changing ownership without AccessExclusiveLock might also create hazards in category (4) for concurrent DDL that performs owner checks.) err, guys, you do realise that changing ownership is staying at AccessExclusiveLock in this patch? (and I haven't ever suggested lowering that). -- 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] ALTER TABLE lock strength reduction patch is unsafe
On Mon, Mar 03, 2014 at 07:19:45PM +, Simon Riggs wrote: On 3 March 2014 18:57, Noah Misch n...@leadboat.com wrote: On Mon, Mar 03, 2014 at 10:19:55AM -0500, Robert Haas wrote: On Thu, Feb 27, 2014 at 3:12 AM, Simon Riggs si...@2ndquadrant.com wrote: Removing SELECT privilege while running a SELECT would be a different matter. This is all a matter of definition; we can make up any rules we like. Doing so is IMHO a separate patch and not something to hold up the main patch. So I think this is an interesting point. There are various things that could go wrong as a result of using the wrong lock level. Worst would be that the server crashes or corrupts data. A little less bad would be that sessions error out with inexplicable error conditions, as in SnapshotNow days. Alternatively, we could just have arguably wrong behavior, like basing query results on the old version of the table's metadata even after it's been changed. I would order the concerns like this: 1. Data corruption 2. Transient, clearly-wrong answers without an error 3. Server crash 4. Catalog logical inconsistency 5. Inexplicable, transient errors 6. Valid behavior capable of surprising more than zero upgraders I like your model for risk assessment. How can we apply it in detail in a way that helps us decide? Or do we just go on gut feel? My experience with mentioning such topics is that without structure it results in an assessment of unacceptable risk just simply because somebody has mentioned some scary words. True; it's tough to make use of such a prioritization. By the time you can confidently assign something to a category, you can probably just fix it. (Or, in the case of category (6), document/release-note it.) Just to be clear, that list is not a commentary on the particular patch at hand. Those are merely the kinds of regressions to look for in a patch affecting this area of the code. err, guys, you do realise that changing ownership is staying at AccessExclusiveLock in this patch? (and I haven't ever suggested lowering that). Yep. -- Noah Misch EnterpriseDB http://www.enterprisedb.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 lock strength reduction patch is unsafe
Noah Misch n...@leadboat.com writes: Just to be clear, that list is not a commentary on the particular patch at hand. Those are merely the kinds of regressions to look for in a patch affecting this area of the code. A complaint on pgsql-bugs just now reminded me of a specific area that needs to be looked at hard: how bad are the implications for pg_dump? Up to now, pg_dump could be reasonably confident that once it had AccessShareLock on every table it intended to dump, there would be no schema changes happening on those tables until it got done. This greatly ameliorates the snapshot-skew problems that arise from its habit of doing some things for itself and other things via backend-internal functions (which historically used SnapshotNow and now use a fresh MVCC snapshot, either way potentially quite newer than the transaction snapshot pg_dump's own queries will use). I suspect that lowering the lock requirements for anything that affects what pg_dump can see is going to make things a whole lot worse in terms of consistency of pg_dump output in the face of concurrent DDL. Admittedly, we're not perfect in that area now, but I'm not sure that's an adequate excuse for making it worse. 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 lock strength reduction patch is unsafe
On 2014-03-03 19:15:27 -0500, Tom Lane wrote: Noah Misch n...@leadboat.com writes: Just to be clear, that list is not a commentary on the particular patch at hand. Those are merely the kinds of regressions to look for in a patch affecting this area of the code. A complaint on pgsql-bugs just now reminded me of a specific area that needs to be looked at hard: how bad are the implications for pg_dump? Up to now, pg_dump could be reasonably confident that once it had AccessShareLock on every table it intended to dump, there would be no schema changes happening on those tables until it got done. The guarantee wasn't actually that strong. It already was quite possible that indexes got created/dropped during that time, which probably is the by far most frequent DDL run in production. This greatly ameliorates the snapshot-skew problems that arise from its habit of doing some things for itself and other things via backend-internal functions (which historically used SnapshotNow and now use a fresh MVCC snapshot, either way potentially quite newer than the transaction snapshot pg_dump's own queries will use). Yea, I wonder if we shouldn't start to make them use a different snapshot. It's the pg_get_*def() functions, or is there something else? Afair (I really haven't rechecked) all the actions that have a changed locklevels affect things that pg_dump recreates clientside, using a repeatable read snapshot, so there shouldn't be much change 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] ALTER TABLE lock strength reduction patch is unsafe
Andres Freund and...@2ndquadrant.com writes: On 2014-03-03 19:15:27 -0500, Tom Lane wrote: This greatly ameliorates the snapshot-skew problems that arise from its habit of doing some things for itself and other things via backend-internal functions (which historically used SnapshotNow and now use a fresh MVCC snapshot, either way potentially quite newer than the transaction snapshot pg_dump's own queries will use). Yea, I wonder if we shouldn't start to make them use a different snapshot. It's the pg_get_*def() functions, or is there something else? See past discussions. By the time you trace all of ruleutils.c's dependencies, a dauntingly large fraction of the backend's basic catalog access support is implicated. For example, you'd need some way of making the catcaches return data that they know to be outdated. And I'm pretty sure pg_dump is making free use of stuff that isn't even in ruleutils. I would like to see pg_dump doing something much more bulletproof, but I'm afraid that there isn't any nice simple fix available. One relatively narrow fix that would probably make it a lot better *in the current state of affairs* is to provide a way whereby, once pg_dump has locks on everything it wants to dump, it can advance its transaction snapshot to current time. Then at least both its own queries and the backend's lookups will see post-locking state. However, if AccessShareLock isn't enough to block DDL on the tables then we're still hosed. Afair (I really haven't rechecked) all the actions that have a changed locklevels affect things that pg_dump recreates clientside, using a repeatable read snapshot, so there shouldn't be much change there? You're missing the point entirely if you think pg_dump recreates everything client-side. It's never done that 100%, and we've migrated more and more stuff to the server side over time to avoid duplicate implementations of things like index expression decompilation. So while a theoretical answer would be to remove all of pg_dump's dependency on server-side functions, I am pretty sure that's never going to happen. 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 lock strength reduction patch is unsafe
On 2014-03-03 20:32:13 -0500, Tom Lane wrote: Afair (I really haven't rechecked) all the actions that have a changed locklevels affect things that pg_dump recreates clientside, using a repeatable read snapshot, so there shouldn't be much change there? You're missing the point entirely if you think pg_dump recreates everything client-side. No, I am not obviously not thinking that. What I mean is that the things that actually change their locking requirement in the proposed patch primarily influence things that are reconstructed clientside by pg_dump. E.g ALTER TABLE ... CLUSTER ON, SET(...), ... 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 lock strength reduction patch is unsafe
Andres Freund and...@2ndquadrant.com writes: On 2014-03-03 20:32:13 -0500, Tom Lane wrote: You're missing the point entirely if you think pg_dump recreates everything client-side. No, I am not obviously not thinking that. What I mean is that the things that actually change their locking requirement in the proposed patch primarily influence things that are reconstructed clientside by pg_dump. E.g ALTER TABLE ... CLUSTER ON, SET(...), ... [ raised eyebrow... ] I'm pretty sure that no such constraint was part of the design discussion to start with. Even if it accidentally happens to be the case now, it sounds utterly fragile. 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 lock strength reduction patch is unsafe
On 03/01/2014 12:06 PM, Simon Riggs wrote: On 27 February 2014 08:48, Simon Riggs si...@2ndquadrant.com wrote: On 26 February 2014 15:25, Andres Freund and...@2ndquadrant.com wrote: On 2014-02-26 15:15:00 +, Simon Riggs wrote: On 26 February 2014 13:38, Andres Freund and...@2ndquadrant.com wrote: Hi, On 2014-02-26 07:32:45 +, Simon Riggs wrote: * This definitely should include isolationtester tests actually performing concurrent ALTER TABLEs. All that's currently there is tests that the locklevel isn't too high, but not that it actually works. There is no concurrent behaviour here, hence no code that would be exercised by concurrent tests. Huh? There's most definitely new concurrent behaviour. Previously no other backends could have a relation open (and locked) while it got altered (which then sends out relcache invalidations). That's something that should be tested. It has been. High volume concurrent testing has been performed, per Tom's original discussion upthread, but that's not part of the test suite. Yea, that's not what I am looking for. For other tests I have no guide as to how to write a set of automated regression tests. Anything could cause a failure, so I'd need to write an infinite set of tests to prove there is no bug *somewhere*. How many tests are required? 0, 1, 3, 30? I think some isolationtester tests for the most important changes in lock levels are appropriate. Say, create a PRIMARY KEY, DROP INHERIT, ... while a query is in progress in a nother session. OK, I'll work on some tests. v18 attached, with v19 coming soon v19 complete apart from requested comment additions I've started to look at this patch and re-read the thread. The first thing I noticed is what seems like an automated replace error. The docs say This form requires only an SHARE x EXCLUSIVE lock. where the an was not changed to a. Attached is a patch-on-patch to fix this. A more complete review will come later. -- Vik *** a/doc/src/sgml/ref/alter_table.sgml --- b/doc/src/sgml/ref/alter_table.sgml *** *** 157,163 ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable table to change. /para para ! This form requires only an literalSHARE ROW EXCLUSIVE/literal lock. /para /listitem /varlistentry --- 157,163 table to change. /para para ! This form requires only a literalSHARE ROW EXCLUSIVE/literal lock. /para /listitem /varlistentry *** *** 188,194 ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable xref linkend=planner-stats. /para para ! This form requires only an literalSHARE UPDATE EXCLUSIVE/literal lock. /para /listitem /varlistentry --- 188,194 xref linkend=planner-stats. /para para ! This form requires only a literalSHARE UPDATE EXCLUSIVE/literal lock. /para /listitem /varlistentry *** *** 223,229 ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable planner, refer to xref linkend=planner-stats. /para para ! This form requires only an literalSHARE UPDATE EXCLUSIVE/literal lock. /para /listitem /varlistentry --- 223,229 planner, refer to xref linkend=planner-stats. /para para ! This form requires only a literalSHARE UPDATE EXCLUSIVE/literal lock. /para /listitem /varlistentry *** *** 344,350 ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable created. Currently only foreign key constraints may be altered. /para para ! This form requires only an literalSHARE ROW EXCLUSIVE/literal lock. /para /listitem /varlistentry --- 344,350 created. Currently only foreign key constraints may be altered. /para para ! This form requires only a literalSHARE ROW EXCLUSIVE/literal lock. /para /listitem /varlistentry *** *** 366,372 ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable does not prevent normal write commands against the table while it runs. /para para ! This form requires only an literalSHARE UPDATE EXCLUSIVE/literal lock on the table being altered. If the constraint is a foreign key then a literalROW SHARE/literal lock is also required on the table referenced by the constraint. --- 366,372 does not prevent normal write commands against the table while it runs. /para para ! This form requires only a literalSHARE UPDATE EXCLUSIVE/literal lock on the table being altered. If the constraint is a foreign key then a literalROW SHARE/literal lock is also required on the table referenced by the constraint. *** ***
Re: [HACKERS] ALTER TABLE lock strength reduction patch is unsafe
On 26 February 2014 07:32, Simon Riggs si...@2ndquadrant.com wrote: * Are you sure AlterConstraint is generally safe without an AEL? It should be safe to change whether something is deferred, but not necessarily whether it's deferrable? We change the lock levels for individual commands. This patch provides some initial settings and infrastructure. It is possible you are correct that changing the deferrability is not safe without an AEL. That was enabled for the first time in this release in a patch added by me after this patch was written. I will think on that and change, if required. Thoughts... It would be a problem to change a DEFERRABLE constraint into a NOT DEFERRABLE constraint concurrently with a transaction that was currently deferring its constraint checks. It would not be a problem to go in the other direction, relaxing the constraint attributes. The patch uses ShareRowExclusive for AlterConstraint, so no writes are possible concurrently with the table being ALTERed, hence the problem situation cannot arise. So in my understanding, no issue is possible. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On 26 February 2014 15:25, Andres Freund and...@2ndquadrant.com wrote: * Why does ChangeOwner need AEL? Ownership affects privileges, which includes SELECTs, hence AEL. So? That reply could be added to any post. Please explain your concern. I don't understand why that means it needs an AEL. After all, e.g. changes in table inheritance do *not* require an AEL. I think it's perfectly ok to not go for the minimally required locklevel for all subcommands, but then it should be commented as such and not with change visible to SELECT where other operations that do so as well require lower locklevels. Those are two separate cases, with separate lock levels, so that argument doesn't hold. My understanding of the argument as to why Inheritance doesn't need AEL is that adding/removing children is akin to inserting or deleting rows from the parent. Removing SELECT privilege while running a SELECT would be a different matter. This is all a matter of definition; we can make up any rules we like. Doing so is IMHO a separate patch and not something to hold up the main patch. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On 26 February 2014 15:25, Andres Freund and...@2ndquadrant.com wrote: On 2014-02-26 15:15:00 +, Simon Riggs wrote: On 26 February 2014 13:38, Andres Freund and...@2ndquadrant.com wrote: Hi, On 2014-02-26 07:32:45 +, Simon Riggs wrote: * This definitely should include isolationtester tests actually performing concurrent ALTER TABLEs. All that's currently there is tests that the locklevel isn't too high, but not that it actually works. There is no concurrent behaviour here, hence no code that would be exercised by concurrent tests. Huh? There's most definitely new concurrent behaviour. Previously no other backends could have a relation open (and locked) while it got altered (which then sends out relcache invalidations). That's something that should be tested. It has been. High volume concurrent testing has been performed, per Tom's original discussion upthread, but that's not part of the test suite. Yea, that's not what I am looking for. For other tests I have no guide as to how to write a set of automated regression tests. Anything could cause a failure, so I'd need to write an infinite set of tests to prove there is no bug *somewhere*. How many tests are required? 0, 1, 3, 30? I think some isolationtester tests for the most important changes in lock levels are appropriate. Say, create a PRIMARY KEY, DROP INHERIT, ... while a query is in progress in a nother session. OK, I'll work on some tests. v18 attached, with v19 coming soon -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services reduce_lock_levels.v18.patch 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] ALTER TABLE lock strength reduction patch is unsafe
Hi, On 2014-02-26 07:32:45 +, Simon Riggs wrote: * This definitely should include isolationtester tests actually performing concurrent ALTER TABLEs. All that's currently there is tests that the locklevel isn't too high, but not that it actually works. There is no concurrent behaviour here, hence no code that would be exercised by concurrent tests. Huh? There's most definitely new concurrent behaviour. Previously no other backends could have a relation open (and locked) while it got altered (which then sends out relcache invalidations). That's something that should be tested. * Why does ChangeOwner need AEL? Ownership affects privileges, which includes SELECTs, hence AEL. So? 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 lock strength reduction patch is unsafe
On 26 February 2014 13:38, Andres Freund and...@2ndquadrant.com wrote: Hi, On 2014-02-26 07:32:45 +, Simon Riggs wrote: * This definitely should include isolationtester tests actually performing concurrent ALTER TABLEs. All that's currently there is tests that the locklevel isn't too high, but not that it actually works. There is no concurrent behaviour here, hence no code that would be exercised by concurrent tests. Huh? There's most definitely new concurrent behaviour. Previously no other backends could have a relation open (and locked) while it got altered (which then sends out relcache invalidations). That's something that should be tested. It has been. High volume concurrent testing has been performed, per Tom's original discussion upthread, but that's not part of the test suite. For other tests I have no guide as to how to write a set of automated regression tests. Anything could cause a failure, so I'd need to write an infinite set of tests to prove there is no bug *somewhere*. How many tests are required? 0, 1, 3, 30? * Why does ChangeOwner need AEL? Ownership affects privileges, which includes SELECTs, hence AEL. So? That reply could be added to any post. Please explain your concern. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On 2014-02-26 15:15:00 +, Simon Riggs wrote: On 26 February 2014 13:38, Andres Freund and...@2ndquadrant.com wrote: Hi, On 2014-02-26 07:32:45 +, Simon Riggs wrote: * This definitely should include isolationtester tests actually performing concurrent ALTER TABLEs. All that's currently there is tests that the locklevel isn't too high, but not that it actually works. There is no concurrent behaviour here, hence no code that would be exercised by concurrent tests. Huh? There's most definitely new concurrent behaviour. Previously no other backends could have a relation open (and locked) while it got altered (which then sends out relcache invalidations). That's something that should be tested. It has been. High volume concurrent testing has been performed, per Tom's original discussion upthread, but that's not part of the test suite. Yea, that's not what I am looking for. For other tests I have no guide as to how to write a set of automated regression tests. Anything could cause a failure, so I'd need to write an infinite set of tests to prove there is no bug *somewhere*. How many tests are required? 0, 1, 3, 30? I think some isolationtester tests for the most important changes in lock levels are appropriate. Say, create a PRIMARY KEY, DROP INHERIT, ... while a query is in progress in a nother session. * Why does ChangeOwner need AEL? Ownership affects privileges, which includes SELECTs, hence AEL. So? That reply could be added to any post. Please explain your concern. I don't understand why that means it needs an AEL. After all, e.g. changes in table inheritance do *not* require an AEL. I think it's perfectly ok to not go for the minimally required locklevel for all subcommands, but then it should be commented as such and not with change visible to SELECT where other operations that do so as well require lower locklevels. 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 lock strength reduction patch is unsafe
On 24 February 2014 16:01, Andres Freund and...@2ndquadrant.com wrote: Hi, I took a quick peek at this, and noticed the following things: * I am pretty sure this patch doesn't compile anymore after the latest set of releases. Refreshed to v18, will post shortly. * This definitely should include isolationtester tests actually performing concurrent ALTER TABLEs. All that's currently there is tests that the locklevel isn't too high, but not that it actually works. There is no concurrent behaviour here, hence no code that would be exercised by concurrent tests. Lock levels are proven in regression tests, so no further tests needed. * So far no DDL uses ShareRowExclusiveLocks, why do so now? Not sure if there aren't relevant cases for with foreign key checks (which afair *do* acquire SRE locks). That was discussed long ago. We can relax it more in the future, if that is considered safe. * Why is AddConstraint so complicated when it's all used SRE locks? To ensure each case was considered, rather than just assume all cases are the same. * Are you sure AlterConstraint is generally safe without an AEL? It should be safe to change whether something is deferred, but not necessarily whether it's deferrable? We change the lock levels for individual commands. This patch provides some initial settings and infrastructure. It is possible you are correct that changing the deferrability is not safe without an AEL. That was enabled for the first time in this release in a patch added by me after this patch was written. I will think on that and change, if required. * Why does ChangeOwner need AEL? Ownership affects privileges, which includes SELECTs, hence AEL. This is not a critical aspect of the patch. * You changed several places to take out lower locks, which in itself is fine, but doesn't that lead to lock upgrade hazard if a later step acquires a stronger lock? Or do we take out a strong enough lock from the beginning. We asess the lock needed at parse time, then use it consistently later. There is no lock upgrade hazard since that has been specifically considered and removed. * There's no explanation why the EOXact TupleDesc stuff is needed? I will update comments. -- 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] ALTER TABLE lock strength reduction patch is unsafe
Hi, I took a quick peek at this, and noticed the following things: * I am pretty sure this patch doesn't compile anymore after the latest set of releases. * This definitely should include isolationtester tests actually performing concurrent ALTER TABLEs. All that's currently there is tests that the locklevel isn't too high, but not that it actually works. * So far no DDL uses ShareRowExclusiveLocks, why do so now? Not sure if there aren't relevant cases for with foreign key checks (which afair *do* acquire SRE locks). * Why is AddConstraint so complicated when it's all used SRE locks? * Are you sure AlterConstraint is generally safe without an AEL? It should be safe to change whether something is deferred, but not necessarily whether it's deferrable? * Why does ChangeOwner need AEL? * You changed several places to take out lower locks, which in itself is fine, but doesn't that lead to lock upgrade hazard if a later step acquires a stronger lock? Or do we take out a strong enough lock from the beginning. * There's no explanation why the EOXact TupleDesc stuff is needed? That's it for now, 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 lock strength reduction patch is unsafe
On 29 January 2014 05:43, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Jan 28, 2014 at 12:36 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Honestly, I would prefer that we push a patch that has been thoroughly reviewed and in which we have more confidence, so that we can push without a GUC. This means mark in CF as needs-review, then some other developer reviews it and marks it as ready-for-committer. I also believe that would be the correct procedure. Personally, I think it would be great if Noah can review this, as he's very good at finding the kind of problems that are likely to crop up here, and has examined previous versions. I also have some interest in looking at it myself. But I doubt that either of us (or any other senior hacker) can do that by Thursday. I think all such people are hip-deep in other patches at the moment; I certainly am. Yeah. I actually have little doubt that the patch is sane on its own terms of discussion, that is that Simon has chosen locking levels that are mutually consistent in an abstract sense. What sank the previous iteration was that he'd failed to consider an implementation detail, namely possible inconsistencies in SnapshotNow-based catalog fetches. I'm afraid that there may be more such problems lurking under the surface. Agreed Noah's pretty good at finding such things, but really two or three of us need to sit down and think about it for awhile before I'd have much confidence in such a fundamental change. And I sure don't have time for this patch right now myself. I've reviewed Noah's earlier comments, fixed things and also further reviewed for any similar relcache related issues. I've also reviewed Hot Standby to see if any locking issues arise, since the ALTER TABLE now won't generate an AccessExclusiveLock as it used to do for certain operations. I can't see any problems there. While doing those reviews, I'd remind everybody that this only affects roughly half of ALTER TABLE subcommands and definitely nothing that affects SELECTs. So the risk profile is much less than it sounds at first glance. If anybody else has any hints or clues about where to look, please mention them and I will investigate. Thanks. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On Mon, Jan 27, 2014 at 08:57:26PM +, Simon Riggs wrote: We get the new behaviour by default and I expect we'll be very happy with it. A second thought is that if we have problems of some kind in the field as a result of the new lock modes then we will be able to turn them off. I'm happy to fix any problems that occur, but that doesn't mean there won't be any. If everybody is confident that we've foreseen every bug, then no problem, lets remove it. I recall being asked to add hot_standby = on | off for similar reasons. Addressing a larger issue, I have no problem with systematically adding GUCs to turn off features we add in each major release if we can also systematically remove those GUCs in the next major release. This would require putting all these settings in the compatibility section of postgresql.conf. However, I don't think it makes sense to do this in a one-off manner. It is also possible that there are enough cases where we _can't_ turn the feature off with a GUC that this would be unworkable. So, if we can't do it systematically, that means we will have enough breakage cases that we just need to rush out new versions to fix major breakage and one-off GUCs just don't buy us much, and add confusion. Does that make sense? -- 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] ALTER TABLE lock strength reduction patch is unsafe
On 28 January 2014 14:55, Bruce Momjian br...@momjian.us wrote: On Mon, Jan 27, 2014 at 08:57:26PM +, Simon Riggs wrote: We get the new behaviour by default and I expect we'll be very happy with it. A second thought is that if we have problems of some kind in the field as a result of the new lock modes then we will be able to turn them off. I'm happy to fix any problems that occur, but that doesn't mean there won't be any. If everybody is confident that we've foreseen every bug, then no problem, lets remove it. I recall being asked to add hot_standby = on | off for similar reasons. Addressing a larger issue, I have no problem with systematically adding GUCs to turn off features we add in each major release if we can also systematically remove those GUCs in the next major release. Agreed. I propose 2 releases since the time between release of 9.4 and the feature freeze of 9.5 is only 4 months, not usually enough for subtle bugs to be discovered. This would require putting all these settings in the compatibility section of postgresql.conf. Agreed, that is where I have added the parameter. However, I don't think it makes sense to do this in a one-off manner. It is also possible that there are enough cases where we _can't_ turn the feature off with a GUC that this would be unworkable. So, if we can't do it systematically, that means we will have enough breakage cases that we just need to rush out new versions to fix major breakage and one-off GUCs just don't buy us much, and add confusion. Does that make sense? For me, reducing the strength of DDL locking is a major change in RDBMS behaviour that could both delight and surprise our users. Maybe a few actually depend upon the locking behaviour, maybe. After some years of various people looking at this, I think we've got it right. Experience tells me that while I think this is the outcome, we are well advised to protect against the possibility that it is not correct and that if we have corner case issues, it would be good to easily disable this in the field. In the current case, a simple parameter works very well to disable the feature; in other cases, not. Summary: This is an atypical case. I do not normally propose such things - this is the third time in 10 years, IIRC. I have no problem removing the parameter if required to. In that case, I would like to leave the parameter in until mid beta, to allow greater certainty. In any case, I would wish to retain as a minimum an extern bool variable allowing it to be turned off by C function if desired. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On Tue, Jan 28, 2014 at 04:36:39PM +, Simon Riggs wrote: For me, reducing the strength of DDL locking is a major change in RDBMS behaviour that could both delight and surprise our users. Maybe a few actually depend upon the locking behaviour, maybe. After some years of various people looking at this, I think we've got it right. Experience tells me that while I think this is the outcome, we are well advised to protect against the possibility that it is not correct and that if we have corner case issues, it would be good to easily disable this in the field. In the current case, a simple parameter works very well to disable the feature; in other cases, not. Summary: This is an atypical case. I do not normally propose such things - this is the third time in 10 years, IIRC. Uh, in my memory, you are the person who is most likely to suggest a GUC of all our developers. I have no problem removing the parameter if required to. In that case, I would like to leave the parameter in until mid beta, to allow greater certainty. In any case, I would wish to retain as a minimum an extern bool variable allowing it to be turned off by C function if desired. Anything changed to postgresql.conf during beta is going to require an initdb. Also, lots of backward-compatibility infrastructure, as you are suggesting above, add complexity to the system. I am basically against a GUC on this. We have far larger problems with 9.3 than backward compatibility, and limited resources. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On 01/28/2014 07:15 PM, Bruce Momjian wrote: On Tue, Jan 28, 2014 at 04:36:39PM +, Simon Riggs wrote: For me, reducing the strength of DDL locking is a major change in RDBMS behaviour that could both delight and surprise our users. Maybe a few actually depend upon the locking behaviour, maybe. After some years of various people looking at this, I think we've got it right. Experience tells me that while I think this is the outcome, we are well advised to protect against the possibility that it is not correct and that if we have corner case issues, it would be good to easily disable this in the field. In the current case, a simple parameter works very well to disable the feature; in other cases, not. I don't understand why anyone would want to turn this feature off, ie. require stronger locks than necessary for a DDL change. If we're not confident that the patch is correct, then it should not be applied. If we are confident enough to commit it, and a bug crops up later, we'll fix the bug as usual. A user savvy enough to fiddle with such a GUC can also do their DDL with: BEGIN; LOCK TABLE table DDL COMMIT; I have no problem removing the parameter if required to. In that case, I would like to leave the parameter in until mid beta, to allow greater certainty. In any case, I would wish to retain as a minimum an extern bool variable allowing it to be turned off by C function if desired. Anything changed to postgresql.conf during beta is going to require an initdb. Huh? Surely not. - Heikki -- 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 lock strength reduction patch is unsafe
On Tue, Jan 28, 2014 at 07:21:50PM +0200, Heikki Linnakangas wrote: I have no problem removing the parameter if required to. In that case, I would like to leave the parameter in until mid beta, to allow greater certainty. In any case, I would wish to retain as a minimum an extern bool variable allowing it to be turned off by C function if desired. Anything changed to postgresql.conf during beta is going to require an initdb. Huh? Surely not. Uh, if we ship beta1 with a GUC in postgresql.conf, and then we remove support for the GUC in beta2, anyone starting a server initdb-ed with beta1 is going to get an error and the server is not going to start: LOG: unrecognized configuration parameter xxx in file /u/pgsql/data/postgresql.conf line 1 FATAL: configuration file /u/pgsql/data/postgresql.conf contains errors so, yeah, it isn't going to require an initdb, but it is going to require everyone to edit their postgresql.conf. My guess is a lot of people are going to assume the old postgresql.conf is not compatible and are going to initdb and reload. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On 01/28/2014 07:26 PM, Bruce Momjian wrote: On Tue, Jan 28, 2014 at 07:21:50PM +0200, Heikki Linnakangas wrote: I have no problem removing the parameter if required to. In that case, I would like to leave the parameter in until mid beta, to allow greater certainty. In any case, I would wish to retain as a minimum an extern bool variable allowing it to be turned off by C function if desired. Anything changed to postgresql.conf during beta is going to require an initdb. Huh? Surely not. Uh, if we ship beta1 with a GUC in postgresql.conf, and then we remove support for the GUC in beta2, anyone starting a server initdb-ed with beta1 is going to get an error and the server is not going to start: LOG: unrecognized configuration parameter xxx in file /u/pgsql/data/postgresql.conf line 1 FATAL: configuration file /u/pgsql/data/postgresql.conf contains errors so, yeah, it isn't going to require an initdb, but it is going to require everyone to edit their postgresql.conf. Only if you uncommented the value in the first place. - Heikki -- 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 lock strength reduction patch is unsafe
On Tue, Jan 28, 2014 at 07:30:47PM +0200, Heikki Linnakangas wrote: On 01/28/2014 07:26 PM, Bruce Momjian wrote: On Tue, Jan 28, 2014 at 07:21:50PM +0200, Heikki Linnakangas wrote: I have no problem removing the parameter if required to. In that case, I would like to leave the parameter in until mid beta, to allow greater certainty. In any case, I would wish to retain as a minimum an extern bool variable allowing it to be turned off by C function if desired. Anything changed to postgresql.conf during beta is going to require an initdb. Huh? Surely not. Uh, if we ship beta1 with a GUC in postgresql.conf, and then we remove support for the GUC in beta2, anyone starting a server initdb-ed with beta1 is going to get an error and the server is not going to start: LOG: unrecognized configuration parameter xxx in file /u/pgsql/data/postgresql.conf line 1 FATAL: configuration file /u/pgsql/data/postgresql.conf contains errors so, yeah, it isn't going to require an initdb, but it is going to require everyone to edit their postgresql.conf. Only if you uncommented the value in the first place. Oh, I had forgotten that. Right. It would still be odd to have a commented-out line in postgresql.conf that was not support. -- 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] ALTER TABLE lock strength reduction patch is unsafe
Alvaro Herrera alvhe...@2ndquadrant.com writes: Honestly, I would prefer that we push a patch that has been thoroughly reviewed and in which we have more confidence, so that we can push without a GUC. This means mark in CF as needs-review, then some other developer reviews it and marks it as ready-for-committer. FWIW, that was the point I was trying to make as well ;-) 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 lock strength reduction patch is unsafe
Bruce Momjian escribió: I have no problem removing the parameter if required to. In that case, I would like to leave the parameter in until mid beta, to allow greater certainty. Uhm. If we remove a GUC during beta we don't need to force an initdb. At worst we will need to keep a no-op GUC variable that is removed in the next devel cycle. That said, if we're going to have a GUC that's going to disappear later, I think it's better to wait for 2 releases as proposed, not remove mid-beta. In any case, I would wish to retain as a minimum an extern bool variable allowing it to be turned off by C function if desired. I think this amounts to having an undocumented GUC that is hard to change. I prefer the GUC, myself. Anything changed to postgresql.conf during beta is going to require an initdb. Also, lots of backward-compatibility infrastructure, as you are suggesting above, add complexity to the system. I am basically against a GUC on this. We have far larger problems with 9.3 than backward compatibility, and limited resources. If we have a clear plan on removing the parameter, it's easy enough to follow through. I don't think lack of resources is a good argument, because at that point there will be little to discuss and it's an easy change to make. And I think the plan is clear: if no bug is found the parameter is removed. If a bug is found, it is fixed and the parameter is removed anyway. Honestly, I would prefer that we push a patch that has been thoroughly reviewed and in which we have more confidence, so that we can push without a GUC. This means mark in CF as needs-review, then some other developer reviews it and marks it as ready-for-committer. -- Álvaro Herrerahttp://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 lock strength reduction patch is unsafe
On 28 January 2014 17:15, Bruce Momjian br...@momjian.us wrote: On Tue, Jan 28, 2014 at 04:36:39PM +, Simon Riggs wrote: For me, reducing the strength of DDL locking is a major change in RDBMS behaviour that could both delight and surprise our users. Maybe a few actually depend upon the locking behaviour, maybe. After some years of various people looking at this, I think we've got it right. Experience tells me that while I think this is the outcome, we are well advised to protect against the possibility that it is not correct and that if we have corner case issues, it would be good to easily disable this in the field. In the current case, a simple parameter works very well to disable the feature; in other cases, not. Summary: This is an atypical case. I do not normally propose such things - this is the third time in 10 years, IIRC. Uh, in my memory, you are the person who is most likely to suggest a GUC of all our developers. (smiles) I have suggested parameters for many features, mostly in the early days of my developments before I saw the light if autotuning. But those were control parameters for tuning features. So yes, I have probably introduced more parameters than most, amongst the many things I've done. I'm guessing you don't recall how much trouble I went to in order to allow sync rep to have only 1 parameter, c'est la vie. What I'm discussing here is a compatibility parameter to allow new features to be disabled. This would be the third time in 10 years I suggested a parameter for that reason, i.e. one that the user would hardly ever wish to set. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On 2014-01-27 15:25:22 -0500, Robert Haas wrote: On Mon, Jan 27, 2014 at 12:58 PM, Simon Riggs si...@2ndquadrant.com wrote: This version adds a GUC called ddl_exclusive_locks which allows us to keep the 9.3 behaviour if we wish it. Some people may be surprised that their programs don't wait in the same places they used to. We hope that is a positive and useful behaviour, but it may not always be so. I'll commit this on Thurs 30 Jan unless I hear objections. I haven't reviewed the patch, but -1 for adding a GUC. Dito. I don't think the patch in a bad shape otherwise. I'd very quickly looked at a previous version and it did look rather sane, and several other people had looked at earlier versions as well. IIRC Noah had a fairly extensive look at some intricate race conditions... 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 lock strength reduction patch is unsafe
On 28 January 2014 17:21, Heikki Linnakangas hlinnakan...@vmware.com wrote: I don't understand why anyone would want to turn this feature off, ie. require stronger locks than necessary for a DDL change. Nobody would *want* to turn it off. They might need to, as explained. If we're not confident that the patch is correct, then it should not be applied. If we are confident enough to commit it, and a bug crops up later, we'll fix the bug as usual. I'd like to point out here that my own customers are already well covered by the support services we offer. They will receive any such fix very quickly. My proposal was of assistance only to those without such contracts in place, as are many of my proposals. It doesn't bother me at all if you insist it should not be added. Just choose v16 of the patch for review rather than v17. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On Tue, Jan 28, 2014 at 12:36 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Bruce Momjian escribió: I have no problem removing the parameter if required to. In that case, I would like to leave the parameter in until mid beta, to allow greater certainty. Uhm. If we remove a GUC during beta we don't need to force an initdb. At worst we will need to keep a no-op GUC variable that is removed in the next devel cycle. That said, if we're going to have a GUC that's going to disappear later, I think it's better to wait for 2 releases as proposed, not remove mid-beta. In any case, I would wish to retain as a minimum an extern bool variable allowing it to be turned off by C function if desired. I think this amounts to having an undocumented GUC that is hard to change. I prefer the GUC, myself. Anything changed to postgresql.conf during beta is going to require an initdb. Also, lots of backward-compatibility infrastructure, as you are suggesting above, add complexity to the system. I am basically against a GUC on this. We have far larger problems with 9.3 than backward compatibility, and limited resources. If we have a clear plan on removing the parameter, it's easy enough to follow through. I don't think lack of resources is a good argument, because at that point there will be little to discuss and it's an easy change to make. And I think the plan is clear: if no bug is found the parameter is removed. If a bug is found, it is fixed and the parameter is removed anyway. Honestly, I would prefer that we push a patch that has been thoroughly reviewed and in which we have more confidence, so that we can push without a GUC. This means mark in CF as needs-review, then some other developer reviews it and marks it as ready-for-committer. I also believe that would be the correct procedure. Personally, I think it would be great if Noah can review this, as he's very good at finding the kind of problems that are likely to crop up here, and has examined previous versions. I also have some interest in looking at it myself. But I doubt that either of us (or any other senior hacker) can do that by Thursday. I think all such people are hip-deep in other patches at the moment; I certainly am. -- Robert Haas EnterpriseDB: 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] ALTER TABLE lock strength reduction patch is unsafe
Robert Haas robertmh...@gmail.com writes: On Tue, Jan 28, 2014 at 12:36 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Honestly, I would prefer that we push a patch that has been thoroughly reviewed and in which we have more confidence, so that we can push without a GUC. This means mark in CF as needs-review, then some other developer reviews it and marks it as ready-for-committer. I also believe that would be the correct procedure. Personally, I think it would be great if Noah can review this, as he's very good at finding the kind of problems that are likely to crop up here, and has examined previous versions. I also have some interest in looking at it myself. But I doubt that either of us (or any other senior hacker) can do that by Thursday. I think all such people are hip-deep in other patches at the moment; I certainly am. Yeah. I actually have little doubt that the patch is sane on its own terms of discussion, that is that Simon has chosen locking levels that are mutually consistent in an abstract sense. What sank the previous iteration was that he'd failed to consider an implementation detail, namely possible inconsistencies in SnapshotNow-based catalog fetches. I'm afraid that there may be more such problems lurking under the surface. Noah's pretty good at finding such things, but really two or three of us need to sit down and think about it for awhile before I'd have much confidence in such a fundamental change. And I sure don't have time for this patch right now myself. 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 lock strength reduction patch is unsafe
On 24 January 2014 08:33, Simon Riggs si...@2ndquadrant.com wrote: On 24 January 2014 07:08, Peter Geoghegan p...@heroku.com wrote: On Wed, Jan 15, 2014 at 6:57 AM, Simon Riggs si...@2ndquadrant.com wrote: v15 to fix the above problem. v16 attached v17 attached This version adds a GUC called ddl_exclusive_locks which allows us to keep the 9.3 behaviour if we wish it. Some people may be surprised that their programs don't wait in the same places they used to. We hope that is a positive and useful behaviour, but it may not always be so. I'll commit this on Thurs 30 Jan unless I hear objections. Thanks -- 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] ALTER TABLE lock strength reduction patch is unsafe
On 27 January 2014 17:58, Simon Riggs si...@2ndquadrant.com wrote: On 24 January 2014 08:33, Simon Riggs si...@2ndquadrant.com wrote: On 24 January 2014 07:08, Peter Geoghegan p...@heroku.com wrote: On Wed, Jan 15, 2014 at 6:57 AM, Simon Riggs si...@2ndquadrant.com wrote: v15 to fix the above problem. v16 attached v17 attached Frostbite... -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services reduce_lock_levels.v17.patch 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] ALTER TABLE lock strength reduction patch is unsafe
On Mon, Jan 27, 2014 at 12:58 PM, Simon Riggs si...@2ndquadrant.com wrote: On 24 January 2014 08:33, Simon Riggs si...@2ndquadrant.com wrote: On 24 January 2014 07:08, Peter Geoghegan p...@heroku.com wrote: On Wed, Jan 15, 2014 at 6:57 AM, Simon Riggs si...@2ndquadrant.com wrote: v15 to fix the above problem. v16 attached v17 attached This version adds a GUC called ddl_exclusive_locks which allows us to keep the 9.3 behaviour if we wish it. Some people may be surprised that their programs don't wait in the same places they used to. We hope that is a positive and useful behaviour, but it may not always be so. I'll commit this on Thurs 30 Jan unless I hear objections. I haven't reviewed the patch, but -1 for adding a GUC. -- Robert Haas EnterpriseDB: 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] ALTER TABLE lock strength reduction patch is unsafe
On Mon, Jan 27, 2014 at 12:25 PM, Robert Haas robertmh...@gmail.com wrote: I haven't reviewed the patch, but -1 for adding a GUC. I'm pretty surprised that it's been suggested that some people might prefer AccessExclusiveLocks. Why would anyone prefer that? -- Peter Geoghegan -- 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 lock strength reduction patch is unsafe
Peter Geoghegan p...@heroku.com writes: On Mon, Jan 27, 2014 at 12:25 PM, Robert Haas robertmh...@gmail.com wrote: I haven't reviewed the patch, but -1 for adding a GUC. I'm pretty surprised that it's been suggested that some people might prefer AccessExclusiveLocks. Why would anyone prefer that? For one thing, so they can back this out if it proves to be broken, as the last committed version was. Given that this patch was marked (by its author) as Ready for Committer without any review in the current CF, I can't say that I have any faith in 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] ALTER TABLE lock strength reduction patch is unsafe
On 27 January 2014 20:35, Peter Geoghegan p...@heroku.com wrote: On Mon, Jan 27, 2014 at 12:25 PM, Robert Haas robertmh...@gmail.com wrote: I haven't reviewed the patch, but -1 for adding a GUC. I'm pretty surprised that it's been suggested that some people might prefer AccessExclusiveLocks. Why would anyone prefer that? Nobody has said prefer. I said... Some people may be surprised that their programs don't wait in the same places they used to. We hope that is a positive and useful behaviour, but it may not always be so. We get the new behaviour by default and I expect we'll be very happy with it. A second thought is that if we have problems of some kind in the field as a result of the new lock modes then we will be able to turn them off. I'm happy to fix any problems that occur, but that doesn't mean there won't be any. If everybody is confident that we've foreseen every bug, then no problem, lets remove it. I recall being asked to add hot_standby = on | off for similar reasons. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On 27 January 2014 20:47, Tom Lane t...@sss.pgh.pa.us wrote: Peter Geoghegan p...@heroku.com writes: On Mon, Jan 27, 2014 at 12:25 PM, Robert Haas robertmh...@gmail.com wrote: I haven't reviewed the patch, but -1 for adding a GUC. I'm pretty surprised that it's been suggested that some people might prefer AccessExclusiveLocks. Why would anyone prefer that? For one thing, so they can back this out if it proves to be broken, as the last committed version was. Agreed Given that this patch was marked (by its author) as Ready for Committer without any review in the current CF True. The main review happened in a previous commitfest and there was a small addition for this CF. It was my understanding that you wanted us to indicate that to allow you to review. I am happy to set status differently, as you wish, presumably back to needs review. I can't say that I have any faith in it. That's a shame. -- 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] ALTER TABLE lock strength reduction patch is unsafe
On 24 January 2014 07:08, Peter Geoghegan p...@heroku.com wrote: On Wed, Jan 15, 2014 at 6:57 AM, Simon Riggs si...@2ndquadrant.com wrote: v15 to fix the above problem. Minor quibble: I get a compiler warning with the patch applied. relcache.c: In function 'RememberToFreeTupleDescAtEOX': relcache.c:2317:3: warning: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]. Thanks, that was a wart, now fixed. v16 attached -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services reduce_lock_levels.v16.patch 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
