Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DBAL FETCH silently returns truncated results when database emits an error after initial prefetch #4553

Closed
amenning opened this issue Mar 27, 2021 · 8 comments · Fixed by #4581 or #4599

Comments

@amenning
Copy link
Contributor

amenning commented Mar 27, 2021

Bug Report

Summary

If one uses a FETCH to loop over a data set that is larger than the set prefetch size, it is possible that the result set will silently be truncated without an exception being thrown in PHP. DBAL should screen for DB errors for each fetch and emit a PHP exception.

This can become a problem when using distributed databases that are replicated from a central database source. Datasets could become invalidated when querying data on a replicated database that started before a DB refresh. The symptom of this issue will be that result sets can be truncated to any increment of the set prefetch size depending on when the results get invalidated on the DB. The rest of the PHP program could then be working with unexpected truncated result sets. Currently the only way to catch truncated result errors is to wrap the fetch method with a custom warning handler that screens for DB errors being emitted as PHP warnings (The PHP OCI8 driver only converts DB errors during a fetch to PHP warnings).

Current behavior

DBAL fetch will ignore db errors on subsequent fetches and can return truncated results without emitting a PHP exception.

How to reproduce

Example reproduction of issue with Oracle:

  • Create a DB connection and assign the Doctrine\DBAL\Connection object to the $dbh variable
  • Create a pipelined function on the database that returns numbers up to your defined php.ini oci8.default_prefetch size plus 10 more. This will cause the result set to require at least 2 DB calls. For example, if your defined prefetch value is 100, than this function will return 110 rows with each row containing a sequential number (e.g. 0 => ['column_value' => 1], 1 => ['column_value' => 2], ... ).
    $dbh->executeQuery(
        "CREATE TYPE return_numbers AS TABLE OF NUMBER(11)"
    );
    $dbh->executeQuery("
        CREATE OR REPLACE FUNCTION test_oracle_fetch_failure 
        RETURN return_numbers PIPELINED 
        AS 
            v_number_list return_numbers; 
        BEGIN 
            SELECT ROWNUM r 
            BULK COLLECT INTO v_number_list 
            FROM DUAL 
            CONNECT BY ROWNUM <= " . ini_get("oci8.default_prefetch") . " + 10;
    
            FOR i IN 1 .. v_number_list.COUNT 
            LOOP 
                PIPE ROW (v_number_list(i)); 
            END LOOP; 
    
            RETURN; 
        END;
    ");
  • Execute a select statement on the pipelined function
    echo "Your default OCI8 prefetch value is " . ini_get("oci8.default_prefetch") . PHP_EOL;
    $statement = $dbh->prepare("SELECT * FROM TABLE(test_oracle_fetch_failure())");
    $statement->execute();
  • Access the first row result causing the prefetch to be executed to locally cache rows up to the default prefetch number (often defaults to 100 rows)
    $firstRow = $statement->fetch();
    echo $firstRow['column_value'] . PHP_EOL // 1
  • Invalidate the original data set by updating the pipelined function on the DB to return numbers up to your defined php.ini prefetch size plus 20
    $dbh->executeQuery("
        CREATE OR REPLACE FUNCTION test_oracle_fetch_failure 
        RETURN return_numbers PIPELINED 
        AS 
            v_number_list return_numbers; 
        BEGIN 
            SELECT ROWNUM r 
            BULK COLLECT INTO v_number_list 
            FROM DUAL 
            CONNECT BY ROWNUM <= " . ini_get("oci8.default_prefetch") . " + 20;
    
            FOR i IN 1 .. v_number_list.COUNT 
            LOOP 
                PIPE ROW (v_number_list(i)); 
            END LOOP; 
    
            RETURN; 
        END;
    ");
  • Attempt to access the remaining rows. However, after iterating through the first batch of locally cached prefetched rows, PHP OCI8 driver will see the oracle error "ORA-04068: existing state of packages has been discarded" when it needs to prefetch the next batch of rows above the first set of prefetched rows and only emit a PHP warning. For example, if your prefetch value is 100, than this will only return the first 100 rows that were originally prefetched, and fail to fetch the remaining 10 rows.
    // Even though one is expecting this to echo out numbers up to 10 higher than your default prefetch size, 
    // the following will only echo numbers up to your default prefetch size (often 100 rows) and no more. 
    // No exception will be thrown; only a PHP warning.
    while ($row = $statement->fetch()) {
        echo $row['column_value'] . PHP_EOL;
    }
  • DBAL fetch does not screen for PHP warnings and DBAL fetch will behave as if all results have been returned without error; the PHP program will continue on without exception even though only a truncated result set was returned instead of the full result set expected.

Expected behavior

DBAL should screen for DB errors on each fetch and emit a PHP exception instead of allowing truncated result sets.

@amenning amenning changed the title DBAL FETCH silently returns truncated results even if database emits an error after initial prefetch DBAL FETCH silently returns truncated results when database emits an error after initial prefetch Mar 28, 2021
@morozov
Copy link
Member

morozov commented Mar 29, 2021

@amenning thank you for the report. Could you convert the code in the description to an integration test or at least a standalone script that reproduces the problem? This way, we could see what exactly is happening.

DBAL fetch does not screen for PHP warnings and DBAL fetch will behave as if all results have been returned without error.

What is the return value in this case? According to the documentation, E_WARNINGs are:

Run-time warnings (non-fatal errors). Execution of the script is not halted.

So we normally don't promote warnings to an exception. IIRC there was a similar edge case where after having received false from fetch() we could check the error state of the connection/statement. Something like that might work here as well but it would have to check for a specific error code since because it may be a notice, not a warning.

@amenning
Copy link
Contributor Author

amenning commented Apr 1, 2021

I'm not entirely sure how to write an integration test for this since it requires an Oracle db to be present. I could mock out a db connection to cause the warning to be thrown, but that would lock in the current DB functionality now and not warn us for future changes/behavior with a db like Oracle.

The warning that is issued is in the OCI8 result method when calling oci_fetch_array. It is an E_WARNING with message "oci_fetch_array(): ORA-04068: existing state of packages has been discarded ORA-04065: not executed, altered or dropped [] []".

Do you have a preference on how the integration test should be developed? Should I attempt to mock out the db with today's behavior?

@morozov
Copy link
Member

morozov commented Apr 1, 2021

We do have integration tests running on Oracle in GitHub actions. You can add a new integration test and submit a pull request.

You can spin up a docker container locally and develop against it:

$ docker run -p 1521:1521 wnameless/oracle-xe-11g-r2

Given that you have posted all the code snippets, I think I could put them together myself and reproduce the issue. It just may take extra time before I get to it.

@amenning
Copy link
Contributor Author

amenning commented Apr 1, 2021

Nice, I didn't realize those types of tests were present in this repo already. I'll give that a go, thank you for that feedback.

@amenning
Copy link
Contributor Author

amenning commented Apr 2, 2021

I have added a PR that adds a failing functional unit test - #4581

@morozov
Copy link
Member

morozov commented Apr 3, 2021

Thanks, @amenning! The test looks good. I'll try to debug it and see how the error handling could be improved.

@amenning
Copy link
Contributor Author

amenning commented Apr 3, 2021

I've added a potential solution in that PR that passes all the tests. It throws a new truncated query result exception using an error handler surrounding the oci_fetch_array call that only screens E_WARNINGS for ORA type errors.

@morozov morozov linked a pull request Apr 10, 2021 that will close this issue
@morozov morozov added this to the 3.1.0 milestone Apr 10, 2021
morozov added a commit to morozov/dbal that referenced this issue Apr 10, 2021
morozov added a commit that referenced this issue Apr 14, 2021
[GH-4553] Simplify error handling in the OCI8 driver
@morozov morozov linked a pull request Apr 17, 2021 that will close this issue
@morozov morozov closed this as completed Apr 17, 2021
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants