sloppy back-patching of column-privilege leak

Branch: master [804b6b6db] 2015-01-28 12:31:30 -0500
Branch: REL9_4_STABLE Release: REL9_4_1 [3cc74a3d6] 2015-01-28 12:32:06 -0500
Branch: REL9_3_STABLE Release: REL9_3_6 [4b9874216] 2015-01-28 12:32:39 -0500
Branch: REL9_2_STABLE Release: REL9_2_10 [d49f84b08] 2015-01-28 12:32:56 -0500
Branch: REL9_1_STABLE Release: REL9_1_15 [9406884af] 2015-01-28 12:33:15 -0500
Branch: REL9_0_STABLE Release: REL9_0_19 [3a2063369] 2015-01-28 12:33:29 -0500

In 9.2 and newer branches, this commit makes substantial changes to
execMain.c. In 9.1 and 9.0, though, the change to that file is just
this:

--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -95,6 +95,15 @@ static void intorel_receive(TupleTableSlot *slot,
DestReceiver *self);
static void intorel_shutdown(DestReceiver *self);
static void intorel_destroy(DestReceiver *self);

+/*
+ * Note that this macro also exists in commands/trigger.c. There does not
+ * appear to be any good header to put it into, given the structures that
+ * it uses, so we let them be duplicated. Be sure to update both if one needs
+ * to be changed, however.
+ */
+#define GetModifiedColumns(relinfo, estate) \
+ (rt_fetch((relinfo)->ri_RangeTableIndex,
(estate)->es_range_table)->modifiedCols)
+
/* end of local decls */

We shouldn't be adding a macro to that file that isn't used anywhere.
Either that file needs more substantial changes, or it doesn't need
this, either. What gives? Besides the sloppiness of back-patching in
that one macro and nothing else, this is a huge fraction of the patch
that's just *gone* in the 9.0 and 9.1 branches, and there's not so
much as a hint about it in the commit message, which is pretty
astonishing to me.

What's even more troubling is the new regression tests. In master,
this commit added this:

+INSERT INTO t1 (c1, c2) VALUES (1, 1); -- fail, but row not shown
+ERROR: duplicate key value violates unique constraint "t1_pkey"
+UPDATE t1 SET c2 = 1; -- fail, but row not shown
+ERROR: duplicate key value violates unique constraint "t1_pkey"
+INSERT INTO t1 (c1, c2) VALUES (null, null); -- fail, but see columns
being inserted
+ERROR: null value in column "c1" violates not-null constraint
+DETAIL: Failing row contains (c1, c2) = (null, null).
+INSERT INTO t1 (c3) VALUES (null); -- fail, but see columns being
inserted or have SELECT
+ERROR: null value in column "c1" violates not-null constraint
+DETAIL: Failing row contains (c1, c3) = (null, null).
+INSERT INTO t1 (c1) VALUES (5); -- fail, but see columns being
inserted or have SELECT
+ERROR: null value in column "c2" violates not-null constraint
+DETAIL: Failing row contains (c1) = (5).
+UPDATE t1 SET c3 = 10; -- fail, but see columns with SELECT rights,
or being modified
+ERROR: new row for relation "t1" violates check constraint "t1_c3_check"
+DETAIL: Failing row contains (c1, c3) = (1, 10).

Notice how the comments match the actual behavior. But in 9.0, you've
got the SAME COMMENTS with DIFFERENT BEHAVIOR:

+INSERT INTO t1 (c1, c2) VALUES (1, 1); -- fail, but row not shown
+ERROR: duplicate key value violates unique constraint "t1_pkey"
+UPDATE t1 SET c2 = 1; -- fail, but row not shown
+ERROR: duplicate key value violates unique constraint "t1_pkey"
+INSERT INTO t1 (c1, c2) VALUES (null, null); -- fail, but see columns
being inserted
+ERROR: null value in column "c1" violates not-null constraint
+INSERT INTO t1 (c3) VALUES (null); -- fail, but see columns being
inserted or have SELECT
+ERROR: null value in column "c1" violates not-null constraint
+INSERT INTO t1 (c1) VALUES (5); -- fail, but see columns being
inserted or have SELECT
+ERROR: null value in column "c2" violates not-null constraint
+UPDATE t1 SET c3 = 10; -- fail, but see columns with SELECT rights,
or being modified
+ERROR: new row for relation "t1" violates check constraint "t1_c3_check"

The comments say that you'll see the relevant columns, but you don't.
Since when is it OK to check things into our regression tests where
the comments say one thing and the behavior is something else?

I don't even know what to say about this. I cannot understand how you
can back-patch something like this and fail to notice that the
regression tests give different output on different branches, and that
that output is inconsistent with the comments.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company