citusdata / pg_shard

ATTENTION: pg_shard is superseded by Citus, its more powerful replacement
https://github.com/citusdata/citus
GNU Lesser General Public License v3.0
1.06k stars 63 forks source link

Add PostgreSQL 9.5 compatibility #137

Closed jasonmp85 closed 8 years ago

jasonmp85 commented 9 years ago

So @mtuncer asked about testing against PostgreSQL 9.5 in Travis. I figured alpha2 might be a bit early to begin that work (9.4 wasn't released until December 2014, so if we're on that schedule it'll be four months until 9.5 is released), but realized I wanted to play around with the custom scan stuff in 9.5 within pg_shard (@anarazel had asked for feedback about whether it would suit our needs).

Naturally, this required me to get pg_shard compiling in PostgreSQL 9.5. So of course I wanted our tests to pass, too. And then I figured "why not fix Travis so it can build against PostgreSQL 9.5, just so we're ready?"

Anyways, now there's this pull request.

jasonmp85 commented 9 years ago

FWIW I went through the usual procedure to get the 9.5 ruleutils.c file into our tree:

  1. Create a branch based on REL9_5_STABLE containing that single file and only its changes, using:

    git filter-branch -f --subdirectory-filter src/backend/utils/adt --prune-empty
     --index-filter 'git rm --quiet --cached --ignore-unmatch $(git ls-files | grep -v ruleutils.c)'
  2. Merge that into this feature branch at the path src/ruleutils_95.c
  3. Remove unused extern functions, as well as any static functions that then become unused
  4. Add conditional compilation and needed pragmas around the entire file
  5. Change generate_relation_name to generate_shard_name and fix all call sites to pass in shardid
  6. Change get_query_def to deparse_shard_query, fixing call sites
  7. Add deparse_shard_query, the sole entry point

Checking out this branch and diffing the new file against one of the older ruleutils files will show that all differences are due to new deparsing features in ruleutils itself and not anything special I had to do with the 9.5 file… the parts we've added are structurally identical across all three versions.

onderkalaci commented 8 years ago

Hey @jasonmp85,

I tested this branch on two different OS (Ubuntu 14.04 and Redhat 7.1), and three different postgres versions (9.4.0, 9.4.4 and 9.5.alpha2). All compiled with no problems.

Also, I executed the steps on github README, and again no problems. However, I observed some problems while running regression tests on RedHat, which is not related with this branch. Because, the same problem exists with master/develop branches as well. I'll send a separate e-mail for that, but, my first intuition is that it is related to Makefile or some language settings.

I have some comments for the changes.

jasonmp85 commented 8 years ago
  • In most places we use
    • #if (PG_VERSION_NUM >= 90500 && PG_VERSION_NUM < 90600)
    • and in one place we use: #if (PG_VERSION_NUM < 90500)
    • Can't we make them all look the same?

I'm not quite clear on what you mean. In one case, I'm handling versions greater than 9.5 and less than 9.6 (because I want to be safe). In the other version, I'm handling everything beneath 9.5.

Do you want all things to be range? I can switch to that, so we're explicitly only covering 9.3, 9.4, and 9.5. Sure. I'll do that.

  • At first, I thought adding only the if statement (ie: not adding the corresponding else) didn't make sense for me such as this:
    • Then, I realized that the required functions exists on some different headers for the previous versions, and those headers are already included in the same source file. Is that right?

That's 100% correct. I can clarify the comment somewhat to reflect this. Many existing function definitions got moved into new files (refactoring, I guess), but since the old file still exists and also has useful definitions, I need to include it in any case. I verified all these changes with include-what-you-use, BTW, so I know this is the correct set of dependencies for each file, in each version.

  • Do you see any other useful tests for this change? If not, I think we are ready to merge this branch after considering the two tiny comments above.

After I merge this, Travis CI will begin building against 9.5, which is the major reason I wanted to do this (so we could know ahead of time if any of our changes break the tests in Travis). I think that's testing enough for now.