Re: [aur-dev] [PATCH 5/6] Hack away at misuse of db_connect() in package details view

25 Aug 2011

On Mon, Aug 22, 2011 at 07:18:55PM -0500, Dan McGee wrote:
...
This drops the MySQL traffic from 55+ actions to under 20. The majority
of it is the needless two operations sent on every db_connect, whether
the connection is new or not:
    <connect to database aur>
    SET NAMES 'utf8' COLLATE 'utf8_general_ci'
Get a handle early, and use it often.
Obviously this performance boost can be applied to other pages as well,
but this seemed like the best one to tackle first as it is likely the
most viewed page, outside of the crazy amounts of RPC traffic.
Signed-off-by: Dan McGee <dan@archlinux.org>
---
Before: http://paste.pocoo.org/show/462712/
After:  http://paste.pocoo.org/show/462725/
Pretty stark difference.
web/html/packages.php         |   25 ++++++++++++-------------
 web/lib/aur.inc.php           |    6 +++---
 web/template/header.php       |   11 +++++++----
 web/template/login_form.php   |    2 +-
 web/template/pkg_comments.php |    3 +--
 web/template/pkg_details.php  |   15 ++++++---------
 6 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/web/html/packages.php b/web/html/packages.php
index dc06c7e..0a7c8d7 100644
--- a/web/html/packages.php
+++ b/web/html/packages.php
@@ -2,13 +2,14 @@
set_include_path(get_include_path() . PATH_SEPARATOR . '../lib');
-include_once("aur.inc.php");      # access AUR common functions
-set_lang();                       # this sets up the visitor's language
-include_once('pkgfuncs.inc.php'); # package specific functions
-check_sid();                      # see if they're still logged in
+include_once("aur.inc.php");
+include_once('pkgfuncs.inc.php');
+$dbh = db_connect();
+set_lang($dbh);
+check_sid($dbh);
# Set the title to the current query if required
-if (isset($_GET['ID']) && ($pkgname = pkgname_from_id($_GET['ID']))) {
+if (isset($_GET['ID']) && ($pkgname = pkgname_from_id($_GET['ID'], $dbh))) {
  $title = $pkgname;
 } else if (!empty($_GET['K'])) {
  $title = __("Search Criteria") . ": " . $_GET['K'];
@@ -18,9 +19,11 @@ if (isset($_GET['ID']) && ($pkgname = pkgname_from_id($_GET['ID']))) {
# Retrieve account type
 if (isset($_COOKIE["AURSID"])) {
-	$atype = account_from_sid($_COOKIE["AURSID"]);
+	$atype = account_from_sid($_COOKIE["AURSID"], $dbh);
+	$uid = uid_from_sid($SID, $dbh);
 } else {
  $atype = "";
+	$uid = null;
 }
# Grab the list of Package IDs to be operated on
@@ -78,7 +81,7 @@ if (current_action("do_Flag")) {
  $output = pkg_change_category($atype);
 }
-html_header($title);
+html_header($title, $dbh);
 ?>
<?php if ($output): ?>
@@ -91,12 +94,8 @@ if (isset($_GET['ID'])) {
  if (!$_GET['ID'] = intval($_GET['ID'])) {
      print __("Error trying to retrieve package details.")."<br />\n";
  } else {
-		if (isset($_COOKIE["AURSID"])) {
-			package_details($_GET['ID'], $_COOKIE["AURSID"]);
-		}
-		else {
-			package_details($_GET['ID'], null);
-		}
+		$sid = isset($_COOKIE["AURSID"]) ? $_COOKIE["AURSID"] : null;
+		package_details($_GET['ID'], $sid, $dbh);
  }
 } else {
  if (!isset($_GET['K']) && !isset($_GET['SB'])) {
diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php
index f432697..4b8a8c8 100644
--- a/web/lib/aur.inc.php
+++ b/web/lib/aur.inc.php
@@ -320,7 +320,7 @@ function set_lang($dbh=NULL) {
# common header
 #
-function html_header($title="") {
+function html_header($title="", $dbh=NULL) {
  global $_SERVER;
  global $_COOKIE;
  global $_POST;
@@ -462,9 +462,9 @@ function uid_from_email($email="", $dbh=NULL)
# check user privileges
 #
-function check_user_privileges()
+function check_user_privileges($dbh=NULL)
 {
-	$type = account_from_sid($_COOKIE['AURSID']);
+	$type = account_from_sid($_COOKIE['AURSID'], $dbh);
  return ($type == 'Trusted User' || $type == 'Developer');
 }
diff --git a/web/template/header.php b/web/template/header.php
index 8313bb3..c56a226 100644
--- a/web/template/header.php
+++ b/web/template/header.php
@@ -1,4 +1,8 @@
-<?php echo '<?xml version="1.0" encoding="UTF-8"?>'; ?>
+<?php echo '<?xml version="1.0" encoding="UTF-8"?>';
+if (!isset($dbh)) {
+	$dbh = db_connect();
+}
+$username = username_from_sid($_COOKIE["AURSID"], $dbh); ?>
You should add some isset() check here to make sure we don't try to
access '$_COOKIE["AURSID"]' if it is unset.
...
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
  "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
 <html xmlns="http://www.w3.org/1999/xhtml"
@@ -37,8 +41,8 @@
      		<li><a href="http://bugs.archlinux.org/index.php?tasks=all&project=2"><?php print __("Bugs"); ?></a></li>
      		<li><a href="http://archlinux.org/mailman/listinfo/aur-general"><?php print __("Discussion"); ?></a></li>
      		<?php if (isset($_COOKIE['AURSID'])): ?>
-					<?php if (check_user_privileges()): ?><li><a href="tu.php"><?php print __("Trusted User"); ?></a></li><?php endif; ?>
-					<li><a href="packages.php?SeB=m&K=<?php print username_from_sid($_COOKIE["AURSID"]); ?>"><?php print __("My Packages"); ?></a></li>
+					<?php if (check_user_privileges($dbh)): ?><li><a href="tu.php"><?php print __("Trusted User"); ?></a></li><?php endif; ?>
+					<li><a href="packages.php?SeB=m&K=<?php print $username; ?>"><?php print __("My Packages"); ?></a></li>
      			<li><a href="pkgsubmit.php"><?php print __("Submit"); ?></a></li>
      		<?php endif; ?>
      	</ul>
@@ -59,4 +63,3 @@ foreach ($SUPPORTED_LANGS as $lang => $lang_name) {
  </div>
  <!-- Start of main content -->
-
diff --git a/web/template/login_form.php b/web/template/login_form.php
index b351a27..419e031 100644
--- a/web/template/login_form.php
+++ b/web/template/login_form.php
@@ -1,7 +1,7 @@
 <div id="login_bar" class="pgbox">
 <?php
 if (isset($_COOKIE["AURSID"])) {
-	print __("Logged-in as: %s", '<b>' . username_from_sid($_COOKIE["AURSID"]) . '</b>');
+	print __("Logged-in as: %s", '<b>' . $username . '</b>');
 ?>
  <a href="logout.php">[<?php print __("Logout"); ?>]</a>
 <?php
diff --git a/web/template/pkg_comments.php b/web/template/pkg_comments.php
index aed9ca8..94e6f08 100644
--- a/web/template/pkg_comments.php
+++ b/web/template/pkg_comments.php
@@ -1,6 +1,5 @@
 <div class="pgbox">
 <?php
-$uid = uid_from_sid($SID);
See below.
...
while (list($indx, $carr) = each($comments)) { ?>
  <div class="comment-header"><?php
@@ -36,7 +35,7 @@ while (list($indx, $carr) = each($comments)) { ?>
 </div>
<?php
-$count = package_comments_count($_GET['ID']);
+$count = package_comments_count($_GET['ID'], $dbh);
 if ($count > 10 && !isset($_GET['comments'])) {
  echo '<div class="pgbox">';
  echo '<a href="'. $_SERVER['REQUEST_URI'] . '&comments=all">'. __('Show all %s comments', $count) . '</a>';
diff --git a/web/template/pkg_details.php b/web/template/pkg_details.php
index 880a675..599b603 100644
--- a/web/template/pkg_details.php
+++ b/web/template/pkg_details.php
@@ -1,12 +1,9 @@
 <?php
-$atype = account_from_sid($SID);
-$uid = uid_from_sid($SID);
-
You did it again! :) "web/template/pkg_details.php" is included in
package_details() where these variables are not visible. Removing these
lines breaks all forms that should be visible to logged in users only.
Same thing happens with the "$uid" initialization that you removed from
"web/template/pkg_comments.php". Adding "global $uid, $atype;" to
package_details() might help as a workaround - this needs urgent
refactoring anyway. 

I do not blame you here as our code isn't clear at all and this is
almost impossible to spot without having a very close look. Maybe you
could try to check if all important parts of the AUR still work and do
not issue any warnings after removing some variable initializations next
time - just as a tip for the future. Our code is a maze and it's almost
impossible to do it right at the first go.
...
$pkgid = intval($_REQUEST['ID']);
 if ($uid == $row["MaintainerUID"] or
  ($atype == "Developer" or $atype == "Trusted User")) {
-	$catarr = pkgCategories();
+	$catarr = pkgCategories($dbh);
  $edit_cat = "<form method='post' action='packages.php?ID=".$pkgid."'>\n";
  $edit_cat.= "<p>";
  $edit_cat.= "<input type='hidden' name='action' value='do_ChangeCategory' />";
@@ -29,7 +26,7 @@ else {
 }
if ($row["SubmitterUID"]) {
-	$submitter = username_from_id($row["SubmitterUID"]);
+	$submitter = username_from_id($row["SubmitterUID"], $dbh);
  if ($SID) {
      $submitter = '<a href="account.php?Action=AccountInfo&ID=' . htmlspecialchars($row['SubmitterUID'], ENT_QUOTES) . '">' . htmlspecialchars($submitter) . '</a>';
  }
@@ -39,7 +36,7 @@ if ($row["SubmitterUID"]) {
 }
if ($row["MaintainerUID"]) {
-	$maintainer = username_from_id($row["MaintainerUID"]);
+	$maintainer = username_from_id($row["MaintainerUID"], $dbh);
  if ($SID) {
      $maintainer = '<a href="account.php?Action=AccountInfo&ID=' . htmlspecialchars($row['MaintainerUID'], ENT_QUOTES) . '">' . htmlspecialchars($maintainer) . '</a>';
  }
@@ -103,8 +100,8 @@ $out_of_date_time = ($row["OutOfDateTS"] == 0) ? $msg : gmdate("r", intval($row[
  </p>
 <?php
-	$deps = package_dependencies($row["ID"]);
-	$requiredby = package_required($row["Name"]);
+	$deps = package_dependencies($row["ID"], $dbh);
+	$requiredby = package_required($row["Name"], $dbh);
if (count($deps) > 0 || count($requiredby) > 0) {
      echo '<p>';
@@ -142,7 +139,7 @@ $out_of_date_time = ($row["OutOfDateTS"] == 0) ? $msg : gmdate("r", intval($row[
# $sources[0] = 'src';
-	$sources = package_sources($row["ID"]);
+	$sources = package_sources($row["ID"], $dbh);
if (count($sources) > 0) {
-- 
1.7.6

    

Re: [aur-dev] [PATCH 5/6] Hack away at misuse of db_connect() in package details view

Lukas Fleischer