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

apgrucza · 2024-03-06T00:37:41Z

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 · 2024-03-28T11:28:53Z

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

davecramer · 2024-03-28T16:13:49Z

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

apgrucza · 2024-04-16T07:13:46Z

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

davecramer · 2024-04-17T17:44:14Z

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

davecramer · 2024-04-18T12:23:15Z

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

apgrucza · 2024-04-22T07:20:20Z

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 · 2024-04-22T12:36:41Z

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

davecramer · 2024-04-25T09:53:02Z

@apgrucza looks like the test is failing ?

apgrucza · 2024-04-25T12:30:18Z

@apgrucza looks like the test is failing ?

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

davecramer · 2024-04-26T13:00:35Z

Thanks!

apgrucza · 2024-05-01T04:31:46Z

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 · 2024-05-01T06:23:49Z

The x86 installer works fine though.

davecramer · 2024-05-01T10:53:37Z

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

apgrucza · 2024-05-01T11:14:57Z

@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 · 2024-05-01T12:45:34Z

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!

davecramer · 2024-05-03T19:49:44Z

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

davecramer · 2024-05-04T00:01:26Z

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 · 2024-05-04T14:21:39Z

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

davecramer · 2024-05-04T18:48:03Z

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

davecramer · 2024-05-06T10:51:46Z

So looking at this some more,

psqlodbc/installer/psqlodbcm_cpu.wxs

Lines 106 to 135 in 3869efe

    
           <?if "$(var.LIBPQMEM0)" != "" ?> 
        
                     <File Source="$(var.LIBPQBINDIR)\$(var.LIBPQMEM0)" /> 
        
           <?endif ?> 
        
           <?if "$(var.LIBPQMEM1)" != "" ?> 
        
                     <File Source="$(var.LIBPQBINDIR)\$(var.LIBPQMEM1)" /> 
        
           <?endif ?> 
        
           <?if "$(var.LIBPQMEM2)" != "" ?> 
        
                     <File Source="$(var.LIBPQBINDIR)\$(var.LIBPQMEM2)" /> 
        
           <?endif ?> 
        
           <?if "$(var.LIBPQMEM3)" != "" ?> 
        
                     <File Source="$(var.LIBPQBINDIR)\$(var.LIBPQMEM3)" /> 
        
           <?endif ?> 
        
           <?if "$(var.LIBPQMEM4)" != "" ?> 
        
                     <File Source="$(var.LIBPQBINDIR)\$(var.LIBPQMEM4)" /> 
        
           <?endif ?> 
        
           <?if "$(var.LIBPQMEM5)" != "" ?> 
        
                     <File Source="$(var.LIBPQBINDIR)\$(var.LIBPQMEM5)" /> 
        
           <?endif ?> 
        
           <?if "$(var.LIBPQMEM6)" != "" ?> 
        
                     <File Source="$(var.LIBPQBINDIR)\$(var.LIBPQMEM6)" /> 
        
           <?endif ?> 
        
           <?if "$(var.LIBPQMEM7)" != "" ?> 
        
                     <File Source="$(var.LIBPQBINDIR)\$(var.LIBPQMEM7)" /> 
        
           <?endif ?> 
        
           <?if "$(var.LIBPQMEM8)" != "" ?> 
        
                     <File Source="$(var.LIBPQBINDIR)\$(var.LIBPQMEM8)" /> 
        
           <?endif ?> 
        
           <?if "$(var.LIBPQMEM9)" != "" ?> 
        
                     <File Source="$(var.LIBPQBINDIR)\$(var.LIBPQMEM9)" /> 
        
           <?endif ?>

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 · 2024-05-06T11:55:26Z

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 · 2024-05-06T12:13:47Z

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 · 2024-05-06T12:48:21Z

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

davecramer · 2024-05-08T09:23:53Z

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

apgrucza · 2024-05-08T12:35:36Z

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

apgrucza · 2024-05-08T12:51:18Z

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 · 2024-05-08T13:49:54Z

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 · 2024-05-08T13:52:29Z

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 · 2024-05-08T13:53:41Z

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

… contention (#6) * Add mimalloc submodule and update build script * Add steps to build and test with UseMimalloc * Update mimalloc submodule * Change UseMimalloc parameter type to switch * Add ExpectMimalloc parameter * Fetch mimalloc submodule and use mimalloc parameters * Prevent MIMALLOC_VERBOSE aborting tests * Uninstall driver after tests; upload mimalloc artifacts

apgrucza · 2024-05-09T01:21:13Z

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 · 2024-05-09T11:20:22Z

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.

x86: libpq.dll (17.0.0.0), libssl-3.dll (3.3.0.0)
x64: libpq.dll (16.0.2.0), libssl-3-x64.dll (3.0.13.0)

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 · 2024-05-10T15:56:25Z

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

davecramer · 2024-05-10T20:43:22Z

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

apgrucza · 2024-05-11T03:06:46Z

I have a fix for the caching issue here: #13

apgrucza · 2024-05-11T03:22:06Z

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

The x86 artifact still contains libpq 17, and looking at the GitHub Actions log reveals that it's because the "build postgresx86" step was again skipped (due to the caching problem). If you want to merge your changes then I can rebase #13 and the problem should go away.

apgrucza · 2024-05-11T03:32:21Z

The EDB installer takes 2m 30s to install. With your recent change, is the EDB installer now only used to run PostgreSQL (so that the psqlodbc tests can run)? If so, we could reduce the workflow duration by running the PostgreSQL Docker image instead.

davecramer · 2024-05-11T11:25:40Z

The EDB installer takes 2m 30s to install. With your recent change, is the EDB installer now only used to run PostgreSQL (so that the psqlodbc tests can run)? If so, we could reduce the workflow duration by running the PostgreSQL Docker image instead.

I didn't think docker ran on github windows images ?

apgrucza · 2024-05-11T11:32:44Z

I didn't think docker ran on github windows images ?

Oh, that surprises me. I haven't used GitHub Actions much before.

davecramer · 2024-05-11T20:31:37Z

I think you can run testcontainers which does run docker but I don't think you can specify the version of postgres. It seemed simpler to use EDB

davecramer force-pushed the main branch from 0f1e52f to 4e1c125 Compare April 19, 2024 19:35

apgrucza force-pushed the feature/mimalloc branch 3 times, most recently from 0df3899 to c602f0b Compare April 23, 2024 12:14

Add mimalloc submodule and update build script

d886ab4

apgrucza force-pushed the feature/mimalloc branch from c602f0b to d886ab4 Compare April 23, 2024 13:27

apgrucza added 5 commits April 24, 2024 00:06

Add steps to build and test with UseMimalloc

92ee217

Update mimalloc submodule

33a7804

Change UseMimalloc parameter type to switch

1bc7ea8

Add ExpectMimalloc parameter

8cd7f28

Fetch mimalloc submodule and use mimalloc parameters

8b4bd08

apgrucza added 2 commits April 25, 2024 22:25

Prevent MIMALLOC_VERBOSE aborting tests

f119320

Uninstall driver after tests; upload mimalloc artifacts

dcae06a

davecramer merged commit 3869efe into postgresql-interfaces:main Apr 26, 2024
1 check passed

apgrucza deleted the feature/mimalloc branch April 29, 2024 06:06

This was referenced May 13, 2024

Use strdup on non-Windows systems #17

Merged

[psqlodbc-REL-16_00_0004][Postgres 15 5][Debian][unixODBC] Need to add CPPFLAGS="-DSQLCOLATTRIBUTE_SQLLEN" to configure to get it to compile #16

Open

#define pg_strdup _strdup #15

Closed

Maniyur1Git mentioned this pull request May 16, 2024

ODBC driver issue while working with Informatica Cloud #22

Open

cubt85iz mentioned this pull request Sep 26, 2024

X64 merge module installer fails #49

Open

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

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

Conversation

apgrucza commented Mar 6, 2024 • edited Loading

davecramer commented Mar 28, 2024

davecramer commented Mar 28, 2024

apgrucza commented Apr 16, 2024

davecramer commented Apr 17, 2024

davecramer commented Apr 18, 2024

apgrucza commented Apr 22, 2024

davecramer commented Apr 22, 2024

davecramer commented Apr 25, 2024

apgrucza commented Apr 25, 2024

davecramer commented Apr 26, 2024

apgrucza commented May 1, 2024

apgrucza commented May 1, 2024

davecramer commented May 1, 2024

apgrucza commented May 1, 2024

davecramer commented May 1, 2024

davecramer commented May 3, 2024

davecramer commented May 4, 2024

apgrucza commented May 4, 2024

davecramer commented May 4, 2024

davecramer commented May 6, 2024

apgrucza commented May 6, 2024 • edited Loading

apgrucza commented May 6, 2024 • edited Loading

davecramer commented May 6, 2024

davecramer commented May 8, 2024

apgrucza commented May 8, 2024

apgrucza commented May 8, 2024

apgrucza commented May 8, 2024

davecramer commented May 8, 2024

davecramer commented May 8, 2024

apgrucza commented May 9, 2024

apgrucza commented May 9, 2024

davecramer commented May 10, 2024

davecramer commented May 10, 2024

apgrucza commented May 11, 2024 • edited Loading

apgrucza commented May 11, 2024

apgrucza commented May 11, 2024

davecramer commented May 11, 2024

apgrucza commented May 11, 2024

davecramer commented May 11, 2024

apgrucza commented Mar 6, 2024 •

edited

Loading

apgrucza commented May 6, 2024 •

edited

Loading

apgrucza commented May 6, 2024 •

edited

Loading

apgrucza commented May 11, 2024 •

edited

Loading