From e9fc3ed22e879c7a33bf5b77e4fa51e88815e612 Mon Sep 17 00:00:00 2001 From: rxu Date: Sun, 3 Nov 2019 18:44:39 +0700 Subject: [ticket/15294] Fix session_gc() selecting expired sessions for unique users Also remove limit of 10 as it does not allow to collect all the garbage. PHPBB3-15294 --- phpBB/phpbb/session.php | 56 ++++++++++++++++++++++++------------------------- 1 file changed, 28 insertions(+), 28 deletions(-) (limited to 'phpBB/phpbb/session.php') diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index 7c76c08b73..4552f3bd82 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -954,8 +954,6 @@ class session { global $db, $config, $phpbb_container, $phpbb_dispatcher; - $batch_size = 10; - if (!$this->time_now) { $this->time_now = time(); @@ -968,14 +966,21 @@ class session $db->sql_query($sql); // Get expired sessions, only most recent for each user - $sql = 'SELECT session_user_id, session_page, MAX(session_time) AS recent_time - FROM ' . SESSIONS_TABLE . ' - WHERE session_time < ' . ($this->time_now - $config['session_length']) . ' - GROUP BY session_user_id, session_page'; - $result = $db->sql_query_limit($sql, $batch_size); + // Inner SELECT gets most recent expired sessions for unique session_user_id + // Outer SELECT gets session_page for them + $sql = 'SELECT s1.session_page, s1.session_user_id, s1.session_time AS recent_time + FROM ' . SESSIONS_TABLE . ' AS s1 + INNER JOIN ( + SELECT session_user_id, MAX(session_time) AS recent_time + FROM ' . SESSIONS_TABLE . ' + WHERE session_time < ' . ($this->time_now - (int) $config['session_length']) . ' + GROUP BY session_user_id + ) AS s2 + ON s1.session_user_id = s2.session_user_id + AND s1.session_time = s2.recent_time'; + $result = $db->sql_query($sql); $del_user_id = array(); - $del_sessions = 0; while ($row = $db->sql_fetchrow($result)) { @@ -985,7 +990,6 @@ class session $db->sql_query($sql); $del_user_id[] = (int) $row['session_user_id']; - $del_sessions++; } $db->sql_freeresult($result); @@ -998,29 +1002,25 @@ class session $db->sql_query($sql); } - if ($del_sessions < $batch_size) - { - // Less than 10 users, update gc timer ... else we want gc - // called again to delete other sessions - $config->set('session_last_gc', $this->time_now, false); + // Update gc timer + $config->set('session_last_gc', $this->time_now, false); - if ($config['max_autologin_time']) - { - $sql = 'DELETE FROM ' . SESSIONS_KEYS_TABLE . ' - WHERE last_login < ' . (time() - (86400 * (int) $config['max_autologin_time'])); - $db->sql_query($sql); - } - - // only called from CRON; should be a safe workaround until the infrastructure gets going - /* @var $captcha_factory \phpbb\captcha\factory */ - $captcha_factory = $phpbb_container->get('captcha.factory'); - $captcha_factory->garbage_collect($config['captcha_plugin']); - - $sql = 'DELETE FROM ' . LOGIN_ATTEMPT_TABLE . ' - WHERE attempt_time < ' . (time() - (int) $config['ip_login_limit_time']); + if ($config['max_autologin_time']) + { + $sql = 'DELETE FROM ' . SESSIONS_KEYS_TABLE . ' + WHERE last_login < ' . (time() - (86400 * (int) $config['max_autologin_time'])); $db->sql_query($sql); } + // only called from CRON; should be a safe workaround until the infrastructure gets going + /* @var $captcha_factory \phpbb\captcha\factory */ + $captcha_factory = $phpbb_container->get('captcha.factory'); + $captcha_factory->garbage_collect($config['captcha_plugin']); + + $sql = 'DELETE FROM ' . LOGIN_ATTEMPT_TABLE . ' + WHERE attempt_time < ' . (time() - (int) $config['ip_login_limit_time']); + $db->sql_query($sql); + /** * Event to trigger extension on session_gc * -- cgit v1.2.1 From 46a68d37ee858db19b3cb9100ff7f4be9e5e994e Mon Sep 17 00:00:00 2001 From: rxu Date: Sun, 3 Nov 2019 23:14:52 +0700 Subject: [ticket/15294] Another approach, optimize updating & purging expired sessions PHPBB3-15294 --- phpBB/phpbb/session.php | 63 ++++++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 32 deletions(-) (limited to 'phpBB/phpbb/session.php') diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index 4552f3bd82..60551bfe99 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -959,48 +959,47 @@ class session $this->time_now = time(); } - // Firstly, delete guest sessions - $sql = 'DELETE FROM ' . SESSIONS_TABLE . ' - WHERE session_user_id = ' . ANONYMOUS . ' - AND session_time < ' . (int) ($this->time_now - $config['session_length']); - $db->sql_query($sql); - - // Get expired sessions, only most recent for each user - // Inner SELECT gets most recent expired sessions for unique session_user_id - // Outer SELECT gets session_page for them - $sql = 'SELECT s1.session_page, s1.session_user_id, s1.session_time AS recent_time + // Get expired sessions, only most recent for each registered user + // Inner SELECT gets most recent expired sessions data for unique session_user_id + // Outer SELECT gets also session_page for them + $sql_select = '( + SELECT s1.session_page, s1.session_user_id, s1.session_time AS recent_time FROM ' . SESSIONS_TABLE . ' AS s1 INNER JOIN ( SELECT session_user_id, MAX(session_time) AS recent_time FROM ' . SESSIONS_TABLE . ' WHERE session_time < ' . ($this->time_now - (int) $config['session_length']) . ' + AND session_user_id <> ' . ANONYMOUS . ' GROUP BY session_user_id ) AS s2 ON s1.session_user_id = s2.session_user_id - AND s1.session_time = s2.recent_time'; - $result = $db->sql_query($sql); - - $del_user_id = array(); - - while ($row = $db->sql_fetchrow($result)) - { - $sql = 'UPDATE ' . USERS_TABLE . ' - SET user_lastvisit = ' . (int) $row['recent_time'] . ", user_lastpage = '" . $db->sql_escape($row['session_page']) . "' - WHERE user_id = " . (int) $row['session_user_id']; - $db->sql_query($sql); - - $del_user_id[] = (int) $row['session_user_id']; + AND s1.session_time = s2.recent_time + ) AS s3'; + + // Update user session data from above selected result + switch ($db->get_sql_layer()) + { + case 'sqlite3': + case 'mysqli': + $sql = 'UPDATE ' . USERS_TABLE . " AS u, + $sql_select + SET u.user_lastvisit = s3.recent_time, u.user_lastpage = s3.session_page + WHERE u.user_id = s3.session_user_id"; + break; + + default: + $sql = 'UPDATE ' . USERS_TABLE . " + SET user_lastvisit = s3.recent_time, user_lastpage = s3.session_page + FROM $sql_select + WHERE user_id = s3.session_user_id"; + break; } - $db->sql_freeresult($result); + $db->sql_query($sql); - if (count($del_user_id)) - { - // Delete expired sessions - $sql = 'DELETE FROM ' . SESSIONS_TABLE . ' - WHERE ' . $db->sql_in_set('session_user_id', $del_user_id) . ' - AND session_time < ' . ($this->time_now - $config['session_length']); - $db->sql_query($sql); - } + // Delete all expired sessions + $sql = 'DELETE FROM ' . SESSIONS_TABLE . ' + WHERE session_time < ' . ($this->time_now - $config['session_length']); + $db->sql_query($sql); // Update gc timer $config->set('session_last_gc', $this->time_now, false); -- cgit v1.2.1 From a340c362bcac64e115ddbb138bb90a8afa7faeb9 Mon Sep 17 00:00:00 2001 From: rxu Date: Mon, 4 Nov 2019 13:36:58 +0700 Subject: [ticket/15294] Back to the all DBMS compliant splitted queries PHPBB3-15294 --- phpBB/phpbb/session.php | 44 +++++++++++++++++--------------------------- 1 file changed, 17 insertions(+), 27 deletions(-) (limited to 'phpBB/phpbb/session.php') diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index 60551bfe99..d8a0d4d708 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -959,11 +959,12 @@ class session $this->time_now = time(); } - // Get expired sessions, only most recent for each registered user - // Inner SELECT gets most recent expired sessions data for unique session_user_id - // Outer SELECT gets also session_page for them - $sql_select = '( - SELECT s1.session_page, s1.session_user_id, s1.session_time AS recent_time + /** + * Get expired sessions for registered users, only most recent for each user + * Inner SELECT gets most recent expired sessions for unique session_user_id + * Outer SELECT gets data for them + */ + $sql = 'SELECT s1.session_page, s1.session_user_id, s1.session_time AS recent_time FROM ' . SESSIONS_TABLE . ' AS s1 INNER JOIN ( SELECT session_user_id, MAX(session_time) AS recent_time @@ -973,32 +974,21 @@ class session GROUP BY session_user_id ) AS s2 ON s1.session_user_id = s2.session_user_id - AND s1.session_time = s2.recent_time - ) AS s3'; - - // Update user session data from above selected result - switch ($db->get_sql_layer()) - { - case 'sqlite3': - case 'mysqli': - $sql = 'UPDATE ' . USERS_TABLE . " AS u, - $sql_select - SET u.user_lastvisit = s3.recent_time, u.user_lastpage = s3.session_page - WHERE u.user_id = s3.session_user_id"; - break; - - default: - $sql = 'UPDATE ' . USERS_TABLE . " - SET user_lastvisit = s3.recent_time, user_lastpage = s3.session_page - FROM $sql_select - WHERE user_id = s3.session_user_id"; - break; + AND s1.session_time = s2.recent_time'; + $result = $db->sql_query($sql); + + while ($row = $db->sql_fetchrow($result)) + { + $sql = 'UPDATE ' . USERS_TABLE . ' + SET user_lastvisit = ' . (int) $row['recent_time'] . ", user_lastpage = '" . $db->sql_escape($row['session_page']) . "' + WHERE user_id = " . (int) $row['session_user_id']; + $db->sql_query($sql); } - $db->sql_query($sql); + $db->sql_freeresult($result); // Delete all expired sessions $sql = 'DELETE FROM ' . SESSIONS_TABLE . ' - WHERE session_time < ' . ($this->time_now - $config['session_length']); + WHERE session_time < ' . ($this->time_now - (int) $config['session_length']); $db->sql_query($sql); // Update gc timer -- cgit v1.2.1 From ba17f16dc87f7f584fa6a56e61ddd44a8aed7f2b Mon Sep 17 00:00:00 2001 From: rxu Date: Mon, 4 Nov 2019 21:48:20 +0700 Subject: [ticket/15294] Get rid of SQL query in a loop as far as possible PHPBB3-15294 --- phpBB/phpbb/session.php | 51 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 8 deletions(-) (limited to 'phpBB/phpbb/session.php') diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index d8a0d4d708..42ee509df4 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -964,7 +964,7 @@ class session * Inner SELECT gets most recent expired sessions for unique session_user_id * Outer SELECT gets data for them */ - $sql = 'SELECT s1.session_page, s1.session_user_id, s1.session_time AS recent_time + $sql_select = 'SELECT s1.session_page, s1.session_user_id, s1.session_time AS recent_time FROM ' . SESSIONS_TABLE . ' AS s1 INNER JOIN ( SELECT session_user_id, MAX(session_time) AS recent_time @@ -975,16 +975,51 @@ class session ) AS s2 ON s1.session_user_id = s2.session_user_id AND s1.session_time = s2.recent_time'; - $result = $db->sql_query($sql); - while ($row = $db->sql_fetchrow($result)) + switch ($db->get_sql_layer()) { - $sql = 'UPDATE ' . USERS_TABLE . ' - SET user_lastvisit = ' . (int) $row['recent_time'] . ", user_lastpage = '" . $db->sql_escape($row['session_page']) . "' - WHERE user_id = " . (int) $row['session_user_id']; - $db->sql_query($sql); + case 'sqlite3': + case 'oracle': + if ($db->get_sql_layer() === 'sqlite3' && phpbb_version_compare($db->sql_server_info(true), '3.8.3', '>=')) + { + // For SQLite versions 3.8.3+ which support Common Table Expressions (CTE) + $sql = "WITH s3 (session_page, session_user_id, session_time) AS ($sql_select) + UPDATE " . USERS_TABLE . ' + SET (user_lastpage, user_lastvisit) = (SELECT session_page, session_time FROM s3 WHERE session_user_id = user_id) + WHERE EXISTS (SELECT session_user_id FROM s3 WHERE session_user_id = user_id)'; + $db->sql_query($sql); + } + else + { + // For SQLite versions prior to 3.8.3 and Oracle + $result = $db->sql_query($sql_select); + while ($row = $db->sql_fetchrow($result)) + { + $sql = 'UPDATE ' . USERS_TABLE . ' + SET user_lastvisit = ' . (int) $row['recent_time'] . ", user_lastpage = '" . $db->sql_escape($row['session_page']) . "' + WHERE user_id = " . (int) $row['session_user_id']; + $db->sql_query($sql); + } + $db->sql_freeresult($result); + } + break; + + case 'mysqli': + $sql = 'UPDATE ' . USERS_TABLE . " u, + ($sql_select) s3 + SET u.user_lastvisit = s3.recent_time, u.user_lastpage = s3.session_page + WHERE u.user_id = s3.session_user_id"; + $db->sql_query($sql); + break; + + default: + $sql = 'UPDATE ' . USERS_TABLE . " + SET user_lastvisit = s3.recent_time, user_lastpage = s3.session_page + FROM ($sql_select) s3 + WHERE user_id = s3.session_user_id"; + $db->sql_query($sql); + break; } - $db->sql_freeresult($result); // Delete all expired sessions $sql = 'DELETE FROM ' . SESSIONS_TABLE . ' -- cgit v1.2.1 From 7ea063100e23234bf0d6a79fd0411e956a1b6668 Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 5 Nov 2019 20:42:59 +0700 Subject: [ticket/15294] Minor code adjusting PHPBB3-15294 --- phpBB/phpbb/session.php | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) (limited to 'phpBB/phpbb/session.php') diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index 42ee509df4..6851bc8188 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -960,10 +960,10 @@ class session } /** - * Get expired sessions for registered users, only most recent for each user - * Inner SELECT gets most recent expired sessions for unique session_user_id - * Outer SELECT gets data for them - */ + * Get expired sessions for registered users, only most recent for each user + * Inner SELECT gets most recent expired sessions for unique session_user_id + * Outer SELECT gets data for them + */ $sql_select = 'SELECT s1.session_page, s1.session_user_id, s1.session_time AS recent_time FROM ' . SESSIONS_TABLE . ' AS s1 INNER JOIN ( @@ -979,8 +979,7 @@ class session switch ($db->get_sql_layer()) { case 'sqlite3': - case 'oracle': - if ($db->get_sql_layer() === 'sqlite3' && phpbb_version_compare($db->sql_server_info(true), '3.8.3', '>=')) + if (phpbb_version_compare($db->sql_server_info(true), '3.8.3', '>=')) { // For SQLite versions 3.8.3+ which support Common Table Expressions (CTE) $sql = "WITH s3 (session_page, session_user_id, session_time) AS ($sql_select) @@ -988,20 +987,21 @@ class session SET (user_lastpage, user_lastvisit) = (SELECT session_page, session_time FROM s3 WHERE session_user_id = user_id) WHERE EXISTS (SELECT session_user_id FROM s3 WHERE session_user_id = user_id)'; $db->sql_query($sql); + + break; } - else + + // No break, for SQLite versions prior to 3.8.3 and Oracle + case 'oracle': + $result = $db->sql_query($sql_select); + while ($row = $db->sql_fetchrow($result)) { - // For SQLite versions prior to 3.8.3 and Oracle - $result = $db->sql_query($sql_select); - while ($row = $db->sql_fetchrow($result)) - { - $sql = 'UPDATE ' . USERS_TABLE . ' - SET user_lastvisit = ' . (int) $row['recent_time'] . ", user_lastpage = '" . $db->sql_escape($row['session_page']) . "' - WHERE user_id = " . (int) $row['session_user_id']; - $db->sql_query($sql); - } - $db->sql_freeresult($result); + $sql = 'UPDATE ' . USERS_TABLE . ' + SET user_lastvisit = ' . (int) $row['recent_time'] . ", user_lastpage = '" . $db->sql_escape($row['session_page']) . "' + WHERE user_id = " . (int) $row['session_user_id']; + $db->sql_query($sql); } + $db->sql_freeresult($result); break; case 'mysqli': @@ -1037,7 +1037,7 @@ class session } // only called from CRON; should be a safe workaround until the infrastructure gets going - /* @var $captcha_factory \phpbb\captcha\factory */ + /* @var \phpbb\captcha\factory $captcha_factory */ $captcha_factory = $phpbb_container->get('captcha.factory'); $captcha_factory->garbage_collect($config['captcha_plugin']); -- cgit v1.2.1