Thread: pgsql: Create libpgcommon, and move pg_malloc et al to it

pgsql: Create libpgcommon, and move pg_malloc et al to it

From
Alvaro Herrera
Date:
Create libpgcommon, and move pg_malloc et al to it

libpgcommon is a new static library to allow sharing code among the
various frontend programs and backend; this lets us eliminate duplicate
implementations of common routines.  We avoid libpgport, because that's
intended as a place for porting issues; per discussion, it seems better
to keep them separate.

The first use case, and the only implemented by this patch, is pg_malloc
and friends, which many frontend programs were already using.

At the same time, we can use this to provide palloc emulation functions
for the frontend; this way, some palloc-using files in the backend can
also be used by the frontend cleanly.  To do this, we change palloc() in
the backend to be a function instead of a macro on top of
MemoryContextAlloc().  This was previously believed to cause loss of
performance, but this implementation has been tweaked by Tom and Andres
so that on modern compilers it provides a slight improvement over the
previous one.

This lets us clean up some places that were already with
localized hacks.

Most of the pg_malloc/palloc changes in this patch were authored by
Andres Freund. Zoltán Böszörményi also independently provided a form of
that.  libpgcommon infrastructure was authored by Álvaro.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/8396447cdbdff0b62914748de2fec04281dc9114

Modified Files
--------------
contrib/oid2name/oid2name.c           |   50 -------------
contrib/pg_upgrade/check.c            |    2 +-
contrib/pg_upgrade/controldata.c      |    2 +-
contrib/pg_upgrade/dump.c             |    2 +-
contrib/pg_upgrade/exec.c             |    2 +-
contrib/pg_upgrade/file.c             |    2 +-
contrib/pg_upgrade/function.c         |    2 +-
contrib/pg_upgrade/info.c             |    2 +-
contrib/pg_upgrade/option.c           |    2 +-
contrib/pg_upgrade/page.c             |    2 +-
contrib/pg_upgrade/parallel.c         |    2 +-
contrib/pg_upgrade/pg_upgrade.c       |    2 +-
contrib/pg_upgrade/pg_upgrade.h       |    4 -
contrib/pg_upgrade/relfilenode.c      |    2 +-
contrib/pg_upgrade/server.c           |    2 +-
contrib/pg_upgrade/tablespace.c       |    2 +-
contrib/pg_upgrade/util.c             |   51 +-------------
contrib/pg_upgrade/version.c          |    2 +-
contrib/pg_upgrade/version_old_8_3.c  |    2 +-
contrib/pgbench/pgbench.c             |   53 --------------
src/Makefile                          |    1 +
src/Makefile.global.in                |   25 ++++---
src/backend/storage/file/copydir.c    |   11 ---
src/backend/utils/mmgr/mcxt.c         |   78 +++++++++++---------
src/bin/initdb/initdb.c               |   39 ----------
src/bin/pg_basebackup/streamutil.c    |   37 ----------
src/bin/pg_basebackup/streamutil.h    |    4 -
src/bin/pg_ctl/pg_ctl.c               |   38 ----------
src/bin/pg_dump/Makefile              |    6 +-
src/bin/pg_dump/common.c              |    2 -
src/bin/pg_dump/compress_io.c         |    1 -
src/bin/pg_dump/dumpmem.c             |   76 -------------------
src/bin/pg_dump/dumpmem.h             |   22 ------
src/bin/pg_dump/dumputils.c           |    1 -
src/bin/pg_dump/nls.mk                |    2 +-
src/bin/pg_dump/pg_backup_archiver.c  |    1 -
src/bin/pg_dump/pg_backup_custom.c    |    1 -
src/bin/pg_dump/pg_backup_db.c        |    1 -
src/bin/pg_dump/pg_backup_directory.c |    1 -
src/bin/pg_dump/pg_backup_null.c      |    1 -
src/bin/pg_dump/pg_backup_tar.c       |    1 -
src/bin/pg_dump/pg_dump.c             |    1 -
src/bin/pg_dump/pg_dump_sort.c        |    1 -
src/bin/pg_dump/pg_dumpall.c          |    1 -
src/bin/pg_dump/pg_restore.c          |    1 -
src/bin/pg_resetxlog/pg_resetxlog.c   |    5 +-
src/bin/psql/common.c                 |   50 -------------
src/bin/psql/common.h                 |    9 ---
src/bin/scripts/common.c              |   49 -------------
src/bin/scripts/common.h              |    4 -
src/common/Makefile                   |   64 ++++++++++++++++
src/common/fe_memutils.c              |  128 +++++++++++++++++++++++++++++++++
src/include/common/fe_memutils.h      |   20 +++++
src/include/postgres_fe.h             |    2 +
src/include/utils/palloc.h            |   30 ++------
src/port/dirmod.c                     |   74 -------------------
src/tools/msvc/Install.pm             |    1 +
src/tools/msvc/Mkvcbuild.pm           |   35 +++++++--
src/tools/msvc/Project.pm             |    2 +-
59 files changed, 332 insertions(+), 684 deletions(-)


Re: pgsql: Create libpgcommon, and move pg_malloc et al to it

From
"Erik Rijkers"
Date:
On Tue, February 12, 2013 15:55, Alvaro Herrera wrote:
> Create libpgcommon, and move pg_malloc et al to it
>

If at all possible, it would be handy (for testers) if initdb-forcing commits are flagged as such.

(it's not a big deal but I often have large databases in a dev/test-setup that take a while to
re-create...)

Thanks,

Erik Rijkers








Re: pgsql: Create libpgcommon, and move pg_malloc et al to it

From
Alvaro Herrera
Date:
Erik Rijkers wrote:
> On Tue, February 12, 2013 15:55, Alvaro Herrera wrote:
> > Create libpgcommon, and move pg_malloc et al to it
> >
>
> If at all possible, it would be handy (for testers) if initdb-forcing commits are flagged as such.

Hmm?  I don't think this patch requires an initdb at all.  It doesn't
touch the database contents at all, does it?

Patches that force an initdb are flagged as such by bumping
CATALOG_VERSION_NO.

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


Re: pgsql: Create libpgcommon, and move pg_malloc et al to it

From
Alvaro Herrera
Date:
Erik Rijkers wrote:
> On Tue, February 12, 2013 17:01, Alvaro Herrera wrote:
> > Erik Rijkers wrote:
> >> On Tue, February 12, 2013 15:55, Alvaro Herrera wrote:
> >> > Create libpgcommon, and move pg_malloc et al to it
> >> >
> >>
> >> If at all possible, it would be handy (for testers) if initdb-forcing commits are flagged as
> >> such.
> >
> > Hmm?  I don't think this patch requires an initdb at all.  It doesn't
> > touch the database contents at all, does it?
> >
> > Patches that force an initdb are flagged as such by bumping
> > CATALOG_VERSION_NO.
> >
>
> I got this:
>
> The database cluster was initialized with PG_CONTROL_VERSION 934, but the server was compiled with
> PG_CONTROL_VERSION 935
> HINT:  It looks like you need to initdb
>
>
> But I immediately admit that I could well be mistaken;  it may have been an older commit that
> triggered this  complaint of 'needing initdb' on this particular machine.  My apologies.

Ah, yes, that's 62401db45c.

I'm not sure are you suggesting, though.  Are you saying that the commit
message should explicitely state "this patch requires initdb" or some
such?

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


Re: pgsql: Create libpgcommon, and move pg_malloc et al to it

From
"Erik Rijkers"
Date:
On Tue, February 12, 2013 17:01, Alvaro Herrera wrote:
> Erik Rijkers wrote:
>> On Tue, February 12, 2013 15:55, Alvaro Herrera wrote:
>> > Create libpgcommon, and move pg_malloc et al to it
>> >
>>
>> If at all possible, it would be handy (for testers) if initdb-forcing commits are flagged as
>> such.
>
> Hmm?  I don't think this patch requires an initdb at all.  It doesn't
> touch the database contents at all, does it?
>
> Patches that force an initdb are flagged as such by bumping
> CATALOG_VERSION_NO.
>

I got this:

The database cluster was initialized with PG_CONTROL_VERSION 934, but the server was compiled with
PG_CONTROL_VERSION 935
HINT:  It looks like you need to initdb


But I immediately admit that I could well be mistaken;  it may have been an older commit that
triggered this  complaint of 'needing initdb' on this particular machine.  My apologies.


Thanks,

Erik Rijkers







Re: pgsql: Create libpgcommon, and move pg_malloc et al to it

From
"Erik Rijkers"
Date:
On Tue, February 12, 2013 17:19, Alvaro Herrera wrote:
> Erik Rijkers wrote:
>> On Tue, February 12, 2013 17:01, Alvaro Herrera wrote:
>> > Erik Rijkers wrote:
>> >> On Tue, February 12, 2013 15:55, Alvaro Herrera wrote:
>> >> > Create libpgcommon, and move pg_malloc et al to it
>> >> >
>> >>
>> >> If at all possible, it would be handy (for testers) if initdb-forcing commits are flagged as
>> >> such.
>> >
>> > Hmm?  I don't think this patch requires an initdb at all.  It doesn't
>> > touch the database contents at all, does it?
>> >
>> > Patches that force an initdb are flagged as such by bumping
>> > CATALOG_VERSION_NO.
>> >
>>
>> I got this:
>>
>> The database cluster was initialized with PG_CONTROL_VERSION 934, but the server was compiled
>> with
>> PG_CONTROL_VERSION 935
>> HINT:  It looks like you need to initdb
>>
>>
>> But I immediately admit that I could well be mistaken;  it may have been an older commit that
>> triggered this  complaint of 'needing initdb' on this particular machine.  My apologies.
>
> Ah, yes, that's 62401db45c.
>
> I'm not sure are you suggesting, though.  Are you saying that the commit
> message should explicitely state "this patch requires initdb" or some
> such?
>

I was under the impression that this was habitually done in commit messages, but looking back
through the commitlog messages it's not all that clear although sometimes catversion bumps are
explicitly mentioned.

Let's just say that an explicit mention/warning is useful for me, so probably for some others too.
 At the same time I'll start monitoring PG_CONTROL_VERSION and CATALOG_VERSION a bit more closely.
 That's easy enough too of course...


Thanks,

Erik Rijkers





Re: pgsql: Create libpgcommon, and move pg_malloc et al to it

From
Tom Lane
Date:
"Erik Rijkers" <er@xs4all.nl> writes:
> On Tue, February 12, 2013 17:19, Alvaro Herrera wrote:
>> I'm not sure are you suggesting, though.  Are you saying that the commit
>> message should explicitely state "this patch requires initdb" or some
>> such?

> I was under the impression that this was habitually done in commit messages, but looking back
> through the commitlog messages it's not all that clear although sometimes catversion bumps are
> explicitly mentioned.

I think usually if the topic is mentioned in a commit message, it's to
explain why we are (or are not) forcing an initdb.  By and large, if a
patch touches catversion.h, that ought to be sufficient warning ...

            regards, tom lane