Skip site navigation (1) Skip section navigation (2)

Re: Should we add crc32 in libpgport?

From: Daniel Farina <daniel(at)heroku(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Should we add crc32 in libpgport?
Date: 2012-01-17 10:14:53
Message-ID: CAAZKuFa3pFundjUa_mVw2MEfy5NfDZEHTN9VREtdK5XW-B7JoA@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
On Mon, Jan 16, 2012 at 9:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Daniel Farina <daniel(at)heroku(dot)com> writes:
>> Copying CRC32 implementations everywhere is not the worst thing, but I
>> find it inadequately explained why it's necessary for now, at least.
>
> Agreed, but I don't care for your proposed solution (put it in
> libpgport) because that assumes a fact not in evidence, namely that
> external projects have access to libpgport either.

I see. Because of ./configure --disabled-shared is a supported option.

> Is it possible to put enough stuff in pg_crc.h so that external code could
> just include that, perhaps after an extra #define to enable extra code?

Yes.  As a nice side effect, we manage to get rid of a self-described
ugly hack, involving exposing the function from libpgport, so outside
the ugly preprocessor dealing, we do score a victory.  Related to
that, I have also demoted the symbol from extern to static.  There are
a couple of build-process special-cases for utilities like
pg_controldata and pg_resetxlog that are thankfully able to be
removed.

In addition, it seemed pretty weird that this wasn't so much a "port"
(like stub gettimeofday implementations) but rather a function desired
on all platforms -- the degenerate case, where zero platforms have the
function already.  So a minor plus of anti-awkwardness of calling it a
'port'.

> As for whether we could drop the existing near-duplicate code in
> contrib/, I think we'd first have to convince ourselves that it was
> functionally identical, because otherwise replacing those versions would
> break existing ltree and hstore indexes.

True.  It *is* billed CRC32, so unless there's a bug it *should* be
identical -- but if not, a version bump of the extension/type may be
necessary (do we even know what to do about that, given pg_upgrade?).
I'm not sure what beyond careful inspection (which I haven't done) and
testing a small corpus for binary equivalence what is to be done about
that to be convincing, though.  I'll submit the dedup patch
separately, I currently only have ltree done.

See the attached patch.  It has a detailed cover letter/comment at the
top of the file.

I have confirmed it applies, builds, and relieves one of my problems
in building xlogdump without access to postgres .o files.  I think the
other is surmountable in that project (sprompt.o, which seems hardly
as fundamental).  I don't think I've tested the CRC64 path at all, as
it is not used anywhere -- it's sort of there just to occupy symbol
space, as well as I can tell, per its comments ("reserved").

-- 
fdr

Attachment: Move-CRC-tables-to-pg_crc.h.patch
Description: text/x-patch (45.7 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Heikki LinnakangasDate: 2012-01-17 10:16:34
Subject: Re: BGWriter latch, power saving
Previous:From: Noah MischDate: 2012-01-17 09:56:04
Subject: Re: foreign key locks, 2nd attempt

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group