Remove PLPGSQL_DTYPE_ARRAYELEM datum type within pl/pgsql.
authorTom Lane
Mon, 4 Jan 2021 17:14:37 +0000 (12:14 -0500)
committerTom Lane
Mon, 4 Jan 2021 17:14:37 +0000 (12:14 -0500)
In the wake of the previous commit, we don't really need this anymore,
since array assignment is primarily handled by the core code.

The only way that that code could still be reached is that a GET
DIAGNOSTICS target variable could be an array element.  But that
doesn't seem like a particularly essential feature.  I'd added it
in commit 55caaaeba, but just because it was easy not because
anyone had actually asked for it.  Hence, revert that patch and
then remove the now-unreachable stuff.  (If we really had to,
we could probably reimplement GET DIAGNOSTICS using the new
assignment machinery; but the cost/benefit ratio looks very poor,
and it'd likely be a bit slower.)

Note that PLPGSQL_DTYPE_RECFIELD remains.  It's possible that we
could get rid of that too, but maintaining the existing behaviors
for RECORD-type variables seems like it might be difficult.  Since
there's not any functional limitation in those code paths as there
was in the ARRAYELEM code, I've not pursued the idea.

Discussion: https://postgr.es/m/4165684.1607707277@sss.pgh.pa.us

src/pl/plpgsql/src/pl_exec.c
src/pl/plpgsql/src/pl_funcs.c
src/pl/plpgsql/src/pl_gram.y
src/pl/plpgsql/src/plpgsql.h
src/test/regress/expected/plpgsql.out
src/test/regress/sql/plpgsql.sql

index 5e4dbd25a2a77cb4c208669439e08607f98f7671..95f0adc81d9f435caf5fb7695be72475098b01f9 100644 (file)
@@ -1311,12 +1311,11 @@ copy_plpgsql_datums(PLpgSQL_execstate *estate,
 
            case PLPGSQL_DTYPE_ROW:
            case PLPGSQL_DTYPE_RECFIELD:
-           case PLPGSQL_DTYPE_ARRAYELEM:
 
                /*
                 * These datum records are read-only at runtime, so no need to
-                * copy them (well, RECFIELD and ARRAYELEM contain cached
-                * data, but we'd just as soon centralize the caching anyway).
+                * copy them (well, RECFIELD contains cached data, but we'd
+                * just as soon centralize the caching anyway).
                 */
                outdatum = indatum;
                break;
@@ -4136,9 +4135,6 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
  *
  * NB: the result of the evaluation is no longer valid after this is done,
  * unless it is a pass-by-value datatype.
- *
- * NB: if you change this code, see also the hacks in exec_assign_value's
- * PLPGSQL_DTYPE_ARRAYELEM case for partial cleanup after subscript evals.
  * ----------
  */
 static void
@@ -5288,198 +5284,6 @@ exec_assign_value(PLpgSQL_execstate *estate,
                break;
            }
 
-       case PLPGSQL_DTYPE_ARRAYELEM:
-           {
-               /*
-                * Target is an element of an array
-                */
-               PLpgSQL_arrayelem *arrayelem;
-               int         nsubscripts;
-               int         i;
-               PLpgSQL_expr *subscripts[MAXDIM];
-               int         subscriptvals[MAXDIM];
-               Datum       oldarraydatum,
-                           newarraydatum,
-                           coerced_value;
-               bool        oldarrayisnull;
-               Oid         parenttypoid;
-               int32       parenttypmod;
-               SPITupleTable *save_eval_tuptable;
-               MemoryContext oldcontext;
-
-               /*
-                * We need to do subscript evaluation, which might require
-                * evaluating general expressions; and the caller might have
-                * done that too in order to prepare the input Datum.  We have
-                * to save and restore the caller's SPI_execute result, if
-                * any.
-                */
-               save_eval_tuptable = estate->eval_tuptable;
-               estate->eval_tuptable = NULL;
-
-               /*
-                * To handle constructs like x[1][2] := something, we have to
-                * be prepared to deal with a chain of arrayelem datums. Chase
-                * back to find the base array datum, and save the subscript
-                * expressions as we go.  (We are scanning right to left here,
-                * but want to evaluate the subscripts left-to-right to
-                * minimize surprises.)  Note that arrayelem is left pointing
-                * to the leftmost arrayelem datum, where we will cache the
-                * array element type data.
-                */
-               nsubscripts = 0;
-               do
-               {
-                   arrayelem = (PLpgSQL_arrayelem *) target;
-                   if (nsubscripts >= MAXDIM)
-                       ereport(ERROR,
-                               (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
-                                errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
-                                       nsubscripts + 1, MAXDIM)));
-                   subscripts[nsubscripts++] = arrayelem->subscript;
-                   target = estate->datums[arrayelem->arrayparentno];
-               } while (target->dtype == PLPGSQL_DTYPE_ARRAYELEM);
-
-               /* Fetch current value of array datum */
-               exec_eval_datum(estate, target,
-                               &parenttypoid, &parenttypmod,
-                               &oldarraydatum, &oldarrayisnull);
-
-               /* Update cached type data if necessary */
-               if (arrayelem->parenttypoid != parenttypoid ||
-                   arrayelem->parenttypmod != parenttypmod)
-               {
-                   Oid         arraytypoid;
-                   int32       arraytypmod = parenttypmod;
-                   int16       arraytyplen;
-                   Oid         elemtypoid;
-                   int16       elemtyplen;
-                   bool        elemtypbyval;
-                   char        elemtypalign;
-
-                   /* If target is domain over array, reduce to base type */
-                   arraytypoid = getBaseTypeAndTypmod(parenttypoid,
-                                                      &arraytypmod);
-
-                   /* ... and identify the element type */
-                   elemtypoid = get_element_type(arraytypoid);
-                   if (!OidIsValid(elemtypoid))
-                       ereport(ERROR,
-                               (errcode(ERRCODE_DATATYPE_MISMATCH),
-                                errmsg("subscripted object is not an array")));
-
-                   /* Collect needed data about the types */
-                   arraytyplen = get_typlen(arraytypoid);
-
-                   get_typlenbyvalalign(elemtypoid,
-                                        &elemtyplen,
-                                        &elemtypbyval,
-                                        &elemtypalign);
-
-                   /* Now safe to update the cached data */
-                   arrayelem->parenttypoid = parenttypoid;
-                   arrayelem->parenttypmod = parenttypmod;
-                   arrayelem->arraytypoid = arraytypoid;
-                   arrayelem->arraytypmod = arraytypmod;
-                   arrayelem->arraytyplen = arraytyplen;
-                   arrayelem->elemtypoid = elemtypoid;
-                   arrayelem->elemtyplen = elemtyplen;
-                   arrayelem->elemtypbyval = elemtypbyval;
-                   arrayelem->elemtypalign = elemtypalign;
-               }
-
-               /*
-                * Evaluate the subscripts, switch into left-to-right order.
-                * Like the expression built by ExecInitSubscriptingRef(),
-                * complain if any subscript is null.
-                */
-               for (i = 0; i < nsubscripts; i++)
-               {
-                   bool        subisnull;
-
-                   subscriptvals[i] =
-                       exec_eval_integer(estate,
-                                         subscripts[nsubscripts - 1 - i],
-                                         &subisnull);
-                   if (subisnull)
-                       ereport(ERROR,
-                               (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
-                                errmsg("array subscript in assignment must not be null")));
-
-                   /*
-                    * Clean up in case the subscript expression wasn't
-                    * simple. We can't do exec_eval_cleanup, but we can do
-                    * this much (which is safe because the integer subscript
-                    * value is surely pass-by-value), and we must do it in
-                    * case the next subscript expression isn't simple either.
-                    */
-                   if (estate->eval_tuptable != NULL)
-                       SPI_freetuptable(estate->eval_tuptable);
-                   estate->eval_tuptable = NULL;
-               }
-
-               /* Now we can restore caller's SPI_execute result if any. */
-               Assert(estate->eval_tuptable == NULL);
-               estate->eval_tuptable = save_eval_tuptable;
-
-               /* Coerce source value to match array element type. */
-               coerced_value = exec_cast_value(estate,
-                                               value,
-                                               &isNull,
-                                               valtype,
-                                               valtypmod,
-                                               arrayelem->elemtypoid,
-                                               arrayelem->arraytypmod);
-
-               /*
-                * If the original array is null, cons up an empty array so
-                * that the assignment can proceed; we'll end with a
-                * one-element array containing just the assigned-to
-                * subscript.  This only works for varlena arrays, though; for
-                * fixed-length array types we skip the assignment.  We can't
-                * support assignment of a null entry into a fixed-length
-                * array, either, so that's a no-op too.  This is all ugly but
-                * corresponds to the current behavior of execExpr*.c.
-                */
-               if (arrayelem->arraytyplen > 0 &&   /* fixed-length array? */
-                   (oldarrayisnull || isNull))
-                   return;
-
-               /* empty array, if any, and newarraydatum are short-lived */
-               oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
-
-               if (oldarrayisnull)
-                   oldarraydatum = PointerGetDatum(construct_empty_array(arrayelem->elemtypoid));
-
-               /*
-                * Build the modified array value.
-                */
-               newarraydatum = array_set_element(oldarraydatum,
-                                                 nsubscripts,
-                                                 subscriptvals,
-                                                 coerced_value,
-                                                 isNull,
-                                                 arrayelem->arraytyplen,
-                                                 arrayelem->elemtyplen,
-                                                 arrayelem->elemtypbyval,
-                                                 arrayelem->elemtypalign);
-
-               MemoryContextSwitchTo(oldcontext);
-
-               /*
-                * Assign the new array to the base variable.  It's never NULL
-                * at this point.  Note that if the target is a domain,
-                * coercing the base array type back up to the domain will
-                * happen within exec_assign_value.
-                */
-               exec_assign_value(estate, target,
-                                 newarraydatum,
-                                 false,
-                                 arrayelem->arraytypoid,
-                                 arrayelem->arraytypmod);
-               break;
-           }
-
        default:
            elog(ERROR, "unrecognized dtype: %d", target->dtype);
    }
@@ -5490,8 +5294,8 @@ exec_assign_value(PLpgSQL_execstate *estate,
  *
  * The type oid, typmod, value in Datum format, and null flag are returned.
  *
- * At present this doesn't handle PLpgSQL_expr or PLpgSQL_arrayelem datums;
- * that's not needed because we never pass references to such datums to SPI.
+ * At present this doesn't handle PLpgSQL_expr datums; that's not needed
+ * because we never pass references to such datums to SPI.
  *
  * NOTE: the returned Datum points right at the stored value in the case of
  * pass-by-reference datatypes.  Generally callers should take care not to
index ac72bfa8c03d0450684aa934b8c668f3648b0730..919b968826c04bf360baa560c35db5e22150a203 100644 (file)
@@ -768,9 +768,6 @@ plpgsql_free_function_memory(PLpgSQL_function *func)
                break;
            case PLPGSQL_DTYPE_RECFIELD:
                break;
-           case PLPGSQL_DTYPE_ARRAYELEM:
-               free_expr(((PLpgSQL_arrayelem *) d)->subscript);
-               break;
            default:
                elog(ERROR, "unrecognized data type: %d", d->dtype);
        }
@@ -1704,12 +1701,6 @@ plpgsql_dumptree(PLpgSQL_function *func)
                       ((PLpgSQL_recfield *) d)->fieldname,
                       ((PLpgSQL_recfield *) d)->recparentno);
                break;
-           case PLPGSQL_DTYPE_ARRAYELEM:
-               printf("ARRAYELEM of VAR %d subscript ",
-                      ((PLpgSQL_arrayelem *) d)->arrayparentno);
-               dump_expr(((PLpgSQL_arrayelem *) d)->subscript);
-               printf("\n");
-               break;
            default:
                printf("??? unknown data type %d\n", d->dtype);
        }
index ad248bc7648c92e53aaad32cf44627ae44b110bd..0fdbaa5eab86e58c1b302e71d593b63d884a1327 100644 (file)
@@ -177,11 +177,10 @@ static    void            check_raise_parameters(PLpgSQL_stmt_raise *stmt);
 %type    decl_cursor_arglist
 %type  decl_aliasitem
 
-%type    expr_until_semi expr_until_rightbracket
+%type    expr_until_semi
 %type    expr_until_then expr_until_loop opt_expr_until_when
 %type    opt_exitcond
 
-%type   assign_var
 %type         cursor_variable
 %type   decl_cursor_arg
 %type     for_variable
@@ -1155,16 +1154,23 @@ getdiag_item :
                    }
                ;
 
-getdiag_target : assign_var
+getdiag_target : T_DATUM
                    {
-                       if ($1->dtype == PLPGSQL_DTYPE_ROW ||
-                           $1->dtype == PLPGSQL_DTYPE_REC)
+                       /*
+                        * In principle we should support a getdiag_target
+                        * that is an array element, but for now we don't, so
+                        * just throw an error if next token is '['.
+                        */
+                       if ($1.datum->dtype == PLPGSQL_DTYPE_ROW ||
+                           $1.datum->dtype == PLPGSQL_DTYPE_REC ||
+                           plpgsql_peek() == '[')
                            ereport(ERROR,
                                    (errcode(ERRCODE_SYNTAX_ERROR),
                                     errmsg("\"%s\" is not a scalar variable",
-                                           ((PLpgSQL_variable *) $1)->refname),
+                                           NameOfDatum(&($1))),
                                     parser_errposition(@1)));
-                       $$ = $1;
+                       check_assignable($1.datum, @1);
+                       $$ = $1.datum;
                    }
                | T_WORD
                    {
@@ -1178,29 +1184,6 @@ getdiag_target   : assign_var
                    }
                ;
 
-
-assign_var     : T_DATUM
-                   {
-                       check_assignable($1.datum, @1);
-                       $$ = $1.datum;
-                   }
-               | assign_var '[' expr_until_rightbracket
-                   {
-                       PLpgSQL_arrayelem   *new;
-
-                       new = palloc0(sizeof(PLpgSQL_arrayelem));
-                       new->dtype      = PLPGSQL_DTYPE_ARRAYELEM;
-                       new->subscript  = $3;
-                       new->arrayparentno = $1->dno;
-                       /* initialize cached type data to "not valid" */
-                       new->parenttypoid = InvalidOid;
-
-                       plpgsql_adddatum((PLpgSQL_datum *) new);
-
-                       $$ = (PLpgSQL_datum *) new;
-                   }
-               ;
-
 stmt_if            : K_IF expr_until_then proc_sect stmt_elsifs stmt_else K_END K_IF ';'
                    {
                        PLpgSQL_stmt_if *new;
@@ -2471,10 +2454,6 @@ expr_until_semi :
                    { $$ = read_sql_expression(';', ";"); }
                ;
 
-expr_until_rightbracket :
-                   { $$ = read_sql_expression(']', "]"); }
-               ;
-
 expr_until_then :
                    { $$ = read_sql_expression(K_THEN, "THEN"); }
                ;
@@ -3493,11 +3472,6 @@ check_assignable(PLpgSQL_datum *datum, int location)
            check_assignable(plpgsql_Datums[((PLpgSQL_recfield *) datum)->recparentno],
                             location);
            break;
-       case PLPGSQL_DTYPE_ARRAYELEM:
-           /* assignable if parent array is */
-           check_assignable(plpgsql_Datums[((PLpgSQL_arrayelem *) datum)->arrayparentno],
-                            location);
-           break;
        default:
            elog(ERROR, "unrecognized dtype: %d", datum->dtype);
            break;
index f80d023a5ad558685e155a986a4105a295e2e33b..32061e54e00a2ab740df55a1636e16f9cd09cc05 100644 (file)
@@ -64,7 +64,6 @@ typedef enum PLpgSQL_datum_type
    PLPGSQL_DTYPE_ROW,
    PLPGSQL_DTYPE_REC,
    PLPGSQL_DTYPE_RECFIELD,
-   PLPGSQL_DTYPE_ARRAYELEM,
    PLPGSQL_DTYPE_PROMISE
 } PLpgSQL_datum_type;
 
@@ -261,7 +260,7 @@ typedef struct PLpgSQL_expr
  * Generic datum array item
  *
  * PLpgSQL_datum is the common supertype for PLpgSQL_var, PLpgSQL_row,
- * PLpgSQL_rec, PLpgSQL_recfield, and PLpgSQL_arrayelem.
+ * PLpgSQL_rec, and PLpgSQL_recfield.
  */
 typedef struct PLpgSQL_datum
 {
@@ -422,30 +421,6 @@ typedef struct PLpgSQL_recfield
    /* if rectupledescid == INVALID_TUPLEDESC_IDENTIFIER, finfo isn't valid */
 } PLpgSQL_recfield;
 
-/*
- * Element of array variable
- */
-typedef struct PLpgSQL_arrayelem
-{
-   PLpgSQL_datum_type dtype;
-   int         dno;
-   /* end of PLpgSQL_datum fields */
-
-   PLpgSQL_expr *subscript;
-   int         arrayparentno;  /* dno of parent array variable */
-
-   /* Remaining fields are cached info about the array variable's type */
-   Oid         parenttypoid;   /* type of array variable; 0 if not yet set */
-   int32       parenttypmod;   /* typmod of array variable */
-   Oid         arraytypoid;    /* OID of actual array type */
-   int32       arraytypmod;    /* typmod of array (and its elements too) */
-   int16       arraytyplen;    /* typlen of array type */
-   Oid         elemtypoid;     /* OID of array element type */
-   int16       elemtyplen;     /* typlen of element type */
-   bool        elemtypbyval;   /* element type is pass-by-value? */
-   char        elemtypalign;   /* typalign of element type */
-} PLpgSQL_arrayelem;
-
 /*
  * Item in the compilers namespace tree
  */
index 0c6c9ba1f59ffa1d66b1daa5731c08591d334135..6ea169d9add54fc8c97c480065bdce94ee37d2f0 100644 (file)
@@ -4230,7 +4230,6 @@ drop function tftest(int);
 create or replace function rttest()
 returns setof int as $$
 declare rc int;
-  rca int[];
 begin
   return query values(10),(20);
   get diagnostics rc = row_count;
@@ -4239,12 +4238,11 @@ begin
   get diagnostics rc = row_count;
   raise notice '% %', found, rc;
   return query execute 'values(10),(20)';
-  -- just for fun, let's use array elements as targets
-  get diagnostics rca[1] = row_count;
-  raise notice '% %', found, rca[1];
+  get diagnostics rc = row_count;
+  raise notice '% %', found, rc;
   return query execute 'select * from (values(10),(20)) f(a) where false';
-  get diagnostics rca[2] = row_count;
-  raise notice '% %', found, rca[2];
+  get diagnostics rc = row_count;
+  raise notice '% %', found, rc;
 end;
 $$ language plpgsql;
 select * from rttest();
index 07c60c80e482aa3fc2f817e5137743a3052a74ce..781666a83a15d25683d90a07ebbc287fa79456c7 100644 (file)
@@ -3497,7 +3497,6 @@ drop function tftest(int);
 create or replace function rttest()
 returns setof int as $$
 declare rc int;
-  rca int[];
 begin
   return query values(10),(20);
   get diagnostics rc = row_count;
@@ -3506,12 +3505,11 @@ begin
   get diagnostics rc = row_count;
   raise notice '% %', found, rc;
   return query execute 'values(10),(20)';
-  -- just for fun, let's use array elements as targets
-  get diagnostics rca[1] = row_count;
-  raise notice '% %', found, rca[1];
+  get diagnostics rc = row_count;
+  raise notice '% %', found, rc;
   return query execute 'select * from (values(10),(20)) f(a) where false';
-  get diagnostics rca[2] = row_count;
-  raise notice '% %', found, rca[2];
+  get diagnostics rc = row_count;
+  raise notice '% %', found, rc;
 end;
 $$ language plpgsql;