Skip to content

ext/pdo_pgsql: Retrieve the memory usage of the query result resource #14260

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

ext/pdo_pgsql: Retrieve the memory usage of the query result resource #14260

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0aaeae6
ext/pdo_pgsql: Retrieve the memory usage of the query result resource
KentarouTakeda May 15, 2024
a420d3a
fix: typo
KentarouTakeda May 17, 2024
b5769e6
impl: subclass stub and their arginfo
KentarouTakeda May 18, 2024
da6b54b
impl: Notify statements that have not been executed using `errorInfo()`.
KentarouTakeda May 19, 2024
c58707d
impl: add a statement name to `errorInfo()`
KentarouTakeda May 19, 2024
37e203e
fix: constant declaration location
KentarouTakeda May 19, 2024
218ff1e
fix: freeing the heap after use
KentarouTakeda May 19, 2024
8701022
fix: unnecessary extension declaration
KentarouTakeda May 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ext/pdo_pgsql/config.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ if test "$PHP_PDO_PGSQL" != "no"; then
LDFLAGS="-L$PGSQL_LIBDIR $LDFLAGS"

AC_CHECK_LIB(pq, PQlibVersion,, AC_MSG_ERROR([Unable to build the PDO PostgreSQL driver: at least libpq 9.1 is required]))
AC_CHECK_LIB(pq, PQresultMemorySize, AC_DEFINE(HAVE_PG_RESULT_MEMORY_SIZE,1,[PostgreSQL 12 or later]))

LIBS=$old_LIBS
LDFLAGS=$old_LDFLAGS
Expand Down
3 changes: 3 additions & 0 deletions ext/pdo_pgsql/pdo_pgsql.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@ PHP_MINIT_FUNCTION(pdo_pgsql)
REGISTER_PDO_CLASS_CONST_LONG("PGSQL_TRANSACTION_INTRANS", (zend_long)PGSQL_TRANSACTION_INTRANS);
REGISTER_PDO_CLASS_CONST_LONG("PGSQL_TRANSACTION_INERROR", (zend_long)PGSQL_TRANSACTION_INERROR);
REGISTER_PDO_CLASS_CONST_LONG("PGSQL_TRANSACTION_UNKNOWN", (zend_long)PGSQL_TRANSACTION_UNKNOWN);
#ifdef HAVE_PG_RESULT_MEMORY_SIZE
REGISTER_PDO_CLASS_CONST_LONG("PGSQL_ATTR_RESULT_MEMORY_SIZE", (zend_long)PDO_PGSQL_ATTR_RESULT_MEMORY_SIZE);
#endif
Comment on lines +159 to +161
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I should write this down.

The purpose of subclasses is to allow driver specific functionality to be provided in the subclasses.

This statement would mean having the same constants in PDO core, which may go against the subclassing philosophy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, since PDO::getAttribute is not a subclass-specific method, I'm not sure what to do.


PdoPgsql_ce = register_class_PdoPgsql(pdo_dbh_ce);
PdoPgsql_ce->create_object = pdo_dbh_new;
Expand Down
5 changes: 5 additions & 0 deletions ext/pdo_pgsql/pdo_pgsql.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ class PdoPgsql extends PDO
/** @cvalue PDO_PGSQL_ATTR_DISABLE_PREPARES */
public const int ATTR_DISABLE_PREPARES = UNKNOWN;

#ifdef HAVE_PG_RESULT_MEMORY_SIZE
/** @cvalue PDO_PGSQL_ATTR_RESULT_MEMORY_SIZE */
public const int ATTR_RESULT_MEMORY_SIZE = UNKNOWN;
#endif

/** @cvalue PGSQL_TRANSACTION_IDLE */
public const int TRANSACTION_IDLE = UNKNOWN;

Expand Down
10 changes: 9 additions & 1 deletion ext/pdo_pgsql/pdo_pgsql_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 25 additions & 1 deletion ext/pdo_pgsql/pgsql_statement.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,30 @@ static int pdo_pgsql_stmt_cursor_closer(pdo_stmt_t *stmt)
return 1;
}

static int pgsql_stmt_get_attr(pdo_stmt_t *stmt, zend_long attr, zval *val)
{
pdo_pgsql_stmt *S = (pdo_pgsql_stmt*)stmt->driver_data;

switch (attr) {
#ifdef HAVE_PG_RESULT_MEMORY_SIZE
case PDO_PGSQL_ATTR_RESULT_MEMORY_SIZE:
if(stmt->executed) {
ZVAL_LONG(val, PQresultMemorySize(S->result));
} else {
char *tmp;
spprintf(&tmp, 0, "statement '%s' has not been executed yet", S->stmt_name);
Copy link
Member

@devnexen devnexen May 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you re allocating on the heap then you need to free it at some point. If you run your own test you ll see.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right...
I fixed: 218ff1e


pdo_pgsql_error_stmt_msg(stmt, 0, "HY000", tmp);
ZVAL_NULL(val);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know if we should emit a PDO exception in this case, @SakiTakamachi what do you think ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can throw an exception here, because it's obviously wrong to try to get the size of a result when there is no "result" yet.

Copy link
Member

@SakiTakamachi SakiTakamachi May 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devnexen

Ah but mainly these are used for debugging purposes?

In that case, throwing an exception might make the method difficult to use.

If it's unexecuted, does it make sense to interpret this as 0 memory allocation and return 0 (or null)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, I prefer to throw an exception for incorrect operations.

On the other hand, if you fetchAll() a statement that has not yet been executed, [] is returned. If consistency is our priority, it may be better not to throw exceptions.

In both cases, it doesn't take me much time since I have the modified code at hand as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather, it seems like the problem is that fetching an unexecuted result doesn't result in an error....

That's a relic of PDO when exceptions didn't exist, and I don't think it should still be that way if don't take BC into account.

I'll wait for David's opinion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, excuse me. This also has a status....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added pdo_pgsql_error_stmt_msg in da6b54b.

If we should not emit exception, I think the current code is the best.

Copy link
Member

@devnexen devnexen May 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can even be more informative since you have access to the statement's name.
Note that, with the last changes, your new test is skipped due to the new constant now missing.

Copy link
Member

@devnexen devnexen May 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see, it works only if you enable pgsql too (symbol is defined here).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your time! ! !
I fixed:

}
return 1;
#endif

default:
return 0;
}
}

const struct pdo_stmt_methods pgsql_stmt_methods = {
pgsql_stmt_dtor,
pgsql_stmt_execute,
Expand All @@ -713,7 +737,7 @@ const struct pdo_stmt_methods pgsql_stmt_methods = {
pgsql_stmt_get_col,
pgsql_stmt_param_hook,
NULL, /* set_attr */
NULL, /* get_attr */
pgsql_stmt_get_attr,
pgsql_stmt_get_column_meta,
NULL, /* next_rowset */
pdo_pgsql_stmt_cursor_closer
Expand Down
1 change: 1 addition & 0 deletions ext/pdo_pgsql/php_pdo_pgsql_int.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ extern const struct pdo_stmt_methods pgsql_stmt_methods;

enum {
PDO_PGSQL_ATTR_DISABLE_PREPARES = PDO_ATTR_DRIVER_SPECIFIC,
PDO_PGSQL_ATTR_RESULT_MEMORY_SIZE,
};

struct pdo_pgsql_lob_self {
Expand Down
46 changes: 46 additions & 0 deletions ext/pdo_pgsql/tests/result_memory_size.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
--TEST--
PDO PgSQL PDOStatement::getAttribute(PDO::PGSQL_ATTR_RESULT_MEMORY_SIZE)
--EXTENSIONS--
pdo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This statement is unnecessary because pdo is always loaded when pdo_pgsql is loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I fixed: 8701022

pdo_pgsql
--SKIPIF--
<?php
require __DIR__ . '/config.inc';
require dirname(__DIR__, 2) . '/pdo/tests/pdo_test.inc';
PDOTest::skip();
if (!defined('PDO::PGSQL_ATTR_RESULT_MEMORY_SIZE')) die('skip constant PDO::PGSQL_ATTR_RESULT_MEMORY_SIZE does not exist');
--FILE--
<?php

require_once __DIR__ . "/config.inc";

/** @var Pdo */
$db = Pdo::connect($config['ENV']['PDOTEST_DSN']);

echo 'Result set with only 1 row: ';
$statement = $db->query('select 1');
$result_1 = $statement->getAttribute(PDO::PGSQL_ATTR_RESULT_MEMORY_SIZE);
var_dump($result_1);

echo 'Result set with many rows: ';
$result = $db->query('select generate_series(1, 10000)');
$result_2 = $result->getAttribute(PDO::PGSQL_ATTR_RESULT_MEMORY_SIZE);
var_dump($result_2);

echo 'Large result sets should require more memory than small ones: ';
var_dump($result_2 > $result_1);

echo 'Statements that are not executed should not consume memory: ';
$statement = $db->prepare('select 1');
$result_3 = $statement->getAttribute(PDO::PGSQL_ATTR_RESULT_MEMORY_SIZE);
var_dump($result_3);

echo 'and should emit Error: ';
printf("%s: %d: %s\n", ...$statement->errorInfo());

--EXPECTF--
Result set with only 1 row: int(%d)
Result set with many rows: int(%d)
Large result sets should require more memory than small ones: bool(true)
Statements that are not executed should not consume memory: NULL
and should emit Error: HY000: 0: statement '%s' has not been executed yet
Loading