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
<mailto: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.
--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org
