Thread: Basic DOMAIN Support
I intend to add other parts of domain support later on (no reason to
hold up committing this though) but would appreciate some feedback
about what I've done.
What's there works, however I intend to finish it off with CHECK
and -- if I can figure out a good way -- REFERENCES.
Implements:
CREATE DOMAIN domain type [NULL | NOT NULL] [DEFAULT expression];
COMMENT ON DOMAIN domain IS '';
DROP DOMAIN domain [RESTRICT | CASCADE]; -- Doesn't actually restrict
due to pg_depends
Affects:
Types can be specified as NOT NULL. No interface is available to set
this for any type other than a domain however. Types may also use a
complex expression (b_expr) for their default.
Various Tasks (output from psql for some simple operations involving
domains):
NOTE: For DEFAULT NULL to have any effect in table creation the
default actually needs to be stored.
Since Type defaults have overridden NULL in the past, I left it so
domains would as well.
Below are some tests I used to check the implementation.
## DOMAIN TEST ##
create domain domainvarchar varchar(15);
create domain domainnumeric numeric(8,2);
create domain domainint4 int4;
create domain domaintext text;
-- Test tables using domains
create table basictest
( testint4 domainint4
, realint4 int4
, testtext domaintext
, realtext text
, testvarchar domainvarchar
, realvarchar varchar(15)
, testnumeric domainnumeric
, realnumeric numeric(8,2)
);
INSERT INTO basictest values ('88', '88', 'haha', 'haha', 'short
text', 'short text', '123.12', '123.12');
select * from basictest;
create domain dnotnull varchar(15) NOT NULL;
create domain dnull varchar(15) NULL;
-- NOT NULL in the domain cannot be overridden
create table nulltest
( col1 dnotnull
, col2 dnotnull NULL
, col3 dnull NOT NULL
, col4 dnull
);
INSERT INTO nulltest DEFAULT VALUES;
INSERT INTO nulltest values ('a', 'b', 'c', 'd'); -- Good
INSERT INTO nulltest values (NULL, 'b', 'c', 'd');
INSERT INTO nulltest values ('a', NULL, 'c', 'd');
INSERT INTO nulltest values ('a', 'b', NULL, 'd');
INSERT INTO nulltest values ('a', 'b', 'c', NULL); -- Good
select * from nulltest;
create domain ddef1 int4 DEFAULT 3;
create domain ddef2 numeric(8,6) DEFAULT random();
-- Type mixing, function returns int8
create domain ddef3 text DEFAULT random();
create sequence ddef4_seq;
create domain ddef4 int4 DEFAULT nextval(cast('ddef4_seq' as text));
create table defaulttest
( col1 ddef1
, col2 ddef2
, col3 ddef3
, col4 ddef4
, col5 ddef1 DEFAULT NULL
, col6 ddef2 DEFAULT '88.1'
, col7 ddef4 DEFAULT random() * 8000
);
insert into defaulttest default values;
insert into defaulttest default values;
insert into defaulttest default values;
select * from defaulttest;
## PSQL OUTPUT ##
newdb=# -- Test Comment / Drop
newdb=# create domain domaindroptest int4;
CREATE DOMAIN
newdb=# comment on domain domaindroptest is 'About to drop this..';
COMMENT
newdb=#
newdb=# select * from pg_type where typname = 'domaindroptest';
typname | typowner | typlen | typprtlen | typbyval | typtype |
typisdefined | typdelim | typrelid | typelem | typinput | typoutput |
typrecei
e | typsend | typalign | typstorage | typnotnull | typmod |
typdefaultbin | typdefault
----------------+----------+--------+-----------+----------+---------+
--------------+----------+----------+---------+----------+-----------+
---------
--+---------+----------+------------+------------+--------+-----------
----+------------
domaindroptest | 1 | 4 | 10 | t | d |
t | , | 0 | 23 | int4in | int4out |
int4in
| int4out | i | p | f | -1 |
|
(1 row)
newdb=#
newdb=# drop domain domaindroptest restrict;
DROP
newdb=#
newdb=# select * from pg_type where typname = 'domaindroptest';
typname | typowner | typlen | typprtlen | typbyval | typtype |
typisdefined | typdelim | typrelid | typelem | typinput | typoutput |
typreceive | ty
send | typalign | typstorage | typnotnull | typmod | typdefaultbin |
typdefault
---------+----------+--------+-----------+----------+---------+-------
-------+----------+----------+---------+----------+-----------+-------
-----+---
-----+----------+------------+------------+--------+---------------+--
----------
(0 rows)
newdb=# -- TEST Domains.
newdb=#
newdb=# create domain domainvarchar varchar(15);
CREATE DOMAIN
newdb=# create domain domainnumeric numeric(8,2);
CREATE DOMAIN
newdb=# create domain domainint4 int4;
CREATE DOMAIN
newdb=# create domain domaintext text;
CREATE DOMAIN
newdb=#
newdb=# -- Test tables using domains
newdb=# create table basictest
newdb-# ( testint4 domainint4
newdb(# , realint4 int4
newdb(# , testtext domaintext
newdb(# , realtext text
newdb(# , testvarchar domainvarchar
newdb(# , realvarchar varchar(15)
newdb(# , testnumeric domainnumeric
newdb(# , realnumeric numeric(8,2)
newdb(# );
CREATE
newdb=#
newdb=# INSERT INTO basictest values ('88', '88', 'haha', 'haha',
'short text', 'short text', '123.12', '123.12');
INSERT 90400 1
newdb=# select * from basictest;
testint4 | realint4 | testtext | realtext | testvarchar | realvarchar
| testnumeric | realnumeric
----------+----------+----------+----------+-------------+------------
-+-------------+-------------
88 | 88 | haha | haha | short text | short text
| 123.12 | 123.12
(1 row)
newdb=#
newdb=# create domain dnotnull varchar(15) NOT NULL;
CREATE DOMAIN
newdb=# create domain dnull varchar(15) NULL;
CREATE DOMAIN
newdb=# -- NOT NULL in the domain cannot be overridden
newdb=# create table nulltest
newdb-# ( col1 dnotnull
newdb(# , col2 dnotnull NULL
newdb(# , col3 dnull NOT NULL
newdb(# , col4 dnull
newdb(# );
CREATE
newdb=# INSERT INTO nulltest DEFAULT VALUES;
ERROR: ExecAppend: Fail to add null value in not null attribute col1
newdb=# INSERT INTO nulltest values ('a', 'b', 'c', 'd'); -- Good
INSERT 90408 1
newdb-# INSERT INTO nulltest values (NULL, 'b', 'c', 'd');
ERROR: ExecAppend: Fail to add null value in not null attribute col1
newdb=# INSERT INTO nulltest values ('a', NULL, 'c', 'd');
ERROR: ExecAppend: Fail to add null value in not null attribute col2
newdb=# INSERT INTO nulltest values ('a', 'b', NULL, 'd');
ERROR: ExecAppend: Fail to add null value in not null attribute col3
newdb=# INSERT INTO nulltest values ('a', 'b', 'c', NULL); -- Good
INSERT 90409 1
newdb-# select * from nulltest;
col1 | col2 | col3 | col4
------+------+------+------
a | b | c | d
a | b | c |
(2 rows)
newdb=# create domain ddef1 int4 DEFAULT 3;
CREATE DOMAIN
newdb=# create domain ddef2 numeric(8,6) DEFAULT random();
CREATE DOMAIN
newdb=# -- Type mixing, function returns int8
newdb=# create domain ddef3 text DEFAULT random();
CREATE DOMAIN
newdb=# create sequence ddef4_seq;
CREATE
newdb=# create domain ddef4 int4 DEFAULT nextval(cast('ddef4_seq' as
text));
CREATE DOMAIN
newdb=#
newdb=# create table defaulttest
newdb-# ( col1 ddef1
newdb(# , col2 ddef2
newdb(# , col3 ddef3
newdb(# , col4 ddef4
newdb(# , col5 ddef1 DEFAULT NULL
newdb(# , col6 ddef2 DEFAULT '88.1'
newdb(# , col7 ddef4 DEFAULT random() * 8000
newdb(# );
CREATE
newdb=# insert into defaulttest default values;
INSERT 90421 1
newdb=# insert into defaulttest default values;
INSERT 90422 1
newdb=# insert into defaulttest default values;
INSERT 90423 1
newdb=# select * from defaulttest;
col1 | col2 | col3 | col4 | col5 | col6 |
col7
------+-------------------+-------------------+------+------+------+--
----
3 | 0.186453586065422 | 0.391880722433273 | 1 | 3 | 88.1 |
1930
3 | 0.999444424174467 | 0.461114872461704 | 2 | 3 | 88.1 |
6024
3 | 0.837450824602251 | 0.632604472633733 | 3 | 3 | 88.1 |
7441
(3 rows)
--
Rod Taylor
Your eyes are weary from staring at the CRT. You feel sleepy. Notice
how restful it is to watch the cursor blink. Close your eyes. The
opinions stated above are yours. You cannot imagine why you ever felt
otherwise.
Attachment
"Rod Taylor" <rbt@zort.ca> writes:
> I intend to add other parts of domain support later on (no reason to
> hold up committing this though) but would appreciate some feedback
> about what I've done.
Still needs some work ...
One serious problem is that I do not think you can get away with reusing
typelem to link domains to base types. All the array code is going to
think that a domain is an array, and proceed to do horribly wrong
things. User applications may think the same thing, so even if you
wanted to change every backend routine that looks at typelem, it
wouldn't be enough. I think the only safe way to proceed is to add a
separate column that links a domain to its base type. This'd also save
you from having to add another meaning to typtype (since a domain could
be recognized by nonzero typbasetype). That should reduce the
likelihood of breaking existing code, and perhaps make life simpler when
it comes time to allow freestanding composite types (why shouldn't a
domain have a composite type as base?).
Come to think of it, even without freestanding composite types it'd be
possible to try to define a domain as a subtype of a composite type,
and to use same as (eg) a function argument or result type. I doubt
you are anywhere near making that behave reasonably, though. Might be
best to disallow it for now.
Speaking of arrays --- have you thought about arrays of domain-type
objects? I'm not sure whether any of the supported features matter for
array elements, but if they do it's not clear how to make it happen.
Another objection is the changes you made in execMain.c. That extra
syscache lookup for every field of every tuple is going to be a rather
nasty performance hit, especially seeing that people will pay it whether
they ever heard of domains or not. And it seems quite unnecessary; if
you copy the domain's notnull bit into the pg_attribute row, then the
existing coding will do fine, no?
I think also that you may have created some subtle changes in the
semantics of type default-value specifications; we'll need to think
if any compatibility problems have been introduced. There are doubtless
hardly any people using the feature, so this is not a serious objection,
but if any change has occurred it should be documented.
Overall, an impressive first cut!
regards, tom lane
> "Rod Taylor" <rbt@zort.ca> writes: > > I intend to add other parts of domain support later on (no reason to > > hold up committing this though) but would appreciate some feedback > > about what I've done. > > Still needs some work ... > > One serious problem is that I do not think you can get away with reusing > typelem to link domains to base types. All the array code is going to <-- snip--> > be recognized by nonzero typbasetype). That should reduce the > likelihood of breaking existing code, and perhaps make life simpler when > it comes time to allow freestanding composite types (why shouldn't a > domain have a composite type as base?). Will add pg_type.typbasetype, and can really easily allow domains of composite types if this is wanted. I don't really understand composite types, so I have no idea how this would work. > Speaking of arrays --- have you thought about arrays of domain-type > objects? I'm not sure whether any of the supported features matter for > array elements, but if they do it's not clear how to make it happen. I wondered about this too, but I've been able to regress the system as well as use it for other basic tasks. The main reason that a domain isn't a base type is to prevent a domain of domains. The few books I have that describe domains state that they shouldn't. Marking it with a 'd' makes it quite obvious it's a domain and not intended for grouping into subdomains. One can make a domain of an array easily enough, then apply constraints to each segment of it -- but hadn't considered an array of domains. In the end they can both come out to the same thing. Check constraints aren't currently available of course, but the below will work the same. -- Potentially allowed, but untested (likley broken, but maybe not...). create domain dom1 int2[2] check (VALUE[1] > 5 and VALUE[2] > 5); create table tab1 (arr1 dom1); or -- Not allowed, and not easily arranged. create domain dom2 int2 check (VALUE > 5); create table tab2 (arr2 dom2[2]); > Another objection is the changes you made in execMain.c. That extra > syscache lookup for every field of every tuple is going to be a rather > nasty performance hit, especially seeing that people will pay it whether > they ever heard of domains or not. And it seems quite unnecessary; if > you copy the domain's notnull bit into the pg_attribute row, then the > existing coding will do fine, no? Did that originally with a function called MergeDomainAttributes() (since removed). The goal was to allow the user to change table attributes without overriding the domain. The constraints are 'merged' on execution for every other constraint type (default is wacky though), so don't see why NOT NULL should be special. This could be done with a new column pg_attribute.attypnotnull and have the type null / not null data copied there. Of course, ALTER DOMAIN is going to get rather complex searching for all of those areas. That said, I rarely see a table without atleast one NOT NULL constraint (primary key usually) except for perhaps a log, so is it really a big performance hit? I thought I saved a couple of cycles by dropping the IF :) > I think also that you may have created some subtle changes in the > semantics of type default-value specifications; we'll need to think > if any compatibility problems have been introduced. There are doubtless > hardly any people using the feature, so this is not a serious objection, > but if any change has occurred it should be documented. Where would I put the docs for this? CREATE TYPE ref notes? or is the history released at release adequate for this?
"Rod Taylor" <rbt@zort.ca> writes:
> Did that originally with a function called MergeDomainAttributes()
> (since removed). The goal was to allow the user to change table
> attributes without overriding the domain. The constraints are
> 'merged' on execution for every other constraint type (default is
> wacky though), so don't see why NOT NULL should be special.
It appeared to me that the intention was to merge at table creation
time, not at execution. I would certainly recommend doing it that
way for simplicity and performance reasons. (This does put ALTER DOMAIN
out of reach, but I feel no pain at not supporting that.)
regards, tom lane
Your tests look pretty good - maybe you should make them into a proper
regression test as well? Also, shouldn't there be some modification to
pg_dump to all DOMAINs to be dumped?
Chris
> -----Original Message-----
> From: pgsql-patches-owner@postgresql.org
> [mailto:pgsql-patches-owner@postgresql.org]On Behalf Of Rod Taylor
> Sent: Monday, 25 February 2002 7:35 AM
> To: pgsql-patches@postgresql.org
> Subject: [PATCHES] Basic DOMAIN Support
>
>
> I intend to add other parts of domain support later on (no reason to
> hold up committing this though) but would appreciate some feedback
> about what I've done.
>
> What's there works, however I intend to finish it off with CHECK
> and -- if I can figure out a good way -- REFERENCES.
>
>
> Implements:
> CREATE DOMAIN domain type [NULL | NOT NULL] [DEFAULT expression];
> COMMENT ON DOMAIN domain IS '';
> DROP DOMAIN domain [RESTRICT | CASCADE]; -- Doesn't actually restrict
> due to pg_depends
>
> Affects:
> Types can be specified as NOT NULL. No interface is available to set
> this for any type other than a domain however. Types may also use a
> complex expression (b_expr) for their default.
>
> Various Tasks (output from psql for some simple operations involving
> domains):
>
> NOTE: For DEFAULT NULL to have any effect in table creation the
> default actually needs to be stored.
>
> Since Type defaults have overridden NULL in the past, I left it so
> domains would as well.
>
> Below are some tests I used to check the implementation.
>
> ## DOMAIN TEST ##
> create domain domainvarchar varchar(15);
> create domain domainnumeric numeric(8,2);
> create domain domainint4 int4;
> create domain domaintext text;
>
> -- Test tables using domains
> create table basictest
> ( testint4 domainint4
> , realint4 int4
> , testtext domaintext
> , realtext text
> , testvarchar domainvarchar
> , realvarchar varchar(15)
> , testnumeric domainnumeric
> , realnumeric numeric(8,2)
> );
>
> INSERT INTO basictest values ('88', '88', 'haha', 'haha', 'short
> text', 'short text', '123.12', '123.12');
> select * from basictest;
>
> create domain dnotnull varchar(15) NOT NULL;
> create domain dnull varchar(15) NULL;
>
> -- NOT NULL in the domain cannot be overridden
> create table nulltest
> ( col1 dnotnull
> , col2 dnotnull NULL
> , col3 dnull NOT NULL
> , col4 dnull
> );
> INSERT INTO nulltest DEFAULT VALUES;
> INSERT INTO nulltest values ('a', 'b', 'c', 'd'); -- Good
> INSERT INTO nulltest values (NULL, 'b', 'c', 'd');
> INSERT INTO nulltest values ('a', NULL, 'c', 'd');
> INSERT INTO nulltest values ('a', 'b', NULL, 'd');
> INSERT INTO nulltest values ('a', 'b', 'c', NULL); -- Good
> select * from nulltest;
>
>
> create domain ddef1 int4 DEFAULT 3;
> create domain ddef2 numeric(8,6) DEFAULT random();
> -- Type mixing, function returns int8
> create domain ddef3 text DEFAULT random();
> create sequence ddef4_seq;
> create domain ddef4 int4 DEFAULT nextval(cast('ddef4_seq' as text));
>
> create table defaulttest
> ( col1 ddef1
> , col2 ddef2
> , col3 ddef3
> , col4 ddef4
> , col5 ddef1 DEFAULT NULL
> , col6 ddef2 DEFAULT '88.1'
> , col7 ddef4 DEFAULT random() * 8000
> );
> insert into defaulttest default values;
> insert into defaulttest default values;
> insert into defaulttest default values;
> select * from defaulttest;
>
> ## PSQL OUTPUT ##
>
> newdb=# -- Test Comment / Drop
> newdb=# create domain domaindroptest int4;
> CREATE DOMAIN
> newdb=# comment on domain domaindroptest is 'About to drop this..';
> COMMENT
> newdb=#
> newdb=# select * from pg_type where typname = 'domaindroptest';
> typname | typowner | typlen | typprtlen | typbyval | typtype |
> typisdefined | typdelim | typrelid | typelem | typinput | typoutput |
> typrecei
> e | typsend | typalign | typstorage | typnotnull | typmod |
> typdefaultbin | typdefault
> ----------------+----------+--------+-----------+----------+---------+
> --------------+----------+----------+---------+----------+-----------+
> ---------
> --+---------+----------+------------+------------+--------+-----------
> ----+------------
> domaindroptest | 1 | 4 | 10 | t | d |
> t | , | 0 | 23 | int4in | int4out |
> int4in
> | int4out | i | p | f | -1 |
> |
> (1 row)
>
> newdb=#
> newdb=# drop domain domaindroptest restrict;
> DROP
> newdb=#
> newdb=# select * from pg_type where typname = 'domaindroptest';
> typname | typowner | typlen | typprtlen | typbyval | typtype |
> typisdefined | typdelim | typrelid | typelem | typinput | typoutput |
> typreceive | ty
> send | typalign | typstorage | typnotnull | typmod | typdefaultbin |
> typdefault
> ---------+----------+--------+-----------+----------+---------+-------
> -------+----------+----------+---------+----------+-----------+-------
> -----+---
> -----+----------+------------+------------+--------+---------------+--
> ----------
> (0 rows)
>
> newdb=# -- TEST Domains.
> newdb=#
> newdb=# create domain domainvarchar varchar(15);
> CREATE DOMAIN
> newdb=# create domain domainnumeric numeric(8,2);
> CREATE DOMAIN
> newdb=# create domain domainint4 int4;
> CREATE DOMAIN
> newdb=# create domain domaintext text;
> CREATE DOMAIN
> newdb=#
> newdb=# -- Test tables using domains
> newdb=# create table basictest
> newdb-# ( testint4 domainint4
> newdb(# , realint4 int4
> newdb(# , testtext domaintext
> newdb(# , realtext text
> newdb(# , testvarchar domainvarchar
> newdb(# , realvarchar varchar(15)
> newdb(# , testnumeric domainnumeric
> newdb(# , realnumeric numeric(8,2)
> newdb(# );
> CREATE
> newdb=#
> newdb=# INSERT INTO basictest values ('88', '88', 'haha', 'haha',
> 'short text', 'short text', '123.12', '123.12');
> INSERT 90400 1
> newdb=# select * from basictest;
> testint4 | realint4 | testtext | realtext | testvarchar | realvarchar
> | testnumeric | realnumeric
> ----------+----------+----------+----------+-------------+------------
> -+-------------+-------------
> 88 | 88 | haha | haha | short text | short text
> | 123.12 | 123.12
> (1 row)
>
> newdb=#
> newdb=# create domain dnotnull varchar(15) NOT NULL;
> CREATE DOMAIN
> newdb=# create domain dnull varchar(15) NULL;
> CREATE DOMAIN
> newdb=# -- NOT NULL in the domain cannot be overridden
> newdb=# create table nulltest
> newdb-# ( col1 dnotnull
> newdb(# , col2 dnotnull NULL
> newdb(# , col3 dnull NOT NULL
> newdb(# , col4 dnull
> newdb(# );
> CREATE
> newdb=# INSERT INTO nulltest DEFAULT VALUES;
> ERROR: ExecAppend: Fail to add null value in not null attribute col1
> newdb=# INSERT INTO nulltest values ('a', 'b', 'c', 'd'); -- Good
> INSERT 90408 1
> newdb-# INSERT INTO nulltest values (NULL, 'b', 'c', 'd');
> ERROR: ExecAppend: Fail to add null value in not null attribute col1
> newdb=# INSERT INTO nulltest values ('a', NULL, 'c', 'd');
> ERROR: ExecAppend: Fail to add null value in not null attribute col2
> newdb=# INSERT INTO nulltest values ('a', 'b', NULL, 'd');
> ERROR: ExecAppend: Fail to add null value in not null attribute col3
> newdb=# INSERT INTO nulltest values ('a', 'b', 'c', NULL); -- Good
> INSERT 90409 1
> newdb-# select * from nulltest;
> col1 | col2 | col3 | col4
> ------+------+------+------
> a | b | c | d
> a | b | c |
> (2 rows)
>
> newdb=# create domain ddef1 int4 DEFAULT 3;
> CREATE DOMAIN
> newdb=# create domain ddef2 numeric(8,6) DEFAULT random();
> CREATE DOMAIN
> newdb=# -- Type mixing, function returns int8
> newdb=# create domain ddef3 text DEFAULT random();
> CREATE DOMAIN
> newdb=# create sequence ddef4_seq;
> CREATE
> newdb=# create domain ddef4 int4 DEFAULT nextval(cast('ddef4_seq' as
> text));
> CREATE DOMAIN
> newdb=#
> newdb=# create table defaulttest
> newdb-# ( col1 ddef1
> newdb(# , col2 ddef2
> newdb(# , col3 ddef3
> newdb(# , col4 ddef4
> newdb(# , col5 ddef1 DEFAULT NULL
> newdb(# , col6 ddef2 DEFAULT '88.1'
> newdb(# , col7 ddef4 DEFAULT random() * 8000
> newdb(# );
> CREATE
> newdb=# insert into defaulttest default values;
> INSERT 90421 1
> newdb=# insert into defaulttest default values;
> INSERT 90422 1
> newdb=# insert into defaulttest default values;
> INSERT 90423 1
> newdb=# select * from defaulttest;
> col1 | col2 | col3 | col4 | col5 | col6 |
> col7
> ------+-------------------+-------------------+------+------+------+--
> ----
> 3 | 0.186453586065422 | 0.391880722433273 | 1 | 3 | 88.1 |
> 1930
> 3 | 0.999444424174467 | 0.461114872461704 | 2 | 3 | 88.1 |
> 6024
> 3 | 0.837450824602251 | 0.632604472633733 | 3 | 3 | 88.1 |
> 7441
> (3 rows)
>
>
> --
> Rod Taylor
>
> Your eyes are weary from staring at the CRT. You feel sleepy. Notice
> how restful it is to watch the cursor blink. Close your eyes. The
> opinions stated above are yours. You cannot imagine why you ever felt
> otherwise.
>
>
Yes, and yes -- although I completely forgot about pg_dump which would have been amusing to see the comments of someone moving a db with domains ;) Can I assume it'll be safe for pgdump to output the domains near the top? I'm going to implement Toms requests -- take a long nap -- and see about regressions and pg_dump tomorrow. I love having sequence lookups in a domain used across several tables. That one feature (in my case for a 'global' transaction id) has made the whole thing worth while. 50 some tables, one serial. Typing nextval all the time in CREATE TABLE was getting annoying. BTW, Toronto is full of nuts. The hockey game ended many hours ago and they're still running up and down the streets cheering. It's also getting annoying. -- Rod Taylor This message represents the official view of the voices in my head ----- Original Message ----- From: "Christopher Kings-Lynne" <chriskl@familyhealth.com.au> To: "Rod Taylor" <rbt@zort.ca>; <pgsql-patches@postgresql.org> Sent: Sunday, February 24, 2002 10:32 PM Subject: RE: [PATCHES] Basic DOMAIN Support > Your tests look pretty good - maybe you should make them into a proper > regression test as well? Also, shouldn't there be some modification to > pg_dump to all DOMAINs to be dumped? > > Chris > > > > -----Original Message----- > > From: pgsql-patches-owner@postgresql.org > > [mailto:pgsql-patches-owner@postgresql.org]On Behalf Of Rod Taylor > > Sent: Monday, 25 February 2002 7:35 AM > > To: pgsql-patches@postgresql.org > > Subject: [PATCHES] Basic DOMAIN Support > > > > > > I intend to add other parts of domain support later on (no reason to > > hold up committing this though) but would appreciate some feedback > > about what I've done. > > > > What's there works, however I intend to finish it off with CHECK > > and -- if I can figure out a good way -- REFERENCES.
> Yes, and yes -- although I completely forgot about pg_dump which would > have been amusing to see the comments of someone moving a db with > domains ;) > > Can I assume it'll be safe for pgdump to output the domains near the > top? Does pg_dump dump CREATE TYPE's and CREATE OPERATOR's somewhere? Maybe it should be near that? > I'm going to implement Toms requests -- take a long nap -- and see > about regressions and pg_dump tomorrow. Chris
Ok. Updated patch attached. - domain.patch -> source patch against pgsql in cvs - drop_domain.sgml and create_domain.sgml -> New doc/src/sgml/ref docs - dominfo.txt -> basic domain related queries I used for testing Enables domains of array elements -> CREATE DOMAIN dom int4[3][2]; Uses a typbasetype column to describe the origin of the domain. Copies data to attnotnull rather than processing in execMain(). Some documentation differences from earlier. If this is approved, I'll start working on pg_dump, and a \dD <domain> option in psql, and regression tests. I don't really feel like doing those until the system table structure settles for pg_type. CHECKS when added, will also be copied to to the table attributes. FK Constraints (if I ever figure out how) will be done similarly. Both will lbe handled by MergeDomainAttributes() which is called shortly before MergeAttributes(). Any other recommendations? -- Rod Taylor This message represents the official view of the voices in my head ----- Original Message ----- From: "Tom Lane" <tgl@sss.pgh.pa.us> To: "Rod Taylor" <rbt@zort.ca> Cc: <pgsql-patches@postgresql.org> Sent: Sunday, February 24, 2002 8:11 PM Subject: Re: [PATCHES] Basic DOMAIN Support > "Rod Taylor" <rbt@zort.ca> writes: > > I intend to add other parts of domain support later on (no reason to > > hold up committing this though) but would appreciate some feedback > > about what I've done. > > Still needs some work ... > > One serious problem is that I do not think you can get away with reusing > typelem to link domains to base types. All the array code is going to > think that a domain is an array, and proceed to do horribly wrong > things. User applications may think the same thing, so even if you > wanted to change every backend routine that looks at typelem, it > wouldn't be enough. I think the only safe way to proceed is to add a > separate column that links a domain to its base type. This'd also save > you from having to add another meaning to typtype (since a domain could > be recognized by nonzero typbasetype). That should reduce the > likelihood of breaking existing code, and perhaps make life simpler when > it comes time to allow freestanding composite types (why shouldn't a > domain have a composite type as base?). > > Come to think of it, even without freestanding composite types it'd be > possible to try to define a domain as a subtype of a composite type, > and to use same as (eg) a function argument or result type. I doubt > you are anywhere near making that behave reasonably, though. Might be > best to disallow it for now. > > Speaking of arrays --- have you thought about arrays of domain-type > objects? I'm not sure whether any of the supported features matter for > array elements, but if they do it's not clear how to make it happen. > > Another objection is the changes you made in execMain.c. That extra > syscache lookup for every field of every tuple is going to be a rather > nasty performance hit, especially seeing that people will pay it whether > they ever heard of domains or not. And it seems quite unnecessary; if > you copy the domain's notnull bit into the pg_attribute row, then the > existing coding will do fine, no? > > I think also that you may have created some subtle changes in the > semantics of type default-value specifications; we'll need to think > if any compatibility problems have been introduced. There are doubtless > hardly any people using the feature, so this is not a serious objection, > but if any change has occurred it should be documented. > > > Overall, an impressive first cut! > > regards, tom lane >
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Rod Taylor wrote:
> Ok. Updated patch attached.
>
> - domain.patch -> source patch against pgsql in cvs
> - drop_domain.sgml and create_domain.sgml -> New doc/src/sgml/ref docs
> - dominfo.txt -> basic domain related queries I used for testing
>
> Enables domains of array elements -> CREATE DOMAIN dom int4[3][2];
>
> Uses a typbasetype column to describe the origin of the domain.
>
> Copies data to attnotnull rather than processing in execMain().
>
> Some documentation differences from earlier.
>
> If this is approved, I'll start working on pg_dump, and a \dD <domain>
> option in psql, and regression tests. I don't really feel like doing
> those until the system table structure settles for pg_type.
>
>
> CHECKS when added, will also be copied to to the table attributes. FK
> Constraints (if I ever figure out how) will be done similarly. Both
> will lbe handled by MergeDomainAttributes() which is called shortly
> before MergeAttributes().
>
>
> Any other recommendations?
>
> --
> Rod Taylor
>
> This message represents the official view of the voices in my head
>
> ----- Original Message -----
> From: "Tom Lane" <tgl@sss.pgh.pa.us>
> To: "Rod Taylor" <rbt@zort.ca>
> Cc: <pgsql-patches@postgresql.org>
> Sent: Sunday, February 24, 2002 8:11 PM
> Subject: Re: [PATCHES] Basic DOMAIN Support
>
>
> > "Rod Taylor" <rbt@zort.ca> writes:
> > > I intend to add other parts of domain support later on (no reason
> to
> > > hold up committing this though) but would appreciate some feedback
> > > about what I've done.
> >
> > Still needs some work ...
> >
> > One serious problem is that I do not think you can get away with
> reusing
> > typelem to link domains to base types. All the array code is going
> to
> > think that a domain is an array, and proceed to do horribly wrong
> > things. User applications may think the same thing, so even if you
> > wanted to change every backend routine that looks at typelem, it
> > wouldn't be enough. I think the only safe way to proceed is to add
> a
> > separate column that links a domain to its base type. This'd also
> save
> > you from having to add another meaning to typtype (since a domain
> could
> > be recognized by nonzero typbasetype). That should reduce the
> > likelihood of breaking existing code, and perhaps make life simpler
> when
> > it comes time to allow freestanding composite types (why shouldn't a
> > domain have a composite type as base?).
> >
> > Come to think of it, even without freestanding composite types it'd
> be
> > possible to try to define a domain as a subtype of a composite type,
> > and to use same as (eg) a function argument or result type. I doubt
> > you are anywhere near making that behave reasonably, though. Might
> be
> > best to disallow it for now.
> >
> > Speaking of arrays --- have you thought about arrays of domain-type
> > objects? I'm not sure whether any of the supported features matter
> for
> > array elements, but if they do it's not clear how to make it happen.
> >
> > Another objection is the changes you made in execMain.c. That extra
> > syscache lookup for every field of every tuple is going to be a
> rather
> > nasty performance hit, especially seeing that people will pay it
> whether
> > they ever heard of domains or not. And it seems quite unnecessary;
> if
> > you copy the domain's notnull bit into the pg_attribute row, then
> the
> > existing coding will do fine, no?
> >
> > I think also that you may have created some subtle changes in the
> > semantics of type default-value specifications; we'll need to think
> > if any compatibility problems have been introduced. There are
> doubtless
> > hardly any people using the feature, so this is not a serious
> objection,
> > but if any change has occurred it should be documented.
> >
> >
> > Overall, an impressive first cut!
> >
> > regards, tom lane
> >
[ Attachment, skipping... ]
[ Attachment, skipping... ]
[ Attachment, skipping... ]
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Patch applied. Thanks. --------------------------------------------------------------------------- Rod Taylor wrote: > Ok. Updated patch attached. > > - domain.patch -> source patch against pgsql in cvs > - drop_domain.sgml and create_domain.sgml -> New doc/src/sgml/ref docs > - dominfo.txt -> basic domain related queries I used for testing > > Enables domains of array elements -> CREATE DOMAIN dom int4[3][2]; > > Uses a typbasetype column to describe the origin of the domain. > > Copies data to attnotnull rather than processing in execMain(). > > Some documentation differences from earlier. > > If this is approved, I'll start working on pg_dump, and a \dD <domain> > option in psql, and regression tests. I don't really feel like doing > those until the system table structure settles for pg_type. > > > CHECKS when added, will also be copied to to the table attributes. FK > Constraints (if I ever figure out how) will be done similarly. Both > will lbe handled by MergeDomainAttributes() which is called shortly > before MergeAttributes(). > > > Any other recommendations? > > -- > Rod Taylor > > This message represents the official view of the voices in my head > > ----- Original Message ----- > From: "Tom Lane" <tgl@sss.pgh.pa.us> > To: "Rod Taylor" <rbt@zort.ca> > Cc: <pgsql-patches@postgresql.org> > Sent: Sunday, February 24, 2002 8:11 PM > Subject: Re: [PATCHES] Basic DOMAIN Support > > > > "Rod Taylor" <rbt@zort.ca> writes: > > > I intend to add other parts of domain support later on (no reason > to > > > hold up committing this though) but would appreciate some feedback > > > about what I've done. > > > > Still needs some work ... > > > > One serious problem is that I do not think you can get away with > reusing > > typelem to link domains to base types. All the array code is going > to > > think that a domain is an array, and proceed to do horribly wrong > > things. User applications may think the same thing, so even if you > > wanted to change every backend routine that looks at typelem, it > > wouldn't be enough. I think the only safe way to proceed is to add > a > > separate column that links a domain to its base type. This'd also > save > > you from having to add another meaning to typtype (since a domain > could > > be recognized by nonzero typbasetype). That should reduce the > > likelihood of breaking existing code, and perhaps make life simpler > when > > it comes time to allow freestanding composite types (why shouldn't a > > domain have a composite type as base?). > > > > Come to think of it, even without freestanding composite types it'd > be > > possible to try to define a domain as a subtype of a composite type, > > and to use same as (eg) a function argument or result type. I doubt > > you are anywhere near making that behave reasonably, though. Might > be > > best to disallow it for now. > > > > Speaking of arrays --- have you thought about arrays of domain-type > > objects? I'm not sure whether any of the supported features matter > for > > array elements, but if they do it's not clear how to make it happen. > > > > Another objection is the changes you made in execMain.c. That extra > > syscache lookup for every field of every tuple is going to be a > rather > > nasty performance hit, especially seeing that people will pay it > whether > > they ever heard of domains or not. And it seems quite unnecessary; > if > > you copy the domain's notnull bit into the pg_attribute row, then > the > > existing coding will do fine, no? > > > > I think also that you may have created some subtle changes in the > > semantics of type default-value specifications; we'll need to think > > if any compatibility problems have been introduced. There are > doubtless > > hardly any people using the feature, so this is not a serious > objection, > > but if any change has occurred it should be documented. > > > > > > Overall, an impressive first cut! > > > > regards, tom lane > > [ Attachment, skipping... ] [ Attachment, skipping... ] [ Attachment, skipping... ] [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Rod, seems there were some problems with this patch, and I have backed it out of CVS. The issues are that it did not compile cleanly against current CVS, there were shift-reduce conflicts in the grammar changes, there were compiler warnings that need fixing, and the patch caused the regression tests to fail. Would you please address these issues and submit a new patch. Thanks. --------------------------------------------------------------------------- pgman wrote: > > Patch applied. Thanks. > > --------------------------------------------------------------------------- > > > > Rod Taylor wrote: > > Ok. Updated patch attached. > > > > - domain.patch -> source patch against pgsql in cvs > > - drop_domain.sgml and create_domain.sgml -> New doc/src/sgml/ref docs > > - dominfo.txt -> basic domain related queries I used for testing > > > > Enables domains of array elements -> CREATE DOMAIN dom int4[3][2]; > > > > Uses a typbasetype column to describe the origin of the domain. > > > > Copies data to attnotnull rather than processing in execMain(). > > > > Some documentation differences from earlier. > > > > If this is approved, I'll start working on pg_dump, and a \dD <domain> > > option in psql, and regression tests. I don't really feel like doing > > those until the system table structure settles for pg_type. > > > > > > CHECKS when added, will also be copied to to the table attributes. FK > > Constraints (if I ever figure out how) will be done similarly. Both > > will lbe handled by MergeDomainAttributes() which is called shortly > > before MergeAttributes(). > > > > > > Any other recommendations? > > > > -- > > Rod Taylor > > > > This message represents the official view of the voices in my head > > > > ----- Original Message ----- > > From: "Tom Lane" <tgl@sss.pgh.pa.us> > > To: "Rod Taylor" <rbt@zort.ca> > > Cc: <pgsql-patches@postgresql.org> > > Sent: Sunday, February 24, 2002 8:11 PM > > Subject: Re: [PATCHES] Basic DOMAIN Support > > > > > > > "Rod Taylor" <rbt@zort.ca> writes: > > > > I intend to add other parts of domain support later on (no reason > > to > > > > hold up committing this though) but would appreciate some feedback > > > > about what I've done. > > > > > > Still needs some work ... > > > > > > One serious problem is that I do not think you can get away with > > reusing > > > typelem to link domains to base types. All the array code is going > > to > > > think that a domain is an array, and proceed to do horribly wrong > > > things. User applications may think the same thing, so even if you > > > wanted to change every backend routine that looks at typelem, it > > > wouldn't be enough. I think the only safe way to proceed is to add > > a > > > separate column that links a domain to its base type. This'd also > > save > > > you from having to add another meaning to typtype (since a domain > > could > > > be recognized by nonzero typbasetype). That should reduce the > > > likelihood of breaking existing code, and perhaps make life simpler > > when > > > it comes time to allow freestanding composite types (why shouldn't a > > > domain have a composite type as base?). > > > > > > Come to think of it, even without freestanding composite types it'd > > be > > > possible to try to define a domain as a subtype of a composite type, > > > and to use same as (eg) a function argument or result type. I doubt > > > you are anywhere near making that behave reasonably, though. Might > > be > > > best to disallow it for now. > > > > > > Speaking of arrays --- have you thought about arrays of domain-type > > > objects? I'm not sure whether any of the supported features matter > > for > > > array elements, but if they do it's not clear how to make it happen. > > > > > > Another objection is the changes you made in execMain.c. That extra > > > syscache lookup for every field of every tuple is going to be a > > rather > > > nasty performance hit, especially seeing that people will pay it > > whether > > > they ever heard of domains or not. And it seems quite unnecessary; > > if > > > you copy the domain's notnull bit into the pg_attribute row, then > > the > > > existing coding will do fine, no? > > > > > > I think also that you may have created some subtle changes in the > > > semantics of type default-value specifications; we'll need to think > > > if any compatibility problems have been introduced. There are > > doubtless > > > hardly any people using the feature, so this is not a serious > > objection, > > > but if any change has occurred it should be documented. > > > > > > > > > Overall, an impressive first cut! > > > > > > regards, tom lane > > > > > [ Attachment, skipping... ] > > [ Attachment, skipping... ] > > [ Attachment, skipping... ] > > [ Attachment, skipping... ] > > > > > ---------------------------(end of broadcast)--------------------------- > > TIP 2: you can get off all lists at once with the unregister command > > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 853-3000 > + If your life is a hard drive, | 830 Blythe Avenue > + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
I've corrected the problem with shell type creation, so regression tests work properly now (with the exception of NOTICE -> INFO / WARNING changes). I have issues with timestamp tests, but the failures match current cvs so I'll assume I've not contributed to them. The shift / reduce problem was fixed by simply removing the ability to make types with complex defaults (reverted back to old simple methods). Appears to still work. Storage is still mixed string / binary. I don't see any of the compile warnings other were receiving though. Anyhow, new version attached. Also attached is the regression sql set I used for domains. Yes, some items are supposed to fail (generally marked or obvious). Sorry about earlier. With any luck, this will do it. Umm.. It should be mentioned this is the first time I've ever dealt with C, and more specifically PostgreSQL internals, so tread lightly. -- Rod Taylor This message represents the official view of the voices in my head ----- Original Message ----- From: "Bruce Momjian" <pgman@candle.pha.pa.us> To: <pgman@candle.pha.pa.us> Cc: "Rod Taylor" <rbt@zort.ca>; "Tom Lane" <tgl@sss.pgh.pa.us>; <pgsql-patches@postgresql.org> Sent: Thursday, March 07, 2002 1:11 PM Subject: Re: [PATCHES] Basic DOMAIN Support > > Rod, seems there were some problems with this patch, and I have backed > it out of CVS. The issues are that it did not compile cleanly against > current CVS, there were shift-reduce conflicts in the grammar changes, > there were compiler warnings that need fixing, and the patch caused the > regression tests to fail. > > Would you please address these issues and submit a new patch. Thanks. > > -------------------------------------------------------------------- ------- > > pgman wrote: > > > > Patch applied. Thanks. > > > > -------------------------------------------------------------------- ------- > > > > > > > > Rod Taylor wrote: > > > Ok. Updated patch attached. > > > > > > - domain.patch -> source patch against pgsql in cvs > > > - drop_domain.sgml and create_domain.sgml -> New doc/src/sgml/ref docs > > > - dominfo.txt -> basic domain related queries I used for testing > > > > > > Enables domains of array elements -> CREATE DOMAIN dom int4[3][2]; > > > > > > Uses a typbasetype column to describe the origin of the domain. > > > > > > Copies data to attnotnull rather than processing in execMain(). > > > > > > Some documentation differences from earlier. > > > > > > If this is approved, I'll start working on pg_dump, and a \dD <domain> > > > option in psql, and regression tests. I don't really feel like doing > > > those until the system table structure settles for pg_type. > > > > > > > > > CHECKS when added, will also be copied to to the table attributes. FK > > > Constraints (if I ever figure out how) will be done similarly. Both > > > will lbe handled by MergeDomainAttributes() which is called shortly > > > before MergeAttributes(). > > > > > > > > > Any other recommendations? > > > > > > -- > > > Rod Taylor > > > > > > This message represents the official view of the voices in my head > > > > > > ----- Original Message ----- > > > From: "Tom Lane" <tgl@sss.pgh.pa.us> > > > To: "Rod Taylor" <rbt@zort.ca> > > > Cc: <pgsql-patches@postgresql.org> > > > Sent: Sunday, February 24, 2002 8:11 PM > > > Subject: Re: [PATCHES] Basic DOMAIN Support > > > > > > > > > > "Rod Taylor" <rbt@zort.ca> writes: > > > > > I intend to add other parts of domain support later on (no reason > > > to > > > > > hold up committing this though) but would appreciate some feedback > > > > > about what I've done. > > > > > > > > Still needs some work ... > > > > > > > > One serious problem is that I do not think you can get away with > > > reusing > > > > typelem to link domains to base types. All the array code is going > > > to > > > > think that a domain is an array, and proceed to do horribly wrong > > > > things. User applications may think the same thing, so even if you > > > > wanted to change every backend routine that looks at typelem, it > > > > wouldn't be enough. I think the only safe way to proceed is to add > > > a > > > > separate column that links a domain to its base type. This'd also > > > save > > > > you from having to add another meaning to typtype (since a domain > > > could > > > > be recognized by nonzero typbasetype). That should reduce the > > > > likelihood of breaking existing code, and perhaps make life simpler > > > when > > > > it comes time to allow freestanding composite types (why shouldn't a > > > > domain have a composite type as base?). > > > > > > > > Come to think of it, even without freestanding composite types it'd > > > be > > > > possible to try to define a domain as a subtype of a composite type, > > > > and to use same as (eg) a function argument or result type. I doubt > > > > you are anywhere near making that behave reasonably, though. Might > > > be > > > > best to disallow it for now. > > > > > > > > Speaking of arrays --- have you thought about arrays of domain-type > > > > objects? I'm not sure whether any of the supported features matter > > > for > > > > array elements, but if they do it's not clear how to make it happen. > > > > > > > > Another objection is the changes you made in execMain.c. That extra > > > > syscache lookup for every field of every tuple is going to be a > > > rather > > > > nasty performance hit, especially seeing that people will pay it > > > whether > > > > they ever heard of domains or not. And it seems quite unnecessary; > > > if > > > > you copy the domain's notnull bit into the pg_attribute row, then > > > the > > > > existing coding will do fine, no? > > > > > > > > I think also that you may have created some subtle changes in the > > > > semantics of type default-value specifications; we'll need to think > > > > if any compatibility problems have been introduced. There are > > > doubtless > > > > hardly any people using the feature, so this is not a serious > > > objection, > > > > but if any change has occurred it should be documented. > > > > > > > > > > > > Overall, an impressive first cut! > > > > > > > > regards, tom lane > > > > > > > > [ Attachment, skipping... ] > > > > [ Attachment, skipping... ] > > > > [ Attachment, skipping... ] > > > > [ Attachment, skipping... ] > > > > > > > > ---------------------------(end of broadcast)--------------------------- > > > TIP 2: you can get off all lists at once with the unregister command > > > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) > > > > -- > > Bruce Momjian | http://candle.pha.pa.us > > pgman@candle.pha.pa.us | (610) 853-3000 > > + If your life is a hard drive, | 830 Blythe Avenue > > + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 853-3000 > + If your life is a hard drive, | 830 Blythe Avenue > + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 >
Attachment
Rod Taylor wrote: > I've corrected the problem with shell type creation, so regression > tests work properly now (with the exception of NOTICE -> INFO / > WARNING changes). I have issues with timestamp tests, but the > failures match current cvs so I'll assume I've not contributed to > them. If you do a CVS update those regression problems should go away. Sorry about that. > The shift / reduce problem was fixed by simply removing the ability to > make types with complex defaults (reverted back to old simple > methods). Appears to still work. Storage is still mixed string / > binary. Great. > I don't see any of the compile warnings other were receiving though. Let me test here and see. Will report back. > Also attached is the regression sql set I used for domains. Yes, some > items are supposed to fail (generally marked or obvious). Good. > Sorry about earlier. With any luck, this will do it. Umm.. It should > be mentioned this is the first time I've ever dealt with C, and more > specifically PostgreSQL internals, so tread lightly. If this is your first time dealing with C, I am amazed. It took me years to get to the level of the patch you submitted. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
On Thu, 2002-03-07 at 17:18, Rod Taylor wrote: > I've corrected the problem with shell type creation, so regression > tests work properly now (with the exception of NOTICE -> INFO / > WARNING changes). I have issues with timestamp tests, but the > failures match current cvs so I'll assume I've not contributed to > them. All the regression tests (including the timestamp related ones) pass for me with current CVS, without the patch applied. With the patch applied, the "privileges" test fails. > The shift / reduce problem was fixed by simply removing the ability to > make types with complex defaults (reverted back to old simple > methods). I still see a shift / reduce conflict in gram.y > I don't see any of the compile warnings other were receiving though. make[3]: Entering directory `/home/nconway/pgsql/src/backend/catalog' gcc-3.0 -O2 -Wall -Wmissing-prototypes -Wmissing-declarations -I../../../src/include -c -o heap.o heap.c heap.c: In function `cookDefault': heap.c:1904: warning: implicit declaration of function `getBaseType' make[3]: Entering directory `/home/nconway/pgsql/src/backend/parser' gcc-3.0 -O2 -Wall -Wmissing-prototypes -Wmissing-declarations -I../../../src/include -c -o analyze.o analyze.c gcc-3.0 -O2 -Wall -Wmissing-prototypes -Wmissing-declarations -I../../../src/include -c -o gram.o gram.c gram.y: In function `yyparse': gram.y:3228: warning: assignment from incompatible pointer type gram.y:3232: warning: assignment from incompatible pointer type gcc-3.0 -O2 -Wall -Wmissing-prototypes -Wmissing-declarations -I../../../src/include -c -o parse_coerce.o parse_coerce.c parse_coerce.c:637:1: warning: no newline at end of file make[3]: Entering directory `/home/nconway/pgsql/src/backend/nodes' gcc-3.0 -O2 -Wall -Wmissing-prototypes -Wmissing-declarations -I../../../src/include -c -o copyfuncs.o copyfuncs.c copyfuncs.c:2232: warning: `_copyCreateDomainStmt' defined but not used make[4]: Entering directory `/home/nconway/pgsql/src/backend/optimizer/prep' gcc-3.0 -O2 -Wall -Wmissing-prototypes -Wmissing-declarations -I../../../../src/include -c -o preptlist.o preptlist.c preptlist.c: In function `build_column_default': preptlist.c:358: warning: unused variable `typedefault' Also: src/backend/catalog/heap.c: spelling: "typically for domiains" Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Sorry for making this a painful process. pg_depend will be alot better :) -- chalk it up to a learning experience I suppose. Bruce: Docs and regress queries still apply. Anyway, dug out a FreeBSD machine and tested this round on it. No compile errors or warnings (not from my parts anyway), bison was quite (see below), and regression tests ran perfectly. My win2k cygwin environment still has a ton of regress errors (postmaster crashes mostly)... > > The shift / reduce problem was fixed by simply removing the ability to > > make types with complex defaults (reverted back to old simple > > methods). > > I still see a shift / reduce conflict in gram.y The primary issue is that I want to do an " '=' c_expr " (through a couple of steps) for the type default. I've tried several things to get rid of it, but nothing works -- and I want an expression (closer to SQL 99 Create Type). Without the '=' between DEFAULT and c_expr everything is fine -- of course that could break applications that use CREATE TYPE. Anyway, I copped out and added a %expect 1. I think this is close enough to the infamous 'if / else' scenario with expressions that it's warrented (the reason they came up with it). Perhaps it could be opt_equal and drop the equal sign in 7.4 or something to remove the conflict entirely? For the errors, I just realized that if I redirect output to a log file I'm left with only warnings and errors. Is there a website with these kinds of tricks listed? > > I don't see any of the compile warnings other were receiving though. > > make[3]: Entering directory `/home/nconway/pgsql/src/backend/catalog' > gcc-3.0 -O2 -Wall -Wmissing-prototypes -Wmissing-declarations > -I../../../src/include -c -o heap.o heap.c > heap.c: In function `cookDefault': > heap.c:1904: warning: implicit declaration of function `getBaseType' header included. > make[3]: Entering directory `/home/nconway/pgsql/src/backend/parser' > gcc-3.0 -O2 -Wall -Wmissing-prototypes -Wmissing-declarations > -I../../../src/include -c -o analyze.o analyze.c > gcc-3.0 -O2 -Wall -Wmissing-prototypes -Wmissing-declarations > -I../../../src/include -c -o gram.o gram.c > gram.y: In function `yyparse': > gram.y:3228: warning: assignment from incompatible pointer type > gram.y:3232: warning: assignment from incompatible pointer type Your line numbers are slightly different but I believe they're CREATE DATABASE WITH OWNER issues. > gcc-3.0 -O2 -Wall -Wmissing-prototypes -Wmissing-declarations > -I../../../src/include -c -o parse_coerce.o parse_coerce.c > parse_coerce.c:637:1: warning: no newline at end of file Umm... Is this a gcc 3 thing? Either way, line added -- but I still think thats a silly thing to complain about. I didn't realize compilers cared about whitespace -- aside from Python anyway. > make[3]: Entering directory `/home/nconway/pgsql/src/backend/nodes' > gcc-3.0 -O2 -Wall -Wmissing-prototypes -Wmissing-declarations > -I../../../src/include -c -o copyfuncs.o copyfuncs.c > copyfuncs.c:2232: warning: `_copyCreateDomainStmt' defined but not used Commented out, but left in for potential use.
Attachment
"Rod Taylor" <rbt@zort.ca> writes:
> Anyway, I copped out and added a %expect 1.
Sorry, but we agreed long ago that %expect is *not* an acceptable
alternative to getting the bugs out of your yacc productions.
%expect is okay for stable grammars or ones that are being hacked by
just one or two closely-involved people. The PG grammar is being hit on
all the time by a lot of people with varying yacc skill, and some of the
rearrangements are not trivial. The problem with %expect for us is: all
right, we just ignored N shift/reduce errors --- but are they the same N
errors that someone once long ago decided were okay? It's too risky.
I believe your real problem has nothing to do with the c_expr anyway,
but is that DEFAULT is an allowable ColLabel. I do not recall which
keywords we currently need to accept as def_elem labels, but perhaps
it'd work to back off the lefthand side from ColLabel to ColId or
some such.
regards, tom lane
> "Rod Taylor" <rbt@zort.ca> writes: > > Anyway, I copped out and added a %expect 1. > > Sorry, but we agreed long ago that %expect is *not* an acceptable > alternative to getting the bugs out of your yacc productions. Just a quick questions. Every time I try to apply one of these patches (patch < domain.patch) in the pgsql directory checked out from latest CVS, I get all sorts of bad hunk errors and stuff. Do these matter? How do I apply a patch? I just thought I'd do a 64bit compile of Rod's code to help him find errors... Chris
"Rod Taylor" <rbt@zort.ca> writes:
>> copyfuncs.c:2232: warning: `_copyCreateDomainStmt' defined but not
> used
> Commented out, but left in for potential use.
Wrong answer --- what this is reminding you is that you forgot to call
it.
Also, I'm fairly confused by the code in MergeDomainAttributes and
DefineDomain. You seem to have modeled this on table creation, not
type creation, which makes little or no sense to me. Where in the
heck does a domain "inherit" from?
regards, tom lane
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
> Just a quick questions. Every time I try to apply one of these patches
> (patch < domain.patch) in the pgsql directory checked out from latest CVS, I
> get all sorts of bad hunk errors and stuff. Do these matter?
Probably.
> How do I apply a patch?
I noticed that Rod's message was (rather badly) MIME-encoded. You may
have to undo the encoding to get a clean patch file to apply. Or maybe
it's something else; I've not tried to apply his patches yet, only
eyeballed them. But I imagine Bruce would've bounced the earlier patch
if he had difficulty applying it.
regards, tom lane
It's a cvs diff -c, rather than a straight diff -c against other sources. Cvs diff tosses on some additional information which patch complains about, but it's ok to ignore. And you can blame MS Outlook for any mime encoding issues of my email.s -- Rod Taylor This message represents the official view of the voices in my head ----- Original Message ----- From: "Tom Lane" <tgl@sss.pgh.pa.us> To: "Christopher Kings-Lynne" <chriskl@familyhealth.com.au> Cc: "Rod Taylor" <rbt@zort.ca>; "Neil Conway" <nconway@klamath.dyndns.org>; "Bruce Momjian" <pgman@candle.pha.pa.us>; "PostgreSQL Patches" <pgsql-patches@postgresql.org> Sent: Thursday, March 07, 2002 10:01 PM Subject: Re: [PATCHES] Basic DOMAIN Support > "Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes: > > Just a quick questions. Every time I try to apply one of these patches > > (patch < domain.patch) in the pgsql directory checked out from latest CVS, I > > get all sorts of bad hunk errors and stuff. Do these matter? > > Probably. > > > How do I apply a patch? > > I noticed that Rod's message was (rather badly) MIME-encoded. You may > have to undo the encoding to get a clean patch file to apply. Or maybe > it's something else; I've not tried to apply his patches yet, only > eyeballed them. But I imagine Bruce would've bounced the earlier patch > if he had difficulty applying it. > > regards, tom lane >
> "Rod Taylor" <rbt@zort.ca> writes: > >> copyfuncs.c:2232: warning: `_copyCreateDomainStmt' defined but not > > used > > > Commented out, but left in for potential use. > > Wrong answer --- what this is reminding you is that you forgot to call > it. Yeah, that was my thought too -- when I created it I was unable to find how to use it. Just that I noticed a TODO item stating that everything should have one. I guess I don't understand -- among other things -- why DomainStmt needs to be copied? Found the switch at the bottom of copyfuncs.c, and have added the element. > Also, I'm fairly confused by the code in MergeDomainAttributes and > DefineDomain. You seem to have modeled this on table creation, not > type creation, which makes little or no sense to me. Where in the > heck does a domain "inherit" from? MergeDomainAttributes pushes the domains configuration onto a table field during table creation where the field type is a domain. Overkill for defaults, but will be very useful once domains have various constraints. In this way the table is very much inheriting the attributes of the domain. This was created as per your suggestion that a table should take on domain attributes during creation rather than during execution. Define domain pulls in attributes of a type BUT has to process things such as Defaults, Not NULL constraints and eventually CHECK constraints, Foreign keys. Those items are very much like table fields, just stored elsewhere.
Tom Lane wrote: > > How do I apply a patch? > > I noticed that Rod's message was (rather badly) MIME-encoded. You may > have to undo the encoding to get a clean patch file to apply. Or maybe > it's something else; I've not tried to apply his patches yet, only > eyeballed them. But I imagine Bruce would've bounced the earlier patch > if he had difficulty applying it. Patch applies cleanly here with zero warnings. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
> I believe your real problem has nothing to do with the c_expr anyway, > but is that DEFAULT is an allowable ColLabel. I do not recall which > keywords we currently need to accept as def_elem labels, but perhaps > it'd work to back off the lefthand side from ColLabel to ColId or > some such. Changing ColLabel to ColId seems to have corrected the problem but now I'm completely confused as to how it works. From the light reading I did an hour ago off various websites about shift reduce conflicts (where I found that %expect thing) was that the DEFAULT element would be moved off the stack immediatly as it was a known element which could not be reduced. Hence my thoughts it was the equal sign and c_expr. Seems Amazon will receive an order for a book on bison tomorrow.
"Rod Taylor" <rbt@zort.ca> writes:
> Changing ColLabel to ColId seems to have corrected the problem but now
> I'm completely confused as to how it works.
Well, you don't really need to know anything about the parsing algorithm
to see that the way you had it was ambiguous. Consider input
DEFAULT = foo
If DEFAULT can be a ColLabel, then there are two valid parsings of
this input: one in which foo is reduced to c_expr and one in which
it's reduced to a typename. That ambiguity is what creates the
shift/reduce conflict.
BTW, I would recommend b_expr not c_expr as the expression nonterminal
to use, if possible. (Actually, I suspect a_expr would work, in which
case you might as well use it.)
regards, tom lane
> "Rod Taylor" <rbt@zort.ca> writes: > > Changing ColLabel to ColId seems to have corrected the problem but now > > I'm completely confused as to how it works. > > Well, you don't really need to know anything about the parsing algorithm > to see that the way you had it was ambiguous. Consider input > DEFAULT = foo > If DEFAULT can be a ColLabel, then there are two valid parsings of > this input: one in which foo is reduced to c_expr and one in which > it's reduced to a typename. That ambiguity is what creates the > shift/reduce conflict. I see.. For some reason I convinced myself that each level was intepreted by itself, if a match couldn't be found it would dig down rule. That is, take the first matching depth it finds. Conflicts were generated by 2 matches at the same deptch. In this case ColLabel is a subrule, where DEFAULT is a direct match to 'default'. Makes alot more sense to pre-resolve the tree when I think about it. I'm used to working with interpreted languages where something like that would kill a ton of CPU for a majority of cases, but when it's compiled it only needs to be resolved once; of course.
"Rod Taylor" <rbt@zort.ca> writes:
> find how to use it. Just that I noticed a TODO item stating that
> everything should have one. I guess I don't understand -- among other
> things -- why DomainStmt needs to be copied?
If it's not copiable then it will fail when used in plpgsql, among
probably other problems. In general there should be no parse
node types that don't have copyObject() and equal() support; and also
outfuncs.c support, for debugging dumps if nothing else. We've been
relatively lax on insisting on readfuncs.c support --- that really only
matters for constructs that can appear in rules.
> MergeDomainAttributes pushes the domains configuration onto a table
> field during table creation where the field type is a domain.
Ah. I had it backwards: thought it was pulling constraints into the
domain definition from elsewhere.
regards, tom lane
> BTW, I would recommend b_expr not c_expr as the expression nonterminal > to use, if possible. (Actually, I suspect a_expr would work, in which > case you might as well use it.) b_expr is what table defaults use, so I will match that.