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

chore: admit general PG booleans for configuration #4494

Merged
merged 15 commits into from
May 28, 2024
Merged

chore: admit general PG booleans for configuration #4494

merged 15 commits into from
May 28, 2024

Conversation

jsilvela
Copy link
Collaborator

@jsilvela jsilvela commented May 9, 2024 •
edited

Allows any valid Postgres boolean for configuration in the webhooks.
Currently, only "off" and "on" are properly understood by the webhooks.

Closes #4252

@jsilvela jsilvela requested a review from a team as a code owner May 9, 2024 11:23
@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 9, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 9, 2024

❗ 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

@jsilvela
Copy link
Collaborator Author

jsilvela commented May 9, 2024

/test limit=local feature_type=smoke

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 9, 2024

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

@github-actions github-actions bot added the ok to merge 👌 This PR can be merged label May 9, 2024
mnencia
mnencia requested changes May 9, 2024
View reviewed changes
Copy link
Member

@mnencia mnencia left a comment •
edited

Choose a reason for hiding this comment

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

I don't think it's correct to have two different functions (IsTrue and IsFalse) to parse a Postgres boolean.

We should have a single function func ParsePostgresBoolean(in string) (bool, error) and then manage the parse errors in the webhook.

Also, the PostgreSQL documentation says "Values can be written as on, off, true, false, yes, no, 1, 0 (all case-insensitive) or any unambiguous prefix of one of these.", and this implementation doesn't recognise "unambiguous prefixes".

@jsilvela
Copy link
Collaborator Author

jsilvela commented May 9, 2024

What is an "unambiguous prefix"? I've been looking for explanations or examples and found none.

@jsilvela
Copy link
Collaborator Author

jsilvela commented May 9, 2024

Because strictly speaking, an unambiguous prefix could be "fa", or "of". Is that something we really want?

@mnencia
Copy link
Member

mnencia commented May 9, 2024

Because strictly speaking, an unambiguous prefix could be "fa", or "of". Is that something we really want?

Postgres does it https://doxygen.postgresql.org/bool_8c_source.html#l00036

@jsilvela
Copy link
Collaborator Author

jsilvela commented May 9, 2024

Realized, archive_mode is not a boolean but an enum. Leaving that one as was.

@jsilvela jsilvela requested a review from mnencia May 9, 2024 15:51
@gabriele-wolfox
Copy link
Contributor

Beside my suggestion, PR looks good to me.

@mnencia
Copy link
Member

mnencia commented May 16, 2024

Realized, archive_mode is not a boolean but an enum. Leaving that one as was.

You are right. Archive mode can only be off, on and always, so we can safely compare it to off.

mnencia
mnencia reviewed May 17, 2024
View reviewed changes
pkg/postgres/booleans.go Outdated Show resolved Hide resolved
@mnencia mnencia force-pushed the dev/4252 branch 2 times, most recently from c4c12a4 to e42e3f4 Compare May 20, 2024 10:26
@mnencia mnencia force-pushed the dev/4252 branch 7 times, most recently from c36e8a2 to ae87bac Compare May 23, 2024 12:10
jsilvela and others added 15 commits May 28, 2024 14:05
@jsilvela @leonardoce
chore: admit general PG booleans for configuration
06b4ff3 
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
@jsilvela @leonardoce
fix: add missing files
559b401 
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
@jsilvela @leonardoce
chore: make test clearer
1f10e42 
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
@jsilvela @leonardoce
fix: binary parsing, archive_mode is not boolean
4e09a79 
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
@jsilvela @leonardoce
chore: add test cases
c206641 
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
@mnencia @leonardoce
chore: review
5e0441b 
Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
@mnencia @gabriele-wolfox @leonardoce
chore: accepring gabriele-wolfox suggestion
3c38a12 
Co-authored-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
@jsilvela @leonardoce
Update pkg/postgres/booleans_test.go
e7d951a 
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
@jsilvela @leonardoce
Update pkg/postgres/booleans_test.go
e57607e 
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
@mnencia @leonardoce
test: be esplicit about o being ambiguos
a9c0707 
Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
@jsilvela @leonardoce
chore: switch to switch
58b8509 
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
@jsilvela @leonardoce
chore: reorder
7e84dde 
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
@mnencia @leonardoce
chore: use correct version of tooling
6a57658 
Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
@mnencia @leonardoce
chore: use the same order in for on and off
1081162 
Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
@mnencia @leonardoce
review: PostgreSQL doesn't allow surrounding booleans with spaces
8b2fc8d 
While it generally allow surrounding spaces when parsing booleans in SQL,
they are not permitted in the configuration file.

Adding a space before or after a value will cause the following log
entry on the reload and a failure on the restart:

{
  "level": "info",
  "ts": "2024-05-20T12:10:40Z",
  "logger": "postgres",
  "msg": "record",
  "logging_pod": "cluster-example-1",
  "record": {
    "log_time": "2024-05-20 12:10:40.498 UTC",
    "process_id": "1348",
    "session_id": "664b3da9.544",
    "session_line_num": "9",
    "session_start_time": "2024-05-20 12:10:17 UTC",
    "transaction_id": "0",
    "error_severity": "LOG",
    "sql_state_code": "22023",
    "message": "parameter \"wal_log_hints\" requires a Boolean value",
    "backend_type": "postmaster",
    "query_id": "0"
  }
}

Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
@leonardoce leonardoce merged commit 9a09548 into main May 28, 2024
29 checks passed
@leonardoce leonardoce deleted the dev/4252 branch May 28, 2024 12:24
cnpg-bot pushed a commit that referenced this pull request May 28, 2024
@jsilvela
chore: admit general PG booleans for configuration (#4494)
e3297b3 
Allows any valid Postgres boolean for configuration in the webhooks.
Currently, only `off` and `on` are properly understood by the webhooks.

Closes #4252

Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
Co-authored-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Co-authored-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
(cherry picked from commit 9a09548)
cnpg-bot pushed a commit that referenced this pull request May 28, 2024
@jsilvela
chore: admit general PG booleans for configuration (#4494)
6af85ea 
Allows any valid Postgres boolean for configuration in the webhooks.
Currently, only `off` and `on` are properly understood by the webhooks.

Closes #4252

Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
Co-authored-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Co-authored-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
(cherry picked from commit 9a09548)
cnpg-bot pushed a commit that referenced this pull request May 28, 2024
@jsilvela
chore: admit general PG booleans for configuration (#4494)
adcc37e 
Allows any valid Postgres boolean for configuration in the webhooks.
Currently, only `off` and `on` are properly understood by the webhooks.

Closes #4252

Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
Co-authored-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Co-authored-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
(cherry picked from commit 9a09548)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnencia mnencia mnencia approved these changes

@gabriele-wolfox gabriele-wolfox gabriele-wolfox approved these changes

@armru armru armru left review comments

@leonardoce leonardoce leonardoce approved these changes

Assignees
No one assigned
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.

[Feature]: Enahance parsing of postgres configuration booleans in webhooks
5 participants
@jsilvela @mnencia @gabriele-wolfox @leonardoce @armru