From 6d30fb1f75a57d80f80e27770d39d88f8aa32d28 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 8 Nov 2016 13:11:15 -0500 Subject: [PATCH] Make SPI_fnumber() reject dropped columns. There's basically no scenario where it's sensible for this to match dropped columns, so put a test for dropped-ness into SPI_fnumber() itself, and excise the test from the small number of callers that were paying attention to the case. (Most weren't :-(.) In passing, normalize tests at call sites: always reject attnum <= 0 if we're disallowing system columns. Previously there was a mixture of "< 0" and "<= 0" tests. This makes no practical difference since SPI_fnumber() never returns 0, but I'm feeling pedantic today. Also, in the places that are actually live user-facing code and not legacy cruft, distinguish "column not found" from "can't handle system column". Per discussion with Jim Nasby; thi supersedes his original patch that just changed the behavior at one call site. Discussion: --- contrib/spi/autoinc.c | 2 +- contrib/spi/insert_username.c | 2 +- contrib/spi/moddatetime.c | 4 ++-- contrib/spi/refint.c | 5 +++-- contrib/spi/timetravel.c | 4 ++-- doc/src/sgml/spi.sgml | 2 +- src/backend/executor/spi.c | 3 ++- src/backend/utils/adt/tsvector_op.c | 1 + src/pl/plperl/plperl.c | 7 ++++++- src/pl/tcl/pltcl.c | 11 +---------- src/test/regress/regress.c | 9 +++++---- 11 files changed, 25 insertions(+), 25 deletions(-) diff --git a/contrib/spi/autoinc.c b/contrib/spi/autoinc.c index 41eae4fdc4..fc657a7c06 100644 --- a/contrib/spi/autoinc.c +++ b/contrib/spi/autoinc.c @@ -71,7 +71,7 @@ autoinc(PG_FUNCTION_ARGS) int32 val; Datum seqname; - if (attnum < 0) + if (attnum <= 0) ereport(ERROR, (errcode(ERRCODE_TRIGGERED_ACTION_EXCEPTION), errmsg("\"%s\" has no attribute \"%s\"", diff --git a/contrib/spi/insert_username.c b/contrib/spi/insert_username.c index 3812525c4c..617c60a81c 100644 --- a/contrib/spi/insert_username.c +++ b/contrib/spi/insert_username.c @@ -67,7 +67,7 @@ insert_username(PG_FUNCTION_ARGS) attnum = SPI_fnumber(tupdesc, args[0]); - if (attnum < 0) + if (attnum <= 0) ereport(ERROR, (errcode(ERRCODE_TRIGGERED_ACTION_EXCEPTION), errmsg("\"%s\" has no attribute \"%s\"", relname, args[0]))); diff --git a/contrib/spi/moddatetime.c b/contrib/spi/moddatetime.c index c6d33b7355..cd700fe6d1 100644 --- a/contrib/spi/moddatetime.c +++ b/contrib/spi/moddatetime.c @@ -84,9 +84,9 @@ moddatetime(PG_FUNCTION_ARGS) /* * This is where we check to see if the field we are supposed to update - * even exists. The above function must return -1 if name not found? + * even exists. */ - if (attnum < 0) + if (attnum <= 0) ereport(ERROR, (errcode(ERRCODE_TRIGGERED_ACTION_EXCEPTION), errmsg("\"%s\" has no attribute \"%s\"", diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c index 01dd717522..78cfedf219 100644 --- a/contrib/spi/refint.c +++ b/contrib/spi/refint.c @@ -135,7 +135,7 @@ check_primary_key(PG_FUNCTION_ARGS) int fnumber = SPI_fnumber(tupdesc, args[i]); /* Bad guys may give us un-existing column in CREATE TRIGGER */ - if (fnumber < 0) + if (fnumber <= 0) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_COLUMN), errmsg("there is no attribute \"%s\" in relation \"%s\"", @@ -362,7 +362,7 @@ check_foreign_key(PG_FUNCTION_ARGS) int fnumber = SPI_fnumber(tupdesc, args[i]); /* Bad guys may give us un-existing column in CREATE TRIGGER */ - if (fnumber < 0) + if (fnumber <= 0) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_COLUMN), errmsg("there is no attribute \"%s\" in relation \"%s\"", @@ -469,6 +469,7 @@ check_foreign_key(PG_FUNCTION_ARGS) char *type; fn = SPI_fnumber(tupdesc, args_temp[k - 1]); + Assert(fn > 0); /* already checked above */ nv = SPI_getvalue(newtuple, tupdesc, fn); type = SPI_gettype(tupdesc, fn); diff --git a/contrib/spi/timetravel.c b/contrib/spi/timetravel.c index 5a345841c6..30dcfd4d3e 100644 --- a/contrib/spi/timetravel.c +++ b/contrib/spi/timetravel.c @@ -157,7 +157,7 @@ timetravel(PG_FUNCTION_ARGS) for (i = 0; i < MinAttrNum; i++) { attnum[i] = SPI_fnumber(tupdesc, args[i]); - if (attnum[i] < 0) + if (attnum[i] <= 0) elog(ERROR, "timetravel (%s): there is no attribute %s", relname, args[i]); if (SPI_gettypeid(tupdesc, attnum[i]) != ABSTIMEOID) elog(ERROR, "timetravel (%s): attribute %s must be of abstime type", @@ -166,7 +166,7 @@ timetravel(PG_FUNCTION_ARGS) for (; i < argc; i++) { attnum[i] = SPI_fnumber(tupdesc, args[i]); - if (attnum[i] < 0) + if (attnum[i] <= 0) elog(ERROR, "timetravel (%s): there is no attribute %s", relname, args[i]); if (SPI_gettypeid(tupdesc, attnum[i]) != TEXTOID) elog(ERROR, "timetravel (%s): attribute %s must be of text type", diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml index 9ae7126ae7..817a5d0120 100644 --- a/doc/src/sgml/spi.sgml +++ b/doc/src/sgml/spi.sgml @@ -2891,7 +2891,7 @@ int SPI_fnumber(TupleDesc rowdesc, const char * Return Value - Column number (count starts at 1), or + Column number (count starts at 1 for user-defined columns), or SPI_ERROR_NOATTRIBUTE if the named column was not found. diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index 38767ae4ce..8e650bc412 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -824,7 +824,8 @@ SPI_fnumber(TupleDesc tupdesc, const char *fname) for (res = 0; res < tupdesc->natts; res++) { - if (namestrcmp(&tupdesc->attrs[res]->attname, fname) == 0) + if (namestrcmp(&tupdesc->attrs[res]->attname, fname) == 0 && + !tupdesc->attrs[res]->attisdropped) return res + 1; } diff --git a/src/backend/utils/adt/tsvector_op.c b/src/backend/utils/adt/tsvector_op.c index ad5a254c57..0e9ae5ff9c 100644 --- a/src/backend/utils/adt/tsvector_op.c +++ b/src/backend/utils/adt/tsvector_op.c @@ -2242,6 +2242,7 @@ tsvector_update_trigger(PG_FUNCTION_ARGS, bool config_column) (errcode(ERRCODE_UNDEFINED_COLUMN), errmsg("tsvector column \"%s\" does not exist", trigger->tgargs[0]))); + /* This will effectively reject system columns, so no separate test: */ if (!IsBinaryCoercible(SPI_gettypeid(rel->rd_att, tsvector_attr_num), TSVECTOROID)) ereport(ERROR, diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c index 4d993e7371..461986cda3 100644 --- a/src/pl/plperl/plperl.c +++ b/src/pl/plperl/plperl.c @@ -1062,11 +1062,16 @@ plperl_build_tuple_result(HV *perlhash, TupleDesc td) char *key = hek2cstr(he); int attn = SPI_fnumber(td, key); - if (attn <= 0 || td->attrs[attn - 1]->attisdropped) + if (attn == SPI_ERROR_NOATTRIBUTE) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_COLUMN), errmsg("Perl hash contains nonexistent column \"%s\"", key))); + if (attn <= 0) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot set system attribute \"%s\"", + key))); values[attn - 1] = plperl_sv_to_datum(val, td->attrs[attn - 1]->atttypid, diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c index 3e52113ee2..20809102ef 100644 --- a/src/pl/tcl/pltcl.c +++ b/src/pl/tcl/pltcl.c @@ -603,6 +603,7 @@ pltcl_init_load_unknown(Tcl_Interp *interp) * leave this code as DString - it's only executed once per session ************************************************************/ fno = SPI_fnumber(SPI_tuptable->tupdesc, "modsrc"); + Assert(fno > 0); Tcl_DStringInit(&unknown_src); @@ -1259,12 +1260,6 @@ pltcl_trigger_handler(PG_FUNCTION_ARGS, pltcl_call_state *call_state, errmsg("cannot set system attribute \"%s\"", ret_name))); - /************************************************************ - * Ignore dropped columns - ************************************************************/ - if (tupdesc->attrs[attnum - 1]->attisdropped) - continue; - /************************************************************ * Lookup the attribute type's input function ************************************************************/ @@ -3077,10 +3072,6 @@ pltcl_build_tuple_result(Tcl_Interp *interp, Tcl_Obj **kvObjv, int kvObjc, errmsg("cannot set system attribute \"%s\"", fieldName))); - /* Ignore dropped attributes */ - if (call_state->ret_tupdesc->attrs[attn - 1]->attisdropped) - continue; - values[attn - 1] = utf_e2u(Tcl_GetString(kvObjv[i + 1])); } diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index e7826a4513..119a59ab07 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -523,11 +523,12 @@ ttdummy(PG_FUNCTION_ARGS) for (i = 0; i < 2; i++) { attnum[i] = SPI_fnumber(tupdesc, args[i]); - if (attnum[i] < 0) - elog(ERROR, "ttdummy (%s): there is no attribute %s", relname, args[i]); + if (attnum[i] <= 0) + elog(ERROR, "ttdummy (%s): there is no attribute %s", + relname, args[i]); if (SPI_gettypeid(tupdesc, attnum[i]) != INT4OID) - elog(ERROR, "ttdummy (%s): attributes %s and %s must be of abstime type", - relname, args[0], args[1]); + elog(ERROR, "ttdummy (%s): attribute %s must be of integer type", + relname, args[i]); } oldon = SPI_getbinval(trigtuple, tupdesc, attnum[0], &isnull); -- 2.40.0