Re: Schema variables - new implementation for Postgres 15

Hi

st 3. 11. 2021 v 14:05 odesílatel Tomas Vondra <
tomas(dot)vondra(at)enterprisedb(dot)com> napsal:

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

Yes, there is inconsistency, but I think it is necessary. The name
"variable" is too generic. Theoretically we can use other adjectives like
session variables or global variables and the name will be valid. But it
doesn't describe the fundamentals of design. This is similar to the
package's variables from PL/SQL. These variables are global, session's
variables too. But the usual name is "package variables". So schema
variables are assigned to schemes, and I think a good name can be "schema
variables". But it is not necessary to repeat keyword schema in the CREATE
COMMAND.

My opinion is not too strong in this case, and I can accept just
"variables" or "session's variables" or "global variables", but I am not
sure if these names describe this feature well, because still they are too
generic. There are too many different implementations of session global
variables (see PL/SQL or T-SQL or DB2).

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

I am sure so I want to use variables in SELECTs. One interesting case is
using variables in RLS.

I prefer to fix this error message to "column or variable ... does not
exist"

>
>
> 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.
>
>
I have to check it.

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

I'll disable it.

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