public inbox for pgsql-performance@postgresql.org
help / color / mirror / Atom feedFrom: Gilles Darold <gilles.darold@dalibo.com>
To: pgsql-hackers@lists.postgresql.org
To: Pavel Stehule <pavel.stehule@gmail.com>
Subject: Re: [HACKERS] proposal: schema variables
Date: Wed, 27 Jun 2018 19:15:54 +0200
Message-ID: <ae98027e-25a7-b229-ffec-b05d68162718@dalibo.com> (raw)
In-Reply-To: <CAFj8pRBRxJ09ibuZT+KK3E+vc3-sXAz7HrbW3oVie7FwQRU-uQ@mail.gmail.com>
References: <CAFj8pRDY+m9OOxfO10R7J0PAkCCauM-TweaTrdsrsLGMb1VbEQ@mail.gmail.com>
<623395617.20171113141500@gf.microolap.com>
<CAFj8pRDdS7ViLBJ6eA93u=C_x15EBv2deiNQDGkBS=LNrjzLLw@mail.gmail.com>
<CAFj8pRBfb-GTZSHSRVTpMzGr26-7e-_RmOmRpmuk+xuDTgC=mA@mail.gmail.com>
<28924bcc-9242-9798-e4e8-9d83cea3fef6@dalibo.com>
<CAFj8pRBRxJ09ibuZT+KK3E+vc3-sXAz7HrbW3oVie7FwQRU-uQ@mail.gmail.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
> <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
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-performance@postgresql.org
Cc: gilles.darold@dalibo.com, pgsql-hackers@lists.postgresql.org, pavel.stehule@gmail.com
Subject: Re: [HACKERS] proposal: schema variables
In-Reply-To: <ae98027e-25a7-b229-ffec-b05d68162718@dalibo.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox