Re: [v9.2] SECURITY LABEL on shared database object

On Wed, Jul 6, 2011 at 1:25 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2011/7/5 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>> Excerpts from Kohei Kaigai's message of mar jul 05 11:46:06 -0400 2011:
>>> > On Tue, Jul 5, 2011 at 10:49 AM, Alvaro Herrera
>>> > <alvherre(at)commandprompt(dot)com> wrote:
>>> > > Excerpts from Robert Haas's message of mar jul 05 10:19:18 -0400 2011:
>>> > >
>>> > >> Hmm, OK.  I guess what I'm not sure about is - how much should we
>>> > >> worry about the fact that this creates several more shared (and
>>> > >> therefore nailed?) system catalogs?  Anyone have an opinion on that?
>>> > >
>>> > > "Several"?  That would worry me, given that we currently have a small
>>> > > number (eight currently).  If it's just one more, I don't think it's
>>> > > such a big deal.  I'm not sure what you mean by nailed though -- I mean,
>>> > > for example pg_shdescription is shared but not nailed in the rd_isnailed
>>> > > sense of the word, AFAICS.
>>> >
>>> > Well, right now the patch has pg_shseclabel, and its index, plus a
>>> > toast table and a toast index.  Not sure why we want/need the toast
>>> > table & index there, but the patch has 'em as of now.
>>> >
>>> As a common belief, TEXT is a variable length data type, so pg_shseclabel
>>> need to have its toast table. However, I don't expect the label field get
>>> represented as a reference to external pointer, because average length of
>>> security context is about 40-60 bytes much less than the threshold to
>>> launch toast_save_datum().
>>> Do I need to remove these toast table & index?
>>
>> We don't have toast tables for pg_database and so on, for example, which
>> means that datacl cannot go over a few hundred bytes long.  I think it
>> makes sense to not have toast tables for pg_shseclabel.  Keep in mind
>> that the label might be compressed before it's stored out of line, which
>> gives you quite a bit of actual space.  If a security context is over
>> 5000 bytes in length I think you're in trouble :-)
>>
> The attached patch removes toast table & index for pg_shseclabel.
>
> The current toasting.h defines toast table & index on pg_database,
> pg_shdescription and pg_db_role_setting only.
> The pg_authid and pg_tablespace don't have toast table & index
> in spite of variable-length field.
> So, it might not be a necessary stuff for all the shared relations.

I have committed this with fairly extensive revisions. The main
things I changed were:

- The pg_dump/pg_dumpall support was broken for databases and
needlessly inefficient for roles and tablespaces. (Parenthetically,
it is hard to blame anyone for screwing up the code here when it is
spaghetti code to begin with.)

- I did not commit the contrib/sepgsql parts of the patch, as I
haven't reviewed them. I think it would be helpful for you to
resubmit those separately.

- I didn't think it was a good idea to make pg_shseclabel.provider a
name while leaving pg_seclabel.provider as a text field. Surely these
two catalogs need to stay parallel. For the same reason, I didn't
like the idea of installing a syscache for pg_shseclabel while
pg_seclabel soldiers on without one. So for now I ripped out that
logic and instead made it work the old, slow way. I know we need a
better solution here, but I want to come up with a plan that handles
both catalogs symmetrically and then do it all at once, rather than
piecemeal.

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