Re: smallserial / serial2 - Mailing list pgsql-hackers

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

Review for '20110607_serial2_v2.diff':

Submission review
- Is the patch in context diff format?
Yes.

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

- 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?
Yes.

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

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

Pros
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?
No.

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

- 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?
Yes.

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

- Are there any assertion failures or crashes?
No.


Performance review
- Does the patch slow down simple tests?
No.

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

- Does it slow down other things?
No.

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?
Yes.

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

- Does it do what it says, correctly?
Yes.

- Does it produce compiler warnings?
No.

- Can you make it crash?
No

Architecture review
- Is everything done in a way that fits together coherently with other
features/modules?
Yes.

- 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!

Regards,

Brar