Re: [HACKERS] proposal: schema variables

Le 27/06/2018 à 13:22, Pavel Stehule a écrit :
> Hi
>
> 2018-06-27 12:21 GMT+02:00 Gilles Darold <gilles.darold@dalibo.com 
> <mailto:gilles.darold@dalibo.com>>:
>
>     Hi,
>
>     I'm reviewing the patch as it was flagged in the current commit
>     fest. Here are my feedback:
>
>      - The patch need to be rebased due to changes in file
>     src/sgml/catalogs.sgml
>
>      - Some compilation warning must be fixed:
>
>         analyze.c: In function ‘transformLetStmt’:
>         analyze.c:1568:17: warning: variable ‘rte’ set but not used
>         [-Wunused-but-set-variable]
>           RangeTblEntry *rte;
>                          ^~~
>         tab-complete.c:1268:21: warning: initialization from
>         incompatible pointer type [-Wincompatible-pointer-types]
>           {"VARIABLE", NULL, &Query_for_list_of_variables},
>
>         In the last warning a NULL is missing, should be written:
>         {"VARIABLE", NULL, NULL, &Query_for_list_of_variables},
>
>
>      - How about Peter's suggestion?:
>         "In DB2, the privileges for variables are named READ and
>     WRITE. That would make more sense to me than reusing the privilege
>     names for tables.
>
>         The patch use SELECT and UPDATE which make sense too for
>     SELECT but less for UPDATE.
>
>      - The implementation of "ALTER VARIABLE varname SET SCHEMA
>     schema_name;" is missing
>
>      - ALTER VARIABLE var1 OWNER TO gilles; ok but not documented and
>     missing in regression test
>
>      - ALTER VARIABLE var1 RENAME TO var2; ok but not documented and
>     missing in regression test
>
>     More generally I think that some comments must be rewritten,
>     especially those talking about a PoC. In documentation there is
>     HTML comments that can be removed.
>
>     Comment at end of file src/backend/commands/schemavar.c generate
>     some "indent with spaces" errors with git apply but perhaps the
>     comment can be entirely removed or undocumented details moved to
>     the right place.
>
>     Otherwise all regression tests passed without issue and especially
>     your new regression tests about schema variables.
>
>     I have a patch rebased, let me known if you want me to post the
>     new diff.
>
>
> I plan significant refactoring of this patch for next commitfest. 
> There was anotherstrong Peter's and Robert comments
>
> 1. The schema variables should to have own system table
> 2. The composite schema variables should to use explicitly defined 
> composite type
> 3. The memory management is not nice - transactional drop table with 
> content is implemented ugly.
>
> I hope, so I can start on these issues next month.
>
> Thank you for review - I'll recheck ALTER commands.
>
>
>     Otherwise all regression tests passed without issue and especially
>     your new regression tests about schema variables.
>
>     I have a patch rebased, let me known if you want me to post the
>     new diff.
>
>
> I plan significant refactoring of this patch for next commitfest. 
> There was anotherstrong Peter's and Robert c
> Regards


Ok Pavel, I've changed the status to "Waiting for authors" so that no 
one will make an other review until you send a new patch.


-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org