Re: OCLASS_ROWSECURITY oversights, and other kvetching

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> The RLS patch added OCLASS_ROWSECURITY but it seems that not enough
> effort was made to grep for places that might require adjustment as a
> result.
>
> In objectaddress.c, getObjectDescription() was updated, but
> getObjectTypeDescription() and getObjectIdentity() were not.
>
> In dependency.c, object_classes didn't get updated.

I'm trying to recall why I didn't think it was necessary to add it into
more places.. I did do the 'grep' as described. I'll go back and
review these.

> I also really question why we've got OCLASS_ROWSECURITY but
> OBJECT_POLICY. In most cases, we name the OBJECT_* construct and the
> OCLASS_* construct similarly. This is actually just the tip of the
> iceberg: we've got OBJECT_POLICY but OCLASS_ROWSECURITY (no underscore
> between row and security) and then we've got DO_ROW_SECURITY (with an
> underscore) and pg_row_security.

I'm guessing you mean pg_rowsecurity.. DO_ROW_SECURITY is in pg_dump
only and the ROW_SECURITY cases in the backend are representing the
'row_security' GUC. That said, I'm not against changing things to be
more consistent, of course. In this case, pg_rowsecurity should really
be pg_policies as that's what's actually in that catalog. The original
naming was from the notion that the table-level attribute is
'ROW LEVEL SECURITY', but on reflection it's clearer to have it as
pg_policies.

> But then on the other hand the
> source code is in policy.c.

Right, the functions for dealing with policies are in policy.c, while
the actual implementation of the table-level 'ROW LEVEL SECURITY'
attribute is in rowsecurity.c.

> pg_dump tries to sit on the fence by
> alternating between all the different names and sometimes combining
> them (row-security policy). Some places refer to row-LEVEL security
> rather than row security or policies.

There's three different things happening in pg_dump, which I suspect is
why it's gotten inconsistent. There's setting the ROW_SECURITY GUC,
dumping the fact that the table has been set to ENABLE ROW LEVEL
SECURITY, and dumping out the actual policies which are defined on the
table.

> I think this kind of messiness makes code really hard to maintain and
> should be cleaned up now while we have a chance. For the most part,
> we have chosen to name our catalogs, SQL commands, and internal
> constants by *what kind of object it is* (in this case, a policy)
> rather than by *the feature it provides* (in this case, row security).
> So I think that everything relates to a policy specifically
> (OCLASS_ROWSECURITY, pg_row_security, etc.) should be renamed to refer
> to policies instead. The references to row security should be
> preserved only when we are talking about the table-level property,
> which is actually called ROW SECURITY, or the feature in general.

I certainly agree that it can and should be improved. Given that the
table property is ROW SECURITY, I'd think we would keep the GUC as
'ROW_SECURITY', but change all of the places which are currently working
with policies to use POLICY, such as OCLASS_ROWSECURITY ->
OCLASS_POLICY. I'll go back through and review it with these three
distinctions top-of-mind and work out what other changes make sense.

Thanks!

Stephen