Skip to content

ext/pgsql: pg_trace allow to refine its trace mode via 2 new constants. #11041

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 2 commits into from
Closed

ext/pgsql: pg_trace allow to refine its trace mode via 2 new constants. #11041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c4ed31
ext/pgsql: pg_trace allow to refine its trace mode via 2 new constants.
devnexen Apr 9, 2023
1dfb14c
add test
devnexen Apr 9, 2023
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
16 changes: 15 additions & 1 deletion ext/pgsql/pgsql.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2115,13 +2115,14 @@ PHP_FUNCTION(pg_trace)
{
char *z_filename, *mode = "w";
size_t z_filename_len, mode_len;
zend_long trace_mode = 0;
zval *pgsql_link = NULL;
PGconn *pgsql;
FILE *fp = NULL;
php_stream *stream;
pgsql_link_handle *link;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "p|sO!", &z_filename, &z_filename_len, &mode, &mode_len, &pgsql_link, pgsql_link_ce) == FAILURE) {
if (zend_parse_parameters(ZEND_NUM_ARGS(), "p|sO!l", &z_filename, &z_filename_len, &mode, &mode_len, &pgsql_link, pgsql_link_ce, &trace_mode) == FAILURE) {
RETURN_THROWS();
}

Expand All @@ -2147,6 +2148,19 @@ PHP_FUNCTION(pg_trace)
}
php_stream_auto_cleanup(stream);
PQtrace(pgsql, fp);
if (trace_mode > 0) {
#ifdef PQTRACE_REGRESS_MODE
if (!(trace_mode & (PQTRACE_SUPPRESS_TIMESTAMPS|PQTRACE_REGRESS_MODE))) {
zend_argument_value_error(4, "must be PGSQL_TRACE_SUPPRESS_TIMESTAMPS and/or PGSQL_TRACE_REGRESS_MODE");
Copy link
Member

Choose a reason for hiding this comment

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

RETURN_THROWS(); is missing here and below. Doesn't matter for now but could be missed if more things are added to the function.

RETURN_THROWS();
} else {
PQsetTraceFlags(pgsql, trace_mode);
}
#else
zend_argument_value_error(4, "cannot set as trace is unsupported");
Copy link
Member

Choose a reason for hiding this comment

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

@iluuu1994 could I have your opinion on this, is it fine to use a value error, or do you think it should be switched back to an E_WARNING as possibly it can't be known in advance if Postgress supports that mode?

Copy link
Member

Choose a reason for hiding this comment

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

I agree that exceptions should be thrown when errors are unexpected and generally not handled immediately. It's not clear to me whether that's the case here, when you're passing those flags you probably expect the extension to have built with support for it. So an exception doesn't seem wrong at the least.

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact PQsetTraceFlags serving only as formatting the output of the said trace, i did not necessarily want a total failure "just" because of this and let the trace beginning. You may argue then throwing an exception might not be appropriate then.

RETURN_THROWS();
#endif
}
RETURN_TRUE;
}
/* }}} */
Expand Down
16 changes: 15 additions & 1 deletion ext/pgsql/pgsql.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,20 @@
* @cvalue PGSQL_DML_STRING
*/
const PGSQL_DML_STRING = UNKNOWN;
#ifdef PQTRACE_SUPPPRESS_TIMESTAMPS
/**
* @var int
* @cvalue PQTRACE_SUPPRESS_TIMESTAMPS
*/
const PGSQL_TRACE_SUPPRESS_TIMESTAMPS = UNKNOWN;
#endif
#ifdef PQTRACE_REGRESS_MODE
/**
* @var int
* @cvalue PQTRACE_REGRESS_MODE
*/
const PGSQL_TRACE_REGRESS_MODE = UNKNOWN;
#endif

#ifdef LIBPQ_HAS_PIPELINING
/**
Expand Down Expand Up @@ -662,7 +676,7 @@ function pg_last_oid(PgSql\Result $result): string|int|false {}
*/
function pg_getlastoid(PgSql\Result $result): string|int|false {}

function pg_trace(string $filename, string $mode = "w", ?PgSql\Connection $connection = null): bool {}
function pg_trace(string $filename, string $mode = "w", ?PgSql\Connection $connection = null, int $trace_mode = 0): bool {}

function pg_untrace(?PgSql\Connection $connection = null): bool {}

Expand Down
9 changes: 8 additions & 1 deletion ext/pgsql/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: 26 additions & 0 deletions ext/pgsql/tests/pg_trace.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
--TEST--
pg_trace
--EXTENSIONS--
pgsql
--SKIPIF--
<?php include("skipif.inc"); ?>
--FILE--
<?php

include('config.inc');

$db = pg_connect($conn_str);
$tracefile = __DIR__ . '/trace.tmp';

try {
pg_trace($tracefile, 'w', $db, 56432);
} catch (ValueError $e) {
echo $e->getMessage() . PHP_EOL;
}
var_dump(pg_trace($tracefile, 'w', $db, 0));
$res = pg_query($db, 'select 1');

?>
--EXPECT--
pg_trace(): Argument #4 ($trace_mode) cannot set as trace is unsupported
bool(true)