Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression

On 2017-05-31 14:24:38 -0700, Andres Freund wrote:
> On 2017-05-24 10:52:37 -0400, Robert Haas wrote:
> > On Wed, May 24, 2017 at 10:32 AM, Andres Freund <andres@anarazel.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?

Here's a patch doing what I suggested above.  The second patch addresses
an independent oversight where the post alter hook was invoked before
doing the catalog update.

- Andres