Re: Schema variables - new implementation for Postgres 15

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.