Re: SQLFunctionCache and generic plans
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
Cc: Pavel Stehule <pavel.stehule@gmail.com>,
Alexander Korotkov <aekorotkov@gmail.com>,
pgsql-hackers@lists.postgresql.org,
Ronan Dunklau <ronan.dunklau@aiven.io>
Date: 2025-03-31T23:29:05Z
Lists: pgsql-hackers
Attachments
- v11-0001-Support-cached-plans-that-work-from-a-parse-anal.patch (text/x-diff) patch v11-0001
- v11-0002-Provide-a-post-rewrite-callback-hook-in-plancach.patch (text/x-diff) patch v11-0002
- v11-0003-Factor-out-plpgsql-s-management-of-its-function-.patch (text/x-diff) patch v11-0003
- v11-0004-Restructure-check_sql_fn_retval.patch (text/x-diff) patch v11-0004
- v11-0005-Add-a-test-case-showing-undesirable-RLS-behavior.patch (text/x-diff) patch v11-0005
- v11-0006-Change-SQL-language-functions-to-use-the-plan-ca.patch (text/x-diff) patch v11-0006
- v11-0007-Delay-parse-analysis-and-rewrite-until-we-re-rea.patch (text/x-diff) patch v11-0007
I wrote: > There's more stuff that could be done, but I feel that all of this > could be left for later: > * I really wanted to do what I mentioned upthread and change things > so we don't even parse the later queries until we've executed the > ones before that. However that seems to be a bit of a mess to > make happen, and the patch is large/complex enough already. I felt more regret about that decision after reading the discussion at [1] and realizing that we already moved those goalposts part way. For example, historically this fails: set check_function_bodies to off; create or replace function create_and_insert() returns void language sql as $$ create table t1 (f1 int); insert into t1 values (1.2); $$; select create_and_insert(); You get "ERROR: relation "t1" does not exist" because we try to parse-analyze the INSERT before the CREATE has been executed. That's still true with patch v10. However, consider this: create table t1 (f1 int); create or replace function alter_and_insert() returns void language sql as $$ alter table t1 alter column f1 type numeric; insert into t1 values (1.2); $$; select alter_and_insert(); Historically that fails with ERROR: table row type and query-specified row type do not match DETAIL: Table has type numeric at ordinal position 1, but query expects integer. CONTEXT: SQL function "alter_and_insert" statement 2 because we built a plan for the INSERT before executing the ALTER. However, with v10 it works! The INSERT is parse-analyzed against the old table definition, but that's close enough that we can successfully make a CachedPlanSource. Then when we come to execute the INSERT, the plancache notices that what it has is already invalidated, so it re-does everything and the correct data type is used. So that left me quite unsatisfied. It's not great to modify edge-case semantics a little bit and then come back and modify them some more in the next release; better for it to all happen at once. Besides which, it'd be quite difficult to document v10's behavior in a fashion that makes any sense to users. I'd abandoned the idea of delaying parse analysis over the weekend because time was running out and I had enough things to worry about, but I resolved to take another look. And it turns out not to be very hard at all to get there from where we were in v10: we already did 90% of the restructuring needed to do the processing incrementally. The main missing thing is we need someplace to stash the raw parse trees or Queries we got from pg_proc until we are ready to do the analyze-rewrite-and-make-a-cached-plan business for them. I had already had the idea of making a second context inside the SQLFunctionHashEntry to keep those trees in until we don't need them anymore (which we don't once the last CachedPlanSource is made). So that part wasn't hard. I then discovered that the error reporting needed some tweaking, which wasn't hard either. So attached is v11. 0001-0006 are identical to v10, and then 0007 is the delta. (I'd plan to squash that in final commit, but I thought it'd be easier to review this way.) Compared to v10, this means that parse analysis work as well as planning work is done in fcontext, so there's a little bit more temporary leakage during the first run of a lazy-evaluation SRF. I still think that this is not going to amount to anything worth worrying about, but maybe it slightly raises the interest level of not doing everything in fcontext. Another point is that the order of the subroutines in functions.c is starting to feel rather random. I left them like this to minimize the amount of pure code motion involved in 0007, but I'm tempted to rearrange them into something closer to order-of-execution before final commit. Anyway, I feel pretty good about this patch now and am quite content to stop here for PG 18. regards, tom lane [1] https://www.postgresql.org/message-id/flat/4115257.1743441667%40sss.pgh.pa.us#ae47bbce3b234245c530ad06eb819d8c
Commits
-
Fix performance issue in deadlock-parallel isolation test.
- 837cc73af29c 18.0 landed
-
functions.c: copy trees from source_list before parse analysis etc.
- 0f43083d16f4 18.0 landed
-
Fix oversight in commit 0dca5d68d.
- dbd437e670b8 18.0 landed
-
Change SQL-language functions to use the plan cache.
- 0dca5d68d7be 18.0 landed
-
Reordering DISTINCT keys to match input path's pathkeys
- a8ccf4e93a7e 18.0 cited