Raising our compiler requirements for 9.6

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

"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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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...

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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 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.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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?

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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&amp;br=HEAD

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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 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.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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 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'

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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 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'

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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 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'

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.h
8 modules mention lwlock.h
7 modules mention LWLock
4 modules mention lock.h
1 module mentions LockAcquire()

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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 a boolean 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(HeapTuple tup, 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 has no 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 has no 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 has no 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’ makes pointer 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 is of type ‘bool {aka char}’
extern Datum heap_getsysattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
^

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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 that we 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 available in 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 without inline,
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 there are
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 least easier. For example because
expressions of various types need to be passed to the macro.
</para>
<para>
When defining an inline function in a header that references
symbols (i.e. variables, functions) that are only available as
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 referenced and the function thus
hidden with a <literal>#ifndef FRONTEND</literal>. This rule
exists because some compilers emit 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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