Large writable variables

Hi,

On 2018-10-15 13:07:54 -0700, Andres Freund wrote:

top itialized allocations:
$ nm -t d --size-sort -r -S src/backend/postgres|grep '\b[dD]\b'|head
0000000008086944 0000000000087904 D fmgr_builtins
0000000008201120 0000000000017280 d ConfigureNamesInt
0000000008218400 0000000000013680 d ConfigureNamesBool
0000000008189248 0000000000008512 d ConfigureNamesString
0000000008077344 0000000000007040 D ScanKeywords
0000000008184928 0000000000004320 d ConfigureNamesEnum
0000000008197760 0000000000003360 d ConfigureNamesReal
0000000008062976 0000000000002304 d DCH_keywords
0000000008069952 0000000000002016 D pg_wchar_table
0000000008075552 0000000000001776 d encoding_match_list

fmgr_builtins isn't readonly even though declared a const - I assume
because it's full of addresses that will be mapped differently from
execution to execution.

ConfigureNames* isn't marked as const, because we update them:
/* Rather than requiring vartype to be filled in by hand, do this: */
conf->gen.vartype = PGC_BOOL;

I'm unclear as to why ScanKeywords, DCH_keywords aren't in a readonly
section.

It's because they contain pointers to strings, which are affected by
relocations (and position independent executables force everything to be
relocatable). They do go into .data.rel.ro however:

$ objdump -t ~/build/postgres/dev-optimize/vpath/src/backend/postgres|grep -E '\b(ScanKeywords|fmgr_builtins|DCH_keywords|pg_wchar_table)\b'
00000000007b0800 l O .data.rel.ro 0000000000000900 DCH_keywords
00000000007b4020 g O .data.rel.ro 0000000000001b80 ScanKeywords
00000000007b65a0 g O .data.rel.ro 0000000000015760 fmgr_builtins
00000000007b2340 g O .data.rel.ro 00000000000007e0 pg_wchar_table

as long as we're using forking rather than EXEC_BACKEND, that's
perfectly fine. I don't quite know how windows handles any of this, so
I can't say whether it's a problem there.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

So we have 500kb of not-initialized memory mapped into every
process. That's, uh, not nothing.

Bleah.

0000000008585088 0000000000131104 b hist_entries
0000000008716192 0000000000016384 b hist_start

I'm unsure what fraction of processes would have use for these.

0000000008435040 0000000000085280 b DCHCache
0000000008391168 0000000000043840 b NUMCache

We could surely allocate these on first use.

0000000008560224 0000000000023440 b tzdefrules_s
0000000008536704 0000000000023440 b gmtmem.7009

I think that tzdefrules_s is not used in common cases (though I could be
wrong about that), so we could win by alloc-on-first-use. The same might
be true for gmtmem, but there's a sticking point: there is no provision
for failure there, so I'm unsure how we avoid crashing on OOM.

0000000008238336 0000000000008192 b PqRecvBuffer
0000000008734208 0000000000005136 B BackendWritebackContext
0000000008386368 0000000000003200 b held_lwlocks

These are below my personal threshold of pain.

fmgr_builtins isn't readonly even though declared a const - I assume
because it's full of addresses that will be mapped differently from
execution to execution.

Check.

I'm unclear as to why ScanKeywords, DCH_keywords aren't in a readonly
section.

I think it's the same problem: pointers can't be truly const because
they have to be changed if one relocates the executable.

We could possibly fix these by changing the data structure so that
what's in a ScanKeywords entry is an offset into some giant string
constant somewhere. No idea how that would affect performance, but
I do notice that we could reduce the sizeof(ScanKeyword), which can't
hurt.

regards, tom lane

Hi,

On 2018-10-15 16:36:26 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

So we have 500kb of not-initialized memory mapped into every
process. That's, uh, not nothing.

Bleah.

Yea...

0000000008585088 0000000000131104 b hist_entries
0000000008716192 0000000000016384 b hist_start

I'm unsure what fraction of processes would have use for these.

Yea, I'm not sure either. Although I suspect that given the cost of
compression having an "allocate on first use" check should be quite
doable.

0000000008435040 0000000000085280 b DCHCache
0000000008391168 0000000000043840 b NUMCache

We could surely allocate these on first use.

yep.

0000000008560224 0000000000023440 b tzdefrules_s
0000000008536704 0000000000023440 b gmtmem.7009

I think that tzdefrules_s is not used in common cases (though I could be
wrong about that), so we could win by alloc-on-first-use. The same might
be true for gmtmem, but there's a sticking point: there is no provision
for failure there, so I'm unsure how we avoid crashing on OOM.

I guess we could return false / NULL to the caller. Not perfect, but
there's not that many callers. We could wrap them in wrappers that throw
errors...

0000000008238336 0000000000008192 b PqRecvBuffer
0000000008734208 0000000000005136 B BackendWritebackContext
0000000008386368 0000000000003200 b held_lwlocks

These are below my personal threshold of pain.

Yea, agreed. PqRecvBuffer and held_lwlocks are common enough that it
makes sense to pre-allocate them anyway. I guess you could argue that
BackendWritebackContext should be dynamically allocated.

I'm unclear as to why ScanKeywords, DCH_keywords aren't in a readonly
section.

I think it's the same problem: pointers can't be truly const because
they have to be changed if one relocates the executable.

Right. It's, as I noticed when looking via objdupm, in .data.rel.ro, so
I think it's not that bad.

We could possibly fix these by changing the data structure so that
what's in a ScanKeywords entry is an offset into some giant string
constant somewhere. No idea how that would affect performance, but
I do notice that we could reduce the sizeof(ScanKeyword), which can't
hurt.

Yea, that might even help performancewise. Alternatively we could change
ScanKeyword to store the keyword name inline, but that'd be a measurable
size increase...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2018-10-15 16:36:26 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

0000000008585088 0000000000131104 b hist_entries
0000000008716192 0000000000016384 b hist_start

I'm unsure what fraction of processes would have use for these.

Yea, I'm not sure either. Although I suspect that given the cost of
compression having an "allocate on first use" check should be quite
doable.

I don't think the extra check would be a problem, but if we end up
needing the space in most processes, we're not really buying anything.
It'd need some investigation.

We could possibly fix these by changing the data structure so that
what's in a ScanKeywords entry is an offset into some giant string
constant somewhere. No idea how that would affect performance, but
I do notice that we could reduce the sizeof(ScanKeyword), which can't
hurt.

Yea, that might even help performancewise. Alternatively we could change
ScanKeyword to store the keyword name inline, but that'd be a measurable
size increase...

Yeah. It also seems like doing it this way would improve locality of
access: the pieces of the giant string would presumably be in the same
order as the ScanKeywords entries, whereas with the current setup,
who knows where the compiler has put 'em or in what order.

We'd need some tooling to generate the constants that way, though;
I can't see how to make it directly from kwlist.h.

regards, tom lane

Hi,

On 2018-10-15 16:54:53 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2018-10-15 16:36:26 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

0000000008585088 0000000000131104 b hist_entries
0000000008716192 0000000000016384 b hist_start

I'm unsure what fraction of processes would have use for these.

Yea, I'm not sure either. Although I suspect that given the cost of
compression having an "allocate on first use" check should be quite
doable.

I don't think the extra check would be a problem, but if we end up
needing the space in most processes, we're not really buying anything.
It'd need some investigation.

I don't think it's particularly uncommon to have processes that don't
use any toasted datums. I'm not sure however how to put numbers on
that? After all, it'll be drastically workload dependant.

We could possibly fix these by changing the data structure so that
what's in a ScanKeywords entry is an offset into some giant string
constant somewhere. No idea how that would affect performance, but
I do notice that we could reduce the sizeof(ScanKeyword), which can't
hurt.

Yea, that might even help performancewise. Alternatively we could change
ScanKeyword to store the keyword name inline, but that'd be a measurable
size increase...

Yeah. It also seems like doing it this way would improve locality of
access: the pieces of the giant string would presumably be in the same
order as the ScanKeywords entries, whereas with the current setup,
who knows where the compiler has put 'em or in what order.

I assume you're talking about the offset approach. Performancewise I
assume that my suggestion of inlining the names into the struct would be
faster. Are there many realistic cases where performance matters enough
to warrant the size increase?

We'd need some tooling to generate the constants that way, though;
I can't see how to make it directly from kwlist.h.

I guess we could create a struct with all the strings as members. But
that seems fairly nasty.

Greetings,

Andres Freund

On Tue, Oct 16, 2018 at 9:07 AM Andres Freund <andres@anarazel.de> wrote:

$ nm -t d --size-sort -r -S src/backend/postgres|grep '\b[bB]\b'|head
0000000008251872 0000000000131144 b LagTracker

...

So we have a two variables sized 130kb. Yikes.

...

that's not actually used very often, nor in all processes... Thomas?

Yeah, here's a patch to move it in to the heap.

--
Thomas Munro
http://www.enterprisedb.com

Andres Freund <andres@anarazel.de> writes:

On 2018-10-15 16:54:53 -0400, Tom Lane wrote:

Yeah. It also seems like doing it this way would improve locality of
access: the pieces of the giant string would presumably be in the same
order as the ScanKeywords entries, whereas with the current setup,
who knows where the compiler has put 'em or in what order.

I assume you're talking about the offset approach. Performancewise I
assume that my suggestion of inlining the names into the struct would be
faster. Are there many realistic cases where performance matters enough
to warrant the size increase?

Doubt it, because there'd be an awful lot of wasted space due to the need
to set the struct size large enough for the longest keyword. (Plus it
would likely not come out to be a power-of-2 size, slowing array
indexing.) If you want this to be cache-friendly, I'd think the smaller
the better.

regards, tom lane

On 2018-10-16 10:16:34 +1300, Thomas Munro wrote:

On Tue, Oct 16, 2018 at 9:07 AM Andres Freund <andres@anarazel.de> wrote:

$ nm -t d --size-sort -r -S src/backend/postgres|grep '\b[bB]\b'|head
0000000008251872 0000000000131144 b LagTracker

...

So we have a two variables sized 130kb. Yikes.

...

that's not actually used very often, nor in all processes... Thomas?

Yeah, here's a patch to move it in to the heap.

--
Thomas Munro
http://www.enterprisedb.com

From f3fb64e67c1e86e11dfafc8a712e9750f650f60b Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Tue, 16 Oct 2018 10:08:57 +1300
Subject: [PATCH] Move the replication lag tracker into heap memory.

Andres Freund complained about the 128KB of .bss occupied by LagTracker.
It's only needed in the walsender process, so allocate it in heap
memory there.

Cool, let's do that.

Greetings,

Andres Freund

On Tue, Oct 16, 2018 at 10:57 AM Andres Freund <andres@anarazel.de> wrote:

Subject: [PATCH] Move the replication lag tracker into heap memory.

Andres Freund complained about the 128KB of .bss occupied by LagTracker.
It's only needed in the walsender process, so allocate it in heap
memory there.

Cool, let's do that.

Pushed.

--
Thomas Munro
http://www.enterprisedb.com

I wrote:

Andres Freund <andres@anarazel.de> writes:

0000000008560224 0000000000023440 b tzdefrules_s
0000000008536704 0000000000023440 b gmtmem.7009

I think that tzdefrules_s is not used in common cases (though I could be
wrong about that), so we could win by alloc-on-first-use. The same might
be true for gmtmem, but there's a sticking point: there is no provision
for failure there, so I'm unsure how we avoid crashing on OOM.

So my intuition was exactly backwards on this.

1. tzdefrules_s is filled in the postmaster, and if it's not filled there
it'd be filled by every child process, so there's zero point in converting
it to malloc-on-the-fly style. This is because tzparse() always wants
it filled. That's actually a tad annoying, because it looks to me like
with many timezone settings the data will not get used, but I'm hesitant
to mess with the IANA code's logic enough to improve that. Maybe I'll try
submitting an upstream patch and see what they think of it first.

2. gmtmem, on the other hand, is only used if somebody calls pg_gmtime,
which already has a perfectly good error-report convention, cf gmtime(3).
We have exactly one caller, which was not bothering to test for error :-(
and would dump core on failure. And that caller isn't reached in most
processes, at least not in the regression tests. So this side of it is
easy to improve.

Hence I propose the attached patch for point 2, which I'd want to
backpatch to keep our copies of the IANA code in sync. Point 1
maybe can be addressed in future.

regards, tom lane

Hi,

I've a patch making .data variables constant, fwiw. Will post in a
bit. Just so we don't duplicate work too much.

On 2018-10-15 19:41:49 -0400, Tom Lane wrote:

I wrote:

Andres Freund <andres@anarazel.de> writes:

0000000008560224 0000000000023440 b tzdefrules_s
0000000008536704 0000000000023440 b gmtmem.7009

I think that tzdefrules_s is not used in common cases (though I could be
wrong about that), so we could win by alloc-on-first-use. The same might
be true for gmtmem, but there's a sticking point: there is no provision
for failure there, so I'm unsure how we avoid crashing on OOM.

So my intuition was exactly backwards on this.

1. tzdefrules_s is filled in the postmaster, and if it's not filled there
it'd be filled by every child process, so there's zero point in converting
it to malloc-on-the-fly style. This is because tzparse() always wants
it filled. That's actually a tad annoying, because it looks to me like
with many timezone settings the data will not get used, but I'm hesitant
to mess with the IANA code's logic enough to improve that. Maybe I'll try
submitting an upstream patch and see what they think of it first.

Ok, that makes sense.

2. gmtmem, on the other hand, is only used if somebody calls pg_gmtime,
which already has a perfectly good error-report convention, cf gmtime(3).
We have exactly one caller, which was not bothering to test for error :-(
and would dump core on failure. And that caller isn't reached in most
processes, at least not in the regression tests. So this side of it is
easy to improve.

Hence I propose the attached patch for point 2, which I'd want to
backpatch to keep our copies of the IANA code in sync.

That makes sense, too.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

top unitialized allocations:
0000000008435040 0000000000085280 b DCHCache
0000000008391168 0000000000043840 b NUMCache

Here's a patch to improve that situation.

regards, tom lane

Hi,

On 2018-10-15 16:45:03 -0700, Andres Freund wrote:

I've a patch making .data variables constant, fwiw. Will post in a
bit. Just so we don't duplicate work too much.

I pushed a few very obvious ones.

An additional fix, for the system attributes in heap.c, is attached. But
that's a bit more invasive, so I wanted to get some input.

I think all of the changes are fairly obvious and the new const
attributes just document what already had to be the case. But to
correctly propagate the const through typedef-the-pointer-away typedefs,
I had to use the underlying type (e.g. use const NameData *n2 instad of
Name n2). To me that's the correct fix, but then I hate these typedefs
with a passion anyway. An alternative fix is to add additional
typedefs like ConstName, but I don't find that particularly
advantageous.

Comments?

Arguably we should use consts more aggresively in signatures anyway, but
I think that's a separate effort.

Greetings,

Andres Freund

Hi,

After the last few changes (including a proposed one), we now have in
the .data segment (i.e. mutable initialized variables):

$ objdump -j .data -t ~/build/postgres/dev-optimize/vpath/src/backend/postgres|awk '{print $4, $5, $6}'|sort -r|lessGreetings,
.data 0000000000004380 ConfigureNamesInt
.data 0000000000003570 ConfigureNamesBool
.data 0000000000002140 ConfigureNamesString
.data 00000000000010e0 ConfigureNamesEnum
.data 0000000000000d20 ConfigureNamesReal

These are by far the largest chunk of the .data segment (like 47152
bytes out of 51168 bytes). It's not entirely trivial to fix. While we
can slap consts onto large parts of the functions taking a struct
config_generic*, there's a few places where *do* modify them.

ISTM, to actually fix, we'd have to split
struct config_generic
{
/* constant fields, must be set correctly in initial value: */
const char *name; /* name of variable - MUST BE FIRST */
GucContext context; /* context required to set the variable */
enum config_group group; /* to help organize variables by function */
const char *short_desc; /* short desc. of this variable's purpose */
const char *long_desc; /* long desc. of this variable's purpose */
int flags; /* flag bits, see guc.h */
/* variable fields, initialized at runtime: */
enum config_type vartype; /* type of variable (set only at startup) */
int status; /* status bits, see below */
GucSource source; /* source of the current actual value */
GucSource reset_source; /* source of the reset_value */
GucContext scontext; /* context that set the current value */
GucContext reset_scontext; /* context that set the reset value */
GucStack *stack; /* stacked prior values */
void *extra; /* "extra" pointer for current actual value */
char *sourcefile; /* file current setting is from (NULL if not
* set in config file) */
int sourceline; /* line in source file */
};

into two different arrays. Namely the already denoted 'constant fields'
and 'variable fields. But it's a bit more complicated than that: The
size doesn't come just from the base config_struct, but also from the
config_{bool,int,real,string,enum} arrays. Where we again have separate
'constant fields' and 'variable fields'.

So we'd have to refactor more heavily than just splitting the above
array into two.

I kind of wonder, if we shouldn't remove the separate ConfigureNamesInt*
int arrays and change things so we have one

struct config_def
{
const char *name; /* name of variable - MUST BE FIRST */
GucContext context; /* context required to set the variable */
enum config_group group; /* to help organize variables by function */
const char *short_desc; /* short desc. of this variable's purpose */
const char *long_desc; /* long desc. of this variable's purpose */
int flags; /* flag bits, see guc.h */
enum config_type vartype; /* type of variable (set only at startup) */

union
{
struct
{
bool *variable;
bool boot_val;
GucBoolCheckHook check_hook;
GucBoolAssignHook assign_hook;
GucShowHook show_hook;
} config_bool;

struct
{
int *variable;
int boot_val;
int min;
int max;
GucIntCheckHook check_hook;
GucIntAssignHook assign_hook;
GucShowHook show_hook;
} config_int

...
} pertype;
}

and then a corresponding struct config_val with a similar union.

Besides reducing modified memory, it'd also have the benefit of making
the grouping within the various arrays much more sensible, because
related fields could be grouped together.

.data 0000000000000420 intRelOpts
.data 00000000000001c0 realRelOpts
.data 0000000000000118 boolRelOpts
.data 00000000000000a8 stringRelOpts

these should be readonly, but the code doesn't make that easy. There's
pending refactorings, I don't know how they effect this.

.data 0000000000000068 SnapshotSelfData
.data 0000000000000068 SnapshotAnyData
.data 0000000000000068 SecondarySnapshotData
.data 0000000000000068 CurrentSnapshotData
.data 0000000000000068 CatalogSnapshotData

The obviously actually are modifyable.

.data 0000000000000028 spi_printtupDR
.data 0000000000000028 printsimpleDR
.data 0000000000000028 donothingDR
.data 0000000000000028 debugtupDR

These we could actually make constant, but CreateDestReceiver() as an
API makes that inconvenient. They also are pretty darn small... There's
a security benefit in making them constant and casting the constness
away - I think that might not be insane.

- Andres

Hi,

On 2018-10-15 21:50:51 -0700, Andres Freund wrote:

.data 0000000000000028 spi_printtupDR
.data 0000000000000028 printsimpleDR
.data 0000000000000028 donothingDR
.data 0000000000000028 debugtupDR

These we could actually make constant, but CreateDestReceiver() as an
API makes that inconvenient. They also are pretty darn small... There's
a security benefit in making them constant and casting the constness
away - I think that might not be insane.

I.e. do something like the attached.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

[ let's rearrange the GUC structs ]

I find it hard to believe that the API breaks you'll cause for extensions
are a good trade for a few kB reduction in the size of the .data segment.

regards, tom lane

I wrote:

Andres Freund <andres@anarazel.de> writes:

top unitialized allocations:
0000000008435040 0000000000085280 b DCHCache
0000000008391168 0000000000043840 b NUMCache

Here's a patch to improve that situation.

Hm, looking at that more closely, there's a problem with the idea of
allocating the cache slots one-at-a-time. Currently,
sizeof(DCHCacheEntry) and sizeof(NUMCacheEntry) are each just a bit more
than a power of 2, which would cause palloc to waste nearly 50% of the
allocation it makes for them.

We could forget the one-at-a-time idea and just allocate the whole
array on first use, but I feel like that's probably not a good answer.
I'm inclined to instead reduce DCH_CACHE_SIZE and NUM_CACHE_SIZE enough
to make the structs just less than powers of 2 instead of just more ---
AFAICS, both of those numbers were picked out of the air, and so there's
no reason not to pick a different number out of the air.

Also, I noticed that the biggest part of those structs are arrays of
FormatNode, which has been designed with complete lack of thought about
size or padding issues. We can very easily cut it in half on 64-bit
machines.

The attached patch does both of those things. It's independent of
the other one but is important to make that one efficient.

regards, tom lane

Hi,

On 2018-10-16 01:49:22 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

[ let's rearrange the GUC structs ]

I find it hard to believe that the API breaks you'll cause for extensions
are a good trade for a few kB reduction in the size of the .data segment.

I'm doubtful that it's worth it too. But more because it seems like a
fair bit of work. I don't think that many extensions use guc_tables.h -
shouldn't most use guc.h, which wouldn't be affected?

Greetings,

Andres Freund

On 2018-10-15 22:02:00 -0700, Andres Freund wrote:

Hi,

On 2018-10-15 21:50:51 -0700, Andres Freund wrote:

.data 0000000000000028 spi_printtupDR
.data 0000000000000028 printsimpleDR
.data 0000000000000028 donothingDR
.data 0000000000000028 debugtupDR

These we could actually make constant, but CreateDestReceiver() as an
API makes that inconvenient. They also are pretty darn small... There's
a security benefit in making them constant and casting the constness
away - I think that might not be insane.

I.e. do something like the attached.

This just reminded me that a couple times I wanted a cast that casts
away const, but otherwise makes sure the type stays the same. I don't
think there's a way to do that in C, but we can write one that verifies
the cast doesn't do something bad if gcc is used:

#if defined(HAVE__BUILTIN_TYPES_COMPATIBLE_P)
#define unconstify(cst, var) StaticAssertExpr(__builtin_types_compatible_p(__typeof(var), const cst), "wrong cast"), (cst) (var)
#else
#define unconstify(cst, var) ((cst) (var))
#endif

Does anybody besides me see value in adding a cleaned up version of
that?

Greetings,

Andres Freund

Hi,

On 2018-10-16 01:59:00 -0400, Tom Lane wrote:

I wrote:

Andres Freund <andres@anarazel.de> writes:

top unitialized allocations:
0000000008435040 0000000000085280 b DCHCache
0000000008391168 0000000000043840 b NUMCache

Here's a patch to improve that situation.

Hm, looking at that more closely, there's a problem with the idea of
allocating the cache slots one-at-a-time. Currently,
sizeof(DCHCacheEntry) and sizeof(NUMCacheEntry) are each just a bit more
than a power of 2, which would cause palloc to waste nearly 50% of the
allocation it makes for them.

Hm, that's a bit annoying...

We could forget the one-at-a-time idea and just allocate the whole
array on first use, but I feel like that's probably not a good answer.

I suspect it'd be fine, but obviously we can do better.

Also, I noticed that the biggest part of those structs are arrays of
FormatNode, which has been designed with complete lack of thought about
size or padding issues. We can very easily cut it in half on 64-bit
machines.

Heh, neat. I feel like we've paid very little attention to that in a
myriad of places :(

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2018-10-16 01:59:00 -0400, Tom Lane wrote:

Also, I noticed that the biggest part of those structs are arrays of
FormatNode, which has been designed with complete lack of thought about
size or padding issues. We can very easily cut it in half on 64-bit
machines.

Heh, neat. I feel like we've paid very little attention to that in a
myriad of places :(

Most of the time, we probably *shouldn't* pay attention to it; logical
field ordering is worth a good deal IMO. But in a case like this,
where there are large arrays of the things and it's not very painful
to avoid padding waste, it's worth the trouble.

regards, tom lane

On Tue, Oct 16, 2018 at 2:30 AM Andres Freund <andres@anarazel.de> wrote:

This just reminded me that a couple times I wanted a cast that casts
away const, but otherwise makes sure the type stays the same. I don't
think there's a way to do that in C, but we can write one that verifies
the cast doesn't do something bad if gcc is used:

#if defined(HAVE__BUILTIN_TYPES_COMPATIBLE_P)
#define unconstify(cst, var) StaticAssertExpr(__builtin_types_compatible_p(__typeof(var), const cst), "wrong cast"), (cst) (var)
#else
#define unconstify(cst, var) ((cst) (var))
#endif

Does anybody besides me see value in adding a cleaned up version of
that?

Under what circumstances would we consider this to be a legitimate thing to use?

I think if we add something this, we'd better accompany it with some
detailed and very clearly-written statements about when you're allowed
to use it. Otherwise, I predict that people will use it in cases
where it's not actually safe, and we'll end up with low-grade bugs.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2018-10-16 11:22:31 -0400, Robert Haas wrote:

On Tue, Oct 16, 2018 at 2:30 AM Andres Freund <andres@anarazel.de> wrote:

This just reminded me that a couple times I wanted a cast that casts
away const, but otherwise makes sure the type stays the same. I don't
think there's a way to do that in C, but we can write one that verifies
the cast doesn't do something bad if gcc is used:

#if defined(HAVE__BUILTIN_TYPES_COMPATIBLE_P)
#define unconstify(cst, var) StaticAssertExpr(__builtin_types_compatible_p(__typeof(var), const cst), "wrong cast"), (cst) (var)
#else
#define unconstify(cst, var) ((cst) (var))
#endif

Does anybody besides me see value in adding a cleaned up version of
that?

Under what circumstances would we consider this to be a legitimate thing to use?

When the variable actually *will not* be modified, but language or API
design reasons makes it unfeasiable to express that. Look e.g.
DestReceiver * CreateDestReceiver(CommandDest dest);
some of the returned receivers (e.g. donothingDR, printsimpleDR) are
statically allocated and *any* modification would be a bug. But other
return values will be modified, e.g. CreateIntoRelDestReceiver().

It's safe to cast constness away if the variable will not actually be
modified after. Which is e.g. the case above. But making the static
allocations const will a) save memory b) trigger sigbuses if you modify
them. So the casting constness away here *increases* robustness.

The problem is that just adding a cast like
case DestNone:
return (DestReceiver *) &donothingDR;
also hides errors. If you e.g. changed the type of donothingDR you'd
still not get an error.

So I was wishing for a form of a cast that only casts the const away,
but errors out if there's any other type difference. That's the above, I
think.

I think if we add something this, we'd better accompany it with some
detailed and very clearly-written statements about when you're allowed
to use it. Otherwise, I predict that people will use it in cases
where it's not actually safe, and we'll end up with low-grade bugs.

Well, right now people will (and have) just cast the const away like
above. So I don't really see it being more likely to cause problems than
we're doing now. But yea, it definitely should have a big red warning
label.

Greetings,

Andres Freund

On Tue, Oct 16, 2018 at 12:06 PM Andres Freund <andres@anarazel.de> wrote:

But yea, it definitely should have a big red warning
label.

That's all I'm asking. And the warning label shouldn't just say "use
with caution!" but should rather explain how to know whether you're
doing the right thing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2018-10-16 12:19:55 -0400, Robert Haas wrote:

On Tue, Oct 16, 2018 at 12:06 PM Andres Freund <andres@anarazel.de> wrote:

But yea, it definitely should have a big red warning
label.

That's all I'm asking. And the warning label shouldn't just say "use
with caution!" but should rather explain how to know whether you're
doing the right thing.

I'm thinking of something like:

/*
* Macro that allows to cast constness away from a variable, but doesn't
* allow changing the underlying type. Enforcement of the latter
* currently only works for gcc like compilers.
*
* Please note IT IS NOT SAFE to cast constness away if the variable will ever
* be modified (it would be undefined behaviour). Doing so anyway can cause
* compiler misoptimizations or runtime crashes (modifying readonly memory).
* It is only safe to use when the the variabble will not be modified, but API
* design or language restrictions prevent you from declaring that
* (e.g. because a function returns both const and non-const variables).
*/

Greetings,

Andres Freund

On Tue, Oct 16, 2018 at 12:26 PM Andres Freund <andres@anarazel.de> wrote:

/*
* Macro that allows to cast constness away from a variable, but doesn't
* allow changing the underlying type. Enforcement of the latter
* currently only works for gcc like compilers.
*
* Please note IT IS NOT SAFE to cast constness away if the variable will ever
* be modified (it would be undefined behaviour). Doing so anyway can cause
* compiler misoptimizations or runtime crashes (modifying readonly memory).
* It is only safe to use when the the variabble will not be modified, but API
* design or language restrictions prevent you from declaring that
* (e.g. because a function returns both const and non-const variables).
*/

"variabble" is a little too rich in "b"s.

In terms of a function that returns both const and non-const
variables, it seems a bit sketchy that the caller would know what the
function is doing in particular cases and make decisions based on it,
but maybe that's just how life is.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2018-10-16 12:41:44 -0400, Robert Haas wrote:

On Tue, Oct 16, 2018 at 12:26 PM Andres Freund <andres@anarazel.de> wrote:

/*
* Macro that allows to cast constness away from a variable, but doesn't
* allow changing the underlying type. Enforcement of the latter
* currently only works for gcc like compilers.
*
* Please note IT IS NOT SAFE to cast constness away if the variable will ever
* be modified (it would be undefined behaviour). Doing so anyway can cause
* compiler misoptimizations or runtime crashes (modifying readonly memory).
* It is only safe to use when the the variabble will not be modified, but API
* design or language restrictions prevent you from declaring that
* (e.g. because a function returns both const and non-const variables).
*/

"variabble" is a little too rich in "b"s.

variababble.

In terms of a function that returns both const and non-const
variables, it seems a bit sketchy that the caller would know what the
function is doing in particular cases and make decisions based on it,
but maybe that's just how life is.

I don't think it's necessary the callers doing so in most cases. E.g. in
the DestReceiver case, it'll be the choice of the testreceiver (say
intorel_receive modifying things for DestIntoRel), not the caller
choosing when to modify things. The caller / users of dest receivers
won't necessarily know.

I agree it's not pretty, but I don't quite see any really realistic
other approaches here.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2018-10-16 12:41:44 -0400, Robert Haas wrote:

In terms of a function that returns both const and non-const
variables, it seems a bit sketchy that the caller would know what the
function is doing in particular cases and make decisions based on it,
but maybe that's just how life is.

I don't think it's necessary the callers doing so in most cases. E.g. in
the DestReceiver case, it'll be the choice of the testreceiver (say
intorel_receive modifying things for DestIntoRel), not the caller
choosing when to modify things. The caller / users of dest receivers
won't necessarily know.

Yeah, I think the use-case is more like "this API specifies non-const
pointers, but some instances can return pointers to const objects,
while others return non-const objects". Changing the API to const
isn't better, it just moves where you have to cast away const.

regards, tom lane

On Mon, Oct 15, 2018 at 4:08 PM Andres Freund <andres@anarazel.de> wrote:

So we have 500kb of not-initialized memory mapped into every
process. That's, uh, not nothing.

Thinking about this a bit more, why is this bad? I mean, if the
memory is never touched, the OS does not really need to allocate or
zero any pages, or even make any page table entries. If somebody
actually accesses the data, then we'll take a page fault and have to
really allocate, but otherwise I would think we could have 50MB of
unused bss floating around and it wouldn't really matter, let alone
500kB.

What am I missing?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2018-10-15 13:07:54 -0700, Andres Freund wrote:

Hi,

while briefly thinking about the per-process overhead of postgres, I
looked at the section sizes of a modern postgres. In particular which
memory areas are *not* shared between processes.

If you look at the section sizes that are mapped read-write:

$ size --format=sysv src/backend/postgres
src/backend/postgres :
section size addr
.rodata 1593617 5730304 (read-only, for reference)
.data.rel.ro 134944 8039904 (read only after start)
.data 53888 8178912 (read-write, initialized)
.bss 510416 8232800 (read-write, uninitialized)
Total 52417197

We improve a fair bit over the last two days:

text data bss dec hex filename
8030289 191888 227024 8449201 80ecb1 src/backend/postgres
8029097 192880 510416 8732393 853ee9 src/backend/postgres.before

breakdown:
section size addr
.rodata 1594577 5730304 (read-only, for reference)
.data.rel.ro 136928 8037920 (read only after start)
.data 50912 8178912 (read-write, initialized)
.bss 227024 8229824 (read-write, uninitialized)

As to be expected .rodata and .data.rel.ro gained a bit in size (as they
contain constant data and we made more vars constant), whereas .data
shrunk a bit, and .bss shrunk drastically by moving things to be
dynamically allocated.

Nice progress, thanks everyone!

The top .bss entries (uninitialized modifyable vars) are now:
.bss 0000000000020020 hist_entries
.bss 0000000000005b90 tzdefrules_s
.bss 0000000000004000 hist_start
.bss 0000000000002000 PqRecvBuffer
.bss 0000000000001410 BackendWritebackContext
.bss 0000000000000c80 held_lwlocks
.bss 0000000000000b00 re_array
.bss 0000000000000800 buffer_lists
.bss 0000000000000600 syscache_callback_list
.bss 0000000000000560 reverse_dispatch_table
.bss 0000000000000400 tzdir.7228
.bss 0000000000000400 pkglib_path
.bss 0000000000000400 OutputFileName
.bss 0000000000000400 my_exec_path
.bss 0000000000000400 ExtraOptions
.bss 0000000000000398 errordata
.bss 0000000000000320 seq_scan_tables

The top .data entries (initialized modifyable vars) are now:
.data 0000000000004380 ConfigureNamesInt
.data 0000000000003570 ConfigureNamesBool
.data 0000000000002140 ConfigureNamesString
.data 00000000000010e0 ConfigureNamesEnum
.data 0000000000000d20 ConfigureNamesReal
.data 0000000000000420 intRelOpts
.data 00000000000001c0 realRelOpts
.data 0000000000000118 boolRelOpts
.data 00000000000000a8 stringRelOpts
.data 0000000000000068 SnapshotSelfData
.data 0000000000000068 SnapshotAnyData
.data 0000000000000068 SecondarySnapshotData
.data 0000000000000068 CurrentSnapshotData
.data 0000000000000068 CatalogSnapshotData

- Andres

Hi,

On 2018-10-16 15:13:43 -0400, Robert Haas wrote:

On Mon, Oct 15, 2018 at 4:08 PM Andres Freund <andres@anarazel.de> wrote:

So we have 500kb of not-initialized memory mapped into every
process. That's, uh, not nothing.

Thinking about this a bit more, why is this bad? I mean, if the
memory is never touched, the OS does not really need to allocate or
zero any pages, or even make any page table entries. If somebody
actually accesses the data, then we'll take a page fault and have to
really allocate, but otherwise I would think we could have 50MB of
unused bss floating around and it wouldn't really matter, let alone
500kB.

What am I missing?

For one the OS will actually reserve all that memory when you use memory
overcommit = 2 (which you should). There's no need to reserve memory for
data that's guaranteed to be shared. It also reduces the frequency of
page faults when looking at other global variables laid out nearby - we
could do something about that by giving the linker placement
instructions, but that's also work. The size of the pagetable also has
performance effects, although it'll often be dwarfed by the shared
buffers entry if not using huge pages. WRT .data entries, marking them
as const where appropriate both reduces memory usage, possibly allows
more compiler optimizations, and allows to catch bugs.

There's also the fact that not every OS has COW pagetables, or we use
processes in a way that don't allow that (say windows, where we don't
fork).

Greetings,

Andres Freund

On 2018-10-16 10:16:33 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2018-10-16 01:59:00 -0400, Tom Lane wrote:

Also, I noticed that the biggest part of those structs are arrays of
FormatNode, which has been designed with complete lack of thought about
size or padding issues. We can very easily cut it in half on 64-bit
machines.

Heh, neat. I feel like we've paid very little attention to that in a
myriad of places :(

Most of the time, we probably *shouldn't* pay attention to it; logical
field ordering is worth a good deal IMO.

Sure. But there's plenty structs which we allocate a bunch off, that are
frequently accessed, where a lot of space is wasted to padding. I agree
that we don't need to contort many structs, but there's plenty where we
should. Often enough it's possible to reorder without making things
make meaningfully less sense.

But in a case like this,
where there are large arrays of the things and it's not very painful
to avoid padding waste, it's worth the trouble.

Attached is a patch that shrinks fmgr_builtins by 25%. That seems
worthwhile, it's pretty frequently accessed, making it more dense is
helpful. Unless somebody protests soon, I'm going to apply that...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

Attached is a patch that shrinks fmgr_builtins by 25%. That seems
worthwhile, it's pretty frequently accessed, making it more dense is
helpful. Unless somebody protests soon, I'm going to apply that...

Hah. I'm pretty sure that struct *was* set up with an eye to padding ...
on 32-bit machines. This does make it shorter on 64-bit, but also
makes the size not a power of 2, which might add a few cycles to
array indexing calculations. Might be worth checking whether that's
going to be an issue anywhere.

What's the point of the extra const decoration on funcName? ISTM
the whole struct should be const, or not.

regards, tom lane

On 17/10/2018 09:36, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Attached is a patch that shrinks fmgr_builtins by 25%. That seems
worthwhile, it's pretty frequently accessed, making it more dense is
helpful. Unless somebody protests soon, I'm going to apply that...

Hah. I'm pretty sure that struct *was* set up with an eye to padding ...
on 32-bit machines. This does make it shorter on 64-bit, but also
makes the size not a power of 2, which might add a few cycles to
array indexing calculations. Might be worth checking whether that's
going to be an issue anywhere.

What's the point of the extra const decoration on funcName? ISTM
the whole struct should be const, or not.

regards, tom lane

Would it be useful to add dummy variable(s) to bring it up to a power of 2?

Cheers,
Gavin

Hi,

On 2018-10-17 09:38:18 +1300, Gavin Flower wrote:

On 17/10/2018 09:36, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Attached is a patch that shrinks fmgr_builtins by 25%. That seems
worthwhile, it's pretty frequently accessed, making it more dense is
helpful. Unless somebody protests soon, I'm going to apply that...

Hah. I'm pretty sure that struct *was* set up with an eye to padding ...
on 32-bit machines. This does make it shorter on 64-bit, but also
makes the size not a power of 2, which might add a few cycles to
array indexing calculations. Might be worth checking whether that's
going to be an issue anywhere.

What's the point of the extra const decoration on funcName? ISTM
the whole struct should be const, or not.

Would it be useful to add dummy variable(s) to bring it up to a power of 2?

Err. Reread what we're talking about. The point was to reduce the size,
it's a power of two right now (32). We could of course also just do
nothing (re-add a dummy variable), which would, drumroll, do nothing.

Greetings,

Andres Freund

On 2018-10-16 16:36:12 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Attached is a patch that shrinks fmgr_builtins by 25%. That seems
worthwhile, it's pretty frequently accessed, making it more dense is
helpful. Unless somebody protests soon, I'm going to apply that...

Hah. I'm pretty sure that struct *was* set up with an eye to padding ...
on 32-bit machines.

Possible, the new layout should work just as well there, luckily.

This does make it shorter on 64-bit, but also
makes the size not a power of 2, which might add a few cycles to
array indexing calculations. Might be worth checking whether that's
going to be an issue anywhere.

I can't imagine that that outweight the cost of additional cache misses
on any platform where performance matters. On x86 I assume indexing
into an array with 24byte stride, will normally be just two leas (lea
eax, [eax + eax * 2]; lea eax, [ebx + eax * 8]; where eax initially is
the index, and ebx the array base). Indexing also plays less of a role
than in the past, because previously we did a binary search, but now we
normally look up the index via fmgr_builtin_oid_index.

What's the point of the extra const decoration on funcName? ISTM
the whole struct should be const, or not.

The whole array is const. There's cases where that allows the compiler
more freedom - but I guess it's really a bit redundant here.

Greetings,

Andres Freund

I wrote:

1. tzdefrules_s is filled in the postmaster, and if it's not filled there
it'd be filled by every child process, so there's zero point in converting
it to malloc-on-the-fly style. This is because tzparse() always wants
it filled. That's actually a tad annoying, because it looks to me like
with many timezone settings the data will not get used, but I'm hesitant
to mess with the IANA code's logic enough to improve that. Maybe I'll try
submitting an upstream patch and see what they think of it first.

I spent some time studying this, and realized that the reason that they
always try to load the TZDEFRULES zone data is that they want to absorb
whatever leap-seconds data it has. We don't want that and would be glad
to just drop said data, but I'm sure that's intentional on their end,
so they are not going to be interested in a patch here.

However ... if you assume that any leap-seconds data in that zone can
be ignored, then the only case where the data need be loaded is when
parsing a POSIX-style zone name that has a DST component but does not
specify a POSIX-style transition date rule. That's possible in our code
but I do not think it is a mainstream case; in particular, that seems
never to be reached when loading a real zone data file.

Hence, the attached patch postpones the TZDEFRULES load until we find
we actually need it, and ignores any leap-second data therein, and
incidentally makes the data space be malloc-on-demand rather than static.

This is actually less of a hack compared to the upstream code than what
we were doing before, so I feel pretty pleased with it.

regards, tom lane

BTW, I looked around for .o files with large BSS numbers, and came across

$ size src/interfaces/ecpg/ecpglib/prepare.o
text data bss dec hex filename
4023 4 1048576 1052603 100fbb src/interfaces/ecpg/ecpglib/prepare.o

That megabyte is from a statically allocated statement cache array.
Seems a bit unfriendly to users of ecpglib, given that many apps
would never use the statement cache (AFAICT you have to explicitly
ask for auto-prepare mode to get to that code).

Doesn't look hard to fix though.

regards, tom lane

On 2018-10-16 19:48:42 -0400, Tom Lane wrote:

BTW, I looked around for .o files with large BSS numbers, and came across

$ size src/interfaces/ecpg/ecpglib/prepare.o
text data bss dec hex filename
4023 4 1048576 1052603 100fbb src/interfaces/ecpg/ecpglib/prepare.o

Btw, I think one can fairly argue that large .data vars have a
higher cost than .bss. That's not to say we shouldn't fix .bss ones ;)

I've this awful shell script to figure out the sizes:

for file in $(find . -type f -name '*.o'); do objdump -j .data -t $file|tail -n +5|grep -v '^$'|awk -v "file=$file" '{print $4, $5, $6, file}';done|sort -nr|less

.data 0000000000001180 datetktbl ./src/interfaces/ecpg/pgtypeslib/dt_common.o
.data 0000000000000c28 ibmkanji ./src/backend/utils/mb/conversion_procs/euc_jp_and_sjis/euc_jp_and_sjis.o
.data 00000000000003f0 deltatktbl ./src/interfaces/ecpg/pgtypeslib/dt_common.o
.data 0000000000000100 sqlca_init ./src/interfaces/ecpg/ecpglib/misc.o
.data 0000000000000100 sqlca_init ./src/interfaces/ecpg/compatlib/informix.o
.data 0000000000000068 day_tab ./src/interfaces/ecpg/pgtypeslib/dt_common.o
.data 0000000000000030 ecpg_query ./src/interfaces/ecpg/preproc/preproc.o
.data 0000000000000030 ecpg_no_indicator ./src/interfaces/ecpg/preproc/preproc.o
.data 0000000000000030 descriptor_type.5770 ./src/interfaces/ecpg/preproc/descriptor.o
.data 000000000000002b ssl_nomem ./src/interfaces/libpq/fe-secure-openssl.o
.data 000000000000002a PLy_subtransaction_doc ./src/pl/plpython/plpy_subxactobject.o
.data 0000000000000023 PLy_cursor_doc ./src/pl/plpython/plpy_cursorobject.o
.data 000000000000001e PLy_result_doc ./src/pl/plpython/plpy_resultobject.o
.data 0000000000000018 PLy_plan_doc ./src/pl/plpython/plpy_planobject.o
.data 0000000000000010 base_yylloc ./src/interfaces/ecpg/preproc/preproc.o

Should also work for other sections.

Hm, there's a lot of ecpg in this :(

That megabyte is from a statically allocated statement cache array.
Seems a bit unfriendly to users of ecpglib, given that many apps
would never use the statement cache (AFAICT you have to explicitly
ask for auto-prepare mode to get to that code).

Doesn't look hard to fix though.

Thanks for tackling that.

Greetings,

Andres Freund

On 16/10/2018 08:30, Andres Freund wrote:

This just reminded me that a couple times I wanted a cast that casts
away const, but otherwise makes sure the type stays the same. I don't
think there's a way to do that in C, but we can write one that verifies
the cast doesn't do something bad if gcc is used:

#if defined(HAVE__BUILTIN_TYPES_COMPATIBLE_P)
#define unconstify(cst, var) StaticAssertExpr(__builtin_types_compatible_p(__typeof(var), const cst), "wrong cast"), (cst) (var)
#else
#define unconstify(cst, var) ((cst) (var))
#endif

Does anybody besides me see value in adding a cleaned up version of
that?

I've had the attached patch lying around. As you can see there, there
are a few places where it could be useful, but not too many.

The name CONST_CAST() is adapted from C++.

Your version with __builtin_types_compatible_p() doesn't work for
casting between char * and const char *, so it wouldn't be very useful.

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

Andres Freund <andres@anarazel.de> writes:

.data 0000000000001180 datetktbl ./src/interfaces/ecpg/pgtypeslib/dt_common.o
.data 0000000000000c28 ibmkanji ./src/backend/utils/mb/conversion_procs/euc_jp_and_sjis/euc_jp_and_sjis.o
.data 00000000000003f0 deltatktbl ./src/interfaces/ecpg/pgtypeslib/dt_common.o

Hmm. I think these can just be marked const, will investigate.

The rest of those look to be mostly below the threshold of pain ...

regards, tom lane

I wrote:

Andres Freund <andres@anarazel.de> writes:

.data 0000000000001180 datetktbl ./src/interfaces/ecpg/pgtypeslib/dt_common.o
.data 0000000000000c28 ibmkanji ./src/backend/utils/mb/conversion_procs/euc_jp_and_sjis/euc_jp_and_sjis.o
.data 00000000000003f0 deltatktbl ./src/interfaces/ecpg/pgtypeslib/dt_common.o

Hmm. I think these can just be marked const, will investigate.

I pushed fixes for these, but curiously, the ibmkanji change did not
make any difference to section sizes --- AFAICT, my toolchain already
figured out that it could treat that table as const. The dt_common.c
changes are a win though.

regards, tom lane

On 2018-10-17 21:06:13 +0200, Peter Eisentraut wrote:

On 16/10/2018 08:30, Andres Freund wrote:

This just reminded me that a couple times I wanted a cast that casts
away const, but otherwise makes sure the type stays the same. I don't
think there's a way to do that in C, but we can write one that verifies
the cast doesn't do something bad if gcc is used:

#if defined(HAVE__BUILTIN_TYPES_COMPATIBLE_P)
#define unconstify(cst, var) StaticAssertExpr(__builtin_types_compatible_p(__typeof(var), const cst), "wrong cast"), (cst) (var)
#else
#define unconstify(cst, var) ((cst) (var))
#endif

Does anybody besides me see value in adding a cleaned up version of
that?

I've had the attached patch lying around.

Heh.

As you can see there, there are a few places where it could be useful,
but not too many.

Yea.

The name CONST_CAST() is adapted from C++.

Your version with __builtin_types_compatible_p() doesn't work for
casting between char * and const char *, so it wouldn't be very useful.

Huh, why wouldn't it work for char *? Seems to work fine for me?

+/*
+ * Safer casting -- use this for casting away const or volatile.  It ensures
+ * that the source and target types are otherwise compatible.
+ */
+#define CONST_CAST(t, x) ((void)((t)0 == (x)), (t)(x))
+

Has the advantage that it probably works for globals, but OTOH, it only
works correctly for pointers, and it doesn't reliably trigger warnigns /
errors.

Greetings,

Andres Freund

On 17/10/2018 22:02, Andres Freund wrote:

Your version with __builtin_types_compatible_p() doesn't work for
casting between char * and const char *, so it wouldn't be very useful.

Huh, why wouldn't it work for char *? Seems to work fine for me?

__builtin_types_compatible_p(const char *, char *) returns false (0) for me.

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

Hi,

On 2018-10-17 23:43:31 +0200, Peter Eisentraut wrote:

On 17/10/2018 22:02, Andres Freund wrote:

Your version with __builtin_types_compatible_p() doesn't work for
casting between char * and const char *, so it wouldn't be very useful.

Huh, why wouldn't it work for char *? Seems to work fine for me?

__builtin_types_compatible_p(const char *, char *) returns false (0) for me.

Right, that's why I added a const, inside the macro, to the type
specified in the unconstify argument. So typeof() yields a const char *,
and the return type is specified as char *, and adding a const in the
argument also yields a const char *.

I'd merged my version since, I don't feel particularly attached to it
though...

Greetings,

Andres Freund

On 17/10/2018 23:51, Andres Freund wrote:

__builtin_types_compatible_p(const char *, char *) returns false (0) for me.

Right, that's why I added a const, inside the macro, to the type
specified in the unconstify argument. So typeof() yields a const char *,
and the return type is specified as char *, and adding a const in the
argument also yields a const char *.

Yeah, that works. The C++-inspired version also allowed casting from
not-const to const, which we don't really need.

I'd perhaps change the signature

#define unconstify(underlying_type, var)

because the "var" doesn't actually have to be a variable.

Attached is my previous patch adapted to your macro.

I'm tempted to get rid of the stuff in dfmgr.c and just let the "older
platforms" get the warning.

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

On 18/10/2018 22:17, Peter Eisentraut wrote:

On 17/10/2018 23:51, Andres Freund wrote:

__builtin_types_compatible_p(const char *, char *) returns false (0) for me.

Right, that's why I added a const, inside the macro, to the type
specified in the unconstify argument. So typeof() yields a const char *,
and the return type is specified as char *, and adding a const in the
argument also yields a const char *.

Yeah, that works. The C++-inspired version also allowed casting from
not-const to const, which we don't really need.

Attached is my previous patch adapted to your macro.

Oh, I forgot to mention, your version doesn't work for this code in
pqexpbuffer.c:

str->data = (char *) oom_buffer;

That's probably not a big deal though.

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

On 2018-10-18 22:20:29 +0200, Peter Eisentraut wrote:

On 18/10/2018 22:17, Peter Eisentraut wrote:

On 17/10/2018 23:51, Andres Freund wrote:

__builtin_types_compatible_p(const char *, char *) returns false (0) for me.

Right, that's why I added a const, inside the macro, to the type
specified in the unconstify argument. So typeof() yields a const char *,
and the return type is specified as char *, and adding a const in the
argument also yields a const char *.

Yeah, that works. The C++-inspired version also allowed casting from
not-const to const, which we don't really need.

Attached is my previous patch adapted to your macro.

Oh, I forgot to mention, your version doesn't work for this code in
pqexpbuffer.c:

str->data = (char *) oom_buffer;

That kind of seems correct ;). Can easily be done via unconstify(char *,
&oom_buffer[0]), if necessary.

That's probably not a big deal though.

Greetings,

Andres Freund

On 2018-10-18 22:17:55 +0200, Peter Eisentraut wrote:

I'd perhaps change the signature

#define unconstify(underlying_type, var)

because the "var" doesn't actually have to be a variable.

Hm, so expr, or what would you use?

Attached is my previous patch adapted to your macro.

I'm tempted to get rid of the stuff in dfmgr.c and just let the "older
platforms" get the warning.

Yea, that works for me.

Are you planning to apply this?

Greetings,

Andres Freund

On 18/10/2018 21:31, Andres Freund wrote:

On 2018-10-18 22:17:55 +0200, Peter Eisentraut wrote:

I'd perhaps change the signature

#define unconstify(underlying_type, var)

because the "var" doesn't actually have to be a variable.

Hm, so expr, or what would you use?

done

Attached is my previous patch adapted to your macro.

I'm tempted to get rid of the stuff in dfmgr.c and just let the "older
platforms" get the warning.

and done

Yea, that works for me.

Are you planning to apply this?

and done

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