Skip to content

ext/pgsql: persistent connections, no need to use such large types fo… #12976

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

Closed

Conversation

devnexen
Copy link
Member

…r boolean states.

also ext/odbc, simplifying odd comparison with non persistent connections.

found while looking at GH-12974.

zend_long allow_persistent;
zend_long auto_reset_persistent;
bool allow_persistent;
bool auto_reset_persistent;
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to also cleanup in the pgsql.c conditions like:

if (PGG(auto_reset_persistent) & 1)
if (PGG(auto_reset_persistent) & 2)

Copy link
Member Author

Choose a reason for hiding this comment

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

not really I need to put it back as previous type, it was done that way on purpose.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's pity that documentation doesn't describe all possible values for this setting, but Detect broken persistent links with pg_pconnect(). Needs a little overhead.

Copy link
Member Author

Choose a reason for hiding this comment

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

true I was thinking this too, I ll look into the doc when I can.

@devnexen devnexen force-pushed the sql_persistent_inconsistencies branch 2 times, most recently from 09a1b03 to 5756063 Compare December 19, 2023 22:10
@devnexen devnexen requested a review from Girgias December 19, 2023 22:14
Comment on lines 186 to 189
zend_long max_links,max_persistent;
zend_long allow_persistent;
bool allow_persistent;
zend_long auto_reset_persistent;
int ignore_notices,log_notices;
Copy link
Member

Choose a reason for hiding this comment

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

@nielsdos wouldn't that change leave some unused bytes in the struct? Should the field be moved to the end?

Copy link
Member

Choose a reason for hiding this comment

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

There's a 7 byte hole after allow_persistent with this change. Moving it to the end won't fill the hole.
I generally also don't try to move too much in the struct to keep all the relevant fields close to each other in the same cache line.
You can fit an int in there though, so you could do:

	zend_long num_links,num_persistent;
	zend_long max_links,max_persistent;
	zend_long allow_persistent;
	bool allow_persistent;
    int ignore_notices;
	zend_long auto_reset_persistent;
	int log_notices;

This is the best you can do.

Copy link
Member Author

Choose a reason for hiding this comment

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

ideally manual padding is better I think but sure I can always do what you mention.

… boolean state.

also ext/odbc, simplifying odd comparison with non persistent connections.
@devnexen devnexen force-pushed the sql_persistent_inconsistencies branch from 5756063 to 2d4bd4a Compare December 20, 2023 17:02
@devnexen devnexen changed the base branch from master to PHP-8.2 December 20, 2023 17:02
@devnexen
Copy link
Member Author

@Girgias does it look good to you now ?

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

LGTM

@devnexen devnexen closed this in d98a45d Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants