Thread: TODO question
what is meant by the TODO-item "Disallow missing columns in INSERT ... VALUES, per ANSI" ? rgds Pavlo Baron
On Thu, 27 Dec 2001, Pavlo Baron wrote: > what is meant by the TODO-item "Disallow missing columns in INSERT ... > VALUES, per ANSI" ? INSERT INTO foo(a,b) values(1,2,3); Vince. -- ========================================================================== Vince Vielhaber -- KA8CSH email: vev@michvhf.com http://www.pop4.net 56K Nationwide Dialup from $16.00/mo atPop4 Networking Online Campground Directory http://www.camping-usa.com Online Giftshop Superstore http://www.cloudninegifts.com ==========================================================================
Vince Vielhaber: > INSERT INTO foo(a,b) values(1,2,3); > sorry, but I'm a little bit confused: what should or should not happen with this: if I execute this INSERT on a table having 2 cols, an error is generated: ERROR: INSERT has more expressions than target columns has it already been fixed? or what should be exactly disallowed? every time I execute such statement with any cols/vals combination having different cols/vals number it fails. Should it be allowed now? rgds Pavlo Baron
Vince Vielhaber <vev@michvhf.com> writes:
> On Thu, 27 Dec 2001, Pavlo Baron wrote:
>> what is meant by the TODO-item "Disallow missing columns in INSERT ...
>> VALUES, per ANSI" ?
> INSERT INTO foo(a,b) values(1,2,3);
I think you mean
regression=# INSERT INTO foo(a,b,c) values(1,2);
INSERT 172219 1
as the other behaves as expected:
regression=# INSERT INTO foo(a,b) values(1,2,3);
ERROR: INSERT has more expressions than target columns
I am not sure why this is on the TODO list, as opposed to having been
fixed out-of-hand; it sure doesn't look complex to the naked eye.
Perhaps there were some compatibility concerns, or some other issue.
Pavlo would be well advised to go search the archives for awhile to
understand the background of the TODO item.
regards, tom lane
On Thu, 27 Dec 2001, Pavlo Baron wrote: > Vince Vielhaber: > > INSERT INTO foo(a,b) values(1,2,3); > > > > sorry, but I'm a little bit confused: what should or should not happen with > this: > if I execute this INSERT on a table having 2 cols, an error is generated: > ERROR: INSERT has more expressions than target columns > > has it already been fixed? or what should be exactly disallowed? every time > I execute such statement with any cols/vals combination having different > cols/vals number it fails. Should it be allowed now? Sorry, I had it backwards. insert into foo(a,b,c) values(1,2); Vince. -- ========================================================================== Vince Vielhaber -- KA8CSH email: vev@michvhf.com http://www.pop4.net 56K Nationwide Dialup from $16.00/mo atPop4 Networking Online Campground Directory http://www.camping-usa.com Online Giftshop Superstore http://www.cloudninegifts.com ==========================================================================
> I am not sure why this is on the TODO list, as opposed to having been > fixed out-of-hand; it sure doesn't look complex to the naked eye. > Perhaps there were some compatibility concerns, or some other issue. > Pavlo would be well advised to go search the archives for awhile to > understand the background of the TODO item. It has the potential to break existing queries so I don't think we were worked up about fixing it quickly. Whoever does will have to weather the complaints, but I agree it should be done. :-) There are actually lots of nice easy items on the TODO list. Most people that they are easy for are focusing on other things. I found some time to pick off some easy ones for 7.2 and I hope more for 7.3. -- 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, Pennsylvania19026
> I am not sure why this is on the TODO list, as opposed to having been > fixed out-of-hand; it sure doesn't look complex to the naked eye. > Perhaps there were some compatibility concerns, or some other issue. > Pavlo would be well advised to go search the archives for awhile to > understand the background of the TODO item. Actually, the bigest problem with these small items is not the coding but making a patch everyone likes (at least for me). :-) -- 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, Pennsylvania19026
Bruce Momjian: > Actually, the bigest problem with these small items is not the coding > but making a patch everyone likes (at least for me). :-) ok, but what's the exact process of integrating patches made by new contributers (or smb. who tries to become a contributer, smb. like me ,-) )? I made (and remade, thank you Tom) a patch for "Allow INSERT INTO my_table VALUES (a, b, c, DEFAULT, x, y, z, ...)" from the TODO list being a small nice item. How do I know if it's been accepted? I explicitely wanted to begin with such small items, because: what looks small'n'easy for you guys would take some time for me to implement, and if I've got it after I jogged through the whole parser, maybe it would look small'n'easy for me, too. rgds Pavlo Baron
> ok, but what's the exact process of integrating patches made by new
> contributers (or smb. who tries to become a contributer, smb. like me ,-) )?
> I made (and remade, thank you Tom) a patch for "Allow INSERT INTO my_table
> VALUES (a, b, c, DEFAULT, x, y, z, ...)" from the TODO list being a small
> nice item. How do I know if it's been accepted? I explicitely wanted to
> begin with such small items, because: what looks small'n'easy for you guys
> would take some time for me to implement, and if I've got it after I jogged
> through the whole parser, maybe it would look small'n'easy for me, too.
You have seen the process for acceptance, with the exception of the "OK,
applied!" exchange that happens at the end. That is because we have not
quite released 7.2, and the source tree is essentially frozen for the
next month or so.
btw, it *may* be premature to assume that your patches are completely
accepted yet, since none of us are very focused on new features at this
moment.
If you had submitted patches three months ago, they would be in the 7.2
release ;)
- Thomas
Thomas Lockhart <lockhart@fourpalms.org> writes:
> btw, it *may* be premature to assume that your patches are completely
> accepted yet, since none of us are very focused on new features at this
> moment.
In fact the patch seemed quite incomplete to me; adding a new parsenode
type requires much more than just a struct declaration. But this isn't
the right time of the cycle to be reviewing new-feature patches.
BTW, patches should usually be sent to pgsql-patches not pgsql-hackers.
regards, tom lane
Tom Lane: > In fact the patch seemed quite incomplete to me; adding a new parsenode > type requires much more than just a struct declaration. btw, it's not correct, that just a new structure has been declared. I added the T_Default to the Type-Enum and it seems to me, my new parsenode type has been full-automatically integrated in the parser-workflow. In the gram.y, there is a new set of rules describing the DEFAULT value in the INSERT stmt - this is the place, where it's being identified and node-ed (using it's type), the transformation has got the new T_Default-case leaving this node "as is", and it's being transformed (replaced by the default value taken from the relation specified by the corresponding parsestate-field) later. > But this isn't > the right time of the cycle to be reviewing new-feature patches. ok, but I hope you've got a 3%-free--ear-capacity at least to answer some of my questions (having a very bad timing ,-) ). I don't ask offen and about every step, but sometimes it breaks through... > > BTW, patches should usually be sent to pgsql-patches not pgsql-hackers. ...where they will get dusty before the new release has been finished... ,) no problem, I'll wait a little with my patches but not with my questions ,) sorry if I increased your current stress level :-) rgds Pavlo Baron
Thomas Lockhart: > You have seen the process for acceptance, with the exception of the "OK, > applied!" exchange that happens at the end. That is because we have not > quite released 7.2, and the source tree is essentially frozen for the > next month or so. no problem, if I don't hear a *NO!*, it could be a *maybe yes...* > > btw, it *may* be premature to assume that your patches are completely > accepted yet, since none of us are very focused on new features at this > moment. I really don't assume the immediate acceptance of my patches, I'm just impregnating the codebase, so I ask some questions, and there are, IMHO, not very lot of them, and I hope it's not a big problem if I submit my patches sometimes with a complete description on what I did so nobody really needs (I know very very very precise about a Release completion phase %-( ) immediately to take a look at the code I wrote but possibly at the concept I used doing that. > > If you had submitted patches three months ago, they would be in the 7.2 > release ;) I think, my timing is perfect! Before you guys are got rid of the 7.2, I'll be ready to take care of some new features and learn it by doing and providing portions which have a chance to survive! I think, I've got a lot of ideas and usefull experiences, so may be some of them would be usefull for PostgreSQL, too. Excuse me my enthusiasm, but I don't think it's a problem rgds Pavlo Baron
> Tom Lane: > > In fact the patch seemed quite incomplete to me; adding a new parsenode > > type requires much more than just a struct declaration. > > btw, it's not correct, that just a new structure has been declared. I added > the T_Default to the Type-Enum and it seems to me, my new parsenode type has > been full-automatically integrated in the parser-workflow. In the gram.y, > there is a new set of rules describing the DEFAULT value in the INSERT > stmt - this is the place, where it's being identified and node-ed (using > it's type), the transformation has got the new T_Default-case leaving this > node "as is", and it's being transformed (replaced by the default value > taken from the relation specified by the corresponding parsestate-field) > later. > > > But this isn't > > the right time of the cycle to be reviewing new-feature patches. > > ok, but I hope you've got a 3%-free--ear-capacity at least to answer some of > my questions (having a very bad timing ,-) ). I don't ask offen and about > every step, but sometimes it breaks through... Sure, ask away. We will do our best. In fact, adding a new node is pretty tricky. There is a developer's FAQ item about it, number 7. I assume you read that already. I may be able to take you patch and add the needed node support stuff, and send the patch back to you so you can continue on it. -- 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, Pennsylvania19026
Bruce Momjian writes:
> Sure, ask away. We will do our best. In fact, adding a new node is
> pretty tricky. There is a developer's FAQ item about it, number 7. I
> assume you read that already.
hm...do you mean the current version of FAQ on the web? there are 2 seventh
items in the table of contents, both jumping to
----------------------------------------snip--------------------------------
-
7) I just added a field to a structure. What else should I do?
The structures passing around from the parser, rewrite, optimizer, and
executor require quite a bit of support. Most structures have support
routines in src/backend/nodes used to create, copy, read, and output those
structures. Make sure you add support for your new field to these files.
Find any other places the structure may need code for your new field. mkid
is helpful with this (see above).
----------------------------------------snap--------------------------------
-
Is that what you mean? It confuses me a bit, surely because I'm new here...
I didn't change any existing struct, and coudn't find any struct I which
could grow bacause of my new parsenode type. The type-enum contains a
corresponding range of values (I think, it was smth. starting with 700 upto
...) - I just added my new type here.
Or do I use a wrong copy of FAQ?
>
> I may be able to take you patch and add the needed node support stuff,
> and send the patch back to you so you can continue on it.
thanx. please, feel free - I attached my last patch to this email.
I see, that my patch provides the needed operation, but maybe it's not
enough and could cause some side effects. I'm looking forward to your
modifications (I hope it doesn't keep you from your current work, at least
not crucial). What I'm just interested in is, if my changes would be a
subset of your code or if what I did is an absolute b*-sh* ,)
rgds
Pavlo Baron
Index: src/backend/parser/parse_target.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/parse_target.c,v
retrieving revision 1.76
diff -c -r1.76 parse_target.c
*** src/backend/parser/parse_target.c 2001/11/05 17:46:26 1.76
--- src/backend/parser/parse_target.c 2001/12/28 23:13:43
***************
*** 60,65 ****
--- 60,77 ---- if (IsA(expr, Ident) &&((Ident *) expr)->isRel) elog(ERROR, "You can't use relation names alone in
thetarget list, try
relation.*.");
+ /* pavlo (pb@pbit.org: 2001-12-27: handle the DEFAULT in INSERT
INTO foo VALUES (..., DEFAULT, ...))*/
+ if (IsA(expr, Default))
+ {
+ if (pstate->p_target_relation->rd_att->attrs[(AttrNumber)
pstate->p_last_resno - 1]->atthasdef)
+ {
+ Const *con = (Const *)
stringToNode(pstate->p_target_relation->rd_att->constr->defval[(AttrNumber)
pstate->p_last_resno - 1].adbin);
+ expr = con;
+ }
+ else
+ elog(ERROR, "no default value for column \"%s\"
found\nDEFAULT cannot be inserted", colname);
+ }
+ type_id = exprType(expr); type_mod = exprTypmod(expr);
***************
*** 260,266 **** { tle->expr = CoerceTargetExpr(pstate, tle->expr, type_id, attrtype, attrtypmod);
! if (tle->expr == NULL) elog(ERROR, "column \"%s\" is of type '%s'" " but expression is of type '%s'"
"\n\tYou will need to rewrite or cast the expression",
--- 272,278 ---- { tle->expr = CoerceTargetExpr(pstate, tle->expr, type_id, attrtype, attrtypmod);
! if (tle->expr == NULL) elog(ERROR, "column \"%s\" is of type '%s'" " but
expressionis of type '%s'" "\n\tYou will need to rewrite or cast the expression",
***************
*** 299,305 **** int32 attrtypmod) { if (can_coerce_type(1, &type_id, &attrtype))
! expr = coerce_type(pstate, expr, type_id, attrtype, attrtypmod);
#ifndef DISABLE_STRING_HACKS
--- 311,317 ---- int32 attrtypmod) { if (can_coerce_type(1, &type_id, &attrtype))
! expr = coerce_type(pstate, expr, type_id, attrtype,
attrtypmod);
#ifndef DISABLE_STRING_HACKS
***************
*** 525,528 **** }
return strength;
! }
--- 537,540 ---- }
return strength;
! }
\ No newline at end of file
Index: src/backend/parser/parse_expr.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/parse_expr.c,v
retrieving revision 1.105
diff -c -r1.105 parse_expr.c
*** src/backend/parser/parse_expr.c 2001/11/12 20:05:24 1.105
--- src/backend/parser/parse_expr.c 2001/12/28 23:15:08
***************
*** 121,126 ****
--- 121,131 ---- result = (Node *) make_const(val); break; }
+ case T_Default: /* pavlo (pb@pbit.org): 2001-12-27:
transormation for the DEFAULT value for INSERT INTO foo VALUES (...,
DEFAULT, ...)*/
+ {
+ result = (Default *) expr;
+ break;
+ } case T_ParamNo: { ParamNo *pno = (ParamNo *) expr;
***************
*** 1066,1069 **** } else return typename->name;
! }
--- 1071,1074 ---- } else return typename->name;
! }
\ No newline at end of file
Index: src/backend/parser/gram.y
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.276
diff -c -r2.276 gram.y
*** src/backend/parser/gram.y 2001/12/09 04:39:39 2.276
--- src/backend/parser/gram.y 2001/12/28 23:15:39
***************
*** 195,201 **** opt_column_list, columnList, opt_name_list, sort_clause, sortby_list, index_params, index_list,
name_list, from_clause, from_list, opt_array_bounds,
! expr_list, attrs, target_list, update_target_list, def_list, opt_indirection, group_clause, TriggerFuncArgs,
select_limit,opt_select_limit
--- 195,201 ---- opt_column_list, columnList, opt_name_list, sort_clause, sortby_list, index_params, index_list,
name_list, from_clause, from_list, opt_array_bounds,
! expr_list, attrs, target_list, insert_target_list, update_target_list, def_list, opt_indirection, group_clause,
TriggerFuncArgs, select_limit, opt_select_limit
***************
*** 253,259 **** %type <node> table_ref %type <jexpr> joined_table %type <range> relation_expr
! %type <target> target_el, update_target_el %type <paramno> ParamNo
%type <typnam> Typename, SimpleTypename, ConstTypename
--- 253,259 ---- %type <node> table_ref %type <jexpr> joined_table %type <range> relation_expr
! %type <target> target_el, update_target_el, insert_target_el %type <paramno> ParamNo
%type <typnam> Typename, SimpleTypename, ConstTypename
***************
*** 3302,3308 **** } ;
! insert_rest: VALUES '(' target_list ')' { $$ = makeNode(InsertStmt); $$->cols = NIL;
--- 3302,3308 ---- } ;
! insert_rest: VALUES '(' insert_target_list ')' { $$ = makeNode(InsertStmt); $$->cols = NIL;
***************
*** 5482,5488 **** *
****************************************************************************
*/
! /* Target lists as found in SELECT ... and INSERT VALUES ( ... ) */
target_list: target_list ',' target_el { $$ = lappend($1, $3); }
--- 5482,5488 ---- *
****************************************************************************
*/
! /* Target lists as found in SELECT ... */
target_list: target_list ',' target_el { $$ = lappend($1, $3); }
***************
*** 5490,5496 ****
--- 5490,5522 ---- { $$ = makeList1($1); } ;
+ /* Target lists as found in INSERT VALUES ( ... ) */
+
+ /* pavlo (pb@pbit.org): 2001-12-27: parse node based handling for the
DEFAULT value added;
+ now it's possible to INSERT INTO ... VALUES (..., FEFAULT, ...)!
+ */
+ insert_target_list: insert_target_list ',' insert_target_el
+ { $$ = lappend($1, $3); }
+ | insert_target_el
+ { $$ = makeList1($1); }
+ | insert_target_list ',' target_el
+ { $$ = lappend($1, $3); }
+ | target_el
+ { $$ = makeList1($1); }
+ ;
+
+ insert_target_el: DEFAULT
+ {
+ Default *n = makeNode(Default);
+ $$ = makeNode(ResTarget);
+ $$->name = NULL;
+ $$->indirection = NULL;
+ $$->val = (Node *)n;
+ }
+ ;
+ /* AS is not optional because shift/red conflict with unary ops */
+ target_el: a_expr AS ColLabel { $$ = makeNode(ResTarget);
***************
*** 6380,6383 **** strcpy(newval+1, oldval); v->val.str = newval; }
! }
--- 6406,6409 ---- strcpy(newval+1, oldval); v->val.str = newval; }
! }
\ No newline at end of file
Index: src/include/nodes/parsenodes.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/nodes/parsenodes.h,v
retrieving revision 1.151
diff -c -r1.151 parsenodes.h
*** src/include/nodes/parsenodes.h 2001/11/05 17:46:34 1.151
--- src/include/nodes/parsenodes.h 2001/12/28 23:16:30
***************
*** 1005,1010 ****
--- 1005,1019 ---- } A_Const;
/*
+ * pavlo (pb@pbit.org): 2001.12.27:
+ * Default - the DEFAULT constant expression used in the target list of
INSERT INTO ... VALUES (..., DEFAULT, ...)
+ */
+ typedef struct Default
+ {
+ NodeTag type;
+ } Default;
+
+ /* * TypeCast - a CAST expression * * NOTE: for mostly historical reasons, A_Const and ParamNo parsenodes
contain
***************
*** 1355,1358 **** */ typedef SortClause GroupClause;
! #endif /* PARSENODES_H */
--- 1364,1367 ---- */ typedef SortClause GroupClause;
! #endif /* PARSENODES_H */
\ No newline at end of file
BTW, inserting the actual default expression in the parser is no good.
We just finished getting rid of that behavior last month:
2001-11-02 15:23 tgl
* src/backend/: catalog/heap.c, optimizer/prep/preptlist.c,parser/analyze.c: Add default expressions to INSERTs
duringplanning,not during parse analysis. This keeps stored rules fromprematurely absorbing default information, which
isnecessary forALTER TABLE SET DEFAULT to work unsurprisingly with rules. Seepgsql-bugs discussion 24-Oct-01.
The way I would tackle this is to do the work in
transformInsertStmt: if you find a Default node in the input targetlist,
you can simply omit the corresponding entry of the column list. In
other words, transform
INSERT INTO foo (col1, col2, col3) VALUES (11, DEFAULT, 13);
into
INSERT INTO foo (col1, col3) VALUES (11, 13);
Then you can rely on the planner to insert the correct defaults at planning
time.
My inclination would be to twiddle the order of operations so that the
Default node is spotted and intercepted before being fed to
transformExpr. This would probably mean doing some surgery on
transformTargetList. The advantage of doing it that way is that
transformExpr and subsequent processing need never see Default nodes
and can just error out if they do. The way you have it, Default needs
to pass through transformExpr, which raises a bunch of questions in my
mind about what parts of the system will need to accept it. There's
an established distinction between "pre-parse-analysis" node types
(eg, Attr) and "post-parse-analysis" node types (eg, Var), and we
understand which places need to support which node types as long as
that's the distinction. If you allow Default to pass through
transformExpr then you're to some extent allowing it to be a
post-parse-analysis node type, which doesn't strike me as a good idea.
regards, tom lane
> Bruce Momjian writes: > > Sure, ask away. We will do our best. In fact, adding a new node is > > pretty tricky. There is a developer's FAQ item about it, number 7. I > > assume you read that already. > > hm...do you mean the current version of FAQ on the web? there are 2 seventh > items in the table of contents, both jumping to > ----------------------------------------snip-------------------------------- > - > 7) I just added a field to a structure. What else should I do? > The structures passing around from the parser, rewrite, optimizer, and > executor require quite a bit of support. Most structures have support > routines in src/backend/nodes used to create, copy, read, and output those > structures. Make sure you add support for your new field to these files. > Find any other places the structure may need code for your new field. mkid > is helpful with this (see above). > > ----------------------------------------snap-------------------------------- > - > > Is that what you mean? It confuses me a bit, surely because I'm new here... > I didn't change any existing struct, and coudn't find any struct I which > could grow bacause of my new parsenode type. The type-enum contains a > corresponding range of values (I think, it was smth. starting with 700 upto > ...) - I just added my new type here. > Or do I use a wrong copy of FAQ? Yes, that is the one. The steps for adding an entry to an existing structure is similar to add an entirely new one. > > I may be able to take you patch and add the needed node support stuff, > > and send the patch back to you so you can continue on it. > > thanx. please, feel free - I attached my last patch to this email. > I see, that my patch provides the needed operation, but maybe it's not > enough and could cause some side effects. I'm looking forward to your > modifications (I hope it doesn't keep you from your current work, at least > not crucial). What I'm just interested in is, if my changes would be a > subset of your code or if what I did is an absolute b*-sh* ,) I think it would be a subset. I haven't looked at it closely yet, though. -- 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, Pennsylvania19026
> Bruce Momjian writes: > > Sure, ask away. We will do our best. In fact, adding a new node is > > pretty tricky. There is a developer's FAQ item about it, number 7. I > > assume you read that already. > > hm...do you mean the current version of FAQ on the web? there are 2 seventh > items in the table of contents, both jumping to I have updated the developer's FAQ to remove the duplicate entries. I have wanted to split the FAQ into two sections anyway, and this was a good time to do it. -- 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, Pennsylvania19026
> > Bruce Momjian writes:
> > > Sure, ask away. We will do our best. In fact, adding a new node is
> > > pretty tricky. There is a developer's FAQ item about it, number 7. I
> > > assume you read that already.
> >
> > hm...do you mean the current version of FAQ on the web? there are 2
seventh
> > items in the table of contents, both jumping to
>
> ----------------------------------------snip------------------------------
--
> > -
> > 7) I just added a field to a structure. What else should I do?
> > The structures passing around from the parser, rewrite, optimizer, and
> > executor require quite a bit of support. Most structures have support
> > routines in src/backend/nodes used to create, copy, read, and output
those
> > structures. Make sure you add support for your new field to these files.
> > Find any other places the structure may need code for your new field.
mkid
> > is helpful with this (see above).
> >
>
> ----------------------------------------snap------------------------------
--
> > -
> >
> > Is that what you mean? It confuses me a bit, surely because I'm new
here...
> > I didn't change any existing struct, and coudn't find any struct I which
> > could grow bacause of my new parsenode type. The type-enum contains a
> > corresponding range of values (I think, it was smth. starting with 700
upto
> > ...) - I just added my new type here.
> > Or do I use a wrong copy of FAQ?
>
> Yes, that is the one. The steps for adding an entry to an existing
> structure is similar to add an entirely new one.
What structure could you suggest me to pick out and to follow in the code
which would be handled similarily to my "Default"? This one is a one-way
use'n'drop struct and wouldn't appear anywhere else.
>
> > > I may be able to take you patch and add the needed node support stuff,
> > > and send the patch back to you so you can continue on it.
> >
> > thanx. please, feel free - I attached my last patch to this email.
> > I see, that my patch provides the needed operation, but maybe it's not
> > enough and could cause some side effects. I'm looking forward to your
> > modifications (I hope it doesn't keep you from your current work, at
least
> > not crucial). What I'm just interested in is, if my changes would be a
> > subset of your code or if what I did is an absolute b*-sh* ,)
>
> I think it would be a subset. I haven't looked at it closely yet,
> though.
>
oops, a new patch attached: the last one wouldn't compile because of missing
T_Default value I just forgot to diff-in (or out) - sorry
rgds
Pavlo Baron
Index: src/backend/parser/parse_target.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/parse_target.c,v
retrieving revision 1.76
diff -c -r1.76 parse_target.c
*** src/backend/parser/parse_target.c 2001/11/05 17:46:26 1.76
--- src/backend/parser/parse_target.c 2001/12/28 23:13:43
***************
*** 60,65 ****
--- 60,77 ---- if (IsA(expr, Ident) &&((Ident *) expr)->isRel) elog(ERROR, "You can't use relation names alone in
thetarget list, try
relation.*.");
+ /* pavlo (pb@pbit.org: 2001-12-27: handle the DEFAULT in INSERT
INTO foo VALUES (..., DEFAULT, ...))*/
+ if (IsA(expr, Default))
+ {
+ if (pstate->p_target_relation->rd_att->attrs[(AttrNumber)
pstate->p_last_resno - 1]->atthasdef)
+ {
+ Const *con = (Const *)
stringToNode(pstate->p_target_relation->rd_att->constr->defval[(AttrNumber)
pstate->p_last_resno - 1].adbin);
+ expr = con;
+ }
+ else
+ elog(ERROR, "no default value for column \"%s\"
found\nDEFAULT cannot be inserted", colname);
+ }
+ type_id = exprType(expr); type_mod = exprTypmod(expr);
***************
*** 260,266 **** { tle->expr = CoerceTargetExpr(pstate, tle->expr, type_id, attrtype, attrtypmod);
! if (tle->expr == NULL) elog(ERROR, "column \"%s\" is of type '%s'" " but expression is of type '%s'"
"\n\tYou will need to rewrite or cast the expression",
--- 272,278 ---- { tle->expr = CoerceTargetExpr(pstate, tle->expr, type_id, attrtype, attrtypmod);
! if (tle->expr == NULL) elog(ERROR, "column \"%s\" is of type '%s'" " but
expressionis of type '%s'" "\n\tYou will need to rewrite or cast the expression",
***************
*** 299,305 **** int32 attrtypmod) { if (can_coerce_type(1, &type_id, &attrtype))
! expr = coerce_type(pstate, expr, type_id, attrtype, attrtypmod);
#ifndef DISABLE_STRING_HACKS
--- 311,317 ---- int32 attrtypmod) { if (can_coerce_type(1, &type_id, &attrtype))
! expr = coerce_type(pstate, expr, type_id, attrtype,
attrtypmod);
#ifndef DISABLE_STRING_HACKS
***************
*** 525,528 **** }
return strength;
! }
--- 537,540 ---- }
return strength;
! }
\ No newline at end of file
Index: src/backend/parser/parse_expr.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/parse_expr.c,v
retrieving revision 1.105
diff -c -r1.105 parse_expr.c
*** src/backend/parser/parse_expr.c 2001/11/12 20:05:24 1.105
--- src/backend/parser/parse_expr.c 2001/12/28 23:15:08
***************
*** 121,126 ****
--- 121,131 ---- result = (Node *) make_const(val); break; }
+ case T_Default: /* pavlo (pb@pbit.org): 2001-12-27:
transormation for the DEFAULT value for INSERT INTO foo VALUES (...,
DEFAULT, ...)*/
+ {
+ result = (Default *) expr;
+ break;
+ } case T_ParamNo: { ParamNo *pno = (ParamNo *) expr;
***************
*** 1066,1069 **** } else return typename->name;
! }
--- 1071,1074 ---- } else return typename->name;
! }
\ No newline at end of file
Index: src/backend/parser/gram.y
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.276
diff -c -r2.276 gram.y
*** src/backend/parser/gram.y 2001/12/09 04:39:39 2.276
--- src/backend/parser/gram.y 2001/12/28 23:15:39
***************
*** 195,201 **** opt_column_list, columnList, opt_name_list, sort_clause, sortby_list, index_params, index_list,
name_list, from_clause, from_list, opt_array_bounds,
! expr_list, attrs, target_list, update_target_list, def_list, opt_indirection, group_clause, TriggerFuncArgs,
select_limit,opt_select_limit
--- 195,201 ---- opt_column_list, columnList, opt_name_list, sort_clause, sortby_list, index_params, index_list,
name_list, from_clause, from_list, opt_array_bounds,
! expr_list, attrs, target_list, insert_target_list, update_target_list, def_list, opt_indirection, group_clause,
TriggerFuncArgs, select_limit, opt_select_limit
***************
*** 253,259 **** %type <node> table_ref %type <jexpr> joined_table %type <range> relation_expr
! %type <target> target_el, update_target_el %type <paramno> ParamNo
%type <typnam> Typename, SimpleTypename, ConstTypename
--- 253,259 ---- %type <node> table_ref %type <jexpr> joined_table %type <range> relation_expr
! %type <target> target_el, update_target_el, insert_target_el %type <paramno> ParamNo
%type <typnam> Typename, SimpleTypename, ConstTypename
***************
*** 3302,3308 **** } ;
! insert_rest: VALUES '(' target_list ')' { $$ = makeNode(InsertStmt); $$->cols = NIL;
--- 3302,3308 ---- } ;
! insert_rest: VALUES '(' insert_target_list ')' { $$ = makeNode(InsertStmt); $$->cols = NIL;
***************
*** 5482,5488 **** *
****************************************************************************
*/
! /* Target lists as found in SELECT ... and INSERT VALUES ( ... ) */
target_list: target_list ',' target_el { $$ = lappend($1, $3); }
--- 5482,5488 ---- *
****************************************************************************
*/
! /* Target lists as found in SELECT ... */
target_list: target_list ',' target_el { $$ = lappend($1, $3); }
***************
*** 5490,5496 ****
--- 5490,5522 ---- { $$ = makeList1($1); } ;
+ /* Target lists as found in INSERT VALUES ( ... ) */
+
+ /* pavlo (pb@pbit.org): 2001-12-27: parse node based handling for the
DEFAULT value added;
+ now it's possible to INSERT INTO ... VALUES (..., FEFAULT, ...)!
+ */
+ insert_target_list: insert_target_list ',' insert_target_el
+ { $$ = lappend($1, $3); }
+ | insert_target_el
+ { $$ = makeList1($1); }
+ | insert_target_list ',' target_el
+ { $$ = lappend($1, $3); }
+ | target_el
+ { $$ = makeList1($1); }
+ ;
+
+ insert_target_el: DEFAULT
+ {
+ Default *n = makeNode(Default);
+ $$ = makeNode(ResTarget);
+ $$->name = NULL;
+ $$->indirection = NULL;
+ $$->val = (Node *)n;
+ }
+ ;
+ /* AS is not optional because shift/red conflict with unary ops */
+ target_el: a_expr AS ColLabel { $$ = makeNode(ResTarget);
***************
*** 6380,6383 **** strcpy(newval+1, oldval); v->val.str = newval; }
! }
--- 6406,6409 ---- strcpy(newval+1, oldval); v->val.str = newval; }
! }
\ No newline at end of file
Index: src/include/nodes/parsenodes.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/nodes/parsenodes.h,v
retrieving revision 1.151
diff -c -r1.151 parsenodes.h
*** src/include/nodes/parsenodes.h 2001/11/05 17:46:34 1.151
--- src/include/nodes/parsenodes.h 2001/12/28 23:16:30
***************
*** 1005,1010 ****
--- 1005,1019 ---- } A_Const;
/*
+ * pavlo (pb@pbit.org): 2001.12.27:
+ * Default - the DEFAULT constant expression used in the target list of
INSERT INTO ... VALUES (..., DEFAULT, ...)
+ */
+ typedef struct Default
+ {
+ NodeTag type;
+ } Default;
+
+ /* * TypeCast - a CAST expression * * NOTE: for mostly historical reasons, A_Const and ParamNo parsenodes
contain
***************
*** 1355,1358 **** */ typedef SortClause GroupClause;
! #endif /* PARSENODES_H */
--- 1364,1367 ---- */ typedef SortClause GroupClause;
! #endif /* PARSENODES_H */
\ No newline at end of file
Index: src/include/nodes/nodes.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/nodes/nodes.h,v
retrieving revision 1.96
diff -c -r1.96 nodes.h
*** src/include/nodes/nodes.h 2001/11/05 17:46:34 1.96
--- src/include/nodes/nodes.h 2001/12/29 09:23:37
***************
*** 222,227 ****
--- 222,228 ---- T_CaseWhen, T_FkConstraint, T_PrivGrantee,
+ T_Default, /* pavlo (pb@pbit.org) : 2001.12.27: DEFAULT element of
the INSERT INTO target list*/
/* * TAGS FOR FUNCTION-CALL CONTEXT AND RESULTINFO NODES (see fmgr.h)
***************
*** 365,368 **** (jointype) == JOIN_FULL || \ (jointype) == JOIN_RIGHT)
! #endif /* NODES_H */
--- 366,369 ---- (jointype) == JOIN_FULL || \ (jointype) == JOIN_RIGHT)
! #endif /* NODES_H */
\ No newline at end of file
Tom Lane writes: > BTW, inserting the actual default expression in the parser is no good. > We just finished getting rid of that behavior last month: > > 2001-11-02 15:23 tgl > > * src/backend/: catalog/heap.c, optimizer/prep/preptlist.c, > parser/analyze.c: Add default expressions to INSERTs during > planning, not during parse analysis. This keeps stored rules from > prematurely absorbing default information, which is necessary for > ALTER TABLE SET DEFAULT to work unsurprisingly with rules. See > pgsql-bugs discussion 24-Oct-01. > > The way I would tackle this is to do the work in > transformInsertStmt: if you find a Default node in the input targetlist, > you can simply omit the corresponding entry of the column list. In > other words, transform > > INSERT INTO foo (col1, col2, col3) VALUES (11, DEFAULT, 13); > > into > > INSERT INTO foo (col1, col3) VALUES (11, 13); > > Then you can rely on the planner to insert the correct defaults at planning > time. this is an information of gold! I was really unhappy with my hacks looked like an alien element to me (and surely being those, too). When I look on the pointer chain I used to get the default value... And I'm not sure if my solution would handle every kind of default expr. correctly. > > My inclination would be to twiddle the order of operations so that the > Default node is spotted and intercepted before being fed to > transformExpr. This would probably mean doing some surgery on > transformTargetList. The advantage of doing it that way is that > transformExpr and subsequent processing need never see Default nodes > and can just error out if they do. The way you have it, Default needs > to pass through transformExpr, which raises a bunch of questions in my > mind about what parts of the system will need to accept it. There's > an established distinction between "pre-parse-analysis" node types > (eg, Attr) and "post-parse-analysis" node types (eg, Var), and we > understand which places need to support which node types as long as > that's the distinction. If you allow Default to pass through > transformExpr then you're to some extent allowing it to be a > post-parse-analysis node type, which doesn't strike me as a good idea. I see. Can I say in simple words, that everything that can be parsed without any knowledge about the database condition has to be pre-parse-analized and the rest has to be post-parse-analized, then? Or, things needed to parse correctly are prepared by the pre-parser-analysis? Could you point me to a summary (if any) where I would find a rough desc. on such distinction? I'll re-code to match the basic concepts you've explained here. BTW, what about constructs like: INSERT INTO foo VALUES (1,,2); wouldn't it make the whole thing a bit simpler? or: INSERT INTO foo(c1, c2, c3) VALUES (,1,); I find, this short form is a bit more finger-freiendly and I think, it could be implemented without dropping the standard one? This would better match your idea about "to omit" the DEFAULT-ed column, since one doesn't care of what value has to be put in if you write smth. like "...VALUES (,,,,,4,,,1,)". Or, there could be an expression defined if not even DEFAULT has been specified, an other expression than the DEFAULT expr., eg, (I just spin around :-) ) it could be allowed to define smth. like OMITED appearing as a place holder for a post-parse replacement where eg column references could be used, eg: db=# CREATE TABLE foo (c1 int2, c2 int2, c3 int2 OMITED c1*20, c4 int2); db=# INSERT INTO foo values (2, 3,, 4); db=# SELECT c3 FROM foo; c3 ---- 40 (1 rows) rgds Pavlo Baron
"Pavlo Baron" <pb@pbit.org> writes:
> I see. Can I say in simple words, that everything that can be parsed without
> any knowledge about the database condition has to be pre-parse-analized and
> the rest has to be post-parse-analized, then?
Exactly. The grammar phase has to operate without examining the
database, because it needs to be able to run even in transaction-aborted
state (so we can recognize COMMIT and ROLLBACK commands). Parse
analysis does the lookups and so forth to determine exactly what the
raw syntax tree really means.
> summary (if any) where I would find a rough desc. on such distinction?
There's no substitute for reading the code ... the point above is
mentioned in the comments at the head of gram.y, for example.
> BTW, what about constructs like:
> INSERT INTO foo VALUES (1,,2);
> wouldn't it make the whole thing a bit simpler?
Maybe, but it's not SQL92. Looks kind of error-prone to me anyway.
regards, tom lane
Tom Lane writes: > My inclination would be to twiddle the order of operations so that the > Default node is spotted and intercepted before being fed to > transformExpr. This would probably mean doing some surgery on > transformTargetList. Why transformTargetList? The comment above this function says that it's indiffernet if there is a SELECT or INSERT or an other stmt. being parsed - the expressions are just to be transformed? Woudn't it break this indifference if I add a new branch handling with the Default node to this function? Or is it uncritical if I simply add it without any distinction of what stmt. is being parsed? Could it become a problem later if the Default node is reused by some other stmt.? rgds Pavlo Baron
"Pavlo Baron" <pb@pbit.org> writes:
> Tom Lane writes:
>> My inclination would be to twiddle the order of operations so that the
>> Default node is spotted and intercepted before being fed to
>> transformExpr. This would probably mean doing some surgery on
>> transformTargetList.
> Why transformTargetList? The comment above this function says that it's
> indiffernet if there is a SELECT or INSERT or an other stmt. being parsed -
> the expressions are just to be transformed? Woudn't it break this
> indifference if I add a new branch handling with the Default node to this
> function?
Well, you could either assume that a DEFAULT node must be okay if
present (relying on the grammar to have allowed it only for INSERT),
or you could add a parameter to transformTargetList saying whether it's
dealing with an INSERT list or not. If not, it could error out if it
sees a DEFAULT node. This might be better than rejecting DEFAULT in
the grammar, since you could give a more specific error message than
just "parse error"; and you wouldn't need two separate targetlist
constructs in the grammar.
You also need to think about what to return in the output targetlist if
you see a DEFAULT node. You can't build a correct, valid Resdom since
you have no info about datatype. I think I'd be inclined to return the
DEFAULT node in place of a TargetEntry, and then transformInsertStmt
would be changed to discard list items that weren't TargetEntrys. A
little ugly, but probably less ugly than building a not-really-valid
TargetEntry structure.
regards, tom lane
Tom Lane writes: > Well, you could either assume that a DEFAULT node must be okay if > present (relying on the grammar to have allowed it only for INSERT), > or you could add a parameter to transformTargetList saying whether it's > dealing with an INSERT list or not. If not, it could error out if it > sees a DEFAULT node. This might be better than rejecting DEFAULT in > the grammar, since you could give a more specific error message than > just "parse error"; and you wouldn't need two separate targetlist > constructs in the grammar. Oh, I really don't think I should modify the parameter list of such *present* function. The comment on it looked very *proud* to me, proud, because it's been highlighted that it doesn't play a role which stmt. is comming in, so everything breaking this *pride* would be an evil hack, wouldn't it? Futhermore Bruce says, such small'n'easy hack must match everybody's expectations before it gets a chance to be accepted ,) Seriously: I know, it's not the finest method to change the grammar, though it looks very compact to me. Questions: I followed your recomendation to create a new parse-node for DEFAULT. Is this step ok? I'll additionally take a look on what to do to *fully* integrate a new node - Bruce said, maybe he would be able to help a little. Further, if the previous step is correct then, do I understand you right: you are not enjoyed of the rejecting DEFAULT in cases other than INSERT, but should DEFAULT be recognized anyway, then translated to my new parse-node "Default" (grammar ends here) and then handled in transformTargetList? Or did you mean, that the grammer hasn't to be changed at all and DEFAULT is to be handled out later from the string constant (unknown type)? I'll take a look on the call chain leading to the final call of transormTargetList - maybe I'll find a way to avoid a modification of it's parameter list. Is there a chance to handle Default somewhere *above* transformTargetList without midifying it's parameters? If no, then can I really feel free to modify this parameter list and adapt it everywhere I find a call to this function? *But*: What would you say, if I add a mitm-function called instead of transformTargetList in the case of INSERT, doing a thing and finally calling transformTargetList? Smth. like transformInsertTargetList? Wouldn't it be more elegant than modifying the parameter list of one of the basic transformation functions? Then, I could add a case to transformTargetList where I would generate an error on every other Default-parse-node comming in, denying it and explaining it's cause? Would it be ok? rgds Pavlo Baron
> oops, a new patch attached: the last one wouldn't compile because of missing > T_Default value I just forgot to diff-in (or out) - sorry > > rgds > Pavlo Baron Never mind. I see it now. We still need to discuss it. -- 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, Pennsylvania19026
Bruce writes: > > oops, a new patch attached: the last one wouldn't compile because of missing > > T_Default value I just forgot to diff-in (or out) - sorry > > > > rgds > > Pavlo Baron > > Never mind. I see it now. We still need to discuss it. > Is it now the right time to discuss new features? I just had stopped to bomb you guys with my questions since you were stressed by the new release completion. There were some communication between Tom Lane and me about where to implement that INSERT...DEFAULT fix - now I really don't know where to begin. Do you possibly have an idea on how that thing can start rolling now? rgds Pavlo Baron
Double-oops. There is a later one under a different email subject
called TODO questino that is a context diff. I just need to know if
this is the newest version and if anyone doesn't like it.
This adds DEFAULT functionality to the INSERT values list.
> oops, a new patch attached: the last one wouldn't compile because of missing
> T_Default value I just forgot to diff-in (or out) - sorry
>
> rgds
> Pavlo Baron
>
>
> Index: src/backend/parser/parse_target.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/parser/parse_target.c,v
> retrieving revision 1.76
> diff -c -r1.76 parse_target.c
> *** src/backend/parser/parse_target.c 2001/11/05 17:46:26 1.76
> --- src/backend/parser/parse_target.c 2001/12/28 23:13:43
> ***************
> *** 60,65 ****
> --- 60,77 ----
> if (IsA(expr, Ident) &&((Ident *) expr)->isRel)
> elog(ERROR, "You can't use relation names alone in the target list, try
> relation.*.");
>
> + /* pavlo (pb@pbit.org: 2001-12-27: handle the DEFAULT in INSERT
> INTO foo VALUES (..., DEFAULT, ...))*/
> + if (IsA(expr, Default))
> + {
> + if (pstate->p_target_relation->rd_att->attrs[(AttrNumber)
> pstate->p_last_resno - 1]->atthasdef)
> + {
> + Const *con = (Const *)
> stringToNode(pstate->p_target_relation->rd_att->constr->defval[(AttrNumber)
> pstate->p_last_resno - 1].adbin);
> + expr = con;
> + }
> + else
> + elog(ERROR, "no default value for column \"%s\"
> found\nDEFAULT cannot be inserted", colname);
> + }
> +
> type_id = exprType(expr);
> type_mod = exprTypmod(expr);
>
> ***************
> *** 260,266 ****
> {
> tle->expr = CoerceTargetExpr(pstate, tle->expr, type_id,
> attrtype, attrtypmod);
> ! if (tle->expr == NULL)
> elog(ERROR, "column \"%s\" is of type '%s'"
> " but expression is of type '%s'"
> "\n\tYou will need to rewrite or cast the expression",
> --- 272,278 ----
> {
> tle->expr = CoerceTargetExpr(pstate, tle->expr, type_id,
> attrtype, attrtypmod);
> ! if (tle->expr == NULL)
> elog(ERROR, "column \"%s\" is of type '%s'"
> " but expression is of type '%s'"
> "\n\tYou will need to rewrite or cast the expression",
> ***************
> *** 299,305 ****
> int32 attrtypmod)
> {
> if (can_coerce_type(1, &type_id, &attrtype))
> ! expr = coerce_type(pstate, expr, type_id, attrtype, attrtypmod);
>
> #ifndef DISABLE_STRING_HACKS
>
> --- 311,317 ----
> int32 attrtypmod)
> {
> if (can_coerce_type(1, &type_id, &attrtype))
> ! expr = coerce_type(pstate, expr, type_id, attrtype,
> attrtypmod);
>
> #ifndef DISABLE_STRING_HACKS
>
> ***************
> *** 525,528 ****
> }
>
> return strength;
> ! }
> --- 537,540 ----
> }
>
> return strength;
> ! }
> \ No newline at end of file
> Index: src/backend/parser/parse_expr.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/parser/parse_expr.c,v
> retrieving revision 1.105
> diff -c -r1.105 parse_expr.c
> *** src/backend/parser/parse_expr.c 2001/11/12 20:05:24 1.105
> --- src/backend/parser/parse_expr.c 2001/12/28 23:15:08
> ***************
> *** 121,126 ****
> --- 121,131 ----
> result = (Node *) make_const(val);
> break;
> }
> + case T_Default: /* pavlo (pb@pbit.org): 2001-12-27:
> transormation for the DEFAULT value for INSERT INTO foo VALUES (...,
> DEFAULT, ...)*/
> + {
> + result = (Default *) expr;
> + break;
> + }
> case T_ParamNo:
> {
> ParamNo *pno = (ParamNo *) expr;
> ***************
> *** 1066,1069 ****
> }
> else
> return typename->name;
> ! }
> --- 1071,1074 ----
> }
> else
> return typename->name;
> ! }
> \ No newline at end of file
> Index: src/backend/parser/gram.y
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/parser/gram.y,v
> retrieving revision 2.276
> diff -c -r2.276 gram.y
> *** src/backend/parser/gram.y 2001/12/09 04:39:39 2.276
> --- src/backend/parser/gram.y 2001/12/28 23:15:39
> ***************
> *** 195,201 ****
> opt_column_list, columnList, opt_name_list,
> sort_clause, sortby_list, index_params, index_list, name_list,
> from_clause, from_list, opt_array_bounds,
> ! expr_list, attrs, target_list, update_target_list,
> def_list, opt_indirection, group_clause, TriggerFuncArgs,
> select_limit, opt_select_limit
>
> --- 195,201 ----
> opt_column_list, columnList, opt_name_list,
> sort_clause, sortby_list, index_params, index_list, name_list,
> from_clause, from_list, opt_array_bounds,
> ! expr_list, attrs, target_list, insert_target_list, update_target_list,
> def_list, opt_indirection, group_clause, TriggerFuncArgs,
> select_limit, opt_select_limit
>
> ***************
> *** 253,259 ****
> %type <node> table_ref
> %type <jexpr> joined_table
> %type <range> relation_expr
> ! %type <target> target_el, update_target_el
> %type <paramno> ParamNo
>
> %type <typnam> Typename, SimpleTypename, ConstTypename
> --- 253,259 ----
> %type <node> table_ref
> %type <jexpr> joined_table
> %type <range> relation_expr
> ! %type <target> target_el, update_target_el, insert_target_el
> %type <paramno> ParamNo
>
> %type <typnam> Typename, SimpleTypename, ConstTypename
> ***************
> *** 3302,3308 ****
> }
> ;
>
> ! insert_rest: VALUES '(' target_list ')'
> {
> $$ = makeNode(InsertStmt);
> $$->cols = NIL;
> --- 3302,3308 ----
> }
> ;
>
> ! insert_rest: VALUES '(' insert_target_list ')'
> {
> $$ = makeNode(InsertStmt);
> $$->cols = NIL;
> ***************
> *** 5482,5488 ****
> *
>
> ****************************************************************************
> */
>
> ! /* Target lists as found in SELECT ... and INSERT VALUES ( ... ) */
>
> target_list: target_list ',' target_el
> { $$ = lappend($1, $3); }
> --- 5482,5488 ----
> *
>
> ****************************************************************************
> */
>
> ! /* Target lists as found in SELECT ... */
>
> target_list: target_list ',' target_el
> { $$ = lappend($1, $3); }
> ***************
> *** 5490,5496 ****
> --- 5490,5522 ----
> { $$ = makeList1($1); }
> ;
>
> + /* Target lists as found in INSERT VALUES ( ... ) */
> +
> + /* pavlo (pb@pbit.org): 2001-12-27: parse node based handling for the
> DEFAULT value added;
> + now it's possible to INSERT INTO ... VALUES (..., FEFAULT, ...)!
> + */
> + insert_target_list: insert_target_list ',' insert_target_el
> + { $$ = lappend($1, $3); }
> + | insert_target_el
> + { $$ = makeList1($1); }
> + | insert_target_list ',' target_el
> + { $$ = lappend($1, $3); }
> + | target_el
> + { $$ = makeList1($1); }
> + ;
> +
> + insert_target_el: DEFAULT
> + {
> + Default *n = makeNode(Default);
> + $$ = makeNode(ResTarget);
> + $$->name = NULL;
> + $$->indirection = NULL;
> + $$->val = (Node *)n;
> + }
> + ;
> +
> /* AS is not optional because shift/red conflict with unary ops */
> +
> target_el: a_expr AS ColLabel
> {
> $$ = makeNode(ResTarget);
> ***************
> *** 6380,6383 ****
> strcpy(newval+1, oldval);
> v->val.str = newval;
> }
> ! }
> --- 6406,6409 ----
> strcpy(newval+1, oldval);
> v->val.str = newval;
> }
> ! }
> \ No newline at end of file
> Index: src/include/nodes/parsenodes.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/include/nodes/parsenodes.h,v
> retrieving revision 1.151
> diff -c -r1.151 parsenodes.h
> *** src/include/nodes/parsenodes.h 2001/11/05 17:46:34 1.151
> --- src/include/nodes/parsenodes.h 2001/12/28 23:16:30
> ***************
> *** 1005,1010 ****
> --- 1005,1019 ----
> } A_Const;
>
> /*
> + * pavlo (pb@pbit.org): 2001.12.27:
> + * Default - the DEFAULT constant expression used in the target list of
> INSERT INTO ... VALUES (..., DEFAULT, ...)
> + */
> + typedef struct Default
> + {
> + NodeTag type;
> + } Default;
> +
> + /*
> * TypeCast - a CAST expression
> *
> * NOTE: for mostly historical reasons, A_Const and ParamNo parsenodes
> contain
> ***************
> *** 1355,1358 ****
> */
> typedef SortClause GroupClause;
>
> ! #endif /* PARSENODES_H */
> --- 1364,1367 ----
> */
> typedef SortClause GroupClause;
>
> ! #endif /* PARSENODES_H */
> \ No newline at end of file
> Index: src/include/nodes/nodes.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/include/nodes/nodes.h,v
> retrieving revision 1.96
> diff -c -r1.96 nodes.h
> *** src/include/nodes/nodes.h 2001/11/05 17:46:34 1.96
> --- src/include/nodes/nodes.h 2001/12/29 09:23:37
> ***************
> *** 222,227 ****
> --- 222,228 ----
> T_CaseWhen,
> T_FkConstraint,
> T_PrivGrantee,
> + T_Default, /* pavlo (pb@pbit.org) : 2001.12.27: DEFAULT element of
> the INSERT INTO target list*/
>
> /*
> * TAGS FOR FUNCTION-CALL CONTEXT AND RESULTINFO NODES (see fmgr.h)
> ***************
> *** 365,368 ****
> (jointype) == JOIN_FULL || \
> (jointype) == JOIN_RIGHT)
>
> ! #endif /* NODES_H */
> --- 366,369 ----
> (jointype) == JOIN_FULL || \
> (jointype) == JOIN_RIGHT)
>
> ! #endif /* NODES_H */
> \ No newline at end of file
>
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
-- 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,
Pennsylvania19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Double-oops. There is a later one under a different email subject
> called TODO questino that is a context diff. I just need to know if
> this is the newest version and if anyone doesn't like it.
I don't like it. As given, it inserts default values into the query
at parse time, whereas this must not be done until planning time.
(Otherwise the defaults sneak into stored rules, and if you change
defaults with ALTER TABLE you will get unexpected results.) The
correct (and actually easier) way is to simply drop the defaulted column
out of the analyzed query altogether.
This is not Pavlo's fault exactly, since he copied the way we used
to do it in 7.1 ... but the patch must be updated to follow 7.2
practice.
Another problem: no copy/equal/outfuncs support for the added node type.
Stylistic issue: we should discourage people from putting their initials
on every bit of code they touch. The code will soon be unreadable if
such becomes common practice.
regards, tom lane
Good analysis. Thanks. Pavlov, I know this code is complex. I think these tips will help. --------------------------------------------------------------------------- Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Double-oops. There is a later one under a different email subject > > called TODO questino that is a context diff. I just need to know if > > this is the newest version and if anyone doesn't like it. > > I don't like it. As given, it inserts default values into the query > at parse time, whereas this must not be done until planning time. > (Otherwise the defaults sneak into stored rules, and if you change > defaults with ALTER TABLE you will get unexpected results.) The > correct (and actually easier) way is to simply drop the defaulted column > out of the analyzed query altogether. > > This is not Pavlo's fault exactly, since he copied the way we used > to do it in 7.1 ... but the patch must be updated to follow 7.2 > practice. > > Another problem: no copy/equal/outfuncs support for the added node type. > > Stylistic issue: we should discourage people from putting their initials > on every bit of code they touch. The code will soon be unreadable if > such becomes common practice. > > regards, tom lane > -- 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, Pennsylvania19026
Tom Lane wrote: > I don't like it. As given, it inserts default values into the query > at parse time, whereas this must not be done until planning time. > (Otherwise the defaults sneak into stored rules, and if you change > defaults with ALTER TABLE you will get unexpected results.) The :( But of course you are right - my solution was just a hack to learn about the internals and I really didn't expect my first patch to be accepted. > correct (and actually easier) way is to simply drop the defaulted column > out of the analyzed query altogether. I know what you mean. > > This is not Pavlo's fault exactly, since he copied the way we used > to do it in 7.1 ... but the patch must be updated to follow 7.2 > practice. I coudn't know about such details. But I see, my solution wasn't completely wrong. > > Another problem: no copy/equal/outfuncs support for the added node type. Is it also interesting in the case of the DEFAULT-pair being droped? > > Stylistic issue: we should discourage people from putting their initials > on every bit of code they touch. The code will soon be unreadable if > such becomes common practice. Understandable. I don't want to immortalize my initials in a foreign work. My comments where a kind of orientaition marks for me to quickly find my own code. As you know, I just wanted to know if I'm on the right way experimenting with this TODO-issue. Sorry, but I don't think I should provide this patch. I really haven't enough energy and time to become an internals-GURU. This software is too mighty and it's internals contain too many special thoughts of it's core developers, so I would never get a required motivation to participate the kernal development. What I'll look for in the next time is what I wanted to do at the beginning of my postgresql-dev-adventure. I'm developing very complex applications based on Oracle. IMHO, it's an expensive and overheaded colossus, so Pg is my absolute favorite when I develop a low cost web-based-solution for smb. w ho don't want (or can't) pay a sack of money for the database. Ross J. Reedstrom pointed me to the developer group working on the Oracle-related issues. So I'll contact those guys to discuss some ideas I've got after working with both Pg and Oracle. BTW, here's smth. really funny about that TODO issue: smb. sent me an SQL-script containig inserts using that "default" placeholder, and my modified Pg-version did it! But now, I use the stable without my modifications. rgds Pavlo Baron
"Pavlo Baron" <pb@pbit.org> writes:
>> Another problem: no copy/equal/outfuncs support for the added node type.
> Is it also interesting in the case of the DEFAULT-pair being droped?
Yes, mainly on general principles: for example, debug dumps of raw parse
trees will fail if there's not outfuncs support for your node type.
We've been lax on this in the past but I think it's important to try to
keep those support routines complete.
> Sorry, but I don't think I should provide this patch.
Sorry to hear that. It needs to be done, and you are not that far away
from having it done.
regards, tom lane