-
Notifications
You must be signed in to change notification settings - Fork 7
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
Choose optimal IOT compression based on table params #6498
Conversation
Generate changelog in
|
atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleDdlTable.java
Outdated
Show resolved
Hide resolved
atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleDdlTable.java
Outdated
Show resolved
Hide resolved
final ImmutableSet<NamedColumnDescription> namedColumns; | ||
|
||
@Nullable | ||
final DynamicColumnDescription dynamicColumn; | ||
|
||
public ColumnMetadataDescription() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate from this PR this class has odd constructors that don't seem to hold the invariant that one field is null and one non-null indicating either dynamic or static columns. There should probably just be an interface or abstract base class and separate implementations for dynamic & static columns, but that might be more painful to graft onto the serDe at this point, but we should tighten up the preconditions for constructors.
Hmm gradle seems to have died, I don't have perms to restart |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really interesting - thanks for the PR! There are a couple of bits in the tests I'm confused by, but generally looks good
...-dbkvs/src/test/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleDdlTableTest.java
Outdated
Show resolved
Hide resolved
...-dbkvs/src/test/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleDdlTableTest.java
Outdated
Show resolved
Hide resolved
...-dbkvs/src/test/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleDdlTableTest.java
Outdated
Show resolved
Hide resolved
...-dbkvs/src/test/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleDdlTableTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thank you!
Oracle has just now introduced a feature in Oracle 23c to set the IOT compression based on the data in the block... https://docs.oracle.com/en/database/oracle/oracle-database/23/nfcoa/oltp-and-core-database.html#GUID-58C3D923-CA1F-4D59-A919-A3A33CF2A491 |
General
Before this PR:
Oracle tables used standard compression
After this PR:
==COMMIT_MSG==
Determine the optimal Index Organized Table compression setting.
When enabling compression on an IOT, we can instruct Oracle to compress at most one fewer columns than the number of columns that make up the PK for the IOT (i.e. the IOT we create have a PK of {row, col, ts} so we can compress 0, 1 or 2 columns - nothing, row, or row & col. It wouldn't make sense to compress all three columns, because necessarily there can be multiple rows that share all three values.
When compression is enabled in an IOT, the block header adds a table the captures the values of the compressed columns. The actual rows in the blocks then refer back to entries in the header table to capture their values for the compressed columns. This extra header table and the slot references for data rows consume space in the block, so misuse of compression results in extra storage being consumed.
Compression only makes sense when we expect that the { row } or { row, col } values will be reused:
If the table's sweep strategy is CONSERVATIVE, then every { row, col } will have at least two rows, the current value (which may have null val if the value is deleted) and a sentinel row with ts = -1. Clearly compression of both { row, col } is valuable.
If the sweep strategy is not CONSERVATIVE, there will be reuse of the { row } if there is more than one named column, or the table uses dynamic columns, which would be generally be expected to have multiple row values. In this case, compression 1 is appropriate so { row } only needs to be store once.
Finally, if the sweep strategy is not CONSERVATIVE, there are no sentinels, and if there is only one named column then there will be no rows that share { row } so compression should be disabled.
==COMMIT_MSG==
Priority:
Low
Concerns / possible downsides (what feedback would you like?):
None: this change won't have any impact on Oracle's query planning, because we only changing how the data will be written at the block level.
Is documentation needed?:
No
Compatibility
Does this PR create any API breaks (e.g. at the Java or HTTP layers) - if so, do we have compatibility?:
No API breaks.
Unfortunately, there isn't a way to alter the compression setting once an IOT is created. It would be possible to migrate existing tables online using DBMS_REDEFINITION (only available on Oracle EE) or offline using CREATE TABLE AS ... or with an explicit migration task.
Since tables using CONSERVATIVE are generally "old", we can assume through time they will be gradually replaced.
The AtlasDB migration support from CONSERVATIVE->THOROUGH will not attempt to set the new, optimal compression setting either.
Does this PR change the persisted format of any data - if so, do we have forward and backward compatibility?:
Only at the very lowest level which is not visible to DB clients.
The code in this PR may be part of a blue-green deploy. Can upgrades from previous versions safely coexist? (Consider restarts of blue or green nodes.):
Yes
Does this PR rely on statements being true about other products at a deployment - if so, do we have correct product dependencies on these products (or other ways of verifying that these statements are true)?:
Does this PR need a schema migration?
Testing and Correctness
What, if any, assumptions are made about the current state of the world? If they change over time, how will we find out?:
What was existing testing like? What have you done to improve it?:
Specific tests (thanks @schlosna) verify the function selecting the compression level.
If this PR contains complex concurrent or asynchronous code, is it correct? The onus is on the PR writer to demonstrate this.:
N/A
If this PR involves acquiring locks or other shared resources, how do we ensure that these are always released?:
N/A
Execution
How would I tell this PR works in production? (Metrics, logs, etc.):
DBA_INDEXES.PREFIX_LENGTH capture the compression setting for IOT.
Has the safety of all log arguments been decided correctly?:
N/A
Will this change significantly affect our spending on metrics or logs?:
No
How would I tell that this PR does not work in production? (monitors, etc.):
N/A
If this PR does not work as expected, how do I fix that state? Would rollback be straightforward?:
N/A
If the above plan is more complex than “recall and rollback”, please tag the support PoC here (if it is the end of the week, tag both the current and next PoC):
N/A
Scale
Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.:
This change is most beneficial at scale, as it helps improve the information density of blocks, the improvement is greatest when the rows/column field are short, as relatively more space is wasted in the overhead of the block header table and references.
Some real world data illustrates the scope of potential savings (though in practice, these tables currently use CONSERVATIVE sweep so we won't achieve these savings without client application changes to migrate):
Would this PR be expected to perform a large number of database calls, and/or expensive database calls (e.g., row range scans, concurrent CAS)?:
No change.
Would this PR ever, with time and scale, become the wrong thing to do - and if so, how would we know that we need to do something differently?:
No
Please tag any other people who should be aware of this PR:
@jeremyk-91
@sverma30
@raiju