I have browsed this patch set a bit to get myself up to date.
General thoughts:
This is a large patch set, and it's not going to be perfect at the first
try. Especially, some of the parsing rules, query semantics, that kind
of thing. So I'm thinking that we should aim at integrating this with
an understanding that it would be somewhat experimental, and then
iterate on some of the details in the tree. Obviously, that would still
require that the overall architecture is ok, that it doesn't crash, that
it satisfies security requirements. Also, it should as much as possible
follow the "if you don't use it, it doesn't affect you" rule. So I'm
specifically looking at where the patch intersects with existing code in
critical ways, especially in parse analysis. I want to spend more time
reviewing that in particular. Much of the rest of patch is
almost-boilerplate: New DDL commands, new catalogs, associated tests and
documentation. It looks a lot, but most of it is not very surprising.
I found two areas where a bit more work could be done to separate the
new code from existing code:
src/backend/utils/cache/inval.c: This looks suspicious. The existing
code only deals with very fundamental catalogs such as pg_class and
pg_index. It doesn't feel right to hardcode additional arguably very
high-level PGQ-related catalogs there. We should think of a different
approach here.
src/test/regress/sql/rowsecurity.sql: I think it would be better to
separate the new test cases into a separate file. This test file is
already large and complicated, and sprinkling a bunch of more stuff all
over the place is going to make review and maintenance more complicated.
We also need to make sure that property graphs have security features
equivalent to views. For example, I suspect we need to integrate them
with restrict_nonsystem_relation_kind. There might be other similar
things, to be checked.
