Performance: reuse query plans instead of re-planning every statement by TheBeast85 · Pull Request #3067 · greenbone/gvmd

TheBeast85 · 2026-07-30T09:31:41Z

What

Cache server side prepared statements per connection in sql_pg.c, keyed by the statement text. The existing _ps API only used PQexecParams, which sends the text every time and plans it every time.
Bind the values of the queries that run once per row of a listing: the Super clause and acl_user_owns_uuid in manage_acl.c, find_resource_with_permission, resource_tag_count and target_in_use.

Why

gvmd re-planned nearly every statement it sent. With pg_stat_statements.track_planning on, get_targets with rows=-1 over 1000 targets showed 10058 statements, 403 ms of planning against 115 ms of execution. track_planning is off by default, which is why pg_stat_statements reported only the 115 ms and the database looked harmless.

The Super clause alone was 241 ms of 387 ms.

Only statements that bind parameters are cached; the printf style calls put their values in the text, so every call would be a new key and a plan for WHERE id = 4711 is of no use to WHERE id = 4712. The cache is dropped wherever the connection goes away, including sql_close_fork. A failed PQprepare is not an error, the caller falls back to PQexecParams.

References

Split out of the original PR as requested. The GET_TARGETS part is now #3080, the severity part #3081. #3068 is based on this branch.

Checklist

Tested manually against a Community Edition stack, ACL isolation verified in both directions

rdttmr

Thank you for putting in the effort to improve our code!

I have added a whole bunch of comments and would kindly ask you to split the caching of prepared statements and the GET_TARGETS performance improvements into two separate PRs.

In addition, please let me know if you prefer to fix our concerns yourself, or if you would prefer for us to take over your PR(s), adjust the changes to our codebase and team decisions and take it from there.

rdttmr · 2026-08-03T07:37:25Z

+                  /* Name and UUID come from the iterator instead of one
+                   * statement each.  Over 1000 targets that saved two
+                   * statements per target; get_targets issued 12196 in
+                   * total while spending only 156 ms of 1498 ms actually
+                   * executing SQL, so the statement count is what costs.
+                   *
+                   * g_strdup keeps ownership as credential_name() and
+                   * credential_uuid() had it - both returned g_strdup'd
+                   * memory - so the free() calls at the end of the loop
+                   * stay correct for this branch and the trash branch
+                   * alike.  The iterator owns its own row buffer. */


This comment explains the change, not the code.

Without knowing the previous code, so outside of the context of this change set, most of this explanation is useless.

rdttmr · 2026-08-03T07:38:51Z

+                  ssh_name
+                    = g_strdup (target_iterator_ssh_credential_name (&targets));
+                  ssh_uuid
+                    = g_strdup (target_iterator_ssh_credential_uuid (&targets));


I think improving the GET_TARGETS behavior should be a separate PR.

Both changes are good, however reusing prepared statements and improving the performance of GET_TARGETS specifically are two different things.

rdttmr · 2026-08-03T07:39:51Z

-                  smb_name = credential_name (smb_credential);
-                  smb_uuid = credential_uuid (smb_credential);
+                  smb_name
+                    = g_strdup (target_iterator_smb_credential_name (&targets));
+                  smb_uuid
+                    = g_strdup (target_iterator_smb_credential_uuid (&targets));


GET_TARGETS improvement, not prepared statements improvement

rdttmr · 2026-08-03T07:40:00Z

-                  esxi_name = credential_name (esxi_credential);
-                  esxi_uuid = credential_uuid (esxi_credential);
+                  esxi_name
+                    = g_strdup (target_iterator_esxi_credential_name (&targets));
+                  esxi_uuid
+                    = g_strdup (target_iterator_esxi_credential_uuid (&targets));


GET_TARGETS improvement, not prepared statements improvement

rdttmr · 2026-08-03T07:40:06Z

-                  snmp_name = credential_name (snmp_credential);
-                  snmp_uuid = credential_uuid (snmp_credential);
+                  snmp_name
+                    = g_strdup (target_iterator_snmp_credential_name (&targets));
+                  snmp_uuid
+                    = g_strdup (target_iterator_snmp_credential_uuid (&targets));


GET_TARGETS improvement, not prepared statements improvement

rdttmr · 2026-08-03T08:07:48Z

+/**
+ * @brief Get the SSH credential name from a target iterator.
+ *
+ * @param[in]  iterator  Iterator.
+ *
+ * @return Credential name, or NULL if iteration is complete.
+ */
+DEF_ACCESS (target_iterator_ssh_credential_name,
+            GET_ITERATOR_COLUMN_COUNT + 23);
+
+/**
+ * @brief Get the SMB credential name from a target iterator.
+ *
+ * @param[in]  iterator  Iterator.
+ *
+ * @return Credential name, or NULL if iteration is complete.
+ */
+DEF_ACCESS (target_iterator_smb_credential_name,
+            GET_ITERATOR_COLUMN_COUNT + 24);
+


GET_TARGETS improvement, not prepared statements improvement

(whole changes below as well)

rdttmr · 2026-08-03T08:25:11Z

+  /* The UUID is bound rather than formatted in, so that the statement text
+   * only varies with type and trash - a handful of combinations, which the
+   * server side statement cache in sql_pg.c can keep plans for.  With the
+   * UUID in the text every call produced a different statement and
+   * PostgreSQL planned it afresh; get_targets over 1000 targets spent 403 ms
+   * planning against 115 ms executing.


This again explains a change

rdttmr · 2026-08-03T08:28:42Z

+          assert (0);
+        case -1:
+          g_free (statement);
+          g_free (quoted_uuid);


quoted_uuid may be freed sooner, since this code path uses a prepared statement now

rdttmr · 2026-08-03T08:32:50Z

+   * Worth doing because it is called once per row of every listing: over
+   * 1000 targets it ran 1000 times and, with the values formatted into the
+   * text, PostgreSQL planned it 1000 times.  Measured with
+   * pg_stat_statements.track_planning on, that was 57 ms of planning per
+   * get_targets call against 2 ms of execution. */


It is worth doing anyway, because our plan is to move every parameterized query to a prepared statement :)

rdttmr · 2026-08-03T08:35:02Z

+  /* Bound, not formatted in: this runs once per row of every target listing,
+   * and with the id in the text each call was a statement PostgreSQL had
+   * never seen before, so it parsed and planned it again every time. */


There is no need to justify moving any query to a prepared statement, as long as there are parameters in the query

TheBeast85 · 2026-08-03T13:08:43Z

Thanks for the review, and for the offer — I would like to do the changes myself.

On splitting: the diff falls into three parts, not two.

Prepared statement reuse — the cache in sql_pg.c, plus the parameterised queries in manage_acl.c, manage_sql_resources.c, manage_sql_tags.c and target_in_use in manage_sql_targets.c.
GET_TARGETS — gmp.c, manage_targets.h, manage_sql_targets.h and the iterator part of manage_sql_targets.c.
report_severity in manage_pg.c, split into a cached and a dynamic function so the expensive branch is only planned when it is reached. That is neither of the two — I would open it as a third PR unless you would rather have it elsewhere.

manage_sql_targets.c is the only file that lands in two of them. I will keep this PR for (1) and open separate ones for (2) and (3).

The remaining points I will fix as requested:

Drop the comments that explain the change instead of the code.
Remove ACL_SUPER_CLAUSE and ACL_SUPER_CLAUSE_ARGS, both unused after the change.
Free quoted_uuid right after the parameter is bound.
Apply your STABLE wording.

One heads-up: #3068 is based on this branch, so I will rebase it once the split is done.

gvmd re-planned nearly every statement it sent. With pg_stat_statements.track_planning turned on, get_targets with rows=-1 over 1000 targets showed where the time went: 10058 statements planning 403 ms execution 115 ms Planning cost three and a half times what executing did. track_planning is off by default, so pg_stat_statements reported only the 115 ms and the database looked harmless; the cgroup CPU told the real story, with postgres at 105 % of a core against gvmd's 13 %. * sql_pg.c gains a server side prepared statement cache. The existing _ps API only used PQexecParams, which sends the statement text every time and plans it every time. Now each statement is prepared once with PQprepare and run with PQexecPrepared, keyed by its text and cached per connection. Only statements that actually bind parameters are cached: the printf style calls put their values in the text, so every call would be a new key and a plan for "WHERE id = 4711" is of no use to "WHERE id = 4712". The cache is dropped wherever the connection goes away, including sql_close_fork, where the child abandons the parent's connection without closing it. A failed PQprepare is not an error, the caller falls back to PQexecParams. * manage_acl.c binds the resource value and the user UUID in the Super clause and in acl_user_owns_uuid. The Super clause was the single most expensive plan in gvmd, 241 ms of 387 ms. acl_user_owns_uuid runs once or twice for every row of every listing. * manage_sql_resources.c, manage_sql_tags.c and manage_sql_targets.c bind the UUID in find_resource_with_permission, the type, resource and parent type in resource_tag_count, and the target id in target_in_use. Verified: ACL isolation in both directions, a test user sees 0 of 1000 foreign targets but its own, and the owner does not see the test user's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

TheBeast85 requested review from a team as code owners July 30, 2026 09:31

greenbonebot enabled auto-merge (rebase) July 30, 2026 09:31

TheBeast85 mentioned this pull request Jul 30, 2026

Performance: speed up creating an override #3068

Open

1 task

rdttmr reviewed Aug 3, 2026

View reviewed changes

auto-merge was automatically disabled August 3, 2026 13:20
Head branch was pushed to by a user without write access

TheBeast85 force-pushed the perf/reuse-query-plans branch from b7c133e to 388ee8a Compare August 3, 2026 13:20

greenbonebot enabled auto-merge (rebase) August 3, 2026 13:21

This was referenced Aug 3, 2026

Performance: take target credential names and UUIDs from the iterator #3080

Open

Performance: only plan the dynamic severity branches when they are used #3081

Open

Navigation Menu

Uh oh!

Performance: reuse query plans instead of re-planning every statement - #3067

Performance: reuse query plans instead of re-planning every statement#3067
TheBeast85 wants to merge 1 commit into
greenbone:mainfrom
TheBeast85:perf/reuse-query-plans

TheBeast85 commented Jul 30, 2026 •

edited

Loading

Uh oh!

rdttmr left a comment

Uh oh!

rdttmr Aug 3, 2026

Uh oh!

rdttmr Aug 3, 2026

Uh oh!

rdttmr Aug 3, 2026

Uh oh!

rdttmr Aug 3, 2026

Uh oh!

rdttmr Aug 3, 2026

Uh oh!

rdttmr Aug 3, 2026

Uh oh!

rdttmr Aug 3, 2026

Uh oh!

rdttmr Aug 3, 2026

Uh oh!

rdttmr Aug 3, 2026

Uh oh!

rdttmr Aug 3, 2026

Uh oh!

TheBeast85 commented Aug 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

TheBeast85 commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

References

Checklist

Uh oh!

rdttmr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBeast85 commented Aug 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

TheBeast85 commented Jul 30, 2026 •

edited

Loading