Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PostgreSQL 10.0 and 9.6 Versioned Formula #19062

Closed
wants to merge 8 commits into from

Conversation

felixbuenemann
Copy link
Sponsor Contributor

This PR bumps postgresql to 10.0 and add a versioned formula for postgresql@9.6.

I'm not sure wether it is correct to submit the versioned formula in the same commit, I can create a separate PR if you prefer.

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@felixbuenemann
Copy link
Sponsor Contributor Author

Btw. I have seen some strange behavior in the following case:

  • postgresql 9.6.5 installed
  • brew install postgresql@9.6
  • brew upgrade postgresql to 10.0

In this state doing brew uninstall postgresql@9.6 removed postresql 10.0 instead of postgresql@9.6. Running brew uninstall postgresql@9.6 removed the versioned formula:

brew remove postgresql@9.6
Uninstalling /usr/local/Cellar/postgresql/10.0... (3,367 files, 38.7MB)
postgresql 9.6.5 1 is still installed.
Remove all versions with `brew uninstall --force postgresql`.

brew remove postgresql@9.6
Uninstalling /usr/local/Cellar/postgresql@9.6/9.6.5... (3,269 files, 36.7MB)

I'm not sure wether I did something wrong when creating the versioned formula or if it is a bug in brew.

@eagleflo eagleflo mentioned this pull request Oct 6, 2017
4 tasks
@eagleflo
Copy link
Contributor

eagleflo commented Oct 6, 2017

PostgreSQL's pg_upgrade also works for migrations across major versions, might want to update caveats to reflect that.

@eagleflo
Copy link
Contributor

eagleflo commented Oct 6, 2017

I would also remove the caveats reference to Homebrew/legacy-homebrew#2510; upgrading from 9.6 to 10.0 was smooth sailing. Upgrading straight from 8 to 10 is not a likely scenario (as it is worded now).

@fxcoudert fxcoudert added new formula PR adds a new formula to Homebrew/homebrew-core legacy Relates to a versioned @ formula labels Oct 6, 2017
@felixbuenemann
Copy link
Sponsor Contributor Author

felixbuenemann commented Oct 6, 2017

Yes, I did an in-place upgrade with pg_upgrade -k (using hardlinks) and it worked fine.

If I understand the release note correctly, than doing an in-place upgrade (without dump/restore) would require recreating hash indexes, but hash indexes are very rarely used (default is btree).

We could also just link to the main upgrade page, because it has links to the different upgrade methods and it's structure has changed compared to 9.6 to feature the different methods at the top of the page:

The info in the caveats also appears wrong, because according to the postgres docs, in-place upgrades with pg_upgrade are supported since Postgres 8.4 not 9.0.

@felixbuenemann
Copy link
Sponsor Contributor Author

The test failures seem to indicate that the following formulas don't yet work with postgres 10:

  • citus
  • pgroonga
  • pgrouting
  • pldebugger
  • temporal_tables

Some of those were "Symbol not found" errors, so they might work if recompiled.

@felixbuenemann
Copy link
Sponsor Contributor Author

I have manually installed the above mentioned formulae with --build-from-source and checked brew test <formula>:

  • citus: works (7.0 is officiall Postgres 10 compatible)
  • pgroonga: works
  • pgrouting: works after bumping postgis formula to 2.4.0 for postgres 10 compatibility
  • pldebugger: works after bumping to v1.0 tag which has postgres 10 compatibility

So form my current testing the following formulas need updates:

  • postgis to v2.4.0
  • pldebugger to v1.0 (tested with :revision => "ca1041dc3db6f516899be669dc6fbfd6339f8168"

I suggest we first bump the formulae cause they should be backwards compatible with older postgres.

It looks like rebuilding packages base don their dependencies in the build bot was not working correctly as it did not spot the postgis incompatibility and also for some reason didn't recompile the above mentioned formulas that failed.

Maybe the errors are due to two formulae in one PR?

This was referenced Oct 6, 2017
@ilovezfs
Copy link
Contributor

ilovezfs commented Oct 8, 2017

@BrewTestBot test this please

@felixbuenemann
Copy link
Sponsor Contributor Author

@ilovezfs For some reason jenkins is not recompiling the dependent formulae against the postgres 10 libpq and just using the existing bottles that were linked against libpq from postgres 9.6 – that's what's causing the "Symbol not found" errors.

@DomT4
Copy link
Member

DomT4 commented Oct 8, 2017

For some reason jenkins is not recompiling the dependent formulae against the postgres 10 libpq and just using the existing bottles that were linked against libpq from postgres 9.6

That's expected. Bump the revisions of the formulae involved if you want CI to build them from source & force the new linkage.

@felixbuenemann
Copy link
Sponsor Contributor Author

@DomT4 Ah OK, so I should bump the revision of all affected formulae as another commit in this PR?

@DomT4
Copy link
Member

DomT4 commented Oct 8, 2017

Anything you've revision bumped locally or rebuilt from source locally with the updated postgresql that compiles & you can run brew test <xyz> on successfully, yes. One commit per revision bump, so:

* citus: revision bump for postgresql
* pgroonga: revision bump for postgresql

and so on.

@felixbuenemann
Copy link
Sponsor Contributor Author

OK, one strange thing is that the tests show a failure for pgrouting, but not for postgis. Because postgis requires bumping to build against the new libpq, but pgrouting only fails because the postgis which it depends on is not rebuilt.

Anyways the error should go away if I bump postgis.

@felixbuenemann felixbuenemann mentioned this pull request Oct 8, 2017
4 tasks
@felixbuenemann
Copy link
Sponsor Contributor Author

I've just looked through the release notes for PGroonga and noticed that the version shipped in homebrew-core is very outdated and does not officially support postgres 10, so I've added PR #19188 for upgrading to pgroonga 2.0.1.

Would be cool if we can get that merged first, so I can keep this PR to only revision bumps.

I have also looked at temporal_tableson GitHub and they're already running CI against Postgres 10, so it should be fine with only a revision bump.

@DomT4
Copy link
Member

DomT4 commented Oct 8, 2017

so I've added PR #19188 for upgrading to pgroonga 2.0.1.

Unless this PR takes ages to resolve or people start pushing, it's probably worth just folding that one into this one, IMO.

@felixbuenemann
Copy link
Sponsor Contributor Author

felixbuenemann commented Oct 8, 2017

@DomT4 By keeping the PR seperate it ensures it works with both old and new postgres. However the tests are already green, so you could merge it quickly and then I'll rebase this branch and bump the revision to force the rebuild against postgres 10 libpq.

I wanted to avoid putting too many changes in a single PR.

@DomT4
Copy link
Member

DomT4 commented Oct 8, 2017

Alright. You'll have to wait for ILZ or another maintainer. I stepped down from that & consequently being able to merge things about a year ago now, so, powerless I am 😉.

@felixbuenemann
Copy link
Sponsor Contributor Author

No problem, @ilovezfs merged my other prerequisite PRs, so he can probably take a look at that one as well.

url "https://ftp.postgresql.org/pub/source/v9.6.5/postgresql-9.6.5.tar.bz2"
sha256 "06da12a7e3dddeb803962af8309fa06da9d6989f49e22865335f0a14bad0744c"

bottle do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a new formula so there shouldn't be a bottle block.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the bottles.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see them …

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, I removed them on another machine and then rebased against origin/master without pulling in my changes from my forks' remote first, so I added them back.

Removed (again).

@philipbergen
Copy link

Just a thank you note to all involved parties. I saw this work started right as v10 was announced. Impressive!

@felixbuenemann
Copy link
Sponsor Contributor Author

felixbuenemann commented Oct 12, 2017

@ilovezfs I could compress/reword the upgrade instructions to only link to the top level upgrade page if you prefer (see discussion with eagleflo at the top), otherwise I don't think there's more to do from my side.

@DomT4 DomT4 mentioned this pull request Oct 15, 2017
Copy link
Contributor

@aleksandrs-ledovskis aleksandrs-ledovskis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted some inconsistency in v10 of formula

@@ -100,18 +100,20 @@ def post_install
end

def caveats; <<-EOS.undent
If builds of PostgreSQL 9 are failing and you have version 8.x installed,
If builds of PostgreSQL 10 are failing and you have version 8.x installed,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8.x -> 9.x? May need to check if is relevant at all

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never had an issue when installing a newer postgres using brew upgrade.
The issue linked indicates that there used to be a problem when upgrading from 8.4.4 to 9.0.

I'm happy to remove the comment completely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fewer caveats the better

@@ -100,18 +100,20 @@ def post_install
end

def caveats; <<-EOS.undent
If builds of PostgreSQL 9 are failing and you have version 8.x installed,
If builds of PostgreSQL 10 are failing and you have version 8.x installed,
you may need to remove the previous version first. See:
https://github.com/Homebrew/legacy-homebrew/issues/2510

To migrate existing data from a previous major version (pre-9.0) of PostgreSQL, see:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most likely, needs change to pre-10.0

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was already wrong before, because both eg. 8.4 and 9.0 are major versions.

The link points to documentation for upgrading from a any previous version of PostgreSQL, including pg_upgrade, which ist supported since PostgreSQL 8.4 and pg_dumpall which works with current and older versions of PostgreSQL, but is a more manual process and cannot do in-place upgrades (it's a manual backup and restore).

Quoting https://www.postgresql.org/docs/10/static/pgupgrade.html:

pg_upgrade supports upgrades from 8.4.X and later to the current major release of PostgreSQL, including snapshot and beta releases.

The conclusion to this is, that for upgrades from anything prior to 8.4, one would have to di a manual pg_dumpall on the old cluster and import that dump on the new cluster.

Because the top-level page linked here, contains links to both pg_upgrade and pg_dumpall methods for upgrading as well as descriptions of when to use which method, we could reduce the comments to link to this single upgrade page.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


To migrate existing data from a previous minor version (9.0-9.5) of PostgreSQL, see:
https://www.postgresql.org/docs/9.6/static/pgupgrade.html
To migrate existing data from a previous minor version (9.0-9.6) of PostgreSQL, see:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As there weren't any minor version of 10.0, should it be substituted for 10.0 Beta1-4?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the comment is wrong, 9.0, 9.1, 9.2 etc. are all major versions, while 9.6.4 to 9.6.5 would be a minor version update, which doesn't require any changes to the database.

In Postgres 10 the versioning schema has changed, and now 10 and 11 would be major versions, but 10.0 and 10.1 would be minor versions.

Quoting https://www.postgresql.org/docs/10/static/upgrading.html:

Current PostgreSQL version numbers consist of a major and a minor version number. For example, in the version number 10.1, the 10 is the major version number and the 1 is the minor version number, meaning this would be the first minor release of the major release 10. For releases before PostgreSQL version 10.0, version numbers consist of three numbers, for example, 9.5.3. In those cases, the major version consists of the first two digit groups of the version number, e.g., 9.5, and the minor version is the third number, e.g., 3, meaning this would be the third minor release of the major release 9.5.

Minor releases never change the internal storage format and are always compatible with earlier and later minor releases of the same major version number. For example, version 10.1 is compatible with version 10.0 and version 10.6. Similarly, for example, 9.5.3 is compatible with 9.5.0, 9.5.1, and 9.5.6. To update between compatible versions, you simply replace the executables while the server is down and restart the server. The data directory remains unchanged — minor upgrades are that simple.

So really the comment should have said "to migrate data from a previous major version".

Beta versions are generally excluded from normal update rules, because they might contain braking changes in the data format that are later reverted, so to upgrade from a beta to a release version often requires an upgrade using full dump and restore.

I don't think we need to mention beta versions here at all, since homebrew didn't release beta versions of postgres as far as I know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@felixbuenemann
Copy link
Sponsor Contributor Author

Hmm, I have pushed a new revision 9c58f619da97b92e7d7851a6127791cfb13d7d43 that resolves the conflicts and updates the caveats, but for some reason github is not showing the new commits.

The caveats now are:

    To migrate existing data from a previous major version of PostgreSQL, see:
      https://www.postgresql.org/docs/10/static/upgrading.html

      You will need your previous PostgreSQL installation from brew to perform
      `pg_upgrade` or `pg_dumpall` depending on your upgrade method.

      Do not run `brew cleanup postgresql` until you have performed the migration.

I also removed the note about needing to rebuild hash indexes, because it will affect a very small number of people (the default index format is btree) and postgres should also display instructions on how to reindex them when running pg_upgrade and generate an sql file to do the REINDEX operations.

I found the following notes on https://www.credativ.com/blog/postgresql-10-bits-wal-logged-hashindex regarding the upgrade of hash indexes:

Your installation contains hash indexes. These indexes have different internal formats between your old and new clusters, so they must be reindexed with the REINDEX command. After upgrading, you will be given REINDEX instructions.
[…]
After pg_upgrade finished, a SQL-Script called reindex_hash.sql appeared in the working-directory. This file contains the necessary REINDEX-commands for the rebuilt.

Im not sure why github isn't showing the new commits, I'll try first resolving the conflicts using the "Resolve conflicts" button here and than do a force push again, maybe that helps.

@felixbuenemann
Copy link
Sponsor Contributor Author

Hmm, something with github caching seems to be acting up. If I try to manually resolve the conflict using the "Resolve conflicts" button, it tells me that the page is out of date, yet the new comments don't show up here …

@felixbuenemann
Copy link
Sponsor Contributor Author

felixbuenemann commented Oct 17, 2017

I amended the HEAD commit and pushed again (4dd7482ee2e6f4f9cc8a329b3a3597888d77643c), still not showing up, I guess we'll have to wait for github to fix their problems or contact github support.

@felixbuenemann
Copy link
Sponsor Contributor Author

felixbuenemann commented Oct 17, 2017

Ok, the GitHub Status page shows there are currently queue backlogs, that must be it:

https://status.github.com/messages

October 17, 2017 – 17:27 CEST We continue to investigate a large queue backlog. Commits will take longer than usual to appear in pull requests.

@ilovezfs
Copy link
Contributor

Yup. I've temporarily blocked all commits on master since what you see is not what you get right now.

@felixbuenemann felixbuenemann force-pushed the postgres-10 branch 2 times, most recently from 9c58f61 to 4dd7482 Compare October 17, 2017 16:00
@ilovezfs
Copy link
Contributor

Excellent work here, @felixbuenemann! Shipped.

@ilovezfs ilovezfs closed this in 2d2146b Oct 17, 2017
@felixbuenemann felixbuenemann deleted the postgres-10 branch October 17, 2017 21:18
@RobAtticus
Copy link

RobAtticus commented Oct 20, 2017

I know this has already been merged, but as a maintainer of a PostgreSQL extension that currently only builds for 9.6 I'm running into a problem. Because the PostgreSQL 9.6 formula installs itself into only its Cellar space, I cannot copy my extension files (.sql, .so, or .control) into the right directories, getting permission denied.

My only solution seems to add a script file to the Caveats section and have the user run it afterwards to move the files to the correct place. Is this correct or am I missing something? Wouldn't it be better to change the layout to have multiple versions of PostgreSQL in the same common area so extensions could still be installed correctly to older versions? e.g. /usr/local/lib/postgresql/9.6/ and /usr/local/lib/postgresql/10/

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
legacy Relates to a versioned @ formula new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants