Re: Schema variables - new implementation for Postgres 15

so 5. 11. 2022 v 17:04 odesílatel Tomas Vondra <
tomas(dot)vondra(at)enterprisedb(dot)com> napsal:

> 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?
>

renamed

>
> - 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)
>

yes, it was not necessary

> - 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.
>

moved to 0002

>
> - AFAIK patches don't need to modify typedefs.list.
>
>
> 0002
> ----
>
> - some whitespace / ordering cleanup
>
> - moving setting hasSessionVariables right after similar fields
>

fixed

>
> - SessionVariableCreatePostprocess prototype is redundant (2x)
>

removed

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

renamed

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

renamed

> - some whitespace / ordering cleanup
>
>
> 0007
> ----
>
> - minor wording change
>

fixed

>
>
> 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.
>

fixed

Thank you very much for this review.

Today's patch should solve all issues reported by Tomas.

Regards

Pavel

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