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.
