Re: [HACKERS] partitioned tables and contrib/sepgsql
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
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
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
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
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
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
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
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
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
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
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
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
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(&object, 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; /* --- 1
Re: [HACKERS] partitioned tables and contrib/sepgsql
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
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
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
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
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 = get_rel_relkind(relOid); - if (get_rel_relkind(relOid) != RELKIND_RELATION) + if (relkind != RELKIND_RELATION && relkind
Re: [HACKERS] partitioned tables and contrib/sepgsql
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 (get_rel_rel
Re: [HACKERS] partitioned tables and contrib/sepgsql
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
On Tue, Apr 4, 2017 at 10:19 AM, Joe Conway wrote: > On 04/04/2017 06:45 AM, Robert Haas wrote: >> On Mon, Apr 3, 2017 at 12:02 PM, Joe Conway wrote: 0002 looks extremely straightforward, but I wonder if we could get one of the people on this list who knows about sepgsql to have a look? (Stephen Frost, Joe Conway, KaiGai Kohei...) >>> >>> Will have a look later today. >> >> I think it is now tomorrow, unless your time zone is someplace very >> far away. :-) > > OBE -- I have scheduled time in 30 minutes from now, after I have gotten > my first cup of coffee ;-) After some discussion off-list, I've rebased and udpated the patches. Please see attached for further review. Thanks, -- Mike Palmiotto Software Engineer Crunchy Data Solutions https://crunchydata.com From be692f8cc94d74033494d140c310e932c705e785 Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Wed, 29 Mar 2017 14:59:37 + Subject: [PATCH 2/2] Add partitioned table support to sepgsql Account for RELKIND_PARTITIONED_RELATIONS in sepgsql and treat the objects like regular relations. This allows for proper object_access hook behavior for partitioned tables. --- contrib/sepgsql/label.c| 3 ++- contrib/sepgsql/relation.c | 33 +++-- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c index 8a72503..c66581a 100644 --- a/contrib/sepgsql/label.c +++ b/contrib/sepgsql/label.c @@ -787,7 +787,8 @@ exec_object_restorecon(struct selabel_handle * sehnd, Oid catalogId) case RelationRelationId: relForm = (Form_pg_class) GETSTRUCT(tuple); -if (relForm->relkind == RELKIND_RELATION) +if (relForm->relkind == RELKIND_RELATION || + relForm->relkind == RELKIND_PARTITIONED_TABLE) objtype = SELABEL_DB_TABLE; else if (relForm->relkind == RELKIND_SEQUENCE) objtype = SELABEL_DB_SEQUENCE; diff --git a/contrib/sepgsql/relation.c b/contrib/sepgsql/relation.c index 4dc48a0..4fcebc1 100644 --- a/contrib/sepgsql/relation.c +++ b/contrib/sepgsql/relation.c @@ -54,12 +54,14 @@ sepgsql_attribute_post_create(Oid relOid, AttrNumber attnum) ObjectAddress object; Form_pg_attribute attForm; StringInfoData audit_name; + char relkind; /* * Only attributes within regular relation have individual security * labels. */ - if (get_rel_relkind(relOid) != RELKIND_RELATION) + relkind = get_rel_relkind(relOid); + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) return; /* @@ -135,8 +137,10 @@ sepgsql_attribute_drop(Oid relOid, AttrNumber attnum) { ObjectAddress object; char *audit_name; + char relkind; - if (get_rel_relkind(relOid) != RELKIND_RELATION) + relkind = get_rel_relkind(relOid); + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) return; /* @@ -167,8 +171,11 @@ sepgsql_attribute_relabel(Oid relOid, AttrNumber attnum, { ObjectAddress object; char *audit_name; + char relkind; - if (get_rel_relkind(relOid) != RELKIND_RELATION) + relkind = get_rel_relkind(relOid); + + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot set security label on non-regular columns"))); @@ -209,8 +216,11 @@ sepgsql_attribute_setattr(Oid relOid, AttrNumber attnum) { ObjectAddress object; char *audit_name; + char relkind; + + relkind = get_rel_relkind(relOid); - if (get_rel_relkind(relOid) != RELKIND_RELATION) + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) return; /* @@ -290,6 +300,7 @@ sepgsql_relation_post_create(Oid relOid) switch (classForm->relkind) { + case RELKIND_PARTITIONED_TABLE: case RELKIND_RELATION: tclass = SEPG_CLASS_DB_TABLE; break; @@ -336,7 +347,8 @@ sepgsql_relation_post_create(Oid relOid) true); /* - * Assign the default security label on the new relation + * Assign the default security label on the new relation or partitioned + * table. */ object.classId = RelationRelationId; object.objectId = relOid; @@ -344,10 +356,10 @@ sepgsql_relation_post_create(Oid relOid) SetSecurityLabel(&object, SEPGSQL_LABEL_TAG, rcontext); /* - * We also assigns a default security label on columns of the new regular - * tables. + * We also assign a default security label on columns of a new table. */ - if (classForm->relkind == RELKIND_RELATION) + if (classForm->relkind == RELKIND_RELATION || + classForm->relkind == RELKIND_PARTITIONED_TABLE) { Relation arel; ScanKeyData akey; @@ -422,6 +434,7 @@ sepgsql_relation_drop(Oid relOid) relkind = get_rel_relkind(relOid); switch (relkind) { + case RELKIND_PARTITIONED_TABLE: case RELKIND_RELATION: tclass = SEPG_CLASS_DB_TABLE; break; @@ -485,7 +498,7 @@ sepgsql_relation_drop(Oid relOid) /* * check db_column:{drop} permission */ - if (relkind == RELKIND_RELATION) + if (relkind == RE
Re: [HACKERS] partitioned tables and contrib/sepgsql
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
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
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
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
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
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 RelationRelationId: relForm = (Form_pg_class) GETSTRUCT(tuple); -if (relForm->relkind == RELKIND_RELATION) +if (relForm->relkind == RELKIND
Re: [HACKERS] partitioned tables and contrib/sepgsql
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 != RELKIND_PARTITIONED_TABLE) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE),
Re: [HACKERS] partitioned tables and contrib/sepgsql
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
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
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
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
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
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
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