Skip to content

PDO_Firebird: Supported Firebird 4.0 datatypes #14897

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

PDO_Firebird: Supported Firebird 4.0 datatypes #14897

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
950de97
PDO_Firebird: Supported Firebird 4.0 datatypes
sim1984 Jul 9, 2024
546af78
tab indent
sim1984 Jul 11, 2024
af79543
add EMPTY_SWITCH_DEFAULT_CASE()
sim1984 Jul 11, 2024
a70cf1b
Merge branch 'php:master' into master
sim1984 Jul 17, 2024
0e056cb
skip if firebird api version < 40
sim1984 Jul 17, 2024
841a1e5
Merge branch 'php:master' into master
sim1984 Jul 18, 2024
4b71218
usage Pdo\Firebird::getApiVersion() in tests
sim1984 Jul 18, 2024
af31c84
Tests fixed.
sim1984 Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions ext/pdo_firebird/firebird_driver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,51 @@ static int php_firebird_preprocess(const zend_string* sql, char* sql_out, HashTa
return 1;
}

#if FB_API_VER >= 40
/* set coercing a data type */
static void set_coercing_data_type(XSQLDA* sqlda)
{
/* Data types introduced in Firebird 4.0 are difficult to process using the Firebird Legacy API. */
/* These data types include DECFLOAT(16), DECFLOAT(34), INT128 (NUMERIC/DECIMAL(38, x)), */
/* TIMESTAMP WITH TIME ZONE, and TIME WITH TIME ZONE. In any case, at least the first three data types */
/* can only be mapped to strings. The last two too, but for them it is potentially possible to set */
/* the display format, as is done for TIMESTAMP. This function allows you to ensure minimal performance */
/* of queries if they contain columns and parameters of the above types. */
unsigned int i;
short dtype;
short nullable;
XSQLVAR* var;
for (i=0, var = sqlda->sqlvar; i < sqlda->sqld; i++, var++) {
dtype = (var->sqltype & ~1); /* drop flag bit */
nullable = (var->sqltype & 1);
switch(dtype) {
case SQL_INT128:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 46;
var->sqlscale = 0;
break;
case SQL_DEC16:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 24;
break;
case SQL_DEC34:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 43;
break;
case SQL_TIMESTAMP_TZ:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 58;
break;
case SQL_TIME_TZ:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 46;
break;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please standardize indentation to TAB.

Suggested change
static void set_coercing_data_type(XSQLDA* sqlda)
{
/* Data types introduced in Firebird 4.0 are difficult to process using the Firebird Legacy API. */
/* These data types include DECFLOAT(16), DECFLOAT(34), INT128 (NUMERIC/DECIMAL(38, x)), */
/* TIMESTAMP WITH TIME ZONE, and TIME WITH TIME ZONE. In any case, at least the first three data types */
/* can only be mapped to strings. The last two too, but for them it is potentially possible to set */
/* the display format, as is done for TIMESTAMP. This function allows you to ensure minimal performance */
/* of queries if they contain columns and parameters of the above types. */
unsigned int i;
short dtype;
short nullable;
XSQLVAR* var;
for (i=0, var = sqlda->sqlvar; i < sqlda->sqld; i++, var++) {
dtype = (var->sqltype & ~1); /* drop flag bit */
nullable = (var->sqltype & 1);
switch(dtype) {
case SQL_INT128:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 46;
var->sqlscale = 0;
break;
case SQL_DEC16:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 24;
break;
case SQL_DEC34:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 43;
break;
case SQL_TIMESTAMP_TZ:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 58;
break;
case SQL_TIME_TZ:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 46;
break;
}
}
}
static void set_coercing_data_type(XSQLDA* sqlda)
{
/* Data types introduced in Firebird 4.0 are difficult to process using the Firebird Legacy API. */
/* These data types include DECFLOAT(16), DECFLOAT(34), INT128 (NUMERIC/DECIMAL(38, x)), */
/* TIMESTAMP WITH TIME ZONE, and TIME WITH TIME ZONE. In any case, at least the first three data types */
/* can only be mapped to strings. The last two too, but for them it is potentially possible to set */
/* the display format, as is done for TIMESTAMP. This function allows you to ensure minimal performance */
/* of queries if they contain columns and parameters of the above types. */
unsigned int i;
short dtype;
short nullable;
XSQLVAR* var;
for (i=0, var = sqlda->sqlvar; i < sqlda->sqld; i++, var++) {
dtype = (var->sqltype & ~1); /* drop flag bit */
nullable = (var->sqltype & 1);
switch(dtype) {
case SQL_INT128:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 46;
var->sqlscale = 0;
break;
case SQL_DEC16:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 24;
break;
case SQL_DEC34:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 43;
break;
case SQL_TIMESTAMP_TZ:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 58;
break;
case SQL_TIME_TZ:
var->sqltype = SQL_VARYING + nullable;
var->sqllen = 46;
break;
}
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, there is an EMPTY_SWITCH_DEFAULT_CASE() macro that can be used for the default case where the switch does nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you insert this macro, the driver crashes as soon as the dtype goes beyond the cases described in case. In my case, for other types you just need to do nothing.

/* }}} */
Copy link
Member

@SakiTakamachi SakiTakamachi Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not necessary /* }}} */

#endif

/* map driver specific error message to PDO error */
void php_firebird_set_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt, const char *state, const size_t state_len,
const char *msg, const size_t msg_len) /* {{{ */
Expand Down Expand Up @@ -605,6 +650,11 @@ static bool firebird_handle_preparer(pdo_dbh_t *dbh, zend_string *sql, /* {{{ */
break;
}

#if FB_API_VER >= 40
/* set coercing a data type */
set_coercing_data_type(&S->out_sqlda);
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, please use consistent indentation.


/* allocate the input descriptors */
if (isc_dsql_describe_bind(H->isc_status, &s, PDO_FB_SQLDA_VERSION, &num_sqlda)) {
break;
Expand All @@ -618,6 +668,11 @@ static bool firebird_handle_preparer(pdo_dbh_t *dbh, zend_string *sql, /* {{{ */
if (isc_dsql_describe_bind(H->isc_status, &s, PDO_FB_SQLDA_VERSION, S->in_sqlda)) {
break;
}

#if FB_API_VER >= 40
/* set coercing a data type */
set_coercing_data_type(S->in_sqlda);
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

}

stmt->driver_data = S;
Expand Down
52 changes: 52 additions & 0 deletions ext/pdo_firebird/tests/fb4_datatypes.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
--TEST--
PDO_Firebird: Supported Firebird 4.0 datatypes
--EXTENSIONS--
pdo_firebird
--SKIPIF--
<?php require('skipif.inc'); ?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Versions lower than 4 may want to skip this test

Copy link
Contributor Author

@sim1984 sim1984 Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to set this restriction?

$dbh->getAttribute(PDO::ATTR_SERVER_VERSION);

Gets the server version, but it looks like this

Firebird/Linux/AMD/Intel/x64 (access method), version "LI-V5.0.1.1391 Firebird 5.0 faca113"
Firebird/Linux/AMD/Intel/x64 (remote server), version "LI-V5.0.1.1391 Firebird 5.0 faca113/tcp (DESKTOP-E3INAFT)/P18:C"
Firebird/Linux/AMD/Intel/x64 (remote interface), version "LI-V5.0.1.1391 Firebird 5.0 faca113/tcp (DESKTOP-E3INAFT)/P18:C"
on disk structure version 13.1

You also need to use a regular expression to make this information simple.

On the other hand, the engine version can be obtained by request

select rdb$get_context('SYSTEM', 'ENGINE_VERSION')
from rdb$database

There is only a version number. But in any case, the server version can only be found after connecting to the database.

Create a separate function that returns the engine version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What matters here is the client version, not the server version. So, could you please try the following constant?

PDO::ATTR_CLIENT_VERSION

Copy link
Contributor Author

@sim1984 sim1984 Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also returns a complex version of the client

LI-V6.3.1.1391 Firebird 5.0

The success of this test depends less on the client (although it does too) and more on the Firebird header files.

#if FB_API_VER >= 40
...
#endif

FB_API_VER is defined in ibase.h, and if the header file is from Firebird 3.0, this branch simply will not end up in the binary file when compiled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you assuming the case that there is a difference between the client version and FB_API_VER? That's certainly a possible problem.

Now, probably the simplest solution here is to be able to retrieve FB_API_VER. In fact, this is useful for users when debugging on their own.

Could you please wait a moment while I open the PR to add?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure. The API version is essentially the version of the header files. And the client version is the version of fbclient.dll or libfbclient.so. They are generally independent.

What is the point of the patch? After preparing query, the server says I have these types of columns and input parameters. We cannot process some of them, so in response we tell the server, instead of these types, return VARCHAR(N) to us. And this part, in theory, can generally work independently of the client (I have not tested).

Now, probably the simplest solution here is to be able to retrieve FB_API_VER. In fact, this is useful for users when debugging on their own.

Could you please wait a moment while I open the PR to add?

OK. I also think this will be useful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened PR
#14916

--XLEAK--
A bug in firebird causes a memory leak when calling `isc_attach_database()`.
See https://github.com/FirebirdSQL/firebird/issues/7849
--FILE--
<?php
require 'testdb.inc';

$sql = <<<'SQL'
SELECT
CAST(15 AS BIGINT) AS i64,
CAST(15 AS INT128) AS i128,
123.97 AS N,
CAST(123.97 AS NUMERIC(38,2)) AS N2,
CAST('2024-05-04 12:59:34.239' AS TIMESTAMP) AS TS,
CAST('2024-05-04 12:59:34.239 Europe/Moscow' AS TIMESTAMP WITH TIME ZONE) AS TS_TZ,
CAST('12:59:34.239' AS TIME) AS T,
CAST('12:59:34.239 Europe/Moscow' AS TIME WITH TIME ZONE) AS T_TZ,
CAST(1.128 AS DECFLOAT(16)) AS df16,
CAST(1.128 AS DECFLOAT(34)) AS df34
FROM RDB$DATABASE
SQL;

$dbh = getDbConnection();

$stmt = $dbh->prepare($sql);
$stmt->execute();
$data = $stmt->fetch(\PDO::FETCH_ASSOC);
$stmt->closeCursor();
$str = json_encode($data, JSON_PRETTY_PRINT);
echo $str;
echo "\ndone\n";
?>
--EXPECT--
{
"I64": 15,
"I128": "15",
"N": "123.97",
"N2": "123.97",
"TS": "2024-05-04 12:59:34",
"TS_TZ": "2024-05-04 12:59:34.2390 Europe\/Moscow",
"T": "12:59:34",
"T_TZ": "12:59:34.2390 Europe\/Moscow",
"DF16": "1.128",
"DF34": "1.128"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my environment, the date format is probably different than yours.

005-     "TS_TZ": "2024-05-04 12:59:34.2390 Europe\/Moscow",
006+     "TS_TZ": "04-MAY-2024 12:59:34.2390 Europe\/Moscow",

I thought about how to specify the format, but I don't want to do that yet because I think it would be quite complicated.

Therefore, could you please change your test expectations to:

Suggested change
--EXPECT--
{
"I64": 15,
"I128": "15",
"N": "123.97",
"N2": "123.97",
"TS": "2024-05-04 12:59:34",
"TS_TZ": "2024-05-04 12:59:34.2390 Europe\/Moscow",
"T": "12:59:34",
"T_TZ": "12:59:34.2390 Europe\/Moscow",
"DF16": "1.128",
"DF34": "1.128"
}
--EXPECTF--
{
"I64": 15,
"I128": "15",
"N": "123.97",
"N2": "123.97",
"TS": "2024-05-04 12:59:34",
"TS_TZ": "%s 12:59:34.2390 Europe\/Moscow",
"T": "12:59:34",
"T_TZ": "12:59:34.2390 Europe\/Moscow",
"DF16": "1.128",
"DF34": "1.128"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

done
44 changes: 44 additions & 0 deletions ext/pdo_firebird/tests/fb4_datatypes_params.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
--TEST--
PDO_Firebird: Supported Firebird 4.0 datatypes (parameters)
--EXTENSIONS--
pdo_firebird
--SKIPIF--
<?php require('skipif.inc'); ?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

--XLEAK--
A bug in firebird causes a memory leak when calling `isc_attach_database()`.
See https://github.com/FirebirdSQL/firebird/issues/7849
--FILE--
<?php
require 'testdb.inc';

$sql = <<<'SQL'
SELECT
CAST(? AS INT128) AS i128,
CAST(? AS NUMERIC(38,2)) AS N2,
CAST(? AS TIMESTAMP WITH TIME ZONE) AS TS_TZ,
CAST(? AS TIME WITH TIME ZONE) AS T_TZ,
CAST(? AS DECFLOAT(16)) AS df16,
CAST(? AS DECFLOAT(34)) AS df34
FROM RDB$DATABASE
SQL;

$dbh = getDbConnection();

$stmt = $dbh->prepare($sql);
$stmt->execute([12, 12.34, '2024-05-04 12:59:34.239 Europe/Moscow', '12:59 Europe/Moscow', 12.34, 12.34]);
$data = $stmt->fetch(\PDO::FETCH_ASSOC);
$stmt->closeCursor();
$str = json_encode($data, JSON_PRETTY_PRINT);
echo $str;
echo "\ndone\n";
?>
--EXPECT--
{
"I128": "12",
"N2": "12.34",
"TS_TZ": "2024-05-04 12:59:34.2390 Europe\/Moscow",
"T_TZ": "12:59:00.0000 Europe\/Moscow",
"DF16": "12.34",
"DF34": "12.34"
}
done
Loading