public inbox for pgsql-performance@postgresql.org
help / color / mirror / Atom feedFrom: Pavel Stehule <pavel.stehule@gmail.com>
To: Gilles Darold <gilles.darold@dalibo.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [HACKERS] proposal: schema variables
Date: Wed, 27 Jun 2018 13:22:10 +0200
Message-ID: <CAFj8pRBRxJ09ibuZT+KK3E+vc3-sXAz7HrbW3oVie7FwQRU-uQ@mail.gmail.com> (raw)
In-Reply-To: <28924bcc-9242-9798-e4e8-9d83cea3fef6@dalibo.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>
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.
Regards
Pavel
>
> --
> Gilles Darold
> Consultant PostgreSQLhttp://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: pavel.stehule@gmail.com, gilles.darold@dalibo.com, pgsql-hackers@lists.postgresql.org
Subject: Re: [HACKERS] proposal: schema variables
In-Reply-To: <CAFj8pRBRxJ09ibuZT+KK3E+vc3-sXAz7HrbW3oVie7FwQRU-uQ@mail.gmail.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