Re: Schema variables - new implementation for Postgres 15

Hi,

I took a quick look at the latest patch version. In general the patch
looks pretty complete and clean, and for now I have only some basic
comments. The attached patch tweaks some of this, along with a couple
additional minor changes that I'll not discuss here.


1) Not sure why we need to call this "schema variables". Most objects
are placed in a schema, and we don't say "schema tables" for example.
And it's CREATE VARIABLE and not CREATE SCHEMA VARIABLE, so it's a bit
inconsistent.

The docs actually use "Global variables" in one place for some reason.


2) I find this a bit confusing:

SELECT non_existent_variable;
test=# select s;
ERROR:  column "non_existent_variable" does not exist
LINE 1: select non_existent_variable;

I wonder if this means using SELECT to read variables is a bad idea, and
we should have a separate command, just like we have LET (instead of
just using UPDATE in some way).


3) I've reworded / tweaked a couple places in the docs, but this really
needs a native speaker - I don't have a very good "feeling" for this
technical language so it's probably still quite cumbersome.


4) Is sequential scan of the hash table  in clean_cache_callback() a
good idea? I wonder how fast (with how many variables) it'll become
noticeable, but it may be good enough for now and we can add something
better (tracing which variables need resetting) later.


5) In what situation would we call clean_cache_callback() without a
transaction state? If that happens it seems more like a bug, so
maybeelog(ERROR) or Assert() would be more appropriate?


6) free_schema_variable does not actually use the force parameter


7) The target_exprkind expression in transformSelectStmt really needs
some explanation. Because that's chance you'll look at this in 6 months
and understand what it does?

    target_exprkind =
        (pstate->p_expr_kind != EXPR_KIND_LET_TARGET ||
         pstate->parentParseState != NULL) ?
                    EXPR_KIND_SELECT_TARGET : EXPR_KIND_LET_TARGET;


8) immutable variables without a default value

IMO this case should not be allowed. On 2021/08/29 you wrote:

    I thought about this case, and I have one scenario, where this
    behaviour can be useful. When the variable is declared as IMMUTABLE
    NOT NULL without not null default, then any access to the content of
    the variable has to fail. I think it can be used for detection,
    where and when the variable is first used. So this behavior is
    allowed just because I think, so this feature can be interesting for
    debugging. If this idea is too strange, I have no problem to disable
    this case.

This seems like a really strange use case. In a production code you'll
not do this, because then the variable is useless and the code does not
work at all (it'll just fail whenever it attempts to access the var).
And if you can modify the code, there are other / better ways to do this
(raising an exception, ...).

So this seems pretty useless to me, +1 to disabling it.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company