Remove dead DBKVS code, fix Oracle hints #7338

CVdV-au · 2024-10-10T06:41:56Z

==COMMIT_MSG==
Oracle hints/query improved; dead DBKVS code removed
==COMMIT_MSG==

Priority:
High, see #pds-594762

changelog-app · 2024-10-10T06:42:00Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

Oracle hints/query improved; dead DBKVS code removed

Check the box to generate changelog(s)

Generate changelog entry

CVdV-au · 2024-10-10T06:43:40Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbQueryFactory.java

@@ -28,27 +28,18 @@ public interface DbQueryFactory {

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

-    FullQuery getLatestRowsQuery(


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

CVdV-au · 2024-10-10T07:02:01Z

...-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleQueryFactory.java

-                + " SELECT"
-                + "   /*+ USE_NL(t m) LEADING(t m) CARDINALITY(t 1) CARDINALITY(m 10) INDEX(m "
-                + PrimaryKeyConstraintNames.get(tableName) + ") */ "
+        String query = "SELECT"


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

More generally:

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.

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.

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.

CVdV-au · 2024-10-10T07:05:12Z

...-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleQueryFactory.java

@@ -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 + ") */ "


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

CVdV-au · 2024-10-10T07:07:12Z

...-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleQueryFactory.java

@@ -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 + ") */ "
-                + "SELECT * FROM ( SELECT m.row_name, m.col_name, max(m.ts) as ts"


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

CVdV-au · 2024-10-10T07:09:57Z

...-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleQueryFactory.java

-                + "  WHERE m.row_name = t.row_name "
-                + "    AND m.ts < ? "
+        String query = "SELECT"
+                + " /*+ USE_NL(t m) LEADING(t m) */"


Added this hint where there was not one. In response to issue seen on #pds-594762 where we observed a plan:

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

berler · 2024-10-11T17:21:52Z

I've confirmed that our internal products that actually use oracle still compile with this change. +1 to removing unused methods.

The other changes also look correct to me.

jeremyk-91 · 2024-10-28T21:56:37Z

Apologies for the delay here, team has been beleaguered with some brutal support issues last week. Picking this back up on my end...

jeremyk-91

👍 Thanks for submitting this!

I confirmed that outside of the specific query we looked at in getRowsColumnRangeFullyLoadedRowsSubQuery where we added the USE_NL and LEADING hints, all other changes to queries were one of

removing INDEX from queries on an IOT table
removing CARDINALITY from queries that have USE_NL and LEADING
removing "names" from queries that already are wrapped in named queries in only one possible way
removing USE_NL and LEADING from queries that don't actually have a join

jeremyk-91 · 2024-10-28T22:06:33Z

...-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleQueryFactory.java

-                + " SELECT"
-                + "   /*+ USE_NL(t m) LEADING(t m) CARDINALITY(t 1) CARDINALITY(m 10) INDEX(m "
-                + PrimaryKeyConstraintNames.get(tableName) + ") */ "
+        String query = "SELECT"


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.

jeremyk-91 · 2024-10-28T22:06:42Z

...-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleQueryFactory.java

-                + "   /*+ 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) */ "


Cardinality hint from above.

autorelease3 · 2024-10-28T22:14:42Z

Released 0.1181.0

Remove dead DBKVS code, fix Oracle hints

d7bf13a

CVdV-au commented Oct 10, 2024

View reviewed changes

Broken hint

f79e812

CVdV-au commented Oct 10, 2024

View reviewed changes

CVdV-au requested review from berler, jeremyk-91, sverma30 and raiju October 10, 2024 07:31

CVdV-au added merge when ready autorelease labels Oct 10, 2024

berler approved these changes Oct 11, 2024

View reviewed changes

Add generated changelog entries

eaf4cb4

jeremyk-91 approved these changes Oct 28, 2024

View reviewed changes

bulldozer-bot bot merged commit 0dfdd21 into develop Oct 28, 2024
21 checks passed

bulldozer-bot bot deleted the cvdv_au/pds-594762 branch October 28, 2024 22:14

CVdV-au mentioned this pull request Nov 1, 2024

Add explicit hints #7418

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dead DBKVS code, fix Oracle hints #7338

Remove dead DBKVS code, fix Oracle hints #7338

CVdV-au commented Oct 10, 2024

changelog-app bot commented Oct 10, 2024 •

edited by berler

Loading

CVdV-au Oct 10, 2024

CVdV-au Oct 10, 2024

jeremyk-91 Oct 28, 2024

CVdV-au Oct 10, 2024

CVdV-au Oct 10, 2024

CVdV-au Oct 10, 2024 •

edited

Loading

berler commented Oct 11, 2024 •

edited

Loading

jeremyk-91 commented Oct 28, 2024

jeremyk-91 left a comment

jeremyk-91 Oct 28, 2024

jeremyk-91 Oct 28, 2024

autorelease3 bot commented Oct 28, 2024

		@@ -28,27 +28,18 @@ public interface DbQueryFactory {

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

		FullQuery getLatestRowsQuery(

Remove dead DBKVS code, fix Oracle hints #7338

Remove dead DBKVS code, fix Oracle hints #7338

Conversation

CVdV-au commented Oct 10, 2024

changelog-app bot commented Oct 10, 2024 • edited by berler Loading

Generate changelog in changelog/@unreleased

CVdV-au Oct 10, 2024

Choose a reason for hiding this comment

CVdV-au Oct 10, 2024

Choose a reason for hiding this comment

jeremyk-91 Oct 28, 2024

Choose a reason for hiding this comment

CVdV-au Oct 10, 2024

Choose a reason for hiding this comment

CVdV-au Oct 10, 2024

Choose a reason for hiding this comment

CVdV-au Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

berler commented Oct 11, 2024 • edited Loading

jeremyk-91 commented Oct 28, 2024

jeremyk-91 left a comment

Choose a reason for hiding this comment

jeremyk-91 Oct 28, 2024

Choose a reason for hiding this comment

jeremyk-91 Oct 28, 2024

Choose a reason for hiding this comment

autorelease3 bot commented Oct 28, 2024

changelog-app bot commented Oct 10, 2024 •

edited by berler

Loading

Generate changelog in `changelog/@unreleased`

CVdV-au Oct 10, 2024 •

edited

Loading

berler commented Oct 11, 2024 •

edited

Loading