Ok, ignore my previous message. I've read the patch now and that's not an
issue. The old code path is not commented out, it's #ifdef'd conditionally on
HAVE_LONG_INT_64 is right (well it seems right, it's a bit hard to tell in
patch form).

A few comments:

1) Please don't include configure in your patch. I don't know why it's checked
   into CVS but it is so that means manually removing it from any patch. It's
   usually a huge portion of the diff so it's worth removing.

2) The genbki.sh change could be a bit tricky for multi-platform builds (ie
   OSX). I don't really see an alternative so it's just something to note for
   the folks setting that up (Hi Dave).

   Actually there is an alternative but I prefer the approach you've taken.
   The alternative would be to have a special value in the catalogs for 8-bit
   maybe-pass-by-value data types and handle the check at run-time.

   Another alternative would be to have initdb fix up these values in C code
   instead of fixing them directly in the bki scripts. That seems like more
   hassle than it's worth though and a bigger break with the rest.

3) You could get rid of a bunch of #ifndef HAVE_LONG_INT_64 snippets by having
   a #define like INT64PASSBYVALUE which is defined to be either "true" or
   "false". It might start getting confusing having three different defines
   for the same thing though. But personally I hate having more #ifdefs than
   necessary, it makes it hard to read the code.

4) Your problems with tsearch and timestamp etc raise an interesting problem.
   We don't need to mark this in pg_control because it's a purely a run-time
   issue and doesn't affect on-disk storage. However it does affect ABI
   compatibility with modules. Perhaps it should be added to

   Actually, why isn't sizeof(Datum) in there already? Do we have any
   protection against loading 64-bit compiled modules in a 32-bit server or
   vice versa?

But generally this is something I've been wanting to do for a while and
basically the same approach I would have taken. It seems sound to me.

  Gregory Stark
  EnterpriseDB          http://www.enterprisedb.com
  Ask me about EnterpriseDB's RemoteDBA services!

Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:

Reply via email to