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".
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" ?
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.
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
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 ?
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.
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"
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 ;-)
Best regards. Philippe.
The new status of this patch is: Waiting on Author
