Обсуждение: smallserial / serial2

<div class="WordSection1"><p class="MsoPlainText">I use tables all the time that have sequences on smallint's; <p
class="MsoPlainText"> <pclass="MsoPlainText">I’d like to simplify my create files by not having to create the sequence
first,but I also don’t want to give up those 2 bytes per column!<p class="MsoPlainText"> <p class="MsoPlainText">Can
thisbe added?<p class="MsoPlainText"> <p class="MsoPlainText">Mike<p class="MsoPlainText"> <p class="MsoPlainText"> <p
class="MsoPlainText"><spanstyle="font-size:10.0pt;font-family:"Lucida Console"">---
postgresql-9.0.4/src/backend/parser/parse_utilcmd.c2011-04-14 23:15:53.000000000 -0400</span><p
class="MsoPlainText"><spanstyle="font-size:10.0pt;font-family:"Lucida Console"">+++
postgresql-9.0.4.new/src/backend/parser/parse_utilcmd.c    2011-04-20 21:10:26.000000000 -0400</span><p
class="MsoPlainText"><spanstyle="font-size:10.0pt;font-family:"Lucida Console"">@@ -280,8 +280,15 @@</span><p
class="MsoPlainText"><spanstyle="font-size:10.0pt;font-family:"Lucida Console"">        {</span><p
class="MsoPlainText"><spanstyle="font-size:10.0pt;font-family:"Lucida Console"">                char       *typname =
strVal(linitial(column->typeName->names));</span><pclass="MsoPlainText"><span
style="font-size:10.0pt;font-family:"LucidaConsole""> </span><p class="MsoPlainText"><span
style="font-size:10.0pt;font-family:"LucidaConsole"">-               if (strcmp(typname, "serial") == 0 ||</span><p
class="MsoPlainText"><spanstyle="font-size:10.0pt;font-family:"Lucida Console"">-                       strcmp(typname,
"serial4")== 0)</span><p class="MsoPlainText"><span style="font-size:10.0pt;font-family:"Lucida
Console"">+              if (strcmp(typname, "smallserial") == 0 ||</span><p class="MsoPlainText"><span
style="font-size:10.0pt;font-family:"LucidaConsole"">+                       strcmp(typname, "serial2") == 0)</span><p
class="MsoPlainText"><spanstyle="font-size:10.0pt;font-family:"Lucida Console"">+               {</span><p
class="MsoPlainText"><spanstyle="font-size:10.0pt;font-family:"Lucida Console"">+                       is_serial =
true;</span><pclass="MsoPlainText"><span style="font-size:10.0pt;font-family:"Lucida Console"">+                      
column->typeName->names= NIL;</span><p class="MsoPlainText"><span style="font-size:10.0pt;font-family:"Lucida
Console"">+                      column->typeName->typeOid = INT2OID;</span><p class="MsoPlainText"><span
style="font-size:10.0pt;font-family:"LucidaConsole"">+               }</span><p class="MsoPlainText"><span
style="font-size:10.0pt;font-family:"LucidaConsole"">+               else if (strcmp(typname, "serial") == 0
||</span><pclass="MsoPlainText"><span style="font-size:10.0pt;font-family:"Lucida
Console"">+                               strcmp(typname, "serial4") == 0)</span><p class="MsoPlainText"><span
style="font-size:10.0pt;font-family:"LucidaConsole"">                {</span><p class="MsoPlainText"><span
style="font-size:10.0pt;font-family:"LucidaConsole"">                        is_serial = true;</span><p
class="MsoPlainText"><spanstyle="font-size:10.0pt;font-family:"Lucida Console"">                       
column->typeName->names= NIL;</span></div> 

"Mike Pultz" <mike@mikepultz.com> writes:
> I use tables all the time that have sequences on smallint's; 
> I'd like to simplify my create files by not having to create the sequence
> first, but I also don't want to give up those 2 bytes per column!

A sequence that can only go to 32K doesn't seem all that generally
useful ...

Are you certain that you're really saving anything?  More likely than
not, the "saved" 2 bytes are going to disappear into alignment padding
of a later column or of the whole tuple.  Even if it really does help
for your case, that's another reason to doubt that it's generally
useful.
        regards, tom lane

<div class="WordSection1"><p class="MsoPlainText">Hey Tom,<p class="MsoPlainText"> <p class="MsoPlainText">I'm sure
thereare plenty of useful tables with <= 32k rows in them? I have a table for customers that uses a smallint (since
thecustomer id is referenced all over the place)- due to the nature of our product, we’re never going to have more than
32kcustomers, but I still want the benefit of the sequence.<p class="MsoPlainText"> <p class="MsoPlainText">And since
serial4and serial8 are simply pseudo-types- effectively there for convenience, I’d argue that it should simply be there
forcompleteness- just because it may be less used, doesn’t mean it shouldn’t be convenient?<p class="MsoPlainText"> <p
class="MsoPlainText">Also,in another case, I’m using it in a small table used to constrain a bigger table- eg:<p
class="MsoPlainText"> <pclass="MsoPlainText"><span style="font-size:9.0pt;font-family:"Lucida Console"">create table
stuff(</span><p class="MsoPlainText"><span style="font-size:9.0pt;font-family:"Lucida Console"">       id serial2
unique</span><pclass="MsoPlainText"><span style="font-size:9.0pt;font-family:"Lucida Console"">);</span><p
class="MsoPlainText"><spanstyle="font-size:9.0pt;font-family:"Lucida Console""> </span><p class="MsoPlainText"><span
style="font-size:9.0pt;font-family:"LucidaConsole"">create table data (</span><p class="MsoPlainText"><span
style="font-size:9.0pt;font-family:"LucidaConsole"">       id serial8 unique,</span><p class="MsoPlainText"><span
style="font-size:9.0pt;font-family:"LucidaConsole"">       stuff smallint not null,</span><p class="MsoPlainText"><span
style="font-size:9.0pt;font-family:"LucidaConsole"">       foreign key(stuff) references stuff(id) on update cascade on
deleterestrict</span><p class="MsoPlainText"><span style="font-size:9.0pt;font-family:"Lucida Console"">);</span><p
class="MsoPlainText"> <pclass="MsoPlainText">Where our “data” table has ~700 million rows right now.<p
class="MsoPlainText"> <pclass="MsoPlainText">And yes- I guess there's nothing to stop me from using a smallint in the
datatable (thus getting the size savings), and reference a int in the stuff table, but it seems like bad form to me to
havea foreign key constraint between two different types.<p class="MsoPlainText"> <p class="MsoPlainText">Mike<p
class="MsoPlainText"> <pclass="MsoPlainText">-----Original Message-----<br />From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
<br/>Sent: Thursday, April 21, 2011 10:26 AM<br />To: Mike Pultz<br />Cc: pgsql-hackers@postgresql.org<br />Subject:
Re:[HACKERS] smallserial / serial2 <p class="MsoPlainText"> <p class="MsoPlainText">"Mike Pultz" <<a
href="mailto:mike@mikepultz.com"><spanstyle="color:windowtext;text-decoration:none">mike@mikepultz.com</span></a>>
writes:<pclass="MsoPlainText">> I use tables all the time that have sequences on smallint's; I'd like <p
class="MsoPlainText">>to simplify my create files by not having to create the sequence <p class="MsoPlainText">>
first,but I also don't want to give up those 2 bytes per column!<p class="MsoPlainText"> <p class="MsoPlainText">A
sequencethat can only go to 32K doesn't seem all that generally useful ...<p class="MsoPlainText"> <p
class="MsoPlainText">Areyou certain that you're really saving anything?  More likely than not, the "saved" 2 bytes are
goingto disappear into alignment padding of a later column or of the whole tuple.  Even if it really does help for your
case,that's another reason to doubt that it's generally useful.<p class="MsoPlainText"> <p
class="MsoPlainText">                                               regards, tom lane</div> 

On Thu, Apr 21, 2011 at 11:06 AM, Mike Pultz <mike@mikepultz.com> wrote:
> And since serial4 and serial8 are simply pseudo-types- effectively there for
> convenience, I’d argue that it should simply be there for completeness- just
> because it may be less used, doesn’t mean it shouldn’t be convenient?

Right now, smallint is a bit like an unwanted stepchild in the
PostgreSQL type system.  In addition to the problem you hit here,
there are various situations where using smallint requires casts in
cases where int4 or int8 would not.  Ken Rosensteel even talked about
this being an obstacle to Oracle to PostgreSQL migrations, in his talk
at PG East (see his slides for details).

Generally, I think this is a bad thing.  We should be trying to put
all types on equal footing, rather than artificially privilege some
over others.  Unfortunately, this is easier said than done, but I
don't think that's a reason to give up trying.

So a tentative +1 from me on supporting this.

You might want to review:

http://wiki.postgresql.org/wiki/Submitting_a_Patch

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

<span id="IDstID">On Wed, 20 Apr 2011 21:27:27 -0400, Mike Pultz <a class="moz-txt-link-rfc2396E"
href="mailto:mike@mikepultz.com"><mike@mikepultz.com></a> wrote:</span><blockquote
cite="mid:023001cbffc3$46f77840$d4e668c0$@mikepultz.com"type="cite"><style><!--
 
/* Font Definitions */
@font-face{font-family:Calibri;panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face{font-family:"Lucida Console";panose-1:2 11 6 9 4 5 4 2 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal,
div.MsoNormal{margin:0in;margin-bottom:.0001pt;font-size:11.0pt;font-family:"Calibri","sans-serif";}
a:link, span.MsoHyperlink{mso-style-priority:99;color:blue;text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed{mso-style-priority:99;color:purple;text-decoration:underline;}
p.MsoPlainText, li.MsoPlainText, div.MsoPlainText{mso-style-priority:99;mso-style-link:"Plain Text
Char";margin:0in;margin-bottom:.0001pt;font-size:11.0pt;font-family:"Calibri","sans-serif";}
span.EmailStyle17{mso-style-type:personal;font-family:"Calibri","sans-serif";color:windowtext;}
span.PlainTextChar{mso-style-name:"Plain Text Char";mso-style-priority:99;mso-style-link:"Plain
Text";font-family:"Calibri","sans-serif";}
.MsoChpDefault{mso-style-type:export-only;font-family:"Calibri","sans-serif";}
@page WordSection1{size:8.5in 11.0in;margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1{page:WordSection1;}
--></style><div class="WordSection1"><br /><p class="MsoPlainText">Can this be added?<br /></div></blockquote> Probably
not- since it's not a complete patch ;-)<br /><br /> I tried to test this one but was unable to find a complete version
ofthe patch in my local mail archives and in the official archives (<a class="moz-txt-link-freetext"
href="http://archives.postgresql.org/message-id/023001cbffc3$46f77840$d4e668c0$@mikepultz.com">http://archives.postgresql.org/message-id/023001cbffc3$46f77840$d4e668c0$@mikepultz.com</a>)<br
/><br/> Could you please repost it for testing?<br /><br /> Regards,<br /><br /> Brar<br /> 

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

On Jun 8, 2011, at 5:36 PM, Brar Piening wrote:
> Pros
> Mike Pultz (patch author): "since serial4 and serial8 are simply pseudo-types- effectively there for convenience, I’d
arguethat 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
havingit improves portability. Especially when using ORM." 
> Perhaps we can get some more opinions...

We have some "dynamic lookup table" metacode that sets up all the infrastructure for a table that normalizes text
valuesto a serial/int. But in many cases, it's a safe bet that we would never need more than 32k (or at worst, 64k)
values.Right now it would be difficult to benefit from the 2 byte savings, but if Postgres was ever able to order
fieldson disk in the most efficient possible format (something we're willing to sponsor, hint hint ;) then this would
bebeneficial for us. 
--
Jim C. Nasby, Database Architect                   jim@nasby.net
512.569.9461 (cell)                         http://jim.nasby.net

On Wed, Jun 8, 2011 at 6:36 PM, Brar Piening <brar@gmx.de> wrote:
>>> New patch attached.
>
> Review for '20110607_serial2_v2.diff':

I see you added this review to the CommitFest application - excellent.

You should also change the status to either "Waiting on Author" or
"Ready for Committer" based on the content of the review.  I think the
latter would be appropriate since your review seems to have been
favorable.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, 8 Jun 2011 19:29:42 -0400, Robert Haas 
<robertmhaas@gmail.com> wrote:
> You should also change the status to either "Waiting on Author" or
> "Ready for Committer" based on the content of the review.  I think the
> latter would be appropriate since your review seems to have been
> favorable.
Well - not being a review profi I was thinking that the appropriate 
status would be "waiting for some more on list discussion about whether 
to include this" or "waiting for a more experienced reviewer  to see if 
my review is ok" (which could admittedly be the commiter).

I've changed it.

Regards,

Brar

On Wed, Jun 8, 2011 at 6:36 PM, Brar Piening <brar@gmx.de> wrote:
> 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!

I took a look at this as well, and I didn't encounter any problems
either. The patch is surprisingly small, but it looks like all the
documentation spots got covered, and I didn't see any missing pieces;
pg_dump copes fine, I didn't try pg_upgrade but I wouldn't expect
problems there either.

Actually, the one piece I think could be added is a regression test. I
didn't see any existing tests that covered bigserial/serial8, either,
so I went ahead and added a few tests for smallerial and bigserial. I
didn't see the testcases Brar posted at first, or I might have adopted
a few from there, but this should cover basic use of smallserial.

Josh

On Thu, Jun 9, 2011 at 10:27 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
> On Wed, Jun 8, 2011 at 6:36 PM, Brar Piening <brar@gmx.de> wrote:
>> 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!
>
> I took a look at this as well, and I didn't encounter any problems
> either. The patch is surprisingly small, but it looks like all the
> documentation spots got covered, and I didn't see any missing pieces;
> pg_dump copes fine, I didn't try pg_upgrade but I wouldn't expect
> problems there either.
>
> Actually, the one piece I think could be added is a regression test. I
> didn't see any existing tests that covered bigserial/serial8, either,
> so I went ahead and added a few tests for smallerial and bigserial. I
> didn't see the testcases Brar posted at first, or I might have adopted
> a few from there, but this should cover basic use of smallserial.

Committed the main patch, and your regression tests.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jun 21, 2011 at 10:58 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> Committed the main patch, and your regression tests.

Hmph, looks like buildfarm members koi and jaguar are failing make check now:
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=koi&dt=2011-06-22%2008%3A06%3A00

due to a difference in sequence.out. I didn't muck with the part about SELECT * FROM foo_seq_new;
which is causing the diff, but it looks like the only difference is in
the log_cnt column, which seems pretty fragile to rely on in a
regression test. Maybe those SELECTS just shouldn't include log_cnt.

Josh

On Wed, Jun 22, 2011 at 9:14 AM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
> On Tue, Jun 21, 2011 at 10:58 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> Committed the main patch, and your regression tests.
>
> Hmph, looks like buildfarm members koi and jaguar are failing make check now:
>  http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=koi&dt=2011-06-22%2008%3A06%3A00
>
> due to a difference in sequence.out. I didn't muck with the part about
>  SELECT * FROM foo_seq_new;
> which is causing the diff, but it looks like the only difference is in
> the log_cnt column, which seems pretty fragile to rely on in a
> regression test. Maybe those SELECTS just shouldn't include log_cnt.

Seems like a reasonable thing to try.  Done.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Josh Kupershmidt <schmiddy@gmail.com> writes:
> Hmph, looks like buildfarm members koi and jaguar are failing make check now:
>   http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=koi&dt=2011-06-22%2008%3A06%3A00

> due to a difference in sequence.out. I didn't muck with the part about
>   SELECT * FROM foo_seq_new;
> which is causing the diff, but it looks like the only difference is in
> the log_cnt column, which seems pretty fragile to rely on in a
> regression test. Maybe those SELECTS just shouldn't include log_cnt.

We've been around on this before:

http://archives.postgresql.org/pgsql-hackers/2008-08/msg01359.php

The eventual solution was bb3f839bfcb396c3066a31559d3a72ef0b4b5fae,
which is not consistent with what Robert just did, in fact he forgot
about that variant output file altogether (which probably means we'll
still get occasional failure reports).

That previous approach of adding extra expected files isn't going to
scale nicely if there are multiple places at risk ... but do we need
multiple places selecting the sequence contents?  I remain of the
opinion that just omitting the value isn't good testing policy.
        regards, tom lane

I wrote:
> That previous approach of adding extra expected files isn't going to
> scale nicely if there are multiple places at risk ... but do we need
> multiple places selecting the sequence contents?  I remain of the
> opinion that just omitting the value isn't good testing policy.

Actually, on looking closer, you didn't add additional selections from
sequences.  The real problem here is simply that you forgot to update
expected/sequence_1.out altogether.  So Robert's "fix" should be
reverted in favor of doing that.
        regards, tom lane