st 26. 2. 2020 v 15:54 odesílatel remi duval <remi.du...@cheops.fr> napsal:
> 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).
>
ALTER VARIABLE is not implemented yet
> 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 ?
>
There was a protection against dump 11, but now it should be Postgres 12.
Fixed.
>
> 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)
>
it is ok - it depends on SEARCH_PATH value
> 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
>
fixed
> 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.
>
This topic is open. I tried to play with it. The problem is syntax. When I
try to reproduce syntax from PLpgSQL, then I need to introduce new reserved
keyword. So my initial idea was wrong.
We need to open discussion about implementable syntax. For this moment you
can use a workaround - any schema variable without WRITE right is constant.
Implementation is easy. Design of syntax is harder.
please see attached patch
Regards
Pavel
>
> Otherwise the documentation looks good to me.
>
> Regards
>
> Rémi
schema-variables-20200226.patch.gz
Description: application/gzip
