Skip to content

fix(arrow_stream): propagate errors instead of panicking on connection setup#910

Open
ricardo-valero wants to merge 1 commit into
sfu-db:mainfrom
ricardo-valero:fix/arrow-stream-ssl-error-propagation
Open

fix(arrow_stream): propagate errors instead of panicking on connection setup#910
ricardo-valero wants to merge 1 commit into
sfu-db:mainfrom
ricardo-valero:fix/arrow-stream-ssl-error-propagation

Conversation

@ricardo-valero
Copy link
Copy Markdown

Summary

Fixes #909return_type=\"arrow_stream\" panicked at connectorx/src/get_arrow.rs:318:22 (or :355 for cursor) with PostgresPoolError(Error(None)) whenever the initial Postgres connection setup failed (notably with sslmode=require), while the equivalent return_type=\"arrow\" succeeded.

The root cause is that new_record_batch_iter used .unwrap() everywhere, panicking the thread and masking the wrapped r2d2 error. get_arrow already returned errors via #[throws(ConnectorXOutError)] and ?; this change brings the streaming path in line.

Changes

  • connectorx/src/get_arrow.rsnew_record_batch_iter is now #[throws(ConnectorXOutError)]; every .unwrap() becomes ?. Returns are typed through let iter: Box<dyn RecordBatchIterator> so the unsizing coercion is explicit.
  • connectorx/src/errors.rs — adds From<…> impls on ConnectorXOutError for each *ArrowStream* transport error so ? can wrap them.
  • connectorx-python/src/arrow.rs — propagates the new Result with ? (the surrounding function already returns ConnectorXPythonError, which has a From<ConnectorXOutError>).
  • connectorx-cpp/src/lib.rs — keeps current behavior with an explicit .unwrap() (FFI boundary; same as the surrounding code).
  • connectorx-python/connectorx/tests/test_postgres.py — adds a TLS arrow_stream regression test and an explicit “bad host raises” test that exercises the error path.

Test plan

  • cargo check + cargo build --all-features on the workspace
  • cargo test --lib (no unit tests defined for this module)
  • CI integration tests against a TLS-enabled Postgres (POSTGRES_URL_TLS) — test_postgres_tls_arrow_stream and test_postgres_tls_arrow_stream_bad_host_raises
  • Manual repro of the issue against AWS RDS with sslmode=require to confirm the panic is replaced with a RuntimeError carrying the underlying r2d2 message

…n setup

new_record_batch_iter used .unwrap() throughout, which masked the
underlying r2d2/Postgres connection error as `Error(None)` and panicked
the thread when TLS handshake or other setup failed. Mirror the error
handling in get_arrow by adding #[throws(ConnectorXOutError)] and
propagating with ?.

Adds From impls on ConnectorXOutError for each *ArrowStream* transport
error variant so ? can wrap them. Updates the Python and C++ callers to
handle the new Result return type.

Fixes sfu-db#909
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.

arrow_stream + sslmode=require panics with PostgresPoolError(Error(None)) — works in arrow mode

1 participant