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)
- 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.
- 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).
- 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
Otherwise, this is a really nice feature and I’m looking forward to have it in
PostgreSQL.
Thanks a lot
Duval Rémi
De : Pavel Stehule [mailto:[email protected]]
Envoyé : jeudi 5 mars 2020 18:54
À : Asif Rehman <[email protected]>
Cc : DUVAL REMI <[email protected]>; PostgreSQL Hackers
<[email protected]>
Objet : Re: proposal: schema variables
čt 5. 3. 2020 v 15:11 odesílatel Asif Rehman
<[email protected]<mailto:[email protected]>> napsal:
On Sat, Feb 29, 2020 at 2:10 PM Pavel Stehule
<[email protected]<mailto:[email protected]>> wrote:
pá 28. 2. 2020 v 16:30 odesílatel Pavel Stehule
<[email protected]<mailto:[email protected]>> napsal:
čt 27. 2. 2020 v 15:37 odesílatel Pavel Stehule
<[email protected]<mailto:[email protected]>> 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<http://www.highgo.ca>
