Re: PL/PGSQL: Dynamic Record Introspection

Patch applied. Thanks.

---------------------------------------------------------------------------

uol(at)freenet(dot)de wrote:
> Bruce Momjian schrieb:
> > This patch cannot be applied. 'active_simple_exprs' is referenced but
> > not defined. I think the new variable name is 'simple_eval_estate',
> > which used to be a structure member of 'active_simple_exprs'.
> >
> > Would you please update it against current CVS and resubmit? Thanks.
> >
> Attached please find the patch against head of today, 2034 UTC
>
> Someone had removed active_simple_exprs. Actually I didn't use it
> but simply had to clean it up before calling freeplan. I removed
> the appropriate lines of code.
>
> Please let me know if anything appears to be wrong with this
> patch.
>
> Regards
> Titus
>

> *** ./doc/src/sgml/plpgsql.sgml.orig Fri Mar 10 20:10:48 2006
> --- ./doc/src/sgml/plpgsql.sgml Mon May 8 23:40:12 2006
> ***************
> *** 865,870 ****
> --- 865,919 ----
> </para>
>
> <para>
> + To obtain the values of the fields the record is made up of,
> + the record variable can be qualified with the column or field
> + name. This can be done either by literally using the column name
> + or the column name for indexing the record can be taken out of a scalar
> + variable. The syntax for this notation is Record_variable.(IndexVariable).
> + To get information about the column field names of the record,
> + a special expression exists that returns all column names as an array:
> + RecordVariable.(*) .
> + Thus, the RECORD can be viewed
> + as an associative array that allows for introspection of it's contents.
> + This feature is especially useful for writing generic triggers that
> + operate on records with unknown structure.
> + Here is an example procedure that shows column names and values
> + of the predefined record NEW in a trigger procedure:
> + <programlisting>
> +
> + CREATE OR REPLACE FUNCTION show_associative_records() RETURNS TRIGGER AS $$
> + DECLARE
> + colname TEXT;
> + colcontent TEXT;
> + colnames TEXT[];
> + coln INT4;
> + coli INT4;
> + BEGIN
> + -- obtain an array with all field names of the record
> + colnames := NEW.(*);
> + RAISE NOTICE 'All column names of test record: %', colnames;
> + -- show field names and contents of record
> + coli := 1;
> + coln := array_upper(colnames,1);
> + RAISE NOTICE 'Number of columns in NEW: %', coln;
> + FOR coli IN 1 .. coln LOOP
> + colname := colnames[coli];
> + colcontent := NEW.(colname);
> + RAISE NOTICE 'column % of NEW: %', quote_ident(colname), quote_literal(colcontent);
> + END LOOP;
> + -- Do it with a fixed field name:
> + -- will have to know the column name
> + RAISE NOTICE 'column someint of NEW: %', quote_literal(NEW.someint);
> + RETURN NULL;
> + END;
> + $$ LANGUAGE plpgsql;
> + --CREATE TABLE test_records (someint INT8, somestring TEXT);
> + --CREATE TRIGGER tr_test_record BEFORE INSERT ON test_records FOR EACH ROW EXECUTE PROCEDURE show_associative_records();
> +
> + </programlisting>
> + </para>
> +
> + <para>
> Note that <literal>RECORD</> is not a true data type, only a placeholder.
> One should also realize that when a <application>PL/pgSQL</application>
> function is declared to return type <type>record</>, this is not quite the
> *** ./src/pl/plpgsql/src/pl_comp.c.orig Tue Mar 14 23:48:23 2006
> --- ./src/pl/plpgsql/src/pl_comp.c Mon May 8 22:49:50 2006
> ***************
> *** 870,876 ****
>
> new = palloc(sizeof(PLpgSQL_recfield));
> new->dtype = PLPGSQL_DTYPE_RECFIELD;
> ! new->fieldname = pstrdup(cp[1]);
> new->recparentno = ns->itemno;
>
> plpgsql_adddatum((PLpgSQL_datum *) new);
> --- 870,877 ----
>
> new = palloc(sizeof(PLpgSQL_recfield));
> new->dtype = PLPGSQL_DTYPE_RECFIELD;
> ! new->fieldindex.fieldname = pstrdup(cp[1]);
> ! new->fieldindex_flag = RECFIELD_USE_FIELDNAME;
> new->recparentno = ns->itemno;
>
> plpgsql_adddatum((PLpgSQL_datum *) new);
> ***************
> *** 976,982 ****
>
> new = palloc(sizeof(PLpgSQL_recfield));
> new->dtype = PLPGSQL_DTYPE_RECFIELD;
> ! new->fieldname = pstrdup(cp[2]);
> new->recparentno = ns->itemno;
>
> plpgsql_adddatum((PLpgSQL_datum *) new);
> --- 977,984 ----
>
> new = palloc(sizeof(PLpgSQL_recfield));
> new->dtype = PLPGSQL_DTYPE_RECFIELD;
> ! new->fieldindex.fieldname = pstrdup(cp[2]);
> ! new->fieldindex_flag = RECFIELD_USE_FIELDNAME;
> new->recparentno = ns->itemno;
>
> plpgsql_adddatum((PLpgSQL_datum *) new);
> ***************
> *** 1423,1428 ****
> --- 1425,1556 ----
> MemoryContextSwitchTo(oldCxt);
> return T_DTYPE;
> }
> +
> + /* ----------
> + * plpgsql_parse_recindex
> + * lookup associative index into record
> + * ----------
> + */
> + int
> + plpgsql_parse_recindex(char *word)
> + {
> + PLpgSQL_nsitem *ns1, *ns2;
> + char *cp[2];
> + int ret = T_ERROR;
> + char *fieldvar;
> + int fl;
> +
> + /* Do case conversion and word separation */
> + plpgsql_convert_ident(word, cp, 2);
> + Assert(cp[1] != NULL);
> +
> + /* cleanup the "(identifier)" string to "identifier" */
> + fieldvar = cp[1];
> + Assert(*fieldvar == '(');
> + ++fieldvar; /* get rid of ( */
> +
> + fl = strlen(fieldvar);
> + Assert(fieldvar[fl-1] == ')');
> + fieldvar[fl-1] = 0; /* get rid of ) */
> +
> + /*
> + * Lookup the first word
> + */
> + ns1 = plpgsql_ns_lookup(cp[0], NULL);
> + if ( ns1 == NULL )
> + {
> + pfree(cp[0]);
> + pfree(cp[1]);
> + return T_ERROR;
> + }
> +
> + ns2 = plpgsql_ns_lookup(fieldvar, NULL);
> + pfree(cp[0]);
> + pfree(cp[1]);
> + if ( ns2 == NULL ) /* name lookup failed */
> + return T_ERROR;
> +
> + switch (ns1->itemtype)
> + {
> + case PLPGSQL_NSTYPE_REC:
> + {
> + /*
> + * First word is a record name, so second word must be an
> + * variable holding the field name in this record.
> + */
> + if ( ns2->itemtype == PLPGSQL_NSTYPE_VAR ) {
> + PLpgSQL_recfield *new;
> +
> + new = palloc(sizeof(PLpgSQL_recfield));
> + new->dtype = PLPGSQL_DTYPE_RECFIELD;
> + new->fieldindex.indexvar_no = ns2->itemno;
> + new->fieldindex_flag = RECFIELD_USE_INDEX_VAR;
> + new->recparentno = ns1->itemno;
> +
> + plpgsql_adddatum((PLpgSQL_datum *) new);
> +
> + plpgsql_yylval.scalar = (PLpgSQL_datum *) new;
> + ret = T_SCALAR;
> + }
> + break;
> + }
> + default:
> + break;
> + }
> + return ret;
> + }
> +
> +
> + /* ----------
> + * plpgsql_parse_recfieldnames
> + * create fieldnames of a record
> + * ----------
> + */
> + int
> + plpgsql_parse_recfieldnames(char *word)
> + {
> + PLpgSQL_nsitem *ns1;
> + char *cp[2];
> + int ret = T_ERROR;
> +
> + /* Do case conversion and word separation */
> + plpgsql_convert_ident(word, cp, 2);
> +
> + /*
> + * Lookup the first word
> + */
> + ns1 = plpgsql_ns_lookup(cp[0], NULL);
> + if ( ns1 == NULL )
> + {
> + pfree(cp[0]);
> + pfree(cp[1]);
> + return T_ERROR;
> + }
> +
> + pfree(cp[0]);
> + pfree(cp[1]);
> +
> + switch (ns1->itemtype)
> + {
> + case PLPGSQL_NSTYPE_REC:
> + {
> + PLpgSQL_recfieldproperties *new;
> +
> + new = palloc(sizeof(PLpgSQL_recfieldproperties));
> + new->dtype = PLPGSQL_DTYPE_RECFIELDNAMES;
> + new->recparentno = ns1->itemno;
> + new->save_fieldnames = NULL;
> + plpgsql_adddatum((PLpgSQL_datum *) new);
> + plpgsql_yylval.scalar = (PLpgSQL_datum *) new;
> + ret = T_SCALAR; /* ??? */
> + break;
> + }
> + default:
> + break;
> + }
> + return ret;
> + }
> +
>
> /*
> * plpgsql_build_variable - build a datum-array entry of a given
> *** ./src/pl/plpgsql/src/pl_exec.c.orig Sat Apr 22 03:26:01 2006
> --- ./src/pl/plpgsql/src/pl_exec.c Mon May 8 23:53:25 2006
> ***************
> *** 726,732 ****
> case PLPGSQL_DTYPE_RECFIELD:
> case PLPGSQL_DTYPE_ARRAYELEM:
> case PLPGSQL_DTYPE_TRIGARG:
> !
> /*
> * These datum records are read-only at runtime, so no need to
> * copy them
> --- 726,732 ----
> case PLPGSQL_DTYPE_RECFIELD:
> case PLPGSQL_DTYPE_ARRAYELEM:
> case PLPGSQL_DTYPE_TRIGARG:
> ! case PLPGSQL_DTYPE_RECFIELDNAMES:
> /*
> * These datum records are read-only at runtime, so no need to
> * copy them
> ***************
> *** 836,841 ****
> --- 836,842 ----
>
> case PLPGSQL_DTYPE_RECFIELD:
> case PLPGSQL_DTYPE_ARRAYELEM:
> + case PLPGSQL_DTYPE_RECFIELDNAMES:
> break;
>
> default:
> ***************
> *** 2164,2169 ****
> --- 2165,2172 ----
> static void
> exec_eval_cleanup(PLpgSQL_execstate *estate)
> {
> + int i;
> + ArrayType *a;
> /* Clear result of a full SPI_execute */
> if (estate->eval_tuptable != NULL)
> SPI_freetuptable(estate->eval_tuptable);
> ***************
> *** 2172,2177 ****
> --- 2175,2188 ----
> /* Clear result of exec_eval_simple_expr (but keep the econtext) */
> if (estate->eval_econtext != NULL)
> ResetExprContext(estate->eval_econtext);
> + for ( i = 0; i < estate->ndatums; ++i ) {
> + if ( estate->datums[i]->dtype == PLPGSQL_DTYPE_RECFIELDNAMES ) {
> + a = ((PLpgSQL_recfieldproperties *)(estate->datums[i]))->save_fieldnames;
> + if ( a )
> + pfree(a);
> + ((PLpgSQL_recfieldproperties *)(estate->datums[i]))->save_fieldnames = NULL;
> + }
> + }
> }
>
>
> ***************
> *** 3141,3147 ****
> */
> PLpgSQL_recfield *recfield = (PLpgSQL_recfield *) target;
> PLpgSQL_rec *rec;
> ! int fno;
> HeapTuple newtup;
> int natts;
> int i;
> --- 3152,3158 ----
> */
> PLpgSQL_recfield *recfield = (PLpgSQL_recfield *) target;
> PLpgSQL_rec *rec;
> ! int fno = 0;
> HeapTuple newtup;
> int natts;
> int i;
> ***************
> *** 3170,3181 ****
> * Get the number of the records field to change and the
> * number of attributes in the tuple.
> */
> ! fno = SPI_fnumber(rec->tupdesc, recfield->fieldname);
> ! if (fno == SPI_ERROR_NOATTRIBUTE)
> ereport(ERROR,
> ! (errcode(ERRCODE_UNDEFINED_COLUMN),
> ! errmsg("record \"%s\" has no field \"%s\"",
> ! rec->refname, recfield->fieldname)));
> fno--;
> natts = rec->tupdesc->natts;
>
> --- 3181,3215 ----
> * Get the number of the records field to change and the
> * number of attributes in the tuple.
> */
> ! if ( recfield->fieldindex_flag == RECFIELD_USE_FIELDNAME ) {
> ! fno = SPI_fnumber(rec->tupdesc, recfield->fieldindex.fieldname);
> ! if (fno == SPI_ERROR_NOATTRIBUTE)
> ! ereport(ERROR,
> ! (errcode(ERRCODE_UNDEFINED_COLUMN),
> ! errmsg("record \"%s\" has no field \"%s\"",
> ! rec->refname, recfield->fieldindex.fieldname)));
> ! }
> ! else if ( recfield->fieldindex_flag == RECFIELD_USE_INDEX_VAR ) {
> ! PLpgSQL_var * idxvar = (PLpgSQL_var *) (estate->datums[recfield->fieldindex.indexvar_no]);
> ! char * fname = convert_value_to_string(idxvar->value, idxvar->datatype->typoid);
> ! if ( fname == NULL )
> ! ereport(ERROR,
> ! (errcode(ERRCODE_UNDEFINED_COLUMN),
> ! errmsg("record \"%s\": cannot evaluate variable to record index string",
> ! rec->refname)));
> ! fno = SPI_fnumber(rec->tupdesc, fname);
> ! pfree(fname);
> ! if (fno == SPI_ERROR_NOATTRIBUTE)
> ! ereport(ERROR,
> ! (errcode(ERRCODE_UNDEFINED_COLUMN),
> ! errmsg("record \"%s\" has no field \"%s\"",
> ! rec->refname, fname)));
> ! }
> ! else
> ereport(ERROR,
> ! (errcode(ERRCODE_UNDEFINED_COLUMN),
> ! errmsg("record \"%s\": internal error",
> ! rec->refname)));
> fno--;
> natts = rec->tupdesc->natts;
>
> ***************
> *** 3495,3501 ****
> {
> PLpgSQL_recfield *recfield = (PLpgSQL_recfield *) datum;
> PLpgSQL_rec *rec;
> ! int fno;
>
> rec = (PLpgSQL_rec *) (estate->datums[recfield->recparentno]);
> if (!HeapTupleIsValid(rec->tup))
> --- 3529,3535 ----
> {
> PLpgSQL_recfield *recfield = (PLpgSQL_recfield *) datum;
> PLpgSQL_rec *rec;
> ! int fno = 0;
>
> rec = (PLpgSQL_rec *) (estate->datums[recfield->recparentno]);
> if (!HeapTupleIsValid(rec->tup))
> ***************
> *** 3504,3525 ****
> errmsg("record \"%s\" is not assigned yet",
> rec->refname),
> errdetail("The tuple structure of a not-yet-assigned record is indeterminate.")));
> ! fno = SPI_fnumber(rec->tupdesc, recfield->fieldname);
> ! if (fno == SPI_ERROR_NOATTRIBUTE)
> ! ereport(ERROR,
> ! (errcode(ERRCODE_UNDEFINED_COLUMN),
> ! errmsg("record \"%s\" has no field \"%s\"",
> ! rec->refname, recfield->fieldname)));
> ! *typeid = SPI_gettypeid(rec->tupdesc, fno);
> ! *value = SPI_getbinval(rec->tup, rec->tupdesc, fno, isnull);
> ! if (expectedtypeid != InvalidOid && expectedtypeid != *typeid)
> ! ereport(ERROR,
> ! (errcode(ERRCODE_DATATYPE_MISMATCH),
> ! errmsg("type of \"%s.%s\" does not match that when preparing the plan",
> ! rec->refname, recfield->fieldname)));
> ! break;
> ! }
> !
> case PLPGSQL_DTYPE_TRIGARG:
> {
> PLpgSQL_trigarg *trigarg = (PLpgSQL_trigarg *) datum;
> --- 3538,3662 ----
> errmsg("record \"%s\" is not assigned yet",
> rec->refname),
> errdetail("The tuple structure of a not-yet-assigned record is indeterminate.")));
> ! if ( recfield->fieldindex_flag == RECFIELD_USE_FIELDNAME ) {
> ! fno = SPI_fnumber(rec->tupdesc, recfield->fieldindex.fieldname);
> ! if (fno == SPI_ERROR_NOATTRIBUTE)
> ! ereport(ERROR,
> ! (errcode(ERRCODE_UNDEFINED_COLUMN),
> ! errmsg("record \"%s\" has no field \"%s\"",
> ! rec->refname, recfield->fieldindex.fieldname)));
> ! }
> ! else if ( recfield->fieldindex_flag == RECFIELD_USE_INDEX_VAR ) {
> ! PLpgSQL_var * idxvar = (PLpgSQL_var *) (estate->datums[recfield->fieldindex.indexvar_no]);
> ! char * fname = convert_value_to_string(idxvar->value, idxvar->datatype->typoid);
> ! if ( fname == NULL )
> ! ereport(ERROR,
> ! (errcode(ERRCODE_UNDEFINED_COLUMN),
> ! errmsg("record \"%s\": cannot evaluate variable to record index string",
> ! rec->refname)));
> ! fno = SPI_fnumber(rec->tupdesc, fname);
> ! pfree(fname);
> ! if (fno == SPI_ERROR_NOATTRIBUTE)
> ! ereport(ERROR,
> ! (errcode(ERRCODE_UNDEFINED_COLUMN),
> ! errmsg("record \"%s\" has no field \"%s\"",
> ! rec->refname, fname)));
> ! }
> ! else
> ! ereport(ERROR,
> ! (errcode(ERRCODE_UNDEFINED_COLUMN),
> ! errmsg("record \"%s\": internal error",
> ! rec->refname)));
> !
> ! /* Do not allow typeids to become "narrowed" by InvalidOids
> ! causing specialized typeids from the tuple restricting the destination */
> ! if ( expectedtypeid != InvalidOid && expectedtypeid != SPI_gettypeid(rec->tupdesc, fno) ) {
> ! Datum cval = SPI_getbinval(rec->tup, rec->tupdesc, fno, isnull);
> ! cval = exec_simple_cast_value(cval,
> ! SPI_gettypeid(rec->tupdesc, fno),
> ! expectedtypeid,
> ! -1,
> ! *isnull);
> !
> ! *value = cval;
> ! *typeid = expectedtypeid;
> ! /* ereport(ERROR,
> ! (errcode(ERRCODE_DATATYPE_MISMATCH),
> ! errmsg("type of \"%s\" does not match that when preparing the plan",
> ! rec->refname)));
> ! */
> ! }
> ! else { /* expected typeid matches */
> ! *value = SPI_getbinval(rec->tup, rec->tupdesc, fno, isnull);
> ! *typeid = SPI_gettypeid(rec->tupdesc, fno);
> ! }
> ! break;
> ! }
> !
> ! case PLPGSQL_DTYPE_RECFIELDNAMES:
> ! /* Construct array datum from record field names */
> ! {
> ! Oid arraytypeid,
> ! arrayelemtypeid = TEXTOID;
> ! int16 arraytyplen,
> ! elemtyplen;
> ! bool elemtypbyval;
> ! char elemtypalign;
> ! ArrayType *arrayval;
> ! PLpgSQL_recfieldproperties * recfp = (PLpgSQL_recfieldproperties *) datum;
> ! PLpgSQL_rec *rec = (PLpgSQL_rec *) (estate->datums[recfp->recparentno]);
> ! int fc, tfc = 0;
> ! Datum *arrayelems;
> ! char *fieldname;
> !
> ! if (!HeapTupleIsValid(rec->tup))
> ! ereport(ERROR,
> ! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> ! errmsg("record \"%s\" is not assigned yet",
> ! rec->refname),
> ! errdetail("The tuple structure of a not-yet-assigned record is indeterminate.")));
> ! arrayelems = palloc(sizeof(Datum) * rec->tupdesc->natts);
> ! arraytypeid = get_array_type(arrayelemtypeid);
> ! arraytyplen = get_typlen(arraytypeid);
> ! get_typlenbyvalalign(arrayelemtypeid,
> ! &elemtyplen,
> ! &elemtypbyval,
> ! &elemtypalign);
> !
> ! if ( expectedtypeid != InvalidOid && expectedtypeid != arraytypeid )
> ! ereport(ERROR,
> ! (errcode(ERRCODE_DATATYPE_MISMATCH),
> ! errmsg("type of \"%s\" does not match array type when preparing the plan",
> ! rec->refname)));
> ! for ( fc = 0; fc < rec->tupdesc->natts; ++fc ) {
> ! fieldname = SPI_fname(rec->tupdesc, fc+1);
> ! if ( fieldname ) {
> ! arrayelems[fc] = DirectFunctionCall1(textin, CStringGetDatum(fieldname));
> ! pfree(fieldname);
> ! ++tfc;
> ! }
> ! }
> ! arrayval = construct_array(arrayelems, tfc,
> ! arrayelemtypeid,
> ! elemtyplen,
> ! elemtypbyval,
> ! elemtypalign);
> !
> !
> ! /* construct_array copies data; free temp elem array */
> ! for ( fc = 0; fc < tfc; ++fc )
> ! pfree(DatumGetPointer(arrayelems[fc]));
> ! pfree(arrayelems);
> ! *value = PointerGetDatum(arrayval);
> ! *typeid = arraytypeid;
> ! *isnull = false;
> ! /* need to save the pointer because otherwise it does not get freed */
> ! if ( recfp->save_fieldnames )
> ! pfree(recfp->save_fieldnames);
> ! recfp->save_fieldnames = arrayval;
> ! break;
> ! }
> !
> case PLPGSQL_DTYPE_TRIGARG:
> {
> PLpgSQL_trigarg *trigarg = (PLpgSQL_trigarg *) datum;
> ***************
> *** 3617,3623 ****
> */
> if (expr->plan == NULL)
> exec_prepare_plan(estate, expr);
> !
> /*
> * If this is a simple expression, bypass SPI and use the executor
> * directly
> --- 3754,3782 ----
> */
> if (expr->plan == NULL)
> exec_prepare_plan(estate, expr);
> ! else {
> ! /*
> ! * check for any subexpressions with varying type in the expression
> ! * currently (July 05), this is a record field of a record indexed by a variable
> ! */
> ! int i;
> ! PLpgSQL_datum *d;
> ! PLpgSQL_recfield *rf;
> ! for ( i = 0; i < expr->nparams; ++i ) {
> ! d = estate->datums[expr->params[i]];
> ! if ( d->dtype == PLPGSQL_DTYPE_RECFIELD ) {
> ! rf = (PLpgSQL_recfield *)d;
> ! if ( rf->fieldindex_flag == RECFIELD_USE_INDEX_VAR )
> ! break;
> ! }
> ! }
> ! if ( i < expr->nparams ) { /* expr may change it's type */
> ! /* now discard the plan and get new one */
> ! SPI_freeplan(expr->plan);
> ! expr->plan = NULL;
> ! exec_prepare_plan(estate, expr);
> ! }
> ! }
> /*
> * If this is a simple expression, bypass SPI and use the executor
> * directly
> *** ./src/pl/plpgsql/src/pl_funcs.c.orig Thu Mar 9 22:29:37 2006
> --- ./src/pl/plpgsql/src/pl_funcs.c Mon May 8 22:49:50 2006
> ***************
> *** 1044,1052 ****
> printf("REC %s\n", ((PLpgSQL_rec *) d)->refname);
> break;
> case PLPGSQL_DTYPE_RECFIELD:
> ! printf("RECFIELD %-16s of REC %d\n",
> ! ((PLpgSQL_recfield *) d)->fieldname,
> ! ((PLpgSQL_recfield *) d)->recparentno);
> break;
> case PLPGSQL_DTYPE_ARRAYELEM:
> printf("ARRAYELEM of VAR %d subscript ",
> --- 1044,1056 ----
> printf("REC %s\n", ((PLpgSQL_rec *) d)->refname);
> break;
> case PLPGSQL_DTYPE_RECFIELD:
> ! if ( ((PLpgSQL_recfield *) d)->fieldindex_flag == RECFIELD_USE_FIELDNAME )
> ! printf("RECFIELD %-16s of REC %d\n",
> ! ((PLpgSQL_recfield *) d)->fieldindex.fieldname,
> ! ((PLpgSQL_recfield *) d)->recparentno);
> ! else
> ! printf("RECFIELD Variable of REC %d\n",
> ! ((PLpgSQL_recfield *) d)->recparentno);
> break;
> case PLPGSQL_DTYPE_ARRAYELEM:
> printf("ARRAYELEM of VAR %d subscript ",
> *** ./src/pl/plpgsql/src/plpgsql.h.orig Thu Mar 9 22:29:38 2006
> --- ./src/pl/plpgsql/src/plpgsql.h Mon May 8 22:49:50 2006
> ***************
> *** 52,58 ****
> PLPGSQL_DTYPE_RECFIELD,
> PLPGSQL_DTYPE_ARRAYELEM,
> PLPGSQL_DTYPE_EXPR,
> ! PLPGSQL_DTYPE_TRIGARG
> };
>
> /* ----------
> --- 52,59 ----
> PLPGSQL_DTYPE_RECFIELD,
> PLPGSQL_DTYPE_ARRAYELEM,
> PLPGSQL_DTYPE_EXPR,
> ! PLPGSQL_DTYPE_TRIGARG,
> ! PLPGSQL_DTYPE_RECFIELDNAMES
> };
>
> /* ----------
> ***************
> *** 251,260 ****
> { /* Field in record */
> int dtype;
> int rfno;
> ! char *fieldname;
> int recparentno; /* dno of parent record */
> } PLpgSQL_recfield;
>
>
> typedef struct
> { /* Element of array variable */
> --- 252,276 ----
> { /* Field in record */
> int dtype;
> int rfno;
> ! union {
> ! char *fieldname;
> ! int indexvar_no; /* dno of variable holding index string */
> ! } fieldindex;
> ! enum {
> ! RECFIELD_USE_FIELDNAME,
> ! RECFIELD_USE_INDEX_VAR,
> ! } fieldindex_flag;
> int recparentno; /* dno of parent record */
> } PLpgSQL_recfield;
>
> + typedef struct
> + { /* Field in record */
> + int dtype;
> + int rfno;
> + int recparentno; /* dno of parent record */
> + ArrayType * save_fieldnames;
> + } PLpgSQL_recfieldproperties;
> +
>
> typedef struct
> { /* Element of array variable */
> ***************
> *** 659,664 ****
> --- 675,682 ----
> extern int plpgsql_parse_tripwordtype(char *word);
> extern int plpgsql_parse_wordrowtype(char *word);
> extern int plpgsql_parse_dblwordrowtype(char *word);
> + extern int plpgsql_parse_recfieldnames(char *word);
> + extern int plpgsql_parse_recindex(char *word);
> extern PLpgSQL_type *plpgsql_parse_datatype(const char *string);
> extern PLpgSQL_type *plpgsql_build_datatype(Oid typeOid, int32 typmod);
> extern PLpgSQL_variable *plpgsql_build_variable(const char *refname, int lineno,
> *** ./src/pl/plpgsql/src/scan.l.orig Thu Mar 9 22:29:38 2006
> --- ./src/pl/plpgsql/src/scan.l Mon May 8 22:49:50 2006
> ***************
> *** 222,227 ****
> --- 222,233 ----
> {param}{space}*\.{space}*{identifier}{space}*%ROWTYPE {
> plpgsql_error_lineno = plpgsql_scanner_lineno();
> return plpgsql_parse_dblwordrowtype(yytext); }
> + {identifier}{space}*\.\(\*\) {
> + plpgsql_error_lineno = plpgsql_scanner_lineno();
> + return plpgsql_parse_recfieldnames(yytext); }
> + {identifier}{space}*\.\({identifier}\) {
> + plpgsql_error_lineno = plpgsql_scanner_lineno();
> + return plpgsql_parse_recindex(yytext); }
>
> {digit}+ { return T_NUMBER; }
>
> *** ./src/test/regress/expected/plpgsql.out.orig Thu Mar 23 05:22:36 2006
> --- ./src/test/regress/expected/plpgsql.out Mon May 8 22:49:50 2006
> ***************
> *** 2725,2730 ****
> --- 2725,2768 ----
> $$ language plpgsql;
> ERROR: end label "outer_label" specified for unlabelled block
> CONTEXT: compile of PL/pgSQL function "end_label4" near line 5
> + -- check introspective records
> + create table ritest (i INT4, t TEXT);
> + insert into ritest (i, t) VALUES (1, 'sometext');
> + create function test_record() returns void as $$
> + declare
> + cname text;
> + tval text;
> + ival int4;
> + tval2 text;
> + ival2 int4;
> + columns text[];
> + r RECORD;
> + begin
> + SELECT INTO r * FROM ritest WHERE i = 1;
> + ival := r.i;
> + tval := r.t;
> + RAISE NOTICE 'ival=%, tval=%', ival, tval;
> + cname := 'i';
> + ival2 := r.(cname);
> + cname :='t';
> + tval2 := r.(cname);
> + RAISE NOTICE 'ival2=%, tval2=%', ival2, tval2;
> + columns := r.(*);
> + RAISE NOTICE 'fieldnames=%', columns;
> + RETURN;
> + end;
> + $$ language plpgsql;
> + select test_record();
> + NOTICE: ival=1, tval=sometext
> + NOTICE: ival2=1, tval2=sometext
> + NOTICE: fieldnames={i,t}
> + test_record
> + -------------
> +
> + (1 row)
> +
> + drop table ritest;
> + drop function test_record();
> -- using list of scalars in fori and fore stmts
> create function for_vect() returns void as $proc$
> <<lbl>>declare a integer; b varchar; c varchar; r record;
> *** ./src/test/regress/sql/plpgsql.sql.orig Thu Mar 23 05:22:37 2006
> --- ./src/test/regress/sql/plpgsql.sql Mon May 8 22:49:50 2006
> ***************
> *** 2281,2286 ****
> --- 2281,2318 ----
> end;
> $$ language plpgsql;
>
> + -- check introspective records
> + create table ritest (i INT4, t TEXT);
> + insert into ritest (i, t) VALUES (1, 'sometext');
> + create function test_record() returns void as $$
> + declare
> + cname text;
> + tval text;
> + ival int4;
> + tval2 text;
> + ival2 int4;
> + columns text[];
> + r RECORD;
> + begin
> + SELECT INTO r * FROM ritest WHERE i = 1;
> + ival := r.i;
> + tval := r.t;
> + RAISE NOTICE 'ival=%, tval=%', ival, tval;
> + cname := 'i';
> + ival2 := r.(cname);
> + cname :='t';
> + tval2 := r.(cname);
> + RAISE NOTICE 'ival2=%, tval2=%', ival2, tval2;
> + columns := r.(*);
> + RAISE NOTICE 'fieldnames=%', columns;
> + RETURN;
> + end;
> + $$ language plpgsql;
> + select test_record();
> + drop table ritest;
> + drop function test_record();
> +
> +
> -- using list of scalars in fori and fore stmts
> create function for_vect() returns void as $proc$
> <<lbl>>declare a integer; b varchar; c varchar; r record;

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Patch applied. Thanks.

I wish to object to this patch; it's poorly designed, poorly coded, and
badly documented. The philosophy seems to be "I want this feature
and I don't care what I have to do to the semantics and performance
of plpgsql to get it". In particular I don't like the bits that go

/* Do not allow typeids to become "narrowed" by InvalidOids
causing specialized typeids from the tuple restricting the destination */

The incoherence of the comment is a good guide to how poorly thought out
the code is. These bits are positioned so that they change the language
semantics even when not using a dynamic field reference; what's more,
they don't make any sense when you *are* using a dynamic field
reference, because you need to keep the actual field type not try
(probably vainly) to cast it to whatever the previous field's type was.

I also dislike the loop added to exec_eval_cleanup(), which will impose
a significant performance penalty on every single expression evaluation
done by any plpgsql function, whether it's ever heard of dynamic field
references or not. I doubt it's even necessary; I think the author
doesn't understand plpgsql's memory allocation strategy very well.

Lastly, and this is maybe a judgement call, I find the changes to
PLpgSQL_recfield fairly ugly. It'd be better to make a separate
datum type PLpgSQL_dynamic_recfield or some such.

This needs to be bounced back for rework. It looks like a first draft
that should have been rewritten once the author had learned enough to
make it sort-of-work.

regards, tom lane

OK, patch reverted, and attached. Would the author please revise?
Thanks.

It seems like a cool feature.

---------------------------------------------------------------------------

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Patch applied. Thanks.
>
> I wish to object to this patch; it's poorly designed, poorly coded, and
> badly documented. The philosophy seems to be "I want this feature
> and I don't care what I have to do to the semantics and performance
> of plpgsql to get it". In particular I don't like the bits that go
>
> /* Do not allow typeids to become "narrowed" by InvalidOids
> causing specialized typeids from the tuple restricting the destination */
>
> The incoherence of the comment is a good guide to how poorly thought out
> the code is. These bits are positioned so that they change the language
> semantics even when not using a dynamic field reference; what's more,
> they don't make any sense when you *are* using a dynamic field
> reference, because you need to keep the actual field type not try
> (probably vainly) to cast it to whatever the previous field's type was.
>
> I also dislike the loop added to exec_eval_cleanup(), which will impose
> a significant performance penalty on every single expression evaluation
> done by any plpgsql function, whether it's ever heard of dynamic field
> references or not. I doubt it's even necessary; I think the author
> doesn't understand plpgsql's memory allocation strategy very well.
>
> Lastly, and this is maybe a judgement call, I find the changes to
> PLpgSQL_recfield fairly ugly. It'd be better to make a separate
> datum type PLpgSQL_dynamic_recfield or some such.
>
> This needs to be bounced back for rework. It looks like a first draft
> that should have been rewritten once the author had learned enough to
> make it sort-of-work.
>
> regards, tom lane
>

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> It seems like a cool feature.

The fundamental problem with it is that plpgsql isn't designed to deal
with dynamically changing data types. The patch as submitted contained
some hacks that sort of dealt with this in some cases (I don't think it
covered them all), but really some serious thought is going to have to
go into making that work. It'd be good to do, because the existing
RECORD-variable feature already creates cases where it's needed; but
AFAICS it's not something that can be fixed off-the-cuff.

regards, tom lane

No, the author would not.

Tom did participate in the discussion at the time
when the original patch was developed.

The patch was now hanging around more than 9 months,
and it was already accepted during the
original discussion by Neil Conway. Please figure out
by yourselves who might be the one to really, really, finally
accept the patch.

A word about the discussion style in general:
I personally don't like general objections on the code
*after* I - again - spent some hours of work to comply to Bruce's
wish to rewrite the patch for HEAD. After all, Tom, you
don't pay me?
And I don't like quoting my comments while at the same
time complaining about "poorly coded" software. This simply is nonsense.
The other attributes you gave to the patch are simply
bad discussion style and at the same time nonsense as well:
The patch tries to *minimize* changes to plpgsql and does not
deal with "fundamental" problems of the interpreter.
For this reason, of course the patch is more or less a hack.

Apparently, the RECORD construct without that patch
does not really make much sense; ROWTYPE would suffice.
This is why the patch is a "cool feature".

If now Tom or the community (being identical?)
in general have the feeling
to start some kind of complete or otherwise "fundamental"
rewrite of plpgsql to handle constructs like the one implemented by me,
I suggest someone else should do that. I do not have
the time to rewrite plpgsql or to participate
in this kind of discussions.

Regards
Titus

Bruce Momjian wrote:
> OK, patch reverted, and attached. Would the author please revise?
> Thanks.
>
> It seems like a cool feature.
>
> ---------------------------------------------------------------------------
>
> Tom Lane wrote:
>> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
>>> Patch applied. Thanks.
>> I wish to object to this patch; it's poorly designed, poorly coded, and
>> badly documented. The philosophy seems to be "I want this feature
>> and I don't care what I have to do to the semantics and performance
>> of plpgsql to get it". In particular I don't like the bits that go
>>
>> /* Do not allow typeids to become "narrowed" by InvalidOids
>> causing specialized typeids from the tuple restricting the destination */
>>
>> The incoherence of the comment is a good guide to how poorly thought out
>> the code is. These bits are positioned so that they change the language
>> semantics even when not using a dynamic field reference; what's more,
>> they don't make any sense when you *are* using a dynamic field
>> reference, because you need to keep the actual field type not try
>> (probably vainly) to cast it to whatever the previous field's type was.
>>
>> I also dislike the loop added to exec_eval_cleanup(), which will impose
>> a significant performance penalty on every single expression evaluation
>> done by any plpgsql function, whether it's ever heard of dynamic field
>> references or not. I doubt it's even necessary; I think the author
>> doesn't understand plpgsql's memory allocation strategy very well.
>>
>> Lastly, and this is maybe a judgement call, I find the changes to
>> PLpgSQL_recfield fairly ugly. It'd be better to make a separate
>> datum type PLpgSQL_dynamic_recfield or some such.
>>
>> This needs to be bounced back for rework. It looks like a first draft
>> that should have been rewritten once the author had learned enough to
>> make it sort-of-work.
>>
>> regards, tom lane
>>
>

uol(at)freenet(dot)de said:
> The patch was now hanging around more than 9 months,
> and it was already accepted during the
> original discussion by Neil Conway.

Nonsense. I provided some specific code cleanup suggestions and said that
I "didn't object to the general feature". That should not be taken as an
endorsement of everything that I didn't specifically comment on.

-Neil

Tom Lane schrieb:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
>> It seems like a cool feature.
>
> The fundamental problem with it is that plpgsql isn't designed to deal
> with dynamically changing data types. The patch as submitted contained
> some hacks that sort of dealt with this in some cases (I don't think it
> covered them all), but really some serious thought is going to have to
> go into making that work. It'd be good to do, because the existing
> RECORD-variable feature already creates cases where it's needed; but
> AFAICS it's not something that can be fixed off-the-cuff.
Tom,

I took some time to reinvestigate my patch.

At a closer look on the code you will find the
free-reprepare statements for the plan being the
only relevant part where changing
types are dealt with besides the cited comment with english of
- I have to admit it - questionable quality.
The "fundamental problem" stated above as well as the patch
code to free and reprepare the plan when encountering
expressions with types changing after stmt preparation
(up to now the newly introduced construct RECORD.(variable))
leads to the fact that the if part where the cited bogus comment is
placed should never get executed, thus no however vainly
casting is ever necessary. Maybe you meant this with your mail?
You can safely change back that if to the previous
version and leave the else part as it is. The complete
if clause as sent in by me is superfluous:
I cannot see any cases when this if would ever get executed
besides usage of a RECORD variable indexed by "hard coded"
column name in a loop where you fill
the RECORD with different row types that happen to have the
same column name. Personally speaking, I'd consider
this as dangerous nonsense code.
My change simply would replace the original error message of
different types between preparation and execution time
with a different error message when trying to assign incompatible
types, thus causing the cast to fail.
The change of the if clause was introduced before I
was fully aware of the "fundamental problem" and I forgot to
remove it afterwards.

However, I apologize for the inconvenience caused by this if clause,
it's bogus comment, and for my insolence to send in a patch with 10
lines of unimportant bogus code before having learned enough.

If the freeing of the array with record field names is unnecessary
and causes any irritation to you, simply remove it.
I'm used to free memory after usage; if postgres or plpgsql
handles that automatically or otherwise differently in that case,
change the patch.
I'd rather get accused for slowing down things than
silently eating up memory. And I'm sure *you* know if it's safe
to drop that pointer or whatever else to do.

Being ugly or not, the changes to PLpgSQL_recfield were deliberatly
chosen over a separate struct to be sure that records are
always dealt with in the same way regardless of the field indexing
method, to avoid code duplication between these two indexing cases
(considering that the code dealing with the actual record field is more
important to hold in common than to distinguish the two indexing
cases with separate structures),
and to make sure to catch all cases where this structure is dealt with
by simply provoking a compilation error because of the moved struct
member.

So the bits do not "change language semantics", just because a
bad comment and a never executed if clause is left in the code.
Rest assured: the code (except the if clause) is in use on several
servers for a year now and the rest of our now quite substantial amount
of plpgsql code not using the newly introduced RECORD indexing construct
remains very compatible with the interpreter without my patch,
and the usual regression test executes without problems
on an interpreter with that patch.
Thus I dare to say that this patch more than "sort-of" works,
even if - as I always would be the first to admit -
my position on the apparently steep learning curve
of pgsql internals is still quite near the origin of the coordinate
system.

I would suggest you change back the bogus if clause and
- if you, as I presume, know better than me what happens -
drop or change the freeing of the memory of the record field names array
and apply the patch. For other changes I'd suggest you should apologize
for your mail in case you would expect me to implement them
or please find someone else to bear with you.

Titus

I am not sure where to go on this patch. I agree there was a long delay
in getting you feedback on it, and some of the feedback was harsh.

However, the patch does need more work to get into PostgreSQL because it
has to work in all cases. It is a neat feature, for sure.

At this point, I have put the three email threads on the TODO list so if
you or someone else wants to work on it, we have references to all the
relevant code and comments.

---------------------------------------------------------------------------

uol(at)freenet(dot)de wrote:
> Tom Lane schrieb:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> >> It seems like a cool feature.
> >
> > The fundamental problem with it is that plpgsql isn't designed to deal
> > with dynamically changing data types. The patch as submitted contained
> > some hacks that sort of dealt with this in some cases (I don't think it
> > covered them all), but really some serious thought is going to have to
> > go into making that work. It'd be good to do, because the existing
> > RECORD-variable feature already creates cases where it's needed; but
> > AFAICS it's not something that can be fixed off-the-cuff.
> Tom,
>
> I took some time to reinvestigate my patch.
>
> At a closer look on the code you will find the
> free-reprepare statements for the plan being the
> only relevant part where changing
> types are dealt with besides the cited comment with english of
> - I have to admit it - questionable quality.
> The "fundamental problem" stated above as well as the patch
> code to free and reprepare the plan when encountering
> expressions with types changing after stmt preparation
> (up to now the newly introduced construct RECORD.(variable))
> leads to the fact that the if part where the cited bogus comment is
> placed should never get executed, thus no however vainly
> casting is ever necessary. Maybe you meant this with your mail?
> You can safely change back that if to the previous
> version and leave the else part as it is. The complete
> if clause as sent in by me is superfluous:
> I cannot see any cases when this if would ever get executed
> besides usage of a RECORD variable indexed by "hard coded"
> column name in a loop where you fill
> the RECORD with different row types that happen to have the
> same column name. Personally speaking, I'd consider
> this as dangerous nonsense code.
> My change simply would replace the original error message of
> different types between preparation and execution time
> with a different error message when trying to assign incompatible
> types, thus causing the cast to fail.
> The change of the if clause was introduced before I
> was fully aware of the "fundamental problem" and I forgot to
> remove it afterwards.
>
> However, I apologize for the inconvenience caused by this if clause,
> it's bogus comment, and for my insolence to send in a patch with 10
> lines of unimportant bogus code before having learned enough.
>
> If the freeing of the array with record field names is unnecessary
> and causes any irritation to you, simply remove it.
> I'm used to free memory after usage; if postgres or plpgsql
> handles that automatically or otherwise differently in that case,
> change the patch.
> I'd rather get accused for slowing down things than
> silently eating up memory. And I'm sure *you* know if it's safe
> to drop that pointer or whatever else to do.
>
> Being ugly or not, the changes to PLpgSQL_recfield were deliberatly
> chosen over a separate struct to be sure that records are
> always dealt with in the same way regardless of the field indexing
> method, to avoid code duplication between these two indexing cases
> (considering that the code dealing with the actual record field is more
> important to hold in common than to distinguish the two indexing
> cases with separate structures),
> and to make sure to catch all cases where this structure is dealt with
> by simply provoking a compilation error because of the moved struct
> member.
>
> So the bits do not "change language semantics", just because a
> bad comment and a never executed if clause is left in the code.
> Rest assured: the code (except the if clause) is in use on several
> servers for a year now and the rest of our now quite substantial amount
> of plpgsql code not using the newly introduced RECORD indexing construct
> remains very compatible with the interpreter without my patch,
> and the usual regression test executes without problems
> on an interpreter with that patch.
> Thus I dare to say that this patch more than "sort-of" works,
> even if - as I always would be the first to admit -
> my position on the apparently steep learning curve
> of pgsql internals is still quite near the origin of the coordinate
> system.
>
> I would suggest you change back the bogus if clause and
> - if you, as I presume, know better than me what happens -
> drop or change the freeing of the memory of the record field names array
> and apply the patch. For other changes I'd suggest you should apologize
> for your mail in case you would expect me to implement them
> or please find someone else to bear with you.
>
> Titus
>

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +