Thread: Re: [GENERAL] C++ port of Postgres
On 8/16/16 2:52 AM, Gavin Flower wrote: > In both cases, part of the motivation to change from C was to appeal to > new developers - from what I remember of the discussions. Moving this to -hackers. Original thread at [1]. tl;dr: A C++ port of Postgres has been created, and several folks on general have commented that this makes it easier to work with the Postgres codebase. This potentially attracts more developers to the project. I hope we can find a way to pull these folks into the fold. People in core have complained that we don't have enough hackers coming in (which I agree with). Part of that is lack of familiarity with C. I think we can all agree that a C++ fork of Postgres would be a huge waste of time, so the question becomes should core postgres start supporting C++. Peter wrote a blog about this in 2013 that makes some good arguments [2]; in particular "easing into" this by first officially supporting C++ compilation. I also like the idea of investigating Rust. I realize there's little technical reason why we *need* C++ support. The level if discipline applied to our codebase negates some of the benefits of C++. But maintaining the discipline takes a lot of time and effort, and makes it more difficult to attract new contributors. My hope is that existing hackers can agree on a reasonable way forward and guide/assist new folks that are interested in walking that path. 1: https://www.postgresql.org/message-id/CABgyVxDBd3EvRdo-Rd6eo8QPEqV8=ShaU2SJfo16wfE0R-hXTA@mail.gmail.com 2: https://petereisentraut.blogspot.com/2013/05/moving-to-c.html -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461
Jim Nasby wrote: > My hope is that existing hackers can agree on a reasonable way > forward and guide/assist new folks that are interested in > walking that path. I tried this path. https://github.com/stalkerg/postgres_cpp And I fully support this desire. But I'm in the minority. >I also like the idea of investigating Rust. I am working on it last few weeks. But it's like seek blocks for new DB. I don't know how we can insert Rust code into Postgres spaghetti. -- Yury Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Well, well, well. Another C vs C++ holly war, really? > > In both cases, part of the motivation to change from C was to > > appeal to new developers - from what I remember of the > > discussions. > > Moving this to -hackers. Original thread at [1]. > > tl;dr: A C++ port of Postgres has been created, and several folks on > general have commented that this makes it easier to work with the > Postgres codebase. This potentially attracts more developers to the > project. I hope we can find a way to pull these folks into the fold. Who are these "folks"? How many more developers it would attract _exactly_, not potentially? > People in core have complained that we don't have enough hackers > coming in (which I agree with). Part of that is lack of familiarity > with C. One again, which "people"? I've seen people complained that there is not enough code reviewers and testers. I doubt very much its something C++ will help with. > I think we can all agree that a C++ fork of Postgres would be a huge > waste of time, so the question becomes should core postgres start > supporting C++. > > Peter wrote a blog about this in 2013 that makes some good arguments > [2]; in particular "easing into" this by first officially supporting > C++ compilation. I also like the idea of investigating Rust. And I wrote a blog post (in Russian) [1] in 2016 why nobody should (if they can) write a new code in C++. In my opinion Rust looks way more promising. However I personally prefer to wait like 5 years before using a new and shiny technology. This is also something I blogged about (in Russian [2], translation [3]). > I realize there's little technical reason why we *need* C++ support. You are right, there is none. > The level if discipline applied to our codebase negates some of the > benefits of C++. But maintaining the discipline takes a lot of time > and effort, and makes it more difficult to attract new contributors. There are companies. They hire developers who write code. Doesn't really matters in which language. Long story short - I strongly believe you are trying to solve a problem that doesn't exist. And probably create a few new ones. [1] http://eax.me/c-vs-cpp/ [2] http://eax.me/cpp-will-never-die/ [3] http://www.viva64.com/en/b/0324/ -- Best regards, Aleksander Alekseev
On 08/16/2016 05:47 PM, Jim Nasby wrote: > I realize there's little technical reason why we *need* C++ support. The > level if discipline applied to our codebase negates some of the benefits > of C++. But maintaining the discipline takes a lot of time and effort, > and makes it more difficult to attract new contributors. I suspect that it would take as much discipline to keep a C++ codebase readable, as the current C codebase. If not more. - Heikki
Aleksander Alekseev wrote: > You are right, there is none. First: trees in parser, planer and etc. Second: normal exception. Two big projects lately move to C++ from C: GCC, Mesa You can read their reasons. Only C++ we can use without full rewrite currently. (or ObjectC maybe) If we wish fix C limitations. -- Yury Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
On Tue, Aug 16, 2016 at 10:47 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: > On 8/16/16 2:52 AM, Gavin Flower wrote: >> In both cases, part of the motivation to change from C was to appeal to >> new developers - from what I remember of the discussions. > > Moving this to -hackers. Original thread at [1]. > > tl;dr: A C++ port of Postgres has been created, and several folks on general > have commented that this makes it easier to work with the Postgres codebase. > This potentially attracts more developers to the project. I hope we can find > a way to pull these folks into the fold. > > People in core have complained that we don't have enough hackers coming in > (which I agree with). Part of that is lack of familiarity with C. > > I think we can all agree that a C++ fork of Postgres would be a huge waste > of time, so the question becomes should core postgres start supporting C++. > > Peter wrote a blog about this in 2013 that makes some good arguments [2]; in > particular "easing into" this by first officially supporting C++ > compilation. I also like the idea of investigating Rust. I'm not really interested in supporting PostgreSQL code written in other languages entirely, such as Rust, but I do think it would make sense to write our code so that it can be compiled using either a C compiler or a C++ compiler. Even if we don't ever use any C++ code in core, this would let people who create forks or extensions use it if they wished. It wouldn't be that much work to maintain, either: we'd just set up some buildfarm members that compiled using C++ and when they turned red, we'd go fix it. I agree with your statement that one of our biggest problems is getting more developers interested in working on PostgreSQL. Even if there's only a 10% chance that something like this will help, why not?We're not talking about moving the earth. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 08/16/2016 09:33 AM, Robert Haas wrote: > On Tue, Aug 16, 2016 at 10:47 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: >> On 8/16/16 2:52 AM, Gavin Flower wrote: > I agree with your statement that one of our biggest problems is > getting more developers interested in working on PostgreSQL. Even if > there's only a 10% chance that something like this will help, why not? > We're not talking about moving the earth. Right. It is just reality that less people are learning C which means less people will be interested in joining a project that is focused or (required) to be C. Sincerely, JD -- Command Prompt, Inc. http://the.postgres.company/ +1-503-667-4564 PostgreSQL Centered full stack support, consulting and development. Everyone appreciates your honesty, until you are honest with them. Unless otherwise stated, opinions are my own.
Robert Haas <robertmhaas@gmail.com> writes:
> I'm not really interested in supporting PostgreSQL code written in
> other languages entirely, such as Rust, but I do think it would make
> sense to write our code so that it can be compiled using either a C
> compiler or a C++ compiler. Even if we don't ever use any C++ code in
> core, this would let people who create forks or extensions use it if
> they wished. It wouldn't be that much work to maintain, either: we'd
> just set up some buildfarm members that compiled using C++ and when
> they turned red, we'd go fix it.
I think this might have advantages purely from the standpoint of new
compilers possibly offering useful warnings we don't get now. But
if we only go this far, I'm pretty dubious that it really helps people
to develop extensions in C++. Almost invariably, if you ask *why* they
want to do that, you'll get an answer involving C++ libraries that are
not going to play very nice with our error handling or memory management
conventions. I do not see how we could C++-ify the error handling without
making a complete break with C compilers ... which is a step I don't
really want to take.
The whole thing would make a lot more sense given a credible design
for error handling that keeps both languages happy.
A lot of the other things people have muttered about, such as heavier
use of inline functions instead of macros, don't particularly need C++
at all.
regards, tom lane
On Tue, Aug 16, 2016 at 12:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I'm not really interested in supporting PostgreSQL code written in >> other languages entirely, such as Rust, but I do think it would make >> sense to write our code so that it can be compiled using either a C >> compiler or a C++ compiler. Even if we don't ever use any C++ code in >> core, this would let people who create forks or extensions use it if >> they wished. It wouldn't be that much work to maintain, either: we'd >> just set up some buildfarm members that compiled using C++ and when >> they turned red, we'd go fix it. > > I think this might have advantages purely from the standpoint of new > compilers possibly offering useful warnings we don't get now. Yeah, that could be nice. > But > if we only go this far, I'm pretty dubious that it really helps people > to develop extensions in C++. Almost invariably, if you ask *why* they > want to do that, you'll get an answer involving C++ libraries that are > not going to play very nice with our error handling or memory management > conventions. I do not see how we could C++-ify the error handling without > making a complete break with C compilers ... which is a step I don't > really want to take. I agree, but we don't have to agree to change everything before we agree to change anything. If we got an agreement to try to make everything compile in both languages, that'd be more agreement than we've ever had before, and I'd rather take that agreement and run than look a gift horse in the mouth. > The whole thing would make a lot more sense given a credible design > for error handling that keeps both languages happy. Well, getting so that we can at least compile in both systems would certainly increase the chances of somebody being willing to work on such a design. And if nobody ever does, then at least people who want to fork and do research projects based on PostgreSQL will have slightly less work to do when they want to hack it up. PostgreSQL seems to be a very popular starting point for research work, but a paper I read recently complained about the antiquity of our code base. I prefer to call that backward-compatibility, but at some point people stop thinking of you as backward-compatible and instead think of you as simply backward. > A lot of the other things people have muttered about, such as heavier > use of inline functions instead of macros, don't particularly need C++ > at all. True. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Aug 16, 2016 at 1:13 PM, Robert Haas <robertmhaas@gmail.com> wrote:
I agree, this was the main reason why we wanted to add support for C++.
On Tue, Aug 16, 2016 at 12:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I'm not really interested in supporting PostgreSQL code written in
>> other languages entirely, such as Rust, but I do think it would make
>> sense to write our code so that it can be compiled using either a C
>> compiler or a C++ compiler. Even if we don't ever use any C++ code in
>> core, this would let people who create forks or extensions use it if
>> they wished. It wouldn't be that much work to maintain, either: we'd
>> just set up some buildfarm members that compiled using C++ and when
>> they turned red, we'd go fix it.
>
> I think this might have advantages purely from the standpoint of new
> compilers possibly offering useful warnings we don't get now.
Yeah, that could be nice.
> But
> if we only go this far, I'm pretty dubious that it really helps people
> to develop extensions in C++. Almost invariably, if you ask *why* they
> want to do that, you'll get an answer involving C++ libraries that are
> not going to play very nice with our error handling or memory management
> conventions. I do not see how we could C++-ify the error handling without
> making a complete break with C compilers ... which is a step I don't
> really want to take.
I agree, but we don't have to agree to change everything before we
agree to change anything. If we got an agreement to try to make
everything compile in both languages, that'd be more agreement than
we've ever had before, and I'd rather take that agreement and run than
look a gift horse in the mouth.
> The whole thing would make a lot more sense given a credible design
> for error handling that keeps both languages happy.
Well, getting so that we can at least compile in both systems would
certainly increase the chances of somebody being willing to work on
such a design. And if nobody ever does, then at least people who want
to fork and do research projects based on PostgreSQL will have
slightly less work to do when they want to hack it up. PostgreSQL
seems to be a very popular starting point for research work, but a
paper I read recently complained about the antiquity of our code base.
I prefer to call that backward-compatibility, but at some point people
stop thinking of you as backward-compatible and instead think of you
as simply backward.
I agree, this was the main reason why we wanted to add support for C++.
> A lot of the other things people have muttered about, such as heavier
> use of inline functions instead of macros, don't particularly need C++
> at all.
True.
2016-08-16 18:52 GMT+03:00 Heikki Linnakangas <hlinnaka@iki.fi>: > On 08/16/2016 05:47 PM, Jim Nasby wrote: >> >> I realize there's little technical reason why we *need* C++ support. The >> level if discipline applied to our codebase negates some of the benefits >> of C++. But maintaining the discipline takes a lot of time and effort, >> and makes it more difficult to attract new contributors. > > > I suspect that it would take as much discipline to keep a C++ codebase > readable, as the current C codebase. If not more. For example, its easier and less error prone to define structures with virtual functions in C++ than write vtables manually in C. So, the adequate subset of the C++ features can be useful to write more readable and maintainable C-style code. These features are: - abstract classes (well, structures with virtual functions); - RTTI; - lambda functions; - constexpr functions; - destructors;- templates (very reservedly). But these features should be avoided (as least for now): - exceptions; - the parts of the standard library which generates exceptions (in particular, regex and thread). -- // Dmitry.
On 8/16/16 12:53 PM, Joy Arulraj wrote: > > The whole thing would make a lot more sense given a credible design > > for error handling that keeps both languages happy. > > Well, getting so that we can at least compile in both systems would > certainly increase the chances of somebody being willing to work on > such a design. And if nobody ever does, then at least people who want > to fork and do research projects based on PostgreSQL will have > slightly less work to do when they want to hack it up. PostgreSQL > seems to be a very popular starting point for research work, but a > paper I read recently complained about the antiquity of our code base. > I prefer to call that backward-compatibility, but at some point people > stop thinking of you as backward-compatible and instead think of you > as simply backward. > > I agree, this was the main reason why we wanted to add support for C++. Joy, do you have an idea what a *minimally invasive* patch for C++ support would look like? That's certainly the first step here. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461
On 2016-08-16 18:52:39 +0300, Heikki Linnakangas wrote: > On 08/16/2016 05:47 PM, Jim Nasby wrote: > > I realize there's little technical reason why we *need* C++ support. The > > level if discipline applied to our codebase negates some of the benefits > > of C++. But maintaining the discipline takes a lot of time and effort, > > and makes it more difficult to attract new contributors. > > I suspect that it would take as much > discipline to keep a C++ codebase > readable, as the current C codebase. If > not more. Well, having typed pg_list.h style lists, ilist.h linked lists, hash-tables, and proper typechecks for pg_nodes.h instead of the NodeTag stuff, would surely make life easier. But given the small subset of C++ available on all our supported platforms... I think we'd first need to make the decision to cut support for some platforms, before using C++. Which imo is a distinct task from *allowing* to compile with a C++ compiler.
On Tue, Aug 16, 2016 at 9:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I think this might have advantages purely from the standpoint of new > compilers possibly offering useful warnings we don't get now. But > if we only go this far, I'm pretty dubious that it really helps people > to develop extensions in C++. Almost invariably, if you ask *why* they > want to do that, you'll get an answer involving C++ libraries that are > not going to play very nice with our error handling or memory management > conventions. FWIW, it's not uncommon to opt-out of C++ exceptions entirely, for various reasons. For example, the Google C++ style guide forbids it (if only for historical reasons), as does the GCC style guide (since GCC was a C program until several years ago [1]). Sometimes, these third party libraries that mandate the use of exceptions do indeed create significant headaches for Postgres, compatibility-wise, but that isn't a given. IMV, it would be useful to use C++ classes (and even template classes) for a small number of data structures, while still largely adhering to earlier practices (this is what GCC did). Specifically, a few modules such as StringInfo, could be made to follow the RAII/scope bound resource management usefully, which doesn't seem incompatible with memory contexts. However, this doesn't seem terribly exciting to me. [1] https://lwn.net/Articles/542457/ -- Peter Geoghegan
I'm sure this wasn't your intent, but the tone of your response is part of why people don't get involved with Postgres development... On 8/16/16 10:39 AM, Aleksander Alekseev wrote: > Well, well, well. Another C vs C++ holly war, really? Please note that you're the only person in the entire thread that's said anything to the effect of a holy war... > Who are these "folks"? How many more developers it would attract > _exactly_, not potentially? As someone else (Robert?) said, there's a decent chance of it attracting some, and it should be rather non-invasive, so why not try? > One again, which "people"? I've seen people complained that there is > not enough code reviewers and testers. I doubt very much its something > C++ will help with. Will it suddenly draw 20 people? Probably not. But if the community actually welcomes the effort Joy put forth and encourages him then we've very likely gained at least one more; maybe several. OTOH, if the community takes the stance of "WTF WHY DO WE NEED THIS?!", we've just driven Joy and anyone else that's a C++ fan away. When it comes specifically to reviewing and testing, you need to provide some kind of reason for people to do that grunt work. A big form of that is supporting people who want to change something about Postgres. (It's certainly possible to get non-hackers to help with this stuff, but that's a different discussion entirely.) > And I wrote a blog post (in Russian) [1] in 2016 why nobody should (if > they can) write a new code in C++. In my opinion Rust looks way more > promising. However I personally prefer to wait like 5 years before > using a new and shiny technology. This is also something I blogged > about (in Russian [2], translation [3]). I agree that Rust is more interesting than C++. I think it'd be great if we supported it as well, but I don't know how practical that would actually be. Note I said support, not use... it's going to be far more challenging to make Rust (or even C++) a requirement to build Postgres. Maybe we'll eventually go that route, after demonstrating the significant benefits that would need to exist to make that work worthwhile. It's going to be FAR easier to demonstrate that if the native project at least supports using it, vs needing a complete fork. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461
On 2016-08-16 12:59:24 -0400, Tom Lane wrote: > I'm pretty dubious that it really helps people > to develop extensions in C++. Almost invariably, if you ask *why* they > want to do that, you'll get an answer involving C++ libraries that are > not going to play very nice with our error handling or memory management > conventions. I do not see how we could C++-ify the error handling without > making a complete break with C compilers ... which is a step I don't > really want to take. I don't think it's *that* hard to make our and C++ error handling play well together, at least when compiled with a C++ compiler. > The whole thing would make a lot more sense given a credible design > for error handling that keeps both languages happy. Using C++ exceptions instead of sigsetjmp()/siglongjmp, when compiled with a C++ compiler, seems not that hard, and could easily be hidden behind PG_TRY/CATCH/RE_THROW/END_TRY. We'd have to hide the "bottom of the exception stack" cases like PostgresMain() behind another macro, but to me that doesn't sound like a bad idea anyway. I do think it makes sense to work towards being able to compile postgres with both C++ and C compilers. Most of the work towards that is pretty boring... > A lot of the other things people have muttered about, such as heavier > use of inline functions instead of macros, don't particularly need C++ > at all. I think the more exciting bit is type safe lists, hash tables, ..., without having to use huge amounts of macro magic. Using actual inheritance for Node* stuff would also surely make some code better checked (checked casts) and easier to write (less pointless downcasting/upcasting from Node).
On Tue, Aug 16, 2016 at 1:29 PM, Peter Geoghegan <pg@heroku.com> wrote: > IMV, it would be useful to use C++ classes (and even template classes) > for a small number of data structures, while still largely adhering to > earlier practices (this is what GCC did). Specifically, a few modules > such as StringInfo, could be made to follow the RAII/scope bound > resource management usefully, which doesn't seem incompatible with > memory contexts. However, this doesn't seem terribly exciting to me. Actually, come to think of it, I guess this is wrong. The problem with what I say here is that longjmp() and setjmp() are incompatible with the stack unwinding used by C++ destructors in general (exceptions are another issue). I think that the practical implication of that is that we can never use any C++ feature that hides the complexity of resource management, unless and until elog() is reimplemented to not use longjmp() and setjmp(). -- Peter Geoghegan
On 8/16/16 3:29 PM, Andres Freund wrote: > Well, having typed pg_list.h style lists, ilist.h linked lists, > hash-tables, and proper typechecks for pg_nodes.h instead of the NodeTag > stuff, would surely make life easier. I certainly wish parts of the system brought code and "data" together in a better way. Nodes are an example; all the Walker stuff in the planner/executor is another. (I'm not saying C++ would make that better, just saying those are parts of the code I find it much harder to grok.) > But given the small subset of C++ available on all our supported > platforms... I think we'd first need to make the decision to cut support > for some platforms, before using C++. Which imo is a distinct task from > *allowing* to compile with a C++ compiler. Exactly. If we at least maintain support for compiling that means people can experiment with other enhancements in a way that's much more compatible with normal community contribution practices, which makes it far more likely for that stuff to be accepted. As for the backwards compatibility... the stance I've seen the community take is cost vs benefit. Right now the benefits are completely hypothetical, because no one could realistically propose a patch to use C++ (or maybe even Rust) features. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461
On 2016-08-16 13:40:06 -0700, Peter Geoghegan wrote: > On Tue, Aug 16, 2016 at 1:29 PM, Peter Geoghegan <pg@heroku.com> wrote: > > IMV, it would be useful to use C++ classes (and even template classes) > > for a small number of data structures, while still largely adhering to > > earlier practices (this is what GCC did). Specifically, a few modules > > such as StringInfo, could be made to follow the RAII/scope bound > > resource management usefully, which doesn't seem incompatible with > > memory contexts. However, this doesn't seem terribly exciting to me. > > Actually, come to think of it, I guess this is wrong. The problem with > what I say here is that longjmp() and setjmp() are incompatible with > the stack unwinding used by C++ destructors in general (exceptions are > another issue). I think that the practical implication of that is that > we can never use any C++ feature that hides the complexity of resource > management, unless and until elog() is reimplemented to not use > longjmp() and setjmp(). FWIW, IIRC that's not true for gcc/glibc, because they IIRC use common codepaths. But obviously that's not all-encompassing enough to rely on that.
Andres Freund <andres@anarazel.de> writes:
> On 2016-08-16 13:40:06 -0700, Peter Geoghegan wrote:
>> Actually, come to think of it, I guess this is wrong. The problem with
>> what I say here is that longjmp() and setjmp() are incompatible with
>> the stack unwinding used by C++ destructors in general (exceptions are
>> another issue). I think that the practical implication of that is that
>> we can never use any C++ feature that hides the complexity of resource
>> management, unless and until elog() is reimplemented to not use
>> longjmp() and setjmp().
> FWIW, IIRC that's not true for gcc/glibc, because they IIRC use common
> codepaths. But obviously that's not all-encompassing enough to rely on that.
I wonder whether it'd be possible to implement the PG_TRY/CATCH macros
to use C++ exceptions when building in C++. This would probably mean
that C and C++ builds would be incompatible as far as loadable extensions
are concerned, because it'd amount to an ABI difference. But maybe
that's OK. We could certainly have the PG_MODULE_MAGIC macro guard
against the case.
regards, tom lane
On 2016-08-16 18:33, Robert Haas wrote:
> It wouldn't be that much work to maintain, either: we'd
> just set up some buildfarm members that compiled using C++ and when
> they turned red, we'd go fix it.
I think that there exist subtle differences between C and C++ that
without compile-time diagnostic could potentially lead to different
run-time behavior. As an artificial example:
$ cat ./test.c
#include <stdio.h>
int main(void) {FILE *f = fopen("test.bin", "w");if (f == NULL) return 1;fwrite("1", sizeof '1', 1,
f);fclose(f);return0;
}
$ clang ./test.c -o test
$ ./test
$ hexdump test.bin
0000000 0031 0000
0000004
$ clang++ ./test.c -o test
clang-3.9: warning: treating 'c' input as 'c++' when in C++ mode, this
behavior is deprecated
$ ./test
$ hexdump test.bin
0000000 0031
0000001
On Tue, Aug 16, 2016 at 1:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> FWIW, IIRC that's not true for gcc/glibc, because they IIRC use common >> codepaths. But obviously that's not all-encompassing enough to rely on that. > > I wonder whether it'd be possible to implement the PG_TRY/CATCH macros > to use C++ exceptions when building in C++. This would probably mean > that C and C++ builds would be incompatible as far as loadable extensions > are concerned, because it'd amount to an ABI difference. But maybe > that's OK. We could certainly have the PG_MODULE_MAGIC macro guard > against the case. Maybe. I think that the best thing about C++ is the ability to encapsulate and simplify some aspects of resource management quite well, which necessitates reimplementing PG_TRY/CATCH. The worst thing about C++ is that ABI compatibility is far messier. This makes a C++ port seem less compelling to me than the idea first appears. Note, for example, that ICU is implemented in C++, but still has C stub functions, not necessarily for the exclusive benefit of C client code. -- Peter Geoghegan
On 2016-08-16 16:59:56 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2016-08-16 13:40:06 -0700, Peter Geoghegan wrote: > >> Actually, come to think of it, I guess this is wrong. The problem with > >> what I say here is that longjmp() and setjmp() are incompatible with > >> the stack unwinding used by C++ destructors in general (exceptions are > >> another issue). I think that the practical implication of that is that > >> we can never use any C++ feature that hides the complexity of resource > >> management, unless and until elog() is reimplemented to not use > >> longjmp() and setjmp(). > > > FWIW, IIRC that's not true for gcc/glibc, because they IIRC use common > > codepaths. But obviously that's not all-encompassing enough to rely on that. > > I wonder whether it'd be possible to implement the PG_TRY/CATCH macros > to use C++ exceptions when building in C++. Yea, I suggested that somewhere nearby. I think that'd be fairly easy - to me the hard part is making it possible to compile postgres with C++, not changing the exception handling itself. > This would probably mean that C and C++ builds would be incompatible > as far as loadable extensions are concerned, because it'd amount to an > ABI difference. But maybe that's OK. We could certainly have the > PG_MODULE_MAGIC macro guard against the case. Right.
On 16 August 2016 at 17:08, Piotr Stefaniak <postgres@piotr-stefaniak.me> wrote: > On 2016-08-16 18:33, Robert Haas wrote: >> It wouldn't be that much work to maintain, either: we'd >> just set up some buildfarm members that compiled using C++ and when >> they turned red, we'd go fix it. > > I think that there exist subtle differences between C and C++ that > without compile-time diagnostic could potentially lead to different > run-time behavior. It seems to me that if we were really keen on attaching in another "totally compiled" language, that C++ wouldn't seem like the best choice. As you say, it's subtly different, which seems a bit dangerous to me. Further, it's not as if C++ is particularly newer than C. C is about 45 years old; C++, at 33, hardly seems like a "spry young whippersnapper" whose inclusion ought to lead to vast excitement. The would-be "spry young things" that head to my mind are Rust and Go. I'm not sure it's terribly plausible to have parts of Postgres written in both C and (Rust|Go); they're different enough that I'm not sure what functionality would mix sensibly. But I think that would be more interesting, all the same. Perhaps it would work out well to be able to create background workers in Rust, or to implement a stored procedure language in Go. -- When confronted by a difficult problem, solve it by reducing it to the question, "How would the Lone Ranger handle this?"
> Well, getting so that we can at least compile in both systems would > certainly increase the chances of somebody being willing to work on > such a design. From my particular perspective it would be enough if all the internal headers (that one needs to use in writing server-sideextensions) were completely usable in C++. It's not so much hacking on the internals, it's more about being tobuild an extension DLL in C++ that makes extensive use of calls to internals without having to write shim layers. Thatlooks like a lot less work than a full C++ port. And if nobody ever does, then at least people who want > to fork and do research projects based on PostgreSQL will have > slightly less work to do when they want to hack it up. PostgreSQL > seems to be a very popular starting point for research work, but a > paper I read recently complained about the antiquity of our code base. > I prefer to call that backward-compatibility, but at some point people > stop thinking of you as backward-compatible and instead think of you > as simply backward. Certainly the positive arguments for sticking with pure C are diminishing over time, perhaps faster in perception than infact. > > A lot of the other things people have muttered about, such as > heavier > > use of inline functions instead of macros, don't particularly need > C++ > > at all. My point is only that C++ can be used to provide better type safety, with little of any effect on performance. Regards David M Bennett FACS Andl - A New Database Language - andl.org
On Tue, Aug 16, 2016 at 4:22 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
On 8/16/16 12:53 PM, Joy Arulraj wrote:> The whole thing would make a lot more sense given a credible design
> for error handling that keeps both languages happy.
Well, getting so that we can at least compile in both systems would
certainly increase the chances of somebody being willing to work on
such a design. And if nobody ever does, then at least people who want
to fork and do research projects based on PostgreSQL will have
slightly less work to do when they want to hack it up. PostgreSQL
seems to be a very popular starting point for research work, but a
paper I read recently complained about the antiquity of our code base.
I prefer to call that backward-compatibility, but at some point people
stop thinking of you as backward-compatible and instead think of you
as simply backward.
I agree, this was the main reason why we wanted to add support for C++.
Joy, do you have an idea what a *minimally invasive* patch for C++ support would look like? That's certainly the first step here.
Jim -- I believe that the patch will be roughly 6K lines long. The majority of the changes correspond to handling language keyword conflicts.
I must mention that some of the changes I have made preclude the possibility of supporting compilation with both C and C++ compilers. However, I am certain that this limitation can be circumvented with some clever hacking.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461
On 2016-08-17 10:45:25 +1000, dandl wrote: > From my particular perspective it would be enough if all the internal > headers (that one needs to use in writing server-side extensions) were > completely usable in C++. That should already be the case. There's even a dirty hack^WWscript that checks that that remains the case (src/tools/pginclude/cpluspluscheck).
From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Peter Geoghegan > I think that the best thing about C++ is the ability to encapsulate and > simplify some aspects of resource management quite well, which necessitates > reimplementing PG_TRY/CATCH. The worst thing about C++ is that ABI > compatibility is far messier. This makes a C++ port seem less compelling > to me than the idea first appears. From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Christopher > Further, it's not as if C++ is particularly newer than C. C is about 45 > years old; C++, at 33, hardly seems like a "spry young whippersnapper" > whose inclusion ought to lead to vast excitement. > > The would-be "spry young things" that head to my mind are Rust and Go. I'm > not sure it's terribly plausible to have parts of Postgres written in both > C and (Rust|Go); they're different enough that I'm not sure what > functionality would mix sensibly. But I think that would be more > interesting, all the same. Perhaps it would work out well to be able to > create background workers in Rust, or to implement a stored procedure > language in Go. First, I'm neither for nor against rewriting PostgreSQL in C++. But I wonder whether it would really pay for the cost. I'm worried about these, for example: * Wouldn't it increase the coding and testing burdon? Coding and testing in C, and coding and testing in C++. PostgreSQLseem to have many features to develop, and I'm not sure C++ will help to speed up the development of them. * Would it really attract more developers of PostgreSQL itself, not extensions? FYI, Tiobe Index says C is nearly twiceas popular as C++. http://www.tiobe.com/tiobe-index/ * Wouldn't it distance some developers if they don't want to learn C++? As Christopher said, C++ is old and there are manynewer languages that attract developers -- C#, Swift, Go, Java, JavaScript, etc. I wonder whether recent developerswant to spend time in learning complex C++ now. I learned C++ because it is still the most popular language ingame development, but maybe I would not want to learn C++ anymore if I didn't know C++. Regards Takayuki Tsunakawa
> > From my particular perspective it would be enough if all the > internal > > headers (that one needs to use in writing server-side extensions) > were > > completely usable in C++. > > That should already be the case. There's even a dirty hack^WWscript > that checks that that remains the case > (src/tools/pginclude/cpluspluscheck). The source code for my project is here: https://github.com/davidandl/Andl/tree/master/plandl https://github.com/davidandl/Andl/blob/master/plandl/plandl.c I was not able to get this file to compile correctly in C++, and my recollection is that at the time I asked on this list and that was the advice. Sorry, I don't remember the error but it seemed to be too deeply embedded to worry about. I just wrote the C code and moved on. Since the Windows COM in the other part is C++ only, I finished up with a mixed build. It works fine, but is not the ideal outcome. Regards David M Bennett FACS Andl - A New Database Language - andl.org
On 2016-08-17 11:51:04 +1000, dandl wrote: > > > From my particular perspective it would be enough if all the > > internal > > > headers (that one needs to use in writing server-side extensions) > > were > > > completely usable in C++. > > > > That should already be the case. There's even a dirty hack^WWscript > > that checks that that remains the case > > (src/tools/pginclude/cpluspluscheck). > > The source code for my project is here: > https://github.com/davidandl/Andl/tree/master/plandl > https://github.com/davidandl/Andl/blob/master/plandl/plandl.c > > I was not able to get this file to compile correctly in C++, and my > recollection is that at the time I asked on this list and that was the > advice. You need to include the files surrounded by extern "C" { }.
On 17 August 2016 at 09:49, Andres Freund <andres@anarazel.de> wrote:
You need to include the files surrounded by extern "C" { }.
I'd really like to adopt the convention used by many libraries etc of doing this automatically - detecting a c++ compiler in the preprocessor and wrapping in "extern "C"" .
Having the codebase c++-clean enough to compile with a c++ compiler seems to be the easiest way to maintain that, but means more "extern "C"" droppings in the .c files, not just the headers. Still, pretty ignoreable.
> Two big projects lately move to C++ from C: > GCC, Mesa > > You can read their reasons. > Only C++ we can use without full rewrite currently. (or ObjectC maybe) > If we wish fix C limitations. > I would like just to leave this link here: https://en.wikipedia.org/wiki/List_of_fallacies Long story short - no one cares who did what in other projects. Recently I rewrote my OpenGL demo [1] from C++ to C. Uber recently moved from PostgreSQL to MySQL. It proves literally nothing. [1] https://github.com/afiskon/c-opengl-text/ -- Best regards, Aleksander Alekseev
> I'm sure this wasn't your intent, but the tone of your response is > part of why people don't get involved with Postgres development... > > Please note that you're the only person in the entire thread that's > said anything to the effect of a holy war... > > OTOH, if the community takes the stance of "WTF WHY DO WE NEED > THIS?!", we've just driven Joy and anyone else that's a C++ fan away. I'm sorry for being maybe to emotional. It's was not (and never is!) my intent to offend anyone. Also I would like to note that I don't speak for community, I speak for myself. What I saw was: "hey, lets rewrite PostgreSQL in C++ without any good reason except (see [1] list)". Naturally I though (and still think) that you people are just trolls. Or maybe "everything should be written in C++ because it's the only right language and anyone who thinks otherwise is wrong" type of fanatics. Thus I don't think you are here to help. Give a concrete reason. Like "hey, we rewrote this part of code in C++ and look, its much more readable, twice as fast as code in C (how to do benchmarks right is a separate good topic!) and it still compiles fast even on Raspberry Pi, works on all platforms you are supporting, etc". Or "hey, we solved xid wraparound problem once and for all, but solution is in C++, so its for you to decide whether to merge it or not". If you really want to help, just solve _real_ problems using instruments you prefer instead of reposting obviously holly war topic from general@ mailing list. [1] https://en.wikipedia.org/wiki/List_of_fallacies -- Best regards, Aleksander Alekseev
On Aug 16, 2016, at 10:16 PM, Craig Ringer <craig@2ndquadrant.com> wrote:On 17 August 2016 at 09:49, Andres Freund <andres@anarazel.de> wrote:
You need to include the files surrounded by extern "C" { }.I'd really like to adopt the convention used by many libraries etc of doing this automatically - detecting a c++ compiler in the preprocessor and wrapping in "extern "C"" .Having the codebase c++-clean enough to compile with a c++ compiler seems to be the easiest way to maintain that, but means more "extern "C"" droppings in the .c files, not just the headers. Still, pretty ignoreable.
Just having community code compilable with a C++ compiler out of the box would go a long way.
Beyond that, on my end I have been working with PG now for a year and a half and here is a quick list of what I sorely miss from my C++ days:
* Overloading of functions (same as in SQL) keeps naming clean
* Named parameters (same as SQL) keeps code readable
* Adding new function parameters with defaults so I don’t need to pass in NULL, 0, … at 20 places (again supported in SQL)
* Member functions greatly help organize code
* simple inheritance (as emulated today in node types)
At my old employer we used C++ for the DBMS in various degrees in different components.
That degree was agreed upon in coding standards, so we could pick what we like about C++ and blacklist what we didn’t.
E.g. C style exception handling was prohibited
Default memory management (new) was prohibited.
Instead new() was overloaded and hooked into the DBMS memory manager.
I see no reason why this couldn’t be done in PG.
I can’t comment of compiling on a Rasperry PI, but know that my former DBMS code compiled and ran on Windows, Linux, AIX, Sun, HP, and Mac.
But again, just having the community code compile so proprietary (for now) enhancements could be written in C++ would be huge.
Cheers
Serge
2016-08-17 14:40 GMT+03:00 Aleksander Alekseev <a.alekseev@postgrespro.ru>: >> I'm sure this wasn't your intent, but the tone of your response is >> part of why people don't get involved with Postgres development... >> >> Please note that you're the only person in the entire thread that's >> said anything to the effect of a holy war... >> >> OTOH, if the community takes the stance of "WTF WHY DO WE NEED >> THIS?!", we've just driven Joy and anyone else that's a C++ fan away. > > I'm sorry for being maybe to emotional. It's was not (and never is!) my > intent to offend anyone. Also I would like to note that I don't speak > for community, I speak for myself. > > What I saw was: "hey, lets rewrite PostgreSQL in C++ without any good > reason except (see [1] list)". Naturally I though (and still think) that > you people are just trolls. Or maybe "everything should be written in > C++ because it's the only right language and anyone who thinks > otherwise is wrong" type of fanatics. Thus I don't think you are here to > help. > > Give a concrete reason. Like "hey, we rewrote this part of code in C++ > and look, its much more readable, twice as fast as code in C (how to do > benchmarks right is a separate good topic!) and it still compiles fast > even on Raspberry Pi, works on all platforms you are supporting, etc". > Or "hey, we solved xid wraparound problem once and for all, but solution > is in C++, so its for you to decide whether to merge it or not". I doubt that someone will rush to rewrite PostgreSQL in C++. At now, the reason to consider the refactoring of current codebase to make the C++ compilers happy, is to make the code more qualitative. I think, that only after that step it is reasonable to consider the use some of C++ features.
On Tue, Aug 16, 2016 at 5:08 PM, Piotr Stefaniak
<postgres@piotr-stefaniak.me> wrote:
> On 2016-08-16 18:33, Robert Haas wrote:
>> It wouldn't be that much work to maintain, either: we'd
>> just set up some buildfarm members that compiled using C++ and when
>> they turned red, we'd go fix it.
>
> I think that there exist subtle differences between C and C++ that
> without compile-time diagnostic could potentially lead to different
> run-time behavior. As an artificial example:
>
> $ cat ./test.c
> #include <stdio.h>
>
> int main(void) {
> FILE *f = fopen("test.bin", "w");
> if (f == NULL)
> return 1;
> fwrite("1", sizeof '1', 1, f);
> fclose(f);
> return 0;
> }
> $ clang ./test.c -o test
> $ ./test
> $ hexdump test.bin
> 0000000 0031 0000
> 0000004
> $ clang++ ./test.c -o test
> clang-3.9: warning: treating 'c' input as 'c++' when in C++ mode, this
> behavior is deprecated
> $ ./test
> $ hexdump test.bin
> 0000000 0031
> 0000001
Hmm, so sizeof() has different semantics in C vs. C++?
While that's a little alarming, I'm wondering whether this sort of
thing is likely to actually be a problem in practice. We have such a
long laundry list of coding conventions already that I am inclined to
believe we could add a few more without breaking our ability to do
development.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
>>>>> "Robert" == Robert Haas <robertmhaas@gmail.com> writes: Robert> Hmm, so sizeof() has different semantics in C vs. C++? No. '1' has different semantics in C vs C++. (In C, '1' is an int, whereas in C++ it's a char. It so happens that (sizeof '1') is the only case which is valid in both C and C++ where this makes a difference.) -- Andrew (irc:RhodiumToad)
On Wed, Aug 17, 2016 at 11:36 AM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: >>>>>> "Robert" == Robert Haas <robertmhaas@gmail.com> writes: > > Robert> Hmm, so sizeof() has different semantics in C vs. C++? > > No. '1' has different semantics in C vs C++. (In C, '1' is an int, > whereas in C++ it's a char. It so happens that (sizeof '1') is the only > case which is valid in both C and C++ where this makes a difference.) OK. Doesn't seem like a big problem. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 17/08/16 23:40, Aleksander Alekseev wrote: >> I'm sure this wasn't your intent, but the tone of your response is >> part of why people don't get involved with Postgres development... >> >> Please note that you're the only person in the entire thread that's >> said anything to the effect of a holy war... >> >> OTOH, if the community takes the stance of "WTF WHY DO WE NEED >> THIS?!", we've just driven Joy and anyone else that's a C++ fan away. > I'm sorry for being maybe to emotional. It's was not (and never is!) my > intent to offend anyone. Also I would like to note that I don't speak > for community, I speak for myself. > > What I saw was: "hey, lets rewrite PostgreSQL in C++ without any good > reason except (see [1] list)". Naturally I though (and still think) that > you people are just trolls. Or maybe "everything should be written in > C++ because it's the only right language and anyone who thinks > otherwise is wrong" type of fanatics. Thus I don't think you are here to > help. > > Give a concrete reason. Like "hey, we rewrote this part of code in C++ > and look, its much more readable, twice as fast as code in C (how to do > benchmarks right is a separate good topic!) and it still compiles fast > even on Raspberry Pi, works on all platforms you are supporting, etc". > Or "hey, we solved xid wraparound problem once and for all, but solution > is in C++, so its for you to decide whether to merge it or not". > > If you really want to help, just solve _real_ problems using instruments > you prefer instead of reposting obviously holly war topic from general@ > mailing list. > > [1] https://en.wikipedia.org/wiki/List_of_fallacies > My main language is Java, and there are a lot of very good reasons for rewriting Postgres in Java, but I'd never push that - as there are also many good reasons for NOT rewriting Postgres in Java! I am not an expert in C++, but I'm interested in its development and growing usage. I've read enough about C++ to think it worthwhile to consider rewriting Postgres in C++. If I had time, I would get deeper into C++, but for now pressures push me towards getting deeper into JavaScript - despite having an intense dislike for JavaScript! The first 2 languages I used commercially were FORTRAN & COBOL back in the 70's, and I've been paid to teacht C to experienced programmers. As far as I am concerned, there is no one language that is perfect. I have written programs i over 20 different languages. So I did not suggest C++, because I'm a C++ Fanatic!!! Cheers, Gavin
On 18 August 2016 at 02:14, Gavin Flower <GavinFlower@archidevsys.co.nz> wrote:
My main language is Java, and there are a lot of very good reasons for rewriting Postgres in Java, but I'd never push that - as there are also many good reasons for NOT rewriting Postgres in Java!
I don't know why folks are jumping on the idea of a "rewrite". The original post mentioned a C++ port, adding C++ compatibility and adopting some C++ features. Hardly a rewrite.
I'm not convinced that's a good idea either, unless it shows compelling advantages in code clarity, performance, static checking, etc. But it's hardly a "rewrite", that's just hyperbolic.
I think that to get anywhere with this you'll need to show a more concrete plan for:
* What happens with libpq
* How this affects existing extensions and what changes they'll need
* How this affects PGXS
* What benefits this change offers. Concrete benefits with examples - performance numbers, code snippets, etc
* Compatibility impact on platform and compiler support
Since there's approximately zero chance of a "one big commit" switchover, you'll also need to present a transition plan, probably something like:
* Make all headers "extern "C"" conditionally if compiled as C++
* Add "extern "C"" to all C files conditionally if compiled as C++
* add a 'configure' option to compile as C++
* Progressively resolve C++-incompatibilities in C files and headers until the whole database compiles as c++
* Resolve any runtime issues
* Add buildfarm client support for optionally building with c++
* Switch one or more buildfarm members to build with c++ or add new ones
* Identify and fix compatibility issues on other platforms
Then down the track we'd:
* Switch to C++ builds by default
* Define a C++ coding standard/policy that strictly identifies what C++ features are permissible
* Shut down the C-only buildfarm members and start permitting some C++ feature use where appropriate
[trimmed cc list because of big attachments]
On 8/16/16 4:22 PM, Jim Nasby wrote:
> Joy, do you have an idea what a *minimally invasive* patch for C++
> support would look like? That's certainly the first step here.
I developed a minimally invasive patch for C++ support a few years ago
shortly after I wrote that blog post. Since there appears to have been
some interest here now, I have updated that and split it up into logical
chunks.
So here you go.
To build this, you need to configure with g++ <= version 5. (4.x works,
too.) g++ version 6 does not work yet because of the issues described
in patch 0013.
Then you also need to edit src/Makefile.custom and set
COPT = -fpermissive -Wno-sign-compare -Wno-write-strings
The -W options are optional just to reduce some noise. Cleaning up
those warnings can be a separate project that might also have some
benefit under C.
The -fpermissive option is a g++ specific option that reduces some
errors to warnings. (So this won't work with clang or other compilers
at all at this point.) In particular, C++ does not allow casting from
or to void pointers without a cast, but -fpermissive allows that. The
step from this to "real" C++ would be adding a bunch of casts around
things like malloc and palloc and other places. That would be mostly
busy work, so I have excluded that here.
The patches are numbered approximately in increasing order of dubiosity.
So 0001 is probably a straight bug fix, 0002 and 0003 are arguably
minor bug fixes as well. The patches through 0012 can probably be
considered for committing in some form. After that it gets a bit hackish.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
- 0001-Fix-use-of-offsetof.patch
- 0002-Use-return-instead-of-exit-in-configure.patch
- 0003-Add-missing-include-files-to-configure-tests.patch
- 0004-Fix-LDFLAGS-test-for-C.patch
- 0005-Add-test-for-Wmissing-prototypes.patch
- 0006-Remove-unnecessary-prototypes.patch
- 0007-Fix-incorrect-type-cast.patch
- 0008-Add-necessary-type-cast.patch
- 0009-Rename-some-typedefs-to-avoid-name-conflicts.patch
- 0010-Reorder-some-things.patch
- 0011-Add-missing-fields-in-struct-initializations.patch
- 0012-Separate-enum-from-struct.patch
- 0013-Avoid-C-key-words.patch
- 0014-Set-up-for-static-asserts-in-C.patch
- 0015-Fix-function-prototypes-for-C.patch
- 0016-Fix-fmgr_oldstyle-for-C.patch
- 0017-Don-t-define-bool-in-C.patch
- 0018-Change-TimeoutId-from-enum-to-integer.patch
- 0019-Add-explicit-cast-for-casting-away-volatile.patch
- 0020-Add-more-extern-key-words.patch
- 0021-Workaround-for-using-typdef-ed-ints-in-loops.patch
- 0022-Disable-conflicting-function.patch
- 0023-Add-C-linkage-to-replacement-declaration-of-fdatasyn.patch
- 0024-Make-inet_net_-to-C-linkage.patch
- 0025-Add-C-linkage-to-functions-exported-by-plugins.patch
- 0026-Add-C-linkage-to-client-libraries-in-non-systematic-.patch
- 0027-Hack-Disable-volatile-that-causes-mysterious-compile.patch
On Wed, Aug 31, 2016 at 9:41 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: >> Joy, do you have an idea what a *minimally invasive* patch for C++ >> support would look like? That's certainly the first step here. > > I developed a minimally invasive patch for C++ support a few years ago > shortly after I wrote that blog post. Since there appears to have been > some interest here now, I have updated that and split it up into logical > chunks. > > So here you go. Hi folks, I'm ready for my first-ever PG code review, and I was considering tackling this one. I.e., https://commitfest.postgresql.org/10/776/ Could someone help me with a few procedural questions? (1) This page: https://wiki.postgresql.org/wiki/Reviewing_a_Patch lists the current commitfest's manager as "(vacant)". But this page: https://commitfest.postgresql.org/ seems to indicate that a commitfest is currently in progress. Is there a commitfest manager at the moment? (2) It seems like there are still a few big questions about this commit: - Is it wanted at the moment? It didn't seem like there's a consensus about whether or not this enhancement shouldbe merged, even if the patch is pretty minimal. - It seems like there are two competing patch sets in play for this enhancement: Joy's and Peter's. Presumably atmost one of them would be merged. Are these signs that I should find a different commit to review for now, so that this one can be further discussed? Thanks very much, Christian
On 08/31/2016 04:41 PM, Peter Eisentraut wrote: > I developed a minimally invasive patch for C++ support a few years ago > shortly after I wrote that blog post. Since there appears to have been > some interest here now, I have updated that and split it up into logical > chunks. > > So here you go. Looking at this with the POV of what would make sense, even if we don't care about C++. > The patches are numbered approximately in increasing order of dubiosity. > So 0001 is probably a straight bug fix, 0002 and 0003 are arguably > minor bug fixes as well. The patches through 0012 can probably be > considered for committing in some form. After that it gets a bit hackish. 0001-0003 look clear to me as well. 0006 - 0009 also seem OK. The rest really only make sense if we decided to make the switch to C++. - Heikki
Christian Convey <christian.convey@gmail.com> writes: > Could someone help me with a few procedural questions? > (1) This page: https://wiki.postgresql.org/wiki/Reviewing_a_Patch > lists the current commitfest's manager as "(vacant)". But this page: > https://commitfest.postgresql.org/ seems to indicate that a commitfest > is currently in progress. Is there a commitfest manager at the > moment? Fabrízio de Royes Mello is it. I guess he hasn't noticed that that page ought to be updated. > (2) It seems like there are still a few big questions about this commit: > - Is it wanted at the moment? It didn't seem like there's a > consensus about whether or not this enhancement should be > merged, even if the patch is pretty minimal. > - It seems like there are two competing patch > sets in play for this enhancement: Joy's and > Peter's. Presumably at most one of them would > be merged. These are things that reviews should be helping to decide. It's probably a squishier topic than some patches, but if you're interested, feel free to read code and weigh in. regards, tom lane
On Tue, Sep 6, 2016 at 3:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> (2) It seems like there are still a few big questions about this commit: >> - Is it wanted at the moment? It didn't seem like there's a >> consensus about whether or not this enhancement should be >> merged, even if the patch is pretty minimal. >> - It seems like there are two competing patch >> sets in play for this enhancement: Joy's and >> Peter's. Presumably at most one of them would >> be merged. > > These are things that reviews should be helping to decide. It's probably > a squishier topic than some patches, but if you're interested, feel free > to read code and weigh in. Thanks. It sounds like worst-case scenario, I perform an unneeded review. I'll give it a shot.
> Thanks. It sounds like worst-case scenario, I perform an unneeded > review. I'll give it a shot. Hi guys, Apologies for more boring process-related questions, but any pointers would be greatly appreciated... I'm a bit confused about how PG's code-review process is meant to handle this C++ port. My confusion may stem from the combination of my inexperience with the process, and there being two competing patch sets. Here's some background: * My intention was to review Joy's patch. * On "commitfest.postgresql.org" (for 2016-09), the only C++ -related patch I found was Peter's: [1] * I wrongly assumed that the commitfest entry would be for Joy's patch, not Peter's, so I signed up as its reviewer. (That'sfine - I don't mind reviewing both authors' patch sets.) But here are my questions: Q1) My understanding of PG's code-review process is that it's a pipeline: Step 1. The discussion starts on the pgsql-hackersmailing list, where the author posts a patch. He/she may also post revised patches basedon the discussion. Step 2. A subset of those discussions are modeled by new entries in the commitfest website. Step 3. A subset of those commitfest items get merged. If that's correct, then it sounds like the only way Joy's commit has a chance of acceptance is if Peter's commit is rejected. Because Peter's commit might be merged as part of the 2016-09 commitfest, but Joy's can show up until 2016-11at the earliest. Is my understanding correct? There seems to be a little ambiguity regarding the exact version of the code to be reviewed. This is true for both Joy's and Peter's submissions: * Joy's email provides a link to a Github repo, but does not specify a particular commit (or even branch)in that repo: [2] * In the email thread, Peter did provide a patch set: [3] but the corresponding commitfest entry references a githubbranch: [4] So I have a few questions here: Q2) Are authors expected to submit an unambiguous patch to frame the discussion? (I.e,. a specific patch file, or a specific git commit hash, as opposed to a github repo or a github branch.) Q3) Are authors expected to submit a single patch/commit, or is it acceptable / desirable for a large patch to be broken up as Peter has done? Q4) Do we require that any submitted patches appear as attachments on the pgsql-hackers email list, or is a github URL good enough? Q5) (This question is more generic.) I'm accustomed to using Github's pull-request system, where I can engage in dialog regarding specifc lines of a patch. I haven't noticed anything similar being used for PG code reviews, but perhaps I'm just looking in the wrong places. Are all PG code reviews basically just back-and-forth email conversations on the pgsql-hackers list? Thanks, Christian [1] https://commitfest.postgresql.org/10/776/ [2] https://www.postgresql.org/message-id/CABgyVxDBd3EvRdo-Rd6eo8QPEqV8%3DShaU2SJfo16wfE0R-hXTA%40mail.gmail.com [3] https://www.postgresql.org/message-id/bf9de63c-b669-4b8c-d33b-4a5ed11cd5d4%402ndquadrant.com [4] https://github.com/petere/postgresql/tree/commitfest/c%2B%2B
Christian Convey <christian.convey@gmail.com> writes:
> If that's correct, then it sounds like the only way Joy's commit has
> a chance of acceptance is if Peter's commit is rejected.
> Because Peter's commit might be merged as part of the 2016-09
> commitfest, but Joy's can show up until 2016-11 at the earliest.
No, we're not that rigid about it. The commitfest mechanism is really
mostly meant to ensure that every submission does get looked at within a
reasonable amount of time and doesn't get forgotten about. (We do tend
to be willing to tell new patches they've got to wait till the next fest,
but that's mainly when we're already feeling overloaded by what's in the
current queue.) In this case it would be nonsensical to consider only
Peter's submission and not Joy's, because part of the issue is whether
one's better than the other.
> There seems to be a little ambiguity regarding the exact version of
> the code to be reviewed. This is true for both Joy's and Peter's
> submissions:
> * Joy's email provides a link to a Github repo, but does not specify
> a particular commit (or even branch) in that repo: [2]
> * In the email thread, Peter did provide a patch set: [3]
> but the corresponding commitfest entry references a github branch: [4]
Well, at the point we're at here, it's probably not that important exactly
which version of a patchset you're looking at; I'd take the latest.
> Q4) Do we require that any submitted patches appear as attachments on
> the pgsql-hackers email list, or is a github URL good enough?
Actually, we *do* require that, mainly as a formality to show that the
author intended to submit it for inclusion in Postgres. (We don't want
people coming back later and saying "hey, you ripped off this code from
my github repo without my permission".) If we were seriously considering
adopting Joy's version then that would have to happen before anything got
merged. But at this point it seems like we're in a very exploratory
phase and there's no need for legal niceties yet.
> Q5) (This question is more generic.) I'm accustomed to using Github's
> pull-request system, where I can engage in dialog regarding specifc
> lines of a patch. I haven't noticed anything similar being used for
> PG code reviews, but perhaps I'm just looking in the wrong places.
> Are all PG code reviews basically just back-and-forth email
> conversations on the pgsql-hackers list?
Yup, that's how we roll ... it's ancient habit from before there was
such a thing as git, let alone github, but we've not seen fit to change.
It does have the advantage that it's about equally amenable to high-
level discussions and line-by-line issues.
regards, tom lane
Hi Heikki, Could I ask you a newbie-reviewer question about something I'm seeing here? https://commitfest.postgresql.org/10/776/ From some reading I've done (e.g., Stephen Frost's PGCon 2011 slides), I got the impression that a successful patch would always have this sequence of states in commitfest: 1. patch-record created ... 2. Needs Review ... 3. Ready for Committer But if I'm reading the patch's activity log correctly, it looks like you marked the patch as "Ready for Committer" (2016-09-06 18:59:02) without any record of it having been reviewed. Was that intentional? Thanks very much, Christian P.S. I'm asking because I was planning to review that patch. But I can't tell if any more review by a non-committer is still required by the commitfest workflow. Kind regards, Christian On Tue, Sep 6, 2016 at 3:15 PM, Christian Convey <christian.convey@gmail.com> wrote: > On Tue, Sep 6, 2016 at 3:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> (2) It seems like there are still a few big questions about this commit: >>> - Is it wanted at the moment? It didn't seem like there's a >>> consensus about whether or not this enhancement should be >>> merged, even if the patch is pretty minimal. >>> - It seems like there are two competing patch >>> sets in play for this enhancement: Joy's and >>> Peter's. Presumably at most one of them would >>> be merged. >> >> These are things that reviews should be helping to decide. It's probably >> a squishier topic than some patches, but if you're interested, feel free >> to read code and weigh in. > > Thanks. It sounds like worst-case scenario, I perform an unneeded > review. I'll give it a shot.
On 09/11/2016 01:20 AM, Christian Convey wrote: > Hi Heikki, > > Could I ask you a newbie-reviewer question about something I'm seeing > here? https://commitfest.postgresql.org/10/776/ > > From some reading I've done (e.g., Stephen Frost's PGCon 2011 slides), > I got the impression that a successful patch would always have this > sequence of states in commitfest: > 1. patch-record created > ... > 2. Needs Review > ... > 3. Ready for Committer > > But if I'm reading the patch's activity log correctly, it looks like > you marked the patch as "Ready for Committer" (2016-09-06 18:59:02) > without any record of it having been reviewed. > > Was that intentional? Yeah, I commented on the patches at https://www.postgresql.org/message-id/e8e7e5a7-0308-2c36-d32a-7aab16ba498c%40iki.fi. It was very cursory, but I figured that would be sufficient feedback for now, for Peter to proceed with the first few straightforward patches in the series. I don't think there's consensus that we want to do more than that, to actually switch to C++. > P.S. I'm asking because I was planning to review that patch. But I > can't tell if any more review by a non-committer is still required by > the commitfest workflow. I think this has gotten enough attention, for the commitfest workflow. But of course, if you're interested, feel free to review and comment anyway! - Heikki
On 09/11/2016 01:20 AM, Christian Convey wrote: > Hi Heikki, > > Could I ask you a newbie-reviewer question about something I'm seeing > here? https://commitfest.postgresql.org/10/776/ > > From some reading I've done (e.g., Stephen Frost's PGCon 2011 slides), > I got the impression that a successful patch would always have this > sequence of states in commitfest: > 1. patch-record created > ... > 2. Needs Review > ... > 3. Ready for Committer > > But if I'm reading the patch's activity log correctly, it looks like > you marked the patch as "Ready for Committer" (2016-09-06 18:59:02) > without any record of it having been reviewed. > > Was that intentional? Yeah, I commented on the patches at https://www.postgresql.org/message-id/e8e7e5a7-0308-2c36-d32a-7aab16ba498c%40iki.fi. It was very cursory, but I figured that would be sufficient feedback for now, for Peter to proceed with the first few straightforward patches in the series. I don't think there's consensus that we want to do more than that, to actually switch to C++. > P.S. I'm asking because I was planning to review that patch. But I > can't tell if any more review by a non-committer is still required by > the commitfest workflow. I think this has gotten enough attention, for the commitfest workflow. The workflow is flexible, depending on the nature of patch. But of course, if you're interested, feel free to review and comment anyway! - Heikki
> P.S. I'm asking because I was planning to review that patch. But I >> can't tell if any more review by a non-committer is still required by >> the commitfest workflow. > > > I think this has gotten enough attention, for the commitfest workflow. The > workflow is flexible, depending on the nature of patch. But of course, if > you're interested, feel free to review and comment anyway! Hi Heikki, Thanks for the help. If you think nothing more is needed for this patch set w.r.t. the 2016-09 commitfest, I'll move on to other things. - Christian
Some of your patches look useful, but unrelated to C++: 7, 8, 15, 16(?), 20. I applied that subset to 9.6 and got a clean "make check". Would it make sense to add them to the next commitfest, regardless of the C++ effort? On Wed, Aug 31, 2016 at 9:41 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > [trimmed cc list because of big attachments] > > On 8/16/16 4:22 PM, Jim Nasby wrote: >> Joy, do you have an idea what a *minimally invasive* patch for C++ >> support would look like? That's certainly the first step here. > > I developed a minimally invasive patch for C++ support a few years ago > shortly after I wrote that blog post. Since there appears to have been > some interest here now, I have updated that and split it up into logical > chunks. > > So here you go. > > To build this, you need to configure with g++ <= version 5. (4.x works, > too.) g++ version 6 does not work yet because of the issues described > in patch 0013. > > Then you also need to edit src/Makefile.custom and set > > COPT = -fpermissive -Wno-sign-compare -Wno-write-strings > > The -W options are optional just to reduce some noise. Cleaning up > those warnings can be a separate project that might also have some > benefit under C. > > The -fpermissive option is a g++ specific option that reduces some > errors to warnings. (So this won't work with clang or other compilers > at all at this point.) In particular, C++ does not allow casting from > or to void pointers without a cast, but -fpermissive allows that. The > step from this to "real" C++ would be adding a bunch of casts around > things like malloc and palloc and other places. That would be mostly > busy work, so I have excluded that here. > > The patches are numbered approximately in increasing order of dubiosity. > So 0001 is probably a straight bug fix, 0002 and 0003 are arguably > minor bug fixes as well. The patches through 0012 can probably be > considered for committing in some form. After that it gets a bit hackish. > > -- > 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 >
On Thu, Sep 1, 2016 at 1:41 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
I looked at a random selection of these patches this morning.
[trimmed cc list because of big attachments]
On 8/16/16 4:22 PM, Jim Nasby wrote:
> Joy, do you have an idea what a *minimally invasive* patch for C++
> support would look like? That's certainly the first step here.
I developed a minimally invasive patch for C++ support a few years ago
shortly after I wrote that blog post. Since there appears to have been
some interest here now, I have updated that and split it up into logical
chunks.
So here you go.
I looked at a random selection of these patches this morning.
> 0001-Fix-use-of-offsetof.patch
I agree that this is already invalid C because it's not constant. It is accepted by GCC's C front end no matter what switches you give it and clearly many other compilers too, but not clang's C front end with "-pedantic".
> 0002-Use-return-instead-of-exit-in-configure.patch
Makes sense. Any reason not to #include <stdlib.h> instead like you did for the 0003 patch?
> 0003-Add-missing-include-files-to-configure-tests.patch
Makes sense.
> 0014-Set-up-for-static-asserts-in-C.patch
+#if __cpp_static_assert >= 201411
+#define _Static_assert(condition, errmessage) static_assert(condition, errmessage)
Don't you mean 201103L? C++11 introduced two-argument static_assert, not C++14.
> 0021-Workaround-for-using-typdef-ed-ints-in-loops.patch
>
> Types made from int don't have a ++ operator in C++, so they can't be
> used in for loops without further work.
ForkNumber is not a typedef'd int: it's an enum. Since it's called a "fork *number*" and clearly treated as a number in various places including loops that want to increment it, perhaps it really should be a typedef of an int, instead of an enum. Then either macros or an enum ForkNumberEnum could define the names (you can assign enums to ints, and compare enums and ints, you just can't assign ints to enums so it'd be no problem to have typedef ForkNumber int and then enum ForkNumberEnum { ... } to define the friendly names, but never actually use the type ForkNumberEnum).
> 0023-Add-C-linkage-to-replacement-declaration-of-fdatasyn.patch
-extern int fdatasync(int fildes);
+extern "C" int fdatasync(int fildes);
Doesn't this need to be made conditional on __cplusplus?
> 0024-Make-inet_net_-to-C-linkage.patch
Same.
> 0025-Add-C-linkage-to-functions-exported-by-plugins.patch
-extern void _PG_init(void);
+extern "C" void _PG_init(void);
Why this way in some places...
+extern "C" {
extern void _PG_init(void);
+}
... and this way in single-function declaration cases in other places?
-char *widget_out(WIDGET *widget);
+char *widget_out(WIDGET * widget);
Noise.
> 0027-Hack-Disable-volatile-that-causes-mysterious-compile.patch
>
> Subject: [PATCH 27/27] Hack: Disable volatile that causes mysterious compiler errors
> Subject: [PATCH 27/27] Hack: Disable volatile that causes mysterious compiler errors
I don't grok the reason for the volatile qualifier in this code the first place but if it actually does something useful, here's one way to fix it. The specific reason for the error reported by GCC is that QueuePosition (a struct) is copied by assignment:
pos = oldpos = QUEUE_BACKEND_POS(MyBackendId);
That means that it invokes the default assignment operator, and you can't do that with a volatile and a non-volatile object either way around according to C++ (though clang seems less fussy than GCC in this case). You could try to untangle that by supplying a suitably qualified explicit member operator:
#ifdef __cplusplus
QueuePosition& operator=(const volatile QueuePosition& other)
{
page = other.page;
offset = other.offset;
return *this;
}
#endif
But that doesn't help with assignments going the other way.... How about just defining a macro in the style of the existing macros and then using that in place of all those incompatible assignment operations:
iff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 716f1c3..469018f 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -194,6 +194,12 @@ typedef struct QueuePosition
(x).offset = (z); \
} while (0)
+#define COPY_QUEUE_POS(x, y) \
+ do { \
+ (x).page = (y).page; \
+ (x).offset = (y).offset; \
+ } while (0)
+
#define QUEUE_POS_EQUAL(x,y) \
((x).page == (y).page && (x).offset == (y).offset)
@@ -1757,7 +1763,8 @@ asyncQueueReadAllNotifications(void)
LWLockAcquire(AsyncQueueLock, LW_SHARED);
/* Assert checks that we have a valid state entry */
Assert(MyProcPid == QUEUE_BACKEND_PID(MyBackendId));
- pos = oldpos = QUEUE_BACKEND_POS(MyBackendId);
+ COPY_QUEUE_POS(pos, QUEUE_BACKEND_POS(MyBackendId));
+ COPY_QUEUE_POS(oldpos, pos);
head = QUEUE_HEAD;
LWLockRelease(AsyncQueueLock);
@@ -1861,7 +1868,7 @@ asyncQueueReadAllNotifications(void)
{
/* Update shared state */
LWLockAcquire(AsyncQueueLock, LW_SHARED);
- QUEUE_BACKEND_POS(MyBackendId) = pos;
+ COPY_QUEUE_POS(QUEUE_BACKEND_POS(MyBackendId), pos);
advanceTail = QUEUE_POS_EQUAL(oldpos, QUEUE_TAIL);
LWLockRelease(AsyncQueueLock);
@@ -1875,7 +1882,7 @@ asyncQueueReadAllNotifications(void)
/* Update shared state */
LWLockAcquire(AsyncQueueLock, LW_SHARED);
- QUEUE_BACKEND_POS(MyBackendId) = pos;
+ COPY_QUEUE_POS(QUEUE_BACKEND_POS(MyBackendId), pos);
advanceTail = QUEUE_POS_EQUAL(oldpos, QUEUE_TAIL);
LWLockRelease(AsyncQueueLock);
@@ -1911,7 +1918,9 @@ asyncQueueProcessPageEntries(volatile QueuePosition *current,
do
{
- QueuePosition thisentry = *current;
+ QueuePosition thisentry;
+
+ COPY_QUEUE_POS(thisentry, *current);
if (QUEUE_POS_EQUAL(thisentry, stop))
break;
@@ -1944,7 +1953,7 @@ asyncQueueProcessPageEntries(volatile QueuePosition *current,
* from a transaction that is not yet visible to snapshots;
* compare the comments at the head of tqual.c.
*/
- *current = thisentry;
+ COPY_QUEUE_POS(*current, thisentry);
reachedStop = true;
break;
} Thomas Munro
http://www.enterprisedb.com
http://www.enterprisedb.com
On Mon, Sep 26, 2016 at 10:57 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Thu, Sep 1, 2016 at 1:41 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>>
>> [trimmed cc list because of big attachments]
>>
>> On 8/16/16 4:22 PM, Jim Nasby wrote:
>> > Joy, do you have an idea what a *minimally invasive* patch for C++
>> > support would look like? That's certainly the first step here.
>>
>> I developed a minimally invasive patch for C++ support a few years ago
>> shortly after I wrote that blog post. Since there appears to have been
>> some interest here now, I have updated that and split it up into logical
>> chunks.
>>
>> So here you go.
>
>
> I looked at a random selection of these patches this morning.
And this morning I looked at the rest of them.
> 0004-Fix-LDFLAGS-test-for-C.patch
Makes sense.
> 0005-Add-test-for-Wmissing-prototypes.patch
This does seem to follow the example of how we test for support for
other warning flags.
> 0006-Remove-unnecessary-prototypes.patch
Looks OK.
> 0007-Fix-incorrect-type-cast.patch
/* array of check flags, reported to consistentFn */
- bool *entryRes;
+ GinTernaryValue *entryRes;
Right. That would be pretty dodgy even in C if we ever use stdbool.h,
because sizeof(_Bool) is implementation defined. The
interchangeability relies on bool and GinTernaryValue both being
typedefs for 'char'. (Not to mention the dangerous contradictions
possible with bools obtained that way: 'b == false || b == true' can
be false, which I guess has been thought about already and is off
topic here.)
I wonder if the following bit of gin.h should be more nuanced: maybe
it's OK to convert between bool and GinTernaryValue, but it's
definitely not OK to cast between pointers types? Or maybe we should
have a function/macro to convert between the types explicitly and not
encourage people to consider them convertible.
/* * A ternary value used by tri-consistent functions. * * For convenience, this is compatible with booleans. A
booleancan be * safely cast to a GinTernaryValue. */ typedef char GinTernaryValue;
> 0008-Add-necessary-type-cast.patch
Maybe instead of this:
- gcv.check = check;
+ gcv.check = (GinTernaryValue *) check;
... it would be better to do this?
- bool *check = (bool *) PG_GETARG_POINTER(0);
+ GinTernaryValue *check = (GinTernaryValue *) PG_GETARG_POINTER(0);
> 0009-Rename-some-typedefs-to-avoid-name-conflicts.patch
I don't know if it's a relevant precedent or not, but I noticed that
fdwapi.h, amapi.h and tsmapi.h used the convention that function
pointer types are named XXX_function, and then the members of a struct
behaving as a kind of vtable are named XXX.
> 0010-Reorder-some-things.patch
> Forward declarations of static variables are not possible in C++, so
> move the full definition before its use.
Right.
> 0011-Add-missing-fields-in-struct-initializations.patch
I don't undestand why this is necessary, unless you're explicitly
choosing to enable a warning like missing-field-initializers for C++
but not for C. Implicit zero-initialisation of trailing missing
initialisers is a feature, not a bug. Also I noticed that 0013 (or a
proper solution to the keyword collision problem) is needed before
this one.
> 0012-Separate-enum-from-struct.patch
Right.
> 0013-Avoid-C-key-words.patch
> This is not a long-term solution, because C header files that are
> included somewhere might have C++ awareness and will break if the key
> word is defined away. But this shows the list of words that would have
> to be renamed around the code.
Right, let's rename them all directly.
> 0015-Fix-function-prototypes-for-C.patch
I wonder if (perhaps in some later later patch) walkers should take
const pointers and mutators non-const. That may require propagating
constness around some more places.
> 0017-Don-t-define-bool-in-C.patch
Check.
> 0018-Change-TimeoutId-from-enum-to-integer.patch
This works, but I feel like we're losing something valuable if we
convert all our enums to ints just because some tiny bit of code
somewhere wants to loop over them. Maybe we should we keep enums like
this, and do the necessary casting in the small number of places that
do int-like-stuff with them? Like so:
diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index 7171a7c..cc5b2c4 100644
--- a/src/backend/utils/misc/timeout.c
+++ b/src/backend/utils/misc/timeout.c
@@ -348,7 +348,7 @@ InitializeTimeouts(void)
for (i = 0; i < MAX_TIMEOUTS; i++) {
- all_timeouts[i].index = i;
+ all_timeouts[i].index = (TimeoutId) i; all_timeouts[i].indicator = false;
all_timeouts[i].timeout_handler= NULL; all_timeouts[i].start_time = 0;
@@ -379,7 +379,8 @@ RegisterTimeout(TimeoutId id, timeout_handler_proc handler) if (id >= USER_TIMEOUT) {
/* Allocate a user-defined timeout reason */
- for (id = USER_TIMEOUT; id < MAX_TIMEOUTS; id++)
+ for (id = USER_TIMEOUT; id < MAX_TIMEOUTS;
+ id = (TimeoutId) ((int) id + 1)) if (all_timeouts[id].timeout_handler ==
NULL) break; if (id >= MAX_TIMEOUTS)
> 0019-Add-explicit-cast-for-casting-away-volatile.patch
- tmp = e->counters;
+ tmp = const_cast<pgssEntry *>(e)->counters;
Obviously we can't just drop C++ casts in here like that. Casting
away volatile might in theory produce the wrong result in some cases,
but we can see that there is a compiler barrier right there already
(included in the preceding SpinLockAcquire) so I guess we could do a
C-style cast to lose volatile so it remains a valid C program:
- tmp = e->counters;
+ tmp = ((pgssEntry *) e)->counters;
> 0020-Add-more-extern-key-words.patch
Don't we just need to put declarations in a header included by guc.c
and the defining transation unit, or explicitly duplicate the
declarations in both (including the one that defines it immediately
after)? Then we would have valid C and C++ with the right linkage.
That is, instead of this:
-const struct config_enum_entry wal_level_options[] = {
+extern const struct config_enum_entry wal_level_options[] = { {"minimal", WAL_LEVEL_MINIMAL, false}, {"replica",
WAL_LEVEL_REPLICA,false}, {"archive", WAL_LEVEL_REPLICA, true}, /* deprecated */
... we would do this:
+extern const struct config_enum_entry wal_level_options[];
const struct config_enum_entry wal_level_options[] = { {"minimal", WAL_LEVEL_MINIMAL, false}, {"replica",
WAL_LEVEL_REPLICA,false}, {"archive", WAL_LEVEL_REPLICA, true}, /* deprecated */
> 0022-Disable-conflicting-function.patch
> This function conflicts with a function in the backend. FIXME
Indeed:
$ git grep ^xml_is_well_formed\(
contrib/xml2/xpath.c:xml_is_well_formed(PG_FUNCTION_ARGS)
src/backend/utils/adt/xml.c:xml_is_well_formed(PG_FUNCTION_ARGS)
The contrib one says:
/** Returns true if document is well-formed** Note: this has been superseded by a core function. We still have to*
haveit in the contrib module so that existing SQL-level references* to the function won't fail; but in normal usage
withup-to-date SQL* definitions for the contrib module, this won't be called.*/
It's been in core since 9.1, ie in every supported version. I'm not
sure what the comment means exactly but I wonder: can we just delete
the contrib version now?
As for the commitfest entry: this thread discusses two different
people's efforts to compile PostgreSQL as C++. Joy Arulraj's github
branch derives in some way from Peter Eisentraut's work, but I have
provided feedback on Peter's patches, because (1) they were posted
here in patch format and (2) there is a commitfest entry listening
Peter as the author. I think several of these patches are
committable, and many obviously are not. Heikki already set the CF
item to 'Ready for Committer' based on an inspection of a few of the
patches, but invited others to continue looking, so I did.
--
Thomas Munro
http://www.enterprisedb.com
On 9/6/16 2:58 PM, Heikki Linnakangas wrote: > 0001-0003 look clear to me as well. 0006 - 0009 also seem OK. The rest > really only make sense if we decided to make the switch to C++. I have committed 0001, 0002, 0003, 0006, as well as 0012. Thomas Munro had some interesting comments on 0007-0009 that are worth considering further. The rest of the patches will be kept around for future amusement. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 9/28/16 10:48 PM, Thomas Munro wrote: > I wonder if the following bit of gin.h should be more nuanced: maybe > it's OK to convert between bool and GinTernaryValue, but it's > definitely not OK to cast between pointers types? Or maybe we should > have a function/macro to convert between the types explicitly and not > encourage people to consider them convertible. The more I look into the this, the more this looks like a web of lies. :-) The GIN consistent and triconsistent functions randomly change around between bool and GinTernaryValue, share some of the same data structures, and there is little guarantee that they each get the right kind of value all the time. This could perhaps use some deeper cleaning at some point. I've left that alone for now. >> 0009-Rename-some-typedefs-to-avoid-name-conflicts.patch > > I don't know if it's a relevant precedent or not, but I noticed that > fdwapi.h, amapi.h and tsmapi.h used the convention that function > pointer types are named XXX_function, and then the members of a struct > behaving as a kind of vtable are named XXX. Good idea, and that also overlaps with some other stuff I have wanted to tidy up in pg_dump, so I might get back to that later. >> 0011-Add-missing-fields-in-struct-initializations.patch > > I don't undestand why this is necessary, unless you're explicitly > choosing to enable a warning like missing-field-initializers for C++ > but not for C. I can't reproduce this anymore, so never mind. > As for the commitfest entry: this thread discusses two different > people's efforts to compile PostgreSQL as C++. Joy Arulraj's github > branch derives in some way from Peter Eisentraut's work, but I have > provided feedback on Peter's patches, because (1) they were posted > here in patch format and (2) there is a commitfest entry listening > Peter as the author. I think several of these patches are > committable, and many obviously are not. Heikki already set the CF > item to 'Ready for Committer' based on an inspection of a few of the > patches, but invited others to continue looking, so I did. Yeah, I have committed a few of the patches now and I'll close the CF entry now. Thanks for your research. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Peter, On 2016-09-30 15:24:09 -0400, Peter Eisentraut wrote: > Yeah, I have committed a few of the patches now and I'll close the CF > entry now. Thanks for your research. Are you planning to push more of these at some point? - Andres
On 1/26/17 22:46, Andres Freund wrote: > On 2016-09-30 15:24:09 -0400, Peter Eisentraut wrote: >> Yeah, I have committed a few of the patches now and I'll close the CF >> entry now. Thanks for your research. > > Are you planning to push more of these at some point? Sure, let's take a look. Here are rebased patches. It now contains a patch that removes all C++ keywords, instead of just defining them away. With that, this now builds with g++-6 (formerly only g++-5), and with that, the static_assert support works as well. Interesting patches to review would be: v2-0001-Fix-mixup-of-bool-and-ternary-value.patch v2-0004-Add-necessary-type-cast.patch v2-0005-Rename-some-typedefs-to-avoid-name-conflicts.patch v2-0007-Eliminate-C-keywords.patch v2-0023-Fix-issue-with-enums-and-va_arg.patch The rest are mostly for amusement. Getting rid of the C++ keywords would open up the possibility of using -Wc++-compat, which you have expressed interest in, I think. I think it also increases clarity in some cases in its own right, so it's worth taking a look. (Then again, there are some rush jobs in there, as well.) (This semi-bug was found while preparing these patches: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=eb75f4fced77e108393f18425ec3f7aba2e70a9d) -- 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
Attachment
- v2-0001-Fix-mixup-of-bool-and-ternary-value.patch
- v2-0002-Fix-LDFLAGS-test-for-C.patch
- v2-0003-Add-test-for-Wmissing-prototypes.patch
- v2-0004-Add-necessary-type-cast.patch
- v2-0005-Rename-some-typedefs-to-avoid-name-conflicts.patch
- v2-0006-Reorder-some-things.patch
- v2-0007-Eliminate-C-keywords.patch
- v2-0008-Set-up-for-static-asserts-in-C.patch
- v2-0009-Fix-function-prototypes-for-C.patch
- v2-0010-Fix-fmgr_oldstyle-for-C.patch
- v2-0011-Don-t-define-bool-in-C.patch
- v2-0012-Change-TimeoutId-from-enum-to-integer.patch
- v2-0013-Add-explicit-cast-for-casting-away-volatile.patch
- v2-0014-Add-more-extern-key-words.patch
- v2-0015-Workaround-for-using-typdef-ed-ints-in-loops.patch
- v2-0016-Disable-conflicting-function.patch
- v2-0017-Add-C-linkage-to-replacement-declaration-of-fdata.patch
- v2-0018-Make-inet_net_-to-C-linkage.patch
- v2-0019-Add-C-linkage-to-functions-exported-by-plugins.patch
- v2-0020-Add-C-linkage-to-client-libraries-in-non-systemat.patch
- v2-0021-Hack-Disable-volatile-that-causes-mysterious-comp.patch
- v2-0022-Add-additional-options-to-silence-warnings-for-C.patch
- v2-0023-Fix-issue-with-enums-and-va_arg.patch
Hi,
On 2017-02-28 23:42:45 -0500, Peter Eisentraut wrote:
> On 1/26/17 22:46, Andres Freund wrote:
> > On 2016-09-30 15:24:09 -0400, Peter Eisentraut wrote:
> >> Yeah, I have committed a few of the patches now and I'll close the CF
> >> entry now. Thanks for your research.
> >
> > Are you planning to push more of these at some point?
>
> Sure, let's take a look.
Cool.
> Getting rid of the C++ keywords would open up the possibility of using
> -Wc++-compat, which you have expressed interest in, I think.
Indeed. And if we go down this path, I'm going to argue that we should
add that by default.
> Subject: [PATCH v2 01/23] Fix mixup of bool and ternary value
>
> ---
> src/backend/access/gin/ginscan.c | 2 +-
> src/include/access/gin_private.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/backend/access/gin/ginscan.c b/src/backend/access/gin/ginscan.c
> index c3ce0479c5..c83375d6b4 100644
> --- a/src/backend/access/gin/ginscan.c
> +++ b/src/backend/access/gin/ginscan.c
> @@ -147,7 +147,7 @@ ginFillScanKey(GinScanOpaque so, OffsetNumber attnum,
> key->nuserentries = nUserQueryValues;
>
> key->scanEntry = (GinScanEntry *) palloc(sizeof(GinScanEntry) * nQueryValues);
> - key->entryRes = (bool *) palloc0(sizeof(bool) * nQueryValues);
> + key->entryRes = (GinTernaryValue *) palloc0(sizeof(GinTernaryValue) * nQueryValues);
>
> key->query = query;
> key->queryValues = queryValues;
> diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
> index 34e7339f05..f1f395ac85 100644
> --- a/src/include/access/gin_private.h
> +++ b/src/include/access/gin_private.h
> @@ -281,7 +281,7 @@ typedef struct GinScanKeyData
> int nadditional;
>
> /* array of check flags, reported to consistentFn */
> - bool *entryRes;
> + GinTernaryValue *entryRes;
> bool (*boolConsistentFn) (GinScanKey key);
> GinTernaryValue (*triConsistentFn) (GinScanKey key);
> FmgrInfo *consistentFmgrInfo;
That seems like a pretty clear-cut case given the usage of entryRes[i].
> From 7c2f4f31df0eec816d6bb17aa6df2e7f3c6da03e Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e@gmx.net>
> Date: Tue, 30 Aug 2016 12:00:00 -0400
> Subject: [PATCH v2 02/23] Fix LDFLAGS test for C++
>
> The test looks to link to particular function, so we need to make that
> function have C linkage.
> ---
> config/c-compiler.m4 | 9 ++++++++-
> configure | 27 ++++++++++++++++++++++++---
> 2 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
> index 7d901e1f1a..21fb4645c4 100644
> --- a/config/c-compiler.m4
> +++ b/config/c-compiler.m4
> @@ -353,7 +353,14 @@ AC_DEFUN([PGAC_PROG_CC_LDFLAGS_OPT],
> AC_CACHE_CHECK([whether $CC supports $1], [Ac_cachevar],
> [pgac_save_LDFLAGS=$LDFLAGS
> LDFLAGS="$pgac_save_LDFLAGS $1"
> -AC_RUN_IFELSE([AC_LANG_PROGRAM([extern void $2 (); void (*fptr) () = $2;],[])],
> +AC_RUN_IFELSE([AC_LANG_PROGRAM([#ifdef __cplusplus
> +extern "C" {
> +#endif
> +extern void $2 ();
> +#ifdef __cplusplus
> +}
> +#endif
> +void (*fptr) () = $2;],[])],
> [Ac_cachevar=yes],
> [Ac_cachevar=no],
> [Ac_cachevar="assuming no"])
Hm. I'm a bit confused here.
The description of PGAC_PROG_CC_LDFLAGS_OPT isn't exactly great:
# PGAC_PROG_CC_LDFLAGS_OPT
# ------------------------
# Given a string, check if the compiler supports the string as a
# command-line option. If it does, add the string to LDFLAGS.
doesn't even mention that the second parameter is something kind of
required, and what it could be used for.
Nor is:
# For reasons you'd really rather not know about, this checks whether
# you can link to a particular function, not just whether you can link.
# In fact, we must actually check that the resulting program runs :-(
exactly helpful...
But if we accept the premise of the current way to do things, this
mostly makes sense. I do wonder if we really should use CC to run a C++
compiler, but ...
> From fd5248ca63ce3cf35f22cebd2088c4dfd3d994a4 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e@gmx.net>
> Date: Tue, 30 Aug 2016 12:00:00 -0400
> Subject: [PATCH v2 03/23] Add test for -Wmissing-prototypes
>
> This was part of the hard-coded default warnings set in C, but the
> option is not valid for C++ (since prototypes are always required).
>
> if test "$GCC" = yes -a "$ICC" = no; then
> - CFLAGS="-Wall -Wmissing-prototypes -Wpointer-arith"
> + CFLAGS="-Wall -Wpointer-arith"
> + # not valid for C++
> + PGAC_PROG_CC_CFLAGS_OPT([-Wmissing-prototypes])
> # These work in some but not all gcc versions
> PGAC_PROG_CC_CFLAGS_OPT([-Wdeclaration-after-statement])
> PGAC_PROG_CC_CFLAGS_OPT([-Wendif-labels])
Makes sense.
> From 3447e7cc85c1f2836d882e19952e6db1950a2607 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e@gmx.net>
> Date: Tue, 30 Aug 2016 12:00:00 -0400
> Subject: [PATCH v2 04/23] Add necessary type cast
>
> ---
> src/backend/utils/adt/tsginidx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/backend/utils/adt/tsginidx.c b/src/backend/utils/adt/tsginidx.c
> index 83a939dfd5..8f38972676 100644
> --- a/src/backend/utils/adt/tsginidx.c
> +++ b/src/backend/utils/adt/tsginidx.c
> @@ -309,7 +309,7 @@ gin_tsquery_consistent(PG_FUNCTION_ARGS)
> * query.
> */
> gcv.first_item = GETQUERY(query);
> - gcv.check = check;
> + gcv.check = (GinTernaryValue *) check;
> gcv.map_item_operand = (int *) (extra_data[0]);
> gcv.need_recheck = recheck;
>
> --
> 2.12.0
k.
> From db1807e62dd6ccf358132a7c56ede2531d33336f Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e@gmx.net>
> Date: Tue, 30 Aug 2016 12:00:00 -0400
> Subject: [PATCH v2 05/23] Rename some typedefs to avoid name conflicts
>
> C++ does not like that in struct _archiveHandle some of the fields have
> the same name as a typedef. This changes the meaning of the typedef in
> the scope of the struct, causing warnings and possibly other problems.
>
> Rename the types so they have names distinct from the struct fields.
k.
> From 997b09fca65a139af8e73d7e6356739383bd8df1 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e@gmx.net>
> Date: Tue, 30 Aug 2016 12:00:00 -0400
> Subject: [PATCH v2 06/23] Reorder some things
>
> Forward declarations of static variables are not possible in C++, so
> move the full definition before its use.
k.
> From e3393f8f5718b31c4ffe0346edabd8b08a2c7a3f Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e@gmx.net>
> Date: Tue, 30 Aug 2016 12:00:00 -0400
> Subject: [PATCH v2 07/23] Eliminate C++ keywords
>
> Remove use of these keywords: class constexpr delete friend namespace
> new operator private public template this typeid typename
Phew, this is going to be fun.
As indicated by the size of this patch, such violations are frequent. I
don't think we should commit this without shortly afterwards adding
-Wc++-compat or even -Werror=c++-compat, otherwise we'll just
continually end up breaking this.
> diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
> index 6bab08b7de..fbedc971c0 100644
> --- a/contrib/hstore/hstore.h
> +++ b/contrib/hstore/hstore.h
> @@ -188,7 +188,7 @@ extern Pairs *hstoreArrayToPairs(ArrayType *a, int *npairs);
> * for now, we default to on for the benefit of people restoring old dumps
> */
> #ifndef HSTORE_POLLUTE_NAMESPACE
> -#define HSTORE_POLLUTE_NAMESPACE 1
> +#define HSTORE_POLLUTE_NAMESPACE 0
> #endif
Doesn't seem like something that should be folded into a patch like this.
> diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
> index a38da3047f..302a55a0bf 100644
> --- a/src/backend/catalog/namespace.c
> +++ b/src/backend/catalog/namespace.c
> @@ -2153,7 +2153,7 @@ TSParserIsVisible(Oid prsId)
> {
> HeapTuple tup;
> Form_pg_ts_parser form;
> - Oid namespace;
> + Oid prsnamespace;
> bool visible;
Not yours, but I really do hate the table table prefixes in catalog
tables :(
> --- a/src/backend/optimizer/geqo/geqo_erx.c
> +++ b/src/backend/optimizer/geqo/geqo_erx.c
> @@ -281,7 +281,7 @@ static Gene
> gimme_gene(PlannerInfo *root, Edge edge, Edge *edge_table)
> {
> int i;
> - Gene friend;
> + Gene friend_;
friend_gene? Not a fan of underscore ending name.
> @@ -869,7 +869,7 @@ boolexpr_startup_fn(Node *clause, PredIterInfo info)
> typedef struct
> {
> OpExpr opexpr;
> - Const constexpr;
> + Const const_expr;
> int next_elem;
> int num_elems;
> Datum *elem_values;
> @@ -913,13 +913,13 @@ arrayconst_startup_fn(Node *clause, PredIterInfo info)
> state->opexpr.args = list_copy(saop->args);
>
> /* Set up a dummy Const node to hold the per-element values */
> - state->constexpr.xpr.type = T_Const;
> - state->constexpr.consttype = ARR_ELEMTYPE(arrayval);
> - state->constexpr.consttypmod = -1;
> - state->constexpr.constcollid = arrayconst->constcollid;
> - state->constexpr.constlen = elmlen;
> - state->constexpr.constbyval = elmbyval;
> - lsecond(state->opexpr.args) = &state->constexpr;
> + state->const_expr.xpr.type = T_Const;
> + state->const_expr.consttype = ARR_ELEMTYPE(arrayval);
> + state->const_expr.consttypmod = -1;
> + state->const_expr.constcollid = arrayconst->constcollid;
> + state->const_expr.constlen = elmlen;
> + state->const_expr.constbyval = elmbyval;
> + lsecond(state->opexpr.args) = &state->const_expr;
.oO(C++ needs more classes of keywords/col_name_keyword)
> diff --git a/src/backend/utils/adt/rangetypes_gist.c b/src/backend/utils/adt/rangetypes_gist.c
> index f81b16c236..9f5390f34b 100644
> --- a/src/backend/utils/adt/rangetypes_gist.c
> +++ b/src/backend/utils/adt/rangetypes_gist.c
> @@ -257,8 +257,8 @@ range_gist_penalty(PG_FUNCTION_ARGS)
> GISTENTRY *origentry = (GISTENTRY *) PG_GETARG_POINTER(0);
> GISTENTRY *newentry = (GISTENTRY *) PG_GETARG_POINTER(1);
> float *penalty = (float *) PG_GETARG_POINTER(2);
> - RangeType *orig = DatumGetRangeType(origentry->key);
> - RangeType *new = DatumGetRangeType(newentry->key);
> + RangeType *origkey = DatumGetRangeType(origentry->key);
> + RangeType *newkey = DatumGetRangeType(newentry->key);
I guess you renamed orig just for symmetry? Because I don't think it's a
keyword?
> @@ -663,9 +663,9 @@ RestoreArchive(Archive *AHX)
> if ((te->reqs & (REQ_SCHEMA | REQ_DATA)) != 0)
> {
> /* Show namespace if available */
> - if (te->namespace)
> + if (te->schema)
Arguable the 'namespace' in the comments should be changed to. But
whatever.
> diff --git a/src/bin/pg_dump/pg_backup_archiver.h b/src/bin/pg_dump/pg_backup_archiver.h
> index 81f0db8d3c..ac2ebb7eea 100644
> --- a/src/bin/pg_dump/pg_backup_archiver.h
> +++ b/src/bin/pg_dump/pg_backup_archiver.h
> @@ -212,7 +212,7 @@ typedef enum
>
> struct _archiveHandle
> {
> - Archive public; /* Public part of archive */
> + Archive archive; /* Public part of archive */
> int version; /* Version of file */
>
> char *archiveRemoteVersion; /* When reading an archive, the
> @@ -342,7 +342,7 @@ struct _tocEntry
> bool hadDumper; /* Archiver was passed a dumper routine (used
> * in restore) */
> char *tag; /* index tag */
> - char *namespace; /* null or empty string if not in a schema */
> + char *schema; /* null or empty string if not in a schema */
> char *tablespace; /* null if not in a tablespace; empty string
> * means use database default */
> char *owner;
> @@ -393,10 +393,10 @@ TocEntry *getTocEntryByDumpId(ArchiveHandle *AH, DumpId id);
> extern bool checkSeek(FILE *fp);
Phew, these are painful due to the widespread impact. Will make
backpatching even less fun. But I don't really see an alternative, so we
better just get it over.
> @@ -265,16 +265,16 @@ SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
> * be done both forward and backward, consider also switching timeline
> * accordingly.
> */
> - while (private->tliIndex < targetNentries - 1 &&
> - targetHistory[private->tliIndex].end < targetSegEnd)
> - private->tliIndex++;
> - while (private->tliIndex > 0 &&
> - targetHistory[private->tliIndex].begin >= targetSegEnd)
> - private->tliIndex--;
> + while (private_data->tliIndex < targetNentries - 1 &&
> + targetHistory[private_data->tliIndex].end < targetSegEnd)
> + private_data->tliIndex++;
> + while (private_data->tliIndex > 0 &&
> + targetHistory[private_data->tliIndex].begin >= targetSegEnd)
> + private_data->tliIndex--;
Hm, isn't this causing a bunch of overlong lines? Which'll then be
awkwardly "fixed" by pgindent? Maybe just go for "priv"?
> diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
> index 3421eed74f..2193a95f71 100644
> --- a/src/pl/plpgsql/src/plpgsql.h
> +++ b/src/pl/plpgsql/src/plpgsql.h
> @@ -1078,7 +1078,7 @@ extern PLpgSQL_rec *plpgsql_build_record(const char *refname, int lineno,
> extern int plpgsql_recognize_err_condition(const char *condname,
> bool allow_sqlstate);
> extern PLpgSQL_condition *plpgsql_parse_err_condition(char *condname);
> -extern void plpgsql_adddatum(PLpgSQL_datum *new);
> +extern void plpgsql_adddatum(PLpgSQL_datum *newdatum);
> extern int plpgsql_add_initdatums(int **varnos);
> extern void plpgsql_HashTableInit(void);
>
> @@ -1104,7 +1104,7 @@ extern Oid plpgsql_exec_get_datum_type(PLpgSQL_execstate *estate,
> PLpgSQL_datum *datum);
> extern void plpgsql_exec_get_datum_type_info(PLpgSQL_execstate *estate,
> PLpgSQL_datum *datum,
> - Oid *typeid, int32 *typmod, Oid *collation);
> + Oid *typid, int32 *typmod, Oid *collation);
Hm, this one (and a bunch of others) should have been caught earlier,
but cpluspluscheck was too centered in src/include... Admittedly those
are probably more important, given that one is more likely to include
them directly.
Wee, through that one. That must have been a fun one to write
> From 7c18f989d0dc430059cce70e5f0e634e18705b91 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e@gmx.net>
> Date: Tue, 30 Aug 2016 12:00:00 -0400
> Subject: [PATCH v2 08/23] Set up for static asserts in C++
>
> ---
> src/include/c.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/src/include/c.h b/src/include/c.h
> index 947bd98067..7f303a4f39 100644
> --- a/src/include/c.h
> +++ b/src/include/c.h
> @@ -744,14 +744,26 @@ typedef NameData *Name;
> * about a negative width for a struct bit-field. This will not include a
> * helpful error message, but it beats not getting an error at all.
> */
> +#ifdef __cplusplus
> +#if __cpp_static_assert >= 200410
> +#define _Static_assert(condition, errmessage) static_assert(condition, errmessage)
> +#define HAVE__STATIC_ASSERT 1
> +#endif
> +#endif
A comment about why we're doing this seems appropriate.
> From dadbc472a1e06f2b17b758f24827ba431d499cee Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e@gmx.net>
> Date: Tue, 30 Aug 2016 12:00:00 -0400
> Subject: [PATCH v2 09/23] Fix function prototypes for C++
>
> C++ needs to have the right number of arguments in function signatures.
Ok.
> From 454f463597d4732883c163b92f62494a282b9dc8 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e@gmx.net>
> Date: Tue, 30 Aug 2016 12:00:00 -0400
> Subject: [PATCH v2 10/23] Fix fmgr_oldstyle for C++
>
> C++ needs the function pointer to have the right number of arguments, so
> some casts are needed here.
I've a lingering patch to just remove fmgr_oldstyle, that seems
preferrable to this.
> From 47a7faf131bbee1e56f31220253798f955b05d65 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e@gmx.net>
> Date: Tue, 30 Aug 2016 12:00:00 -0400
> Subject: [PATCH v2 11/23] Don't define bool in C++
>
> ---
> src/test/thread/thread_test.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/src/test/thread/thread_test.c b/src/test/thread/thread_test.c
> index cb93bcc5ab..7ee0991988 100644
> --- a/src/test/thread/thread_test.c
> +++ b/src/test/thread/thread_test.c
> @@ -24,6 +24,8 @@
> #include "postgres.h"
> #else
> /* From src/include/c.h" */
> +#ifndef __cplusplus
> +
> #ifndef bool
> typedef char bool;
> #endif
> @@ -37,6 +39,8 @@ typedef char bool;
> #endif
> #endif
>
> +#endif
> +
I wonder if we shouldn't also use stdbool.h type booleans if available.
> From 6c22f8b7f692d4fe26f807453a2b97993db4cae0 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e@gmx.net>
> Date: Tue, 30 Aug 2016 12:00:00 -0400
> Subject: [PATCH v2 12/23] Change TimeoutId from enum to integer
>
> RegisterTimeout() treats it like an integer. C++ doesn't like enums to
> be treated that way.
> -typedef enum TimeoutId
> -{
> +typedef int TimeoutId;
> +
> /* Predefined timeout reasons */
> - STARTUP_PACKET_TIMEOUT,
> - DEADLOCK_TIMEOUT,
> - LOCK_TIMEOUT,
> - STATEMENT_TIMEOUT,
> - STANDBY_DEADLOCK_TIMEOUT,
> - STANDBY_TIMEOUT,
> - STANDBY_LOCK_TIMEOUT,
> - IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
> +const int STARTUP_PACKET_TIMEOUT = 1;
> +const int DEADLOCK_TIMEOUT = 2;
> +const int LOCK_TIMEOUT = 3;
> +const int STATEMENT_TIMEOUT = 4;
> +const int STANDBY_DEADLOCK_TIMEOUT = 5;
> +const int STANDBY_TIMEOUT = 6;
> +const int STANDBY_LOCK_TIMEOUT = 7;
> +const int IDLE_IN_TRANSACTION_SESSION_TIMEOUT = 8;
> /* First user-definable timeout reason */
> - USER_TIMEOUT,
> +const int USER_TIMEOUT = 9;
> /* Maximum number of timeout reasons */
> - MAX_TIMEOUTS = 16
> -} TimeoutId;
> +const int MAX_TIMEOUTS = 16;
I dislike this one a fair bit. On some stupid compilers you're going to
emit actual symbols with this. We could just cast in RegisterTimeout()?
(skipping to 23 here)
> From 07dbc4ef099c70af672764bc84e1f7ad9a8c7917 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e@gmx.net>
> Date: Wed, 5 Oct 2016 12:00:00 -0400
> Subject: [PATCH v2 23/23] Fix issue with enums and va_arg()
Seems reasonable.
- Andres
Hi Peter, On 2017-02-28 22:30:16 -0800, Andres Freund wrote: > On 2017-02-28 23:42:45 -0500, Peter Eisentraut wrote: > > On 1/26/17 22:46, Andres Freund wrote: > > > On 2016-09-30 15:24:09 -0400, Peter Eisentraut wrote: > > >> Yeah, I have committed a few of the patches now and I'll close the CF > > >> entry now. Thanks for your research. > > > > > > Are you planning to push more of these at some point? > > > > Sure, let's take a look. > [ partial review ] Are you planning to get any of this into v10? There's one or two patches here that could qualify as bugfixes (the GinTernary stuff), but the rest doesn't strike me as something that should be committed at the tail end of the merge window. Regards, Andres
On 4/5/17 19:14, Andres Freund wrote: > Hi Peter, > > On 2017-02-28 22:30:16 -0800, Andres Freund wrote: >> On 2017-02-28 23:42:45 -0500, Peter Eisentraut wrote: >>> On 1/26/17 22:46, Andres Freund wrote: >>>> On 2016-09-30 15:24:09 -0400, Peter Eisentraut wrote: >>>>> Yeah, I have committed a few of the patches now and I'll close the CF >>>>> entry now. Thanks for your research. >>>> >>>> Are you planning to push more of these at some point? >>> >>> Sure, let's take a look. >> [ partial review ] > > Are you planning to get any of this into v10? There's one or two > patches here that could qualify as bugfixes (the GinTernary stuff), but > the rest doesn't strike me as something that should be committed at the > tail end of the merge window. Done. I'll keep refining the keywords issue based on your feedback. That might be something to look into early in PG11. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services