Re: Schema variables - new implementation for Postgres 15
От | Tomas Vondra |
---|---|
Тема | Re: Schema variables - new implementation for Postgres 15 |
Дата | |
Msg-id | 68eedf25-79ca-d0e7-1af6-bad225fdcd97@enterprisedb.com обсуждение исходный текст |
Ответ на | Re: Schema variables - new implementation for Postgres 15 (Pavel Stehule <pavel.stehule@gmail.com>) |
Ответы |
Re: Schema variables - new implementation for Postgres 15
Re: Schema variables - new implementation for Postgres 15 Re: Schema variables - new implementation for Postgres 15 Re: Schema variables - new implementation for Postgres 15 |
Список | pgsql-hackers |
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
Вложения
В списке pgsql-hackers по дате отправления: