Skip to content

dbt-materialize: Support connection overrides#36127

Open
SangJunBak wants to merge 3 commits intoMaterializeInc:mainfrom
SangJunBak:jun/dbt-oidc
Open

dbt-materialize: Support connection overrides#36127
SangJunBak wants to merge 3 commits intoMaterializeInc:mainfrom
SangJunBak:jun/dbt-oidc

Conversation

@SangJunBak
Copy link
Copy Markdown
Contributor

@SangJunBak SangJunBak commented Apr 16, 2026

Motivation

Unblocks OIDC auth because our dbt adapter doesn't allow inputting arbitrary options: https://materializeinc.slack.com/archives/C0A23PK183Z/p1776360806044789

Verification

  1. Had a local materialize instance running via bin/environmentd
  2. Saved following profiles.yml in the current directory:
dbt_test:
  target: dev
  outputs:
    dev:
      type: materialize
      host: localhost
      port: 6875
      user: materialize
      pass: materialize
      database: materialize
      schema: public
      options:
        oidc_auth_enabled: "true"
        welcome_message: "on"
  1. Ran
cd misc/dbt-materialize
uv venv dbt-venv
uv pip install --python dbt-venv/bin/python pip
dbt-venv/bin/pip install .
dbt-venv/bin/dbt debug --profiles-dir .
  1. Verified output had:
18:43:49    application_name: dbt-materialize v1.9.8
18:43:49    options: {'oidc_auth_enabled': 'true', 'welcome_message': 'on'}

Wasn't sure the best way to automate a unit test for this however.

@SangJunBak SangJunBak force-pushed the jun/dbt-oidc branch 2 times, most recently from 7ea9bf0 to 88e356d Compare April 17, 2026 18:36
@SangJunBak SangJunBak requested a review from bobbyiliev April 17, 2026 18:37
@SangJunBak SangJunBak changed the title dbt-materialize: Support OIDC auth dbt-materialize: Support connection overrides Apr 17, 2026
@SangJunBak SangJunBak marked this pull request as ready for review April 17, 2026 18:54
@SangJunBak SangJunBak requested a review from a team as a code owner April 17, 2026 18:54
@SangJunBak
Copy link
Copy Markdown
Contributor Author

SangJunBak commented Apr 17, 2026

@bobbyiliev Unsure how we actually deploy an update to our dbt adapter!

Also I decided to go with the approach of allowing an options override instead of accepting a oidc_auth_enabled boolean as a connection option. This is because:

  1. This oidc_auth_enabled option doesn't exist for cloud
  2. Future flexibility if we need to specify a connection option

@SangJunBak SangJunBak marked this pull request as draft April 20, 2026 15:04
@SangJunBak SangJunBak marked this pull request as ready for review April 20, 2026 15:13
Purpose is to allow OIDC auth via the oidc_auth_enabled connection option variable.
- Removes the initial monkey patch
- Copies much of the implementation in PGConnectionManager
@bobbyiliev
Copy link
Copy Markdown
Contributor

@bobbyiliev Unsure how we actually deploy an update to our dbt adapter!

@SangJunBak This gets auto released after bumping the version bump (version.py + setup.py). What we usually do is to split the version bump itself out into a follow-up release PR. We usually merge the feature + CHANGELOG entry first, then do a separate PR to cut the release to PyPI.

Would you mind reverting the verions bump and adding a changelog entry here? Then we can have a follow-up PR for the release itself. That way if there are other pending changes they all get released together.

@bobbyiliev
Copy link
Copy Markdown
Contributor

  1. Ran
cd misc/dbt-materialize
uv venv dbt-venv
uv pip install --python dbt-venv/bin/python pip
dbt-venv/bin/pip install .
dbt-venv/bin/dbt debug --profiles-dir .

Did you by any chance test this with a full dbt run / dbt build, not just dbt debug? The verification shows the options making it into the connection string, but I'd feel better knowing a real query round-trip works with an override that actually changes behavior (e.g. setting auto_route_catalog_queries: off). Happy to try this on my end if you hit any blockers setting this all up!

Comment on lines +44 to +45
options:
hint: 'overrides the PostgreSQL `options` connection parameter'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I totally hear you that this is tricky to unit-test, but do you think we could add an integration test in tests/adapter/ e.g. set a non-default option like welcome_message: on via a dbt_profile_target override and assert the server honored it? The options_dict.update(credentials.options) merge is the whole point of the feature. Happy to help sketch something out if useful!

Comment on lines 100 to +104
@classmethod
def open(cls, connection):
connection = super().open(connection)
# Much of the `open` method setup is copied from the `PostgresConnectionManager.open` method
# https://github.com/dbt-labs/dbt-adapters/blob/v1.17.3/dbt-postgres/src/dbt/adapters/postgres/connections.py#L102,
# except we allow users to override options.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This new open method copies a chunk of PostgresConnectionManager.open from dbt-postgres. Did you consider overriding a smaller surface instead (e.g. just the options-string construction and then super().open())? My worry is that when dbt-postgres adds new kwargs or changes retry logic, we'll silently fall behind. Totally fine if you already looked at this and it wasn't workable, just curious.

Copy link
Copy Markdown
Contributor Author

@SangJunBak SangJunBak Apr 22, 2026

Choose a reason for hiding this comment

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

My worry is that when dbt-postgres adds new kwargs or changes retry logic, we'll silently fall behind

I think this is a valid concern :( I tried to but because kwargs is defined internally and there's no way to override options through the credentials object, we can't use super().open(). The best way I could think of is extending off the previous approach of monkey patching psycopg, then doing some weird python thread blocking such that the monkey patch can accept input from our adapter's .open, but that seems worse than copying their implementation. FWIW prior art to this is something like Clickhouse https://github.com/ClickHouse/dbt-clickhouse/blob/main/dbt/adapters/clickhouse/connections.py

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe the correct implementation here however is inheriting off SQLConnectionManager instead of PostgresqlConnectionManager?


# If you bump this version, bump it in setup.py too.
version = "1.9.7"
version = "1.9.8"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mentioned this in the other comment but could we split the version bump (version.py + setup.py) out of this PR? Normally I prefer to merge the feature + document it under "Unreleased" in the CHANGELOG first, then do a separate release PR that bumps the version and publishes to PyPI. Keeps the feature history and release history cleanly separate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For sure!

@SangJunBak
Copy link
Copy Markdown
Contributor Author

Did you by any chance test this with a full dbt run / dbt build, not just dbt debug? The verification shows the options making it into the connection string, but I'd feel better knowing a real query round-trip works with an override that actually changes behavior (e.g. setting auto_route_catalog_queries: off). Happy to try this on my end if you hit any blockers setting this all up!

I think the verification also tries to connect, so the way I actually tested it was toggling welcome_message to on and off and seeing the output! I can write a unit test for this however!

@SangJunBak
Copy link
Copy Markdown
Contributor Author

SangJunBak commented Apr 22, 2026

Created an integration test in the latest commit. Can test via bin/mzcompose --find dbt-materialize run default -k test_options_override

@SangJunBak
Copy link
Copy Markdown
Contributor Author

Followup PR to bump the versions #36213

@bobbyiliev
Copy link
Copy Markdown
Contributor

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Claude finished @bobbyiliev's task in 1m 32s —— View job


Code Review

  • Gather context
  • Read changed files
  • Compose final review

Overall this is a clean, well-scoped implementation. A few issues worth addressing before merge:

connections.py — options string escaping (potential bug)

misc/dbt-materialize/dbt/adapters/materialize/connections.py:141

kwargs["options"] = " ".join([f"--{k}={v}" for k, v in options_parts])

Values containing spaces will produce a malformed options string. For example, options: {application_name: "my app"} would emit --application_name=my app, which psycopg2 passes directly to the server and would be parsed incorrectly (the token after the space would be treated as a separate, unrecognized option).

The PostgreSQL wire protocol expects values with spaces to be quoted: --application_name='my app'. Consider adding quoting for values that contain spaces or special characters:

def _format_option(k, v):
    # Quote values containing spaces
    if ' ' in str(v):
        return f"--{k}='{v}'"
    return f"--{k}={v}"
kwargs["options"] = " ".join([_format_option(k, v) for k, v in options_parts])

Fix this →

connections.pysearch_path can appear twice if also in credentials.options

misc/dbt-materialize/dbt/adapters/materialize/connections.py:137-139

search_path = credentials.search_path
if search_path is not None and search_path != "":
    options_parts.append(("search_path", search_path))

If a user also specifies search_path in their options: dict (e.g. to override the dbt-managed value), it will appear in options_parts twice — once from the options merge, and once from this block. The second one wins since psycopg2 uses the last occurrence, so the behavior is correct, but it's redundant. Consider skipping the append if search_path is already in options_dict:

if search_path is not None and search_path != "" and "search_path" not in options_dict:
    options_parts.append(("search_path", search_path))

connections.py — upstream divergence comment could be stronger

misc/dbt-materialize/dbt/adapters/materialize/connections.py:102-104

The comment pins to v1.17.3 which is good. One minor improvement: including a direct link to the upstream source file (not just a line anchor) would make it easier to diff in the future. That said, the existing comment is already better than most.

connections.pyapplication_name can be set both ways

The application_name is passed as a first-class kwarg at line 129, but a user could also put it in credentials.options. These are both sent to psycopg2, but via different mechanisms — psycopg2 keyword arg vs. the options string. The kwargs value takes precedence, meaning a user cannot override application_name via options: even though they might expect to. This is a minor UX inconsistency; not a bug.

test_connection_options.py — looks good

The integration test is well-designed: it flips both options to their non-default values and queries current_setting() to confirm they were applied server-side. This directly validates the options_dict.update(credentials.options) merge. The scope="class" fixture level is appropriate.

One suggestion: the test file currently has no schema key in dbt_profile_target. Check whether the base class fixture provides a default, or whether this should be explicit. If tests fail intermittently due to a missing schema, this could be the cause.

profile_template.yml — no type annotation for options

The new options field at line 44 has no type: annotation. For dbt init, this means it'll be prompted as a plain string, which would be confusing for a dict-valued field. The field is optional (no default:), so it may just be skipped by dbt init in practice — but it's worth verifying or adding type: dict if supported.

CHANGELOG.md — clean

Entry under Unreleased is correct and the version bump has been removed per the review discussion.


Summary: The core logic is correct and the approach (copying open() and adding the merge before passing options to psycopg2) is a reasonable trade-off given the upstream API limitations. The main thing worth fixing before merge is the options string value escaping for values with spaces.

@SangJunBak
Copy link
Copy Markdown
Contributor Author

Regarding claude's output, so I did notice that spaces had no negative effect in the connection. I.e. setting the cluster to "weewoo quickstart" would still work, even with other arguments. This might be a postgres specific thing that Materialize already handles?

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