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

add more env var config for postgres #5365

Merged
merged 8 commits into from
Sep 12, 2024
Merged

add more env var config for postgres #5365

merged 8 commits into from
Sep 12, 2024

Conversation

trinity-1686a
Copy link
Contributor

@trinity-1686a trinity-1686a commented Aug 29, 2024

Description

add environment variables related to postgres metastore:

  • QW_POSTGRES_SKIP_MIGRATIONS: don't run migrations, but verifies all migrations were already run
  • QW_POSTGRES_SKIP_MIGRATION_LOCKING: prevent locking database for migrations. This might make us more compatible with cockroachdb. We might not be entirely compatible still, as it doesn't support DDL in transaction.
  • QW_POSTGRES_READ_ONLY: make all connections read-only by default. This isn't really a security feature as we can do BEGIN READ WRITE to get a rw transaction, but it should prevent involuntary writes. While this doesn't imply QW_POSTGRES_SKIP_MIGRATIONS, being read only and running migrations always fail (even if migrations are up to date)

How was this PR tested?

tested manually against existing databases with missing and all migrations, and added unit test.

@trinity-1686a trinity-1686a requested a review from guilload August 29, 2024 12:24
Copy link

github-actions bot commented Aug 29, 2024

On SSD:

Average search latency is 1.0x that of the reference (lower is better).
Ref run id: 3372, ref commit: 20b4956
Link

On GCS:

Average search latency is 0.965x that of the reference (lower is better).
Ref run id: 3373, ref commit: 20b4956
Link

@@ -838,6 +838,15 @@ Disables [telemetry](../telemetry.md) when set to any non-empty value.

`QW_DISABLE_TELEMETRY=1 quickwit help`

### QW_POSTGRES_SKIP_MIGRATIONS

Don't run database migrations (but verify migrations were run succesfully before, and no unknown migration was run).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Don't run database migrations (but verify migrations were run succesfully before, and no unknown migration was run).
Don't run database migrations (but verify that migrations were run successfully before and that no unknown migration was run).


### QW_POSTGRES_SKIP_MIGRATION_LOCKING

Don't lock the database during migration. This may increase compatiblity with altenrative databases using the postgres wire protocol. It is dangerous
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Don't lock the database during migration. This may increase compatiblity with altenrative databases using the postgres wire protocol. It is dangerous
Don't lock the database during migration. This may increase compatibility with alternative databases using the PostgreSQL wire protocol. However, it is dangerous to use this if you can't guarantee that only one node will run the migrations.

@@ -0,0 +1,2 @@
ALTER TABLE shards
DROP IF EXISTS update_timestamps;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DROP IF EXISTS update_timestamps;
DROP IF EXISTS update_timestamp;

@trinity-1686a trinity-1686a enabled auto-merge (squash) September 12, 2024 13:27
@trinity-1686a trinity-1686a merged commit d142f59 into main Sep 12, 2024
5 checks passed
@trinity-1686a trinity-1686a deleted the trinity/pg-config branch September 12, 2024 13:40
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