Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Sat, Jul 09, 2011 at 10:52:33AM +0200, Kohei KaiGai wrote:
> 2011/7/9 Noah Misch :
> > On Sat, Jul 09, 2011 at 09:00:30AM +0200, Kohei KaiGai wrote:
> >> The attached patch is a revised version according to the approach that
> >> updates
> >> pg_class system catalog before AlterTableInternal().
> >> It invokes the new ResetViewOptions when rel->rd_options is not null, and
> >> it set
> >> null on the pg_class.reloptions of the view and increments command counter.
> >
> >> + /*
> >> + * ResetViewOptions
> >> + *
> >> + * It clears all the reloptions prior to replacing
> >> + */
> >> + static void
> >> + ResetViewOptions(Oid viewOid)
> >> + {
> >> + Relation pg_class;
> >> + HeapTuple oldtup;
> >> + HeapTuple newtup;
> >> + Datum values[Natts_pg_class];
> >> + bool nulls[Natts_pg_class];
> >> + bool replaces[Natts_pg_class];
> >> +
> >> + pg_class = heap_open(RelationRelationId, RowExclusiveLock);
> >> +
> >> + oldtup = SearchSysCache1(RELOID, DatumGetObjectId(viewOid));
> >
> > Use SearchSysCacheCopy1, since you're modifying the tuple.
> >
> The heap_modify_tuple() allocates a new tuple as a copy of old tuple.
> No need to worry about.
Ah, yes. Sorry for the noise.
> >> + if (!HeapTupleIsValid(oldtup))
> >> + elog(ERROR, "cache lookup failed for relation %u", viewOid);
> >> +
> >> + memset(values, 0, sizeof(values));
> >> + memset(nulls, false, sizeof(nulls));
> >> + memset(replaces, false, sizeof(replaces));
> >> +
> >> + replaces[Anum_pg_class_reloptions - 1] = true;
> >> + nulls[Anum_pg_class_reloptions - 1] = true;
> >> +
> >> + newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_class),
> >> + values, nulls,
> >> replaces);
> >> + simple_heap_update(pg_class, &newtup->t_self, newtup);
> >> +
> >> + CatalogUpdateIndexes(pg_class, newtup);
> >> +
> >> + ReleaseSysCache(oldtup);
> >> +
> >> + heap_close(pg_class, RowExclusiveLock);
> >> +
> >> + CommandCounterIncrement();
> >
> > Why is a CCI necessary?
> >
> ATExecSetRelOptions() reference the view to be updated using syscache,
> however, this update will not become visible without CCI.
> In the result, it will reference old tuple, then get an error because
> it tries to
> update already updated tuple.
Okay, thanks.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
2011/7/9 Noah Misch :
> On Sat, Jul 09, 2011 at 09:00:30AM +0200, Kohei KaiGai wrote:
>> The attached patch is a revised version according to the approach that
>> updates
>> pg_class system catalog before AlterTableInternal().
>> It invokes the new ResetViewOptions when rel->rd_options is not null, and it
>> set
>> null on the pg_class.reloptions of the view and increments command counter.
>
>> + /*
>> + * ResetViewOptions
>> + *
>> + * It clears all the reloptions prior to replacing
>> + */
>> + static void
>> + ResetViewOptions(Oid viewOid)
>> + {
>> + Relation pg_class;
>> + HeapTuple oldtup;
>> + HeapTuple newtup;
>> + Datum values[Natts_pg_class];
>> + bool nulls[Natts_pg_class];
>> + bool replaces[Natts_pg_class];
>> +
>> + pg_class = heap_open(RelationRelationId, RowExclusiveLock);
>> +
>> + oldtup = SearchSysCache1(RELOID, DatumGetObjectId(viewOid));
>
> Use SearchSysCacheCopy1, since you're modifying the tuple.
>
The heap_modify_tuple() allocates a new tuple as a copy of old tuple.
No need to worry about.
>> + if (!HeapTupleIsValid(oldtup))
>> + elog(ERROR, "cache lookup failed for relation %u", viewOid);
>> +
>> + memset(values, 0, sizeof(values));
>> + memset(nulls, false, sizeof(nulls));
>> + memset(replaces, false, sizeof(replaces));
>> +
>> + replaces[Anum_pg_class_reloptions - 1] = true;
>> + nulls[Anum_pg_class_reloptions - 1] = true;
>> +
>> + newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_class),
>> + values, nulls,
>> replaces);
>> + simple_heap_update(pg_class, &newtup->t_self, newtup);
>> +
>> + CatalogUpdateIndexes(pg_class, newtup);
>> +
>> + ReleaseSysCache(oldtup);
>> +
>> + heap_close(pg_class, RowExclusiveLock);
>> +
>> + CommandCounterIncrement();
>
> Why is a CCI necessary?
>
ATExecSetRelOptions() reference the view to be updated using syscache,
however, this update will not become visible without CCI.
In the result, it will reference old tuple, then get an error because
it tries to
update already updated tuple.
>> + }
>
> In any event, we seem to be converging on a version of parts 0 and 1 that are
> ready for committer. However, Robert contends that this will not be committed
> separately from part 2. Unless someone wishes to contest that, I suggest we
> mark this Returned with Feedback and let the CF entry for part 2 subsume its
> future development. Does that sound reasonable?
>
At least, it seems to me we don't need to tackle to this matter from
the beginning
on the next commit fest again.
Thanks,
--
KaiGai Kohei
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Sat, Jul 09, 2011 at 09:00:30AM +0200, Kohei KaiGai wrote:
> The attached patch is a revised version according to the approach that updates
> pg_class system catalog before AlterTableInternal().
> It invokes the new ResetViewOptions when rel->rd_options is not null, and it
> set
> null on the pg_class.reloptions of the view and increments command counter.
> + /*
> + * ResetViewOptions
> + *
> + * It clears all the reloptions prior to replacing
> + */
> + static void
> + ResetViewOptions(Oid viewOid)
> + {
> + Relationpg_class;
> + HeapTuple oldtup;
> + HeapTuple newtup;
> + Datum values[Natts_pg_class];
> + boolnulls[Natts_pg_class];
> + boolreplaces[Natts_pg_class];
> +
> + pg_class = heap_open(RelationRelationId, RowExclusiveLock);
> +
> + oldtup = SearchSysCache1(RELOID, DatumGetObjectId(viewOid));
Use SearchSysCacheCopy1, since you're modifying the tuple.
> + if (!HeapTupleIsValid(oldtup))
> + elog(ERROR, "cache lookup failed for relation %u", viewOid);
> +
> + memset(values, 0, sizeof(values));
> + memset(nulls, false, sizeof(nulls));
> + memset(replaces, false, sizeof(replaces));
> +
> + replaces[Anum_pg_class_reloptions - 1] = true;
> + nulls[Anum_pg_class_reloptions - 1] = true;
> +
> + newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_class),
> +values, nulls,
> replaces);
> + simple_heap_update(pg_class, &newtup->t_self, newtup);
> +
> + CatalogUpdateIndexes(pg_class, newtup);
> +
> + ReleaseSysCache(oldtup);
> +
> + heap_close(pg_class, RowExclusiveLock);
> +
> + CommandCounterIncrement();
Why is a CCI necessary?
> + }
In any event, we seem to be converging on a version of parts 0 and 1 that are
ready for committer. However, Robert contends that this will not be committed
separately from part 2. Unless someone wishes to contest that, I suggest we
mark this Returned with Feedback and let the CF entry for part 2 subsume its
future development. Does that sound reasonable?
Thanks,
nm
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
2011/7/8 Noah Misch : > On Fri, Jul 08, 2011 at 09:20:46AM +0100, Kohei KaiGai wrote: >> 2011/7/7 Noah Misch : >> > On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote: >> >> 2011/7/7 Noah Misch : >> >> > On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote: > >> >> > That gets the job done for today, but DefineVirtualRelation() should >> >> > not need >> >> > to know all view options by name to simply replace the existing list >> >> > with a >> >> > new one. ?I don't think you can cleanly use the ALTER TABLE SET/RESET >> >> > code for >> >> > this. ?Instead, compute an option list similar to how DefineRelation() >> >> > does so >> >> > at tablecmds.c:491, then update pg_class. >> >> > >> >> My opinion is ALTER TABLE SET/RESET code should be enhanced to accept >> >> an operation to reset all the existing options, rather than tricky >> >> updates of pg_class. >> > >> > The pg_class update has ~20 lines of idiomatic code; see >> > tablecmds.c:7931-7951. >> > >> Even if idiomatic, another part of DefineVirtualRelation() uses >> AlterTableInternal(). >> I think a common way is more straightforward. > > The fact that we use ALTER TABLE ADD COLUMN in DefineVirtualRelation() is not > itself cause to use ALTER TABLE SET (...) nearby. We should do so only if it > brings some advantage, like simpler or more-robust code. I'm not seeing > either > advantage. Those can be points of style, so perhaps I have the poor taste > here. > The attached patch is a revised version according to the approach that updates pg_class system catalog before AlterTableInternal(). It invokes the new ResetViewOptions when rel->rd_options is not null, and it set null on the pg_class.reloptions of the view and increments command counter. Rest of stuffs are not changed from the v5. Thanks, -- KaiGai Kohei pgsql-v9.2-fix-leaky-view-part-0.v6.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Fri, Jul 08, 2011 at 09:20:46AM +0100, Kohei KaiGai wrote: > 2011/7/7 Noah Misch : > > On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote: > >> 2011/7/7 Noah Misch : > >> > On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote: > >> > That gets the job done for today, but DefineVirtualRelation() should not > >> > need > >> > to know all view options by name to simply replace the existing list > >> > with a > >> > new one. ?I don't think you can cleanly use the ALTER TABLE SET/RESET > >> > code for > >> > this. ?Instead, compute an option list similar to how DefineRelation() > >> > does so > >> > at tablecmds.c:491, then update pg_class. > >> > > >> My opinion is ALTER TABLE SET/RESET code should be enhanced to accept > >> an operation to reset all the existing options, rather than tricky > >> updates of pg_class. > > > > The pg_class update has ~20 lines of idiomatic code; see > > tablecmds.c:7931-7951. > > > Even if idiomatic, another part of DefineVirtualRelation() uses > AlterTableInternal(). > I think a common way is more straightforward. The fact that we use ALTER TABLE ADD COLUMN in DefineVirtualRelation() is not itself cause to use ALTER TABLE SET (...) nearby. We should do so only if it brings some advantage, like simpler or more-robust code. I'm not seeing either advantage. Those can be points of style, so perhaps I have the poor taste here. > So, how about an idea to add a function that pull-out existing options > from syscache, > and merge with the supplied options list prior to AlterTableInternal()? It seems wrong to me to trawl through the view's existing option list just to replace it completely with a new option list. Again, it's subjective. If you'd like to proceed with this and let the committer decide, that's fine with me. Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
The attached patch is a revised one; that utilizes untransformRelOptions()
to construct a list of DefElem to be supplied into AT_ResetRelOptions
commands. It enabled me to implement more compact as I expected.
How about this approach to reset existing reloptions?
I'll consolidate part-0, 1 and 2 patches after we make fix the direction to
distinguish leaky qualifiers from others, in the thread of part-2.
Right now, I'm considering the right way to choose qualifiers to be
transformed into index scans.
Thanks,
2011/7/7 Noah Misch :
> On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote:
>> 2011/7/7 Noah Misch :
>> > On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:
>> >> *** a/src/backend/commands/view.c
>> >> --- b/src/backend/commands/view.c
>> >
>> >> --- 227,257
>> >> atcmd->def = (Node *) lfirst(c);
>> >> atcmds = lappend(atcmds, atcmd);
>> >> }
>> >> }
>> >>
>> >> /*
>> >> + * If optional parameters are specified, we must set options
>> >> + * using ALTER TABLE SET OPTION internally.
>> >> + */
>> >> + if (list_length(options) > 0)
>> >> + {
>> >> + atcmd = makeNode(AlterTableCmd);
>> >> + atcmd->subtype = AT_SetRelOptions;
>> >> + atcmd->def = (List *)options;
>> >> +
>> >> + atcmds = lappend(atcmds, atcmd);
>> >> + }
>> >> + else
>> >> + {
>> >> + atcmd = makeNode(AlterTableCmd);
>> >> + atcmd->subtype = AT_ResetRelOptions;
>> >> + atcmd->def = (Node *)
>> >> list_make1(makeDefElem("security_barrier",
>> >> +
>> >> NULL));
>> >> + }
>> >> + if (atcmds != NIL)
>> >> + AlterTableInternal(viewOid, atcmds, true);
>> >> +
>> >> + /*
>> >> * Seems okay, so return the OID of the pre-existing view.
>> >> */
>> >> relation_close(rel, NoLock); /* keep the lock! */
>> >
>> > That gets the job done for today, but DefineVirtualRelation() should not
>> > need
>> > to know all view options by name to simply replace the existing list with a
>> > new one. I don't think you can cleanly use the ALTER TABLE SET/RESET code
>> > for
>> > this. Instead, compute an option list similar to how DefineRelation()
>> > does so
>> > at tablecmds.c:491, then update pg_class.
>> >
>> My opinion is ALTER TABLE SET/RESET code should be enhanced to accept
>> an operation to reset all the existing options, rather than tricky
>> updates of pg_class.
>
> The pg_class update has ~20 lines of idiomatic code; see
> tablecmds.c:7931-7951.
>
>> How about an idea to add AT_ResetAllRelOptions for internal use only?
>
> If some operation is purely internal and does not otherwise benefit from the
> ALTER TABLE infrastructure, there's no benefit in involving ALTER TABLE.
> DefineVirtualRelation() uses ALTER TABLE to add columns because all that code
> needs to exist anyway. You could make a plain function to do the update that
> gets called from both ATExecSetRelOptions() and DefineVirtualRelation().
>
> Thanks,
> nm
>
--
KaiGai Kohei
*** a/doc/src/sgml/ref/alter_view.sgml
--- b/doc/src/sgml/ref/alter_view.sgml
***
*** 26,31 ALTER VIEW name ALTER [ COLUMN ] name OWNER TO new_owner
ALTER VIEW name RENAME TO new_name
ALTER VIEW name SET SCHEMA new_schema
+ ALTER VIEW name SET ( parameter [= value] [, ... ] )
+ ALTER VIEW name RESET ( parameter [, ... ] )
+
***
*** 102,107 ALTER VIEW name SET SCHEMA
+
+
+ parameter
+
+
+ Name of the view option to be set.
+
+
+
+
+
+ value
+
+
+ The new value for the view option.
+
+
+
*** a/doc/src/sgml/ref/create_view.sgml
--- b/doc/src/sgml/ref/create_view.sgml
***
*** 22,27 PostgreSQL documentation
--- 22,28
CREATE [ OR REPLACE ] [ TEMP | TEMPORARY ] VIEW name [ ( column_name [, ...] ) ]
+ [ WITH ( parameter [= value] [, ... ] ) ]
AS query
***
*** 99,104 CREATE [ OR REPLACE ] [ TEMP | TEMPORARY ] VIEW n
--- 100,128
+ WITH (parameter [= value])
+
+
+ This clause allows to specify optional parameters for a view.
+
+
+ If security_barrier=TRUE is specified, this view
+ shall performs as security barrier that prevent unexpected information
+ leaks. It is a recommendable configuration when the view is defined
+ to apply row-level security, in spite of performance trade-off.
+
+
+ It is a commonly-used technique that
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
2011/7/7 Noah Misch :
> On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote:
>> 2011/7/7 Noah Misch :
>> > On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:
>> >> *** a/src/backend/commands/view.c
>> >> --- b/src/backend/commands/view.c
>> >
>> >> --- 227,257
>> >> atcmd->def = (Node *) lfirst(c);
>> >> atcmds = lappend(atcmds, atcmd);
>> >> }
>> >> }
>> >>
>> >> /*
>> >> + * If optional parameters are specified, we must set options
>> >> + * using ALTER TABLE SET OPTION internally.
>> >> + */
>> >> + if (list_length(options) > 0)
>> >> + {
>> >> + atcmd = makeNode(AlterTableCmd);
>> >> + atcmd->subtype = AT_SetRelOptions;
>> >> + atcmd->def = (List *)options;
>> >> +
>> >> + atcmds = lappend(atcmds, atcmd);
>> >> + }
>> >> + else
>> >> + {
>> >> + atcmd = makeNode(AlterTableCmd);
>> >> + atcmd->subtype = AT_ResetRelOptions;
>> >> + atcmd->def = (Node *)
>> >> list_make1(makeDefElem("security_barrier",
>> >> +
>> >> NULL));
>> >> + }
>> >> + if (atcmds != NIL)
>> >> + AlterTableInternal(viewOid, atcmds, true);
>> >> +
>> >> + /*
>> >> * Seems okay, so return the OID of the pre-existing view.
>> >> */
>> >> relation_close(rel, NoLock); /* keep the lock! */
>> >
>> > That gets the job done for today, but DefineVirtualRelation() should not
>> > need
>> > to know all view options by name to simply replace the existing list with a
>> > new one. I don't think you can cleanly use the ALTER TABLE SET/RESET code
>> > for
>> > this. Instead, compute an option list similar to how DefineRelation()
>> > does so
>> > at tablecmds.c:491, then update pg_class.
>> >
>> My opinion is ALTER TABLE SET/RESET code should be enhanced to accept
>> an operation to reset all the existing options, rather than tricky
>> updates of pg_class.
>
> The pg_class update has ~20 lines of idiomatic code; see
> tablecmds.c:7931-7951.
>
Even if idiomatic, another part of DefineVirtualRelation() uses
AlterTableInternal().
I think a common way is more straightforward.
So, how about an idea to add a function that pull-out existing options
from syscache,
and merge with the supplied options list prior to AlterTableInternal()?
Thanks,
--
KaiGai Kohei
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote:
> 2011/7/7 Noah Misch :
> > On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:
> >> *** a/src/backend/commands/view.c
> >> --- b/src/backend/commands/view.c
> >
> >> --- 227,257
> >> atcmd->def = (Node *) lfirst(c);
> >> atcmds = lappend(atcmds, atcmd);
> >> }
> >> }
> >>
> >> /*
> >> + * If optional parameters are specified, we must set options
> >> + * using ALTER TABLE SET OPTION internally.
> >> + */
> >> + if (list_length(options) > 0)
> >> + {
> >> + atcmd = makeNode(AlterTableCmd);
> >> + atcmd->subtype = AT_SetRelOptions;
> >> + atcmd->def = (List *)options;
> >> +
> >> + atcmds = lappend(atcmds, atcmd);
> >> + }
> >> + else
> >> + {
> >> + atcmd = makeNode(AlterTableCmd);
> >> + atcmd->subtype = AT_ResetRelOptions;
> >> + atcmd->def = (Node *)
> >> list_make1(makeDefElem("security_barrier",
> >> +
> >> NULL));
> >> + }
> >> + if (atcmds != NIL)
> >> + AlterTableInternal(viewOid, atcmds, true);
> >> +
> >> + /*
> >> * Seems okay, so return the OID of the pre-existing view.
> >> */
> >> relation_close(rel, NoLock); /* keep the lock! */
> >
> > That gets the job done for today, but DefineVirtualRelation() should not
> > need
> > to know all view options by name to simply replace the existing list with a
> > new one. I don't think you can cleanly use the ALTER TABLE SET/RESET code
> > for
> > this. Instead, compute an option list similar to how DefineRelation() does
> > so
> > at tablecmds.c:491, then update pg_class.
> >
> My opinion is ALTER TABLE SET/RESET code should be enhanced to accept
> an operation to reset all the existing options, rather than tricky
> updates of pg_class.
The pg_class update has ~20 lines of idiomatic code; see tablecmds.c:7931-7951.
> How about an idea to add AT_ResetAllRelOptions for internal use only?
If some operation is purely internal and does not otherwise benefit from the
ALTER TABLE infrastructure, there's no benefit in involving ALTER TABLE.
DefineVirtualRelation() uses ALTER TABLE to add columns because all that code
needs to exist anyway. You could make a plain function to do the update that
gets called from both ATExecSetRelOptions() and DefineVirtualRelation().
Thanks,
nm
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Thu, Jul 7, 2011 at 10:56 AM, Kohei KaiGai wrote: > My opinion is ALTER TABLE SET/RESET code should be enhanced to accept > an operation to reset all the existing options, rather than tricky > updates of pg_class. > How about an idea to add AT_ResetAllRelOptions for internal use only? > > I'll fix up the regression test also. On an only semi-related note, ISTM that you may as well merge parts 0, 1, and 2 into a single patch, since there is no way we are going to apply any of them without the others. I suggest closing one of the CommitFest entries and revising the other one to point to the consolidated patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
2011/7/7 Noah Misch :
> On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:
>> *** a/src/backend/commands/view.c
>> --- b/src/backend/commands/view.c
>
>> --- 227,257
>> atcmd->def = (Node *) lfirst(c);
>> atcmds = lappend(atcmds, atcmd);
>> }
>> }
>>
>> /*
>> + * If optional parameters are specified, we must set options
>> + * using ALTER TABLE SET OPTION internally.
>> + */
>> + if (list_length(options) > 0)
>> + {
>> + atcmd = makeNode(AlterTableCmd);
>> + atcmd->subtype = AT_SetRelOptions;
>> + atcmd->def = (List *)options;
>> +
>> + atcmds = lappend(atcmds, atcmd);
>> + }
>> + else
>> + {
>> + atcmd = makeNode(AlterTableCmd);
>> + atcmd->subtype = AT_ResetRelOptions;
>> + atcmd->def = (Node *)
>> list_make1(makeDefElem("security_barrier",
>> +
>> NULL));
>> + }
>> + if (atcmds != NIL)
>> + AlterTableInternal(viewOid, atcmds, true);
>> +
>> + /*
>> * Seems okay, so return the OID of the pre-existing view.
>> */
>> relation_close(rel, NoLock); /* keep the lock! */
>
> That gets the job done for today, but DefineVirtualRelation() should not need
> to know all view options by name to simply replace the existing list with a
> new one. I don't think you can cleanly use the ALTER TABLE SET/RESET code for
> this. Instead, compute an option list similar to how DefineRelation() does so
> at tablecmds.c:491, then update pg_class.
>
My opinion is ALTER TABLE SET/RESET code should be enhanced to accept
an operation to reset all the existing options, rather than tricky
updates of pg_class.
How about an idea to add AT_ResetAllRelOptions for internal use only?
I'll fix up the regression test also.
Thanks,
--
KaiGai Kohei
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:
> *** a/src/backend/commands/view.c
> --- b/src/backend/commands/view.c
> --- 227,257
> atcmd->def = (Node *) lfirst(c);
> atcmds = lappend(atcmds, atcmd);
> }
> }
>
> /*
> + * If optional parameters are specified, we must set options
> + * using ALTER TABLE SET OPTION internally.
> + */
> + if (list_length(options) > 0)
> + {
> + atcmd = makeNode(AlterTableCmd);
> + atcmd->subtype = AT_SetRelOptions;
> + atcmd->def = (List *)options;
> +
> + atcmds = lappend(atcmds, atcmd);
> + }
> + else
> + {
> + atcmd = makeNode(AlterTableCmd);
> + atcmd->subtype = AT_ResetRelOptions;
> + atcmd->def = (Node *)
> list_make1(makeDefElem("security_barrier",
> +
> NULL));
> + }
> + if (atcmds != NIL)
> + AlterTableInternal(viewOid, atcmds, true);
> +
> + /*
>* Seems okay, so return the OID of the pre-existing view.
>*/
> relation_close(rel, NoLock);/* keep the lock! */
That gets the job done for today, but DefineVirtualRelation() should not need
to know all view options by name to simply replace the existing list with a
new one. I don't think you can cleanly use the ALTER TABLE SET/RESET code for
this. Instead, compute an option list similar to how DefineRelation() does so
at tablecmds.c:491, then update pg_class.
> 2011/7/5 Noah Misch :
> > On Sun, Jul 03, 2011 at 11:33:38AM +0200, Kohei KaiGai wrote:
> >> --- a/src/test/regress/sql/select_views.sql
> >> +++ b/src/test/regress/sql/select_views.sql
> >> +-- cleanups
> >> +DROP ROLE IF EXISTS alice;
> >> +DROP FUNCTION IF EXISTS f_leak(text);
> >> +DROP TABLE IF EXISTS credit_cards CASCADE;
> >
> > Keep the view around. That way, pg_dump tests of the regression database
> > will
> > test the dumping of this view option. (Your pg_dump support for this
> > feature
> > does work fine, though.)
The latest version of part 1 still drops everything here.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Sun, Jul 03, 2011 at 11:33:38AM +0200, Kohei KaiGai wrote:
> The attached patches are revised version.
>
> The part-0 provides 'security_barrier' option for view definition, and
> performs
> as a common basis of part-1 and part-2 patches.
> Syntax is extended as follows:
>
> CREATE VIEW view_name [WITH (param [=value])] AS query;
>
> We can also turn on/off this security_barrier setting by ALTER TABLE with
> SET/RESET options.
>
> The part-1 patch enforces the qualifiers originally located under the security
> barrier view to be launched prior to ones supplied on upper level.
> The differences from the previous version is this barrier become conditional,
> not always. So, existing optimization will be applied without any changes
> onto non-security-barrier views.
I tested various query trees I considered interesting, and this version had
sound semantics for all of them. I have one suggestion for CREATE OR REPLACE
VIEW semantics, plus various cosmetic comments.
These patches are unified diffs, rather than project-standard context diffs.
Part 0:
> --- a/doc/src/sgml/ref/create_view.sgml
> +++ b/doc/src/sgml/ref/create_view.sgml
> @@ -22,6 +22,7 @@ PostgreSQL documentation
>
>
> CREATE [ OR REPLACE ] [ TEMP | TEMPORARY ] VIEW class="PARAMETER">name [ ( class="PARAMETER">column_name [, ...] ) ]
> +[ WITH ( parameter [= value] [, ... ] ) ]
This needs a bit more markup; see the corresponding case in create_table.sgml.
alter_view.sgml also needs an update. Incidentally, we should use ALTER VIEW
SET OPTION when referring to setting this for a view. ALTER TABLE SET OPTION
will also support views, since that's the general pattern for tablecmds.c type
checks, but that's largely an implementation detail.
> --- a/src/backend/commands/view.c
> +++ b/src/backend/commands/view.c
> @@ -97,7 +97,8 @@ isViewOnTempTable_walker(Node *node, void *context)
> *-
> */
> static Oid
> -DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
> +DefineVirtualRelation(const RangeVar *relation, List *tlist,
> + bool replace, List *options)
> {
> Oid viewOid,
> namespaceId;
This hunk and the hunk for the function's caller get rejects due to another
recent signature change.
> @@ -167,6 +168,8 @@ DefineVirtualRelation(const RangeVar *relation, List
> *tlist, bool replace)
> {
> Relationrel;
> TupleDesc descriptor;
> + List *atcmds = NIL;
> + AlterTableCmd *atcmd;
>
> /*
>* Yes. Get exclusive lock on the existing view ...
> @@ -211,14 +214,11 @@ DefineVirtualRelation(const RangeVar *relation, List
> *tlist, bool replace)
>*/
> if (list_length(attrList) > rel->rd_att->natts)
> {
> - List *atcmds = NIL;
> ListCell *c;
> int skip = rel->rd_att->natts;
>
> foreach(c, attrList)
> {
> - AlterTableCmd *atcmd;
> -
> if (skip > 0)
> {
> skip--;
> @@ -229,10 +229,24 @@ DefineVirtualRelation(const RangeVar *relation, List
> *tlist, bool replace)
> atcmd->def = (Node *) lfirst(c);
> atcmds = lappend(atcmds, atcmd);
> }
> - AlterTableInternal(viewOid, atcmds, true);
> }
>
> /*
> + * If optional parameters are specified, we must set options
> + * using ALTER TABLE SET OPTION internally.
I think CREATE OR REPLACE VIEW should replace the option list, while ALTER
VIEW SET OPTION should retain its current behavior. That is, this should
leave the view with no options set:
create or replace view v0(n) with (security_barrier) as values (1), (2), (3),
(4);
select reloptions from pg_class where oid = 'v0'::regclass;
create or replace view v0(n) as values (4), (3), (2), (1);
select reloptions from pg_class where oid = 'v0'::regclass;
> + */
> + if (list_length(options) > 0)
> + {
> + atcmd = makeNode(AlterTableCmd);
> + atcmd->subtype = AT_SetRelOptions;
> + atcmd->def = options;
This line produces a warning:
view.c: In function `DefineVirtualRelation':
view.c:240: warning: assignment from incompatible pointer type
> +
> + atcmds = lappend(atcmds, atcmd);
> + }
> + if (atcmds != NIL)
> + AlterTableInternal(viewOid, atcmds, true);
> +
> + /*
>* Seems okay, so return the OID of the pre-ex
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Sat, Jul 2, 2011 at 3:46 PM, Tom Lane wrote: > Robert Haas writes: >> On Sat, Jul 2, 2011 at 1:54 PM, Kohei KaiGai wrote: >>> BTW, regarding to the statement support for security barrier views, >>> the following syntax might be more consistent with existing ones: >>> CREATE VIEW view_name WITH ( param [=value]) AS query ... ; >>> rather than >>> CREATE SECURITY VIEW view_name AS query ...; >>> >>> Any comments? > >> I think I mildly prefer CREATE SECURITY VIEW to the parameter syntax >> in this case, but I don't hate the other one. > > The WITH idea seems a bit more future-proof; in particular it would > easily accommodate specifying a security type, if we decide we need > various levels of leak-proof-ness. Or other kinds of view options. I'm not going to argue against that too forcefully, since I've advocated introducing that sort of syntax elsewhere. I think it's mostly that I thought this feature might be significant enough to merit a syntax that would make it a little more prominent, but perhaps not. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
Robert Haas writes: > On Sat, Jul 2, 2011 at 1:54 PM, Kohei KaiGai wrote: >> BTW, regarding to the statement support for security barrier views, >> the following syntax might be more consistent with existing ones: >> CREATE VIEW view_name WITH ( param [=value]) AS query ... ; >> rather than >> CREATE SECURITY VIEW view_name AS query ...; >> >> Any comments? > I think I mildly prefer CREATE SECURITY VIEW to the parameter syntax > in this case, but I don't hate the other one. The WITH idea seems a bit more future-proof; in particular it would easily accommodate specifying a security type, if we decide we need various levels of leak-proof-ness. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Sat, Jul 2, 2011 at 1:54 PM, Kohei KaiGai wrote: > BTW, regarding to the statement support for security barrier views, > the following syntax might be more consistent with existing ones: > CREATE VIEW view_name WITH ( param [=value]) AS query ... ; > rather than > CREATE SECURITY VIEW view_name AS query ...; > > Any comments? I think I mildly prefer CREATE SECURITY VIEW to the parameter syntax in this case, but I don't hate the other one. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
BTW, regarding to the statement support for security barrier views, the following syntax might be more consistent with existing ones: CREATE VIEW view_name WITH ( param [=value]) AS query ... ; rather than CREATE SECURITY VIEW view_name AS query ...; Any comments? 2011/7/2 Noah Misch : > On Sat, Jul 02, 2011 at 12:48:32PM +0200, Kohei KaiGai wrote: >> > Let's see. ?Every qual list will have some depth d such that all quals >> > having >> > depth >= d are security-relevant, and all others are not security-relevant. >> > (This does not hold for all means of identifying security-relevant quals, >> > but >> > it does hold for the CREATE SECURITY VIEW/reloptions strategy proposed in >> > your >> > part 2 patch.) ?Suppose you track whether each Query node represents a >> > security view, then only increment the qualifier depth for such Query >> > nodes, >> > rather than all Query nodes. ?The tracked depth then becomes a security >> > partition depth. ?Keep the actual sorting algorithm the same. >> > ?(Disclaimer: I >> > haven't been thinking about this nearly as long as you have, so I may be >> > missing something relatively obvious.) >> > >> It might be an idea to increment the depth only when we go across security >> barrier view. In other words, all the qualifiers will have same depth unless >> it does not come from inside of the security view. > > Yes; that sounds suitable. > >> > As it stands, the patch badly damages the performance of this example: >> > >> > CREATE FUNCTION expensive(int) RETURNS boolean LANGUAGE sql >> > ? ? ? ?AS 'SELECT pg_sleep(1); SELECT true' COST 100; >> > CREATE TABLE t(c) AS SELECT * FROM generate_series(1,3); >> > EXPLAIN ANALYZE >> > ? ? ? ?SELECT * FROM (SELECT * FROM t WHERE expensive(c)) t0 WHERE c = 2; >> > >> > That doesn't even use a view, let alone a security view. ?While I like the >> > patch's current simplicity, we need to narrow its impact. >> > >> If we apply above idea I explained, c=2 and expensive(c) will belong >> to same depth, >> then it shall be reordered according to cost estimation. >> In the case when "(SELECT * FROM t WHERE expensive(c))" come from security >> view, the performance damage is unavoidable, because DBA explicitly specified >> its main purpose is security. >> >> So, it might be a good idea to split out my two patches into three. >> 1. Add "SECURITY VIEW" support. >> 2. Fix leaky view part.1 - order of qualifiers >> 3. Fix leaky view part.2 - unexpected pushing down >> >> How about your opinion? > > I'd say, for CommitFest purposes, keep SECURITY VIEW attached to one of the > other patches. It's not likely it would be committed without anything hooked > up > to actually use it. Splitting it out into its own patch *file* and attaching > that and the part 1 patch to the same email would be fine, though. > -- KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Sat, Jul 02, 2011 at 12:48:32PM +0200, Kohei KaiGai wrote: > > Let's see. ?Every qual list will have some depth d such that all quals > > having > > depth >= d are security-relevant, and all others are not security-relevant. > > (This does not hold for all means of identifying security-relevant quals, > > but > > it does hold for the CREATE SECURITY VIEW/reloptions strategy proposed in > > your > > part 2 patch.) ?Suppose you track whether each Query node represents a > > security view, then only increment the qualifier depth for such Query nodes, > > rather than all Query nodes. ?The tracked depth then becomes a security > > partition depth. ?Keep the actual sorting algorithm the same. ?(Disclaimer: > > I > > haven't been thinking about this nearly as long as you have, so I may be > > missing something relatively obvious.) > > > It might be an idea to increment the depth only when we go across security > barrier view. In other words, all the qualifiers will have same depth unless > it does not come from inside of the security view. Yes; that sounds suitable. > > As it stands, the patch badly damages the performance of this example: > > > > CREATE FUNCTION expensive(int) RETURNS boolean LANGUAGE sql > > ? ? ? ?AS 'SELECT pg_sleep(1); SELECT true' COST 100; > > CREATE TABLE t(c) AS SELECT * FROM generate_series(1,3); > > EXPLAIN ANALYZE > > ? ? ? ?SELECT * FROM (SELECT * FROM t WHERE expensive(c)) t0 WHERE c = 2; > > > > That doesn't even use a view, let alone a security view. ?While I like the > > patch's current simplicity, we need to narrow its impact. > > > If we apply above idea I explained, c=2 and expensive(c) will belong > to same depth, > then it shall be reordered according to cost estimation. > In the case when "(SELECT * FROM t WHERE expensive(c))" come from security > view, the performance damage is unavoidable, because DBA explicitly specified > its main purpose is security. > > So, it might be a good idea to split out my two patches into three. > 1. Add "SECURITY VIEW" support. > 2. Fix leaky view part.1 - order of qualifiers > 3. Fix leaky view part.2 - unexpected pushing down > > How about your opinion? I'd say, for CommitFest purposes, keep SECURITY VIEW attached to one of the other patches. It's not likely it would be committed without anything hooked up to actually use it. Splitting it out into its own patch *file* and attaching that and the part 1 patch to the same email would be fine, though. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
>> > I was referring to this paragraph: >> > >> > ?On the technical side, I am pretty doubtful that the approach of adding a >> > ?nestlevel to FuncExpr and RelOptInfo is the right way to go. ?I believe we >> > ?have existing code (to handle left joins) that prevents quals from being >> > ?pushed down too far by fudging the set of relations that are supposedly >> > needed >> > ?to evaluate the qual. ?I suspect a similar approach would work here. >> > >> It seems to me the later half of this paragraph is talking about the problem >> of >> unexpected qualifier pushing-down over the security barrier; I'm trying to >> solve >> the problem with the part.2 patch. >> The scenario the part.1 patch tries to solve is order to launch qualifiers, >> not >> unexpected pushing-down. > > Okay, you're probably correct that it wasn't referring to the topic at hand. > I'm still suspicious of the silent assumption about how quals can be assigned > to plan nodes, but I don't have any concrete ideas for avoiding that. > If a subquery is enough simple to pull up, pull_up_simple_subquery() pulls up the subquery. Its simpleness is checked by is_simple_subquery(). At that timing, qualifiers of the subquery are also pulled-up, so upper level query shall have qualifiers with mixed nest-level. Then, every qualifiers shall be distributed to a particular scan plan on the distribute_qual_to_rels(); Its current criteria to distribute them is "as deep as possible" according to the references of qualifiers. This criteria cause a problem as I tried to tackle on the part.2 patch, so it tries to put security-barrier to prevent unexpected pushing-down. After the distribution, a scan plan will have qualifiers come from different nest-levels. The order_qual_clauses() reorders the qualifiers according to its cost estimation, so it possibly launches qualifiers come from upper level prior to deeper one. >> >> In addition, implementation will become complex, if both of qualifiers >> >> pulled-up >> >> from security barrier view and qualifiers pulled-up from regular views >> >> are mixed >> >> within a single qualifier list. >> > >> > I only scanned the part 2 patch, but isn't the bookkeeping already >> > happening for >> > its purposes? ?How much more complexity would we get to apply the same >> > strategy >> > to the behavior of this patch? >> > >> If conditional, what criteria we should have to reorder the quelifier? >> The current patch checks the depth at first, then it checks cost if same >> deptn. >> It is quite simple rule. I have no idea of the criteria to order the >> mixed qualifier >> come from security-barrier views and regular views. > > Let's see. Every qual list will have some depth d such that all quals having > depth >= d are security-relevant, and all others are not security-relevant. > (This does not hold for all means of identifying security-relevant quals, but > it does hold for the CREATE SECURITY VIEW/reloptions strategy proposed in your > part 2 patch.) Suppose you track whether each Query node represents a > security view, then only increment the qualifier depth for such Query nodes, > rather than all Query nodes. The tracked depth then becomes a security > partition depth. Keep the actual sorting algorithm the same. (Disclaimer: I > haven't been thinking about this nearly as long as you have, so I may be > missing something relatively obvious.) > It might be an idea to increment the depth only when we go across security barrier view. In other words, all the qualifiers will have same depth unless it does not come from inside of the security view. > As it stands, the patch badly damages the performance of this example: > > CREATE FUNCTION expensive(int) RETURNS boolean LANGUAGE sql > AS 'SELECT pg_sleep(1); SELECT true' COST 100; > CREATE TABLE t(c) AS SELECT * FROM generate_series(1,3); > EXPLAIN ANALYZE > SELECT * FROM (SELECT * FROM t WHERE expensive(c)) t0 WHERE c = 2; > > That doesn't even use a view, let alone a security view. While I like the > patch's current simplicity, we need to narrow its impact. > If we apply above idea I explained, c=2 and expensive(c) will belong to same depth, then it shall be reordered according to cost estimation. In the case when "(SELECT * FROM t WHERE expensive(c))" come from security view, the performance damage is unavoidable, because DBA explicitly specified its main purpose is security. So, it might be a good idea to split out my two patches into three. 1. Add "SECURITY VIEW" support. 2. Fix leaky view part.1 - order of qualifiers 3. Fix leaky view part.2 - unexpected pushing down How about your opinion? -- KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Wed, Jun 29, 2011 at 05:05:22PM +0100, Kohei KaiGai wrote: > 2011/6/28 Noah Misch : > > On Tue, Jun 28, 2011 at 10:11:59PM +0200, Kohei KaiGai wrote: > > CREATE VIEW a AS SELECT * FROM ta WHERE ac = 5; > > ALTER VIEW a OWNER TO alice; > > CREATE VIEW b AS SELECT * FROM tb WHERE bc = 6; > > ALTER VIEW b OWNER TO bob; > > SELECT * FROM a, b; > > > > Both the ac=5 and the bc=6 quals do run at the same depth despite enforcing > > security for different principals. ?I can't think of a way that one view > > owner > > could use this situation to subvert the security of the other view owner, > > but I > > wanted to throw it out. > > > Even if view owner set a trap in his view, we have no way to reference > variables > come from outside of the view. In above example, even if I added f_leak() into > the definition of VIEW A, we cannot give argument to reference VIEW B. Good point. Yes, it should be rigorously safe on that account. > > I was referring to this paragraph: > > > > ?On the technical side, I am pretty doubtful that the approach of adding a > > ?nestlevel to FuncExpr and RelOptInfo is the right way to go. ?I believe we > > ?have existing code (to handle left joins) that prevents quals from being > > ?pushed down too far by fudging the set of relations that are supposedly > > needed > > ?to evaluate the qual. ?I suspect a similar approach would work here. > > > It seems to me the later half of this paragraph is talking about the problem > of > unexpected qualifier pushing-down over the security barrier; I'm trying to > solve > the problem with the part.2 patch. > The scenario the part.1 patch tries to solve is order to launch qualifiers, > not > unexpected pushing-down. Okay, you're probably correct that it wasn't referring to the topic at hand. I'm still suspicious of the silent assumption about how quals can be assigned to plan nodes, but I don't have any concrete ideas for avoiding that. > >> In addition, implementation will become complex, if both of qualifiers > >> pulled-up > >> from security barrier view and qualifiers pulled-up from regular views are > >> mixed > >> within a single qualifier list. > > > > I only scanned the part 2 patch, but isn't the bookkeeping already > > happening for > > its purposes? ?How much more complexity would we get to apply the same > > strategy > > to the behavior of this patch? > > > If conditional, what criteria we should have to reorder the quelifier? > The current patch checks the depth at first, then it checks cost if same > deptn. > It is quite simple rule. I have no idea of the criteria to order the > mixed qualifier > come from security-barrier views and regular views. Let's see. Every qual list will have some depth d such that all quals having depth >= d are security-relevant, and all others are not security-relevant. (This does not hold for all means of identifying security-relevant quals, but it does hold for the CREATE SECURITY VIEW/reloptions strategy proposed in your part 2 patch.) Suppose you track whether each Query node represents a security view, then only increment the qualifier depth for such Query nodes, rather than all Query nodes. The tracked depth then becomes a security partition depth. Keep the actual sorting algorithm the same. (Disclaimer: I haven't been thinking about this nearly as long as you have, so I may be missing something relatively obvious.) As it stands, the patch badly damages the performance of this example: CREATE FUNCTION expensive(int) RETURNS boolean LANGUAGE sql AS 'SELECT pg_sleep(1); SELECT true' COST 100; CREATE TABLE t(c) AS SELECT * FROM generate_series(1,3); EXPLAIN ANALYZE SELECT * FROM (SELECT * FROM t WHERE expensive(c)) t0 WHERE c = 2; That doesn't even use a view, let alone a security view. While I like the patch's current simplicity, we need to narrow its impact. Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
2011/6/28 Noah Misch :
> On Tue, Jun 28, 2011 at 10:11:59PM +0200, Kohei KaiGai wrote:
>> 2011/6/28 Noah Misch :
>> > Suppose your query references two views owned by different roles. ?The
>> > quals
>> > of those views will have the same depth. ?Is there a way for information to
>> > leak from one view owner to another due to that?
>> >
>> Even if multiple subqueries were pulled-up and qualifiers got merged, user
>> given
>> qualifiers shall have smaller depth value, so it will be always
>> launched after the
>> qualifiers originally used in the subqueries.
>>
>> Of course, it is another topic in the case when the view is originally
>> defined with
>> leaky functions.
>
> Right. I was thinking of a pair of quals, one in each of two view
> definitions.
> The views are mutually-untrusting. Something like this:
>
> CREATE VIEW a AS SELECT * FROM ta WHERE ac = 5;
> ALTER VIEW a OWNER TO alice;
> CREATE VIEW b AS SELECT * FROM tb WHERE bc = 6;
> ALTER VIEW b OWNER TO bob;
> SELECT * FROM a, b;
>
> Both the ac=5 and the bc=6 quals do run at the same depth despite enforcing
> security for different principals. I can't think of a way that one view owner
> could use this situation to subvert the security of the other view owner, but
> I
> wanted to throw it out.
>
Even if view owner set a trap in his view, we have no way to reference variables
come from outside of the view. In above example, even if I added f_leak() into
the definition of VIEW A, we cannot give argument to reference VIEW B.
> I was referring to this paragraph:
>
> On the technical side, I am pretty doubtful that the approach of adding a
> nestlevel to FuncExpr and RelOptInfo is the right way to go. I believe we
> have existing code (to handle left joins) that prevents quals from being
> pushed down too far by fudging the set of relations that are supposedly
> needed
> to evaluate the qual. I suspect a similar approach would work here.
>
It seems to me the later half of this paragraph is talking about the problem of
unexpected qualifier pushing-down over the security barrier; I'm trying to solve
the problem with the part.2 patch.
The scenario the part.1 patch tries to solve is order to launch qualifiers, not
unexpected pushing-down.
As long as we focus on the ordering problem, it is reasonable approach to
track original nestlevel to enforce to launch qualifier come from deeper level
earlier. Because it makes performance damages, if we prevent pull-up
subqueries come from security view.
For example, if we cannot pull-up this subquery, we will need to scan the
relation twice.
SELECT * FROM (SELECT * FROM tbl WHERE f_policy(x)) WHERE f_leak(y);
Normally, it should be pulled-up into the following form:
SELECT * FROM tbl WHERE f_policy(x) AND f_leak(y);
>> In addition, implementation will become complex, if both of qualifiers
>> pulled-up
>> from security barrier view and qualifiers pulled-up from regular views are
>> mixed
>> within a single qualifier list.
>
> I only scanned the part 2 patch, but isn't the bookkeeping already happening
> for
> its purposes? How much more complexity would we get to apply the same
> strategy
> to the behavior of this patch?
>
If conditional, what criteria we should have to reorder the quelifier?
The current patch checks the depth at first, then it checks cost if same deptn.
It is quite simple rule. I have no idea of the criteria to order the
mixed qualifier
come from security-barrier views and regular views.
>> > On Mon, Jun 06, 2011 at 01:37:11PM +0100, Kohei Kaigai wrote:
>> >> --- a/src/backend/optimizer/plan/planner.c
>> >> +++ b/src/backend/optimizer/plan/planner.c
>> >
>> >> + ? ? else if (IsA(node, Query))
>> >> + ? ? {
>> >> + ? ? ? ? ? ? depth += 2;
>> >
>> > Why two?
>> >
>> This patch is a groundwork for the upcoming row-level security feature.
>> In the case when the backend appends security policy functions, it should
>> be launched prior to user given qualifiers. So, I intends to give odd depth
>> numbers for these functions to have higher priorities to other one.
>> Of course, 1 might work well right now.
>
> I'd say it should either be 1 until such time as that's needed, or it needs a
> comment noting why it's 2.
>
OK, I'll add comment to introduce why the depth is incremented by 2.
Thanks,
--
KaiGai Kohei
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Tue, Jun 28, 2011 at 10:11:59PM +0200, Kohei KaiGai wrote:
> 2011/6/28 Noah Misch :
> > Suppose your query references two views owned by different roles. ?The quals
> > of those views will have the same depth. ?Is there a way for information to
> > leak from one view owner to another due to that?
> >
> Even if multiple subqueries were pulled-up and qualifiers got merged, user
> given
> qualifiers shall have smaller depth value, so it will be always
> launched after the
> qualifiers originally used in the subqueries.
>
> Of course, it is another topic in the case when the view is originally
> defined with
> leaky functions.
Right. I was thinking of a pair of quals, one in each of two view definitions.
The views are mutually-untrusting. Something like this:
CREATE VIEW a AS SELECT * FROM ta WHERE ac = 5;
ALTER VIEW a OWNER TO alice;
CREATE VIEW b AS SELECT * FROM tb WHERE bc = 6;
ALTER VIEW b OWNER TO bob;
SELECT * FROM a, b;
Both the ac=5 and the bc=6 quals do run at the same depth despite enforcing
security for different principals. I can't think of a way that one view owner
could use this situation to subvert the security of the other view owner, but I
wanted to throw it out.
> > This makes assumptions, at a distance, about the possible scan types and how
> > quals can be fitted to them. ?Specifically, this patch achieves its goals
> > because any indexable qual is trustworthy, and any non-indexable qual cannot
> > be pushed down further than the view's own quals. ?That seems to be true
> > with
> > current plan types, but it feels fragile. ?I don't have a concrete idea for
> > improvement, though. ?Robert suggested another approach in
> > http://archives.postgresql.org/message-id/aanlktimbn_6tyxreh5rc7pmizt-vjb3xgp8cuho0o...@mail.gmail.com
> > ; might that approach avoid this hazard?
> >
> The reason why we didn't adopt the idea to check privileges of underlying
> tables
> is that PostgreSQL checks privileges on executor phase, not planner phase.
>
> If we try to have a flag on pg_proc, it is a tough work to categolize
> trustworth
> functions and non-trustworh ones from beginning, because we have more than
> 2000 of built-in functions.
> So, it is reasonable assumption that index access methods does not leak
> contents of tuples scanned, because only superuser can define them.
I was referring to this paragraph:
On the technical side, I am pretty doubtful that the approach of adding a
nestlevel to FuncExpr and RelOptInfo is the right way to go. I believe we
have existing code (to handle left joins) that prevents quals from being
pushed down too far by fudging the set of relations that are supposedly needed
to evaluate the qual. I suspect a similar approach would work here.
> > The part 2 patch in this series implements the planner restriction more
> > likely
> > to yield performance regressions, so it introduces a mechanism for
> > identifying
> > when to apply the restriction. ?This patch, however, applies its restriction
> > unconditionally. ?Since we will inevitably have a such mechanism before you
> > are done sealing the leaks in our view implementation, the restriction in
> > this
> > patch should also use that mechanism. ?Therefore, I think we should review
> > and
> > apply part 2 first, then update this patch to use its conditional behavior.
> >
> The reason why this patch always gives the depth higher priority is the matter
> is relatively minor compared to the issue the part.2 patch tries to tackle.
> That affects the selection of scan plan (IndexScan or SeqScan), so it
> is significant
> decision to be controllable. However, this issue is just on a particular scan.
True. The lost optimization opportunity is relatively minor, but perhaps not
absolutely minor. It would be one thing if we could otherwise get away without
designing any mechanism for applying these restrictions conditionally. However,
since you have implemented the conditional behavior elsewhere, it would be nice
to apply it here.
> In addition, implementation will become complex, if both of qualifiers
> pulled-up
> from security barrier view and qualifiers pulled-up from regular views are
> mixed
> within a single qualifier list.
I only scanned the part 2 patch, but isn't the bookkeeping already happening for
its purposes? How much more complexity would we get to apply the same strategy
to the behavior of this patch?
> > On Mon, Jun 06, 2011 at 01:37:11PM +0100, Kohei Kaigai wrote:
> >> --- a/src/backend/optimizer/plan/planner.c
> >> +++ b/src/backend/optimizer/plan/planner.c
> >
> >> + ? ? else if (IsA(node, Query))
> >> + ? ? {
> >> + ? ? ? ? ? ? depth += 2;
> >
> > Why two?
> >
> This patch is a groundwork for the upcoming row-level security feature.
> In the case when the backend appends security policy functions, it should
> be launched prior to user given qualifiers. So, I intends to give odd depth
> numbers for these functions to have higher priorities to other one.
> Of course,
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
Thanks for your reviewing,
2011/6/28 Noah Misch :
> I took a look at this patch. It's incredibly simple, which is great, and it
> seems to achieve its goal.
>
> Suppose your query references two views owned by different roles. The quals
> of those views will have the same depth. Is there a way for information to
> leak from one view owner to another due to that?
>
Even if multiple subqueries were pulled-up and qualifiers got merged, user given
qualifiers shall have smaller depth value, so it will be always
launched after the
qualifiers originally used in the subqueries.
Of course, it is another topic in the case when the view is originally
defined with
leaky functions.
> I like how you've assumed that the table owner trusts the operator class
> functions of indexes on his table to not leak information. That handily
> catches some basic and important qual pushdowns that would otherwise be lost.
>
> This makes assumptions, at a distance, about the possible scan types and how
> quals can be fitted to them. Specifically, this patch achieves its goals
> because any indexable qual is trustworthy, and any non-indexable qual cannot
> be pushed down further than the view's own quals. That seems to be true with
> current plan types, but it feels fragile. I don't have a concrete idea for
> improvement, though. Robert suggested another approach in
> http://archives.postgresql.org/message-id/aanlktimbn_6tyxreh5rc7pmizt-vjb3xgp8cuho0o...@mail.gmail.com
> ; might that approach avoid this hazard?
>
The reason why we didn't adopt the idea to check privileges of underlying tables
is that PostgreSQL checks privileges on executor phase, not planner phase.
If we try to have a flag on pg_proc, it is a tough work to categolize trustworth
functions and non-trustworh ones from beginning, because we have more than
2000 of built-in functions.
So, it is reasonable assumption that index access methods does not leak
contents of tuples scanned, because only superuser can define them.
> The part 2 patch in this series implements the planner restriction more likely
> to yield performance regressions, so it introduces a mechanism for identifying
> when to apply the restriction. This patch, however, applies its restriction
> unconditionally. Since we will inevitably have a such mechanism before you
> are done sealing the leaks in our view implementation, the restriction in this
> patch should also use that mechanism. Therefore, I think we should review and
> apply part 2 first, then update this patch to use its conditional behavior.
>
The reason why this patch always gives the depth higher priority is the matter
is relatively minor compared to the issue the part.2 patch tries to tackle.
That affects the selection of scan plan (IndexScan or SeqScan), so it
is significant
decision to be controllable. However, this issue is just on a particular scan.
In addition, implementation will become complex, if both of qualifiers pulled-up
from security barrier view and qualifiers pulled-up from regular views are mixed
within a single qualifier list.
> A few minor questions/comments on the implementation:
>
> On Mon, Jun 06, 2011 at 01:37:11PM +0100, Kohei Kaigai wrote:
>> --- a/src/backend/optimizer/plan/planner.c
>> +++ b/src/backend/optimizer/plan/planner.c
>
>> + else if (IsA(node, Query))
>> + {
>> + depth += 2;
>
> Why two?
>
This patch is a groundwork for the upcoming row-level security feature.
In the case when the backend appends security policy functions, it should
be launched prior to user given qualifiers. So, I intends to give odd depth
numbers for these functions to have higher priorities to other one.
Of course, 1 might work well right now.
>> --- a/src/test/regress/expected/select_views.out
>> +++ b/src/test/regress/expected/select_views.out
>
>> +EXPLAIN SELECT * FROM your_credit WHERE f_leak(number,expired);
>> + QUERY PLAN
>> +--
>> + Seq Scan on credit_cards (cost=0.00..181.20 rows=1 width=96)
>
> Use "EXPLAIN (COSTS OFF)" in regression tests. We do not put much effort into
> the stability of exact cost values, and they do not matter for the purpose of
> this test.
>
OK, fixed.
>> --- a/src/test/regress/sql/select_views.sql
>> +++ b/src/test/regress/sql/select_views.sql
>> @@ -8,3 +8,46 @@ SELECT * FROM street;
>> SELECT name, #thepath FROM iexit ORDER BY 1, 2;
>>
>> SELECT * FROM toyemp WHERE name = 'sharon';
>> +
>> +--
>> +-- Test for leaky-view problem
>> +--
>> +
>> +-- setups
>> +SET client_min_messages TO 'warning';
>> +
>> +DROP ROLE IF EXISTS alice;
>> +DROP FUNCTION IF EXISTS f_leak(text);
>> +DROP TABLE IF EXISTS credit_cards;
>> +
>> +RESET client_min_messages;
>
> No need for this. The regression tests always start on a clean database.
>
Fixed.
>> +
>> +CREATE USER alice;
>> +CREATE FUNCTION f_leak(text, text)
>> + RETURNS bool LANGUAGE 'plpgsql'
>> + AS 'begin raise n
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
I took a look at this patch. It's incredibly simple, which is great, and it
seems to achieve its goal.
Suppose your query references two views owned by different roles. The quals
of those views will have the same depth. Is there a way for information to
leak from one view owner to another due to that?
I like how you've assumed that the table owner trusts the operator class
functions of indexes on his table to not leak information. That handily
catches some basic and important qual pushdowns that would otherwise be lost.
This makes assumptions, at a distance, about the possible scan types and how
quals can be fitted to them. Specifically, this patch achieves its goals
because any indexable qual is trustworthy, and any non-indexable qual cannot
be pushed down further than the view's own quals. That seems to be true with
current plan types, but it feels fragile. I don't have a concrete idea for
improvement, though. Robert suggested another approach in
http://archives.postgresql.org/message-id/aanlktimbn_6tyxreh5rc7pmizt-vjb3xgp8cuho0o...@mail.gmail.com
; might that approach avoid this hazard?
The part 2 patch in this series implements the planner restriction more likely
to yield performance regressions, so it introduces a mechanism for identifying
when to apply the restriction. This patch, however, applies its restriction
unconditionally. Since we will inevitably have a such mechanism before you
are done sealing the leaks in our view implementation, the restriction in this
patch should also use that mechanism. Therefore, I think we should review and
apply part 2 first, then update this patch to use its conditional behavior.
A few minor questions/comments on the implementation:
On Mon, Jun 06, 2011 at 01:37:11PM +0100, Kohei Kaigai wrote:
> --- a/src/backend/optimizer/plan/planner.c
> +++ b/src/backend/optimizer/plan/planner.c
> + else if (IsA(node, Query))
> + {
> + depth += 2;
Why two?
> --- a/src/test/regress/expected/select_views.out
> +++ b/src/test/regress/expected/select_views.out
> +EXPLAIN SELECT * FROM your_credit WHERE f_leak(number,expired);
> +QUERY PLAN
> +--
> + Seq Scan on credit_cards (cost=0.00..181.20 rows=1 width=96)
Use "EXPLAIN (COSTS OFF)" in regression tests. We do not put much effort into
the stability of exact cost values, and they do not matter for the purpose of
this test.
> --- a/src/test/regress/sql/select_views.sql
> +++ b/src/test/regress/sql/select_views.sql
> @@ -8,3 +8,46 @@ SELECT * FROM street;
> SELECT name, #thepath FROM iexit ORDER BY 1, 2;
>
> SELECT * FROM toyemp WHERE name = 'sharon';
> +
> +--
> +-- Test for leaky-view problem
> +--
> +
> +-- setups
> +SET client_min_messages TO 'warning';
> +
> +DROP ROLE IF EXISTS alice;
> +DROP FUNCTION IF EXISTS f_leak(text);
> +DROP TABLE IF EXISTS credit_cards;
> +
> +RESET client_min_messages;
No need for this. The regression tests always start on a clean database.
> +
> +CREATE USER alice;
> +CREATE FUNCTION f_leak(text, text)
> +RETURNS bool LANGUAGE 'plpgsql'
> +AS 'begin raise notice ''% => %'', $1, $2; return true; end';
I ran this test case on master, and it did not reproduce the problem.
However, adding "COST 0.1" to this CREATE FUNCTION did yield the expected
problem behavior. I suggest adding that.
Thanks,
nm
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [v9.2] Fix leaky-view problem, part 1
This patch enables to fix up leaky-view problem using functions with tiny cost estimation scenario. The point of this scenario is criteria to reorder qualifiers of scanning plan in order_qual_clauses(). The optimizer may pull up simple subqueries into upper level, then its qualifier will get merged with ones in the upper level. When executor scans a relation, qualifiers with smaller cost shall be executed earlier to minimize cost to filter out invisible tuples. However, we know unpreferable side-effects when we use a view for row-level security. Even if a certain subquery rewritten from a view is defined for row-level security, a function with tiny cost appended from outside of the view may executed earlier than qualifiers to perform as security policy of the view, as long as the view is enough simple and the supplied function has tiny cost. In the result, this function can see the arguments come from invisible tuples, and leak them into somewhere. The solution is quite simple. This patch enables to track original depth of qualifiers and modify criteria to sort qualifiers in order_qual_clauses(). Even if a function with tiny cost is supplied from outside of views, the patched optimizer does not prioritize cost estimation more than the depth. It fixes up the scenario [1] in the bellow descriprions. The background of the leaky-view problem is well summarized at: http://wiki.postgresql.org/wiki/RLS We had discussed several scenarios in v9.1 development cycle, and the last developer meeting. We almost concluded the following criteria to characterize whether a leak-view scenario is problematic to be fixed, or not. * If unprived user can directly reference contents of invisible tuples, it is a problem to be fixed. * As long as contents of invisible tuples are consumed by internal stuff (eg, index-access method), it is not a problem to be fixed. Thus, the scenario [1] and [2] are problematic to be fixed, but [3] and [4] are not. So, I'll try to fix up these two scenario with the patch part-1 amd part-2. [1] unexpected reorder of functions with tiny-cost and side-effects Qualifiers of WHERE or JOIN ... IN clause shall be sorted by estimated cost, not depth of nest level. Thus, this logic can make order reversal when user-given qualifier has smaller cost than qualifiers to perform as security policy inside of view. In the result, these qualifiers can reference both of visible and invisible tuples prior to the filtering by row-level security policy of the view. Thus, this behavior can be used to leak contents of invisible tuples. [2] unexpected push-down of functions with side-effect into join-loop If arguments of qualifier being appended on outside of join-loop references only one-side of the join-loop, it is a good strategy to distribute this qualifier into inside of the join-loop to minimize number of tuples to be joined, from the viewpoint of performance. However, it also makes order reversal when the join-loop is a part of view definition that should perform row-level security policy. Then, these exogenetic qualifiers may be executed prior to the filtering by row-level security policy of the view. Thus, this behavior can be used to leak contents of invisible tuple. [3] estimation of hidden value using iteration of PK/FK proves Due to the nature of PK/FK constraints, we can infer existence of key values being stored within invisible tuple, even if we never allows users to reference contents of invisible tuples. We commonly call this type of information leaks "covert-channel", and it is basically impossible to prevent according to the previous security research, however, its risk is also relatively small because of slow bandwidth to leak. We already made consensus this scenario is not a problem to be fixed. [4] estimation of hidden value using statistics One example was selectivity-estimator function; that may reference statistical information delivered from the tables have invisible tuples for optimization. Here are two points to be considered. The one is purely internal stuff may be able to reference invisible tuples, however, it is not a problem as long as it does not leak them into end-users; such as index access methods. The second is statistical or other form of date delivered from invisible tuples. We can set up a table that contains data delivered from invisible tuples using row-level triggers, however, it is quite a matter of database administration. Unless owner of tables set up such a leakable configuration, other users cannot reference them. Thanks, -- NEC Europe Ltd, SAP Global Competence Center KaiGai Kohei pgsql-fix-leaky-view-part-1.patch Description: pgsql-fix-leaky-view-part-1.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
