patchPSPP - Patches: patch #6388, Support for reading postgres...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #6388: Support for reading postgres databases

Submitter:  John Darrington <jmd>
Submitted:  Sat 02 Feb 2008 11:35:47 AM UTC
   
 
Category:  None Item Group:  None
Status:  Done Assigned to:  None
Open/Closed:  Closed

Mon 04 Feb 2008 01:31:46 AM UTC, comment #5: 

Thanks.  I checked this in.

John Darrington <jmd>
Group administrator
Sun 03 Feb 2008 06:51:39 PM UTC, comment #4: 

Something broke in my brain and I closed the patch, which is obviously not what I wanted to do.  Re-opened.

Ben Pfaff <blp>
Group administrator
Sun 03 Feb 2008 06:50:53 PM UTC, comment #3: 

I didn't look at the new patch, but I'm sure that it's ready to check in now.  Thanks!

Ben Pfaff <blp>
Group administrator
Sun 03 Feb 2008 08:31:52 AM UTC, comment #2: 

    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)

John Darrington <jmd>
Group administrator
Sun 03 Feb 2008 07:32:08 AM UTC, comment #1: 

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?

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

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

retreived -> retrieved

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

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>?

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.

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.

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

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));

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.

Ben Pfaff <blp>
Group administrator
Sat 02 Feb 2008 11:35:47 AM UTC, original submission:  

This patch adds a TYPE=PSQL option to the GET DATA command, to enable reading from Postgres databases.

John Darrington <jmd>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #14944:  psql.patch added by jmd (43KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by blp (Posted a comment)
  • -email is unavailable- added by jmd (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-02-04 jmd StatusWorks For Me Done
        Open/ClosedOpen Closed
    2008-02-03 blp StatusDone Works For Me
        Open/ClosedClosed Open
    2008-02-03 blp StatusWorks For Me Done
        Open/ClosedOpen Closed
    2008-02-03 jmd Attached File#14924 Removed
    2008-02-03 jmd Attached File- Added psql.patch, #14944
    2008-02-03 blp StatusReady For Test/Review Works For Me
    2008-02-02 jmd StatusNone Ready For Test/Review
    2008-02-02 jmd Attached File- Added psql.patch, #14924

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code