Re: PL/PGSQL: Dynamic Record Introspection

Titus von Boxberg wrote:
> With the following patch it's possible to
> - extract all field names of a record into an array
> - extract field count of a record
> - address a single field of a record with a variable
> containing the field name (additional to the usual record.fieldname
> notation where the fieldname is hardcoded).

I wonder if this is the right syntax. record%identifier is doing
something fundamentally different from record.identifier, but the syntax
doesn't make that clear. I don't have any concrete suggestions for
improvement, mind you... :)

> Test code for the patch can be extracted from an example I put into
> plpgsql.sgml

Can you supply some proper regression tests, please? i.e. patch
sql/plpgsql.sql and expected/plpgsql.out in src/test/regress

A few minor comments from skimming the patch:

> ***************
> *** 995,1001 ****
>
> new = palloc(sizeof(PLpgSQL_recfield));
> new->dtype = PLPGSQL_DTYPE_RECFIELD;
> ! new->fieldname = pstrdup(cp[1]);
> new->recparentno = ns->itemno;
>
> plpgsql_adddatum((PLpgSQL_datum *) new);
> --- 995,1002 ----
>
> new = palloc(sizeof(PLpgSQL_recfield));
> new->dtype = PLPGSQL_DTYPE_RECFIELD;
> ! new->fieldindex.fieldname = strdup(cp[1]);
> ! new->fieldindex_flag = RECFIELD_USE_FIELDNAME;
> new->recparentno = ns->itemno;
>
> plpgsql_adddatum((PLpgSQL_datum *) new);

Should be pstrdup().

> ***************
> *** 1101,1107 ****
>
> new = palloc(sizeof(PLpgSQL_recfield));
> new->dtype = PLPGSQL_DTYPE_RECFIELD;
> ! new->fieldname = pstrdup(cp[2]);
> new->recparentno = ns->itemno;
>
> plpgsql_adddatum((PLpgSQL_datum *) new);
> --- 1102,1109 ----
>
> new = palloc(sizeof(PLpgSQL_recfield));
> new->dtype = PLPGSQL_DTYPE_RECFIELD;
> ! new->fieldindex.fieldname = strdup(cp[2]);
> ! new->fieldindex_flag = RECFIELD_USE_FIELDNAME;
> new->recparentno = ns->itemno;
>
> plpgsql_adddatum((PLpgSQL_datum *) new);

Ibid.

> + switch (ns1->itemtype)
> + {
> + case PLPGSQL_NSTYPE_REC:
> + {
> + PLpgSQL_recfieldproperties *new;
> +
> + new = malloc(sizeof(PLpgSQL_recfieldproperties));
> + new->dtype = PLPGSQL_DTYPE_NRECFIELD;
> + new->recparentno = ns1->itemno;
> + plpgsql_adddatum((PLpgSQL_datum *) new);
> + plpgsql_yylval.scalar = (PLpgSQL_datum *) new;
> + ret = T_SCALAR; /* ??? */
> + break;

Should be palloc().

> + return ret;
> + } /* plpgsql_parse_wordnfields */

The Postgres convention is not to include comments like this.

> + case PLPGSQL_NSTYPE_REC:
> + {
> + PLpgSQL_recfieldproperties *new;
> +
> + new = malloc(sizeof(PLpgSQL_recfieldproperties));
> + new->dtype = PLPGSQL_DTYPE_RECFIELDNAMES;
> + new->recparentno = ns1->itemno;
> + plpgsql_adddatum((PLpgSQL_datum *) new);
> + plpgsql_yylval.scalar = (PLpgSQL_datum *) new;
> + ret = T_SCALAR; /* ??? */
> + break;
> + }

Should be palloc().

> ! /* construct_array copies data; free temp elem array */
> ! #if 0
> ! for ( fc = 0; fc < tfc; ++fc )
> ! pfree(DatumGetPointer(arrayelems[fc]);
> ! pfree(arrayelems);
> ! #endif

Unexplained #if 0 blocks aren't good.

-Neil