Disallow system columns in COPY FROM WHERE conditions.

These columns haven't been computed yet when the filtering happens
(since we've not written the candidate tuple into the table); so
any check on them is wrong or useless.  Worse, since aa606b931 such a
reference results in an access off the end of a TupleDesc, potentially
causing a phony "generated columns are not supported in COPY FROM
WHERE conditions" error; and since c98ad086a it throws an Assert
instead.

Actually we could allow tableoid, which has been set to the OID of the
table named as the COPY target.  However, plausible uses for tests of
tableoid would involve a partitioned target table, and the user would
wish it to read as the OID of the destination partition.  There has
been some discussion of changing things to make it work like that,
but pending that happening we should just disallow tableoid along
with other system columns.

It seems best though to install this prohibition only in HEAD.
In the back branches we'll just guard the unsafe TupleDesc access,
and people will keep getting whatever semantics they got before.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/6f435023-8ab6-47c2-ba07-035d0c4212f9@gmail.com

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e837f417d0d194bd05c2380fe021266fc467cc2e..003b70852bb90a60752987c701ea5d9685323693 100644 (file)
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -137,22 +137,22 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
                        Bitmapset  *expr_attrs = NULL;
                        int                     i;
 
-                       /* add nsitem to query namespace */
+                       /* Add nsitem to query namespace */
                        addNSItemToQuery(pstate, nsitem, false, true, true);
 
                        /* Transform the raw expression tree */
                        whereClause = transformExpr(pstate, stmt->whereClause, EXPR_KIND_COPY_WHERE);
 
-                       /* Make sure it yields a boolean result. */
+                       /* Make sure it yields a boolean result */
                        whereClause = coerce_to_boolean(pstate, whereClause, "WHERE");
 
-                       /* we have to fix its collations too */
+                       /* We have to fix its collations too */
                        assign_expr_collations(pstate, whereClause);
 
                        /*
-                        * Examine all the columns in the WHERE clause expression.  When
-                        * the whole-row reference is present, examine all the columns of
-                        * the table.
+                        * Identify all columns used in the WHERE clause's expression.  If
+                        * there's a whole-row reference, replace it with a range of all
+                        * the user columns (caution: that'll include dropped columns).
                         */
                        pull_varattnos(whereClause, 1, &expr_attrs);
                        if (bms_is_member(0 - FirstLowInvalidHeapAttributeNumber, expr_attrs))
@@ -163,12 +163,30 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
                                expr_attrs = bms_del_member(expr_attrs, 0 - FirstLowInvalidHeapAttributeNumber);
                        }
 
+                       /* Now we can scan each column needed in the WHERE clause */
                        i = -1;
                        while ((i = bms_next_member(expr_attrs, i)) >= 0)
                        {
                                AttrNumber      attno = i + FirstLowInvalidHeapAttributeNumber;
+                               Form_pg_attribute att;
 
-                               Assert(attno != 0);
+                               Assert(attno != 0); /* removed above */
+
+                               /*
+                                * Prohibit system columns in the WHERE clause.  They won't
+                                * have been filled yet when the filtering happens.  (We could
+                                * allow tableoid, but right now it isn't really useful: it
+                                * will read as the target table's OID.  Any conceivable use
+                                * for such a WHERE clause would probably wish it to read as
+                                * the target partition's OID, which is not known yet.
+                                * Disallow it to keep flexibility to change that sometime.)
+                                */
+                               if (attno < 0)
+                                       ereport(ERROR,
+                                                       errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+                                                       errmsg("system columns are not supported in COPY FROM WHERE conditions"),
+                                                       errdetail("Column \"%s\" is a system column.",
+                                                                         get_attname(RelationGetRelid(rel), attno, false)));
 
                                /*
                                 * Prohibit generated columns in the WHERE clause.  Stored
@@ -177,7 +195,8 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
                                 * would need to expand them somewhere around here), but for
                                 * now we keep them consistent with the stored variant.
                                 */
-                               if (TupleDescAttr(RelationGetDescr(rel), attno - 1)->attgenerated)
+                               att = TupleDescAttr(RelationGetDescr(rel), attno - 1);
+                               if (att->attgenerated && !att->attisdropped)
                                        ereport(ERROR,
                                                        errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
                                                        errmsg("generated columns are not supported in COPY FROM WHERE conditions"),
@@ -185,6 +204,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
                                                                          get_attname(RelationGetRelid(rel), attno, false)));
                        }
 
+                       /* Reduce WHERE clause to standard list-of-AND-terms form */
                        whereClause = eval_const_expressions(NULL, whereClause);
 
                        whereClause = (Node *) canonicalize_qual((Expr *) whereClause, false);

diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 01101c7105106c0ad2b86179f935754df4934578..7600e5239d29c0d4b53c5ec7c261be0afa46a4fc 100644 (file)
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -204,6 +204,9 @@ COPY x from stdin WHERE a = row_number() over(b);
 ERROR:  window functions are not allowed in COPY FROM WHERE conditions
 LINE 1: COPY x from stdin WHERE a = row_number() over(b);
                                     ^
+COPY x from stdin WHERE tableoid = 'x'::regclass;
+ERROR:  system columns are not supported in COPY FROM WHERE conditions
+DETAIL:  Column "tableoid" is a system column.
 -- check results of copy in
 SELECT * FROM x;
    a   | b  |     c      |   d    |          e           

diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql
index 889dcf1383f3fee9a63cde4119e73ec4526facd9..e0810109473b4b5df1e077999001fc3b2f04f32a 100644 (file)
--- a/src/test/regress/sql/copy2.sql
+++ b/src/test/regress/sql/copy2.sql
@@ -168,6 +168,8 @@ COPY x from stdin WHERE a IN (generate_series(1,5));
 
 COPY x from stdin WHERE a = row_number() over(b);
 
+COPY x from stdin WHERE tableoid = 'x'::regclass;
+
 
 -- check results of copy in
 SELECT * FROM x;