Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/09/2017 04:14 PM, Tom Lane wrote:
> Joe Conway  writes:
>>> I turned on the buildfarm "keep" setting and looked at the diffs. The
>>> issue is that in there are a few places that do "SELECT ... FROM
>>> pg_seclabels ... ORDER BY ..." and when manually testing I get default
>>> database encoding "UTF8" but with the buildfarm I get "SQL_ASCII", hence
>>> a different sorting.
> 
>> Looks like adding COLLATE "C" in the ORDER BY clauses does the trick. Is
>> that the preferred method for this kind of issue?
> 
> Yeah, that's pretty much the standard fix nowadays.


Good to hear, thanks -- rhino is back to green.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Tom Lane]

Joe Conway  writes:
>> I turned on the buildfarm "keep" setting and looked at the diffs. The
>> issue is that in there are a few places that do "SELECT ... FROM
>> pg_seclabels ... ORDER BY ..." and when manually testing I get default
>> database encoding "UTF8" but with the buildfarm I get "SQL_ASCII", hence
>> a different sorting.

> Looks like adding COLLATE "C" in the ORDER BY clauses does the trick. Is
> that the preferred method for this kind of issue?

Yeah, that's pretty much the standard fix nowadays.

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

[Joe Conway]

On 04/09/2017 03:01 PM, Joe Conway wrote:
> On 04/09/2017 02:49 PM, Andres Freund wrote:
>> On 2017-04-09 14:28:48 -0700, Joe Conway wrote:
>>> Interesting -- rhino is now failing. I tested a minute ago manually on
>>> the same buildfarm animal and it passed. I'm on it.
>> 
>> The module for segpsql really needs to be improved so it logs
>> regression.diffs - iirc several other modules do.
> 
> Sure, but for another day I think.
> 
> I turned on the buildfarm "keep" setting and looked at the diffs. The
> issue is that in there are a few places that do "SELECT ... FROM
> pg_seclabels ... ORDER BY ..." and when manually testing I get default
> database encoding "UTF8" but with the buildfarm I get "SQL_ASCII", hence
> a different sorting.

Looks like adding COLLATE "C" in the ORDER BY clauses does the trick. Is
that the preferred method for this kind of issue?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/09/2017 02:49 PM, Andres Freund wrote:
> On 2017-04-09 14:28:48 -0700, Joe Conway wrote:
>> Interesting -- rhino is now failing. I tested a minute ago manually on
>> the same buildfarm animal and it passed. I'm on it.
> 
> The module for segpsql really needs to be improved so it logs
> regression.diffs - iirc several other modules do.

Sure, but for another day I think.

I turned on the buildfarm "keep" setting and looked at the diffs. The
issue is that in there are a few places that do "SELECT ... FROM
pg_seclabels ... ORDER BY ..." and when manually testing I get default
database encoding "UTF8" but with the buildfarm I get "SQL_ASCII", hence
a different sorting.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Andres Freund]

On 2017-04-09 14:28:48 -0700, Joe Conway wrote:
> Interesting -- rhino is now failing. I tested a minute ago manually on
> the same buildfarm animal and it passed. I'm on it.

The module for segpsql really needs to be improved so it logs
regression.diffs - iirc several other modules do.

Greetings,

Andres Freund


-- 
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

[Joe Conway]

On 04/09/2017 02:04 PM, Joe Conway wrote:
> On 04/08/2017 07:29 AM, Stephen Frost wrote:
>> * Joe Conway (m...@joeconway.com) wrote:
>>> On 04/07/2017 05:36 PM, Robert Haas wrote:
>>> > On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway  wrote:
>>> >> 1) commit the 0002 patch now before the feature freeze and follow up
>>> >>with the regression test patch when ready in a couple of days
>>> >> 2) hold off on both patches until ready
>>> >> 3) push both patches to the next commitfest/pg11
>>> >>
>>> >> Some argue this is an open issue against the new partitioning feature in
>>> >> pg10 and therefore should be addressed now, and others do not. I can see
>>> >> both sides of that argument.
>>> >>
>>> >> In any case, thoughts on what to do?
>>> > 
>>> > Speaking only for myself, I'm OK with any of those options, provided
>>> > that that "a couple" means what my dictionary says it means.
>>> 
>>> Thanks. I'd prefer not to do #1 actually, and I think we can adhere to
>>> the dictionary meaning of "a couple" (i.e. by EOD Sunday). Assuming we
>>> are ready by Sunday I will push both together (#2) or else I will move
>>> them both together to the next CF (#3).
>> 
>> Sounds good to me.
> 
> Having heard no objections, committed and pushed as per option #2.

Interesting -- rhino is now failing. I tested a minute ago manually on
the same buildfarm animal and it passed. I'm on it.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/08/2017 07:29 AM, Stephen Frost wrote:
> * Joe Conway (m...@joeconway.com) wrote:
>> On 04/07/2017 05:36 PM, Robert Haas wrote:
>> > On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway  wrote:
>> >> 1) commit the 0002 patch now before the feature freeze and follow up
>> >>with the regression test patch when ready in a couple of days
>> >> 2) hold off on both patches until ready
>> >> 3) push both patches to the next commitfest/pg11
>> >>
>> >> Some argue this is an open issue against the new partitioning feature in
>> >> pg10 and therefore should be addressed now, and others do not. I can see
>> >> both sides of that argument.
>> >>
>> >> In any case, thoughts on what to do?
>> > 
>> > Speaking only for myself, I'm OK with any of those options, provided
>> > that that "a couple" means what my dictionary says it means.
>> 
>> Thanks. I'd prefer not to do #1 actually, and I think we can adhere to
>> the dictionary meaning of "a couple" (i.e. by EOD Sunday). Assuming we
>> are ready by Sunday I will push both together (#2) or else I will move
>> them both together to the next CF (#3).
> 
> Sounds good to me.

Having heard no objections, committed and pushed as per option #2.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Stephen Frost]

Joe,

* Joe Conway (m...@joeconway.com) wrote:
> On 04/07/2017 05:36 PM, Robert Haas wrote:
> > On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway  wrote:
> >> 1) commit the 0002 patch now before the feature freeze and follow up
> >>with the regression test patch when ready in a couple of days
> >> 2) hold off on both patches until ready
> >> 3) push both patches to the next commitfest/pg11
> >>
> >> Some argue this is an open issue against the new partitioning feature in
> >> pg10 and therefore should be addressed now, and others do not. I can see
> >> both sides of that argument.
> >>
> >> In any case, thoughts on what to do?
> > 
> > Speaking only for myself, I'm OK with any of those options, provided
> > that that "a couple" means what my dictionary says it means.
> 
> Thanks. I'd prefer not to do #1 actually, and I think we can adhere to
> the dictionary meaning of "a couple" (i.e. by EOD Sunday). Assuming we
> are ready by Sunday I will push both together (#2) or else I will move
> them both together to the next CF (#3).

Sounds good to me.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/07/2017 05:36 PM, Robert Haas wrote:
> On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway  wrote:
>> 1) commit the 0002 patch now before the feature freeze and follow up
>>with the regression test patch when ready in a couple of days
>> 2) hold off on both patches until ready
>> 3) push both patches to the next commitfest/pg11
>>
>> Some argue this is an open issue against the new partitioning feature in
>> pg10 and therefore should be addressed now, and others do not. I can see
>> both sides of that argument.
>>
>> In any case, thoughts on what to do?
> 
> Speaking only for myself, I'm OK with any of those options, provided
> that that "a couple" means what my dictionary says it means.

Thanks. I'd prefer not to do #1 actually, and I think we can adhere to
the dictionary meaning of "a couple" (i.e. by EOD Sunday). Assuming we
are ready by Sunday I will push both together (#2) or else I will move
them both together to the next CF (#3).

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Robert Haas]

On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway  wrote:
> On 04/07/2017 11:37 AM, Mike Palmiotto wrote:
 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.
>>
>> Actually attached this time.
>
> Based on my review and testing of the 0002 patch I believe it is
> correct. However Mike and I just went through the regression test patch
> line by line and there are issues he needs to address -- there is no way
> that is happening by tonight as the output is very verbose and we need
> to be sure we are both testing the correct things and getting the
> correct behaviors.
>
> Based on that I can:
>
> 1) commit the 0002 patch now before the feature freeze and follow up
>with the regression test patch when ready in a couple of days
> 2) hold off on both patches until ready
> 3) push both patches to the next commitfest/pg11
>
> Some argue this is an open issue against the new partitioning feature in
> pg10 and therefore should be addressed now, and others do not. I can see
> both sides of that argument.
>
> In any case, thoughts on what to do?

Speaking only for myself, I'm OK with any of those options, provided
that that "a couple" means what my dictionary says it means.

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

[Joe Conway]

On 04/07/2017 11:37 AM, Mike Palmiotto wrote:
>>> 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.
> 
> Actually attached this time.

Based on my review and testing of the 0002 patch I believe it is
correct. However Mike and I just went through the regression test patch
line by line and there are issues he needs to address -- there is no way
that is happening by tonight as the output is very verbose and we need
to be sure we are both testing the correct things and getting the
correct behaviors.

Based on that I can:

1) commit the 0002 patch now before the feature freeze and follow up
   with the regression test patch when ready in a couple of days
2) hold off on both patches until ready
3) push both patches to the next commitfest/pg11

Some argue this is an open issue against the new partitioning feature in
pg10 and therefore should be addressed now, and others do not. I can see
both sides of that argument.

In any case, thoughts on what to do?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

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

[Joe Conway]

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.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Add partitioned table support to sepgsql

The new partitioned table capability added a new relkind, namely
RELKIND_PARTITIONED_TABLE. Update sepgsql to treat this new relkind
exactly the same way it does RELKIND_RELATION.

Issue raised by Stephen Frost and initial patch by Mike Palmiotto.
Review by Tom Lane and Robert Haas, and editorializing by me.

Discussion: https://postgr.es/m/flat/623bcaae-112e-ced0-8c22-a84f75ae0c53%40joeconway.com

diff --git a/contrib/sepgsql/dml.c b/contrib/sepgsql/dml.c
index bc17089..b643720 100644
*** a/contrib/sepgsql/dml.c
--- b/contrib/sepgsql/dml.c
*** check_relation_privileges(Oid relOid,
*** 190,195 
--- 190,196 
  	switch (relkind)
  	{
  		case RELKIND_RELATION:
+ 		case RELKIND_PARTITIONED_TABLE:
  			result = sepgsql_avc_check_perms(,
  			 SEPG_CLASS_DB_TABLE,
  			 required,
*** check_relation_privileges(Oid relOid,
*** 225,231 
  	/*
  	 * Only columns owned by relations shall be checked
  	 */
! 	if (relkind != RELKIND_RELATION)
  		return true;
  
  	/*
--- 226,232 
  	/*
  	 * Only columns owned by relations shall be checked
  	 */
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		return true;
  
  	/*
diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 1a8f884..6239800 100644
*** a/contrib/sepgsql/label.c
--- b/contrib/sepgsql/label.c
*** exec_object_restorecon(struct selabel_ha
*** 779,785 
  			case RelationRelationId:
  relForm = (Form_pg_class) GETSTRUCT(tuple);
  
! if (relForm->relkind == RELKIND_RELATION)
  	objtype = SELABEL_DB_TABLE;
  else if (relForm->relkind == RELKIND_SEQUENCE)
  	objtype = SELABEL_DB_SEQUENCE;
--- 787,794 
  			case RelationRelationId:
  relForm = (Form_pg_class) GETSTRUCT(tuple);
  
! if (relForm->relkind == RELKIND_RELATION ||
! 	relForm->relkind == RELKIND_PARTITIONED_TABLE)
  	objtype = SELABEL_DB_TABLE;
  else if (relForm->relkind == RELKIND_SEQUENCE)
  	objtype = SELABEL_DB_SEQUENCE;
*** exec_object_restorecon(struct selabel_ha
*** 803,809 
  			case AttributeRelationId:
  attForm = (Form_pg_attribute) GETSTRUCT(tuple);
  
! if (get_rel_relkind(attForm->attrelid) != RELKIND_RELATION)
  	continue;	/* no need to assign security label */
  
  objtype = SELABEL_DB_COLUMN;
--- 812,819 
  			case AttributeRelationId:
  attForm = (Form_pg_attribute) GETSTRUCT(tuple);
  
! if (get_rel_relkind(attForm->attrelid) != RELKIND_RELATION &&
! 	get_rel_relkind(attForm->attrelid) != RELKIND_PARTITIONED_TABLE)
  	continue;	/* no need to assign security label */
  
  objtype = SELABEL_DB_COLUMN;
diff --git a/contrib/sepgsql/relation.c b/contrib/sepgsql/relation.c
index ab98a9b..59a6d9b 100644
*** a/contrib/sepgsql/relation.c
--- b/contrib/sepgsql/relation.c
*** sepgsql_attribute_post_create(Oid relOid
*** 54,65 
  	ObjectAddress object;
  	Form_pg_attribute attForm;
  	StringInfoData audit_name;
  
  	/*
! 	 * Only attributes within regular relation have individual security
! 	 * labels.
  	 */
! 	if (get_rel_relkind(relOid) != RELKIND_RELATION)
  		return;
  
  	/*
--- 54,66 
  	ObjectAddress object;
  	Form_pg_attribute attForm;
  	StringInfoData audit_name;
+ 	char		relkind = get_rel_relkind(relOid);
  
  	/*
! 	 * Only attributes within regular relations or partition relations have
! 	 * individual security labels.
  	 */
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		return;
  
  	/*
*** sepgsql_attribute_drop(Oid relOid, AttrN
*** 135,142 
  {
  	ObjectAddress object;
  	char	   *audit_name;
  
! 	if (get_rel_relkind(relOid) != RELKIND_RELATION)
  		return;
 

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Tom Lane]

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.

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

[Joe Conway]

On 04/06/2017 08:29 AM, Tom Lane wrote:
> Joe Conway  writes:
>> I'm going to push the attached in a few hours unless there is any
>> additional discussion. As stated above we'll do the regression changes
>> in a separate patch once that is sorted. I used Tom's approach and
>> comment wording for 0001a.
> 
> Looks generally sane, but I noticed a grammatical nitpick:
> 
> -  * Only attributes within regular relation or partition relations have
> +  * Only attributes within regular relations or partition relations have

Good call -- thanks!

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.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Tom Lane]

Joe Conway  writes:
> I'm going to push the attached in a few hours unless there is any
> additional discussion. As stated above we'll do the regression changes
> in a separate patch once that is sorted. I used Tom's approach and
> comment wording for 0001a.

Looks generally sane, but I noticed a grammatical nitpick:

-* Only attributes within regular relation or partition relations have
+* Only attributes within regular relations or partition relations have

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

[Joe Conway]

On 04/05/2017 02:29 PM, Mike Palmiotto wrote:
> 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.

I'm going to push the attached in a few hours unless there is any
additional discussion. As stated above we'll do the regression changes
in a separate patch once that is sorted. I used Tom's approach and
comment wording for 0001a.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 1a8f884..5e2eba6 100644
*** a/contrib/sepgsql/label.c
--- b/contrib/sepgsql/label.c
***
*** 10,15 
--- 10,25 
   */
  #include "postgres.h"
  
+ #include 
+ 
+ /*
+  *  includes , which creates an incompatible
+  * #define for bool.  Get rid of that so we can use our own typedef.
+  * (We don't care if  redefines "true"/"false"; those are close
+  * enough.)
+  */
+ #undef bool
+ 
  #include "access/heapam.h"
  #include "access/htup_details.h"
  #include "access/genam.h"
***
*** 37,44 
  
  #include "sepgsql.h"
  
- #include 
- 
  /*
   * Saved hook entries (if stacked)
   */
--- 47,52 
diff --git a/contrib/sepgsql/relation.c b/contrib/sepgsql/relation.c
index ab98a9b..2ea6bfb 100644
*** a/contrib/sepgsql/relation.c
--- b/contrib/sepgsql/relation.c
*** sepgsql_relation_post_create(Oid relOid)
*** 243,249 
  	HeapTuple	tuple;
  	Form_pg_class classForm;
  	ObjectAddress object;
! 	uint16		tclass;
  	char	   *scontext;		/* subject */
  	char	   *tcontext;		/* schema */
  	char	   *rcontext;		/* relation */
--- 243,249 
  	HeapTuple	tuple;
  	Form_pg_class classForm;
  	ObjectAddress object;
! 	uint16_t	tclass;
  	char	   *scontext;		/* subject */
  	char	   *tcontext;		/* schema */
  	char	   *rcontext;		/* relation */
*** sepgsql_relation_drop(Oid relOid)
*** 413,419 
  {
  	ObjectAddress object;
  	char	   *audit_name;
! 	uint16_t	tclass;
  	char		relkind;
  
  	relkind = get_rel_relkind(relOid);
--- 413,419 
  {
  	ObjectAddress object;
  	char	   *audit_name;
! 	uint16_t	tclass = 0;
  	char		relkind;
  
  	relkind = get_rel_relkind(relOid);
diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 1a8f884..4dda82a 100644
*** a/contrib/sepgsql/label.c
--- b/contrib/sepgsql/label.c
*** exec_object_restorecon(struct selabel_ha
*** 779,785 
  			case RelationRelationId:
  relForm = (Form_pg_class) GETSTRUCT(tuple);
  
! if (relForm->relkind == RELKIND_RELATION)
  	objtype = SELABEL_DB_TABLE;
  else if (relForm->relkind == RELKIND_SEQUENCE)
  	objtype = SELABEL_DB_SEQUENCE;
--- 779,786 
  			case RelationRelationId:
  relForm = (Form_pg_class) GETSTRUCT(tuple);
  
! 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 ab98a9b..f8689c0 100644
*** a/contrib/sepgsql/relation.c
--- b/contrib/sepgsql/relation.c
*** sepgsql_attribute_post_create(Oid relOid
*** 54,65 
  	ObjectAddress object;
  	Form_pg_attribute attForm;
  	StringInfoData audit_name;
  
  	/*
! 	 * Only attributes within regular relation have individual security
! 	 * labels.
  	 */
! 	if (get_rel_relkind(relOid) != RELKIND_RELATION)
  		return;
  
  	/*
--- 54,66 
  	ObjectAddress object;
  	Form_pg_attribute attForm;
  	StringInfoData audit_name;
+ 	char		relkind = get_rel_relkind(relOid);
  
  	/*
! 	 * Only attributes within regular relation or partition relations have
! 	 * individual security labels.
  	 */
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		return;
  
  	/*
*** sepgsql_attribute_drop(Oid relOid, AttrN
*** 135,142 
  {
  	ObjectAddress object;
  	char	   *audit_name;
  
! 	if (get_rel_relkind(relOid) != RELKIND_RELATION)
  		return;
  
  	/*
--- 136,144 
  {
  	ObjectAddress object;
  	char	   *audit_name;
+ 	char		relkind = get_rel_relkind(relOid);
  
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		return;
  
  	/*
*** sepgsql_attribute_relabel(Oid relOid, At
*** 167,174 
  {
  	ObjectAddress object;
  	char	   *audit_name;
  
! 	if (get_rel_relkind(relOid) != RELKIND_RELATION)
  		ereport(ERROR,
  (errcode(ERRCODE_WRONG_OBJECT_TYPE),
   errmsg("cannot set security label on non-regular columns")));
--- 169,177 
  {
  	ObjectAddress object;
  	char	   *audit_name;
+ 	char		relkind = get_rel_relkind(relOid);
  
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		ereport(ERROR,
  (errcode(ERRCODE_WRONG_OBJECT_TYPE),
  	

Re: [HACKERS] partitioned tables and contrib/sepgsql

[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(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 = 

Re: [HACKERS] partitioned tables and contrib/sepgsql

[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

[Tom Lane]

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.)".

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

[Peter Eisentraut]

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.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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

[Tom Lane]

Andres Freund  writes:
> GCC generally doesn't warn about macro redefinitions, if both
> definitions are equivalent.

But they're *not* equivalent.  c.h has

#define true((bool) 1)

whereas so far as I can see the  definition is just

#define true1

And if you put those two definitions together you will get a warning.
The lack of one with c.h +  means somebody is cheating.

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

[Tom Lane]

Peter Eisentraut  writes:
> On 4/5/17 01:20, Tom Lane wrote:
>> $ cat test.c
>> typedef char bool;
>> typedef char bool;
>> $ gcc -c test.c
>> test.c:2: error: redefinition of typedef 'bool'
>> test.c:1: note: previous declaration of 'bool' was here

> But the above is not how the current code looks.

No, but it is what the proposed 0001a patch would result in.

The experiments I just did prove that F25's gcc is cheating like mad in
its implementation of "#include ".  I suspect that it's somehow
turning off the warning that you ought to get about redefining typedef
bool, as well, though I'm very unclear on how.  In any case we don't need
the extra typedef and should leave it out.

> So if you get stdbool.h first, then c.h does nothing.  If you get c.h
> first, then the macro from stdbool.h will mask the c.h typedef.

Right, and *either* of those is disastrous if sizeof(_Bool) isn't 1.

> Where this gets really fun is when you include other header files
> between, say, postgres.h and selinux/label.h, because then the function
> definitions from those header files will be incompatible before and
> after the stdbool.h inclusion.  Which indeed currently leads to the
> following compilation error:

The compilation complaint comes from the fact that we have

static bool
sepgsql_needs_fmgr_hook(Oid functionId)

appearing after the inclusion of , causing it to mean
something else than what a Postgres programmer would expect.
The other headers aren't at issue really.

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

[Andres Freund]

On April 5, 2017 9:04:00 AM PDT, Tom Lane  wrote:
>Joe Conway  writes:
>> On 04/04/2017 09:58 PM, Tom Lane wrote:
>>> Another issue is whether you won't get compiler complaints about
>>> redefinition of the "true" and "false" macros.  But those would
>>> likely only be warnings, not flat-out errors.
>
>> I have not been able to generate warnings or errors around "true" and
>> "false".
>
>Interesting.  Poking at it on a Fedora 25 machine, I also see a
>bool-type-related warning in sepgsql/label.c, but nothing around macro
>redefinitions.  In particular, I find that
>
>#include "postgres.h"
>
>#include 
>
>is completely silent.  On the other hand,
>
>#include "postgres.h"
>
>#define bool_Bool
>#define true1
>#define false   0
>
>generates the warnings I expected about "true" and "false" being
>redefined.  Which is damn odd, because I copied-and-pasted those
>lines out of
>/usr/lib/gcc/x86_64-redhat-linux/6.3.1/include/stdbool.h
>
>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.
>
>Anyway, I'd recommend that we do something like
>
> #include 
>+
>+/*
>+ *  includes , which creates an
>incompatible
>+ * #define for bool.  Get rid of that so we can use our own typedef.
>+ * (For obscure reasons, the "true" and "false" macros don't cause
>issues.)
>+ */

GCC generally doesn't warn about macro redefinitions, if both definitions are 
equivalent.  IIRC we have some examples of that in the tree.  There's a flag to 
warn regardless.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.


-- 
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

[Tom Lane]

Joe Conway  writes:
> On 04/04/2017 09:58 PM, Tom Lane wrote:
>> Another issue is whether you won't get compiler complaints about
>> redefinition of the "true" and "false" macros.  But those would
>> likely only be warnings, not flat-out errors.

> I have not been able to generate warnings or errors around "true" and
> "false".

Interesting.  Poking at it on a Fedora 25 machine, I also see a
bool-type-related warning in sepgsql/label.c, but nothing around macro
redefinitions.  In particular, I find that

#include "postgres.h"

#include 

is completely silent.  On the other hand,

#include "postgres.h"

#define bool_Bool
#define true1
#define false   0

generates the warnings I expected about "true" and "false" being
redefined.  Which is damn odd, because I copied-and-pasted those
lines out of
/usr/lib/gcc/x86_64-redhat-linux/6.3.1/include/stdbool.h

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.

Anyway, I'd recommend that we do something like

 #include 
+
+/*
+ *  includes , which creates an incompatible
+ * #define for bool.  Get rid of that so we can use our own typedef.
+ * (For obscure reasons, the "true" and "false" macros don't cause issues.)
+ */
+ #undef bool

and call it good.

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

[Peter Eisentraut]

On 4/5/17 01:20, Tom Lane wrote:
>> The complaint about bool is also just a warning.
> 
> Really?
> 
> $ cat test.c
> typedef char bool;
> typedef char bool;
> $ gcc -c test.c
> test.c:2: error: redefinition of typedef 'bool'
> test.c:1: note: previous declaration of 'bool' was here
> 
> This is with gcc 4.4.7.

But the above is not how the current code looks.

stdbool.h does

#define bool _Bool

c.h does

#ifndef bool
typedef char bool;
#endif

So if you get stdbool.h first, then c.h does nothing.  If you get c.h
first, then the macro from stdbool.h will mask the c.h typedef.

Where this gets really fun is when you include other header files
between, say, postgres.h and selinux/label.h, because then the function
definitions from those header files will be incompatible before and
after the stdbool.h inclusion.  Which indeed currently leads to the
following compilation error:

label.c: In function ‘sepgsql_init_client_label’:
label.c:437:18: error: assignment from incompatible pointer type
[-Werror=incompatible-pointer-types]
  needs_fmgr_hook = sepgsql_needs_fmgr_hook;
  ^

So I can't reproduce the original complaint, but this is potentially worse.

(The above is on Fedora 25.  You need a fairly new selinux.)

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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

[Joe Conway]

On 04/04/2017 09:58 PM, Tom Lane wrote:
> I doubt that works at all, TBH.  What I'd expect to happen with a
> typical compiler is a complaint about redefinition of typedef bool,
> because c.h already declared it and here this fragment is doing
> so again.  It'd make sense to me to do
> 
> + #ifdef bool
> + #undef bool
> + #endif
> 
> to get rid of the macro definition of bool that stdbool.h is
> supposed to provide.  But there should be no reason to declare
> our typedef a second time.

makes sense

> Another issue is whether you won't get compiler complaints about
> redefinition of the "true" and "false" macros.  But those would
> likely only be warnings, not flat-out errors.

I have not been able to generate warnings or errors around "true" and
"false".

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Tom Lane]

Andres Freund  writes:
> On 2017-04-05 10:45:06 -0400, Tom Lane wrote:
>> Andres Freund  writes:
>>> I wonder if there's any compiler that has _Bool/stdbool.h where it's not
>>> 1 byte sized. It's definitely not guaranteed by the standard.

>> Hm.  I'd supposed that it'd be pretty common to make _Bool be int-sized.

> I think nearly all x86 compilers use 1byte, but I assume there's some
> architectures where that'd be expensive.

[ pokes around... ]  prairiedog's gcc (Apple PPC) thinks sizeof(_Bool)
is 4.

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

[Tom Lane]

Andres Freund  writes:
> On 2017-04-05 10:48:27 -0400, Tom Lane wrote:
>> I'd really rather not.  It might be safe here, because this code
>> only works on Linux anyway, but it's still a dangerous precedent.

> Well, what's the alternative for v10?  There's already precedent btw.,
> cf plperl.h undefining bool.

Well, the 0001a patch amounted to undefining bool, once you drop
the redundant typedef redeclaration.  So that's a direction we have
some track record with.

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

[Andres Freund]

On 2017-04-05 10:48:27 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-03-31 20:23:39 -0400, Robert Haas wrote:
> >> 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.
> 
> > I think in this case it makes sense to deviate from that rule,
> > temporarily and locally.
> 
> I'd really rather not.  It might be safe here, because this code
> only works on Linux anyway, but it's still a dangerous precedent.

Well, what's the alternative for v10?  There's already precedent btw.,
cf plperl.h undefining bool.

Greetings,

Andres Freund


-- 
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

[Andres Freund]

On 2017-04-05 10:45:06 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-04-05 09:43:53 -0400, Tom Lane wrote:
> >> Yeah, I was just thinking about that.  The core problem though is that
> >> we need the "bool" fields in the system catalog structs (or anyplace
> >> else that it represents an on-disk bool datum) to be understood as
> >> being 1 byte wide. I do not think we can assume that that's true of
> >> every compiler's _Bool type.  So we'd need some workaround for that.
> >> There are probably other places such as isnull arrays where it'd be
> >> wise to force the width to be 1 byte.
> 
> > I wonder if there's any compiler that has _Bool/stdbool.h where it's not
> > 1 byte sized. It's definitely not guaranteed by the standard.
> 
> Hm.  I'd supposed that it'd be pretty common to make _Bool be int-sized.

I think nearly all x86 compilers use 1byte, but I assume there's some
architectures where that'd be expensive.


> If it is char-sized almost everywhere, we could create a policy of
> using stdbool.h unless configure sees that _Bool is not char-sized.
> OTOH, that doesn't improve our existing situation that we have
> platform-dependent semantics around "bool" (eg, what happens when
> a non-char-sized value is assigned).  It would just change which
> one is the majority case, and not in a very safe direction :-(

Yea, no, that doesn't like fun :(.


Might make sense to temporarily add a configure test checking for
_Bool/stdbool size, so we have some idea what we're talking about.


Greetings,

Andres Freund


-- 
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

[Tom Lane]

Andres Freund  writes:
> On 2017-03-31 20:23:39 -0400, Robert Haas wrote:
>> 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.

> I think in this case it makes sense to deviate from that rule,
> temporarily and locally.

I'd really rather not.  It might be safe here, because this code
only works on Linux anyway, but it's still a dangerous precedent.

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

[Tom Lane]

Andres Freund  writes:
> On 2017-04-05 09:43:53 -0400, Tom Lane wrote:
>> Yeah, I was just thinking about that.  The core problem though is that
>> we need the "bool" fields in the system catalog structs (or anyplace
>> else that it represents an on-disk bool datum) to be understood as
>> being 1 byte wide. I do not think we can assume that that's true of
>> every compiler's _Bool type.  So we'd need some workaround for that.
>> There are probably other places such as isnull arrays where it'd be
>> wise to force the width to be 1 byte.

> I wonder if there's any compiler that has _Bool/stdbool.h where it's not
> 1 byte sized. It's definitely not guaranteed by the standard.

Hm.  I'd supposed that it'd be pretty common to make _Bool be int-sized.

If it is char-sized almost everywhere, we could create a policy of
using stdbool.h unless configure sees that _Bool is not char-sized.
OTOH, that doesn't improve our existing situation that we have
platform-dependent semantics around "bool" (eg, what happens when
a non-char-sized value is assigned).  It would just change which
one is the majority case, and not in a very safe direction :-(

>> In any case, that's a research project that's not getting done for v10.

> Agreed.

Yeah, it's off-topic for right now.

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

[Andres Freund]

On 2017-03-31 20:23:39 -0400, 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.

I think in this case it makes sense to deviate from that rule,
temporarily and locally.

- Andres


-- 
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

[Andres Freund]

On 2017-04-05 09:43:53 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > I argued before that we should migrate to stdbool.h by default, because
> > it's only going to get more common.  We already do so in a way for c++
> > compilers...
> 
> Yeah, I was just thinking about that.  The core problem though is that
> we need the "bool" fields in the system catalog structs (or anyplace
> else that it represents an on-disk bool datum) to be understood as
> being 1 byte wide. I do not think we can assume that that's true of
> every compiler's _Bool type.  So we'd need some workaround for that.
> There are probably other places such as isnull arrays where it'd be
> wise to force the width to be 1 byte.

I wonder if there's any compiler that has _Bool/stdbool.h where it's not
1 byte sized. It's definitely not guaranteed by the standard.

Having a seperate booldatum type or such shouldn't be too bad, either
way.


> In any case, that's a research project that's not getting done for v10.

Agreed.


- Andres


-- 
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

[Robert Haas]

On Wed, Apr 5, 2017 at 12:58 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Apr 4, 2017 at 6:56 PM, Joe Conway  wrote:
>>> Any objections?
>
>> I'm guessing Tom's going to have a strong feeling about whether 0001a
>> is the right way to address the stdbool issue,
>
> I will?  [ looks ... ]  Yup, you're right.

It happens occasionally.  :-)

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

[Tom Lane]

Andres Freund  writes:
> I argued before that we should migrate to stdbool.h by default, because
> it's only going to get more common.  We already do so in a way for c++
> compilers...

Yeah, I was just thinking about that.  The core problem though is that
we need the "bool" fields in the system catalog structs (or anyplace
else that it represents an on-disk bool datum) to be understood as
being 1 byte wide.  I do not think we can assume that that's true of
every compiler's _Bool type.  So we'd need some workaround for that.
There are probably other places such as isnull arrays where it'd be
wise to force the width to be 1 byte.

In any case, that's a research project that's not getting done for v10.

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

[Andres Freund]

On 2017-04-05 00:58:07 -0400, Tom Lane wrote:
> Robert Haas  writes:
> > On Tue, Apr 4, 2017 at 6:56 PM, Joe Conway  wrote:
> >> Any objections?
> 
> > I'm guessing Tom's going to have a strong feeling about whether 0001a
> > is the right way to address the stdbool issue,
> 
> I will?  [ looks ... ]  Yup, you're right.
> 
> I doubt that works at all, TBH.  What I'd expect to happen with a
> typical compiler is a complaint about redefinition of typedef bool,
> because c.h already declared it and here this fragment is doing
> so again.  It'd make sense to me to do
> 
> + #ifdef bool
> + #undef bool
> + #endif
> 
> to get rid of the macro definition of bool that stdbool.h is
> supposed to provide.  But there should be no reason to declare
> our typedef a second time.

> Another issue is whether you won't get compiler complaints about
> redefinition of the "true" and "false" macros.  But those would
> likely only be warnings, not flat-out errors.

I argued before that we should migrate to stdbool.h by default, because
it's only going to get more common.  We already do so in a way for c++
compilers...

- Andres


-- 
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

[Tom Lane]

Peter Eisentraut  writes:
> On 4/5/17 00:58, Tom Lane wrote:
>> Another issue is whether you won't get compiler complaints about
>> redefinition of the "true" and "false" macros.  But those would
>> likely only be warnings, not flat-out errors.

> The complaint about bool is also just a warning.

Really?

$ cat test.c
typedef char bool;
typedef char bool;
$ gcc -c test.c
test.c:2: error: redefinition of typedef 'bool'
test.c:1: note: previous declaration of 'bool' was here

This is with gcc 4.4.7.

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

[Peter Eisentraut]

On 4/5/17 00:58, Tom Lane wrote:
> Another issue is whether you won't get compiler complaints about
> redefinition of the "true" and "false" macros.  But those would
> likely only be warnings, not flat-out errors.

The complaint about bool is also just a warning.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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

[Tom Lane]

Robert Haas  writes:
> On Tue, Apr 4, 2017 at 6:56 PM, Joe Conway  wrote:
>> Any objections?

> I'm guessing Tom's going to have a strong feeling about whether 0001a
> is the right way to address the stdbool issue,

I will?  [ looks ... ]  Yup, you're right.

I doubt that works at all, TBH.  What I'd expect to happen with a
typical compiler is a complaint about redefinition of typedef bool,
because c.h already declared it and here this fragment is doing
so again.  It'd make sense to me to do

+ #ifdef bool
+ #undef bool
+ #endif

to get rid of the macro definition of bool that stdbool.h is
supposed to provide.  But there should be no reason to declare
our typedef a second time.

Another issue is whether you won't get compiler complaints about
redefinition of the "true" and "false" macros.  But those would
likely only be warnings, not flat-out errors.

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

[Robert Haas]

On Tue, Apr 4, 2017 at 6:56 PM, Joe Conway  wrote:
> On 04/04/2017 10:02 AM, Joe Conway wrote:
>> On 04/04/2017 09:55 AM, Mike Palmiotto wrote:
>>> After some discussion off-list, I've rebased and udpated the patches.
>>> Please see attached for further review.
>>
>> Thanks -- will have another look and test on a machine with selinux
>> setup. Robert, did you want me to take responsibility to commit on this
>> or just provide review/feedback?
>
> I did some editorializing on these.
>
> In particular I did not like the approach to fixing "warning: ‘tclass’
> may be used uninitialized" and ended up just doing it the same as was
> done elsewhere in relation.c already (set tclass = 0 in the variable
> declaration). Along the way I also changed one instance of tclass from
> uint16 to uint16_t for the sake of consistency.
>
> Interestingly we figured out that the warning was present with -Og, but
> not present with -O0, -O2, or -O3.
>
> If you want to test, apply 0001a and 0001b before 0002.
>
> Any objections?

I'm guessing Tom's going to have a strong feeling about whether 0001a
is the right way to address the stdbool issue, but I don't personally
know what the right thing to do is so I will avoid opining on that
topic.

So, nope, no objections here to you committing those.

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

[Joe Conway]

On 04/04/2017 10:02 AM, Joe Conway wrote:
> On 04/04/2017 09:55 AM, Mike Palmiotto wrote:
>> After some discussion off-list, I've rebased and udpated the patches.
>> Please see attached for further review.
> 
> Thanks -- will have another look and test on a machine with selinux
> setup. Robert, did you want me to take responsibility to commit on this
> or just provide review/feedback?

I did some editorializing on these.

In particular I did not like the approach to fixing "warning: ‘tclass’
may be used uninitialized" and ended up just doing it the same as was
done elsewhere in relation.c already (set tclass = 0 in the variable
declaration). Along the way I also changed one instance of tclass from
uint16 to uint16_t for the sake of consistency.

Interestingly we figured out that the warning was present with -Og, but
not present with -O0, -O2, or -O3.

If you want to test, apply 0001a and 0001b before 0002.

Any objections?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 1a8f884..320c098 100644
*** a/contrib/sepgsql/label.c
--- b/contrib/sepgsql/label.c
***
*** 10,15 
--- 10,25 
   */
  #include "postgres.h"
  
+ #include 
+ /*
+  * selinux/label.h includes stdbool.h, which redefines bool, so
+  * revert to the postgres definition of bool from c.h
+  */
+ #ifdef bool
+ #undef bool
+ typedef char bool;
+ #endif
+ 
  #include "access/heapam.h"
  #include "access/htup_details.h"
  #include "access/genam.h"
***
*** 37,44 
  
  #include "sepgsql.h"
  
- #include 
- 
  /*
   * Saved hook entries (if stacked)
   */
--- 47,52 
diff --git a/contrib/sepgsql/relation.c b/contrib/sepgsql/relation.c
index ab98a9b..2ea6bfb 100644
*** a/contrib/sepgsql/relation.c
--- b/contrib/sepgsql/relation.c
*** sepgsql_relation_post_create(Oid relOid)
*** 243,249 
  	HeapTuple	tuple;
  	Form_pg_class classForm;
  	ObjectAddress object;
! 	uint16		tclass;
  	char	   *scontext;		/* subject */
  	char	   *tcontext;		/* schema */
  	char	   *rcontext;		/* relation */
--- 243,249 
  	HeapTuple	tuple;
  	Form_pg_class classForm;
  	ObjectAddress object;
! 	uint16_t	tclass;
  	char	   *scontext;		/* subject */
  	char	   *tcontext;		/* schema */
  	char	   *rcontext;		/* relation */
*** sepgsql_relation_drop(Oid relOid)
*** 413,419 
  {
  	ObjectAddress object;
  	char	   *audit_name;
! 	uint16_t	tclass;
  	char		relkind;
  
  	relkind = get_rel_relkind(relOid);
--- 413,419 
  {
  	ObjectAddress object;
  	char	   *audit_name;
! 	uint16_t	tclass = 0;
  	char		relkind;
  
  	relkind = get_rel_relkind(relOid);
diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 1a8f884..4dda82a 100644
*** a/contrib/sepgsql/label.c
--- b/contrib/sepgsql/label.c
*** exec_object_restorecon(struct selabel_ha
*** 779,785 
  			case RelationRelationId:
  relForm = (Form_pg_class) GETSTRUCT(tuple);
  
! if (relForm->relkind == RELKIND_RELATION)
  	objtype = SELABEL_DB_TABLE;
  else if (relForm->relkind == RELKIND_SEQUENCE)
  	objtype = SELABEL_DB_SEQUENCE;
--- 779,786 
  			case RelationRelationId:
  relForm = (Form_pg_class) GETSTRUCT(tuple);
  
! 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 ab98a9b..f8689c0 100644
*** a/contrib/sepgsql/relation.c
--- b/contrib/sepgsql/relation.c
*** sepgsql_attribute_post_create(Oid relOid
*** 54,65 
  	ObjectAddress object;
  	Form_pg_attribute attForm;
  	StringInfoData audit_name;
  
  	/*
! 	 * Only attributes within regular relation have individual security
! 	 * labels.
  	 */
! 	if (get_rel_relkind(relOid) != RELKIND_RELATION)
  		return;
  
  	/*
--- 54,66 
  	ObjectAddress object;
  	Form_pg_attribute attForm;
  	StringInfoData audit_name;
+ 	char		relkind = get_rel_relkind(relOid);
  
  	/*
! 	 * Only attributes within regular relations or partition relations have
! 	 * individual security labels.
  	 */
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		return;
  
  	/*
*** sepgsql_attribute_drop(Oid relOid, AttrN
*** 135,142 
  {
  	ObjectAddress object;
  	char	   *audit_name;
  
! 	if (get_rel_relkind(relOid) != RELKIND_RELATION)
  		return;
  
  	/*
--- 136,144 
  {
  	ObjectAddress object;
  	char	   *audit_name;
+ 	char		relkind = get_rel_relkind(relOid);
  
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		return;
  
  	/*
*** sepgsql_attribute_relabel(Oid relOid, At
*** 167,174 
  {
  	ObjectAddress object;
  	char	   *audit_name;
  
! 	if 

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/04/2017 09:55 AM, Mike Palmiotto wrote:
> 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 -- will have another look and test on a machine with selinux
setup. Robert, did you want me to take responsibility to commit on this
or just provide review/feedback?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[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(, 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;
 			break;
@@ -485,7 +498,7 @@ sepgsql_relation_drop(Oid relOid)
 	/*
 	 * check db_column:{drop} 

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

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 ;-)

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Robert Haas]

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.  :-)

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

[Joe Conway]

On 03/31/2017 05: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.
> 
> 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.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[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

[Robert Haas]

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.

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...)

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

[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 @@ exec_object_restorecon(struct selabel_handle * sehnd, Oid catalogId)
 			case 

Re: [HACKERS] partitioned tables and contrib/sepgsql

[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_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 != 

Re: [HACKERS] partitioned tables and contrib/sepgsql

[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

[Robert Haas]

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.

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.
 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.

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

[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

[Robert Haas]

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?

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

[Stephen Frost]

Mike,

* Mike Palmiotto (mike.palmio...@crunchydata.com) 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.  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?

At least on first blush that seems like the right approach.  We'll need
to make sure that the SECURITY LABEL system will properly work with
partitioned tables too, of course, and that the checks are called when a
user queries a partitioned table.  Then we'll need regression tests to
make sure we get it all correct and don't screw it up in the future. ;)

> > 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.

Excellent, thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[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

[HACKERS] partitioned tables and contrib/sepgsql

[Stephen Frost]

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.

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

Thanks!

Stephen


signature.asc
Description: Digital signature