Re: Concurrent ALTER SEQUENCE RESTART Regression

On Mon, May 8, 2017 at 8:43 AM, Andres Freund <andres(at)anarazel(dot)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