Обсуждение: pgsql: RLS refactoring
RLS refactoring This refactors rewrite/rowsecurity.c to simplify the handling of the default deny case (reducing the number of places where we check for and add the default deny policy from three to one) by splitting up the retrival of the policies from the application of them. This also allowed us to do away with the policy_id field. A policy_name field was added for WithCheckOption policies and is used in error reporting, when available. Patch by Dean Rasheed, with various mostly cosmetic changes by me. Back-patch to 9.5 where RLS was introduced to avoid unnecessary differences, since we're still in alpha, per discussion with Robert. Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/22eaf35c1d247407b7cf1fffb310a26cd9b9ceb1 Modified Files -------------- src/backend/commands/policy.c | 41 - src/backend/executor/execMain.c | 20 +- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/equalfuncs.c | 1 + src/backend/nodes/outfuncs.c | 1 + src/backend/nodes/readfuncs.c | 1 + src/backend/rewrite/rewriteHandler.c | 5 +- src/backend/rewrite/rowsecurity.c | 816 ++++++++++---------- src/backend/utils/cache/relcache.c | 2 - src/include/nodes/parsenodes.h | 1 + src/include/rewrite/rowsecurity.h | 3 +- .../test_rls_hooks/expected/test_rls_hooks.out | 10 +- src/test/modules/test_rls_hooks/test_rls_hooks.c | 2 - 13 files changed, 447 insertions(+), 457 deletions(-)
Stephen Frost <sfrost@snowman.net> writes:
> RLS refactoring
It looks to me like this changed the representation of stored rules, so it
should have included a catversion bump. This is particularly relevant to
the 9.5 branch where people already have alpha installations.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > RLS refactoring > > It looks to me like this changed the representation of stored rules, so it > should have included a catversion bump. This is particularly relevant to > the 9.5 branch where people already have alpha installations. I had considererd if a bump was needed and figured it wasn't. The WithCheckOption node which was changed doesn't ever end up in the catalog, I don't believe; certainly not in pg_policy which just stores the expressions which come from transformWhereClause, which haven't changed. I don't mind doing a bump if we feel it's necessary and maybe I'm missing that there's a way to cause that node type to end up in the catalog, but I don't think so, as we only ever build WithCheckOption nodes in the rewriter. Thanks! Stephen
Вложения
Stephen Frost wrote: > * Tom Lane (tgl@sss.pgh.pa.us) wrote: > > Stephen Frost <sfrost@snowman.net> writes: > > > > It looks to me like this changed the representation of stored rules, so it > > should have included a catversion bump. This is particularly relevant to > > the 9.5 branch where people already have alpha installations. > > I had considererd if a bump was needed and figured it wasn't. > > The WithCheckOption node which was changed doesn't ever end up in the > catalog, I don't believe; certainly not in pg_policy which just stores > the expressions which come from transformWhereClause, which haven't > changed. Uhm, so why is it in readfuncs.c? If you create a view "WITH CHECK OPTION", the pg_rewrite row says ":withCheckOptions <>". Does that not change with your commit? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> It looks to me like this changed the representation of stored rules, so it
>> should have included a catversion bump. This is particularly relevant to
>> the 9.5 branch where people already have alpha installations.
> I had considererd if a bump was needed and figured it wasn't.
> I don't mind doing a bump if we feel it's necessary and maybe I'm
> missing that there's a way to cause that node type to end up in the
> catalog, but I don't think so, as we only ever build WithCheckOption
> nodes in the rewriter.
Oh, I see. In that case you should remove WithCheckOption from the set of
node types supported by readfuncs.c, both because it's dead code and to
clarify that the node is not meant to ever end up on disk.
(outfuncs.c support is useful for debugging though, so keep that.)
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > * Tom Lane (tgl@sss.pgh.pa.us) wrote: > >> It looks to me like this changed the representation of stored rules, so it > >> should have included a catversion bump. This is particularly relevant to > >> the 9.5 branch where people already have alpha installations. > > > I had considererd if a bump was needed and figured it wasn't. > > > I don't mind doing a bump if we feel it's necessary and maybe I'm > > missing that there's a way to cause that node type to end up in the > > catalog, but I don't think so, as we only ever build WithCheckOption > > nodes in the rewriter. > > Oh, I see. In that case you should remove WithCheckOption from the set of > node types supported by readfuncs.c, both because it's dead code and to > clarify that the node is not meant to ever end up on disk. Yeah, I was just thinking the same. > (outfuncs.c support is useful for debugging though, so keep that.) Right, makes sense. I should be able to get to that tomorrow afternoon, til then I'm pretty tied up with PostgresOpen. Thanks! Stephen
Вложения
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Stephen Frost wrote:
>> The WithCheckOption node which was changed doesn't ever end up in the
>> catalog, I don't believe; certainly not in pg_policy which just stores
>> the expressions which come from transformWhereClause, which haven't
>> changed.
> Uhm, so why is it in readfuncs.c? If you create a view "WITH CHECK
> OPTION", the pg_rewrite row says ":withCheckOptions <>". Does that not
> change with your commit?
That field would always be NIL in a query produced by the parser;
it's only ever filled by the rewriter. But if this is documented
anywhere, I couldn't find it, and the placement of the field in struct
Query seems designed to be as confusing as possible about that. I'd have
put it down near the end myself, and certainly have documented that it is
NOT the parse-time representation of a WITH CHECK OPTION clause. For that
matter I don't even find it to be named very well, because it's impossible
to avoid that impression with the name as-is. Perhaps something like
insertedCheckClauses would have been better.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > Stephen Frost wrote: > >> The WithCheckOption node which was changed doesn't ever end up in the > >> catalog, I don't believe; certainly not in pg_policy which just stores > >> the expressions which come from transformWhereClause, which haven't > >> changed. > > > Uhm, so why is it in readfuncs.c? If you create a view "WITH CHECK > > OPTION", the pg_rewrite row says ":withCheckOptions <>". Does that not > > change with your commit? > > That field would always be NIL in a query produced by the parser; Right. > it's only ever filled by the rewriter. But if this is documented > anywhere, I couldn't find it, and the placement of the field in struct > Query seems designed to be as confusing as possible about that. I'd have > put it down near the end myself, and certainly have documented that it is > NOT the parse-time representation of a WITH CHECK OPTION clause. For that > matter I don't even find it to be named very well, because it's impossible > to avoid that impression with the name as-is. Perhaps something like > insertedCheckClauses would have been better. Agreed. I will see about improving on that situation with at least documentation changes. If we want to remove it completely then we'd need to bump catversion.. Not against doing that if we want to though. Might be better that way. Thanks! Stephen
Вложения
Stephen Frost <sfrost@snowman.net> writes:
> Agreed. I will see about improving on that situation with at least
> documentation changes. If we want to remove it completely then we'd
> need to bump catversion.. Not against doing that if we want to though.
> Might be better that way.
readfuncs.c doesn't actually stop to verify that the field name in stored
rules is what it expects. So I believe (but you'd better check) that
renaming the field could be done without initdb. If we wanted to change
its placement, we'd need initdb --- unless we wanted to move it in the
struct definition but not in _outQuery/_readQuery, which I wouldn't do
in HEAD but it might be acceptable in back branches.
So the limiting factor here is not initdb but avoiding an API/ABI break
for extensions that look at struct Query. It's clearly too late to do any
such thing in 9.4, but we still could accept API breaks for 9.5 I think.
My vote would be to rename and reposition the field in HEAD and 9.5
and accept the corresponding initdb. We already forced an initdb
since alpha2, so it's basically free as far as testers are concerned,
and keeping 9.5 in sync with HEAD in this area seems like a really
good idea for awhile to come.
regards, tom lane
On 2015-09-15 19:16:06 -0400, Tom Lane wrote: > readfuncs.c doesn't actually stop to verify that the field name in stored > rules is what it expects. This reminds me: Is there a reason for that? ISTM that adding checks for the field names would make error messages about borked stored trees much easier to understand? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2015-09-15 19:16:06 -0400, Tom Lane wrote:
>> readfuncs.c doesn't actually stop to verify that the field name in stored
>> rules is what it expects.
> This reminds me: Is there a reason for that? ISTM that adding checks for
> the field names would make error messages about borked stored trees much
> easier to understand?
Dunno, how often have you seen such an error message, and how much would
it help anyway? I'm not sure it'd be worth the code and cycles to check.
regards, tom lane
Tom, all, * Tom Lane (tgl@sss.pgh.pa.us) wrote: > My vote would be to rename and reposition the field in HEAD and 9.5 > and accept the corresponding initdb. We already forced an initdb > since alpha2, so it's basically free as far as testers are concerned, > and keeping 9.5 in sync with HEAD in this area seems like a really > good idea for awhile to come. Alright, attached is an attempt at doing these renames. I went a bit farther since it seemed to make sense to me (at least at the time, I'm wondering a bit about it now), so, please provide any thoughts or feedback you have regarding these changes. Thanks! Stephen