Skip to content
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.

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

Fix use of prepared statement in async module #5214

Merged

Conversation

erimatnor
Copy link
Contributor

@erimatnor erimatnor commented Jan 24, 2023

Broken code caused the async connection module to never send queries
using prepared statements. Instead, queries were sent using the
parameterized query statement instead.

The regression was introduced a long time ago in commit 86858e3

Fix this so that prepared statements are used when created.

@erimatnor erimatnor self-assigned this Jan 24, 2023
@erimatnor erimatnor force-pushed the fix-use-of-async-prepared-statement branch from af91ef1 to 2144aa3 Compare January 24, 2023 17:27
@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Merging #5214 (da1d5b8) into main (1a3e7ad) will decrease coverage by 0.01%.
The diff coverage is 82.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5214      +/-   ##
==========================================
- Coverage   89.01%   89.01%   -0.01%     
==========================================
  Files         225      225              
  Lines       51786    51823      +37     
==========================================
+ Hits        46099    46131      +32     
- Misses       5687     5692       +5     
Impacted Files Coverage Δ
tsl/src/remote/async.c 78.76% <82.92%> (+0.33%) ⬆️
src/bgw/job_stat.c 91.87% <0.00%> (-0.32%) ⬇️
src/loader/loader.c 94.38% <0.00%> (-0.26%) ⬇️
src/bgw/scheduler.c 83.63% <0.00%> (-0.26%) ⬇️
tsl/src/bgw_policy/job.c 87.45% <0.00%> (-0.05%) ⬇️
src/planner/expand_hypertable.c 94.24% <0.00%> (+<0.01%) ⬆️
src/planner/planner.c 95.90% <0.00%> (+0.01%) ⬆️
tsl/src/fdw/data_node_scan_plan.c 97.15% <0.00%> (+0.02%) ⬆️
tsl/src/data_node.c 95.84% <0.00%> (+0.02%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a3e7ad...da1d5b8. Read the comment docs.

@@ -162,6 +164,16 @@ async_request_send_internal(AsyncRequest *req, int elevel)
remote_connection_configure_if_changed(req->conn);

if (req->stmt_name)
{
ret = PQsendQueryPrepared(remote_connection_get_pg_conn(req->conn),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PQsendQueryPrepared() didn't exist in our code, so prepared statements were never executed.

@erimatnor erimatnor marked this pull request as ready for review January 25, 2023 09:50
Copy link
Contributor

@pmwkaa pmwkaa left a comment

Choose a reason for hiding this comment

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

Is there a way to test it?

@erimatnor
Copy link
Contributor Author

Is there a way to test it?

It is already tested by existing tests. You can see in CodeCov that the code path for the new libpq call is covered (except for some error handling cases that in practice don't happen).

tsl/src/remote/async.c Outdated Show resolved Hide resolved
@erimatnor erimatnor force-pushed the fix-use-of-async-prepared-statement branch from dbf451c to 3731a5b Compare January 30, 2023 19:30
@erimatnor erimatnor requested review from jnidzwetzki and removed request for 65278 January 30, 2023 19:31
@erimatnor erimatnor force-pushed the fix-use-of-async-prepared-statement branch from 3731a5b to ec24cd9 Compare January 31, 2023 08:29
Copy link
Contributor

@jnidzwetzki jnidzwetzki left a comment

Choose a reason for hiding this comment

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

I added one comment, the rest looks good to me.

tsl/src/remote/async.c Outdated Show resolved Hide resolved
Broken code caused the async connection module to never send queries
using prepared statements. Instead, queries were sent using the
parameterized query statement instead.

Fix this so that prepared statements are used when created.
@erimatnor erimatnor force-pushed the fix-use-of-async-prepared-statement branch from ec24cd9 to da1d5b8 Compare January 31, 2023 09:15
@erimatnor erimatnor merged commit d489ed6 into timescale:main Jan 31, 2023
@erimatnor erimatnor deleted the fix-use-of-async-prepared-statement branch January 31, 2023 11:01
@mahipv mahipv mentioned this pull request Feb 14, 2023
mahipv added a commit that referenced this pull request Feb 20, 2023
This release contains new features and bug fixes since the 2.9.3
release.

This release is high priority for upgrade. We strongly recommend that
you upgrade as soon as possible.

**Features**
* #5241 Allow RETURNING clause when inserting into compressed chunks
* #5245 Manage life-cycle of connections via memory contexts
* #5246 Make connection establishment interruptible
* #5253 Make data node command execution interruptible
* #5243 Enable real-time aggregation for continuous aggregates with
joins
* #5262 Extend enabling compression on a continuous aggregrate with
'compress_segmentby' and 'compress_orderby' parameters

**Bugfixes**
* #4926 Fix corruption when inserting into compressed chunks
* #5218 Add role-level security to job error log
* #5214 Fix use of prepared statement in async module
* #5290 Compression can't be enabled on continuous aggregates when
segmentby/orderby columns need quotation
* #5239 Fix next_start calculation for fixed schedules
mahipv pushed a commit that referenced this pull request Feb 21, 2023
This release contains new features and bug fixes since the 2.9.3 release.
We deem it moderate priority for upgrading.

This release includes these noteworthy features:
* Joins in continuous aggregates
* Re-architecture of how compression works: ~2x improvement on INSERT rate into compressed chunks.
* Full PostgreSQL 15 support for all existing features. Support for the newly introduced MERGE command on hypertables will be introduced on a follow-up release.

**PostgreSQL 12 deprecation announcement**
We will continue supporting PostgreSQL 12 until July 2023. Sooner to that time, we will announce the specific version of TimescaleDB in which PostgreSQL 12 support will not be included going forward.

**Old format of Continuous Aggregates deprecation announcement**
TimescaleDB 2.7 introduced a new format for continuous aggregates that improves performance.
All instances with Continuous Aggregates using the old format should [migrate to the new format](https://docs.timescale.com/api/latest/continuous-aggregates/cagg_migrate/) by July 2023,
when support for the old format will be removed.
Sooner to that time, we will announce the specific version of TimescaleDB in which support for this feature will not be included going forward.

**Features**
* #4874 Allow joins in continuous aggregates
* #4926 Refactor INSERT into compressed chunks
* #5241 Allow RETURNING clause when inserting into compressed chunks
* #5245 Manage life-cycle of connections via memory contexts
* #5246 Make connection establishment interruptible
* #5253 Make data node command execution interruptible
* #5262 Extend enabling compression on a continuous aggregrate with 'compress_segmentby' and 'compress_orderby' parameters

**Bugfixes**
* #5214 Fix use of prepared statement in async module
* #5218 Add role-level security to job error log
* #5239 Fix next_start calculation for fixed schedules
* #5290 Fix enabling compression on continuous aggregates with columns requiring quotation

**Thanks**
* @henriquegelio for reporting the issue on fixed schedules
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants