Feature: Output correct error message from `pdo_firebird` by SakiTakamachi · Pull Request #12669 · php/php-src

SakiTakamachi · 2023-11-14T15:57:02Z

The current implementation does not retrieve sqlstatus from the DB, and any errors will be treated as General errors.

So I changed it to get the SQL state and error messages from the vendor driver.

I also used the same naming conventions as other PDO drivers for function names, etc.

Other changes require error handling, so I'd be happy if I could merge them from this PR.

SakiTakamachi · 2023-11-17T12:59:02Z

@Girgias

Sorry for the change in order, but first, I would like to ask you to review this PR.

SakiTakamachi · 2023-11-17T22:37:57Z

I found some unnecessary parts, so I'll edit them.

SakiTakamachi · 2023-11-18T00:37:43Z

done

Girgias

Looks reasonable to me and only have some suggestions

Girgias · 2023-11-18T04:59:17Z

Won't this duplicate 512 bytes every time?

Judging from the implementation of pestrdup, it looks fine.

php-src/Zend/zend_alloc.c

Line 2684 in a35a69f

ZEND_API char* ZEND_FASTCALL _estrdup(const char *s ZEND_FILE_LINE_DC ZEND_FILE_LINE_ORIG_DC)

However, considering the balance with other changes, it seemed better to use pestrndup, so I changed it.

This has nothing to do with this change, but I'm a little concerned that it haven't given +1 here.

php-src/Zend/zend_alloc.c

Line 2706 in a35a69f

memcpy(p, s, length);

No, it's the opposite. +1 to potentially lost the terminating null

Here, _estrdup is getting the length, so there seems to be no problem. Excuse me, I understand.

Girgias · 2023-11-18T05:04:14Z

Maybe add the length component so that we don't need to recompute known lengths when converting this to zend_string ?

Suggested change

typedef struct {

int sqlcode;

char *errmsg;

} pdo_firebird_error_info;

typedef struct {

int sqlcode;

char *errmsg;

size_t errmsg_length;

} pdo_firebird_error_info;

That's a good idea. Changed pdo_firebird_fetch_error_func to use add_next_index_stringl.

SakiTakamachi · 2023-11-18T07:07:35Z

@Girgias
Thank you, the corrections you pointed out have been completed.

Girgias

Sorry for being slow, but possibly some more improvements (but they could also be left for later)

Girgias · 2023-11-19T07:19:59Z

It seems that the msg argument is always constant at the call site. Precomputing the length at call site and passing it as an argument should allow the compiler to precompute it instead of needing a runtime strlen() in the function.

Changed to require length in argument

Maybe you meant writing the numbers directly in the code without using strlen at all? I may have misunderstood something.

Girgias · 2023-11-19T07:20:43Z

Ditto here, we can precompute the length of the strings and potentially use memcpy() instead.

Changed to require length in argument, and to explicitly pass HY000 where NULL was passed to state

SakiTakamachi · 2023-11-19T09:11:46Z

@Girgias
Thank you, ~~I fixed these.~~

My changes may be inappropriate to save runtime overhead.

SakiTakamachi · 2023-11-19T10:05:36Z

I fixed, I think I've eliminated runtime overhead as much as possible.

Girgias · 2023-11-20T01:21:21Z

This should be better AFAIK

Suggested change

memcpy(*error_code, "HY000", 6);

memcpy(*error_code, "HY000", sizeof("HY000");

Girgias · 2023-11-20T01:22:05Z

Using sizeof of strlen is better, the compiler will fill in the integer constant by itself

Girgias · 2023-11-20T01:22:16Z

SakiTakamachi · 2023-11-20T02:21:20Z

Thank you, I didn't even consider the behavior of the compiler.
I tried changing it, does this work as expected? Or is there a better way to write it?

I'm looking into it while studying while looking at the assembly code.

Girgias · 2023-11-20T03:28:51Z

 							   we won't know what to do with it */
-							H->last_app_error = "Cannot enable auto-commit while a transaction is already open";
+							const char *msg = "Cannot enable auto-commit while a transaction is already open";
+							firebird_error_with_info(dbh, "HY000", strlen("HY000"), msg, strlen(msg));


@nielsdos just to sanity check, but this is sensible right? The compiler will replace at compile time the strlen call by the actual value

Yes it does

Thank you both! I was able to understand it a little. I'll learn some more.

What I wanted to say was a little different.

Although I understand the content, I am still not very good at reading the differences in assembly code, so I will learn to read assembly code better.

I tried various things. I understand that strlen("HY000") will be replaced, but strlen(msg) will not be replaced, is this correct?

Yes, because msg is a variable, where the content is only known at runtime.

I see, thank you. Should msg be replaced as well? I feel the msg is a little too long for that.

I didn't look at this specific case, but in this one the compiler should be smart enough :)

SakiTakamachi · 2023-11-20T13:22:36Z

Thank you!

github-actions Bot added the Extension: pdo_firebird label Nov 14, 2023

SakiTakamachi force-pushed the feature/pdo_firebird_error_code_and_message branch 13 times, most recently from 9c2cc79 to 12fc53b Compare November 17, 2023 02:10

SakiTakamachi changed the title ~~[WIP] Feature: Output correct error message from pdo_firebird~~ Feature: Output correct error message from pdo_firebird Nov 17, 2023

SakiTakamachi marked this pull request as ready for review November 17, 2023 10:24

SakiTakamachi force-pushed the feature/pdo_firebird_error_code_and_message branch from 12fc53b to 7bfb5e6 Compare November 17, 2023 12:05

SakiTakamachi force-pushed the feature/pdo_firebird_error_code_and_message branch 3 times, most recently from 5d5b183 to c252527 Compare November 17, 2023 23:36

Girgias reviewed Nov 18, 2023

View reviewed changes

SakiTakamachi force-pushed the feature/pdo_firebird_error_code_and_message branch from c252527 to 9a435f3 Compare November 18, 2023 06:58

SakiTakamachi force-pushed the feature/pdo_firebird_error_code_and_message branch from 9a435f3 to 2ca4077 Compare November 18, 2023 10:09

Girgias reviewed Nov 19, 2023

View reviewed changes

SakiTakamachi force-pushed the feature/pdo_firebird_error_code_and_message branch from 2ca4077 to 9ae039c Compare November 19, 2023 08:53

SakiTakamachi force-pushed the feature/pdo_firebird_error_code_and_message branch from 9ae039c to 4810a72 Compare November 19, 2023 10:04

Girgias reviewed Nov 20, 2023

View reviewed changes

Changed to get sqlstatus and output appropriate error message

700f92a

SakiTakamachi force-pushed the feature/pdo_firebird_error_code_and_message branch from 4810a72 to 700f92a Compare November 20, 2023 02:19

Girgias reviewed Nov 20, 2023

View reviewed changes

Girgias approved these changes Nov 20, 2023

View reviewed changes

Girgias merged commit 239379b into php:master Nov 20, 2023

SakiTakamachi deleted the feature/pdo_firebird_error_code_and_message branch November 21, 2023 07:18

-typedef struct {
-	int sqlcode;
-	char *errmsg;
-} pdo_firebird_error_info;
+typedef struct {
+	int sqlcode;
+	char *errmsg;
+	size_t errmsg_length;
+} pdo_firebird_error_info;

	memcpy(*error_code, "HY000", 6);
	memcpy(*error_code, "HY000", sizeof("HY000");

Conversation

SakiTakamachi commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Nov 17, 2023

Uh oh!

SakiTakamachi commented Nov 17, 2023

Uh oh!

SakiTakamachi commented Nov 18, 2023

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

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!

SakiTakamachi Nov 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi commented Nov 18, 2023

Uh oh!

Girgias 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!

SakiTakamachi commented Nov 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Nov 19, 2023

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!

SakiTakamachi commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

SakiTakamachi commented Nov 20, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

SakiTakamachi commented Nov 14, 2023 •

edited

Loading

SakiTakamachi Nov 18, 2023 •

edited

Loading

SakiTakamachi commented Nov 19, 2023 •

edited

Loading

SakiTakamachi commented Nov 20, 2023 •

edited

Loading

SakiTakamachi Nov 20, 2023 •

edited

Loading