Ensure the pg_depend relation is for a table object #5800

allenisalai · 2022-10-25T19:04:21Z

This fixes a bug where there was a pg_proc which was a dependency of an extension and also had an identical oid to the table class being described. Oids are not guaranteed to be unique because they will wrap around once they hit the unsigned integer max. The added conditional will ensure that the target object of the dependency is a table.

Q	A
Type	bug
Fixed issues	#5781

Summary

See #5781 for an in depth description of the issue.

Since the issue itself it hard to replicate, I checked the backwards compatibility with #5668 (comment) which was the original cause of the change.

derrabus · 2022-10-25T19:56:11Z

Thank you. Would it be possible to write a functional test for this issue?

allenisalai · 2022-10-25T20:33:23Z

@derrabus There are 2 ways I can think to write a functional test for this. Let me know if you have any preference or other ideas.

Write a script that will cause the oid wrap around. I have a script that attempted to do that, but it would take 2 days to run on my machine since we have to insert so many records. See below for an example of what I was trying. Maybe there's a more efficient way to load that many records.
I can try to hack the pg_class table to force a conflict. Would that be a valid enough test? I not familiar with postgres internals to know what else I would have to change to ensure that it was a valid state but it seems pretty straight forward.

CREATE TABLE IF NOT EXISTS test(id INT NOT NULL);
CREATE EXTENSION IF NOT EXISTS pg_prewarm;
UPDATE pg_class SET oid = (
    SELECT objid 
    FROM pg_depend 
        JOIN pg_extension as ex on ex.oid = pg_depend.refobjid 
         WHERE ex.extname = 'pg_prewarm' 
         ORDER BY objid
         LIMIT 1
)
WHERE pg_class.relname='test';

Wrap around script

<?php

// touch empty_file.txt
// docker run --rm -p 5432:5432 -e POSTGRES_PASSWORD=Passw0rd -v $(pwd)/empty_file.txt:/tmp/empty_file.txt postgres:14.2-alpine

require 'vendor/autoload.php';

use Doctrine\DBAL\DriverManager;


$connection = DriverManager::getConnection([
    'driver' => 'pdo_pgsql',
    'host' => 'localhost',
    'user' => 'postgres',
    'password' => 'Passw0rd',
]);


$connection->executeStatement('CREATE TABLE IF NOT EXISTS oid_wrap (oid_number oid)');
$connection->executeStatement('CREATE EXTENSION IF NOT EXISTS pg_prewarm');

$procOid = $connection->fetchOne("
    SELECT objid 
    FROM pg_depend 
        JOIN pg_extension as ex on ex.oid = pg_depend.refobjid 
         WHERE ex.extname = 'pg_prewarm' 
         ORDER BY objid
         LIMIT 1"
);

echo sprintf("proc id: %s\n",$procOid);

$connection->executeStatement('CREATE TABLE IF NOT EXISTS abc (id INT NOT NULL)');
$tableOid = $connection->fetchOne("
    SELECT oid
    FROM pg_class
        WHERE relname='abc'
        LIMIT 1"
);
echo sprintf("Table id: %s\n", $tableOid);

$maxOidValue = 4294967295;
$totalObjsToInsert = $maxOidValue + $procOid - ($tableOid - $procOid); // 10000 + 16388 - (16391-16388)

echo sprintf("Objs To Insert: %s\n", $totalObjsToInsert);

$start = microtime(true);
while($totalObjsToInsert !== 0)
{
    $objsToInsert = 1000000;
    if ($totalObjsToInsert < 100000) {
        $objsToInsert = $totalObjsToInsert;
    }

    $connection->executeStatement("INSERT INTO oid_wrap 
            SELECT lo_import('/tmp/blob.txt')
            FROM generate_series(1, $objsToInsert)");
    
    $totalObjsToInsert -= $objsToInsert;
}
$end = microtime(true);

$connection->executeStatement('DROP TABLE IF EXISTS abc');
$connection->executeStatement('CREATE TABLE IF NOT EXISTS abc (id INT NOT NULL)');
$tableOid = $connection->fetchOne("
    SELECT oid
    FROM pg_class
        WHERE relname='abc'
        LIMIT 1"
);

echo sprintf("Time to run: %s\n", ($end - $start));
echo sprintf("Final Table id: %s\n", $tableOid);

derrabus · 2022-10-27T11:36:24Z

Option 1 sounds like it's too slow to run that test in the CI. Can you give option 2 a try?

allenisalai · 2022-11-02T22:50:32Z

@derrabus I added the functional test you requested.

allenisalai · 2022-11-23T19:37:35Z

Since psql 9.4 doesn't support setting the oid (https://github.com/doctrine/dbal/actions/runs/3380906837/jobs/5879984623#step:6:84) I mark the test as skipped for the 9.4 platform.

tests/Functional/Schema/PostgreSQLSchemaManagerTest.php

klaussilveira · 2023-01-25T16:01:58Z

Just checking the status on this. Would help us unlock the upgrade to the newest dbal version.

derrabus · 2023-01-28T14:08:29Z

Just checking the status on this. Would help us unlock the upgrade to the newest dbal version.

Have you tested the changes in this PR? If you need this change, please don't query us for a status, but help us ship it.

derrabus · 2023-01-28T14:04:00Z

tests/Functional/Schema/PostgreSQLSchemaManagerTest.php

@@ -561,6 +564,73 @@ public function testAlterTableAutoIncrementIntToBigInt(
        self::assertTrue($tableFinal->getColumn('id')->getAutoincrement());
    }

+    public function testListTableColumnsOidConflictWithNonTableObject(): void
+    {
+        $semanticVersion = $this->connection->fetchOne('SHOW SERVER_VERSION');


Doesn't this work?

Suggested change

$semanticVersion = $this->connection->fetchOne('SHOW SERVER_VERSION');

$semanticVersion = $this->connection->getServerVersion();

getServerVersion on the connection object is private. $this->connection->getWrappedConnection() has a public method, but is deprecated so I went with$this->connection->getNativeConnection()->getAttribute(PDO::ATTR_SERVER_VERSION).

getServerVersion on the connection object is private.

Indeed.

$this->connection->getWrappedConnection() has a public method, but is deprecated

Let's use that anyway. I'll have to adjust the call in 4.0.x, but that's fine.

so I went with$this->connection->getNativeConnection()->getAttribute(PDO::ATTR_SERVER_VERSION).

This will fail as soon as we merge your change to 3.6.x because we have a Postgres driver there that does not use PDO.

tests/Functional/Schema/PostgreSQLSchemaManagerTest.php

derrabus · 2023-01-28T14:10:36Z

tests/Functional/Schema/PostgreSQLSchemaManagerTest.php

+        // revert to the original oid
+        $this->connection->executeStatement(
+            'UPDATE pg_attribute SET attrelid = ? WHERE attrelid = ?',
+            [$originalTableOid, $conflictingOid],
+        );
+        $this->connection->executeStatement(
+            'UPDATE pg_description SET objoid = ? WHERE objoid = ?',
+            [$originalTableOid, $conflictingOid],
+        );
+        $this->connection->executeStatement(
+            'UPDATE pg_class SET oid = ? WHERE oid = ?',
+            [$originalTableOid, $conflictingOid],
+        );
+
+        $this->connection->executeStatement(sprintf('DROP TABLE IF EXISTS %s', $table));


Why do we need to change thing in the database after we have made our assertion?

Since I'm modifying the pg catalog directly, I wanted to put the original values back so that this wouldn't affect downstream tests. I tried wrapping the changes in a transaction but unfortunately that didn't work and pg threw an error when rolling back. Even if it doesn't cause problems now, if it does affect a test in the future I think it will be impossible to debug for whatever poor soul runs into it.

Okay, but that means, if the assertion fails, your cleanup won't run?

I moved the assertion after undoing the database changes to account for this.

allenisalai · 2023-02-16T22:41:30Z

FWIW I have had my fork running in production since the 8th based off of 3.5.3

derrabus · 2023-02-16T22:55:57Z

tests/Functional/Schema/PostgreSQLSchemaManagerTest.php

+        $versionParts      = [];
+        $wrappedConnection = $this->connection->getWrappedConnection();
+        assert($wrappedConnection instanceof ServerInfoAwareConnection);
+        assert(preg_match('/^(?P<major>\d+)/', $wrappedConnection->getServerVersion(), $versionParts) > 0);


Since we're going to use $versionParts afterwards, we need to be sure the preg_match() is actually executed. Since assert()s can be disabled, you'll have to move that call out of the assert().

That being said, you might just use PHP's version_compare() function to check if we're on Postgres 10 or later.

I'm not sure if I implemented what you wanted. If assert can be disabled, should I throw an exception if $versionParts['major'] isn't set? That's what I was trying to accomplish with the original assert call.

derrabus · 2023-02-16T23:02:51Z

FWIW I have had my fork running in production since the 8th based off of 3.5.3

I have no doubt that your change works. It's just that I need a proper test to make sure nobody's going to break it in the future. But I think we're close to merging the PR. 🤞🏻

derrabus · 2023-02-17T16:09:41Z

tests/Functional/Schema/PostgreSQLSchemaManagerTest.php

+        $matchCount = preg_match('/^(?P<major>\d+)/', $wrappedConnection->getServerVersion(), $versionParts);
+        assert($matchCount > 0);
+
+        $majorVersion = $versionParts['major'];
+        if (version_compare($majorVersion, '10', '<')) {


I don't think you understood my last comment about version_compare(). Have you tried if this works?

Suggested change

$matchCount = preg_match('/^(?P<major>\d+)/', $wrappedConnection->getServerVersion(), $versionParts);

assert($matchCount > 0);

$majorVersion = $versionParts['major'];

if (version_compare($majorVersion, '10', '<')) {

if (version_compare($wrappedConnection->getServerVersion(), '10.0', '<')) {

Using version_compare() is a bit pointless if we still have to parse the server version.

It worked. Thanks for the clarification.

This fixes a bug where there was a pg_proc which was a dependency of an extention and also had an identical oid to the table class being described. Oids are not guaranteed to be unique because they will wrap around once they hit the unsigned integer max. The added conditional will ensure that the target object of the dependency is a table. Fixes: doctrine#5781

allenisalai · 2023-03-02T22:46:29Z

Thanks for your time and patience @derrabus

derrabus · 2023-03-06T10:10:42Z

tests/Functional/Schema/PostgreSQLSchemaManagerTest.php

+        if (version_compare($wrappedConnection->getServerVersion(), '10.0', '<')) {
+            self::markTestSkipped('Manually setting the Oid is not supported in 9.4');
+        }


It appears we need to skip that test on Postgres 10 and 11 as well. #5955

:( Sorry about that

No worries, my comment was just FYI. 🙂

derrabus changed the title ~~Ensure the pg_depend relation is for a table object.~~ Ensure the pg_depend relation is for a table object Oct 27, 2022

derrabus added PostgreSQL Bug Schema Management labels Oct 27, 2022

allenisalai force-pushed the psql-schema-manager-error branch 2 times, most recently from c60537d to 6fb7ab2 Compare November 2, 2022 20:30

allenisalai force-pushed the psql-schema-manager-error branch from 6fb7ab2 to 6c8aee7 Compare November 23, 2022 19:34

derrabus reviewed Dec 12, 2022

View reviewed changes

tests/Functional/Schema/PostgreSQLSchemaManagerTest.php Outdated Show resolved Hide resolved

allenisalai force-pushed the psql-schema-manager-error branch 2 times, most recently from f183fcd to 6ddef91 Compare December 27, 2022 19:37

allenisalai force-pushed the psql-schema-manager-error branch from 6ddef91 to 0351a09 Compare January 16, 2023 17:49

derrabus requested changes Jan 28, 2023

View reviewed changes

allenisalai force-pushed the psql-schema-manager-error branch 2 times, most recently from e97e9f9 to 7fd18a5 Compare February 1, 2023 00:44

derrabus added the Requires Feedback label Feb 2, 2023

derrabus changed the base branch from 3.5.x to 3.6.x February 7, 2023 23:07

allenisalai force-pushed the psql-schema-manager-error branch from 7fd18a5 to 2cd23b6 Compare February 16, 2023 22:28

derrabus requested changes Feb 16, 2023

View reviewed changes

allenisalai force-pushed the psql-schema-manager-error branch from 2cd23b6 to 27996ed Compare February 17, 2023 00:46

derrabus reviewed Feb 17, 2023

View reviewed changes

allenisalai force-pushed the psql-schema-manager-error branch from 27996ed to b35b52c Compare February 22, 2023 03:12

derrabus approved these changes Mar 2, 2023

View reviewed changes

derrabus removed the Requires Feedback label Mar 2, 2023

derrabus added this to the 3.6.1 milestone Mar 2, 2023

derrabus merged commit 974a5ab into doctrine:3.6.x Mar 2, 2023

allenisalai deleted the psql-schema-manager-error branch March 2, 2023 19:39

derrabus mentioned this pull request Mar 6, 2023

Skip test on Postgres 10/11 #5955

Merged

derrabus reviewed Mar 6, 2023

View reviewed changes

github-actions bot locked as resolved and limited conversation to collaborators Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure the pg_depend relation is for a table object #5800

Ensure the pg_depend relation is for a table object #5800

allenisalai commented Oct 25, 2022 •

edited

Loading

derrabus commented Oct 25, 2022

allenisalai commented Oct 25, 2022

derrabus commented Oct 27, 2022

allenisalai commented Nov 2, 2022

allenisalai commented Nov 23, 2022

klaussilveira commented Jan 25, 2023

derrabus commented Jan 28, 2023

derrabus Jan 28, 2023

allenisalai Feb 1, 2023

derrabus Feb 1, 2023

derrabus Jan 28, 2023

allenisalai Feb 1, 2023

derrabus Feb 1, 2023

allenisalai Feb 16, 2023

allenisalai commented Feb 16, 2023

derrabus Feb 16, 2023

allenisalai Feb 17, 2023

derrabus commented Feb 16, 2023

derrabus Feb 17, 2023

allenisalai Feb 22, 2023

allenisalai commented Mar 2, 2023

derrabus Mar 6, 2023

allenisalai Mar 7, 2023

derrabus Mar 7, 2023

	$semanticVersion = $this->connection->fetchOne('SHOW SERVER_VERSION');
	$semanticVersion = $this->connection->getServerVersion();

Ensure the pg_depend relation is for a table object #5800

Ensure the pg_depend relation is for a table object #5800

Conversation

allenisalai commented Oct 25, 2022 • edited Loading

Summary

derrabus commented Oct 25, 2022

allenisalai commented Oct 25, 2022

derrabus commented Oct 27, 2022

allenisalai commented Nov 2, 2022

allenisalai commented Nov 23, 2022

klaussilveira commented Jan 25, 2023

derrabus commented Jan 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenisalai commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenisalai commented Mar 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenisalai commented Oct 25, 2022 •

edited

Loading