Re: proposal: schema variables

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