ext/pgsql: pg_trace allow to refine its trace mode via 2 new constants. #11041
Conversation
devnexen
commented
Apr 9, 2023
devnexen
commented
- PGSQL_TRACE_SUPPRESS_TIMESTAMPS.
- PGSQL_TRACE_REGRESS_MODE to have a more verbose and observable output to check possible regressions.
ext/pgsql/pgsql.c
Outdated
| PQsetTraceFlags(pgsql, trace_mode); | ||
| } | ||
| #else | ||
| php_error_docref(NULL, E_WARNING, "Trace mode unsupported"); |
There was a problem hiding this comment.
Not sure if this should return false or not if the mode is unssuported :/
ext/pgsql/pgsql.c
Outdated
| if (trace_mode > 0) { | ||
| #ifdef PQTRACE_REGRESS_MODE | ||
| if (trace_mode > (PQTRACE_SUPPRESS_TIMESTAMPS|PQTRACE_REGRESS_MODE)) { | ||
| zend_argument_value_error(2, "must be PGSQL_TRACE_SUPPRESS_TIMESTAMPS and/or PGSQL_TRACE_REGRESS_MODE"); |
There was a problem hiding this comment.
| zend_argument_value_error(2, "must be PGSQL_TRACE_SUPPRESS_TIMESTAMPS and/or PGSQL_TRACE_REGRESS_MODE"); | |
| zend_argument_value_error(4, "must be PGSQL_TRACE_SUPPRESS_TIMESTAMPS and/or PGSQL_TRACE_REGRESS_MODE"); |
Maybe add a test which hits this code path? As it would have indicated you were targeting the wrong arg num
devnexen
force-pushed
the
pg_trace_upd
branch
2 times, most recently
from
0705d93 to
c957f22
Compare
| PQsetTraceFlags(pgsql, trace_mode); | ||
| } | ||
| #else | ||
| zend_argument_value_error(4, "cannot set as trace is unsupported"); |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ext/pgsql/pgsql.c
Outdated
| @@ -2147,6 +2148,17 @@ 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)) { | |||
There was a problem hiding this comment.
This test is a bit odd, why not !(trace_mode & (PQTRACE_SUPPRESS_TIMESTAMPS|PQTRACE_REGRESS_MODE))? This would hide unhandled, smaller flags if they didn't start at 0.
There was a problem hiding this comment.
Does this also need a php_stream_close(stream); as above? Edit: This comment was meant to go below, to the zend_argument_value_error.
| PQsetTraceFlags(pgsql, trace_mode); | ||
| } | ||
| #else | ||
| zend_argument_value_error(4, "cannot set as trace is unsupported"); |
There was a problem hiding this comment.
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.
| 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"); |
There was a problem hiding this comment.
RETURN_THROWS(); is missing here and below. Doesn't matter for now but could be missed if more things are added to the function.
- PGSQL_TRACE_SUPPRESS_TIMESTAMPS. - PGSQL_TRACE_REGRESS_MODE to have a more verbose and observable output to check possible regressions.
