Re: Schema variables - new implementation for Postgres 15 (typo)

Hi,

I'm continuing review the patch slowly, and have one more issue plus one
philosophical question.

The issue have something to do with variables invalidation. Enabling
debug_discard_caches = 1 (about which I've learned from this thread) and
running this subset of the test suite:

CREATE SCHEMA svartest;
SET search_path = svartest;
CREATE VARIABLE var3 AS int;

CREATE OR REPLACE FUNCTION inc(int)
RETURNS int AS $$
BEGIN
LET svartest.var3 = COALESCE(svartest.var3 + $1, $1);
RETURN var3;
END;
$$ LANGUAGE plpgsql;

SELECT inc(1);
SELECT inc(1);
SELECT inc(1);

crashes in my setup like this:

#2 0x0000000000b432d4 in ExceptionalCondition (conditionName=0xce9b99 "n >= 0 && n < list->length", fileName=0xce9c18 "list.c", lineNumber=770) at assert.c:66
#3 0x00000000007d3acd in list_delete_nth_cell (list=0x18ab248, n=-3388) at list.c:770
#4 0x00000000007d3b88 in list_delete_cell (list=0x18ab248, cell=0x18dc028) at list.c:842
#5 0x00000000006bcb52 in sync_sessionvars_all (filter_lxid=true) at session_variable.c:524
#6 0x00000000006bd4cb in SetSessionVariable (varid=16386, value=2, isNull=false) at session_variable.c:844
#7 0x00000000006bd617 in SetSessionVariableWithSecurityCheck (varid=16386, value=2, isNull=false) at session_variable.c:885
#8 0x00007f763b890698 in exec_stmt_let (estate=0x7ffcc6fd5190, stmt=0x18aa920) at pl_exec.c:5030
#9 0x00007f763b88a746 in exec_stmts (estate=0x7ffcc6fd5190, stmts=0x18aaaa0) at pl_exec.c:2116
#10 0x00007f763b88a247 in exec_stmt_block (estate=0x7ffcc6fd5190, block=0x18aabf8) at pl_exec.c:1935
#11 0x00007f763b889a49 in exec_toplevel_block (estate=0x7ffcc6fd5190, block=0x18aabf8) at pl_exec.c:1626
#12 0x00007f763b8879df in plpgsql_exec_function (func=0x18781b0, fcinfo=0x18be110, simple_eval_estate=0x0, simple_eval_resowner=0x0, procedure_resowner=0x0, atomic=true) at pl_exec.c:615
#13 0x00007f763b8a2320 in plpgsql_call_handler (fcinfo=0x18be110) at pl_handler.c:277
#14 0x0000000000721716 in ExecInterpExpr (state=0x18bde28, econtext=0x18bd3d0, isnull=0x7ffcc6fd56d7) at execExprInterp.c:730
#15 0x0000000000723642 in ExecInterpExprStillValid (state=0x18bde28, econtext=0x18bd3d0, isNull=0x7ffcc6fd56d7) at execExprInterp.c:1855
#16 0x000000000077a78b in ExecEvalExprSwitchContext (state=0x18bde28, econtext=0x18bd3d0, isNull=0x7ffcc6fd56d7) at ../../../src/include/executor/executor.h:344
#17 0x000000000077a7f4 in ExecProject (projInfo=0x18bde20) at ../../../src/include/executor/executor.h:378
#18 0x000000000077a9dc in ExecResult (pstate=0x18bd2c0) at nodeResult.c:136
#19 0x0000000000738ea0 in ExecProcNodeFirst (node=0x18bd2c0) at execProcnode.c:464
#20 0x000000000072c6e3 in ExecProcNode (node=0x18bd2c0) at ../../../src/include/executor/executor.h:262
#21 0x000000000072f426 in ExecutePlan (estate=0x18bd098, planstate=0x18bd2c0, use_parallel_mode=false, operation=CMD_SELECT, sendTuples=true, numberTuples=0, direction=ForwardScanDirection, dest=0x18b3eb8, execute_once=true) at execMain.c:1691
#22 0x000000000072cf76 in standard_ExecutorRun (queryDesc=0x189c688, direction=ForwardScanDirection, count=0, execute_once=true) at execMain.c:423
#23 0x000000000072cdb3 in ExecutorRun (queryDesc=0x189c688, direction=ForwardScanDirection, count=0, execute_once=true) at execMain.c:367
#24 0x000000000099afdc in PortalRunSelect (portal=0x1866648, forward=true, count=0, dest=0x18b3eb8) at pquery.c:927
#25 0x000000000099ac99 in PortalRun (portal=0x1866648, count=9223372036854775807, isTopLevel=true, run_once=true, dest=0x18b3eb8, altdest=0x18b3eb8, qc=0x7ffcc6fd5a70) at pquery.c:771
#26 0x000000000099487d in exec_simple_query (query_string=0x17edcc8 "SELECT inc(1);") at postgres.c:1238

It seems that sync_sessionvars_all tries to remove a cell that either doesn't
belong to the xact_recheck_varids or weird in some other way:

+>>> p l - xact_recheck_varids->elements
$81 = -3388

The second thing I wanted to ask about is a more strategical question. Does
anyone have clear understanding where this patch is going? The thread is quite
large, and it's hard to catch up with all the details -- it would be great if
someone could summarize the current state of things, are there any outstanding
design issues or not addressed concerns?

From the first look it seems some major topics the discussion is evolving are about:

* Validity of the use case. Seems to be quite convincingly addressed in [1] and
[2].

* Complicated logic around invalidation, concurrent create/drop etc. (I guess
the issue above is falling into the same category).

* Concerns that session variables could repeat some problems of temporary tables.

Is there anything else?

[1]: https://www.postgresql.org/message-id/CAFj8pRBT-bRQJBqkzon7tHcoFZ1byng06peZfZa0G72z46YFxg%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/flat/CAFj8pRBHSAHdainq5tRhN2Nns62h9-SZi0pvNq9DTe0VM6M1%3Dg%40mail.gmail.com#4faccb978d60e9b0b5d920e1a8a05bbf