Re: Split-up ECPG patches - Mailing list pgsql-hackers

Michael Meskes írta:
> On Mon, Aug 03, 2009 at 06:59:30PM +0200, Boszormenyi Zoltan wrote:
>
>>> Why is this messing with the core grammar?
>>>
>> ...
>>
>
> Zoltan, could you please explain why you unrolled FORWARD and BACKWARD? I tried
> applying the rest of your patch, without this unrolling but didn't get any
> shift/reduce problem. Might have been that I missed something, so could you please try again?
>

Without a re-quoted explanation, please, compare
your modified patch with the attached one. I rolled
FORWARD and BACKWARD back into fetch_direction
in the core grammar, deleting the newly introduced FETCH
and MOVE rules from the core and ECPG grammar and
again I got this during the ECPG grammar compilation:

...
"/usr/bin/perl" ./parse.pl . < ../../../../src/backend/parser/gram.y >
preproc.y
/usr/bin/bison -d  -o preproc.c preproc.y
preproc.y: conflicts: 2 shift/reduce
preproc.y: expected 0 shift/reduce conflicts
make[4]: *** [preproc.c] Error 1
make[4]: Leaving directory
`/home/zozo/Schönig-számlák/leoni/2/pgsql/src/interfaces/ecpg/preproc'
...

FYI:

$ rpm -q bison flex
bison-2.3-5.fc9.x86_64
flex-2.5.35-2.fc9.x86_64

> Tom, AFAICT we only need one core grammar change, moving the cursor name to
> it's own rule that only resolves back to name. This rule should be eliminated
> by bison during the build process anyway, so I see no problem adding it. It
> does make the ecpg changes way smaller though. Is this okay with you?
>
> Zoltan, two more things about this patch need to be cleared:
> - I don't think your code is able to handle varchars.
>

I will test that, thanks.

> - There is no test. Please add this to some of our test cases or write a new one.
>

I will write some regression tests, of course.

> Some variable handling commands look suspicious to me, a test case might
> alleviate my concerns.
>

Best regards,
Zoltán Böszörményi

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

diff -dcrpN pgsql.orig/src/backend/parser/gram.y pgsql/src/backend/parser/gram.y
*** pgsql.orig/src/backend/parser/gram.y    2009-08-03 10:38:28.000000000 +0200
--- pgsql/src/backend/parser/gram.y    2009-08-08 17:26:00.000000000 +0200
*************** static TypeName *TableFuncTypeName(List
*** 253,259 ****

  %type <str>        relation_name copy_file_name
                  database_name access_method_clause access_method attr_name
!                 index_name name file_name cluster_index_specification

  %type <list>    func_name handler_name qual_Op qual_all_Op subquery_Op
                  opt_class opt_validator validator_clause
--- 253,259 ----

  %type <str>        relation_name copy_file_name
                  database_name access_method_clause access_method attr_name
!                 index_name name cursor_name file_name cluster_index_specification

  %type <list>    func_name handler_name qual_Op qual_all_Op subquery_Op
                  opt_class opt_validator validator_clause
*************** reloption_elem:
*** 1915,1921 ****
   *****************************************************************************/

  ClosePortalStmt:
!             CLOSE name
                  {
                      ClosePortalStmt *n = makeNode(ClosePortalStmt);
                      n->portalname = $2;
--- 1915,1921 ----
   *****************************************************************************/

  ClosePortalStmt:
!             CLOSE cursor_name
                  {
                      ClosePortalStmt *n = makeNode(ClosePortalStmt);
                      n->portalname = $2;
*************** comment_text:
*** 4082,4095 ****
   *
   *****************************************************************************/

! FetchStmt:    FETCH fetch_direction from_in name
                  {
                      FetchStmt *n = (FetchStmt *) $2;
                      n->portalname = $4;
                      n->ismove = FALSE;
                      $$ = (Node *)n;
                  }
!             | FETCH name
                  {
                      FetchStmt *n = makeNode(FetchStmt);
                      n->direction = FETCH_FORWARD;
--- 4082,4095 ----
   *
   *****************************************************************************/

! FetchStmt:    FETCH fetch_direction from_in cursor_name
                  {
                      FetchStmt *n = (FetchStmt *) $2;
                      n->portalname = $4;
                      n->ismove = FALSE;
                      $$ = (Node *)n;
                  }
!             | FETCH cursor_name
                  {
                      FetchStmt *n = makeNode(FetchStmt);
                      n->direction = FETCH_FORWARD;
*************** FetchStmt:    FETCH fetch_direction from_in
*** 4098,4111 ****
                      n->ismove = FALSE;
                      $$ = (Node *)n;
                  }
!             | MOVE fetch_direction from_in name
                  {
                      FetchStmt *n = (FetchStmt *) $2;
                      n->portalname = $4;
                      n->ismove = TRUE;
                      $$ = (Node *)n;
                  }
!             | MOVE name
                  {
                      FetchStmt *n = makeNode(FetchStmt);
                      n->direction = FETCH_FORWARD;
--- 4098,4111 ----
                      n->ismove = FALSE;
                      $$ = (Node *)n;
                  }
!             | MOVE fetch_direction from_in cursor_name
                  {
                      FetchStmt *n = (FetchStmt *) $2;
                      n->portalname = $4;
                      n->ismove = TRUE;
                      $$ = (Node *)n;
                  }
!             | MOVE cursor_name
                  {
                      FetchStmt *n = makeNode(FetchStmt);
                      n->direction = FETCH_FORWARD;
*************** set_target_list:
*** 6847,6853 ****
   *                CURSOR STATEMENTS
   *
   *****************************************************************************/
! DeclareCursorStmt: DECLARE name cursor_options CURSOR opt_hold FOR SelectStmt
                  {
                      DeclareCursorStmt *n = makeNode(DeclareCursorStmt);
                      n->portalname = $2;
--- 6847,6853 ----
   *                CURSOR STATEMENTS
   *
   *****************************************************************************/
! DeclareCursorStmt: DECLARE cursor_name cursor_options CURSOR opt_hold FOR SelectStmt
                  {
                      DeclareCursorStmt *n = makeNode(DeclareCursorStmt);
                      n->portalname = $2;
*************** DeclareCursorStmt: DECLARE name cursor_o
*** 6858,6863 ****
--- 6858,6866 ----
                  }
          ;

+ cursor_name:    name                        { $$ = $1; }
+         ;
+
  cursor_options: /*EMPTY*/                    { $$ = 0; }
              | cursor_options NO SCROLL        { $$ = $1 | CURSOR_OPT_NO_SCROLL; }
              | cursor_options SCROLL            { $$ = $1 | CURSOR_OPT_SCROLL; }
diff -dcrpN pgsql.orig/src/interfaces/ecpg/preproc/ecpg.addons pgsql/src/interfaces/ecpg/preproc/ecpg.addons
*** pgsql.orig/src/interfaces/ecpg/preproc/ecpg.addons    2009-01-30 17:28:46.000000000 +0100
--- pgsql/src/interfaces/ecpg/preproc/ecpg.addons    2009-08-08 17:28:02.000000000 +0200
*************** ECPG: fetch_directionBACKWARDSignedIcons
*** 221,226 ****
--- 221,235 ----
              free($2);
              $2 = make_str("$0");
          }
+ ECPG: cursor_namename rule
+     | char_civar
+         {
+             char *curname = mm_alloc(strlen($1) + 2);
+             sprintf(curname, ":%s", $1);
+             free($1);
+             $1 = curname;
+             $$ = $1;
+         }
  ECPG: PrepareStmtPREPAREprepared_nameprep_type_clauseASPreparableStmt block
      {
          $$.name = $2;
*************** ECPG: PrepareStmtPREPAREprepared_namepre
*** 235,243 ****
      }
  ECPG: ExecuteStmtEXECUTEprepared_nameexecute_param_clauseexecute_rest block
      { $$ = $2; }
! ECPG: DeclareCursorStmtDECLAREnamecursor_optionsCURSORopt_holdFORSelectStmt block
      {
          struct cursor *ptr, *this;

          for (ptr = cur; ptr != NULL; ptr = ptr->next)
          {
--- 244,253 ----
      }
  ECPG: ExecuteStmtEXECUTEprepared_nameexecute_param_clauseexecute_rest block
      { $$ = $2; }
! ECPG: DeclareCursorStmtDECLAREcursor_namecursor_optionsCURSORopt_holdFORSelectStmt block
      {
          struct cursor *ptr, *this;
+         char *cursor_marker = $2[0] == ':' ? make_str("$0") : mm_strdup($2);

          for (ptr = cur; ptr != NULL; ptr = ptr->next)
          {
*************** ECPG: DeclareCursorStmtDECLAREnamecursor
*** 251,257 ****
          this->name = $2;
          this->connection = connection;
          this->opened = false;
!         this->command =  cat_str(7, make_str("declare"), mm_strdup($2), $3, make_str("cursor"), $5, make_str("for"),
$7);
          this->argsinsert = argsinsert;
          this->argsresult = argsresult;
          argsinsert = argsresult = NULL;
--- 261,267 ----
          this->name = $2;
          this->connection = connection;
          this->opened = false;
!         this->command =  cat_str(7, make_str("declare"), cursor_marker, $3, make_str("cursor"), $5, make_str("for"),
$7);
          this->argsinsert = argsinsert;
          this->argsresult = argsresult;
          argsinsert = argsresult = NULL;
*************** ECPG: DeclareCursorStmtDECLAREnamecursor
*** 262,267 ****
--- 272,282 ----
          else
              $$ = cat_str(3, make_str("/*"), mm_strdup(this->command), make_str("*/"));
      }
+ ECPG: ClosePortalStmtCLOSEcursor_name block
+     {
+         char *cursor_marker = $2[0] == ':' ? make_str("$0") : $2;
+         $$ = cat2_str(make_str("close"), cursor_marker);
+     }
  ECPG: opt_hold block
      {
          if (compat == ECPG_COMPAT_INFORMIX_SE && autocommit == true)
*************** ECPG: VariableShowStmtSHOWALL block
*** 326,371 ****
          mmerror(PARSE_ERROR, ET_ERROR, "SHOW ALL is not implemented");
          $$ = EMPTY;
      }
! ECPG: FetchStmtFETCHfetch_directionfrom_inname block
      {
          add_additional_variables($4, false);
!         $$ = cat_str(4, make_str("fetch"), $2, $3, $4);
      }
! ECPG: FetchStmtFETCHname block
      {
          add_additional_variables($2, false);
!         $$ = cat_str(2, make_str("fetch"), $2);
      }
! ECPG: FetchStmtMOVEname rule
!     | FETCH fetch_direction from_in name ecpg_into
          {
              add_additional_variables($4, false);
!             $$ = cat_str(4, make_str("fetch"), $2, $3, $4);
          }
!     | FETCH fetch_direction name ecpg_into
          {
              add_additional_variables($3, false);
!             $$ = cat_str(4, make_str("fetch"), $2, make_str("from"), $3);
          }
!     | FETCH from_in name ecpg_into
          {
              add_additional_variables($3, false);
!             $$ = cat_str(3, make_str("fetch"), $2, $3);
          }
!     | FETCH name ecpg_into
          {
              add_additional_variables($2, false);
!             $$ = cat2_str(make_str("fetch"), $2);
          }
!     | FETCH fetch_direction name
          {
              add_additional_variables($3, false);
!             $$ = cat_str(4, make_str("fetch"), $2, make_str("from"), $3);
          }
!     | FETCH from_in name
          {
              add_additional_variables($3, false);
!             $$ = cat_str(3, make_str("fetch"), $2, $3);
          }
  ECPG: SpecialRuleRelationOLD addon
          if (!QueryIsRule)
--- 341,394 ----
          mmerror(PARSE_ERROR, ET_ERROR, "SHOW ALL is not implemented");
          $$ = EMPTY;
      }
! ECPG: FetchStmtFETCHfetch_directionfrom_incursor_name block
      {
+         char *cursor_marker = $4[0] == ':' ? make_str("$0") : $4;
          add_additional_variables($4, false);
!         $$ = cat_str(4, make_str("fetch"), $2, $3, cursor_marker);
      }
! ECPG: FetchStmtFETCHcursor_name block
      {
+         char *cursor_marker = $2[0] == ':' ? make_str("$0") : $2;
          add_additional_variables($2, false);
!         $$ = cat_str(2, make_str("fetch"), cursor_marker);
      }
! ECPG: FetchStmtMOVEcursor_name rule
!     | FETCH fetch_direction from_in cursor_name ecpg_into
          {
+             char *cursor_marker = $4[0] == ':' ? make_str("$0") : $4;
              add_additional_variables($4, false);
!             $$ = cat_str(4, make_str("fetch"), $2, $3, cursor_marker);
          }
!     | FETCH fetch_direction cursor_name ecpg_into
          {
+             char *cursor_marker = $3[0] == ':' ? make_str("$0") : $3;
              add_additional_variables($3, false);
!             $$ = cat_str(4, make_str("fetch"), $2, make_str("from"), cursor_marker);
          }
!     | FETCH from_in cursor_name ecpg_into
          {
+             char *cursor_marker = $3[0] == ':' ? make_str("$0") : $3;
              add_additional_variables($3, false);
!             $$ = cat_str(3, make_str("fetch"), $2, cursor_marker);
          }
!     | FETCH cursor_name ecpg_into
          {
+             char *cursor_marker = $2[0] == ':' ? make_str("$0") : $2;
              add_additional_variables($2, false);
!             $$ = cat2_str(make_str("fetch"), cursor_marker);
          }
!     | FETCH fetch_direction cursor_name
          {
+             char *cursor_marker = $3[0] == ':' ? make_str("$0") : $3;
              add_additional_variables($3, false);
!             $$ = cat_str(4, make_str("fetch"), $2, make_str("from"), cursor_marker);
          }
!     | FETCH from_in cursor_name
          {
+             char *cursor_marker = $3[0] == ':' ? make_str("$0") : $3;
              add_additional_variables($3, false);
!             $$ = cat_str(3, make_str("fetch"), $2, cursor_marker);
          }
  ECPG: SpecialRuleRelationOLD addon
          if (!QueryIsRule)
diff -dcrpN pgsql.orig/src/interfaces/ecpg/preproc/ecpg.trailer pgsql/src/interfaces/ecpg/preproc/ecpg.trailer
*** pgsql.orig/src/interfaces/ecpg/preproc/ecpg.trailer    2009-08-07 13:06:28.000000000 +0200
--- pgsql/src/interfaces/ecpg/preproc/ecpg.trailer    2009-08-08 17:23:11.000000000 +0200
*************** prepared_name: name             {
*** 285,293 ****
   * Declare a prepared cursor. The syntax is different from the standard
   * declare statement, so we create a new rule.
   */
! ECPGCursorStmt:  DECLARE name cursor_options CURSOR opt_hold FOR prepared_name
          {
              struct cursor *ptr, *this;
              struct variable *thisquery = (struct variable *)mm_alloc(sizeof(struct variable));
              const char *con = connection ? connection : "NULL";

--- 285,294 ----
   * Declare a prepared cursor. The syntax is different from the standard
   * declare statement, so we create a new rule.
   */
! ECPGCursorStmt:  DECLARE cursor_name cursor_options CURSOR opt_hold FOR prepared_name
          {
              struct cursor *ptr, *this;
+             char *cursor_marker = $2[0] == ':' ? make_str("$0") : mm_strdup($2);
              struct variable *thisquery = (struct variable *)mm_alloc(sizeof(struct variable));
              const char *con = connection ? connection : "NULL";

*************** ECPGCursorStmt:  DECLARE name cursor_opt
*** 304,310 ****
              this->next = cur;
              this->name = $2;
              this->connection = connection;
!             this->command =  cat_str(6, make_str("declare"), mm_strdup($2), $3, make_str("cursor"), $5, make_str("for
$1"));
              this->argsresult = NULL;

              thisquery->type = &ecpg_query;
--- 305,311 ----
              this->next = cur;
              this->name = $2;
              this->connection = connection;
!             this->command =  cat_str(6, make_str("declare"), cursor_marker, $3, make_str("cursor"), $5, make_str("for
$1"));
              this->argsresult = NULL;

              thisquery->type = &ecpg_query;
*************** ECPGCursorStmt:  DECLARE name cursor_opt
*** 314,319 ****
--- 315,326 ----
              sprintf(thisquery->name, "ECPGprepared_statement(%s, %s, __LINE__)", con, $7);

              this->argsinsert = NULL;
+             if ($2[0] == ':')
+             {
+                 struct variable *var = find_variable($2 + 1);
+                 remove_variable_from_list(&argsinsert, var);
+                 add_variable_to_head(&(this->argsinsert), var, &no_indicator);
+             }
              add_variable_to_head(&(this->argsinsert), thisquery, &no_indicator);

              cur = this;
*************** ECPGFree:    SQL_FREE name    { $$ = $2; }
*** 954,960 ****
  /*
   * open is an open cursor, at the moment this has to be removed
   */
! ECPGOpen: SQL_OPEN name opt_ecpg_using { $$ = $2; };

  opt_ecpg_using: /*EMPTY*/    { $$ = EMPTY; }
          | ecpg_using        { $$ = $1; }
--- 961,976 ----
  /*
   * open is an open cursor, at the moment this has to be removed
   */
! ECPGOpen: SQL_OPEN cursor_name opt_ecpg_using
!         {
!             if ($2[0] == ':')
!             {
!                 struct variable *var = find_variable($2 + 1);
!                 remove_variable_from_list(&argsinsert, var);
!             }
!             $$ = $2;
!         }
!         ;

  opt_ecpg_using: /*EMPTY*/    { $$ = EMPTY; }
          | ecpg_using        { $$ = $1; }
*************** civarind: cvariable indicator
*** 1779,1784 ****
--- 1795,1807 ----
          }
          ;

+ char_civar: char_variable
+         {
+             add_variable_to_head(&argsinsert, find_variable($1), &no_indicator);
+             $$ = $1;
+         }
+         ;
+
  civar: cvariable
          {
              add_variable_to_head(&argsinsert, find_variable($1), &no_indicator);
diff -dcrpN pgsql.orig/src/interfaces/ecpg/preproc/ecpg.type pgsql/src/interfaces/ecpg/preproc/ecpg.type
*** pgsql.orig/src/interfaces/ecpg/preproc/ecpg.type    2008-11-14 11:03:33.000000000 +0100
--- pgsql/src/interfaces/ecpg/preproc/ecpg.type    2009-08-08 17:23:11.000000000 +0200
***************
*** 43,48 ****
--- 43,49 ----
  %type <str> c_term
  %type <str> c_thing
  %type <str> char_variable
+ %type <str> char_civar
  %type <str> civar
  %type <str> civarind
  %type <str> ColLabel
diff -dcrpN pgsql.orig/src/interfaces/ecpg/preproc/extern.h pgsql/src/interfaces/ecpg/preproc/extern.h
*** pgsql.orig/src/interfaces/ecpg/preproc/extern.h    2009-07-17 07:50:56.000000000 +0200
--- pgsql/src/interfaces/ecpg/preproc/extern.h    2009-08-08 17:23:11.000000000 +0200
*************** extern struct descriptor *lookup_descrip
*** 91,96 ****
--- 91,97 ----
  extern struct variable *descriptor_variable(const char *name, int input);
  extern void add_variable_to_head(struct arguments **, struct variable *, struct variable *);
  extern void add_variable_to_tail(struct arguments **, struct variable *, struct variable *);
+ extern void remove_variable_from_list(struct arguments ** list, struct variable * var);
  extern void dump_variables(struct arguments *, int);
  extern struct typedefs *get_typedef(char *);
  extern void adjust_array(enum ECPGttype, char **, char **, char *, char *, int, bool);
diff -dcrpN pgsql.orig/src/interfaces/ecpg/preproc/variable.c pgsql/src/interfaces/ecpg/preproc/variable.c
*** pgsql.orig/src/interfaces/ecpg/preproc/variable.c    2009-08-07 13:06:28.000000000 +0200
--- pgsql/src/interfaces/ecpg/preproc/variable.c    2009-08-08 17:23:11.000000000 +0200
*************** add_variable_to_tail(struct arguments **
*** 401,406 ****
--- 401,430 ----
          *list = new;
  }

+ void
+ remove_variable_from_list(struct arguments ** list, struct variable * var)
+ {
+     struct arguments *p, *prev = NULL;
+     bool found = false;
+
+     for (p = *list; p; p = p->next)
+     {
+         if (p->variable == var)
+         {
+             found = true;
+             break;
+         }
+         prev = p;
+     }
+     if (found)
+     {
+         if (prev)
+             prev->next = p->next;
+         else
+             *list = p->next;
+     }
+ }
+
  /* Dump out a list of all the variable on this list.
     This is a recursive function that works from the end of the list and
     deletes the list as we go on.