Subject: Re: pgsql: Apply RLS policies to partitioned tables.
References: <E1dK59q-0005XI-2U@gemulon.postgresql.org>
 <10974.1497227727@sss.pgh.pa.us>
 <0c5b8b18-262d-de09-50e1-6cf4815d74ab@joeconway.com>
 <bea1642d-2791-fe5f-e9cf-df12ea5cf162@lab.ntt.co.jp>
 <25990.1497235885@sss.pgh.pa.us>
From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Message-ID: <89e6ae66-96ee-b6eb-7195-d88dbb988f2f@lab.ntt.co.jp>
Date: Mon, 12 Jun 2017 15:48:09 +0900
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.1.1
MIME-Version: 1.0
In-Reply-To: <25990.1497235885@sss.pgh.pa.us>
Content-Type: multipart/mixed;
 boundary="------------451EA74BFB0A7F53EDD1DBCA"
Content-Language: en-US
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Joe Conway <mail@joeconway.com>, pgsql-committers@postgresql.org
Precedence: bulk
Sender: pgsql-committers-owner@postgresql.org

This is a multi-part message in MIME format.
--------------451EA74BFB0A7F53EDD1DBCA
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit

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

--------------451EA74BFB0A7F53EDD1DBCA
Content-Type: text/plain; charset=UTF-8;
 name="0001-Don-t-pass-invalid-PlanState-pointer-to-ExecInitQual.patch"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename*0="0001-Don-t-pass-invalid-PlanState-pointer-to-ExecInitQual.pa";
 filename*1="tch"

RnJvbSA5MGZkYWQyN2MwNGRiNzBmNzhjMjRlNDk5OWNkM2FlZjdjMDZkMWY3
IE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBhbWl0IDxhbWl0bGFu
Z290ZTA5QGdtYWlsLmNvbT4KRGF0ZTogTW9uLCAxMiBKdW4gMjAxNyAxNDoz
NTowNSArMDkwMApTdWJqZWN0OiBbUEFUQ0hdIERvbid0IHBhc3MgaW52YWxp
ZCBQbGFuU3RhdGUgcG9pbnRlciB0byBFeGVjSW5pdFF1YWwKCkN1cnJlbnQg
Y29kZSBpbiBFeGVjSW5pdE1vZGlmeVRhYmxlIHRoYXQgaW5pdGlhbGl6ZXMK
V2l0aENoZWNrT3B0aW9uLnF1YWwgZmFpbGVkIHRvIGNvbnNpZGVyIHRoYXQg
dGhlcmUgKmlzbid0KiBvbmUKcGxhbiBwZXIgcGFydGl0aW9uIGluIHRoZSBN
b2RpZnlUYWJsZVN0YXRlLm10X3BsYW5zW10gYXJyYXkuCi0tLQogc3JjL2Jh
Y2tlbmQvZXhlY3V0b3Ivbm9kZU1vZGlmeVRhYmxlLmMgfCAxNiArKysrKysr
KysrKystLS0tCiAxIGZpbGUgY2hhbmdlZCwgMTIgaW5zZXJ0aW9ucygrKSwg
NCBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9zcmMvYmFja2VuZC9leGVj
dXRvci9ub2RlTW9kaWZ5VGFibGUuYyBiL3NyYy9iYWNrZW5kL2V4ZWN1dG9y
L25vZGVNb2RpZnlUYWJsZS5jCmluZGV4IGJmMjY0ODhjNTEuLjg5OTkxZTM0
YWUgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL2V4ZWN1dG9yL25vZGVNb2Rp
ZnlUYWJsZS5jCisrKyBiL3NyYy9iYWNrZW5kL2V4ZWN1dG9yL25vZGVNb2Rp
ZnlUYWJsZS5jCkBAIC0xODQxLDggKzE4NDEsMTcgQEAgRXhlY0luaXRNb2Rp
ZnlUYWJsZShNb2RpZnlUYWJsZSAqbm9kZSwgRVN0YXRlICplc3RhdGUsIGlu
dCBlZmxhZ3MpCiAJaWYgKG5vZGUtPndpdGhDaGVja09wdGlvbkxpc3RzICE9
IE5JTCAmJiBtdHN0YXRlLT5tdF9udW1fcGFydGl0aW9ucyA+IDApCiAJewog
CQlMaXN0CSAgICp3Y29MaXN0OworCQlQbGFuU3RhdGUgICpwbGFuOwogCi0J
CUFzc2VydChvcGVyYXRpb24gPT0gQ01EX0lOU0VSVCk7CisJCS8qCisJCSAq
IEluIGNhc2Ugb2YgSU5TRVJUIG9uIHBhcnRpdGlvbmVkIHRhYmxlcywgdGhl
cmUgaXMgb25seSBwbGFuOyBpbiB0aGUKKwkJICogcGxhbm5lciB3ZSBkb24n
dCBidWlsZCBvbmUgZm9yIGVhY2ggcGFydGl0aW9uLiAgTGlrZXdpc2UsIHRo
ZXJlIGlzCisJCSAqIG9ubHkgb25lIFdJVEggQ0hFQ0sgT1BUSU9OUyBsaXN0
LCBub3Qgb25lIHBlciBwYXJ0aXRpb24uCisJCSAqLworCQlBc3NlcnQob3Bl
cmF0aW9uID09IENNRF9JTlNFUlQgJiYKKwkJCSAgIGxpc3RfbGVuZ3RoKG5v
ZGUtPndpdGhDaGVja09wdGlvbkxpc3RzKSA9PSAxICYmCisJCQkgICBtdHN0
YXRlLT5tdF9ucGxhbnMgPT0gMSk7CisJCXBsYW4gPSBtdHN0YXRlLT5tdF9w
bGFuc1swXTsJLyogVGhlIG9ubHkgcGxhbiAqLwogCQlyZXN1bHRSZWxJbmZv
ID0gbXRzdGF0ZS0+bXRfcGFydGl0aW9uczsKIAkJd2NvTGlzdCA9IGxpbml0
aWFsKG5vZGUtPndpdGhDaGVja09wdGlvbkxpc3RzKTsKIAkJZm9yIChpID0g
MDsgaSA8IG10c3RhdGUtPm10X251bV9wYXJ0aXRpb25zOyBpKyspCkBAIC0x
ODU4LDEwICsxODY3LDkgQEAgRXhlY0luaXRNb2RpZnlUYWJsZShNb2RpZnlU
YWJsZSAqbm9kZSwgRVN0YXRlICplc3RhdGUsIGludCBlZmxhZ3MpCiAJCQkJ
CQkJCQkJCQkJIHBhcnRyZWwsIHJlbCk7CiAJCQlmb3JlYWNoKGxsLCBtYXBw
ZWRfd2NvTGlzdCkKIAkJCXsKLQkJCQlXaXRoQ2hlY2tPcHRpb24gKndjbyA9
IChXaXRoQ2hlY2tPcHRpb24gKikgbGZpcnN0KGxsKTsKLQkJCQlFeHByU3Rh
dGUgICp3Y29FeHByID0gRXhlY0luaXRRdWFsKChMaXN0ICopIHdjby0+cXVh
bCwKLQkJCQkJCQkJCQkJCSAgIG10c3RhdGUtPm10X3BsYW5zW2ldKTsKKwkJ
CQlXaXRoQ2hlY2tPcHRpb24gKndjbyA9IGNhc3ROb2RlKFdpdGhDaGVja09w
dGlvbiwgbGZpcnN0KGxsKSk7CiAKKwkJCQlFeHByU3RhdGUgICp3Y29FeHBy
ID0gRXhlY0luaXRRdWFsKChMaXN0ICopIHdjby0+cXVhbCwgcGxhbik7CiAJ
CQkJd2NvRXhwcnMgPSBsYXBwZW5kKHdjb0V4cHJzLCB3Y29FeHByKTsKIAkJ
CX0KIAotLSAKMi4xMS4wCgo=

--------------451EA74BFB0A7F53EDD1DBCA
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

--------------451EA74BFB0A7F53EDD1DBCA--