From e721efeb850ac6ceddb45942e1a51d7d34056b0c Mon Sep 17 00:00:00 2001 From: Vedran Pavic Date: Tue, 10 Nov 2020 21:42:53 +0100 Subject: [PATCH] Optimize insert attribute statement in JdbcIndexedSessionRepository At present, the SQL statement used to insert a session attribute record contains a nested select statement that verifies the existence of parent record in the session table. Such approach can be susceptible to deadlocks on certain RDMBSs. This commit optimizes the SQL statement used to insert session attribute so that it doesn't perform a nested select statement. Closes: #1550 --- ...dbcIndexedSessionRepositoryCustomizer.java | 4 +- .../jdbc/JdbcIndexedSessionRepository.java | 96 ++++++++++++------- ...dbcIndexedSessionRepositoryCustomizer.java | 4 +- ...dbcIndexedSessionRepositoryCustomizer.java | 5 +- ...dbcIndexedSessionRepositoryCustomizer.java | 4 +- ...dbcIndexedSessionRepositoryCustomizer.java | 4 +- .../JdbcIndexedSessionRepositoryTests.java | 18 ++-- 7 files changed, 78 insertions(+), 57 deletions(-) diff --git a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/Db2JdbcIndexedSessionRepositoryCustomizer.java b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/Db2JdbcIndexedSessionRepositoryCustomizer.java index 1f5e37fb..d2df717a 100644 --- a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/Db2JdbcIndexedSessionRepositoryCustomizer.java +++ b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/Db2JdbcIndexedSessionRepositoryCustomizer.java @@ -32,9 +32,7 @@ public class Db2JdbcIndexedSessionRepositoryCustomizer private static final String CREATE_SESSION_ATTRIBUTE_QUERY = "" + "MERGE INTO %TABLE_NAME%_ATTRIBUTES SA " + "USING ( " - + " SELECT PRIMARY_ID, ?, ? " - + " FROM %TABLE_NAME% " - + " WHERE SESSION_ID = ? " + + " VALUES (?, ?, ?) " + ") A (SESSION_PRIMARY_ID, ATTRIBUTE_NAME, ATTRIBUTE_BYTES) " + "ON (SA.SESSION_PRIMARY_ID = A.SESSION_PRIMARY_ID and SA.ATTRIBUTE_NAME = A.ATTRIBUTE_NAME) " + "WHEN MATCHED THEN " diff --git a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java index 3610c3c8..8f05c6aa 100644 --- a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java +++ b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java @@ -40,6 +40,8 @@ import org.springframework.core.convert.support.GenericConversionService; import org.springframework.core.serializer.support.DeserializingConverter; import org.springframework.core.serializer.support.SerializingConverter; import org.springframework.dao.DataAccessException; +import org.springframework.dao.DataIntegrityViolationException; +import org.springframework.dao.DuplicateKeyException; import org.springframework.jdbc.core.BatchPreparedStatementSetter; import org.springframework.jdbc.core.JdbcOperations; import org.springframework.jdbc.core.ResultSetExtractor; @@ -139,55 +141,64 @@ public class JdbcIndexedSessionRepository private static final String SPRING_SECURITY_CONTEXT = "SPRING_SECURITY_CONTEXT"; // @formatter:off - private static final String CREATE_SESSION_QUERY = "INSERT INTO %TABLE_NAME%(PRIMARY_ID, SESSION_ID, CREATION_TIME, LAST_ACCESS_TIME, MAX_INACTIVE_INTERVAL, EXPIRY_TIME, PRINCIPAL_NAME) " + private static final String CREATE_SESSION_QUERY = "" + + "INSERT INTO %TABLE_NAME% (PRIMARY_ID, SESSION_ID, CREATION_TIME, LAST_ACCESS_TIME, MAX_INACTIVE_INTERVAL, EXPIRY_TIME, PRINCIPAL_NAME) " + "VALUES (?, ?, ?, ?, ?, ?, ?)"; // @formatter:on // @formatter:off - private static final String CREATE_SESSION_ATTRIBUTE_QUERY = "INSERT INTO %TABLE_NAME%_ATTRIBUTES(SESSION_PRIMARY_ID, ATTRIBUTE_NAME, ATTRIBUTE_BYTES) " - + "SELECT PRIMARY_ID, ?, ? " - + "FROM %TABLE_NAME% " - + "WHERE SESSION_ID = ?"; + private static final String CREATE_SESSION_ATTRIBUTE_QUERY = "" + + "INSERT INTO %TABLE_NAME%_ATTRIBUTES (SESSION_PRIMARY_ID, ATTRIBUTE_NAME, ATTRIBUTE_BYTES) " + + "VALUES (?, ?, ?)"; // @formatter:on // @formatter:off - private static final String GET_SESSION_QUERY = "SELECT S.PRIMARY_ID, S.SESSION_ID, S.CREATION_TIME, S.LAST_ACCESS_TIME, S.MAX_INACTIVE_INTERVAL, SA.ATTRIBUTE_NAME, SA.ATTRIBUTE_BYTES " + private static final String GET_SESSION_QUERY = "" + + "SELECT S.PRIMARY_ID, S.SESSION_ID, S.CREATION_TIME, S.LAST_ACCESS_TIME, S.MAX_INACTIVE_INTERVAL, SA.ATTRIBUTE_NAME, SA.ATTRIBUTE_BYTES " + "FROM %TABLE_NAME% S " - + "LEFT OUTER JOIN %TABLE_NAME%_ATTRIBUTES SA ON S.PRIMARY_ID = SA.SESSION_PRIMARY_ID " + + "LEFT JOIN %TABLE_NAME%_ATTRIBUTES SA ON S.PRIMARY_ID = SA.SESSION_PRIMARY_ID " + "WHERE S.SESSION_ID = ?"; // @formatter:on // @formatter:off - private static final String UPDATE_SESSION_QUERY = "UPDATE %TABLE_NAME% SET SESSION_ID = ?, LAST_ACCESS_TIME = ?, MAX_INACTIVE_INTERVAL = ?, EXPIRY_TIME = ?, PRINCIPAL_NAME = ? " + private static final String UPDATE_SESSION_QUERY = "" + + "UPDATE %TABLE_NAME% " + + "SET SESSION_ID = ?, LAST_ACCESS_TIME = ?, MAX_INACTIVE_INTERVAL = ?, EXPIRY_TIME = ?, PRINCIPAL_NAME = ? " + "WHERE PRIMARY_ID = ?"; // @formatter:on // @formatter:off - private static final String UPDATE_SESSION_ATTRIBUTE_QUERY = "UPDATE %TABLE_NAME%_ATTRIBUTES SET ATTRIBUTE_BYTES = ? " + private static final String UPDATE_SESSION_ATTRIBUTE_QUERY = "" + + "UPDATE %TABLE_NAME%_ATTRIBUTES " + + "SET ATTRIBUTE_BYTES = ? " + "WHERE SESSION_PRIMARY_ID = ? " + "AND ATTRIBUTE_NAME = ?"; // @formatter:on // @formatter:off - private static final String DELETE_SESSION_ATTRIBUTE_QUERY = "DELETE FROM %TABLE_NAME%_ATTRIBUTES " + private static final String DELETE_SESSION_ATTRIBUTE_QUERY = "" + + "DELETE FROM %TABLE_NAME%_ATTRIBUTES " + "WHERE SESSION_PRIMARY_ID = ? " + "AND ATTRIBUTE_NAME = ?"; // @formatter:on // @formatter:off - private static final String DELETE_SESSION_QUERY = "DELETE FROM %TABLE_NAME% " + private static final String DELETE_SESSION_QUERY = "" + + "DELETE FROM %TABLE_NAME% " + "WHERE SESSION_ID = ?"; // @formatter:on // @formatter:off - private static final String LIST_SESSIONS_BY_PRINCIPAL_NAME_QUERY = "SELECT S.PRIMARY_ID, S.SESSION_ID, S.CREATION_TIME, S.LAST_ACCESS_TIME, S.MAX_INACTIVE_INTERVAL, SA.ATTRIBUTE_NAME, SA.ATTRIBUTE_BYTES " + private static final String LIST_SESSIONS_BY_PRINCIPAL_NAME_QUERY = "" + + "SELECT S.PRIMARY_ID, S.SESSION_ID, S.CREATION_TIME, S.LAST_ACCESS_TIME, S.MAX_INACTIVE_INTERVAL, SA.ATTRIBUTE_NAME, SA.ATTRIBUTE_BYTES " + "FROM %TABLE_NAME% S " - + "LEFT OUTER JOIN %TABLE_NAME%_ATTRIBUTES SA ON S.PRIMARY_ID = SA.SESSION_PRIMARY_ID " + + "LEFT JOIN %TABLE_NAME%_ATTRIBUTES SA ON S.PRIMARY_ID = SA.SESSION_PRIMARY_ID " + "WHERE S.PRINCIPAL_NAME = ?"; // @formatter:on // @formatter:off - private static final String DELETE_SESSIONS_BY_EXPIRY_TIME_QUERY = "DELETE FROM %TABLE_NAME% " + private static final String DELETE_SESSIONS_BY_EXPIRY_TIME_QUERY = "" + + "DELETE FROM %TABLE_NAME% " + "WHERE EXPIRY_TIME < ?"; // @formatter:on @@ -463,30 +474,49 @@ public class JdbcIndexedSessionRepository Assert.notEmpty(attributeNames, "attributeNames must not be null or empty"); try (LobCreator lobCreator = this.lobHandler.getLobCreator()) { if (attributeNames.size() > 1) { - this.jdbcOperations.batchUpdate(this.createSessionAttributeQuery, new BatchPreparedStatementSetter() { + try { + this.jdbcOperations.batchUpdate(this.createSessionAttributeQuery, + new BatchPreparedStatementSetter() { - @Override - public void setValues(PreparedStatement ps, int i) throws SQLException { - String attributeName = attributeNames.get(i); - ps.setString(1, attributeName); - lobCreator.setBlobAsBytes(ps, 2, serialize(session.getAttribute(attributeName))); - ps.setString(3, session.getId()); - } + @Override + public void setValues(PreparedStatement ps, int i) throws SQLException { + String attributeName = attributeNames.get(i); + ps.setString(1, session.primaryKey); + ps.setString(2, attributeName); + lobCreator.setBlobAsBytes(ps, 3, serialize(session.getAttribute(attributeName))); + } - @Override - public int getBatchSize() { - return attributeNames.size(); - } + @Override + public int getBatchSize() { + return attributeNames.size(); + } - }); + }); + } + catch (DuplicateKeyException ex) { + throw ex; + } + catch (DataIntegrityViolationException ex) { + // parent record not found - we are ignoring this error because we + // assume that a concurrent request has removed the session + } } else { - this.jdbcOperations.update(this.createSessionAttributeQuery, (ps) -> { - String attributeName = attributeNames.get(0); - ps.setString(1, attributeName); - lobCreator.setBlobAsBytes(ps, 2, serialize(session.getAttribute(attributeName))); - ps.setString(3, session.getId()); - }); + try { + this.jdbcOperations.update(this.createSessionAttributeQuery, (ps) -> { + String attributeName = attributeNames.get(0); + ps.setString(1, session.primaryKey); + ps.setString(2, attributeName); + lobCreator.setBlobAsBytes(ps, 3, serialize(session.getAttribute(attributeName))); + }); + } + catch (DuplicateKeyException ex) { + throw ex; + } + catch (DataIntegrityViolationException ex) { + // parent record not found - we are ignoring this error because we + // assume that a concurrent request has removed the session + } } } } diff --git a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/MySqlJdbcIndexedSessionRepositoryCustomizer.java b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/MySqlJdbcIndexedSessionRepositoryCustomizer.java index 0910879d..0745c332 100644 --- a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/MySqlJdbcIndexedSessionRepositoryCustomizer.java +++ b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/MySqlJdbcIndexedSessionRepositoryCustomizer.java @@ -31,9 +31,7 @@ public class MySqlJdbcIndexedSessionRepositoryCustomizer // @formatter:off private static final String CREATE_SESSION_ATTRIBUTE_QUERY = "" + "INSERT INTO %TABLE_NAME%_ATTRIBUTES (SESSION_PRIMARY_ID, ATTRIBUTE_NAME, ATTRIBUTE_BYTES) " - + " SELECT PRIMARY_ID, ?, ? " - + " FROM %TABLE_NAME% " - + " WHERE SESSION_ID = ? " + + "VALUES (?, ?, ?) " + "ON DUPLICATE KEY UPDATE ATTRIBUTE_BYTES = VALUES(ATTRIBUTE_BYTES)"; // @formatter:on diff --git a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/OracleJdbcIndexedSessionRepositoryCustomizer.java b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/OracleJdbcIndexedSessionRepositoryCustomizer.java index 3f14570a..2ea3fd66 100644 --- a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/OracleJdbcIndexedSessionRepositoryCustomizer.java +++ b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/OracleJdbcIndexedSessionRepositoryCustomizer.java @@ -32,9 +32,8 @@ public class OracleJdbcIndexedSessionRepositoryCustomizer private static final String CREATE_SESSION_ATTRIBUTE_QUERY = "" + "MERGE INTO %TABLE_NAME%_ATTRIBUTES SA " + "USING ( " - + " SELECT PRIMARY_ID AS SESSION_PRIMARY_ID, ? AS ATTRIBUTE_NAME, ? AS ATTRIBUTE_BYTES " - + " FROM %TABLE_NAME% " - + " WHERE SESSION_ID = ? " + + " SELECT ? AS SESSION_PRIMARY_ID, ? AS ATTRIBUTE_NAME, ? AS ATTRIBUTE_BYTES " + + " FROM DUAL " + ") A " + "ON (SA.SESSION_PRIMARY_ID = A.SESSION_PRIMARY_ID and SA.ATTRIBUTE_NAME = A.ATTRIBUTE_NAME) " + "WHEN MATCHED THEN " diff --git a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/PostgreSqlJdbcIndexedSessionRepositoryCustomizer.java b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/PostgreSqlJdbcIndexedSessionRepositoryCustomizer.java index ce11e691..9b805e7b 100644 --- a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/PostgreSqlJdbcIndexedSessionRepositoryCustomizer.java +++ b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/PostgreSqlJdbcIndexedSessionRepositoryCustomizer.java @@ -31,9 +31,7 @@ public class PostgreSqlJdbcIndexedSessionRepositoryCustomizer // @formatter:off private static final String CREATE_SESSION_ATTRIBUTE_QUERY = "" + "INSERT INTO %TABLE_NAME%_ATTRIBUTES (SESSION_PRIMARY_ID, ATTRIBUTE_NAME, ATTRIBUTE_BYTES) " - + " SELECT PRIMARY_ID, ?, ? " - + " FROM %TABLE_NAME% " - + " WHERE SESSION_ID = ? " + + "VALUES (?, ?, ?) " + "ON CONFLICT (SESSION_PRIMARY_ID, ATTRIBUTE_NAME) " + "DO UPDATE SET ATTRIBUTE_BYTES = EXCLUDED.ATTRIBUTE_BYTES"; // @formatter:on diff --git a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/SqlServerJdbcIndexedSessionRepositoryCustomizer.java b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/SqlServerJdbcIndexedSessionRepositoryCustomizer.java index 9fa09dac..26f34d39 100644 --- a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/SqlServerJdbcIndexedSessionRepositoryCustomizer.java +++ b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/SqlServerJdbcIndexedSessionRepositoryCustomizer.java @@ -32,9 +32,7 @@ public class SqlServerJdbcIndexedSessionRepositoryCustomizer private static final String CREATE_SESSION_ATTRIBUTE_QUERY = "" + "MERGE INTO %TABLE_NAME%_ATTRIBUTES SA " + "USING ( " - + " SELECT PRIMARY_ID, ?, ? " - + " FROM %TABLE_NAME% " - + " WHERE SESSION_ID = ? " + + " VALUES (?, ?, ?) " + ") A (SESSION_PRIMARY_ID, ATTRIBUTE_NAME, ATTRIBUTE_BYTES) " + "ON (SA.SESSION_PRIMARY_ID = A.SESSION_PRIMARY_ID and SA.ATTRIBUTE_NAME = A.ATTRIBUTE_NAME) " + "WHEN MATCHED THEN " diff --git a/spring-session-jdbc/src/test/java/org/springframework/session/jdbc/JdbcIndexedSessionRepositoryTests.java b/spring-session-jdbc/src/test/java/org/springframework/session/jdbc/JdbcIndexedSessionRepositoryTests.java index d1e81c76..63e2bb89 100644 --- a/spring-session-jdbc/src/test/java/org/springframework/session/jdbc/JdbcIndexedSessionRepositoryTests.java +++ b/spring-session-jdbc/src/test/java/org/springframework/session/jdbc/JdbcIndexedSessionRepositoryTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2019 the original author or authors. + * Copyright 2014-2020 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -290,9 +290,9 @@ class JdbcIndexedSessionRepositoryTests { this.repository.save(session); assertThat(session.isNew()).isFalse(); - verify(this.jdbcOperations, times(1)).update(startsWith("INSERT INTO SPRING_SESSION("), + verify(this.jdbcOperations, times(1)).update(startsWith("INSERT INTO SPRING_SESSION ("), isA(PreparedStatementSetter.class)); - verify(this.jdbcOperations, times(1)).update(startsWith("INSERT INTO SPRING_SESSION_ATTRIBUTES("), + verify(this.jdbcOperations, times(1)).update(startsWith("INSERT INTO SPRING_SESSION_ATTRIBUTES ("), isA(PreparedStatementSetter.class)); verifyNoMoreInteractions(this.jdbcOperations); } @@ -306,9 +306,9 @@ class JdbcIndexedSessionRepositoryTests { this.repository.save(session); assertThat(session.isNew()).isFalse(); - verify(this.jdbcOperations, times(1)).update(startsWith("INSERT INTO SPRING_SESSION("), + verify(this.jdbcOperations, times(1)).update(startsWith("INSERT INTO SPRING_SESSION ("), isA(PreparedStatementSetter.class)); - verify(this.jdbcOperations, times(1)).batchUpdate(startsWith("INSERT INTO SPRING_SESSION_ATTRIBUTES("), + verify(this.jdbcOperations, times(1)).batchUpdate(startsWith("INSERT INTO SPRING_SESSION_ATTRIBUTES ("), isA(BatchPreparedStatementSetter.class)); verifyNoMoreInteractions(this.jdbcOperations); } @@ -321,7 +321,7 @@ class JdbcIndexedSessionRepositoryTests { this.repository.save(session); assertThat(session.isNew()).isFalse(); - verify(this.jdbcOperations, times(1)).update(startsWith("INSERT INTO SPRING_SESSION_ATTRIBUTES("), + verify(this.jdbcOperations, times(1)).update(startsWith("INSERT INTO SPRING_SESSION_ATTRIBUTES ("), isA(PreparedStatementSetter.class)); verifyNoMoreInteractions(this.jdbcOperations); } @@ -335,7 +335,7 @@ class JdbcIndexedSessionRepositoryTests { this.repository.save(session); assertThat(session.isNew()).isFalse(); - verify(this.jdbcOperations, times(1)).batchUpdate(startsWith("INSERT INTO SPRING_SESSION_ATTRIBUTES("), + verify(this.jdbcOperations, times(1)).batchUpdate(startsWith("INSERT INTO SPRING_SESSION_ATTRIBUTES ("), isA(BatchPreparedStatementSetter.class)); verifyNoMoreInteractions(this.jdbcOperations); } @@ -424,7 +424,7 @@ class JdbcIndexedSessionRepositoryTests { this.repository.save(session); assertThat(session.isNew()).isFalse(); - verify(this.jdbcOperations).update(startsWith("INSERT INTO SPRING_SESSION_ATTRIBUTES("), + verify(this.jdbcOperations).update(startsWith("INSERT INTO SPRING_SESSION_ATTRIBUTES ("), isA(PreparedStatementSetter.class)); verifyNoMoreInteractions(this.jdbcOperations); } @@ -679,7 +679,7 @@ class JdbcIndexedSessionRepositoryTests { JdbcSession session = this.repository.new JdbcSession(new MapSession(), "primaryKey", false); String attrName = "someAttribute"; session.setAttribute(attrName, "someValue"); - verify(this.jdbcOperations).update(startsWith("INSERT INTO SPRING_SESSION_ATTRIBUTES("), + verify(this.jdbcOperations).update(startsWith("INSERT INTO SPRING_SESSION_ATTRIBUTES ("), isA(PreparedStatementSetter.class)); verifyNoMoreInteractions(this.jdbcOperations); }