Skip to content
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

Add sslmode support for PostgreSQL database #271

Merged
merged 2 commits into from
Sep 14, 2021

Conversation

christianricci
Copy link
Contributor

Allow pg_sslmode argument to be passed into the postgres driver. Keep default argument value to prefer to make it compatible with default postgres value (refer to https://www.postgresql.org/docs/13/libpq-ssl.html)

pg_sslmode can be changed as follows:

hammerdb>dbset db pg
Database set to PostgreSQL

hammerdb>print dict
Dictionary Settings for PostgreSQL
connection {
 pg_host    = localhost
 pg_port    = 5432
 pg_sslmode = prefer
}
...

hammerdb>diset connection pg_sslmode disable
Changed connection:pg_sslmode from prefer to disable for PostgreSQL

hammerdb>print dict
Dictionary Settings for PostgreSQL
connection {
 pg_host    = localhost
 pg_port    = 5432
 pg_sslmode = disable
}
...

All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

Allow pg_sslmode argument to be passed into the postgres driver. Keep default argument value to prefer to make it compatible with default postgres value (refer to https://www.postgresql.org/docs/13/libpq-ssl.html)

pg_sslmode can be changed as follows:

    hammerdb>dbset db pg
    Database set to PostgreSQL

    hammerdb>print dict
    Dictionary Settings for PostgreSQL
    connection {
     pg_host    = localhost
     pg_port    = 5432
     pg_sslmode = prefer
    }
    ...

    hammerdb>diset connection pg_sslmode disable
    Changed connection:pg_sslmode from prefer to disable for PostgreSQL

    hammerdb>print dict
    Dictionary Settings for PostgreSQL
    connection {
     pg_host    = localhost
     pg_port    = 5432
     pg_sslmode = disable
    }
    ...

All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
@sm-shaw
Copy link
Contributor

sm-shaw commented Aug 26, 2021

Many thanks for the pull request - this is something I will take a look at and then put forward for discussion at the TPC-OSS subcommittee. One thing from a quick scan is that it doesn't look like it updates the GUI - so new options would need to appear in here as well.

pgopt

This isn't necessarily a showstopper as it is straightforward to update, so help could be given if needed for this part.

@sm-shaw
Copy link
Contributor

sm-shaw commented Sep 3, 2021

I have run tests on the changes. Overall, most of the implementation is fine, although some options when selected give errors and need updates. So far the issues identified are:

  1. As noted initially the GUI needs updating to include an option to set the sslmode. This is need in both the TPROC-C and TPROC-H interfaces as well as the Transaction Counter interface.
  2. Transaction Counter doesn't work for either TPROC-C or H giving error invalid sslmode value: "user login" - looks like the arguments are being passed in the wrong order.
  3. TPROC-C
    a. When selecting asynchronus scaling it errors with can't read sslmode no such variable.
    b. Connect Pooling option errors with can't read sslmode no such variable. Needs updating to include SSL option in the XML configuration in the connectpool directory.
  4. TPROC-H
    a. When Refresh option is selected, it errors with wrong # of args - looks like there is an extra sslmode parameter in the wrong order.
    I will take a look at fixing these issues and submitting an update to the pull request.

@sm-shaw
Copy link
Contributor

sm-shaw commented Sep 3, 2021

I have created a branch on my fork https://github.com/sm-shaw/HammerDB/tree/pg-sslmode with additional commits that address the issues. More testing is needed to validate all the updates, but once this is done I will create a pull request back to christianricci:pg-sslmode and once accepted this should be good to accept into the main HammerDB repository.

An example of the GUI with the prefer SSL mode update is shown. On is enable, off is disable and the correct option appears in the build or driver script as required.

pgopt

@christianricci
Copy link
Contributor Author

Thanks for taking a look and fixing the bugs, also for adding the UI changes. It looks great !!!

@sm-shaw
Copy link
Contributor

sm-shaw commented Sep 6, 2021

I have done some more tests and found and fixed one more issue when the asynchronous connections and connect pooling were selected at the same time. I have opened a pull request to your branch here christianricci#1. Once accepted, this should update the pull request to the main HammerDB repository with the changes needed.

…se (#1)

* Fix invalid sslmode value for Transaction Counter

* Fix can't read sslmode value when using asynchronous scaling

* Fix wrong number of args with sslmode for TPROC-H Refresh Function

* Add sslmode to XML config for PostgreSQL connect pool

* Add PostgreSQL sslmode option to GUI interface for TPROC-C/H and TX Counter

* Fix sslmode for TPROC-C Asynch with Connect Pool
@christianricci
Copy link
Contributor Author

Thanks Steve. I have squashed/merged your changes into my pull request.

@sm-shaw
Copy link
Contributor

sm-shaw commented Sep 7, 2021

Great, this has now been reviewed and tested all the changes needed are now in the Pull Request. All Pull Requests are discussed by the TPC-OSS subcommittee, so this will be discussed at the next meeting, and accepted subject to approval.

@abondvt89
Copy link
Contributor

@christianricci Hey Christian. Thank you for submitting this PR and working with Steve. Your contribution is greatly appreciated. I noticed above you mentioned submitting the code with the BSD-Free license. Is this a requirement from your company or can it be submitted as GPL code since that is what HammerDB is licensed under? Also, do you have any performance data on SSL on vs off that you could share as we evaluate the performance neutrality of code changes vs the 4.2 release?

@sm-shaw
Copy link
Contributor

sm-shaw commented Sep 8, 2021

For additional clarity the contribution says "BSD-new" license (rather than BSD-Free) which is the Modified BSD as described here by the FSF: https://www.gnu.org/licenses/license-list.html#ModifiedBSD

Modified BSD license (#ModifiedBSD)
This is the original BSD license, modified by removal of the advertising clause. It is a lax, permissive non-copyleft free software license, compatible with the GNU GPL.

Therefore, according to the FSF it is fine to take the contribution as "BSD-new" and include it in HammerDB which is GPLv3 without modifying the contribution to be GPL first, @christianricci is this your understanding as well?

@abondvt89
Copy link
Contributor

Thanks Steve for correcting my error. I think my brain was used to associating the word Free with the word BSD (FreeBSD) ;-). Yes, the license in question is BSD-new. It sounds like it should be fine to accept. @christianricci is that your interpretation as well?

@christianricci
Copy link
Contributor Author

christianricci commented Sep 9, 2021

All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc. Full license text is at https://opensource.org/licenses/BSD-3-Clause and code published with this license is allowed to be combined and republished in HammerDB as GPLv3 by HammerDB.

@sm-shaw sm-shaw merged commit c8e987e into TPC-Council:master Sep 14, 2021
@sm-shaw
Copy link
Contributor

sm-shaw commented Sep 14, 2021

Accepting pull request as voted on September 14, 2021 in the TPC-OSS subcommittee.

@christianricci christianricci deleted the pg-sslmode branch September 14, 2021 22:36
@christianricci
Copy link
Contributor Author

Thanks Steve, most appreciated your bug fixes and UI contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants