Concurrent ALTER SEQUENCE RESTART Regression
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
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
Attachments:
alter-seq-lock.patchapplication/octet-stream; name=alter-seq-lock.patchDownload+11-2
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>
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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.
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
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
