Thread: [COMMITTERS] pgsql: Apply RLS policies to partitioned tables.
Apply RLS policies to partitioned tables. The new partitioned table capability added a new relkind, namely RELKIND_PARTITIONED_TABLE. Update fireRIRrules() to apply RLS policies on RELKIND_PARTITIONED_TABLE as it does RELKIND_RELATION. In addition, add RLS regression test coverage for partitioned tables. Issue raised by Fakhroutdinov Evgenievich and patch by Mike Palmiotto. Regression test editorializing by me. Discussion: https://postgr.es/m/flat/20170601065959.1486.69906@wrigleys.postgresql.org Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/4f7a95be2c112bdc8da5f7e46cbb743b8ba4cc21 Modified Files -------------- src/backend/rewrite/rewriteHandler.c | 3 +- src/test/regress/expected/rowsecurity.out | 428 ++++++++++++++++++++++++++++++ src/test/regress/sql/rowsecurity.sql | 142 ++++++++++ 3 files changed, 572 insertions(+), 1 deletion(-)
Joe Conway <mail@joeconway.com> writes:
> Apply RLS policies to partitioned tables.
Buildfarm member skink has grown a "make check" failure with this commit.
==28150== VALGRINDERROR-BEGIN
==28150== Invalid read of size 8
==28150== at 0x39A355: ExecInitModifyTable (nodeModifyTable.c:1862)
==28150== by 0x37F0F8: ExecInitNode (execProcnode.c:168)
==28150== by 0x37C219: InitPlan (execMain.c:1044)
==28150== by 0x37C3AF: standard_ExecutorStart (execMain.c:256)
==28150== by 0x37C4B8: ExecutorStart (execMain.c:151)
==28150== by 0x4CCCCE: ProcessQuery (pquery.c:157)
==28150== by 0x4CCEDF: PortalRunMulti (pquery.c:1287)
==28150== by 0x4CDE19: PortalRun (pquery.c:800)
==28150== by 0x4C9E85: exec_simple_query (postgres.c:1099)
==28150== by 0x4CBF20: PostgresMain (postgres.c:4087)
==28150== by 0x44DC04: BackendRun (postmaster.c:4331)
==28150== by 0x44FD9B: BackendStartup (postmaster.c:4003)
==28150== Address 0xbbdbec8 is 8,008 bytes inside a recently re-allocated block of size 8,192 alloc'd
==28150== at 0x4C2AB76: malloc (vg_replace_malloc.c:299)
==28150== by 0x6002D2: AllocSetAlloc (aset.c:760)
==28150== by 0x606EB5: MemoryContextAllocZeroAligned (mcxt.c:791)
==28150== by 0x3832B1: CreateExecutorState (execUtils.c:99)
==28150== by 0x37C2E6: standard_ExecutorStart (execMain.c:186)
==28150== by 0x37C4B8: ExecutorStart (execMain.c:151)
==28150== by 0x4CCCCE: ProcessQuery (pquery.c:157)
==28150== by 0x4CCEDF: PortalRunMulti (pquery.c:1287)
==28150== by 0x4CDE19: PortalRun (pquery.c:800)
==28150== by 0x4C9E85: exec_simple_query (postgres.c:1099)
==28150== by 0x4CBF20: PostgresMain (postgres.c:4087)
==28150== by 0x44DC04: BackendRun (postmaster.c:4331)
==28150==
==28150== VALGRINDERROR-END
The cited line is the ExecInitQual call here:
/* varno = node->nominalRelation */
mapped_wcoList = map_partition_varattnos(wcoList,
node->nominalRelation,
partrel, rel);
foreach(ll, mapped_wcoList)
{
WithCheckOption *wco = (WithCheckOption *) lfirst(ll);
ExprState *wcoExpr = ExecInitQual((List *) wco->qual,
mtstate->mt_plans[i]);
wcoExprs = lappend(wcoExprs, wcoExpr);
}
First guess is that map_partition_varattnos has forgotten to handle
WithCheckOption.qual. If so, though, and if that's not resulting
in visible misbehavior in the regression tests, then we are missing
a case that the regression tests should be covering.
BTW, it might be advisable to use castNode(WithCheckOption, ...)
in the line before that.
regards, tom lane
On 06/11/2017 05:35 PM, Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>> Apply RLS policies to partitioned tables.
>
> Buildfarm member skink has grown a "make check" failure with this commit.
>
> ==28150== VALGRINDERROR-BEGIN
> ==28150== Invalid read of size 8
> ==28150== at 0x39A355: ExecInitModifyTable (nodeModifyTable.c:1862)
> ==28150== by 0x37F0F8: ExecInitNode (execProcnode.c:168)
> ==28150== by 0x37C219: InitPlan (execMain.c:1044)
> ==28150== by 0x37C3AF: standard_ExecutorStart (execMain.c:256)
> ==28150== by 0x37C4B8: ExecutorStart (execMain.c:151)
> ==28150== by 0x4CCCCE: ProcessQuery (pquery.c:157)
> ==28150== by 0x4CCEDF: PortalRunMulti (pquery.c:1287)
> ==28150== by 0x4CDE19: PortalRun (pquery.c:800)
> ==28150== by 0x4C9E85: exec_simple_query (postgres.c:1099)
> ==28150== by 0x4CBF20: PostgresMain (postgres.c:4087)
> ==28150== by 0x44DC04: BackendRun (postmaster.c:4331)
> ==28150== by 0x44FD9B: BackendStartup (postmaster.c:4003)
> ==28150== Address 0xbbdbec8 is 8,008 bytes inside a recently re-allocated block of size 8,192 alloc'd
> ==28150== at 0x4C2AB76: malloc (vg_replace_malloc.c:299)
> ==28150== by 0x6002D2: AllocSetAlloc (aset.c:760)
> ==28150== by 0x606EB5: MemoryContextAllocZeroAligned (mcxt.c:791)
> ==28150== by 0x3832B1: CreateExecutorState (execUtils.c:99)
> ==28150== by 0x37C2E6: standard_ExecutorStart (execMain.c:186)
> ==28150== by 0x37C4B8: ExecutorStart (execMain.c:151)
> ==28150== by 0x4CCCCE: ProcessQuery (pquery.c:157)
> ==28150== by 0x4CCEDF: PortalRunMulti (pquery.c:1287)
> ==28150== by 0x4CDE19: PortalRun (pquery.c:800)
> ==28150== by 0x4C9E85: exec_simple_query (postgres.c:1099)
> ==28150== by 0x4CBF20: PostgresMain (postgres.c:4087)
> ==28150== by 0x44DC04: BackendRun (postmaster.c:4331)
> ==28150==
> ==28150== VALGRINDERROR-END
>
> The cited line is the ExecInitQual call here:
>
> /* varno = node->nominalRelation */
> mapped_wcoList = map_partition_varattnos(wcoList,
> node->nominalRelation,
> partrel, rel);
> foreach(ll, mapped_wcoList)
> {
> WithCheckOption *wco = (WithCheckOption *) lfirst(ll);
> ExprState *wcoExpr = ExecInitQual((List *) wco->qual,
> mtstate->mt_plans[i]);
>
> wcoExprs = lappend(wcoExprs, wcoExpr);
> }
>
> First guess is that map_partition_varattnos has forgotten to handle
> WithCheckOption.qual. If so, though, and if that's not resulting
> in visible misbehavior in the regression tests, then we are missing
> a case that the regression tests should be covering.
>
> BTW, it might be advisable to use castNode(WithCheckOption, ...)
> in the line before that.
Drat. I'll take a look, but it would probably be good if someone
generally familiar with the partitioned tables patches have a look as well.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Attachment
On 2017/06/12 11:09, Joe Conway wrote:
> On 06/11/2017 05:35 PM, Tom Lane wrote:
>> Joe Conway <mail@joeconway.com> writes:
>>> Apply RLS policies to partitioned tables.
>>
>> Buildfarm member skink has grown a "make check" failure with this commit.
>>
>> ==28150== VALGRINDERROR-BEGIN
>> ==28150== Invalid read of size 8
>> ==28150== at 0x39A355: ExecInitModifyTable (nodeModifyTable.c:1862)
>> ==28150== by 0x37F0F8: ExecInitNode (execProcnode.c:168)
>> ==28150== by 0x37C219: InitPlan (execMain.c:1044)
>> ==28150== by 0x37C3AF: standard_ExecutorStart (execMain.c:256)
>> ==28150== by 0x37C4B8: ExecutorStart (execMain.c:151)
>> ==28150== by 0x4CCCCE: ProcessQuery (pquery.c:157)
>> ==28150== by 0x4CCEDF: PortalRunMulti (pquery.c:1287)
>> ==28150== by 0x4CDE19: PortalRun (pquery.c:800)
>> ==28150== by 0x4C9E85: exec_simple_query (postgres.c:1099)
>> ==28150== by 0x4CBF20: PostgresMain (postgres.c:4087)
>> ==28150== by 0x44DC04: BackendRun (postmaster.c:4331)
>> ==28150== by 0x44FD9B: BackendStartup (postmaster.c:4003)
>> ==28150== Address 0xbbdbec8 is 8,008 bytes inside a recently re-allocated block of size 8,192 alloc'd
>> ==28150== at 0x4C2AB76: malloc (vg_replace_malloc.c:299)
>> ==28150== by 0x6002D2: AllocSetAlloc (aset.c:760)
>> ==28150== by 0x606EB5: MemoryContextAllocZeroAligned (mcxt.c:791)
>> ==28150== by 0x3832B1: CreateExecutorState (execUtils.c:99)
>> ==28150== by 0x37C2E6: standard_ExecutorStart (execMain.c:186)
>> ==28150== by 0x37C4B8: ExecutorStart (execMain.c:151)
>> ==28150== by 0x4CCCCE: ProcessQuery (pquery.c:157)
>> ==28150== by 0x4CCEDF: PortalRunMulti (pquery.c:1287)
>> ==28150== by 0x4CDE19: PortalRun (pquery.c:800)
>> ==28150== by 0x4C9E85: exec_simple_query (postgres.c:1099)
>> ==28150== by 0x4CBF20: PostgresMain (postgres.c:4087)
>> ==28150== by 0x44DC04: BackendRun (postmaster.c:4331)
>> ==28150==
>> ==28150== VALGRINDERROR-END
>>
>> The cited line is the ExecInitQual call here:
>>
>> /* varno = node->nominalRelation */
>> mapped_wcoList = map_partition_varattnos(wcoList,
>> node->nominalRelation,
>> partrel, rel);
>> foreach(ll, mapped_wcoList)
>> {
>> WithCheckOption *wco = (WithCheckOption *) lfirst(ll);
>> ExprState *wcoExpr = ExecInitQual((List *) wco->qual,
>> mtstate->mt_plans[i]);
>>
>> wcoExprs = lappend(wcoExprs, wcoExpr);
>> }
>>
>> First guess is that map_partition_varattnos has forgotten to handle
>> WithCheckOption.qual. If so, though, and if that's not resulting
>> in visible misbehavior in the regression tests, then we are missing
>> a case that the regression tests should be covering.
>>
>> BTW, it might be advisable to use castNode(WithCheckOption, ...)
>> in the line before that.
>
> Drat. I'll take a look, but it would probably be good if someone
> generally familiar with the partitioned tables patches have a look as well.
I am looking too. Commit 587cda35ca3 which added that code did add a test
in updatable_views.sql, but tests added by this commit have perhaps
exposed something not previously covered.
Thanks,
Amit
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2017/06/12 11:09, Joe Conway wrote:
>> On 06/11/2017 05:35 PM, Tom Lane wrote:
>>> First guess is that map_partition_varattnos has forgotten to handle
>>> WithCheckOption.qual.
>> Drat. I'll take a look, but it would probably be good if someone
>> generally familiar with the partitioned tables patches have a look as well.
> I am looking too. Commit 587cda35ca3 which added that code did add a test
> in updatable_views.sql, but tests added by this commit have perhaps
> exposed something not previously covered.
My first guess above is wrong -- the undefined reference is actually
"mtstate->mt_plans[i]". This is because mtstate->mt_num_partitions is
more than mtstate->mt_nplans in the failing case, so that the blithe
assumption that we can use the partition number to index into
mtstate->mt_plans is certainly wrong. Don't know how this should
be corrected, offhand, but do we really have more than one plan
for a partitioned query?
The specific query that's failing is
(gdb) p debug_query_string
$1 = 0x11b6220 "INSERT INTO part_document VALUES (100, 11, 5, 'regress_rls_dave', 'testing pp1');"
and you can get a crash without all the valgrind effort by adding
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index bf26488..5013a6d 100644
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
*************** ExecInitModifyTable(ModifyTable *node, E
*** 1844,1849 ****
--- 1844,1850 ----
Assert(operation == CMD_INSERT);
resultRelInfo = mtstate->mt_partitions;
+ Assert(mtstate->mt_num_partitions == mtstate->mt_nplans);
wcoList = linitial(node->withCheckOptionLists);
for (i = 0; i < mtstate->mt_num_partitions; i++)
{
regards, tom lane
On 2017/06/12 11:51, Tom Lane wrote: > Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes: >> On 2017/06/12 11:09, Joe Conway wrote: >>> On 06/11/2017 05:35 PM, Tom Lane wrote: >>>> First guess is that map_partition_varattnos has forgotten to handle >>>> WithCheckOption.qual. > >>> Drat. I'll take a look, but it would probably be good if someone >>> generally familiar with the partitioned tables patches have a look as well. > >> I am looking too. Commit 587cda35ca3 which added that code did add a test >> in updatable_views.sql, but tests added by this commit have perhaps >> exposed something not previously covered. > > My first guess above is wrong -- the undefined reference is actually > "mtstate->mt_plans[i]". This is because mtstate->mt_num_partitions is > more than mtstate->mt_nplans in the failing case, so that the blithe > assumption that we can use the partition number to index into > mtstate->mt_plans is certainly wrong. Yes, the code should never access mtstate->mt_plans[i] for i > 0, because in this case (the INSERT case), there is only one plan. Unlike UPDATE/DELETE cases, we don't build plans for every partition in the INSERT case. > Don't know how this should > be corrected, offhand, but do we really have more than one plan > for a partitioned query? There can't be, as mentioned, in the INSERT case. How about something like the attached? The patch makes mtstate->mt_plans[0] to be passed for all invocations of ExecInitQual() on WithCheckOption.qual that is being initialized for each partition. Note that we are passing a PlanState that's based on the root parent table's properties to initialize WITH CHECK OPTION quals to be checked against rows that will be inserted into partitions (that is, after tuple-routing). I tried to come up with a test that would cause the existing (unpatched) code to crash, but couldn't. How would one define a policy or WITH CHECK OPTIONS view or something else, such that ExecInitQual() called on WithCheckOptions.qual would try to access parent planstate and would crash once parent becomes an invalid pointer (mtstate->mt_plans[i] for i > 0)? After looking at the code downstream to ExecInitQual(), it seems that the following cases would cases would cause parent planstate to be accessed: * Qual references a whole row var * Qual references an Aggref or a GroupingFunc or a WindowFunc * Qual references a (Alternative) SubPlan Thanks, Amit
Attachment
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> How about something like the attached? The patch makes
> mtstate->mt_plans[0] to be passed for all invocations of ExecInitQual() on
> WithCheckOption.qual that is being initialized for each partition.
Well, the question then is whether that behaves correctly. Where it would
matter is if the WCO qual contains a SubPlan, because you'd be adding
multiple instances of the subplan to the PlanState's list. I think it's
probably all right, but it might be wise to add a test case where there
is such a subplan.
Spacing and comment wording seem a bit random in this, too.
regards, tom lane
On 2017/06/12 22:46, Tom Lane wrote: > Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes: >> How about something like the attached? The patch makes >> mtstate->mt_plans[0] to be passed for all invocations of ExecInitQual() on >> WithCheckOption.qual that is being initialized for each partition. > > Well, the question then is whether that behaves correctly. Where it would > matter is if the WCO qual contains a SubPlan, because you'd be adding > multiple instances of the subplan to the PlanState's list. I think it's > probably all right, but it might be wise to add a test case where there > is such a subplan. > > Spacing and comment wording seem a bit random in this, too. Was about to send the updated patch, but noticed that you already committed it after adding the test. Thanks a lot. Regards, Amit