Обсуждение: Placement of permissions checks for large object operations

While looking at the 64-bit large object patch, I happened to notice
that the LO permissions checks that were added a release or two back
seem to be done exceedingly inefficiently.  To wit, they're done in
lo_read() and lo_write(), which means that if the user say reads an 8MB
large object with an 8K buffer size, we'll repeat the same permissions
check 1000 times.

This is particularly bizarre on the read side, where the code has gone
to probably-unreasonable lengths to make dead certain that it will get
the same answer each time.  But even on the write side, it's not
apparent to me that it's useful or sensible to allow a few writes and
then stop allowing them if some other transaction commits an ACL change
meanwhile.  (There would be a race condition there anyway, since our
transaction might examine the ACL just before somebody else changes it.)

I thought about proposing that the permissions checks be done in lo_open
instead.  However, that would result in semantic changes --- for
instance, it's legal and sometimes useful to supply INV_WRITE and not
actually write, if you want up-to-date read results.  So we can't really
enforce permissions at open time based on the supplied flags.

However, it wouldn't be hard to make the code apply the checks at most
once per lo_open, by keeping flags in the LargeObjectDesc entries
showing whether we've already checked read or write privilege on each
descriptor.  So the check would be made only during the first lo_read
or lo_write call on a given descriptor.

Comments?
        regards, tom lane

On Mon, Oct 8, 2012 at 6:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> While looking at the 64-bit large object patch, I happened to notice
> that the LO permissions checks that were added a release or two back
> seem to be done exceedingly inefficiently.  To wit, they're done in
> lo_read() and lo_write(), which means that if the user say reads an 8MB
> large object with an 8K buffer size, we'll repeat the same permissions
> check 1000 times.
>
> This is particularly bizarre on the read side, where the code has gone
> to probably-unreasonable lengths to make dead certain that it will get
> the same answer each time.  But even on the write side, it's not
> apparent to me that it's useful or sensible to allow a few writes and
> then stop allowing them if some other transaction commits an ACL change
> meanwhile.  (There would be a race condition there anyway, since our
> transaction might examine the ACL just before somebody else changes it.)
>
> I thought about proposing that the permissions checks be done in lo_open
> instead.  However, that would result in semantic changes --- for
> instance, it's legal and sometimes useful to supply INV_WRITE and not
> actually write, if you want up-to-date read results.  So we can't really
> enforce permissions at open time based on the supplied flags.
>
> However, it wouldn't be hard to make the code apply the checks at most
> once per lo_open, by keeping flags in the LargeObjectDesc entries
> showing whether we've already checked read or write privilege on each
> descriptor.  So the check would be made only during the first lo_read
> or lo_write call on a given descriptor.

Seems like a good idea.

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