Thread: Raising our compiler requirements for 9.6
Hi,
During the 9.5 cycle, and earlier, the topic of increasing our minimum
bar for compilers came up a bunch of times. Specifically whether we
still should continue to use C90 as a baseline.
I think the time has come to rely at least on some newer features.
At the very least I think we should start to rely on 'static inline's
working. There is not, and hasn't been for a while, any buildfarm animal
that does not support it and we go through some ugly lengths to avoid
relying on inline functions in headers. It's a feature that has been
there in most compilers long before C99.
My feeling is that we shouldn't go the full way to C99 because there's
still common compilers without a complete coverage. But individual
features are fine.
The list of features, in the order of perceived importance, that might
be worthwhile thinking about are:
* static inline
* variadic macros
* designated initializers (e.g. somestruct foo = { .bar = 3 } )
* // style comments (I don't care, but it comes up often enough ...)
Others might have different items. I think we should *not* decide on all
of them at once. We should pick items that are supported everywhere and
uncontroversial and do those first.
Greetings,
Andres Freund
On Wed, Jul 1, 2015 at 9:14 AM, Andres Freund <andres@anarazel.de> wrote:
> At the very least I think we should start to rely on 'static inline's
> working. There is not, and hasn't been for a while, any buildfarm animal
> that does not support it and we go through some ugly lengths to avoid
> relying on inline functions in headers. It's a feature that has been
> there in most compilers long before C99.
>
> My feeling is that we shouldn't go the full way to C99 because there's
> still common compilers without a complete coverage. But individual
> features are fine.
I am in full agreement.
> The list of features, in the order of perceived importance, that might
> be worthwhile thinking about are:
> * static inline
> * variadic macros
> * designated initializers (e.g. somestruct foo = { .bar = 3 } )
> * // style comments (I don't care, but it comes up often enough ...)
I don't want to add // style comments, FWIW.
What is the state of support like for variadic macros and designated
initializers? Unlike static inline, I am not aware that they are
something that was widely implemented before C99 was formalized.
--
Peter Geoghegan
Andres Freund <andres@anarazel.de> writes:
> At the very least I think we should start to rely on 'static inline's
> working. There is not, and hasn't been for a while, any buildfarm animal
> that does not support it
pademelon doesn't.
Also, I think there are some other non-gcc animals that nominally allow
"static inline" but will generate warnings when such functions are
unreferenced in a particular compile (that's what the "quiet inline"
configure test is about). That would be hugely annoying for development,
though maybe we don't care too much if it's only a build target.
I'm not against requiring static inline; it would be a huge improvement
really. But we should not fool ourselves that this comes at zero
compatibility cost.
> The list of features, in the order of perceived importance, that might
> be worthwhile thinking about are:
> * static inline
> * variadic macros
> * designated initializers (e.g. somestruct foo = { .bar = 3 } )
> * // style comments (I don't care, but it comes up often enough ...)
Of these I think only the first is really worth breaking portability
for.
regards, tom lane
Peter Geoghegan <pg@heroku.com> writes:
> On Wed, Jul 1, 2015 at 9:14 AM, Andres Freund <andres@anarazel.de> wrote:
>> The list of features, in the order of perceived importance, that might
>> be worthwhile thinking about are:
>> * static inline
>> * variadic macros
>> * designated initializers (e.g. somestruct foo = { .bar = 3 } )
>> * // style comments (I don't care, but it comes up often enough ...)
> I don't want to add // style comments, FWIW.
I concur with that one. I think the portability risk is nil (even
pademelon's compiler takes // without complaint, which surprised me
when I realized it). Rather, I think the argument for continuing to
disallow // has to do with maintaining code style consistency. A mishmash
of // and /* comments looks like heck. (Note, BTW, that pgindent will
forcibly convert // to /* in some though seemingly not all cases.)
As far as "static inline" goes, it occurs to me after more thought that
there really is zero risk of build failures if we start relying on that,
because we already have the "#define inline as empty" trick in configure.
What would happen, on a compiler like pademelon's, is that you'd get a
whole lot of warnings about unreferenced static functions. And maybe some
bloat in the executable, if the compiler is not smart enough to drop those
functions from its output. I think both of these effects are probably
acceptable considering what a small minority of platforms would have any
issue.
A potentially larger issue is that I think we have places where frontend
code is #include'ing backend headers that contain macros we might wish
to convert to inline functions, and that will not work if the macros
contain references to backend functions or global variables. But we could
solve that by refactoring headers where necessary.
> What is the state of support like for variadic macros and designated
> initializers? Unlike static inline, I am not aware that they are
> something that was widely implemented before C99 was formalized.
I agree that the value-for-portability-risk tradeoff doesn't look
great for these features.
regards, tom lane
On 07/01/2015 01:33 PM, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: >> At the very least I think we should start to rely on 'static inline's >> working. There is not, and hasn't been for a while, any buildfarm animal >> that does not support it > > pademelon doesn't. Other reasoning aside, pademelon is running an HPUX version that is 10 years old. I don't think we should care. JD -- Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564 PostgreSQL Centered full stack support, consulting and development. Announcing "I'm offended" is basically telling the world you can't control your own emotions, so everyone else should do it for you.
01.07.2015, 23:33, Tom Lane kirjoitti: > Andres Freund <andres@anarazel.de> writes: >> At the very least I think we should start to rely on 'static inline's >> working. There is not, and hasn't been for a while, any buildfarm animal >> that does not support it > > pademelon doesn't. HP-UX 10.20 was released in 1996, last shipped in July 2002 and unsupported since July 2003. Assuming the new features aren't going to be used in release branches PG 9.5 is probably going to support that platform longer than there's hardware to run it.. > But we should not fool ourselves that this comes at zero > compatibility cost. But compatibility with obsolete platforms doesn't come with zero cost either -- and judging from the fact that no one noticed until now that you couldn't even configure PG 9.0 .. 9.3 on recent Solaris 10 releases (which I believe was the most popular proprietary Unix) suggests that not a whole lot of people care about those platforms anymore. / Oskari
"Joshua D. Drake" <jd@commandprompt.com> writes:
> On 07/01/2015 01:33 PM, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> At the very least I think we should start to rely on 'static inline's
>>> working. There is not, and hasn't been for a while, any buildfarm animal
>>> that does not support it
>> pademelon doesn't.
> Other reasoning aside, pademelon is running an HPUX version that is 10
> years old. I don't think we should care.
Try 16. The reason I run it is not because anyone cares about actually
running Postgres on such a machine; it's just so that we will know when
we are breaking compatibility with ancient C compilers. I brought it up
merely to refute Andres' claim that we do not have buildfarm coverage
of the case.
regards, tom lane
On 2015-07-01 16:33:24 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > At the very least I think we should start to rely on 'static inline's
> > working. There is not, and hasn't been for a while, any buildfarm animal
> > that does not support it
>
> pademelon doesn't.
Oh. I'd gone through all the animals somewhere in the middle of the 9.5
cycle. pademelon was either dormant or not yet reincarnated back then.
I'll go through all again then. Interesting that anole's compiler
supports inlines.
> Also, I think there are some other non-gcc animals that nominally allow
> "static inline" but will generate warnings when such functions are
> unreferenced in a particular compile (that's what the "quiet inline"
> configure test is about). That would be hugely annoying for development,
> though maybe we don't care too much if it's only a build target.
I know that 4c8aa8b5aea1e032f569222d4b6c1019e84622dc "fixed" that test
on a number of older platforms. Apparently even 10+ years ago some
compiler authors added the smarts to recognize whether static inlines
where declared in a header (don't warn) or in a .c file (warn).
> I'm not against requiring static inline; it would be a huge improvement
> really. But we should not fool ourselves that this comes at zero
> compatibility cost.
It's not zero, but I think it's quite small.
> > The list of features, in the order of perceived importance, that might
> > be worthwhile thinking about are:
> > * static inline
> > * variadic macros
> > * designated initializers (e.g. somestruct foo = { .bar = 3 } )
> > * // style comments (I don't care, but it comes up often enough ...)
>
> Of these I think only the first is really worth breaking portability
> for.
If variadic macros, or even designated initializers, were supported by
the same set of animals in the buildfarm I'd happily use it, but they're
unfortunately not...
On 2015-07-01 23:39:06 +0200, Andres Freund wrote: > On 2015-07-01 16:33:24 -0400, Tom Lane wrote: > > Andres Freund <andres@anarazel.de> writes: > > > At the very least I think we should start to rely on 'static inline's > > > working. There is not, and hasn't been for a while, any buildfarm animal > > > that does not support it > > > > pademelon doesn't. > > Oh. I'd gone through all the animals somewhere in the middle of the 9.5 > cycle. pademelon was either dormant or not yet reincarnated back then. > > I'll go through all again then. Interesting that anole's compiler > supports inlines. Here are a list of animals and what they support. I left out several redundant entries (don't need 30 reports of newish linux + gcc supporting everything). animal OS compiler inline quiet inline varargs anole HPUX 11.31 HP C/aC++A.06.25 y y y axolotl fed-pi gcc-4.9 y y y baiji vista MSVC 2005 y y y binturong solaris 10 sun studio 12.3 y y y baiji win 8 MSVC 2012 y y y brolga cygwin gcc-4.3 y y castoroides solaris 10 sun studio 12.1 y y y catamount OSX 10.8 llvm-gcc 4.2 y y y coypu netbsd 5.1 gcc-4.1 y y y currawong win xp MSVC 2008 y y y damselfly illumos 201311 gcc-4.7 y y y dingo solaris 10 gcc-4.9 y y y dromedary OSX 10.6 gcc-4.2 y y y dunlin gento 13.0 icc 13.1 y y y elk freebsd 7.1 gcc-4.2 y y y frogmouth win xp gcc-4.5 y y y gaur HP-UX 10.20 gcc-2.95 y y y gharial HP-UX 11.31 gcc-4.6 y y y locust OSX 10.5 gcc-4.0 y y y mallard fedora 21 clang-3.5 y y y mouflon openbsd 5.7 gcc-4.2 y n* y narwhal win 2003 gcc-3.4 y y y okapi gentoo 12.14 icc 11.1 y y y olinguito openbsd 5.3 gcc-4.2 y n* y opossum netbsd 5.2 gcc-4.1 y y y orangutan OSX 10.4 gcc-4.0 y y y pademelon HP-UX 10.2 HP C Compiler 10.32 n n n pika netbsd 4.99 gcc-4.1 y y ?** protosciurus solaris 10 gcc-3.4 y y y rover_firef OmniOS gcc-4.6 ?*** ?*** ?*** smew debian 6.0 clang 2.9 y y y spoonbill openbsd 3.6 gcc-3.3 y y y * fails due to idiotic warnings we can't do much about: /usr/local/lib/libxml2.so.15.1: warning: strcpy() is almost always misused, please use strlcpy() /usr/local/lib/libxml2.so.15.1: warning: strcat() is almost always misused, please use strlcat() /usr/local/lib/libxslt.so.3.8: warning: sprintf() is often misused, please use snprintf() ** hasn't run since check was added, but gcc-4.1 supports *** doesn't report any details, too old buildfarm client? All supported by 4.6
On 2015-07-02 00:15:14 +0200, Andres Freund wrote: > animal OS compiler inline quiet inline varargs > brolga cygwin gcc-4.3 y y 4.3 obviously supports varargs. Human error. > pademelon HP-UX 10.2 HP C Compiler 10.32 n n n Since, buildfarm/quiet inline test issues aside, pademelon is the only animal not supporting inlines and varargs, I think we should just go ahead and start to use both. - Andres
Andres Freund <andres@anarazel.de> writes:
> Since, buildfarm/quiet inline test issues aside, pademelon is the only
> animal not supporting inlines and varargs, I think we should just go
> ahead and start to use both.
I'm good with using inlines, since as I pointed out upthread, that won't
actually break anything. I'm much less convinced that varargs macros
represent a winning tradeoff. Using those *will* irredeemably break
pre-C99 compilers, and AFAICS we do not have an urgent need for them.
(BTW, where are you drawing the conclusion that all these compilers
support varargs? I do not see a configure test for it.)
regards, tom lane
On 2015-07-01 19:05:08 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Since, buildfarm/quiet inline test issues aside, pademelon is the only > > animal not supporting inlines and varargs, I think we should just go > > ahead and start to use both. > > I'm good with using inlines, since as I pointed out upthread, that won't > actually break anything. I'm much less convinced that varargs macros > represent a winning tradeoff. Using those *will* irredeemably break > pre-C99 compilers, and AFAICS we do not have an urgent need for them. Well, I'll happily take that. > (BTW, where are you drawing the conclusion that all these compilers > support varargs? I do not see a configure test for it.) There is, although not in all branches: PGAC_C_VA_ARGS. We optionally use vararg macros today, for elog (b853eb9), so I assume it works ;) Greetings, Andres Freund
On Wed, Jul 1, 2015 at 6:24 PM, Andres Freund <andres@anarazel.de> wrote: > On 2015-07-02 00:15:14 +0200, Andres Freund wrote: >> animal OS compiler inline quiet inline varargs > >> brolga cygwin gcc-4.3 y y > > 4.3 obviously supports varargs. Human error. > >> pademelon HP-UX 10.2 HP C Compiler 10.32 n n n > > Since, buildfarm/quiet inline test issues aside, pademelon is the only > animal not supporting inlines and varargs, I think we should just go > ahead and start to use both. So far this thread is all about the costs of desupporting compilers that don't have these features, and you're making a good argument (that I think we all agree with) that the cost is small. But you haven't really said what the benefits are. In the case of static inline, the main problem with the status quo AFAICS is that you have to do a little dance any time you'd otherwise use a static inline function (for those following along at home, "git grep INCLUDE_DEFINITIONS src/include" to see how this is done). Now, it is obviously the case that the first time somebody has to do this dance, they will be somewhat inconvenienced, but once you know how to do it, it's not incredibly complicated. So, just to play the devil's advocate here, who really cares? The way we're doing it right now works everywhere and is as efficient on each platform as the compiler on that platform can support. I accept that there is some developer convenience to not having to worry about the INCLUDE_DEFINITIONS thing, and maybe that's a sufficient reason to do it, but is that the only benefit we're hoping to get? I'd consider an argument for adopting one of these features to be much stronger if were accompanied by arguments like: - If we require feature X, we can reduce the size of the generated code and improve performance. - If we require feature X, we can reduce the risk of bugs. - If we require feature X, static analyzers will be able to understand our code better. If everybody else here says "working around the lack of feature X is too much of a pain in the butt and we want to adopt it purely too make development easier", I'm not going to sit here and try to fight the tide. But I personally don't find such arguments all that exciting. It's not at all clear to me that static inline or variadic macros would make our lives meaningfully better, and like Peter, I think // comments are a not something we should adopt, because one style is better than two. Designated initializers would have meaningful documentation value and might help to decrease the risk of bugs, so that almost seems more interesting to me than static inline. We'd need to check what pgindent thinks of them, though, and how much platform coverage we'd be surrendering. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-07-02 11:46:25 -0400, Robert Haas wrote: > In the case of static inline, the main problem with the status quo > AFAICS is that you have to do a little dance any time you'd otherwise > use a static inline function (for those following along at home, "git > grep INCLUDE_DEFINITIONS src/include" to see how this is done). > Now, > it is obviously the case that the first time somebody has to do this > dance, they will be somewhat inconvenienced, but once you know how to > do it, it's not incredibly complicated. I obviously know the schema (having introduced it in pg), but I think the costs are actually rather significant. It makes development more complex, it makes things considerably harder to follow, and it's quite annoying to test (manually redefine PG_USE_INLINE, recompile whole tree). Several times people also argued against using inlines with that trick because it's slowing down older platforms. > So, just to play the devil's advocate here, who really cares? I consider our time one of the most scarce resources around. > I'd consider an argument for adopting one of these features to be much > stronger if were accompanied by arguments like: > > - If we require feature X, we can reduce the size of the generated > code and improve performance. Converting some of the bigger macros (I tested fastgetattr) to inliens, actually does both. See also http://archives.postgresql.org/message-id/4407.1435763473%40sss.pgh.pa.us Now, all that is possible with the INCLUDE_DEFINITIONS stuff, but it makes it considerably more expensive time/complexity wise. > If everybody else here says "working around the lack of feature X is > too much of a pain in the butt and we want to adopt it purely too make > development easier", I'm not going to sit here and try to fight the > tide. But I personally don't find such arguments all that exciting. I find that an odd attitude. > I think // comments are a not something we should adopt, because one > style is better than two I don't particularly care about // comments either. They do have the advantage of allowing three more characters of actual content in one line comments ... Greetings, Andres Freund
Robert Haas <robertmhaas@gmail.com> writes:
> So far this thread is all about the costs of desupporting compilers
> that don't have these features, and you're making a good argument
> (that I think we all agree with) that the cost is small. But you
> haven't really said what the benefits are.
I made the same remark with respect to varargs macros, and I continue
to think that the cost-benefit ratio there is pretty marginal.
However, I do think that there's a case to be made for adopting static
inline. The INCLUDE_DEFINITIONS dance is very inconvenient, so it
discourages people from using static inlines over macros, even in cases
where the macro approach is pretty evil (usually, because of multiple-
evaluation hazards). If we allowed static inlines then we could work
towards having a safer coding standard along the lines of "thou shalt
not write macros that evaluate their arguments more than once".
So the benefits here are easier development and less risk of bugs.
On the other side of the ledger, the costs are pretty minimal; we will
not actually break any platforms, at worst we'll make them unpleasant
to develop on because of lots of warning messages. We have some platforms
like that already, and it's not been a huge problem.
regards, tom lane
On Thu, Jul 2, 2015 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> So far this thread is all about the costs of desupporting compilers >> that don't have these features, and you're making a good argument >> (that I think we all agree with) that the cost is small. But you >> haven't really said what the benefits are. > > I made the same remark with respect to varargs macros, and I continue > to think that the cost-benefit ratio there is pretty marginal. > > However, I do think that there's a case to be made for adopting static > inline. The INCLUDE_DEFINITIONS dance is very inconvenient, so it > discourages people from using static inlines over macros, even in cases > where the macro approach is pretty evil (usually, because of multiple- > evaluation hazards). If we allowed static inlines then we could work > towards having a safer coding standard along the lines of "thou shalt > not write macros that evaluate their arguments more than once". > So the benefits here are easier development and less risk of bugs. > On the other side of the ledger, the costs are pretty minimal; we will > not actually break any platforms, at worst we'll make them unpleasant > to develop on because of lots of warning messages. We have some platforms > like that already, and it's not been a huge problem. OK, so do we want to rip out all instances of the static inline dance in favor of more straightforward coding? Do we then shut pandemelon and any other affected buildfarm members down as unsupported, or what? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-08-04 15:20:14 -0400, Robert Haas wrote: > OK, so do we want to rip out all instances of the static inline dance > in favor of more straightforward coding? Do we then shut pandemelon > and any other affected buildfarm members down as unsupported, or what? I think all that happens is that they'll log a couple more warnings about defined but unused static functions. configure already defines inline away if not supported.
Andres Freund <andres@anarazel.de> writes:
> On 2015-08-04 15:20:14 -0400, Robert Haas wrote:
>> OK, so do we want to rip out all instances of the static inline dance
>> in favor of more straightforward coding? Do we then shut pandemelon
>> and any other affected buildfarm members down as unsupported, or what?
> I think all that happens is that they'll log a couple more warnings
> about defined but unused static functions. configure already defines
> inline away if not supported.
Right. We had already concluded that this would be safe to do, it's
just a matter of somebody being motivated to do it.
I'm not sure that there's any great urgency about changing the instances
that exist now; the real point of this discussion is that we will allow
new code to use static inlines in headers.
regards, tom lane
On 2015-08-04 15:45:44 -0400, Tom Lane wrote: > I'm not sure that there's any great urgency about changing the instances > that exist now; the real point of this discussion is that we will allow > new code to use static inlines in headers. I agree that we don't have to (and probably shouldn't) make the required configure changes and change definitions. But I do think some of the current macro usage deserves to be cleaned up at some point. I, somewhere during 9.4 or 9.5, redefined some of the larger macros into static inlines and it both reduced the binary size and gave minor performance benefits. - Andres
On Tue, Aug 4, 2015 at 3:55 PM, Andres Freund <andres@anarazel.de> wrote: > On 2015-08-04 15:45:44 -0400, Tom Lane wrote: >> I'm not sure that there's any great urgency about changing the instances >> that exist now; the real point of this discussion is that we will allow >> new code to use static inlines in headers. > > I agree that we don't have to (and probably shouldn't) make the required > configure changes and change definitions. But I do think some of the > current macro usage deserves to be cleaned up at some point. I, > somewhere during 9.4 or 9.5, redefined some of the larger macros into > static inlines and it both reduced the binary size and gave minor > performance benefits. We typically recommend that people write their new code like the existing code. If we say that the standards for new code are now different from old code in this one way, I don't think that's going to be very helpful to anyone. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-08-04 16:55:12 -0400, Robert Haas wrote: > On Tue, Aug 4, 2015 at 3:55 PM, Andres Freund <andres@anarazel.de> wrote: > > On 2015-08-04 15:45:44 -0400, Tom Lane wrote: > >> I'm not sure that there's any great urgency about changing the instances > >> that exist now; the real point of this discussion is that we will allow > >> new code to use static inlines in headers. > > > > I agree that we don't have to (and probably shouldn't) make the required > > configure changes and change definitions. But I do think some of the > > current macro usage deserves to be cleaned up at some point. I, > > somewhere during 9.4 or 9.5, redefined some of the larger macros into > > static inlines and it both reduced the binary size and gave minor > > performance benefits. > > We typically recommend that people write their new code like the > existing code. If we say that the standards for new code are now > different from old code in this one way, I don't think that's going to > be very helpful to anyone. I'm coming around to actually changing more code initially. While I don't particularly buy the severity of the "make it look like existing code" issue, I do think it'll be rather confusing to have code dependent on PG_USE_INLINE when it's statically defined. So unless somebody protests I'm going to prepare (and commit after posting) a patch to rip out the bits of code that currently depend on PG_USE_INLINE. Greetings, Andres Freund
On 2015-08-05 14:05:24 +0200, Andres Freund wrote: > So unless somebody protests I'm going to prepare (and commit after > posting) a patch to rip out the bits of code that currently depend on > PG_USE_INLINE. Here's that patch. I only removed code dependant on PG_USE_INLINE. We might later want to change some of the harder to maintain macros to inline functions, but that seems better done separately. Regards, Andres
Attachment
On 2015-08-05 15:08:29 +0200, Andres Freund wrote: > We might later want to change some of the harder to maintain macros to > inline functions, but that seems better done separately. Here's a conversion for fastgetattr() and heap_getattr(). Not only is the resulting code significantly more readable, but the conversion also shrinks the code size: $ ls -l src/backend/postgres_stock src/backend/postgres -rwxr-xr-x 1 andres andres 37054832 Aug 5 15:18 src/backend/postgres_stock -rwxr-xr-x 1 andres andres 37209288 Aug 5 15:23 src/backend/postgres $ size src/backend/postgres_stock src/backend/postgres text data bss dec hex filename 7026843 49982 298584 7375409 708a31 src/backend/postgres_stock 7023851 49982 298584 7372417 707e81 src/backend/postgres stock is the binary compiled without the conversion. A lot of the size difference is debugging information (which now needs less duplicated information on each callsite), but you can see that the text section (the actual executable code) shrank by 3k. stripping the binary shows exactly that: -rwxr-xr-x 1 andres andres 7076760 Aug 5 15:44 src/backend/postgres_s -rwxr-xr-x 1 andres andres 7079512 Aug 5 15:45 src/backend/postgres_stock_s To be sure this doesn't cause problems I ran a readonly pgbench. There's a very slight, but reproducible, performance benefit. I don't think that's a reason for the change, I just wanted to make sure there's no regressions. Regards, Andres
Attachment
Andres Freund <andres@anarazel.de> writes:
> On 2015-08-05 14:05:24 +0200, Andres Freund wrote:
>> So unless somebody protests I'm going to prepare (and commit after
>> posting) a patch to rip out the bits of code that currently depend on
>> PG_USE_INLINE.
> Here's that patch. I only removed code dependant on PG_USE_INLINE. We
> might later want to change some of the harder to maintain macros to
> inline functions, but that seems better done separately.
Hmm. I notice that this removes Noah's hack from commit c53f73879f552a3c.
Do we care about breaking old versions of xlc, and if so, how are we going
to fix that? (I assume it should be possible to override AC_C_INLINE's
result, but I'm not sure where would be a good place to do so.)
regards, tom lane
On 2015-08-05 10:08:10 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2015-08-05 14:05:24 +0200, Andres Freund wrote: > >> So unless somebody protests I'm going to prepare (and commit after > >> posting) a patch to rip out the bits of code that currently depend on > >> PG_USE_INLINE. > > > Here's that patch. I only removed code dependant on PG_USE_INLINE. We > > might later want to change some of the harder to maintain macros to > > inline functions, but that seems better done separately. > > Hmm. I notice that this removes Noah's hack from commit c53f73879f552a3c. > Do we care about breaking old versions of xlc, and if so, how are we going > to fix that? (I assume it should be possible to override AC_C_INLINE's > result, but I'm not sure where would be a good place to do so.) Hm. That's a good point. How about moving that error check into into the aix template file and erroring out there? Since this is master I think it's perfectly fine to refuse to work with the buggy unsupported 32 bit compiler. The argument not to do so was that PG previously worked in the back branches depending on the minor version, but that's not an argument on master.
Andres Freund <andres@anarazel.de> writes:
> On 2015-08-05 10:08:10 -0400, Tom Lane wrote:
>> Hmm. I notice that this removes Noah's hack from commit c53f73879f552a3c.
>> Do we care about breaking old versions of xlc, and if so, how are we going
>> to fix that? (I assume it should be possible to override AC_C_INLINE's
>> result, but I'm not sure where would be a good place to do so.)
> Hm. That's a good point.
> How about moving that error check into into the aix template file and
> erroring out there? Since this is master I think it's perfectly fine to
> refuse to work with the buggy unsupported 32 bit compiler. The argument
> not to do so was that PG previously worked in the back branches
> depending on the minor version, but that's not an argument on master.
The check as Noah wrote it rejects *all* 32-bit IBM compilers, not just
buggy ones. That was okay when the effect was only a rather minor
performance loss, but refusing to build at all would raise the stakes
quite a lot. Unless you are volunteering to find out how to tell broken
compilers from fixed ones more accurately, I think you need to confine
the effects of the check to disabling inlining.
regards, tom lane
On 2015-08-05 10:23:31 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > How about moving that error check into into the aix template file and > > erroring out there? Since this is master I think it's perfectly fine to > > refuse to work with the buggy unsupported 32 bit compiler. The argument > > not to do so was that PG previously worked in the back branches > > depending on the minor version, but that's not an argument on master. > > The check as Noah wrote it rejects *all* 32-bit IBM compilers, not just > buggy ones. That was okay when the effect was only a rather minor > performance loss, but refusing to build at all would raise the stakes > quite a lot. Unless you are volunteering to find out how to tell broken > compilers from fixed ones more accurately, I think you need to confine > the effects of the check to disabling inlining. Wasn't the point that 32 bit AIX as a whole hasn't been supported for a couple years now? My willingness to expend effort for that is rather limited. I mean I'd otherwise ok with a PG_FORCE_DISABLE_INLINE flag that takes effect in c.h or so. That could easily be set in src/template/aix. Might also be useful for investigatory purposes every couple years or so. Andres
Andres Freund <andres@anarazel.de> writes:
> Wasn't the point that 32 bit AIX as a whole hasn't been supported for a
> couple years now? My willingness to expend effort for that is rather
> limited.
Well, there's one in the buildfarm. We don't generally turn off
buildfarm critters just because the underlying OS is out of support
(the population would be quite a bit lower if we did).
> I mean I'd otherwise ok with a PG_FORCE_DISABLE_INLINE flag that takes
> effect in c.h or so. That could easily be set in src/template/aix. Might
> also be useful for investigatory purposes every couple years or so.
+1, that could have other use-cases.
regards, tom lane
On 2015-08-05 10:32:48 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Wasn't the point that 32 bit AIX as a whole hasn't been supported for a > > couple years now? My willingness to expend effort for that is rather > > limited. > > Well, there's one in the buildfarm. Oh nice. That's new. I see it has been added less than a week ago ;) > We don't generally turn off buildfarm critters just because the > underlying OS is out of support (the population would be quite a bit > lower if we did). I didn't know there was a buildfarm animal. We'd been pleading a bunch of people over the last few years to add one. If there's an actual way to see platforms breaking I'm more accepting to try and keep them alive. > > I mean I'd otherwise ok with a PG_FORCE_DISABLE_INLINE flag that takes > > effect in c.h or so. That could easily be set in src/template/aix. Might > > also be useful for investigatory purposes every couple years or so. > > +1, that could have other use-cases. Ok, lets' do it that way then. It seems the easiest way to test for this is to use something like # "IBM XL C/C++ for AIX, V12.1" miscompiles, for 32-bit, some inline # expansions of ginCompareItemPointers() "long long" arithmetic. To # take advantage of inlining, build a 64-bit PostgreSQL. test $(getconf HARDWARE_BITMODE) == '32' then CFLAGS="$CFLAGS -DPG_FORCE_DISABLE_INLINE" fi in the xlc part of the template? do we want to add something like $as_echo "$as_me: WARNING: disabling inlining on 32 bit aix due to compiler bugs" ? Seems like a good idea to me. Andres
Andres Freund <andres@anarazel.de> writes:
> Ok, lets' do it that way then. It seems the easiest way to test for this
> is to use something like
> # "IBM XL C/C++ for AIX, V12.1" miscompiles, for 32-bit, some inline
> # expansions of ginCompareItemPointers() "long long" arithmetic. To
> # take advantage of inlining, build a 64-bit PostgreSQL.
> test $(getconf HARDWARE_BITMODE) == '32' then
> CFLAGS="$CFLAGS -DPG_FORCE_DISABLE_INLINE"
> fi
> in the xlc part of the template?
Actually, much the easiest way to convert what Noah did would be to add
#if defined(__ILP32__) && defined(__IBMC__)
#define PG_FORCE_DISABLE_INLINE
#endif
in src/include/port/aix.h.
regards, tom lane
On 2015-08-05 11:12:34 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Ok, lets' do it that way then. It seems the easiest way to test for this > > is to use something like > > > # "IBM XL C/C++ for AIX, V12.1" miscompiles, for 32-bit, some inline > > # expansions of ginCompareItemPointers() "long long" arithmetic. To > > # take advantage of inlining, build a 64-bit PostgreSQL. > > test $(getconf HARDWARE_BITMODE) == '32' then > > CFLAGS="$CFLAGS -DPG_FORCE_DISABLE_INLINE" > > fi > > > in the xlc part of the template? (there's a ; missing and it should be CPPFLAGS rather than CFLAGS) > Actually, much the easiest way to convert what Noah did would be to add > > #if defined(__ILP32__) && defined(__IBMC__) > #define PG_FORCE_DISABLE_INLINE > #endif > > in src/include/port/aix.h. I'm ok with that too, although I do like the warning at configure time. I'd go with the template approach due to that, but I don't feel strongly about it. Andres
Andres Freund <andres@anarazel.de> writes:
> I'm ok with that too, although I do like the warning at configure
> time. I'd go with the template approach due to that, but I don't feel
> strongly about it.
Meh. I did *not* like the way you proposed doing that: it looked far too
dependent on autoconf internals (the kind that they change regularly).
If you can think of a way of emitting a warning that isn't going to break
in a future autoconf release, then ok.
regards, tom lane
On 2015-08-05 11:23:22 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > I'm ok with that too, although I do like the warning at configure > > time. I'd go with the template approach due to that, but I don't feel > > strongly about it. > > Meh. I did *not* like the way you proposed doing that: it looked far too > dependent on autoconf internals (the kind that they change regularly). Hm, I'd actually checked that as_echo worked back till 9.0. But it doesn't exist in much older releases. > If you can think of a way of emitting a warning that isn't going to break > in a future autoconf release, then ok. echo "$as_me: WARNING: disabling inlining on 32 bit aix due to a bug in xlc" 2>&1 then. That'd have worked back in 7.4 and the worst that'd happen is that $as_me is empty.
On 2015-08-05 17:19:05 +0200, Andres Freund wrote: > On 2015-08-05 11:12:34 -0400, Tom Lane wrote: > > Andres Freund <andres@anarazel.de> writes: > > > Ok, lets' do it that way then. It seems the easiest way to test for this > > > is to use something like > > > > > # "IBM XL C/C++ for AIX, V12.1" miscompiles, for 32-bit, some inline > > > # expansions of ginCompareItemPointers() "long long" arithmetic. To > > > # take advantage of inlining, build a 64-bit PostgreSQL. > > > test $(getconf HARDWARE_BITMODE) == '32' then > > > CFLAGS="$CFLAGS -DPG_FORCE_DISABLE_INLINE" > > > fi So that approach doesn't work out well because the 32 bit xlc can be installed on the 64 bit system. > > Actually, much the easiest way to convert what Noah did would be to add > > > > #if defined(__ILP32__) && defined(__IBMC__) > > #define PG_FORCE_DISABLE_INLINE > > #endif > > > > in src/include/port/aix.h. Therefore I'm going to reshuffle things in that direction tomorrow. I'll wait for other fallout first though. So far only gcc, xlc and clang (via gcc frontend) have run... Greetings, Andres Freund
On Wed, Aug 05, 2015 at 10:32:48AM -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Wasn't the point that 32 bit AIX as a whole hasn't been supported for a > > couple years now? My willingness to expend effort for that is rather > > limited. > > Well, there's one in the buildfarm. We don't generally turn off > buildfarm critters just because the underlying OS is out of support > (the population would be quite a bit lower if we did). For the record, mandrill's OS and compiler are both in support and not so old (June 2012 compiler, February 2013 OS). The last 32-bit AIX *kernel* did exit support in 2012, but 32-bit executables remain the norm.
Andres Freund <andres@anarazel.de> writes:
> ... I'm going to reshuffle things in that direction tomorrow. I'll
> wait for other fallout first though. So far only gcc, xlc and clang (via
> gcc frontend) have run...
In the department of "other fallout", pademelon is not happy:
cc -Ae -g +O0 -Wp,-H16384 -I../../../src/include -D_XOPEN_SOURCE_EXTENDED -I/usr/local/libxml2-2.6.23/include/libxml2
-I/usr/local/include -c -o pg_resetxlog.o pg_resetxlog.c
cc -Ae -g +O0 -Wp,-H16384 pg_resetxlog.o -L../../../src/port -L../../../src/common -L/usr/local/libxml2-2.6.23/lib
-L/usr/local/lib-Wl,+b -Wl,'/home/bfarm/bf-data/HEAD/inst/lib' -Wl,-z -lpgcommon -lpgport -lxnet -lxml2 -lz -lreadline
-ltermcap-lm -o pg_resetxlog
/usr/ccs/bin/ld: Unsatisfied symbols: pg_atomic_clear_flag_impl (code) pg_atomic_init_flag_impl (code)
pg_atomic_compare_exchange_u32_impl(code) pg_atomic_fetch_add_u32_impl (code) pg_atomic_test_set_flag_impl (code)
pg_atomic_init_u32_impl(code)
make[3]: *** [pg_resetxlog] Error 1
I'd have thought that port/atomics.c would be included in libpgport, but
it seems not. (But is pademelon really the only buildfarm critter relying
on the "fallback" atomics implementation?)
Another possible angle is: what the heck does pg_resetxlog need with
atomic ops, anyway? Should these things have a different, stub
implementation for FRONTEND code?
regards, tom lane
On 2015-08-05 23:18:08 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > ... I'm going to reshuffle things in that direction tomorrow. I'll > > wait for other fallout first though. So far only gcc, xlc and clang (via > > gcc frontend) have run... > > In the department of "other fallout", pademelon is not happy: > > cc -Ae -g +O0 -Wp,-H16384 -I../../../src/include -D_XOPEN_SOURCE_EXTENDED -I/usr/local/libxml2-2.6.23/include/libxml2-I/usr/local/include -c -o pg_resetxlog.o pg_resetxlog.c > cc -Ae -g +O0 -Wp,-H16384 pg_resetxlog.o -L../../../src/port -L../../../src/common -L/usr/local/libxml2-2.6.23/lib -L/usr/local/lib-Wl,+b -Wl,'/home/bfarm/bf-data/HEAD/inst/lib' -Wl,-z -lpgcommon -lpgport -lxnet -lxml2 -lz -lreadline -ltermcap-lm -o pg_resetxlog > /usr/ccs/bin/ld: Unsatisfied symbols: > pg_atomic_clear_flag_impl (code) > pg_atomic_init_flag_impl (code) > pg_atomic_compare_exchange_u32_impl (code) > pg_atomic_fetch_add_u32_impl (code) > pg_atomic_test_set_flag_impl (code) > pg_atomic_init_u32_impl (code) > make[3]: *** [pg_resetxlog] Error 1 > > I'd have thought that port/atomics.c would be included in libpgport, but > it seems not. Given that it requires spinlocks for the fallback, which in turn may require semaphores, that didn't seem like a good idea. Thus atomics.c is in src/backend/port not src/port (just like other locking stuff like spinlocks, semaphores etc). > (But is pademelon really the only buildfarm critter relying > on the "fallback" atomics implementation?) I don't think so. At least that didn't use to be the case. My guess is that less ancient compilers don't emit code for unreferenced functions and this thus doesn't show up. > Another possible angle is: what the heck does pg_resetxlog need with > atomic ops, anyway? It really doesn't. It's just fallout from indirectly including lwlock.h which includes an atomic variable. The include path leading to it is In file included from /home/andres/src/postgresql/src/include/storage/lwlock.h:19:0, from /home/andres/src/postgresql/src/include/storage/lock.h:18, from /home/andres/src/postgresql/src/include/access/tuptoaster.h:18, from /home/andres/src/postgresql/src/bin/pg_resetxlog/pg_resetxlog.c:49: /home/andres/src/postgresql/src/include/port/atomics.h:41:2: error: #error "THOU SHALL NOT REQUIRE ATOMICS"#error "THOU SHALLNOT REQUIRE ATOMICS" There's other path's (slot.h) as well if that were fixed. > Should these things have a different, stub implementation for FRONTEND > code? I'm honestly not yet sure what the best approach here would be. One approach is to avoid including lwlock.h/slot.h in frontend code. That'll require some minor surgery and adding a couple includes, but it doesn't look that bad. Greetings, Andres Freund
On 2015-08-06 09:09:02 +0200, Andres Freund wrote: > It really doesn't. It's just fallout from indirectly including lwlock.h > which includes an atomic variable. The include path leading to it is > > In file included from /home/andres/src/postgresql/src/include/storage/lwlock.h:19:0, > from /home/andres/src/postgresql/src/include/storage/lock.h:18, > from /home/andres/src/postgresql/src/include/access/tuptoaster.h:18, > from /home/andres/src/postgresql/src/bin/pg_resetxlog/pg_resetxlog.c:49: > /home/andres/src/postgresql/src/include/port/atomics.h:41:2: error: #error "THOU SHALL NOT REQUIRE ATOMICS" > #error "THOU SHALL NOT REQUIRE ATOMICS" > > There's other path's (slot.h) as well if that were fixed. > > > Should these things have a different, stub implementation for FRONTEND > > code? > > I'm honestly not yet sure what the best approach here would be. > > One approach is to avoid including lwlock.h/slot.h in frontend > code. That'll require some minor surgery and adding a couple includes, > but it doesn't look that bad. Patch doing that attached. It's easy enough right now, but I'm not entirely sure how feasible it is going forward. I mean it's rather good if frontends do not end up including s_lock.h, lwlock.h, atomics.h and such. But if some more code ends up using lwlocks inline instead of referencing them that might get harder. On the other hand no code doing that has business being included by frontend code. In the end I think that this separation is worth some pain. As a consequence I think we should actually add a bunch of #ifdef FRONTEND #error checks in code we definitely do not want to included in frontend code. The attached patch so far adds a check to atomics.h, lwlock.h and s_lock.h. Before ea9df812d - "Relax the requirement that all lwlocks be stored in a single array." it was kinda ok that lock.h includes lwlock.h since that didn't expose its implementation details much. But after that it started to need s_lock.h exposed (and now atomics.h as well). I think that changed the game somewhat. Comments? - Andres
Attachment
Andres Freund <andres@anarazel.de> writes:
>> One approach is to avoid including lwlock.h/slot.h in frontend
>> code. That'll require some minor surgery and adding a couple includes,
>> but it doesn't look that bad.
> Patch doing that attached.
This seems kinda messy. Looking at the contents of lock.h, it seems like
getting rid of its dependency on lwlock.h is not really very appropriate,
because there is boatloads of other backend-only stuff in there. Why is
any frontend code including lock.h at all? If there is a valid reason,
should we refactor lock.h into two separate headers, one that is safe to
expose to frontends and one with the rest of the stuff?
Also, I think the reason for the #include is that lock.h has a couple
of macros that reference MainLWLockArray. The fact that you can omit
the #include lwlock.h is therefore only accidental: if we had done those
as static inline functions, this wouldn't work at all. So I think this
solution is not very future-proof either.
> As a consequence I think we should actually add a bunch of #ifdef
> FRONTEND #error checks in code we definitely do not want to included in
> frontend code. The attached patch so far adds a check to atomics.h,
> lwlock.h and s_lock.h.
I agree with that idea anyway.
regards, tom lane
On Thu, Aug 6, 2015 at 3:09 AM, Andres Freund <andres@anarazel.de> wrote: > It really doesn't. It's just fallout from indirectly including lwlock.h > which includes an atomic variable. The include path leading to it is > > In file included from /home/andres/src/postgresql/src/include/storage/lwlock.h:19:0, > from /home/andres/src/postgresql/src/include/storage/lock.h:18, > from /home/andres/src/postgresql/src/include/access/tuptoaster.h:18, > from /home/andres/src/postgresql/src/bin/pg_resetxlog/pg_resetxlog.c:49: > /home/andres/src/postgresql/src/include/port/atomics.h:41:2: error: #error "THOU SHALL NOT REQUIRE ATOMICS" > #error "THOU SHALL NOT REQUIRE ATOMICS" Isn't that #include entirely superfluous? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-08-06 10:29:39 -0400, Robert Haas wrote: > On Thu, Aug 6, 2015 at 3:09 AM, Andres Freund <andres@anarazel.de> wrote: > > It really doesn't. It's just fallout from indirectly including lwlock.h > > which includes an atomic variable. The include path leading to it is > > > > In file included from /home/andres/src/postgresql/src/include/storage/lwlock.h:19:0, > > from /home/andres/src/postgresql/src/include/storage/lock.h:18, > > from /home/andres/src/postgresql/src/include/access/tuptoaster.h:18, > > from /home/andres/src/postgresql/src/bin/pg_resetxlog/pg_resetxlog.c:49: > > /home/andres/src/postgresql/src/include/port/atomics.h:41:2: error: #error "THOU SHALL NOT REQUIRE ATOMICS" > > #error "THOU SHALL NOT REQUIRE ATOMICS" > > Isn't that #include entirely superfluous? Which one?
On 2015-08-06 10:27:52 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > >> One approach is to avoid including lwlock.h/slot.h in frontend > >> code. That'll require some minor surgery and adding a couple includes, > >> but it doesn't look that bad. > > > Patch doing that attached. > > This seems kinda messy. Looking at the contents of lock.h, it seems like > getting rid of its dependency on lwlock.h is not really very appropriate, > because there is boatloads of other backend-only stuff in there. Why is > any frontend code including lock.h at all? If there is a valid reason, > should we refactor lock.h into two separate headers, one that is safe to > expose to frontends and one with the rest of the stuff? I think the primary reason for lock.h being included pretty widely is to have the declaration of LOCKMODE. That's pretty widely used in headers included by clients for various reasons. There's also a bit of fun around xl_standby_locks. I can have a go at trying to separate them, but I'm not convinced it's going to look particularly pretty. > Also, I think the reason for the #include is that lock.h has a couple > of macros that reference MainLWLockArray. The fact that you can omit > the #include lwlock.h is therefore only accidental: if we had done those > as static inline functions, this wouldn't work at all. So I think this > solution is not very future-proof either. Yea, I saw that and concluded I'm not particularly bothered by it. Most of the other similar macros are in lwlock.h itself, and if it became a problem we could just move them alongside. Greetings, Andres Freund
On Thu, Aug 6, 2015 at 10:31 AM, Andres Freund <andres@anarazel.de> wrote: > On 2015-08-06 10:29:39 -0400, Robert Haas wrote: >> On Thu, Aug 6, 2015 at 3:09 AM, Andres Freund <andres@anarazel.de> wrote: >> > It really doesn't. It's just fallout from indirectly including lwlock.h >> > which includes an atomic variable. The include path leading to it is >> > >> > In file included from /home/andres/src/postgresql/src/include/storage/lwlock.h:19:0, >> > from /home/andres/src/postgresql/src/include/storage/lock.h:18, >> > from /home/andres/src/postgresql/src/include/access/tuptoaster.h:18, >> > from /home/andres/src/postgresql/src/bin/pg_resetxlog/pg_resetxlog.c:49: >> > /home/andres/src/postgresql/src/include/port/atomics.h:41:2: error: #error "THOU SHALL NOT REQUIRE ATOMICS" >> > #error "THOU SHALL NOT REQUIRE ATOMICS" >> >> Isn't that #include entirely superfluous? > > Which one? Never mind, I'm confused. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Andres Freund wrote: > On 2015-08-06 10:27:52 -0400, Tom Lane wrote: > > Andres Freund <andres@anarazel.de> writes: > > >> One approach is to avoid including lwlock.h/slot.h in frontend > > >> code. That'll require some minor surgery and adding a couple includes, > > >> but it doesn't look that bad. > > > > > Patch doing that attached. > > > > This seems kinda messy. Looking at the contents of lock.h, it seems like > > getting rid of its dependency on lwlock.h is not really very appropriate, > > because there is boatloads of other backend-only stuff in there. Why is > > any frontend code including lock.h at all? If there is a valid reason, > > should we refactor lock.h into two separate headers, one that is safe to > > expose to frontends and one with the rest of the stuff? > > I think the primary reason for lock.h being included pretty widely is to > have the declaration of LOCKMODE. That's pretty widely used in headers > included by clients for various reasons. There's also a bit of fun > around xl_standby_locks. I think it is a good idea to split up LOCKMODE so that most headers do not need to include lock.h at all; you will need to add an explicit #include "storage/lock.h" to a lot of C files, but to me that's a good thing. See http://doxygen.postgresql.org/lock_8h.html Funnily enough, the "included by" graph is so large that my browser (arguably a bit dated) fails to display it and shows a black rectangle instead. Chromium shows it, though it's illegible. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2015-08-06 12:05:24 -0300, Alvaro Herrera wrote: > Andres Freund wrote: > > On 2015-08-06 10:27:52 -0400, Tom Lane wrote: > > > Andres Freund <andres@anarazel.de> writes: > > > >> One approach is to avoid including lwlock.h/slot.h in frontend > > > >> code. That'll require some minor surgery and adding a couple includes, > > > >> but it doesn't look that bad. > > > > > > > Patch doing that attached. > > > > > > This seems kinda messy. Looking at the contents of lock.h, it seems like > > > getting rid of its dependency on lwlock.h is not really very appropriate, > > > because there is boatloads of other backend-only stuff in there. Why is > > > any frontend code including lock.h at all? If there is a valid reason, > > > should we refactor lock.h into two separate headers, one that is safe to > > > expose to frontends and one with the rest of the stuff? > > > > I think the primary reason for lock.h being included pretty widely is to > > have the declaration of LOCKMODE. That's pretty widely used in headers > > included by clients for various reasons. There's also a bit of fun > > around xl_standby_locks. > > I think it is a good idea to split up LOCKMODE so that most headers do > not need to include lock.h at all; you will need to add an explicit > #include "storage/lock.h" to a lot of C files, but to me that's a good > thing. I had to split of three things: LOCKMASK, the individual lock levels and xl_standby_lock to be able to prohibit lock.h to be included by frontend code. lockdefs.h works for me, counter proposals? There weren't any places that needed additional lock.h includes. But hashfn.c somewhat hilariously missed utils/hsearch.h ;) Regards, Andres
Attachment
Andres Freund wrote: > I had to split of three things: LOCKMASK, the individual lock levels and > xl_standby_lock to be able to prohibit lock.h to be included by frontend > code. lockdefs.h works for me, counter proposals? > > There weren't any places that needed additional lock.h includes. Ah, but that's because you cheated and didn't remove the include from namespace.h ... > But hashfn.c somewhat hilariously missed utils/hsearch.h ;) hah. > diff --git a/src/include/storage/lockdefs.h b/src/include/storage/lockdefs.h > new file mode 100644 > index 0000000..bfbcdba > --- /dev/null > +++ b/src/include/storage/lockdefs.h > @@ -0,0 +1,56 @@ > +/*------------------------------------------------------------------------- > + * > + * lockdefs.h > + * Frontend exposed parts of postgres' low level lock mechanism > + * > + * The split between lockdefs.h and lock.h is not very principled. No kidding! -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote: > Andres Freund wrote: > > > I had to split of three things: LOCKMASK, the individual lock levels and > > xl_standby_lock to be able to prohibit lock.h to be included by frontend > > code. lockdefs.h works for me, counter proposals? > > > > There weren't any places that needed additional lock.h includes. > > Ah, but that's because you cheated and didn't remove the include from > namespace.h ... Well, it's not included from frontend code, so I didn't see the need? Going through all the backend code and replacing lock.h by lockdefs.h and some other includes doesn't seem particularly beneficial to me. FWIW, removing it from namespace.h is relatively easy. It starts to get a lot more noisy when you want to touch heapam.h. > > diff --git a/src/include/storage/lockdefs.h b/src/include/storage/lockdefs.h > > new file mode 100644 > > index 0000000..bfbcdba > > --- /dev/null > > +++ b/src/include/storage/lockdefs.h > > @@ -0,0 +1,56 @@ > > +/*------------------------------------------------------------------------- > > + * > > + * lockdefs.h > > + * Frontend exposed parts of postgres' low level lock mechanism > > + * > > + * The split between lockdefs.h and lock.h is not very principled. > > No kidding! Do you have a good suggestion about the split? I wanted to expose the minimal amount necessary, and those were the ones.
Andres Freund wrote: > On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote: > > Ah, but that's because you cheated and didn't remove the include from > > namespace.h ... > > Well, it's not included from frontend code, so I didn't see the need? > Going through all the backend code and replacing lock.h by lockdefs.h > and some other includes doesn't seem particularly beneficial to me. > > FWIW, removing it from namespace.h is relatively easy. It starts to get > a lot more noisy when you want to touch heapam.h. Ah, heapam.h is the granddaddy of all header messes, isn't it. (Actually it's execnodes.h or something like that.) > > > diff --git a/src/include/storage/lockdefs.h b/src/include/storage/lockdefs.h > > > new file mode 100644 > > > index 0000000..bfbcdba > > > --- /dev/null > > > +++ b/src/include/storage/lockdefs.h > > > @@ -0,0 +1,56 @@ > > > +/*------------------------------------------------------------------------- > > > + * > > > + * lockdefs.h > > > + * Frontend exposed parts of postgres' low level lock mechanism > > > + * > > > + * The split between lockdefs.h and lock.h is not very principled. > > > > No kidding! > > Do you have a good suggestion about the split? I wanted to expose the > minimal amount necessary, and those were the ones. Nope, nothing useful, sorry. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Aug 06, 2015 at 05:34:50PM +0200, Andres Freund wrote: > On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote: > > Andres Freund wrote: > > > @@ -0,0 +1,56 @@ > > > +/*------------------------------------------------------------------------- > > > + * > > > + * lockdefs.h > > > + * Frontend exposed parts of postgres' low level lock mechanism > > > + * > > > + * The split between lockdefs.h and lock.h is not very principled. > > > > No kidding! > > Do you have a good suggestion about the split? I wanted to expose the > minimal amount necessary, and those were the ones. lock.h includes lwlock.h only for the benefit of the three LockHashPartition* macros. Those macros are pieces of the lock.c implementation that cross into proc.c, not pieces of the lock.c public API. I suggest instead making a lock_internals.h for those macros and for anything else private to the various files of the lock implementation. For example, the PROCLOCK struct and the functions that take arguments of that type would fit in lock_internals.h. With that, indirect frontend lock.h inclusion will work fine. Thanks, nm
On 2015-08-06 23:23:43 -0400, Noah Misch wrote: > On Thu, Aug 06, 2015 at 05:34:50PM +0200, Andres Freund wrote: > > On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote: > > > Andres Freund wrote: > > > > @@ -0,0 +1,56 @@ > > > > +/*------------------------------------------------------------------------- > > > > + * > > > > + * lockdefs.h > > > > + * Frontend exposed parts of postgres' low level lock mechanism > > > > + * > > > > + * The split between lockdefs.h and lock.h is not very principled. > > > > > > No kidding! > > > > Do you have a good suggestion about the split? I wanted to expose the > > minimal amount necessary, and those were the ones. > > lock.h includes lwlock.h only for the benefit of the three LockHashPartition* > macros. Those macros are pieces of the lock.c implementation that cross into > proc.c, not pieces of the lock.c public API. I argued that way as well upthread. But I do think that Tom has a good point when he argues that frontend code really has no business including lock.h in total. The only reason we need it is because a few headers we need to include have LOCKMODE parameters and/or contain macros that refer to lock levels. So I do think that having a version that doesn't expose any unnecessary details is a good idea. > With that, indirect frontend lock.h inclusion will work fine. But there seems little reason trying to support doing so. It's not very hard to imagine that lock.c and by extension lock.h get more complicated in the future as it's already a scalability bottleneck. that very well might require including atomics, spinlocks and the like in lock.h. Greetings, Andres Freund
On 2015-08-05 21:39:26 -0400, Noah Misch wrote: > On Wed, Aug 05, 2015 at 10:32:48AM -0400, Tom Lane wrote: > > Andres Freund <andres@anarazel.de> writes: > > > Wasn't the point that 32 bit AIX as a whole hasn't been supported for a > > > couple years now? My willingness to expend effort for that is rather > > > limited. > > > > Well, there's one in the buildfarm. We don't generally turn off > > buildfarm critters just because the underlying OS is out of support > > (the population would be quite a bit lower if we did). > > For the record, mandrill's OS and compiler are both in support and not so old > (June 2012 compiler, February 2013 OS). The last 32-bit AIX *kernel* did exit > support in 2012, but 32-bit executables remain the norm. Ugh, sorry, I misunderstood you then in the earlier thread. Unless you have a better idea I'll now move the detection of that case to aix.h. I rather liked being able to emit a warning about disabling inlines *once* during configuration, but it's also probably not worth a lot of effort given how few users that platform has. Greetings, Andres Freund
Andres Freund wrote: > > lock.h includes lwlock.h only for the benefit of the three LockHashPartition* > > macros. Those macros are pieces of the lock.c implementation that cross into > > proc.c, not pieces of the lock.c public API. > > I argued that way as well upthread. But I do think that Tom has a good > point when he argues that frontend code really has no business including > lock.h in total. The only reason we need it is because a few headers we > need to include have LOCKMODE parameters and/or contain macros that > refer to lock levels. So I do think that having a version that doesn't > expose any unnecessary details is a good idea. > > > With that, indirect frontend lock.h inclusion will work fine. > > But there seems little reason trying to support doing so. It's not very > hard to imagine that lock.c and by extension lock.h get more complicated > in the future as it's already a scalability bottleneck. that very well > might require including atomics, spinlocks and the like in lock.h. I don't disagree with any of your points, but nevertheless I think the split suggested by Noah is a good one (it's more principled than the one you suggest, at any rate) and perhaps it would be a good compromise to do both things in a fell swoop. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Aug 07, 2015 at 03:20:00PM +0200, Andres Freund wrote: > Unless you have a better idea I'll now move the detection of that case > to aix.h. Nope, that seemed right to me. > I rather liked being able to emit a warning about disabling inlines > *once* during configuration, but it's also probably not worth a lot of > effort given how few users that platform has. If we were precisely detecting buggy compilers, the warning would have more value; users could take it as a signal to consider upgrading or downgrading. Given the test's present coarseness, I don't see much value.
On Thu, Aug 6, 2015 at 11:34 AM, Andres Freund <andres@anarazel.de> wrote: > On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote: >> Andres Freund wrote: >> >> > I had to split of three things: LOCKMASK, the individual lock levels and >> > xl_standby_lock to be able to prohibit lock.h to be included by frontend >> > code. lockdefs.h works for me, counter proposals? >> > >> > There weren't any places that needed additional lock.h includes. >> >> Ah, but that's because you cheated and didn't remove the include from >> namespace.h ... > > Well, it's not included from frontend code, so I didn't see the need? > Going through all the backend code and replacing lock.h by lockdefs.h > and some other includes doesn't seem particularly beneficial to me. It may not be included from any IN CORE frontend code, but that is not the same thing as saying it's not included from any frontend code at all. For example, EDB has code that includes namespace.h in frontend code. That compiled before this commit; now it doesn't. It turns out the fix is pretty simple: the code in question is including namespace.h, but it doesn't need to. So I can just remove the include in this instance. But I don't think it's always going to be that simple. A significant reduction in the number of headers that can be compiled from frontend code WILL inconvenience extension authors. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-08-07 12:30:04 -0400, Robert Haas wrote: > It may not be included from any IN CORE frontend code, but that is not > the same thing as saying it's not included from any frontend code at > all. For example, EDB has code that includes namespace.h in frontend > code. That compiled before this commit; now it doesn't. Nothing in namespace.h seems to be of any possible use for frontend code. If there were possible use-cases I'd be inclined to agree, but you obvoiusly can't use any of the functions, the structs and the guc make no sense either. So I really don't why we should cater for that? I think the likelihood of actually breaking correct working extension code that uses namespace.h that'd be broken if we removed lock.h from namespace.h is an order of magnitude bigger than the possible impact on frontend code. Greetings, Andres Freund
On 2015-08-07 19:11:52 +0200, Andres Freund wrote: > I think the likelihood of actually breaking correct working extension > code that uses namespace.h that'd be broken if we removed lock.h from > namespace.h is an order of magnitude bigger than the possible impact on > frontend code. Same with dropping lwlock.h from lock.h. I tried both and the former required more than 20 added headers in backend code, and the latter about 5. I'm pretty sure the same would be true external extensions.
On Fri, Aug 7, 2015 at 1:11 PM, Andres Freund <andres@anarazel.de> wrote: > On 2015-08-07 12:30:04 -0400, Robert Haas wrote: >> It may not be included from any IN CORE frontend code, but that is not >> the same thing as saying it's not included from any frontend code at >> all. For example, EDB has code that includes namespace.h in frontend >> code. That compiled before this commit; now it doesn't. > > Nothing in namespace.h seems to be of any possible use for frontend > code. If there were possible use-cases I'd be inclined to agree, but you > obvoiusly can't use any of the functions, the structs and the guc make > no sense either. So I really don't why we should cater for that? > > I think the likelihood of actually breaking correct working extension > code that uses namespace.h that'd be broken if we removed lock.h from > namespace.h is an order of magnitude bigger than the possible impact on > frontend code. Well, I just work here, but it seems silly to me to reorgnize the headers so that you can include fewer definitions where necessary, but then not revise the existing headers to use the slimmed-down versions where possible. Yeah, somebody might have to adjust their #includes and that is annoying, but I don't think the cost of your new #error directives is going to be zero. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-08-07 14:15:58 -0400, Robert Haas wrote: > On Fri, Aug 7, 2015 at 1:11 PM, Andres Freund <andres@anarazel.de> wrote: > > On 2015-08-07 12:30:04 -0400, Robert Haas wrote: > >> It may not be included from any IN CORE frontend code, but that is not > >> the same thing as saying it's not included from any frontend code at > >> all. For example, EDB has code that includes namespace.h in frontend > >> code. That compiled before this commit; now it doesn't. > > > > Nothing in namespace.h seems to be of any possible use for frontend > > code. If there were possible use-cases I'd be inclined to agree, but you > > obvoiusly can't use any of the functions, the structs and the guc make > > no sense either. So I really don't why we should cater for that? > > > > I think the likelihood of actually breaking correct working extension > > code that uses namespace.h that'd be broken if we removed lock.h from > > namespace.h is an order of magnitude bigger than the possible impact on > > frontend code. > > Well, I just work here, but it seems silly to me to reorgnize the > headers so that you can include fewer definitions where necessary, but > then not revise the existing headers to use the slimmed-down versions > where possible. Yeah, somebody might have to adjust their #includes > and that is annoying, but I don't think the cost of your new #error > directives is going to be zero. So first your argument was that it broke stuff and now you want to break more? I am not really against removing removing lock.h from a few more places, but normally you were the one arguing against breaking working code by reorganizing headers when there's no really clear win. Removing lock.h from namespace.h and removing lwlock.h from lock.h will have a noticeably higher cost than what I committed and in contrast to the benefit of separating frontend code from backend implementation details (which caused linker errors) the only benefit will be a bit less code included. Greetings, Andres Freund
On Fri, Aug 7, 2015 at 2:27 PM, Andres Freund <andres@anarazel.de> wrote: > On 2015-08-07 14:15:58 -0400, Robert Haas wrote: >> On Fri, Aug 7, 2015 at 1:11 PM, Andres Freund <andres@anarazel.de> wrote: >> > On 2015-08-07 12:30:04 -0400, Robert Haas wrote: >> >> It may not be included from any IN CORE frontend code, but that is not >> >> the same thing as saying it's not included from any frontend code at >> >> all. For example, EDB has code that includes namespace.h in frontend >> >> code. That compiled before this commit; now it doesn't. >> > >> > Nothing in namespace.h seems to be of any possible use for frontend >> > code. If there were possible use-cases I'd be inclined to agree, but you >> > obvoiusly can't use any of the functions, the structs and the guc make >> > no sense either. So I really don't why we should cater for that? >> > >> > I think the likelihood of actually breaking correct working extension >> > code that uses namespace.h that'd be broken if we removed lock.h from >> > namespace.h is an order of magnitude bigger than the possible impact on >> > frontend code. >> >> Well, I just work here, but it seems silly to me to reorgnize the >> headers so that you can include fewer definitions where necessary, but >> then not revise the existing headers to use the slimmed-down versions >> where possible. Yeah, somebody might have to adjust their #includes >> and that is annoying, but I don't think the cost of your new #error >> directives is going to be zero. > > So first your argument was that it broke stuff and now you want to break > more? > > I am not really against removing removing lock.h from a few more places, > but normally you were the one arguing against breaking working code by > reorganizing headers when there's no really clear win. Removing lock.h > from namespace.h and removing lwlock.h from lock.h will have a > noticeably higher cost than what I committed and in contrast to the > benefit of separating frontend code from backend implementation details > (which caused linker errors) the only benefit will be a bit less code > included. Well, we can wait and see if we get any more complaints before doing anything, if you like. We've got a year before any of this is going to be released, so there's no rush. My point, which I think is valid, is that if the set of #includes you need to compile your stuff changes, that's easy to fix. If one of the #includes you need to compile your stuff starts doing #error, you're hosed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> Well, I just work here, but it seems silly to me to reorgnize the
> headers so that you can include fewer definitions where necessary, but
> then not revise the existing headers to use the slimmed-down versions
> where possible. Yeah, somebody might have to adjust their #includes
> and that is annoying, but I don't think the cost of your new #error
> directives is going to be zero.
I'm a bit concerned about that too; what it means is that any addition
of new #includes in backend header files carries a nontrivial risk of
breaking frontend code that used to be fine (at least on most platforms).
As an example, the proximate cause of the pademelon breakage was that
pg_resetxlog needs to #include tuptoaster.h to get TOAST_MAX_CHUNK_SIZE.
That was perfectly safe up till commit 2ef085d0e6960f50, when somebody
semi-randomly decided that it'd be a good idea to declare a function
taking a LOCKMODE argument in that header.
Eventually I think we're going to have to spend some effort on making a
clearer separation between "front end safe" and "not front end safe"
header files. Until we do that, though, adding these #error directives
may just do more harm than good. We don't know which backend headers
are being used by third-party code, but we can be 100% sure it's more
than what's used by the frontend code in the core distribution.
regards, tom lane
On 2015-08-07 14:32:35 -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > Well, I just work here, but it seems silly to me to reorgnize the > > headers so that you can include fewer definitions where necessary, but > > then not revise the existing headers to use the slimmed-down versions > > where possible. Yeah, somebody might have to adjust their #includes > > and that is annoying, but I don't think the cost of your new #error > > directives is going to be zero. > > I'm a bit concerned about that too; what it means is that any addition > of new #includes in backend header files carries a nontrivial risk of > breaking frontend code that used to be fine (at least on most platforms). > As an example, the proximate cause of the pademelon breakage was that > pg_resetxlog needs to #include tuptoaster.h to get TOAST_MAX_CHUNK_SIZE. > That was perfectly safe up till commit 2ef085d0e6960f50, when somebody > semi-randomly decided that it'd be a good idea to declare a function > taking a LOCKMODE argument in that header. > > Eventually I think we're going to have to spend some effort on making a > clearer separation between "front end safe" and "not front end safe" > header files. Until we do that, though, adding these #error directives > may just do more harm than good. We don't know which backend headers > are being used by third-party code, but we can be 100% sure it's more > than what's used by the frontend code in the core distribution. Right now the #errors are in only in places that'd also break without them, but only on the old platforms without inline support and in a more subtle way. I'm ok with getting lock.h from the list of headers where that's the case, done by removing lwlock.h from it, which I proposed that upthread. But then you objected to that approach. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2015-08-07 14:32:35 -0400, Tom Lane wrote:
>> Eventually I think we're going to have to spend some effort on making a
>> clearer separation between "front end safe" and "not front end safe"
>> header files. Until we do that, though, adding these #error directives
>> may just do more harm than good. We don't know which backend headers
>> are being used by third-party code, but we can be 100% sure it's more
>> than what's used by the frontend code in the core distribution.
> Right now the #errors are in only in places that'd also break without
> them, but only on the old platforms without inline support and in a more
> subtle way.
Indeed, but the buildfarm results say that the set of such platforms is
nearly empty. It's very likely that a lot of third-party authors won't
ever care if their code doesn't build on such platforms; certainly not
nearly as much as they'll care if their code doesn't build *at all*,
and they have no recourse except to modify the PG headers because they
need some symbol that happens to be defined in a header that also has
some lock-related junk.
My point is simply that adding those #errors represents a large bet that
the separation between frontend and backend headers is clean enough.
I really doubt that it is, and I don't see anyone volunteering to put
adequate time into fixing that right now. I'm afraid we'll put in
ugly, hurried, piecemeal hacks in response to complaints.
> I'm ok with getting lock.h from the list of headers where that's the
> case, done by removing lwlock.h from it, which I proposed that
> upthread. But then you objected to that approach.
Well, we're still discussing what's the best compromise.
regards, tom lane
On 2015-08-07 14:48:43 -0400, Tom Lane wrote:
> Indeed, but the buildfarm results say that the set of such platforms is
> nearly empty. It's very likely that a lot of third-party authors won't
> ever care if their code doesn't build on such platforms; certainly not
> nearly as much as they'll care if their code doesn't build *at all*,
> and they have no recourse except to modify the PG headers because they
> need some symbol that happens to be defined in a header that also has
> some lock-related junk.
I'm all for de-supporting platforms without working inlining support,
but if we do so, we should do it explicitly. Imo that's what it comes
down to.
It's imo more or less a happy optimization accident that apparently all
inlining supporting compilers never emit references from the contents of
static inline functions that aren't referenced.
There is one instance of code that tried to work around that:
#ifndef FRONTEND
#ifndef PG_USE_INLINE
extern MemoryContext MemoryContextSwitchTo(MemoryContext context);
#endif /* !PG_USE_INLINE */
#if defined(PG_USE_INLINE) || defined(MCXT_INCLUDE_DEFINITIONS)
STATIC_IF_INLINE MemoryContext
MemoryContextSwitchTo(MemoryContext context)
{MemoryContext old = CurrentMemoryContext;
CurrentMemoryContext = context;return old;
}
#endif /* PG_USE_INLINE || MCXT_INCLUDE_DEFINITIONS */
#endif /* FRONTEND */
You re-added the #ifndef FRONTEND there in a9baeb361d635 referencing a
buildfarm failure. It seems to originally have been added in
7507b193bc54 referencing buildfarm member warthog which unfortunately
doesn't exist anymore...
> My point is simply that adding those #errors represents a large bet that
> the separation between frontend and backend headers is clean enough.
> I really doubt that it is, and I don't see anyone volunteering to put
> adequate time into fixing that right now.
I agree that there's a lot of work needed to make that separation
cleaner. I'm not so sure these files are relevantly often needed in
frontend cdoe.
> I'm afraid we'll put in ugly, hurried, piecemeal hacks in response to
> complaints.
I think we should leave them in for now. It's the beginning of the cycle
and imo the removal of lock.h from the headers where it was referenced
was a good step. We might find some more easy cases - the removal of
lock.h from tuptoaster.h and other headers included by frontend code imo
is a good thing. If it proves to bothersome we can still take it out.
Andres
Andres Freund wrote: > You re-added the #ifndef FRONTEND there in a9baeb361d635 referencing a > buildfarm failure. It seems to originally have been added in > 7507b193bc54 referencing buildfarm member warthog which unfortunately > doesn't exist anymore... FWIW we make a point of not reusing buildfarm member names, so that this kind of history is not completely obliterated. You can still see warthog's history here: http://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=warthog&br=HEAD -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Aug 07, 2015 at 03:18:06PM +0200, Andres Freund wrote: > On 2015-08-06 23:23:43 -0400, Noah Misch wrote: > > On Thu, Aug 06, 2015 at 05:34:50PM +0200, Andres Freund wrote: > > > On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote: > > > > Andres Freund wrote: > > > > > @@ -0,0 +1,56 @@ > > > > > +/*------------------------------------------------------------------------- > > > > > + * > > > > > + * lockdefs.h > > > > > + * Frontend exposed parts of postgres' low level lock mechanism > > > > > + * > > > > > + * The split between lockdefs.h and lock.h is not very principled. > > > > > > > > No kidding! > > > > > > Do you have a good suggestion about the split? I wanted to expose the > > > minimal amount necessary, and those were the ones. > > > > lock.h includes lwlock.h only for the benefit of the three LockHashPartition* > > macros. Those macros are pieces of the lock.c implementation that cross into > > proc.c, not pieces of the lock.c public API. > > I argued that way as well upthread. But I do think that Tom has a good > point when he argues that frontend code really has no business including > lock.h in total. The only reason we need it is because a few headers we > need to include have LOCKMODE parameters and/or contain macros that > refer to lock levels. So I do think that having a version that doesn't > expose any unnecessary details is a good idea. I agree that lock.h offers little to frontend code. Headers that the lockdefs.h patch made usable in the frontend, particularly genam.h and hash.h, are no better. The lock.h/lockdefs.h unprincipled split would do more harm than letting frontends continue to pull in lock.h. If we're going to do something unprincipled, let's make it small, perhaps this: --- a/src/include/storage/lock.h +++ b/src/include/storage/lock.h @@ -17,3 +17,5 @@#include "storage/backendid.h" +#ifndef FRONTEND#include "storage/lwlock.h" +#endif#include "storage/shmem.h" On another note, I'm perplexed by the high speed commits from this thread. Commit de6fd1c landed just 191 minutes after you posted the first version of its patch. Now lockdefs.h is committed, right in the middle of discussing it.
On 2015-08-07 20:16:20 -0400, Noah Misch wrote: > I agree that lock.h offers little to frontend code. Headers that the > lockdefs.h patch made usable in the frontend, particularly genam.h and hash.h, > are no better. It's not that simple. Those two, and tuptoaster.h, are actually somewhat validly included by frontend code via the rmgr descriptor routines. > The lock.h/lockdefs.h unprincipled split would do more harm > than letting frontends continue to pull in lock.h. Why? Consider what happens when lock.h/c gets more complicated and e.g. grows some atomics. None of the frontend code should see that, and it's not much effort to keep it that way. Allowing client code to see LOCKMODE isn't something that's going to cause any harm. > On another note, I'm perplexed by the high speed commits from this thread. > Commit de6fd1c landed just 191 minutes after you posted the first version of > its patch. Now lockdefs.h is committed, right in the middle of discussing it. Hm. We'd essentially decided what we're going to do about the inline stuff weeks ago, so I don't feel particularly bad pushing it quickly. A lot of platform dependent stuff like this is going to have some iterations to deal with compilers you don't have access to. The only reason I committed the lockdefs.h split relatively quickly is that I wanted to get the buildfarm green to see wether there are other problems hiding behind the linker error. Which does, so far, not appear to be the case. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2015-08-07 20:16:20 -0400, Noah Misch wrote:
>> On another note, I'm perplexed by the high speed commits from this thread.
>> Commit de6fd1c landed just 191 minutes after you posted the first version of
>> its patch. Now lockdefs.h is committed, right in the middle of discussing it.
> Hm. We'd essentially decided what we're going to do about the inline
> stuff weeks ago, so I don't feel particularly bad pushing it quickly. A
> lot of platform dependent stuff like this is going to have some
> iterations to deal with compilers you don't have access to. The only
> reason I committed the lockdefs.h split relatively quickly is that I
> wanted to get the buildfarm green to see wether there are other problems
> hiding behind the linker error. Which does, so far, not appear to be the
> case.
FWIW, I agree with that: leaving buildfarm members red for any sustained
amount of time is a bad practice. Code cleanliness is something we can
argue about at leisure, but if you have critters that aren't building
then you don't know what might be hiding behind that. We've had bad
experiences in the past with leaving that sort of thing unfixed.
regards, tom lane
On Sat, Aug 08, 2015 at 02:30:47AM +0200, Andres Freund wrote: > On 2015-08-07 20:16:20 -0400, Noah Misch wrote: > > I agree that lock.h offers little to frontend code. Headers that the > > lockdefs.h patch made usable in the frontend, particularly genam.h and hash.h, > > are no better. > > It's not that simple. Those two, and tuptoaster.h, are actually somewhat > validly included by frontend code via the rmgr descriptor routines. genam.h is a dependency of the non-frontend-relevant content of some frontend-relevant headers, _exactly_ as lock.h has been. I count zero things in genam.h that a frontend program could harness. The frontend includes hash.h for two hashdesc.c prototypes, less than the material you moved out of lock.h for frontend benefit. Yes, it is that simple. > > The lock.h/lockdefs.h unprincipled split would do more harm > > than letting frontends continue to pull in lock.h. > > Why? Your header comment for lockdefs.h sums up the harm nicely. Additionally, the term "defs" does nothing to explain the split. "lock2.h" would be no less evocative. > Consider what happens when lock.h/c gets more complicated and > e.g. grows some atomics. None of the frontend code should see that, and > it's not much effort to keep it that way. Allowing client code to see > LOCKMODE isn't something that's going to cause any harm. Readying the headers for that day brings some value, but you added a worse mess to achieve it. The overall achievement has negative value. nm
On 2015-08-05 15:46:36 +0200, Andres Freund wrote: > On 2015-08-05 15:08:29 +0200, Andres Freund wrote: > > We might later want to change some of the harder to maintain macros to > > inline functions, but that seems better done separately. > > Here's a conversion for fastgetattr() and heap_getattr(). Not only is > the resulting code significantly more readable, but the conversion also > shrinks the code size: > > $ ls -l src/backend/postgres_stock src/backend/postgres > -rwxr-xr-x 1 andres andres 37054832 Aug 5 15:18 src/backend/postgres_stock > -rwxr-xr-x 1 andres andres 37209288 Aug 5 15:23 src/backend/postgres > > $ size src/backend/postgres_stock src/backend/postgres > text data bss dec hex filename > 7026843 49982 298584 7375409 708a31 src/backend/postgres_stock > 7023851 49982 298584 7372417 707e81 src/backend/postgres > > stock is the binary compiled without the conversion. > > A lot of the size difference is debugging information (which now needs > less duplicated information on each callsite), but you can see that the > text section (the actual executable code) shrank by 3k. > > stripping the binary shows exactly that: > -rwxr-xr-x 1 andres andres 7076760 Aug 5 15:44 src/backend/postgres_s > -rwxr-xr-x 1 andres andres 7079512 Aug 5 15:45 src/backend/postgres_stock_s > > To be sure this doesn't cause problems I ran a readonly pgbench. There's > a very slight, but reproducible, performance benefit. I don't think > that's a reason for the change, I just wanted to make sure there's no > regressions. Slightly updated version attached. The only changes are updates to some comments referencing the 'fastgetattr macro' and the like. Oh, and an additional newline. In my opinion this drastically increases readability and thus should be applied. Will do so sometime tomorrow unless there's protest. Btw, I found that many changes are much more readable when changing git's config to use histogramm diffs (git config --global diff.algorithm histogram or --histogram). Regards, Andres
Attachment
On Tue, Aug 11, 2015 at 01:04:48PM +0200, Andres Freund wrote: > On 2015-08-05 15:46:36 +0200, Andres Freund wrote: > > On 2015-08-05 15:08:29 +0200, Andres Freund wrote: > > > We might later want to change some of the harder to maintain macros to > > > inline functions, but that seems better done separately. > > > > Here's a conversion for fastgetattr() and heap_getattr() > Slightly updated version attached. > In my opinion this drastically increases readability and thus should be > applied. Will do so sometime tomorrow unless there's protest. -1 to introducing more inline functions before committable code replaces what you've already pushed for this thread.
On 2015-08-11 22:34:40 -0400, Noah Misch wrote: > On Tue, Aug 11, 2015 at 01:04:48PM +0200, Andres Freund wrote: > > On 2015-08-05 15:46:36 +0200, Andres Freund wrote: > > > On 2015-08-05 15:08:29 +0200, Andres Freund wrote: > > > > We might later want to change some of the harder to maintain macros to > > > > inline functions, but that seems better done separately. > > > > > > Here's a conversion for fastgetattr() and heap_getattr() > > > Slightly updated version attached. > > > In my opinion this drastically increases readability and thus should be > > applied. Will do so sometime tomorrow unless there's protest. > > -1 to introducing more inline functions before committable code replaces what > you've already pushed for this thread. Seriously? I've no problem with "fixing" anything. So far we have don't seem to have to come to a agreement what exactly that fix would be. Tom has stated that he doesn't want lock.h made smaller on account of frontend code including it, and you see that as the right way. Greetings, Andres Freund
On 2015-08-08 02:30:44 -0400, Noah Misch wrote: > On Sat, Aug 08, 2015 at 02:30:47AM +0200, Andres Freund wrote: > > On 2015-08-07 20:16:20 -0400, Noah Misch wrote: > > > I agree that lock.h offers little to frontend code. Headers that the > > > lockdefs.h patch made usable in the frontend, particularly genam.h and hash.h, > > > are no better. > > > > It's not that simple. Those two, and tuptoaster.h, are actually somewhat > > validly included by frontend code via the rmgr descriptor routines. > > genam.h is a dependency of the non-frontend-relevant content of some > frontend-relevant headers, _exactly_ as lock.h has been. I count zero things > in genam.h that a frontend program could harness. The frontend includes > hash.h for two hashdesc.c prototypes, less than the material you moved out of > lock.h for frontend benefit. Yes, it is that simple. The point is that it's included from various headers and that there's really no need to include lock.h from a header that just wants to declare a LOCKMODE as it's parameter. There's no other contents in lock.h afaics that fit the billet similarly. To me it's a rather worthwhile goald to want *am.h not to pull in details of the locking implementations, but they're going to have to define a LOCKMODE parameter and the values passed in. We're not entirely there, but it's not much further work. We could split some stuff in a more 'locking internals' oriented headers (PROC_QUEUE, LockMethodData and dependants, LOCKTAG and depenedants), but afaics it'd not be a proper lock_internal.h header either, because there's code like index.c that currently does SET_LOCKTAG_RELATION itself. > > > The lock.h/lockdefs.h unprincipled split would do more harm > > > than letting frontends continue to pull in lock.h. > > > > Why? > > Your header comment for lockdefs.h sums up the harm nicely. Additionally, the > term "defs" does nothing to explain the split. "lock2.h" would be no less > evocative. Oh, come on. It's not the only header that's split that way (see xlogdefs.h). Splitting away some key definitions so not all the internals are dragged in when needing just the simplest definitions is not an absurd concept. > > Consider what happens when lock.h/c gets more complicated and > > e.g. grows some atomics. None of the frontend code should see that, and > > it's not much effort to keep it that way. Allowing client code to see > > LOCKMODE isn't something that's going to cause any harm. > > Readying the headers for that day brings some value, but you added a worse > mess to achieve it. The overall achievement has negative value. I honestly can't follow. Why is it so absurd to avoid including lock.h from more code, including FRONTEND one? Or is it just the lockdefs.h split along the 'as-currently-required' line that you object to? Greetings, Andres Freund
On Tue, Aug 11, 2015 at 10:34 PM, Noah Misch <noah@leadboat.com> wrote: >> In my opinion this drastically increases readability and thus should be >> applied. Will do so sometime tomorrow unless there's protest. > > -1 to introducing more inline functions before committable code replaces what > you've already pushed for this thread. This appears to be intended as an insult, but maybe I'm misreading it. I am not thrilled with the rate at which this stuff is getting whacked around. Less-significant changes have been debated for far longer, and I really doubt that the rate at which Andres is committing changes in this area is healthy. I don't doubt that he has good intentions, though. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-08-12 13:00:25 -0400, Robert Haas wrote: > On Tue, Aug 11, 2015 at 10:34 PM, Noah Misch <noah@leadboat.com> wrote: > >> In my opinion this drastically increases readability and thus should be > >> applied. Will do so sometime tomorrow unless there's protest. > > > > -1 to introducing more inline functions before committable code replaces what > > you've already pushed for this thread. > > This appears to be intended as an insult, but maybe I'm misreading it. I read it as one. > I am not thrilled with the rate at which this stuff is getting whacked > around. Less-significant changes have been debated for far longer, > and I really doubt that the rate at which Andres is committing changes > in this area is healthy. There was one "feature" patch committed about the actual topic so far (de6fd1c), and then some fixes to handle the portability fallout and a admittedly stypid typo. We've debated the inline thing for years now and seem to have a come to a conclusion about a month ago (http://archives.postgresql.org/message-id/27127.1435791908%40sss.pgh.pa.us). You then brought up the thread again (CA+TgmoaaVfx1KVz5WBkvi1o6oZRxzF0micStTAO7gGUV5a4MwQ@mail.gmail.com) sometimes after that I started to work on a patch. Maybe I should have waited bit more to commit the initial, rather mechanical, conversion to inlines patch, although I don't see what that'd really have bought us: This kind of patch (tinkering with autoconf, portability and the like) normally triggers just about no feedback until it has caused problems. The buildfarm exists to catch such portability problems. The issue we're actually arguing about (lockdefs split) was indeed necessitated by a portability issue (30786.1438831088@sss.pgh.pa.us). One that actually has existed at least since atomics went in - it just happens that most (but not all, c.f. a9baeb361d and 7507b19) compilers that support inlines don't emit references to symbols referenced in a unused inline function. Since nobody noticed that issue in the 1+ years atomics was learning to walk on list, and not in the 8 months since it was committed, it's quite obviously not obvious. WRT the lockdefs.h split. It wasn't my idea (IIRC Alvaro's; I wanted to do something closes to what Noah suggested before Tom objected to lock.h in FRONTEND programs!), and I did wait for a day, while the buildfarm was red!, after posting it. At least two committers did comment on the approach before I pushed it. We've seen far more hot-needled patches to fix the buildfarm in topics with less portability risks. I could have left the buildfarm red for longer, but that might have hidden more severe portability problems. It's not like I committed that patch after somebody had suggested another way: Noah only commented after I had already pushed the split. The only actual separate patch since then (fastgetattr as inline function) was posted 2015-08-05 and I yesterday suggested to push it today. And it's just replacing two existing macros by inline functions. Imo there are far more complex patches regularly get committed by various people, with less discussion and review. There's afaik nothing broken due to either the inline patch or the lockdefs split right now. There's one portability bandaid, judged to be ugly by some, that we're discussing right now, and I'm happy to rejigger it if the various people with contradicting opinions can come to an agreement.
On Wed, Aug 12, 2015 at 2:57 PM, Andres Freund <andres@anarazel.de> wrote: > The only actual separate patch since then (fastgetattr as inline > function) was posted 2015-08-05 and I yesterday suggested to push it > today. And it's just replacing two existing macros by inline functions. I'm a little concerned about that one. Your testing shows that's a win for you, but is it a win for everyone? Maybe we should go ahead and do it anyway, but I'm not sure. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 08/12/2015 11:25 PM, Robert Haas wrote: > On Wed, Aug 12, 2015 at 2:57 PM, Andres Freund <andres@anarazel.de> wrote: >> The only actual separate patch since then (fastgetattr as inline >> function) was posted 2015-08-05 and I yesterday suggested to push it >> today. And it's just replacing two existing macros by inline functions. > > I'm a little concerned about that one. Your testing shows that's a > win for you, but is it a win for everyone? Maybe we should go ahead > and do it anyway, but I'm not sure. Andres didn't mention how big the performance benefit he saw with pgbench was, but I bet it was barely distinguishible from noise. But that's OK. In fact, there's no reason to believe this would make any difference to performance. The point is to make the code more readable, and it certainly achieves that. - Heikki
On Wed, Aug 12, 2015 at 1:34 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Andres didn't mention how big the performance benefit he saw with pgbench > was, but I bet it was barely distinguishible from noise. But that's OK. In > fact, there's no reason to believe this would make any difference to > performance. The point is to make the code more readable, and it certainly > achieves that. +1 -- Peter Geoghegan
On 2015-08-12 16:25:17 -0400, Robert Haas wrote: > On Wed, Aug 12, 2015 at 2:57 PM, Andres Freund <andres@anarazel.de> wrote: > > The only actual separate patch since then (fastgetattr as inline > > function) was posted 2015-08-05 and I yesterday suggested to push it > > today. And it's just replacing two existing macros by inline functions. > > I'm a little concerned about that one. Your testing shows that's a > win for you, but is it a win for everyone? Maybe we should go ahead > and do it anyway, but I'm not sure. I don't think it's likely to affect performance in any significant way in either direction. I mean, the absolute worst case would be that a formerly in-place fastgetattr() call gets changed into a function call in the same translation unit. That might or might not have a performance impact in either direction, but it's not going to be large. The only reason this has improved performance is imo that it shrank the code size a bit (increasing cache hit ratio, increase use of the branch prediction unit etc.). In all the profiles I've seen where fastgetattr (or rather the in-place code it has) is responsible for some portion of the runtime it was the actual looping, the cache misses, et al, not the stack and the indirect call. It's debatable that it's inline (via macro or inline function) in the first place. The advantage is not performance, it's readability. I've several times re-wrapped fastgetattr() just to understand what it does, because I otherwise find the code so hard to read. Maybe I just utterly suck at reading macros... You might argue that it's nothing we have touched frequently. And you're right. But I think that's a mistake. We spend far too much time in the various pieces of code dissembling tuples, and I think at some point somebody really needs to spend time on this. Regards, Andres
On 2015-08-12 23:34:38 +0300, Heikki Linnakangas wrote: > Andres didn't mention how big the performance benefit he saw with pgbench > was, but I bet it was barely distinguishible from noise. I think it was discernible (I played around with changing unrelated code trying to exclude unrelated layout issues), but it definitely was small. I think the only reason it had any effect is the reduced overall code size. > The point is to make the code more readable, and it certainly achieves > that. Exactly. - Andres
On Wed, Aug 12, 2015 at 4:34 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Andres didn't mention how big the performance benefit he saw with pgbench > was, but I bet it was barely distinguishible from noise. But that's OK. In > fact, there's no reason to believe this would make any difference to > performance. The point is to make the code more readable, and it certainly > achieves that. I think that when Bruce macro-ized this ten years ago or whenever it was, he got a significant performance benefit from it; otherwise I don't think he would have done it. It may well be that the march of time has improved compiler technology to the point where no benefit remains, and that's fine. But just as with any other part of the code, we shouldn't start with the premise that the existing code is bad the way it is. If a careful analysis leads us to that conclusion, that's fine. In this particular case, I am more concerned about making sure that people who have an opinion get a chance to air it than I am with the outcome. I have no reason to believe that we shouldn't make this change; I merely want to make sure that anyone who does have a concern about this (or other changes in this area) has a chance to be heard before we get too far down the path. Nobody's going to want to turn back the clock once we do this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, On 2015-08-06 12:43:06 -0300, Alvaro Herrera wrote: > Andres Freund wrote: > > On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote: > > > > Ah, but that's because you cheated and didn't remove the include from > > > namespace.h ... > > > > Well, it's not included from frontend code, so I didn't see the need? > > Going through all the backend code and replacing lock.h by lockdefs.h > > and some other includes doesn't seem particularly beneficial to me. > > > > FWIW, removing it from namespace.h is relatively easy. It starts to get > > a lot more noisy when you want to touch heapam.h. > > Ah, heapam.h is the granddaddy of all header messes, isn't it. (Actually > it's execnodes.h or something like that.) > > > > diff --git a/src/include/storage/lockdefs.h b/src/include/storage/lockdefs.h > > > > new file mode 100644 > > > > index 0000000..bfbcdba > > > > --- /dev/null > > > > +++ b/src/include/storage/lockdefs.h > > > > @@ -0,0 +1,56 @@ > > > > +/*------------------------------------------------------------------------- > > > > + * > > > > + * lockdefs.h > > > > + * Frontend exposed parts of postgres' low level lock mechanism > > > > + * > > > > + * The split between lockdefs.h and lock.h is not very principled. > > > > > > No kidding! > > > > Do you have a good suggestion about the split? I wanted to expose the > > minimal amount necessary, and those were the ones. > > Nope, nothing useful, sorry. To pick up on the general discussion and on the points you made here: I actually think the split came out to work far better than I'd anticipated. Having a slimmed-down version of lock.h for code that just needs to declare/pass lockmode parameters seems to improve our layering considerably. I got round to Alvaro's and Roberts position that removing lock.h from namespace.h and heapam.h would be a really nice improvemen on that front. Attached is a WIP patch to that end. After the further changes only few headers still have to include lock.h and they're pretty firmly in the backend-only territory. It also allows to remove the uglyness of passing around LOCKMODE as an int in parserOpenTable(). Imo lockdefs.h should be updated to describe that it contains the part of the lock infrastructure needed by indirect users of lock.h infrastructure, and that that currently unfortunately may include some frontend programs. I *also* think that removing lwlock.h from lock.h would be a good idea. In my opinion it's a bad idea to pointlessly expose so much low-level things to the wider world, even if it's only needed by relatively low level things. Given that dependent macros are in lwlock.h, it seems pretty sane to move LockHash* there too. We could additionally move all but LOCKMETHODID, LockTagType, LockAcquire*(), LockRelease() and probably one or two more to lock_internals.h (although I'd maybe rather name it lock_details?). I think it'd be an improvement, although possibly not a tremendous one given how many places it's likely going to be included from. Greetings, Andres Freund
Attachment
On Fri, Aug 14, 2015 at 2:40 PM, Andres Freund <andres@anarazel.de> wrote: > To pick up on the general discussion and on the points you made here: > > I actually think the split came out to work far better than I'd > anticipated. Having a slimmed-down version of lock.h for code that just > needs to declare/pass lockmode parameters seems to improve our layering > considerably. I got round to Alvaro's and Roberts position that > removing lock.h from namespace.h and heapam.h would be a really nice > improvemen on that front. > > Attached is a WIP patch to that end. After the further changes only few > headers still have to include lock.h and they're pretty firmly in the > backend-only territory. It also allows to remove the uglyness of passing > around LOCKMODE as an int in parserOpenTable(). Yes, I like this. > Imo lockdefs.h should be updated to describe that it contains the part > of the lock infrastructure needed by indirect users of lock.h > infrastructure, and that that currently unfortunately may include some > frontend programs. Sure. > I *also* think that removing lwlock.h from lock.h would be a good > idea. In my opinion it's a bad idea to pointlessly expose so much > low-level things to the wider world, even if it's only needed by > relatively low level things. Given that dependent macros are in > lwlock.h, it seems pretty sane to move LockHash* there too. Hmm. I dunno, lwlock.h is a pretty hideous layering violation as it is. I'd like to find a way to make that better, not worse. > We could additionally move all but LOCKMETHODID, LockTagType, > LockAcquire*(), LockRelease() and probably one or two more to > lock_internals.h (although I'd maybe rather name it lock_details?). I > think it'd be an improvement, although possibly not a tremendous one > given how many places it's likely going to be included from. What benefit would we get out of this? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Aug 11, 2015 at 01:04:48PM +0200, Andres Freund wrote: > On 2015-08-05 15:46:36 +0200, Andres Freund wrote: > > Here's a conversion for fastgetattr() and heap_getattr(). > In my opinion this drastically increases readability and thus should be > applied. Atomics were a miner's canary for pademelon's trouble with post-de6fd1c inlining. Expect pademelon to break whenever a frontend-included file gains an inline function that calls a backend function. Atomics were the initial examples, but this patch adds more: $ make -s PROFILE='-O0 -DPG_FORCE_DISABLE_INLINE=1' pg_resetxlog.o: In function `fastgetattr': /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:754: undefined reference to`nocachegetattr' pg_resetxlog.o: In function `heap_getattr': /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:783: undefined reference to`heap_getsysattr' The htup_details.h case is trickier in a way, because pg_resetxlog has a legitimate need for SizeofHeapTupleHeader via TOAST_MAX_CHUNK_SIZE. Expect this class of problem to recur as we add more inline functions. Our method to handle these first two instances will set a precedent. That gave me new respect for STATIC_IF_INLINE. While it does add tedious work to the task of introducing a new batch of inline functions, the work is completely mechanical. Anyone can write it; anyone can review it; there's one correct way to write it. Header surgery like 0001-Don-t-include-low-level-locking-code-from-frontend-c.patch requires expert design from scratch, and it (trivially) breaks builds for a sample of non-core code. Let's return to STATIC_IF_INLINE and convert fastgetattr() therewith. (A possible future line of inquiry is to generate the STATIC_IF_INLINE transformation at build time, so the handwritten headers can use C99 inlining directly as though it is always available.) Thanks, nm
On August 15, 2015 6:47:09 PM GMT+02:00, Noah Misch <noah@leadboat.com> wrote: >On Tue, Aug 11, 2015 at 01:04:48PM +0200, Andres Freund wrote: >> On 2015-08-05 15:46:36 +0200, Andres Freund wrote: >> > Here's a conversion for fastgetattr() and heap_getattr(). > >> In my opinion this drastically increases readability and thus should >be >> applied. > >Atomics were a miner's canary for pademelon's trouble with post-de6fd1c >inlining. Expect pademelon to break whenever a frontend-included file >gains >an inline function that calls a backend function. Atomics were the >initial >examples, but this patch adds more: > >$ make -s PROFILE='-O0 -DPG_FORCE_DISABLE_INLINE=1' >pg_resetxlog.o: In function `fastgetattr': >/data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:754: >undefined reference to `nocachegetattr' >pg_resetxlog.o: In function `heap_getattr': >/data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:783: >undefined reference to `heap_getsysattr' > >The htup_details.h case is trickier in a way, because pg_resetxlog has >a >legitimate need for SizeofHeapTupleHeader via TOAST_MAX_CHUNK_SIZE. >Expect >this class of problem to recur as we add more inline functions. Our >method to >handle these first two instances will set a precedent. > >That gave me new respect for STATIC_IF_INLINE. While it does add >tedious work >to the task of introducing a new batch of inline functions, the work is >completely mechanical. Anyone can write it; anyone can review it; >there's one >correct way to write it. Header surgery like >0001-Don-t-include-low-level-locking-code-from-frontend-c.patch >requires >expert design from scratch, and it (trivially) breaks builds for a >sample of >non-core code. Let's return to STATIC_IF_INLINE and convert >fastgetattr() >therewith. (A possible future line of inquiry is to generate the >STATIC_IF_INLINE transformation at build time, so the handwritten >headers can >use C99 inlining directly as though it is always available.) I'll respond in detail later. But wouldn't it be easy in this case to just ifndef FRONTEND things in this case? --- Please excuse brevity and formatting - I am writing this on my mobile phone.
Andres Freund <andres@anarazel.de> writes:
> On August 15, 2015 6:47:09 PM GMT+02:00, Noah Misch <noah@leadboat.com> wrote:
>> That gave me new respect for STATIC_IF_INLINE. While it does add
>> tedious work to the task of introducing a new batch of inline
>> functions, the work is completely mechanical. Anyone can write it;
>> anyone can review it; there's one correct way to write it. Header
>> surgery like
>> 0001-Don-t-include-low-level-locking-code-from-frontend-c.patch
>> requires expert design from scratch, and it (trivially) breaks builds
>> for a sample of non-core code. Let's return to STATIC_IF_INLINE and
>> convert fastgetattr() therewith. (A possible future line of inquiry is
>> to generate the STATIC_IF_INLINE transformation at build time, so the
>> handwritten headers can use C99 inlining directly as though it is
>> always available.)
> I'll respond in detail later. But wouldn't it be easy in this case to
> just ifndef FRONTEND things in this case?
I think that's missing Noah's point. Yeah, we'll probably always be able
to hack things to continue to work, but the key word there is "hack".
And if we don't have buildfarm coverage for compilers that are stupid
about inlining, we can expect to break the case regularly. (pademelon
won't last forever, though maybe by the time that box dies we'll figure
we no longer need to care about such compilers.)
I'm not especially in love with STATIC_IF_INLINE, but it did offer a
mechanical if tedious solution.
Realistically, with what we're doing now, we *have* broken things for
stupid-about-inlines compilers; because even if everything in the core
distribution builds, we've almost certainly created failures for
third-party modules that need to #include headers that no contrib
module includes.
Maybe we should just say "okay, we're raising the bar for 9.6: compilers
that don't elide unused static inlines need not apply". But I'd be
happier if we had a clear idea which those were. And if we go that route,
we ought to add a configure test checking it.
regards, tom lane
On 2015-08-15 12:47:09 -0400, Noah Misch wrote: > Atomics were a miner's canary for pademelon's trouble with post-de6fd1c > inlining. Expect pademelon to break whenever a frontend-included file gains > an inline function that calls a backend function. Atomics were the initial > examples, but this patch adds more: That's a good point. > $ make -s PROFILE='-O0 -DPG_FORCE_DISABLE_INLINE=1' > pg_resetxlog.o: In function `fastgetattr': > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:754: undefined referenceto `nocachegetattr' > pg_resetxlog.o: In function `heap_getattr': > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:783: undefined referenceto `heap_getsysattr' > > The htup_details.h case is trickier in a way, because pg_resetxlog has a > legitimate need for SizeofHeapTupleHeader via TOAST_MAX_CHUNK_SIZE. Expect > this class of problem to recur as we add more inline functions. Our method to > handle these first two instances will set a precedent. > > That gave me new respect for STATIC_IF_INLINE. While it does add tedious work > to the task of introducing a new batch of inline functions, the work is > completely mechanical. Anyone can write it; anyone can review it; there's one > correct way to write it. What's the advantage of using STATIC_IF_INLINE over just #ifndef FRONTEND? That doesn't work well for entire headers in my opinion, but for individual functions it shouldn't be a problem? In fact we've done it for years for MemoryContextSwitchTo(). And by the problem definition such functions cannot be required by frontend code. STATIC_IF_INLINE is really tedious because to know whether it works you essentially need to recompile postgres with inlines enabled/disabled. In fact STATIC_IF_INLINE does *not* even help here unless we also detect compilers that support inlining but balk when inline functions reference symbols not available. There was an example of that in the buildfarm as of 2014, c.f. a9baeb361d63 . We'd have to disable inlining for such compilers. > Header surgery like > 0001-Don-t-include-low-level-locking-code-from-frontend-c.patch > requires expert design from scratch, and it (trivially) breaks builds > for a sample of non-core code. I agree that such surgery isn't always a good idea. In my opinion the removing proc.h from the number of headers in the above and the followon WIP patch I posted has value completely independently from fixing problems. Greetings, Andres Freund
On Sun, Aug 16, 2015 at 02:03:01AM +0200, Andres Freund wrote: > On 2015-08-15 12:47:09 -0400, Noah Misch wrote: > > $ make -s PROFILE='-O0 -DPG_FORCE_DISABLE_INLINE=1' > > pg_resetxlog.o: In function `fastgetattr': > > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:754: undefined referenceto `nocachegetattr' > > pg_resetxlog.o: In function `heap_getattr': > > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:783: undefined referenceto `heap_getsysattr' > What's the advantage of using STATIC_IF_INLINE over just #ifndef > FRONTEND? > In fact STATIC_IF_INLINE does *not* even help here unless we also detect > compilers that support inlining but balk when inline functions reference > symbols not available. There was an example of that in the buildfarm as > of 2014, c.f. a9baeb361d63 . We'd have to disable inlining for such > compilers. Neat; I didn't know that. Solaris Studio 12.3 (newest version as of Oct 2014) still does that when optimization is disabled, and I place sufficient value on keeping inlining enabled for such a new compiler. The policy would then be (already is?) to wrap in "#ifdef FRONTEND" any inline function that uses a backend symbol. When a header is dedicated to such functions, we might avoid the whole header in the frontend instead of wrapping each function. That policy works for me. On Sat, Aug 15, 2015 at 01:48:17PM -0400, Tom Lane wrote: > Realistically, with what we're doing now, we *have* broken things for > stupid-about-inlines compilers; because even if everything in the core > distribution builds, we've almost certainly created failures for > third-party modules that need to #include headers that no contrib > module includes. Only FRONTEND code (e.g. repmgr, pg_reorg) will be at hazard, not ordinary third-party (backend) modules. We could have a test frontend that includes every header minus a blacklist, but I don't think it's essential. External FRONTEND code is an order of magnitude less common than external backend code. Unlike backend module development, we don't document the existence of the FRONTEND programming environment.
On 2015-08-15 23:50:09 -0400, Noah Misch wrote: > On Sun, Aug 16, 2015 at 02:03:01AM +0200, Andres Freund wrote: > > On 2015-08-15 12:47:09 -0400, Noah Misch wrote: > > > $ make -s PROFILE='-O0 -DPG_FORCE_DISABLE_INLINE=1' > > > pg_resetxlog.o: In function `fastgetattr': > > > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:754: undefined referenceto `nocachegetattr' > > > pg_resetxlog.o: In function `heap_getattr': > > > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:783: undefined referenceto `heap_getsysattr' > > > What's the advantage of using STATIC_IF_INLINE over just #ifndef > > FRONTEND? > > > In fact STATIC_IF_INLINE does *not* even help here unless we also detect > > compilers that support inlining but balk when inline functions reference > > symbols not available. There was an example of that in the buildfarm as > > of 2014, c.f. a9baeb361d63 . We'd have to disable inlining for such > > compilers. > > Neat; I didn't know that. Personally I don't find that particularly neat, rather annoying actually :P > Solaris Studio 12.3 (newest version as of Oct 2014) still does that > when optimization is disabled, and I place sufficient value on keeping > inlining enabled for such a new compiler. Ah, that's cool. I was wondering generally how we could find an animal to detect that case once pademelon met its untimely (or timely by now?) end. > The policy would then be > (already is?) to wrap in "#ifdef FRONTEND" any inline function that uses a > backend symbol. When a header is dedicated to such functions, we might avoid > the whole header in the frontend instead of wrapping each function. That > policy works for me. Cool. I was wondering before where we'd document policy around this. sources.sgml? Greetings, Andres Freund
On Sun, Aug 16, 2015 at 05:58:17AM +0200, Andres Freund wrote: > On 2015-08-15 23:50:09 -0400, Noah Misch wrote: > > On Sun, Aug 16, 2015 at 02:03:01AM +0200, Andres Freund wrote: > > > On 2015-08-15 12:47:09 -0400, Noah Misch wrote: > > > > $ make -s PROFILE='-O0 -DPG_FORCE_DISABLE_INLINE=1' > > > > pg_resetxlog.o: In function `fastgetattr': > > > > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:754: undefined referenceto `nocachegetattr' > > > > pg_resetxlog.o: In function `heap_getattr': > > > > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:783: undefined referenceto `heap_getsysattr' > > > > > What's the advantage of using STATIC_IF_INLINE over just #ifndef > > > FRONTEND? > > > > > In fact STATIC_IF_INLINE does *not* even help here unless we also detect > > > compilers that support inlining but balk when inline functions reference > > > symbols not available. There was an example of that in the buildfarm as > > > of 2014, c.f. a9baeb361d63 . We'd have to disable inlining for such > > > compilers. > > Solaris Studio 12.3 (newest version as of Oct 2014) still does that > > when optimization is disabled, and I place sufficient value on keeping > > inlining enabled for such a new compiler. > > Ah, that's cool. I was wondering generally how we could find an animal > to detect that case once pademelon met its untimely (or timely by now?) > end. I would just make a "gcc -O0 -DPG_FORCE_DISABLE_INLINE=1" animal, since the Solaris machine time supply is small. > > The policy would then be > > (already is?) to wrap in "#ifdef FRONTEND" any inline function that uses a > > backend symbol. When a header is dedicated to such functions, we might avoid > > the whole header in the frontend instead of wrapping each function. That > > policy works for me. > > Cool. I was wondering before where we'd document policy around > this. sources.sgml? +1. Most coding rules go undocumented, but I'm in favor of changing that as the opportunity arises.
On Fri, Aug 14, 2015 at 08:40:13PM +0200, Andres Freund wrote: > > > On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote: > > > > > + * lockdefs.h > > > > > + * Frontend exposed parts of postgres' low level lock mechanism > I actually think the split came out to work far better than I'd > anticipated. Having a slimmed-down version of lock.h for code that just > needs to declare/pass lockmode parameters seems to improve our layering > considerably. I got round to Alvaro's and Roberts position that > removing lock.h from namespace.h and heapam.h would be a really nice > improvemen on that front. I assessed 0001-WIP-Decrease-usage-of-lock.h-further.patch and reassessed 0001-Don-t-include-low-level-locking-code-from-frontend-c.patch (commit 4eda0a6). I changed the details of my position compared to my last review. As we see from the patches' need to add #include statements to .c files and from Robert's report of a broken EDB build, this change creates work for maintainers of non-core code, both backend code (modules) and frontend code (undocumented, but folks do it). That's to be expected and doesn't take a great deal of justification, but users should get benefits in connection with the work. This brings to mind the htup_details.h introduction, which created so much busy work in non-core code. I don't expect the lock.h split to be quite that disruptive. Statistics on PGXN modules: 29 modules mention htup_details.h8 modules mention lwlock.h7 modules mention LWLock4 modules mention lock.h1 module mentionsLockAcquire() Four modules (imcs, pg_stat_kcache, query_histogram, query_recorder) mentioned LWLock without mentioning lwlock.h. These patches (trivially) break the imcs build. The other three failed to build for unrelated reasons, but I suspect these patches give those modules one more thing to update. What do users get out of this? They'll learn if their code is not portable to pademelon, but #warning "atomics.h in frontend code is not portable" would communicate the same without compelling non-core code to care. Other than that benefit, making headers #error-on-FRONTEND mostly lets us congratulate ourselves for having introduced the start of a header layer distinction. I'd be for that if PostgreSQL were new, but I can't justify doing it at the user cost already apparent. That would be bad business. Therefore, I urge you to instead add the aforementioned #warning and wrap with #ifdef FRONTEND the two #include "port/atomics.h" in headers. That tightly limits build breakage; it can only break frontend code, which is rare outside core. Hackers will surely notice if a patch makes the warning bleat, so mistakes won't survive long. Non-core frontend code, if willing to cede a shred of portability, can experiment with atomics. Folks could do nifty things with that. Thanks, nm
Andres Freund <andres@anarazel.de> writes:
> On 2015-08-15 23:50:09 -0400, Noah Misch wrote:
>> Solaris Studio 12.3 (newest version as of Oct 2014) still does that
>> when optimization is disabled, and I place sufficient value on keeping
>> inlining enabled for such a new compiler.
> Ah, that's cool. I was wondering generally how we could find an animal
> to detect that case once pademelon met its untimely (or timely by now?)
> end.
Yeah. If we get to the point where we can't actually find any toolchains
that work that way, it may be time to revise our portability policy. But
for now Solaris Studio is a good-enough reason to not move the goalposts.
>> The policy would then be
>> (already is?) to wrap in "#ifdef FRONTEND" any inline function that uses a
>> backend symbol. When a header is dedicated to such functions, we might avoid
>> the whole header in the frontend instead of wrapping each function. That
>> policy works for me.
Works for me as well, as long as we have buildfarm critters that will
notice oversights.
regards, tom lane
On 2015-08-16 03:31:48 -0400, Noah Misch wrote: > As we see from the patches' need to add #include statements to .c files and > from Robert's report of a broken EDB build, this change creates work for > maintainers of non-core code, both backend code (modules) and frontend code > (undocumented, but folks do it). That's to be expected and doesn't take a > great deal of justification, but users should get benefits in connection with > the work. This brings to mind the htup_details.h introduction, which created > so much busy work in non-core code. I don't expect the lock.h split to be > quite that disruptive. Statistics on PGXN modules: > Four modules (imcs, pg_stat_kcache, query_histogram, query_recorder) mentioned > LWLock without mentioning lwlock.h. These patches (trivially) break the imcs > build. The other three failed to build for unrelated reasons, but I suspect > these patches give those modules one more thing to update. What do users get > out of this? They'll learn if their code is not portable to pademelon, but > #warning "atomics.h in frontend code is not portable" would communicate the > same without compelling non-core code to care. I'd love to make it a #warning intead of an error, but unfortunately that's not standard C :( > Other than that benefit, making headers #error-on-FRONTEND mostly lets > us congratulate ourselves for having introduced the start of a header > layer distinction. I'd be for that if PostgreSQL were new, but I > can't justify doing it at the user cost already apparent. That would > be bad business. To me that's basically saying that we'll never ever have any better separation between frontend/backend headers since each incremental improvement won't be justifiable. Adding an explicit include which exists in all version of postgres is really rather low cost, you don't even need version dependant define. The modules you name are, as you noticed, likely to require minor surgery for new major versions anyway, being rather low level. As you say breaking C code over major releases doesn't have to meet a high barrier, and getting rid of the wart of lock.h being used that widely seems to be more than suffient to meet that qualification. If others think this is the right way, ok, I can live with that and implement it, but I won't agree. > Therefore, I urge you to instead add the aforementioned #warning and wrap with > #ifdef FRONTEND the two #include "port/atomics.h" in headers. That tightly > limits build breakage; it can only break frontend code, which is rare outside > core. Hackers will surely notice if a patch makes the warning bleat, so > mistakes won't survive long. > Non-core frontend code, if willing to cede a shred of portability, can > experiment with atomics. Folks could do nifty things with that. I don't think that's something worth keeping in mind from our side. If you don't care about portability you can just use C11 atomics or such. If somebody actually wants to add FRONTEND support for the fallback code - possibly falling back to pthread mutexes - ok, but before that... Greetings, Andres Freund
On Wed, Jul 1, 2015 at 11:14 AM, Andres Freund <andres@anarazel.de> wrote: > Hi, > > During the 9.5 cycle, and earlier, the topic of increasing our minimum > bar for compilers came up a bunch of times. Specifically whether we > still should continue to use C90 as a baseline. Minor question: is there any value to keeping the client side code to older standards? On a previous project compiled libpq on many legacy architectures because we were using it as frontend to backup software. The project didn't end up taking off so it's no big deal to me, but just throwing it out there: libpq has traditionally enjoyed broader compiler support than the full project (borland, windows back in the day). merlin
On Wed, Jul 1, 2015 at 5:14 PM, Andres Freund <andres@anarazel.de> wrote: > During the 9.5 cycle, and earlier, the topic of increasing our minimum > bar for compilers came up a bunch of times. Specifically whether we > still should continue to use C90 as a baseline. > > I think the time has come to rely at least on some newer features. I don't have much opinion on the topic (aside from "it's nice that we run on old systems" but that's neither here nor there). But I'm not clear from the discussion exactly which compilers we're thinking of ruling out. For GCC are we talking about bumping the minimum version required or are all current versions of GCC new enough and we're only talking about old HPUX/Solaris/etc compilers? -- greg
On 2015-08-17 15:51:33 +0100, Greg Stark wrote: > But I'm not clear from the discussion exactly which compilers we're > thinking of ruling out. None.
On Sat, Aug 15, 2015 at 8:03 PM, Andres Freund <andres@anarazel.de> wrote: >> That gave me new respect for STATIC_IF_INLINE. While it does add tedious work >> to the task of introducing a new batch of inline functions, the work is >> completely mechanical. Anyone can write it; anyone can review it; there's one >> correct way to write it. > > What's the advantage of using STATIC_IF_INLINE over just #ifndef > FRONTEND? That doesn't work well for entire headers in my opinion, but > for individual functions it shouldn't be a problem? In fact we've done > it for years for MemoryContextSwitchTo(). And by the problem definition > such functions cannot be required by frontend code. > > STATIC_IF_INLINE is really tedious because to know whether it works you > essentially need to recompile postgres with inlines enabled/disabled. > > In fact STATIC_IF_INLINE does *not* even help here unless we also detect > compilers that support inlining but balk when inline functions reference > symbols not available. There was an example of that in the buildfarm as > of 2014, c.f. a9baeb361d63 . We'd have to disable inlining for such > compilers. The advantage of STATIC_IF_INLINE is that we had everything working in that model. We seem to be replacing problems that we had solved and code that worked on our entire buildfarm with new problems that we haven't solved yet and which don't seem to be a whole lot simpler than the ones they replaced. As far as I can see, the anticipated benefits of what we're doing here are: - Get a cleaner separation of frontend and backend headers (this could also be done independently of STATIC_IF_INLINE, but removing STATIC_IF_INLINE increases the urgency). - Eliminate multiple evaluations hazards. - Modest improvements to code generation. And the costs are: - Lots of warnings with compilers that are not smart about static inline, and probably significantly worse code generation. - The possibility that may repeatedly break #define FRONTEND compilation when we add static inline functions, where instead adding macros would not have caused breakage, thus resulting in continual tinkering with the header files. What I'm basically worried about is that second one. Actually, what I'm specifically worried about is that we will break somebody's #ifdef FRONTEND code, they will eventually complain, and we will refuse to fix it because we don't think their use case is valid. If that happens even a few times, it will lead me to think that this whole effort was misguided. :-( -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-08-17 12:30:56 -0400, Robert Haas wrote: > - The possibility that may repeatedly break #define FRONTEND > compilation when we add static inline functions, where instead adding > macros would not have caused breakage, thus resulting in continual > tinkering with the header files. Again, that's really independent. Inlines have that problem, even with STATIC_IF_INLINE. C.f. MemoryContextSwitch() and a9baeb361d. Greetings, Andres Freund
On 2015-08-17 12:30:56 -0400, Robert Haas wrote: > As far as I can see, the anticipated benefits of what we're doing here are: > > - Get a cleaner separation of frontend and backend headers (this could > also be done independently of STATIC_IF_INLINE, but removing > STATIC_IF_INLINE increases the urgency). > - Eliminate multiple evaluations hazards. > - Modest improvements to code generation. Plus: * Not having 7k long macros, that e.g. need extra flags to even be supported. C.f. http://archives.postgresql.org/message-id/4407.1435763473%40sss.pgh.pa.us * Easier development due to actual type checking and such. Compare the errors from heap_getattr as a macro being passed aboolean with the same from an inline function: Inline: /home/andres/src/postgresql/src/backend/executor/spi.c: In function ‘SPI_getvalue’: /home/andres/src/postgresql/src/backend/executor/spi.c:883:46: error: incompatible type for argument 4 of ‘heap_getattr’val = heap_getattr(tuple, fnumber, tupdesc, isnull); ^ In file included from /home/andres/src/postgresql/src/backend/executor/spi.c:17:0: /home/andres/src/postgresql/src/include/access/htup_details.h:765:1: note: expected ‘_Bool *’ but argument is of type ‘_Bool’heap_getattr(HeapTupletup, int attnum, TupleDesc tupleDesc,^ Macro: In file included from /home/andres/src/postgresql/src/backend/executor/spi.c:17:0: /home/andres/src/postgresql/src/backend/executor/spi.c: In function ‘SPI_getvalue’: /home/andres/src/postgresql/src/include/access/htup_details.h:750:6: error: invalid type argument of unary ‘*’ (have ‘int’) (*(isnull) = true), \ ^ /home/andres/src/postgresql/src/backend/executor/spi.c:883:8: note: in expansion of macro ‘heap_getattr’ val = heap_getattr(tuple,fnumber, tupdesc, isnull); ^ /home/andres/src/postgresql/src/include/access/htup_details.h:750:23: warning: left-hand operand of comma expression hasno effect [-Wunused-value] (*(isnull) = true), \ ^ /home/andres/src/postgresql/src/backend/executor/spi.c:883:8: note: in expansion of macro ‘heap_getattr’ val = heap_getattr(tuple,fnumber, tupdesc, isnull); ^ /home/andres/src/postgresql/src/include/access/htup_details.h:697:3: error: invalid type argument of unary ‘*’ (have ‘int’)(*(isnull) = false), \ ^ /home/andres/src/postgresql/src/include/access/htup_details.h:754:5: note: in expansion of macro ‘fastgetattr’ fastgetattr((tup),(attnum), (tupleDesc), (isnull)) \ ^ /home/andres/src/postgresql/src/backend/executor/spi.c:883:8: note: in expansion of macro ‘heap_getattr’ val = heap_getattr(tuple,fnumber, tupdesc, isnull); ^ /home/andres/src/postgresql/src/include/access/htup_details.h:713:5: error: invalid type argument of unary ‘*’ (have ‘int’) (*(isnull) = true), \ ^ /home/andres/src/postgresql/src/include/access/htup_details.h:754:5: note: in expansion of macro ‘fastgetattr’ fastgetattr((tup),(attnum), (tupleDesc), (isnull)) \ ^ /home/andres/src/postgresql/src/backend/executor/spi.c:883:8: note: in expansion of macro ‘heap_getattr’ val = heap_getattr(tuple,fnumber, tupdesc, isnull); ^ /home/andres/src/postgresql/src/include/access/htup_details.h:713:22: warning: left-hand operand of comma expression hasno effect [-Wunused-value] (*(isnull) = true), \ ^ /home/andres/src/postgresql/src/include/access/htup_details.h:754:5: note: in expansion of macro ‘fastgetattr’ fastgetattr((tup),(attnum), (tupleDesc), (isnull)) \ ^ /home/andres/src/postgresql/src/backend/executor/spi.c:883:8: note: in expansion of macro ‘heap_getattr’ val = heap_getattr(tuple,fnumber, tupdesc, isnull); ^ /home/andres/src/postgresql/src/include/access/htup_details.h:697:21: warning: left-hand operand of comma expression hasno effect [-Wunused-value] (*(isnull) = false), \ ^ /home/andres/src/postgresql/src/include/access/htup_details.h:754:5: note: in expansion of macro ‘fastgetattr’ fastgetattr((tup),(attnum), (tupleDesc), (isnull)) \ ^ /home/andres/src/postgresql/src/backend/executor/spi.c:883:8: note: in expansion of macro ‘heap_getattr’ val = heap_getattr(tuple,fnumber, tupdesc, isnull); ^ /home/andres/src/postgresql/src/include/access/htup_details.h:757:50: warning: passing argument 4 of ‘heap_getsysattr’ makespointer from integer without a cast [-Wint-conversion] heap_getsysattr((tup), (attnum), (tupleDesc), (isnull)) \ ^ /home/andres/src/postgresql/src/backend/executor/spi.c:883:8: note: in expansion of macro ‘heap_getattr’ val = heap_getattr(tuple,fnumber, tupdesc, isnull); ^ /home/andres/src/postgresql/src/include/access/htup_details.h:771:14: note: expected ‘bool * {aka char *}’ but argument isof type ‘bool {aka char}’extern Datum heap_getsysattr(HeapTuple tup, int attnum, TupleDesc tupleDesc, ^
On Mon, Aug 17, 2015 at 12:36 PM, Andres Freund <andres@anarazel.de> wrote: > On 2015-08-17 12:30:56 -0400, Robert Haas wrote: >> - The possibility that may repeatedly break #define FRONTEND >> compilation when we add static inline functions, where instead adding >> macros would not have caused breakage, thus resulting in continual >> tinkering with the header files. > > Again, that's really independent. Inlines have that problem, even with > STATIC_IF_INLINE. C.f. MemoryContextSwitch() and a9baeb361d. Inlines, yes, but macros don't. I'm not saying we shouldn't do this, but I *am* saying that we need to be prepared to treat breaking FRONTEND compilation as a problem, not just today and tomorrow, but way off into the future. It's not at all a stretch to think that we could still be hitting fallout from these changes in 2-3 years time. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Aug 17, 2015 at 12:06:42PM +0200, Andres Freund wrote: > On 2015-08-16 03:31:48 -0400, Noah Misch wrote: > I'd love to make it a #warning intead of an error, but unfortunately > that's not standard C :( Okay. > > Other than that benefit, making headers #error-on-FRONTEND mostly lets > > us congratulate ourselves for having introduced the start of a header > > layer distinction. I'd be for that if PostgreSQL were new, but I > > can't justify doing it at the user cost already apparent. That would > > be bad business. > > To me that's basically saying that we'll never ever have any better > separation between frontend/backend headers since each incremental > improvement won't be justifiable. Exactly. This is one of those proposals that can never repay its costs. Header refactoring seduces hackers, but the benefits don't materialize.
On 2015-08-16 05:58:17 +0200, Andres Freund wrote:
> On 2015-08-15 23:50:09 -0400, Noah Misch wrote:
> > The policy would then be
> > (already is?) to wrap in "#ifdef FRONTEND" any inline function that uses a
> > backend symbol. When a header is dedicated to such functions, we might avoid
> > the whole header in the frontend instead of wrapping each function. That
> > policy works for me.
>
> Cool. I was wondering before where we'd document policy around
> this. sources.sgml?
As Noah I think it'd be good if we, over time, started to document a few
more things one currently have to pick up over time. I'm wondering
whether these should be subsections under a new sect1 ('Code Structure'?
Don't like that much), or all independent sect1s.
Stuff I'd like to see documented there over time includes:
1) Definition of the standard that we require, i.e. for now C89.
2) error handling with setjmp, specifically that and when volatile has to be used.
3) Signal handlers, and what you can/cannod do.
4) That we rely on 4 byte aligned single-copy atomicity (i.e. some recent value is read, not a mixture of two), but
thatwe do not realy on atomic 8 byte writes/reads.
The WIP patch I have on C89 and static inline is: <sect1 id="source-structure"> <title>Structure</title> <simplesect>
<title>C Standard</title> <para> Code in <productname>PostgreSQL</> should only rely on language features
availablein the C89 standard. That means a conforming C89 compiler has to be able to compile postgres. Features from
later revision of the C standard or compiler specific features can be used, if a fallback is provided. </para>
<para> For example <literal>static inline</> and <literal>_StaticAssert()</literal> are used, even though they are
from newer revisions of the C standard. If not available we respectively fall back to defining the functions
withoutinline, and to using a C89 compatible replacement that also emits errors, but emits far less readable
errors. </para> </simplesect> <simplesect> <title>Function-Like Macros and Inline Functions</title> <para>
Both, macros with arguments and <literal>static inline</> functions, may be used. The latter are preferrable if
thereare multiple-evaluation hazards when written as a macro, as e.g. the case with
<programlisting>
#define Max(x, y) ((x) > (y) ? (x) : (y))
</programlisting> or when the macro would be very long. In other cases it's only possible to use macros, or at
leasteasier. For example because expressions of various types need to be passed to the macro. </para> <para>
Whendefining an inline function in a header that references symbols (i.e. variables, functions) that are only
availableas part of the backend, the function may not be visible when included from frontend code.
<programlisting>
#ifndef FRONTEND
static inline MemoryContext
MemoryContextSwitchTo(MemoryContext context)
{ MemoryContext old = CurrentMemoryContext;
CurrentMemoryContext = context; return old;
}
#endif /* FRONTEND */
</programlisting> In this example <literal>CurrentMemoryContext</>, which is only available in the backend, is
referencedand the function thus hidden with a <literal>#ifndef FRONTEND</literal>. This rule exists because some
compilersemit references to symbols contained in inline functions even if the function is not used. </para>
</simplesect></sect1>
That's not yet perfect, but shows what I'm thinking of. Comments?
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
> As Noah I think it'd be good if we, over time, started to document a few
> more things one currently have to pick up over time. I'm wondering
> whether these should be subsections under a new sect1 ('Code Structure'?
> Don't like that much), or all independent sect1s.
"Structure" is certainly not what this material is. Maybe "Miscellaneous
Coding Conventions" is the best we can do for a title. The items seem too
short to be their own <sect1>'s.
> That's not yet perfect, but shows what I'm thinking of. Comments?
Needs a bit of copy-editing in places, but +1 overall.
regards, tom lane
On Thu, Aug 27, 2015 at 4:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> That's not yet perfect, but shows what I'm thinking of. Comments? > > Needs a bit of copy-editing in places, but +1 overall. Yeah. I bet there's a lot more useful stuff we could include also, but everything Andres mentioned is certainly good to put in there. Alternatively, some of this stuff could go into a README file instead of the documentation, but I think we've been leaning toward documenting more C stuff lately, and I'm fine with that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> Yeah. I bet there's a lot more useful stuff we could include also,
> but everything Andres mentioned is certainly good to put in there.
> Alternatively, some of this stuff could go into a README file instead
> of the documentation, but I think we've been leaning toward
> documenting more C stuff lately, and I'm fine with that.
I think we've mostly used READMEs for documentation that's relevant to
particular subparts of the source tree, eg, planner, nbtree, etc. Stuff
like this would only make sense if you put it in a top-level README, which
is a file that contains user-facing info in most projects including ours.
So I think sticking it into some portion of Part VII (Internals) is the
right approach.
It strikes me that the information in backend/utils/mmgr/README would be
a good candidate to move into the Internals SGML, too. Almost none of
that is "stuff you only care about when reading utils/mmgr/".
regards, tom lane
Tom Lane wrote: > It strikes me that the information in backend/utils/mmgr/README would be > a good candidate to move into the Internals SGML, too. Almost none of > that is "stuff you only care about when reading utils/mmgr/". Completely agreed. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Aug 27, 2015 at 11:31:44AM -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > As Noah I think it'd be good if we, over time, started to document a few
> > more things one currently have to pick up over time. I'm wondering
> > whether these should be subsections under a new sect1 ('Code Structure'?
> > Don't like that much), or all independent sect1s.
>
> "Structure" is certainly not what this material is. Maybe "Miscellaneous
> Coding Conventions" is the best we can do for a title. The items seem too
> short to be their own <sect1>'s.
"C Language Features" comes to mind. I assume we're talking about names for a
<sect1> inside <chapter id="source">.
> > That's not yet perfect, but shows what I'm thinking of. Comments?
>
> Needs a bit of copy-editing in places, but +1 overall.
+1
On Wed, Aug 5, 2015 at 03:46:36PM +0200, Andres Freund wrote: > On 2015-08-05 15:08:29 +0200, Andres Freund wrote: > > We might later want to change some of the harder to maintain macros to > > inline functions, but that seems better done separately. > > Here's a conversion for fastgetattr() and heap_getattr(). Not only is > the resulting code significantly more readable, but the conversion also > shrinks the code size: Hey, the fastgetattr() macro was a work of art! ;-) (And more of my hacks disappear.) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Wed, Aug 12, 2015 at 04:47:55PM -0400, Robert Haas wrote: > On Wed, Aug 12, 2015 at 4:34 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > Andres didn't mention how big the performance benefit he saw with pgbench > > was, but I bet it was barely distinguishible from noise. But that's OK. In > > fact, there's no reason to believe this would make any difference to > > performance. The point is to make the code more readable, and it certainly > > achieves that. > > I think that when Bruce macro-ized this ten years ago or whenever it > was, he got a significant performance benefit from it; otherwise I > don't think he would have done it. (You over-estimate me. ;-) ) What happened is that I was looking at call graph counts and fastgetattr() was called a bazillion times, so I inlined it, and saw a noticeably performance improvement, maybe 2% on an in-memory SELECT-only workload. Same with a few other macros I created in those early years. Frankly, my hacks last a lot longer than I expected. (Did someone say pg_upgrade. :-) ) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Wed, Aug 12, 2015 at 10:40:53PM +0200, Andres Freund wrote: > You might argue that it's nothing we have touched frequently. And you're > right. But I think that's a mistake. We spend far too much time in the > various pieces of code dissembling tuples, and I think at some point > somebody really needs to spend time on this. Yes, this will need to be addressed some day --- I have heard rumors that we use more CPU than some proprietary relational database for the same workload. Interestingly, we are not necessary slower, just consume more CPU, causing us to max out the CPU sooner. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 2015-08-27 11:31:44 -0400, Tom Lane wrote: > Needs a bit of copy-editing in places, but +1 overall. Heres a slightly expanded version of this. I tried to do some of the copy-editing, but I'm sure a look from a native speaker won't hurt. Greetings, Andres Freund