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

fix: diesel metrics for postgres #3943

Merged
merged 12 commits into from
Feb 28, 2024

Conversation

sabify
Copy link
Contributor

@sabify sabify commented Feb 22, 2024

Note: Quaint has been temporarily removed from the CI benchmark due to its feature set inconsistencies that cause compile errors. (#3942 (comment))

Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

Thanks for putting together this PR. 👍

I've left a couple of minor questions and some remarks about fixing some version numbers. Otherwise this should be fine.

sudo apt-get install -y libpq-dev postgresql
echo "host all all 127.0.0.1/32 md5" > sudo tee -a /etc/postgresql/10/main/pg_hba.conf
sudo service postgresql restart && sleep 3
sudo DEBIAN_FRONTEND=noninteractive apt-get --purge remove postgresql\* -y
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why you changed the postgresql version here?

Copy link
Contributor Author

@sabify sabify Feb 24, 2024

Choose a reason for hiding this comment

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

I did that to create a predictable environment for configuration and testing. Each distro with distinct versions has a different postgresql package installed, and sometimes its default service is up and running (like in github actions containers). By removing all installed postgresql packages, it will also stop their services and clusters.

Please consider that the default ubuntu package manager does not ship the latest stable PostgreSQL package (version 16, not even 15).

I chose postgresql version 16 as it is the latest stable version. If you are not happy with the latest stable version, simply changing the PG_VERSION job env var can install whatever is suitable for this job.

PS: Previous script tried using the default installed postgresql (currently 14), and then tried to configure postgresql 10 (/etc/postgresql/10/main/pg_hba.conf)! Something that never happened...

diesel_bench/Cargo.toml Outdated Show resolved Hide resolved
diesel_bench/Cargo.toml Outdated Show resolved Hide resolved
diesel_bench/Cargo.toml Outdated Show resolved Hide resolved
"runtime-tokio-rustls",
] }
futures = { version = "0.3", optional = true }
diesel-async = { version = "0", optional = true, default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

Again please prefer a more specific version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

diesel-async should always be in sync and compatible with the base diesel repository, is it still necessary to specify a minimum version here? We can benchmark against every commit in these two repositories.

.github/workflows/run_benches.yml Show resolved Hide resolved
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

This looks fine with these minor notes fixed.

echo 'DATABASE_URL=postgres://postgres:postgres@localhost/' >> $GITHUB_ENV
sudo service postgresql restart $PG_VERSION && sleep 3
echo 'DATABASE_URL=postgres://postgres:postgres@localhost:5432/' >> $GITHUB_ENV
env:
Copy link
Member

Choose a reason for hiding this comment

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

Can we move that to the top of the script block? I would expect that people (including me) will miss that place in the future otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

echo 'DATABASE_URL=postgres://postgres:postgres@localhost/' >> $GITHUB_ENV
sudo service postgresql restart $PG_VERSION && sleep 3
echo 'DATABASE_URL=postgres://postgres:postgres@localhost:5432/' >> $GITHUB_ENV
env:
Copy link
Member

Choose a reason for hiding this comment

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

Same as in the other file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@weiznich weiznich added this pull request to the merge queue Feb 28, 2024
Merged via the queue into diesel-rs:master with commit 33ddddc Feb 28, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants