Re: proposal: schema variables

Hi

so 7. 12. 2024 v 3:13 odesílatel jian he <jian.universality@gmail.com>
napsal:

> On Thu, Dec 5, 2024 at 2:52 PM Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> >
> > Hi
> >
> > only rebase
>
> hi.
> disclaimer, i *only* applied
> v20241205-0001-Enhancing-catalog-for-support-session-variables-and-.patch.
>
> create variable v2 as text;
> alter variable v2 rename to v2;
> ERROR:  session variable "v2" already exists in schema "public"
>
> the above is  coverage tests for report_namespace_conflict:
>         case VariableRelationId:
>             Assert(OidIsValid(nspOid));
>             msgfmt = gettext_noop("session variable \"%s\" already
> exists in schema \"%s\"");
>             break;
>

I don't understand what is an issue?

This is consistent with other database object

(2024-12-07 18:25:29) postgres=# create table foo(a int);
CREATE TABLE
(2024-12-07 18:25:35) postgres=# alter table foo rename
a           COLUMN      CONSTRAINT  TO
(2024-12-07 18:25:35) postgres=# alter table foo rename to foo;
ERROR:  relation "foo" already exists



>
> create type t1 as (a int, b int);
> CREATE VARIABLE var1 AS t1;
> alter type t1 drop attribute a;
> should "alter type t1 drop attribute a;" not allowed?
>

you can add a new attribute, or you can drop the attribute. You cannot
change the type of the attribute.


>
> GetCommandLogLevel also needs to deal with  case T_CreateSessionVarStmt?
>

yes - it should be there - I fixed this + regress test


>
> there are no regress tests for the change we made in
> find_composite_type_dependencies?
> It looks like it will be reachable for sure.
>

It was tested in patch02, where the correct work with variable's values.

But I wrote few tests, that better describe this functionality and I added
to patch01

+SET log_statement TO ddl;
 -- should be ok
 CREATE VARIABLE var1 AS int;
 -- should fail, pseudotypes are not allowed
@@ -15,6 +16,36 @@ CREATE VARIABLE var1 AS int;
 ERROR:  session variable "var1" already exists
 -- should be ok
 DROP VARIABLE IF EXISTS var1;
+-- the variable can use composite types
+CREATE TABLE t1 (a int, b int);
+CREATE VARIABLE var1 AS t1;
+-- should fail
+DROP TABLE t1;
+ERROR:  cannot drop table t1 because other objects depend on it
+DETAIL:  session variable var1 depends on type t1
+HINT:  Use DROP ... CASCADE to drop the dependent objects too.
+-- should be ok
+ALTER TABLE t1 ADD COLUMN c int;
+-- should fail
+ALTER TABLE t1 ALTER COLUMN b TYPE numeric;
+ERROR:  cannot alter table "t1" because session variable "public.var1"
uses it
+DROP VARIABLE var1;
+DROP TABLE t1;
+CREATE TYPE t1 AS (a int, b int);
+CREATE VARIABLE var1 AS t1;
+-- should fail
+DROP TYPE t1;
+ERROR:  cannot drop type t1 because other objects depend on it
+DETAIL:  session variable var1 depends on type t1
+HINT:  Use DROP ... CASCADE to drop the dependent objects too.
+-- should be ok
+ALTER TYPE t1 ADD ATTRIBUTE c int;
+-- should fail
+ALTER TYPE t1 ALTER ATTRIBUTE b  TYPE numeric;
+ERROR:  cannot alter type "t1" because session variable "public.var1" uses
it
+DROP VARIABLE var1;
+DROP TYPE t1;
+SET log_statement TO default;



>
>
> CreateVariable, print out error position:
>     if (get_typtype(typid) == TYPTYPE_PSEUDO)
>         ereport(ERROR,
>                 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>                  errmsg("session variable cannot be pseudo-type %s",
>                         format_type_be(typid)),
>                  parser_errposition(pstate, stmt->typeName->location)));
>
> Alvaro Herrera told me actually, you don't need the extra parentheses
> around errcode.
> so you can:
>     if (get_typtype(typid) == TYPTYPE_PSEUDO)
>         ereport(ERROR,
>                 errcode(ERRCODE_WRONG_OBJECT_TYPE),
>                 errmsg("session variable cannot be pseudo-type %s",
>                         format_type_be(typid)),
>                 parser_errposition(pstate, stmt->typeName->location))
>

yes, it is not necessary, but it is used almost everywhere in Postgres
source code. So I prefer
to be consistent with usual ereport notation. It can be fixed later, but it
is used 7136 times in pg code.
Probably this needs a wide discussion. Without extra parenthesis the code
looks little bit nicer,
but this should be changed by a dedicated patch everywhere.



>
> pg_variable_is_visible seems to have done twice the system cache call.
> maybe follow through with the pg_table_is_visible, pg_type_is_visible
> code pattern.
>
>
done


>
> IN src/bin/psql/describe.c
> + appendPQExpBufferStr(&buf, "\nWHERE true\n");
> this is not needed?
>

Yes, it is a little bit messy, but it is necessary due to the usage of a
ValidateSQLNamePattern - it uses the "AND" operator, and
then there should be some expression before. I think that in this way, the
code is most simple. For objects based on
pg_class table is common usage "\nWHERE prokind = \"x\"\n" with the next
enhancement of filtering. But for variables
There is nothing like filtering inside the table pg_variable, so "WHERE
true\n" is the best analogy. See listConversions()

------------------------------------------------
> some of the `foreach` can change to foreach_oid, foreach_node
> see:
> https://git.postgresql.org/cgit/postgresql.git/commit/?id=14dd0f27d7cd56ffae9ecdbe324965073d01a9ff


done


>
> ------------------------------------------------
> doc/src/sgml/ref/create_variable.sgml
> <programlisting>
> CREATE VARIABLE var1 AS date;
> LET var1 = current_date;
> SELECT var1;
> </programlisting>
>
> v20241205-0001-Enhancing-catalog-for-support-session-variables-and-.patch
> alone cannot do `LET var1 = current_date;`, `SELECT var1;`
> maybe the following patch can do it. but that makes
> we cannot commit
> v20241205-0001-Enhancing-catalog-for-support-session-variables-and-.patch
> alone.
>

moved.

The patches 01 and 02 should be committed together to carry some valuable
functionality.



> ------------------------------------------------
> since CREATE VARIABLE is an extension to standard, so in create_schema.sgml
> Compatibility section,
> do we need to mention CREATE SCHEMA CREATE VARIABLE is an extension
> from standard
> ?
>

done


> -----------------------------------------------
>
> /*
>  * Drop variable by OID, and register the needed session variable
>  * cleanup.
>  */
> void
> DropVariableById(Oid varid)
> i am not sure of the meaning of "register the needed session variable
> cleanup".
>

Without context it is messy. It is related to functionality introduced in
patch [PATCH 05/21] memory cleaning after DROP VARIABLE

So I moved text ", and register the needed session variable
 * cleanup." there

Thank you very much for review

updated patchset attached

Regards

Pavel