Thread: [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
On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost <sfrost@snowman.net> 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
Mike, * Mike Palmiotto (mike.palmiotto@crunchydata.com) wrote: > On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost <sfrost@snowman.net> 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
On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost <sfrost@snowman.net> 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
On Mon, Mar 27, 2017 at 10:47 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost <sfrost@snowman.net> 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
On Mon, Mar 27, 2017 at 11:22 AM, Mike Palmiotto <mike.palmiotto@crunchydata.com> wrote: > On Mon, Mar 27, 2017 at 10:47 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost <sfrost@snowman.net> 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 RLSyet, 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
On Mon, Mar 27, 2017 at 11:46 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Mar 27, 2017 at 11:22 AM, Mike Palmiotto > <mike.palmiotto@crunchydata.com> wrote: >> On Mon, Mar 27, 2017 at 10:47 AM, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost <sfrost@snowman.net> 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
On Mon, Mar 27, 2017 at 12:09 PM, Mike Palmiotto <mike.palmiotto@crunchydata.com> wrote: > On Mon, Mar 27, 2017 at 11:46 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> <snip> >> 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
Attachment
On Fri, Mar 31, 2017 at 2:14 PM, Mike Palmiotto <mike.palmiotto@crunchydata.com> wrote: > On Mon, Mar 27, 2017 at 12:09 PM, Mike Palmiotto > <mike.palmiotto@crunchydata.com> wrote: >> On Mon, Mar 27, 2017 at 11:46 AM, Robert Haas <robertmhaas@gmail.com> wrote: >>> <snip> >>> 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
Attachment
On Fri, Mar 31, 2017 at 2:14 PM, Mike Palmiotto <mike.palmiotto@crunchydata.com> 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
On Fri, Mar 31, 2017 at 8:23 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Mar 31, 2017 at 2:14 PM, Mike Palmiotto > <mike.palmiotto@crunchydata.com> 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
On 03/31/2017 05:23 PM, Robert Haas wrote: > On Fri, Mar 31, 2017 at 2:14 PM, Mike Palmiotto > <mike.palmiotto@crunchydata.com> 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
On Mon, Apr 3, 2017 at 12:02 PM, Joe Conway <mail@joeconway.com> 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
On 04/04/2017 06:45 AM, Robert Haas wrote: > On Mon, Apr 3, 2017 at 12:02 PM, Joe Conway <mail@joeconway.com> 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
On Tue, Apr 4, 2017 at 10:19 AM, Joe Conway <mail@joeconway.com> wrote: > On 04/04/2017 06:45 AM, Robert Haas wrote: >> On Mon, Apr 3, 2017 at 12:02 PM, Joe Conway <mail@joeconway.com> 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
Attachment
On 04/04/2017 09:55 AM, Mike Palmiotto wrote: > On Tue, Apr 4, 2017 at 10:19 AM, Joe Conway <mail@joeconway.com> wrote: >> On 04/04/2017 06:45 AM, Robert Haas wrote: >>> On Mon, Apr 3, 2017 at 12:02 PM, Joe Conway <mail@joeconway.com> 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
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
Attachment
On Tue, Apr 4, 2017 at 6:56 PM, Joe Conway <mail@joeconway.com> 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 4, 2017 at 6:56 PM, Joe Conway <mail@joeconway.com> 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
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
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
On 2017-04-05 00:58:07 -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Tue, Apr 4, 2017 at 6:56 PM, Joe Conway <mail@joeconway.com> 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
Andres Freund <andres@anarazel.de> 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
On Wed, Apr 5, 2017 at 12:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Apr 4, 2017 at 6:56 PM, Joe Conway <mail@joeconway.com> 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
On 2017-04-05 09:43:53 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> 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
On 2017-03-31 20:23:39 -0400, Robert Haas wrote: > On Fri, Mar 31, 2017 at 2:14 PM, Mike Palmiotto > <mike.palmiotto@crunchydata.com> 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
Andres Freund <andres@anarazel.de> 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
Andres Freund <andres@anarazel.de> 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
On 2017-04-05 10:45:06 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> 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
On 2017-04-05 10:48:27 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> 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
Andres Freund <andres@anarazel.de> 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
Andres Freund <andres@anarazel.de> writes:
> On 2017-04-05 10:45:06 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> 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
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
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
Joe Conway <mail@joeconway.com> 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 <stdbool.h>
is completely silent. On the other hand,
#include "postgres.h"
#define bool _Bool
#define true 1
#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 <stdbool.h>"; 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 <selinux/label.h>
+
+/*
+ * <selinux/label.h> includes <stdbool.h>, 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
On April 5, 2017 9:04:00 AM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote: >Joe Conway <mail@joeconway.com> 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 <stdbool.h> > >is completely silent. On the other hand, > >#include "postgres.h" > >#define bool _Bool >#define true 1 >#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 <stdbool.h>"; 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 <selinux/label.h> >+ >+/* >+ * <selinux/label.h> includes <stdbool.h>, 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 ofthat in the tree. There's a flag to warn regardless. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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 <stdbool.h>". 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 <stdbool.h>, causing it to mean
something else than what a Postgres programmer would expect.
The other headers aren't at issue really.
regards, tom lane
Andres Freund <andres@anarazel.de> 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 <stdbool.h> definition is just
#define true 1
And if you put those two definitions together you will get a warning.
The lack of one with c.h + <stdbool.h> means somebody is cheating.
regards, tom lane
On 4/5/17 12:04, Tom Lane wrote: > Conclusion: Fedora's gcc is playing fast and loose somehow with the > command "#include <stdbool.h>"; 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
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 4/5/17 12:04, Tom Lane wrote:
>> Conclusion: Fedora's gcc is playing fast and loose somehow with the
>> command "#include <stdbool.h>"; 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
<stdbool.h> redefines "true"/"false"; those are close enough.)".
regards, tom lane
On Wed, Apr 5, 2017 at 1:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> On 4/5/17 12:04, Tom Lane wrote: >>> Conclusion: Fedora's gcc is playing fast and loose somehow with the >>> command "#include <stdbool.h>"; 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 > <stdbool.h> 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
On Wed, Apr 5, 2017 at 1:22 PM, Mike Palmiotto <mike.palmiotto@crunchydata.com> wrote: > On Wed, Apr 5, 2017 at 1:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >>> On 4/5/17 12:04, Tom Lane wrote: >>>> Conclusion: Fedora's gcc is playing fast and loose somehow with the >>>> command "#include <stdbool.h>"; 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 >> <stdbool.h> 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
Attachment
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
Attachment
Joe Conway <mail@joeconway.com> 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
On 04/06/2017 08:29 AM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> 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
Joe Conway <mail@joeconway.com> 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
On 04/06/2017 12:35 PM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> 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
Attachment
On Thu, Apr 6, 2017 at 5:52 PM, Joe Conway <mail@joeconway.com> wrote: > On 04/06/2017 12:35 PM, Tom Lane wrote: >> Joe Conway <mail@joeconway.com> 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
On Fri, Apr 7, 2017 at 2:36 PM, Mike Palmiotto <mike.palmiotto@crunchydata.com> wrote: > On Thu, Apr 6, 2017 at 5:52 PM, Joe Conway <mail@joeconway.com> wrote: >> On 04/06/2017 12:35 PM, Tom Lane wrote: >>> Joe Conway <mail@joeconway.com> 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. Actually attached this time. -- 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
Attachment
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 coupleof 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
On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway <mail@joeconway.com> 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
On 04/07/2017 05:36 PM, Robert Haas wrote: > On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway <mail@joeconway.com> 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
Joe, * Joe Conway (mail@joeconway.com) wrote: > On 04/07/2017 05:36 PM, Robert Haas wrote: > > On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway <mail@joeconway.com> 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
On Sat, Apr 8, 2017 at 10:29 AM, Stephen Frost <sfrost@snowman.net> wrote: > Joe, > > * Joe Conway (mail@joeconway.com) wrote: >> On 04/07/2017 05:36 PM, Robert Haas wrote: >> > On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway <mail@joeconway.com> 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). Without further ado, the updated patch is attached. I added some comment updates, making it clearer where partitioned tables are used and modified the test-cases to be as close to lock-step with regular table regression testing as possible. Let me know if you find any further issues -- I made sure to review pretty thoroughly, so I'd be surprised, but it happens. 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
Attachment
On 04/08/2017 07:29 AM, Stephen Frost wrote: > * Joe Conway (mail@joeconway.com) wrote: >> On 04/07/2017 05:36 PM, Robert Haas wrote: >> > On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway <mail@joeconway.com> 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
On 04/09/2017 02:04 PM, Joe Conway wrote: > On 04/08/2017 07:29 AM, Stephen Frost wrote: >> * Joe Conway (mail@joeconway.com) wrote: >>> On 04/07/2017 05:36 PM, Robert Haas wrote: >>> > On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway <mail@joeconway.com> 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
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
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
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
Joe Conway <mail@joeconway.com> 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
On 04/09/2017 04:14 PM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> 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