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

On Sat, Sep 03, 2022 at 11:00:51PM +0800, Julien Rouhaud wrote:
> Hi,
>
> On Fri, Sep 02, 2022 at 07:42:00AM +0200, Pavel Stehule wrote:
> >
> > rebased today
>
> After some off-list discussion with Pavel, I'm sending some proposal patches
> (in .txt extension) to apply to the last patchset.
>
> To sum up, when a session issues a DROP VARIABLE, the session will receive an
> sinval notification for its own drop and we don't want to process it
> immediately as we need to hold the value in case the transaction is rollbacked.
> The current patch avoided that by forcing a single processing of sinval per
> transaction, and forcing it before dropping the variable.  It works but it
> seems to me that postponing all but the first VARIABLEOID sinval to the next
> transaction is a big hammer, and the sooner we can free some memory the better.
>
> For an alternative approach the attached patch store the lxid in the SVariable
> itself when dropping a currently set variable, so we can process all sinval and
> simply defer to the next transaction the memory cleanup of the variable(s) we
> know we just dropped.  What do you think of that approach?
>
> As I was working on some changes I also made a pass on session_variable.c.  I
> tried to improve a bit some comments, and also got rid of the "first_time"
> variable.  The name wasn't really great, and AFAICS it can be replaced by
> testing whether the memory context has been created yet or not.
>
> But once that done I noticed the get_rowtype_value() function.  I don't think
> this function is necessary as the core code already knows how to deal with
> stored datum when the underlying composite type was modified.  I tried to
> bypass that function and always simply return the stored value and all the
> tests run fine.  Is there really any cases when this extra code is needed?
>
> FTR I tried to do a bunch of additional testing using relation as base type for
> variable, as you can do more with those than plain composite types, but it
> still always works just fine.
>
> However, while doing so I noticed that find_composite_type_dependencies()
> failed to properly handle dependencies on relation (plain tables, matviews and
> partitioned tables).  I'm also adding 2 additional patches to fix this corner
> case and add an additional regression test for the plain table case.

I forgot to mention this chunk:

+    /*
+     * Although the value of domain type should be valid (it is
+     * checked when it is assigned to session variable), we have to
+     * check related constraints anytime. It can be more expensive
+     * than in PL/pgSQL. PL/pgSQL forces domain checks when value
+     * is assigned to the variable or when value is returned from
+     * function. Fortunately, domain types manage cache of constraints by
+     * self.
+     */
+    if (svar->is_domain)
+    {
+        MemoryContext oldcxt = CurrentMemoryContext;
+
+        /*
+         * Store domain_check extra in CurTransactionContext. When we are
+         * in other transaction, the domain_check_extra cache is not valid.
+         */
+        if (svar->domain_check_extra_lxid != MyProc->lxid)
+            svar->domain_check_extra = NULL;
+
+        domain_check(svar->value, svar->isnull,
+                     svar->typid, &svar->domain_check_extra,
+                     CurTransactionContext);
+
+        svar->domain_check_extra_lxid = MyProc->lxid;
+
+        MemoryContextSwitchTo(oldcxt);
+    }

I agree that storing the domain_check_extra in the transaction context sounds
sensible, but the memory context handling is not quite right.

Looking at domain_check, it doesn't change the current memory context, so as-is
all the code related to oldcxt is unnecessary.

Some other callers like expandedrecord.c do switch to a short lived context to
limit the lifetime of the possible leak by the expression evaluation, but I
don't think that's an option here.