2018-06-27 19:15 GMT+02:00 Gilles Darold <gilles.dar...@dalibo.com>: > Le 27/06/2018 à 13:22, Pavel Stehule a écrit : > > Hi > > 2018-06-27 12:21 GMT+02:00 Gilles Darold <gilles.dar...@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. > sure
Thank you Pavel > > -- > Gilles Darold > Consultant PostgreSQLhttp://dalibo.com - http://dalibo.org > >