Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

Remove dead DBKVS code, fix Oracle hints #7338

Merged
merged 3 commits into from
Oct 28, 2024
Merged

Remove dead DBKVS code, fix Oracle hints #7338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7bf13a
Remove dead DBKVS code, fix Oracle hints
CVdV-au Oct 10, 2024
f79e812
Broken hint
CVdV-au Oct 10, 2024
eaf4cb4
Add generated changelog entries
svc-changelog Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbQueryFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,27 +28,18 @@ public interface DbQueryFactory {

FullQuery getLatestRowsQuery(Iterable<byte[]> rows, long ts, ColumnSelection columns, boolean includeValue);

FullQuery getLatestRowsQuery(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These methods are all unused, so deleted from this interface and the Postgres/Oracle implementation classes.

Collection<Map.Entry<byte[], Long>> rows, ColumnSelection columns, boolean includeValue);

FullQuery getAllRowQuery(byte[] row, long ts, ColumnSelection columns, boolean includeValue);

FullQuery getAllRowsQuery(Iterable<byte[]> rows, long ts, ColumnSelection columns, boolean includeValue);

FullQuery getAllRowsQuery(Collection<Map.Entry<byte[], Long>> rows, ColumnSelection columns, boolean includeValue);

FullQuery getLatestCellQuery(Cell cell, long ts, boolean includeValue);

FullQuery getLatestCellsQuery(Iterable<Cell> cells, long ts, boolean includeValue);

FullQuery getLatestCellsQuery(Collection<Map.Entry<Cell, Long>> cells, boolean includeValue);

FullQuery getAllCellQuery(Cell cell, long ts, boolean includeValue);

FullQuery getAllCellsQuery(Iterable<Cell> cells, long ts, boolean includeValue);

FullQuery getAllCellsQuery(Collection<Map.Entry<Cell, Long>> cells, boolean includeValue);

FullQuery getRangeQuery(RangeRequest range, long ts, int maxRows);

boolean hasOverflowValues();
Expand Down
142 changes: 18 additions & 124 deletions ...kvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleQueryFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,7 @@ public OracleQueryFactory(OracleDdlConfig config, String tableName, boolean hasO

@Override
public FullQuery getLatestRowQuery(byte[] row, long ts, ColumnSelection columns, boolean includeValue) {
String query = " /* GET_LATEST_ONE_ROW_INNER (" + tableName + ") */ "
+ " SELECT"
+ " /*+ USE_NL(t m) LEADING(t m) CARDINALITY(t 1) CARDINALITY(m 10) INDEX(m "
+ PrimaryKeyConstraintNames.get(tableName) + ") */ "
String query = "SELECT"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the hints here are simply wrong - there is no table alias t.

More generally:

  1. Hints of the form INDEX(alias PrimaryKeyConstraintNames.get(tableName)) are redundant because in the Oracle implementation of DBKVS, all these tables are Index Organised Tables (IOT) for which the only access is via the index (caveat being if there were secondary indexes on the non-key columns of the IOT we wanted to use such a hint could make sense - but there are not). Specifying an INDEX hint doesn't preclude Oracle from using a non-ideal scan method eg fast full scan, range scan etc) - it just says use this particular index. So these hints provide no utility whatsoever.

  2. Where we have USE_NL(t m) LEADING (t m) we are providing a verify specific access pathway that can only be interpreted one way. The CARDINALITY hints are thus redundant (these provide Oracle with expected counts from each table) because they would normally be fed to the optimiser to help it formulate a plan. But since we've stipulated a plan with USE_NL/LEADING, the cardinality is irrelevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I imagine the t m references are probably copied from somewhere, but you're right, they don't make sense here.

Confirmed that:

  • we use ORGANIZATION INDEX in creating the tables (OracleDdlTable:285)
  • on that basis, the INDEX hint on the primary key index is redundant
  • for this query, there is no join/product to speak of, so USE_NL and LEADING don't make sense.

+ " m.row_name, m.col_name, max(m.ts) as ts "
+ " FROM " + tableName + " m "
+ " WHERE m.row_name = ? "
Expand All @@ -71,10 +68,8 @@ public FullQuery getLatestRowQuery(byte[] row, long ts, ColumnSelection columns,

@Override
public FullQuery getLatestRowsQuery(Iterable<byte[]> rows, long ts, ColumnSelection columns, boolean includeValue) {
String query = " /* GET_LATEST_ROWS_SINGLE_BOUND_INNER (" + tableName + ") */ "
+ " SELECT"
+ " /*+ USE_NL(t m) LEADING(t m) CARDINALITY(t 1) CARDINALITY(m 10) INDEX(m "
+ PrimaryKeyConstraintNames.get(tableName) + ") */ "
String query = "SELECT"
+ " /*+ USE_NL(t m) LEADING(t m) */ "
Copy link
Contributor

Choose a reason for hiding this comment

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

Cardinality hint from above.

+ " m.row_name, m.col_name, max(m.ts) as ts "
+ " FROM " + tableName + " m, TABLE(CAST(? AS " + structArrayPrefix() + "CELL_TS_TABLE)) t "
+ " WHERE m.row_name = t.row_name "
Expand All @@ -95,39 +90,10 @@ public FullQuery getLatestRowsQuery(Iterable<byte[]> rows, long ts, ColumnSelect
: fullQuery.withArg(rowsToOracleArray(columns.getSelectedColumns()));
}

@Override
public FullQuery getLatestRowsQuery(
Collection<Map.Entry<byte[], Long>> rows, ColumnSelection columns, boolean includeValue) {
String query = " /* GET_LATEST_ROWS_MANY_BOUNDS_INNER (" + tableName + ") */ "
+ " SELECT"
+ " /*+ USE_NL(t m) LEADING(t m) CARDINALITY(t 1) CARDINALITY(m 10) INDEX(m "
+ PrimaryKeyConstraintNames.get(tableName) + ") */ "
+ " m.row_name, m.col_name, max(m.ts) as ts "
+ " FROM " + tableName + " m, TABLE(CAST(? AS " + structArrayPrefix() + "CELL_TS_TABLE)) t "
+ " WHERE m.row_name = t.row_name "
+ " AND m.ts < t.max_ts "
+ (columns.allColumnsSelected()
? ""
: " AND EXISTS ("
+ "SELECT"
+ " /*+ NL_SJ */"
+ " 1"
+ " FROM TABLE(CAST(? AS " + structArrayPrefix() + "CELL_TS_TABLE))"
+ " WHERE row_name = m.col_name) ")
+ " GROUP BY m.row_name, m.col_name";
query = wrapQueryWithIncludeValue("GET_LATEST_ROWS_MANY_BOUNDS", query, includeValue);
FullQuery fullQuery = new FullQuery(query).withArg(rowsAndTimestampsToOracleArray(rows));
return columns.allColumnsSelected()
? fullQuery
: fullQuery.withArg(rowsToOracleArray(columns.getSelectedColumns()));
}

@Override
public FullQuery getAllRowQuery(byte[] row, long ts, ColumnSelection columns, boolean includeValue) {
String query = " /* GET_ALL_ONE_ROW (" + tableName + ") */ "
+ " SELECT"
+ " /*+ INDEX(m " + PrimaryKeyConstraintNames.get(tableName) + ") */ "
+ " m.row_name, m.col_name, m.ts" + getValueSubselect("m", includeValue)
+ " SELECT m.row_name, m.col_name, m.ts" + getValueSubselect("m", includeValue)
+ " FROM " + tableName + " m "
+ " WHERE m.row_name = ? "
+ " AND m.ts < ? "
Expand All @@ -149,8 +115,7 @@ public FullQuery getAllRowQuery(byte[] row, long ts, ColumnSelection columns, bo
public FullQuery getAllRowsQuery(Iterable<byte[]> rows, long ts, ColumnSelection columns, boolean includeValue) {
String query = " /* GET_ALL_ROWS_SINGLE_BOUND (" + tableName + ") */ "
+ " SELECT"
+ " /*+ USE_NL(t m) LEADING(t m) CARDINALITY(t 1) CARDINALITY(m 10) INDEX(m "
+ PrimaryKeyConstraintNames.get(tableName) + ") */ "
+ " /*+ USE_NL(t m) LEADING(t m) */"
+ " m.row_name, m.col_name, m.ts" + getValueSubselect("m", includeValue)
+ " FROM " + tableName + " m, TABLE(CAST(? AS " + structArrayPrefix() + "CELL_TS_TABLE)) t "
+ " WHERE m.row_name = t.row_name "
Expand All @@ -169,37 +134,9 @@ public FullQuery getAllRowsQuery(Iterable<byte[]> rows, long ts, ColumnSelection
: fullQuery.withArg(rowsToOracleArray(columns.getSelectedColumns()));
}

@Override
public FullQuery getAllRowsQuery(
Collection<Map.Entry<byte[], Long>> rows, ColumnSelection columns, boolean includeValue) {
String query = " /* GET_ALL_ROWS_MANY_BOUNDS (" + tableName + ") */ "
+ " SELECT"
+ " /*+ USE_NL(t m) LEADING(t m) CARDINALITY(t 1) CARDINALITY(m 10) INDEX(m "
+ PrimaryKeyConstraintNames.get(tableName) + ") */ "
+ " m.row_name, m.col_name, m.ts" + getValueSubselect("m", includeValue)
+ " FROM " + tableName + " m, TABLE(CAST(? AS " + structArrayPrefix() + "CELL_TS_TABLE)) t "
+ " WHERE m.row_name = t.row_name "
+ " AND m.ts < t.max_ts "
+ (columns.allColumnsSelected()
? ""
: " AND EXISTS ("
+ "SELECT"
+ " /*+ NL_SJ */"
+ " 1"
+ " FROM TABLE(CAST(? AS " + structArrayPrefix() + "CELL_TS_TABLE))"
+ " WHERE row_name = m.col_name) ");
FullQuery fullQuery = new FullQuery(query).withArg(rowsAndTimestampsToOracleArray(rows));
return columns.allColumnsSelected()
? fullQuery
: fullQuery.withArg(rowsToOracleArray(columns.getSelectedColumns()));
}

@Override
public FullQuery getLatestCellQuery(Cell cell, long ts, boolean includeValue) {
String query = " /* GET_LATEST_ONE_CELLS_INNER (" + tableName + ") */ "
+ " SELECT "
+ " /*+ INDEX(m " + PrimaryKeyConstraintNames.get(tableName) + ") */ "
+ " m.row_name, m.col_name, max(m.ts) as ts "
String query = "SELECT m.row_name, m.col_name, max(m.ts) as ts"
+ " FROM " + tableName + " m "
+ " WHERE m.row_name = ? "
+ " AND m.col_name = ? "
Expand All @@ -209,28 +146,10 @@ public FullQuery getLatestCellQuery(Cell cell, long ts, boolean includeValue) {
return new FullQuery(query).withArgs(cell.getRowName(), cell.getColumnName(), ts);
}

@Override
public FullQuery getLatestCellsQuery(Iterable<Cell> cells, long ts, boolean includeValue) {
String query = " /* GET_LATEST_CELLS_SINGLE_BOUND_INNER (" + tableName + ") */ "
+ " SELECT"
+ " /*+ USE_NL(t m) LEADING(t m) CARDINALITY(t 1) CARDINALITY(m 10) INDEX(m "
+ PrimaryKeyConstraintNames.get(tableName) + ") */ "
+ " m.row_name, m.col_name, max(m.ts) as ts "
+ " FROM " + tableName + " m, TABLE(CAST(? AS " + structArrayPrefix() + "CELL_TS_TABLE)) t "
+ " WHERE m.row_name = t.row_name "
+ " AND m.col_name = t.col_name "
+ " AND m.ts < ? "
+ " GROUP BY m.row_name, m.col_name";
query = wrapQueryWithIncludeValue("GET_LATEST_CELLS_SINGLE_BOUND", query, includeValue);
return new FullQuery(query).withArgs(cellsToOracleArray(cells), ts);
}

@Override
public FullQuery getLatestCellsQuery(Collection<Map.Entry<Cell, Long>> cells, boolean includeValue) {
String query = " /* GET_LATEST_CELLS_MANY_BOUNDS_INNER (" + tableName + ") */ "
+ " SELECT"
+ " /*+ USE_NL(t m) LEADING(t m) CARDINALITY(t 1) CARDINALITY(m 10) INDEX(m "
+ PrimaryKeyConstraintNames.get(tableName) + ") */ "
String query = "SELECT"
+ " /*+ USE_NL(t m) LEADING(t m) */ "
+ " m.row_name, m.col_name, max(m.ts) as ts "
+ " FROM " + tableName + " m, TABLE(CAST(? AS " + structArrayPrefix() + "CELL_TS_TABLE)) t "
+ " WHERE m.row_name = t.row_name "
Expand All @@ -245,7 +164,6 @@ public FullQuery getLatestCellsQuery(Collection<Map.Entry<Cell, Long>> cells, bo
public FullQuery getAllCellQuery(Cell cell, long ts, boolean includeValue) {
String query = " /* GET_ALL_ONE_CELL (" + tableName + ") */ "
+ " SELECT"
+ " /*+ INDEX(m " + PrimaryKeyConstraintNames.get(tableName) + ") */ "
+ " m.row_name, m.col_name, m.ts" + getValueSubselect("m", includeValue)
+ " FROM " + tableName + " m "
+ " WHERE m.row_name = ? "
Expand All @@ -258,8 +176,7 @@ public FullQuery getAllCellQuery(Cell cell, long ts, boolean includeValue) {
public FullQuery getAllCellsQuery(Iterable<Cell> cells, long ts, boolean includeValue) {
String query = " /* GET_ALL_CELLS_SINGLE_BOUND (" + tableName + ") */ "
+ " SELECT"
+ " /*+ USE_NL(t m) LEADING(t m) CARDINALITY(t 1) CARDINALITY(m 10) INDEX(m "
+ PrimaryKeyConstraintNames.get(tableName) + ") */ "
+ " /*+ USE_NL(t m) LEADING(t m) */ "
+ " m.row_name, m.col_name, m.ts" + getValueSubselect("m", includeValue)
+ " FROM " + tableName + " m, TABLE(CAST(? AS " + structArrayPrefix() + "CELL_TS_TABLE)) t "
+ " WHERE m.row_name = t.row_name "
Expand All @@ -268,20 +185,6 @@ public FullQuery getAllCellsQuery(Iterable<Cell> cells, long ts, boolean include
return new FullQuery(query).withArgs(cellsToOracleArray(cells), ts);
}

@Override
public FullQuery getAllCellsQuery(Collection<Map.Entry<Cell, Long>> cells, boolean includeValue) {
String query = " /* GET_ALL_CELLS_MANY_BOUNDS (" + tableName + ") */ "
+ " SELECT"
+ " /*+ USE_NL(t m) LEADING(t m) CARDINALITY(t 1) CARDINALITY(m 10) INDEX(m "
+ PrimaryKeyConstraintNames.get(tableName) + ") */ "
+ " m.row_name, m.col_name, m.ts" + getValueSubselect("m", includeValue)
+ " FROM " + tableName + " m, TABLE(CAST(? AS " + structArrayPrefix() + "CELL_TS_TABLE)) t "
+ " WHERE m.row_name = t.row_name "
+ " AND m.col_name = t.col_name "
+ " AND m.ts < t.max_ts ";
return new FullQuery(query).withArg(cellsAndTimestampsToOracleArray(cells));
}

@Override
public FullQuery getRangeQuery(RangeRequest range, long ts, int maxRows) {
List<String> bounds = new ArrayList<>(2);
Expand All @@ -298,7 +201,7 @@ public FullQuery getRangeQuery(RangeRequest range, long ts, int maxRows) {
}
if (maxRows == 1) {
String query = " /* GET_RANGE_ONE_ROW (" + tableName + ") */ "
+ " SELECT /*+ INDEX(m " + PrimaryKeyConstraintNames.get(tableName) + ") */ "
+ " SELECT "
+ (range.isReverse() ? "max" : "min") + "(m.row_name) as row_name "
+ " FROM " + tableName + " m "
+ (bounds.isEmpty() ? "" : " WHERE " + Joiner.on(" AND ").join(bounds));
Expand All @@ -307,7 +210,7 @@ public FullQuery getRangeQuery(RangeRequest range, long ts, int maxRows) {

String query = " /* GET_RANGE_ROWS (" + tableName + ") */ "
+ " SELECT inner.row_name FROM "
+ " ( SELECT /*+ INDEX(m " + PrimaryKeyConstraintNames.get(tableName) + ") */ "
+ " ( SELECT"
+ " DISTINCT m.row_name "
+ " FROM " + tableName + " m "
+ (bounds.isEmpty() ? "" : " WHERE " + Joiner.on(" AND ").join(bounds))
Expand Down Expand Up @@ -373,15 +276,16 @@ protected FullQuery getRowsColumnRangeSubQuery(
@Override
protected FullQuery getRowsColumnRangeFullyLoadedRowsSubQuery(
List<byte[]> rows, long ts, ColumnRangeSelection columnRangeSelection) {
String query = " /* GET_ROWS_COLUMN_RANGE_FULLY_LOADED_ROWS (" + tableName + ") */ "
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't duplicate the query "name" - the wrapQueryWithIncludeValue wraps these inner queries, and we only need to specify the name there - once.

+ "SELECT * FROM ( SELECT m.row_name, m.col_name, max(m.ts) as ts"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove redundant outer select, ie:
SELECT * FROM (X) -> X

+ " FROM " + tableName + " m, TABLE(CAST(? AS " + structArrayPrefix() + "CELL_TS_TABLE)) t "
+ " WHERE m.row_name = t.row_name "
+ " AND m.ts < ? "
String query = "SELECT"
+ " /*+ USE_NL(t m) LEADING(t m) */"
Copy link
Contributor Author

@CVdV-au CVdV-au Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Added this hint where there was not one. In response to issue seen on #pds-594762 where we observed a plan:
image (7)
rather than the expected/sensible form:

               Operation                 |             Options              |            Object Name           | Cardinality  |    Bytes     |     Cost
---------------------------------------- | -------------------------------- | -------------------------------- | ------------ | ------------ | ------------
SELECT STATEMENT                         |                                  |                                  |            0 |            0 |         1959
 SORT                                    | ORDER BY                         |                                  |         1627 |       109009 |         1959
  NESTED LOOPS                           |                                  |                                  |         1627 |       109009 |         1959
   VIEW                                  |                                  |                                  |         1627 |        47183 |          330
    HASH                                 | GROUP BY                         |                                  |         1627 |        39048 |          330
     NESTED LOOPS                        |                                  |                                  |         1627 |        39048 |          329
      COLLECTION ITERATOR                | PICKLER FETCH                    |                                  |          100 |          200 |           29
      INDEX                              | RANGE SCAN                       | PK_PT_MET_OBJ_PARENT_LINKS_ACL   |           16 |          352 |            3
   INDEX                                 | UNIQUE SCAN                      | PK_PT_MET_OBJ_PARENT_LINKS_ACL   |            1 |           38 |            2

+ " m.row_name, m.col_name, max(m.ts) as ts"
+ " FROM " + tableName + " m, TABLE(CAST(? AS " + structArrayPrefix() + "CELL_TS_TABLE)) t "
+ " WHERE m.row_name = t.row_name "
+ " AND m.ts < ? "
+ (columnRangeSelection.getStartCol().length > 0 ? " AND m.col_name >= ?" : "")
+ (columnRangeSelection.getEndCol().length > 0 ? " AND m.col_name < ?" : "")
+ " GROUP BY m.row_name, m.col_name"
+ " ORDER BY m.row_name ASC, m.col_name ASC )";
+ " ORDER BY m.row_name ASC, m.col_name ASC";
String wrappedQuery = wrapQueryWithIncludeValue("GET_ROWS_COLUMN_RANGE_FULLY_LOADED_ROWS", query, true);
FullQuery fullQuery = new FullQuery(wrappedQuery).withArgs(rowsToOracleArray(rows), ts);
if (columnRangeSelection.getStartCol().length > 0) {
Expand Down Expand Up @@ -436,16 +340,6 @@ private ArrayHandler cellsToOracleArray(Iterable<Cell> cells) {
structArrayPrefix() + "CELL_TS", "" + structArrayPrefix() + "CELL_TS_TABLE", oraRows);
}

private ArrayHandler rowsAndTimestampsToOracleArray(Collection<Map.Entry<byte[], Long>> rows) {
List<Object[]> oraRows = new ArrayList<>(rows.size());
for (Map.Entry<byte[], Long> entry : rows) {
oraRows.add(new Object[] {entry.getKey(), null, entry.getValue()});
}
return config.jdbcHandler()
.createStructArray(
structArrayPrefix() + "CELL_TS", "" + structArrayPrefix() + "CELL_TS_TABLE", oraRows);
}

private ArrayHandler cellsAndTimestampsToOracleArray(Collection<Map.Entry<Cell, Long>> cells) {
List<Object[]> oraRows = new ArrayList<>(cells.size());
for (Map.Entry<Cell, Long> entry : cells) {
Expand Down
Loading