db-migrate / pg

A postgresql driver for db-migrate.
Other
67 stars 51 forks source link

Test suite fails on Postgres 10+ #40

Closed sonya closed 1 year ago

sonya commented 5 years ago

Additionally, I am unclear in general about how Postgres version support is intended to be handled for this package (see patches below)

First off, these are the test suite failure messages:

    createTable has column metadata for the event table
      ✗ that has raw column
        »
        actual expected

        CURRENT_TIMESTAMPnow()
         // macros.js:14

      ✗ that has special CURRENT_TIMESTAMP column
        »
        actual expected

        CURRENT_TIMESTAMPnow()
         // macros.js:14

I have a patch that addresses these test cases. However the Travis configuration appears to only test one version, so the conditionals in this patch would not be covered. I am not familiar enough with Travis to know if a setup with multiple Postgres versions is possible.

I used a quick and dirty docker-compose setup to test multiple versions locally (branch here) and found that a handful of tests also fail on Postgres 8.

The motivation for all this was that I want to be able to use Postgres 10 declarative partitioning in migrations. I have a PoC here (with some very sad test hacks because I can't determine the version before the code runs). But it seems there are some issues (such as this ticket) that require more insight from the core team before version-specific features can be implemented in a straightforward way.

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/71764120-test-suite-fails-on-postgres-10?utm_campaign=plugin&utm_content=tracker%2F14258523&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F14258523&utm_medium=issues&utm_source=github).
sonya commented 5 years ago

Oh man, I totally did that thing where I fixed the tests and broke the library (and of course notice after posting the issue). So sad.

wzrdtales commented 5 years ago

@sonya Instead of patches, put out a a PR :) Your changes are however indeed breaking it for all other platforms. This is not just postgres, the pg driver is the base for a lot of other platforms that rely on the pg wire protocol.

Do you know what exactly has changed in pg 10, maybe I can have a look though?

sonya commented 5 years ago

Thanks for the feedback, @wzrdtales

The specific change in Postgres 10 that broke the tests is that when you create a column with CURRENT_TIMESTAMP as the default value, in pg10 the table metadata displays CURRENT_TIMESTAMP as the default value, whereas in pg9 the metadata displays now(). (I can't find actual Postgres documentation about this change, but this discussion on the Rails repo has a lot of good info.)

I didn't open a PR because I'm more interested in starting a discussion with you guys about what approach would align best with the library. After I filed this issue I did some more digging and at this point I believe the "right" solution would be along the lines of exposing a method in the shared interface (in node-db-migrate and db-migrate-base) to get the server version.