Limit and Offset not working correctly with MSSQL

[Shnoulle]

What steps will reproduce the problem?

Use an (MS) SQL server up to 11 get a data table of 150 lines in SQL server Create a SQL request on a big table and add limit of 100 and offset of 100

$criteria = new CDBCriteria();
$criteria->order = 'id';
$criteria->limit = 100;
$criteria->offset = 100;

$Lines = MyModel::model()->findAll($criteria);
echo count($Lines);

Real code : https://github.com/LimeSurvey/LimeSurvey/blob/5f7030cc4cdc15bb2ec35768f933d50dbe048096/application/helpers/admin/exportresults_helper.php#L115 Can be seen too on Grid https://bugs.limesurvey.org/view.php?id=18420

What is the expected result?

get 50 lines

What do you get instead?

get 100 lines

Additional info

Q	A
Yii version	1.1.26
PHP version	7.4
Operating system	Windows IIS
SQL Server	SQLServerVersion 15.00.4249

• Related YII2 fix : https://github.com/yiisoft/yii2/blob/364e907875fd57ee218085cca796ac5d1c3c8d51/framework/db/mssql/QueryBuilder.php#L73
• Possible Yii1 fix : https://github.com/LimeSurvey/LimeSurvey/pull/2673

If OK : i create the pull request, i know it's not related to PHP version, but to SQL version.

[marcovtwout]

Could you clarify: Is this a forward compatibility issue with newer MSSQL versions? Or a bug when using older versions of MSSQL? And which versions exactly are working/broken?

This seems to be the related issue for Yii 2: https://github.com/yiisoft/yii2/pull/4254, and the fix: https://github.com/yiisoft/yii2/commit/212c5ee3ef2eb244f7ca346ea1825427200b3394

[Shnoulle]

Could you clarify: Is this a forward compatibility issue with newer MSSQL versions? Or a bug when using older versions of MSSQL? And which versions exactly are working/broken?

It's forward compatibility issue with newer MSSQL. I'm sure it broke with SQL15, the related fix in Yii2 show SQL11

It's already fixed in Yii2, but not in Yii1 :) only Yii1 have issue .

Unsure MSSQL issue are OK to be merged.

[marcovtwout]

@Shnoulle If this is the same issue that was fixed in Yii 2, that would mean that offset/pagination has never worked on Yii 1 withMSSQL 2012 ( "v11") and up? Seems strange that this is reported now if this problem already exists for 10 years.

Could anyone perhaps try to reproduce the issue on older MSSQL versions, such as 14, 11 and 10?

[Shnoulle]

For LimeSurvey : it exist since a lot of time, but the real issue shown only on grid part. And clearly : see the last 10 element and not 5 aren't seen like an issue

You can compare

Yii1 limit/offset version : https://github.com/yiisoft/yii/blob/cea893f9afda96f35ba99a284dd5dcd09992f0e2/framework/db/schema/mssql/CMssqlCommandBuilder.php#L216

And Yii2 limit/offset/order compatible version https://github.com/yiisoft/yii2/blob/364e907875fd57ee218085cca796ac5d1c3c8d51/framework/db/mssql/QueryBuilder.php#L115

For LimeSurvey : seem to be fixed for 2 SQL server…

[wtommyw]

Using the given example I was able to reproduce the issue in MSSQL versions 10, 11, 12, 14 and 15.

An example: The table item contains 100 rows. Using the following code:

$criteria = new CDbCriteria();
$criteria->limit = 10;
$criteria->offset = 95;

$models = Item::model()->findAll($criteria);

The following SQL is generated:

SELECT *
FROM (SELECT TOP 10 * -- limit
    FROM (SELECT TOP 105 * -- offset+limit
        FROM [dbo].[item] [t]
            ORDER BY id) AS [__inner__]
    ORDER BY id DESC) AS [__outer__]
ORDER BY id ASC

This query first selects the top 105 rows, ordered by their id descending. Off these 105 rows it selects the top 10 and finally it flips these 10 back around so they are in ascending order.

The table does not contain 105 rows, so the inner query will return the top 100 rows. From which we then select the top 10. Causing the last page in the paginations to overlap with the previous (as @Shnoulle described)

Concluson: this is not a forward compatibility issue as this behaviour is the same in version 10 through 15. It appears that pagination hasn't worked properly for MSSQL since at least 2008. (I was not able to get any old versions running)

How does Yii2 handle this? Yii2 diffirentiates between MSSQL <11 and MSSQL >=11, producing two different query's: The following SQL is generated for MSSQL <11:

SELECT TOP 10 * FROM (
    SELECT rowNum = ROW_NUMBER() 
    OVER (
        ORDER BY (SELECT NULL)
    ), * FROM [item]
) sub WHERE rowNum > 95

The following SQL is generated for MSSQL >=11:

SELECT * FROM [item] 
    ORDER BY (SELECT NULL) 
    OFFSET 95 ROWS 
    FETCH NEXT 10 ROWS ONLY

Both of these queries return the expected result. (e.g. item 95 through 100).

I propose Yii2's MSSQL version check and Yii2's fix for MSSQL >11 are backported so that CMssqlCommandBuilder will generate the correct SQL for MSSQl version 11 and newer.

[Shnoulle]

Thank you for confirmation

I can not really create the PR for MSSQL <11 since i can not test it :(

[marcovtwout]

@Shnoulle Could you test if https://github.com/yiisoft/yii/pull/4501 fixes your problem and could you verify the MSSQL version you are testing with?

[marcovtwout]

Fixed with https://github.com/yiisoft/yii/pull/4501

[jjdunn]

@marcovtwout wrote:

@Shnoulle If this is the same issue that was fixed in Yii 2, that would mean that offset/pagination has never worked on Yii 1 withMSSQL 2012 ( "v11") and up? Seems strange that this is reported now if this problem already exists for 10 years.

Hi all - I've been Yii1.x user with SQL Server since 2010 :-) The issue has existed for a long time !! See https://code.google.com/archive/p/yii/issues/1501 - I am "Happy Lion" in the comments on this ticket. Also https://code.google.com/archive/p/yii/issues/997 which is a related ticket.

I've been running with a patched framework since Feb 2012 (11+ years!) - using the fix proposed in issue 1501 comment (7)

BUT - I am very glad to see a better solution. The fix in #4501 is more elegant. We are currently running Yii 1.1.23; now upgrading to 1.1.28 in prep for upgrade from PHP 8.0.x to 8.1/8.2

Big thanks to everyone who continues to develop & maintain Yii, including 1.1.x !! We considered upgrading to 2.x but it was a lot of work, we have a lot of custom components built on top of 1.1, and not much payoff. If I was starting a new app I'd certainly use 2.x

[jjdunn]

we are testing using Yii 1.1.28, and the change in this ticket fails with a complex ORDER BY clause, as follows:

$crit = new CDbCriteria;
$crit->alias = 't';
$crit->addColumnCondition(....):
$crit->addInCondition(...);
$SQL= 'exists (select 1 from .... where ....)';
$crit->addCondition($SQL);
$crit->with = [....];
$FN= .....; // fieldname for sorting
$crit->order = '(case when t.Sitting = 0 then 1 else 0 end), s.Gender, (case when isnull(s.{$FN},0) >0 then 1 else 0 end), s.LastName, s.FirstName';

$crit->limit = $limit; // defined in prior code
$crit->offset = $offset; // defined in prior code
$total = TableName::model()->findAll($crit);  <<===== ERROR

2023/03/29 04:34:45[24.60.188.13] [error] [php] preg_match():
Compilation failed: missing closing parenthesis at offset 52
(C:\web\test\framework\db\schema\mssql\CMssqlCommandBuilder.php:304)

Note that this complex ORDER BY structure containing parentheses and a CASE statement, worked OK using the code cited in previous comment : comment: 7 from https://code.google.com/archive/p/yii/issues/1501

Environment: PHP 8.0.26 Win/x64 on Windows Server 2019; Apache httpd 2.4.54; SQL Server 14.0.3460.9; PHP PDO drivers 5.10.1.15814

[marcovtwout]

@jjdunn This is a known limitation of the Yii implementation, both before and after the fix from this ticket:

https://github.com/yiisoft/yii/blob/d701ef06a67ef270d5496a16aba9c6886baeef2a/framework/db/schema/mssql/CMssqlCommandBuilder.php#L170-L178

It looks like the comma in (case when isnull(s.{$FN},0) >0 then 1 else 0 end) is not allowed.

Since this code is pretty old and complex already I'm not too keen in adding support now, especially using the referenced implementation from google code, as it states "TODO optimize and redone cuz current implementation is very ugly". So I'm inclined to mark this a wontfix, but a thouroughly tested PR with proper implementation is always welcome.

[jjdunn]

@marcovtwout - thanks for responding quickly. I completely understand reluctance to patch Yii 1.1.x, especially given 1) SQL Server is uncommon; 2) edge case of complex ORDER BY case.

We will simply revert to our long-standing / working patch (however ugly) :-)