Ticket #559 (closed enhancement: fixed)

Opened 3 years ago

Last modified 18 months ago

Add prepared query caching support to PropelPDO

Reported by: hans Owned by: hans
Priority: high Milestone: 1.3
Component: Generator Version: 1.3.0beta3
Severity: major Keywords:
Cc: cache prepared statement close cursor

Description

From Torsten Zander on list:

We tried to implement a query cache in propel 1.3 for that we overwrote PropelPDO::prepare()

<?php

private static $preparedStatements;

public function prepare($sql)
{
        if(!array_key_exists($sql, self::$preparedStatements)) {
            Propel::log($sql, Propel::LOG_DEBUG);
            return parent::prepare($sql);
        } else {
            return self::$preparedStatements($sql);
        }
}

this works in most cases fine only if run run all tests we get the error:

SQLSTATE[HY000]: General error: 2014 Cannot execute queries while other unbuffered queries are active.

This is caused due to the fact PDO method closeCursor is not used after a fetch method.

Follow-up message from Torsten: \ I think i've got it solved with the query cache. I overwrote PropelPDO::prepare().

<?php

        /**
         * Overrides PDO::prepare() to add logging and caching
         * .
         * @param  string $sql
         * @param  array
         * @return PDOStatement
         *
         */
        public function prepare($sql, $driver_options = array())
        {
        $key = md5($sql);
            if(!array_key_exists($key, self::$preparedStatements)) {
                Propel::log($sql, Propel::LOG_DEBUG);
                return parent::prepare($sql, $driver_options);
            } else {
            return self::$preparedStatements[$key];
            }
        }

and then I modified the PeerBuilder classes.

I added $stmt->closeCursor() after every fetch method

<?php 

if ($row = $stmt->fetch(PDO::FETCH_NUM)) {
   $stmt->closeCursor();
   return (int) \$row[0];

or

<?php 
while ($row = $stmt->fetch(PDO::FETCH_NUM)) {

}
$stmt->closeCursor();

This passed our allTests with about 1300 tests and most of them using propel, so I don't see any problem. Maybe you can consider to add it to propel sources.

Change History

comment:1 Changed 3 years ago by hans

  • Status changed from new to assigned

comment:2 Changed 3 years ago by hans

(In [912]) Refs #559 - Adding prepared statement caching to PropelPDO and the generated classes. (+ some other minor test fixes)

comment:3 Changed 3 years ago by hans

  • Status changed from assigned to closed
  • Resolution set to fixed

Ok, this support has been added in r912. All unit tests passing, so I don't think this will cause any problems for people.

A couple of notes:

  • I changed the prepared statement cache to not be static. I wasn't sure what the side-effects of having multiple connections sharing prepared statements would be, but it seemed very likely that this would cause problems. (e.g. if same SQL statement was executed by different connections).
  • I didn't notice any speed improvement with my MySQL configuration. It was actually slower with the new prepare() method (and non-emulated prepared statements). Telling the driver to emulate prepares actually sped it up a bit, and disabling the overridden prepare() statement sped it up further (back to pre-patch levels, of course). This was a very unscientific benchmarking test, however. Furthermore, my instance is not at all tuned for caching statements. I suspect that there is a performance gain to be had with this patch. Indeed, on Postgres there was a statistically significant speed improvement. My GeneratedObjectTest suite took ~5.1 seconds without this patch and ~4.7 seconds with this patch.

comment:4 Changed 3 years ago by torstenzander@…

  • Status changed from closed to reopened
  • Resolution fixed deleted

Hi Hans,

some how i send you the wrong method this one never really caches !!

/

  • Overrides PDO::prepare() to add logging and caching
  • .
  • @param string $sql
  • @param array
  • @return PDOStatement * */

public function prepare($sql, $driver_options = array()) { $key = md5($sql);

if(!array_key_exists($key, self::$preparedStatements)) {

Propel::log($sql, Propel::LOG_DEBUG); $stmt = parent::prepare($sql, $driver_options); self::$preparedStatements[$key] = $stmt; return $stmt;

} else {

return self::$preparedStatements[$key];

}

}

sorry for that

comment:5 Changed 3 years ago by hans

  • Status changed from reopened to closed
  • Resolution set to fixed

No problem -- I caught that as I was applying the patch & fixed it. Take a look at [912]; let me know if you see a problem with the code I committed.

comment:6 Changed 3 years ago by hans

  • Status changed from closed to reopened
  • Resolution fixed deleted

I'm reopening this ticket. While the current solution does work, storing the prepared statements inside the object has the side-effect that the object will not get destroyed & so connections will not get closed by Propel::close(). This has to do with ref counts, though I'm not entirely sure I understand why :)

I'm going to disable this behavior by default but add the ability to have this enabled via an attribute in the runtime conf.

comment:7 Changed 3 years ago by hans

(In [962]) Refs #559 - Making the prepared statement caching optional (and disabled by default) with PropelPDO.

comment:8 Changed 3 years ago by hans

(In [962]) Refs #559 - Making the prepared statement caching optional (and disabled by default) with PropelPDO.

comment:9 Changed 3 years ago by hans

  • Status changed from reopened to closed
  • Resolution set to fixed

Ok, in r962, the behavior here was changed to make this optional. If you want to enable the prepared statement caching, you must specify a value of true for the PropelPDO::PROPEL_ATTR_CACHE_PREPARES attribute.

Example snippet from runtime-conf.xml:

<connection>
        <dsn>mysql:dbname=test</dsn>
        <attributes>
                <option id="PROPEL_ATTR_CACHE_PREPARES">true</option>
        </attributes>
</connection>
Note: See TracTickets for help on using tickets.