[SPARK-59336][SQL] Fix syntax error classifier in Postgres connector by urosstan-db · Pull Request #58621 · apache/spark

urosstan-db · 2026-09-08T13:12:00Z

What changes were proposed in this pull request?

Don't classify Postgres 42501 error as syntax error, since that error is permission issue.

https://www.postgresql.org/docs/17/errcodes-appendix.html

Why are the changes needed?

To properly classify syntax errors in Postgres connector.

Does this PR introduce any user-facing change?

Yes. For PostgreSQL JDBC queries, Spark now wraps only SQLSTATEs 42000 and 42601 as syntax errors. Other class-42 errors, such as 42501 (insufficient privilege) and 42P01 (undefined table), are propagated as their original PostgreSQL JDBC errors.

How was this patch tested?

New integration test where we assert no permission error is not classified as SyntaxError (that is behavioural change)
New unit tests
Existing positive test for syntax error classification here: https://github.com/urosstan-db/spark/blob/6697d711944e4c6cbcf038ce80df8d2272def3a3/connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/SharedJDBCIntegrationSuite.scala#L57-L74

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Codex CLI

…ax errors

urosstan-db · 2026-09-08T14:27:20Z

    verify(conn).setAutoCommit(false)
  }

+  test("SPARK-59336: classify only syntax error SQLSTATEs as syntax errors") {


There is positive test already here:
https://github.com/urosstan-db/spark/blob/6697d711944e4c6cbcf038ce80df8d2272def3a3/connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/SharedJDBCIntegrationSuite.scala#L57-L74

uros-b

cc @vladanvasi-db regarding #56899

urosstan-db · 2026-09-08T15:10:44Z

cc @vladanvasi-db regarding #56899

Discussed offline and decided to go with current approach in this PR.

cloud-fan

Review summary

The patch correctly stops treating every PostgreSQL class-42 error as syntax and adds useful unit and integration coverage for the permission path. One non-blocking issue remains: 42000 is a combined syntax-or-access-rule condition, so accepting it unconditionally can still turn an authorization failure into Spark's syntax-error wrapper. Restricting the positive match to 42601 and reversing the new 42000 assertion would align the implementation with the public classifier contract.

Findings

1 total: 0 P0, 0 P1, 1 P2, 0 P3.

Non-blocking (P2)

Do not treat PostgreSQL SQLSTATE 42000 as syntax-only — sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala:264 — see inline.

PR metadata suggestions

Narrow the user-facing-change statement from generic '42x errors' to the specific PostgreSQL SQLSTATE behavior, and note that only unambiguous syntax conditions are classified as syntax errors.

cloud-fan · 2026-09-09T04:57:04Z

  override def isSyntaxErrorBestEffort(exception: SQLException): Boolean = {
-    Option(exception.getSQLState).exists(_.startsWith("42"))
+    exception.getSQLState match {
+      case "42000" | "42601" => true


Non-blocking (P2): PostgreSQL defines 42000 as the combined syntax_error_or_access_rule_violation condition, while 42601 is the syntax-specific condition. Since isSyntaxErrorBestEffort guarantees that every true result is a syntax error and JDBCRDD uses it to replace the original exception with JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR, an access-rule failure reported as 42000 is still mislabeled here. Could we match only 42601 and change the unit coverage so an access-rule-flavored 42000 is asserted false?

Thanks a lot for comment, I was thinking whether we would need to exclude 42000 and be more strict, or we want to be a little bit broader. And decided to go with a little bit broader catch, since most 42x common errors have subtype and it is hard to test every syntax issue in integration tests, while there is no big issue to classify some new uknown 42000 error as syntax error. We can iterate by the time, for now, I wanted to exclude 42x errors that are clearly false positives.

uros-b · 2026-09-09T12:12:23Z

+          .load()
+      }
+      assertResult("42501")(postgresError.getSQLState)
+      assertResult("ERROR: permission denied for table bar")(postgresError.getMessage)


The exact message match is a bit brittle in the Docker test.

assertResult("ERROR: permission denied for table bar")(postgresError.getMessage)

PSQLException.getMessage often appends Position: / Detail:, and pre-15 servers say relation instead of table. The suite already allows POSTGRES_DOCKER_IMAGE_NAME to vary. Assert SQLState == "42501" and getMessage.contains("permission denied"), same style as the existing SPARK-47886 checks in this suite.

Also use DROP USER IF EXISTS in cleanup. If CREATE USER fails, the finally DROP USER can hide the original error; if a connection is still held, DROP USER can fail after a green assertion.

Good proposal, I will make error less restrictive and use IF EXISTS syntax (it is supported from pg server 8.2, released in 20 years ago), so even if someone provide older docker image version for testing, syntax would work.

I also, used some special spark's Util method (I forgot the exact name) to preserve stack trace if finally fails (#58621 (comment)).

uros-b · 2026-09-09T12:13:26Z


  // See https://www.postgresql.org/docs/current/errcodes-appendix.html
  override def isSyntaxErrorBestEffort(exception: SQLException): Boolean = {
-    Option(exception.getSQLState).exists(_.startsWith("42"))


Same startsWith("42") pattern is still in H2, Derby, DB2, Teradata, and Databricks dialects. This might be out of scope in this particular PR, but perhaps we can file follow-up tickets?

Totally agreed, I planned to make a folow-up with additional tests (e.g. querying non existing table should not be clasified as syntax error). And in that additional testing, we may catch issues on other dialects.
https://issues.apache.org/jira/browse/SPARK-59369

uros-b · 2026-09-09T12:14:24Z

Left a few more comments, thank you @urosstan-db and @cloud-fan!

uros-b · 2026-09-09T12:15:01Z

Also, please consider updating the user-facing section of the PR description. The impact is a bit broader than the description currently says.

The write-up reads as “don’t treat 42501 as syntax.” The allowlist also stops wrapping other class-42 states (42P01 undefined table, 42703 undefined column, grouping/type errors, …).

That is the intended contract, and it matters on a real path: JDBCRDD.resolveTable only consults isObjectNotFoundException when ident is defined. A normal spark.read.format("jdbc").option("dbtable", …) probe of a missing table currently becomes JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR because 42P01 starts with 42. After this PR it surfaces as a SQLException.

Please state that explicitly under “user-facing change,” including that the error type/class changes for those cases.

uros-b · 2026-09-09T20:28:00Z

+    val restrictedJdbcUrl = s"jdbc:postgresql://$dockerIp:$externalPort/postgres"
+
+    Using.resource(getConnection()) { conn =>
+      conn.prepareStatement(s"CREATE USER $restrictedUser PASSWORD '$restrictedPassword'")


CREATE USER sits outside tryWithSafeFinally. If that statement fails (role already exists from a previous keepContainer run), cleanup never runs.

Put both CREATE and DROP inside the same tryWithSafeFinally, and DROP USER IF EXISTS before CREATE.

Good comment, but if create user fails, then there is nothing to clean, while after this executeUpdate, we will for sure execute drop user. I would not add drop user before create, since I like to have clean lifecycle, and we use docker container for each new run, even if we decide to reuse docker containers in CI in the future (highly unlike), we should be fine with execution of this test

uros-b · 2026-09-09T20:28:41Z

+    assert(!dialect.isSyntaxErrorBestEffort(new SQLException("permission denied", "42501")))
+    assert(!dialect.isSyntaxErrorBestEffort(new SQLException("undefined table", "42P01")))
+    assert(!dialect.isSyntaxErrorBestEffort(new SQLException("error without SQLSTATE")))
+  }


Unit coverage is thin for the new contract. The suite checks 42501 and 42P01. Adding 42703 (undefined column) and 42883 (undefined function) would lock the other common class-42 false positives the PR description calls out. Cheap and they always run, unlike Docker tests.

I would not go that deep with unit testing for one-liner method, since it does not protect us more much more than current change, while it adds burden for future change of logic

urosstan-db commented Sep 8, 2026

View reviewed changes

Comment thread ...er-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/PostgresIntegrationSuite.scala Outdated