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

Add access to the PostgREST version through SQL #2740

Merged
merged 12 commits into from
Jul 3, 2023

Conversation

laurenceisla
Copy link
Member

@laurenceisla laurenceisla commented Apr 6, 2023

Closes #2647.

@steve-chavez
Copy link
Member

Check #2647 (comment). It might not be necessary to mess with the conn URI.

@steve-chavez
Copy link
Member

There's an issue with the env var approach on #2647 (comment).

Also, seems osm2pgsql-dev/osm2pgsql#867 also adds the fallback_application_name by parsing the connstring. Maybe check their implementation.

@laurenceisla
Copy link
Member Author

Also, seems openstreetmap/osm2pgsql#867 also adds the fallback_application_name by parsing the connstring. Maybe check their implementation.

They don't use a URI connection string but a Key-Value one instead. Also they receive each connection parameter separately by the user (user, password, database, etc.) and then build the connstring themselves, so it's easier to add the fallback parameter there.

@steve-chavez
Copy link
Member

They don't use a URI connection string but a Key-Value one instead.
and then build the connstring themselves

Got it. We have it a bit more complicated. But we should be fine if we doctest(maybe io test too) our connstring mutation.

src/PostgREST/Config.hs Outdated Show resolved Hide resolved
@laurenceisla
Copy link
Member Author

I need some confirmation in the way this feature behaves:

  1. When doing a

    select application_name 
    from pg_stat_activity;

    It returns the version twice:

    application_name
    ----------------------------
     PostgREST 11.1.0 (11a9849)    
     PostgREST 11.1.0 (11a9849)
    

    One due to the initial connection and other for the listener.

  2. After that, the initial connection is released once the db-pool-acquisition-timeout is reached and it will show only one PostgREST 11.1.0 (11a9849) due to the listener.

  3. If the listener is deactivated by db-channel-enabled=false then no PostgREST <version> will be shown after the acquisition timeout. That is, selecting the application_name will show no PostgREST version (no active connection is in the pool).

  4. After that timeout, only doing a request that reaches the database will show the version again after querying the application_name in the database. Once it's released it won't show it anymore.

@steve-chavez
Copy link
Member

steve-chavez commented Jun 20, 2023

@laurenceisla Those are not problems IMO. This is just an extra nicety for debugging. We can document it's not reliable if there's no pg connection(and by default there will always be one bc of the listen channel). For 1, the user can do a distinct(#2647, this will be documented too).

@laurenceisla laurenceisla force-pushed the select-version branch 2 times, most recently from bb0c835 to 5a04ec7 Compare June 22, 2023 22:55
@laurenceisla laurenceisla marked this pull request as ready for review June 22, 2023 23:30
@laurenceisla
Copy link
Member Author

I'm having a problem with the doctest. Running postgrest-run-doctests in a local environment and the CI throws this error:

src/PostgREST/Config.hs:469: failure in expression `addPgrstVerToDbUri "postgres://user:pass@host:5432/postgres"'
expected: "postgres://user:pass@host:5432/postgres?fallback_application_name=PostgREST%20..."
 but got: 
          ^
          <interactive>:27:1: error:
              Variable not in scope: addPgrstVerToDbUri :: t0 -> t

Examples: 150  Tried: 147  Errors: 0  Failures: 1

But stack test works normally:

postgrest> test (suite: doctests)
                       
Examples: 150  Tried: 150  Errors: 0  Failures: 0

postgrest> Test suite doctests passed
postgrest> test (suite: spec)

@steve-chavez
Copy link
Member

steve-chavez commented Jun 27, 2023

@laurenceisla So the full error is:

$ postgrest-test-doctests

src/PostgREST/Version.hs:12:1: error:
    Could not find module ‘Paths_postgrest’
    Perhaps you meant Paths_doctest
    Use -v (or `:set -v` in ghci) to see a list of the files searched for.
src/PostgREST/Config.hs:469: failure in expression `addPgrstVerToDbUri "postgres://user:pass@host:5432/postgres"'
expected: "postgres://user:pass@host:5432/postgres?fallback_application_name=PostgREST%20..."
 but got:
          ^
          <interactive>:27:1: error:
              Variable not in scope: addPgrstVerToDbUri :: t0 -> t

Examples: 150  Tried: 147  Errors: 0  Failures: 1

See:

    Could not find module ‘Paths_postgrest’
    Perhaps you meant Paths_doctest

Seems the doctest environment can find Paths_postgrest. This is bc you import PostgREST.Version in Config.hs and that is dependent on Paths_postgrest.

@steve-chavez
Copy link
Member

Tried adding

postgrest.cabal

test-suite doctests
  other-modules:      Paths_postgrest

But no dice

@steve-chavez
Copy link
Member

steve-chavez commented Jun 27, 2023

@steve-chavez
Copy link
Member

Fixed the doctests by making the addPgrstVerToDbUri pure, i.e. not depend on Paths_postgrest.

-- >>> let ver = "11.1.0 (5a04ec7)"::ByteString
--
-- >>> addFallbackAppName ver "postgres://user:pass@host:5432/postgres"
-- "postgres://user:pass@host:5432/postgres?fallback_application_name=PostgREST%20..."
Copy link
Member

Choose a reason for hiding this comment

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

Btw, I've noticed these ... work as a wildcard. Do you have a link for this behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, found it in their repo's README: https://github.com/sol/doctest#matching-arbitrary-output

test/io/test_io.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Obtain the PostgREST version through SQL
3 participants