Skip to content

Fix pg instrumentation raising ArgumentError when params is omitted#6020

Merged
TonyCTHsu merged 4 commits into
masterfrom
tonycthsu/repro-pg-exec-prepared-arity
Jul 14, 2026
Merged

Fix pg instrumentation raising ArgumentError when params is omitted#6020
TonyCTHsu merged 4 commits into
masterfrom
tonycthsu/repro-pg-exec-prepared-arity

Conversation

@TonyCTHsu

@TonyCTHsu TonyCTHsu commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What does this PR do?
Fixes the pg integration's instrumentation wrapper so that params is optional on exec_params, exec_prepared, and their async_/sync_ variants, matching native pg's own method signatures.

Motivation:
The instrumentation wraps these methods with a mandatory params positional argument, but native pg treats params as optional on all of them (e.g. conn.exec_prepared(statement_name) is valid). Once the pg integration is instrumented, calling any of these methods with only the required arguments raises ArgumentError: wrong number of arguments (given 1, expected 2+), even though the same call succeeds with plain pg.

Change log entry
Yes. Fixed an issue where pg instrumentation raised ArgumentError when calling exec_params, exec_prepared, or their async_/sync_ variants without a params argument.

Additional Notes:
All six affected methods (exec, exec_params, exec_prepared, and their async_/sync_ variants) are defined in native pg as C functions with variadic arity (rb_define_method(..., -1)), meaning pg itself performs argument-count validation internally rather than via a fixed Ruby method signature. This was confirmed empirically: calling conn.exec_params('SELECT 1') and conn.exec_prepared('stmt') (both omitting params) succeed against plain pg (verified on pg gem 1.5.9), matching the documented signatures at https://rubydoc.info/gems/pg/PG/Connection#exec_prepared-instance_method and https://rubydoc.info/gems/pg/PG/Connection#exec_params-instance_method, which show params as optional (defaulting to no bind parameters) for both.

How to test the change?
Added regression tests covering calls without params for exec_params, exec_prepared, async_exec_params, async_exec_prepared, sync_exec_params, and sync_exec_prepared in spec/datadog/tracing/contrib/pg/patcher_spec.rb. These tests fail with the reported ArgumentError against the prior code and pass with the fix.

TonyCTHsu added 3 commits July 10, 2026 10:40
@TonyCTHsu
Add regression test for exec_prepared without params
1a2c156 
pg instrumentation makes exec_prepared's params argument mandatory,
raising ArgumentError for calls like exec_prepared(statement_name)
that work fine with plain pg.
@TonyCTHsu
Extend regression tests to exec_params, async/sync variants
1b76c3a 
The same mandatory-params arity bug affects exec_params,
async_exec_params, sync_exec_params, async_exec_prepared, and
sync_exec_prepared, not just exec_prepared, since native pg treats
params as optional across all of them.
@TonyCTHsu
Fix exec_params/exec_prepared and variants to make params optional
91dbb4f 
Forward params via *args instead of a mandatory positional parameter,
matching native pg's arity where params defaults to nil/none.
@TonyCTHsu
TonyCTHsu requested review from a team as code owners July 10, 2026 09:02
@TonyCTHsu TonyCTHsu added the AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos label Jul 10, 2026
@TonyCTHsu
TonyCTHsu requested a review from marcotc July 10, 2026 09:02
@TonyCTHsu TonyCTHsu added the AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos label Jul 10, 2026
@TonyCTHsu
TonyCTHsu requested a review from vpellan July 10, 2026 09:02
@dd-octo-sts dd-octo-sts Bot added integrations Involves tracing integrations tracing dbm Database Monitoring product labels Jul 10, 2026
@datadog-datadog-prod-us1

This comment has been minimized.

Sign in to view
@pr-commenter

pr-commenter Bot commented Jul 10, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-07-10 09:27:35

Comparing candidate commit 91dbb4f in PR branch tonycthsu/repro-pg-exec-prepared-arity with baseline commit 3755d93 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

@TonyCTHsu TonyCTHsu modified the milestones: 1.23.4, 2.38.0 Jul 14, 2026
@eregon

eregon commented Jul 14, 2026

Copy link
Copy Markdown
Member

but native pg treats params as optional on all of them (e.g. conn.exec_prepared(statement_name) is valid)

@TonyCTHsu Would you have a link for that so it's easy to see?

Also I wonder if any of these methods can take keyword arguments.
If they do we are delegating incorrectly and losing the keyword arguments (passing them as positional).

@eregon

eregon commented Jul 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

https://rubydoc.info/gems/pg/PG/Connection#exec_prepared-instance_method and https://rubydoc.info/gems/pg/PG/Connection#exec_params-instance_method, which show params as optional (defaulting to no bind parameters) for both.

Actually the second shows #exec_params(sql, params[, result_format [, type_map ]]) ⇒ nil which looks required but:

  • Regardless, for forwarding it's best to only capture the arguments we need
  • The C function still accepts argc == 1 with a deprecation warning

@TonyCTHsu
TonyCTHsu merged commit b632c43 into master Jul 14, 2026
52 of 54 checks passed
@TonyCTHsu
TonyCTHsu deleted the tonycthsu/repro-pg-exec-prepared-arity branch July 14, 2026 14:15
@TonyCTHsu TonyCTHsu mentioned this pull request Jul 17, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eregon eregon eregon approved these changes
@marcotc marcotc Awaiting requested review from marcotc marcotc is a code owner automatically assigned from DataDog/apm-idm-ruby
@vpellan vpellan Awaiting requested review from vpellan vpellan is a code owner automatically assigned from DataDog/apm-sdk-capabilities-ruby

Assignees

No one assigned

Labels

AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos dbm Database Monitoring product integrations Involves tracing integrations tracing

Projects

None yet

Milestone

2.38.0

Development

Successfully merging this pull request may close these issues.

2 participants

@TonyCTHsu @eregon