Skip to content
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.

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

SNOW-1903844: Closing an already closed SnowflakeDatabaseMetaDataResultSet triggers logs in DriverManager.getLogWriter() #2057

Open
fabiencelier opened this issue Jan 30, 2025 · 2 comments
Assignees
Labels
feature status-information_needed Additional information is required from the reporter status-triage_done Initial triage done, will be further handled by the driver team

Comments

@fabiencelier
Copy link
Contributor

fabiencelier commented Jan 30, 2025

What is the current behavior?

This is the close() method of SnowflakeDatabaseMetaDataResultSet:

  public void close() throws SQLException {
    // no exception
    try {
      getStatement().close(); // should close both result set and statement.
    } catch (SQLException ex) {
      logger.debug("Failed to close", ex);
    }
  }

If the result set is already closed, getStatement() will create a SnowflakeSQLException catched in the catch blog.
But creating such SqlException will call the DriverManager.getLogWriter() and log the whole StackTrace if the log writer is set.

Closing an already closed SnowflakeDatabaseMetaDataResultSet can easily happen as calling next() on the last row will close it, so a code like this will call close() twice:

try (ResultSet columns = connection.getMetaData().getColumns( ... ) ) {
  while (columns.next()) { // Call close on the last row
     // Do something
  }
} // call close to close the ResulSet resource

This is not a big deal but it is really polluting my logs a lot for no reason when setting up a log writer in the DriverManager.

What is the desired behavior?

Do not create a SqlException if the ResultSet is already closed, just do nothing

How would this improve snowflake-jdbc?

It would make the driver easier to debug as we have less noise when enabling the DriverManager log writer.

I can make a PR to improve it if needed, I think you can simply do

  @Override
  public void close() throws SQLException {
    // no exception
    if (!isClosed()) { // Nothing to do if already closed
      try {
        getStatement().close(); // should close both result set and statement.
      } catch (SQLException ex) {
        logger.debug("Failed to close", ex);
      }
    }
  }

References, Other Background

this is a stack trace which I se for every metadata query I'm doing:

net.snowflake.client.jdbc.SnowflakeSQLException: Result set has been closed.
 	at net.snowflake.client.jdbc.SnowflakeBaseResultSet.raiseSQLExceptionIfResultSetIsClosed(SnowflakeBaseResultSet.java:135)
 	at net.snowflake.client.jdbc.SnowflakeBaseResultSet.getStatement(SnowflakeBaseResultSet.java:830)
 	at net.snowflake.client.jdbc.SnowflakeDatabaseMetaDataResultSet.close(SnowflakeDatabaseMetaDataResultSet.java:159)
        [... my code]
@github-actions github-actions bot changed the title Closing an already closed SnowflakeDatabaseMetaDataResultSet triggers logs in DriverManager.getLogWriter() SNOW-1903844: Closing an already closed SnowflakeDatabaseMetaDataResultSet triggers logs in DriverManager.getLogWriter() Jan 30, 2025
@sfc-gh-sghosh sfc-gh-sghosh self-assigned this Feb 3, 2025
@sfc-gh-sghosh
Copy link
Contributor

Thank you @fabiencelier for raising this issue and offer the solution as well.
We will work on it and update.

Regards,
Sujan

@sfc-gh-sghosh sfc-gh-sghosh added the status-triage_done Initial triage done, will be further handled by the driver team label Feb 3, 2025
@sfc-gh-dprzybysz
Copy link
Collaborator

Hi @fabiencelier
I checked your PR, please take a look at my comment.
I think the PR is not fixing the root cause of the problem

@sfc-gh-dprzybysz sfc-gh-dprzybysz added the status-information_needed Additional information is required from the reporter label Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature status-information_needed Additional information is required from the reporter status-triage_done Initial triage done, will be further handled by the driver team
Projects
None yet
Development

No branches or pull requests

4 participants