Thread: OCLASS_ROWSECURITY oversights, and other kvetching
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 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. But then on the other hand the source code is in policy.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. 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.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
Robert, all,
Apologies for this taking so long, it was a bit bigger than I had
anticipated and other things were going on also, as happens.
* Stephen Frost (sfrost@snowman.net) wrote:
> * Robert Haas (robertmhaas@gmail.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.
These should now all be addressed. Please let me know if there are any
cases you're aware of which were missed.
> > 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.
I've changed this to 'pg_policy', following existing convention. There
is also a 'pg_policies' view which makes it a bit more 'user-friendly',
similar to things like 'pg_index' and 'pg_indexes'.
> > 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.
Hopefully this all looks better now- pg_policy.h goes with policy.c,
etc.
> > 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've attempted to clean this up as well, but please feel free to point
out any remaining issues or concerns.
> > 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.
I've just pushed one set of changes regarding this but I'll be looking
at it further over the next few days, and please feel free to point out
any issues or concerns.
Thanks!
Stephen