The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, passed
Spec compliant: tested, failed
Documentation: tested, failed
Hello Pavel
First thanks for working on this patch cause it might be really helpful for
those of us trying to migrate PL code between RDBMs.
I tried your patch for migrating an Oracle package body to PL/pgSQL after also
testing a solution using set_config and current_setting (which works but I'm
not really satisfied by this workaround solution).
So I compiled latest postgres sources from github on Linux (redhat 7.7) using
only your patch number 1 (I did not try the second part of the patch).
For my use-case it's working great, performances are excellent (compared to
other solution for porting "package variables").
I did not test all the features involved by the patch (especially ALTER
variable).
I have some feedback however :
1) Failure when using pg_dump 13 on a 12.1 database
When exporting a 12.1 database using pg_dump from the latest development
sources I have an error regarding variables export
[pg12@TST-LINUX-PG-03 ~]$ /opt/postgres12/pg12/bin/pg_dump -h localhost -p 5432
-U postgres -f dump_pg12.sql database1
pg_dump: error: query failed: ERROR: relation "pg_variable" does not exist
LINE 1: ...og.pg_get_expr(v.vardefexpr,0) as vardefexpr FROM pg_variabl...
^
pg_dump: error: query was: SELECT v.tableoid, v.oid, v.varname, v.vareoxaction,
v.varnamespace,
(SELECT rolname FROM pg_catalog.pg_roles WHERE oid = varowner) AS rolname
, (SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM (SELECT acl, row_n
FROM pg_catalog.unnest(coalesce(v.varacl,pg_catalog.acldefault('V',v.varowner)))
WITH ORDINALITY AS perm(acl,row_n)
WHERE NOT EXISTS ( SELECT 1 FROM
pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault('V',v.varowner)))
AS init(init_acl)
WHERE acl = init_acl)) as foo) as varacl, ...:
I think that it should have worked anyway cause the documentation states :
https://www.postgresql.org/docs/current/upgrading.html
"It is recommended that you use the pg_dump and pg_dumpall programs from the
newer version of PostgreSQL, to take advantage of enhancements that might have
been made in these programs." (that's what I did here)
I think there should be a way to avoid dumping the variable if they don't
exist, should'nt it ?
2) Displaying the variables + completion
I created 2 variables using :
CREATE VARIABLE my_pkg.g_dat_deb varchar(11);
CREATE VARIABLE my_pkg.g_dat_fin varchar(11);
When I try to display them, I can only see them when prefixing by the schema :
bdd13=> \dV
"Did not find any schema variables."
bdd13=> \dV my_pkg.*
List of variables
Schema | Name | Type | Is nullable | Default |
Owner | Transactional end action
------------+----------------+-----------------------+-------------+---------+-------+--------------------------
my_pkg| g_dat_deb | character varying(11) | t | | myowner
|
my_pkg| g_dat_fin | character varying(11) | t | |
myowner |
(3 rows)
bdd13=> \dV my_pkg
Did not find any schema variable named "my_pck".
NB : Using this template, functions are returned, maybe variables should also
be listed ? (here by querying on "my_pkg%")
cts_get13=> \dV my_p [TAB]
=> completion using [TAB] key is not working
Is this normal that I cannot see all the variables when not specifying any
schema ?
Also the completion works for functions, but not for variable.
That's just some bonus but it might be good to have it.
I think the way variables are listed using \dV should match with \df for
querying functions
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.
Otherwise the documentation looks good to me.
Regards
Rémi
