Review: rollback sequence reset for TRUNCATE ... RESTART IDENTITY

Hi, Here is my review of 'rollback sequence reset for TRUNCATE ... RESTART
IDENTITY' patch.

- Is the patch in context diff format?
It's in git diff format. I guess it's OK ?

- Does it apply cleanly to the current git master?
Yes

- Does it include reasonable tests, necessary doc patches, etc?
Doc: Yes, it removes the warning about TRUNCATE ... RESTART IDENTITY, which is
the point of the patch
Tests: There is a new regression test added for restart identity. And 'make
check' passes (tested on linux).

- Usability review (skills needed: test-fu, ability to find and read spec)

- Read what the patch is supposed to do, and consider:

- Does the patch actually implement that?
Yes.

- Do we want that?
I think so, it removes a trap limitation of truncate

- Do we already have it?
No

- Does it follow SQL spec, or the community-agreed behavior?
I think so

- Does it include pg_dump support (if applicable)?
Not applicable

- Are there dangers?
Not that I think of

- Have all the bases been covered?
I think so


- Feature test (skills needed: patch, configure, make, pipe errors to log)

- Apply the patch, compile it and test:
- Does the feature work as advertised?
Yes. It works consistently, isn't fooled by savepoints or multiple serials in
a table, or concurrent transactions

- Are there corner cases the author has failed to consider?
I don't think so

- Are there any assertion failures or crashes?
No


Performance review (skills needed: ability to time performance)

- Does the patch slow down simple tests?
No

- If it claims to improve performance, does it?
Not applicable

- Does it slow down other things?
No

- Read the changes to the code in detail and consider:
- Does it follow the project coding guidelines?
Yes

- Are there portability issues?
I don't think so

- Will it work on Windows/BSD etc?
Yes. There is no OS-specific code added.

- Are the comments sufficient and accurate?
Yes

- Does it do what it says, correctly?
Yes

- Does it produce compiler warnings?
No

- Can you make it crash?
No

- Consider the changes to the code in the context of the project as a whole:
- Is everything done in a way that fits together coherently with other
features/modules?
Yes

- Are there interdependencies that can cause problems?
No

On Wed, Nov 17, 2010 at 8:13 AM, Marc Cousin <cousinmarc(at)gmail(dot)com> wrote:
>
> - Does the feature work as advertised?
>
> Yes. It works consistently, isn't fooled by savepoints or multiple serials
> in a table, or concurrent transactions
>

i haven't tested this nor readed the patch but i wondering what
happens in the presence of a prepared transaction (2PC), did you try
with concurrent transactions with different serialization levels?

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte y capacitación de PostgreSQL

The Wednesday 17 November 2010 15:50:36, Jaime Casanova wrote :
> On Wed, Nov 17, 2010 at 8:13 AM, Marc Cousin <cousinmarc(at)gmail(dot)com> wrote:
> > - Does the feature work as advertised?
> >
> > Yes. It works consistently, isn't fooled by savepoints or multiple
> > serials in a table, or concurrent transactions
>
> i haven't tested this nor readed the patch but i wondering what
> happens in the presence of a prepared transaction (2PC), did you try
> with concurrent transactions with different serialization levels?
I haven't tested with 2PC.

I didn't check with different isolations levels either.

I just verified that locking was happening as it should : truncate is blocked
by a transaction already locking the table with an AccessShareLock and vice-
versa.

And that Rollbacking and rollbacking to savepoint restores the sequence to the
correct state : the sequence isn't restored to its value at the savepoint, but
at its last value before the truncate.

I don't see a special test-case with different isolation levels or 2PC. What
do you have in mind ?

Marc Cousin <cousinmarc(at)gmail(dot)com> writes:
>>> - Does the feature work as advertised?
>>>
>>> Yes. It works consistently, isn't fooled by savepoints or multiple
>>> serials in a table, or concurrent transactions

I think there's a rather nasty problem here, which is what to do with
the cached nextval/currval state. As submitted, the patch does the same
thing as ALTER SEQUENCE RESTART (to wit, clear any cached unissued
nextval values, but don't touch currval) at the time of resetting the
sequence. That's fine, but what if the transaction later rolls back?
The cached state is untouched by rollback, so if the transaction had
done any nextval()s meanwhile, the cache will be out of step with the
rolled-back sequence contents.

We never had to worry about this before because sequence operations
didn't roll back, by definition. If we're going to add a situation
where they do roll back, we need to consider the case.

I think we can arrange to clear cached unissued values on the next
attempt to nextval() the sequence, by dint of adding the relfilenode
to SeqTable entries and clearing cached state whenever we note that
it doesn't match the current relfilenode of the sequence. However,
I'm unsure what ought to happen to currval. It doesn't seem too
practical to try to roll it back to its pre-transaction value.
Should we leave it alone (ie, possibly reflecting a value that was
assigned inside the failed transaction)? The other alternative would
be to clear it as though nextval had never been issued at all in the
session.

Thoughts?

regards, tom lane

The Wednesday 17 November 2010 19:41:19, Tom Lane wrote :
> Marc Cousin <cousinmarc(at)gmail(dot)com> writes:
> >>> - Does the feature work as advertised?
> >>>
> >>> Yes. It works consistently, isn't fooled by savepoints or multiple
> >>> serials in a table, or concurrent transactions
>
> I think there's a rather nasty problem here, which is what to do with
> the cached nextval/currval state. As submitted, the patch does the same
> thing as ALTER SEQUENCE RESTART (to wit, clear any cached unissued
> nextval values, but don't touch currval) at the time of resetting the
> sequence. That's fine, but what if the transaction later rolls back?
> The cached state is untouched by rollback, so if the transaction had
> done any nextval()s meanwhile, the cache will be out of step with the
> rolled-back sequence contents.

Yes, I completely missed testing with non default cache value. And it fails,
of course, some values are generated a second time twice after a rollback

>
> We never had to worry about this before because sequence operations
> didn't roll back, by definition. If we're going to add a situation
> where they do roll back, we need to consider the case.
>
> I think we can arrange to clear cached unissued values on the next
> attempt to nextval() the sequence, by dint of adding the relfilenode
> to SeqTable entries and clearing cached state whenever we note that
> it doesn't match the current relfilenode of the sequence. However,
> I'm unsure what ought to happen to currval. It doesn't seem too
> practical to try to roll it back to its pre-transaction value.
> Should we leave it alone (ie, possibly reflecting a value that was
> assigned inside the failed transaction)? The other alternative would
> be to clear it as though nextval had never been issued at all in the
> session.

Should currval really be used after a failed transaction ? Right now, we can
have a value that has been generated inside a rollbacked transaction too. I'd
vote for leave it alone.

On 10-11-17 03:00 PM, Marc Cousin wrote:
> The Wednesday 17 November 2010 19:41:19, Tom Lane wrote :
>> Marc Cousin<cousinmarc(at)gmail(dot)com> writes:
>>>>> - Does the feature work as advertised?
>>>>>
>>>>> Yes. It works consistently, isn't fooled by savepoints or multiple
>>>>> serials in a table, or concurrent transactions
>>
>> I think there's a rather nasty problem here, which is what to do with
>> the cached nextval/currval state. As submitted, the patch does the same
>> thing as ALTER SEQUENCE RESTART (to wit, clear any cached unissued
>> nextval values, but don't touch currval) at the time of resetting the
>> sequence. That's fine, but what if the transaction later rolls back?
>> The cached state is untouched by rollback, so if the transaction had
>> done any nextval()s meanwhile, the cache will be out of step with the
>> rolled-back sequence contents.
>
> Yes, I completely missed testing with non default cache value. And it fails,
> of course, some values are generated a second time twice after a rollback
>

I will look at addressing this in an updated patch.

>>
>> We never had to worry about this before because sequence operations
>> didn't roll back, by definition. If we're going to add a situation
>> where they do roll back, we need to consider the case.
>>
>> I think we can arrange to clear cached unissued values on the next
>> attempt to nextval() the sequence, by dint of adding the relfilenode
>> to SeqTable entries and clearing cached state whenever we note that
>> it doesn't match the current relfilenode of the sequence. However,
>> I'm unsure what ought to happen to currval. It doesn't seem too
>> practical to try to roll it back to its pre-transaction value.
>> Should we leave it alone (ie, possibly reflecting a value that was
>> assigned inside the failed transaction)? The other alternative would
>> be to clear it as though nextval had never been issued at all in the
>> session.
>
>
> Should currval really be used after a failed transaction ? Right now, we can
> have a value that has been generated inside a rollbacked transaction too. I'd
> vote for leave it alone.
>

I agree probably shouldn't be using curval after a failed transaction
which is why having it return as if it hadn't been issued sounds like a
more reasonable behaviour. If an application tries a currval following
the rollback then at least the application won't get a bogus value. It
is better to return an error than to let the application continuing on
thinking it has a sequence value that won't be (or has not) been
assigned to some other session.

Steve Singer <ssinger(at)ca(dot)afilias(dot)info> writes:
> I will look at addressing this in an updated patch.

I've nearly finished revising the patch already, don't worry about it.

>> Should currval really be used after a failed transaction ? Right now, we can
>> have a value that has been generated inside a rollbacked transaction too. I'd
>> vote for leave it alone.

> I agree probably shouldn't be using curval after a failed transaction

Well, people can do that now, and it doesn't throw an error. I'm
inclined to agree with Marc that just leaving it alone (ie, it returns
the last value produced, whether the transaction rolls back or not)
is the best thing. There's inherently going to be some inconsistency
here, since there's no such thing as a transactional sequence change
otherwise. I don't see the point of going way out of our way to move
the inconsistencies around.

regards, tom lane