sql: commitPrepStmtNamespace is allocation heavy and almost never needed

[nvanbenschoten]

Above is a screenshot of a CPU profile from a kv95 run on a single-node m5d.12xlarge cluster. We can see that commitPrepStmtNamespace is responsible for 2.48% of CPU utilization. This is pretty insane considering there's nothing in the prepared stmt namespace that needs rolling back.

We should page this prepStmtNamespace stuff back in (including its interaction with the pgwire protocol) and then audit all uses of it. We should then make it cheaper to use, introduce fast-paths that avoid touching it at all when not necessary, or do some combination of both.

cc. @knz @andreimatei

Jira issue: CRDB-4747

Epic CRDB-42584

[knz]

@jordanlewis @petermattis which team / project is this landing in?

[petermattis]

That code is in no-man's land. I think the SQL execution team might have more bandwidth for this, though. @jordanlewis feel free to push back if you disagree.

[jordanlewis]

We can take this on eventually, though it doesn't seem particularly high priority, and I wouldn't make a claim about having any extra bandwidth.

[nvanbenschoten]

There's a 6.4% speedup sitting here for sysbench's oltp_point_select workload (and presumably kv100). I'll see if I can knock this out later this week.

[nvanbenschoten]

I took a shot at this in this branch: nvanbenschoten/prepStmtPerf. The approach was to only copy these prepStmtNamespaces when they were actually changed. What I didn't take into consideration is that their portals are almost always mutated because clients repeatedly bind with prepared statements, so the change didn't have the desired effect.

@andreimatei this got me thinking about whether we have the wrong structure here. We need to be able to roll back prepared statements during a txn retry, but do we need to be able to roll back portals? Will clients ever rely on the result of a previously prepared statement binding instead of just rebinding every time they want to use a prepared statement? If not then we could constrain the namespace checkpointing to just prepared statements and fix this easily. If we do need to be able to roll back portals then we'll need to rework the connExecutor's interaction with the prepStmtNamespaces checkpointing. We'll need to push down information about when it's ok to modify the checkpoint directly (e.g. when we're running an implicit txn and can't possibly roll it back).

[andreimatei]

We need to be able to roll back prepared statements during a txn retry, but do we need to be able to roll back portals?

I don't know; I don't know what the lifetimes of both portals or prepared statements is, and the docs unfortunately are not very clear.

[nvanbenschoten]

From https://www.postgresql.org/docs/10/protocol-flow.html:

If successfully created, a named portal object lasts till the end of the current transaction, unless explicitly destroyed. An unnamed portal is destroyed at the end of the transaction, or as soon as the next Bind statement specifying the unnamed portal as destination is issued. (Note that a simple Query message also destroys the unnamed portal.) Named portals must be explicitly closed before they can be redefined by another Bind message, but this is not required for the unnamed portal. Named portals can also be created and accessed at the SQL command level, using DECLARE CURSOR and FETCH.

So it looks like we're already in violation of this. It hasn't seemed to cause any issues though, but I'm sure it would if clients were expecting portals to be cleaned up after txns, which could lead to an unbounded number of portals.

This gives us some flexibility though, as it implies that portals don't need to persist through txn retries.

[github-actions]

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
5 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

[knz]

This is still relevant it seems.

[rafiss]

Closing since this is an old issue, and it seems that #134183 addressed the current known improvement. Since performance optimization is an active area of effort, this will resurface through future testing if needed.