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
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 Peterin CC) that has changed the way sequence metadata is handled. Thefailure happens in CatalogTupleUpdate() which usessimple_heap_update() that caller can only use if updates areconcurrent safe. But since 1753b1b0 that is not true as the sequenceis locked with AccessShareLock. The correct answer is to use astronger lock, but not something that would block attempts to readnext sequence values as it is assumed that ALTER SEQUENCE should benon-disruptive. Hence I think that ShareUpdateExclusiveLock is a goodmatch what what we are looking for here: protect concurrent updates ofthe sequence metadata for other ALTER SEQUENCE commands but not blockother transactions reading this data.Attached is a patch to address the issue.And looking at things deeper... I am seeing at least one other ALTERcommand that is not concurrent safe... I'll keep that for anotherthread because it is an older bug.-- Michael
<alter-seq-lock.patch>
