Re: Schema variables - new implementation for Postgres 15

ne 4. 9. 2022 v 6:31 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com> napsal:

> 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?
>

Yes, it can works because there is not visible difference between NULL and
dropped columns, and real number of attributes is saved in HeapTupleHeader

so I removed this function and related code

> >
> > 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.
>

removed

>
> 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.
>

merged your patches, big thanks

Regards

Pavel