has_column_privilege behavior (was Re: Assert failed in snprintf.c) - Mailing list pgsql-hackers

I wrote:
> Jaime Casanova <jaime.casanova@2ndquadrant.com> writes:
>> sqlsmith made it again, attached is the query (run against regression
>> database) that makes the assert fail and the backtrace.
>> this happens in head only (or at least 11 is fine).

> Ah.  Looks like the has_column_privilege stuff is incautious about whether
> it's handed a valid table OID:
> regression=# select has_column_privilege(42::oid, 'z'::text, 'q'::text);
> server closed the connection unexpectedly

So my first thought was to just throw an error for bad table OID,
as per the attached quick-hack patch.

However, on closer inspection, I wonder whether that's what we really
want.  The rest of the OID-taking have_foo_privilege functions are
designed to return null, not fail, if handed a bad OID.  This is meant to
allow queries scanning system catalogs to not die if an object is
concurrently dropped.  So I think this should do likewise.

But it's not quite clear to me what we want the behavior for bad column
name to be.  A case could be made for either of:

* If either the table OID is bad, or the OID is OK but there's no such
column, return null.

* Return null for bad OID, but if it's OK, continue to throw error
for bad column name.

The second case seems weirdly inconsistent, but it might actually
be the most useful definition.  Not detecting a misspelled column
name is likely to draw complaints.

Thoughts?

            regards, tom lane

diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index d5285e2..3b963a0 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -2837,10 +2837,20 @@ convert_column_name(Oid tableoid, text *column)
     colname = text_to_cstring(column);
     attnum = get_attnum(tableoid, colname);
     if (attnum == InvalidAttrNumber)
+    {
+        char       *tablename = get_rel_name(tableoid);
+
+        /* Paranoia is necessary since tableoid could be anything */
+        if (tablename == NULL)
+            ereport(ERROR,
+                    (errcode(ERRCODE_UNDEFINED_TABLE),
+                     errmsg("relation with OID %u does not exist",
+                            tableoid)));
         ereport(ERROR,
                 (errcode(ERRCODE_UNDEFINED_COLUMN),
                  errmsg("column \"%s\" of relation \"%s\" does not exist",
-                        colname, get_rel_name(tableoid))));
+                        colname, tablename)));
+    }
     pfree(colname);
     return attnum;
 }