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 missing docs for release 12.2.0 #3582

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

laurenceisla
Copy link
Member

Adds missing docs for new logging features.

@wolfgangwalther
Copy link
Member

Make sure to back-port this to the v12 branch, once you're done.

@laurenceisla
Copy link
Member Author

@wolfgangwalther Just to confirm, cherry-picking and merging to v12 should be enough, right?

@steve-chavez
Copy link
Member

Github CI is failing https://www.githubstatus.com/

Update - Customers may see issues running Actions
Jun 11, 2024 - 20:34 UTC

@wolfgangwalther
Copy link
Member

@wolfgangwalther Just to confirm, cherry-picking and merging to v12 should be enough, right?

Yes, correct.

Comment on lines 25 to 28
.. code:: postgresql

NOTIFY pgrst;
ERROR: cannot execute NOTIFY during recovery
Copy link
Member

@steve-chavez steve-chavez Jun 12, 2024

Choose a reason for hiding this comment

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

I think we should put the actual PostgREST error here. IIRC it says the same but with additional logs.

Something like this should work:

$ postgrest-with-* --replica postgrest-run

Maybe you'll have to edit the withTmp command on the nix file

Copy link
Member Author

Choose a reason for hiding this comment

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

The error in psql is exactly the same:

postgres=# notify pgrst;
ERROR:  cannot execute NOTIFY during recovery

Now, in the PostgREST 12.2.0 logs, when connecting to a read replica only, it shows:

12/Jun/2024:15:19:57 -0500: Failed listening for notifications on the "pgrst" channel. Just "connection to server on socket \"/run/user/1000/postgrest/postgrest-with-postgresql-16-Y3M/socket_replica_6039/.s.PGSQL.5432\" failed: session is read-only\n"
12/Jun/2024:15:19:57 -0500: Failed listening for notifications on the "pgrst" channel. ExitFailure 1
12/Jun/2024:15:19:57 -0500: Attempting to connect to the database...
12/Jun/2024:15:19:57 -0500: Retrying listening for notifications in 8 seconds...

Are you referring to the latter?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, maybe just put:

12/Jun/2024:15:19:57 -0500: Failed listening for notifications on the "pgrst" channel... connection to server .. failed: session is read-only

That error message needs fixing btw. Can be another issue.

@laurenceisla laurenceisla force-pushed the docs-log-stderr branch 3 times, most recently from 11dc0a8 to ebc14c4 Compare June 12, 2024 22:31
laurenceisla and others added 3 commits June 12, 2024 18:14
Co-authored-by: Steve Chavez <stevechavezast@gmail.com>
- Schema cache stats are now logged to stderr
- Log when the LISTEN channel gets a notification
@laurenceisla laurenceisla merged commit b52937e into PostgREST:main Jun 12, 2024
5 checks passed
@laurenceisla laurenceisla deleted the docs-log-stderr branch June 12, 2024 23:19

.. code:: console

12/Jun/2024:15:19:57 -0500: Failed listening for notifications on the "pgrst" channel... connection to server... failed: session is read-only
Copy link
Member

Choose a reason for hiding this comment

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

Ah, damn. I just realized this error doesn't make sense here, it's caused because of our target_session_attrs=read-write.

The previous one with plain SQL was much better. Maybe add something like:

-- check if the instance is a replica
postgres=# select pg_is_in_recovery();
 pg_is_in_recovery 
-------------------
 t
 (1 row)

postgres=# LISTEN pgrst;
<ERROR message here>

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.

None yet

3 participants