ext/pdo_pgsql: Retrieve the memory usage of the query result resource by KentarouTakeda · Pull Request #14260 · php/php-src

KentarouTakeda · 2024-05-17T13:48:47Z

This implementation is similar to Pull Request #14214.

I have implemented the same functionality in PDOStatement, specifically for PdoPgsql.

By calling:

$statement->getAttribute(PDO::PGSQL_ATTR_RESULT_MEMORY_SIZE);

You can find out the amount of memory used internally by the client library libpq.
This value was not reflected in the memory_get_usage() output, so it was not possible to determine until now.

PDOStatement differs from PgSql\Result in that it may represent a statement that has not yet been executed.
In this case, libpq does not allocate memory either.Therefore, PDO::PGSQL_ATTR_RESULT_MEMORY_SIZE returns null in such cases.

Since this feature did not require an RFC for ext/pgsql, I am wondering if it is also unnecessary for this implementation. Please let me know if an RFC is needed or if there are any internals discussions required.

devnexen · 2024-05-17T14:44:44Z

ext/pdo_pgsql/pgsql_statement.c

+			if(stmt->executed) {
+				ZVAL_LONG(val, PQresultMemorySize(S->result));
+			} else {
+				ZVAL_NULL(val);


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

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.

@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)?

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.

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.

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

I added pdo_pgsql_error_stmt_msg in da6b54b.

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

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.

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

Thank you for your time! ! !
I fixed:

add a statement name to errorInfo()

constant declaration location

SakiTakamachi

You're probably better off putting the constants in a stub for the subclass.

e.g. (firebird):
https://github.com/php/php-src/blob/master/ext/pdo_firebird/pdo_firebird.stub.php

ext/pdo_pgsql/pdo_pgsql.c

ext/pdo_pgsql/php_pdo_pgsql_int.h

KentarouTakeda · 2024-05-18T02:34:57Z

I added a stub and generated arginfo in b5769e6 .

SakiTakamachi · 2024-05-18T06:34:05Z

ext/pdo_pgsql/pdo_pgsql.c

+#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


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.

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

devnexen · 2024-05-19T08:03:24Z

ext/pdo_pgsql/pgsql_statement.c

+				ZVAL_LONG(val, PQresultMemorySize(S->result));
+			} else {
+				char *tmp;
+				spprintf(&tmp, 0, "statement '%s' has not been executed yet", S->stmt_name);


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.

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

devnexen · 2024-05-19T08:59:50Z

Looks ok to me overall, I do not think it needs to me made more complicated than it is. As for returning null vs 0, I think null sends stronger message that the usage is wrong which will lead the user to investigate further via the errorinfo. 0 would have been enough in case of exception raised. But that s just me, so @SakiTakamachi feel free to review and merge it when you see fit.

SakiTakamachi

I've thought again about what's been discussed and agree that the current state looks good.

Regarding whether or not to include constants in pdo core, it seems fine to leave it as it is, i.e. "include" it.

LGMT, There's really just one small nits :)

SakiTakamachi · 2024-05-19T09:51:13Z

ext/pdo_pgsql/tests/result_memory_size.phpt

+--TEST--
+PDO PgSQL PDOStatement::getAttribute(PDO::PGSQL_ATTR_RESULT_MEMORY_SIZE)
+--EXTENSIONS--
+pdo


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

Thanks, I fixed: 8701022

KentarouTakeda · 2024-05-20T00:17:02Z

Your very thorough reviews and valuable advice are greatly appreciated. Thank you both so much.

KentarouTakeda requested review from SakiTakamachi and devnexen as code owners May 17, 2024 13:48

github-actions bot added the Extension: pdo_pgsql label May 17, 2024

KentarouTakeda force-pushed the pgsql-pdo_attr_result_memory_size branch from a0dee6a to 215ef4a Compare May 17, 2024 14:11

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

0aaeae6

KentarouTakeda force-pushed the pgsql-pdo_attr_result_memory_size branch from 215ef4a to 0aaeae6 Compare May 17, 2024 14:12

devnexen reviewed May 17, 2024

View reviewed changes

SakiTakamachi reviewed May 17, 2024

View reviewed changes

ext/pdo_pgsql/pdo_pgsql.c Outdated Show resolved Hide resolved

ext/pdo_pgsql/php_pdo_pgsql_int.h Outdated Show resolved Hide resolved

KentarouTakeda added 2 commits May 17, 2024 23:28

fix: typo

a420d3a

impl: subclass stub and their arginfo

b5769e6

KentarouTakeda requested a review from kocsismate as a code owner May 18, 2024 02:31

SakiTakamachi reviewed May 18, 2024

View reviewed changes

KentarouTakeda added 3 commits May 19, 2024 02:38

impl: Notify statements that have not been executed using errorInfo().

da6b54b

impl: add a statement name to errorInfo()

c58707d

fix: constant declaration location

37e203e

github-actions bot added the Category: Build System label May 19, 2024

devnexen reviewed May 19, 2024

View reviewed changes

fix: freeing the heap after use

218ff1e

SakiTakamachi approved these changes May 19, 2024

View reviewed changes

fix: unnecessary extension declaration

8701022

SakiTakamachi closed this in b7dd3d8 May 20, 2024

KentarouTakeda deleted the pgsql-pdo_attr_result_memory_size branch May 20, 2024 00:17

KentarouTakeda mentioned this pull request Nov 1, 2024

[PHP 8.4] Add manual for PDO::PGSQL_ATTR_RESULT_MEMORY_SIZE php/doc-en#3983

Merged

Conversation

KentarouTakeda commented May 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi May 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devnexen May 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devnexen May 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KentarouTakeda commented May 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devnexen May 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devnexen commented May 19, 2024

Uh oh!

SakiTakamachi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KentarouTakeda commented May 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SakiTakamachi May 18, 2024 •

edited

Loading

devnexen May 19, 2024 •

edited

Loading

devnexen May 19, 2024 •

edited

Loading

SakiTakamachi left a comment •

edited

Loading

devnexen May 19, 2024 •

edited

Loading