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':
- 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.
- 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
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)?
- 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.
- Does the feature work as advertised?
- Are there corner cases the author has failed to consider?
- Are there any assertion failures or crashes?
- 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?
- 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?
- 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
- 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!
In response to
pgsql-hackers by date
|Next:||From: Jim Nasby||Date: 2011-06-08 22:39:08|
|Subject: Re: Autoanalyze and OldestXmin|
|Previous:||From: Kevin Grittner||Date: 2011-06-08 22:29:13|
|Subject: Re: SSI heap_insert and page-level predicate locks|