fix bug 81343: inconsistent type conversion after closeCursor

[pilif]

S->cols is already freed in the statement destructor and since caa7100 the column data is only populated on the first execute() which means that on subsequent execute()s after closeCursor was called, all meta-data for column types was removed and never restored, so any possible data type conversion was not happening any more.

[pilif]

as I said on the bug report, I would love to get @nikic's 👀 on this one as he did the big PDO result refactoring and as I'm not 100% sure it's safe to not free the column information on closeCursor, though I'm pretty sure it is freed as part of the handle destructor.

The other approach would be to restore the freeing of the column metadata and call describe again on subsequent execs, though I really don't see a way how a once prepared statement could have different column types in subsequent executions, so I really think the approach of the patch is fine.

[cmb69]

Don't we need to PQclear(S->result) in pdo_pgsql_stmt_cursor_closer()?

[pilif]

I don't know. It wasn't happening prior to caa7100 either. I just checked PHP-8.0 HEAD

[nikic]

Does pgsql have any notion of multiple result sets (where each result set might have different columns)?

[nikic]

AppVeyor fails with:

========DIFF========
001+ Memory Leak Detected: 446 != 442
     done
========DONE========
FAIL PDO PgSQL Bug #69752 (memory leak with closeCursor) [C:\projects\php-src\ext\pdo_pgsql\tests\bug69752.phpt] 

I haven't seen this kind of message before ... where does it come from?

[pilif]

it's hard-coded in ext/pdo_pgsql/tests/bug69752.phpt, but I think the test is not quite correct: yes, the metadata persists after closeCursor, but it's not growing unbounded (increasing the iteration count in the test does not cause the "leak" to grow)

With regards to result sets: Postgres gained that feature with Postgres 11 recently, but I don't think PDO had or has any way for accessing these.

[cmb69]

That's from https://github.com/php/php-src/blob/master/ext/pdo_pgsql/tests/bug69752.phpt#L49.

[pilif]

but I think the test is not quite correct:

never mind. it is - something is not being freed. I missed the break.

I guess we need to continue to free the column info but instead use describe again.

[pilif]

however, I don't understand where the leak detected by the test is coming from. As far as I can see, memory for cols is only allocated here:

php-src/ext/pdo_pgsql/pgsql_statement.c

Lines 243 to 246 in a4e2068

	if (!stmt->executed && (!stmt->column_count || S->cols == NULL)) {
	stmt->column_count = (int) PQnfields(S->result);
	S->cols = ecalloc(stmt->column_count, sizeof(pdo_pgsql_column));
	}

anyways - I'll find out.

Calling describe on each execute would need to be done through the PDO extension itself which might come at a considerable performance cost for other database drivers.

[pilif]

aha! this also checks for !stmt->column_count, so this leaks only on statements which don't return anything (like the insert in the test).

I'm off for family duties now, but I'll have a correct fix tomorrow Central Europe summer time without describing again.

Sorry for the confusion.

[pilif]

I have added an additional commit that does the ecalloc for the column metadata only dependent on S->cols being set or not. As @nikic correctly pointed out, this is not correct in light of multiple result sets, but the Postgres PDO driver does not support those (next_rowset handler is set to NULL) anyways.

Doing this correctly even with multiple rowsets would require a bit of a rework, but that's probably true for the rest of the driver anyways.

On my machine all ext/pdo_pgsql tests now pass.

[nikic]

It would be good to check how pgsql behaves when you modify the table schema between two executes. This used to crash with MySQL: https://github.com/php/php-src/blob/master/ext/pdo_mysql/tests/change_column_count.phpt I suspect that the way this is currently done in the pgsql driver could also result in a crash...

[pilif]

very good idea. I'll check and I'm quite sure that in the best case it will behave like the bug 81343 and return newly added values as strings and in the worst case, it will crash.

Looking at the PDO framework itself, I think I can make the Postgres driver ask the framework to call pdo_stmt_describe_columns after every execution - that's probably the better solution.

[pilif]

good news: Postgres thought of this and will throw an error if the schema has changed behind a prepared statement's back:

SQLSTATE[0A000]: Feature not supported: 7 ERROR: cached plan must not change result type

however, with PDO::ATTR_EMULATE_PREPARES enabled, things don't crash, but seriously go out of whack, making up data for non-existent columns, but that's also true for current HEAD of PHP-8.0, so I think that's outside of the scope of this PR.

Given the correct handling in case of native prepares and given the unchanged behavior of emulated prepares, I think the current implementation of the PR is fine

[nikic]

I think the whole "describe" functionality in PDO probably needs a redesign, but let's go with this for now to address the immediate problem...

[kamil-tekiela]

@nikic Why does the commit show up as unverified?

[pilif]

That’s because I told GitHub that my commits are signed but this was of course rebased and committed by @nikic but authored by me.

I will update my GitHub config.