Skip to content

Conversation

@vmercierfr
Copy link
Collaborator

@vmercierfr vmercierfr commented Apr 16, 2024

Objective

Initial import of PostgreSQL Partition Manager (aka PPM)

Why

This opinionated tool is designed to simplify the use of PostgreSQL partitions for developers by providing a secure, non-blocking, and intuitive tool.

Notes for reviews

The README provides information about supported features, limitations and how to setup local development environment.

Notes:

  • google/uuid implements RFC 4122 and can parse UUIDv7 but can't generate UUIDv7 from a timestamp. So, I created a UUID7 package. At some point, we should use a more reliable implementation.
  • We automatically retry DDL queries 3 times since they may fail if other queries have higher locks
  • lock_timeout and statement_timeout will prevent any infinite connections
  • We have unit tests in Go and end-to-end tests via bats. GitHub workflows perform e2e tests across support PostgreSQL versions (14, 15, and 16)
  • Code coverage can be easily viewed with the command make test && open cover.html
  • PPM package in pkg/ppm is designed to be a public resource that we can embed in future projects (e.g. a Kubernetes operator implementation of PPM)

Control points for the review:

  • General
    • CLI parameter names
    • Configuration default values
  • Go
    • Package names, split and public/private function
    • Postgresql package in internal/infra/postgresql is very large and may be split

CLI

./postgresql-partition-manager --help 
Simplified PostgreSQL partioning management

Usage:
  ppm [command]

Available Commands:
  completion  Generate the autocompletion script for the specified shell
  help        Help about any command
  run         Perform partition operations
  validate    Check configuration file

Flags:
  -c, --config string              config file (default is $HOME/postgresql-partition-manager.yaml)
  -u, --connection-url string      Path under which to expose metrics
  -d, --debug                      Enable debug mode
  -h, --help                       help for ppm
      --lock-timeout string        Set lock_timeout (ms) (default "100")
  -l, --log-format string          Log format (text or json) (default "json")
      --statement-timeout string   Set statement_timeout (ms) (default "3000")
  -v, --version                    version for ppm

Use "ppm [command] --help" for more information about a command.
./postgresql-partition-manager run --help 
Perform partition operations

Usage:
  ppm run [flags]
  ppm run [command]

Available Commands:
  all          Perform partitions provisioning, cleanup, and check
  check        Check existing partitions
  cleanup      Remove outdated partitions
  provisioning Create and attach new partitions

Flags:
  -h, --help   help for run

Global Flags:
  -c, --config string              config file (default is $HOME/postgresql-partition-manager.yaml)
  -u, --connection-url string      Path under which to expose metrics
  -d, --debug                      Enable debug mode
      --lock-timeout string        Set lock_timeout (ms) (default "100")
  -l, --log-format string          Log format (text or json) (default "json")
      --statement-timeout string   Set statement_timeout (ms) (default "3000")

Use "ppm run [command] --help" for more information about a command.

bats

ok 1 Test returns its version
ok 2 Test help message
ok 3 Test exit code on PostgreSQL connection error
ok 4 Program should exit when passed an empty configuration file
ok 5 Ensure validate command executes successfully with a valid configuration file
ok 6 Test check return a success on valid configuration
ok 7 Test check return an error when retention partitions are missing
ok 8 Test check return an error when preProvisioned partitions are missing
ok 9 Test that provisioning succeed on up-to-date partitioning
ok 10 Test that preProvisioned and retention partitions can be increased
ok 11 Test that useless partitions are removed by the cleanup

How

n/a

Release plan

  • Merge this PR

@vmercierfr vmercierfr linked an issue Apr 16, 2024 that may be closed by this pull request
@vmercierfr vmercierfr requested a review from dverite April 16, 2024 16:10
@vmercierfr vmercierfr force-pushed the init branch 7 times, most recently from 4a4f5ca to da18503 Compare April 18, 2024 14:14
Copy link
Contributor

@qfritz qfritz left a comment

Choose a reason for hiding this comment

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

First round of review from me, commenting to keep track myself (currently at commit 5eb61b6)

Copy link
Contributor

@qfritz qfritz left a comment

Choose a reason for hiding this comment

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

still in progress on postgresql package

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed in team, this package could be improved to select a few errors codes where the retry is needed like 25P03 or 57P05 or 08***, which could be reused at higher levels. Rest can fall into error without retries.

@qfritz
Copy link
Contributor

qfritz commented Apr 19, 2024

👇👇👇
Opened #6 to discuss postgresql package
👆👆👆

@qfritz qfritz linked an issue Apr 19, 2024 that may be closed by this pull request
Copy link
Contributor

@qfritz qfritz left a comment

Choose a reason for hiding this comment

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

At 33dc866 commit (still ongoing)

Copy link
Contributor

@dcupif dcupif left a comment

Choose a reason for hiding this comment

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

First batch of comments, I'll answer to @qfritz issue to tackle "code structure"

@vmercierfr vmercierfr force-pushed the init branch 4 times, most recently from d2b431a to 304aeef Compare April 25, 2024 07:20
@vmercierfr vmercierfr requested review from dcupif and qfritz April 25, 2024 07:20
Copy link
Contributor

@qfritz qfritz left a comment

Choose a reason for hiding this comment

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

  • discussed IRL about credentials and the network issue

@vmercierfr vmercierfr force-pushed the init branch 2 times, most recently from 5f0a3b9 to bc74975 Compare April 25, 2024 13:47
Copy link
Contributor

@dcupif dcupif left a comment

Choose a reason for hiding this comment

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

🚀 👏

@github-actions
Copy link

Code Coverage

Package Line Rate Health
github.com/qonto/postgresql-partition-manager/internal/infra/postgresql 89%
github.com/qonto/postgresql-partition-manager/internal/infra/retry 100%
github.com/qonto/postgresql-partition-manager/internal/infra/uuid7 91%
github.com/qonto/postgresql-partition-manager/pkg/ppm 73%
Summary 78% (540 / 688)

Minimum allowed line rate is 60%

@vmercierfr vmercierfr merged commit d359970 into main Apr 25, 2024
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.

Incorrect bounds with uuid type?

5 participants