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

gpgsql: make statement names actually unique #4929

Merged
merged 3 commits into from Mar 10, 2017

Conversation

Projects
None yet
7 participants
@zeha
Collaborator

zeha commented Jan 21, 2017

Short description

(Likely) Fix #4928. PG needs per-session unique statement names, and that gettimeofday trickery wasn't doing it.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added regression tests
  • added unit tests

@zeha zeha added the auth label Jan 21, 2017

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Jan 23, 2017

Member

@rgacogne it was pointed out by @zeha that this leaks pointers over the network, do you see an issue with that?

Member

Habbie commented Jan 23, 2017

@rgacogne it was pointed out by @zeha that this leaks pointers over the network, do you see an issue with that?

@mind04

This comment has been minimized.

Show comment
Hide comment
@mind04

mind04 Jan 23, 2017

Contributor

if the pointer is a problem, the hash of the query is also unique for the connection

Contributor

mind04 commented Jan 23, 2017

if the pointer is a problem, the hash of the query is also unique for the connection

@rgacogne

This comment has been minimized.

Show comment
Hide comment
@rgacogne

rgacogne Jan 24, 2017

Member

Leaking pointer over the network kind of defeats the point of having ASLR so it would be better to use something else like the hash of the query, yes.

Member

rgacogne commented Jan 24, 2017

Leaking pointer over the network kind of defeats the point of having ASLR so it would be better to use something else like the hash of the query, yes.

@cmouse

This comment has been minimized.

Show comment
Hide comment
@cmouse

cmouse Jan 24, 2017

Contributor

so the statements ACTUALLY ended up to having same microsecond value?

Contributor

cmouse commented Jan 24, 2017

so the statements ACTUALLY ended up to having same microsecond value?

@cmouse

This comment has been minimized.

Show comment
Hide comment
@cmouse

cmouse Jan 24, 2017

Contributor

hash of the query should be fine, or just pick random number in front of the timestamp?

Contributor

cmouse commented Jan 24, 2017

hash of the query should be fine, or just pick random number in front of the timestamp?

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Jan 24, 2017

Member

If it's per session, why not just i++?

Member

Habbie commented Jan 24, 2017

If it's per session, why not just i++?

@cmouse

This comment has been minimized.

Show comment
Hide comment
@cmouse

cmouse Jan 24, 2017

Contributor

From postgres manual,

An arbitrary name given to this particular prepared statement. It must be unique within a single session and is subsequently used to execute or deallocate a previously prepared statement.

So yes, simple i++ should work nicely.

Contributor

cmouse commented Jan 24, 2017

From postgres manual,

An arbitrary name given to this particular prepared statement. It must be unique within a single session and is subsequently used to execute or deallocate a previously prepared statement.

So yes, simple i++ should work nicely.

@zeha

This comment has been minimized.

Show comment
Hide comment
@zeha

zeha Jan 31, 2017

Collaborator

Put a counter in there now. Using this certainly needed less changes ;)

Collaborator

zeha commented Jan 31, 2017

Put a counter in there now. Using this certainly needed less changes ;)

@rgacogne

This comment has been minimized.

Show comment
Hide comment
@rgacogne

rgacogne Feb 20, 2017

Member

Looks good, although there is now a conflict that needs to be fixed :)

Member

rgacogne commented Feb 20, 2017

Looks good, although there is now a conflict that needs to be fixed :)

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Mar 9, 2017

Member

Ready to merge this when conflict is resolved

Member

Habbie commented Mar 9, 2017

Ready to merge this when conflict is resolved

@zeha

This comment has been minimized.

Show comment
Hide comment
@zeha

zeha Mar 9, 2017

Collaborator

Resolved using the web editor.

Collaborator

zeha commented Mar 9, 2017

Resolved using the web editor.

@pieterlexis pieterlexis merged commit 9ab3a1c into PowerDNS:master Mar 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

pieterlexis added a commit to pieterlexis/pdns that referenced this pull request Nov 7, 2017

pieterlexis added a commit to pieterlexis/pdns that referenced this pull request Nov 13, 2017

@zeha zeha deleted the zeha:postgres-stmt-name-collision branch Nov 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment