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.

Sign up for GitHub

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

Jump to bottom

fix: avoid reloading Postgres when unnecessary #4531

Merged
merged 4 commits into from
May 15, 2024
Merged

fix: avoid reloading Postgres when unnecessary #4531

merged 4 commits into from
May 15, 2024

Conversation

leonardoce
Copy link
Contributor

@leonardoce leonardoce commented May 14, 2024 •
edited

The instance manager stores the PostgreSQL configuration inside a dictionary, mapping GUC names to their actual value.

When writing the configuration file, the code is iterating over a map. Since the iteration order is not defined, the configuration file was re-written even when the parameters didn't change.

This made the instance manager SIGHUP the postmaster every time, even if this was not needed.

This patch fixes the issue by writing the configuration file in lexicographic order.

Closes: #4529

@leonardoce leonardoce requested a review from a team as a code owner May 14, 2024 14:53
@github-actions github-actions bot added backport-requested ◀️ This pull request should be backported to all supported releases release-1.21 release-1.22 release-1.23 labels May 14, 2024
@github-actions GitHub Actions
Copy link
Contributor

❗ By default, the pull request is configured to backport to all release branches.

  • To stop backporting this pr, remove the label: backport-requested ◀️ or add the label 'do not backport'
  • To stop backporting this pr to a certain release branch, remove the specific branch label: release-x.y

@leonardoce
Copy link
Contributor Author

/test limit=local

@github-actions GitHub Actions
Copy link
Contributor

@leonardoce, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/9081617544

@leonardoce leonardoce changed the title fix: avoid reload PG when the parameters didn't change fix: avoid reloading PG when the parameters didn't change May 14, 2024
@gbartolini gbartolini changed the title fix: avoid reloading PG when the parameters didn't change fix: avoid reloading Postgres when unnecessary May 14, 2024
@leonardoce leonardoce self-assigned this May 14, 2024
@leonardoce
Copy link
Contributor Author

I'm waiting for the E2e test to finish before merging it.

@leonardoce
Copy link
Contributor Author

/test limit=local

@github-actions GitHub Actions
Copy link
Contributor

@leonardoce, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/9084593252

leonardoce and others added 4 commits May 15, 2024 08:12
@leonardoce
fix: avoid reload PG when the parameters didn't change
c5bceff 
The instance manages stores the PostgreSQL configuration inside a
dictionary, mapping GUC names to their actual value.

When writing the configuration file, the code is iterating over a map
and since the iteration order is not defined, was re-writing the
configuration file even when the parameters didn't really change.

This made the instance manager SIGHUP the postmaster everytime, even if
this is not really needed.

This patch fixes the issue by writing che configuration file in
lexicographic order.

Closes: cloudnative-pg#4529

Signed-off-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>
@gbartolini @leonardoce
Update pkg/configfile/configfile.go
f589568 
Signed-off-by: Gabriele Bartolini <gabriele.bartolini@enterprisedb.com>
@armru @leonardoce
chore: use sprint
7891549 
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
@armru @leonardoce
chore: use sprint
44b46a6 
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
@leonardoce
Copy link
Contributor Author

/ok-to-merge

@cnpg-bot cnpg-bot added the ok to merge 👌 This PR can be merged label May 15, 2024
@leonardoce leonardoce merged commit 5686af3 into cloudnative-pg:main May 15, 2024
28 of 29 checks passed
cnpg-bot pushed a commit that referenced this pull request May 15, 2024
@leonardoce
fix: avoid reloading Postgres when unnecessary (#4531)
2ddb144 
The instance manager stores the PostgreSQL configuration inside a
dictionary, mapping GUC names to their actual value.

When writing the configuration file, the code is iterating over a map.
Since the iteration order is not defined, the configuration file was
re-written even when the parameters didn't change.

This made the instance manager SIGHUP the postmaster every time, even if
this was not needed.

This patch fixes the issue by writing the configuration file in
lexicographic order.

Closes: #4529

Signed-off-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>
Signed-off-by: Gabriele Bartolini <gabriele.bartolini@enterprisedb.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Gabriele Bartolini <gabriele.bartolini@enterprisedb.com>
Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
(cherry picked from commit 5686af3)
cnpg-bot pushed a commit that referenced this pull request May 15, 2024
@leonardoce
fix: avoid reloading Postgres when unnecessary (#4531)
ccdfd30 
The instance manager stores the PostgreSQL configuration inside a
dictionary, mapping GUC names to their actual value.

When writing the configuration file, the code is iterating over a map.
Since the iteration order is not defined, the configuration file was
re-written even when the parameters didn't change.

This made the instance manager SIGHUP the postmaster every time, even if
this was not needed.

This patch fixes the issue by writing the configuration file in
lexicographic order.

Closes: #4529

Signed-off-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>
Signed-off-by: Gabriele Bartolini <gabriele.bartolini@enterprisedb.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Gabriele Bartolini <gabriele.bartolini@enterprisedb.com>
Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
(cherry picked from commit 5686af3)
cnpg-bot pushed a commit that referenced this pull request May 15, 2024
@leonardoce
fix: avoid reloading Postgres when unnecessary (#4531)
7f6d5ea 
The instance manager stores the PostgreSQL configuration inside a
dictionary, mapping GUC names to their actual value.

When writing the configuration file, the code is iterating over a map.
Since the iteration order is not defined, the configuration file was
re-written even when the parameters didn't change.

This made the instance manager SIGHUP the postmaster every time, even if
this was not needed.

This patch fixes the issue by writing the configuration file in
lexicographic order.

Closes: #4529

Signed-off-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>
Signed-off-by: Gabriele Bartolini <gabriele.bartolini@enterprisedb.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Gabriele Bartolini <gabriele.bartolini@enterprisedb.com>
Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
(cherry picked from commit 5686af3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbartolini gbartolini gbartolini approved these changes

@mnencia mnencia mnencia approved these changes

@armru armru armru approved these changes

Assignees

@leonardoce leonardoce

Labels
backport-requested ◀️ This pull request should be backported to all supported releases ok to merge 👌 This PR can be merged release-1.21 release-1.22 release-1.23
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Triggering SIGHUP too many times
5 participants
@leonardoce @mnencia @gbartolini @armru @cnpg-bot