Re: Concurrent ALTER SEQUENCE RESTART Regression

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