-
-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added the ability to change sql security option without specifying the entire body of the function/procedure/package #7807
Conversation
Added the ability to change sql security (definer / invoker / drop) to the existing syntax ALTER FUNCTION { DETERMINISTIC | NOT DETERMINISTIC }. I did it by analogy with "alter_domain_ops" The deterministic field type was changed from bool to TriState. The sql security field type was changed from TriState to std::optional because the SS_DROP state was added. |
SQL SECURITY must be able to change in procedures too. Moreover, it is desirable to have a similar syntax. |
See also #7744 |
I'm fine with this implementation, please go ahead for stored procedures. |
src/dsql/PackageNodes.epp
Outdated
@@ -215,6 +215,9 @@ string CreateAlterPackageNode::internalPrint(NodePrinter& printer) const | |||
|
|||
DdlNode* CreateAlterPackageNode::dsqlPass(DsqlCompilerScratch* dsqlScratch) | |||
{ | |||
if(alter && !items) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(alter && !items) | |
if (alter && !items) |
src/dsql/PackageNodes.epp
Outdated
if (alter) | ||
{ | ||
if (!executeAlter(tdbb, dsqlScratch, transaction)) | ||
if(alterIndividualParameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(alterIndividualParameters) | |
if (alterIndividualParameters) |
src/dsql/PackageNodes.epp
Outdated
if (!executeAlter(tdbb, dsqlScratch, transaction)) | ||
if(alterIndividualParameters) | ||
{ | ||
if(!executeAlterIndividualParameters(tdbb, dsqlScratch, transaction)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(!executeAlterIndividualParameters(tdbb, dsqlScratch, transaction)) | |
if (!executeAlterIndividualParameters(tdbb, dsqlScratch, transaction)) |
src/dsql/PackageNodes.epp
Outdated
MODIFY PKG | ||
if (ssDefiner.has_value()) | ||
{ | ||
if(ssDefiner.value() != SqlSecurity::SS_DROP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(ssDefiner.value() != SqlSecurity::SS_DROP) | |
if (ssDefiner.value() != SqlSecurity::SS_DROP) |
src/dsql/parse.y
Outdated
@@ -2707,9 +2707,13 @@ procedure_clause | |||
| external_procedure_clause | |||
; | |||
|
|||
%type <createAlterProcedureNode> change_opt_procedure_clause | |||
change_opt_procedure_clause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this empty rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not empty, the definition is below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why you "declared it" here?
%type <createAlterProcedureNode> change_opt_procedure_clause
change_opt_procedure_clause
;
src/dsql/parse.y
Outdated
@@ -2966,6 +2987,16 @@ package_clause | |||
} | |||
; | |||
|
|||
%type <createAlterPackageNode> change_opt_package_clause | |||
change_opt_package_clause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change_opt_package_clause
and change_opt_procedure_clause
could be put directly in the alter rules?
I think these rules has not so great names to be put standalone and they are not reused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made rules change_opt_package_clause
and change_opt_procedure_clause
to separate full alter and partial alter. I can change the names to partial_alter_package_clause
and partial_alter_procedure_clause
. This way it will be clearer and will separate full alter and partial alter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You call it partial and I call it just another form of alter. Every alter is partial, they change what was specified. So my suggestion to just use another branch of the original rules directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Every alter is partial, they change what was specified." - is not correct for functions, procedures and packages.
Functions/procedures/packages change all parameters, even those that were not specified. For example:
ALTER FUNCTION func1 (str varchar(5))
RETURNS INT
AS
DECLARE result INTEGER;
BEGIN
result = POSITION('a', str);
RETURN result;
END;
The deterministic and sql security parameters were not specified, but are still being modified
(reset).
I will call this a full alter (the executeAlter
method is used).
Now partial alter (executeAlterIndividualParameters
method is used):
Here parameters that are not specified will not be modified. Only the individual specified parameters will be changed.
For example:
ALTER FUNCTION name SQL SECURITY INVOKER
The deterministic parameter is not specified here and it is not modified.
My opinion:
This is different behavior and it should be in different rules.
But it is better to change the name to a more clarifying one: partial_alter_package_clause
for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we have a big inconsistency with triggers:
SQL> create table t1 (n1 integer)!
SQL> create trigger t1_ins inactive before insert position 4 on t1 SQL SECURITY DEFINER as begin end!
SQL> show trigger t1_ins!
Triggers on Table T1:
T1_INS, Sequence: 4, Type: BEFORE INSERT, Inactive, SQL SECURITY DEFINER
Trigger text:
=============================================================================
as begin end
=============================================================================
SQL> alter trigger t1_ins as begin end!
SQL> show trigger t1_ins!
Triggers on Table T1:
T1_INS, Sequence: 4, Type: BEFORE INSERT, Inactive, SQL SECURITY DEFINER
Trigger text:
=============================================================================
as begin end
=============================================================================
src/dsql/DdlNodes.epp
Outdated
{ | ||
altered = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | |
altered = true; | |
} | |
altered = true; |
Unless there will be other issues posted, I'll merge this PR later this week. |
@Zhdanov0, please resolve the existing conflicts. |
QA is covered by test for #7744 |
Added the ability to change sql security opt without specifiying the entire body of the function/procedure/package.
Examples:
ALTER FUNCTION name DETERMINISTIC SQL SECURITY DEFINER
ALTER FUNCTION name SQL SECURITY INVOKER
ALTER FUNCTION name NOT DETERMINISTIC DROP SQL SECURITY