[patch #6388] Support for reading postgres databases

[John Darrington]

Follow-up Comment #2, patch #6388 (project pspp):

    In the documentation, would it make sense to drop the FILE subcommand
syntax out of the generic GET DATA syntax summary, since it no longer applies
to all of the file types?

Done.

      In at least one place "PostgresQL" is written in place of "PostgreSQL"
(missing capitalization on the "s").

Fixed.

      Should "Postgres" be consistently capitalized in the documentation?
Sometimes it is capitalized, sometimes it is not.

What I've done is to write PostgreSQL in the first instance, in  recognition
of the canonical name; the way they like it.  In subsequent reference I've
written "postgres", because anything else is cumbersome IMO.


      retreived -> retrieved

     "Whether or not the connection is encryption"
     ->
     "Whether or not the connection is encrypted"

Thanks. Fixed.

     I am surprised that the best way to obtain the OID macros is to copy the
set of macros directly from the postgres source. I assume that you considered
some alternatives, such as #include <catalog/pg_type.h>?

Yeah.  If you #include that file, then you also have to include 
several others, which ends up with you pulling in the config.h from the
postgres source, which conflicts with PSPP's config.h and all hell breaks
loose.  According to the postgres gurus, pg_type.h shouldn't be distributed,
although the debian package chooses to do so.  The advice I was given was to
copy the macros.
Of course, if they ever change, then it'll break this interface, but it'll
also break every other postgres interface ever written, so that's unlikely to
happen.

     Is there something really subtle in the two implementations of
data_to_native()? As far as I can tell, they do exactly the same job (copying
bytes and reversing them), but they write the output starting from opposite
ends. I think that this only makes a difference if the input and output
buffers overlap.

Thanks for noticing that.  I've fixed it now (I think); I don't have a
bigendian machine handy to test upon.

     The #ifndef USE_SSL/#define USE_SSL 1/#endif is curious. It implies that
there's some code later on that depends on it, but I can't see any.

I'd misunderstood the postgres docs.  It's correct now, I think.


     The //case_destroy (...) in psql_casereader_destroy() probably indicates
some missing code cleanup?


Fixed.  Thanks.



     The following:
     + if ( var_is_numeric (v))
     + {
     + val->f = SYSMIS                  ;
     + }
     + else
     + {
     + memset (val->s, ' ', var_get_width (v)) ;
     + }
     can be simplified to:
     value_set_missing (val, var_get_width (v)) ;


Done.

     
     I wonder whether the test should exit with status 77 if a postgres
server cannot be located? The automake documentation says that tests that exit
with 77 are ignored in the final count and that it should be used for
nonportable tests in environments where they don't make sense.

I didn't know about that feature.  Thanks for mentioning it.

New patch is attached.



(file #14944)
    _______________________________________________________

Additional Item Attachment:

File name: psql.patch                     Size:43 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/patch/?6388>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/