postgresql-interfaces / psqlodbc

Other
16 stars 13 forks source link

Use mimalloc to improve performance and reduce memory allocation lock contention #6

Closed apgrucza closed 5 months ago

apgrucza commented 7 months ago

We have a multi-threaded Windows application that was experiencing delays due to high lock contention in memory allocations from the PostgreSQL ODBC driver. We tried modifying the driver to use mimalloc, which is a memory allocator with better performance characteristics. After deploying this change, the delays due to lock contention disappeared. It has been running on thousands of our production deployments for 9 months without issue.

I've created this pull request so that others can benefit from this change by building the driver with the _MIMALLOC_ symbol defined and linking to the mimalloc library. If building on Windows, BuildAll.ps1 accepts a -UseMimalloc argument that does this for you (requires a toolset of v141, v142 or later). Below is an example of how to build with mimalloc on Windows:

.\BuildAll.ps1 -Platform x64 -Toolset v141 -UseMimalloc

Currently the usage of mimalloc is off by default, but I'd like to get people's thoughts on whether it should be enabled by default.

davecramer commented 6 months ago

I'm looking at enabling github actions on this repo. Once I get that working I can look at this PR

davecramer commented 6 months ago

any chance you can test the installers from https://github.com/postgresql-interfaces/psqlodbc/actions/runs/8469691901

apgrucza commented 6 months ago

Sorry I was off for a few weeks. Do you still need these installers tested?

davecramer commented 6 months ago

@apgrucza yes please. I have been working on automating the release process. Latest is https://github.com/postgresql-interfaces/psqlodbc/actions/runs/8723906820

davecramer commented 6 months ago

Can you rebase and add your tests to the github actions ?

apgrucza commented 6 months ago

I ran the psqlODBC x64 Installer and successfully used pyodbc to connect to and query a PostgreSQL database using the PostgreSQL Unicode(x64) driver.

Can you rebase and add your tests to the github actions ?

Not sure which tests you are referring to. This pull request does not add any tests.

davecramer commented 6 months ago

@apgrucza right, but wouldn't we have to build with your option turned on, and then run all of the tests ?

davecramer commented 5 months ago

@apgrucza looks like the test is failing ?

apgrucza commented 5 months ago

@apgrucza looks like the test is failing ?

@davecramer my changes are now complete. Please try again.

davecramer commented 5 months ago

Thanks!

apgrucza commented 5 months ago

any chance you can test the installers from https://github.com/postgresql-interfaces/psqlodbc/actions/runs/8469691901

@davecramer I tried running the 64-bit installer again (the linked artifact has expired but I still have the file), and I now get the following error:

Error installing ODBC driver: PostgreSQL ANSI(x64), ODBC error 13: The setup routines for the PostgreSQL ANSI(x64) ODBC driver could not be loaded due to system error code 126: The specified module could not be found. (C:\Program Files\psqlODBC\1600\bin\psqlodbc30a.dll).. Verify that the file PostgreSQL ANSI(x64) exists and that you can access it.

Not sure why I didn't get this the first time around. The same error occurs whether I install fresh or upgrade from an old version. I've tested on two different machines. The psqlodbc30a.dll file does exist, so I wonder whether it's failing due to a missing dependency?

apgrucza commented 5 months ago

The x86 installer works fine though.

davecramer commented 5 months ago

@apgrucza can you try https://github.com/postgresql-interfaces/psqlodbc/releases/tag/REL-16_00_0003 ? Thanks for testing this!

apgrucza commented 5 months ago

@davecramer that link only contains the x86 installer. I did test that one (in addition to the build artifacts) and it installed without error. The problem seems to be just with the x64 installer.

I don't think it's related, but I'm also curious to know why the version of libpq.dll in these packages (both x86 and x64) is 17.0.0.0, but the latest version of PostgreSQL is 16.2.

davecramer commented 5 months ago

Interesting. As for 17. it's because I build against head. Presumably the next release will be against 17 The fact that all the artifacts are not there is a problem though. Thanks for testing!

apgrucza commented 5 months ago

Another question. Was it a deliberate decision to use the latest version of Visual C++ (VC17) in the GitHub Actions? Looking at the build script, the "official" version to build against is VC15 (it will use that version even if newer versions are installed). When VC16 was added to the build script, I think it was for development purposes only (rather than for building releases).

I'm not opposed to building against VC17, but the change should be documented as I believe it may require users to install a newer version of the Microsoft Visual C++ Redistributable. And this page suggests if psqlODBC required the VC17 runtime, it would no longer run on Windows XP:

The last version of the Visual C++ Redistributable that works on Windows XP shipped in Visual Studio 2019 version 16.7 (file versions starting with 14.27).

davecramer commented 5 months ago

Well "deliberate" is an interesting word. The better word would be "ignorant" I'll switch it to 15 if that is going to stop it from working on XP.

davecramer commented 5 months ago
Windows XP Support: Microsoft ended support for Windows XP on April 8, 2014. Current versions of the Visual C++ Redistributable for Visual Studio 2015-2022 only support Windows 7, 8.1, 10, and 11. The last version of the Visual C++ Redistributable that works on Windows XP shipped in Visual Studio 2019 version 16.7 (file versions starting with 14.27). The Redistributable is available in the [my.visualstudio.com Downloads](https://my.visualstudio.com/Downloads/) section as Visual C++ Redistributable for Visual Studio 2019 (version 16.7). Use the Search box to find this version. To download the files, select the platform and language you need, and then choose the Download button.

So the question is do we need/want to support XP ?

davecramer commented 5 months ago

Looks like we also need to add all of the dll's used

davecramer commented 5 months ago

Can you try these https://github.com/davecramer/psqlodbc/releases/tag/REL-16_00_0005

apgrucza commented 5 months ago

So the question is do we need/want to support XP ?

On the mailing list I have noticed a few queries from people running legacy systems. But to be honest I don't really know if it currently even works on Windows XP. This post suggests it may have stopped working on Windows XP since version 11. Nobody should really be using an operating system that was retired 10 years ago and expect new software to run on it.

Can you try these https://github.com/davecramer/psqlodbc/releases/tag/REL-16_00_0005

Still getting the same error:

image

The x86 installer works fine. It must be something to do with the build environment, because it works if I build the x64 installer myself locally.

davecramer commented 5 months ago

Interesting, If I install it on a windows system it works. Thanks for checking. I will keep digging.

I uninstall the previous one first. Are you doing yours on a clean system ?

apgrucza commented 5 months ago

The error occurs regardless of whether I uninstall first. I’ve tested on two different systems. They are not new machines, but both have no problems when installing previous versions of the x64 driver.

davecramer commented 5 months ago

Hmmm OK, so to be clear I am using the installer. The MSI file. I think you may be using the setup file?

davecramer commented 5 months ago

I just tried the setup file and it worked fine. Can you send me the file that works on your end? Perhaps we can figure out what is different about the files ? My email is davecramer@gmail.com

apgrucza commented 5 months ago

I get the same error regardless of whether I use the MSI installer or the setup EXE. I was testing the files in the release you just made. So:

If both are working for you then something on my systems must be causing it to fail. Perhaps the presence (or lack of) a particular DLL.

apgrucza commented 5 months ago

I loaded psqlodbc30a.dll from both REL-16_00_0005 installers into Dependencies to compare. For some reason the x64 one tries to load extra DLLs (libcrypto and libssl) via libq.dll but the x86 one doesn't. These DLLs fail to load (module could not be found on disk), which explains why only the x64 installer fails.

x86 image

x64 image

Neither MSI files include these DLLs:

x86 image

x64 image

If I compare with the MSIs from the last official release at https://www.postgresql.org/ftp/odbc/versions/msi/, libcrypto and libssl DLLs are present (and the x64 one has a few more: libiconv, libintl, libwinpthread):

x86 image

x64 image

So, I think the problem lies with these DLLs being absent from the MSI files.

davecramer commented 5 months ago

AHA, good detective work. Which explains why it works on my machine and not yours.

Thanks!

apgrucza commented 5 months ago

And when I build the installer locally, all those extra libs are included in the MSI file.

I noticed configuration.xml in the GitHub workflow references the directories from your PostgreSQL build (d:\postgresql86 and d:\postgresql) rather than the directory that the EDB installer creates. On my system I didn't have to build PostgreSQL. I just ran the EDB installer, which creates C:\Program Files\PostgreSQL\16, and then my configuration.xml simply references this directory:

    <libpq version="y">
      <include>C:\Program Files\PostgreSQL\16\include</include>
      <lib>C:\Program Files\PostgreSQL\16\lib</lib>
      <bin>C:\Program Files\PostgreSQL\16\bin</bin>
    </libpq>

Perhaps you should do the same in the GitHub workflow? You may need change the --enable-components option for the include directory to be created. Once you do this, I don't think you need the GitHub workflow to build PostgreSQL at all.

Not sure if this will fix the problem, but it's worth a try and should simplify the workflow too.

davecramer commented 5 months ago

Well I added the EDB one after, since the XML tests failed and I was unable to build PostgreSQL with XML on windows. We still need the x86 one as the EDB build is 64 bit

apgrucza commented 5 months ago

As a test, I updated configuration.xml in my fork to point to the EDB one (for x64):

  <x64>
    <libpq version="y">
      <include>C:\Program Files\PostgreSQL\16\include</include>
      <lib>C:\Program Files\PostgreSQL\16\lib</lib>
      <bin>C:\Program Files\PostgreSQL\16\bin</bin>
    </libpq>
    ...

This time the x64 MSI produced by the GitHub workflow included the extra DLLs (libcrypto, libssl, libiconv, libintl, libwinpthread) and installed successfully. But as you point out, EDB does not provide an x86 build for PostgreSQL 16.

I compared d:\postgresql\lib with C:\Program Files\PostgreSQL\16\lib and there were a lot of differences. The former was missing files such as libcrypto.lib and libssl.lib. Perhaps EDB have a different build process. Regardless, it's probably better to build a stable release from https://www.postgresql.org/ftp/source/ rather than from HEAD.

davecramer commented 5 months ago

As a test, I updated configuration.xml in my fork to point to the EDB one (for x64):

  <x64>
    <libpq version="y">
      <include>C:\Program Files\PostgreSQL\16\include</include>
      <lib>C:\Program Files\PostgreSQL\16\lib</lib>
      <bin>C:\Program Files\PostgreSQL\16\bin</bin>
    </libpq>
    ...

This time the x64 MSI produced by the GitHub workflow included the extra DLLs (libcrypto, libssl, libiconv, libintl, libwinpthread) and installed successfully. But as you point out, EDB does not provide an x86 build for PostgreSQL 16.

I compared d:\postgresql\lib with C:\Program Files\PostgreSQL\16\lib and there were a lot of differences. The former was missing files such as libcrypto.lib and libssl.lib. Perhaps EDB have a different build process. Regardless, it's probably better to build a stable release from https://www.postgresql.org/ftp/source/ rather than from HEAD.

Building from source is the answer. I guess I'll have to figure that out.

Dave

davecramer commented 5 months ago

The instructions on how to create the installers are not helpful here.

davecramer commented 5 months ago

I found the problem. I deleted

    echo Building Installer Merge Module
        $(CANDLE) -nologo -dLIBPQMEM0="libssl-3-x64.dll" -dLIBPQMEM1="libcrypto-3-x64.dll" -dLIBPQMEM2="libintl-9.dll" -dLIBPQMEM3="libwinpthread-1.dll" -dLIBPQMEM4="libiconv-2.dll" -dLIBPQMEM5="" -dLIBPQMEM6="" -dLIBPQMEM7="" -dLIBPQMEM8="" -dLIBPQMEM9="" -dPlatform="$(TARGET_CPU)" -dVERSION=$(PG_VER) -dLIBPQBINDIR=$(PG_BIN) -dLIBPQMSVCDLL="" -dLIBPQMSVCSYS="" -dPODBCMSVCDLL=$(VCRT_DLL) -dPODBCMSVPDLL=$(MSVCP_DLL) -dPODBCMSVCSYS="" -dPODBCMSVPSYS="" -dNoPDB="False" -dBINBASE=".." -o .\x64\psqlodbcm.wixobj psqlodbcm_cpu.wxs
    $(LIGHT) -nologo -o $(TARGET_CPU)\psqlodbc_$(TARGET_CPU).msm $(TARGET_CPU)\psqlodbcm.wixobj

$(TARGET_CPU)\psqlodbc_$(TARGET_CPU).msi: psqlodbc_cpu.wxs $(DRIVER_FILES)
    echo Building Installer
    $(CANDLE) -nologo -dPlatform="$(TARGET_CPU)" -dVERSION=$(POSTGRESDRIVERVERSION) -dSUBLOC=$(SUBLOC) -dPRODUCTCODE=$(PRODUCTCODE) -o $(TARGET_CPU)\psqlodbc.wixobj psqlodbc_cpu.wxs
    $(LIGHT) -nologo -ext WixUIExtension -cultures:en-us -o $(TARGET_CPU)\psqlodbc_$(TARGET_CPU).msi $(TARGET_CPU)\psqlodbc.wixobj
    cscript modify_msi.vbs $(TARGET_CPU)\psqlodbc_$(TARGET_CPU).msi

out of installer.mak. This is where the extra files are added

apgrucza commented 5 months ago

When was that? installer.mak hasn't been changed for 10 years.

davecramer commented 5 months ago

Not sure, all I know is that it is in the original code, and that is where the additional dll's are added

davecramer commented 5 months ago

So looking at this some more, https://github.com/postgresql-interfaces/psqlodbc/blob/3869efe76653ce9e56edf10478e30dcf1072123c/installer/psqlodbcm_cpu.wxs#L106-L135 is where those variables are used. It's becoming clear to me that the actual release steps are not documented anywhere.

You are correct installer.mak hasn't changed and I found the code in a private repo that I have access to.

Either way we need to fix the release code to include those libraries.

Also need to figure out how to get the 32bit versions of those libraries.

apgrucza commented 5 months ago

I just took a deeper look too.

installer.mak is not even used by buildInstallers.ps1, so changing that file won't help. In buildInstallers.ps1 there is a call to Get-RelatedDlls which uses dumpbin.exe to inspect libpq.dll and figure out its dependencies. As long as the bin directory referenced in configuration.xml contains these libraries, dumpbin.exe will find them and they will be included in the installer. That's why it worked when I changed configuration.xml to reference the EDB installation (Get-RelatedDlls returned libssl-3-x64.dll libcrypto-3-x64.dll libintl-9.dll libwinpthread-1.dll libiconv-2.dll). So, there should be no need to change the release code.

As you say, we still need to figure out how to get the 32-bit versions of those libraries. @winpg was the one who always prepared and announced psqlODBC releases. Perhaps he could provide some insight here, or is he also unavailable?

apgrucza commented 5 months ago

The EDB installer must include those extra DLLs because they are required by PostgreSQL. So I checked the Windows build instructions for PostgreSQL and it lists the requirements. One of them is OpenSSL, and it provides a link to download binaries. I downloaded OpenSSL v3.3.0 Light and found that it includes both the libssl-3 and libcrypto-3 DLLs. So perhaps all we need to do is get those into d:\postgresql86\bin.

davecramer commented 5 months ago

Yes, I am in the process of doing that now!. Thanks

davecramer commented 5 months ago

can you check the installers from https://github.com/postgresql-interfaces/psqlodbc/actions/runs/8992393964

apgrucza commented 5 months ago

Both installers now work without errors. However, the x86 installer still does not contain the libssl and libcrypto DLLs.

apgrucza commented 5 months ago

One problem I can see is that this:

cp c:\openssl32\*.dll d:\postgresql86\bin

needs to be changed to this:

cp c:\openssl32\bin\*.dll d:\postgresql86\bin
apgrucza commented 5 months ago

BTW the usage of actions/cache is causing certain steps to be skipped when they shouldn't be. For example, the "build postgresx86" step in this run was skipped even though the commit that triggered it changed that step. Not sure what the best solution to this is, but one option is to include in the cache key a hash of all files that affect the contents of d:\postgresql86, including .github/workflows/main.yml itself.

davecramer commented 5 months ago

One problem I can see is that this:

cp c:\openssl32\*.dll d:\postgresql86\bin

needs to be changed to this:

cp c:\openssl32\bin\*.dll d:\postgresql86\bin

The dll's are installed in both places so it is currently correct.

davecramer commented 5 months ago

BTW the usage of actions/cache is causing certain steps to be skipped when they shouldn't be. For example, the "build postgresx86" step in this run was skipped even though the commit that triggered it changed that step. Not sure what the best solution to this is, but one option is to include in the cache key a hash of all files that affect the contents of d:\postgresql86, including .github/workflows/main.yml itself.

I'm not sure how to handle that either. I guess we need to figure out how to invalidate the cache when the version of postgres changes as well.

Thanks for testing this

apgrucza commented 5 months ago

One problem I can see is that this:

cp c:\openssl32\*.dll d:\postgresql86\bin

needs to be changed to this:

cp c:\openssl32\bin\*.dll d:\postgresql86\bin

The dll's are installed in both places so it is currently correct.

Yes you are correct. So I guess the installers you built weren't working due to the caching issue.

apgrucza commented 5 months ago

I disabled the caching in my fork so that I could test your changes. The x86 installer does now install the libssl and libcrypto DLLs. However, the DLL versions are not consistent between x86 and x64.

Is it possible to build the x64 driver the same way as the x86 driver so that we can have some consistency? And to get the PostgreSQL code from a stable release, from https://www.postgresql.org/ftp/source/?

davecramer commented 5 months ago

Ah, good point. Yes, it is possible to build the driver the same way

davecramer commented 5 months ago

can you check these https://github.com/davecramer/psqlodbc/actions/runs/9036264469 ?