setMaxResults() does not apply LIMIT in QueryBuilder on delete.

[rokas-reizgys-tg]

Bug Report

Q	A
Version	4.3.2
Previous Version if the bug is a regression	x.y.z

Summary

Found an issue with setMaxResults() method with DELETE query. Regarding to Doctrine official docs, I assume that LIMIT clause is supported by doctrine and should be added when build a query with QueryBuilder. But in my case - it is not a case. When i found out that this does not limit the records - I tried adding setFirstResult(0), but it did not help. Here is my code and printed SQL query using getSQL():

Code:

        $query = $this->dbal
            ->createQueryBuilder()
            ->delete($this->table->getTableName())
            ->where('created_at < :retention_date')
            ->andWhere('status = :status')
            ->setParameter('retention_date', $retentionDate, Types::DATETIME_IMMUTABLE)
            ->setParameter('status', MessageStatus::HANDLED->value)
            ->setFirstResult( 0 )
            ->setMaxResults(50)
        ;

Generated query:

DELETE FROM messenger_messages_outbox_storage WHERE (created_at < ?) AND (status = ?)

This code is expected to delete 50 records with LIMIT clause applied.
Expected query:

DELETE FROM messenger_messages_outbox_storage 
WHERE (created_at < ?) AND (status = ?) 
LIMIT 50

[greg0ire]

DELETE FROM messenger_messages_outbox_storage 
WHERE (created_at < ?) AND (status = ?) 
LIMIT 50

That's not valid SQL, not with Postgres anyway: https://www.postgresql.org/docs/current/sql-delete.html

[rokas-reizgys-tg]

But setMaxResults does not get applied anyways.

[greg0ire]

Well DELETE does not return results, now does it? 🤷

[eugene-borovov]

@greg0ire It's valid in MySQL and other

[greg0ire]

Cool, but it does not change the fact that using a LIMIT on something that returns no results makes no sense.

[greg0ire]

That being said, maybe we should do something similar to what @janedbal did here for the ORM: doctrine/orm#12068

[eugene-borovov]

@greg0ire The LIMIT clause places a limit on the number of rows that can be updated or deleted. Very useful in my opinion.

[greg0ire]

I never said it was useless. I said it's not portable, and semantically, it clashes with setMaxResults(). With Postgresql, you can have a RETURNING clause, so you can have results with a DELETE, and that's where IMO setMaxResults() if it was portable, but it isn't so… 🤷

[rokas-reizgys-tg]

Thank you @greg0ire for your insights. I assume as this is not a bug and it's intended functionality, maybe at least the documentation should be updated to state that only specific operations can be used with the setMaxResults() method. Specifically I am referring to this doc, where the example only shows the SELECT, but does not explicitly mention that DELETE is not supported.
https://www.doctrine-project.org/projects/doctrine-dbal/en/4.3/reference/query-builder.html#limit-clause

[greg0ire]

Ok, I'm reopening this as a documentation issue then. If you want to send a pull request, the code is here:

dbal/docs/en/reference/query-builder.rst

Lines 208 to 221 in d5a5a21

	Only a few database vendors have the ``LIMIT`` clause as known from MySQL,
	but we support this functionality for all vendors using workarounds.
	To use this functionality you have to call the methods ``setFirstResult($offset)``
	to set the offset and ``setMaxResults($limit)`` to set the limit of results
	returned.
	.. code-block:: php
	<?php
	$queryBuilder
	->select('id', 'name')
	->from('users')
	->setFirstResult(10)
	->setMaxResults(20);