Re: [HACKERS] smallserial / serial2
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
Josh Kupershmidt 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
On Wed, Jun 22, 2011 at 9:14 AM, Josh Kupershmidt wrote: > On Tue, Jun 21, 2011 at 10:58 PM, Robert Haas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
On Tue, Jun 21, 2011 at 10:58 PM, Robert Haas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
On Thu, Jun 9, 2011 at 10:27 PM, Josh Kupershmidt wrote: > On Wed, Jun 8, 2011 at 6:36 PM, Brar Piening 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
On Wed, Jun 8, 2011 at 6:36 PM, Brar Piening 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
diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index 13e1565..968fcce 100644
*** a/src/test/regress/expected/sequence.out
--- b/src/test/regress/expected/sequence.out
*** SELECT * FROM serialTest;
*** 16,21
--- 16,99
force | 100
(3 rows)
+ -- test smallserial / bigserial
+ CREATE TABLE serialTest2 (f1 text, f2 serial, f3 smallserial, f4 serial2,
+ f5 bigserial, f6 serial8);
+ NOTICE: CREATE TABLE will create implicit sequence "serialtest2_f2_seq" for serial column "serialtest2.f2"
+ NOTICE: CREATE TABLE will create implicit sequence "serialtest2_f3_seq" for serial column "serialtest2.f3"
+ NOTICE: CREATE TABLE will create implicit sequence "serialtest2_f4_seq" for serial column "serialtest2.f4"
+ NOTICE: CREATE TABLE will create implicit sequence "serialtest2_f5_seq" for serial column "serialtest2.f5"
+ NOTICE: CREATE TABLE will create implicit sequence "serialtest2_f6_seq" for serial column "serialtest2.f6"
+ INSERT INTO serialTest2 (f1)
+ VALUES ('test_defaults');
+ INSERT INTO serialTest2 (f1, f2, f3, f4, f5, f6)
+ VALUES ('test_max_vals', 2147483647, 32767, 32767, 9223372036854775807,
+ 9223372036854775807),
+ ('test_min_vals', -2147483648, -32768, -32768, -9223372036854775808,
+ -9223372036854775808);
+ -- All these INSERTs should fail:
+ INSERT INTO serialTest2 (f1, f3)
+ VALUES ('bogus', -32769);
+ ERROR: smallint out of range
+ INSERT INTO serialTest2 (f1, f4)
+ VALUES ('bogus', -32769);
+ ERROR: smallint out of range
+ INSERT INTO serialTest2 (f1, f3)
+ VALUES ('bogus', 32768);
+ ERROR: smallint out of range
+ INSERT INTO serialTest2 (f1, f4)
+ VALUES ('bogus', 32768);
+ ERROR: smallint out of range
+ INSERT INTO serialTest2 (f1, f5)
+ VALUES ('bogus', -9223372036854775809);
+ ERROR: bigint out of range
+ INSERT INTO serialTest2 (f1, f6)
+ VALUES ('bogus', -9223372036854775809);
+ ERROR: bigint out of range
+ INSERT INTO serialTest2 (f1, f5)
+ VALUES ('bogus', 9223372036854775808);
+ ERROR: bigint out of range
+ INSERT INTO serialTest2 (f1, f6)
+ VALUES ('bogus', 9223372036854775808);
+ ERROR: bigint out of range
+ SELECT * FROM serialTest2 ORDER BY f2 ASC;
+ f1 | f2 | f3 | f4 | f5 | f6
+ ---+-+++--+--
+ test_min_vals | -2147483648 | -32768 | -32768 | -9223372036854775808 | -9223372036854775808
+ test_defaults | 1 | 1 | 1 |1 |1
+ test_max_vals | 2147483647 | 32767 | 32767 | 9223372036854775807 | 9223372036854775807
+ (3 rows)
+
+ SELECT nextval('serialTest2_f2_seq');
+ nextval
+ -
+2
+ (1 row)
+
+ SELECT nextval('serialTest2_f3_seq');
+ nextval
+ -
+2
+ (1 row)
+
+ SELECT nextval('serialTest2_f4_seq');
+ nextval
+ -
+2
+ (1 row)
+
+ SELECT nextval('serialTest2_f5_seq');
+ nextval
+ -
+2
+ (1 row)
+
+ SELECT nextval('serialTest2_f6_seq');
+ nextval
+ -
+2
+ (1 row)
+
-- basic sequence operations using both text and oid references
CREATE SEQUENCE sequence_test;
SELECT nextval('sequence_test'::text);
*** SELECT nextval('sequence_test2');
*** 221,231
(1 row)
-- Information schema
! SELECT * FROM information_schema.sequences WHERE sequence_name IN ('sequence_test2');
! sequence_catalog | sequence_schema | sequence_name | data_type | numeric_precision | numeric_precision_radix | numeric_scale | start_value | minimum_value | maximum_value | increment | cycle_option
! --+-++---+---+-+---+-+---+---+---+--
! regression | public | sequence_test2 | bigint|64 | 2 | 0 | 32 | 5 | 36| 4 | YES
! (1 row)
-- Test comments
COMMENT ON SEQUENCE
Re: [HACKERS] smallserial / serial2
On Wed, 8 Jun 2011 19:29:42 -0400, Robert Haas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
On Wed, Jun 8, 2011 at 6:36 PM, Brar Piening 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
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 j...@nasby.net 512.569.9461 (cell) http://jim.nasby.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
On Tue, 7 Jun 2011 20:35:12 -0400, Mike Pultz 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
CREATE DATABASE smallserial_test_db;
\connect smallserial_test_db
CREATE TABLE test_smallserial
(
id smallserial NOT NULL PRIMARY KEY,
val integer NOT NULL
);
CREATE TABLE test_smallserial2
(
id serial2 NOT NULL PRIMARY KEY,
val integer NOT NULL
);
\d
DROP TABLE test_smallserial2;
\d test_smallserial
INSERT INTO test_smallserial (val)
VALUES(1),(2),(3);
SELECT * FROM test_smallserial;
TRUNCATE TABLE test_smallserial;
SELECT setval('test_smallserial_id_seq', 1, false);
INSERT INTO test_smallserial (val)
SELECT * FROM generate_series(1,32767);
SELECT * FROM test_smallserial LIMIT ALL OFFSET 32764;
TRUNCATE TABLE test_smallserial;
SELECT setval('test_smallserial_id_seq', 1, false);
INSERT INTO test_smallserial (val)
SELECT * FROM generate_series(1,32768);
\connect postgres
DROP database smallserial_test_db;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
Yup- it's attached. Mike From: Brar Piening [mailto:b...@gmx.de] Sent: Tuesday, June 07, 2011 6:58 PM To: Mike Pultz Cc: pgsql-hackers@postgresql.org Subject: Re: [HACKERS] smallserial / serial2 On Wed, 20 Apr 2011 21:27:27 -0400, Mike Pultz <mailto:m...@mikepultz.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 20110607_serial2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
Sorry, forgot the documentation- I guess that stuff doesn't magically happen! New patch attached. Mike From: Brar Piening [mailto:b...@gmx.de] Sent: Tuesday, June 07, 2011 6:58 PM To: Mike Pultz Cc: pgsql-hackers@postgresql.org Subject: Re: [HACKERS] smallserial / serial2 On Wed, 20 Apr 2011 21:27:27 -0400, Mike Pultz <mailto:m...@mikepultz.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 20110607_serial2_v2.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
On Wed, 20 Apr 2011 21:27:27 -0400, Mike Pultz 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
Re: [HACKERS] smallserial / serial2
On Thu, Apr 21, 2011 at 11:06 AM, Mike Pultz 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] smallserial / serial2
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:t...@sss.pgh.pa.us] Sent: Thursday, April 21, 2011 10:26 AM To: Mike Pultz Cc: pgsql-hackers@postgresql.org Subject: Re: [HACKERS] smallserial / serial2 "Mike Pultz" < <mailto:m...@mikepultz.com> m...@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
Re: [HACKERS] smallserial / serial2
"Mike Pultz" 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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.0 -0400
+++ postgresql-9.0.4.new/src/backend/parser/parse_utilcmd.c 2011-04-20
21:10:26.0 -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;
