Hi all,
I've cleaned this up a bit so that assigning to a dynamic record field
now works - ie NEW.(myvar) := 'someval' - and accessing a field by
number works - ie myvar := OLD.(1)
It still doesn't work in a loop if the type of field referenced by the
expression changes - looking at it more I'm really not sure this is
possible, but that's a limitation I could live with. I'll also try
figuring out freeing stuff after casting values over the weekend.
But is this a worthwhile approach? If there are objections, please let
me know!
For myself, being able to pass a column name as an argument to a trigger
would make writing generic triggers a whole lot easier. And going back
through the archives on this list I see I'm not alone.
Regards,
Matt
On Thu, 2004-11-18 at 13:18, Matt wrote:
> Hi,
>
> I got extremely frustrated with having to create a temp table every time
> I wanted to access an arbitrary column from a record plpgsql. After
> seeing a note on the developers TODO about accessing plpgsql records
> with a 'dot bracket' notation I started digging into the plpgsql source.
>
> My diff (against 8beta4) is attached.
>
> Warning: I Am Not a C Programmer! I haven't even written a hello world
> in C before, and I knew nothing about Flex before yesterday. It was fun
> figuring stuff out, I'm amazed it mostly works, but I'm really hoping
> someone can point out my mistakes.
>
> Goal:
>
> Enable users to access fields in record variables using the following
> syntax like the following:
> rec.(1)
> rec.('foo')
> rec.(myvar::int)
> rec.(myvar || '_id')
>
> Files changed:
>
> plpgsql.h
> - added 'expr' member to PLpgSQL_recfield type for holding the
> PLpgSQL_expr structure.
>
> scan.l
> - added match for {identifier}{space}*\. AFAIK this should only match
> if a longer expression doesn't?
>
> pl_comp.c
> - added plpgsql_parse_wordexpr() function called by above match. Ripped
> off code from plpgsql_parse_word that deals with arg_v[expr] to find our
> expression. Prob a dumb name for the function!
>
> pl_exec.c
> - hacked exec_assign_value() and exec_eval_datum() to use the expression
> to get the field name/number.
>
> Stuff I need help with:
>
> 1. It should recognise OLD.(1) as a field number, not a column name. I
> think I've got to check the returned type from exec_eval_expr() then
> exec_simple_cast_value() on it, but that seems beyond me.
>
> 2. Freeing stuff. As I explained, this is all pretty new to me, and the
> comments about it around exec_eval_expr() et al just confused me :(
> Please give some hints about what needs freeing!
>
> 3. Would probably be good to add check in pl_comp.c to see if the
> expression actually needs to be evaluated at runtime (ie isn't just a
> field name/number). How?
>
> 4. Make this also work for row.(expr), label.record.(expr) and
> label.row.(expr) - but want to get the basics working first!
>
> 5. Because of the way the expression is parsed (looking for closing
> parenth), this will choke if you try and put a function in there. Would
> it be better to use curly braces '{expr}' or another character to mark
> the expression?
>
> I hope at this eventually leads to some really useful extra
> functionality, particularly for writing generic triggers. And it's a
> tribute to the existing code that a complete newbie can cut-and-paste
> their way to a halfarsed solution in a (rather long) night!
>
> Regards,
>
> Matt
>
> ______________________________________________________________________
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index scan if your
> joining column's datatypes do not match
diff -u src.orig/pl_comp.c src/pl_comp.c
--- src.orig/pl_comp.c 2004-11-19 14:01:15.053237796 +0000
+++ src/pl_comp.c 2004-11-19 14:23:04.192899809 +0000
@@ -783,6 +783,75 @@
return T_WORD;
}
+/* ----------
+ * plpgsql_parse_wordexpr Same lookup for word followed by dot.
+ * Should only get here if it wasn't followed by
+ * an identifier.
+ * ----------
+ */
+int
+plpgsql_parse_wordexpr(char *word)
+{
+ PLpgSQL_nsitem *ns;
+ char *cp[1];
+ int save_spacescanned = plpgsql_SpaceScanned;
+
+ /* Do case conversion and word separation */
+ /* drop dot to keep converter happy */
+ word[strlen(word) - 1] = '\0';
+ plpgsql_convert_ident(word, cp, 1);
+
+ /* Make sure we've got an open parenthesis */
+
+ /*
+ * Do a lookup on the compilers namestack
+ */
+ ns = plpgsql_ns_lookup(cp[0], NULL);
+ if (ns == NULL)
+ {
+ pfree(cp[0]);
+ return T_ERROR;
+ }
+ switch (ns->itemtype)
+ {
+ case PLPGSQL_NSTYPE_REC:
+ {
+ /*
+ * First word is a record name, so expression refers to
+ * field in this record.
+ */
+ PLpgSQL_recfield *new;
+
+ new = malloc(sizeof(PLpgSQL_recfield));
+ memset(new, 0, sizeof(PLpgSQL_recfield));
+
+ if (plpgsql_yylex() != '(')
+ plpgsql_yyerror("expected identifier or \"(\"");
+ new->expr = plpgsql_read_expression(')', ")");
+ new->recparentno = ns->itemno;
+ /* just to be sure - we'll test on this later */
+ new->fieldname = '\0';
+ new->dtype = PLPGSQL_DTYPE_RECFIELD;
+
+ plpgsql_adddatum((PLpgSQL_datum *) new);
+
+ plpgsql_yylval.scalar = (PLpgSQL_datum *) new;
+
+ plpgsql_SpaceScanned = save_spacescanned;
+
+ pfree(cp[0]);
+ return T_SCALAR;
+ }
+
+ /* TODO: deal with rows, too */
+
+ default:
+ break;
+
+ }
+ pfree(cp[0]);
+ return T_ERROR;
+}
/* ----------
* plpgsql_parse_dblword Same lookup for two words
diff -u src.orig/pl_exec.c src/pl_exec.c
--- src.orig/pl_exec.c 2004-11-19 14:01:15.053237796 +0000
+++ src/pl_exec.c 2004-11-19 15:20:33.160897466 +0000
@@ -2965,7 +2965,8 @@
PLpgSQL_recfield *recfield = (PLpgSQL_recfield *) target;
PLpgSQL_rec *rec;
int fno;
- HeapTuple newtup;
+ char *fname;
+ HeapTuple newtup;
int natts;
int i;
Datum *values;
@@ -2974,7 +2975,7 @@
bool attisnull;
Oid atttype;
int32 atttypmod;
-
+
rec = (PLpgSQL_rec *) (estate->datums[recfield->recparentno]);
/*
@@ -2988,20 +2989,71 @@
errmsg("record \"%s\" is not assigned yet",
rec->refname),
errdetail("The tuple structure of a not-yet-assigned record is indeterminate.")));
-
- /*
+
+
+ /* Have we got an expr to deal with? */
+ fno = 0;
+ fname = '\0';
+ if (recfield->fieldname == '\0')
+ {
+ Datum exprdatum;
+ Oid exprtype;
+ bool *exprisnull;
+
+ /* Evaluate expression */
+ exprdatum = exec_eval_expr(estate, recfield->expr,
+ exprisnull, &exprtype);
+
+ /* If we've got an integer, it's a field number, otherwise
+ * it's a fieldname
+ */
+ if (exprtype == INT4OID) {
+ exprdatum = exec_simple_cast_value(exprdatum,
+ exprtype,
+ INT4OID, -1,
+ exprisnull);
+ fno = DatumGetInt32(exprdatum);
+ }
+ else {
+ fname = convert_value_to_string(
+ exprdatum, exprtype);
+ }
+
+ /* do we need to free datum? */
+ }
+ else {
+ fname = recfield->fieldname;
+ }
+
+
+ /*
* Get the number of the records field to change and the
- * number of attributes in the tuple.
+ * number of attributes in the tuple, if we didn't get it
+ * above.
*/
- 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;
+ if (fno < 1)
+ {
+ fno = SPI_fnumber(rec->tupdesc, fname);
+ if (fno == SPI_ERROR_NOATTRIBUTE)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("record \"%s\" has no field \"%s\"",
+ rec->refname, fname)));
+ }
+ fno--;
+ natts = rec->tupdesc->natts;
+
+ /*
+ * Check that fno is within range
+ */
+ if (fno >= natts) {
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("record \"%s\" has no field number \"%d\"",
+ rec->refname, fno)));
+ }
+
/*
* Set up values/datums arrays for heap_formtuple. For
* all the attributes except the one we want to replace,
@@ -3297,7 +3349,8 @@
PLpgSQL_recfield *recfield = (PLpgSQL_recfield *) datum;
PLpgSQL_rec *rec;
int fno;
-
+ char *fname;
+
rec = (PLpgSQL_rec *) (estate->datums[recfield->recparentno]);
if (!HeapTupleIsValid(rec->tup))
ereport(ERROR,
@@ -3305,19 +3358,73 @@
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);
+
+ /* Have we got an expr to deal with? */
+ fno = 0;
+ fname = '\0';
+ if (recfield->fieldname == '\0')
+ {
+ Datum exprdatum;
+ Oid exprtype;
+ bool *exprisnull;
+
+ /* Evaluate expression */
+ exprdatum = exec_eval_expr(estate, recfield->expr,
+ exprisnull, &exprtype);
+
+
+ /* If we've got an integer, it's a field number, otherwise
+ * it's a fieldname
+ */
+ if (exprtype == INT4OID) {
+ exprdatum = exec_simple_cast_value(exprdatum,
+ exprtype,
+ INT4OID, -1,
+ exprisnull);
+ fno = DatumGetInt32(exprdatum);
+ }
+ else {
+ fname = convert_value_to_string(
+ exprdatum, exprtype);
+ }
+ }
+ else {
+ fname = recfield->fieldname;
+ }
+
+ /*
+ * Get the number of the records field to change and the
+ * number of attributes in the tuple.
+ */
+ if (fno < 1)
+ {
+ fno = SPI_fnumber(rec->tupdesc, fname);
+ if (fno == SPI_ERROR_NOATTRIBUTE)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("record \"%s\" has no field \"%s\"",
+ rec->refname, fname)));
+ }
+
+ *typeid = SPI_gettypeid(rec->tupdesc, fno);
*value = SPI_getbinval(rec->tup, rec->tupdesc, fno, isnull);
- if (expectedtypeid != InvalidOid && expectedtypeid != *typeid)
- ereport(ERROR,
+ if (*value == SPI_ERROR_NOATTRIBUTE)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("record \"%s\" has no field number \"%d\"",
+ rec->refname, fno)));
+
+ if (expectedtypeid != InvalidOid && expectedtypeid != *typeid)
+ /*
+ * If an expression is used to determine the fieldname and
+ * the type of that field changes, we'll end up here.
+ * Should we cast it to the original type, or just throw
+ * an error?
+ */
+ ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("type of \"%s.%s\" does not match that when preparing the plan",
- rec->refname, recfield->fieldname)));
+ rec->refname, fname)));
break;
}
diff -u src.orig/plpgsql.h src/plpgsql.h
--- src.orig/plpgsql.h 2004-11-19 14:01:15.054237644 +0000
+++ src/plpgsql.h 2004-11-19 14:23:17.751845320 +0000
@@ -269,6 +269,7 @@
int dtype;
int rfno;
char *fieldname;
+ PLpgSQL_expr *expr;
int recparentno; /* dno of parent record */
} PLpgSQL_recfield;
@@ -675,6 +676,7 @@
extern PLpgSQL_function *plpgsql_compile(FunctionCallInfo fcinfo,
bool forValidator);
extern int plpgsql_parse_word(char *word);
+extern int plpgsql_parse_wordexpr(char *word);
extern int plpgsql_parse_dblword(char *word);
extern int plpgsql_parse_tripword(char *word);
extern int plpgsql_parse_wordtype(char *word);
diff -u src.orig/scan.l src/scan.l
--- src.orig/scan.l 2004-11-19 14:01:15.054237644 +0000
+++ src/scan.l 2004-11-19 14:31:24.079117322 +0000
@@ -195,6 +195,9 @@
{identifier} {
plpgsql_error_lineno = plpgsql_scanner_lineno();
return plpgsql_parse_word(yytext); }
+{identifier}{space}*\. {
+ plpgsql_error_lineno = plpgsql_scanner_lineno();
+ return plpgsql_parse_wordexpr(yytext); }
{identifier}{space}*\.{space}*{identifier} {
plpgsql_error_lineno = plpgsql_scanner_lineno();
return plpgsql_parse_dblword(yytext); }
---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster
