Re: proposal: schema variables

pá 6. 3. 2020 v 16:44 odesílatel DUVAL REMI <REMI(dot)DUVAL(at)cheops(dot)fr> napsal:

> Hello Pavel
>
>
>
> I tested your patch again and I think things are better now, close to
> perfect for me.
>
>
>
> *1) **Patch review*
>
>
>
> - We can define CONSTANTs with CREATE IMMUTABLE VARIABLE … I’m
> really pleased with this
>
> - The previous bug I mentioned to you by private mail (see
> detail below) has been fixed and a non-regression test case has been added
> for it.
>
> - I’m now able to export a 12.1 database using pg_dump from the
> latest git HEAD (internal version 130000).
>
> NB: the condition is “if internal_version < 130000 => don’t try to export
> any schema variable”.
>
>
>
> Also I was able to test a use case for a complex Oracle package I migrated
> to PostgreSQL (it has a total of 194 variables and constants in it !).
>
> The Oracle package has been migrated to a PostgreSQL schema with one
> routine per Oracle subprogram.
>
> I’m able to run my business test case using schema variables on those
> routines and it’s giving me the expected result.
>
>
>
> NB: Previous bug was
>
> database1=> CREATE VARIABLE T0_var AS char(14);
> CREATE VARIABLE
> database1=> CREATE IMMUTABLE VARIABLE Taille_var AS numeric DEFAULT 14;
> CREATE VARIABLE
> database1=> LET T0_var = LPAD('999', trunc(Taille_var::NUMERIC)::INTEGER,
> '0');
> *ERROR: schema variable "taille_var" is declared IMMUTABLE*
> database1=> LET T0_var = LPAD('999', trunc(Taille_var::NUMERIC)::INTEGER,
> '0');
> *ERROR: variable "taille_var" has not valid content*
>
> ð It’s now fixed !
>
>
>
> *2) **Regarding documentation *
>
>
>
> It’s pretty good except some small details :
>
> - sql-createvariable.html => IMMUTABLE parameter : The value of
> *the* variable cannot be changed. => I think an article is needed here
> (the)
>

fixed

- sql-createvariable.html => ON COMMIT DROP : The ON COMMIT DROP
> clause specifies the bahaviour of temporary => behaviour
> Also there are “tabs” between words (here between “of” and “temporary”),
> making the paragraph look strange.
>

fixed

> - sql-createvariable.html => Maybe we should mention that the
> IMMUTABLE parameter (CREATE IMMUTABLE VARIABLE …) is the way to define
> global CONSTANTs, because people will look for a way to create global
> constants in the documentation and it would be good if they can find the
> CONSTANT word in it.
> Also maybe it’s worth mentioning that “schema variables” relates to
> “global variables” (for the same purpose of people searching in the
> documentation).
>
probably it should be somewhere
https://www.postgresql.org/docs/current/plpgsql-porting.html

I wrote note there

> - sql-altervariable.html => sentence “These restrictions enforce
> that altering the owner doesn't do anything you couldn't do by dropping and
> recreating the variable.“ => not sure I understand this one. Isn’t it
> “does not do anything you could do” instead of “you couln’t do” .. but
> maybe it’s me
>
This sentence is used more times (from alter_view0

To alter the owner, you must also be a direct or indirect member of the new
owning role, and that role must have <literal>CREATE</literal> privilege
on
the view's schema. (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the view.
However, a superuser can alter ownership of any view anyway.)

>
>
> Otherwise, this is a really nice feature and I’m looking forward to have
> it in PostgreSQL.
>

Thank you very much

updated patch attached

>
> Thanks a lot
>
>
>
> Duval Rémi
>
>
>
> *De :* Pavel Stehule [mailto:pavel(dot)stehule(at)gmail(dot)com]
> *Envoyé :* jeudi 5 mars 2020 18:54
> *À :* Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
> *Cc :* DUVAL REMI <REMI(dot)DUVAL(at)CHEOPS(dot)FR>; PostgreSQL Hackers <
> pgsql-hackers(at)lists(dot)postgresql(dot)org>
> *Objet :* Re: proposal: schema variables
>
>
>
>
>
>
>
> čt 5. 3. 2020 v 15:11 odesílatel Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
> napsal:
>
>
>
>
>
> On Sat, Feb 29, 2020 at 2:10 PM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
>
>
>
>
>
> pá 28. 2. 2020 v 16:30 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> napsal:
>
>
>
>
>
> čt 27. 2. 2020 v 15:37 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> napsal:
>
>
>
> Hi
>
>
>
>
> 3) Any way to define CONSTANTs ?
> We already talked a bit about this subject and also Gilles Darold
> introduces it in this mailing-list topic but I'd like to insist on it.
> I think it would be nice to have a way to say that a variable should not
> be changed once defined.
> Maybe it's hard to implement and can be implemented later, but I just want
> to know if this concern is open.
>
>
>
> I played little bit with it and I didn't find any nice solution, but maybe
> I found the solution. I had ideas about some variants, but almost all time
> I had a problem with parser's shifts because all potential keywords are not
> reserved.
>
>
>
> last variant, but maybe best is using keyword WITH
>
>
>
> So the syntax can looks like
>
>
>
> CREATE [ TEMP ] VARIABLE varname [ AS ] type [ NOT NULL ] [ DEFAULT
> expression ] [ WITH [ OPTIONS ] '(' ... ')' ] ]
>
>
>
> What do you think about this syntax? It doesn't need any new keyword, and
> it easy to enhance it.
>
>
>
> CREATE VARIABLE foo AS int DEFAULT 10 WITH OPTIONS ( CONSTANT);
>
>
>
> After some more thinking and because in other patch I support syntax
> CREATE TRANSACTION VARIABLE ... I change my opinion and implemented support
> for
>
> syntax CREATE IMMUTABLE VARIABLE for define constants.
>
>
>
> second try to fix pg_dump
>
>
>
> Regards
>
>
>
> Pavel
>
>
>
>
>
> See attached patch
>
>
>
> Regards
>
>
>
> Pavel
>
>
>
>
>
> ?
>
>
>
> Regards
>
>
>
> Pavel
>
>
>
>
>
>
>
> Hi Pavel,
>
>
>
> I have been reviewing the latest patch (schema-variables-20200229.patch.gz)
>
> and here are few comments:
>
>
>
> 1- There is a compilation error, when compiled with --with-llvm enabled on
>
> CentOS 7.
>
>
>
> llvmjit_expr.c: In function ‘llvm_compile_expr’:
>
> llvmjit_expr.c:1090:5: warning: initialization from incompatible pointer
> type [enabled by default]
>
> build_EvalXFunc(b, mod, "ExecEvalParamVariable",
>
> ^
>
> llvmjit_expr.c:1090:5: warning: (near initialization for ‘(anonymous)[0]’)
> [enabled by default]
>
> llvmjit_expr.c:1090:5: warning: initialization from incompatible pointer
> type [enabled by default]
>
> llvmjit_expr.c:1090:5: warning: (near initialization for ‘(anonymous)[0]’)
> [enabled by default]
>
> llvmjit_expr.c:1090:5: warning: initialization from incompatible pointer
> type [enabled by default]
>
> llvmjit_expr.c:1090:5: warning: (near initialization for ‘(anonymous)[0]’)
> [enabled by default]
>
> llvmjit_expr.c:1090:5: warning: passing argument 5 of ‘build_EvalXFuncInt’
> from incompatible pointer type [enabled by default]
>
> llvmjit_expr.c:60:21: note: expected ‘struct ExprEvalStep *’ but argument
> is of type ‘LLVMValueRef’
>
> static LLVMValueRef build_EvalXFuncInt(LLVMBuilderRef b, LLVMModuleRef
> mod,
>
> ^
>
> llvmjit_expr.c:1092:29: error: ‘i’ undeclared (first use in this function)
>
> LLVMBuildBr(b, opblocks[i + 1]);
>
> ^
>
> llvmjit_expr.c:1092:29: note: each undeclared identifier is reported only
> once for each function it appears in
>
> make[2]: *** [llvmjit_expr.o] Error 1
>
>
>
>
>
> After looking into it, it turns out that:
>
> - parameter order was incorrect in build_EvalXFunc()
>
> - LLVMBuildBr() is using the undeclared variable 'i' whereas it should be
>
> using 'opno'.
>
>
>
>
>
> 2- Similarly, If the default expression is referencing a function or
> object,
>
> dependency should be marked, so if the function is not dropped silently.
>
> otherwise, a cache lookup error will come.
>
>
>
> postgres=# create or replace function foofunc() returns timestamp as $$
> begin return now(); end; $$ language plpgsql;
>
> CREATE FUNCTION
>
> postgres=# create schema test;
>
> CREATE SCHEMA
>
> postgres=# create variable test.v1 as timestamp default foofunc();
>
> CREATE VARIABLE
>
> postgres=# drop function foofunc();
>
> DROP FUNCTION
>
> postgres=# select test.v1;
>
> ERROR: cache lookup failed for function 16437
>
>
>
> Thank you for this analyze and patches. I merged them to attached patch
>
>
>
>
>
>
>
>
>
> 3- Variable DEFAULT expression is apparently being evaluated at the time of
>
> first access. whereas I think that It should be at the time of variable
>
> creation. consider the following example:
>
>
>
> postgres=# create variable test.v2 as timestamp default now();
>
> CREATE VARIABLE
>
> postgres=# select now();
>
> now
>
> -------------------------------
>
> 2020-03-05 12:13:29.775373+00
>
> (1 row)
>
> postgres=# select test.v2;
>
> v2
>
> ----------------------------
>
> 2020-03-05 12:13:37.192317 -- I was expecting this to be earlier than the
> above timestamp.
>
> (1 row)
>
>
>
> postgres=# select test.v2;
>
> v2
>
> ----------------------------
>
> 2020-03-05 12:13:37.192317
>
> (1 row)
>
> postgres=# let test.v2 = default;
>
> LET
>
> postgres=# select test.v2;
>
> v2
>
> ----------------------------
>
> 2020-03-05 12:14:07.538615
>
> (1 row)
>
>
>
> This is expected and wanted - same behave has plpgsql variables.
>
>
>
> CREATE OR REPLACE FUNCTION public.foo()
> RETURNS void
> LANGUAGE plpgsql
> AS $function$
> declare x timestamp default current_timestamp;
> begin
> raise notice '%', x;
> end;
> $function$
>
>
>
> postgres=# select foo();
> NOTICE: 2020-03-05 18:49:12.465054
> ┌─────┐
> │ foo │
> ╞═════╡
> │ │
> └─────┘
> (1 row)
>
> postgres=# select foo();
> NOTICE: 2020-03-05 18:49:13.255197
> ┌─────┐
> │ foo │
> ╞═════╡
> │ │
> └─────┘
> (1 row)
>
>
>
> You can use
>
>
>
> CREATE VARIABLE cuser AS text DEFAULT session_user;
>
>
>
> Has not any sense to use a value from creating time
>
>
>
> And a analogy with CREATE TABLE
>
>
>
> CREATE TABLE fooo(a timestamp DEFAULT current_timestamp) -- there is not a
> create time timestamp
>
>
>
>
>
> I fixed buggy behave of IMMUTABLE variables
>
>
>
> Regards
>
>
>
> Pavel
>
>
>
>
>
>
>
> To continue my testing of the patch I made few fixes for the
> above-mentioned
>
> comments. The patch for those changes is attached if it could be of any
> use.
>
>
>
> --
>
> Asif Rehman
>
> Highgo Software (Canada/China/Pakistan)
> URL : www.highgo.ca
>
>