Skip to content

[SPARK-57780][SQL] Do not classify Postgres permission-denied as a syntax error #56899

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.

Sign up for GitHub

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

Open
vladanvasi-db wants to merge 1 commit into
base: master
Choose a base branch
from
+46 −2
Open

[SPARK-57780][SQL] Do not classify Postgres permission-denied as a syntax error #56899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,22 @@ private case class PostgresDialect()
}

// See https://www.postgresql.org/docs/current/errcodes-appendix.html
// Class 42 is "Syntax Error or Access Rule Violation", so it bundles genuine syntax errors
// together with authorization failures. 42501 is the insufficient_privilege state (e.g.

@urosstan-db urosstan-db Jun 30, 2026

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.

Too verbose, can we make this more concise, e.g.

// Class 42 mixes syntax and auth errors (https://www.postgresql.org/docs/current/errcodes-appendix.html).
// Filter out 42501 and check message keywords because some Postgres-compatible engines 
// use generic 42000 for auth failures. Never wrap these as JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR.

// "permission denied for table ..."), an access rule violation, not a syntax error. Additionally
// guard by message: some Postgres-compatible engines surface permission errors under the generic
// 42000 state, and access-control failures ("permission denied", "access denied", "unauthorized")
// must never be wrapped as JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR.
override def isSyntaxErrorBestEffort(exception: SQLException): Boolean = {
Option(exception.getSQLState).exists(_.startsWith("42"))
lazy val isAccessControlError = Option(exception.getMessage)

@urosstan-db urosstan-db Jun 30, 2026

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.

When we get permission error, we should report table not found as well. Can you take a look on method isObjectNotFoundException and check whether we need to update it?

.map(_.toLowerCase(Locale.ROOT))
.exists { message =>
message.contains("permission denied") ||
message.contains("access denied") ||
message.contains("unauthorized")
}
Option(exception.getSQLState).exists(s => s.startsWith("42") && s != "42501") &&

@MaxGekk MaxGekk Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The base contract (JdbcDialect Scaladoc) is "if this method returns true, the exception is guaranteed to be a syntax error." This denylist (class 42 minus 42501 minus access-control messages) still returns true for non-syntax class-42 codes — e.g. 42P01 (undefined_table), 42703 (undefined_column) — so the contract is only partially restored.

It's not purely cosmetic: in resolveTable the preceding isObjectNotFoundException (Postgres matches 42P01/3F000/42704) masks 42P01, but the query-execution path (JDBCRDD ~L373) has no not-found pre-check, so there a 42P01/42703 is still wrapped as JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR.DURING_QUERY_EXECUTION.

For a genuine "true ⟹ syntax" guarantee, consider allowlisting the real syntax states (e.g. 42601, 42P02) rather than denylisting from class 42. Non-blocking — this extends @urosstan-db's thread about class-42 codes beyond 42501.

!isAccessControlError
}

// SHOW INDEX syntax
Expand Down
32 changes: 31 additions & 1 deletion sql/core/src/test/scala/org/apache/spark/sql/jdbc/PostgresDialectSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

package org.apache.spark.sql.jdbc

import java.sql.Connection
import java.sql.{Connection, SQLException}

import org.mockito.Mockito._
import org.scalatestplus.mockito.MockitoSugar
Expand Down Expand Up @@ -78,4 +78,34 @@ class PostgresDialectSuite extends SparkFunSuite with MockitoSugar {
dialect.beforeFetch(conn, createJDBCOptions(Map.empty))
verify(conn).setAutoCommit(false)
}

test("isSyntaxErrorBestEffort: genuine syntax error (42601) is classified as a syntax error") {

@urosstan-db urosstan-db Jun 30, 2026

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.

Since we have docker instance here, can we create new user and do integration testing as well

assert(dialect.isSyntaxErrorBestEffort(
new SQLException("ERROR: syntax error at or near \"FROM\"", "42601")))
}

// Cases that must NOT be classified as syntax errors because they are access-control failures,
// regardless of whether the engine reports the precise 42501 or the generic 42000 state.
private case class AccessControlErrorCase(name: String, sqlState: String, message: String) {
override def toString: String = name
}

gridTest("isSyntaxErrorBestEffort: access-control failures are not syntax errors")(Seq(
AccessControlErrorCase(
"42501 insufficient_privilege", "42501", "ERROR: permission denied for table foo"),
AccessControlErrorCase(
"42000 permission denied", "42000", "ERROR: permission denied for table pg_statistic"),
AccessControlErrorCase("42000 access denied", "42000", "ERROR: access denied for relation foo"),
AccessControlErrorCase("42000 unauthorized", "42000", "ERROR: unauthorized")
)) { c =>
assert(!dialect.isSyntaxErrorBestEffort(new SQLException(c.message, c.sqlState)))
}

test("isSyntaxErrorBestEffort: non-class-42 SQLState is not a syntax error") {
assert(!dialect.isSyntaxErrorBestEffort(new SQLException("ERROR: some failure", "28000")))
}

test("isSyntaxErrorBestEffort: null SQLState is not a syntax error") {
assert(!dialect.isSyntaxErrorBestEffort(new SQLException("ERROR: connection lost")))
}
}