Statement nullified after loading results

[SharkyKZ]

Steps to reproduce the issue

Run a prepared statement in a loop.

$ids = [1, 2, 3];

$query
	->select('*')
	->from('#__table')
	->where('id = :id')
	->bind(':id', $id, ParameterType::INTEGER);
$db->setQuery($query);

foreach ($ids as $id)
{
	$db->loadObject();
}

Expected result

Rows with specified IDs loaded.

Actual result

Call to a member function bindParam() on null

System information (as much as possible)

PDO and MySQLi.

Additional comments

I'm again not sure whether this is expected or not. One can just move $db->setQuery() inside the loop to workaround this. However, this is not required when performing queries that are executed with $db->execute() (e.g. insert/update). To my (limited) understanding, given the nature of prepared statements, DatabaseDriver::freeResult() should nullify only the result set and not the entire statement.

[mbabker]

That example looks bad, you aren’t iterating over a result set but running 3 separate queries.

On Fri, Nov 1, 2019 at 12:08 PM SharkyKZ ***@***.***> wrote: Steps to reproduce the issue Run a prepared statement in a loop. $ids = [1, 2, 3]; $query ->select('*') ->from('#__table') ->where('id = :id') ->bind(':id', $id, ParameterType::INTEGER); $db->setQuery($query); foreach ($ids as $id) { $db->loadObject(); } Expected result Rows with specified IDs loaded. Actual result Call to a member function bindParam() on null System information (as much as possible) PDO and MySQLi. Additional comments I'm again not sure whether this is expected or not. One can just move $db->setQuery() inside the loop to workaround this. However, this is not required when performing queries that are executed with $db->execute() (e.g. insert/update). To my (limited) understanding, given the nature of prepared statements, DatabaseDriver::freeResult() should nullify only the result set and not the entire statement. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#204?email_source=notifications&email_token=AACZ7IL5CI4X2LSWGNSE2B3QRRH7XA5CNFSM4JH4ZH22YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HWEVO3A>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACZ7IJBSFHOOVRXAX3N6QTQRRH7XANCNFSM4JH4ZH2Q> .

-- - Michael Please pardon any errors, this message was sent from my iPhone.

[SharkyKZ]

Yes, I am. I know it's not needed for this specific query, but that's beside the point. It's just an example.

[SharkyKZ]

This should be clearer https://github.com/joomla/joomla-cms/blob/68f5004fefde38a09a3a70e7c42ec76f4be144fa/administrator/components/com_menus/Model/MenusModel.php#L79-L135

I would expect the query run in rapid succession without having to be re-set over and over again. That's how prepared statements are supposed to work, right?

And this does work when, for example, running update queries:

$query
	->update('#__table')
	->set('column = :data')
	->where('id = :id')
	->bind(':data', $data, ParameterType::STRING);
	->bind(':id', $id, ParameterType::INTEGER);
$db->setQuery($query);

foreach ($dataArray as $id => $data)
{
	$db->execute();
}

[mbabker]

A PDOStatement is meant to run one query, iterate the result set, and go away; the non-PDO code emulates this. So yes, for each individual query you must re-bind parameters ($db->setQuery($query)->execute() for every unique query, or whatever executor you’re aiming to use).

On Fri, Nov 1, 2019 at 12:49 PM SharkyKZ ***@***.***> wrote: This should be clearer https://github.com/joomla/joomla-cms/blob/68f5004fefde38a09a3a70e7c42ec76f4be144fa/administrator/components/com_menus/Model/MenusModel.php#L79-L135 I would expect the query run in rapid succession without having to be re-set over and over again. That's how prepared statements are supposed to work, right? And this does work when, for example, running update queries: $query ->update('#__table') ->set('column = :data') ->where('id = :id') ->bind(':data', $data, ParameterType::STRING); ->bind(':id', $id, ParameterType::INTEGER); $db->setQuery($query); foreach ($dataArray as $id => $data) { $db->execute(); } — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#204?email_source=notifications&email_token=AACZ7IN55T7SJ5MPVYI5BVTQRRM2FA5CNFSM4JH4ZH22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC3PSAA#issuecomment-548862208>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACZ7IKAD7XBZUXK36QPMKDQRRM2FANCNFSM4JH4ZH2Q> .

-- - Michael Please pardon any errors, this message was sent from my iPhone.

[SharkyKZ]

PDO documentation says otherwise:

The query only needs to be parsed (or prepared) once, but can be executed multiple times with the same or different parameters.

[mbabker]

You’re making a big ask here if you are looking for the database driver to remove all of its memory clearing logic just so a statement object can be reused. Because ultimately that’s what it’s going to take to run that set of queries exactly as you have it laid out now. And then you run the risk of new memory leaks because result set cursors aren’t getting cleared out of the system correctly. Personally, I would suggest everyone treat query objects and statements as one time use things instead of trying to reuse objects and trying to mangle the API. Sooner or later the reuse just asks for trouble.

On Fri, Nov 1, 2019 at 1:30 PM SharkyKZ ***@***.***> wrote: PDO documentation says otherwise: The query only needs to be parsed (or prepared) once, but can be executed multiple times with the same or different parameters. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#204?email_source=notifications&email_token=AACZ7IJHYA2TI546GFII5EDQRRYS7A5CNFSM4JH4ZH22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC3ZAFA#issuecomment-548900884>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACZ7IIIDYWGXDGFA3MU6PLQRRYS7ANCNFSM4JH4ZH2Q> .

-- - Michael Please pardon any errors, this message was sent from my iPhone.

[HLeithner]

That's one of the main features of prepared statements and we support it in the database drivers bind function with the reference value parameter.

I'm pretty sure we use this in the cms already but with an additional setQuery() before execute().

If we don't want to support this we should remove the reference in the bind function because it's a pain for alternative syntax like using arrays.

[SharkyKZ]

execute() works correctly without the need to run setQuery() every time. It's only load*() methods that have the issue. If we have to run setQuery() in the loop, we're not making use of performance improvements prepared statements have to offer.

[mbabker]

The execute method is generally meant to run with another method in the case of a SELECT query (if you look, all the load stuff does an execute first). So, you have to dig into the freeResult method to determine which memory releasing operations must go. Right now freeResult unsets the statement, that creates your problem. You need to determine what the side effects are if the statement isn’t unset with that. If it’s not the unset that’s the problem, then it’s the call to the statement’s closeCursor that is, and that is what calls the PHP extension code for cleaning memory. I’m not just arbitrarily removing the freeResult method and all calls to it, that to me is asking for trouble. So someone needs to test whatever changes they are looking for and clearly demonstrate the benefits and issues introduced by them.

On Sat, Nov 2, 2019 at 5:37 AM SharkyKZ ***@***.***> wrote: execute() works correctly without the need to run setQuery() every time. It's only load*() methods that have the issue. If we have to run setQuery() in the loop, we're not making use of performance improvements prepared statements have to offer. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#204?email_source=notifications&email_token=AACZ7IKYEVSPU5PDNKDAWSTQRVKAJA5CNFSM4JH4ZH22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC4Y4PY#issuecomment-549031487>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACZ7IN3RPAIIX5SPUWP5CDQRVKAJANCNFSM4JH4ZH2Q> .

-- - Michael Please pardon any errors, this message was sent from my iPhone.

[mbabker]

Ultimately, there is a reason execute standalone works the way you want it to. 1) The method is the backbone for all load actions (otherwise you have to manually $db->setQuery()->execute()->loadResult();) 2) While execute actually performs the query, it does not read the result set in; the result set is contained in a cursor that has to be iterated over to load each row into memory The API design has always been that the result set cursor is released after the result loaded. This works without prepared statements. It doesn’t work now because the prepared statement is destroyed as part of the memory cleanup. So if something is going to change, IMO it needs to be adequately benchmarked that there is not a major performance penalty and that memory leaks are not introduced (basically what happens if you don’t close 30 result set cursors as is a pretty typical CMS request’s number of queries), and it very well could be the answer is “there is no issue with it”. Otherwise, changing this code because “it worked this way before but doesn’t now” isn’t a strong enough justification to blindly change this part of the database drivers.

On Sat, Nov 2, 2019 at 5:37 AM SharkyKZ ***@***.***> wrote: execute() works correctly without the need to run setQuery() every time. It's only load*() methods that have the issue. If we have to run setQuery() in the loop, we're not making use of performance improvements prepared statements have to offer. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#204?email_source=notifications&email_token=AACZ7IKYEVSPU5PDNKDAWSTQRVKAJA5CNFSM4JH4ZH22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC4Y4PY#issuecomment-549031487>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACZ7IN3RPAIIX5SPUWP5CDQRVKAJANCNFSM4JH4ZH2Q> .

-- - Michael Please pardon any errors, this message was sent from my iPhone.

[SharkyKZ]

Closing the cursor is fine and possibly required on some systems, according to PDO documentation. It's the statement itself that should only be discarded when no longer needed.

What would be the most correct way to do this? Adding a parameter to load*() methods that's passed to freeResult() to make nullifying the statement optional? And a public method for nullifying the statement manually? Or some other way?

[mbabker]

No parameter. There should be no public API changes for this. The memory releasing is all internal implementation details, any public API change leaks this in a bad way. Hence, the only right thing to do being to adequately test that whatever changes you want to make inside the freeResult method does not create more problems than the one you are trying to solve.

On Sun, Nov 3, 2019 at 7:53 AM SharkyKZ ***@***.***> wrote: Closing the cursor is fine and possibly required on some systems, according to PDO documentation. It's the statement itself that should only be discarded when no longer needed. What would be the most correct way to do this? Adding a parameter to load*() methods that's passed to freeResult() to make nullifying the statement optional? And a public method for nullifying the statement manually? Or some other way? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#204?email_source=notifications&email_token=AACZ7IMGI3JXXBIXQWO7PCLQR3JXDA5CNFSM4JH4ZH22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC5TCKY#issuecomment-549138731>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACZ7IJNCL5TTLHK7PWXXWDQR3JXDANCNFSM4JH4ZH2Q> .

-- - Michael Please pardon any errors, this message was sent from my iPhone.

[SharkyKZ]

PR #205.

May I ask though, why are statements done in such a reclusive way? I noticed you mentioning Doctrine on multiple occasions. By comparison, it's a lot more public regarding this. Users could call $db->prepare() and get the prepared statements and then do whatever with it directly. Meanwhile here everything is done through the driver instance which holds at most a single statement at any given time. So while #205 allows to run statements in rapid succession, we still can't prepare multiple statements for later use.

[mbabker]

Before the full on prepared statement support the only internal implementation detail you could get to was the database connection resource (PDO instance, mysqli class instance, PostgreSQL resource handle, etc.). At no point is the result set cursor or the prepared statement resource ever made available as a public API. If someone wants to propose it then go for it but being honest I don’t really see the point of exposing API for every driver specific resource at any time in the application lifecycle (does someone really need to be able to access the mysqli_result object or the resource created by sqlsrv_prepare?). Let’s just be honest here. The Joomla database API was not designed with the current state of affairs in mind. A lot of it was built to deal with ext/mysql and ext/mysqli bridging, it wasn’t built with full cross platform design in mind, and certainly not with prepared statement support, and even more so not with this idea that any resource that isn’t the query builder or the connection itself being a reusable object.

On Wed, Nov 6, 2019 at 2:26 AM SharkyKZ ***@***.***> wrote: PR #205 <#205>. May I ask though, why are statements done in such a reclusive way? I noticed you mentioning Doctrine on multiple occasions. By comparison, it's a lot more public regarding this. Users could call $db->prepare() and get the prepared statements and then do whatever with it directly. Meanwhile here everything is done through the driver instance which holds at most a single statement at any given time. So while #205 <#205> allows to run statements in rapid succession, we still can't prepare multiple statements for later use. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#204?email_source=notifications&email_token=AACZ7IKZCMAZWZGWDD77LJLQSJ5UVA5CNFSM4JH4ZH22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDFWINI#issuecomment-550200373>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACZ7IJQ6GEOJLZXNZ4DJATQSJ5UVANCNFSM4JH4ZH2Q> .

-- - Michael Please pardon any errors, this message was sent from my iPhone.

[SharkyKZ]

Exposing the statement allows to run both prepared and non-prepared statements and to maintain multiple prepared statements. The latter isn't very suitable for use in the CMS though, as picking which statements to keep globally is very project-specific. But for custom applications using the framework it would make sense.

Anyways, for now I'd just like to continue writing parameterized queries in the CMS. #205 is enough for that.

Thanks.