Re: Schema variables - new implementation for Postgres 15

Hi

ne 26. 3. 2023 v 19:44 odesílatel Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
napsal:

> > 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(dot)stehule(at)gmail(dot)com
> >
> > napsal:
> >
> > > čt 23. 3. 2023 v 16:33 odesílatel Peter Eisentraut <
> > > peter(dot)eisentraut(at)enterprisedb(dot)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.
>

There is not too much space for reduction - more - sometimes there is code
reuse between features.

I can reduce temporary session variables, but the same AtSubXact routines
are used by memory purging routines, and if only if you drop all dependent
features, then you can get some interesting number of reduced lines. I can
imagine very reduced feature set like

1) no temporary variables, no reset at transaction end
2) without default expressions - default is null
3) direct memory cleaning on drop (without possibility of saved value after
reverted drop) or cleaning at session end always

Note - @1 and @3 shares code

This reduced implementation can still be useful. Probably it doesn't reduce
too much code, but it can reduce non trivial code. I believe so almost all
not reduced code will be almost trivial

>
> 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 think cleaning, dropping can be moved to a separate patch. ACL support
uses generic support (it is only a few lines).

The patch 02 can be splitted - I am not sure how these parts can be
independent. I'll try to split this patch, and we will see if it will be
better.

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

Maybe I didn't use the correct words. Implementation of transactional
behaviour can be relatively simple, but only if there is support for non-
transactional behaviour already.

The transactional variables need a little bit more code, because you should
implement mvcc. Current implementation is partially transactional - there
are supported transactions and sub-transactions on catalog (and related
memory cleaning), the variables by themselves are not transactional.
Implementing mvcc is not too difficult - because there are already routines
related to handling subtransactions. But it increases the complexity of
these routines, so I postponed support for transactional variables to the
next step.

Regards

Pavel