Hi, On Tue, Sep 06, 2022 at 06:23:12PM +0800, Julien Rouhaud wrote: > On Tue, Sep 06, 2022 at 08:43:59AM +0200, Pavel Stehule wrote: > > Hi > > > > After talking with Julian I removed "debug" fields name and nsname from > > SVariable structure. When it is possible it is better to read these fields > > from catalog without risk of obsoletely or necessity to refresh these > > fields. In other cases we display only oid of variable instead name and > > nsname (It is used just for debug purposes). > > Thanks! I'm just adding back the forgotten Cc list.
About the last change: +static void +pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue) +{ [...] + elog(DEBUG1, "session variable \"%s.%s\" (oid:%u) should be rechecked (forced by sinval)", + get_namespace_name(get_session_variable_namespace(svar->varid)), + get_session_variable_name(svar->varid), + svar->varid); There's no guarantee that the variable still exists in cache (for variables dropped in the current transaction), or even that the callback is called while in a transaction state, so we should only display the oid here. FTR just to be sure I ran all the new regression tests (with this fix) with CCA and log_min_messages = DEBUG1 and it didn't hit any problem, so it doesn't seem that there's any other issue hidden somewhere. Other than that I don't see any remaining problems in session_variable.c. I still have a few nitpicking comments though: +static SVariable +prepare_variable_for_reading(Oid varid) +{ [...] + /* Store result before releasing Executor memory */ + set_session_variable(svar, value, isnull, true); + + MemoryContextSwitchTo(oldcxt); + + FreeExecutorState(estate); The comment and code is a bit misleading, as it's not immediately obvious that set_session_variable() doesn't rely on the current memory contex for allocations. Simply moving the MemoryContextSwitchTo() before the set_session_variable() would be better. +typedef struct SVariableData +{ [...] + bool is_domain; + Oid basetypeid; + void *domain_check_extra; + LocalTransactionId domain_check_extra_lxid; AFAICS basetypeid isn't needed anymore. + /* Both lists hold fields of SVariableXActActionItem type */ + static List *xact_on_commit_drop_actions = NIL; + static List *xact_on_commit_reset_actions = NIL; Is it possible to merge both in a single list? I don't think that there's much to gain trying to separate those. They shouldn't contain a lot of entries, and they're usually scanned at the same time anyway. This is especially important as one of the tricky parts of this patchset is maintaining those lists across subtransactions, and since both have the same heuristics all the related code is duplicated. I see that in AtPreEOXact_SessionVariable_on_xact_actions() both lists are handled interleaved with the xact_recheck_varids, but I don't see any reason why we couldn't process both action lists first and then process the rechecks. I did a quick test and don't see any failure in the regression tests. +void +RemoveSessionVariable(Oid varid) +{ I looks like a layering violation to have (part of) the code for CREATE VARIABLE in pg_variable.[ch] and the code for DROP VARIABLE in session_variable.[ch]. I think it was done mostly because it was the initial sync_sessionvars_all() that was responsible to avoid cleaning up memory for variables dropped in the current transaction, but that's not a requirement anymore. So I don't see anything preventing us from moving RemoveSessionVariable() in pg_variable, and export some function in session_variable to do the additional work for properly maintaining the hash table if needed (with that knowledge held in session_variable, not in pg_variable). You should only need to pass the oid of the variable and the eoxaction. Simlarly, why not move DefineSessionVariable() in pg_variable and expose some API in session_variable to register the needed SVAR_ON_COMMIT_DROP action? Also, while not a problem I don't think that the CommandCounterIncrement() is necessary in DefineSessionVariable(). CREATE VARIABLE is a single operation and you can't have anything else running in the same ProcessUtility() call. And since cd3e27464cc you have the guarantee that a CommandCounterIncrement() will happen at the end of the utility command processing. While at it, maybe it would be good to add some extra tests in src/test/modules/test_extensions. I'm thinking a version 1.0 that creates a variable and initialize the value (and and extra step after creating the extension to make sure that the value is really set), and an upgrade to 2.0 that creates a temp variable on commit drop, that has to fail due to the dependecy on the extension.