Issue Details (XML | Word | Printable)

Key: CORE-5905
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Adriano dos Santos Fernandes
Reporter: Maxim Kuzmin
Votes: 0
Watchers: 5
Operations

If you were logged in you would be able to see more operations.
Firebird Core

Inconsistencies with PSQL FUNCTION vs UDF

Created: 30/Aug/18 02:05 AM   Updated: 14/Sep/18 06:37 AM
Component/s: None
Affects Version/s: 3.0.0, 3.0.1, 3.0.2, 3.0.3
Fix Version/s: 3.0.4, 4.0 Beta 1

QA Status: Done successfully


 Description  « Hide
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.

 All   Comments   Change History   Subversion Commits      Sort Order: Descending order - Click to sort in ascending order
Dmitry Yemanov added a comment - 13/Sep/18 10:50 AM
@Adriano: I agree.

Adriano dos Santos Fernandes added a comment - 13/Sep/18 10:42 AM
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.

Maxim Kuzmin added a comment - 13/Sep/18 07:13 AM
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?

Dmitry Yemanov added a comment - 13/Sep/18 05:54 AM - edited
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.

Maxim Kuzmin added a comment - 13/Sep/18 05:12 AM
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.

Pavel Zotov added a comment - 13/Sep/18 05:07 AM
> 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.

Adriano dos Santos Fernandes added a comment - 13/Sep/18 01:23 AM
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.

Maxim Kuzmin added a comment - 12/Sep/18 10:41 PM
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.

Adriano dos Santos Fernandes added a comment - 12/Sep/18 03:15 PM
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.

Maxim Kuzmin added a comment - 12/Sep/18 07:47 AM
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.

Pavel Zotov added a comment - 12/Sep/18 04:38 AM
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.


Sean Leyne added a comment - 11/Sep/18 10:39 PM
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.

Maxim Kuzmin added a comment - 11/Sep/18 10:20 PM
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.

Adriano dos Santos Fernandes added a comment - 11/Sep/18 03:28 PM
Cannot reproduce. Please send full reproducible test case.