Re: Using subselects in INSERTs?

J Smith <dark_panda@hushmail.com> writes:
> I managed to trace the problem back to a RULE that was created while I must
> have been asleep at the wheel. The problem goes away when I disable the
> RULE or replace the subquery with an actual value.

I dug into this and found that the misbehavior occurs when the
sub-SELECT that is present in the INSERT:

> INSERT INTO clip (program_id, clip_name) VALUES (
> (SELECT program_id FROM program WHERE program_code = '9531443001'),
> 'Canada: A Diverse Culture');

is inserted to replace "new.program_id" in the RULE:

> CREATE RULE program_clip_insert_only_1 AS ON INSERT TO clip WHERE
> ((SELECT count(*) AS count FROM clip WHERE clip.program_id =
> new.program_id) >= 1) DO INSTEAD NOTHING;

As far as I can tell, this problem has existed for a long time; it is
certainly not new in 7.3.4.  (I see the same failure in 7.2.4 as 7.3.4.)
Are you sure you weren't changing your application at the same time you
updated?

I've applied the attached patch to the 7.3 branch, if you want to use
it.

            regards, tom lane


Index: src/backend/rewrite/rewriteManip.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/rewrite/rewriteManip.c,v
retrieving revision 1.67
diff -c -r1.67 rewriteManip.c
*** src/backend/rewrite/rewriteManip.c    20 Oct 2002 00:58:55 -0000    1.67
--- src/backend/rewrite/rewriteManip.c    20 Oct 2003 19:09:48 -0000
***************
*** 74,80 ****
  checkExprHasSubLink(Node *node)
  {
      /*
!      * If a Query is passed, examine it --- but we will not recurse into
       * sub-Queries.
       */
      if (node && IsA(node, Query))
--- 74,80 ----
  checkExprHasSubLink(Node *node)
  {
      /*
!      * If a Query is passed, examine it --- but we need not recurse into
       * sub-Queries.
       */
      if (node && IsA(node, Query))
***************
*** 644,653 ****
      /*
       * Make sure query is marked correctly if added qual has sublinks or
       * aggregates (not sure it can ever have aggs, but sublinks
!      * definitely).
       */
!     parsetree->hasAggs |= checkExprHasAggs(copy);
!     parsetree->hasSubLinks |= checkExprHasSubLink(copy);
  }

  /*
--- 644,655 ----
      /*
       * Make sure query is marked correctly if added qual has sublinks or
       * aggregates (not sure it can ever have aggs, but sublinks
!      * definitely).  Need not search qual when query is already marked.
       */
!     if (!parsetree->hasAggs)
!         parsetree->hasAggs = checkExprHasAggs(copy);
!     if (!parsetree->hasSubLinks)
!         parsetree->hasSubLinks = checkExprHasSubLink(copy);
  }

  /*
***************
*** 684,693 ****
      /*
       * Make sure query is marked correctly if added qual has sublinks or
       * aggregates (not sure it can ever have aggs, but sublinks
!      * definitely).
       */
!     parsetree->hasAggs |= checkExprHasAggs(copy);
!     parsetree->hasSubLinks |= checkExprHasSubLink(copy);
  }


--- 686,697 ----
      /*
       * Make sure query is marked correctly if added qual has sublinks or
       * aggregates (not sure it can ever have aggs, but sublinks
!      * definitely).  Need not search qual when query is already marked.
       */
!     if (!parsetree->hasAggs)
!         parsetree->hasAggs = checkExprHasAggs(copy);
!     if (!parsetree->hasSubLinks)
!         parsetree->hasSubLinks = checkExprHasSubLink(copy);
  }


***************
*** 758,763 ****
--- 762,773 ----
   * entry with matching resno from targetlist, if there is one.
   * If not, we either change the unmatched Var's varno to update_varno
   * (when event == CMD_UPDATE) or replace it with a constant NULL.
+  *
+  * Note: the business with inserted_sublink is needed to update hasSubLinks
+  * in subqueries when the replacement adds a subquery inside a subquery.
+  * Messy, isn't it?  We do not need to do similar pushups for hasAggs,
+  * because it isn't possible for this transformation to insert a level-zero
+  * aggregate reference into a subquery --- it could only insert outer aggs.
   */

  typedef struct
***************
*** 767,772 ****
--- 777,783 ----
      List       *targetlist;
      int            event;
      int            update_varno;
+     bool        inserted_sublink;
  } ResolveNew_context;

  static Node *
***************
*** 814,819 ****
--- 825,833 ----
                  /* Adjust varlevelsup if tlist item is from higher query */
                  if (this_varlevelsup > 0)
                      IncrementVarSublevelsUp(n, this_varlevelsup, 0);
+                 /* Report it if we are adding a sublink to query */
+                 if (!context->inserted_sublink)
+                     context->inserted_sublink = checkExprHasSubLink(n);
                  return n;
              }
          }
***************
*** 840,849 ****
--- 854,868 ----
      {
          Query       *query = (Query *) node;
          Query       *newnode;
+         bool        save_inserted_sublink;

          FLATCOPY(newnode, query, Query);
          context->sublevels_up++;
+         save_inserted_sublink = context->inserted_sublink;
+         context->inserted_sublink = false;
          query_tree_mutator(newnode, ResolveNew_mutator, context, 0);
+         newnode->hasSubLinks |= context->inserted_sublink;
+         context->inserted_sublink = save_inserted_sublink;
          context->sublevels_up--;
          return (Node *) newnode;
      }
***************
*** 862,867 ****
--- 881,887 ----
      context.targetlist = targetlist;
      context.event = event;
      context.update_varno = update_varno;
+     context.inserted_sublink = false;

      /*
       * Must be prepared to start with a Query or a bare expression tree;