Skip to content

sql: introduce and mandate explicit PostgreSQL connections#13552

Merged
benesch merged 2 commits intoMaterializeInc:mainfrom
benesch:postgres-connection
Jul 11, 2022
Merged

sql: introduce and mandate explicit PostgreSQL connections#13552
benesch merged 2 commits intoMaterializeInc:mainfrom
benesch:postgres-connection

Conversation

@benesch
Copy link
Contributor

@benesch benesch commented Jul 10, 2022

Introduce a new PostgreSQL connection type, as in...

CREATE CONNECTION pgconn FOR POSTGRES
  HOST postgres,
  DATABASE postgres,
  USER postgres,
  PASSWORD SECRET pgpass;

CREATE SOURCE pg FOR POSTGRES CONNECTION pgconn;

The connection options correspond to the old libpq connection parameters
in the obvious way.

As decided in https://github.com/MaterializeInc/database-issues/issues/3838, this commit also removes the old syntax for
specifying PostgreSQL connection details inline in the CREATE SOURCE
statement; connection details must now be specified in CREATE CONNECTION.

Touches https://github.com/MaterializeInc/database-issues/issues/3354.
Touches https://github.com/MaterializeInc/database-issues/issues/3838.

Motivation

  • This PR adds a known-desirable feature.

Testing

  • This PR has adequate test coverage / QA involvement has been duly considered.

Release notes

This PR includes the following user-facing behavior changes:

  • See description.

CREATE SOURCE psychic FROM POSTGRES CONNECTION 'host=kanto user=ash password=teamrocket dbname=pokemon' PUBLICATION 'red'
=>
CreateSource(CreateSourceStatement { name: UnresolvedObjectName([Ident("psychic")]), col_names: [], connection: Postgres { conn: "host=kanto user=ash password=teamrocket dbname=pokemon", publication: "red", details: None }, with_options: [], include_metadata: [], format: None, envelope: None, if_not_exists: false, materialized: false, key_constraint: None, remote: None })
error: Expected FOR, found HOST
Copy link
Contributor

Choose a reason for hiding this comment

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

Think this was meant to be a passing test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

D'oh! Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed!


> DROP SOURCE IF EXISTS mz_source CASCADE;
> DROP SECRET IF EXISTS pgpass CASCADE;
> DROP CONNECTION IF EXISTS pg CASCADE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, derp--this shouldn't be necessary! Looks like we missed adding this diff:

diff --git a/src/testdrive/src/action/sql.rs b/src/testdrive/src/action/sql.rs
index b9b58f807..1e129c2c7 100644
--- a/src/testdrive/src/action/sql.rs
+++ b/src/testdrive/src/action/sql.rs
@@ -27,9 +27,9 @@ use mz_ore::retry::Retry;
 use mz_ore::str::StrExt;
 use mz_pgrepr::{Interval, Jsonb, Numeric};
 use mz_sql_parser::ast::{
-    CreateClusterReplicaStatement, CreateClusterStatement, CreateDatabaseStatement,
-    CreateSchemaStatement, CreateSecretStatement, CreateSourceStatement, CreateTableStatement,
-    CreateViewStatement, Raw, ReplicaDefinition, Statement, ViewDefinition,
+    CreateClusterReplicaStatement, CreateClusterStatement, CreateConnectionStatement,
+    CreateDatabaseStatement, CreateSchemaStatement, CreateSecretStatement, CreateSourceStatement,
+    CreateTableStatement, CreateViewStatement, Raw, ReplicaDefinition, Statement, ViewDefinition,
 };
 
 use crate::action::{Action, ControlFlow, State};
@@ -140,6 +140,13 @@ impl Action for SqlAction {
                 )
                 .await
             }
+            Statement::CreateConnection(CreateConnectionStatement { name, .. }) => {
+                self.try_drop(
+                    &mut state.pgclient,
+                    &format!("DROP CONNECTION IF EXISTS {} CASCADE", name),
+                )
+                .await
+            }
             _ => Ok(()),
         }

Line above should already be unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're totally right that this was missed, but also this testdrive script runs with --no-reset so the logic there doesn't apply. So I applied your patch and left the code in question in place!

Copy link
Contributor

Choose a reason for hiding this comment

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

ah til

benesch added 2 commits July 11, 2022 14:17
Use the same TLS types in CSR connections and Kafka connections, where
appropriate. Also use a consistent naming scheme.
Introduce a new PostgreSQL connection type, as in...

    CREATE CONNECTION pgconn FOR POSTGRES
      HOST postgres,
      DATABASE postgres,
      USER postgres,
      PASSWORD SECRET pgpass;

    CREATE SOURCE pg FOR POSTGRES CONNECTION pgconn;

The connection options correspond to the old libpq connection parameters
in the obvious way.

As decided in #13374, this commit also removes the old syntax for
specifying PostgreSQL connection details inline in the `CREATE SOURCE`
statement; connection details *must* now be specified in `CREATE
CONNECTION`.

Touches #11504.
Touches #13374.
@benesch benesch force-pushed the postgres-connection branch from 4c3782a to 746b58b Compare July 11, 2022 18:27
@benesch benesch enabled auto-merge (rebase) July 11, 2022 18:31
@benesch
Copy link
Contributor Author

benesch commented Jul 11, 2022

Thanks very much for the thorough review, @sploiselle!

@benesch benesch merged commit fb6244a into MaterializeInc:main Jul 11, 2022
@benesch benesch deleted the postgres-connection branch July 11, 2022 19:14
def- added a commit to def-/materialize that referenced this pull request Mar 13, 2026
`X509::from_pem` only reads the first certificate from a PEM bundle,
so if `ssl_root_cert` contains multiple CA certificates (e.g., root +
intermediates or multiple roots), only the first is loaded into the
trust store. This causes TLS verification to fail with "unknown ca"
when the relevant trust anchor is not the first cert in the bundle,
diverging from libpq behavior which accepts CA bundles.

Fix by using `X509::stack_from_pem` (which parses all certs) and
adding each to the cert store, mirroring the pattern already used in
`pkcs12der_from_pem` in the same file.

Bug introduced in PR MaterializeInc#13552 (commit fb6244a) which changed from
file-based `set_ca_file()` to in-memory `add_cert(X509::from_pem())`.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
def- added a commit that referenced this pull request Mar 20, 2026
`X509::from_pem` only reads the first certificate from a PEM bundle, so
if `ssl_root_cert` contains multiple CA certificates (e.g., root +
intermediates or multiple roots), only the first is loaded into the
trust store. This causes TLS verification to fail with "unknown ca" when
the relevant trust anchor is not the first cert in the bundle, diverging
from libpq behavior which accepts CA bundles.

Fix by using `X509::stack_from_pem` (which parses all certs) and adding
each to the cert store, mirroring the pattern already used in
`pkcs12der_from_pem` in the same file.

Bug introduced in PR #13552 which changed from file-based
`set_ca_file()` to in-memory `add_cert(X509::from_pem())`.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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