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

P0 | Disable pg_statements extension with CI test pipeline #1406

Closed
geleeroyale opened this issue Mar 12, 2024 · 19 comments
Closed

P0 | Disable pg_statements extension with CI test pipeline #1406

geleeroyale opened this issue Mar 12, 2024 · 19 comments
Assignees
Labels

Comments

@geleeroyale
Copy link
Collaborator

We need to use the postgres extension pg_statements in our databases to improve query monitoring. It is the prime reason why we switched hosted databases from Digitalocean to AWS.

Now - when we use a backup of our staging database as part of our CI-pipelines we get the following error:
image


Possible fix: Try to remove the whole extension instead of just dropping the view

DROP EXTENSION IF EXISTS pg_stat_statements CASCADE;
@Rolazo
Copy link
Contributor

Rolazo commented Mar 12, 2024

CI working and test passed. But off course we disable pg_stat_statements.
LIke here

Although this extension is needed because we build query tracking with Grafana + Prometheus.

WIthout this extension we can make it work the monitoring built

@Rolazo
Copy link
Contributor

Rolazo commented Mar 12, 2024

@CarlosQ96
To fix this is pretty straighfoward.

Avoid using CASCADE, you can drop any view, but when you said CASCADE is going to delete anything related.

image

@Rolazo
Copy link
Contributor

Rolazo commented Mar 12, 2024

Currently CI test pipeline works perfectly fine without that extension. It's a temporary solution, now we need to figure it out how to disabling it from the test, specifically the CASCADE

@mhmdksh
Copy link
Collaborator

mhmdksh commented Mar 13, 2024

@Rolazo @geleeroyale After thinking about this, and instead of modifying anything in the tests.

I would suggest going with the below solution:

WDYT??

@mhmdksh
Copy link
Collaborator

mhmdksh commented Mar 13, 2024

I left this PR: Giveth/postgres-givethio#9 to add this feature and tested it.

What is left to do is:

  1. Merge the PR
  2. Enable the pg_stat_statements again from AWS
  3. Monitor the CI-CD pipeline

Let me know if this helps @Rolazo @geleeroyale

@Rolazo
Copy link
Contributor

Rolazo commented Mar 13, 2024

@mhmdksh @geleeroyale

Explaining the current problem:

We can enable and disable functions on our current RDS Postgres database, which is why also we use AWS. Others host providers doesn't allow us to do it.

Those functions once enabled, are 100% always enabled. Therefore when we do backups of our databases, those backups will contains all details (data-functions-procedures-schema)

The current staging-pipeline, is not using our staging database. It's using the backup store in s3.

So it's not a problem for enabling and disabling functions.

Why we shoudn't:

1. Disabling and enabling it: Since we have query tracking, we have prometheus containers and grafana dashboards expecting pg_stat_statements. I'm aiming to build alerts based on query-timings. This means disabling that function from our database is gonna make the whole thing failed.

So regardles to your PR
We have already enabled that function.

After searching there's a easy solution for this:

Backups excluding function (pg_stat_statements):
We only need to specify arguments when we take the backup, excluding the problematic function. Once the function is excluded in the backup, the staging pipeline is going to retrieve a backups without that function. Which is going to pass the test
=)

There's info our there:
old but gold
Going to exclude that function from the backup

@mhmdksh
Copy link
Collaborator

mhmdksh commented Mar 13, 2024

@Rolazo It seems to me you are not 100% aware of how we are using our own testing Docker Container in the CI that is found in the repository here: https://github.com/Giveth/postgres-givethio

Again, I am not able to understand why you need to over-complicate this. As I see it:

Currently, the scenario is:

  1. The backup is tested in the CI on a Docker Image that doesn't support pg_stat_statements ---> Results: FAILING

And after Adding the pg_stat_statements support to our image, the expected scenario should be:

  1. The backup is tested in the CI on a Docker Image that supports pg_stat_statements ---> Results: SUCCESS (Hopefully)

Why dig very deep into postgres if we can try simpler approaches?? Please don't do anything regarding the backups (Don't do any exclusions) Before we all meet up and agree on this. @geleeroyale FYI

@Rolazo
Copy link
Contributor

Rolazo commented Mar 13, 2024

@mhmdksh
Look the logs, the view cant be drop becouse pg_stat_statements requires it
image
The postgres container in the CI doesn't contains the module pg_stat_statements. But when you insert the backup, containing the module. It will enable that module. So the module is going to be enable by the backup

The current scenario is:

  1. Without module pg_stat_statements --> Working
    I disabled the module from our database, did the backup without the module. Run the CI staging and worked

  2. With module pg_stat_statements --> Failing

@Rolazo
Copy link
Contributor

Rolazo commented Mar 13, 2024

It's actually pretty straighfoward.

Just modifying in the options here with:
image

@mhmdksh
Copy link
Collaborator

mhmdksh commented Mar 13, 2024

@Rolazo I thought this option was added already, but it seems it might be a different thing. Please go ahead and try it. And update us how it goes.

@geleeroyale
Copy link
Collaborator Author

@Rolazo I thought this option was added already, but it seems it might be a different thing. Please go ahead and try it. And update us how it goes.

Hey @mhmdksh - I am not as deep into the thing as you guys when it comes to postgres, but from what I have seen, I do think that we should try @Rolazo 's approach. Excluding the extension from the backup is a great thing - saves us some trouble.

Also - you should know that we met together with Carlos to find a way to solve it and Krati signed off on these changes. We are getting to a place where we can have better planning before work starts.

But I have one question to @Rolazo - can you think of a negative implication removing the extension. We definitely need to test that we can use the backups to fully restore a Giveth db

@Rolazo
Copy link
Contributor

Rolazo commented Mar 15, 2024

Alright I came to the following conclusions:

  1. There is no test like DROP VIEW, after searching for all the tests. It seems this is a pre-test doing things by default any dev hasn't built the annoying test that we're looking for.

For future reference, how did I solve it.

I create another schema called stats_schema:

List of our current schemas

       List of schemas
     Name     |     Owner     
--------------+---------------
 cron         | rds_superuser
 public       | giveth
 stats_schema | giveth

Then I created the extension under that schema:
CREATE EXTENSION pg_stat_statements SCHEMA stats_schema;
After that, I set that schema to look also for public schema (The schema that we use), to track stats for our database.
SET search_path TO stats_schema, public;

This query proof that the extension belongs to the stats_schema

SELECT e.extname AS extension_name,
       n.nspname AS schema_name
FROM pg_extension e
JOIN pg_namespace n ON e.extnamespace = n.oid
WHERE e.extname = 'pg_stat_statements';
   extension_name   | schema_name  
--------------------+--------------
 pg_stat_statements | stats_schema
(1 row)

Since pg_stat_statements is under the stats_schema, we only need to exclude that schema from our backups, which is not being used for anything else.

Commit excluding the schema:
https://github.com/Giveth/devops/commit/799b2b9386388dd4cbb197cd18d4aba38b9b9224

This will allow us to make use of pg_stat_statements, passing the test without affecting our database =)

@geleeroyale @mhmdksh FYI

@Rolazo
Copy link
Contributor

Rolazo commented Mar 15, 2024

I re run the problematic CI with the latest backups, excluding stats_schema and it worked.
https://github.com/Giveth/impact-graph/actions/runs/8233230115/job/22685936824
Feel free to close this issue:

@mhmdksh
Copy link
Collaborator

mhmdksh commented Mar 15, 2024

@Rolazo
So you are saying that you enabled the pg_stat_statements again in the staging, but with a different schema???
It will perform query monitoring in the same way?? Or will changing the schema exclude the monitoring for the public schema? <---- (Did you test this in the Monitoring?)

Regardless of the CI working again or not, Is the query monitoring (The original Functionality that we need) performing the same with this change??

@Rolazo
Copy link
Contributor

Rolazo commented Mar 17, 2024

@mhmdksh
Exactly.
Cool feature about Postgres right ? You can have a module in a different schema and set it for other schema like I did:
SET search_path TO stats_schema, public;
stats_schema which contains pg_stats_statements will track public schema which is where our data is located

This is going to make our tracking work and passing the test

@mhmdksh
Copy link
Collaborator

mhmdksh commented Mar 18, 2024

Good job @Rolazo let's do a final review together before we close this issue

@jainkrati
Copy link
Collaborator

Pls move it to QA in zenhub once you have finalized the solution and deployed.

@mhmdksh
Copy link
Collaborator

mhmdksh commented Mar 19, 2024

@jainkrati I moved it, and reviewed it. It seems to be working as intended.

We can close this now

@Rolazo
Copy link
Contributor

Rolazo commented Mar 19, 2024

Yep, it was deployed and tested! Worked as intended

@mhmdksh mhmdksh closed this as completed Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants