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: <b2de8258-c4c0-1cb8-7b97-e8538e5c975c@BlueTreble.com>

diff --git a/contrib/spi/autoinc.c b/contrib/spi/autoinc.c
index 41eae4fdc459c8fbfc0ddf1e7c78de055f1eb425..fc657a7c06e203177abade3db6df7f9b3df5ecaf 100644 (file)
--- 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 3812525c4c083cb749314f5b15dc780e1c2026f2..617c60a81c57e619d539b41f5878f191197a899a 100644 (file)
--- 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 c6d33b73557ff499faa0d475e032fd1f8164afbd..cd700fe6d131cc2d8356e7adfa37293a5c9e9de1 100644 (file)
--- 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 01dd717522c42ecbe46878470bdbaa30864e13d5..78cfedf219f03965b7baf6b3dfc445fab2d3f1d7 100644 (file)
--- 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 5a345841c6a9cbc3dbfa83b5a34b8ab6e3083915..30dcfd4d3ecc90cce6f5a9aaa0c8c4c46f2e1a79 100644 (file)
--- 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 9ae7126ae7f144c0005eb16ca314056de2934232..817a5d0120ad4c6e640bc1f4e94272fe84c89add 100644 (file)
--- a/doc/src/sgml/spi.sgml
+++ b/doc/src/sgml/spi.sgml
@@ -2891,7 +2891,7 @@ int SPI_fnumber(TupleDesc <parameter>rowdesc</parameter>, const char * <paramete
   <title>Return Value</title>
 
   <para>
-   Column number (count starts at 1), or
+   Column number (count starts at 1 for user-defined columns), or
    <symbol>SPI_ERROR_NOATTRIBUTE</symbol> if the named column was not
    found.
   </para>

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 38767ae4cedc7dabc80cd08f3f49e8620d1ba65d..8e650bc4123d10d0b81521664fecdb914114001a 100644 (file)
--- 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 ad5a254c57e385c86d8d8cda223f971c253399f0..0e9ae5ff9cf538b5570f63ff82bc8d3cd48f17ff 100644 (file)
--- 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 4d993e7371da43bce0caa71e7c7a070b03cb246b..461986cda31ea0b4e1eb7ad5e08889bc8e2a079d 100644 (file)
--- 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 3e52113ee253762fdabae5e4cec3ac8b9557a712..20809102efbe7400158dc791b3910a093c533dfe 100644 (file)
--- 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 e7826a4513b3f69eb162172689dd36813185361f..119a59ab0732ca12419f63ec19a3bbace3a9ba1d 100644 (file)
--- 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);