Re: smallserial / serial2

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!

Can this be added?

Mike

--- postgresql-9.0.4/src/backend/parser/parse_utilcmd.c 2011-04-14
23:15:53.000000000 -0400

+++ postgresql-9.0.4.new/src/backend/parser/parse_utilcmd.c 2011-04-20
21:10:26.000000000 -0400

@@ -280,8 +280,15 @@

{

char *typname =
strVal(linitial(column->typeName->names));

- if (strcmp(typname, "serial") == 0 ||

- strcmp(typname, "serial4") == 0)

+ if (strcmp(typname, "smallserial") == 0 ||

+ strcmp(typname, "serial2") == 0)

+ {

+ is_serial = true;

+ column->typeName->names = NIL;

+ column->typeName->typeOid = INT2OID;

+ }

+ else if (strcmp(typname, "serial") == 0 ||

+ strcmp(typname, "serial4") == 0)

{

is_serial = true;

column->typeName->names = NIL;

"Mike Pultz" <mike(at)mikepultz(dot)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

Hey Tom,

I'm sure there are plenty of useful tables with <= 32k rows in them? I have
a table for customers that uses a smallint (since the customer id is
referenced all over the place)- due to the nature of our product, we're
never going to have more than 32k customers, but I still want the benefit of
the sequence.

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?

Also, in another case, I'm using it in a small table used to constrain a
bigger table- eg:

create table stuff (

id serial2 unique

);

create table data (

id serial8 unique,

stuff smallint not null,

foreign key(stuff) references stuff(id) on update cascade on delete
restrict

);

Where our "data" table has ~700 million rows right now.

And yes- I guess there's nothing to stop me from using a smallint in the
data table (thus getting the size savings), and reference a int in the stuff
table, but it seems like bad form to me to have a foreign key constraint
between two different types.

Mike

-----Original Message-----
From: Tom Lane [mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us]
Sent: Thursday, April 21, 2011 10:26 AM
To: Mike Pultz
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [HACKERS] smallserial / serial2

"Mike Pultz" < <mailto:mike(at)mikepultz(dot)com> mike(at)mikepultz(dot)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

On Thu, Apr 21, 2011 at 11:06 AM, Mike Pultz <mike(at)mikepultz(dot)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

On Wed, 20 Apr 2011 21:27:27 -0400, Mike Pultz <mike(at)mikepultz(dot)com> wrote:
>
> Can this be added?
>
>
Probably not - since it's not a complete patch ;-)

I tried to test this one but was unable to find a complete version of
the patch in my local mail archives and in the official archives
(http://archives.postgresql.org/message-id/023001cbffc3$46f77840$d4e668c0$@mikepultz.com)

Could you please repost it for testing?

Regards,

Brar

Yup- it's attached.

Mike

From: Brar Piening [mailto:brar(at)gmx(dot)de]
Sent: Tuesday, June 07, 2011 6:58 PM
To: Mike Pultz
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [HACKERS] smallserial / serial2

On Wed, 20 Apr 2011 21:27:27 -0400, Mike Pultz <mailto:mike(at)mikepultz(dot)com>
<mike(at)mikepultz(dot)com> wrote:

Can this be added?

Probably not - since it's not a complete patch ;-)

I tried to test this one but was unable to find a complete version of the
patch in my local mail archives and in the official archives
(http://archives.postgresql.org/message-id/023001cbffc3$46f77840$d4e668c0$@m
ikepultz.com)

Could you please repost it for testing?

Regards,

Brar

Sorry, forgot the documentation- I guess that stuff doesn't magically
happen!

New patch attached.

Mike

From: Brar Piening [mailto:brar(at)gmx(dot)de]
Sent: Tuesday, June 07, 2011 6:58 PM
To: Mike Pultz
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [HACKERS] smallserial / serial2

On Wed, 20 Apr 2011 21:27:27 -0400, Mike Pultz <mailto:mike(at)mikepultz(dot)com>
<mike(at)mikepultz(dot)com> wrote:

Can this be added?

Probably not - since it's not a complete patch ;-)

I tried to test this one but was unable to find a complete version of the
patch in my local mail archives and in the official archives
(http://archives.postgresql.org/message-id/023001cbffc3$46f77840$d4e668c0$@m
ikepultz.com)

Could you please repost it for testing?

Regards,

Brar

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

We have some "dynamic lookup table" metacode that sets up all the infrastructure for a table that normalizes text values to 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 fields on disk in the most efficient possible format (something we're willing to sponsor, hint hint ;) then this would be beneficial for us.
--
Jim C. Nasby, Database Architect jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net

On Wed, Jun 8, 2011 at 6:36 PM, Brar Piening <brar(at)gmx(dot)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(at)gmail(dot)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(at)gmx(dot)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(at)gmail(dot)com> wrote:
> On Wed, Jun 8, 2011 at 6:36 PM, Brar Piening <brar(at)gmx(dot)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(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Tue, Jun 21, 2011 at 10:58 PM, Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)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