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

ADBDEV-6577: Use short-lived SPI contexts #40

Merged
merged 49 commits into from
Nov 1, 2024
Merged

ADBDEV-6577: Use short-lived SPI contexts #40

merged 49 commits into from
Nov 1, 2024

Conversation

RekGRpth
Copy link
Member

@RekGRpth RekGRpth commented Oct 28, 2024

Use short-lived SPI contexts

diskquota uses long-lived SPI contexts in most places: at the beginning it
calls the SPI_connect function, which creates an SPI context and switches to
its use, then it does a lot of work, including SPI_execute, and only at the
very end it calls the SPI_finish function, which clears the SPI context. This
usage architecture is incorrect and can lead to memory leaks while the SPI
context is alive, because the SPI_execute function, for example, allocates
memory in it for parsing a SQL query. Correct use of SPI involves opening and
closing the SPI as close as possible, so that the SPI context is as short-lived
as possible. This patch adds new wrappers for connecting and disconnecting
functions from SPI and their usage, and moves the connection to SPI into
functions closer to making requests over SPI.


It is easier to view the changes with the "Hide whitespace" option enabled.

@RekGRpth RekGRpth marked this pull request as ready for review October 28, 2024 11:20
@RekGRpth RekGRpth marked this pull request as draft October 29, 2024 03:33
@RekGRpth RekGRpth marked this pull request as ready for review October 29, 2024 03:40
src/diskquota.h Outdated Show resolved Hide resolved
src/quotamodel.c Outdated Show resolved Hide resolved
@silent-observer
Copy link

Can you list the functions that opened SPI connection before this patch and after, and which contain which? As far as I can see

  1. create_monitor_db_table (both before and after)
  2. init_database_list (both before and after)
  3. do_process_extension_ddl_message (both before and after)
  4. worker_spi_get_extension_version (both before and after)
  5. check_diskquota_state_is_ready (both before and after)
  6. refresh_disk_quota_usage (removed)
    • load_table_size (added)
    • get_rel_oid_list (added)
    • delete_from_table_size_map (added)
    • insert_into_table_size_map (added)

@RekGRpth
Copy link
Member Author

Can you list the functions that opened SPI connection before this patch and after, and which contain which? As far as I can see

1. `create_monitor_db_table` (both before and after)

2. `init_database_list` (both before and after)

3. `do_process_extension_ddl_message` (both before and after)

4. `worker_spi_get_extension_version` (both before and after)

5. `check_diskquota_state_is_ready` (both before and after)

6. `refresh_disk_quota_usage` (removed)
   
   * `load_table_size` (added)
   * `get_rel_oid_list` (added)
   * `delete_from_table_size_map` (added)
   * `insert_into_table_size_map` (added)
  1. load_quotas (both before and after)

@silent-observer
Copy link

Can you add this to the description? At least the refresh_disk_quota_usage and functions called from it, since the behavior was changed

@RekGRpth
Copy link
Member Author

since the behavior was changed

It's just the behavior that hasn't changed!

@RekGRpth
Copy link
Member Author

Can you add this to the description?

added

src/diskquota.h Outdated Show resolved Hide resolved
src/diskquota.h Outdated Show resolved Hide resolved
src/gp_activetable.c Outdated Show resolved Hide resolved
@RekGRpth RekGRpth merged commit 92ca25b into gpdb Nov 1, 2024
2 checks passed
@RekGRpth RekGRpth deleted the ADBDEV-6577 branch November 1, 2024 03:37
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