Thread

Commits

Make ALTER SEQUENCE, including RESTART, fully transactional.
- 3d79013b970d 10.0 landed
Modify sequence catalog tuple before invoking post alter hook.
- 665104557fdc 10.0 landed
Use weaker locks when updating pg_subscription_rel
- 521fd4795e3e 10.0 cited
Add pg_sequence system catalog
- 1753b1b02703 10.0 cited
Modify sequence state storage to eliminate dangling-pointer problem
- a2597ef17958 7.3.1 cited

Concurrent ALTER SEQUENCE RESTART Regression

Jason Petersen <jason@citusdata.com> — 2017-04-24T19:52:33Z

I recently found a behavior regression for ALTER SEQUENCE.

Repro. Steps
============

  1. Create a new sequence: CREATE SEQUENCE my_seq;
  2. Start this loop twice in different shells:
       while true; do psql -1Xtc 'ALTER SEQUENCE my_seq RESTART 1'; done

Expected (pre-10) Behavior
==========================

Each loop should repeatedly succeed and simply print ALTER SEQUENCE over and over.

Actual (PG 10) Behavior
=======================

The output stream is punctuated by occasional "ERROR:  tuple concurrently updated" messages.

Summary
=======

While I understand the above workload is nonsensical, given the non-transactional behavior of ALTER SEQUENCE statements, previous PostgreSQL versions did not produce an error. It is likely applications have been coded with that assumption and will not deal well with the new behavior.

Having poked around the code a bit, I see the functions to access for sequence state have changed; I’m assuming this is an unintended side-effect of that change.

I haven’t worked up a patch myself, but I have some hope someone more familiar with the underlying changes could make quick work of this.

--
Jason Petersen
Software Engineer | Citus Data
303.736.9255
jason@citusdata.com

Re: Concurrent ALTER SEQUENCE RESTART Regression

Michael Paquier <michael.paquier@gmail.com> — 2017-04-25T01:53:39Z

On Tue, Apr 25, 2017 at 4:52 AM, Jason Petersen <jason@citusdata.com> wrote:
> While I understand the above workload is nonsensical, given the non-transactional behavior of ALTER SEQUENCE statements, previous PostgreSQL versions did not produce an error. It is likely applications have been coded with that assumption and will not deal well with the new behavior.

Yes, that's a bug.

> Having poked around the code a bit, I see the functions to access for sequence state have changed; I’m assuming this is an unintended side-effect of that change.
>
> I haven’t worked up a patch myself, but I have some hope someone more familiar with the underlying changes could make quick work of this.

I am working on it, will send a patch soon. My first intuition is that
this is some wild lock issue. I am checking as well other code paths.
An open item has been added for the time being.
-- 
Michael

Re: Concurrent ALTER SEQUENCE RESTART Regression

Michael Paquier <michael.paquier@gmail.com> — 2017-04-25T04:26:14Z

On Tue, Apr 25, 2017 at 10:53 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Apr 25, 2017 at 4:52 AM, Jason Petersen <jason@citusdata.com> wrote:
>> While I understand the above workload is nonsensical, given the non-transactional behavior of ALTER SEQUENCE statements, previous PostgreSQL versions did not produce an error. It is likely applications have been coded with that assumption and will not deal well with the new behavior.
>
> Yes, that's a bug.
>
>> Having poked around the code a bit, I see the functions to access for sequence state have changed; I’m assuming this is an unintended side-effect of that change.
>>
>> I haven’t worked up a patch myself, but I have some hope someone more familiar with the underlying changes could make quick work of this.
>
> I am working on it, will send a patch soon. My first intuition is that
> this is some wild lock issue. I am checking as well other code paths.
> An open item has been added for the time being.

So things are broken for sequences since commit 1753b1b0 (adding Peter
in CC) that has changed the way sequence metadata is handled. The
failure happens in CatalogTupleUpdate() which uses
simple_heap_update() that caller can only use if updates are
concurrent safe. But since 1753b1b0 that is not true as the sequence
is locked with AccessShareLock. The correct answer is to use a
stronger lock, but not something that would block attempts to read
next sequence values as it is assumed that ALTER SEQUENCE should be
non-disruptive. Hence I think that ShareUpdateExclusiveLock is a good
match what what we are looking for here: protect concurrent updates of
the sequence metadata for other ALTER SEQUENCE commands but not block
other transactions reading this data.

Attached is a patch to address the issue.

And looking at things deeper... I am seeing at least one other ALTER
command that is not concurrent safe... I'll keep that for another
thread because it is an older bug.
-- 
Michael

Re: Concurrent ALTER SEQUENCE RESTART Regression

Jason Petersen <jason@citusdata.com> — 2017-04-25T05:18:47Z

FWIW that was my gut read as well; take a slightly more restrictive lock,
possibly blocking other ALTERs (unsure of prior behavior) to avoid ERROR
but not at the cost of blocking readers. This seems about right to me.

Haven't reported a bug before; what's next? Get a reviewer?

--
*Jason Petersen*
Software Engineer | Citus Data
303.736.9255
jason@citusdata.com

On Apr 24, 2017, at 10:26 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Tue, Apr 25, 2017 at 10:53 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Tue, Apr 25, 2017 at 4:52 AM, Jason Petersen <jason@citusdata.com> wrote:

While I understand the above workload is nonsensical, given the
non-transactional behavior of ALTER SEQUENCE statements, previous
PostgreSQL versions did not produce an error. It is likely applications
have been coded with that assumption and will not deal well with the new
behavior.


Yes, that's a bug.


Having poked around the code a bit, I see the functions to access for
sequence state have changed; I’m assuming this is an unintended side-effect
of that change.


I haven’t worked up a patch myself, but I have some hope someone more
familiar with the underlying changes could make quick work of this.


I am working on it, will send a patch soon. My first intuition is that

this is some wild lock issue. I am checking as well other code paths.

An open item has been added for the time being.


So things are broken for sequences since commit 1753b1b0 (adding Peter
in CC) that has changed the way sequence metadata is handled. The
failure happens in CatalogTupleUpdate() which uses
simple_heap_update() that caller can only use if updates are
concurrent safe. But since 1753b1b0 that is not true as the sequence
is locked with AccessShareLock. The correct answer is to use a
stronger lock, but not something that would block attempts to read
next sequence values as it is assumed that ALTER SEQUENCE should be
non-disruptive. Hence I think that ShareUpdateExclusiveLock is a good
match what what we are looking for here: protect concurrent updates of
the sequence metadata for other ALTER SEQUENCE commands but not block
other transactions reading this data.

Attached is a patch to address the issue.

And looking at things deeper... I am seeing at least one other ALTER
command that is not concurrent safe... I'll keep that for another
thread because it is an older bug.
-- 
Michael

<alter-seq-lock.patch>

Re: Concurrent ALTER SEQUENCE RESTART Regression

Michael Paquier <michael.paquier@gmail.com> — 2017-04-25T05:43:58Z

On Tue, Apr 25, 2017 at 2:18 PM, Jason Petersen <jason@citusdata.com> wrote:
> FWIW that was my gut read as well; take a slightly more restrictive lock,
> possibly blocking other ALTERs (unsure of prior behavior) to avoid ERROR but
> not at the cost of blocking readers. This seems about right to me.
>
> Haven't reported a bug before; what's next? Get a reviewer?

We have done everything that can be done, and for sure more review is
welcome. I have added an open item here:
https://wiki.postgresql.org/index.php?title=PostgreSQL_10_Open_Items
And that's a commit of Peter, who is also the author, now in CC, which
is causing the regression.
-- 
Michael

Re: Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-04-25T19:17:39Z

On 4/25/17 00:26, Michael Paquier wrote:
> So things are broken for sequences since commit 1753b1b0 (adding Peter
> in CC) that has changed the way sequence metadata is handled. The
> failure happens in CatalogTupleUpdate() which uses
> simple_heap_update() that caller can only use if updates are
> concurrent safe. But since 1753b1b0 that is not true as the sequence
> is locked with AccessShareLock.

I think you are confusing locking the sequence versus locking the
pg_sequence catalog.  The error is coming from CatalogTupleUpdate() on
pg_sequence, which is locked using RowExclusiveLock, which is what we
use for most DDL commands doing catalog changes.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Concurrent ALTER SEQUENCE RESTART Regression

Michael Paquier <michael.paquier@gmail.com> — 2017-04-26T01:24:40Z

On Wed, Apr 26, 2017 at 4:17 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 4/25/17 00:26, Michael Paquier wrote:
>> So things are broken for sequences since commit 1753b1b0 (adding Peter
>> in CC) that has changed the way sequence metadata is handled. The
>> failure happens in CatalogTupleUpdate() which uses
>> simple_heap_update() that caller can only use if updates are
>> concurrent safe. But since 1753b1b0 that is not true as the sequence
>> is locked with AccessShareLock.
>
> I think you are confusing locking the sequence versus locking the
> pg_sequence catalog.  The error is coming from CatalogTupleUpdate() on
> pg_sequence, which is locked using RowExclusiveLock, which is what we
> use for most DDL commands doing catalog changes.

Yes, and that's fine, taking a stronger lock on pg_sequence would be
disruptive for other sessions, including the ones updating pg_sequence
for different sequences. The point I am trying to make here is that
the code path updating pg_sequence should make sure that the
underlying object is properly locked first, so as the update is
concurrent-safe because this uses simple_heap_update that assumes that
the operation will be concurrent-safe. For example, take tablecmds.c,
we make sure that any relation ALTER TABLE works on gets a proper lock
with relation_open first, in what sequences would be different now
that they have their own catalog?
-- 
Michael

Re: Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-04-26T16:15:53Z

On 4/25/17 21:24, Michael Paquier wrote:
> Yes, and that's fine, taking a stronger lock on pg_sequence would be
> disruptive for other sessions, including the ones updating pg_sequence
> for different sequences. The point I am trying to make here is that
> the code path updating pg_sequence should make sure that the
> underlying object is properly locked first, so as the update is
> concurrent-safe because this uses simple_heap_update that assumes that
> the operation will be concurrent-safe. For example, take tablecmds.c,
> we make sure that any relation ALTER TABLE works on gets a proper lock
> with relation_open first, in what sequences would be different now
> that they have their own catalog?

Pretty much everything other than tables is a counterexample.

git grep RowExclusiveLock src/backend/commands/*.c

Only tables have an underlying object to lock.  Most other DDL commands
don't have anything else to lock and run DDL under RowExclusiveLock.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-04-26T17:48:13Z

On 2017-04-26 12:15:53 -0400, Peter Eisentraut wrote:
> On 4/25/17 21:24, Michael Paquier wrote:
> > Yes, and that's fine, taking a stronger lock on pg_sequence would be
> > disruptive for other sessions, including the ones updating pg_sequence
> > for different sequences. The point I am trying to make here is that
> > the code path updating pg_sequence should make sure that the
> > underlying object is properly locked first, so as the update is
> > concurrent-safe because this uses simple_heap_update that assumes that
> > the operation will be concurrent-safe. For example, take tablecmds.c,
> > we make sure that any relation ALTER TABLE works on gets a proper lock
> > with relation_open first, in what sequences would be different now
> > that they have their own catalog?
> 
> Pretty much everything other than tables is a counterexample.
> 
> git grep RowExclusiveLock src/backend/commands/*.c
> 
> Only tables have an underlying object to lock.  Most other DDL commands
> don't have anything else to lock and run DDL under RowExclusiveLock.

What's your proposed fix?

- Andres

Re: Concurrent ALTER SEQUENCE RESTART Regression

Michael Paquier <michael.paquier@gmail.com> — 2017-04-26T23:12:30Z

On Thu, Apr 27, 2017 at 2:48 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2017-04-26 12:15:53 -0400, Peter Eisentraut wrote:
>> On 4/25/17 21:24, Michael Paquier wrote:
>> > Yes, and that's fine, taking a stronger lock on pg_sequence would be
>> > disruptive for other sessions, including the ones updating pg_sequence
>> > for different sequences. The point I am trying to make here is that
>> > the code path updating pg_sequence should make sure that the
>> > underlying object is properly locked first, so as the update is
>> > concurrent-safe because this uses simple_heap_update that assumes that
>> > the operation will be concurrent-safe. For example, take tablecmds.c,
>> > we make sure that any relation ALTER TABLE works on gets a proper lock
>> > with relation_open first, in what sequences would be different now
>> > that they have their own catalog?
>>
>> Pretty much everything other than tables is a counterexample.
>>
>> git grep RowExclusiveLock src/backend/commands/*.c
>>
>> Only tables have an underlying object to lock.  Most other DDL commands
>> don't have anything else to lock and run DDL under RowExclusiveLock.

Well, there are more DDL commands where it is possible to see "tuple
concurrently updated" easily, an example is ALTER ROLE. So nothing is
concurrent-proof with this code and I think needs a careful lookup
because this error should never be something that is user-visible.

> What's your proposed fix?

Extra opinions/ideas are welcome.
-- 
Michael

Re: Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-04-27T01:07:20Z

On 4/26/17 19:12, Michael Paquier wrote:
> Well, there are more DDL commands where it is possible to see "tuple
> concurrently updated" easily, an example is ALTER ROLE. So nothing is
> concurrent-proof with this code and I think needs a careful lookup
> because this error should never be something that is user-visible.

Yeah, it's been like this since time immemorial, so I don't think we
need a last minute fix now.

One thing we could do, since all catalog updates now go through
CatalogTupleUpdate(), is not use simple_heap_update() there but do the
heap_update() directly and provide a better user-facing error message.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-04-27T01:12:58Z

On 2017-04-26 21:07:20 -0400, Peter Eisentraut wrote:
> On 4/26/17 19:12, Michael Paquier wrote:
> > Well, there are more DDL commands where it is possible to see "tuple
> > concurrently updated" easily, an example is ALTER ROLE. So nothing is
> > concurrent-proof with this code and I think needs a careful lookup
> > because this error should never be something that is user-visible.
> 
> Yeah, it's been like this since time immemorial, so I don't think we
> need a last minute fix now.

Uh.  Until v10 this worked in a somewhat weird way, but it worked.

> One thing we could do, since all catalog updates now go through
> CatalogTupleUpdate(), is not use simple_heap_update() there but do the
> heap_update() directly and provide a better user-facing error message.

I think it's unacceptable to regress with an error message here.  I've
seen sequence DDL being used while concurrent DML was onging in a number
of production use cases, and just starting to error out instead of
properly blocking doesn't seem acceptable to me.

- Andres

Re: Concurrent ALTER SEQUENCE RESTART Regression

Michael Paquier <michael.paquier@gmail.com> — 2017-04-27T01:51:39Z

On Thu, Apr 27, 2017 at 10:12 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2017-04-26 21:07:20 -0400, Peter Eisentraut wrote:
>> One thing we could do, since all catalog updates now go through
>> CatalogTupleUpdate(), is not use simple_heap_update() there but do the
>> heap_update() directly and provide a better user-facing error message.
>
> I think it's unacceptable to regress with an error message here.  I've
> seen sequence DDL being used while concurrent DML was onging in a number
> of production use cases, and just starting to error out instead of
> properly blocking doesn't seem acceptable to me.

+1'ing on this argument.
-- 
Michael

Re: Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-04-27T02:07:03Z

On 4/26/17 21:12, Andres Freund wrote:
> I think it's unacceptable to regress with an error message here.  I've
> seen sequence DDL being used while concurrent DML was onging in a number
> of production use cases, and just starting to error out instead of
> properly blocking doesn't seem acceptable to me.

It's not clear to me what the use case is here that we are optimizing
for.  The best solution would depend on that.  Running concurrent ALTER
SEQUENCE in a tight loop is probably not it. ;-)

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-04-27T05:52:11Z

On 2017-04-26 22:07:03 -0400, Peter Eisentraut wrote:
> On 4/26/17 21:12, Andres Freund wrote:
> > I think it's unacceptable to regress with an error message here.  I've
> > seen sequence DDL being used while concurrent DML was onging in a number
> > of production use cases, and just starting to error out instead of
> > properly blocking doesn't seem acceptable to me.
>
> It's not clear to me what the use case is here that we are optimizing
> for.  The best solution would depend on that.  Running concurrent ALTER
> SEQUENCE in a tight loop is probably not it. ;-)

I don't think the use-case actually matters all that much.  It's bad
enough that you can get "concurrent update" errors for some forms of DDL
already, and there's plenty enough complaints about it.  Adding new
forms of that is completely unacceptable.  Just properly locking the
object while doing DDL - which'll be a behavioural change too - seems
like the minimal thing to do.

As far as I can see the issue here is that the locking in
AlterSequence() is plainly broken:

ObjectAddress
AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
{
...
	relid = RangeVarGetRelid(stmt->sequence, AccessShareLock, stmt->missing_ok);
...
	/* lock page' buffer and read tuple into new sequence structure */
	seqdata = read_seq_tuple(seqrel, &buf, &seqdatatuple);
...
	/* Now okay to update the on-disk tuple */
	START_CRIT_SECTION();
..
		XLogRegisterBuffer(0, buf, REGBUF_WILL_INIT);
...
		recptr = XLogInsert(RM_SEQ_ID, XLOG_SEQ_LOG);
...
	END_CRIT_SECTION();
...
	UnlockReleaseBuffer(buf);
...
	CatalogTupleUpdate(rel, &tuple->t_self, tuple);
..

In contrast to <v10, the actual change of the tuple is *not* happening
with the page lock held.  But now we do log XLOG_SEQ_LOG, then unlock
the buffer, and then do a CatalogTupleUpdate().  How is that correct?
And if so, why isn't there a honking large comment explaining why it is?

Imagine two of these running concurrently - you might end up with
A:XLogInsert B:XLogInsert B:CatalogTupleUpdate A:CatalogTupleUpdate

that can't be right, no?

- Andres

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-04-27T05:58:06Z

On 2017-04-26 22:07:03 -0400, Peter Eisentraut wrote:
> On 4/26/17 21:12, Andres Freund wrote:
> > I think it's unacceptable to regress with an error message here.  I've
> > seen sequence DDL being used while concurrent DML was onging in a number
> > of production use cases, and just starting to error out instead of
> > properly blocking doesn't seem acceptable to me.
> 
> It's not clear to me what the use case is here that we are optimizing
> for.  The best solution would depend on that.  Running concurrent ALTER
> SEQUENCE in a tight loop is probably not it. ;-)

Oh, and there's absolutely no need for a loop or anything:

A: CREATE SEQUENCE someseq
A: BEGIN;
A: ALTER SEQUENCE someseq RESTART ;
B: ALTER SEQUENCE someseq RESTART ;
A: COMMIT;
B: ERROR:  XX000: tuple concurrently updated

- Andres

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-04-27T06:23:04Z

On 2017-04-26 22:58:06 -0700, Andres Freund wrote:
> On 2017-04-26 22:07:03 -0400, Peter Eisentraut wrote:
> > On 4/26/17 21:12, Andres Freund wrote:
> > > I think it's unacceptable to regress with an error message here.  I've
> > > seen sequence DDL being used while concurrent DML was onging in a number
> > > of production use cases, and just starting to error out instead of
> > > properly blocking doesn't seem acceptable to me.
> >
> > It's not clear to me what the use case is here that we are optimizing
> > for.  The best solution would depend on that.  Running concurrent ALTER
> > SEQUENCE in a tight loop is probably not it. ;-)
>
> Oh, and there's absolutely no need for a loop or anything:
>
> A: CREATE SEQUENCE someseq
> A: BEGIN;
> A: ALTER SEQUENCE someseq RESTART ;
> B: ALTER SEQUENCE someseq RESTART ;
> A: COMMIT;
> B: ERROR:  XX000: tuple concurrently updated

More fun:

A: CREATE SEQUENCE someseq;
A: BEGIN;
A: ALTER SEQUENCE someseq MAXVALUE 10;
B: SELECT nextval('someseq') FROM generate_series(1, 1000);

=> ignores maxvalue

Re: Concurrent ALTER SEQUENCE RESTART Regression

Michael Paquier <michael.paquier@gmail.com> — 2017-04-27T07:06:55Z

On Thu, Apr 27, 2017 at 3:23 PM, Andres Freund <andres@anarazel.de> wrote:
> More fun:
>
> A: CREATE SEQUENCE someseq;
> A: BEGIN;
> A: ALTER SEQUENCE someseq MAXVALUE 10;
> B: SELECT nextval('someseq') FROM generate_series(1, 1000);
>
> => ignores maxvalue

Well, for this one that's because the catalog change is
transactional... I am not sure that using heap_inplace_update() would
make things better just to be compatible with previous versions.
-- 
Michael

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-04-27T07:10:09Z


On April 27, 2017 12:06:55 AM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:
>On Thu, Apr 27, 2017 at 3:23 PM, Andres Freund <andres@anarazel.de>
>wrote:
>> More fun:
>>
>> A: CREATE SEQUENCE someseq;
>> A: BEGIN;
>> A: ALTER SEQUENCE someseq MAXVALUE 10;
>> B: SELECT nextval('someseq') FROM generate_series(1, 1000);
>>
>> => ignores maxvalue
>
>Well, for this one that's because the catalog change is
>transactional... 

Or because the locking model is borked. 

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: Concurrent ALTER SEQUENCE RESTART Regression

Michael Paquier <michael.paquier@gmail.com> — 2017-04-28T07:55:45Z

On Thu, Apr 27, 2017 at 4:10 PM, Andres Freund <andres@anarazel.de> wrote:
> On April 27, 2017 12:06:55 AM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:
>>On Thu, Apr 27, 2017 at 3:23 PM, Andres Freund <andres@anarazel.de>
>>wrote:
>>> More fun:
>>>
>>> A: CREATE SEQUENCE someseq;
>>> A: BEGIN;
>>> A: ALTER SEQUENCE someseq MAXVALUE 10;
>>> B: SELECT nextval('someseq') FROM generate_series(1, 1000);
>>>
>>> => ignores maxvalue
>>
>>Well, for this one that's because the catalog change is
>>transactional...
>
> Or because the locking model is borked.

The operation actually relies heavily on the fact that the exclusive
lock on the buffer of pg_sequence is hold until the end of the catalog
update. And using heap_inplace_update() seems mandatory to me as the
metadata update should be non-transactional, giving the attached. I
have added some isolation tests. Thoughts? The attached makes HEAD map
with the pre-9.6 behavior.
-- 
Michael

Re: Concurrent ALTER SEQUENCE RESTART Regression

Noah Misch <noah@leadboat.com> — 2017-04-30T08:05:17Z

On Fri, Apr 28, 2017 at 04:55:45PM +0900, Michael Paquier wrote:
> On Thu, Apr 27, 2017 at 4:10 PM, Andres Freund <andres@anarazel.de> wrote:
> > On April 27, 2017 12:06:55 AM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:
> >>On Thu, Apr 27, 2017 at 3:23 PM, Andres Freund <andres@anarazel.de>
> >>wrote:
> >>> More fun:
> >>>
> >>> A: CREATE SEQUENCE someseq;
> >>> A: BEGIN;
> >>> A: ALTER SEQUENCE someseq MAXVALUE 10;
> >>> B: SELECT nextval('someseq') FROM generate_series(1, 1000);
> >>>
> >>> => ignores maxvalue
> >>
> >>Well, for this one that's because the catalog change is
> >>transactional...
> >
> > Or because the locking model is borked.
> 
> The operation actually relies heavily on the fact that the exclusive
> lock on the buffer of pg_sequence is hold until the end of the catalog
> update. And using heap_inplace_update() seems mandatory to me as the
> metadata update should be non-transactional, giving the attached. I
> have added some isolation tests. Thoughts? The attached makes HEAD map
> with the pre-9.6 behavior.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Peter,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

Re: Concurrent ALTER SEQUENCE RESTART Regression

Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-04-30T10:00:47Z

On 28/04/17 09:55, Michael Paquier wrote:
> On Thu, Apr 27, 2017 at 4:10 PM, Andres Freund <andres@anarazel.de> wrote:
>> On April 27, 2017 12:06:55 AM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:
>>> On Thu, Apr 27, 2017 at 3:23 PM, Andres Freund <andres@anarazel.de>
>>> wrote:
>>>> More fun:
>>>>
>>>> A: CREATE SEQUENCE someseq;
>>>> A: BEGIN;
>>>> A: ALTER SEQUENCE someseq MAXVALUE 10;
>>>> B: SELECT nextval('someseq') FROM generate_series(1, 1000);
>>>>
>>>> => ignores maxvalue
>>>
>>> Well, for this one that's because the catalog change is
>>> transactional...
>>
>> Or because the locking model is borked.
> 
> The operation actually relies heavily on the fact that the exclusive
> lock on the buffer of pg_sequence is hold until the end of the catalog
> update. And using heap_inplace_update() seems mandatory to me as the
> metadata update should be non-transactional, giving the attached. I
> have added some isolation tests. Thoughts? The attached makes HEAD map
> with the pre-9.6 behavior.
> 

The question is if we want the metadata update to be transactional or
not (I don't know what was Peter's goal here). If we did want
transactionality, we'd have to change lock levels for the sequence
relation in ALTER SEQUENCE so that it blocks other ALTERs and nextval().

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

Re: Concurrent ALTER SEQUENCE RESTART Regression

Michael Paquier <michael.paquier@gmail.com> — 2017-04-30T12:30:08Z

On Sun, Apr 30, 2017 at 7:00 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> On 28/04/17 09:55, Michael Paquier wrote:
>> On Thu, Apr 27, 2017 at 4:10 PM, Andres Freund <andres@anarazel.de> wrote:
>>> On April 27, 2017 12:06:55 AM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:
>>>> On Thu, Apr 27, 2017 at 3:23 PM, Andres Freund <andres@anarazel.de>
>>>> wrote:
>>>>> More fun:
>>>>>
>>>>> A: CREATE SEQUENCE someseq;
>>>>> A: BEGIN;
>>>>> A: ALTER SEQUENCE someseq MAXVALUE 10;
>>>>> B: SELECT nextval('someseq') FROM generate_series(1, 1000);
>>>>>
>>>>> => ignores maxvalue
>>>>
>>>> Well, for this one that's because the catalog change is
>>>> transactional...
>>>
>>> Or because the locking model is borked.
>>
>> The operation actually relies heavily on the fact that the exclusive
>> lock on the buffer of pg_sequence is hold until the end of the catalog
>> update. And using heap_inplace_update() seems mandatory to me as the
>> metadata update should be non-transactional, giving the attached. I
>> have added some isolation tests. Thoughts? The attached makes HEAD map
>> with the pre-9.6 behavior.
>>
>
> The question is if we want the metadata update to be transactional or
> not (I don't know what was Peter's goal here). If we did want
> transactionality, we'd have to change lock levels for the sequence
> relation in ALTER SEQUENCE so that it blocks other ALTERs and nextval().

Yeah, though it is not strictly necessary to block nextval() by using
a ShareUpdateExclusive lock. It seems to me that Andres has a good
point upthreadt houhg: things should remain non-transactional as this
has been the case since sequences are in Postgres. Also, the docs
still mention that changes take immediately effect and ALTER SEQUENCE
changes are non-reversible. HEAD fails to keep both promises.
-- 
Michael

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-01T17:49:28Z

On 2017-04-30 12:00:47 +0200, Petr Jelinek wrote:
> On 28/04/17 09:55, Michael Paquier wrote:
> > On Thu, Apr 27, 2017 at 4:10 PM, Andres Freund <andres@anarazel.de> wrote:
> >> On April 27, 2017 12:06:55 AM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:
> >>> On Thu, Apr 27, 2017 at 3:23 PM, Andres Freund <andres@anarazel.de>
> >>> wrote:
> >>>> More fun:
> >>>>
> >>>> A: CREATE SEQUENCE someseq;
> >>>> A: BEGIN;
> >>>> A: ALTER SEQUENCE someseq MAXVALUE 10;
> >>>> B: SELECT nextval('someseq') FROM generate_series(1, 1000);
> >>>>
> >>>> => ignores maxvalue
> >>>
> >>> Well, for this one that's because the catalog change is
> >>> transactional...
> >>
> >> Or because the locking model is borked.
> > 
> > The operation actually relies heavily on the fact that the exclusive
> > lock on the buffer of pg_sequence is hold until the end of the catalog
> > update. And using heap_inplace_update() seems mandatory to me as the
> > metadata update should be non-transactional, giving the attached. I
> > have added some isolation tests. Thoughts? The attached makes HEAD map
> > with the pre-9.6 behavior.
> > 
> 
> The question is if we want the metadata update to be transactional or
> not (I don't know what was Peter's goal here). If we did want
> transactionality, we'd have to change lock levels for the sequence
> relation in ALTER SEQUENCE so that it blocks other ALTERs and nextval().

Well, previously it wasn't transactional and didn't block, but largely
consistently so.  Now it's a weird mix: RESTART is not transactional,
MAXVAL etc are transactional - even when done at the same time as
RESTART -, but don't block, so you get inconsistent results until the
transaction commits.  And you get failures that are not consistent with
transactional DDL.

- Andres

Re: Concurrent ALTER SEQUENCE RESTART Regression

Robert Haas <robertmhaas@gmail.com> — 2017-05-02T02:10:36Z

On Wed, Apr 26, 2017 at 9:51 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Apr 27, 2017 at 10:12 AM, Andres Freund <andres@anarazel.de> wrote:
>> On 2017-04-26 21:07:20 -0400, Peter Eisentraut wrote:
>>> One thing we could do, since all catalog updates now go through
>>> CatalogTupleUpdate(), is not use simple_heap_update() there but do the
>>> heap_update() directly and provide a better user-facing error message.
>>
>> I think it's unacceptable to regress with an error message here.  I've
>> seen sequence DDL being used while concurrent DML was onging in a number
>> of production use cases, and just starting to error out instead of
>> properly blocking doesn't seem acceptable to me.
>
> +1'ing on this argument.

+1 from me, too.  The fact that we have some other places where we get
"ERROR: tuple concurrently updated" is an awfulness that nobody's
gotten around to fixing, not an excuse to regress cases that were
previously working properly (never mind the other breakage reported
downthread).  If this can't be fixed the whole patch should be ripped
out, IMHO.

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

Re: Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-05-02T14:37:07Z

On 4/30/17 08:30, Michael Paquier wrote:
> Also, the docs
> still mention that changes take immediately effect and ALTER SEQUENCE
> changes are non-reversible.

Fixed that.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-05-02T14:53:19Z

On 4/24/17 15:52, Jason Petersen wrote:
>   1. Create a new sequence: CREATE SEQUENCE my_seq;
>   2. Start this loop twice in different shells:
>        while true; do psql -1Xtc 'ALTER SEQUENCE my_seq RESTART 1'; done

> Each loop should repeatedly succeed and simply print ALTER SEQUENCE over and over.

> The output stream is punctuated by occasional "ERROR:  tuple concurrently updated" messages.

This message comes from the pg_sequence catalog update.  But in the case
of the RESTART clause, you don't need to update the catalog, because it
just needs to write to the sequence's relation.  So I have tweaked the
code a little to omit the catalog update if it's not needed.  Your test
case works without errors now.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-05-02T15:05:38Z

On 4/27/17 01:52, Andres Freund wrote:
> In contrast to <v10, the actual change of the tuple is *not* happening
> with the page lock held.  But now we do log XLOG_SEQ_LOG, then unlock
> the buffer, and then do a CatalogTupleUpdate().  How is that correct?

The change to the sequence data and the change to the catalog are two
separate operations.  There is no need AFAICT for the latter to be done
while the former is locked or vice versa.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-02T15:07:44Z

On 2017-05-02 10:53:19 -0400, Peter Eisentraut wrote:
> On 4/24/17 15:52, Jason Petersen wrote:
> >   1. Create a new sequence: CREATE SEQUENCE my_seq;
> >   2. Start this loop twice in different shells:
> >        while true; do psql -1Xtc 'ALTER SEQUENCE my_seq RESTART 1'; done
> 
> > Each loop should repeatedly succeed and simply print ALTER SEQUENCE over and over.
> 
> > The output stream is punctuated by occasional "ERROR:  tuple concurrently updated" messages.
> 
> This message comes from the pg_sequence catalog update.  But in the case
> of the RESTART clause, you don't need to update the catalog, because it
> just needs to write to the sequence's relation.  So I have tweaked the
> code a little to omit the catalog update if it's not needed.  Your test
> case works without errors now.

Wait, how does this *actually* solve anything, but scratch at the
surface?  You just add a MAXVALUE and it starts failing (and not being
adhered to) again?

Greetings,

Andres Freund

Re: Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-05-02T15:19:08Z

On 5/2/17 11:07, Andres Freund wrote:
> On 2017-05-02 10:53:19 -0400, Peter Eisentraut wrote:
>> On 4/24/17 15:52, Jason Petersen wrote:
>>>   1. Create a new sequence: CREATE SEQUENCE my_seq;
>>>   2. Start this loop twice in different shells:
>>>        while true; do psql -1Xtc 'ALTER SEQUENCE my_seq RESTART 1'; done
>>
>>> Each loop should repeatedly succeed and simply print ALTER SEQUENCE over and over.
>>
>>> The output stream is punctuated by occasional "ERROR:  tuple concurrently updated" messages.
>>
>> This message comes from the pg_sequence catalog update.  But in the case
>> of the RESTART clause, you don't need to update the catalog, because it
>> just needs to write to the sequence's relation.  So I have tweaked the
>> code a little to omit the catalog update if it's not needed.  Your test
>> case works without errors now.
> 
> Wait, how does this *actually* solve anything, but scratch at the
> surface?  You just add a MAXVALUE and it starts failing (and not being
> adhered to) again?

The just committed patch somewhat disentangles the transactional from
the nontransactional parts of ALTER SEQUENCE.

RESTART is a nontransactional action.  Now you get the same concurrency
behavior for RESTART as you would for setval and nextval.  This was not
the case prior to the fix.

Other clauses such as MAXVALUE are transactional actions.  You get the
same concurrency behavior as for most DDL in PostgreSQL.  You could
argue that the RowExclusiveLock on pg_sequence in not enough and should
be perhaps ShareRowExclusiveLock to avoid "tuple concurrently updated"
messages.  But just over in 521fd4795e3ec3d0b263b62e5eb58e1557be9c86 it
was argued that that sort of thing was undesirable.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-02T15:39:44Z

Hi,

On 2017-05-02 11:19:08 -0400, Peter Eisentraut wrote:
> On 5/2/17 11:07, Andres Freund wrote:
> > On 2017-05-02 10:53:19 -0400, Peter Eisentraut wrote:
> >> This message comes from the pg_sequence catalog update.  But in the case
> >> of the RESTART clause, you don't need to update the catalog, because it
> >> just needs to write to the sequence's relation.  So I have tweaked the
> >> code a little to omit the catalog update if it's not needed.  Your test
> >> case works without errors now.
> > 
> > Wait, how does this *actually* solve anything, but scratch at the
> > surface?  You just add a MAXVALUE and it starts failing (and not being
> > adhered to) again?
> 
> The just committed patch somewhat disentangles the transactional from
> the nontransactional parts of ALTER SEQUENCE.
> 
> RESTART is a nontransactional action.  Now you get the same concurrency
> behavior for RESTART as you would for setval and nextval.  This was not
> the case prior to the fix.

Well, except if specified together with other options.


> Other clauses such as MAXVALUE are transactional actions.

Yes, and that's a complete mess. Because RESTART relies on the current
catalog state in the restarting transaction, but affect other
transactions immediately, you can all kind of weird things.

S1: CREATE SEQUENCE foo START WITH 100 MINVALUE 10 MAXVALUE 1000;
S1: SELECT nextval('foo');
┌─────────┐
│ nextval │
├─────────┤
│     100 │
└─────────┘
S1: BEGIN;
S1: ALTER SEQUENCE foo RESTART START WITH 1 MINVALUE 1 MAXVALUE 1000;
S2: SELECT nextval('foo');
┌─────────┐
│ nextval │
├─────────┤
│       1 │
└─────────┘
(1 row)
-- uh, so we get a value that's below minvalue. Borked transactionality
S1: ROLLBACK;
S1: SELECT nextval('foo');
┌─────────┐
│ nextval │
├─────────┤
│       2 │
└─────────┘
-- uh, so even worse.  We have transactional behaviour (minvalue
change), that's not even transactional.  And we'll continue to return
values that aren't legal to the sequence's definition.


In other words, you made alter sequence transactional, except randomly
not.  I'm increasingly thinking that this change hasn't gotten even
close enough scrutiny and should just be reverted.


> You get the same concurrency behavior as for most DDL in PostgreSQL.

I don't see any evidence of that.


> You could
> argue that the RowExclusiveLock on pg_sequence in not enough and should
> be perhaps ShareRowExclusiveLock to avoid "tuple concurrently updated"
> messages.  But just over in 521fd4795e3ec3d0b263b62e5eb58e1557be9c86 it
> was argued that that sort of thing was undesirable.

No, we'd need proper locking on the sequence object, not on pg_sequence.
Alter table, and a lot of other commands, primarily rely on proper
locking of the underlying object, not on heavyweight locks of pg_class.

- Andres

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-02T15:41:48Z

On 2017-05-02 11:05:38 -0400, Peter Eisentraut wrote:
> On 4/27/17 01:52, Andres Freund wrote:
> > In contrast to <v10, the actual change of the tuple is *not* happening
> > with the page lock held.  But now we do log XLOG_SEQ_LOG, then unlock
> > the buffer, and then do a CatalogTupleUpdate().  How is that correct?
> 
> The change to the sequence data and the change to the catalog are two
> separate operations.  There is no need AFAICT for the latter to be done
> while the former is locked or vice versa.

You snipped the salient part of my response:

> Imagine two of these running concurrently - you might end up with
> A:XLogInsert B:XLogInsert B:CatalogTupleUpdate A:CatalogTupleUpdate

Which'll lead, yet another avenue, to sequence states that aren't in
sync with the catalog.

- Andres

Re: Concurrent ALTER SEQUENCE RESTART Regression

Robert Haas <robertmhaas@gmail.com> — 2017-05-02T16:45:35Z

On Thu, Apr 27, 2017 at 1:52 AM, Andres Freund <andres@anarazel.de> wrote:=
> In contrast to <v10, the actual change of the tuple is *not* happening
> with the page lock held.  But now we do log XLOG_SEQ_LOG, then unlock
> the buffer, and then do a CatalogTupleUpdate().  How is that correct?
> And if so, why isn't there a honking large comment explaining why it is?

I can't actually understand this complaint.  I think this code is
buggy, but for different reasons than you do.  The page lock is held,
because read_seq_tuple() acquires it.  And then we call heap_open()
with the buffer lock held?!  I don't think that's a good idea in any
way - we shouldn't be doing complex operations that might acquire a
buffer lock while we're holding a buffer lock.  But by the same token
surely we don't want to do CatalogUpdateIndexes() while holding the
buffer lock either; mutual exclusion needs to be managed at some
higher level, using, say, a heavyweight tuple lock.

Another thing that doesn't look so good about AlterSequence is that it
uses RangeVarGetRelid() instead of RangeVarGetsRelidExtended() with
RangeVarCallbackOwnsRelation or some derivative thereof.  That of
course means you can obstruct access to a sequence you don't own.

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

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-02T17:36:05Z

On 2017-05-02 12:45:35 -0400, Robert Haas wrote:
> On Thu, Apr 27, 2017 at 1:52 AM, Andres Freund <andres@anarazel.de> wrote:=
> > In contrast to <v10, the actual change of the tuple is *not* happening
> > with the page lock held.  But now we do log XLOG_SEQ_LOG, then unlock
> > the buffer, and then do a CatalogTupleUpdate().  How is that correct?
> > And if so, why isn't there a honking large comment explaining why it is?
> 
> I can't actually understand this complaint.

My concern is that this allows two XLOG_SEQ_LOG records in two sessions
to be interleaved in a bad manner:
S1:XLogInsert
S2:XLogInsert
S2:CatalogTupleUpdate
S1:CatalogTupleUpdate

which'd mean that S1's catalog state would apply to the XLOG_SEQ_LOG'ed
state from S2.

> I think this code is
> buggy, but for different reasons than you do.  The page lock is held,
> because read_seq_tuple() acquires it.  And then we call heap_open()
> with the buffer lock held?!  I don't think that's a good idea in any
> way - we shouldn't be doing complex operations that might acquire a
> buffer lock while we're holding a buffer lock.

That's pretty broken, yes.


> But by the same token surely we don't want to do
> CatalogUpdateIndexes() while holding the buffer lock either; mutual
> exclusion needs to be managed at some higher level, using, say, a
> heavyweight tuple lock.

Right, I don't want that to happen - I think it means we need a proper
lock here, but Peter seems to be against that for reasons I don't
understand.  It's what Michael had suggested in:
http://archives.postgresql.org/message-id/CAB7nPqRev_wK4k39hQBpQZRQ17v29guxfobnnmTYT_-hUU67BA%40mail.gmail.com

- Andres

Re: Concurrent ALTER SEQUENCE RESTART Regression

Robert Haas <robertmhaas@gmail.com> — 2017-05-02T18:40:38Z

On Tue, May 2, 2017 at 1:36 PM, Andres Freund <andres@anarazel.de> wrote:
>> But by the same token surely we don't want to do
>> CatalogUpdateIndexes() while holding the buffer lock either; mutual
>> exclusion needs to be managed at some higher level, using, say, a
>> heavyweight tuple lock.
>
> Right, I don't want that to happen - I think it means we need a proper
> lock here, but Peter seems to be against that for reasons I don't
> understand.  It's what Michael had suggested in:
> http://archives.postgresql.org/message-id/CAB7nPqRev_wK4k39hQBpQZRQ17v29guxfobnnmTYT_-hUU67BA%40mail.gmail.com

Yes, I didn't understand Peter's objection, either.  It's true that
there are multiple levels of locks here, but if we've got things
failing that used to work, then we've not got all the right ones.

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

Re: Concurrent ALTER SEQUENCE RESTART Regression

Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-05-03T05:19:16Z

On 02/05/17 20:40, Robert Haas wrote:
> On Tue, May 2, 2017 at 1:36 PM, Andres Freund <andres@anarazel.de> wrote:
>>> But by the same token surely we don't want to do
>>> CatalogUpdateIndexes() while holding the buffer lock either; mutual
>>> exclusion needs to be managed at some higher level, using, say, a
>>> heavyweight tuple lock.
>>
>> Right, I don't want that to happen - I think it means we need a proper
>> lock here, but Peter seems to be against that for reasons I don't
>> understand.  It's what Michael had suggested in:
>> http://archives.postgresql.org/message-id/CAB7nPqRev_wK4k39hQBpQZRQ17v29guxfobnnmTYT_-hUU67BA%40mail.gmail.com
> 
> Yes, I didn't understand Peter's objection, either.  It's true that
> there are multiple levels of locks here, but if we've got things
> failing that used to work, then we've not got all the right ones.
> 

I do understand the objection, Peter wants to keep metadata
transactional which I would prefer as well (and that's not going to be
the case with Michael's approach). It could be done if ALTER SEQUENCE
held stronger lock on the sequence relation though, but it needs to
block nextval as well in that case (which I think would mean nextval
would need row share lock, unless we are okay with doing access
exclusive lock during ALTER) as I mentioned up thread.

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

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-03T05:22:46Z

On 2017-05-03 07:19:16 +0200, Petr Jelinek wrote:
> On 02/05/17 20:40, Robert Haas wrote:
> > On Tue, May 2, 2017 at 1:36 PM, Andres Freund <andres@anarazel.de> wrote:
> >>> But by the same token surely we don't want to do
> >>> CatalogUpdateIndexes() while holding the buffer lock either; mutual
> >>> exclusion needs to be managed at some higher level, using, say, a
> >>> heavyweight tuple lock.
> >>
> >> Right, I don't want that to happen - I think it means we need a proper
> >> lock here, but Peter seems to be against that for reasons I don't
> >> understand.  It's what Michael had suggested in:
> >> http://archives.postgresql.org/message-id/CAB7nPqRev_wK4k39hQBpQZRQ17v29guxfobnnmTYT_-hUU67BA%40mail.gmail.com
> >
> > Yes, I didn't understand Peter's objection, either.  It's true that
> > there are multiple levels of locks here, but if we've got things
> > failing that used to work, then we've not got all the right ones.
> >
>
> I do understand the objection, Peter wants to keep metadata
> transactional which I would prefer as well (and that's not going to be
> the case with Michael's approach).

Huh? How does increasing the locklevel (from AccessShare to
ShareUpdateExclusive) make it nontransactional?


> It could be done if ALTER SEQUENCE held stronger lock on the sequence
> relation though, but it needs to block nextval as well in that case
> (which I think would mean nextval would need row share lock, unless we
> are okay with doing access exclusive lock during ALTER) as I mentioned
> up thread.

That one is more complicated, because AccessShareLocks on sequences are
held on for performance reasons...  Possibly not really required
anymore, due to fast-path locks? Still'd increase the number of
lock/unlock cycles.

- Andres

Re: Concurrent ALTER SEQUENCE RESTART Regression

Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-05-03T05:26:21Z

On 03/05/17 07:22, Andres Freund wrote:
> On 2017-05-03 07:19:16 +0200, Petr Jelinek wrote:
>> On 02/05/17 20:40, Robert Haas wrote:
>>> On Tue, May 2, 2017 at 1:36 PM, Andres Freund <andres@anarazel.de> wrote:
>>>>> But by the same token surely we don't want to do
>>>>> CatalogUpdateIndexes() while holding the buffer lock either; mutual
>>>>> exclusion needs to be managed at some higher level, using, say, a
>>>>> heavyweight tuple lock.
>>>>
>>>> Right, I don't want that to happen - I think it means we need a proper
>>>> lock here, but Peter seems to be against that for reasons I don't
>>>> understand.  It's what Michael had suggested in:
>>>> http://archives.postgresql.org/message-id/CAB7nPqRev_wK4k39hQBpQZRQ17v29guxfobnnmTYT_-hUU67BA%40mail.gmail.com
>>>
>>> Yes, I didn't understand Peter's objection, either.  It's true that
>>> there are multiple levels of locks here, but if we've got things
>>> failing that used to work, then we've not got all the right ones.
>>>
>>
>> I do understand the objection, Peter wants to keep metadata
>> transactional which I would prefer as well (and that's not going to be
>> the case with Michael's approach).
> 
> Huh? How does increasing the locklevel (from AccessShare to
> ShareUpdateExclusive) make it nontransactional?
> 

Ah damn, I looked at wrong patch (the one that did inline heap update,
Michael produces too many patches ;)).

Yes that one is good.

>> It could be done if ALTER SEQUENCE held stronger lock on the sequence
>> relation though, but it needs to block nextval as well in that case
>> (which I think would mean nextval would need row share lock, unless we
>> are okay with doing access exclusive lock during ALTER) as I mentioned
>> up thread.
> 
> That one is more complicated, because AccessShareLocks on sequences are
> held on for performance reasons...  Possibly not really required
> anymore, due to fast-path locks? Still'd increase the number of
> lock/unlock cycles.

Right but won't we still have problem with nextval ignoring the ALTER
until it commits without that?

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

Re: Concurrent ALTER SEQUENCE RESTART Regression

Michael Paquier <michael.paquier@gmail.com> — 2017-05-03T07:39:00Z

On Wed, May 3, 2017 at 2:26 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
>>> It could be done if ALTER SEQUENCE held stronger lock on the sequence
>>> relation though, but it needs to block nextval as well in that case
>>> (which I think would mean nextval would need row share lock, unless we
>>> are okay with doing access exclusive lock during ALTER) as I mentioned
>>> up thread.
>>
>> That one is more complicated, because AccessShareLocks on sequences are
>> held on for performance reasons...  Possibly not really required
>> anymore, due to fast-path locks? Still'd increase the number of
>> lock/unlock cycles.
>
> Right but won't we still have problem with nextval ignoring the ALTER
> until it commits without that?

Right, the only thing that you can really do here is to take a
stronger lock on the parent object, and that will be disruptive for
concurrent sessions. Not sure what Peter wants to do here, but
3d092fe5 is just putting sugar powder on top of a cake badly-cooked.
What we ought to do instead is review the recipy as users should never
be able to face "tuple concurrently updated". So as far as I can see,
this open item is not addressed. And the issues with locking around
XLOG/catalog updates are not solved either.
-- 
Michael

Re: [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-05-04T03:29:29Z

On 4/30/17 04:05, Noah Misch wrote:
> The above-described topic is currently a PostgreSQL 10 open item.  Peter,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.

I'm working on this and will report on Friday.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

Noah Misch <noah@leadboat.com> — 2017-05-06T19:02:46Z

On Wed, May 03, 2017 at 11:29:29PM -0400, Peter Eisentraut wrote:
> On 4/30/17 04:05, Noah Misch wrote:
> > The above-described topic is currently a PostgreSQL 10 open item.  Peter,
> > since you committed the patch believed to have created it, you own this open
> > item.  If some other commit is more relevant or if this does not belong as a
> > v10 open item, please let us know.  Otherwise, please observe the policy on
> > open item ownership[1] and send a status update within three calendar days of
> > this message.  Include a date for your subsequent status update.  Testers may
> > discover new open items at any time, and I want to plan to get them all fixed
> > well in advance of shipping v10.  Consequently, I will appreciate your efforts
> > toward speedy resolution.  Thanks.
> 
> I'm working on this and will report on Friday.

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-07T23:43:34Z

Hi,

Moving discussion to -hackers, this isn't really a bug, it's an
architectural issue with the new in-tree approach.

Short recap: With the patch applied in [1] ff, sequences partially
behave transactional (because pg_sequence is updated transactionally),
partially non-transctionally (because there's no locking enforcing it,
and it's been stated as undesirable to change that). This leads to
issues like described in [2]. For more context, read the whole
discussion.

On 2017-05-03 23:29:29 -0400, Peter Eisentraut wrote:
> I'm working on this and will report on Friday.

What's the plan here? I think the problem is that the code as is is
trying to marry two incompatible things: You're trying to make nextval()
not block, but have ALTER SEQUENCE be transactional. Given MAXVAL,
INCREMENT, etc. those two simply aren't compatible.

I think there's three basic ways to a resolution here:
1. Revert the entire patch for now. That's going to be very messy because
of the number of followup patches & features.
2. Keep the catalog, but only ever update the records using
heap_inplace_update. That'll make the transactional behaviour very
similar to before. It'd medium-term also allow to move the actual
sequence block into the pg_sequence catalog itself.
3. Keep the catalog, make ALTER properly transactional, blocking
concurrent nextval()s. This resolves the issue that nextval() can't
see the changed definition of the sequence.

I think this really needs design agreement from multiple people, because
any of these choices will have significant impact.

To me 3. seems like the best approach long-term, because both the
current and the <v10 behaviour certainly is confusing and inconsistent
(but in different ways). But it'll be quite noticeable to users.

- Andres

[1] http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=1753b1b027035029c2a2a1649065762fafbf63f3
[2] http://archives.postgresql.org/message-id/20170427062304.gxh3rczhh6vblrwh%40alap3.anarazel.de

Re: Concurrent ALTER SEQUENCE RESTART Regression

Michael Paquier <michael.paquier@gmail.com> — 2017-05-08T02:27:20Z

On Mon, May 8, 2017 at 8:43 AM, Andres Freund <andres@anarazel.de> wrote:
> Moving discussion to -hackers, this isn't really a bug, it's an
> architectural issue with the new in-tree approach.
>
> Short recap: With the patch applied in [1] ff, sequences partially
> behave transactional (because pg_sequence is updated transactionally),
> partially non-transactionally (because there's no locking enforcing it,
> and it's been stated as undesirable to change that).  This leads to
> issues like described in [2].  For more context, read the whole
> discussion.

Thanks for summarizing.

> On 2017-05-03 23:29:29 -0400, Peter Eisentraut wrote:
>> I'm working on this and will report on Friday.
>
> What's the plan here?  I think the problem is that the code as is is
> trying to marry two incompatible things: You're trying to make nextval()
> not block, but have ALTER SEQUENCE be transactional.  Given MAXVAL,
> INCREMENT, etc. those two simply aren't compatible.
>
> I think there's three basic ways to a resolution here:
> 1. Revert the entire patch for now. That's going to be very messy because
>    of the number of followup patches & features.
> 2. Keep the catalog, but only ever update the records using
>    heap_inplace_update.  That'll make the transactional behaviour very
>    similar to before.  It'd medium-term also allow to move the actual
>    sequence block into the pg_sequence catalog itself.

In this case it is enough to use ShareUpdateExclusiveLock on the
sequence object, not pg_sequence.

> 3. Keep the catalog, make ALTER properly transactional, blocking
>    concurrent nextval()s. This resolves the issue that nextval() can't
>    see the changed definition of the sequence.

This will need an even stronger lock, AccessExclusiveLock to make this
work properly.

> I think this really needs design agreement from multiple people, because
> any of these choices will have significant impact.

> To me 3. seems like the best approach long-term, because both the
> current and the <v10 behaviour certainly is confusing and inconsistent
> (but in different ways).  But it'll be quite noticeable to users.

Count me in for the 3. bucket. This loses the properly of having
nextval() available to users when a parallel ALTER SEQUENCE is
running, but consistency matters even more. I think that the final
patch closing this thread should also have proper isolation tests.
-- 
Michael

Re: Concurrent ALTER SEQUENCE RESTART Regression

Noah Misch <noah@leadboat.com> — 2017-05-08T06:58:09Z

On Sat, May 06, 2017 at 07:02:46PM +0000, Noah Misch wrote:
> On Wed, May 03, 2017 at 11:29:29PM -0400, Peter Eisentraut wrote:
> > On 4/30/17 04:05, Noah Misch wrote:
> > > The above-described topic is currently a PostgreSQL 10 open item.  Peter,
> > > since you committed the patch believed to have created it, you own this open
> > > item.  If some other commit is more relevant or if this does not belong as a
> > > v10 open item, please let us know.  Otherwise, please observe the policy on
> > > open item ownership[1] and send a status update within three calendar days of
> > > this message.  Include a date for your subsequent status update.  Testers may
> > > discover new open items at any time, and I want to plan to get them all fixed
> > > well in advance of shipping v10.  Consequently, I will appreciate your efforts
> > > toward speedy resolution.  Thanks.
> > 
> > I'm working on this and will report on Friday.
> 
> This PostgreSQL 10 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past due
for your status update.  Please reacquaint yourself with the policy on open
item ownership[1] and then reply immediately.  If I do not hear from you by
2017-05-09 07:00 UTC, I will transfer this item to release management team
ownership without further notice.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

Re: [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-05-08T16:28:38Z

On 5/8/17 02:58, Noah Misch wrote:
> IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past due
> for your status update.  Please reacquaint yourself with the policy on open
> item ownership[1] and then reply immediately.  If I do not hear from you by
> 2017-05-09 07:00 UTC, I will transfer this item to release management team
> ownership without further notice.

I got side-tracked by the minor releases preparation.  I will work on
this now and report on Wednesday.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-08T16:42:52Z

On 2017-05-08 12:28:38 -0400, Peter Eisentraut wrote:
> On 5/8/17 02:58, Noah Misch wrote:
> > IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past due
> > for your status update.  Please reacquaint yourself with the policy on open
> > item ownership[1] and then reply immediately.  If I do not hear from you by
> > 2017-05-09 07:00 UTC, I will transfer this item to release management team
> > ownership without further notice.
> 
> I got side-tracked by the minor releases preparation.  I will work on
> this now and report on Wednesday.

This first needs discussion, rather than work on a patch.  Could you
please reply to
http://archives.postgresql.org/message-id/20170507234334.yimkp6aq2o43wtt2%40alap3.anarazel.de

You've so far not actually replied to the concerns raised in this thread
in a more than superficial way, not responded to questions, and it's
been two weeks.  This is an architectural issue, not just a bug.  We
can't even think of going to beta before it's resolved.  It's understand
you not having time to properly resolve it immediately, but we need to
be able to have a discussion about where to go from here.

- Andres

Re: Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-05-10T05:09:37Z

On 5/7/17 19:43, Andres Freund wrote:
> 3. Keep the catalog, make ALTER properly transactional, blocking
>    concurrent nextval()s. This resolves the issue that nextval() can't
>    see the changed definition of the sequence.

This was the intended choice.

I think I have more clarity about the different, overlapping issues:

1. Concurrent ALTER SEQUENCE causes "tuple concurrently updated"
   error, caused by updates to pg_sequence catalog.  This can be fixed
   by taking a self-exclusive lock somewhere.

   A typical answer for other catalogs is to use
   ShareRowExclusiveLock.  But in another context it was argued that
   is undesirable for other reasons, and it's better to stick with
   RowExclusiveLock and deal with the occasional consequences.  But
   this question is obsoleted by #2.

2. There is some inconsistency or disagreement about what to lock
   when modifying relation metadata.  When you alter a non-relation
   object, you just have the respective catalog to lock, and you have
   to make the trade-offs described in #1.  When you alter a relation
   object, you can lock the catalog or the actual relation, or both.
   I had gone with locking the catalog, but existing practice in
   tablecmds.c is to lock the relation with the most appropriate lock
   level, and use RowExclusiveLock for the catalog.  We can make
   sequences do that, too.

3. Sequence WAL writes and catalog updates are not protected by same
   lock.  We can fix that by holding a lock longer.  If per #2 we make
   the lock on the sequence the "main" one, then we just hold that one
   across the catalog update.

4. Some concerns about in AlterSequence() opening the pg_sequence
   catalog while holding the sequence buffer lock.  Move some things
   around to fix that.

5. nextval() disregarding uncommitted ALTER SEQUENCE changes.  In
   <PG10, it would read the uncommitted metadata and observe it.
   Currently, it goes ahead even if there is an uncommitted ALTER
   SEQUENCE pending that would prohibit what it's about to do (e.g.,
   MAXVALUE change).

   I think the correct fix is to have nextval() and ALTER SEQUENCE use
   sensible lock levels so that they block each other.  Since
   nextval() currently uses AccessShareLock, the suggestion was for
   ALTER SEQUENCE to therefore use AccessExclusiveLock.  But I think a
   better idea would be for nextval() to use RowExclusiveLock
   (analogous to UPDATE) and ALTER SEQUENCE to use
   ShareRowExclusiveLock, which would also satisfy issue #1.

Attached patches:

0001 Adds isolation tests for sequence behavior, in particular regarding
point #5.  The expected files in 0001 show how it behaves in 9.6, and if
you apply it to 10 without the subsequent fixes, it will show some problems.

0002 Locking changes discussed above, with test changes.

0003 (optional) Reduce locking if only RESTART is specified (because
that does not do a catalog update, so it can run concurrently with nextval).

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Concurrent ALTER SEQUENCE RESTART Regression

Michael Paquier <michael.paquier@gmail.com> — 2017-05-10T13:12:40Z

On Wed, May 10, 2017 at 2:09 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> I think I have more clarity about the different, overlapping issues:
>
> 1. Concurrent ALTER SEQUENCE causes "tuple concurrently updated"
>    error, caused by updates to pg_sequence catalog.  This can be fixed
>    by taking a self-exclusive lock somewhere.
>
>    A typical answer for other catalogs is to use
>    ShareRowExclusiveLock.  But in another context it was argued that
>    is undesirable for other reasons, and it's better to stick with
>    RowExclusiveLock and deal with the occasional consequences.  But
>    this question is obsoleted by #2.

Yes.

> 2. There is some inconsistency or disagreement about what to lock
>    when modifying relation metadata.  When you alter a non-relation
>    object, you just have the respective catalog to lock, and you have
>    to make the trade-offs described in #1.  When you alter a relation
>    object, you can lock the catalog or the actual relation, or both.
>    I had gone with locking the catalog, but existing practice in
>    tablecmds.c is to lock the relation with the most appropriate lock
>    level, and use RowExclusiveLock for the catalog.  We can make
>    sequences do that, too.

Agreed, sequences are relations as well at the end. Consistency sounds
good to me.

> 3. Sequence WAL writes and catalog updates are not protected by same
>    lock.  We can fix that by holding a lock longer.  If per #2 we make
>    the lock on the sequence the "main" one, then we just hold that one
>    across the catalog update.
>
> 4. Some concerns about in AlterSequence() opening the pg_sequence
>    catalog while holding the sequence buffer lock.  Move some things
>    around to fix that.
>
> 5. nextval() disregarding uncommitted ALTER SEQUENCE changes.  In
>    <PG10, it would read the uncommitted metadata and observe it.
>    Currently, it goes ahead even if there is an uncommitted ALTER
>    SEQUENCE pending that would prohibit what it's about to do (e.g.,
>    MAXVALUE change).

Yes, and we want to block all nextval() calls until concurrent ALTER
SEQUENCE are committed.

>    I think the correct fix is to have nextval() and ALTER SEQUENCE use
>    sensible lock levels so that they block each other.  Since
>    nextval() currently uses AccessShareLock, the suggestion was for
>    ALTER SEQUENCE to therefore use AccessExclusiveLock.  But I think a
>    better idea would be for nextval() to use RowExclusiveLock
>    (analogous to UPDATE) and ALTER SEQUENCE to use
>    ShareRowExclusiveLock, which would also satisfy issue #1.

No objections to that.

> Attached patches:
>
> 0001 Adds isolation tests for sequence behavior, in particular regarding
> point #5.  The expected files in 0001 show how it behaves in 9.6, and if
> you apply it to 10 without the subsequent fixes, it will show some problems.
>
> 0002 Locking changes discussed above, with test changes.
>
> 0003 (optional) Reduce locking if only RESTART is specified (because
> that does not do a catalog update, so it can run concurrently with nextval).

Looking at 0001 and 0002... So you are correctly blocking nextval()
when ALTER SEQUENCE holds a lock on the sequence object. And
concurrent calls of nextval() don't conflict. As far as I can see this
matches the implementation of 3.

Here are some minor comments.

+   /* lock page' buffer and read tuple into new sequence structure */
Nit: there is a single quote in this comment.

 /*
+ * TODO rename for lock level change
+ *
  * Open the sequence and acquire AccessShareLock if needed
  *
Some of the functions calling init_sequence() reference
AccessShareLock in comments. Those will need an update as well.

+++ b/src/test/isolation/expected/sequence-nextval.out
@@ -0,0 +1,35 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1a s1b s2a s2b s1c s1d s2c s2d
+step s1a: SELECT nextval('seq1');
+nextval
I am not sure that this brings much value...
-- 
Michael

Re: Concurrent ALTER SEQUENCE RESTART Regression

Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-05-10T13:52:14Z

On 10/05/17 07:09, Peter Eisentraut wrote:
> On 5/7/17 19:43, Andres Freund wrote:
>> 3. Keep the catalog, make ALTER properly transactional, blocking
>>    concurrent nextval()s. This resolves the issue that nextval() can't
>>    see the changed definition of the sequence.
> 
> This was the intended choice.
> 
> [...]
> 
> 5. nextval() disregarding uncommitted ALTER SEQUENCE changes.  In
>    <PG10, it would read the uncommitted metadata and observe it.
>    Currently, it goes ahead even if there is an uncommitted ALTER
>    SEQUENCE pending that would prohibit what it's about to do (e.g.,
>    MAXVALUE change).
> 
>    I think the correct fix is to have nextval() and ALTER SEQUENCE use
>    sensible lock levels so that they block each other.  Since
>    nextval() currently uses AccessShareLock, the suggestion was for
>    ALTER SEQUENCE to therefore use AccessExclusiveLock.  But I think a
>    better idea would be for nextval() to use RowExclusiveLock
>    (analogous to UPDATE) and ALTER SEQUENCE to use
>    ShareRowExclusiveLock, which would also satisfy issue #1.
> 

When I proposed this upstream, Andres raised concern about performance
of nextval() if we do this, did you try to run any benchmark on this?

Looking at the changes to open_share_lock(), I wonder if we need to have
lock level as parameter so that lastval() is not blocked.

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

Re: [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-10T14:29:02Z

Petr Jelinek <petr.jelinek@2ndquadrant.com> writes:
> On 10/05/17 07:09, Peter Eisentraut wrote:
>> I think the correct fix is to have nextval() and ALTER SEQUENCE use
>> sensible lock levels so that they block each other.  Since
>> nextval() currently uses AccessShareLock, the suggestion was for
>> ALTER SEQUENCE to therefore use AccessExclusiveLock.  But I think a
>> better idea would be for nextval() to use RowExclusiveLock
>> (analogous to UPDATE) and ALTER SEQUENCE to use
>> ShareRowExclusiveLock, which would also satisfy issue #1.

> When I proposed this upstream, Andres raised concern about performance
> of nextval() if we do this, did you try to run any benchmark on this?

As long as it doesn't block, the change in lock strength doesn't actually
make any speed difference does it?

If we were taking AccessExclusiveLock somewhere, I'd be worried about
the cost of WAL-logging those; but this proposal does not include any.

			regards, tom lane

Re: Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-10T16:24:50Z

On 2017-05-10 10:29:02 -0400, Tom Lane wrote:
> Petr Jelinek <petr.jelinek@2ndquadrant.com> writes:
> > On 10/05/17 07:09, Peter Eisentraut wrote:
> >> I think the correct fix is to have nextval() and ALTER SEQUENCE use
> >> sensible lock levels so that they block each other.  Since
> >> nextval() currently uses AccessShareLock, the suggestion was for
> >> ALTER SEQUENCE to therefore use AccessExclusiveLock.  But I think a
> >> better idea would be for nextval() to use RowExclusiveLock
> >> (analogous to UPDATE) and ALTER SEQUENCE to use
> >> ShareRowExclusiveLock, which would also satisfy issue #1.
> 
> > When I proposed this upstream, Andres raised concern about performance
> > of nextval() if we do this, did you try to run any benchmark on this?
> 
> As long as it doesn't block, the change in lock strength doesn't actually
> make any speed difference does it?

The issue isn't the strength, but that we currently have this weird
hackery around open_share_lock():
/*
 * Open the sequence and acquire AccessShareLock if needed
 *
 * If we haven't touched the sequence already in this transaction,
 * we need to acquire AccessShareLock.  We arrange for the lock to
 * be owned by the top transaction, so that we don't need to do it
 * more than once per xact.
 */

This'd probably need to be removed, as we'd otherwise would get very
weird semantics around aborted subxacts.  Upthread I theorized whether
that's actually still meaningful given fastpath locking and such, but I
guess we'll have to evaluate that.

Greetings,

Andres Freund

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-10T17:28:39Z

Andres Freund <andres@anarazel.de> writes:
> On 2017-05-10 10:29:02 -0400, Tom Lane wrote:
>> As long as it doesn't block, the change in lock strength doesn't actually
>> make any speed difference does it?

> The issue isn't the strength, but that we currently have this weird
> hackery around open_share_lock():

Oh!  I'd forgotten about that.  Yes, if we change that then we'd
need to do some performance checking.

			regards, tom lane

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-05-11T15:35:22Z

On 5/10/17 12:24, Andres Freund wrote:
> The issue isn't the strength, but that we currently have this weird
> hackery around open_share_lock():
> /*
>  * Open the sequence and acquire AccessShareLock if needed
>  *
>  * If we haven't touched the sequence already in this transaction,
>  * we need to acquire AccessShareLock.  We arrange for the lock to
>  * be owned by the top transaction, so that we don't need to do it
>  * more than once per xact.
>  */
> 
> This'd probably need to be removed, as we'd otherwise would get very
> weird semantics around aborted subxacts.

Can you explain in more detail what you mean by this?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-05-11T20:27:48Z

On 5/10/17 12:24, Andres Freund wrote:
> Upthread I theorized whether
> that's actually still meaningful given fastpath locking and such, but I
> guess we'll have to evaluate that.

I did some testing.

I ran this script

CREATE SEQUENCE seq1;

DO LANGUAGE plpythonu $$
plan = plpy.prepare("SELECT nextval('seq1')")
for i in range(0, 10000000):
    plpy.execute(plan)
$$;

and timed the "DO".

I compared 9.1 (pre fast-path locking) with 9.2 (with fast-path locking).

I compared the stock releases with a patched version that replaces the
body of open_share_lock() with just

    return relation_open(seq->relid, AccessShareLock);

First, a single session:

9.1 unpatched
Time: 57634.098 ms

9.1 patched
Time: 58674.655 ms

(These were within each other's variance over several runs.)

9.2 unpatched
Time: 64868.305 ms

9.2 patched
Time: 60585.317 ms

(So without contention fast-path locking beats the extra dance that
open_share_lock() does.)

Then, with four concurrent sessions (one timed, three just running):

9.1 unpatched
Time: 73123.661 ms

9.1 patched
Time: 78019.079 ms

So the "hack" avoids the slowdown from contention here.

9.2 unpatched
Time: 73308.873 ms

9.2 patched
Time: 70353.971 ms

Here, fast-path locking beats out the "hack".

If I add more concurrent sessions, everything gets much slower but the
advantage of the patched version stays about the same.  But I can't
reliably test that on this hardware.

(One wonders whether these differences remain relevant if this is run
within the context of an INSERT or COPY instead of just a tight loop.)

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-11T20:34:33Z

On 2017-05-11 11:35:22 -0400, Peter Eisentraut wrote:
> On 5/10/17 12:24, Andres Freund wrote:
> > The issue isn't the strength, but that we currently have this weird
> > hackery around open_share_lock():
> > /*
> >  * Open the sequence and acquire AccessShareLock if needed
> >  *
> >  * If we haven't touched the sequence already in this transaction,
> >  * we need to acquire AccessShareLock.  We arrange for the lock to
> >  * be owned by the top transaction, so that we don't need to do it
> >  * more than once per xact.
> >  */
> > 
> > This'd probably need to be removed, as we'd otherwise would get very
> > weird semantics around aborted subxacts.
> 
> Can you explain in more detail what you mean by this?

Well, right now we don't do proper lock-tracking for sequences, always
assigning them to the toplevel transaction.  But that doesn't seem
proper when nextval() would conflict with ALTER SEQUENCE et al, because
then locks would continue to be held by aborted savepoints.

- Andres

Re: [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-11T20:35:14Z

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 5/10/17 12:24, Andres Freund wrote:
>> Upthread I theorized whether
>> that's actually still meaningful given fastpath locking and such, but I
>> guess we'll have to evaluate that.

> [ with or without contention, fast-path locking beats the extra dance that
> open_share_lock() does. ]

That is pretty cool.  It would be good to verify the same on master,
but assuming it holds up, I think it's ok to remove open_share_lock().

			regards, tom lane

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-11T20:40:12Z

Hi,

On 2017-05-11 16:27:48 -0400, Peter Eisentraut wrote:
> On 5/10/17 12:24, Andres Freund wrote:
> > Upthread I theorized whether
> > that's actually still meaningful given fastpath locking and such, but I
> > guess we'll have to evaluate that.
> 
> I did some testing.

That's with the open_share_lock stuff ripped out entirely, replaced by a
plain lock acquisition within the current subxact?

> (These were within each other's variance over several runs.)
> 
> 9.2 unpatched
> Time: 64868.305 ms
> 
> 9.2 patched
> Time: 60585.317 ms
> 
> (So without contention fast-path locking beats the extra dance that
> open_share_lock() does.)

That's kind of surprising, I really wouldn't have thought it'd be faster
without.  I guess it's the overhead of sigsetjmp().  Cool.

- Andres

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-11T21:21:18Z

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> I ran this script

> CREATE SEQUENCE seq1;

> DO LANGUAGE plpythonu $$
> plan = plpy.prepare("SELECT nextval('seq1')")
> for i in range(0, 10000000):
>     plpy.execute(plan)
> $$;

> and timed the "DO".

It occurred to me that plpy.execute is going to run a subtransaction for
each call, which makes this kind of a dubious test case, both because of
the extra subtransaction overhead and because subtransaction lock
ownership effects will possibly skew the results.  So I tried to
reproduce the test using plain plpgsql,

CREATE SEQUENCE IF NOT EXISTS seq1;

\timing on

do $$
declare x int;
begin
  for i in 0 .. 10000000 loop
    x := nextval('seq1');
  end loop;
end
$$;

On HEAD, building without cassert, I got a fairly reproducible result
of about 10.6 seconds.  I doubt my machine is 6X faster than yours,
so this indicates that the subtransaction overhead is pretty real.

> I compared the stock releases with a patched version that replaces the
> body of open_share_lock() with just
>     return relation_open(seq->relid, AccessShareLock);

Hm.  I don't think that's a sufficient code change, because if you do it
like that then the lock remains held after nextval() returns.  This means
that (a) subsequent calls aren't hitting the shared lock manager at all,
they're just incrementing a local lock counter, and whether it would be
fastpath or not is irrelevant; and (b) this means that the semantic issues
Andres is worried about remain in place, because we will hold the lock
till transaction end.

I experimented with something similar, just replacing open_share_lock
as above, and I got runtimes of just about 12 seconds, which surprised
me a bit.  I'd have thought the locallock-already-exists code path
would be faster than that.

I then further changed the code so that nextval_internal ends with
"relation_close(seqrel, AccessShareLock);" rather than NoLock,
so that the lock is actually released between calls.  This boosted
the runtime up to 15.5 seconds, or a 50% penalty over HEAD.

My conclusion is that in low-overhead cases, such as using a sequence
to assign default values during COPY IN, the percentage overhead from
acquiring and releasing the lock could be pretty dire.  Still, we might
not have much choice if we want nice semantics.

			regards, tom lane

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-11T21:28:12Z

Andres Freund <andres@anarazel.de> writes:
> On 2017-05-11 16:27:48 -0400, Peter Eisentraut wrote:
>> (So without contention fast-path locking beats the extra dance that
>> open_share_lock() does.)

> That's kind of surprising, I really wouldn't have thought it'd be faster
> without.  I guess it's the overhead of sigsetjmp().  Cool.

My results (posted nearby) lead me to suspect that the improvement
Peter sees from 9.1 to 9.2 has little to do with fastpath locking
and a lot to do with some improvement or other in subtransaction
lock management.

			regards, tom lane

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-11T21:28:39Z

On 2017-05-11 17:21:18 -0400, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > I ran this script
> 
> > CREATE SEQUENCE seq1;
> 
> > DO LANGUAGE plpythonu $$
> > plan = plpy.prepare("SELECT nextval('seq1')")
> > for i in range(0, 10000000):
> >     plpy.execute(plan)
> > $$;
> 
> > and timed the "DO".
> 
> It occurred to me that plpy.execute is going to run a subtransaction for
> each call, which makes this kind of a dubious test case, both because of
> the extra subtransaction overhead and because subtransaction lock
> ownership effects will possibly skew the results.  So I tried to
> reproduce the test using plain plpgsql,
> 
> CREATE SEQUENCE IF NOT EXISTS seq1;
> 
> \timing on
> 
> do $$
> declare x int;
> begin
>   for i in 0 .. 10000000 loop
>     x := nextval('seq1');
>   end loop;
> end
> $$;
> 
> On HEAD, building without cassert, I got a fairly reproducible result
> of about 10.6 seconds.  I doubt my machine is 6X faster than yours,
> so this indicates that the subtransaction overhead is pretty real.

Isn't that pretty much the point?  The whole open_share_lock()
optimization looks like it really only can make a difference with
subtransactions?


> > I compared the stock releases with a patched version that replaces the
> > body of open_share_lock() with just
> >     return relation_open(seq->relid, AccessShareLock);
> 
> Hm.  I don't think that's a sufficient code change, because if you do it
> like that then the lock remains held after nextval() returns.

Hm?  That's not new, is it?  We previously did a LockRelationOid(seq->relid) and
then relation_open(seq->relid, NoLock)?


> This means
> that (a) subsequent calls aren't hitting the shared lock manager at all,
> they're just incrementing a local lock counter, and whether it would be
> fastpath or not is irrelevant; and (b) this means that the semantic issues
> Andres is worried about remain in place, because we will hold the lock
> till transaction end.

My concern was aborted subtransactions, and those should release their
lock via resowner stuff at abort?


> I experimented with something similar, just replacing open_share_lock
> as above, and I got runtimes of just about 12 seconds, which surprised
> me a bit.

Yea, I suspect we'd need something like the current open_share_lock,
except with the resowner trickery removed.


> I'd have thought the locallock-already-exists code path would be
> faster than that.

It's a hash-table lookup, via dynahash no less, that's definitley going
to be more expensive than a single seq->lxid != thislxid...


Greetings,

Andres Freund

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-12T03:59:56Z

Andres Freund <andres@anarazel.de> writes:
> On 2017-05-11 17:21:18 -0400, Tom Lane wrote:
>> I doubt my machine is 6X faster than yours,
>> so this indicates that the subtransaction overhead is pretty real.

> Isn't that pretty much the point?  The whole open_share_lock()
> optimization looks like it really only can make a difference with
> subtransactions?

Uh, no; I'm pretty sure that that code is older than subtransactions.
The point of it is to avoid taking and releasing a lock over and over
within a single transaction.

>> Hm.  I don't think that's a sufficient code change, because if you do it
>> like that then the lock remains held after nextval() returns.

> Hm?  That's not new, is it?  We previously did a LockRelationOid(seq->relid) and
> then relation_open(seq->relid, NoLock)?

Right, but the existing code is *designed* to hold the lock till end of
top-level transaction, regardless of what happens in any subtransaction.
My understanding of your complaint is that you do not think that's OK
for any lock stronger than AccessShareLock.

			regards, tom lane

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-05-12T14:20:02Z

On 5/11/17 16:34, Andres Freund wrote:
>>> This'd probably need to be removed, as we'd otherwise would get very
>>> weird semantics around aborted subxacts.
>> Can you explain in more detail what you mean by this?
> Well, right now we don't do proper lock-tracking for sequences, always
> assigning them to the toplevel transaction.  But that doesn't seem
> proper when nextval() would conflict with ALTER SEQUENCE et al, because
> then locks would continue to be held by aborted savepoints.

I see what you mean here.  We already have this issue with DROP SEQUENCE.

While it would be nice to normalize this, I think it's quite esoteric.
I doubt users have any specific expectations how sequences behave in
aborted subtransactions.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-05-12T14:28:10Z

On 5/11/17 17:28, Andres Freund wrote:
> Isn't that pretty much the point?  The whole open_share_lock()
> optimization looks like it really only can make a difference with
> subtransactions?

Yeah that confused me too.  That keep-the-lock-for-the-whole-transaction
logic was introduced in a2597ef17958e75e7ba26507dc407249cc9e7134 around
7.3, way before subtransactions (8.0).  The point there was apparently
just to avoid hitting the lock manager on subsequent calls.  That code
has since been moved around end refactored many times.  When
subtransactions were introduced, the current logic of keeping the lock
across subtransactions was added in order to keep the original intent.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-05-12T14:35:56Z

On 5/11/17 23:59, Tom Lane wrote:
> Right, but the existing code is *designed* to hold the lock till end of
> top-level transaction, regardless of what happens in any subtransaction.
> My understanding of your complaint is that you do not think that's OK
> for any lock stronger than AccessShareLock.

What he is saying (I believe) is: Because of that custom logic to hold
the lock until the end of the transaction across subtransactions, you
will keep holding the lock even if a subtransaction aborts, which is
nonstandard behavior.  Previously, nobody cared, because nobody else was
locking sequences.  But now we're proposing to make ALTER SEQUENCE lock
the sequence, so this behavior would be visible:

S1: BEGIN;
S1: SAVEPOINT s1;
S1: SELECT nextval('seq1');
S1: ROLLBACK TO SAVEPOINT s1;
S2: ALTER SEQUENCE seq1 MAXVALUE 100;  -- would now wait for S1 commit

My amendment to that is that "previously nobody cared" is not quite
correct, because the same already happens currently with DROP SEQUENCE.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Concurrent ALTER SEQUENCE RESTART Regression

Noah Misch <noah@leadboat.com> — 2017-05-15T03:50:36Z

On Mon, May 08, 2017 at 12:28:38PM -0400, Peter Eisentraut wrote:
> On 5/8/17 02:58, Noah Misch wrote:
> > IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past due
> > for your status update.  Please reacquaint yourself with the policy on open
> > item ownership[1] and then reply immediately.  If I do not hear from you by
> > 2017-05-09 07:00 UTC, I will transfer this item to release management team
> > ownership without further notice.
> 
> I got side-tracked by the minor releases preparation.  I will work on
> this now and report on Wednesday.

IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past due
for your status update.  Please reacquaint yourself with the policy on open
item ownership[1] and then reply immediately.  If I do not hear from you by
2017-05-16 04:00 UTC, I will transfer this item to release management team
ownership without further notice.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

Re: Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-05-15T14:34:02Z

On 5/10/17 09:12, Michael Paquier wrote:
> Looking at 0001 and 0002... So you are correctly blocking nextval()
> when ALTER SEQUENCE holds a lock on the sequence object. And
> concurrent calls of nextval() don't conflict. As far as I can see this
> matches the implementation of 3.
> 
> Here are some minor comments.

Committed after working in your comments.  Thanks!

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-18T20:54:28Z

On 2017-05-15 10:34:02 -0400, Peter Eisentraut wrote:
> On 5/10/17 09:12, Michael Paquier wrote:
> > Looking at 0001 and 0002... So you are correctly blocking nextval()
> > when ALTER SEQUENCE holds a lock on the sequence object. And
> > concurrent calls of nextval() don't conflict. As far as I can see this
> > matches the implementation of 3.
> > 
> > Here are some minor comments.
> 
> Committed after working in your comments.  Thanks!

There's still weird behaviour, unfortunately.  If you do an ALTER
SEQUENCE changing minval/maxval w/ restart in a transaction, and abort,
you'll a) quite possibly not be able to use the sequence anymore,
because it may of bounds b) DDL still isn't transactional.

At the very least that'd need to be documented.

- Andres

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Robert Haas <robertmhaas@gmail.com> — 2017-05-19T12:31:15Z

On Thu, May 18, 2017 at 4:54 PM, Andres Freund <andres@anarazel.de> wrote:
> There's still weird behaviour, unfortunately.  If you do an ALTER
> SEQUENCE changing minval/maxval w/ restart in a transaction, and abort,
> you'll a) quite possibly not be able to use the sequence anymore,
> because it may of bounds b) DDL still isn't transactional.

Your emails would be a bit easier to understand if you included a few
more words.

I'm guessing "may of bounds" is supposed to say "may be out of bounds"?

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

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-22T15:42:27Z

On 2017-05-19 08:31:15 -0400, Robert Haas wrote:
> On Thu, May 18, 2017 at 4:54 PM, Andres Freund <andres@anarazel.de> wrote:
> > There's still weird behaviour, unfortunately.  If you do an ALTER
> > SEQUENCE changing minval/maxval w/ restart in a transaction, and abort,
> > you'll a) quite possibly not be able to use the sequence anymore,
> > because it may of bounds b) DDL still isn't transactional.
>
> Your emails would be a bit easier to understand if you included a few
> more words.

Yea - I'd explained this one already somewhere upthread, and I'd hoped
it'd be enough, but I probably was wrong.


> I'm guessing "may of bounds" is supposed to say "may be out of bounds"?

Yes.


Consider a scenario like:

S1: CREATE SEQUENCE oobounds MINVALUE 1 START 1;
S1: SELECT nextval('oobounds'); -> 1
S2: BEGIN;
S2: ALTER SEQUENCE oobounds MAXVALUE -10 START -10 MINVALUE -1000 INCREMENT BY -1 RESTART;
S2: SELECT nextval('oobounds'); -> -10
S2: ROLLBACK;
S1: SELECT * FROM pg_sequence WHERE seqrelid = 'oobounds'::regclass;
┌──────────┬──────────┬──────────┬──────────────┬─────────────────────┬────────┬──────────┬──────────┐
│ seqrelid │ seqtypid │ seqstart │ seqincrement │       seqmax        │ seqmin │ seqcache │ seqcycle │
├──────────┼──────────┼──────────┼──────────────┼─────────────────────┼────────┼──────────┼──────────┤
│   203401 │       20 │        1 │            1 │ 9223372036854775807 │      1 │        1 │ f        │
└──────────┴──────────┴──────────┴──────────────┴─────────────────────┴────────┴──────────┴──────────┘
S1: SELECT nextval('oobounds'); -> -9

Ooops.

Two issues: Firstly, we get a value smaller than seqmin, obviously
that's not ok. But even if we'd error out, it'd imo still not be ok,
because we have a command that behaves partially transactionally
(keeping the seqmax/min transactionally), partially not (keeping the
current sequence state at -9).

- Andres

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Robert Haas <robertmhaas@gmail.com> — 2017-05-24T02:47:07Z

On Mon, May 22, 2017 at 11:42 AM, Andres Freund <andres@anarazel.de> wrote:
> Ooops.
>
> Two issues: Firstly, we get a value smaller than seqmin, obviously
> that's not ok. But even if we'd error out, it'd imo still not be ok,
> because we have a command that behaves partially transactionally
> (keeping the seqmax/min transactionally), partially not (keeping the
> current sequence state at -9).

I don't really agree that this is broken.  In 9.6, the update appears
to be fully non-transactional, which you could argue is more
consistent, but I don't think I'd agree.  In other cases where we
can't perform an operation transactionally, we call
PreventTransactionChain(), but in 9.6, ALTER SEQUENCE oobounds
MAXVALUE -10 START -10 MINVALUE -1000 INCREMENT BY -1 RESTART seems to
be allowed in a transaction but a subsequent ROLLBACK has no effect,
which seems fairly awful.

I think it's important to keep in mind that nextval() more or less has
to be non-transactional.  From a certain point of view, that's why we
have sequences.  If nextval() didn't notice actions performed by
uncommitted transactions, then sequences wouldn't deliver unique
values; if it blocked waiting to see whether the other transaction
committed and didn't return a value until it either committed or
aborted, then sequences would have terrible performance.  However,
just because nextval() has to be non-transactional doesn't mean that
ALL sequence operations have to be non-transactional.

I don't really know whether the behavior Peter has chosen here is the
best possible choice, so I'm not intending to mount any sort of
vigorous defense of it. However, this thread started with the
complaint about occasional "ERROR:  tuple concurrently updated"
messages; in my opinion, any such behavior in new code is
unacceptable, and now it's been fixed.  "Totally broken" != "not the
behavior I prefer".

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

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-24T03:25:17Z

On 2017-05-23 22:47:07 -0400, Robert Haas wrote:
> On Mon, May 22, 2017 at 11:42 AM, Andres Freund <andres@anarazel.de> wrote:
> > Ooops.
> >
> > Two issues: Firstly, we get a value smaller than seqmin, obviously
> > that's not ok. But even if we'd error out, it'd imo still not be ok,
> > because we have a command that behaves partially transactionally
> > (keeping the seqmax/min transactionally), partially not (keeping the
> > current sequence state at -9).
> 
> I don't really agree that this is broken.

Just a quick clarification question: You did notice that nextval() in S1
after the rollback returned -9, despite seqmin being 0?  I can see
erroring out being acceptable, but returning flat out wrong values....?

- Andres

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Robert Haas <robertmhaas@gmail.com> — 2017-05-24T10:57:08Z

On Tue, May 23, 2017 at 11:25 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2017-05-23 22:47:07 -0400, Robert Haas wrote:
>> On Mon, May 22, 2017 at 11:42 AM, Andres Freund <andres@anarazel.de> wrote:
>> > Ooops.
>> >
>> > Two issues: Firstly, we get a value smaller than seqmin, obviously
>> > that's not ok. But even if we'd error out, it'd imo still not be ok,
>> > because we have a command that behaves partially transactionally
>> > (keeping the seqmax/min transactionally), partially not (keeping the
>> > current sequence state at -9).
>>
>> I don't really agree that this is broken.
>
> Just a quick clarification question: You did notice that nextval() in S1
> after the rollback returned -9, despite seqmin being 0?  I can see
> erroring out being acceptable, but returning flat out wrong values....?

I did see that.  I'm unclear what you think it should do instead.  I
mean, it could notice that cur_val < min_val and return min_val, but
that doesn't have a very good chance of being correct either.  I
suppose the aborted transaction could try to restore the old cur_val
when it rolls back, but that's clearly the wrong thing when there's no
DDL involved (plus I'm not sure it would even be safe to try to do
that).  Do you have an idea?

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

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-24T13:04:41Z


On May 24, 2017 6:57:08 AM EDT, Robert Haas <robertmhaas@gmail.com> wrote:
>On Tue, May 23, 2017 at 11:25 PM, Andres Freund <andres@anarazel.de>
>wrote:
>> On 2017-05-23 22:47:07 -0400, Robert Haas wrote:
>>> On Mon, May 22, 2017 at 11:42 AM, Andres Freund <andres@anarazel.de>
>wrote:
>>> > Ooops.
>>> >
>>> > Two issues: Firstly, we get a value smaller than seqmin, obviously
>>> > that's not ok. But even if we'd error out, it'd imo still not be
>ok,
>>> > because we have a command that behaves partially transactionally
>>> > (keeping the seqmax/min transactionally), partially not (keeping
>the
>>> > current sequence state at -9).
>>>
>>> I don't really agree that this is broken.
>>
>> Just a quick clarification question: You did notice that nextval() in
>S1
>> after the rollback returned -9, despite seqmin being 0?  I can see
>> erroring out being acceptable, but returning flat out wrong
>values....?
>
>I did see that.  I'm unclear what you think it should do instead.  I
>mean, it could notice that cur_val < min_val and return min_val, but
>that doesn't have a very good chance of being correct either.  I
>suppose the aborted transaction could try to restore the old cur_val
>when it rolls back, but that's clearly the wrong thing when there's no
>DDL involved (plus I'm not sure it would even be safe to try to do
>that).  Do you have an idea?

At the very least we'll have to error out. That's still not nice usability wise, but it sure beats returning flat out wrong values.

I suspect that the proper fix would be to use a different relfilenode after ddl, when changing the seq file itself (I.e. setval and restart).  That seems like it'd be architecturally more appropriate, but also some work.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Robert Haas <robertmhaas@gmail.com> — 2017-05-24T14:24:19Z

On Wed, May 24, 2017 at 9:04 AM, Andres Freund <andres@anarazel.de> wrote:
> At the very least we'll have to error out. That's still not nice usability wise, but it sure beats returning flat out wrong values.

I'm not sure.  That seems like it might often be worse.  Now you need
manual intervention before anything even has a hope of working.

> I suspect that the proper fix would be to use a different relfilenode after ddl, when changing the seq file itself (I.e. setval and restart).  That seems like it'd be architecturally more appropriate, but also some work.

I can see some advantages to that, but it seems far too late to think
about doing that in v10.

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

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-24T14:32:40Z

On 2017-05-24 10:24:19 -0400, Robert Haas wrote:
> On Wed, May 24, 2017 at 9:04 AM, Andres Freund <andres@anarazel.de> wrote:
> > At the very least we'll have to error out. That's still not nice usability wise, but it sure beats returning flat out wrong values.
> 
> I'm not sure.  That seems like it might often be worse.  Now you need
> manual intervention before anything even has a hope of working.

Well, but then we should just remove minval/maxval if we can't rely on
it.


> > I suspect that the proper fix would be to use a different relfilenode after ddl, when changing the seq file itself (I.e. setval and restart).  That seems like it'd be architecturally more appropriate, but also some work.
> 
> I can see some advantages to that, but it seems far too late to think
> about doing that in v10.

I wonder if that's not actually very little new code, and I think we
might end up regretting having yet another inconsistent set of semantics
in v10, which we'll then end up changing again in v11.

- Andres

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Robert Haas <robertmhaas@gmail.com> — 2017-05-24T14:52:37Z

On Wed, May 24, 2017 at 10:32 AM, Andres Freund <andres@anarazel.de> wrote:
> Well, but then we should just remove minval/maxval if we can't rely on
> it.

That seems like a drastic overreaction to me.

> I wonder if that's not actually very little new code, and I think we
> might end up regretting having yet another inconsistent set of semantics
> in v10, which we'll then end up changing again in v11.

I'm not exercised enough about it to spend time on it or to demand
that Peter do so, but feel free to propose something.

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

Re: [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-05-31T21:24:38Z

On 2017-05-24 10:52:37 -0400, Robert Haas wrote:
> On Wed, May 24, 2017 at 10:32 AM, Andres Freund <andres@anarazel.de> wrote:
> > Well, but then we should just remove minval/maxval if we can't rely on
> > it.
>
> That seems like a drastic overreaction to me.

Well, either they work, or they don't.  But since it turns out to be
easy enough to fix anyway...

> > I wonder if that's not actually very little new code, and I think we
> > might end up regretting having yet another inconsistent set of semantics
> > in v10, which we'll then end up changing again in v11.
>
> I'm not exercised enough about it to spend time on it or to demand
> that Peter do so, but feel free to propose something.

This turns out to be fairly simple patch.  We already do precisely what
I describe when resetting a sequence for TRUNCATE, so it's an already
tested codepath.  It also follows a lot more established practice around
transactional schema changes, so I think that's architecturally better
too.  Peter, to my understanding, agreed with that reasoning at pgcon.

I just have two questions:
1) We've introduced, in 3d092fe540, infrastructure to avoid unnecessary
   catalog updates, in an attemt to fix the "concurrently updated"
   error. But that turned out to not be sufficient anyway, and it bulks
   up the code.  I'd vote for just removing that piece of logic, it
   doesn't buy us anything meaningful, and it's bulky.

2) There's currently logic that takes a lower level lock for ALTER
   SEQUENCE RESET without other options.  I think that's a bit confusing
   with the proposed change, because then it's unclear when ALTER
   SEQUENCE is transactional and when not.  I think it's actually a lot
   easier to understand if we keep nextval()/setval() as
   non-transactional, and ALTER SEQUENCE as transactional.

Comments?

- Andres

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-06-01T00:07:16Z

On 2017-05-31 14:24:38 -0700, Andres Freund wrote:
> On 2017-05-24 10:52:37 -0400, Robert Haas wrote:
> > On Wed, May 24, 2017 at 10:32 AM, Andres Freund <andres@anarazel.de> wrote:
> > > Well, but then we should just remove minval/maxval if we can't rely on
> > > it.
> >
> > That seems like a drastic overreaction to me.
> 
> Well, either they work, or they don't.  But since it turns out to be
> easy enough to fix anyway...
> 
> 
> > > I wonder if that's not actually very little new code, and I think we
> > > might end up regretting having yet another inconsistent set of semantics
> > > in v10, which we'll then end up changing again in v11.
> >
> > I'm not exercised enough about it to spend time on it or to demand
> > that Peter do so, but feel free to propose something.
> 
> This turns out to be fairly simple patch.  We already do precisely what
> I describe when resetting a sequence for TRUNCATE, so it's an already
> tested codepath.  It also follows a lot more established practice around
> transactional schema changes, so I think that's architecturally better
> too.  Peter, to my understanding, agreed with that reasoning at pgcon.
> 
> I just have two questions:
> 1) We've introduced, in 3d092fe540, infrastructure to avoid unnecessary
>    catalog updates, in an attemt to fix the "concurrently updated"
>    error. But that turned out to not be sufficient anyway, and it bulks
>    up the code.  I'd vote for just removing that piece of logic, it
>    doesn't buy us anything meaningful, and it's bulky.
> 
> 2) There's currently logic that takes a lower level lock for ALTER
>    SEQUENCE RESET without other options.  I think that's a bit confusing
>    with the proposed change, because then it's unclear when ALTER
>    SEQUENCE is transactional and when not.  I think it's actually a lot
>    easier to understand if we keep nextval()/setval() as
>    non-transactional, and ALTER SEQUENCE as transactional.
> 
> Comments?

Here's a patch doing what I suggested above.  The second patch addresses
an independent oversight where the post alter hook was invoked before
doing the catalog update.

- Andres

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Robert Haas <robertmhaas@gmail.com> — 2017-06-01T14:51:10Z

On Wed, May 31, 2017 at 8:07 PM, Andres Freund <andres@anarazel.de> wrote:
> Here's a patch doing what I suggested above.  The second patch addresses
> an independent oversight where the post alter hook was invoked before
> doing the catalog update.

0002 is a slam-dunk.  I don't object to 0001 but haven't reviewed it carefully.

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

Re: [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-06-01T17:08:33Z

On 01/06/17 16:51, Robert Haas wrote:
> On Wed, May 31, 2017 at 8:07 PM, Andres Freund <andres@anarazel.de> wrote:
>> Here's a patch doing what I suggested above.  The second patch addresses
>> an independent oversight where the post alter hook was invoked before
>> doing the catalog update.
> 
> 0002 is a slam-dunk.  I don't object to 0001 but haven't reviewed it carefully.
> 

I did go over the code (ie didn't do actual testing), and I like it much
better than the current state. Both in terms of making the behavior more
consistent and the fact that the code is simpler.

So +1 to go ahead with both patches.

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

Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

Andres Freund <andres@anarazel.de> — 2017-06-01T21:23:28Z

On 2017-06-01 19:08:33 +0200, Petr Jelinek wrote:
> On 01/06/17 16:51, Robert Haas wrote:
> > On Wed, May 31, 2017 at 8:07 PM, Andres Freund <andres@anarazel.de> wrote:
> >> Here's a patch doing what I suggested above.  The second patch addresses
> >> an independent oversight where the post alter hook was invoked before
> >> doing the catalog update.
> > 
> > 0002 is a slam-dunk.  I don't object to 0001 but haven't reviewed it carefully.
> > 
> 
> I did go over the code (ie didn't do actual testing), and I like it much
> better than the current state. Both in terms of making the behavior more
> consistent and the fact that the code is simpler.
> 
> So +1 to go ahead with both patches.

Thanks for the look!  I unfortunately forgot to credit you as a
reviewer, sorry for that.

Pushed both.

- Andres

Re: Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-06-13T19:04:35Z

On 5/2/17 12:45, Robert Haas wrote:
> Another thing that doesn't look so good about AlterSequence is that it
> uses RangeVarGetRelid() instead of RangeVarGetsRelidExtended() with
> RangeVarCallbackOwnsRelation or some derivative thereof.  That of
> course means you can obstruct access to a sequence you don't own.

Here is a patch for this.  Note that this is old code, but it seems
worth fixing now while we're at it.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Concurrent ALTER SEQUENCE RESTART Regression

Robert Haas <robertmhaas@gmail.com> — 2017-06-14T17:58:57Z

On Tue, Jun 13, 2017 at 3:04 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 5/2/17 12:45, Robert Haas wrote:
>> Another thing that doesn't look so good about AlterSequence is that it
>> uses RangeVarGetRelid() instead of RangeVarGetsRelidExtended() with
>> RangeVarCallbackOwnsRelation or some derivative thereof.  That of
>> course means you can obstruct access to a sequence you don't own.
>
> Here is a patch for this.  Note that this is old code, but it seems
> worth fixing now while we're at it.

+1.

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

Re: Concurrent ALTER SEQUENCE RESTART Regression

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-06-16T16:15:55Z

On 6/14/17 13:58, Robert Haas wrote:
> On Tue, Jun 13, 2017 at 3:04 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 5/2/17 12:45, Robert Haas wrote:
>>> Another thing that doesn't look so good about AlterSequence is that it
>>> uses RangeVarGetRelid() instead of RangeVarGetsRelidExtended() with
>>> RangeVarCallbackOwnsRelation or some derivative thereof.  That of
>>> course means you can obstruct access to a sequence you don't own.
>>
>> Here is a patch for this.  Note that this is old code, but it seems
>> worth fixing now while we're at it.
> 
> +1.

committed

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services