From 10e979173d3485af9d9c434a07c2af58be5dce64 Mon Sep 17 00:00:00 2001 From: Guillaume Outters Date: Fri, 4 Oct 2024 23:14:18 +0200 Subject: [PATCH 1/4] Pdo\Pgsql: optimize calls to foreach(columns of the same table) getColumnMeta() - each call queried the DB to know the name associated with the table's OID: cache the result between two calls - make pdo_pgsql_translate_oid_to_table higher-level, with the last parameter being the handle instead of the raw connection; thus the statement is cleaner, letting the handle do all memory handling on the table oid-to-name translation cache (which by the way is a driver feature more than a statement one) --- ext/pdo_pgsql/pgsql_driver.c | 5 +++++ ext/pdo_pgsql/pgsql_statement.c | 20 ++++++++++++++++---- ext/pdo_pgsql/php_pdo_pgsql_int.h | 2 ++ 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/ext/pdo_pgsql/pgsql_driver.c b/ext/pdo_pgsql/pgsql_driver.c index f29b4ca973cc6..8fdba74e4a36e 100644 --- a/ext/pdo_pgsql/pgsql_driver.c +++ b/ext/pdo_pgsql/pgsql_driver.c @@ -252,6 +252,10 @@ static void pgsql_handle_closer(pdo_dbh_t *dbh) /* {{{ */ PQfinish(H->server); H->server = NULL; } + if (H->cached_table_name) { + efree(H->cached_table_name); + H->cached_table_name = NULL; + } if (H->einfo.errmsg) { pefree(H->einfo.errmsg, dbh->is_persistent); H->einfo.errmsg = NULL; @@ -1445,6 +1449,7 @@ static int pdo_pgsql_handle_factory(pdo_dbh_t *dbh, zval *driver_options) /* {{{ H->attached = 1; H->pgoid = -1; + H->cached_table_oid = InvalidOid; dbh->methods = &pgsql_methods; dbh->alloc_own_columns = 1; diff --git a/ext/pdo_pgsql/pgsql_statement.c b/ext/pdo_pgsql/pgsql_statement.c index 169fa49af4e14..39052bec6802a 100644 --- a/ext/pdo_pgsql/pgsql_statement.c +++ b/ext/pdo_pgsql/pgsql_statement.c @@ -702,12 +702,23 @@ static int pgsql_stmt_get_col(pdo_stmt_t *stmt, int colno, zval *result, enum pd return 1; } -static zend_always_inline char * pdo_pgsql_translate_oid_to_table(Oid oid, PGconn *conn) +static zend_always_inline char * pdo_pgsql_translate_oid_to_table(Oid oid, pdo_pgsql_db_handle *H) { + PGconn *conn = H->server; char *table_name = NULL; PGresult *tmp_res; char *querystr = NULL; + if (oid == H->cached_table_oid) { + return H->cached_table_name; + } + + if (H->cached_table_name) { + efree(H->cached_table_name); + H->cached_table_name = NULL; + H->cached_table_oid = InvalidOid; + } + spprintf(&querystr, 0, "SELECT RELNAME FROM PG_CLASS WHERE OID=%d", oid); if ((tmp_res = PQexec(conn, querystr)) == NULL || PQresultStatus(tmp_res) != PGRES_TUPLES_OK) { @@ -724,6 +735,8 @@ static zend_always_inline char * pdo_pgsql_translate_oid_to_table(Oid oid, PGcon return 0; } + H->cached_table_oid = oid; + H->cached_table_name = estrdup(table_name); table_name = estrdup(table_name); PQclear(tmp_res); @@ -752,10 +765,9 @@ static int pgsql_stmt_get_column_meta(pdo_stmt_t *stmt, zend_long colno, zval *r table_oid = PQftable(S->result, colno); add_assoc_long(return_value, "pgsql:table_oid", table_oid); - table_name = pdo_pgsql_translate_oid_to_table(table_oid, S->H->server); + table_name = pdo_pgsql_translate_oid_to_table(table_oid, S->H); if (table_name) { - add_assoc_string(return_value, "table", table_name); - efree(table_name); + add_assoc_string(return_value, "table", S->H->cached_table_name); } switch (S->cols[colno].pgsql_type) { diff --git a/ext/pdo_pgsql/php_pdo_pgsql_int.h b/ext/pdo_pgsql/php_pdo_pgsql_int.h index da77d01c61ec7..bba92098261de 100644 --- a/ext/pdo_pgsql/php_pdo_pgsql_int.h +++ b/ext/pdo_pgsql/php_pdo_pgsql_int.h @@ -43,6 +43,8 @@ typedef struct { unsigned _reserved:31; pdo_pgsql_error_info einfo; Oid pgoid; + Oid cached_table_oid; + char *cached_table_name; unsigned int stmt_counter; /* The following two variables have the same purpose. Unfortunately we need to keep track of two different attributes having the same effect. */ From d677c52763cc37312e0068f2b367eac2518c3ae9 Mon Sep 17 00:00:00 2001 From: Guillaume Outters Date: Sat, 5 Oct 2024 15:47:14 +0200 Subject: [PATCH 2/4] GH-15750 Pdo\Pgsql with ATTR_PREFETCH = 0: fix getColumnMeta() doing an (internal) query to fetch metadata from the server broke the currently-running query; additionnally, fix the condition to interrupt the previous request (we shall test if *it* was lazy, not if the *new* one will) --- ext/pdo_pgsql/pgsql_statement.c | 12 +++++++++++- ext/pdo_pgsql/tests/gh15287.phpt | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/ext/pdo_pgsql/pgsql_statement.c b/ext/pdo_pgsql/pgsql_statement.c index 39052bec6802a..99d2fc25e1968 100644 --- a/ext/pdo_pgsql/pgsql_statement.c +++ b/ext/pdo_pgsql/pgsql_statement.c @@ -190,7 +190,7 @@ static int pgsql_stmt_execute(pdo_stmt_t *stmt) * and returns a PGRES_FATAL_ERROR when PQgetResult gets called for stmt 2 if DEALLOCATE * was called for stmt 1 inbetween * (maybe it will change with pipeline mode in libpq 14?) */ - if (S->is_unbuffered && H->running_stmt) { + if (H->running_stmt && H->running_stmt->is_unbuffered) { pgsql_stmt_finish(H->running_stmt, FIN_CLOSE); H->running_stmt = NULL; } @@ -713,6 +713,12 @@ static zend_always_inline char * pdo_pgsql_translate_oid_to_table(Oid oid, pdo_p return H->cached_table_name; } + if (H->running_stmt && H->running_stmt->is_unbuffered) { + /* in single-row mode, libpq forbids passing a new query + * while we're still flushing the current one's result */ + return NULL; + } + if (H->cached_table_name) { efree(H->cached_table_name); H->cached_table_name = NULL; @@ -806,6 +812,10 @@ static int pgsql_stmt_get_column_meta(pdo_stmt_t *stmt, zend_long colno, zval *r break; default: /* Fetch metadata from Postgres system catalogue */ + if (S->H->running_stmt && S->H->running_stmt->is_unbuffered) { + /* libpq forbids calling a query while we're still reading the preceding one's */ + break; + } spprintf(&q, 0, "SELECT TYPNAME FROM PG_TYPE WHERE OID=%u", S->cols[colno].pgsql_type); res = PQexec(S->H->server, q); efree(q); diff --git a/ext/pdo_pgsql/tests/gh15287.phpt b/ext/pdo_pgsql/tests/gh15287.phpt index 72bcc44b363b4..37928d4fef039 100644 --- a/ext/pdo_pgsql/tests/gh15287.phpt +++ b/ext/pdo_pgsql/tests/gh15287.phpt @@ -132,6 +132,17 @@ $res = []; while (($re = $stmt->fetch())) $res[] = $re; display($res); $stmt->execute([ 0 ]); $res = []; for ($i = -1; ++$i < 2;) $res[] = $stmt->fetch(); display($res); display($pdo->query("select * from t2")->fetchAll()); + +// Metadata calls the server for some operations (notably table oid-to-name conversion). +// This will break libpq (that forbids a second PQexec before we consumed the first one). +// Instead of either letting libpq return an error, or blindly forbid this call, we expect +// being transparently provided at least attributes which do not require a server roundtrip. +// And good news: column name is one of those "local" attributes. +echo "=== meta ===\n"; +$stmt = $pdo->query("select * from t limit 2"); +echo "Starting with column " . $stmt->getColumnMeta(0)['name'] . ":\n"; +display($stmt->fetchAll()); + ?> --EXPECTF-- === non regression === @@ -181,3 +192,7 @@ multiple calls to the same prepared statement, some interrupted before having re 0 1 678 ok +=== meta === +Starting with column n: +0 original +1 non original From 5c6f33d8f7015a1fbc6dabea32fa56be7897aedd Mon Sep 17 00:00:00 2001 From: Guillaume Outters Date: Sun, 6 Oct 2024 15:17:03 +0200 Subject: [PATCH 3/4] GH-15750 Pdo\Pgsql with ATTR_PREFETCH = 0: handle handle's internal queries not only the statements, but the driver, love to make internal queries. We make sure no unfinished query still runs when having to pass an internal one. by the way factorize the loops that consumed the preceding query's results --- ext/pdo_pgsql/pgsql_driver.c | 53 ++++++++++++++++++++++++------- ext/pdo_pgsql/pgsql_statement.c | 10 +++--- ext/pdo_pgsql/php_pdo_pgsql_int.h | 6 ++++ 3 files changed, 52 insertions(+), 17 deletions(-) diff --git a/ext/pdo_pgsql/pgsql_driver.c b/ext/pdo_pgsql/pgsql_driver.c index 8fdba74e4a36e..3cd79932121ea 100644 --- a/ext/pdo_pgsql/pgsql_driver.c +++ b/ext/pdo_pgsql/pgsql_driver.c @@ -36,6 +36,7 @@ #include "pgsql_driver_arginfo.h" static bool pgsql_handle_in_transaction(pdo_dbh_t *dbh); +void pgsql_stmt_finish(pdo_pgsql_stmt *S, int fin_mode); static char * _pdo_pgsql_trim_message(const char *message, int persistent) { @@ -103,6 +104,37 @@ int _pdo_pgsql_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt, int errcode, const char * } /* }}} */ +static zend_always_inline void pgsql_finish_running_stmt(pdo_pgsql_db_handle *H) +{ + if (H->running_stmt) { + pgsql_stmt_finish(H->running_stmt, 0); + } +} + +static zend_always_inline void pgsql_discard_running_stmt(pdo_pgsql_db_handle *H) +{ + if (H->running_stmt) { + pgsql_stmt_finish(H->running_stmt, FIN_DISCARD); + } + + PGresult *pgsql_result; + bool first = true; + while ((pgsql_result = PQgetResult(H->server))) { + /* We should not arrive here, where libpq has a result to deliver without us + * having registered a running statement: + * every result discarding should go through the unified pgsql_stmt_finish, + * but maybe there still is an internal query that we omitted to adapt. + * So instead of asserting let's just emit an informational notice, + * and consume anyway (results consumption is handle-wise, so we have no formal + * need for the statement). */ + if (first) { + php_error_docref("ref.pgsql", E_NOTICE, "Internal error: unable to link a libpq result to consume, to its origin statement"); + first = false; + } + PQclear(pgsql_result); + } +} + static void _pdo_pgsql_notice(void *context, const char *message) /* {{{ */ { pdo_dbh_t * dbh = (pdo_dbh_t *)context; @@ -349,6 +381,7 @@ static zend_long pgsql_handle_doer(pdo_dbh_t *dbh, const zend_string *sql) bool in_trans = pgsql_handle_in_transaction(dbh); + pgsql_finish_running_stmt(H); if (!(res = PQexec(H->server, ZSTR_VAL(sql)))) { /* fatal error */ pdo_pgsql_error(dbh, PGRES_FATAL_ERROR, NULL); @@ -416,6 +449,7 @@ static zend_string *pdo_pgsql_last_insert_id(pdo_dbh_t *dbh, const zend_string * PGresult *res; ExecStatusType status; + pgsql_finish_running_stmt(H); if (name == NULL) { res = PQexec(H->server, "SELECT LASTVAL()"); } else { @@ -579,6 +613,7 @@ static bool pdo_pgsql_transaction_cmd(const char *cmd, pdo_dbh_t *dbh) PGresult *res; bool ret = true; + pgsql_finish_running_stmt(H); res = PQexec(H->server, cmd); if (PQresultStatus(res) != PGRES_COMMAND_OK) { @@ -684,9 +719,8 @@ void pgsqlCopyFromArray_internal(INTERNAL_FUNCTION_PARAMETERS) /* Obtain db Handle */ H = (pdo_pgsql_db_handle *)dbh->driver_data; - while ((pgsql_result = PQgetResult(H->server))) { - PQclear(pgsql_result); - } + pgsql_discard_running_stmt(H); + pgsql_result = PQexec(H->server, query); efree(query); @@ -808,9 +842,8 @@ void pgsqlCopyFromFile_internal(INTERNAL_FUNCTION_PARAMETERS) H = (pdo_pgsql_db_handle *)dbh->driver_data; - while ((pgsql_result = PQgetResult(H->server))) { - PQclear(pgsql_result); - } + pgsql_discard_running_stmt(H); + pgsql_result = PQexec(H->server, query); efree(query); @@ -904,9 +937,7 @@ void pgsqlCopyToFile_internal(INTERNAL_FUNCTION_PARAMETERS) RETURN_FALSE; } - while ((pgsql_result = PQgetResult(H->server))) { - PQclear(pgsql_result); - } + pgsql_discard_running_stmt(H); /* using pre-9.0 syntax as PDO_pgsql is 7.4+ compatible */ if (pg_fields) { @@ -995,9 +1026,7 @@ void pgsqlCopyToArray_internal(INTERNAL_FUNCTION_PARAMETERS) H = (pdo_pgsql_db_handle *)dbh->driver_data; - while ((pgsql_result = PQgetResult(H->server))) { - PQclear(pgsql_result); - } + pgsql_discard_running_stmt(H); /* using pre-9.0 syntax as PDO_pgsql is 7.4+ compatible */ if (pg_fields) { diff --git a/ext/pdo_pgsql/pgsql_statement.c b/ext/pdo_pgsql/pgsql_statement.c index 99d2fc25e1968..1d1b29b304053 100644 --- a/ext/pdo_pgsql/pgsql_statement.c +++ b/ext/pdo_pgsql/pgsql_statement.c @@ -56,14 +56,14 @@ #define FLOAT8LABEL "float8" #define FLOAT8OID 701 -#define FIN_DISCARD 0x1 -#define FIN_CLOSE 0x2 -#define FIN_ABORT 0x4 - -static void pgsql_stmt_finish(pdo_pgsql_stmt *S, int fin_mode) +void pgsql_stmt_finish(pdo_pgsql_stmt *S, int fin_mode) { + if (!S) { + return; + } + pdo_pgsql_db_handle *H = S->H; if (S->is_running_unbuffered && S->result && (fin_mode & FIN_ABORT)) { diff --git a/ext/pdo_pgsql/php_pdo_pgsql_int.h b/ext/pdo_pgsql/php_pdo_pgsql_int.h index bba92098261de..7bb493becede2 100644 --- a/ext/pdo_pgsql/php_pdo_pgsql_int.h +++ b/ext/pdo_pgsql/php_pdo_pgsql_int.h @@ -95,6 +95,12 @@ extern int _pdo_pgsql_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt, int errcode, const extern const struct pdo_stmt_methods pgsql_stmt_methods; +#define FIN_DISCARD 0x1 +#define FIN_CLOSE 0x2 +#define FIN_ABORT 0x4 + +extern void pgsql_stmt_finish(pdo_pgsql_stmt *S, int fin_mode); + #define pdo_pgsql_sqlstate(r) PQresultErrorField(r, PG_DIAG_SQLSTATE) enum { From 75fd500faf3e1983f2fb19d6765cba3684550cf2 Mon Sep 17 00:00:00 2001 From: Guillaume Outters Date: Mon, 21 Oct 2024 00:39:14 +0200 Subject: [PATCH 4/4] fix Pdo\Pgsql running_stmt not being cleared we unconditionally set it, but conditionally unset it, letting a dangling pointer that was tentatively freed a second time --- ext/pdo_pgsql/pgsql_statement.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ext/pdo_pgsql/pgsql_statement.c b/ext/pdo_pgsql/pgsql_statement.c index 1d1b29b304053..73d72c989a425 100644 --- a/ext/pdo_pgsql/pgsql_statement.c +++ b/ext/pdo_pgsql/pgsql_statement.c @@ -113,9 +113,10 @@ void pgsql_stmt_finish(pdo_pgsql_stmt *S, int fin_mode) } S->is_prepared = false; - if (H->running_stmt == S) { - H->running_stmt = NULL; - } + } + + if (H->running_stmt == S && (fin_mode & (FIN_CLOSE|FIN_ABORT))) { + H->running_stmt = NULL; } } @@ -192,7 +193,6 @@ static int pgsql_stmt_execute(pdo_stmt_t *stmt) * (maybe it will change with pipeline mode in libpq 14?) */ if (H->running_stmt && H->running_stmt->is_unbuffered) { pgsql_stmt_finish(H->running_stmt, FIN_CLOSE); - H->running_stmt = NULL; } /* ensure that we free any previous unfetched results */ pgsql_stmt_finish(S, 0);