Re: [HACKERS] partitioned tables and contrib/sepgsql

2017-03-09 Thread Mike Palmiotto 
On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost  wrote:
> Greetings,
>
> While going over the contrib modules, I noticed that sepgsql was not
> updated for partitioned tables.  What that appears to mean is that it's
> not possible to define labels on partitioned tables.  As I recall,
> accessing the parent of a table will, similar to the GRANT system, not
> perform checkes against the child tables, meaning that there's no way to
> have SELinux checks properly enforced when partitioned tables are being
> used.

I'll start taking a look at this. Presumably we'd just extend existing
object_access_hooks to cover partitioned tables?

>
> This is an issue which should be resolved for PG10, so I'll add it to
> the open items list.

I'll grab it. Thanks.

--Mike


-- 
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] partitioned tables and contrib/sepgsql

2017-03-27 Thread Mike Palmiotto 
On Mon, Mar 27, 2017 at 10:47 AM, Robert Haas  wrote:
> On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost  wrote:
>> While going over the contrib modules, I noticed that sepgsql was not
>> updated for partitioned tables.  What that appears to mean is that it's
>> not possible to define labels on partitioned tables.
>
> It works for me:
>
> rhaas=# load 'dummy_seclabel';
> LOAD
> rhaas=# create table foo (a int, b text) partition by range (a);
> CREATE TABLE
> rhaas=# security label on table foo is 'classified';
> SECURITY LABEL
>
> What exactly is the problem you're seeing?

IIRC the initial concern was that contrib/sepgsql was not updated for
RELKIND_PARTITIONED_TABLE, so the post-create hook would not work
properly for partitioned tables.

I've looked into it a bit and saw a post-alter hook in
StoreCatalogInheritance1. It seems like it may just be an issue of
adding the RELKIND_PARTITIONED_TABLE to sepgsql_relation_post_create.

-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com


-- 
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] partitioned tables and contrib/sepgsql

2017-03-27 Thread Mike Palmiotto 
On Mon, Mar 27, 2017 at 11:46 AM, Robert Haas  wrote:
> On Mon, Mar 27, 2017 at 11:22 AM, Mike Palmiotto
>  wrote:
>> On Mon, Mar 27, 2017 at 10:47 AM, Robert Haas  wrote:
>>> On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost  wrote:
>>>> While going over the contrib modules, I noticed that sepgsql was not
>>>> updated for partitioned tables.  What that appears to mean is that it's
>>>> not possible to define labels on partitioned tables.
>>>
>>> It works for me:
>>>
>>> rhaas=# load 'dummy_seclabel';
>>> LOAD
>>> rhaas=# create table foo (a int, b text) partition by range (a);
>>> CREATE TABLE
>>> rhaas=# security label on table foo is 'classified';
>>> SECURITY LABEL
>>>
>>> What exactly is the problem you're seeing?
>>
>> IIRC the initial concern was that contrib/sepgsql was not updated for
>> RELKIND_PARTITIONED_TABLE, so the post-create hook would not work
>> properly for partitioned tables.
>>
>> I've looked into it a bit and saw a post-alter hook in
>> StoreCatalogInheritance1. It seems like it may just be an issue of
>> adding the RELKIND_PARTITIONED_TABLE to sepgsql_relation_post_create.
>
> I thought that kind of thing might be the issue, but it didn't seem to
> match Stephen's description.  Adding RELKIND_PARTITIONED_TABLE to that
> function seems like it would probably be sufficient to make that hook
> work, although that would need to be tested, but there are numerous
> other references to RELKIND_RELATION in contrib/sepgsql, some of which
> probably also need to be updated to consider
> RELKIND_PARTITIONED_TABLE.

Agreed.

>
> In my view, this is really an enhancement to sepgsql to handle a new
> PostgreSQL feature rather than a defect in the partitioning commit, so
> I don't think it falls on Amit Langote (as the author) or me (as the
> committer) to fix.  There might similarly be updates to sepgsql to do
> something with permissions on logical replication's new publication
> and subscription objects, but we should similarly regard those as
> possible new features, not things that Petr or Peter need to work on.

I'll keep these items in mind for future reference. Thanks.

>  Note that sepgsql hasn't been updated to work with RLS yet, either,
> but we didn't regard that as an open item for RLS, or if we did the
> resolution was just to document it.  I am not opposed to giving a
> little more time to get this straightened out, but if a patch doesn't
> show up fairly soon then I think we should just document that sepgsql
> doesn't support partitioned tables in v10.  sepgsql has a fairly
> lengthy list of implementation restrictions already, so one more is
> not going to kill anybody -- or if it will then that person should
> produce a patch soon.

Okay, I'll make sure I get something fleshed out today or tomorrow.

-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com


-- 
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] partitioned tables and contrib/sepgsql

2017-03-31 Thread Mike Palmiotto 
On Mon, Mar 27, 2017 at 12:09 PM, Mike Palmiotto
 wrote:
> On Mon, Mar 27, 2017 at 11:46 AM, Robert Haas  wrote:
>> 
>>  Note that sepgsql hasn't been updated to work with RLS yet, either,
>> but we didn't regard that as an open item for RLS, or if we did the
>> resolution was just to document it.  I am not opposed to giving a
>> little more time to get this straightened out, but if a patch doesn't
>> show up fairly soon then I think we should just document that sepgsql
>> doesn't support partitioned tables in v10.  sepgsql has a fairly
>> lengthy list of implementation restrictions already, so one more is
>> not going to kill anybody -- or if it will then that person should
>> produce a patch soon.
>
> Okay, I'll make sure I get something fleshed out today or tomorrow.

Apologies for the delay. I was waffling over whether to reference
PartitionedRelationId in sepgsql, but ended up deciding to just handle
RELKIND_PARTITIONED_TABLE and treat the classOid as
RelationRelationId. Seeing as there is a relid in pg_class which
corresponds to the partitioned table, this chosen route seemed
acceptable.

Here is a demonstration of the partitioned table working with sepgsql hooks:
https://gist.github.com/anonymous/b10f476a95ae9cdd39b83ef872d4b1e6

Attached you will find two patches, which were rebased on master as of
156d388 (applied with `git am --revert [patch file]`). The first gets
rid of some pesky compiler warnings and the second implements the
sepgsql handling of partitioned tables.

Thanks,

-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com
From 06463a4545c1cd0a2740e201d06a36b78dc2da8c Mon Sep 17 00:00:00 2001
From: Mike Palmiotto 
Date: Wed, 29 Mar 2017 14:59:37 +
Subject: [PATCH 2/2] Add partitioned table support to sepgsql

Account for RELKIND_PARTITIONED_RELATIONS in sepgsql and treat the objects like
regular relations. This allows for proper create/alter/drop hook behavior for
partitioned tables.
---
 contrib/sepgsql/hooks.c|  1 +
 contrib/sepgsql/label.c|  4 +++-
 contrib/sepgsql/relation.c | 34 --
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c
index 93cc8de..89e71e3 100644
--- a/contrib/sepgsql/hooks.c
+++ b/contrib/sepgsql/hooks.c
@@ -15,6 +15,7 @@
 #include "catalog/pg_class.h"
 #include "catalog/pg_database.h"
 #include "catalog/pg_namespace.h"
+#include "catalog/pg_partitioned_table.h"
 #include "catalog/pg_proc.h"
 #include "commands/seclabel.h"
 #include "executor/executor.h"
diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index a404cd7..546 100644
--- a/contrib/sepgsql/label.c
+++ b/contrib/sepgsql/label.c
@@ -23,6 +23,7 @@
 #include "catalog/pg_class.h"
 #include "catalog/pg_database.h"
 #include "catalog/pg_namespace.h"
+#include "catalog/pg_partitioned_table.h"
 #include "catalog/pg_proc.h"
 #include "commands/dbcommands.h"
 #include "commands/seclabel.h"
@@ -779,7 +780,8 @@ exec_object_restorecon(struct selabel_handle * sehnd, Oid catalogId)
 			case RelationRelationId:
 relForm = (Form_pg_class) GETSTRUCT(tuple);
 
-if (relForm->relkind == RELKIND_RELATION)
+if (relForm->relkind == RELKIND_RELATION ||
+	relForm->relkind == RELKIND_PARTITIONED_TABLE)
 	objtype = SELABEL_DB_TABLE;
 else if (relForm->relkind == RELKIND_SEQUENCE)
 	objtype = SELABEL_DB_SEQUENCE;
diff --git a/contrib/sepgsql/relation.c b/contrib/sepgsql/relation.c
index fd6fe8b..93022d4 100644
--- a/contrib/sepgsql/relation.c
+++ b/contrib/sepgsql/relation.c
@@ -19,6 +19,7 @@
 #include "catalog/pg_attribute.h"
 #include "catalog/pg_class.h"
 #include "catalog/pg_namespace.h"
+#include "catalog/pg_partitioned_table.h"
 #include "commands/seclabel.h"
 #include "lib/stringinfo.h"
 #include "utils/builtins.h"
@@ -54,12 +55,14 @@ sepgsql_attribute_post_create(Oid relOid, AttrNumber attnum)
 	ObjectAddress object;
 	Form_pg_attribute attForm;
 	StringInfoData audit_name;
+	char		relkind;
 
 	/*
 	 * Only attributes within regular relation have individual security
 	 * labels.
 	 */
-	if (get_rel_relkind(relOid) != RELKIND_RELATION)
+	relkind = get_rel_relkind(relOid);
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
 		return;
 
 	/*
@@ -135,8 +138,10 @@ sepgsql_attribute_drop(Oid relOid, AttrNumber attnum)
 {
 	ObjectAddress object;
 	char	   *audit_name;
+	char		relkind;
 
-	if (get_rel_relkind(relOid) != RELKIND_RELATION)
+	relkind = get_rel_relkind(relOid);
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
 		return;
 
 	/*
@@ -167,8 +172,11 @@ sepgsql_attribut

Re: [HACKERS] partitioned tables and contrib/sepgsql

2017-03-31 Thread Mike Palmiotto 
On Fri, Mar 31, 2017 at 2:14 PM, Mike Palmiotto
 wrote:
> On Mon, Mar 27, 2017 at 12:09 PM, Mike Palmiotto
>  wrote:
>> On Mon, Mar 27, 2017 at 11:46 AM, Robert Haas  wrote:
>>> 
>>>  Note that sepgsql hasn't been updated to work with RLS yet, either,
>>> but we didn't regard that as an open item for RLS, or if we did the
>>> resolution was just to document it.  I am not opposed to giving a
>>> little more time to get this straightened out, but if a patch doesn't
>>> show up fairly soon then I think we should just document that sepgsql
>>> doesn't support partitioned tables in v10.  sepgsql has a fairly
>>> lengthy list of implementation restrictions already, so one more is
>>> not going to kill anybody -- or if it will then that person should
>>> produce a patch soon.
>>
>> Okay, I'll make sure I get something fleshed out today or tomorrow.
>
> Apologies for the delay. I was waffling over whether to reference
> PartitionedRelationId in sepgsql, but ended up deciding to just handle
> RELKIND_PARTITIONED_TABLE and treat the classOid as
> RelationRelationId. Seeing as there is a relid in pg_class which
> corresponds to the partitioned table, this chosen route seemed
> acceptable.

Newest patches remove cruft from said waffling. No need to include
pg_partitioned_table.h if we're not referencing PartitionedRelationId.

>
> Here is a demonstration of the partitioned table working with sepgsql hooks:
> https://gist.github.com/anonymous/b10f476a95ae9cdd39b83ef872d4b1e6
>
> Attached you will find two patches, which were rebased on master as of
> 156d388 (applied with `git am --revert [patch file]`). The first gets
> rid of some pesky compiler warnings and the second implements the
> sepgsql handling of partitioned tables.

That should have read `git am --reject [patch file]`. Apologies for
the inaccuracy.

Thanks,

-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com
From 82ff9a4e18d3baefa5530b8515e46cf8225519de Mon Sep 17 00:00:00 2001
From: Mike Palmiotto 
Date: Tue, 28 Mar 2017 16:44:54 +
Subject: [PATCH 1/2] Silence some sepgsql compiler warnings

selinux/label.h includes stdbool.h, which redefines the bool type and results in
a warning: assignment from incompatible pointer type for sepgsql_fmgr_hook. Move
selinux/label.h above postgres.h, so the bool type is properly defined.

Additionally, sepgsql throws compiler warnings due to possibly uninitialized
tclass in code paths for indexes. Set tclass to a bogus -1 to silence these
warnings.
---
 contrib/sepgsql/label.c| 4 ++--
 contrib/sepgsql/relation.c | 8 ++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 1a8f884..a404cd7 100644
--- a/contrib/sepgsql/label.c
+++ b/contrib/sepgsql/label.c
@@ -8,6 +8,8 @@
  *
  * -
  */
+#include 
+
 #include "postgres.h"
 
 #include "access/heapam.h"
@@ -37,8 +39,6 @@
 
 #include "sepgsql.h"
 
-#include 
-
 /*
  * Saved hook entries (if stacked)
  */
diff --git a/contrib/sepgsql/relation.c b/contrib/sepgsql/relation.c
index ab98a9b..fd6fe8b 100644
--- a/contrib/sepgsql/relation.c
+++ b/contrib/sepgsql/relation.c
@@ -300,8 +300,10 @@ sepgsql_relation_post_create(Oid relOid)
 			tclass = SEPG_CLASS_DB_VIEW;
 			break;
 		case RELKIND_INDEX:
-			/* deal with indexes specially; no need for tclass */
+			/* other indexes are handled specially below; set tclass to -1 to
+			 * silence compiler warning */
 			sepgsql_index_modify(relOid);
+			tclass = -1;
 			goto out;
 		default:
 			/* ignore other relkinds */
@@ -432,7 +434,9 @@ sepgsql_relation_drop(Oid relOid)
 			/* ignore indexes on toast tables */
 			if (get_rel_namespace(relOid) == PG_TOAST_NAMESPACE)
 return;
-			/* other indexes are handled specially below; no need for tclass */
+			/* other indexes are handled specially below; set tclass to -1 to
+			 * silence compiler warning */
+			tclass = -1;
 			break;
 		default:
 			/* ignore other relkinds */
-- 
2.7.4

From b3a44aa37b5724ea88fb0d1110ba18be6618e283 Mon Sep 17 00:00:00 2001
From: Mike Palmiotto 
Date: Wed, 29 Mar 2017 14:59:37 +
Subject: [PATCH 2/2] Add partitioned table support to sepgsql

Account for RELKIND_PARTITIONED_RELATIONS in sepgsql and treat the objects like
regular relations. This allows for proper create/alter/drop hook behavior for
partitioned tables.
---
 contrib/sepgsql/label.c|  3 ++-
 contrib/sepgsql/relation.c | 33 +++--
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index a404cd7..88a04a4 100644
--- a/contrib/sepgsql/label.c
+++ b/contrib/sepgsql/label.c
@@ -779,7 +779,8 @@ ex

Re: [HACKERS] partitioned tables and contrib/sepgsql

2017-04-03 Thread Mike Palmiotto 
On Fri, Mar 31, 2017 at 8:23 PM, Robert Haas  wrote:
> On Fri, Mar 31, 2017 at 2:14 PM, Mike Palmiotto
>  wrote:
>> Attached you will find two patches, which were rebased on master as of
>> 156d388 (applied with `git am --revert [patch file]`). The first gets
>> rid of some pesky compiler warnings and the second implements the
>> sepgsql handling of partitioned tables.
>
> 0001 has the problem that we have a firm rule against putting any
> #includes whatsoever before "postgres.h".  This stdbool issue has come
> up before, though, and I fear we're going to need to do something
> about it.

Yeah, I recall this rule. The only things I can really think of to
solve the problem are:
1) #define _STDBOOL_H in postgres's c.h once bool and the like have
been defined, so we can avoid re-definition.
2) Enforce that any code utilizing the stdbool header manage the
re-definition with some combination of #undef/#define/#typedef and
document the issue somewhere.

I'd be more inclined to believe 2) is the correct solution, since 1)
is more of a hack than anything.

Thoughts?

>
> 0002 looks extremely straightforward, but I wonder if we could get one
> of the people on this list who knows about sepgsql to have a look?
> (Stephen Frost, Joe Conway, KaiGai Kohei...)

I welcome any and all feedback.

Thanks,
-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com


-- 
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] partitioned tables and contrib/sepgsql

2017-04-04 Thread Mike Palmiotto 
On Tue, Apr 4, 2017 at 10:19 AM, Joe Conway  wrote:
> On 04/04/2017 06:45 AM, Robert Haas wrote:
>> On Mon, Apr 3, 2017 at 12:02 PM, Joe Conway  wrote:
>>>> 0002 looks extremely straightforward, but I wonder if we could get one
>>>> of the people on this list who knows about sepgsql to have a look?
>>>> (Stephen Frost, Joe Conway, KaiGai Kohei...)
>>>
>>> Will have a look later today.
>>
>> I think it is now tomorrow, unless your time zone is someplace very
>> far away.  :-)
>
> OBE -- I have scheduled time in 30 minutes from now, after I have gotten
> my first cup of coffee ;-)

After some discussion off-list, I've rebased and udpated the patches.
Please see attached for further review.

Thanks,
-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com
From be692f8cc94d74033494d140c310e932c705e785 Mon Sep 17 00:00:00 2001
From: Mike Palmiotto 
Date: Wed, 29 Mar 2017 14:59:37 +
Subject: [PATCH 2/2] Add partitioned table support to sepgsql

Account for RELKIND_PARTITIONED_RELATIONS in sepgsql and treat the objects like
regular relations. This allows for proper object_access hook behavior for
partitioned tables.
---
 contrib/sepgsql/label.c|  3 ++-
 contrib/sepgsql/relation.c | 33 +++--
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 8a72503..c66581a 100644
--- a/contrib/sepgsql/label.c
+++ b/contrib/sepgsql/label.c
@@ -787,7 +787,8 @@ exec_object_restorecon(struct selabel_handle * sehnd, Oid catalogId)
 			case RelationRelationId:
 relForm = (Form_pg_class) GETSTRUCT(tuple);
 
-if (relForm->relkind == RELKIND_RELATION)
+if (relForm->relkind == RELKIND_RELATION ||
+	relForm->relkind == RELKIND_PARTITIONED_TABLE)
 	objtype = SELABEL_DB_TABLE;
 else if (relForm->relkind == RELKIND_SEQUENCE)
 	objtype = SELABEL_DB_SEQUENCE;
diff --git a/contrib/sepgsql/relation.c b/contrib/sepgsql/relation.c
index 4dc48a0..4fcebc1 100644
--- a/contrib/sepgsql/relation.c
+++ b/contrib/sepgsql/relation.c
@@ -54,12 +54,14 @@ sepgsql_attribute_post_create(Oid relOid, AttrNumber attnum)
 	ObjectAddress object;
 	Form_pg_attribute attForm;
 	StringInfoData audit_name;
+	char		relkind;
 
 	/*
 	 * Only attributes within regular relation have individual security
 	 * labels.
 	 */
-	if (get_rel_relkind(relOid) != RELKIND_RELATION)
+	relkind = get_rel_relkind(relOid);
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
 		return;
 
 	/*
@@ -135,8 +137,10 @@ sepgsql_attribute_drop(Oid relOid, AttrNumber attnum)
 {
 	ObjectAddress object;
 	char	   *audit_name;
+	char		relkind;
 
-	if (get_rel_relkind(relOid) != RELKIND_RELATION)
+	relkind = get_rel_relkind(relOid);
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
 		return;
 
 	/*
@@ -167,8 +171,11 @@ sepgsql_attribute_relabel(Oid relOid, AttrNumber attnum,
 {
 	ObjectAddress object;
 	char	   *audit_name;
+	char		relkind;
 
-	if (get_rel_relkind(relOid) != RELKIND_RELATION)
+	relkind = get_rel_relkind(relOid);
+
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
 		ereport(ERROR,
 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
  errmsg("cannot set security label on non-regular columns")));
@@ -209,8 +216,11 @@ sepgsql_attribute_setattr(Oid relOid, AttrNumber attnum)
 {
 	ObjectAddress object;
 	char	   *audit_name;
+	char		relkind;
+
+	relkind = get_rel_relkind(relOid);
 
-	if (get_rel_relkind(relOid) != RELKIND_RELATION)
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
 		return;
 
 	/*
@@ -290,6 +300,7 @@ sepgsql_relation_post_create(Oid relOid)
 
 	switch (classForm->relkind)
 	{
+		case RELKIND_PARTITIONED_TABLE:
 		case RELKIND_RELATION:
 			tclass = SEPG_CLASS_DB_TABLE;
 			break;
@@ -336,7 +347,8 @@ sepgsql_relation_post_create(Oid relOid)
   true);
 
 	/*
-	 * Assign the default security label on the new relation
+	 * Assign the default security label on the new relation or partitioned
+	 * table.
 	 */
 	object.classId = RelationRelationId;
 	object.objectId = relOid;
@@ -344,10 +356,10 @@ sepgsql_relation_post_create(Oid relOid)
 	SetSecurityLabel(&object, SEPGSQL_LABEL_TAG, rcontext);
 
 	/*
-	 * We also assigns a default security label on columns of the new regular
-	 * tables.
+	 * We also assign a default security label on columns of a new table.
 	 */
-	if (classForm->relkind == RELKIND_RELATION)
+	if (classForm->relkind == RELKIND_RELATION ||
+		classForm->relkind == RELKIND_PARTITIONED_TABLE)
 	{
 		Relation	arel;
 		ScanKeyData akey;
@@ -422,6 +434,7 @@ sepgsql_relation_drop(Oid relOid)
 	relkind = get_rel_relkind(relOid);
 	switch (relkind)
 	{
+		case RELKIND_PARTITIONED_TABLE:
 		case RELKIND_RELATION:
 			tclass = SEPG_CLASS_DB_TABLE

Re: [HACKERS] partitioned tables and contrib/sepgsql

2017-04-05 Thread Mike Palmiotto 
On Wed, Apr 5, 2017 at 1:19 PM, Tom Lane  wrote:
> Peter Eisentraut  writes:
>> On 4/5/17 12:04, Tom Lane wrote:
>>> Conclusion: Fedora's gcc is playing fast and loose somehow with the
>>> command "#include "; that does not include the file
>>> you'd think it does, it does something magic inside the compiler.
>>> The magic evidently includes not complaining about duplicate macro
>>> definitions for true and false.
>
>> Perhaps -Wsystem-headers would change the outcome in your case.
>
> Hah, you're right: with that,
>
> In file included from /usr/include/selinux/label.h:9:0,
>  from label.c:40:
> /usr/lib/gcc/x86_64-redhat-linux/6.3.1/include/stdbool.h:34:0: warning: 
> "true" redefined
>  #define true 1
>
> In file included from ../../src/include/postgres.h:47:0,
>  from label.c:11:
> ../../src/include/c.h:206:0: note: this is the location of the previous 
> definition
>  #define true ((bool) 1)
>
> and likewise for "false".  So that mystery is explained.
>
> I stand by my previous patch suggestion, except that we can replace
> the parenthetical comment with something like "(We don't care if
>  redefines "true"/"false"; those are close enough.)".
>

Sounds good. Updated patchset will include that verbiage, along with
some regression tests for partitioned tables.

Thanks,

-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com


-- 
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] partitioned tables and contrib/sepgsql

2017-04-05 Thread Mike Palmiotto 
On Wed, Apr 5, 2017 at 1:22 PM, Mike Palmiotto
 wrote:
> On Wed, Apr 5, 2017 at 1:19 PM, Tom Lane  wrote:
>> Peter Eisentraut  writes:
>>> On 4/5/17 12:04, Tom Lane wrote:
>>>> Conclusion: Fedora's gcc is playing fast and loose somehow with the
>>>> command "#include "; that does not include the file
>>>> you'd think it does, it does something magic inside the compiler.
>>>> The magic evidently includes not complaining about duplicate macro
>>>> definitions for true and false.
>>
>>> Perhaps -Wsystem-headers would change the outcome in your case.
>>
>> Hah, you're right: with that,
>>
>> In file included from /usr/include/selinux/label.h:9:0,
>>  from label.c:40:
>> /usr/lib/gcc/x86_64-redhat-linux/6.3.1/include/stdbool.h:34:0: warning: 
>> "true" redefined
>>  #define true 1
>>
>> In file included from ../../src/include/postgres.h:47:0,
>>  from label.c:11:
>> ../../src/include/c.h:206:0: note: this is the location of the previous 
>> definition
>>  #define true ((bool) 1)
>>
>> and likewise for "false".  So that mystery is explained.
>>
>> I stand by my previous patch suggestion, except that we can replace
>> the parenthetical comment with something like "(We don't care if
>>  redefines "true"/"false"; those are close enough.)".
>>
>
> Sounds good. Updated patchset will include that verbiage, along with
> some regression tests for partitioned tables.

Looks like the label regression test is failing even without these changes.

Weirdly enough, this only happens in one place:

 84 SELECT objtype, objname, label FROM pg_seclabels
 85 WHERE provider = 'selinux' AND objtype = 'column' AND (objname
like 't3.%' OR objname like 't4.%');

I haven't figured out why this may be (yet), but it seems like we
shouldn't assume the order of output from a SELECT. As I understand
it, the order of the output is subject to change with changes to the
planner/configuration.

I'm going to hold the partition table regression changes for a
separate patch and include some ORDER BY fixes. Will post tomorrow

In the meantime, attached are the latest and greatest patches.

Thanks,
-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com
From 50969159f0fd7c93a15bfb7b9046512399d0b099 Mon Sep 17 00:00:00 2001
From: Mike Palmiotto 
Date: Wed, 29 Mar 2017 14:59:37 +
Subject: [PATCH 3/3] Add partitioned table support to sepgsql

Account for RELKIND_PARTITIONED_RELATIONS in sepgsql and treat the objects like
regular relations. This allows for proper object_access hook behavior for
partitioned tables.
---
 contrib/sepgsql/label.c|  3 ++-
 contrib/sepgsql/relation.c | 30 +-
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index f7d79ab..1a1ec57 100644
--- a/contrib/sepgsql/label.c
+++ b/contrib/sepgsql/label.c
@@ -789,7 +789,8 @@ exec_object_restorecon(struct selabel_handle * sehnd, Oid catalogId)
 			case RelationRelationId:
 relForm = (Form_pg_class) GETSTRUCT(tuple);
 
-if (relForm->relkind == RELKIND_RELATION)
+if (relForm->relkind == RELKIND_RELATION ||
+	relForm->relkind == RELKIND_PARTITIONED_TABLE)
 	objtype = SELABEL_DB_TABLE;
 else if (relForm->relkind == RELKIND_SEQUENCE)
 	objtype = SELABEL_DB_SEQUENCE;
diff --git a/contrib/sepgsql/relation.c b/contrib/sepgsql/relation.c
index b794abe..0d8905c 100644
--- a/contrib/sepgsql/relation.c
+++ b/contrib/sepgsql/relation.c
@@ -54,12 +54,14 @@ sepgsql_attribute_post_create(Oid relOid, AttrNumber attnum)
 	ObjectAddress object;
 	Form_pg_attribute attForm;
 	StringInfoData audit_name;
+	char		relkind;
 
 	/*
 	 * Only attributes within regular relation have individual security
 	 * labels.
 	 */
-	if (get_rel_relkind(relOid) != RELKIND_RELATION)
+	relkind = get_rel_relkind(relOid);
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
 		return;
 
 	/*
@@ -135,8 +137,10 @@ sepgsql_attribute_drop(Oid relOid, AttrNumber attnum)
 {
 	ObjectAddress object;
 	char	   *audit_name;
+	char		relkind;
 
-	if (get_rel_relkind(relOid) != RELKIND_RELATION)
+	relkind = get_rel_relkind(relOid);
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
 		return;
 
 	/*
@@ -167,8 +171,11 @@ sepgsql_attribute_relabel(Oid relOid, AttrNumber attnum,
 {
 	ObjectAddress object;
 	char	   *audit_name;
+	char		relkind;
 
-	if (get_rel_relkind(relOid) != RELKIND_RELATION)
+	relkind = get_rel_relkind(relOid);
+
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
 		ereport

Re: [HACKERS] partitioned tables and contrib/sepgsql

2017-04-07 Thread Mike Palmiotto 
On Thu, Apr 6, 2017 at 5:52 PM, Joe Conway  wrote:
> On 04/06/2017 12:35 PM, Tom Lane wrote:
>> Joe Conway  writes:
>>> Any thoughts on whether 0001a and 0001b ought to be backpatched? I'm
>>> thinking not given the lack of past complaints but it might make sense
>>> to do.
>>
>> I think 0001a absolutely needs to be, because it is fixing what is really
>> an ABI violation: sepgsql_needs_fmgr_hook is supposed to return our notion
>> of bool, but as things stand it's returning _Bool (which is why the
>> compiler is complaining).  Now we might get away with that on most
>> hardware, but on platforms where those are different widths, it's possible
>> to imagine function-return conventions that would make it fail.
>>
>> 0001b seems to only be needed for compilers that aren't smart enough
>> to see that tclass won't be referenced for RELKIND_INDEX, so it's
>> just cosmetic.
>
> Ok, committed/pushed that way.
>
> I found some missing bits in the 0002 patch -- new version attached.
> Will wait on new regression tests before committing, but I expect we'll
> have those by end of today and be able to commit the rest tomorrow.

Attached are the regression test updates for partitioned tables.

Thanks,
-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com


-- 
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] [BUGS] BUG #14682: row level security not work with partitioned table

2017-06-06 Thread Mike Palmiotto 
On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas  wrote:
> On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway  wrote:
>> Unless Robert objects, I'll work with Mike to get a fix posted and
>> committed in the next day or two.
>
> That would be great.  Thanks.

I have the updated patch with rowsecurity regression tests and rebased
on master. I've run these and verified locally by feeding
rowsecurity.sql to psql, but have yet to get the full regression suite
passing -- it's failing on the constraints regtest and then gets stuck
in recovery. Undoubtedly something to do with my
configuration/environment over here. I'm working through those issues
right now. In the meantime, if you want to see the regression tests as
they stand, please see the attached patch.

Thanks,

-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com
From 48a9586881872d4b8c9ca77e0c0da48db611e326 Mon Sep 17 00:00:00 2001
From: Mike Palmiotto 
Date: Wed, 24 May 2017 16:54:49 +
Subject: [PATCH] Add RLS support to partitioned tables

This is needed to get RLS policies to apply to the parent partitioned table.
Without this change partitioned tables are skipped.
---
 src/backend/rewrite/rewriteHandler.c |   3 +-
 src/test/regress/sql/rowsecurity.sql | 223 +++
 2 files changed, 225 insertions(+), 1 deletion(-)

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 35ff8bb..6cd73c1 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1835,7 +1835,8 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
 
 		/* Only normal relations can have RLS policies */
 		if (rte->rtekind != RTE_RELATION ||
-			rte->relkind != RELKIND_RELATION)
+			(rte->relkind != RELKIND_RELATION &&
+			rte->relkind != RELKIND_PARTITIONED_TABLE))
 			continue;
 
 		rel = heap_open(rte->relid, NoLock);
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index 1b6896e..c4ba136 100644
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -44,6 +44,7 @@ GRANT EXECUTE ON FUNCTION f_leak(text) TO public;
 -- BASIC Row-Level Security Scenario
 
 SET SESSION AUTHORIZATION regress_rls_alice;
+
 CREATE TABLE uaccount (
 pguser  name primary key,
 seclv   int
@@ -308,6 +309,228 @@ SET row_security TO OFF;
 SELECT * FROM t1 WHERE f_leak(b);
 EXPLAIN (COSTS OFF) SELECT * FROM t1 WHERE f_leak(b);
 
+--
+-- Partitioned Tables
+--
+
+SET SESSION AUTHORIZATION regress_rls_alice;
+
+CREATE TABLE part_category (
+cidint primary key,
+cname  text
+);
+GRANT ALL ON part_category TO public;
+INSERT INTO part_category VALUES
+(11, 'fiction'),
+(55, 'satire'),
+(99, 'nonfiction');
+
+CREATE TABLE part_document (
+did int,
+cid int,
+dlevel  int not null,
+dauthor name,
+dtitle  text
+) PARTITION BY RANGE (cid);
+GRANT ALL ON part_document TO public;
+
+-- Create partitions for document categories
+CREATE TABLE part_document_fiction (
+	LIKE part_document INCLUDING ALL
+) PARTITION BY RANGE (dlevel);
+
+CREATE TABLE part_document_satire (
+	LIKE part_document INCLUDING ALL
+) PARTITION BY RANGE (dlevel);
+
+CREATE TABLE part_document_nonfiction (
+	LIKE part_document INCLUDING ALL
+) PARTITION BY RANGE (dlevel);
+
+ALTER TABLE part_document ATTACH PARTITION part_document_fiction FOR VALUES FROM ('11') TO ('12');
+ALTER TABLE part_document ATTACH PARTITION part_document_satire FOR VALUES FROM ('55') TO ('56');
+ALTER TABLE part_document ATTACH PARTITION part_document_nonfiction FOR VALUES FROM ('99') TO ('100');
+
+-- Create partitions for document levels
+CREATE TABLE part_document_fiction_1 (LIKE part_document_fiction INCLUDING ALL);
+CREATE TABLE part_document_fiction_2 (LIKE part_document_fiction INCLUDING ALL);
+CREATE TABLE part_document_satire_1 (LIKE part_document_satire INCLUDING ALL);
+CREATE TABLE part_document_satire_2 (LIKE part_document_satire INCLUDING ALL);
+CREATE TABLE part_document_nonfiction_1 (LIKE part_document_nonfiction INCLUDING ALL);
+CREATE TABLE part_document_nonfiction_2 (LIKE part_document_nonfiction INCLUDING ALL);
+
+GRANT ALL ON part_document_fiction_1 TO public;
+GRANT ALL ON part_document_fiction_2 TO public;
+GRANT ALL ON part_document_satire_1 TO public;
+GRANT ALL ON part_document_satire_2 TO public;
+GRANT ALL ON part_document_nonfiction_1 TO public;
+GRANT ALL ON part_document_nonfiction_2 TO public;
+
+ALTER TABLE part_document_fiction ATTACH PARTITION part_document_fiction_1 FOR VALUES FROM ('1') TO ('2');
+ALTER TABLE part_document_fiction ATTACH PARTITION part_document_fiction_2 FOR VALUES FROM ('2') TO ('3');
+ALTER TABLE part_document_sat

Re: [HACKERS] [BUGS] BUG #14682: row level security not work with partitioned table

2017-06-06 Thread Mike Palmiotto 
On Tue, Jun 6, 2017 at 4:07 PM, Joe Conway  wrote:
> On 06/06/2017 11:57 AM, Mike Palmiotto wrote:
>> On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas  wrote:
>>> On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway  wrote:
>>>> Unless Robert objects, I'll work with Mike to get a fix posted and
>>>> committed in the next day or two.
>>>
>>> That would be great.  Thanks.
>>
>> I have the updated patch with rowsecurity regression tests and rebased
>> on master. I've run these and verified locally by feeding
>> rowsecurity.sql to psql, but have yet to get the full regression suite
>> passing -- it's failing on the constraints regtest and then gets stuck
>> in recovery. Undoubtedly something to do with my
>> configuration/environment over here. I'm working through those issues
>> right now. In the meantime, if you want to see the regression tests as
>> they stand, please see the attached patch.
>
> The constraints test passes here, so presumably something you borked
> locally. I only see a rowsecurity failure, which is not surprising since
> your patch does not include the changes to expected output ;-)
> Please resend with src/test/regress/expected/rowsecurity.out included.

It was indeed an issue on my end. Attached are the rowsecurity
regression tests and the expected out. Unsurprisingly, all tests pass,
because I said so. :)

Let me know if you want me to make any revisions.

Thanks,

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com
From 08432d93ed753a1e5cd4585ccf00e900abbd685f Mon Sep 17 00:00:00 2001
From: Mike Palmiotto 
Date: Wed, 24 May 2017 16:54:49 +
Subject: [PATCH] Add RLS support to partitioned tables

This is needed to get RLS policies to apply to the parent partitioned table.
Without this change partitioned tables are skipped.
---
 src/backend/rewrite/rewriteHandler.c  |   3 +-
 src/test/regress/expected/rowsecurity.out | 812 ++
 src/test/regress/sql/rowsecurity.sql  | 222 
 3 files changed, 1036 insertions(+), 1 deletion(-)

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 35ff8bb..6cd73c1 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1835,7 +1835,8 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
 
 		/* Only normal relations can have RLS policies */
 		if (rte->rtekind != RTE_RELATION ||
-			rte->relkind != RELKIND_RELATION)
+			(rte->relkind != RELKIND_RELATION &&
+			rte->relkind != RELKIND_PARTITIONED_TABLE))
 			continue;
 
 		rel = heap_open(rte->relid, NoLock);
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 7bf2936..1e35498 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -899,6 +899,818 @@ EXPLAIN (COSTS OFF) SELECT * FROM t1 WHERE f_leak(b);
  Filter: f_leak(b)
 (7 rows)
 
+--
+-- Partitioned Tables
+--
+SET SESSION AUTHORIZATION regress_rls_alice;
+CREATE TABLE part_category (
+cidint primary key,
+cname  text
+);
+GRANT ALL ON part_category TO public;
+INSERT INTO part_category VALUES
+(11, 'fiction'),
+(55, 'satire'),
+(99, 'nonfiction');
+CREATE TABLE part_document (
+did int,
+cid int,
+dlevel  int not null,
+dauthor name,
+dtitle  text
+) PARTITION BY RANGE (cid);
+GRANT ALL ON part_document TO public;
+-- Create partitions for document categories
+CREATE TABLE part_document_fiction (
+	LIKE part_document INCLUDING ALL
+) PARTITION BY RANGE (dlevel);
+CREATE TABLE part_document_satire (
+	LIKE part_document INCLUDING ALL
+) PARTITION BY RANGE (dlevel);
+CREATE TABLE part_document_nonfiction (
+	LIKE part_document INCLUDING ALL
+) PARTITION BY RANGE (dlevel);
+ALTER TABLE part_document ATTACH PARTITION part_document_fiction FOR VALUES FROM ('11') TO ('12');
+ALTER TABLE part_document ATTACH PARTITION part_document_satire FOR VALUES FROM ('55') TO ('56');
+ALTER TABLE part_document ATTACH PARTITION part_document_nonfiction FOR VALUES FROM ('99') TO ('100');
+-- Create partitions for document levels
+CREATE TABLE part_document_fiction_1 (LIKE part_document_fiction INCLUDING ALL);
+CREATE TABLE part_document_fiction_2 (LIKE part_document_fiction INCLUDING ALL);
+CREATE TABLE part_document_satire_1 (LIKE part_document_satire INCLUDING ALL);
+CREATE TABLE part_document_satire_2 (LIKE part_document_satire INCLUDING ALL);
+CREATE TABLE part_document_nonfiction_1 (LIKE part_document_nonfiction INCLUDING ALL);
+CREATE TABLE part_document_nonfiction_2 (LIKE part_document_nonfiction INCLUDING ALL);
+GRANT ALL ON part_document_fiction_1 TO p

Re: [HACKERS] [BUGS] BUG #14682: row level security not work with partitioned table

2017-06-07 Thread Mike Palmiotto 
On Tue, Jun 6, 2017 at 9:12 PM, Michael Paquier
 wrote:
> On Wed, Jun 7, 2017 at 9:52 AM, Joe Conway  wrote:
>> Thanks Mike. I'll take a close look to verify output correctnes, but I
>> am concerned that the new tests are unnecessarily complex. Any other
>> opinions on that?
>
> Some tests would be good to have. Now, if I read those regression
> tests correctly, this is using 10 relations where two would be enough,
> one as the parent relation and one as a partition. Then policies apply
> on the parent relation. The same kind of policy is defined 4 times,
> and there is bloat with GRANT and ALTER TABLE commands.

I ended up narrowing it down to 4 tables (one parent and 3 partitions)
in order to demonstrate policy sorting and order of RLS/partition
constraint checking. It should be much more straight-forward now, but
let me know if there are any further recommended changes.

One thing that concerns me is the first EXPLAIN plan from regress_rls_dave:
+EXPLAIN (COSTS OFF) SELECT * FROM part_document WHERE f_leak(dtitle);
+ QUERY PLAN
+
+ Append
+   InitPlan 1 (returns $0)
+ ->  Index Scan using uaccount_pkey on uaccount
+   Index Cond: (pguser = CURRENT_USER)
+   ->  Seq Scan on part_document_fiction
+ Filter: ((cid <> 55) AND (cid <> 55) AND (cid < 99) AND
(dlevel <= $0) AND f_leak(dtitle))
+   ->  Seq Scan on part_document_satire
+ Filter: ((cid <> 55) AND (cid <> 55) AND (cid < 99) AND
(dlevel <= $0) AND f_leak(dtitle))
+(8 rows)

I would expect that both part_document_satire (cid == 55) and
part_document_nonfiction (cid == 99) would be excluded from the
explain, but only cid < 99 seems to work. Interestingly, when I change
policy pp1r to cid < 55, I see the following:

+EXPLAIN (COSTS OFF) SELECT * FROM part_document WHERE f_leak(dtitle);
+QUERY PLAN
+---
+ Append
+   InitPlan 1 (returns $0)
+ ->  Index Scan using uaccount_pkey on uaccount
+   Index Cond: (pguser = CURRENT_USER)
+   ->  Seq Scan on part_document_fiction
+ Filter: ((cid < 55) AND (cid <> 55) AND (cid < 99) AND
(dlevel <= $0) AND f_leak(dtitle))
+(6 rows)

Is this a demonstration of a non-immutable function backing the
operator and thus not being able to filter it from the planner, or is
it a bug?

>
> +SELECT * FROM part_document;
> + did | cid | dlevel |  dauthor  | dtitle
> +-+-++---+-
> +   1 |  11 |  1 | regress_rls_bob   | my first novel
> Adding an "ORDER BY did" as well here would make the test output more
> predictable.

Done.

Thanks,
-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com
From 8c55045bd2d856fe4707582de0270c26d3a4c285 Mon Sep 17 00:00:00 2001
From: Mike Palmiotto 
Date: Wed, 24 May 2017 16:54:49 +
Subject: [PATCH] Add RLS support to partitioned tables

This is needed to get RLS policies to apply to the parent partitioned table.
Without this change partitioned tables are skipped.
---
 src/backend/rewrite/rewriteHandler.c  |   3 +-
 src/test/regress/expected/rowsecurity.out | 425 ++
 src/test/regress/sql/rowsecurity.sql  | 148 +++
 3 files changed, 575 insertions(+), 1 deletion(-)

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 35ff8bb..6cd73c1 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1835,7 +1835,8 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
 
 		/* Only normal relations can have RLS policies */
 		if (rte->rtekind != RTE_RELATION ||
-			rte->relkind != RELKIND_RELATION)
+			(rte->relkind != RELKIND_RELATION &&
+			rte->relkind != RELKIND_PARTITIONED_TABLE))
 			continue;
 
 		rel = heap_open(rte->relid, NoLock);
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 7bf2936..792d24e 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -899,6 +899,431 @@ EXPLAIN (COSTS OFF) SELECT * FROM t1 WHERE f_leak(b);
  Filter: f_leak(b)
 (7 rows)
 
+--
+-- Partitioned Tables
+--
+SET SESSION AUTHORIZATION regress_rls_alice;
+CREATE TABLE part_document (
+did int,
+cid int,
+dlevel  int not null,
+dauthor name,
+dtitle  text
+) PARTITION BY RANGE (cid);
+GRANT ALL ON part_document TO public;
+-- Create partitions for document categories
+CREATE TABLE part_document_fiction

Re: [HACKERS] [BUGS] BUG #14682: row level security not work with partitioned table

2017-06-07 Thread Mike Palmiotto 
On Wed, Jun 7, 2017 at 9:49 AM, Mike Palmiotto
 wrote:
> One thing that concerns me is the first EXPLAIN plan from regress_rls_dave:
> +EXPLAIN (COSTS OFF) SELECT * FROM part_document WHERE f_leak(dtitle);
> + QUERY PLAN
> +
> + Append
> +   InitPlan 1 (returns $0)
> + ->  Index Scan using uaccount_pkey on uaccount
> +   Index Cond: (pguser = CURRENT_USER)
> +   ->  Seq Scan on part_document_fiction
> + Filter: ((cid <> 55) AND (cid <> 55) AND (cid < 99) AND
> (dlevel <= $0) AND f_leak(dtitle))
> +   ->  Seq Scan on part_document_satire
> + Filter: ((cid <> 55) AND (cid <> 55) AND (cid < 99) AND
> (dlevel <= $0) AND f_leak(dtitle))
> +(8 rows)
>
> I would expect that both part_document_satire (cid == 55) and
> part_document_nonfiction (cid == 99) would be excluded from the
> explain, but only cid < 99 seems to work. Interestingly, when I change
> policy pp1r to cid < 55, I see the following:
>
> +EXPLAIN (COSTS OFF) SELECT * FROM part_document WHERE f_leak(dtitle);
> +QUERY PLAN
> +---
> + Append
> +   InitPlan 1 (returns $0)
> + ->  Index Scan using uaccount_pkey on uaccount
> +   Index Cond: (pguser = CURRENT_USER)
> +   ->  Seq Scan on part_document_fiction
> + Filter: ((cid < 55) AND (cid <> 55) AND (cid < 99) AND
> (dlevel <= $0) AND f_leak(dtitle))
> +(6 rows)
>
> Is this a demonstration of a non-immutable function backing the
> operator and thus not being able to filter it from the planner, or is
> it a bug?

Assuming my digging is correct, there's some other explanation for
this not working as expected...
postgres=> select po.oprname, pp.proname, pp.provolatile from pg_proc
pp join pg_operator po on pp.proname::text = po.oprcode::text where
po.oprname = '<>' and pp.proname like 'int%ne';
 oprname |   proname   | provolatile
-+-+-
 <>  | int4ne  | i
 <>  | int2ne  | i
 <>  | int24ne | i
 <>  | int42ne | i
 <>  | int8ne  | i
 <>  | int84ne | i
 <>  | int48ne | i
 <>  | interval_ne | i
 <>  | int28ne | i
 <>  | int82ne | i
(10 rows)

Thoughts?

Thanks,
-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com


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