Re: Schema variables - new implementation for Postgres 15 - Mailing list pgsql-hackers

Hi,

I did a quick initial review of this patch series - attached is a
version with "review" commits for some of the parts. The current patch
seems in pretty good shape, most of what I noticed are minor issues. I
plan to do a more thorough review later.

A quick overview of the issues:

0001
----

- AtPreEOXact_SessionVariable_on_xact_actions name seems unnecessarily
complicated and redundant, and mismatching nearby functions. Why not
call it AtEOXact_SessionVariable, similar to AtEOXact_LargeObject?

- some whitespace / ordering cleanup

- I'm not sure why find_composite_type_dependencies needs the extra
"else if" branch (instead of just doing "if" as before)

- NamesFromList and IdentifyVariable seem introduced unnecessarily
early, as they are only used in 0002 and 0003 parts (in the original
patch series). Not sure if the plan is to squash everything into a
single patch, or commit individual patches.

- AFAIK patches don't need to modify typedefs.list.


0002
----

- some whitespace / ordering cleanup

- moving setting hasSessionVariables right after similar fields

- SessionVariableCreatePostprocess prototype is redundant (2x)

- I'd probably rename pg_debug_show_used_session_variables to
pg_session_variables (assuming we want to keep this view)

0003
----

- I'd rename svariableState to SVariableState, to keep the naming
consistent with other similar/related typedefs.

- some whitespace / ordering cleanup


0007
----

- minor wording change

Aside from that, I tried running this under valgrind, and that produces
this report:

==250595== Conditional jump or move depends on uninitialised value(s)
==250595==    at 0x731A48: sync_sessionvars_all (session_variable.c:513)
==250595==    by 0x7321A6: prepare_variable_for_reading
(session_variable.c:727)
==250595==    by 0x7320BA: CopySessionVariable (session_variable.c:898)
==250595==    by 0x7BC3BF: standard_ExecutorStart (execMain.c:252)
==250595==    by 0x7BC042: ExecutorStart (execMain.c:146)
==250595==    by 0xA89283: PortalStart (pquery.c:520)
==250595==    by 0xA84E8D: exec_simple_query (postgres.c:1199)
==250595==    by 0xA8425B: PostgresMain (postgres.c:4551)
==250595==    by 0x998B03: BackendRun (postmaster.c:4482)
==250595==    by 0x9980EC: BackendStartup (postmaster.c:4210)
==250595==    by 0x996F0D: ServerLoop (postmaster.c:1804)
==250595==    by 0x9948CA: PostmasterMain (postmaster.c:1476)
==250595==    by 0x8526B6: main (main.c:197)
==250595==  Uninitialised value was created by a heap allocation
==250595==    at 0xCD86F0: MemoryContextAllocExtended (mcxt.c:1138)
==250595==    by 0xC9FA1F: DynaHashAlloc (dynahash.c:292)
==250595==    by 0xC9FEC1: element_alloc (dynahash.c:1715)
==250595==    by 0xCA102A: get_hash_entry (dynahash.c:1324)
==250595==    by 0xCA0879: hash_search_with_hash_value (dynahash.c:1097)
==250595==    by 0xCA0432: hash_search (dynahash.c:958)
==250595==    by 0x731614: SetSessionVariable (session_variable.c:846)
==250595==    by 0x82FEED: svariableReceiveSlot (svariableReceiver.c:138)
==250595==    by 0x7BD277: ExecutePlan (execMain.c:1726)
==250595==    by 0x7BD0C5: standard_ExecutorRun (execMain.c:422)
==250595==    by 0x7BCE63: ExecutorRun (execMain.c:366)
==250595==    by 0x7332F0: ExecuteLetStmt (session_variable.c:1310)
==250595==    by 0xA8CC15: standard_ProcessUtility (utility.c:1076)
==250595==    by 0xA8BC72: ProcessUtility (utility.c:533)
==250595==    by 0xA8B2B9: PortalRunUtility (pquery.c:1161)
==250595==    by 0xA8A454: PortalRunMulti (pquery.c:1318)
==250595==    by 0xA89A16: PortalRun (pquery.c:794)
==250595==    by 0xA84F9E: exec_simple_query (postgres.c:1238)
==250595==    by 0xA8425B: PostgresMain (postgres.c:4551)
==250595==    by 0x998B03: BackendRun (postmaster.c:4482)
==250595==

Which I think means this:

    if (filter_lxid && svar->drop_lxid == MyProc->lxid)
        continue;

accesses drop_lxid, which was not initialized in init_session_variable.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company