-
Notifications
You must be signed in to change notification settings - Fork 169
chore: compile postgres statically to reduce the PLT function call #1064
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
Conversation
8ff9d04
to
097d18c
Compare
097d18c
to
42e6c83
Compare
42e6c83
to
e47a61a
Compare
dc0a421
to
f57317e
Compare
1df9a1d
to
03b5cf8
Compare
03b5cf8
to
e1244e6
Compare
Can wait for #1081 to be merged. |
e1244e6
to
76a530e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ic-cbdb-parallel (pull_request) still failed . |
9c60438
to
890474c
Compare
The job failed with the following error log, it looks like the github-ci runner has no space left on device
When compiling postgres, statically linking all object files causes the postgres binary to grow from less than 100KB @edespino @my-ship-it could someone help check the disk space limit of the runner? |
@tuhaihe @edespino Could you please help adjust space limit? thanks! |
890474c
to
c0b56a7
Compare
any coredump file that takes up space ? |
The default storage is 14GB (https://docs.github.com/en/actions/using-github-hosted-runners/using-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories), ~~diff --git a/.github/workflows/build-cloudberry.yml b/.github/workflows/build-cloudberry.yml
index a6e659596b5..0faa72e8ff1 100644
--- a/.github/workflows/build-cloudberry.yml
+++ b/.github/workflows/build-cloudberry.yml
@@ -887,6 +887,7 @@ jobs:
--ulimit core=-1
--cgroupns=host
-v /sys/fs/cgroup:/sys/fs/cgroup:rw
+ --storage-opt size=20G
~~~
Update:
|
f4632a3
to
14d9f83
Compare
1f63146
to
b293297
Compare
@gongxun0928 In a diagnostic manner, I am going to try and reproduce this issue (using your branch). I will let you know what I find. Obviously, please do not commit these changes until we identify the root cause. |
@edespino I added an option --link-postgres-with-shared, and when using --link-postgres-with-shared=no together with --enable-shared-postgres-backend, the ic-cbdb-parallel test triggers a disk space warning and make test failed: From previous debugging information, the available disk space during the execution of ic-cbdb-parallel was only 23GB. On my local machine, when running ic-cbdb-parallel, the filesystem usage for datadirs during the test exceeds 23GB. |
Here is the 1TB TPC-DS performance comparison before and after the PR Hardware and Operating System
Database Cluster Configuration
GUC Configuration
Query Performance Results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
4b00031
to
b16daa3
Compare
dfd774c
to
09a2eab
Compare
Two commits also needed to be cherry picked: |
6275817
to
3632053
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments, FYI.
Also, need to generate the latest configure file using the autoconf cmd.
f5e8a8f
to
00c6b06
Compare
00c6b06
to
0cb0c1f
Compare
0cb0c1f
to
f61616b
Compare
During TPC-DS testing, we observed that compiling postgres with libpostgres.so introduces PTL function call overhead for some functions. By linking object files (*.o) directly instead, we achieved a 5-8% performance improvement in the TPC-DS 1TB benchmark. This commit added an option enable_link_postgres_with_shared to link libpostgres.so when compiling postgres, and The default value is false, just like greenplum, statically linking all object files when compiling postgres. Additionally, this update fixes a minor bug: the pax extension has a dependency on libpostgres.so. Now, when enabling the pax entension, we check that enable_shared_postgres_backend is set to 'yes' to ensure proper functionality. And the ic-cbdb-parallel test has been migrated to use the release version instead of the debug version. This change was made because running the test on the debug version caused disk space issues. When both libpostgres.so and postgres are compiled in the debug version, disk usage increases by several hundred megabytes compared to the release version. As a result, the ic-cbdb-parallel test failed due to insufficient disk space. By switching to the release version, this issue is resolved, and the test runs faster as well.
f61616b
to
f925e2b
Compare
As discussed with Engineers, we have adopted most of the responses, and there is a long time no response yet.
When building postgres, link all the *.o files directly into the binary instead of linking libpostgres.so.
This approach reduces the overhead of PLT function calls. Based on our internal performance tests,
this change improves the performance of a 1TB TPC-DS benchmark by 5-8%.
Fixes #ISSUE_Number
What does this PR do?
Type of Change
Breaking Changes
Test Plan
make installcheck
make -C src/test installcheck-cbdb-parallel
Impact
Performance:
User-facing changes:
Dependencies:
Checklist
Additional Context
CI Skip Instructions