Re: Concurrent ALTER SEQUENCE RESTART Regression

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(at)citusdata(dot)com

On Apr 24, 2017, at 10:26 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

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

On Tue, Apr 25, 2017 at 4:52 AM, Jason Petersen <jason(at)citusdata(dot)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>