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

> On Fri, Mar 24, 2023 at 08:04:08AM +0100, Pavel Stehule wrote:
> čt 23. 3. 2023 v 19:54 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
> napsal:
>
> > čt 23. 3. 2023 v 16:33 odesílatel Peter Eisentraut <
> > peter.eisentraut@enterprisedb.com> napsal:
> >
> >> The other issue is that by its nature this patch adds a lot of code in a
> >> lot of places.  Large patches are more likely to be successful if they
> >> add a lot of code in one place or smaller amounts of code in a lot of
> >> places.  But this patch does both and it's just overwhelming.  There is
> >> so much new internal functionality and terminology.  Variables can be
> >> created, registered, initialized, stored, copied, prepared, set, freed,
> >> removed, released, synced, dropped, and more.  I don't know if anyone
> >> has actually reviewed all that in detail.
> >>
> >> Has any effort been made to make this simpler, smaller, reduce scope,
> >> refactoring, find commonalities with other features, try to manage the
> >> complexity somehow?
> >>
> > I agree that this patch is large, but almost all code is simple. Complex
> > code is "only" in 0002-session-variables.patch (113KB/438KB).
> >
> > Now, I have no idea how the functionality can be sensibly reduced or
> > divided (no without significant performance loss). I see two difficult
> > points in this code:
> >
> > 1. when to clean memory. The code implements cleaning very accurately -
> > and this is unique in Postgres. Partially I implement some functionality of
> > storage manager. Probably no code from Postgres can be reused, because
> > there is not any support for global temporary objects. Cleaning based on
> > sinval messages processing is difficult, but there is nothing else.  The
> > code is a little bit more complex, because there are three types of session
> > variables: a) session variables, b) temp session variables, c) session
> > variables with transaction scope. Maybe @c can be removed, and maybe we
> > don't need to support not null default (this can simplify initialization).
> > What do you think about it?
> >
> > 2. how to pass a variable's value to the executor. The implementation is
> > based on extending the Param node, but it cannot reuse query params buffers
> > and implements own.
> > But it is hard to simplify code, because we want to support usage
> > variables in queries, and usage in PL/pgSQL expressions too. And both are
> > processed differently.
> >
>
> Maybe I can divide the  patch 0002-session-variables to three sections -
> related to memory management, planning and execution?

I agree, the patch scale is a bit overwhelming. It's worth noting that
due to the nature of this change certain heavy lifting has to be done in
any case, plus I've got an impression that some part of the patch are
quite solid (although I haven't reviewed everything, did anyone achieve
that milestone?). But still, it would be of great help to simplify the
current implementation, and I'm afraid the only way of doing this is to
make trades-off about functionality vs change size & complexity.

Maybe instead splitting the patch into implementation components, it's
possible to split it feature-by-feature, where every single patch would
represent an independent (to a certain degree) functionality? I have in
mind something like: catalog changes; base implementation; ACL support;
xact actions implementation (on commit drop, etc); variables with
default value; shadowing; etc. If such approach is possible, it will
give us: flexibility to apply only a subset of the whole patch series;
some understanding how much complexity is coming from each feature. What
do you think about this idea?

I also recall somewhere earlier in the thread Pavel has mentioned that a
transactional version of session variables patch would be actually
simpler, and he has plans to implement it later on. Is there another
trade-off on the table we could think of, transactional vs
non-transactional session variables?