Horton

Re: [HACKERS] proposal: schema variables

Pavel Stehule <pavel.stehule@gmail.com>

View thread
From: Pavel Stehule <pavel.stehule@gmail.com>
To: Gilles Darold <gilles.darold@dalibo.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2018-06-27T17:17:05Z
Lists: pgsql-hackers, pgsql-performance

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Move WAL sequence code into its own file

  2. Add ExplainState argument to pg_plan_query() and planner().

  3. Don't include access/htup_details.h in executor/tuptable.h

  4. Refactor to avoid code duplication in transformPLAssignStmt.

  5. Avoid including commands/dbcommands.h in so many places

  6. Restrict psql meta-commands in plain-text dumps.

  7. Split func.sgml into more manageable pieces

  8. Fix squashing algorithm for query texts

  9. EXPLAIN: Always use two fractional digits for row counts.

  10. Preliminary refactoring of plpgsql expression construction.

  11. plpgsql: pure parser and reentrant scanner

  12. Add some sanity checks in executor for query ID reporting

  13. Fix misleading error message context

  14. Add macros for looping through a List without a ListCell.

2018-06-27 19:15 GMT+02:00 Gilles Darold <gilles.darold@dalibo.com>:

> 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>:
>
>> 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
>
>