Skip to content

Conversation

Kontinuation
Copy link
Member

This patch depends on #70.

The pytest may hang when some of the test fails. This is a workflow run that exhibited this problem: https://github.com/apache/sedona-db/actions/runs/17668086933/job/50213528921

Pytest does not free up resources held by local variables by calling __del__ in time when a test fails. When pytest captured the AssertionError, it effectively adds a reference to the local c object, preventing it from being del-ed when the function ends. This leaves the transaction started by the failed test active when the next test starts running.

We generate test data by dropping a temporary table, inserting data into the temporary table, and finally renaming the temporary table. Dropping the temporary table will be blocked by a lock held by the still-active transaction started by the previously failed test:

postgres=# select pid, query, state, wait_event, wait_event_type, backend_type from pg_stat_activity;
  pid  |                                           query                                            |        state        |     wait_event      | wait_event_type |         backend_type         
-------+--------------------------------------------------------------------------------------------+---------------------+---------------------+-----------------+------------------------------
 99895 |                                                                                            |                     | AutoVacuumMain      | Activity        | autovacuum launcher
 99896 |                                                                                            |                     | LogicalLauncherMain | Activity        | logical replication launcher
 83813 | select pid, query, state, wait_event, wait_event_type, backend_type from pg_stat_activity; | active              |                     |                 | client backend
 47073 | DROP TABLE IF EXISTS "public" . "sjoin_polygon_xtemp"                                      | idle in transaction | ClientRead          | Client          | client backend
 51565 | DROP TABLE IF EXISTS "public" . "sjoin_point_xtemp"                                        | active              | relation            | Lock            | client backend
 66330 | DROP TABLE IF EXISTS "public" . "sjoin_point_xtemp"                                        | active              | relation            | Lock            | client backend
 71494 | DROP TABLE IF EXISTS "public" . "sjoin_point_xtemp"                                        | active              | relation            | Lock            | client backend
 99892 |                                                                                            |                     | BgWriterHibernate   | Activity        | background writer
 99891 |                                                                                            |                     | CheckpointerMain    | Activity        | checkpointer
 99894 |                                                                                            |                     | WalWriterMain       | Activity        | walwriter

This patch fixes this problem by managing connections using with statement. The transaction will be committed or rolled back when the connection is closed. This prevents us from leaking transactions and blocking ourselves on table locks.

@Kontinuation Kontinuation marked this pull request as ready for review September 12, 2025 13:30
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a critical issue where pytest hangs when tests fail due to PostgreSQL connection transactions not being properly closed. The fix involves wrapping database engine connections with context managers to ensure proper resource cleanup and also includes improvements to synthetic geometry generation for more diverse test data.

  • Implements context manager (with statement) pattern for database connections to ensure proper cleanup
  • Adds randomization to circular vertex generation for more diverse synthetic geometry patterns
  • Updates all test files to use the new context manager pattern for database engines

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
rust/sedona-testing/src/datagen.rs Adds randomization parameter to circular vertex generation and randomizes starting angles
python/sedonadb/python/sedonadb/testing.py Implements context manager methods for DBEngine base class and PostGIS connection cleanup
python/sedonadb/tests/test_testing.py Updates tests to use context managers for database connections
python/sedonadb/tests/test_sjoin.py Updates spatial join tests to use context managers
python/sedonadb/tests/test_knnjoin.py Updates KNN join tests to use context managers
python/sedonadb/tests/functions/test_distance.py Updates distance function tests to use context managers
benchmarks/test_bench_base.py Improves synthetic geometry generation with overlapping spatial distributions

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Thank you!

@jiayuasu
Copy link
Member

Please address the git conflict. Thanks! @Kontinuation

@Kontinuation Kontinuation merged commit 82439b2 into apache:main Sep 12, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants