Advice: SQL optimization for enable_step() and disable_step()

Discussion forum for Extension Writers regarding Extension Development.
Post Reply
User avatar
primehalo
Former Team Member
Posts: 2864
Joined: Fri May 06, 2005 5:58 pm
Location: Redding, CA
Contact:

Advice: SQL optimization for enable_step() and disable_step()

Post by primehalo »

I have someone who was receiving out of memory issues when installing or disabling one of my extensions and I believe it has something to do with the database code in the enable_step() and disable_step() functions in the ext.php file. I'm not an expert at database code so I was hoping someone could take a look and offer suggestions as to optimizing this code.

When they tried installing it, they got this error in the logs:
PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 72 bytes) in .../phpbb/db/driver/postgres.php on line 281, referer: .../adm/index.php?i=acp_extensions&sid=7c260313b4165271649e578aa1e360b6&mode=main&action=enable_pre&ext_name=primehalo%2Fprimenotify
After increasing the memory limit they got it to install, but when disabling it they got this:
SQL ERROR [ postgres ]

ERROR: syntax error at or near "t1" LINE 1: DELETE t1 FROM phpbb_notifications t1 JOIN phpbb_notificatio... ^ []

SQL

DELETE t1 FROM phpbb_notifications t1 JOIN phpbb_notification_types t2 ON t1.notification_type_id = t2.notification_type_id AND t2.notification_type_name LIKE 'primehalo.primenotify.notification.type.%'

BACKTRACE

FILE: (not given by php)
LINE: (not given by php)
CALL: msg_handler()

FILE: [ROOT]/phpbb/db/driver/driver.php
LINE: 997
CALL: trigger_error()

FILE: [ROOT]/phpbb/db/driver/postgres.php
LINE: 194
CALL: phpbb\db\driver\driver->sql_error()

FILE: [ROOT]/phpbb/db/driver/factory.php
LINE: 329
CALL: phpbb\db\driver\postgres->sql_query()

FILE: [ROOT]/ext/primehalo/primenotify/ext.php
LINE: 125
CALL: phpbb\db\driver\factory->sql_query()

FILE: [ROOT]/phpbb/extension/manager.php
LINE: 274
CALL: primehalo\primenotify\ext->disable_step()

FILE: [ROOT]/includes/acp/acp_extensions.php
LINE: 267
CALL: phpbb\extension\manager->disable_step()

FILE: [ROOT]/includes/functions_module.php
LINE: 676
CALL: acp_extensions->main()

FILE: [ROOT]/adm/index.php
LINE: 82
CALL: p_master->load_active()
Increasing the memory limit again apparently made that error go away but I'm confused as to why. Here are the enable_step() and disable_step() functions:

Code: Select all

public function enable_step($old_state)
{
    if ($old_state === false)
    {
		$this->db = !$this->db ? $this->container->get('dbal.conn') : $this->db;

		// Grab all of our existing custom notifications (there shouldn't be any but we have to make sure otherwise we'll get an SQL error in the next step)
		$sql = 'SELECT * FROM ' . USER_NOTIFICATIONS_TABLE . ' WHERE item_type ' . $this->db->sql_like_expression('primehalo.primenotify.notification.type.' . $this->db->get_any_char());
		$result = $this->db->sql_query($sql);
		while ($row = $this->db->sql_fetchrow($result))
		{
			$user_ids_per_type[$row['item_type']][] = $row['user_id'];
		}

		// Now loop through each notification type that has an equivalent primenotify type so that we can make a primenotify copy of it
		foreach (self::$notification_types as $our_type => $orig_type)
		{
			// Create our notification type entries for each equivalent default notification type that already exists, copying over the notify status setting
			$sql = 'SELECT * FROM ' . USER_NOTIFICATIONS_TABLE . " WHERE item_type = '" . $this->db->sql_escape($orig_type) . "' AND method = 'notification.method.email'";
			$result = $this->db->sql_query($sql);
			if ($result)
			{
				$sql_ary = array();
				while ($row = $this->db->sql_fetchrow($result))
				{
					if (!isset($user_ids_per_type[$our_type][$row['user_id']]))
					{
						$sql_ary[] = array('item_type' => $our_type, 'user_id' => $row['user_id'], 'method' => 'notification.method.email', 'notify' => $row['notify']);
					}
				}
				$this->db->sql_freeresult($result);
				$this->db->sql_multi_insert(USER_NOTIFICATIONS_TABLE, $sql_ary);
			}

		}

		// Purge the cache to make sure our notification types show up on the UCP instead of the original types
		$cache = $this->container->get('cache');
		$cache->purge();

		return 'cache_purged';
    }

    return parent::enable_step($old_state);
}

Code: Select all

public function disable_step($old_state)
{
    if ($old_state === false)
    {
		$this->db = !$this->db ? $this->container->get('dbal.conn') : $this->db;

		// Convert all existing custom notifications into default notifications
		// Part 1/4: Get the notification type IDs so we know which notification IDs to find and what to convert them to
		$sql_in = array_merge(array_values(self::$notification_types), array_keys(self::$notification_types));
		$sql = 'SELECT * FROM ' . NOTIFICATION_TYPES_TABLE . ' WHERE ' . $this->db->sql_in_set('notification_type_name', $sql_in);
		$result = $this->db->sql_query($sql);

		// Part 2/4: Organize the notification IDs for easy access, using the type_name as the key and the type_id as the value
		$notification_type_ids = array();
		while ($row = $this->db->sql_fetchrow($result))
		{
			$notification_type_ids[$row['notification_type_name']] = $row['notification_type_id'];
		}
		$this->db->sql_freeresult($result);

		// Part 3/4: Change notification_type_id from our custom one to the equivalent default one
		foreach (self::$notification_types as $from => $to)
		{
			if (isset($notification_type_ids[$to]) && isset($notification_type_ids[$from]))
			{
				$sql = 'UPDATE ' . NOTIFICATIONS_TABLE . " SET notification_type_id = " . $this->db->sql_escape($notification_type_ids[$to]) . " WHERE notification_type_id = " . $this->db->sql_escape($notification_type_ids[$from]);
				$this->db->sql_query($sql);
			}
		}

		// Part 4/4: All notifications should have been converted, but just in case lets try deleting all notifications still containing our custom notification IDs
		$sql = 'DELETE t1 FROM ' . NOTIFICATIONS_TABLE . ' t1 JOIN ' . NOTIFICATION_TYPES_TABLE . ' t2 ON t1.notification_type_id = t2.notification_type_id AND t2.notification_type_name ' . $this->db->sql_like_expression('primehalo.primenotify.notification.type.' . $this->db->get_any_char());
		$this->db->sql_query($sql);

		// Delete our custom notification types from the Notification Types Table
		$sql = 'DELETE FROM ' . NOTIFICATION_TYPES_TABLE . ' WHERE notification_type_name ' . $this->db->sql_like_expression('primehalo.primenotify.notification.type.' . $this->db->get_any_char());
		$this->db->sql_query($sql);

		// Delete our custom notification types from the User Notifications Table
		$sql = 'DELETE FROM ' . USER_NOTIFICATIONS_TABLE . ' WHERE item_type ' . $this->db->sql_like_expression('primehalo.primenotify.notification.type.' . $this->db->get_any_char());
		$result = $this->db->sql_query($sql);

		// Clear the cache
		$cache = $this->container->get('cache');
		$cache->purge();

		return 'cache_purged';
    }

    return parent::disable_step($old_state);
}
Ken F. Innes IV
My Extensions | My MODs | My Topics | My Site: Absolute Anime
Experience the wonder of Japanese Animation!

rxu
Extensions Development Team
Posts: 3066
Joined: Wed Oct 25, 2006 12:46 pm
Location: Siberia, Russian Federation
Name: Ruslan
Contact:

Re: Advice: SQL optimization for enable_step() and disable_step()

Post by rxu »

I'm pretty sure use of JOIN statement with DELETE in Postgres is not allowed, hence the syntax error.

User avatar
VSE
Extensions Development Coordinator
Extensions Development Coordinator
Posts: 5095
Joined: Sat Jan 17, 2009 9:37 am
Location: Los Angeles, CA
Name: Matt Friedman
Contact:

Re: Advice: SQL optimization for enable_step() and disable_step()

Post by VSE »

Maybe also wrap sql_transaction()'s around those foreach loops
🧩 My Extensions 🧩 ✨ YES...they ALL work with phpBB 3.3!! ✨
Please do not PM me for support.

User avatar
primehalo
Former Team Member
Posts: 2864
Joined: Fri May 06, 2005 5:58 pm
Location: Redding, CA
Contact:

Re: Advice: SQL optimization for enable_step() and disable_step()

Post by primehalo »

Thanks for the tips. I rewrote that SQL so that it did not use JOIN with DELETE and added sql_transaction() statements around the foreach loops. I have neither Postgres or an 11 GB database so can't really accuracy test to see if this fixes the problem, but it's a start.

Code: Select all

public function enable_step($old_state)
{
    if ($old_state === false)
    {
		$this->db = !$this->db ? $this->container->get('dbal.conn') : $this->db;

		// Grab all of our existing custom notifications (there shouldn't be any but we have to make sure otherwise we'll get an SQL error in the next step)
		$sql = 'SELECT * FROM ' . USER_NOTIFICATIONS_TABLE . ' WHERE item_type ' . $this->db->sql_like_expression('primehalo.primenotify.notification.type.' . $this->db->get_any_char());
		$result = $this->db->sql_query($sql);
		while ($row = $this->db->sql_fetchrow($result))
		{
			$user_ids_per_type[$row['item_type']][] = $row['user_id'];
		}

		// Now loop through each notification type that has an equivalent primenotify type so that we can make a primenotify copy of it
		$this->db->sql_transaction('begin');
		foreach (self::$notification_types as $our_type => $orig_type)
		{
			// Create our notification type entries for each equivalent default notification type that already exists, copying over the notify status setting
			$sql = 'SELECT * FROM ' . USER_NOTIFICATIONS_TABLE . " WHERE item_type = '" . $this->db->sql_escape($orig_type) . "' AND method = 'notification.method.email'";
			$result = $this->db->sql_query($sql);
			if ($result)
			{
				$sql_ary = array();
				while ($row = $this->db->sql_fetchrow($result))
				{
					if (!isset($user_ids_per_type[$our_type][$row['user_id']]))
					{
						$sql_ary[] = array('item_type' => $our_type, 'user_id' => $row['user_id'], 'method' => 'notification.method.email', 'notify' => $row['notify']);
					}
				}
				$this->db->sql_freeresult($result);
				$this->db->sql_multi_insert(USER_NOTIFICATIONS_TABLE, $sql_ary);
			}

		}
		$this->db->sql_transaction('commit');

		// Purge the cache to make sure our notification types show up on the UCP instead of the original types
		$cache = $this->container->get('cache');
		$cache->purge();

		return 'cache_purged';
    }

    return parent::enable_step($old_state);
}

Code: Select all

public function disable_step($old_state)
{
    if ($old_state === false)
    {
		$this->db = !$this->db ? $this->container->get('dbal.conn') : $this->db;

		// Convert all existing custom notifications into default notifications
		// Part 1/4: Get the notification type IDs so we know which notification IDs to find and what to convert them to
		$sql_in = array_merge(array_values(self::$notification_types), array_keys(self::$notification_types));
		$sql = 'SELECT * FROM ' . NOTIFICATION_TYPES_TABLE . ' WHERE ' . $this->db->sql_in_set('notification_type_name', $sql_in);
		$result = $this->db->sql_query($sql);

		// Part 2/4: Organize the notification IDs for easy access, using the type_name as the key and the type_id as the value
		$notification_type_ids = array();
		while ($row = $this->db->sql_fetchrow($result))
		{
			$notification_type_ids[$row['notification_type_name']] = $row['notification_type_id'];
		}
		$this->db->sql_freeresult($result);

		// Part 3/4: Change notification_type_id from our custom one to the equivalent default one
		$this->db->sql_transaction('begin');
		foreach (self::$notification_types as $from => $to)
		{
			if (isset($notification_type_ids[$to]) && isset($notification_type_ids[$from]))
			{
				$sql = 'UPDATE ' . NOTIFICATIONS_TABLE . " SET notification_type_id = " . $this->db->sql_escape($notification_type_ids[$to]) . " WHERE notification_type_id = " . $this->db->sql_escape($notification_type_ids[$from]);
				$this->db->sql_query($sql);
			}
			unset($notification_type_ids[$to]);	// Remove default notification IDs so in the end we'll only be left with our custom notification IDs
		}
		$this->db->sql_transaction('commit');

		// Part 4/4: All notifications should have been converted, but just in case lets try deleting all notifications still containing our custom notification IDs
		$sql_in = array_values($notification_type_ids); // At this point $notification_type_ids contains only our custom notification IDs
		$sql = 'DELETE FROM ' . NOTIFICATIONS_TABLE .  ' WHERE ' . $this->db->sql_in_set('notification_type_id', $sql_in);
		$this->db->sql_query($sql);

		// Delete our custom notification types from the Notification Types Table
		$sql = 'DELETE FROM ' . NOTIFICATION_TYPES_TABLE . ' WHERE notification_type_name ' . $this->db->sql_like_expression('primehalo.primenotify.notification.type.' . $this->db->get_any_char());
		$this->db->sql_query($sql);

		// Delete our custom notification types from the User Notifications Table
		$sql = 'DELETE FROM ' . USER_NOTIFICATIONS_TABLE . ' WHERE item_type ' . $this->db->sql_like_expression('primehalo.primenotify.notification.type.' . $this->db->get_any_char());
		$result = $this->db->sql_query($sql);

		// Clear the cache
		$cache = $this->container->get('cache');
		$cache->purge();

		return 'cache_purged';
    }

    return parent::disable_step($old_state);
}
Ken F. Innes IV
My Extensions | My MODs | My Topics | My Site: Absolute Anime
Experience the wonder of Japanese Animation!

Post Reply

Return to “Extension Writers Discussion”