PDO_OCI - Feature PDOStatement::getColumnMeta #3542

Letargie · 2018-09-20T10:26:00Z

Hi,

This PR add the PDOStatement::getColumnMeta() function to the pdo_oci driver that does not implement it.

The 'table' parameter is missing, I didn't find any way of getting it with the oci functions.

Also it seems there isn't any consensus on what the 'native_type' parameter should return (the values are quite different between all implementations of the function)

php bug associated :
https://bugs.php.net/bug.php?id=76908

Regards.

remicollet · 2018-09-20T10:30:55Z

@cjbj can you please review this ?

cjbj · 2018-09-20T12:11:43Z

I'll take a look. At first glance it looks nice and clean.

KalleZ · 2018-09-20T12:28:14Z

The only minor comment I have is the // style comments as according to over CS should be /* */ but thats about it

cjbj

It's nice to see clean code with a solid test. A few tweaks to refine the NCHAR/FLOAT/BINARY_* types would be nice.

cjbj · 2018-09-20T23:25:48Z

ext/pdo_oci/oci_statement.c

+			add_assoc_string(return_value, "oci:decl_type", "DATE");
+			add_assoc_string(return_value, "native_type", "DATE");
+			break;
+		case SQLT_NUM:


You could distinguish between NUMBER and FLOAT by checking the scale
and precision of SQLT_NUM, see
https://www.oracle.com/pls/topic/lookup?ctx=dblatest&id=GUID-529D89A1-EBDF-4940-8CAE-70C6D6594E79

cjbj · 2018-09-20T23:26:17Z

ext/pdo_oci/oci_statement.c

+			add_assoc_string(return_value, "native_type", "ROWID");
+			break;
+		case SQLT_FLT :
+		case SQLT_BFLOAT:


BFLOAT text should be BINARY_FLOAT

You can cross check PDO_OCI by using DESCRIBE in SQL*Plus

cjbj · 2018-09-20T23:26:36Z

ext/pdo_oci/oci_statement.c

+			add_assoc_string(return_value, "oci:decl_type", "FLOAT");
+			add_assoc_string(return_value, "native_type", "FLOAT");
+			break;
+		case SQLT_BDOUBLE:


BDOUBLE text should be BINARY_DOUBLE

cjbj · 2018-09-20T23:27:47Z

ext/pdo_oci/oci_statement.c

+			break;
+		case SQLT_FLT :
+		case SQLT_BFLOAT:
+		case SQLT_IBFLOAT:


I'd drop some of these case constants; Oracle's flexibility of type mappings and use of 'internal' and 'external' types in doc is confusing, even to me. Use the values in https://github.com/oracle/odpi/blob/v3.0.0/src/dpiOracleType.c#L295-L357 (Each DPI_SQLT_xxx is the same as an OCI SQLT_xxx)

From my tests, the OCI_ATTR_DATA_TYPE on the BINARY_FLOAT returns a SQLT_IBFLOAT rather than a SQLT_BFLOAT, same thing for the BINARY_DOUBLE with a SQLT_IBDOUBLE
For now I haven't found a column type returning a SQLT_FLT

cjbj · 2018-09-20T23:28:37Z

ext/pdo_oci/oci_statement.c

+			add_assoc_string(return_value, "oci:decl_type", "LONG RAW");
+			add_assoc_string(return_value, "native_type", "LONG RAW");
+			break;
+		case SQLT_CHR:


Although neither PHP OCI8 or PDO_OCI support fetching NCHAR, NVARCHAR or NCLOB, it should be easy to add those data type names here by checking the "charset form" with OCI_ATTR_CHARSET_FORM, see https://github.com/oracle/odpi/blob/v3.0.0/src/dpiOracleType.c#L400-L408 and https://github.com/oracle/odpi/blob/v3.0.0/src/dpiOracleType.c#L295-L357

cjbj · 2018-09-20T23:31:26Z

ext/pdo_oci/oci_statement.c

+	}
+
+	add_assoc_zval(return_value, "flags", &flags);
+	OCIDescriptorFree(param, OCI_DTYPE_PARAM);


Do we care that errors from calls like STMT_CALL_MSG will cause the function to return without freeing the descriptor? It's a small leak in a rare (?) failure case. Your call.

My code is based on the oci_stmt_describe function which does the same things.
It doesn't free the OCIParam, so i don't know if it's even necessary as it may be free somewhere else.
Or there is also a memory leak problem in that function.

cjbj · 2018-09-20T23:35:22Z

ext/pdo_oci/tests/pdo_oci_stmt_getcolumnmeta.phpt

+?>
+--EXPECT--
+Testing native PS...
+done!


I've never been a fan of the space-saving true/false test output style. How do I know that the test actually does anything? And for me to verify, I have to add my own print calls. And then if any future lazy programmer changes without adding print calls then the test may get silently broken. I would accept this test (after I run it!) but I prefer a different style.

cjbj · 2018-09-20T23:38:31Z

ext/pdo_oci/tests/pdo_oci_stmt_getcolumnmeta.phpt

+
+		$sql = sprintf('CREATE TABLE test(id INT, label %s)', $sql_type);
+		if (!($stmt = @$db->prepare($sql)) || (!@$stmt->execute())) {
+			// Some engines might not support the data type


Since this is being run only for Oracle, I think errors should not be ignored. And all the types validated have been around for some versions of Oracle, so you don't need version dependent code IMHO.

. Add the distinction between NUMBER and FLOAT types . Changing BFLOAT text to be BINARY_FLOAT . Changing BDOUBLE text to be BINARY_DOUBLE . Add the data types names for NCHAR, NVARCHAR and NCLOB . Few changes in the tests Also, my commit is correcting a bug in the function oci_stmt_describe. PDO override the values given for the precision using the content of the pdo_column_data property 'precision'. That value was set in oci_stmt_describe but with the value of OCI_ATTR_SCALE, causing a bug for the precision value returned by getColumnMeta() (values between 12800 and 14000 probably due to the conversion of a sb1 to a zend_ulong)

…hp-src into feature-getcolumnmeta

cjbj

Fix the compilation warnings, add the NLS session setting and this could be merged. I have a few other suggested tweaks, but feel free to ignore them.

Don't forget to update NEWS and mention the enhancement and bug fix separately.

If you are keen, update or add a note to http://php.net/manual/en/pdostatement.getcolumnmeta.php mentioning that Oracle is supported. And that the table name isn't available. The decl_type is shown for functions in Oracle (you can't tell it was a function), which the doc says doesn't happen; this could be noted too.

Good work.

cjbj · 2018-09-23T07:51:17Z

ext/pdo_oci/oci_statement.c

+	pdo_oci_stmt *S = (pdo_oci_stmt*)stmt->driver_data;
+	pdo_oci_column *C;
+	OCIParam *param = NULL;
+	OraText *colname;


@Letargie Can you take a look at the compilation warnings?:

/Users/cjones/php-src/ext/pdo_oci/oci_statement.c:802:11: warning: unused variable 'colname' [-Wunused-variable] OraText *colname; ^ /Users/cjones/php-src/ext/pdo_oci/oci_statement.c:803:12: warning: unused variable 'schema' [-Wunused-variable] OraText * schema; ^ /Users/cjones/php-src/ext/pdo_oci/oci_statement.c:806:6: warning: unused variable 'namelen' [-Wunused-variable] ub4 namelen, schemalen, typelen, objlen; ^ /Users/cjones/php-src/ext/pdo_oci/oci_statement.c:806:15: warning: unused variable 'schemalen' [-Wunused-variable] ub4 namelen, schemalen, typelen, objlen; ^ /Users/cjones/php-src/ext/pdo_oci/oci_statement.c:806:26: warning: unused variable 'typelen' [-Wunused-variable] ub4 namelen, schemalen, typelen, objlen; ^ /Users/cjones/php-src/ext/pdo_oci/oci_statement.c:806:35: warning: unused variable 'objlen' [-Wunused-variable] ub4 namelen, schemalen, typelen, objlen; ^ /Users/cjones/php-src/ext/pdo_oci/oci_statement.c:808:8: warning: unused variable 'str' [-Wunused-variable] char *str; ^ 7 warnings generated.

Oh yes, my bad, will do.

cjbj · 2018-09-23T08:10:08Z

ext/pdo_oci/tests/pdo_oci_stmt_getcolumnmeta.phpt

+
+	echo "Test 2.3 testing temporal columns\n";
+
+	test_meta($db, 160, 'DATE'           , '2008-04-23'        , 'DATE', PDO::PARAM_STR);


This causes a diff in locales like the USA. Try by setting:

export NLS_LANG=.utf8 # or whatever you like - this var is needed otherwise the next isn't looked up export NLS_DATE_FORMAT=DD-MON-YY make test TESTS=ext/pdo_oci/tests/pdo_oci_stmt_getcolumnmeta.phpt

This fails with:

Warning: PDO::exec(): SQLSTATE[HY000]: General error: 1861 OCIStmtExecute: ORA-01861: literal does not match format string

One solution is to wrap the insert with a TO_DATE (which may be tricky since your INSERT forces quotes around the data):

SQL> insert into t values ('2018-09-29'); insert into t values ('2018-09-29') * ERROR at line 1: ORA-01861: literal does not match format string SQL> insert into t values (TO_DATE('2018-09-29', 'YYYY-MM-DD')); 1 row created. SQL>

But it may just be easier to force the connection to the given format by executing this before the rest of the test:

$db->exec("alter session set nls_date_format='YYYY-MM-DD'");

cjbj · 2018-09-23T08:12:50Z

ext/pdo_oci/tests/pdo_oci_stmt_getcolumnmeta.phpt

+		$stmt = $db->prepare($sql);
+		$stmt->execute();
+
+		if (!$db->exec(sprintf("INSERT INTO test(id, label) VALUES (1, '%s')", $value))) {


We strongly advise developers not to use string concatenation for SQL since it reduces statement caching (i.e impacts performance and scalability) and is open to SQL Injection security attacks. Instead, binding is preferred. In the context of this test, the concatenation is OK to do, but not a habit to continue with!

cjbj · 2018-09-23T08:26:17Z

ext/pdo_oci/tests/pdo_oci_stmt_getcolumnmeta.phpt

+	if (false !== ($tmp = $stmt->getColumnMeta(1)))
+		printf("[007] Expecting false because of invalid offset got %s\n", var_export($tmp, true));
+
+	echo "Test 2. testing return values\n";


What about testing the type of some functions like 'select count(*) from dual', or use to_char or to_date?

cjbj · 2018-09-23T08:32:32Z

ext/pdo_oci/oci_statement.c

+	ub2 dtype, data_size;
+	ub2 precis = 0;
+	ub4 namelen, schemalen, typelen, objlen;
+	sb1 scale;


What do you think about changing the definition of getColumnMeta for Oracle and also adding the scale to the output array? In Oracle-land, the precision & scale go together.

Well, IMHO it's a good idea, Oracle provide the scale so why not add it too.
Maybe with 'oci:scale' or just 'scale'.

I'd vote 'scale'.

Letargie · 2018-09-23T18:01:57Z

Thank you very much.
I'll see to make those changes.

…getcolumnmeta

. force the nls_date_format . add the scale to the return of the function . add tests on some function return . removing unused variables

Letargie · 2018-09-26T11:45:28Z

Sorry, took some time.
Didn't have a Oracle database at hand.

remicollet · 2018-09-27T06:59:43Z

@cjbj can you please do the final review ?

cjbj · 2018-09-27T11:24:01Z

@Letargie @remicollet give me a day or two.

cjbj · 2018-10-01T03:17:55Z

@Letargie looks good. LGTM.
@remicollet can you handle the merge for me? Thank you both.

remicollet · 2018-10-01T05:24:38Z

@Letargie thanks for your work
@cjbj thanks for the review

Merged

KalleZ · 2018-10-01T05:45:09Z

Thanks for merging @remicollet just remember to squash the commits ^^

cjbj · 2018-10-02T03:51:02Z

@Letargie the NEWS file was kindly updated by @KalleZ but you could also do a quick PR for it to mention the oci_stmt_describe bug fix 5dedf15

Letargie · 2018-10-02T16:02:18Z

Well I don't know if it really needs a line in the NEWS.
The problem of oci_stmt_describe was just that the scale was passed instead of the precision in the columns.
But the value was only used in getMetaColumn that wasn't implemented yet.

Letargie and others added 2 commits September 20, 2018 12:03

Add the PDOStatement::getColumnMeta() function to the pdo_oci driver

d25d927

Update oci_statement.c

61ae69e

remicollet added the Feature label Sep 20, 2018

fixing comments

b994445

cjbj requested changes Sep 20, 2018

View reviewed changes

Letargie added 3 commits September 21, 2018 13:16

Merge branch 'feature-getcolumnmeta' of https://github.com/Letargie/p…

957ed3c

…hp-src into feature-getcolumnmeta

cs

19e64db

cjbj requested changes Sep 23, 2018

View reviewed changes

Letargie added 3 commits September 26, 2018 11:41

Merge branch 'master' of https://github.com/php/php-src into feature-…

f185ab7

…getcolumnmeta

Last few changes :

478f3d5

. force the nls_date_format . add the scale to the return of the function . add tests on some function return . removing unused variables

Removing last unused

ac842c7

php-pulls merged commit ac842c7 into php:master Oct 1, 2018


		echo "Test 2.3 testing temporal columns\n";

		test_meta($db, 160, 'DATE' , '2008-04-23' , 'DATE', PDO::PARAM_STR);

PDO_OCI - Feature PDOStatement::getColumnMeta #3542

PDO_OCI - Feature PDOStatement::getColumnMeta #3542

Uh oh!

Conversation

Letargie commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

remicollet commented Sep 20, 2018

Uh oh!

cjbj commented Sep 20, 2018

Uh oh!

KalleZ commented Sep 20, 2018

Uh oh!

cjbj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjbj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Letargie commented Sep 23, 2018

Uh oh!

Letargie commented Sep 26, 2018

Uh oh!

remicollet commented Sep 27, 2018

Uh oh!

cjbj commented Sep 27, 2018

Uh oh!

cjbj commented Oct 1, 2018

Uh oh!

remicollet commented Oct 1, 2018

Uh oh!

KalleZ commented Oct 1, 2018

Uh oh!

cjbj commented Oct 2, 2018

Uh oh!

Letargie commented Oct 2, 2018

Uh oh!

Uh oh!

Letargie commented Sep 20, 2018 •

edited

Loading