Re: ALTER TABLE SET STATISTICS requires AccessExclusiveLock

Is there a good reason for $subject, other than that the code is entangled
with other ALTER TABLE code?

The new SET DISTINCT might be equally affected.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Is there a good reason for $subject, other than that the code is entangled
> with other ALTER TABLE code?

I think it could be lower, but it would take nontrivial restructuring of
the ALTER TABLE support. In particular, consider what happens when you
have a list of subcommands that don't all require the same lock level.
I think you'd need to scan the list and find the highest required lock
level before starting ...

regards, tom lane

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > Is there a good reason for $subject, other than that the code is entangled
> > with other ALTER TABLE code?
>
> I think it could be lower, but it would take nontrivial restructuring of
> the ALTER TABLE support. In particular, consider what happens when you
> have a list of subcommands that don't all require the same lock level.
> I think you'd need to scan the list and find the highest required lock
> level before starting ...

IIRC there was a patch from Simon to address this issue, but it had some
holes which he didn't have time to close, so it sank. Maybe this can be
resurrected and fixed.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
> Tom Lane wrote:
> > Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > > Is there a good reason for $subject, other than that the code is entangled
> > > with other ALTER TABLE code?
> >
> > I think it could be lower, but it would take nontrivial restructuring of
> > the ALTER TABLE support. In particular, consider what happens when you
> > have a list of subcommands that don't all require the same lock level.
> > I think you'd need to scan the list and find the highest required lock
> > level before starting ...
>
> IIRC there was a patch from Simon to address this issue, but it had some
> holes which he didn't have time to close, so it sank. Maybe this can be
> resurrected and fixed.

I was intending to finish that patch in this release cycle.

MERGE needs further work if you are looking for a project. It isn't
immediately obvious but MERGE logic is a requirement for maintaining
materialized views, which is why I was working on that.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs wrote:
>
> On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
> > Tom Lane wrote:
> > > Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > > > Is there a good reason for $subject, other than that the code is entangled
> > > > with other ALTER TABLE code?
> > >
> > > I think it could be lower, but it would take nontrivial restructuring of
> > > the ALTER TABLE support. In particular, consider what happens when you
> > > have a list of subcommands that don't all require the same lock level.
> > > I think you'd need to scan the list and find the highest required lock
> > > level before starting ...
> >
> > IIRC there was a patch from Simon to address this issue, but it had some
> > holes which he didn't have time to close, so it sank. Maybe this can be
> > resurrected and fixed.
>
> I was intending to finish that patch in this release cycle.

Since you're busy with Hot Standby, any chance you could pass it on?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Mon, 2009-10-19 at 12:56 -0300, Alvaro Herrera wrote:
> Simon Riggs wrote:
> >
> > On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
> > > Tom Lane wrote:
> > > > Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > > > > Is there a good reason for $subject, other than that the code is entangled
> > > > > with other ALTER TABLE code?
> > > >
> > > > I think it could be lower, but it would take nontrivial restructuring of
> > > > the ALTER TABLE support. In particular, consider what happens when you
> > > > have a list of subcommands that don't all require the same lock level.
> > > > I think you'd need to scan the list and find the highest required lock
> > > > level before starting ...
> > >
> > > IIRC there was a patch from Simon to address this issue, but it had some
> > > holes which he didn't have time to close, so it sank. Maybe this can be
> > > resurrected and fixed.
> >
> > I was intending to finish that patch in this release cycle.
>
> Since you're busy with Hot Standby, any chance you could pass it on?

If you'd like. It's mostly finished, just one last thing to finish:
atomic changes to pg_class via an already agreed API.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs wrote:
> On Mon, 2009-10-19 at 12:56 -0300, Alvaro Herrera wrote:
> > Simon Riggs wrote:
> > >
> > > On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
> > > > Tom Lane wrote:
> > > > > Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > > > > > Is there a good reason for $subject, other than that the code is entangled
> > > > > > with other ALTER TABLE code?
> > > > >
> > > > > I think it could be lower, but it would take nontrivial restructuring of
> > > > > the ALTER TABLE support. In particular, consider what happens when you
> > > > > have a list of subcommands that don't all require the same lock level.
> > > > > I think you'd need to scan the list and find the highest required lock
> > > > > level before starting ...
> > > >
> > > > IIRC there was a patch from Simon to address this issue, but it had some
> > > > holes which he didn't have time to close, so it sank. Maybe this can be
> > > > resurrected and fixed.
> > >
> > > I was intending to finish that patch in this release cycle.
> >
> > Since you're busy with Hot Standby, any chance you could pass it on?
>
> If you'd like. It's mostly finished, just one last thing to finish:
> atomic changes to pg_class via an already agreed API.

I assume this did not get done for 9.0. Do we want a TODO item?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
+ If your life is a hard drive, Christ can be your backup. +

On mån, 2010-02-22 at 10:32 -0500, Bruce Momjian wrote:
> Simon Riggs wrote:
> > On Mon, 2009-10-19 at 12:56 -0300, Alvaro Herrera wrote:
> > > Simon Riggs wrote:
> > > >
> > > > On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
> > > > > Tom Lane wrote:
> > > > > > Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > > > > > > Is there a good reason for $subject, other than that the code is entangled
> > > > > > > with other ALTER TABLE code?
> > > > > >
> > > > > > I think it could be lower, but it would take nontrivial restructuring of
> > > > > > the ALTER TABLE support. In particular, consider what happens when you
> > > > > > have a list of subcommands that don't all require the same lock level.
> > > > > > I think you'd need to scan the list and find the highest required lock
> > > > > > level before starting ...
> > > > >
> > > > > IIRC there was a patch from Simon to address this issue, but it had some
> > > > > holes which he didn't have time to close, so it sank. Maybe this can be
> > > > > resurrected and fixed.
> > > >
> > > > I was intending to finish that patch in this release cycle.
> > >
> > > Since you're busy with Hot Standby, any chance you could pass it on?
> >
> > If you'd like. It's mostly finished, just one last thing to finish:
> > atomic changes to pg_class via an already agreed API.
>
> I assume this did not get done for 9.0. Do we want a TODO item?

Yes.

Peter Eisentraut wrote:
> On m?n, 2010-02-22 at 10:32 -0500, Bruce Momjian wrote:
> > Simon Riggs wrote:
> > > On Mon, 2009-10-19 at 12:56 -0300, Alvaro Herrera wrote:
> > > > Simon Riggs wrote:
> > > > >
> > > > > On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
> > > > > > Tom Lane wrote:
> > > > > > > Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > > > > > > > Is there a good reason for $subject, other than that the code is entangled
> > > > > > > > with other ALTER TABLE code?
> > > > > > >
> > > > > > > I think it could be lower, but it would take nontrivial restructuring of
> > > > > > > the ALTER TABLE support. In particular, consider what happens when you
> > > > > > > have a list of subcommands that don't all require the same lock level.
> > > > > > > I think you'd need to scan the list and find the highest required lock
> > > > > > > level before starting ...
> > > > > >
> > > > > > IIRC there was a patch from Simon to address this issue, but it had some
> > > > > > holes which he didn't have time to close, so it sank. Maybe this can be
> > > > > > resurrected and fixed.
> > > > >
> > > > > I was intending to finish that patch in this release cycle.
> > > >
> > > > Since you're busy with Hot Standby, any chance you could pass it on?
> > >
> > > If you'd like. It's mostly finished, just one last thing to finish:
> > > atomic changes to pg_class via an already agreed API.
> >
> > I assume this did not get done for 9.0. Do we want a TODO item?
>
> Yes.

Added:

Reduce locking required for ALTER commands

* http://archives.postgresql.org/pgsql-hackers/2009-08/msg00533.php
* http://archives.postgresql.org/pgsql-hackers/2009-10/msg01083.php
* http://archives.postgresql.org/pgsql-hackers/2010-01/msg02349.php

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

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

2010/3/3 Bruce Momjian <bruce(at)momjian(dot)us>:
> Peter Eisentraut wrote:
>> On m?n, 2010-02-22 at 10:32 -0500, Bruce Momjian wrote:
>> > Simon Riggs wrote:
>> > > On Mon, 2009-10-19 at 12:56 -0300, Alvaro Herrera wrote:
>> > > > Simon Riggs wrote:
>> > > > >
>> > > > > On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
>> > > > > > Tom Lane wrote:
>> > > > > > > Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> > > > > > > > Is there a good reason for $subject, other than that the code is entangled
>> > > > > > > > with other ALTER TABLE code?
>> > > > > > >
>> > > > > > > I think it could be lower, but it would take nontrivial restructuring of
>> > > > > > > the ALTER TABLE support.  In particular, consider what happens when you
>> > > > > > > have a list of subcommands that don't all require the same lock level.
>> > > > > > > I think you'd need to scan the list and find the highest required lock
>> > > > > > > level before starting ...
>> > > > > >
>> > > > > > IIRC there was a patch from Simon to address this issue, but it had some
>> > > > > > holes which he didn't have time to close, so it sank.  Maybe this can be
>> > > > > > resurrected and fixed.
>> > > > >
>> > > > > I was intending to finish that patch in this release cycle.
>> > > >
>> > > > Since you're busy with Hot Standby, any chance you could pass it on?
>> > >
>> > > If you'd like. It's mostly finished, just one last thing to finish:
>> > > atomic changes to pg_class via an already agreed API.
>> >
>> > I assume this did not get done for 9.0.  Do we want a TODO item?
>>
>> Yes.
>
> Added:
>
>        Reduce locking required for ALTER commands

I just faced production issue where it is impossible to alter table to
adjust autovacuum settings in a pg8.4. (5K tps, 260M rows table, lock
too much)

Can we add some mechanism to prevent that situation also in the TODO item ?

(alternative is actualy to alter other tables and adjust the
postgresql.conf for biggest tables, but not an ideal solution anyway)

>
>            * http://archives.postgresql.org/pgsql-hackers/2009-08/msg00533.php
>            * http://archives.postgresql.org/pgsql-hackers/2009-10/msg01083.php
>            * http://archives.postgresql.org/pgsql-hackers/2010-01/msg02349.php
>
> --
>  Bruce Momjian  <bruce(at)momjian(dot)us>        http://momjian.us
>  EnterpriseDB                             http://enterprisedb.com
>
>  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

--
Cédric Villemain 2ndQuadrant
http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support

On 7/7/10 6:04 PM, Cédric Villemain wrote:
> I just faced production issue where it is impossible to alter table to
> adjust autovacuum settings in a pg8.4. (5K tps, 260M rows table, lock
> too much)

We could try to resolve the COMMENT ON issue with the same mechanism.
What we need is a table lock which blocks other DDL statements, but not DML.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

On Wed, Jul 7, 2010 at 9:04 PM, Cédric Villemain
<cedric(dot)villemain(dot)debian(at)gmail(dot)com> wrote:
>>> > I assume this did not get done for 9.0.  Do we want a TODO item?
>>>
>>> Yes.
>>
>> Added:
>>
>>        Reduce locking required for ALTER commands
>
> I just faced production issue where it is impossible to alter table to
> adjust autovacuum settings in a pg8.4. (5K tps, 260M rows table, lock
> too much)
>
> Can we add some mechanism to prevent that situation also in the TODO item ?
>
> (alternative is actualy to alter other tables and adjust the
> postgresql.conf for biggest tables, but not an ideal solution anyway)
>
>>
>>            * http://archives.postgresql.org/pgsql-hackers/2009-08/msg00533.php
>>            * http://archives.postgresql.org/pgsql-hackers/2009-10/msg01083.php
>>            * http://archives.postgresql.org/pgsql-hackers/2010-01/msg02349.php

Bruce, that last link is about something else completely. Here are
some better ones:

http://archives.postgresql.org/pgsql-hackers/2008-10/msg01248.php
http://archives.postgresql.org/pgsql-hackers/2008-10/msg00242.php

All,

Rereading the thread, I'm a bit confused by why we're proposing to use
a SHARE lock; it seems to me that a self-conflicting lock type would
simplify things. There's a bunch of discussion on the thread about
how to handle pg_class updates atomically, but doesn't using a
self-conflicting lock type eliminate that problem?

It strikes me that for the following operations, which don't affect
queries at all, we could use a SHARE UPDATE EXCLUSIVE, which is likely
superior to SHARE for this purpose because it wouldn't lock out
concurrent DML write operations:

ALTER [ COLUMN ] column SET STATISTICS integer
ALTER [ COLUMN ] column SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
ALTER [ COLUMN ] column SET ( attribute_option = value [, ... ] )
ALTER [ COLUMN ] column RESET ( attribute_option [, ... ] )
CLUSTER ON index_name
SET WITHOUT CLUSTER
SET ( storage_parameter = value [, ... ] )
RESET ( storage_parameter [, ... ] )

(Of the above list, arguably SET STORAGE and [RE]SET (fillfactor) do
in fact affect DML writes, but it seems like changing them on the fly
should still be safe.)

The remaining commands which Simon proposed to downgrade to share-locks were:

ALTER [ COLUMN ] column SET DEFAULT expression
CREATE RULE (only non-ON SELECT rules)
CREATE TRIGGER
ALTER [ COLUMN ] column SET NOT NULL (but not DROP NOT NULL)
ADD table_constraint (but not DROP CONSTRAINT)
DISABLE TRIGGER [ trigger_name | ALL | USER ]
ENABLE TRIGGER [ trigger_name | ALL | USER ]
ENABLE REPLICA TRIGGER trigger_name
ENABLE ALWAYS TRIGGER trigger_name

Setting a column default, creating a non-select RULE, and
creating/disabling a trigger shouldn't affect SELECT statements, so as
long as we lock out all updates we should be OK. For these it seems
we could use SHARE ROW EXCLUSIVE, which will conflict with any other
DML command and with any data change, but not with SELECTs.

I am somewhat fuzzy on what the correct locking is for SET NOT NULL
and ADD table_constraint. I believe that the idea here is that a
query plan might rely on the existence of a constraint for
correctness, so we must lock out all queries when dropping one; but a
query plan can't rely on the absence of a constraint for correctness
(since the constraint could be true anyway), so it's safe to allow one
to be added even when there are queries in flight. If that's correct
then it seems like we could use SHARE ROW EXCLUSIVE for these command
types as well. However, these two particular commands have another
distinguishing characteristic also: they might run for a while, so it
would be useful to be able to do more than one at once. So maybe it's
worth thinking a little harder about how to weaken those two in
particular to some non-self-conflicting lock type. Then again, even
SHARE ROW EXCLUSIVE is a big improvement over ACCESS EXCLUSIVE, so
maybe that would be enough for a first go at the problem.

Thoughts?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Wed, 2010-07-07 at 22:26 -0400, Robert Haas wrote:

> Rereading the thread, I'm a bit confused by why we're proposing to use
> a SHARE lock; it seems to me that a self-conflicting lock type would
> simplify things. There's a bunch of discussion on the thread about
> how to handle pg_class updates atomically, but doesn't using a
> self-conflicting lock type eliminate that problem?

The use of the SHARE lock had nothing to do with the pg_class update
requirement, so that suggestion won't help there.

> It strikes me that for the following operations, which don't affect
> queries at all, we could use a SHARE UPDATE EXCLUSIVE, which is likely
> superior to SHARE for this purpose because it wouldn't lock out
> concurrent DML write operations:

Yes, we can also use SHARE UPDATE EXCLUSIVE for some of them. The use of
SHARE lock was specifically targeted at ADD FOREIGN KEY, to allow
multiple concurrent FKs. Not much use for production however, so SUE
looks better to me.

Not sure I agree with the "don't affect queries at all" bit....

I'll take my previous patch through to completion now, I'm funded to do
that work now. Sept commitfest though.

--
Simon Riggs www.2ndQuadrant.com

On Thu, Jul 8, 2010 at 2:16 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Wed, 2010-07-07 at 22:26 -0400, Robert Haas wrote:
>> Rereading the thread, I'm a bit confused by why we're proposing to use
>> a SHARE lock; it seems to me that a self-conflicting lock type would
>> simplify things.  There's a bunch of discussion on the thread about
>> how to handle pg_class updates atomically, but doesn't using a
>> self-conflicting lock type eliminate that problem?
>
> The use of the SHARE lock had nothing to do with the pg_class update
> requirement, so that suggestion won't help there.

Forgive me if I press on this just a bit further, but ISTM that an
atomic pg_class update functionality isn't intrinsically required,
because if it were the current code would need it. So what is
changing in this patch that makes it necessary when it isn't now?
ISTM it must be that the lock is weaker. What am I missing?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Thu, 2010-07-08 at 06:08 -0400, Robert Haas wrote:
> On Thu, Jul 8, 2010 at 2:16 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On Wed, 2010-07-07 at 22:26 -0400, Robert Haas wrote:
> >> Rereading the thread, I'm a bit confused by why we're proposing to use
> >> a SHARE lock; it seems to me that a self-conflicting lock type would
> >> simplify things. There's a bunch of discussion on the thread about
> >> how to handle pg_class updates atomically, but doesn't using a
> >> self-conflicting lock type eliminate that problem?
> >
> > The use of the SHARE lock had nothing to do with the pg_class update
> > requirement, so that suggestion won't help there.
>
> Forgive me if I press on this just a bit further, but ISTM that an
> atomic pg_class update functionality isn't intrinsically required,
> because if it were the current code would need it. So what is
> changing in this patch that makes it necessary when it isn't now?
> ISTM it must be that the lock is weaker. What am I missing?

Not sure I follow that logic. Discussion on the requirement is in the
archives. I don't wish to question that aspect myself.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

On Thu, Jul 8, 2010 at 5:09 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Thu, 2010-07-08 at 06:08 -0400, Robert Haas wrote:
>> On Thu, Jul 8, 2010 at 2:16 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> > On Wed, 2010-07-07 at 22:26 -0400, Robert Haas wrote:
>> >> Rereading the thread, I'm a bit confused by why we're proposing to use
>> >> a SHARE lock; it seems to me that a self-conflicting lock type would
>> >> simplify things.  There's a bunch of discussion on the thread about
>> >> how to handle pg_class updates atomically, but doesn't using a
>> >> self-conflicting lock type eliminate that problem?
>> >
>> > The use of the SHARE lock had nothing to do with the pg_class update
>> > requirement, so that suggestion won't help there.
>>
>> Forgive me if I press on this just a bit further, but ISTM that an
>> atomic pg_class update functionality isn't intrinsically required,
>> because if it were the current code would need it.  So what is
>> changing in this patch that makes it necessary when it isn't now?
>> ISTM it must be that the lock is weaker.  What am I missing?
>
> Not sure I follow that logic. Discussion on the requirement is in the
> archives. I don't wish to question that aspect myself.

The relevant link from the archives is here:

http://archives.postgresql.org/pgsql-hackers/2008-10/msg00242.php

Tom asked what happens when two transactions attempt to do concurrent
actions on the same table. Your response was that we should handle it
like CREATE INDEX, and handle the update of the pg_class row
non-transactionally. But of course, if you use a self-conflicting
lock at the relation level, then the relation locks conflict and you
never have to worry about how to update the pg_class entry in the face
of concurrent updates.

Which, come to think of it, is probably a good thing, because on
further reflection I'm pretty sure the proposed approach will fall
down completely for some of these operations. heap_inplace_update()
can only be used when (a) the new tuple is identical in size to the
old tuple, and (b) no action is required on rollback. That's fine for
updating things like relpages and reltuples (which are just hints
anyway) but it ain't gonna work for changing, say, reloptions, which
is variable-length. It's also not going to work for changing things
like attstorage, even though a change there can't affect the tuple
size, because modifying the tuple in place won't handle rollbacks
correctly.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Fri, 2010-07-09 at 13:04 -0400, Robert Haas wrote:

> Tom asked what happens when two transactions attempt to do concurrent
> actions on the same table. Your response was that we should handle it
> like CREATE INDEX, and handle the update of the pg_class row
> non-transactionally. But of course, if you use a self-conflicting
> lock at the relation level, then the relation locks conflict and you
> never have to worry about how to update the pg_class entry in the face
> of concurrent updates.

>From memory, Tom was also worried about the prospect of people updating
pg_class directly using SQL. That seems a rare, yet valid concern.

I've already agreed with your point that we should use SHARE UPDATE
EXCLUSIVE.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

On Fri, Jul 9, 2010 at 1:18 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Fri, 2010-07-09 at 13:04 -0400, Robert Haas wrote:
>> Tom asked what happens when two transactions attempt to do concurrent
>> actions on the same table.  Your response was that we should handle it
>> like CREATE INDEX, and handle the update of the pg_class row
>> non-transactionally.  But of course, if you use a self-conflicting
>> lock at the relation level, then the relation locks conflict and you
>> never have to worry about how to update the pg_class entry in the face
>> of concurrent updates.
>
> From memory, Tom was also worried about the prospect of people updating
> pg_class directly using SQL. That seems a rare, yet valid concern.

Yes, and it's another another reason why we shouldn't use
non-transactional updates.

http://archives.postgresql.org/pgsql-hackers/2008-11/msg00744.php

> I've already agreed with your point that we should use SHARE UPDATE
> EXCLUSIVE.

The point you seem to be missing is that once we make that decision,
we can throw all the heap_inplace_update() stuff out the window, and
the whole problem becomes much simpler.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas wrote:
> On Wed, Jul 7, 2010 at 9:04 PM, C?dric Villemain
> <cedric(dot)villemain(dot)debian(at)gmail(dot)com> wrote:
> >>> > I assume this did not get done for 9.0. ?Do we want a TODO item?
> >>>
> >>> Yes.
> >>
> >> Added:
> >>
> >> ? ? ? ?Reduce locking required for ALTER commands
> >
> > I just faced production issue where it is impossible to alter table to
> > adjust autovacuum settings in a pg8.4. (5K tps, 260M rows table, lock
> > too much)
> >
> > Can we add some mechanism to prevent that situation also in the TODO item ?
> >
> > (alternative is actualy to alter other tables and adjust the
> > postgresql.conf for biggest tables, but not an ideal solution anyway)
> >
> >>
> >> ? ? ? ? ? ?* http://archives.postgresql.org/pgsql-hackers/2009-08/msg00533.php
> >> ? ? ? ? ? ?* http://archives.postgresql.org/pgsql-hackers/2009-10/msg01083.php
> >> ? ? ? ? ? ?* http://archives.postgresql.org/pgsql-hackers/2010-01/msg02349.php
>
> Bruce, that last link is about something else completely. Here are
> some better ones:
>
> http://archives.postgresql.org/pgsql-hackers/2008-10/msg01248.php
> http://archives.postgresql.org/pgsql-hackers/2008-10/msg00242.php

Thanks, TODO updated.

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

+ None of us is going to be here forever. +

On Fri, 2010-07-09 at 15:03 -0400, Robert Haas wrote:
> On Fri, Jul 9, 2010 at 1:18 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On Fri, 2010-07-09 at 13:04 -0400, Robert Haas wrote:
> >> Tom asked what happens when two transactions attempt to do concurrent
> >> actions on the same table. Your response was that we should handle it
> >> like CREATE INDEX, and handle the update of the pg_class row
> >> non-transactionally. But of course, if you use a self-conflicting
> >> lock at the relation level, then the relation locks conflict and you
> >> never have to worry about how to update the pg_class entry in the face
> >> of concurrent updates.
> >
> > From memory, Tom was also worried about the prospect of people updating
> > pg_class directly using SQL. That seems a rare, yet valid concern.
>
> Yes, and it's another another reason why we shouldn't use
> non-transactional updates.
>
> http://archives.postgresql.org/pgsql-hackers/2008-11/msg00744.php
>
> > I've already agreed with your point that we should use SHARE UPDATE
> > EXCLUSIVE.
>
> The point you seem to be missing is that once we make that decision,
> we can throw all the heap_inplace_update() stuff out the window, and
> the whole problem becomes much simpler.

That is a point I missed.

Considering this further, it seems we have two conflicting requirements

1. ALTER TABLE ... ADD FOREIGN KEY needs a SHARE mode lock if we want to
run that concurrently with itself and CREATE INDEX operations during a
pg_restore. This was my original goal.

2. In most other cases, SHARE UPDATE EXCLUSIVE is the most useful lock,
especially during heavy operational use.

Since adding an FK requires adding triggers also that puts both of the
above in direct conflict.

ISTM that we should follow (2) and let (1) be added to the TODO for
later work, as an option. I'll followu up on (2).

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

On Thu, 2010-07-08 at 07:16 +0100, Simon Riggs wrote:

> I'll take my previous patch through to completion now

Patch to reduce lock levels for
ALTER TABLE
CREATE TRIGGER
CREATE RULE

I've completely re-analyzed the required lock levels for sub-commands,
so lock levels can now also be these, if appropriate.
ShareUpdateExclusiveLock - allows db reads and writes
ShareRowExclusiveLock - allows db reads only

When ALTER TABLE is specified with multiple subcommands the highest lock
level required by any subcommand is applied to the whole combined
command.

The lock levels are in many ways different from both my own earlier
patch and much of the discussion on this thread, which I have taken to
be general comments rather than considered thought.

Nothing much speculative here, so will commit in a few days barring
objections.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

Hi Simon,

Your patch implements part of a feature I desire greatly - thanks!

Some comments:

On Thursday 15 July 2010 11:24:27 Simon Riggs wrote:
>> ! LOCKMODE
>> ! AlterTableGreatestLockLevel(List *cmds)
>> ! {
>> ! ListCell *lcmd;
>> ! LOCKMODE lockmode = ShareUpdateExclusiveLock; /* default for compiler */
Actually its not only for the compiler, its necessary for correctness
as you omit the default at least in the AT_AddConstraint case.

>> !
>> ! foreach(lcmd, cmds)
>> ! {
>> ! AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
>> ! LOCKMODE cmd_lockmode = AccessExclusiveLock; /* default for compiler */
>> !
>> ! switch (cmd->subtype)
>> ! {
>> ! /*
>> ! * Need AccessExclusiveLock for these subcommands because they
>> ! * affect or potentially affect both read and write operations.
>> ! *
>> ! * New subcommand types should be added here by default.
>> ! */
>> ! case AT_AddColumn: /* may rewrite heap, in some cases and visible to SELECT */
>> ! case AT_DropColumn: /* change visible to SELECT */
>> ! case AT_AddColumnToView: /* CREATE VIEW */
>> ! case AT_AlterColumnType: /* must rewrite heap */
>> ! case AT_DropConstraint: /* as DROP INDEX */
>> ! case AT_AddOids:
>> ! case AT_DropOids: /* calls AT_DropColumn */
>> ! case AT_EnableAlwaysRule: /* as DROP INDEX */
>> ! case AT_EnableReplicaRule: /* as DROP INDEX */
>> ! case AT_EnableRule: /* as DROP INDEX */
>> ! case AT_DisableRule: /* as DROP INDEX */
>> ! case AT_ChangeOwner: /* change visible to SELECT */
>> ! case AT_SetTableSpace: /* must rewrite heap */
>> ! case AT_DropNotNull: /* may change some SQL plans */
>> ! case AT_SetNotNull:
>> ! cmd_lockmode = AccessExclusiveLock;
>> ! break;
>> !
>> ! /*
>> ! * These subcommands affect write operations only.
>> ! */
>> ! case AT_ColumnDefault:
>> ! case AT_ProcessedConstraint: /* becomes AT_AddConstraint */
>> ! case AT_AddConstraintRecurse: /* becomes AT_AddConstraint */
>> ! case AT_EnableTrig:
>> ! case AT_EnableAlwaysTrig:
>> ! case AT_EnableReplicaTrig:
>> ! case AT_EnableTrigAll:
>> ! case AT_EnableTrigUser:
>> ! case AT_DisableTrig:
>> ! case AT_DisableTrigAll:
>> ! case AT_DisableTrigUser:
>> ! case AT_AddIndex: /* from ADD CONSTRAINT */
>> ! cmd_lockmode = ShareRowExclusiveLock;
>> ! break;
>> !
>> ! case AT_AddConstraint:
>> ! if (IsA(cmd->def, Constraint))
>> ! {
>> ! Constraint *con = (Constraint *) cmd->def;
>> !
>> ! switch (con->contype)
>> ! {
>> ! case CONSTR_EXCLUSION:
>> ! case CONSTR_PRIMARY:
>> ! case CONSTR_UNIQUE:
>> ! /*
>> ! * Cases essentially the same as CREATE INDEX. We
>> ! * could reduce the lock strength to ShareLock if we
>> ! * can work out how to allow concurrent catalog updates.
>> ! */
>> ! cmd_lockmode = ShareRowExclusiveLock;
>> ! break;
>> ! case CONSTR_FOREIGN:
>> ! /*
>> ! * We add triggers to both tables when we add a
>> ! * Foreign Key, so the lock level must be at least
>> ! * as strong as CREATE TRIGGER.
>> ! */
>> ! cmd_lockmode = ShareRowExclusiveLock;
>> ! break;
>> !
>> ! default:
>> ! cmd_lockmode = ShareRowExclusiveLock;
>> ! }
>> ! }
>> ! break;

You argue above that you cant change SET [NOT] NULL to be less
restrictive because it might change plans - isnt that true for some of the
above cases as well?

For example UNIQUE/PRIMARY might make join removal possible - which could
only be valid after "invalid" tuples where deleted earlier in that
transaction. Another case which it influences are grouping plans...

So I think the only case where its actually possible to lower the
level is CONSTR_EXCLUSION and _FOREIGN.
The latter might get impossible soon by planner improvements (Peter's
functional dependencies patch for example).

Sorry, thats it for now...

Andres

On Friday 16 July 2010 20:41:44 Andres Freund wrote:
> >> ! */
> >> ! case AT_AddColumn: /* may
> >> rewrite heap, in some cases and visible to SELECT */ !
> >> case AT_DropColumn: /* change
> >> visible to SELECT */ ! case
> >> AT_AddColumnToView: /* CREATE VIEW */ ! case
> >> AT_AlterColumnType: /* must rewrite heap */ !
> >> case AT_DropConstraint: /* as DROP INDEX */
> >> ! case AT_AddOids:
> >> ! case AT_DropOids: /* calls
> >> AT_DropColumn */ ! case
> >> AT_EnableAlwaysRule: /* as DROP INDEX */ !
> >> case AT_EnableReplicaRule: /* as DROP INDEX */
> >> ! case AT_EnableRule: /* as DROP
> >> INDEX */
Another remark:

Imho it would be usefull to keep that list in same order as in the enum -
currently its hard to make sure no case is missing.

Andres

On Fri, 2010-07-16 at 20:41 +0200, Andres Freund wrote:

> You argue above that you cant change SET [NOT] NULL to be less
> restrictive because it might change plans - isnt that true for some of the
> above cases as well?
>
> For example UNIQUE/PRIMARY might make join removal possible - which could
> only be valid after "invalid" tuples where deleted earlier in that
> transaction. Another case which it influences are grouping plans...

This is only for adding a constraint, not removing it. Join removal
would be possible after the ALTER finishes, but won't change plans
already in progress. The idea is to minimise the impact, not maximise
the benefit of the newly added constraint; I don't think we should block
all queries just because a few might benefit.

> So I think the only case where its actually possible to lower the
> level is CONSTR_EXCLUSION and _FOREIGN.
> The latter might get impossible soon by planner improvements (Peter's
> functional dependencies patch for example).

Same, I don't see why it would block queries.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

On Fri, 2010-07-16 at 21:10 +0200, Andres Freund wrote:
> On Friday 16 July 2010 20:41:44 Andres Freund wrote:
> > >> ! */
> > >> ! case AT_AddColumn: /* may
> > >> rewrite heap, in some cases and visible to SELECT */ !
> > >> case AT_DropColumn: /* change
> > >> visible to SELECT */ ! case
> > >> AT_AddColumnToView: /* CREATE VIEW */ ! case
> > >> AT_AlterColumnType: /* must rewrite heap */ !
> > >> case AT_DropConstraint: /* as DROP INDEX */
> > >> ! case AT_AddOids:
> > >> ! case AT_DropOids: /* calls
> > >> AT_DropColumn */ ! case
> > >> AT_EnableAlwaysRule: /* as DROP INDEX */ !
> > >> case AT_EnableReplicaRule: /* as DROP INDEX */
> > >> ! case AT_EnableRule: /* as DROP
> > >> INDEX */
> Another remark:
>
> Imho it would be usefull to keep that list in same order as in the enum -
> currently its hard to make sure no case is missing.

Not really; the default case is to reject, so any full test suite will
pick that up.

The cases are ordered by resulting lock type, which seemed the best way
to check we didn't accidentally assign an incorrect lock type.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

On Friday 16 July 2010 21:12:33 Simon Riggs wrote:
> On Fri, 2010-07-16 at 20:41 +0200, Andres Freund wrote:
> > You argue above that you cant change SET [NOT] NULL to be less
> > restrictive because it might change plans - isnt that true for some of
> > the above cases as well?
> >
> > For example UNIQUE/PRIMARY might make join removal possible - which could
> > only be valid after "invalid" tuples where deleted earlier in that
> > transaction. Another case which it influences are grouping plans...
>
> This is only for adding a constraint, not removing it. Join removal
> would be possible after the ALTER finishes, but won't change plans
> already in progress. The idea is to minimise the impact, not maximise
> the benefit of the newly added constraint; I don't think we should block
> all queries just because a few might benefit.
Its not about benefit, its about correctness:

CREATE TABLE testsnap(t int);
INSERT INTO testsnap VALUES(1),(1);

T1:
test=# BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
BEGIN
Time: 0.853 ms
test=# explain analyze SELECT t1.* FROM testsnap t1 LEFT JOIN testsnap t2 USING(t); QUERY PLAN
---------------------------------------------------------------------------------------------------------------------
Merge Left Join (cost=337.49..781.49 rows=28800 width=4) (actual time=0.090..0.118 rows=4 loops=1)
Merge Cond: (t1.t = t2.t)
-> Sort (cost=168.75..174.75 rows=2400 width=4) (actual time=0.049..0.051 rows=2 loops=1)
Sort Key: t1.t
Sort Method: quicksort Memory: 25kB
-> Seq Scan on testsnap t1 (cost=0.00..34.00 rows=2400 width=4) (actual time=0.018..0.023 rows=2 loops=1)
-> Sort (cost=168.75..174.75 rows=2400 width=4) (actual time=0.026..0.033 rows=3 loops=1)
Sort Key: t2.t
Sort Method: quicksort Memory: 25kB
-> Seq Scan on testsnap t2 (cost=0.00..34.00 rows=2400 width=4) (actual time=0.005..0.009 rows=2 loops=1)
Total runtime: 0.279 ms
(11 rows)

T2:
test=# DELETE FROM testsnap;
DELETE 2
Time: 1.184 ms
test=# ALTER TABLE testsnap ADD CONSTRAINT t unique(t);
NOTICE: 00000: ALTER TABLE / ADD UNIQUE will create implicit index "t" for table "testsnap"
LOCATION: DefineIndex, indexcmds.c:471
ALTER TABLE
Time: 45.639 ms

T1:
Time: 1.948 ms
test=# explain analyze SELECT t1.* FROM testsnap t1 LEFT JOIN testsnap t2 USING(t);
QUERY PLAN
-----------------------------------------------------------------------------------------------------
Seq Scan on testsnap t1 (cost=0.00..1.02 rows=2 width=4) (actual time=0.013..0.016 rows=2 loops=1)
Total runtime: 0.081 ms
(2 rows)

Time: 2.004 ms
test=#

boom.

Andres

On Friday 16 July 2010 21:15:44 Simon Riggs wrote:
> On Fri, 2010-07-16 at 21:10 +0200, Andres Freund wrote:
> > On Friday 16 July 2010 20:41:44 Andres Freund wrote:
> > > >> ! */
> > > >> ! case AT_AddColumn: /* may
> > > >> rewrite heap, in some cases and visible to SELECT */ !
> > > >>
> > > >> case AT_DropColumn: /* change
> > > >>
> > > >> visible to SELECT */ ! case
> > > >> AT_AddColumnToView: /* CREATE VIEW */ !
> > > >> case AT_AlterColumnType: /* must rewrite heap */ !
> > > >>
> > > >> case AT_DropConstraint: /* as DROP INDEX
> > > >> */
> > > >>
> > > >> ! case AT_AddOids:
> > > >> ! case AT_DropOids: /*
> > > >> calls AT_DropColumn */ ! case
> > > >> AT_EnableAlwaysRule: /* as DROP INDEX */ !
> > > >>
> > > >> case AT_EnableReplicaRule: /* as DROP INDEX
> > > >> */
> > > >>
> > > >> ! case AT_EnableRule: /* as
> > > >> DROP INDEX */
> >
> > Another remark:
> >
> > Imho it would be usefull to keep that list in same order as in the enum -
> > currently its hard to make sure no case is missing.
>
> Not really; the default case is to reject, so any full test suite will
> pick that up.
>
> The cases are ordered by resulting lock type, which seemed the best way
> to check we didn't accidentally assign an incorrect lock type.
Well, I meant ordering it correctly inside the locktypes, sorry for the
inprecision.

Andres

On Fri, 2010-07-16 at 21:38 +0200, Andres Freund wrote:
> boom

Your test case would still occur in the case where the first query had
not been executed against the same table. So the test case illustrates a
failing of join removal, not of this patch.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

On Friday 16 July 2010 22:24:32 Simon Riggs wrote:
> On Fri, 2010-07-16 at 21:38 +0200, Andres Freund wrote:
> > boom
>
> Your test case would still occur in the case where the first query had
> not been executed against the same table. So the test case illustrates a
> failing of join removal, not of this patch.
Well, yes. Thats a well known (and documented) issue of pg's serialized
transactions - which you can protect against quite easily (see the trunctate
docs for example).
The difference is that I know of no sensible way you sensibly could protect
against such issues with the patch applied while its easy before(LOCK TABLE
... IN SHARE MODE for all used tables).
I know of several sites which have *long* running serialized transactions open
for analysis and I know there have been other cases of it.

Sure its not that bad, but at least it needs to get documented imho. Likely
others should chime in here ;-)

What could the join removal path (and similar places) *possibly* do against
such a case? Without stopping to use SnapshotNow I dont see any way :-(

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> What could the join removal path (and similar places) *possibly* do against
> such a case? Without stopping to use SnapshotNow I dont see any way :-(

But the planner, along with most of the rest of the backend, *does* use
SnapshotNow when examining the system catalogs.

I share your feeling of discomfort but so far I don't see a hole in
Simon's argument. Adding a constraint should never make a
previously-correct plan incorrect. Removing one is a very different
story, but he says he's not changing that case. (Disclaimer: I have
not read the patch.)

regards, tom lane

On Jul 16, 2010, at 6:01 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
>> What could the join removal path (and similar places) *possibly* do against
>> such a case? Without stopping to use SnapshotNow I dont see any way :-(
>
> But the planner, along with most of the rest of the backend, *does* use
> SnapshotNow when examining the system catalogs.
>
> I share your feeling of discomfort but so far I don't see a hole in
> Simon's argument. Adding a constraint should never make a
> previously-correct plan incorrect. Removing one is a very different
> story, but he says he's not changing that case. (Disclaimer: I have
> not read the patch.)

Perhaps we should start by deciding whether Andres' case is a bug in the first place, and then we can argue about whether it's a join-removal bug, a lock-weakening bug, or a preexisting bug.

...Robert

On Saturday 17 July 2010 01:53:24 Robert Haas wrote:
> On Jul 16, 2010, at 6:01 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Andres Freund <andres(at)anarazel(dot)de> writes:
> >> What could the join removal path (and similar places) *possibly* do
> >> against such a case? Without stopping to use SnapshotNow I dont see any
> >> way :-(
> >
> > But the planner, along with most of the rest of the backend, *does* use
> > SnapshotNow when examining the system catalogs.
> >
> > I share your feeling of discomfort but so far I don't see a hole in
> > Simon's argument.
Neither do I.

> > Adding a constraint should never make a
> > previously-correct plan incorrect. Removing one is a very different
> > story, but he says he's not changing that case. (Disclaimer: I have
> > not read the patch.)
>
> Perhaps we should start by deciding whether Andres' case is a bug in the
> first place, and then we can argue about whether it's a join-removal bug,
> a lock-weakening bug, or a preexisting bug.
The case where its possible to produce such a case *after* having used/locked
all participating relations is new I think.
Being able to create invalid results by doing DDL in another connection on
not-yet-used tables is at least as old as constraint exclusion. Its a bit
easier to work around, but thats it.

So I personally would not consider this patch as having a bug anymore
(thinking helps...).

Whether the general issue is a bug or a to-be-more-exhausitive-documented-
gotcha I have no idea. I know two people having hit it in production - I dont
think its a that common issue though. Starting with the fact that not that
many people use serializable.

Just to help me: The primary reasons for using SnapshotNow is speed and in
some cases correctness (referential integrity). Right? Any other reasons?

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> Just to help me: The primary reasons for using SnapshotNow is speed and in
> some cases correctness (referential integrity). Right? Any other reasons?

Well, the main point for system catalog accesses is that you *must* have
an up-to-date view of the table schemas. As an example, if someone just
added an index to an existing table, it would not do for an INSERT to
fail to update that index --- no matter whether it's from a serializable
transaction or not. So the DDL-executing transaction must hold a lock
that would block any operation that had better be able to see what it
did, and once another transaction has acquired the lock that lets it go
ahead with another operation, it had better see the results of the DDL
transaction.

However that argument mostly applies to what the executor does. A plan
could still be usable despite having been made against a now-obsolete
version of the table schema.

In the case at hand, I think most constraint-adding situations would
require at least ShareLock, because they had better block execution of
INSERT/UPDATE/DELETE operations that could fail to honor the constraint
if they didn't see it in the catalogs. But AFAICS, addition of a
constraint need not block SELECT, and it need not invalidate existing
plans.

CREATE INDEX uses ShareLock because it's okay to run multiple CREATE
INDEXes in parallel (thanks to some rather dodgy coding of the catalog
updates). For other cases of constraint additions, it might not be
practical to run two constraint additions in parallel. In that case we
could use ShareRowExclusive instead, which is self-exclusive but is not
any stronger than Share from the perspective of DML commands. Since
it's not, I'm unconvinced that it's worth taking any great pains to try
to make constraint additions run in parallel.

regards, tom lane

On Fri, 2010-07-16 at 23:03 +0200, Andres Freund wrote:

> Sure its not that bad, but at least it needs to get documented imho.
> Likely others should chime in here ;-)

Don't understand you. This is a clear bug in join removal, test case
attached, a minor rework of your original test case.

> What could the join removal path (and similar places) *possibly* do
> against such a case? Without stopping to use SnapshotNow I dont see
> any way :-(

The bug is caused by allowing join removal to work in serializable
transactions. The fix for 9.0 is easy and clear: disallow join removal
when planning a query as the second or subsequent query in a
serializable transaction.

A wider fix might be worth doing for 9.1, not sure.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

On Fri, 2010-07-16 at 20:45 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > Just to help me: The primary reasons for using SnapshotNow is speed and in
> > some cases correctness (referential integrity). Right? Any other reasons?
>
> Well, the main point for system catalog accesses is that you *must* have
> an up-to-date view of the table schemas. As an example, if someone just
> added an index to an existing table, it would not do for an INSERT to
> fail to update that index --- no matter whether it's from a serializable
> transaction or not. So the DDL-executing transaction must hold a lock
> that would block any operation that had better be able to see what it
> did, and once another transaction has acquired the lock that lets it go
> ahead with another operation, it had better see the results of the DDL
> transaction.
>
> However that argument mostly applies to what the executor does. A plan
> could still be usable despite having been made against a now-obsolete
> version of the table schema.
>
> In the case at hand, I think most constraint-adding situations would
> require at least ShareLock, because they had better block execution of
> INSERT/UPDATE/DELETE operations that could fail to honor the constraint
> if they didn't see it in the catalogs. But AFAICS, addition of a
> constraint need not block SELECT, and it need not invalidate existing
> plans.
>
> CREATE INDEX uses ShareLock because it's okay to run multiple CREATE
> INDEXes in parallel (thanks to some rather dodgy coding of the catalog
> updates). For other cases of constraint additions, it might not be
> practical to run two constraint additions in parallel. In that case we
> could use ShareRowExclusive instead, which is self-exclusive but is not
> any stronger than Share from the perspective of DML commands. Since
> it's not, I'm unconvinced that it's worth taking any great pains to try
> to make constraint additions run in parallel.

The patch follows all of the above exactly.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

On Saturday 17 July 2010 09:55:37 Simon Riggs wrote:
> On Fri, 2010-07-16 at 23:03 +0200, Andres Freund wrote:
> > Sure its not that bad, but at least it needs to get documented imho.
> > Likely others should chime in here ;-)
>
> Don't understand you. This is a clear bug in join removal, test case
> attached, a minor rework of your original test case.
As shown below the same issue exists in other codepaths that we cant easily fix
in a stable release :-( - so I think documenting it is the only viable action
for the back-branches.

> > What could the join removal path (and similar places) *possibly* do
> > against such a case? Without stopping to use SnapshotNow I dont see
> > any way :-(
> The bug is caused by allowing join removal to work in serializable
> transactions. The fix for 9.0 is easy and clear: disallow join removal
> when planning a query as the second or subsequent query in a
> serializable transaction.
>
> A wider fix might be worth doing for 9.1, not sure.

Unfortunately the same issue exists with constraint exclusion - and we can
hardly disable that for serializable transactions...

CREATE TABLE testconstr(data int);
INSERT INTO testconstr VALUES(1),(10);

T1:
test=# explain analyze SELECT * FROM testconstr WHERE data > 5;
QUERY PLAN
-------------------------------------------------------------------------------------------------------
Seq Scan on testconstr (cost=0.00..40.00 rows=800 width=4) (actual
time=0.029..0.032 rows=1 loops=1)
Filter: (data > 5)
Total runtime: 0.097 ms
(3 rows)

test=# BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
BEGIN

--make sure we do have a snapshot
test=# SELECT * FROM pg_class WHERE 0 = 1

T2:
DELETE FROM testconstr WHERE data >= 5;
ALTER TABLE testconstr ADD CONSTRAINT t CHECK(data < 5);

T1:
test=# explain analyze SELECT * FROM testconstr WHERE data > 5;
QUERY PLAN
------------------------------------------------------------------------------------
Result (cost=0.00..0.01 rows=1 width=0) (actual time=0.003..0.003 rows=0
loops=1)
One-Time Filter: false
Total runtime: 0.045 ms
(3 rows)

test=# SET constraint_exclusion = false;
SET
test=# explain analyze SELECT * FROM testconstr WHERE data > 5;
QUERY PLAN
-------------------------------------------------------------------------------------------------------
Seq Scan on testconstr (cost=0.00..40.00 rows=800 width=4) (actual
time=0.030..0.033 rows=1 loops=1)
Filter: (data > 5)
Total runtime: 0.099 ms
(3 rows)

Thats seems to be an issue that you realistically can hit in production...

I think the same problem exists with inheritance planning - i.e. a child table
added to a relation in T1 while T2 already holds a snapshot but hasnt used
that specific table was created will see the new child. Thats less severe but
still annoying.

Beside using an actual Snapshot in portions of the planner (i.e. stats should
continue using SnapshotNow) I dont really see a fix here.

Andres

Andres

On Sun, 2010-07-18 at 17:28 +0200, Andres Freund wrote:

> Unfortunately the same issue exists with constraint exclusion - and we
> can hardly disable that for serializable transactions...

Then I think the fix is to look at the xmin values on all of the tables
used during planning and ensure that we only use constraint-based
optimisations in a serializable transaction when our top xmin is later
than the last DDL change (via its xmin).

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

On Sunday 18 July 2010 18:02:26 Simon Riggs wrote:
> On Sun, 2010-07-18 at 17:28 +0200, Andres Freund wrote:
> > Unfortunately the same issue exists with constraint exclusion - and we
> > can hardly disable that for serializable transactions...
>
> Then I think the fix is to look at the xmin values on all of the tables
> used during planning and ensure that we only use constraint-based
> optimisations in a serializable transaction when our top xmin is later
> than the last DDL change (via its xmin).
Why not just use a the normal snapshot at that point? Any older constraints
should be just as valid for the tuples visible for the to-be-planned query.
I also think that would lay groundwork for reducing lock-levels further in the
future.

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> On Sunday 18 July 2010 18:02:26 Simon Riggs wrote:
>> Then I think the fix is to look at the xmin values on all of the tables
>> used during planning and ensure that we only use constraint-based
>> optimisations in a serializable transaction when our top xmin is later
>> than the last DDL change (via its xmin).

> Why not just use a the normal snapshot at that point?

There isn't a "normal snapshot" that the planner should be relying on.
It doesn't know what snap the resulting plan will be used with.

I'm unconvinced that this is a problem worth worrying about, but if it
is then Simon's probably got the right idea: check the xmin of a
pg_constraint row before depending on it for planning. Compare the
handling of indexes made with CREATE INDEX CONCURRENTLY.

regards, tom lane

On Sunday 18 July 2010 19:20:25 Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On Sunday 18 July 2010 18:02:26 Simon Riggs wrote:
> >> Then I think the fix is to look at the xmin values on all of the tables
> >> used during planning and ensure that we only use constraint-based
> >> optimisations in a serializable transaction when our top xmin is later
> >> than the last DDL change (via its xmin).
> >
> > Why not just use a the normal snapshot at that point?
> There isn't a "normal snapshot" that the planner should be relying on.
> It doesn't know what snap the resulting plan will be used with.
Ok, I will write more stupid stuff in the next paragraph. Feel free to ignore.

What I meant was to use
* the transactions snapshot if we are in a transaction while planning
* SnapshotNow otherwise (not sure if thats a situation really existing - I yet
have no idea how such utitlity statements are handled snapshot-wise)

The errors I described shouldn't matter for an already existing plan. Also the
problem with a stale plan is already existing (only slightly aggravated due to
the change) and should be handled via plan invalidation. Right?

Phantasizing:
If you continued with that you even could allow read only access to tables
during ALTER TABLE et al. if the actual unlinking of the old filerelnode would
get moved to the checkpoint or similar...

> I'm unconvinced that this is a problem worth worrying about, but if it
> is then Simon's probably got the right idea: check the xmin of a
> pg_constraint row before depending on it for planning. Compare the
> handling of indexes made with CREATE INDEX CONCURRENTLY.
I am happy enough to write a docpatch for those issues and leave it there.

Andres

On Sun, Jul 18, 2010 at 1:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
>> On Sunday 18 July 2010 18:02:26 Simon Riggs wrote:
>>> Then I think the fix is to look at the xmin values on all of the tables
>>> used during planning and ensure that we only use constraint-based
>>> optimisations in a serializable transaction when our top xmin is later
>>> than the last DDL change (via its xmin).
>
>> Why not just use a the normal snapshot at that point?
>
> There isn't a "normal snapshot" that the planner should be relying on.
> It doesn't know what snap the resulting plan will be used with.
>
> I'm unconvinced that this is a problem worth worrying about, but if it
> is then Simon's probably got the right idea: check the xmin of a
> pg_constraint row before depending on it for planning.  Compare the
> handling of indexes made with CREATE INDEX CONCURRENTLY.

It generally seems like a Bad Thing to use one snapshot for planning
and another snapshot for execution. For example, if one transaction
(ostensibly serializable) runs a query twice in a row and in the mean
time some other transaction redefines a function used by that query,
the two runs will return different results, which is inconsistent with
any serial order of execution of those transactions. But it seems
that it's far from clear what to do about it, and it's not the job of
this patch to fix it anyway.

Regarding the actual patch, it looks mostly good. Questions:

1. Why in rewriteSupport.c are we adding a call to
heap_inplace_update() in some situations? Doesn't seem like this is
something we should need or want to be monkeying with.

2. Instead of AlterTableGreatestLockLevel(), how about
AlterTableGetLockLevel()? Yeah, it's going to be the highest lock
level required by any subcommand, but it seems mildly overspecified.
I don't feel strongly about this one, though, if someone has a strong
contrary opinion...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Sun, 2010-07-18 at 22:47 -0400, Robert Haas wrote:
> But it seems
> that it's far from clear what to do about it, and it's not the job of
> this patch to fix it anyway.

Agreed.

> Regarding the actual patch, it looks mostly good. Questions:
>
> 1. Why in rewriteSupport.c are we adding a call to
> heap_inplace_update() in some situations? Doesn't seem like this is
> something we should need or want to be monkeying with.

Hmm, yes, that looks like a hangover. Will change. No others similar.

> 2. Instead of AlterTableGreatestLockLevel(), how about
> AlterTableGetLockLevel()? Yeah, it's going to be the highest lock
> level required by any subcommand, but it seems mildly overspecified.
> I don't feel strongly about this one, though, if someone has a strong
> contrary opinion...

I felt it indicated the process it's using. Happy to change.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Jul 19, 2010 at 2:46 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Sun, 2010-07-18 at 22:47 -0400, Robert Haas wrote:
>>  But it seems
>> that it's far from clear what to do about it, and it's not the job of
>> this patch to fix it anyway.
>
> Agreed.
>
>> Regarding the actual patch, it looks mostly good.  Questions:
>>
>> 1. Why in rewriteSupport.c are we adding a call to
>> heap_inplace_update() in some situations?  Doesn't seem like this is
>> something we should need or want to be monkeying with.
>
> Hmm, yes, that looks like a hangover. Will change. No others similar.
>
>> 2. Instead of AlterTableGreatestLockLevel(), how about
>> AlterTableGetLockLevel()?  Yeah, it's going to be the highest lock
>> level required by any subcommand, but it seems mildly overspecified.
>> I don't feel strongly about this one, though, if someone has a strong
>> contrary opinion...
>
> I felt it indicated the process it's using. Happy to change.

Cool. I think with those two changes it's time to commit this. It's
possible there's some case we've overlooked, but I think that we've
been over this fairly thoroughly, so hopefully not. Anyway, we're
doing this at the beginning of the 9.1 cycle rather than the end, so
there's hopefully time for any lingering bugs to be found...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On tor, 2010-07-15 at 10:24 +0100, Simon Riggs wrote:
> Patch to reduce lock levels for
> ALTER TABLE
> CREATE TRIGGER
> CREATE RULE

Tried this out, but $subject is still the case. The problem is that
ATRewriteCatalogs() calls AlterTableCreateToastTable() based on what it
thinks the subcommands are, and AlterTableCreateToastTable() takes an
AccessExclusiveLock.

This could possibly be addressed by moving AT_SetStatistics to
AT_PASS_MISC in order to avoid the TOAST table call.

In a related matter, assigning ShareUpdateExclusiveLock to AT_SetStorage
doesn't work either, because the TOAST table call needs to be done in
that case.

Perhaps this logic needs to be refactored a bit more so that there
aren't any inconsistencies between the lock modes and the "passes",
which could lead to false expectations and deadlocks.

On Wed, 2010-07-28 at 15:24 +0300, Peter Eisentraut wrote:
> On tor, 2010-07-15 at 10:24 +0100, Simon Riggs wrote:
> > Patch to reduce lock levels for
> > ALTER TABLE
> > CREATE TRIGGER
> > CREATE RULE
>
> Tried this out, but $subject is still the case. The problem is that
> ATRewriteCatalogs() calls AlterTableCreateToastTable() based on what it
> thinks the subcommands are, and AlterTableCreateToastTable() takes an
> AccessExclusiveLock.
>
> This could possibly be addressed by moving AT_SetStatistics to
> AT_PASS_MISC in order to avoid the TOAST table call.
>
> In a related matter, assigning ShareUpdateExclusiveLock to AT_SetStorage
> doesn't work either, because the TOAST table call needs to be done in
> that case.
>
> Perhaps this logic needs to be refactored a bit more so that there
> aren't any inconsistencies between the lock modes and the "passes",
> which could lead to false expectations and deadlocks.

Thanks for your comments. Will reconsider and update.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

On Wed, 2010-07-28 at 15:24 +0300, Peter Eisentraut wrote:
> On tor, 2010-07-15 at 10:24 +0100, Simon Riggs wrote:
> > Patch to reduce lock levels for
> > ALTER TABLE
> > CREATE TRIGGER
> > CREATE RULE
>
> Tried this out, but $subject is still the case. The problem is that
> ATRewriteCatalogs() calls AlterTableCreateToastTable() based on what it
> thinks the subcommands are, and AlterTableCreateToastTable() takes an
> AccessExclusiveLock.
>
> This could possibly be addressed by moving AT_SetStatistics to
> AT_PASS_MISC in order to avoid the TOAST table call.
>
> In a related matter, assigning ShareUpdateExclusiveLock to AT_SetStorage
> doesn't work either, because the TOAST table call needs to be done in
> that case.
>
> Perhaps this logic needs to be refactored a bit more so that there
> aren't any inconsistencies between the lock modes and the "passes",
> which could lead to false expectations and deadlocks.

Changes as suggested, plus tests to confirm lock levels for
ShareUpdateExclusiveLock changes. Will commit soon, if no objection.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services