Hi
ne 22. 12. 2019 v 13:04 odesÃlatel Philippe BEAUDOIN <ph...@apra.asso.fr>
napsal:
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Implements feature: tested, failed
> Spec compliant: not tested
> Documentation: tested, failed
>
> Hi Pavel,
>
> First of all, I would like to congratulate you for this great work. This
> patch is really cool. The lack of package variables is sometimes a blocking
> issue for Oracle to Postgres migrations, because the usual emulation with
> GUC is sometimes not enough, in particular when there are security concerns
> or when the database is used in a public cloud.
>
> As I look forward to having this patch commited, I decided to spend some
> time to participate to the review, although I am not a C specialist and I
> have not a good knowledge of the Postgres internals. Here is my report.
>
> A) Installation
>
> The patch applies correctly and the compilation is fine. The "make check"
> doesn't report any issue.
>
> B) Basic usage
>
> I tried some simple schema variables use cases. No problem.
>
> C) The interface
>
> The SQL changes look good to me.
>
> However, in the CREATE VARIABLE command, I would replace the "TRANSACTION"
> word by "TRANSACTIONAL".
>
There is not technical problem - the problem is in introduction new keyword
"transactional" that is near to "transaction". I am not sure if it is
practical to have two "similar" keyword and how much the CREATE statement
has to use correct English grammar.
I am not native speaker, so I am not able to see how bad is using
"TRANSACTION" instead "TRANSACTIONAL" in this context. So I see a risk to
have two important (it is not syntactic sugar) similar keywords.
Just I afraid so using TRANSACTIONAL instead just TRANSACTION is not too
user friendly. I have not strong opinion about this - and the
implementation is easy, but I am not feel comfortable with introduction
this keyword.
> I have also tried to replace this word by a ON ROLLBACK clause at the end
> of the statement, like for ON COMMIT, but I have not found a satisfying
> wording to propose.
>
>
> D) Behaviour
>
> I am ok with variables not being transactional by default. That's the most
> simple, the most efficient, it emulates the package variables of other
> RDBMS and it will probably fit the most common use cases.
>
> Note that I am not strongly opposed to having by default transactional
> variables. But I don't know whether this change would be a great work. We
> would have at least to find another keyword in the CREATE VARIABLE
> statement. Something like "NON-TRANSACTIONAL VARIABLE" ?
>
Variables almost everywhere (global user settings - GUC is only one planet
exception) are non transactional by default. I don't see any reason
introduce new different design than is wide used.
> It is possible to create a NOT NULL variable without DEFAULT. When trying
> to read the variable before a LET statement, one gets an error massage
> saying that the NULL value is not allowed (and the documentation is clear
> about this case). Just for the records, I wondered whether it wouldn't be
> better to forbid a NOT NULL variable creation that wouldn't have a DEFAULT
> value. But finally, I think this behaviour provides a good way to force the
> variable initialisation before its use. So let's keep it as is.
>
This is a question - and there are two possibilities
postgres=# do $$
declare x int not null;
begin
raise notice '%', x;
end;
$$ ;
ERROR: variable "x" must have a default value, since it's declared NOT NULL
LINE 2: declare x int not null;
^
PLpgSQL requires it. But there is not a possibility to enforce future
setting.
So I know so behave of schema variables is little bit different, but I
think so this difference has interesting use case. You can check if the
variable was modified somewhere or not.
> E) ACL and Rights
>
> I played a little bit with the GRANT and REVOKE statements.
>
> I have got an error (Issue 1). The following statement chain:
> create variable public.sv1 int;
> grant read on variable sv1 to other_user;
> drop owned by other_user;
> reports : ERROR: unexpected object class 4287
>
this is bug and should be fixed
> I then tried to use DEFAULT PRIVILEGES. Despite this is not documented, I
> successfuly performed:
> alter default privileges in schema public grant read on variables to
> simple_user;
> alter default privileges in schema public grant write on variables to
> simple_user;
>
> When variables are then created, the grants are properly given.
> And the psql \ddp command perfectly returns:
> Default access privileges
> Owner | Schema | Type | Access privileges
> ----------+--------+------+-------------------------
> postgres | public | | simple_user=SW/postgres
> (1 row)
>
> So the ALTER DEFAULT PRIVILEGES documentation chapter has to reflect this
> new syntax (Issue 2).
>
> BTW, in the ACL, the READ privilege is represented by a S letter. A
> comment in the source reports that the R letter was used in the past for
> rule privilege. Looking at the postgres sources, I see that this privilege
> on rules has been suppressed in 8.2, so 13 years ago. As this R letter
> would be a so much better choice, I wonder whether it couldn't be reused
> now for this new purpose. Is it important to keep this letter frozen ?
>
I have not a idea why it is. I'll recheck it - but in this moment I prefer
a consistency with existing ACL - it can be in future as one block if it
will be necessary for somebody.
>
> F) Extension
>
> I then created an extension, whose installation script creates a schema
> variable and functions that use it. The schema variable is correctly linked
> to the extension, so that dropping the extension drops the variable.
>
> But there is an issue when dumping the database (Issue 3). The script
> generated by pg_dump includes the CREATE EXTENSION statement as expected
> but also a redundant CREATE VARIABLE statement for the variable that
> belongs to the extension. As a result, one of course gets an error at
> restore time.
>
It is bug and should be fixed
> G) Row Level Security
>
> I did a test activating RLS on a table and creating a POLICY that
> references a schema variable in its USING and WITH CHECK clauses.
> Everything worked fine.
>
> H) psql
>
> A \dV meta-command displays all the created variables.
> I would change a little bit the provided view. More precisely I would:
> - rename "Constraint" into "Is nullable" and report it as a boolean
> - rename "Special behave" into "Is transactional" and report it as a
> boolean
> - change the order of columns so to have:
> Schema | Name | Type | Is nullable | Default | Owner | Is transactional |
> Transaction end action
> "Is nullable" being aside "Default"
>
ok
> I) Performance
>
> I just quickly looked at the performance, and didn't notice any issue.
>
> About variables read performance, I have noticed that:
> select sum(1) from generate_series(1,10000000);
> and
> select sum(sv1) from generate_series(1,10000000);
> have similar response times.
>
> About planning, a condition with a variable used as a constant is
> indexable, as if it were a literal.
>
> J) Documentation
>
> There are some wordings to improve in the documentation. But I am not the
> best person to give advice about english language ;-).
>
> However, aside the already mentionned lack of changes in the ALTER DEFAULT
> PRIVILEGES chapter, I also noticed :
> - line 50 of the patch, the sentence "(hidden attribute; must be
> explicitly selected)" looks false as the oid column of pg_variable is
> displayed, as for other tables of the catalog;
> - at several places, the word "behave" should be replaced by "behaviour"
> - line 433, a get_schema_variable() function is mentionned; is it a
> function that can really be called by users ?
>
> May be it would be interesting to also add a chapter in the Section V of
> the documentation, in order to more globally present the schema variables
> concept, aside the new or the modified statements.
>
> K) Coding
>
> I am not able to appreciate the way the feature has been coded. So I let
> this for other reviewers ;-)
>
>
> To conclude, again, thanks a lot for this feature !
> And if I may add this. I dream of an additional feature: adding a SHARED
> clause to the CREATE VARIABLE statement in order to be able to create
> memory spaces that could be shared by all connections on the database and
> accessible in SQL and PL, under the protection of ACL. But that's another
> story ;-)
>
sure, it is another story :-).
Thank you for review - I'll try to fix bugs this week.
Pavel
> Best regards. Philippe.
>
> The new status of this patch is: Waiting on Author
>
