Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #8049 (closed enhancement: fixed)

Opened 2 years ago

Last modified 1 year ago

[PATCH] Improve PostgreSQL adapter compatibility, feature set, and performance

Reported by: roderickvd Assigned to: bitsweat
Priority: normal Milestone: 1.x
Component: ActiveRecord Version: edge
Severity: normal Keywords: postgresql verified
Cc: skaes, aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale, fotos, tarmo, Erkki, grzm, rtomayko@gmail.com

Description

This represents a fairly significant update to the PostgreSQL adapter. Here's what's changed:

  • Improve compatibility
    • Use string escaping syntax if available. This will be mandatory for all string input in future PostgreSQL versions, and PostgreSQL has been cluttering the logs with warnings about this as of version 8.2 by default. The implementation is a general one so that other adapters can easily hook into it.
    • Parenthesize Ruby code for future Ruby compatibility.
  • Fix support for current data types
    • Fix money data type that is supposed to be supported according to #2872, but whose input nor output was working.
    • While we're there, improve money data type to increase precision from 8 to 17 on the upcoming PostgreSQL 8.3.
    • Binary fixtures weren't working unless #7987 was applied, but this adapter is resilient enough to work regardless. (Which doesn't deprecate that other patch.)
  • Add support for additional data types
    • Support bitstring data type and both bit-string and hexadecimal notation.
    • Support double precision, cidr, inet, macaddr, and oid data types.
    • Support xml data type and appropriate escaping on the upcoming PostgreSQL 8.3.
    • Support future implementations of line data type.
    • Sync extraction of default column values with all supported data types. (Support for interval, time, money, geometrics, and arrays was missing.)
  • Improve feature set
    • Implement reconnecting with the postgres-pr driver.
    • Fix reconnecting to also reset the configured schema search path.
    • Query the maximum table alias length at runtime instead of hard coding the default.
    • Add support for querying the PostgreSQL server version.
  • Improve performance by ~10% (on the Active Record test suite when using the native driver)
    • Escape strings in the driver.
    • Tighter regular expressions allow for better expression optimization and faster runtime.
    • Remove superfluous joins and select columns from queries.
    • Micro-optimizations:
      • Save the schema search path when set (instead of quering for it again).
      • Use module_eval and instance_eval instead of send to prevent memory leaks.
      • Use string literals when no string substitution is required.
  • Improve code quality
    • Move PostgreSQL-specific data handling to a new PostgreSQLColumn < Column class. This reduces procedural programming in favor of object oriented programming and improves code clarity.
    • Fix code style to use && and || in favor of and & or.
    • Some whitespace and tab cleanup.
  • Improve documentation
    • Document all methods.
    • Remove #:nodoc: where applicable.
    • Clarify and correct some bits of existing documentation.
    • Change postgresql.yml to be more consistent with the default mysql.yml.
    • Change postgresql.yml to reference the improved native ruby-postgres drivers, which are now the preferred drivers as per the comment at #3114. These have the added advantage that they have win32 builds available, so that postgres-pr support can slowly but surely be deprecated.

Tested on PostgreSQL 7.4 and 8.2 with postgres-pr-0.4.0, postgres-0.7.1, and ruby-postgres-0.7.1.2006.04.06. All tests were executed double: both with Unicode enabled, and with Unicode disabled. The test suite also passes on MySQL and SQLite as it did before.

Attachments

improve_postgresql_adapter.diff (58.5 kB) - added by roderickvd on 04/12/07 21:41:44.
unit_tests.diff (15.9 kB) - added by FooBarWidget on 05/15/07 13:16:14.
New PostgreSQL unit tests
postgresql_config.diff (1.6 kB) - added by FooBarWidget on 05/15/07 13:16:34.
Changes in the PostgreSQL default configuration file
quoting.diff (1.7 kB) - added by FooBarWidget on 05/15/07 13:16:52.
Changes to the Quoting module
postgresql_adapter.diff (38.6 kB) - added by FooBarWidget on 05/15/07 13:17:15.
Changes to the PostgreSQLAdapter class
functionally_improve_postgresql_adapter.diff (49.8 kB) - added by roderickvd on 07/25/07 10:27:12.
Functional changes to the PostgreSQL adapter
8049_patch_minor_fixes.diff (2.7 kB) - added by tarmo on 08/02/07 09:41:37.
Fixes to a couple of minor problems with functionally_improve_postgresql_adapter.diff

Change History

04/11/07 14:32:20 changed by roderickvd

I still need to apply the #7981 test suite fixes to make all tests pass, with or without this improved adapter, and regardless of database server.

04/11/07 14:51:08 changed by aeden

  • cc set to aeden.

04/12/07 21:41:44 changed by roderickvd

  • attachment improve_postgresql_adapter.diff added.

04/12/07 21:44:58 changed by roderickvd

Replaced attachment to correct the decimal precision of the money data type. (It's 19 on PostgreSQL 8.3 and beyond, and 10 on any versions before that. I forgot to figure in the scale decimals.)

04/13/07 18:56:48 changed by purcell

  • cc changed from aeden to aeden, purcell.

04/14/07 11:43:01 changed by spicycode

  • cc changed from aeden, purcell to aeden, purcell, spicycode.

04/14/07 12:22:09 changed by mislav

  • cc changed from aeden, purcell, spicycode to aeden, purcell, spicycode, mislav.

Awesome...!

04/15/07 14:18:47 changed by masterkain

  • cc changed from aeden, purcell, spicycode, mislav to aeden, purcell, spicycode, mislav, masterkain.

04/19/07 14:26:44 changed by f.svehla

  • cc changed from aeden, purcell, spicycode, mislav, masterkain to aeden, purcell, spicycode, mislav, masterkain, f.svehla.

04/26/07 00:00:12 changed by josh

  • keywords set to postgresql.

04/27/07 17:01:23 changed by bytheway

  • cc changed from aeden, purcell, spicycode, mislav, masterkain, f.svehla to aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway.

05/14/07 06:41:12 changed by FooBarWidget

  • cc changed from aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway to aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget.

When will this patch be reviewed and applied? I use PostgreSQL in Rails and right now I can't use binary columns because of a bug, which this patch fixes.

05/14/07 13:23:49 changed by paolo.negri

  • cc changed from aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget to aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri.

05/15/07 13:16:14 changed by FooBarWidget

  • attachment unit_tests.diff added.

New PostgreSQL unit tests

05/15/07 13:16:34 changed by FooBarWidget

  • attachment postgresql_config.diff added.

Changes in the PostgreSQL default configuration file

05/15/07 13:16:52 changed by FooBarWidget

  • attachment quoting.diff added.

Changes to the Quoting module

05/15/07 13:17:15 changed by FooBarWidget

  • attachment postgresql_adapter.diff added.

Changes to the PostgreSQLAdapter class

05/15/07 13:18:32 changed by FooBarWidget

I was told that it is a good idea to split the patch into smaller files, for easier review. I've done that and uploaded the new, splitted patch. I added 4 new files: - unit_tests.diff - postgresql_config.diff - quoting.diff - postgresql_adapter.diff Together, these files replace improve_postgresql_adapter.diff.

I also verify that these patches apply cleanly (even without fuzziness) to Rails edge.

05/21/07 15:30:02 changed by roderickvd

Thanks for the effort, FooBarWidget. Reviewing them they look to be in fine order!

If the little birds that told you that bit of information would be so forthcoming to share that with me in the future as well, we could speed up the approval process. That, and I would appreciate the feedback. :-)

05/29/07 16:02:52 changed by vesaria

I'd like to apply the patch.

Running a simple (patch < name.diff) didn't work, at least on Windows/Cygwin/Rails 1.2.2 in vendor/rails. How do I apply it?

Also, does this affect #5454 ?

05/29/07 19:40:04 changed by FooBarWidget

The patch is for edge, not for 1.2.

05/29/07 20:59:40 changed by bitsweat

  • owner changed from core to bitsweat.

05/29/07 21:02:24 changed by bitsweat

For the unit tests, please add the new tables to test/fixtures/db_definitions/postgresql.sql instead of creating/dropping in setup/teardown.

05/29/07 21:20:09 changed by bitsweat

  • status changed from new to closed.
  • resolution set to incomplete.

For the adapter changes:

Don't remove the :nodoc: on Base.postgresql_connection.

MONEY is a deprecated data type, no reason to add support.

Most of the docs changes are fine, but some are bad: 

 -     # Is this connection alive and ready for queries? 
 +     # Returns if this connection is alive and ready for queries.

Unexplained query change in pk_and_sequence_for removing join guaranteeing that table and sequence are in the same schema.

Please extract the 'SHOW standard_conforming_strings' from connect into a method.

This patch has a lot of great functionality; you've clearly put a lot of work into it. However, splitting into three functional pieces compounds the megapatch problem rather than addressing it.

Do functional changes (code change + test + docs) in one patch, and the myriad style + whitespace changes in a second cleanup patch. Bundling the two together makes it difficult to discern the meat of the changes.

Update the patch for latest trunk, address these concerns, and I'll happily commit. Thanks for working on this!

(follow-up: ↓ 21 ) 05/30/07 07:07:40 changed by roderickvd

Thanks for your extensive review. I will update the patch shortly. Responding to two bits of your feedback:

The MONEY type was set to be deprecated, but things have changed for the better. See http://archives.postgresql.org/pgsql-hackers/2007-03/msg01181.php for the relevant discussion.

The primary key sequence always lives in the same schema as the table, so the join is unnecessary. While it's true that sequences may be manually created in other schemas, this is only relevant to user-defined number sequences and not to sequences facilitating primary keys. Should this explanation be added to the code?

(in reply to: ↑ 20 ) 05/30/07 07:41:12 changed by bitsweat

Replying to roderickvd:

The MONEY type was set to be deprecated, but things have changed for the better. See http://archives.postgresql.org/pgsql-hackers/2007-03/msg01181.php for the relevant discussion.

Interesting. Sounds strange to me, but good to know :)

The primary key sequence always lives in the same schema as the table, so the join is unnecessary. While it's true that sequences may be manually created in other schemas, this is only relevant to user-defined number sequences and not to sequences facilitating primary keys. Should this explanation be added to the code?

No, that's fine, it makes sense.

Thank you!

06/06/07 08:03:34 changed by codahale

  • cc changed from aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri to aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale.

06/09/07 00:45:39 changed by fotos

  • cc changed from aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale to aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale, fotos.

(follow-up: ↓ 26 ) 06/21/07 05:47:03 changed by vesaria

What is the status of this? It looks very promisng, yet I see that for some reason it's been closed...

06/21/07 08:17:33 changed by roderickvd

Status is: still needs work by me, but I'm swamped with other work right now. I hope to resubmit an appropriate patch in a week or two or three from now.

(in reply to: ↑ 24 ) 06/21/07 20:34:18 changed by bitsweat

Replying to vesaria:

What is the status of this? It looks very promisng, yet I see that for some reason it's been closed...

"resolution set to incomplete."

Please read the front page for a quick explanation of how we use ticket resolutions as a form of workflow, rather than simply open/closed.

07/25/07 10:27:12 changed by roderickvd

  • attachment functionally_improve_postgresql_adapter.diff added.

Functional changes to the PostgreSQL adapter

07/25/07 10:29:31 changed by roderickvd

  • status changed from closed to reopened.
  • resolution deleted.

I finally found the time to make this a wrap now that Nedforce has sponsored me the time.

The diff I just attached only contains the functional improvements. I will tackle the style changes and whitespace cleanups in a later diff and separate ticket.

07/29/07 14:06:32 changed by tarmo

  • cc changed from aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale, fotos to aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale, fotos, tarmo.

07/29/07 14:23:49 changed by skaes

  • cc changed from aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale, fotos, tarmo to skaes, aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale, fotos, tarmo.

08/02/07 09:09:43 changed by Erkki

  • cc changed from skaes, aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale, fotos, tarmo to skaes, aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale, fotos, tarmo, Erkki.

08/02/07 09:41:37 changed by tarmo

  • attachment 8049_patch_minor_fixes.diff added.

Fixes to a couple of minor problems with functionally_improve_postgresql_adapter.diff

08/02/07 09:46:42 changed by tarmo

I've attached a patch that was needed for me to get all the tests to pass on rails edge [7259].

I did have one problem though, when I had lc_monetary='et_EE.UTF-8' in postgresql.conf I got 22 failures in PostgresqlDataTypeTest, all of them looked like this: 

 1) Error:
test_array_values(PostgresqlDataTypeTest):
ActiveRecord::StatementInvalid: PGError: ERROR:  invalid input syntax for type money: "$567.89"
: INSERT INTO postgresql_moneys (wealth) VALUES ('$567.89')

Setting lc_monetary to 'en_US.UTF-8' made the problem go away though. I don't know if and how this issue can be resolved or even if it needs to be resolved.

08/02/07 09:52:27 changed by tarmo

Forgot to mention, Postgresq 8.2.4, ruby-postgres-0.7.1.20060406

08/02/07 09:56:53 changed by Erkki

I had the same problems than tarmo, applying his patch made the tests pass. Tested with lc_all="C", databases with utf-8 encoding, ruby-postgres (0.7.1.2006.04.06), postgresql 8.2.3

08/02/07 15:16:58 changed by roderickvd

Thanks tarmo. Looks like I completely forgot that the database-specific tests weren't run by all.sh. The locale issue is a feature, not a bug :-) because PostgreSQL understands that that dollar notation isn't correct for your locale.

08/08/07 17:53:28 changed by grzm

  • cc changed from skaes, aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale, fotos, tarmo, Erkki to skaes, aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale, fotos, tarmo, Erkki, grzm.

08/10/07 07:23:26 changed by roderickvd

Quoting koz from Rails-core:

"Are there any active users of postgres around who want to give the patch a test / +1? My own postgres installation hasn't been used in some time, and I don't feel particularly qualified to pass judgement on this patch."

So it'd be great if most everyone on the Cc list could do that and report back to Rails-core :-)

08/12/07 02:18:46 changed by tarmo

I tried this patch on one of my applications and all tests passed. The patch (including my fix) applies cleanly to current Rails edge [7308] and all activerecord tests also pass.

+1

08/12/07 14:44:36 changed by rtomayko

  • cc changed from skaes, aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale, fotos, tarmo, Erkki, grzm to skaes, aeden, purcell, spicycode, mislav, masterkain, f.svehla, bytheway, FooBarWidget, paolo.negri, codahale, fotos, tarmo, Erkki, grzm, rtomayko@gmail.com.

Will be testing on FreeBSD 6.1, MacOS 10.4.10, and Fedora Core 6 over the next couple of days.

08/13/07 04:03:01 changed by rtomayko

Applied functionally_improve_postgresql_adapter.diff cleanly to r7309 but test/fixtures/db_definitions/postgresql.sql didn't load up due to syntax errors. Patch 8049_patch_minor_fixes.diff applied cleanly on top and fixed the syntax error (among other things).

All postgresql tests passed in the configurations listed below.

Reviewed the patch and it appears to be quite pleasant.

+1

(I wasn't able to run my app test suite because I'm still on the Rails stable. Any breakage caused would be well worth the functional and performance gains.)

FreeBSD 6.1 / Ruby 1.8.6 / PostgreSQL 8.2.4 

$ uname -a
FreeBSD ... 6.1-RELEASE FreeBSD 6.1-RELEASE #0: Sun May  7 04:42:56 UTC 2006     root@opus.cse.buffalo.edu:/usr/obj/usr/src/sys/SMP  i386
$ ruby --version
ruby 1.8.6 (2007-03-13 patchlevel 0) [i386-freebsd6]
$ psql --version
psql (PostgreSQL) 8.2.4
$ gem list ruby-postgres
*** LOCAL GEMS ***
ruby-postgres (0.7.1.2006.04.06)
    Ruby extension library providing an API to PostgreSQL
$ rake test_postgresql
<snip>
Finished in 31.26266 seconds.
1134 tests, 4218 assertions, 0 failures, 0 errors

Linux FC6 / Ruby 1.8.5 / PostgreSQL 8.1.9 

$ uname -a
Linux ... 2.6.20-1.2952.fc6 #1 SMP Wed May 16 18:59:18 EDT 2007 i686 i686 i386 GNU/Linux
$ ruby --version
ruby 1.8.5 (2007-06-07 patchlevel 52) [i386-linux]
$ psql --version
psql (PostgreSQL) 8.1.9
$ gem list ruby-postgres
*** LOCAL GEMS ***
ruby-postgres (0.7.1.2006.04.06)
    Ruby extension library providing an API to PostgreSQL
$ rake test_postgresql
<snip>
Finished in 48.759124 seconds.
1143 tests, 4243 assertions, 0 failures, 0 errors

MacOS X / Ruby 1.8.6 / PostgreSQL 8.2.4 

$ uname -a
Darwin ... 8.10.1 Darwin Kernel Version 8.10.1: Wed May 23 16:33:00 PDT 2007; root:xnu-792.22.5~1/RELEASE_I386 i386 i386
$ ruby --version
ruby 1.8.6 (2007-03-13 patchlevel 0) [i686-darwin8.9.1]
$ psql --version
psql (PostgreSQL) 8.2.4
$ gem list ruby-postgres
*** LOCAL GEMS ***
ruby-postgres (0.7.1.2006.04.06)
    Ruby extension library providing an API to PostgreSQL
$ rake test_postgresql
<snip>
Finished in 35.040515 seconds.
1143 tests, 4243 assertions, 0 failures, 0 errors

08/13/07 14:56:22 changed by jw

  • keywords changed from postgresql to postgresql verified.

+1

Tested it with my app specific test suite.

08/16/07 06:26:33 changed by nzkoz

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [7329]) Improve performance and functionality of the postgresql adapter. Closes #8049 [roderickvd]

11/22/07 01:29:23 changed by bitsweat

(In [8185]) PostgreSQL: correct binary escaping. References #8049, closes #10176 [jbasdf, tmacedo]