Re: [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

On 2017-05-24 10:52:37 -0400, Robert Haas wrote:
> On Wed, May 24, 2017 at 10:32 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > Well, but then we should just remove minval/maxval if we can't rely on
> > it.
>
> That seems like a drastic overreaction to me.

Well, either they work, or they don't. But since it turns out to be
easy enough to fix anyway...

> > I wonder if that's not actually very little new code, and I think we
> > might end up regretting having yet another inconsistent set of semantics
> > in v10, which we'll then end up changing again in v11.
>
> I'm not exercised enough about it to spend time on it or to demand
> that Peter do so, but feel free to propose something.

This turns out to be fairly simple patch. We already do precisely what
I describe when resetting a sequence for TRUNCATE, so it's an already
tested codepath. It also follows a lot more established practice around
transactional schema changes, so I think that's architecturally better
too. Peter, to my understanding, agreed with that reasoning at pgcon.

I just have two questions:
1) We've introduced, in 3d092fe540, infrastructure to avoid unnecessary
catalog updates, in an attemt to fix the "concurrently updated"
error. But that turned out to not be sufficient anyway, and it bulks
up the code. I'd vote for just removing that piece of logic, it
doesn't buy us anything meaningful, and it's bulky.

2) There's currently logic that takes a lower level lock for ALTER
SEQUENCE RESET without other options. I think that's a bit confusing
with the proposed change, because then it's unclear when ALTER
SEQUENCE is transactional and when not. I think it's actually a lot
easier to understand if we keep nextval()/setval() as
non-transactional, and ALTER SEQUENCE as transactional.

Comments?

- Andres