Horton

Thread

Commits

  1. Fix corner-case failures in has_foo_privilege() family of functions.

  1. Assert failed in snprintf.c

    Jaime Casanova <jaime.casanova@2ndquadrant.com> — 2018-10-01T04:58:03Z

    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

  2. Re: Assert failed in snprintf.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-01T14:48:30Z

    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

  3. has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-01T17:47:35Z

    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

  4. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Stephen Frost <sfrost@snowman.net> — 2018-10-01T18:41:53Z

    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

  5. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-01T18:57:54Z

    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

  6. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Joe Conway <mail@joeconway.com> — 2018-10-01T19:00:42Z

    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

  7. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Stephen Frost <sfrost@snowman.net> — 2018-10-01T19:05:53Z

    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

  8. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-01T19:24:46Z

    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

  9. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Stephen Frost <sfrost@snowman.net> — 2018-10-01T19:39:14Z

    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

  10. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-01T19:46:34Z

    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

  11. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Stephen Frost <sfrost@snowman.net> — 2018-10-01T19:54:59Z

    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

  12. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-01T20:27:19Z

    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

  13. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Stephen Frost <sfrost@snowman.net> — 2018-10-01T21:11:41Z

    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

  14. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-01T23:31:31Z

    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

  15. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Stephen Frost <sfrost@snowman.net> — 2018-10-02T10:46:42Z

    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

  16. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-02T15:58:53Z

    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

  17. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Stephen Frost <sfrost@snowman.net> — 2018-10-02T16:10:49Z

    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

  18. Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-02T16:45:04Z

    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