Skip to content
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

Inconsistencies with PSQL FUNCTION vs UDF [CORE5905] #6163

Closed
firebird-issue-importer opened this issue Aug 30, 2018 · 22 comments
Closed

Inconsistencies with PSQL FUNCTION vs UDF [CORE5905] #6163

firebird-issue-importer opened this issue Aug 30, 2018 · 22 comments

Comments

@firebird-issue-importer

Submitted by: Maxim Kuzmin (cybermax)

In database created UDF "FORMAT_DATE". For test, I create INTERNAL FUNCTION FORMAT_DATE (introduced in FB 3.0):
CREATE FUNCTION FORMAT_DATE
RETURNS INTEGER
AS
BEGIN
RETURN 1;
END

After this, I can't use UDF - Firebird always use internal function.
Upon attempt drop internal function FORMAT_DATE, I get error:
DROP FUNCTION FORMAT_DATE

This operation is not defined for system tables.
unsuccessful metadata update.
cannot delete.
UDF FORMAT_DATE.
there are 294 dependencies.

And now I can not delete the test function.

Commits: b54ff50 a7e1624

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Aug 30, 2018

Modified by: @asfernandes

assignee: Adriano dos Santos Fernandes [ asfernandes ]

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 11, 2018

Commented by: @asfernandes

Cannot reproduce. Please send full reproducible test case.

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 12, 2018

Commented by: Maxim Kuzmin (cybermax)

declare external function addDay
timestamp, int
returns timestamp
entry_point 'addDay' module_name 'fbudf';

--
SET TERM ^ ;

CREATE OR ALTER PROCEDURE SP_ADDDAY (
TS TIMESTAMP,
DAYS INTEGER)
RETURNS (
RESULT TIMESTAMP)
AS
BEGIN
RESULT = ADDDAY(:TS, :DAYS);

SUSPEND;

END^

SET TERM ; ^

--
CREATE OR ALTER FUNCTION ADDDAY (
DAYS INTEGER,
TS TIMESTAMP)
RETURNS TIMESTAMP
AS
BEGIN
RETURN DATEADD(DAY, :DAYS, :TS);
END

After this, PSQL-function ADDDAY have a 1 dependence.

--
DROP FUNCTION ADDDAY

This operation is not defined for system tables.
unsuccessful metadata update.
cannot delete.
UDF ADDDAY.
there are 1 dependencies.

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 12, 2018

Commented by: Sean Leyne (seanleyne)

I believe the correct handling of this issue is to prevent the new FUNCTION from being defined, since it's name collides with the existing/previously defined EXTERNAL FUNCTION.

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 12, 2018

Commented by: @pavel-zotov

It is 'ALTER FUNCTION' that is causes problem:

-- doing on new DB:
set bail on;
set list on;

set term ^;
declare external function substrlen
cstring(255), smallint, smallint
returns cstring(255) free_it
entry_point 'IB_UDF_substrlen' module_name 'ib_udf'
^

alter function substrlen(input_str varchar(255), i smallint, n smallint) returns varchar(255) as
begin
rdb$set_context('USER_SESSION', 'WHO_WAS_INVOKED', 'Yes');
return substring(input_str from i for n);
end
^
set term ^;
commit;

select substrlen( '1234567890', 5, 4) from rdb$database;
commit;

select rdb$get_context('USER_SESSION', 'WHO_WAS_INVOKED') as "was_psql_invoked ?" from rdb$database;
commit;

-- Issues:

was_psql_invoked ? Yes

So, UDF became 'invisible' after ALTER FUNCTION was done.
Statement ''DROP FUNCTION substrlen' will remove both of them - PSQL and UDF.

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 12, 2018

Commented by: Maxim Kuzmin (cybermax)

Pavel Zotov,

You're right, reason in ALTER. When use CREATE:
CREATE FUNCTION ADDDAY (
DAYS INTEGER,
TS TIMESTAMP)
RETURNS TIMESTAMP
AS
BEGIN
RETURN DATEADD(DAY, :DAYS, :TS);
END

Get an error:
This operation is not defined for system tables.
unsuccessful metadata update.
CREATE FUNCTION ADDDAY failed.
Function ADDDAY already exists.

Description wrote from memory, and did not notice that in the original query was CREATE OR ALTER.

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 12, 2018

Commented by: @asfernandes

It seems people does not understood relation of UDFs and functions.

It's perfectly valid to change a DECLARE'd function to a PSQL function using ALTER FUNCTION.

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 12, 2018

Modified by: @asfernandes

status: Open [ 1 ] => Resolved [ 5 ]

resolution: Won't Fix [ 2 ]

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 12, 2018

Modified by: @pcisar

status: Resolved [ 5 ] => Closed [ 6 ]

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 13, 2018

Commented by: Maxim Kuzmin (cybermax)

Adriano,

I partially agree with you. Let the ALTER of UDF to PSQL-function is valid operation. But when executing DROP this function, it is no longer UDF, although text of message says it's UDF.
This operation is not defined for system tables.
unsuccessful metadata update.
cannot delete.
UDF ADDDAY.
there are 1 dependencies.

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 13, 2018

Commented by: @asfernandes

1) That has nothing to do with the mix of DECLARE/FUNCTION too.
2) Why you think a PSQL function is not also an UDF (User Defined Function)? :) It is not a function, and it is not defined by user?
3) It can be changed, then someone will say that when he tries to delete a UDF the term Function appears.

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 13, 2018

Commented by: @pavel-zotov

> PSQL function is not also an UDF (User Defined Function)?
No, PSQL is not UDF from those who work with FB for many years. This is another kind of DB object.
UDF requires external library; UDF often is out of control - its source code can be unavaliable; UDF is dangerous because can crash FB; UDF is platform-dependent; UDF can not be declared as deterministic, and so on.

At least warning should be raised when we try to replace UDF with PSQL function, because this is implicit replacement of different DB object kinds.
Or, better, do not allow such replacement at all - developer must recognize that he is attempting to introduce duplicate names and first has to DROP old object.
IMO.

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 13, 2018

Commented by: Maxim Kuzmin (cybermax)

Let's look in RDB$FUNCTIONS.

SELECT 'RDB$FUNCTION_NAME: ' || COALESCE(F.RDB$FUNCTION_NAME, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$FUNCTION_NAME: ' || COALESCE(F.RDB$FUNCTION_NAME, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$QUERY_NAME: ' || COALESCE(F.RDB$QUERY_NAME, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$DESCRIPTION: ' || COALESCE(F.RDB$DESCRIPTION, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$MODULE_NAME: ' || COALESCE(F.RDB$MODULE_NAME, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$ENTRYPOINT: ' || COALESCE(F.RDB$ENTRYPOINT, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$RETURN_ARGUMENT: ' || COALESCE(F.RDB$RETURN_ARGUMENT, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$SYSTEM_FLAG: ' || COALESCE(F.RDB$SYSTEM_FLAG, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$ENGINE_NAME: ' || COALESCE(F.RDB$ENGINE_NAME, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$PACKAGE_NAME: ' || COALESCE(F.RDB$PACKAGE_NAME, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$FUNCTION_SOURCE: ' || IIF(F.RDB$FUNCTION_SOURCE IS NOT NULL, 'EXIST', 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$FUNCTION_ID: ' || COALESCE(F.RDB$FUNCTION_ID, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$FUNCTION_BLR: ' || IIF(F.RDB$FUNCTION_BLR IS NOT NULL, 'EXIST', 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$DEBUG_INFO: ' || IIF(F.RDB$DEBUG_INFO IS NOT NULL, 'EXIST', 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$SECURITY_CLASS: ' || COALESCE(F.RDB$SECURITY_CLASS, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$OWNER_NAME: ' || COALESCE(F.RDB$OWNER_NAME, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$LEGACY_FLAG: ' || COALESCE(F.RDB$LEGACY_FLAG, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
|| 'RDB$DETERMINISTIC_FLAG: ' || COALESCE(F.RDB$DETERMINISTIC_FLAG, 'NULL') || ASCII_CHAR(13) || ASCII_CHAR(10)
FROM RDB$FUNCTIONS F WHERE F.RDB$FUNCTION_NAME = 'ADDDAY'

If UDF changed to PSQL, we have next data:
RDB$FUNCTION_NAME: ADDDAY
RDB$FUNCTION_NAME: ADDDAY
RDB$QUERY_NAME: NULL
RDB$DESCRIPTION: NULL
RDB$MODULE_NAME: NULL
RDB$ENTRYPOINT: NULL
RDB$RETURN_ARGUMENT: 0
RDB$SYSTEM_FLAG: 0
RDB$ENGINE_NAME: NULL
RDB$PACKAGE_NAME: NULL
RDB$FUNCTION_SOURCE: EXIST
RDB$FUNCTION_ID: 2
RDB$FUNCTION_BLR: EXIST
RDB$DEBUG_INFO: EXIST
RDB$SECURITY_CLASS: SQL$428
RDB$OWNER_NAME: SYSDBA
RDB$LEGACY_FLAG: 1
RDB$DETERMINISTIC_FLAG: 0

If ADDDAY created at once as PSQL, we have:
RDB$FUNCTION_NAME: ADDDAY
RDB$FUNCTION_NAME: ADDDAY
RDB$QUERY_NAME: NULL
RDB$DESCRIPTION: NULL
RDB$MODULE_NAME: NULL
RDB$ENTRYPOINT: NULL
RDB$RETURN_ARGUMENT: 0
RDB$SYSTEM_FLAG: 0
RDB$ENGINE_NAME: NULL
RDB$PACKAGE_NAME: NULL
RDB$FUNCTION_SOURCE: EXIST
RDB$FUNCTION_ID: 3
RDB$FUNCTION_BLR: EXIST
RDB$DEBUG_INFO: EXIST
RDB$SECURITY_CLASS: SQL$462
RDB$OWNER_NAME: SYSDBA
RDB$LEGACY_FLAG: 0
RDB$DETERMINISTIC_FLAG: 0

As you see, difference in RDB$LEGACY_FLAG. After ALTER, the change of type is not pure.

Also, if ADDAY created as PSQL-function, execute query below:
DROP EXTERNAL FUNCTION ADDDAY
This operation is not defined for system tables.
unsuccessful metadata update.
cannot delete.
UDF ADDDAY.
there are 1 dependencies.

But ADDDAY is not external, it's PSQL function!

2) Historically, UDF - it's EXTERNAL function. Pre-3.0 UDF have an entry point and module name, written by c++, delphi and other language. Hiding from developer an internal core of function - external or PSQL - IMO, it's bad idea, which only confuses.
Exact messaging to user a type of function is necessary for normal work. For example:
cannot delete.
External UDF ADDDAY.
there are 1 dependencies.

Or

cannot delete.
PSQL/Internal UDF ADDDAY.
there are 1 dependencies.

I may not know how the function is declared, and for this knowledge it is necessary to look into the function.

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 13, 2018

Commented by: @dyemanov

From the engine perspective, UDF is *any* function, be it external or internal. Moreover, it can be external (backed by DLL) even if defined using the new syntax for functions (CREATE FUNCTION ... EXTERNAL NAME | EXTERNAL ENGINE) -- it's not UDF as you prefer to see it but still external. So I believe reporting all these cases as UDF (or just function) is OK.

The question is whether changing from legacy UDF to new-style UDF should be allowed via ALTER. From one side, ALTER FUNCTION is allowed to change between PSQL and external (UDR) bodies, so it looks logical to apply this to legacy UDFs. But backward conversion PSQL->UDF is impossible this way. And we also have a separate ALTER EXTERNAL FUNCTION statement to change ENTRY_POINT | MODULE_NAME for legacy UDFs, that adds even more confusion. From another side, the current behaviour allows you to migrate from legacy UDFs (which are deprecated) to newer/safer alternatives (either PSQL-based or UDR) without need to care about dependencies which IMHO is a lovely bonus.

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 13, 2018

Commented by: Maxim Kuzmin (cybermax)

Dmitry Yemanov,

Possible change type of function from old-style to new - it's greatly. But:
1. User can change type accidentally - as I, while testing or any another reason. Be sure that the user wants this. I suggest adding a keyword that allows a type change, or make possible return to old-style without drop all dependencies.
2. This possible described in the documentation? I not remember this.

I disagree with using word UDF and FUNCTION in different messages for one purpose.
For drop:
cannot delete.
UDF ADDDAY.
there are 1 dependencies.

For select
SELECT ADDDAY(1) FROM RDB$DATABASE

Unsuccessful execution caused by a system error that precludes successful execution of subsequent statements.
Dynamic SQL Error.
Input parameter mismatch for function ADDDAY.

Exception in function:
Overflow occurred during data type conversion.
conversion error from string "14-SEP-2018 17:08:37.4740".
At function 'ADDDAY' line: 7, col: 5.

Adriano,

Bug with RDB$LEGACY_FLAG will be fixed?

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 13, 2018

Commented by: @asfernandes

I propose fix of RDB$LEGACY_FLAG if there is problem and change message "UDF" (reported in dependencies) to "Function".

Surely there is a problem to revert from PSQL function to legacy UDF, but if for a long time it got unnoticed, I don't see a need to implement this now after legacy UDF has been officialy deprecated.

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 13, 2018

Commented by: @dyemanov

@adriano: I agree.

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 13, 2018

Modified by: @asfernandes

summary: Mixing INTERNAL FUNCTION and UDF => Inconsistencies with PSQL FUNCTION vs UDF

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 13, 2018

Modified by: @asfernandes

status: Closed [ 6 ] => Reopened [ 4 ]

resolution: Won't Fix [ 2 ] =>

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 13, 2018

Modified by: @asfernandes

status: Reopened [ 4 ] => Resolved [ 5 ]

resolution: Fixed [ 1 ]

Fix Version: 4.0 Beta 1 [ 10750 ]

Fix Version: 3.0.4 [ 10863 ]

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 14, 2018

Modified by: @pavel-zotov

status: Resolved [ 5 ] => Resolved [ 5 ]

QA Status: No test => Done successfully

@firebird-issue-importer
Copy link
Author

firebird-issue-importer commented Sep 14, 2018

Modified by: @pavel-zotov

status: Resolved [ 5 ] => Closed [ 6 ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment