Re: Concurrent ALTER SEQUENCE RESTART Regression

Hi,

On 2017-05-02 11:19:08 -0400, Peter Eisentraut wrote:
> On 5/2/17 11:07, Andres Freund wrote:
> > On 2017-05-02 10:53:19 -0400, Peter Eisentraut wrote:
> >> This message comes from the pg_sequence catalog update. But in the case
> >> of the RESTART clause, you don't need to update the catalog, because it
> >> just needs to write to the sequence's relation. So I have tweaked the
> >> code a little to omit the catalog update if it's not needed. Your test
> >> case works without errors now.
> >
> > Wait, how does this *actually* solve anything, but scratch at the
> > surface? You just add a MAXVALUE and it starts failing (and not being
> > adhered to) again?
>
> The just committed patch somewhat disentangles the transactional from
> the nontransactional parts of ALTER SEQUENCE.
>
> RESTART is a nontransactional action. Now you get the same concurrency
> behavior for RESTART as you would for setval and nextval. This was not
> the case prior to the fix.

Well, except if specified together with other options.

> Other clauses such as MAXVALUE are transactional actions.

Yes, and that's a complete mess. Because RESTART relies on the current
catalog state in the restarting transaction, but affect other
transactions immediately, you can all kind of weird things.

S1: CREATE SEQUENCE foo START WITH 100 MINVALUE 10 MAXVALUE 1000;
S1: SELECT nextval('foo');
┌─────────┐
│ nextval │
├─────────┤
│ 100 │
└─────────┘
S1: BEGIN;
S1: ALTER SEQUENCE foo RESTART START WITH 1 MINVALUE 1 MAXVALUE 1000;
S2: SELECT nextval('foo');
┌─────────┐
│ nextval │
├─────────┤
│ 1 │
└─────────┘
(1 row)
-- uh, so we get a value that's below minvalue. Borked transactionality
S1: ROLLBACK;
S1: SELECT nextval('foo');
┌─────────┐
│ nextval │
├─────────┤
│ 2 │
└─────────┘
-- uh, so even worse. We have transactional behaviour (minvalue
change), that's not even transactional. And we'll continue to return
values that aren't legal to the sequence's definition.

In other words, you made alter sequence transactional, except randomly
not. I'm increasingly thinking that this change hasn't gotten even
close enough scrutiny and should just be reverted.

> You get the same concurrency behavior as for most DDL in PostgreSQL.

I don't see any evidence of that.

> You could
> argue that the RowExclusiveLock on pg_sequence in not enough and should
> be perhaps ShareRowExclusiveLock to avoid "tuple concurrently updated"
> messages. But just over in 521fd4795e3ec3d0b263b62e5eb58e1557be9c86 it
> was argued that that sort of thing was undesirable.

No, we'd need proper locking on the sequence object, not on pg_sequence.
Alter table, and a lot of other commands, primarily rely on proper
locking of the underlying object, not on heavyweight locks of pg_class.

- Andres