Re: [PATCH] Make various variables read-only (const)

On Sun, Dec 22, 2013 at 09:43:57PM -0500, Robert Haas wrote:
> On Fri, Dec 20, 2013 at 12:01 PM, Oskari Saarenmaa <os(at)ohmu(dot)fi> wrote:
> > This allows the variables to be moved from .data to .rodata section which
> > means that more data can be shared by processes and makes sure that nothing
> > can accidentally overwrite the read-only definitions. On a x86-64 Linux
> > system this moves roughly 9 kilobytes of previously writable data to the
> > read-only data segment in the backend and 4 kilobytes in libpq.
> >
> > https://github.com/saaros/postgres/compare/constify
> >
> > 24 files changed, 108 insertions(+), 137 deletions(-)
>
> This sounds like a broadly good thing, but I've had enough painful
> experiences with const to be a little wary. And how much does this
> really affect data sharing? Doesn't copy-on-write do the same thing
> for writable data? Could we get most of the benefit by const-ifying
> one or two large data structures and forget the rest?

Thanks for the review and sorry for the late reply, I was offline for a
while.

As Wim Lewis pointed out in his mail the const data is most likely
mixed with non-const data and copy-on-write won't help with all of it.
Also, some of the const data includes duplicates and thus .data actually
shrinks more than .rodata grows.

We'd probably get most of the space-saving benefits by just constifying the
biggest variables, but I think applying const to more things will also make
things more correct.

> Other comments:
>
> - The first hunk of the patch mutilates the comment it modifies for no
> apparent reason. Please revert.
>
> - Why change the API of transformRelOptions()?

The comment was changed to reflect the new API, I modified
transformRelOptions to only accept a single valid namespace to make things
simpler in the calling code. Nothing used more than one valid namespace
anyway, and it allows us to just use a constant "toast" without having to
create a 2 char* array with a NULL.

> -#define DEF_ENC2NAME(name, codepage) { #name, PG_##name }
> +/* The extra NUL-terminator will make sure a warning is raised if the
> + * storage space for name is too small, otherwise when strlen(name) ==
> + * sizeof(pg_enc2name.name) the NUL-terminator would be silently dropped.
> + */
> +#define DEF_ENC2NAME(name, codepage) { #name "\0", PG_##name }
>
> - The above hunk is not related to the primary purpose of this patch.

It sort-of is. Without fixed size char-arrays it's not possible to move
everything to .rodata, but fixed size char-arrays come with the drawback of
silently dropping the NUL-terminator when strlen(str) == sizeof(array), by
forcing a NUL-terminator in we always get a warning if it would've been
dropped and the size of the array can then be increased.

Thanks,
Oskari