Обсуждение: Assert failed in snprintf.c

Hi,

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).

-- 
Jaime Casanova                      www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

In older branches I get

regression=# select has_column_privilege(42::oid, 'z'::text, 'q'::text);
ERROR:  column "z" of relation "(null)" does not exist

but that's only because glibc's snprintf is forgiving about getting a
NULL pointer for %s.  On some other platforms it'd end in SIGSEGV.

Will fix, thanks for report!

            regards, tom lane

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;
 }

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> 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.

Agreed.

> 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?

What are we going to do for dropped columns..?  Seems like with what
you're suggesting we'd throw an error, but that'd make querying with
this function similairly annoying at times.

My inclination would be to make the function return NULL in any case
where we can't find what the user is asking for (and to not throw an
error in general).

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> 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?

> What are we going to do for dropped columns..?  Seems like with what
> you're suggesting we'd throw an error, but that'd make querying with
> this function similairly annoying at times.

True, but I think dropping individual columns is much less common
than dropping whole tables.

The general plan in the has_foo_privilege functions is to throw errors for
failing name-based lookups, but return null for failing numerically-based
lookups (object OID or column number).  I'm inclined to think we should
stick to that.  In the case at hand, we'd be supporting queries that
iterate over pg_attribute, but they'd have to pass attnum not attname
to avoid snapshot-skew failures.  That's a bit annoying, but not throwing
error for a typo'ed name is annoying to a different and probably larger
set of users.

            regards, tom lane

On 10/01/2018 02:41 PM, Stephen Frost wrote:
> Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> 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.

Seems you could make the same argument for not detecting a misspelled
table name for this and has_table_privilege...


> My inclination would be to make the function return NULL in any case
> where we can't find what the user is asking for (and to not throw an
> error in general).

+1

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> 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?
>
> > What are we going to do for dropped columns..?  Seems like with what
> > you're suggesting we'd throw an error, but that'd make querying with
> > this function similairly annoying at times.
>
> True, but I think dropping individual columns is much less common
> than dropping whole tables.

That certainly doesn't mean that it doesn't happen though, nor does it
mean we don't have to come up with an answer to the question.

> The general plan in the has_foo_privilege functions is to throw errors for
> failing name-based lookups, but return null for failing numerically-based
> lookups (object OID or column number).  I'm inclined to think we should
> stick to that.  In the case at hand, we'd be supporting queries that
> iterate over pg_attribute, but they'd have to pass attnum not attname
> to avoid snapshot-skew failures.  That's a bit annoying, but not throwing
> error for a typo'ed name is annoying to a different and probably larger
> set of users.

... and what's going to happen when they pass in a dropped column,
either via number or name?

I don't have an issue with throwing a failure for name-based lookups but
returning null for failing numerically-based lookups, but I don't really
want us throwing errors on dropped columns.  I would think we'd return
null in that case.  In particular, I can see this function being used in
a where clause across pg_attribute.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> The general plan in the has_foo_privilege functions is to throw errors for
>> failing name-based lookups, but return null for failing numerically-based
>> lookups (object OID or column number).  I'm inclined to think we should
>> stick to that.  In the case at hand, we'd be supporting queries that
>> iterate over pg_attribute, but they'd have to pass attnum not attname
>> to avoid snapshot-skew failures.  That's a bit annoying, but not throwing
>> error for a typo'ed name is annoying to a different and probably larger
>> set of users.

> ... and what's going to happen when they pass in a dropped column,
> either via number or name?

Well, it'll have to fail for the name case, but not for the attnum case.

> I don't have an issue with throwing a failure for name-based lookups but
> returning null for failing numerically-based lookups, but I don't really
> want us throwing errors on dropped columns.  I would think we'd return
> null in that case.

You can't have it both ways.  Either you throw an error if the name's
not there, or you don't.

> In particular, I can see this function being used in
> a where clause across pg_attribute.

As said above, it can work as long as you use attnum not attname.
I don't think this is really so different from iterating across
pg_class (or any other catalog) and passing relname instead of OID.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> The general plan in the has_foo_privilege functions is to throw errors for
> >> failing name-based lookups, but return null for failing numerically-based
> >> lookups (object OID or column number).  I'm inclined to think we should
> >> stick to that.  In the case at hand, we'd be supporting queries that
> >> iterate over pg_attribute, but they'd have to pass attnum not attname
> >> to avoid snapshot-skew failures.  That's a bit annoying, but not throwing
> >> error for a typo'ed name is annoying to a different and probably larger
> >> set of users.
>
> > ... and what's going to happen when they pass in a dropped column,
> > either via number or name?
>
> Well, it'll have to fail for the name case, but not for the attnum case.

Why?

> > I don't have an issue with throwing a failure for name-based lookups but
> > returning null for failing numerically-based lookups, but I don't really
> > want us throwing errors on dropped columns.  I would think we'd return
> > null in that case.
>
> You can't have it both ways.  Either you throw an error if the name's
> not there, or you don't.

I'm not following why we couldn't handle a dropped column differently.

Dropped tables don't hang around in the catalog long after they've been
dropped.

> > In particular, I can see this function being used in
> > a where clause across pg_attribute.
>
> As said above, it can work as long as you use attnum not attname.
> I don't think this is really so different from iterating across
> pg_class (or any other catalog) and passing relname instead of OID.

If you really insist that we not do something better when it comes to
dropped columns then I'll drop my argument, but I do see dropped columns
as a materially different thing from dropped tables because of how we
keep them around in the catalog.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> You can't have it both ways.  Either you throw an error if the name's
>> not there, or you don't.

> I'm not following why we couldn't handle a dropped column differently.

Different from what?  A name-based lookup isn't going to find a dropped
column, because its attname has been replaced with
"........pg.dropped.N........"

> Dropped tables don't hang around in the catalog long after they've been
> dropped.

If you are talking about the case where a lookup by attnum finds a dropped
column, that does return null already, cf column_privilege_check().
But I don't see a way for a name-based lookup to do the same without
losing all semblance of error detection.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> You can't have it both ways.  Either you throw an error if the name's
> >> not there, or you don't.
>
> > I'm not following why we couldn't handle a dropped column differently.
>
> Different from what?  A name-based lookup isn't going to find a dropped
> column, because its attname has been replaced with
> "........pg.dropped.N........"

My complaint is specifically trying to do something like:

=*# select *
from
  pg_class
  join pg_attribute on (pg_class.oid = pg_attribute.attrelid)
  join pg_namespace on (pg_class.relnamespace = pg_namespace.oid)
                               
where
  has_column_privilege(quote_ident(nspname) || '.' || quote_ident(relname),attname,'SELECT');

and getting this:

ERROR:  column "........pg.dropped.2........" of relation "t1" does not exist

> > Dropped tables don't hang around in the catalog long after they've been
> > dropped.
>
> If you are talking about the case where a lookup by attnum finds a dropped
> column, that does return null already, cf column_privilege_check().
> But I don't see a way for a name-based lookup to do the same without
> losing all semblance of error detection.

Perhaps it's not worth it then, though I still contend that it's pretty
annoying that we throw an error in that case.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> My complaint is specifically trying to do something like:

> =*# select *
> from
>   pg_class
>   join pg_attribute on (pg_class.oid = pg_attribute.attrelid)
>   join pg_namespace on (pg_class.relnamespace = pg_namespace.oid)
                                 
> where
>   has_column_privilege(quote_ident(nspname) || '.' || quote_ident(relname),attname,'SELECT');

> and getting this:

> ERROR:  column "........pg.dropped.2........" of relation "t1" does not exist

That code is kinda broken anyway, because it won't survive relation drops
either.  What you *should* be writing is

    has_column_privilege(pg_class.oid, attnum, 'SELECT');

which is not only not sensitive to these problems but significantly more
efficient.

Having said that, I'm fine with having it return NULL if the given
attname matches an attisdropped column.  What I was on about is what
happens when you write

    has_column_privilege('sometab'::regclass, 'somecol', 'SELECT');

and sometab exists but somecol doesn't.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > My complaint is specifically trying to do something like:
[...]
> That code is kinda broken anyway, because it won't survive relation drops
> either.  What you *should* be writing is

Yes, I agree, but it still seems like we're throwing an ERROR
unnecessairly.

> Having said that, I'm fine with having it return NULL if the given
> attname matches an attisdropped column.

Ok, that's really all I was asking about.

> ... What I was on about is what
> happens when you write
>
>     has_column_privilege('sometab'::regclass, 'somecol', 'SELECT');
>
> and sometab exists but somecol doesn't.

Yeah, having that throw an error seems reasonable to me.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Having said that, I'm fine with having it return NULL if the given
>> attname matches an attisdropped column.

> Ok, that's really all I was asking about.

Ah, we were just talking past each other then :-(.  That behavior existed
already, it wasn't something my draft patch introduced, so I was confused
what you were talking about.

>> ... What I was on about is what
>> happens when you write
>> has_column_privilege('sometab'::regclass, 'somecol', 'SELECT');
>> and sometab exists but somecol doesn't.

> Yeah, having that throw an error seems reasonable to me.

OK, so here's a patch that I think does the right things.

I noticed that has_foreign_data_wrapper_privilege() and some other
recently-added members of the has_foo_privilege family had not gotten
the word about not failing on bogus OIDs, so I also fixed those.

            regards, tom lane

diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index d5285e2..c5f7918 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -2447,8 +2447,12 @@ has_any_column_privilege_id_id(PG_FUNCTION_ARGS)
  *
  *        The result is a boolean value: true if user has the indicated
  *        privilege, false if not.  The variants that take a relation OID
- *        and an integer attnum return NULL (rather than throwing an error)
- *        if the column doesn't exist or is dropped.
+ *        return NULL (rather than throwing an error) if that relation OID
+ *        doesn't exist.  Likewise, the variants that take an integer attnum
+ *        return NULL (rather than throwing an error) if there is no such
+ *        pg_attribute entry.  All variants return NULL if an attisdropped
+ *        column is selected.  These rules are meant to avoid unnecessary
+ *        failures in queries that scan pg_attribute.
  */

 /*
@@ -2466,6 +2470,12 @@ column_privilege_check(Oid tableoid, AttrNumber attnum,
     Form_pg_attribute attributeForm;

     /*
+     * If convert_column_name failed, we can just return -1 immediately.
+     */
+    if (attnum == InvalidAttrNumber)
+        return -1;
+
+    /*
      * First check if we have the privilege at the table level.  We check
      * existence of the pg_class row before risking calling pg_class_aclcheck.
      * Note: it might seem there's a race condition against concurrent DROP,
@@ -2826,21 +2836,59 @@ has_column_privilege_id_attnum(PG_FUNCTION_ARGS)

 /*
  * Given a table OID and a column name expressed as a string, look it up
- * and return the column number
+ * and return the column number.  Returns InvalidAttrNumber in cases
+ * where caller should return NULL instead of failing.
  */
 static AttrNumber
 convert_column_name(Oid tableoid, text *column)
 {
-    AttrNumber    attnum;
     char       *colname;
+    HeapTuple    attTuple;
+    AttrNumber    attnum;

     colname = text_to_cstring(column);
-    attnum = get_attnum(tableoid, colname);
-    if (attnum == InvalidAttrNumber)
-        ereport(ERROR,
-                (errcode(ERRCODE_UNDEFINED_COLUMN),
-                 errmsg("column \"%s\" of relation \"%s\" does not exist",
-                        colname, get_rel_name(tableoid))));
+
+    /*
+     * We don't use get_attnum() here because it will report that dropped
+     * columns don't exist.  We need to treat dropped columns differently from
+     * nonexistent columns.
+     */
+    attTuple = SearchSysCache2(ATTNAME,
+                               ObjectIdGetDatum(tableoid),
+                               CStringGetDatum(colname));
+    if (HeapTupleIsValid(attTuple))
+    {
+        Form_pg_attribute attributeForm;
+
+        attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
+        /* We want to return NULL for dropped columns */
+        if (attributeForm->attisdropped)
+            attnum = InvalidAttrNumber;
+        else
+            attnum = attributeForm->attnum;
+        ReleaseSysCache(attTuple);
+    }
+    else
+    {
+        char       *tablename = get_rel_name(tableoid);
+
+        /*
+         * If the table OID is bogus, or it's just been dropped, we'll get
+         * NULL back.  In such cases we want has_column_privilege to return
+         * NULL too, so just return InvalidAttrNumber.
+         */
+        if (tablename != NULL)
+        {
+            /* tableoid exists, colname does not, so throw error */
+            ereport(ERROR,
+                    (errcode(ERRCODE_UNDEFINED_COLUMN),
+                     errmsg("column \"%s\" of relation \"%s\" does not exist",
+                            colname, tablename)));
+        }
+        /* tableoid doesn't exist, so act like attisdropped case */
+        attnum = InvalidAttrNumber;
+    }
+
     pfree(colname);
     return attnum;
 }
@@ -3144,6 +3192,9 @@ has_foreign_data_wrapper_privilege_name_id(PG_FUNCTION_ARGS)
     roleid = get_role_oid_or_public(NameStr(*username));
     mode = convert_foreign_data_wrapper_priv_string(priv_type_text);

+    if (!SearchSysCacheExists1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(fdwid)))
+        PG_RETURN_NULL();
+
     aclresult = pg_foreign_data_wrapper_aclcheck(fdwid, roleid, mode);

     PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3167,6 +3218,9 @@ has_foreign_data_wrapper_privilege_id(PG_FUNCTION_ARGS)
     roleid = GetUserId();
     mode = convert_foreign_data_wrapper_priv_string(priv_type_text);

+    if (!SearchSysCacheExists1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(fdwid)))
+        PG_RETURN_NULL();
+
     aclresult = pg_foreign_data_wrapper_aclcheck(fdwid, roleid, mode);

     PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3211,6 +3265,9 @@ has_foreign_data_wrapper_privilege_id_id(PG_FUNCTION_ARGS)

     mode = convert_foreign_data_wrapper_priv_string(priv_type_text);

+    if (!SearchSysCacheExists1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(fdwid)))
+        PG_RETURN_NULL();
+
     aclresult = pg_foreign_data_wrapper_aclcheck(fdwid, roleid, mode);

     PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3910,6 +3967,9 @@ has_server_privilege_name_id(PG_FUNCTION_ARGS)
     roleid = get_role_oid_or_public(NameStr(*username));
     mode = convert_server_priv_string(priv_type_text);

+    if (!SearchSysCacheExists1(FOREIGNSERVEROID, ObjectIdGetDatum(serverid)))
+        PG_RETURN_NULL();
+
     aclresult = pg_foreign_server_aclcheck(serverid, roleid, mode);

     PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3933,6 +3993,9 @@ has_server_privilege_id(PG_FUNCTION_ARGS)
     roleid = GetUserId();
     mode = convert_server_priv_string(priv_type_text);

+    if (!SearchSysCacheExists1(FOREIGNSERVEROID, ObjectIdGetDatum(serverid)))
+        PG_RETURN_NULL();
+
     aclresult = pg_foreign_server_aclcheck(serverid, roleid, mode);

     PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3977,6 +4040,9 @@ has_server_privilege_id_id(PG_FUNCTION_ARGS)

     mode = convert_server_priv_string(priv_type_text);

+    if (!SearchSysCacheExists1(FOREIGNSERVEROID, ObjectIdGetDatum(serverid)))
+        PG_RETURN_NULL();
+
     aclresult = pg_foreign_server_aclcheck(serverid, roleid, mode);

     PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -4092,6 +4158,9 @@ has_tablespace_privilege_name_id(PG_FUNCTION_ARGS)
     roleid = get_role_oid_or_public(NameStr(*username));
     mode = convert_tablespace_priv_string(priv_type_text);

+    if (!SearchSysCacheExists1(TABLESPACEOID, ObjectIdGetDatum(tablespaceoid)))
+        PG_RETURN_NULL();
+
     aclresult = pg_tablespace_aclcheck(tablespaceoid, roleid, mode);

     PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -4115,6 +4184,9 @@ has_tablespace_privilege_id(PG_FUNCTION_ARGS)
     roleid = GetUserId();
     mode = convert_tablespace_priv_string(priv_type_text);

+    if (!SearchSysCacheExists1(TABLESPACEOID, ObjectIdGetDatum(tablespaceoid)))
+        PG_RETURN_NULL();
+
     aclresult = pg_tablespace_aclcheck(tablespaceoid, roleid, mode);

     PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -4159,6 +4231,9 @@ has_tablespace_privilege_id_id(PG_FUNCTION_ARGS)

     mode = convert_tablespace_priv_string(priv_type_text);

+    if (!SearchSysCacheExists1(TABLESPACEOID, ObjectIdGetDatum(tablespaceoid)))
+        PG_RETURN_NULL();
+
     aclresult = pg_tablespace_aclcheck(tablespaceoid, roleid, mode);

     PG_RETURN_BOOL(aclresult == ACLCHECK_OK);

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> Having said that, I'm fine with having it return NULL if the given
> >> attname matches an attisdropped column.
>
> > Ok, that's really all I was asking about.
>
> Ah, we were just talking past each other then :-(.  That behavior existed
> already, it wasn't something my draft patch introduced, so I was confused
> what you were talking about.

No worries.

> >> ... What I was on about is what
> >> happens when you write
> >> has_column_privilege('sometab'::regclass, 'somecol', 'SELECT');
> >> and sometab exists but somecol doesn't.
>
> > Yeah, having that throw an error seems reasonable to me.
>
> OK, so here's a patch that I think does the right things.
>
> I noticed that has_foreign_data_wrapper_privilege() and some other
> recently-added members of the has_foo_privilege family had not gotten
> the word about not failing on bogus OIDs, so I also fixed those.

I just glanced through it pretty quickly, but looks good to me.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> OK, so here's a patch that I think does the right things.
>> I noticed that has_foreign_data_wrapper_privilege() and some other
>> recently-added members of the has_foo_privilege family had not gotten
>> the word about not failing on bogus OIDs, so I also fixed those.

> I just glanced through it pretty quickly, but looks good to me.

Pushed with some test cases; thanks for reviewing!

BTW, I noticed while making the test cases that there are some odd-seeming
behaviors as a result of early exits from the test functions.  For
instance,

regression=# create table mytab(f1 int, f2 int);
CREATE TABLE
regression=# select has_column_privilege('mytab',99::int2,'select');
 has_column_privilege 
----------------------
 t
(1 row)

One might reasonably expect NULL there, but column_privilege_check
observes that you have table-level select privilege so it doesn't
bother to look up the column number.  Not sure if this is worth
doing something about.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> OK, so here's a patch that I think does the right things.
> >> I noticed that has_foreign_data_wrapper_privilege() and some other
> >> recently-added members of the has_foo_privilege family had not gotten
> >> the word about not failing on bogus OIDs, so I also fixed those.
>
> > I just glanced through it pretty quickly, but looks good to me.
>
> Pushed with some test cases; thanks for reviewing!

Thanks for hacking on it.

> BTW, I noticed while making the test cases that there are some odd-seeming
> behaviors as a result of early exits from the test functions.  For
> instance,
>
> regression=# create table mytab(f1 int, f2 int);
> CREATE TABLE
> regression=# select has_column_privilege('mytab',99::int2,'select');
>  has_column_privilege
> ----------------------
>  t
> (1 row)

Ah, yeah, that's the whole "you have access to all columns if you have
SELECT rights on the table".

> One might reasonably expect NULL there, but column_privilege_check
> observes that you have table-level select privilege so it doesn't
> bother to look up the column number.  Not sure if this is worth
> doing something about.

Yeah, I'm on the fence about if it makes sense to do anything here or
not.  Hard to see how getting a NULL back is really more useful in this
case.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> One might reasonably expect NULL there, but column_privilege_check
>> observes that you have table-level select privilege so it doesn't
>> bother to look up the column number.  Not sure if this is worth
>> doing something about.

> Yeah, I'm on the fence about if it makes sense to do anything here or
> not.  Hard to see how getting a NULL back is really more useful in this
> case.

Yeah, I'm not terribly excited about changing it either.  I just
thought it'd be a good idea to point it out in case somebody else
feels differently.

            regards, tom lane