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

Hi,

On Sun, Jan 30, 2022 at 08:09:18PM +0100, Pavel Stehule wrote:
>
> rebase after 02b8048ba5dc36238f3e7c3c58c5946220298d71

Here are a few comments, mostly about pg_variable.c and sessionvariable.c.  I
stopped before reading the whole patch as I have some concern about the sinval
machanism, which ould change a bit the rest of the patch.  I'm also attaching a
patch (with .txt extension to avoid problem with the cfbot) with some comment
update propositions.

In sessionvariable.c, why VariableEOXAction and VariableEOXActionCodes?  Can't
the parser emit directly the char value, like e.g. relpersistence?

extraneous returns for 2 functions:

+void
+get_session_variable_type_typmod_collid(Oid varid, Oid *typid, int32 *typmod,
+                                       Oid *collid)
+{
[...]
+   return;
+}

+void
+initVariable(Variable *var, Oid varid, bool fast_only)
+{
[...]
+   return;
+}

VariableCreate():

Maybe add a bunch of AssertArg() for all the mandatory parametrers?

Also, the check for variable already existing should be right after the
AssertArg(), and using SearchSysCacheExistsX().

Maybe also adding an Assert(OidIsValid(xxxoid)) just after the
CatalogTupleInsert(), similarly to some other creation functions?

event-triggers.sgml needs updating for the firing matrix, as session variable
are compatible with even triggers.

+typedef enum SVariableXActAction
+{
+   ON_COMMIT_DROP,     /* used for ON COMMIT DROP */
+   ON_COMMIT_RESET,    /* used for DROP VARIABLE */
+   RESET,              /* used for ON TRANSACTION END RESET */
+   RECHECK             /* recheck if session variable is living */
+} SVariableXActAction;

The names seem a bit generic, maybe add a prefix like SVAR_xxx?

ON_COMMIT_RESET is also confusing as it looks like an SQL clause.  Maybe
PERFORM_DROP or something?

+static List *xact_drop_actions = NIL;
+static List *xact_reset_actions = NIL;

Maybe add a comment saying both are lists of SVariableXActAction?

+typedef SVariableData * SVariable;

looks like a missing bump to typedefs.list.

+char *
+get_session_variable_name(Oid varid)
+{
+   HeapTuple   tup;
+   Form_pg_variable varform;
+   char       *varname;
+
+   tup = SearchSysCache1(VARIABLEOID, ObjectIdGetDatum(varid));
+
+   if (!HeapTupleIsValid(tup))
+       elog(ERROR, "cache lookup failed for session variable %u", varid);
+
+   varform = (Form_pg_variable) GETSTRUCT(tup);
+
+   varname = NameStr(varform->varname);
+
+   ReleaseSysCache(tup);
+
+   return varname;
+}

This kind of function should return a palloc'd copy of the name.

+void
+ResetSessionVariables(void)
[...]
+   list_free_deep(xact_drop_actions);
+   xact_drop_actions = NIL;
+
+   list_free_deep(xact_reset_actions);
+   xact_drop_actions = NIL;
+}

The 2nd chunk should be xact_reset_actions = NIL

+static void register_session_variable_xact_action(Oid varid, SVariableXActAction action);
+static void delete_session_variable_xact_action(Oid varid, SVariableXActAction action);

The naming is a bit confusing, maybe unregister_session_cable_xact_action() for
consistency?

+void
+register_session_variable_xact_action(Oid varid,
+                                     SVariableXActAction action)

the function is missing the static keyword.

In AtPreEOXact_SessionVariable_on_xact_actions(), those 2 instructions are
executed twice (once in the middle and once at the end):

        list_free_deep(xact_drop_actions);
        xact_drop_actions = NIL;

+    * If this entry was created during the current transaction,
+    * creating_subid is the ID of the creating subxact; if created in a prior
+    * transaction, creating_subid is zero.

I don't see any place in the code where creating_subid can be zero? It looks
like it's only there for future transactional implementation, but for now this
attribute seems unnecessary?

                /* at transaction end recheck sinvalidated variables */
                RegisterXactCallback(sync_sessionvars_xact_callback, NULL);

I don't think it's ok to use xact callback for in-core code.  The function
explicitly says:

> * These functions are intended for use by dynamically loaded modules.
> * For built-in modules we generally just hardwire the appropriate calls
> * (mainly because it's easier to control the order that way, where needed).

Also, this function and AtPreEOXact_SessionVariable_on_xact_actions() are
skipping all or part of the processing if there is no active transaction.  Is
that really ok?

I'm particularly sceptical about AtPreEOXact_SessionVariable_on_xact_actions
and the RECHECK actions, as the xact_reset_actions list is reset whether the
recheck was done or not, so it seems to me that it could be leaking some
entries in the hash table.  If the database has a lot of object, it seems
possible (while unlikely) that a subsequent CREATE VARIABLE can get the same
oid leading to incorrect results?

If that's somehow ok, wouldn't it be better to rearrange the code to call those
functions less often, and only when they can do their work, or at least split
the recheck in some different function / list?

+static void
+pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue)
[...]
+   if (hashvalue != 0)
+   {
[...]
+   }
+   else
+       sync_sessionvars_all = true;

The rechecks being somewhat expensive, I think it could be a win to remove all
pending rechecks when setting the sync_sessionvars_all.