what is meant by the TODO-item "Disallow missing columns in INSERT ...
VALUES, per ANSI" ?

rgds
Pavlo Baron


Thu, 27 Dec 2001 21:58:06 +0100
"Pavlo Baron" <pb@pbit.org>
TODO question
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 at Pop4 Networking
        Online Campground Directory    http://www.camping-usa.com
       Online Giftshop Superstore    http://www.cloudninegifts.com
==========================================================================




Thu, 27 Dec 2001 17:21:22 -0500 (EST)
Vince Vielhaber <vev@michvhf.com>
Re: TODO question
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


Thu, 27 Dec 2001 23:54:14 +0100
"Pavlo Baron" <pb@pbit.org>
Re: TODO question
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

Thu, 27 Dec 2001 18:07:07 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: TODO question 
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 at Pop4 Networking
        Online Campground Directory    http://www.camping-usa.com
       Online Giftshop Superstore    http://www.cloudninegifts.com
==========================================================================




Thu, 27 Dec 2001 18:17:29 -0500 (EST)
Vince Vielhaber <vev@michvhf.com>
Re: TODO question
> 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, Pennsylvania 19026

Thu, 27 Dec 2001 19:12:59 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: TODO question
> 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, Pennsylvania 19026

Thu, 27 Dec 2001 19:17:10 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: TODO question
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


Fri, 28 Dec 2001 10:15:59 +0100
"Pavlo Baron" <pb@pbit.org>
Re: TODO question
> 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

Fri, 28 Dec 2001 15:07:12 +0000
Thomas Lockhart <lockhart@fourpalms.org>
Re: TODO question
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

Fri, 28 Dec 2001 10:39:32 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: TODO question 
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


Fri, 28 Dec 2001 17:17:19 +0100
"Pavlo Baron" <pb@pbit.org>
Re: TODO question 
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


Fri, 28 Dec 2001 17:29:36 +0100
"Pavlo Baron" <pb@pbit.org>
Re: TODO question
> 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, Pennsylvania 19026

Fri, 28 Dec 2001 12:57:53 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: TODO question
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 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
Sat, 29 Dec 2001 00:25:11 +0100
"Pavlo Baron" <pb@pbit.org>
Re: TODO question
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.

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

Fri, 28 Dec 2001 19:15:08 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: TODO question 
> 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, Pennsylvania 19026

Fri, 28 Dec 2001 22:44:50 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: TODO question
> 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, Pennsylvania 19026

Fri, 28 Dec 2001 22:51:06 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: TODO question
> > 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 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
Sat, 29 Dec 2001 10:28:34 +0100
"Pavlo Baron" <pb@pbit.org>
Re: TODO question
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


Sat, 29 Dec 2001 13:09:58 +0100
"Pavlo Baron" <pb@pbit.org>
Re: TODO question 
"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

Sat, 29 Dec 2001 11:54:39 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: TODO question 
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


Sat, 29 Dec 2001 21:12:09 +0100
"Pavlo Baron" <pb@pbit.org>
Re: TODO question 
"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

Sat, 29 Dec 2001 15:56:33 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: TODO question 
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


Sat, 29 Dec 2001 23:06:48 +0100
"Pavlo Baron" <pb@pbit.org>
Re: TODO question 
> 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, Pennsylvania 19026

Fri, 22 Feb 2002 20:51:02 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: TODO question
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


Sat, 2 Mar 2002 21:01:44 +0100
"Pavlo Baron" <pb@pbit.org>
Re: TODO question

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, Pennsylvania 19026

Thu, 7 Mar 2002 20:03:52 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: TODO question
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

Thu, 07 Mar 2002 21:02:35 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: TODO question 

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, Pennsylvania 19026

Thu, 7 Mar 2002 21:05:12 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: TODO question
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


Sat, 9 Mar 2002 09:45:38 +0100
"Pavlo Baron" <pb@pbit.org>
Re: TODO question 
"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

Sat, 09 Mar 2002 10:26:31 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: TODO question 