Thread: 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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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