Skip to content

Account memory for OrcDeletedRows - fix memory accounting for ORC ACID delete delta#9914

Merged
losipiuk merged 1 commit intotrinodb:masterfrom
losipiuk:lo/account-orc-deleted
Nov 19, 2021
Merged

Account memory for OrcDeletedRows - fix memory accounting for ORC ACID delete delta#9914
losipiuk merged 1 commit intotrinodb:masterfrom
losipiuk:lo/account-orc-deleted

Conversation

@losipiuk
Copy link
Copy Markdown
Member

@losipiuk losipiuk commented Nov 9, 2021

No description provided.

@cla-bot cla-bot bot added the cla-signed label Nov 9, 2021
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sizeOfObjectArray(rowCount) + rowCount * RowId.INSTANCE_SIZE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth doing setting this as we loop vs just doing it once at the end?
Also, can we safely ignore the return value of setBytes?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benefit of doing that in the loop (I think) is that we can pause some other query for a while, allowing us to get to the end of loop without OOM. Other than that it does not change much I think.
@findepi am I missing something?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should assume that building deletedRows list can exhaust node memory.
if we set memory usage only at the end, we still do not protect ourselves against node OOM.

Also, can we safely ignore the return value of setBytes?

we cannot block here (can we?).
we could use trySetBytes here, perhaps.

@losipiuk losipiuk force-pushed the lo/account-orc-deleted branch from 8a18ee9 to 40446fb Compare November 10, 2021 09:53
@losipiuk
Copy link
Copy Markdown
Member Author

AC

@losipiuk
Copy link
Copy Markdown
Member Author

losipiuk commented Nov 10, 2021

we cannot block here (can we?).

It does not look like. Maybe with a significant refactoring.

we could use trySetBytes here, perhaps.

I was thinking about that. But then what if it returns that we cannot allocate. Throw exception ourselves?

@findepi
Copy link
Copy Markdown
Member

findepi commented Nov 10, 2021

we could use trySetBytes here, perhaps.

I was thinking about that. But then what if it returns that we cannot allocate. Throw exception ourselves?

yes, we would throw then.

cc @sopel39 for LocalMemoryContext.setBytes vs trySetBytes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment why

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropped as data is not passed to operator context immediately.

@losipiuk losipiuk force-pushed the lo/account-orc-deleted branch from 40446fb to 2c5a097 Compare November 18, 2021 20:56
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra space before as

@losipiuk losipiuk force-pushed the lo/account-orc-deleted branch from 2c5a097 to e3560ed Compare November 18, 2021 22:47
@losipiuk losipiuk merged commit 92d7eea into trinodb:master Nov 19, 2021
@github-actions github-actions bot added this to the 365 milestone Nov 19, 2021
@losipiuk losipiuk mentioned this pull request Nov 30, 2021
12 tasks
@findepi findepi changed the title Account memory for OrcDeletedRows Account memory for OrcDeletedRows - fix memory accounting for ORC ACID delete delta Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants