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

$PGHOST & $PGPORT are not respected. #492

Open
azazeal opened this issue Oct 26, 2023 · 2 comments
Open

$PGHOST & $PGPORT are not respected. #492

azazeal opened this issue Oct 26, 2023 · 2 comments

Comments

@azazeal
Copy link

azazeal commented Oct 26, 2023

Given the following environment variables:

  • $DATABASE_URL: postgres://
  • $PGHOST: some-host
  • $PGPORT: 8888
  • $PGDATABASE: some-db

I believe it'd be a logical assumption, for callers of dbmate, to expect that (dbmate) would attempt, in any such case, to connect to some-db, via dialing some-host on port 8888, while retaining postgres:// as the value of its url.URL.

However, what happens instead is that dbmate tries to connect via /var/run/postgresql/.s.PGSQL.5432 (or the equivalent platform default, as defined in connectionString), disregarding any Postgres-related environment variable that may be defined in its environment, and replaces the empty tokens (host, port, etc) with hard-coded values.

I believe that lib/pg, the underlying driver, supports these environment variables when considering each connection string / DSN.

Is this behavior intended? If not, happy to take a stab at a patch for it.

@azazeal azazeal added the bug label Oct 26, 2023
@azazeal
Copy link
Author

azazeal commented Oct 26, 2023

This snippet exhibits the expected behavior described above, when using lib/pq directly:

package main

import (
	"database/sql"
	"os"
	"testing"

	_ "github.com/lib/pq"
	"github.com/stretchr/testify/require"
)

func Test(t *testing.T) {
	// $PGHOST is not defined in either the environment nor DATABASE_URL, 
	// so lib/pq will use a default value in its place.
	t.Setenv("PGPORT", "16431")
	t.Setenv("PGUSER", "some-user")
	t.Setenv("PGPASSWORD", "some-password")
	t.Setenv("DATABASE_URL", "postgres:///postgres?sslmode=disable")

	db, err := sql.Open("postgres", os.Getenv("DATABASE_URL"))
	require.NoError(t, err)
	t.Cleanup(func() { assert.NoError(t, db.Close()) })

	row := db.QueryRow("SELECT current_database(), version()")
	require.NoError(t, row.Err())

	var dbName, version string
	require.NoError(t, row.Scan(&dbName, &version))

	assert.Equal(t, "postgres", dbName)
	assert.NotEmpty(t, version)
}

@dossy
Copy link
Collaborator

dossy commented Nov 15, 2023

dbmate's pg driver doesn't use the environment variables as default connection parameter values the way the libpq C client library does, but I think that would be a useful enhancement, if you'd like to contribute it.

I don't know if this is so much a bug as it is a feature request for dbmate, but that's not especially important.

This is how lib/pq handles supporting the environment variables, which may be interesting:

https://github.com/lib/pq/blob/3d613208bca2e74f2a20e04126ed30bcb5c4cc27/conn.go#L2002-L2082

I suppose if we're adding this capability in now, we should keep in mind what the latest Postgres 16 libpq C client library documents as available vs. what the underlying lib/pq Go client library supports, both as environment variables and connection string parameter keys. Ultimately, we can only support what the underlying lib/pq implements, and not what the libpq C client documents, so that's important to be aware of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants