Re: smallserial / serial2

From: Brar Piening <brar(at)gmx(dot)de>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: smallserial / serial2
Date: 2011-06-08 22:36:44
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Tue, 7 Jun 2011 20:35:12 -0400, Mike Pultz <mike(at)mikepultz(dot)com> wrote:
> New patch attached.

Review for '20110607_serial2_v2.diff':

Submission review
- Is the patch in context diff format?

- Does it apply cleanly to the current git master?

- Does it include reasonable tests, necessary doc patches, etc?
It doesn't have any test but as it is really tiny and relies on the same
longstanding principles as serial and bigserial that seems ok.
It has documentation in the places where I'd expect it.

Usability review
- Does the patch actually implement that?

- Do we want that?
Well, it depends whom you ask ;-)

Tom Lane: "A sequence that can only go to 32K doesn't seem all that
generally useful"

Mike Pultz (patch author): "since serial4 and serial8 are simply
pseudo-types- effectively there for convenience, I’d argue that it
should simply be there for completeness"
Robert Haas: "We should be trying to put all types on equal footing,
rather than artificially privilege some over others."
Brar Piening (me): "I'm with the above arguments. In addition I'd like
to mention that other databases have it too so having it improves
portability. Especially when using ORM."
Perhaps we can get some more opinions...

- Do we already have it?

- Does it follow SQL spec, or the community-agreed behavior?
It has consistent behavior with the existing pseudo-types serial and

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

- Are there dangers?
Not for the code base. One could consider it as a foot gun to allow
autoincs that must not exceed 32K but other databases offer even smaller
autoinc types (tinyint identity in MSSQL is a byte).

- Have all the bases been covered?
I guess so. A quick grep for bigint shows that it covers the same areas.

Feature test
- Does the feature work as advertised?

- Are there corner cases the author has failed to consider?
Probably not.

- Are there any assertion failures or crashes?

Performance review
- Does the patch slow down simple tests?

- If it claims to improve performance, does it?
It doesn't claim anything about performance.

- Does it slow down other things?

Coding review
- Does it follow the project coding guidelines?
Im not an expert in the project coding guidelines but it maches the
style of the surrounding code.

- Are there portability issues?
Probably not. At least not more than for smallint or serial.

- Will it work on Windows/BSD etc?

- Are the comments sufficient and accurate?
Self explanatory - no comments needed.

- Does it do what it says, correctly?

- Does it produce compiler warnings?

- Can you make it crash?

Architecture review
- Is everything done in a way that fits together coherently with other

- Are there interdependencies that can cause problems?
Interdependencies exist with sequences and the smallint type. No
problems probable.

Review review
- Did the reviewer cover all the things that kind of reviewer is
supposed to do?
I tried to look at everything and cover everthing but please consider
that this is my first review so please have a second look at it!



Attachment Content-Type Size
20110607_serial2_v2_tests.psql text/plain 847 bytes

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Jim Nasby 2011-06-08 22:39:08 Re: Autoanalyze and OldestXmin
Previous Message Kevin Grittner 2011-06-08 22:29:13 Re: SSI heap_insert and page-level predicate locks