ANSI Compliant Inserts
Reports missing values as bad.
BAD: INSERT INTO tab (col1, col2) VALUES ('val1');
GOOD: INSERT INTO tab (col1, col2) VALUES ('val1', 'val2');
Regress tests against DEFAULT and normal values as they're managed
slightly different.
Attachments:
ansiinsert.patchtext/plain; charset=ISO-8859-1; name=ansiinsert.patchDownload
Index: src/backend/parser/analyze.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/analyze.c,v
retrieving revision 1.229
diff -c -r1.229 analyze.c
*** src/backend/parser/analyze.c 2002/04/12 19:11:49 1.229
--- src/backend/parser/analyze.c 2002/04/15 02:03:51
***************
*** 547,556 ****
}
/*
! * XXX It is possible that the targetlist has fewer entries than were
! * in the columns list. We do not consider this an error. Perhaps we
! * should, if the columns list was explicitly given?
*/
/* done building the range table and jointree */
qry->rtable = pstate->p_rtable;
--- 547,558 ----
}
/*
! * Ensure that the targetlist has the same number of entries
! * that were present in the columns list. Don't do the check
! * for select statements.
*/
+ if (stmt->cols != NIL && (icolumns != NIL || attnos != NIL))
+ elog(ERROR, "INSERT has more target columns than expressions");
/* done building the range table and jointree */
qry->rtable = pstate->p_rtable;
***************
*** 3245,3251 ****
}
}
! result = NIL;
result = nconc(result, cxt.tables);
result = nconc(result, cxt.views);
result = nconc(result, cxt.grants);
--- 3247,3253 ----
}
}
! result = NIL;
result = nconc(result, cxt.tables);
result = nconc(result, cxt.views);
result = nconc(result, cxt.grants);
Index: src/test/regress/expected/insert.out
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/insert.out,v
retrieving revision 1.1
diff -c -r1.1 insert.out
*** src/test/regress/expected/insert.out 2002/04/05 11:56:55 1.1
--- src/test/regress/expected/insert.out 2002/04/15 02:03:55
***************
*** 17,20 ****
--- 17,40 ----
| 7 | testing
(4 rows)
+ --
+ -- insert with similar expression / target_list values (all fail)
+ --
+ insert into inserttest (col1, col2, col3) values (DEFAULT, DEFAULT);
+ ERROR: INSERT has more target columns than expressions
+ insert into inserttest (col1, col2, col3) values (1, 2);
+ ERROR: INSERT has more target columns than expressions
+ insert into inserttest (col1) values (1, 2);
+ ERROR: INSERT has more expressions than target columns
+ insert into inserttest (col1) values (DEFAULT, DEFAULT);
+ ERROR: INSERT has more expressions than target columns
+ select * from inserttest;
+ col1 | col2 | col3
+ ------+------+---------
+ | 3 | testing
+ | 5 | testing
+ | 5 | test
+ | 7 | testing
+ (4 rows)
+
drop table inserttest;
Index: src/test/regress/sql/insert.sql
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/sql/insert.sql,v
retrieving revision 1.1
diff -c -r1.1 insert.sql
*** src/test/regress/sql/insert.sql 2002/04/05 11:56:55 1.1
--- src/test/regress/sql/insert.sql 2002/04/15 02:03:55
***************
*** 9,12 ****
--- 9,22 ----
insert into inserttest values (DEFAULT, 7);
select * from inserttest;
+
+ --
+ -- insert with similar expression / target_list values (all fail)
+ --
+ insert into inserttest (col1, col2, col3) values (DEFAULT, DEFAULT);
+ insert into inserttest (col1, col2, col3) values (1, 2);
+ insert into inserttest (col1) values (1, 2);
+ insert into inserttest (col1) values (DEFAULT, DEFAULT);
+
+ select * from inserttest;
drop table inserttest;
Rod Taylor <rbt@zort.ca> writes:
/* ! * XXX It is possible that the targetlist has fewer entries than were ! * in the columns list. We do not consider this an error. Perhaps we ! * should, if the columns list was explicitly given? */ =20=20 /* done building the range table and jointree */ qry->rtable =3D pstate->p_rtable; --- 547,558 ---- } =20=20 /* ! * Ensure that the targetlist has the same number of entries ! * that were present in the columns list. Don't do the check ! * for select statements. */ + if (stmt->cols !=3D NIL && (icolumns !=3D NIL || attnos !=3D NIL)) + elog(ERROR, "INSERT has more target columns than expressions");
What's the rationale for changing this exactly?
The code might or might not need changing (I believe the XXX comment
questioning it is mine, in fact) but changing behavior without any
pghackers discussion is not the way to approach this.
In general I'm suspicious of rejecting cases we used to accept for
no good reason other than that it's not in the spec. There is a LOT
of Postgres behavior that's not in the spec.
regards, tom lane
Tom Lane wrote:
Rod Taylor <rbt@zort.ca> writes:
/* ! * XXX It is possible that the targetlist has fewer entries than were ! * in the columns list. We do not consider this an error. Perhaps we ! * should, if the columns list was explicitly given? */ =20=20 /* done building the range table and jointree */ qry->rtable =3D pstate->p_rtable; --- 547,558 ---- } =20=20 /* ! * Ensure that the targetlist has the same number of entries ! * that were present in the columns list. Don't do the check ! * for select statements. */ + if (stmt->cols !=3D NIL && (icolumns !=3D NIL || attnos !=3D NIL)) + elog(ERROR, "INSERT has more target columns than expressions");What's the rationale for changing this exactly?
The code might or might not need changing (I believe the XXX comment
questioning it is mine, in fact) but changing behavior without any
pghackers discussion is not the way to approach this.In general I'm suspicious of rejecting cases we used to accept for
no good reason other than that it's not in the spec. There is a LOT
of Postgres behavior that's not in the spec.
TODO has:
o Disallow missing columns in INSERT ... VALUES, per ANSI
I think it should be done because it is very easy to miss columns on
INSERT without knowing it. I think our current behavior is too
error-prone. Now, if we want to just throw a NOTICE is such cases, that
would work too.
Clearly he didn't need discussion because it was already on the TODO
list. I guess the question is whether it should have had a question
mark. I certainly didn't think so.
Also, I thought we were going to fix COPY to reject missing columns too.
I just can't see a valid reason for allowing missing columns in either
case, except to hide errors.
--
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 19026Bruce Momjian <pgman@candle.pha.pa.us> writes:
In general I'm suspicious of rejecting cases we used to accept for
no good reason other than that it's not in the spec. There is a LOT
of Postgres behavior that's not in the spec.
TODO has:
o Disallow missing columns in INSERT ... VALUES, per ANSI
Where's the discussion that's the basis of that entry? I don't recall
any existing consensus on this (though maybe I forgot).
There are a fair number of things in the TODO list that you put there
because you liked 'em, but that doesn't mean everyone else agrees.
I certainly will not accept "once it's on the TODO list it cannot be
questioned"...
regards, tom lane
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
In general I'm suspicious of rejecting cases we used to accept for
no good reason other than that it's not in the spec. There is a LOT
of Postgres behavior that's not in the spec.TODO has:
o Disallow missing columns in INSERT ... VALUES, per ANSIWhere's the discussion that's the basis of that entry? I don't recall
any existing consensus on this (though maybe I forgot).
I assume someone (Peter?) looked it up and reported that our current
behavior was incorrect and not compliant. I didn't do the research in
whether it was compliant.
There are a fair number of things in the TODO list that you put there
because you liked 'em, but that doesn't mean everyone else agrees.
I certainly will not accept "once it's on the TODO list it cannot be
questioned"...
I put it there because I didn't think there was any question. If I was
wrong, I can add a question mark to it.
Do you want to argue we should continue allowing it? Also, what about
missing trailling columns in COPY?
If we continue allowing missing INSERT columns, I am not sure we can
claim it is an extension or whether the standard requires the query to
fail.
--
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 19026A team member had a bug in their SQL code which would have been caught
with this, so I looked it up. Found the TODO entry indicating it was
something that should be done. It was fairly simple to do, so I went
forward with it.
If it's not wanted, then feel free to reject the patch and remove the
TODO item -- or change the TODO item to indicate discussion is
required.
--
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.
----- Original Message -----
From: "Tom Lane" <tgl@sss.pgh.pa.us>
To: "Rod Taylor" <rbt@zort.ca>
Cc: <pgsql-patches@postgresql.org>
Sent: Sunday, April 14, 2002 11:09 PM
Subject: Re: [PATCHES] ANSI Compliant Inserts
Rod Taylor <rbt@zort.ca> writes:
/*
! * XXX It is possible that the targetlist has fewer entries than
were
! * in the columns list. We do not consider this an error.
Perhaps we
! * should, if the columns list was explicitly given? */ =20=20 /* done building the range table and jointree */ qry->rtable =3D pstate->p_rtable; --- 547,558 ---- } =20=20 /* ! * Ensure that the targetlist has the same number of entries ! * that were present in the columns list. Don't do the check ! * for select statements. */ + if (stmt->cols !=3D NIL && (icolumns !=3D NIL || attnos !=3D
NIL))
+ elog(ERROR, "INSERT has more target columns than expressions");
What's the rationale for changing this exactly?
The code might or might not need changing (I believe the XXX comment
questioning it is mine, in fact) but changing behavior without any
pghackers discussion is not the way to approach this.In general I'm suspicious of rejecting cases we used to accept for
no good reason other than that it's not in the spec. There is a LOT
of Postgres behavior that's not in the spec.regards, tom lane
---------------------------(end of
broadcast)---------------------------
Show quoted text
TIP 4: Don't 'kill -9' the postmaster
Rod Taylor wrote:
A team member had a bug in their SQL code which would have been caught
with this, so I looked it up. Found the TODO entry indicating it was
something that should be done. It was fairly simple to do, so I went
forward with it.If it's not wanted, then feel free to reject the patch and remove the
TODO item -- or change the TODO item to indicate discussion is
required.
Yes, that is the point. Tom thinks discussion is required on this item,
while I can't imagine why. OK, Tom, let's discuss.
--
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 19026Bruce Momjian <pgman@candle.pha.pa.us> writes:
Do you want to argue we should continue allowing it?
No; I'm objecting that there hasn't been adequate discussion about
this change of behavior.
BTW, if the rationale for the change is "ANSI compliance" then the patch
is still wrong. SQL92 says:
3) No <column name> of T shall be identified more than once. If the
<insert column list> is omitted, then an <insert column list>
that identifies all columns of T in the ascending sequence of
their ordinal positions within T is implicit.
5) Let QT be the table specified by the <query expression>. The
degree of QT shall be equal to the number of <column name>s in
the <insert column list>.
The patch enforces equality only for the case of an explicit <insert
column list> --- which is the behavior I suggested in the original
comment, but the spec clearly requires an exact match for an implicit
list too. How tight do we want to get?
In any case this discussion should be taking place someplace more public
than -patches.
regards, tom lane
I submitted a patch which would make Postgresql ANSI compliant in
regards to INSERT with a provided column list. As Tom states below,
this is not full compliance.
CREATE TABLE tab(col1 text, col2 text);
INSERT INTO tab (col1, col2) VALUES ('val1'); -- bad by spec (enforced
by patch)
INSERT INTO tab (col1, col2) VALUES ('val1', 'val2'); -- good
INSERT INTO tab VALUES ('val1'); -- bad by spec (not enforced)
INSERT INTO tab VALUES ('val1', 'val2'); -- good
Currently in postgres all of the above are valid. I'd like to rule
out the first case (as enforced by the patch) as it's obvious the user
had intended to have two values. Especially useful when the user
misses a value and inserts bad data into the table as a result.
For the latter one, it could be argued that the user understands the
table in question and has inserted the values they require. New
columns are added at the end, and probably don't affect the operation
in question so why should it be changed to suit new columns? But,
automated code should always be written with the columns explicitly
listed, so this may be a user who has simply forgotten to add the
value -- easy to do on wide tables.
Thoughts?
--
Rod Taylor
----- Original Message -----
From: "Tom Lane" <tgl@sss.pgh.pa.us>
To: "Bruce Momjian" <pgman@candle.pha.pa.us>
Cc: "Rod Taylor" <rbt@zort.ca>; <pgsql-patches@postgresql.org>
Sent: Sunday, April 14, 2002 11:49 PM
Subject: Re: [PATCHES] ANSI Compliant Inserts
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Do you want to argue we should continue allowing it?
No; I'm objecting that there hasn't been adequate discussion about
this change of behavior.BTW, if the rationale for the change is "ANSI compliance" then the
patch
is still wrong. SQL92 says:
3) No <column name> of T shall be identified more than
once. If the
<insert column list> is omitted, then an <insert column
list>
that identifies all columns of T in the ascending
sequence of
their ordinal positions within T is implicit.
5) Let QT be the table specified by the <query expression>.
The
degree of QT shall be equal to the number of <column
name>s in
the <insert column list>.
The patch enforces equality only for the case of an explicit <insert
column list> --- which is the behavior I suggested in the original
comment, but the spec clearly requires an exact match for an
implicit
list too. How tight do we want to get?
In any case this discussion should be taking place someplace more
public
Show quoted text
than -patches.
regards, tom lane
"Rod Taylor" <rbt@zort.ca> writes:
CREATE TABLE tab(col1 text, col2 text);
INSERT INTO tab (col1, col2) VALUES ('val1'); -- bad by spec (enforced
by patch)
INSERT INTO tab (col1, col2) VALUES ('val1', 'val2'); -- good
INSERT INTO tab VALUES ('val1'); -- bad by spec (not enforced)
INSERT INTO tab VALUES ('val1', 'val2'); -- good
Currently in postgres all of the above are valid. I'd like to rule
out the first case (as enforced by the patch) as it's obvious the user
had intended to have two values.
Seems reasonable.
For the latter one, it could be argued that the user understands the
table in question and has inserted the values they require.
Ruling out this case would break a technique that I've used a lot in the
past, which is to put defaultable columns (eg, SERIAL columns) at the
end, so that they can simply be left out of quick manual inserts.
So I agree with this part too. (I wouldn't necessarily write
application code that way, but then I believe in the theory that robust
application code should always specify an explicit column list.)
For the record --- I actually am in favor of this patch; but I wanted
to see the change discussed and defended in a more widely-read mailing
list than -patches. If there are no objections from the assembled
hackers, apply away ...
regards, tom lane
[ Discussion moved to hackers.]
We are discussing TODO item:
o Disallow missing columns in INSERT ... VALUES, per ANSI
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Do you want to argue we should continue allowing it?
No; I'm objecting that there hasn't been adequate discussion about
this change of behavior.
So, you don't want to allow it, I don't want to allow it, the patch
author doesn't want to allow it. The reason the item doesn't require
much discussion is that I can't imagine anyone arguing we should allow
it. If there is anyone out there that doesn't want the listed TODO item
completed, please chime in now.
BTW, if the rationale for the change is "ANSI compliance" then the patch
is still wrong. SQL92 says:3) No <column name> of T shall be identified more than once. If the
<insert column list> is omitted, then an <insert column list>
that identifies all columns of T in the ascending sequence of
their ordinal positions within T is implicit.5) Let QT be the table specified by the <query expression>. The
degree of QT shall be equal to the number of <column name>s in
the <insert column list>.The patch enforces equality only for the case of an explicit <insert
column list> --- which is the behavior I suggested in the original
comment, but the spec clearly requires an exact match for an implicit
list too. How tight do we want to get?
Yes, I think we want both implicit and explicit column names to match
the VALUES list. We do have DEFAULT for INSERT now, so that should make
things somewhat easier for people wanting to insert DEFAULT values
without specifying the column list.
--
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 19026Rod Taylor wrote:
For the latter one, it could be argued that the user understands the
table in question and has inserted the values they require. New
columns are added at the end, and probably don't affect the operation
in question so why should it be changed to suit new columns? But,
automated code should always be written with the columns explicitly
listed, so this may be a user who has simply forgotten to add the
value -- easy to do on wide tables.
I think our new DEFAULT for insert allows people to properly match all
columns, and I think it is too error prone to allow missing columns in
any INSERT.
--
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 19026Tom Lane wrote:
"Rod Taylor" <rbt@zort.ca> writes:
CREATE TABLE tab(col1 text, col2 text);
INSERT INTO tab (col1, col2) VALUES ('val1'); -- bad by spec (enforced
by patch)
INSERT INTO tab (col1, col2) VALUES ('val1', 'val2'); -- goodINSERT INTO tab VALUES ('val1'); -- bad by spec (not enforced)
INSERT INTO tab VALUES ('val1', 'val2'); -- goodCurrently in postgres all of the above are valid. I'd like to rule
out the first case (as enforced by the patch) as it's obvious the user
had intended to have two values.Seems reasonable.
For the latter one, it could be argued that the user understands the
table in question and has inserted the values they require.Ruling out this case would break a technique that I've used a lot in the
past, which is to put defaultable columns (eg, SERIAL columns) at the
end, so that they can simply be left out of quick manual inserts.
So I agree with this part too. (I wouldn't necessarily write
application code that way, but then I believe in the theory that robust
application code should always specify an explicit column list.)
Yes, I understand the tempation to put the columns needing default at
the end and skipping them on INSERT. However, our new DEFAULT insert
value seems to handle that nicely, certainly better than the old code
did, and I think the added robustness of now requiring full columns on
INSERT is worth it.
I realize this could break some apps, but with the new DEFAULT value, it
seems like a good time to reign in this error-prone capability.
--
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 19026INSERT INTO tab VALUES ('val1'); -- bad by spec (not enforced)
INSERT INTO tab VALUES ('val1', 'val2'); -- goodI recall that this was the behavior we agreed we wanted. IMHO, it
would
be conditional on the INSERT ... VALUES (DEFAULT) capability being
provided. I'm not sure if that is there yet.
My patch for that was applied a couple weeks ago.
Import Notes
Reference msg id not found: Pine.LNX.4.30.0204150023520.717-100000@peter.localdomain | Resolved by subject fallback
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
Ruling out this case would break a technique that I've used a lot in the
past, which is to put defaultable columns (eg, SERIAL columns) at the
end, so that they can simply be left out of quick manual inserts.
Yes, I understand the tempation to put the columns needing default at
the end and skipping them on INSERT. However, our new DEFAULT insert
value seems to handle that nicely, certainly better than the old code
did, and I think the added robustness of now requiring full columns on
INSERT is worth it.
If I have two or three defaultable columns (say, a SERIAL primary key
and an insertion timestamp), it's going to be a pain in the neck to
have to write DEFAULT, DEFAULT, ... at the end of every insert.
I feel that people who want error cross-checking on this will have used
an explicit column list anyway. Therefore, Rod's patch tightens the
case that should be tight, while still being appropriately loose for
casual manual inserts.
BTW, I do *not* agree with equating this case with COPY. COPY is mostly
used for loading dumped data, and so it's reasonable to make different
tradeoffs between error checking and friendliness for COPY and INSERT.
regards, tom lane
Rod Taylor writes:
I submitted a patch which would make Postgresql ANSI compliant in
regards to INSERT with a provided column list. As Tom states below,
this is not full compliance.CREATE TABLE tab(col1 text, col2 text);
INSERT INTO tab (col1, col2) VALUES ('val1'); -- bad by spec (enforced
by patch)
INSERT INTO tab (col1, col2) VALUES ('val1', 'val2'); -- goodINSERT INTO tab VALUES ('val1'); -- bad by spec (not enforced)
INSERT INTO tab VALUES ('val1', 'val2'); -- good
I recall that this was the behavior we agreed we wanted. IMHO, it would
be conditional on the INSERT ... VALUES (DEFAULT) capability being
provided. I'm not sure if that is there yet.
--
Peter Eisentraut peter_e@gmx.net
Peter Eisentraut <peter_e@gmx.net> writes:
I recall that this was the behavior we agreed we wanted. IMHO, it would
be conditional on the INSERT ... VALUES (DEFAULT) capability being
provided. I'm not sure if that is there yet.
That is there now. Do you recall when this was discussed before?
I couldn't remember if there'd been any real discussion or not.
regards, tom lane
Peter Eisentraut wrote:
Rod Taylor writes:
I submitted a patch which would make Postgresql ANSI compliant in
regards to INSERT with a provided column list. As Tom states below,
this is not full compliance.CREATE TABLE tab(col1 text, col2 text);
INSERT INTO tab (col1, col2) VALUES ('val1'); -- bad by spec (enforced
by patch)
INSERT INTO tab (col1, col2) VALUES ('val1', 'val2'); -- goodINSERT INTO tab VALUES ('val1'); -- bad by spec (not enforced)
INSERT INTO tab VALUES ('val1', 'val2'); -- goodI recall that this was the behavior we agreed we wanted. IMHO, it would
be conditional on the INSERT ... VALUES (DEFAULT) capability being
provided. I'm not sure if that is there yet.
Yes, it is key to have DEFAULT working before we change this, and it is
in CVS now, committed a week or two ago.
Peter, are you saying you don't want to require all columns to be
specified when INSERT doesn't list the columns?
--
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 19026Bruce Momjian writes:
Peter, are you saying you don't want to require all columns to be
specified when INSERT doesn't list the columns?
Yes, that's what I'm saying. Too much breakage and annoyance potential in
that change.
--
Peter Eisentraut peter_e@gmx.net
IMHO, from a developers/users prospective I want to get atleast a NOTICE
when they mismatch, and really preferably I want the query to fail
because if I'm naming a column list then forget a value for it something
is wrong...
Bruce Momjian wrote:
Show quoted text
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
In general I'm suspicious of rejecting cases we used to accept for
no good reason other than that it's not in the spec. There is a LOT
of Postgres behavior that's not in the spec.TODO has:
o Disallow missing columns in INSERT ... VALUES, per ANSIWhere's the discussion that's the basis of that entry? I don't recall
any existing consensus on this (though maybe I forgot).I assume someone (Peter?) looked it up and reported that our current
behavior was incorrect and not compliant. I didn't do the research in
whether it was compliant.There are a fair number of things in the TODO list that you put there
because you liked 'em, but that doesn't mean everyone else agrees.
I certainly will not accept "once it's on the TODO list it cannot be
questioned"...I put it there because I didn't think there was any question. If I was
wrong, I can add a question mark to it.Do you want to argue we should continue allowing it? Also, what about
missing trailling columns in COPY?If we continue allowing missing INSERT columns, I am not sure we can
claim it is an extension or whether the standard requires the query to
fail.
Michael Loftis <mloftis@wgops.com> writes:
IMHO, from a developers/users prospective I want to get atleast a NOTICE
when they mismatch, and really preferably I want the query to fail
because if I'm naming a column list then forget a value for it something
is wrong...
So far I think everyone agrees that if an explicit column name list is
given, then it should fail if the column values don't match up. But
what do you think about the case with no column name list?
regards, tom lane
Tom Lane wrote:
So far I think everyone agrees that if an explicit column name list is
given, then it should fail if the column values don't match up. But
what do you think about the case with no column name list?
I'm on the fence in that situation. Though I'd lean towards a patch
thats a sort of compromise. IIF the 'remaining' columns (IE columns
unspecified) have some sort of default or auto-generated value (forgive
me I'm just getting back into workign with postgresql) like a SERIAL or
TIMESTAMP allow it, IFF any of them do not have a default value then
fail. This will make it 'do the right thing' -- it's not exactly what
the spec does, but it's close to the current behavior that several
others (including myself) see as beneficial in the case of interactive use.
As far as implementation of this sort of compromise, I'm not sure, but
it hsould be possible, assuming the planner knows/flags triggers on
column inserts and can make decisions and reject the query based on that
information (I don't think that information would be in the parser)
Show quoted text
regards, tom lane
On Mon, 15 Apr 2002, Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
I recall that this was the behavior we agreed we wanted. IMHO, it would
be conditional on the INSERT ... VALUES (DEFAULT) capability being
provided. I'm not sure if that is there yet.That is there now. Do you recall when this was discussed before?
I couldn't remember if there'd been any real discussion or not.
It has to be at least a year, Tom. I brought it up in hackers after
I got bit by it. I had a rather long insert statement and missed a
value in the middle somewhere which shifted everything by one. It
was agreed that it shouldn't happen but I don't recall what else was
decided.
Vince.
--
==========================================================================
Vince Vielhaber -- KA8CSH email: vev@michvhf.com http://www.pop4.net
56K Nationwide Dialup from $16.00/mo at Pop4 Networking
Online Campground Directory http://www.camping-usa.com
Online Giftshop Superstore http://www.cloudninegifts.com
==========================================================================
Bruce Momjian wrote:
Tom Lane wrote:
There are a fair number of things in the TODO list that you put there
because you liked 'em, but that doesn't mean everyone else agrees.
I certainly will not accept "once it's on the TODO list it cannot be
questioned"...I put it there because I didn't think there was any question.
Honetsly I don't understand what TODO means.
Can a developer solve the TODOs any way he likes ?
regards,
Hiroshi Inoue
Michael Loftis <mloftis@wgops.com> writes:
I'm on the fence in that situation. Though I'd lean towards a patch
thats a sort of compromise. IIF the 'remaining' columns (IE columns
unspecified) have some sort of default or auto-generated value (forgive
me I'm just getting back into workign with postgresql) like a SERIAL or
TIMESTAMP allow it, IFF any of them do not have a default value then
fail. This will make it 'do the right thing'
I think the apparent security is illusory. Given the presence of ALTER
TABLE ADD/DROP DEFAULT, the parser might easily accept a statement for
which an end-column default has been dropped by the time the statement
comes to be executed. (Think about an INSERT in a rule.)
Another reason for wanting it to work as proposed is ADD COLUMN.
Consider
CREATE TABLE foo (a, b, c);
create rule including INSERT INTO foo(a,b,c) VALUES(..., ..., ...);
ALTER TABLE foo ADD COLUMN d;
The rule still works, and will be interpreted as inserting the default
value (NULL if unspecified) into column d.
Now consider same scenario except I write the rule's INSERT without
an explicit column list. If we follow the letter of the spec, the
rule will now fail. How is this sensible or consistent behavior?
The case that should be laxer/easier is being treated *more* rigidly.
In any case, the above comparison shows that it's not very consistent
to require explicit defaults to be available for the omitted column(s).
INSERT with an explicit column list does not have any such requirement.
regards, tom lane
Tom Lane wrote:
Michael Loftis <mloftis@wgops.com> writes:
\<snip snip -- no I'm not ignoring anything just cutting down on quoting>
Now consider same scenario except I write the rule's INSERT without
an explicit column list. If we follow the letter of the spec, the
rule will now fail. How is this sensible or consistent behavior?
The case that should be laxer/easier is being treated *more* rigidly.In any case, the above comparison shows that it's not very consistent
to require explicit defaults to be available for the omitted column(s).
INSERT with an explicit column list does not have any such requirement.regards, tom lane
In the case of an implicit response it's to be treated as if all columns
had been specified (according to the spec). Which is why the spec says
that if you miss a column it's a bad INSERT unless you have specified
only a subset.
Unless I'm misreading (which I could be)
Either way, as I said I'm not wholly in favor of either way because both
solutions to me make equal sense, but keepign the ability to 'assume'
default values (whether it's NULL or derived) is the better one in this
case, if the race condition is indeed an issue.
BTW tom, I can't send mail directly to you --
black-holes.five-ten-sg.com (or something like that) lists most of
Speakeasy's netblocks (As well as the rest of the world heh) as
'dialups' and such. It's a liiittle over-paranoid to drop mail based on
their listings, but just wanted to letcha know you are losing some
legitimate mail :) No biggie though I just post ot the list anyway so
you get it still ;)
Michael
Hiroshi Inoue wrote:
Bruce Momjian wrote:
Tom Lane wrote:
There are a fair number of things in the TODO list that you put there
because you liked 'em, but that doesn't mean everyone else agrees.
I certainly will not accept "once it's on the TODO list it cannot be
questioned"...I put it there because I didn't think there was any question.
Honetsly I don't understand what TODO means.
Can a developer solve the TODOs any way he likes ?
I meant to say there was no question we wanted this item fixed, not that
there was no need for implementation discussions.
In summary, code changes have three stages:
o Do we want this feature?
o How do we want the feature to behave?
o How do we want the feature implemented?
Tom was complaining because the patch appeared without enough discussion
on these items. However, from my perspective, this is really trying to
micromanage the process. When people post patches, we aren't forced to
apply them. If people want to code things up and just send them in and
then are willing to get into a discussion to make sure all our questions
listed above are dealt with, that is fine with me.
To think that everyone is going to follow the process of going through
those three stages before submitting a patch isn't realistic. I think
we have to be a little flexible and work with these submitters so their
involvment in the project is both positive for them and positive for the
project. Doing discussion sometimes in a backward order is sometimes
required to accomplish this.
--
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 19026Vince Vielhaber wrote:
On Mon, 15 Apr 2002, Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
I recall that this was the behavior we agreed we wanted. IMHO, it would
be conditional on the INSERT ... VALUES (DEFAULT) capability being
provided. I'm not sure if that is there yet.That is there now. Do you recall when this was discussed before?
I couldn't remember if there'd been any real discussion or not.It has to be at least a year, Tom. I brought it up in hackers after
I got bit by it. I had a rather long insert statement and missed a
value in the middle somewhere which shifted everything by one. It
was agreed that it shouldn't happen but I don't recall what else was
decided.
Yes, I do remember Vince's comment, and I do believe that is the time it
was added.
--
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 19026Peter Eisentraut wrote:
Bruce Momjian writes:
Peter, are you saying you don't want to require all columns to be
specified when INSERT doesn't list the columns?Yes, that's what I'm saying. Too much breakage and annoyance potential in
that change.
OK, how about a NOTICE stating that the missing columns were filled in
with defaults?
--
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 19026Bruce Momjian writes:
OK, how about a NOTICE stating that the missing columns were filled in
with defaults?
Please not.
--
Peter Eisentraut peter_e@gmx.net
Bruce Momjian wrote:
Hiroshi Inoue wrote:
Bruce Momjian wrote:
Tom Lane wrote:
There are a fair number of things in the TODO list that you put there
because you liked 'em, but that doesn't mean everyone else agrees.
I certainly will not accept "once it's on the TODO list it cannot be
questioned"...I put it there because I didn't think there was any question.
Honetsly I don't understand what TODO means.
Can a developer solve the TODOs any way he likes ?I meant to say there was no question we wanted this item fixed, not that
there was no need for implementation discussions.In summary, code changes have three stages:
o Do we want this feature?
o How do we want the feature to behave?
o How do we want the feature implemented?Tom was complaining because the patch appeared without enough discussion
on these items. However, from my perspective, this is really trying to
micromanage the process. When people post patches, we aren't forced to
apply them.
But shouldn't someone check the patch ?
If the patch is small, making the patch seems
the simplest way for anyone but if the patch
is big, it seems painful for anyone to check
the patch. If no one checks the patch, would
we apply the patch blindly or reject it ?
regards,
Hiroshi Inoue
Hiroshi Inoue wrote:
I meant to say there was no question we wanted this item fixed, not that
there was no need for implementation discussions.In summary, code changes have three stages:
o Do we want this feature?
o How do we want the feature to behave?
o How do we want the feature implemented?Tom was complaining because the patch appeared without enough discussion
on these items. However, from my perspective, this is really trying to
micromanage the process. When people post patches, we aren't forced to
apply them.But shouldn't someone check the patch ?
If the patch is small, making the patch seems
the simplest way for anyone but if the patch
is big, it seems painful for anyone to check
the patch. If no one checks the patch, would
we apply the patch blindly or reject it ?
Of course, we would review any patch before application. I guess the
full path is:
o Do we want this feature?
o How do we want the feature to behave?
o How do we want the feature implemented?
o Submit patch
o Review patch
o Apply patch
I assume your point is that people shouldn't send in big patches
without going through the discussion first. Yes, that is ideal, but if
they don't, we just discuss it after the patch appears, and then decide
if we want to apply it or ask for modifications.
--
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 19026Bruce Momjian wrote:
Hiroshi Inoue wrote:
Of course, we would review any patch before application. I guess the
full path is:o Do we want this feature?
o How do we want the feature to behave?
o How do we want the feature implemented?
o Submit patch
o Review patch
o Apply patchI assume your point is that people shouldn't send in big patches
without going through the discussion first. Yes, that is ideal, but if
they don't, we just discuss it after the patch appears, and then decide
if we want to apply it or ask for modifications.
For example, I don't understand what pg_depend intends.
Is there any consensus on it ?
regards,
Hiroshi Inoue
...
OK, how about a NOTICE stating that the missing columns were filled in
with defaults?
Yuck. There is a short path from that to rejecting the insert, but
printing the entire insert statement which would have been acceptable in
the error message ;)
- Thomas
Hiroshi Inoue wrote:
Bruce Momjian wrote:
Hiroshi Inoue wrote:
Of course, we would review any patch before application. I guess the
full path is:o Do we want this feature?
o How do we want the feature to behave?
o How do we want the feature implemented?
o Submit patch
o Review patch
o Apply patchI assume your point is that people shouldn't send in big patches
without going through the discussion first. Yes, that is ideal, but if
they don't, we just discuss it after the patch appears, and then decide
if we want to apply it or ask for modifications.For example, I don't understand what pg_depend intends.
Is there any consensus on it ?
Uh, we know we want dependency checking to fix many problems; see TODO
dependency checking section. As far as how it is implemented, I haven't
studied it. I was going to wait for Tom to like it first and then give
it a review.
--
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 19026Bruce Momjian wrote:
Hiroshi Inoue wrote:
For example, I don't understand what pg_depend intends.
Is there any consensus on it ?Uh, we know we want dependency checking to fix many problems; see TODO
dependency checking section.
Yes I know it's a very siginificant mechanism. It would contibute
e.g. to the DROP COLUMN implementation considerably but no one
referred to pg_depend in the recent discussion about DROP COLUMN.
So I've thought pg_depend is for something else.
regards,
Hiroshi Inoue
Hiroshi Inoue wrote:
Bruce Momjian wrote:
Hiroshi Inoue wrote:
For example, I don't understand what pg_depend intends.
Is there any consensus on it ?Uh, we know we want dependency checking to fix many problems; see TODO
dependency checking section.Yes I know it's a very siginificant mechanism. It would contibute
e.g. to the DROP COLUMN implementation considerably but no one
referred to pg_depend in the recent discussion about DROP COLUMN.
So I've thought pg_depend is for something else.
Oh, clearly pg_depend will fix many of our problems, or make some
problems much easier to fix. I am excited to see it happening!
We had a pg_depend discussion about 9 months ago and hashed out a plan
but were just waiting for someone to do the work.
--
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 19026Bruce Momjian wrote:
Hiroshi Inoue wrote:
Bruce Momjian wrote:
Hiroshi Inoue wrote:
For example, I don't understand what pg_depend intends.
Is there any consensus on it ?Uh, we know we want dependency checking to fix many problems; see TODO
dependency checking section.Yes I know it's a very siginificant mechanism. It would contibute
e.g. to the DROP COLUMN implementation considerably but no one
referred to pg_depend in the recent discussion about DROP COLUMN.
So I've thought pg_depend is for something else.Oh, clearly pg_depend will fix many of our problems, or make some
problems much easier to fix. I am excited to see it happening!We had a pg_depend discussion about 9 months ago and hashed out a plan
but were just waiting for someone to do the work.
Probably I overlooked the conclusion then.
What was the conclusion of the discussion ?
regards,
Hiroshi Inoue
Hiroshi Inoue wrote:
Oh, clearly pg_depend will fix many of our problems, or make some
problems much easier to fix. I am excited to see it happening!We had a pg_depend discussion about 9 months ago and hashed out a plan
but were just waiting for someone to do the work.Probably I overlooked the conclusion then.
What was the conclusion of the discussion ?
Here is one of the threads:
--
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 19026Hiroshi Inoue writes:
Oh, clearly pg_depend will fix many of our problems, or make some
problems much easier to fix. I am excited to see it happening!We had a pg_depend discussion about 9 months ago and hashed out a plan
but were just waiting for someone to do the work.Probably I overlooked the conclusion then.
What was the conclusion of the discussion ?
Personally, I think there wasn't any. Personally part 2, showing some
code is always a good way for new contributors to show they're for real.
--
Peter Eisentraut peter_e@gmx.net
Bruce Momjian <pgman@candle.pha.pa.us> writes:
I was going to wait for Tom to like it first and then give
it a review.
My review is posted ;-)
regards, tom lane
Peter Eisentraut wrote:
Hiroshi Inoue writes:
Probably I overlooked the conclusion then.
What was the conclusion of the discussion ?Personally, I think there wasn't any. Personally part 2, showing some
code is always a good way for new contributors to show they're for real.
Certainly but once an accomplished fact is there, it requires
a great deal of efforts to put it back to the neutral position.
For example, all I've done this month are such kind of things.
regards,
Hiroshi Inoue
Hiroshi Inoue wrote:
Peter Eisentraut wrote:
Hiroshi Inoue writes:
Probably I overlooked the conclusion then.
What was the conclusion of the discussion ?Personally, I think there wasn't any. Personally part 2, showing some
code is always a good way for new contributors to show they're for real.Certainly but once an accomplished fact is there, it requires
a great deal of efforts to put it back to the neutral position.
For example, all I've done this month are such kind of things.
Yes, certain features have different implementation possibilities.
Sometimes this information is coverd in TODO.detail, but often it is
not.
I assume your point is that once a patch appears, it is harder to argue
to change the implementation than if they had asked for a discussion
first.
I guess the only thing I can say is that we shouldn't feel bad about
asking more questions and opening up implementation issues, even if a
patch has already been prepared. I should give a larger delay for
applying those patches so people can ask more questions and bring up
implementation issues.
--
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 19026I have added this bullet list to the developer's FAQ.
---------------------------------------------------------------------------
Bruce Momjian wrote:
Hiroshi Inoue wrote:
Bruce Momjian wrote:
Hiroshi Inoue wrote:
Of course, we would review any patch before application. I guess the
full path is:o Do we want this feature?
o How do we want the feature to behave?
o How do we want the feature implemented?
o Submit patch
o Review patch
o Apply patchI assume your point is that people shouldn't send in big patches
without going through the discussion first. Yes, that is ideal, but if
they don't, we just discuss it after the patch appears, and then decide
if we want to apply it or ask for modifications.For example, I don't understand what pg_depend intends.
Is there any consensus on it ?Uh, we know we want dependency checking to fix many problems; see TODO
dependency checking section. As far as how it is implemented, I haven't
studied it. I was going to wait for Tom to like it first and then give
it a review.-- 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---------------------------(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 19026Your 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:
Reports missing values as bad.
BAD: INSERT INTO tab (col1, col2) VALUES ('val1');
GOOD: INSERT INTO tab (col1, col2) VALUES ('val1', 'val2');Regress tests against DEFAULT and normal values as they're managed
slightly different.
[ 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 19026Patch applied. Thanks.
---------------------------------------------------------------------------
Rod Taylor wrote:
Reports missing values as bad.
BAD: INSERT INTO tab (col1, col2) VALUES ('val1');
GOOD: INSERT INTO tab (col1, col2) VALUES ('val1', 'val2');Regress tests against DEFAULT and normal values as they're managed
slightly different.
[ 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