Free heap allocated Datums after the flush #2469
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Alex Kasko <[email protected]>
This was referenced Apr 10, 2024
staticlibs
added a commit
to wiltondb/babelfish_extensions
that referenced
this pull request
Apr 14, 2024
…2469) Signed-off-by: Alex Kasko <[email protected]>
KushaalShroff
requested changes
Apr 18, 2024
| for (int i = 0; i < rowCount * colCount; i++) | ||
| { | ||
| cstate->bufferedValues = lappend(cstate->bufferedValues, (void *) Values[i]); | ||
| cstate->bufferedValueAllocFlags = lappend_int(cstate->bufferedValueAllocFlags, ValueAllocFlags[i] ? 1 : 0); |
There was a problem hiding this comment.
Why cant we directly store booleans in cstate->bufferedValueAllocFlags instead of integers?
There was a problem hiding this comment.
Just wanted to be explicit with T_IntList usage. Updated this line to remove int conversion.
Comment on lines
724
to
734
| rowData->isAllocated[i] = true; | ||
| break; | ||
| case TDS_TYPE_NCHAR: | ||
| case TDS_TYPE_NVARCHAR: | ||
| rowData->columnValues[i] = TdsTypeNCharToDatum(temp); | ||
| rowData->isAllocated[i] = true; | ||
| break; | ||
| case TDS_TYPE_BINARY: | ||
| case TDS_TYPE_VARBINARY: | ||
| rowData->columnValues[i] = TdsTypeVarbinaryToDatum(temp); | ||
| rowData->isAllocated[i] = true; |
There was a problem hiding this comment.
common condition in all of the cases, can we move it out?
There was a problem hiding this comment.
Moved it out of the condition and added a comment.
Comment on lines
804
to
812
| rowData->isAllocated[i] = true; | ||
| break; | ||
| case TDS_TYPE_NTEXT: | ||
| rowData->columnValues[i] = TdsTypeNCharToDatum(temp); | ||
| rowData->isAllocated[i] = true; | ||
| break; | ||
| case TDS_TYPE_IMAGE: | ||
| rowData->columnValues[i] = TdsTypeVarbinaryToDatum(temp); | ||
| rowData->isAllocated[i] = true; |
There was a problem hiding this comment.
Common condition between all cases, can we move it out?
There was a problem hiding this comment.
Moved it out of the condition and added a comment.
Signed-off-by: Alex Kasko <[email protected]>
shardgupta
pushed a commit
that referenced
this pull request
Jul 19, 2024
In `ExecuteBulkCopy` indices on the target table are opened for every incoming batch, but never closed. It is proposed to open indices instead for every executor batch just before the call to `table_multi_insert` and close them after they are updated with tuples from this batch. Backend memory usage (with both this patch and #2469 patch included) while importing 16 million varchar records (4x the table from the linked issue):  Signed-off-by: Alex Kasko <[email protected]>
tanscorpio7
pushed a commit
to tanscorpio7/babelfish_extensions
that referenced
this pull request
Jul 23, 2024
…stgresql#2487) In `ExecuteBulkCopy` indices on the target table are opened for every incoming batch, but never closed. It is proposed to open indices instead for every executor batch just before the call to `table_multi_insert` and close them after they are updated with tuples from this batch. Backend memory usage (with both this patch and babelfish-for-postgresql#2469 patch included) while importing 16 million varchar records (4x the table from the linked issue):  Signed-off-by: Alex Kasko <[email protected]>
shardgupta
pushed a commit
that referenced
this pull request
Jul 23, 2024
In `ExecuteBulkCopy` indices on the target table are opened for every incoming batch, but never closed. It is proposed to open indices instead for every executor batch just before the call to `table_multi_insert` and close them after they are updated with tuples from this batch. Backend memory usage (with both this patch and #2469 patch included) while importing 16 million varchar records (4x the table from the linked issue):  Signed-off-by: Alex Kasko <[email protected]>
sharathbp
pushed a commit
to amazon-aurora/babelfish_extensions
that referenced
this pull request
Aug 20, 2024
…stgresql#2487) In `ExecuteBulkCopy` indices on the target table are opened for every incoming batch, but never closed. It is proposed to open indices instead for every executor batch just before the call to `table_multi_insert` and close them after they are updated with tuples from this batch. Backend memory usage (with both this patch and babelfish-for-postgresql#2469 patch included) while importing 16 million varchar records (4x the table from the linked issue):  Signed-off-by: Alex Kasko <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When BCP import is performed,
Datumsare created for all non-NULL incoming fields. Some of theseDatumsare allocated on heap. TheseDatumsneed to be kept in memory until the call totable_multi_insert.It seems to be not possible to distinguish between heap-allocated
Datumsand passed-by-valueDatumsusing only the pointer to suchDatum. The proposed patch attempts to keep track of all heap-allocatedDatums(in a similar manner, likeisNullis tracked) so they can be freed immediately after the flush.Pointers to
DatumsandisAllocatedflags are kept inBulkCopyStateDatain growing lists. Pointers from incoming batch are appended there before processing. And theseDatumsare freed and lists are trimmed after each flush.There are also 2 unrelated memory cleanup changes included: freeing of
TDSRequestBulkLoadData->rowDatacontents on TDS side and freeing ofattnumslist on TSQL side.attnumsis created for every batch (I assume this is needed for error-checking), but only used for the first incoming batch.Memory usage when importing 1 million of
varchars(see details in linked issue), without the patch:With patch applied:
Issues Resolved
#2468
Test Scenarios Covered
There are no functional changes, so no new tests.
Check List
By submitting this pull request, I confirm that my contribution is under the terms of the Apache 2.0 and PostgreSQL licenses, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Signed-off-by: Alex Kasko [email protected]