-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
ext/pgsql: using ZPP api for calls. #14099
Conversation
851d6c6
to
e3f3972
Compare
I did some testing to improve BCMath performance, but in most cases using On the other hand, there were some areas where it was quite effective. In my opinion, when adding |
To be honest it was mostly negligible most of the time (why I kept as separate commit too), I may not keep it. |
ext/pgsql/pgsql.c
Outdated
if (ZSTR_LEN(tmp) > 0 && *(query + ZSTR_LEN(tmp) - 1) != '\n') { | ||
strlcat(query, "\n", ZSTR_LEN(tmp) + 2); | ||
zquery = zend_string_alloc(ZSTR_LEN(tmp) + 2, false); | ||
if (UNEXPECTED(!zquery)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that zquery
cannot be NULL because zend_string_alloc
cannot fail.
ping :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only made a quick check, I might've missed some things, but you can start from this.
Please fix these mistakes. There's a lot of repeated mistakes so after fixing what I found please double check everything.
ext/pgsql/pgsql.c
Outdated
} | ||
ZEND_PARSE_PARAMETERS_START(0, 1) | ||
Z_PARAM_OPTIONAL | ||
Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no longer accepts pg_close(null) anymore, but the signature is function pg_close(?PgSql\Connection $connection = null): true {}
so it should be allowed.
The same remark holds in other places too.
ext/pgsql/pgsql.c
Outdated
} | ||
ZEND_PARSE_PARAMETERS_START(2, 2) | ||
Z_PARAM_STRING(query, query_len) | ||
Z_PARAM_ZVAL(pv_param_arr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the old parameter parsing using "a" for array, but you now use zval?
ext/pgsql/pgsql.c
Outdated
ZEND_PARSE_PARAMETERS_START(3, 3) | ||
Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce) | ||
Z_PARAM_STRING(query, query_len) | ||
Z_PARAM_ZVAL(pv_param_arr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
ext/pgsql/pgsql.c
Outdated
} | ||
ZEND_PARSE_PARAMETERS_START(2, 2) | ||
Z_PARAM_STRING(stmtname, stmtname_len) | ||
Z_PARAM_ZVAL(pv_param_arr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
ext/pgsql/pgsql.c
Outdated
ZEND_PARSE_PARAMETERS_START(3, 3) | ||
Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce) | ||
Z_PARAM_STRING(stmtname, stmtname_len) | ||
Z_PARAM_ZVAL(pv_param_arr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
ext/pgsql/pgsql.c
Outdated
ZEND_PARSE_PARAMETERS_START(3, 4) | ||
Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce) | ||
Z_PARAM_STR(table) | ||
Z_PARAM_ZVAL(ids) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array
ext/pgsql/pgsql.c
Outdated
} | ||
ZEND_PARSE_PARAMETERS_START(2, 5) | ||
Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce) | ||
Z_PARAM_STR(table) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path
ext/pgsql/pgsql.c
Outdated
Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce) | ||
Z_PARAM_STR(table) | ||
Z_PARAM_OPTIONAL | ||
Z_PARAM_ZVAL(ids) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array
ext/pgsql/pgsql.c
Outdated
} | ||
ZEND_PARSE_PARAMETERS_START(2, 4) | ||
Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce) | ||
Z_PARAM_STR(table_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path
ext/pgsql/pgsql.c
Outdated
} | ||
ZEND_PARSE_PARAMETERS_START(3, 5) | ||
Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce) | ||
Z_PARAM_STR(table_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path
@devnexen Note that we have |
You mean |
Oops, yes |
71692f6
to
4309af0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that if I say "The same remark holds in other places too." it means that there are many other places where it applies, but not necessarily all of the places.
Also a nit: the old code was already using ZPP, ZPP just stands for "Zend Parameter Parsing", the PR title should be "using fast ZPP API for calls"
ext/pgsql/pgsql.c
Outdated
RETURN_THROWS(); | ||
} | ||
ZEND_PARSE_PARAMETERS_START(1, 1) | ||
Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't nullable before, but you made it nullable now. Looking at the stub, it should not be nullable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, at least I learnt a lot of new possible param I did not know existed (esp. path).
ext/pgsql/pgsql.c
Outdated
RETURN_THROWS(); | ||
} | ||
ZEND_PARSE_PARAMETERS_START(3, 3) | ||
Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be nullable.
ext/pgsql/pgsql.c
Outdated
RETURN_THROWS(); | ||
} | ||
ZEND_PARSE_PARAMETERS_START(3, 3) | ||
Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not nullable
ext/pgsql/pgsql.c
Outdated
RETURN_THROWS(); | ||
} | ||
ZEND_PARSE_PARAMETERS_START(3, 4) | ||
Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be nullable
ext/pgsql/pgsql.c
Outdated
RETURN_THROWS(); | ||
} | ||
ZEND_PARSE_PARAMETERS_START(3, 4) | ||
Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be nullable.
ext/pgsql/pgsql.c
Outdated
RETURN_THROWS(); | ||
} | ||
ZEND_PARSE_PARAMETERS_START(4, 5) | ||
Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be nullable
ext/pgsql/pgsql.c
Outdated
RETURN_THROWS(); | ||
} | ||
ZEND_PARSE_PARAMETERS_START(3, 4) | ||
Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be nullable
ext/pgsql/pgsql.c
Outdated
RETURN_THROWS(); | ||
} | ||
ZEND_PARSE_PARAMETERS_START(2, 5) | ||
Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be nullable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one remark remaining
ext/pgsql/pgsql.c
Outdated
if (ZSTR_LEN(tmp) > 0 && *(query + ZSTR_LEN(tmp) - 1) != '\n') { | ||
strlcat(query, "\n", ZSTR_LEN(tmp) + 2); | ||
zend_string *zquery = zend_string_alloc(ZSTR_LEN(tmp) + 2, false); | ||
memcpy(ZSTR_VAL(zquery), ZSTR_VAL(tmp), ZSTR_LEN(tmp) + 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong, no?
You're copying 1 byte outside of the tmp string. It should be + 1 instead of + 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true
ext/pgsql/pgsql.c
Outdated
} | ||
if (PQputCopyData(pgsql, query, (int)strlen(query)) != 1) { | ||
efree(query); | ||
if (PQputCopyData(pgsql, ZSTR_VAL(tmp), ZSTR_LEN(tmp)) != 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of ZSTR_LEN(tmp) is wrong, because you need to include the '\n' if you added it in the if-check above.
ext/pgsql/pgsql.c
Outdated
} | ||
if (PQputCopyData(pgsql, query, (int)strlen(query)) != 1) { | ||
efree(query); | ||
if (PQputCopyData(pgsql, ZSTR_VAL(zquery), ZSTR_LEN(tmp) > 0 ? ZSTR_LEN(tmp) + 1 : 0) != 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you do here, and I'm not sure it matters that we can send over an extra NUL terminator.
However, wouldn't it be better to simply adjust the length of the string zquery
if it didn't have \n at the end initially?
I also realize now that you only need one additional byte instead of 2 for zend_string_alloc.
ext/pgsql/pgsql.c
Outdated
} | ||
if (PQputCopyData(pgsql, query, (int)strlen(query)) != 1) { | ||
efree(query); | ||
if (PQputCopyData(pgsql, ZSTR_VAL(zquery), ZSTR_LEN(tmp) > 0 ? ZSTR_LEN(tmp) + 1 : 0) != 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this what we want?
if (PQputCopyData(pgsql, ZSTR_VAL(zquery), ZSTR_LEN(tmp) > 0 ? ZSTR_LEN(tmp) + 1 : 0) != 1) { | |
if (PQputCopyData(pgsql, ZSTR_VAL(zquery), ZSTR_LEN(zquery)) != 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that s the point of the length changes :) I forgot to update before going to sleep it seems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relatable :P
No description provided.