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

Allow optional -D/--pgdata flag for PG upgrade #49135

Conversation

stevenharman
Copy link
Contributor

@stevenharman stevenharman commented Jan 17, 2020

Postgres can be installed with different data directories, or with a non-default location. This allows a user to upgrade those instances too.

  • 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?
  • Is your test running fine brew test <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>)?

This is for an external command, rather than a formula, so I'm not sure the later three apply. That said, I have tested this locally with both the defaults and a --pgdata ~/.pgdata option and successfully upgraded from PG 11 to 12.

Postgres can be installed with different data directories, or with a
non-default location. This allows a user to upgrade those instances too.
require "optparse"
options = {}
OptionParser.new do |opts|
opts.on("-D", "--pgdata DATADIR", "Location of the database configuration files.") do |data_dir|
Copy link
Contributor Author

@stevenharman stevenharman Jan 17, 2020

Choose a reason for hiding this comment

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

I mirrored the option names from pg_ctl in hopes of keeping it consistent for folks using that option. But I'm happy to use another name.

@fxcoudert fxcoudert requested a review from MikeMcQuaid Jan 18, 2020
@fxcoudert fxcoudert added the maintainer feedback label Jan 18, 2020
@MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Jan 18, 2020

Postgres can be installed with different data directories, or with a non-default location. This allows a user to upgrade those instances too.

That's never done by Homebrew's formula, though, right?

@stevenharman
Copy link
Contributor Author

@stevenharman stevenharman commented Jan 18, 2020

That's never done by Homebrew's formula, though, right?

I think we're talking about different things here, and it's probably my fault for saying "can be installed into different data directories."

Postgres is installed, by Homebrew, into wherever Homebrew is configured to install things. On my machine, that's:

brew --prefix
/usr/local

That's where the PostgreSQL binaries and such live. So when you pg_ctl, psql, etc... that's what's running. Those binaries use the configuration and backing data files, as defined by the DATADIR to spin up a running Postgres instance. i.e., a running Postgres database instance is the binaries plus the config + data files. (See also: PGDATA docs)

When initializing, starting, or stopping an instance of Postgres, you can tell it which DATADIR to use via the -D or --pgdata flags.

initdb -D <some/path>

pg_ctl -D <some/path> start

pg_ctl -D <some/path> stop

If you don't provide this option, it uses the PGDATA env var, which has a default value of something like /usr/local/var/postgres/ on my machine. But that's just a default, and so it's also what things like brew services start postgresql will use, as a default. brew info mentions this, though it's not very obvious:

$ brew info postgresql
postgresql: stable 12.1 (bottled), HEAD
[... snip ...]
==> Caveats
To migrate existing data from a previous major version of PostgreSQL run:
  brew postgresql-upgrade-database

To have launchd start postgresql now and restart at login:
  brew services start postgresql
Or, if you don't want/need a background service you can just run:
  pg_ctl -D /usr/local/var/postgres start
[... snip ...]

That last line ⬆️ shows the option, but it's using the default value, making this all the more confusing. The Postgres upgrade script was relying on that default always being used, and thus it was hard-coded in. But it is totally possible to have a single Hombrew-installed PostgreSQL, and run multiple instances of it, each pointing at a different data directory. Or to do what I do, and only run a single instance, but keep the data w/in my user space.

Does that make sense?

@MikeMcQuaid MikeMcQuaid removed their request for review Jan 20, 2020
@MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Jan 20, 2020

Does that make sense?

Yes, thanks for the complete explanation.

My thinking is that unless brew install postgresql or brew postinstall postgresql installs into another directory I'm hesitant to add functionality to this script (that will be indefinitely supported/relied on) that exists only to operate on configurations that Homebrew hasn't set up.

@stevenharman
Copy link
Contributor Author

@stevenharman stevenharman commented Jan 20, 2020

I understand the hesitation, for sure. However, the option to specify a datadir is so fundamental to PostgreSQL that it's not really option. initdb, which is used by brew post install postgresql, requires either the PGDATA Env var be set, or the -D flag to be passed. It could be argued that the Homebrew Postgres formula has a bug; it ignores the PGDATA Env var, and doesn't accept a -D flag, instead forcing a hard-coded path upon users.

Similarly, the pg_ctl command requires either PGDATA or the -D flag, and that flag is again hard-coded for brew services via the formula's plist_options (

plist_options :manual => "pg_ctl -D #{HOMEBREW_PREFIX}/var/postgres start"
).

In its current form, brew postinstall postgresql is creating an Postgres database cluster, which a user might never use. And an unfortunate side effect of the same hard-coding in the database upgrade script is that other clusters cannot be upgraded with this script.

My hope was to allow an optional datadir to be specified in the upgrade, while still defaulting to the same hard-coded values that the formula itself uses. And I realize now, the upgrade should probably also respect any PGDATA Env var that's set.

Does that make a better case for the proposed change? Or would a corresponding change in the formula itself need to be proposed and accepted?

Thank you for your time and all of the effort you put into this project.

@MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Jan 21, 2020

It could be argued that the Homebrew Postgres formula has a bug; it ignores the PGDATA Env var, and doesn't accept a -D flag, instead forcing a hard-coded path upon users.

This is intentional behaviour.

My hope was to allow an optional datadir to be specified in the upgrade, while still defaulting to the same hard-coded values that the formula itself uses. And I realize now, the upgrade should probably also respect any PGDATA Env var that's set.

Homebrew intentionally filters out all environment variables to make a clean and consistent configuration.

As a result and discussion above: sorry but passing on this PR.

Thank you for your time and all of the effort you put into this project.

Thanks for the kind words and the PR ❤️

@lock lock bot added the outdated label Feb 21, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintainer feedback outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants