Thread: palloc unification
There was some discussion about unifying backend and frontend code/headers for palloc et al, particularly so that programs that want to mix both can be easily compiled; see http://www.postgresql.org/message-id/20130118150629.GC29501@alap2.anarazel.de for what I believe to be the latest and greatest patch in that area. The good news from that patch is that there is a reported (small) performance increase from the approach suggested there, on Intel and non-Intel platforms. So it seems we're okay on that front. On the source code organization front, however, I'm not too happy with that particular proposal. There would be two files named palloc.h, for one thing, and also the changes to utils/palloc.h leave way too much of the backend-only implementation exposed to the frontend world. I propose to have a new subdirectory src/include/shared, and two header files: src/include/utils/palloc.h (same as today) src/include/shared/fe_memutils.h(pg_malloc, pg_free, pg_strdup decls) utils/palloc.h would be modified so that #ifdef FRONTEND, only the basic function declarations (palloc, pfree, pstrdup, pnstrdup) are exposed; frontend environment would be safe from CurrentMemoryContext etc. The frontend (pg_malloc) function definitions would live somewhere in, say, src/shared/fe_memutils.c. For the palloc() implementation, I think we should create another file, so that frontend-only programs that do not require those symbols (most of them) are not unnecessarily bloated. Opinions on having a new subdir? We could eventually move some stuff from timestamp.c in there, so that pg_xlogdump could get timestamp_to_str from there; we could perhaps remove the ecpg implementation of that as well and make it use this one. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Feb 4, 2013 at 5:50 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > There was some discussion about unifying backend and frontend > code/headers for palloc et al, particularly so that programs that want > to mix both can be easily compiled; see > http://www.postgresql.org/message-id/20130118150629.GC29501@alap2.anarazel.de > for what I believe to be the latest and greatest patch in that area. > The good news from that patch is that there is a reported (small) > performance increase from the approach suggested there, on Intel and > non-Intel platforms. So it seems we're okay on that front. > > On the source code organization front, however, I'm not too happy with > that particular proposal. There would be two files named palloc.h, for > one thing, and also the changes to utils/palloc.h leave way too much of > the backend-only implementation exposed to the frontend world. > > I propose to have a new subdirectory src/include/shared, and two > header files: > > src/include/utils/palloc.h (same as today) > src/include/shared/fe_memutils.h > (pg_malloc, pg_free, pg_strdup decls) > > utils/palloc.h would be modified so that #ifdef FRONTEND, only the basic > function declarations (palloc, pfree, pstrdup, pnstrdup) are exposed; > frontend environment would be safe from CurrentMemoryContext etc. > > The frontend (pg_malloc) function definitions would live somewhere in, > say, src/shared/fe_memutils.c. For the palloc() implementation, I think > we should create another file, so that frontend-only programs that do > not require those symbols (most of them) are not unnecessarily bloated. > > Opinions on having a new subdir? > We could eventually move some stuff from timestamp.c in there, so that > pg_xlogdump could get timestamp_to_str from there; we could perhaps > remove the ecpg implementation of that as well and make it use this one. I like the idea of having a place for shared frontend and backend code very much, but I don't think src/include/shared or src/shared is a good name, because "shared" can mean a lot of things - like "shared library", for example. I think that this should be set up in a manner analogous to libpgport, except not for portability code, but instead for other stuff. Maybe we could call it libpgframework or something. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas escribió: > On Mon, Feb 4, 2013 at 5:50 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > I propose to have a new subdirectory src/include/shared, and two > > header files: > > The frontend (pg_malloc) function definitions would live somewhere in, > > say, src/shared/fe_memutils.c. For the palloc() implementation, I think > > we should create another file, so that frontend-only programs that do > > not require those symbols (most of them) are not unnecessarily bloated. > > > > Opinions on having a new subdir? > I like the idea of having a place for shared frontend and backend code > very much, but I don't think src/include/shared or src/shared is a > good name, because "shared" can mean a lot of things - like "shared > library", for example. I think that this should be set up in a manner > analogous to libpgport, except not for portability code, but instead > for other stuff. Maybe we could call it libpgframework or something. Yeah, I am doing this right now and the "shared" name doesn't seem so good. "libpgframework" sounds decent. So since libpgport comes from src/port, are we okay with src/framework and src/include/framework? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Feb 6, 2013 at 9:38 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Robert Haas escribió: >> On Mon, Feb 4, 2013 at 5:50 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > >> > I propose to have a new subdirectory src/include/shared, and two >> > header files: > >> > The frontend (pg_malloc) function definitions would live somewhere in, >> > say, src/shared/fe_memutils.c. For the palloc() implementation, I think >> > we should create another file, so that frontend-only programs that do >> > not require those symbols (most of them) are not unnecessarily bloated. >> > >> > Opinions on having a new subdir? > >> I like the idea of having a place for shared frontend and backend code >> very much, but I don't think src/include/shared or src/shared is a >> good name, because "shared" can mean a lot of things - like "shared >> library", for example. I think that this should be set up in a manner >> analogous to libpgport, except not for portability code, but instead >> for other stuff. Maybe we could call it libpgframework or something. > > Yeah, I am doing this right now and the "shared" name doesn't seem so > good. "libpgframework" sounds decent. So since libpgport comes from > src/port, are we okay with src/framework and src/include/framework? That seems reasonable to me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 6 February 2013 14:38, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Robert Haas escribió: >> On Mon, Feb 4, 2013 at 5:50 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > >> > I propose to have a new subdirectory src/include/shared, and two >> > header files: > >> > The frontend (pg_malloc) function definitions would live somewhere in, >> > say, src/shared/fe_memutils.c. For the palloc() implementation, I think >> > we should create another file, so that frontend-only programs that do >> > not require those symbols (most of them) are not unnecessarily bloated. >> > >> > Opinions on having a new subdir? > >> I like the idea of having a place for shared frontend and backend code >> very much, but I don't think src/include/shared or src/shared is a >> good name, because "shared" can mean a lot of things - like "shared >> library", for example. I think that this should be set up in a manner >> analogous to libpgport, except not for portability code, but instead >> for other stuff. Maybe we could call it libpgframework or something. > > Yeah, I am doing this right now and the "shared" name doesn't seem so > good. "libpgframework" sounds decent. So since libpgport comes from > src/port, are we okay with src/framework and src/include/framework? "common" ? src/backend/common src/include/common -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Feb 6, 2013 at 9:59 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 6 February 2013 14:38, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> Robert Haas escribió: >>> On Mon, Feb 4, 2013 at 5:50 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> >>> > I propose to have a new subdirectory src/include/shared, and two >>> > header files: >> >>> > The frontend (pg_malloc) function definitions would live somewhere in, >>> > say, src/shared/fe_memutils.c. For the palloc() implementation, I think >>> > we should create another file, so that frontend-only programs that do >>> > not require those symbols (most of them) are not unnecessarily bloated. >>> > >>> > Opinions on having a new subdir? >> >>> I like the idea of having a place for shared frontend and backend code >>> very much, but I don't think src/include/shared or src/shared is a >>> good name, because "shared" can mean a lot of things - like "shared >>> library", for example. I think that this should be set up in a manner >>> analogous to libpgport, except not for portability code, but instead >>> for other stuff. Maybe we could call it libpgframework or something. >> >> Yeah, I am doing this right now and the "shared" name doesn't seem so >> good. "libpgframework" sounds decent. So since libpgport comes from >> src/port, are we okay with src/framework and src/include/framework? > > "common" ? > > src/backend/common > src/include/common +1 > > -- > Simon Riggs http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, 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
Simon Riggs <simon@2ndQuadrant.com> writes:
> On 6 February 2013 14:38, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> Yeah, I am doing this right now and the "shared" name doesn't seem so
>> good. "libpgframework" sounds decent. So since libpgport comes from
>> src/port, are we okay with src/framework and src/include/framework?
> "common" ?
> src/backend/common
> src/include/common
To me the term "framework" carries a lot of baggage that doesn't fit
this usage. "common" seems better.
regards, tom lane
Tom Lane escribió: > Simon Riggs <simon@2ndQuadrant.com> writes: > > On 6 February 2013 14:38, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > >> Yeah, I am doing this right now and the "shared" name doesn't seem so > >> good. "libpgframework" sounds decent. So since libpgport comes from > >> src/port, are we okay with src/framework and src/include/framework? > > > "common" ? > > > src/backend/common > > src/include/common > > To me the term "framework" carries a lot of baggage that doesn't fit > this usage. "common" seems better. Okay, here's an attempt at doing it that way. Notably this creates libpgcommon, a static library, to be used by both frontend and backend. There's only a frontend file now (fe_memutils.c); the backend side of it is empty. I verified that the backend makefile rules work, but there's no attempt to link it into the backend. libpgcommon piggybacks on libpgport: for instance there is no separate submake-pgcommon rule on which to depend, or LDFLAGS additions and the like, I just appended it all to existing pgport rules. Duplicating it would be much more verbose and pointless; if we ever need to distinguish these two libs, that can easily be done. Some existing pg_malloc() implementations tried to do something other than exit(EXIT_FAILURE); pg_upgrade did pg_log(FATAL) for instance. But I don't think there is much point in that, so I've just made them all use fprintf(stderr); exit(EXIT_FAILURE); This is the same approach Zoltan ended up with in his patch. MSVC is known broken (I didn't touch Andres' hunk of that. I will look into that later.) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On 2013-02-06 15:51:15 -0300, Alvaro Herrera wrote: > Okay, here's an attempt at doing it that way. Notably this creates > libpgcommon, a static library, to be used by both frontend and backend. > There's only a frontend file now (fe_memutils.c); the backend side of it > is empty. I verified that the backend makefile rules work, but there's > no attempt to link it into the backend. libpgcommon piggybacks on > libpgport: for instance there is no separate submake-pgcommon rule on > which to depend, or LDFLAGS additions and the like, I just appended it > all to existing pgport rules. Duplicating it would be much more verbose > and pointless; if we ever need to distinguish these two libs, that can > easily be done. Looks good to me. Although I have to admit I am wondering whether there still is a point in using pg_malloc() et al. instead of just using palloc() et al. There doesn't seem to be too much gained by that. But thats probably better done as another patch ontop of this if at all. > Some existing pg_malloc() implementations tried to do something other > than exit(EXIT_FAILURE); pg_upgrade did pg_log(FATAL) for instance. But > I don't think there is much point in that, so I've just made them all > use fprintf(stderr); exit(EXIT_FAILURE); This is the same approach > Zoltan ended up with in his patch. If anything it should be plain write()s, as that wouldn't allocate memory... But otherwise I aggree. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Here's a more finalized version of this. There are two main interesting changes here: 1. postgres_fe.h includes common/fe_memutils.h This means all frontend programs get the #include without having to do it explicitely by themselves. postgres.h includes utils/palloc.h which I used as precendent for this. 2. fe_memutils.h includes utils/palloc.h This means all frontend programs get the palloc() et al prototypes. Some other things of note: a. pg_upgrade was using postgres.h all over the place instead of postgres_fe.h. There's no reason for this AFAICS so I changed it. No warnings, make check passes. b. pg_resetxlog is the only frontend place that uses pg_malloc() and which needs to #include postgres.h instead of postgres_fe.h, so it's the only place that needs to include fe_memutils.h directly. (pg_controldata is the other place that includes postgres.h, but it doesn't need the pg_malloc etc declarations). c. I added the MSVC bits. I tested that most of it works, but the various regress executables as well as zic failed to build due to lack of libpgcommon at link time. I think I fixed it; I'm waiting on new tests to run. (This patch is the fixed version). -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Alvaro Herrera escribió: > c. I added the MSVC bits. I tested that most of it works, but the > various regress executables as well as zic failed to build due to > lack of libpgcommon at link time. I think I fixed it; I'm waiting on > new tests to run. (This patch is the fixed version). The attached is all that's necessary on top of the submitted v2 to make it build on MSVC. Barring objections, I will push this. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Alvaro Herrera escribió: > Alvaro Herrera escribió: > > > c. I added the MSVC bits. I tested that most of it works, but the > > various regress executables as well as zic failed to build due to > > lack of libpgcommon at link time. I think I fixed it; I'm waiting on > > new tests to run. (This patch is the fixed version). > > The attached is all that's necessary on top of the submitted v2 to make > it build on MSVC. > > Barring objections, I will push this. Pushed. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services