Re: ALTER TABLE lock strength reduction patch is unsafe

On Sun, Jul 7, 2013 at 9:24 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 3 January 2012 18:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

Another point that requires some thought is that switching SnapshotNow
to be MVCC-based will presumably result in a noticeable increase in each
backend's rate of wanting to acquire snapshots.

BTW, I wonder if this couldn't be ameliorated by establishing some
ground rules about how up-to-date a snapshot really needs to be.
Arguably, it should be okay for successive SnapshotNow scans to use the
same snapshot as long as we have not acquired a new lock in between.
If not, reusing an old snap doesn't introduce any race condition that
wasn't there already.

Now that has been implemented using the above design, we can resubmit
the lock level reduction patch, with thanks to Robert.

Submitted patch passes original complaint/benchmark.

Changes
* various forms of ALTER TABLE, notably ADD constraint and VALIDATE
* CREATE TRIGGER

One minor coirrections to earlier thinking with respect to toast
tables. That might be later relaxed.

Full tests including proof of lock level reductions, plus docs.

I'm quite glad to see this patch back again for another round. I
haven't reviewed it in detail yet, so the purpose of this email is
just to lay out some general areas of concern and food for thought
rather than to critique anything in the patch too specifically.

Generally, the question on the table is: to what extent do MVCC
catalog scans make the world safe for concurrent DDL, or put
negatively, what hazards remain?

Noah discovered an interesting one recently: apparently, the relcache
machinery has some logic in it that depends on the use of
AccessExclusiveLock in subtle ways. I'm going to attempt to explain
it, and maybe he can jump in and explain it better. Essentially, the
problem is that when a relcache reload occurs, certain data structures
(like the tuple descriptor, but there are others) are compared against
the old version of the same data structure. If there are no changes,
we do nothing; else, we free the old one and install the new one. The
reason why we don't free the old one and install the new one
unconditionally is because other parts of the backend might have
pointers to the old data structure, so just replacing it all the time
would result in crashes. Since DDL requires AccessExclusiveLock +
CheckTableNotInUse(), any actual change to the data structure will
happen when we haven't got any pointers anyway.

A second thing I'm concerned about is that, although MVCC catalog
scans guarantee that we won't miss a concurrently-updated row
entirely, or see a duplicate, they don't guarantee that the rows we
see during a scan of catalog A will be consistent with the rows we see
during a scan of catalog B moments later. For example, hilarity will
ensue if relnatts doesn't match what we see in pg_attribute. Now I
don't think this particular example matters very much because I think
there are probably lots of other things that would also break if we
try to add a column without a full table lock, so we're probably
doomed there anyway. But there might be other instances of this
problem that are more subtle.

I'll try to find some time to look at this in more detail.

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

On 2013-07-15 10:06:31 -0400, Robert Haas wrote:

Noah discovered an interesting one recently: apparently, the relcache
machinery has some logic in it that depends on the use of
AccessExclusiveLock in subtle ways. I'm going to attempt to explain
it, and maybe he can jump in and explain it better. Essentially, the
problem is that when a relcache reload occurs, certain data structures
(like the tuple descriptor, but there are others) are compared against
the old version of the same data structure. If there are no changes,
we do nothing; else, we free the old one and install the new one. The
reason why we don't free the old one and install the new one
unconditionally is because other parts of the backend might have
pointers to the old data structure, so just replacing it all the time
would result in crashes. Since DDL requires AccessExclusiveLock +
CheckTableNotInUse(), any actual change to the data structure will
happen when we haven't got any pointers anyway.

Aren't we swapping in the new data on a data level for that reason? See
RelationClearRelation().

A second thing I'm concerned about is that, although MVCC catalog
scans guarantee that we won't miss a concurrently-updated row
entirely, or see a duplicate, they don't guarantee that the rows we
see during a scan of catalog A will be consistent with the rows we see
during a scan of catalog B moments later. For example, hilarity will
ensue if relnatts doesn't match what we see in pg_attribute. Now I
don't think this particular example matters very much because I think
there are probably lots of other things that would also break if we
try to add a column without a full table lock, so we're probably
doomed there anyway. But there might be other instances of this
problem that are more subtle.

Hm. Other transactions basically should be protected against this
because they can't se uncommitted data anyway, right? ISTM that our own
session basically already has be safe against hilarity of this kind,
right?

I am concerned about stuff like violating constraints because we haven't
yet seen the new constraint definition and the like... Or generating
wrong plans because we haven't seen that somebody has dropped a
constraint and inserted data violating the old one.

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

On Mon, Jul 15, 2013 at 10:32 AM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2013-07-15 10:06:31 -0400, Robert Haas wrote:

Noah discovered an interesting one recently: apparently, the relcache
machinery has some logic in it that depends on the use of
AccessExclusiveLock in subtle ways. I'm going to attempt to explain
it, and maybe he can jump in and explain it better. Essentially, the
problem is that when a relcache reload occurs, certain data structures
(like the tuple descriptor, but there are others) are compared against
the old version of the same data structure. If there are no changes,
we do nothing; else, we free the old one and install the new one. The
reason why we don't free the old one and install the new one
unconditionally is because other parts of the backend might have
pointers to the old data structure, so just replacing it all the time
would result in crashes. Since DDL requires AccessExclusiveLock +
CheckTableNotInUse(), any actual change to the data structure will
happen when we haven't got any pointers anyway.

Aren't we swapping in the new data on a data level for that reason? See
RelationClearRelation().

Look at the keep_tupdesc and keep_rules variables.

A second thing I'm concerned about is that, although MVCC catalog
scans guarantee that we won't miss a concurrently-updated row
entirely, or see a duplicate, they don't guarantee that the rows we
see during a scan of catalog A will be consistent with the rows we see
during a scan of catalog B moments later. For example, hilarity will
ensue if relnatts doesn't match what we see in pg_attribute. Now I
don't think this particular example matters very much because I think
there are probably lots of other things that would also break if we
try to add a column without a full table lock, so we're probably
doomed there anyway. But there might be other instances of this
problem that are more subtle.

Hm. Other transactions basically should be protected against this
because they can't se uncommitted data anyway, right? ISTM that our own
session basically already has be safe against hilarity of this kind,
right?

Other transactions are protected only within a single scan. If they
do two or more separate scans one after the another, some other
transaction can commit in the middle of things. Any commits that
happen after starting the first scan and before starting the second
scan will be reflected in the second scan, but not in the first.
That's what I'm concerned about. Our own session can't have a problem
of this kind, because we'll never commit a subtransaction (or, heaven
forbid, a top-level transaction) while in the middle of a system
catalog scan.

I am concerned about stuff like violating constraints because we haven't
yet seen the new constraint definition and the like... Or generating
wrong plans because we haven't seen that somebody has dropped a
constraint and inserted data violating the old one.

Yes, we need to carefully audit the commands for dependencies of that type.

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

On 15 July 2013 15:06, Robert Haas <robertmhaas@gmail.com> wrote:

Generally, the question on the table is: to what extent do MVCC
catalog scans make the world safe for concurrent DDL, or put
negatively, what hazards remain?

On Tom's test I've been unable to find a single problem.

Noah discovered an interesting one recently: apparently, the relcache
machinery has some logic in it that depends on the use of
AccessExclusiveLock in subtle ways. I'm going to attempt to explain
it, and maybe he can jump in and explain it better. Essentially, the
problem is that when a relcache reload occurs, certain data structures
(like the tuple descriptor, but there are others) are compared against
the old version of the same data structure. If there are no changes,
we do nothing; else, we free the old one and install the new one. The
reason why we don't free the old one and install the new one
unconditionally is because other parts of the backend might have
pointers to the old data structure, so just replacing it all the time
would result in crashes. Since DDL requires AccessExclusiveLock +
CheckTableNotInUse(), any actual change to the data structure will
happen when we haven't got any pointers anyway.

A second thing I'm concerned about is that, although MVCC catalog
scans guarantee that we won't miss a concurrently-updated row
entirely, or see a duplicate, they don't guarantee that the rows we
see during a scan of catalog A will be consistent with the rows we see
during a scan of catalog B moments later. For example, hilarity will
ensue if relnatts doesn't match what we see in pg_attribute. Now I
don't think this particular example matters very much because I think
there are probably lots of other things that would also break if we
try to add a column without a full table lock, so we're probably
doomed there anyway. But there might be other instances of this
problem that are more subtle.

If you look at this as a generalised problem you probably can find
some issues, but that doesn't mean they apply in the specific cases
we're addressing.

The lock reductions we are discussing in all cases have nothing at all
to do with structure and only relate to various options. Except in the
case of constraints, though even there I see no issues as yet.

I've no problem waiting awhile to apply while others try to find loopholes.

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

On Mon, Jul 15, 2013 at 05:50:40PM +0100, Simon Riggs wrote:

On 15 July 2013 15:06, Robert Haas <robertmhaas@gmail.com> wrote:

Generally, the question on the table is: to what extent do MVCC
catalog scans make the world safe for concurrent DDL, or put
negatively, what hazards remain?

On Tom's test I've been unable to find a single problem.

Noah discovered an interesting one recently: apparently, the relcache
machinery has some logic in it that depends on the use of
AccessExclusiveLock in subtle ways. I'm going to attempt to explain
it, and maybe he can jump in and explain it better. Essentially, the
problem is that when a relcache reload occurs, certain data structures
(like the tuple descriptor, but there are others) are compared against
the old version of the same data structure. If there are no changes,
we do nothing; else, we free the old one and install the new one. The
reason why we don't free the old one and install the new one
unconditionally is because other parts of the backend might have
pointers to the old data structure, so just replacing it all the time
would result in crashes. Since DDL requires AccessExclusiveLock +
CheckTableNotInUse(), any actual change to the data structure will
happen when we haven't got any pointers anyway.

If you look at this as a generalised problem you probably can find
some issues, but that doesn't mean they apply in the specific cases
we're addressing.

The lock reductions we are discussing in all cases have nothing at all
to do with structure and only relate to various options. Except in the
case of constraints, though even there I see no issues as yet.

I was able to distill the above hypothesis into an actual crash with
reduce_lock_levels.v13.patch. Test recipe:

1. Build with --enable-cassert and with -DCATCACHE_FORCE_RELEASE=1. An
AcceptInvalidationMessages() will then happen at nearly every syscache lookup,
making it far easier to hit a problem of this sort.

2. Run these commands as setup:
create table root (c int);
create table part (check (c > 0), check (c > 0)) inherits (root);

3. Attach a debugger to your session and set a breakpoint at plancat.c:660 (as
of commit 16f38f72ab2b8a3b2d45ba727d213bb31111cea4).

4. Run this in your session; the breakpoint will trip:
select * from root where c = -1;

5. Start another session and run:
alter table part add check (c > 0);

6. Exit the debugger to release the first session. It will crash.

plancache.c:657 stashes a pointer to memory belonging to the rd_att of a
relcache entry. It then proceeds to call eval_const_expressions(), which
performs a syscache lookup in its simplify_function() subroutine. Under
CATCACHE_FORCE_RELEASE, the syscache lookup reliably prompts an
AcceptInvalidationMessages(). The ensuing RelationClearRelation() against
"part" notices the new constraint, decides keep_tupdesc = false, and frees the
existing tupdesc. plancache.c is now left holding a pointer into freed
memory. The next loop iteration will crash when it dereferences a pointer
stored within that freed memory at plancat.c:671.

A remediation strategy that seemed attractive when I last contemplated this
problem is to repoint rd_att immediately but arrange to free the obsolete
TupleDesc in AtEOXact_RelationCache().

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

On 1 August 2013 01:53, Noah Misch <noah@leadboat.com> wrote:

I was able to distill the above hypothesis into an actual crash with
reduce_lock_levels.v13.patch. Test recipe:

Thank you for the report; thank you again for reporting in sufficient
time to allow me to investigate and fix by the next CF.

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

On 1 August 2013 01:53, Noah Misch <noah@leadboat.com> wrote:

A remediation strategy that seemed attractive when I last contemplated this
problem is to repoint rd_att immediately but arrange to free the obsolete
TupleDesc in AtEOXact_RelationCache().

I agree that the best way to resolve this is to retain a copy of the
TupleDesc, so that copied pointers to it remain valid.

EOXact is actually longer than strictly necessary in some cases, but
trying to work out a more minimal approach seems hard and possibly
inefficient.

Comments in relcache.c indicate that the Relation swapping concept
might be replaced by refcounting approach. I can't see how that
differs from your suggested route.

Which means I can't see any other way of doing this other than the way
you suggest. Will implement.

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

On 7 July 2013 14:24, Simon Riggs <simon@2ndquadrant.com> wrote:

On 3 January 2012 18:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

Another point that requires some thought is that switching SnapshotNow
to be MVCC-based will presumably result in a noticeable increase in each
backend's rate of wanting to acquire snapshots.

BTW, I wonder if this couldn't be ameliorated by establishing some
ground rules about how up-to-date a snapshot really needs to be.
Arguably, it should be okay for successive SnapshotNow scans to use the
same snapshot as long as we have not acquired a new lock in between.
If not, reusing an old snap doesn't introduce any race condition that
wasn't there already.

Now that has been implemented using the above design, we can resubmit
the lock level reduction patch, with thanks to Robert.

Submitted patch passes original complaint/benchmark.

Changes
* various forms of ALTER TABLE, notably ADD constraint and VALIDATE
* CREATE TRIGGER

One minor coirrections to earlier thinking with respect to toast
tables. That might be later relaxed.

Full tests including proof of lock level reductions, plus docs.

Rebased to v14

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

On 1 August 2013 01:53, Noah Misch <noah@leadboat.com> wrote:

On Mon, Jul 15, 2013 at 05:50:40PM +0100, Simon Riggs wrote:

On 15 July 2013 15:06, Robert Haas <robertmhaas@gmail.com> wrote:

Generally, the question on the table is: to what extent do MVCC
catalog scans make the world safe for concurrent DDL, or put
negatively, what hazards remain?

On Tom's test I've been unable to find a single problem.

Noah discovered an interesting one recently: apparently, the relcache
machinery has some logic in it that depends on the use of
AccessExclusiveLock in subtle ways. I'm going to attempt to explain
it, and maybe he can jump in and explain it better. Essentially, the
problem is that when a relcache reload occurs, certain data structures
(like the tuple descriptor, but there are others) are compared against
the old version of the same data structure. If there are no changes,
we do nothing; else, we free the old one and install the new one. The
reason why we don't free the old one and install the new one
unconditionally is because other parts of the backend might have
pointers to the old data structure, so just replacing it all the time
would result in crashes. Since DDL requires AccessExclusiveLock +
CheckTableNotInUse(), any actual change to the data structure will
happen when we haven't got any pointers anyway.

If you look at this as a generalised problem you probably can find
some issues, but that doesn't mean they apply in the specific cases
we're addressing.

The lock reductions we are discussing in all cases have nothing at all
to do with structure and only relate to various options. Except in the
case of constraints, though even there I see no issues as yet.

I was able to distill the above hypothesis into an actual crash with
reduce_lock_levels.v13.patch. Test recipe:

1. Build with --enable-cassert and with -DCATCACHE_FORCE_RELEASE=1. An
AcceptInvalidationMessages() will then happen at nearly every syscache lookup,
making it far easier to hit a problem of this sort.

2. Run these commands as setup:
create table root (c int);
create table part (check (c > 0), check (c > 0)) inherits (root);

3. Attach a debugger to your session and set a breakpoint at plancat.c:660 (as
of commit 16f38f72ab2b8a3b2d45ba727d213bb31111cea4).

4. Run this in your session; the breakpoint will trip:
select * from root where c = -1;

5. Start another session and run:
alter table part add check (c > 0);

6. Exit the debugger to release the first session. It will crash.

plancache.c:657 stashes a pointer to memory belonging to the rd_att of a
relcache entry. It then proceeds to call eval_const_expressions(), which
performs a syscache lookup in its simplify_function() subroutine. Under
CATCACHE_FORCE_RELEASE, the syscache lookup reliably prompts an
AcceptInvalidationMessages(). The ensuing RelationClearRelation() against
"part" notices the new constraint, decides keep_tupdesc = false, and frees the
existing tupdesc. plancache.c is now left holding a pointer into freed
memory. The next loop iteration will crash when it dereferences a pointer
stored within that freed memory at plancat.c:671.

A remediation strategy that seemed attractive when I last contemplated this
problem is to repoint rd_att immediately but arrange to free the obsolete
TupleDesc in AtEOXact_RelationCache().

v15 to fix the above problem.

Looking at other potential problems around plancache but nothing found as yet.

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

On Wed, Jan 15, 2014 at 6:57 AM, Simon Riggs <simon@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]".

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

On 24 January 2014 07:08, Peter Geoghegan <pg@heroku.com> wrote:

On Wed, Jan 15, 2014 at 6:57 AM, Simon Riggs <simon@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

On 24 January 2014 08:33, Simon Riggs <simon@2ndquadrant.com> wrote:

On 24 January 2014 07:08, Peter Geoghegan <pg@heroku.com> wrote:

On Wed, Jan 15, 2014 at 6:57 AM, Simon Riggs <simon@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

On 27 January 2014 17:58, Simon Riggs <simon@2ndquadrant.com> wrote:

On 24 January 2014 08:33, Simon Riggs <simon@2ndquadrant.com> wrote:

On 24 January 2014 07:08, Peter Geoghegan <pg@heroku.com> wrote:

On Wed, Jan 15, 2014 at 6:57 AM, Simon Riggs <simon@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

On Mon, Jan 27, 2014 at 12:58 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 24 January 2014 08:33, Simon Riggs <simon@2ndquadrant.com> wrote:

On 24 January 2014 07:08, Peter Geoghegan <pg@heroku.com> wrote:

On Wed, Jan 15, 2014 at 6:57 AM, Simon Riggs <simon@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

On Mon, Jan 27, 2014 at 12:25 PM, Robert Haas <robertmhaas@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

Peter Geoghegan <pg@heroku.com> writes:

On Mon, Jan 27, 2014 at 12:25 PM, Robert Haas <robertmhaas@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

On 27 January 2014 20:35, Peter Geoghegan <pg@heroku.com> wrote:

On Mon, Jan 27, 2014 at 12:25 PM, Robert Haas <robertmhaas@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

On 27 January 2014 20:47, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Geoghegan <pg@heroku.com> writes:

On Mon, Jan 27, 2014 at 12:25 PM, Robert Haas <robertmhaas@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

On Mon, Jan 27, 2014 at 08:57:26PM +0000, 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 <bruce@momjian.us> http://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