Skip to content

Add oracle, redshift, postgresql jdbc fetch size#22670

Merged
Praveen2112 merged 3 commits intotrinodb:masterfrom
vlad-lyutenko:vlad-lyutenko/oracle-jdbc-fetch-size
Aug 7, 2024
Merged

Add oracle, redshift, postgresql jdbc fetch size#22670
Praveen2112 merged 3 commits intotrinodb:masterfrom
vlad-lyutenko:vlad-lyutenko/oracle-jdbc-fetch-size

Conversation

@vlad-lyutenko
Copy link
Copy Markdown
Contributor

@vlad-lyutenko vlad-lyutenko commented Jul 15, 2024

trino specific heuristic is applied if empty or zero

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Oracle, Redshift, PostgreSql
* Allow customising fetch size using `oracle.fetch-size`, `redshift.fetch-size` , `postgresql.fetch-size` respectively for each connector. 

@cla-bot cla-bot bot added the cla-signed label Jul 15, 2024
@vlad-lyutenko vlad-lyutenko changed the title Add oracle jdbc fetch size, Add oracle jdbc fetch size Jul 15, 2024
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/oracle-jdbc-fetch-size branch from 5f75046 to 67c384c Compare July 15, 2024 10:47
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/oracle-jdbc-fetch-size branch 2 times, most recently from fcaa8f6 to 956503a Compare July 15, 2024 11:13
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/oracle-jdbc-fetch-size branch from 956503a to 35b2958 Compare July 15, 2024 12:33
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/oracle-jdbc-fetch-size branch from 35b2958 to 116adca Compare July 15, 2024 13:10
@vlad-lyutenko vlad-lyutenko changed the title Add oracle jdbc fetch size Add oracle, redshift, postgresql jdbc fetch size Jul 15, 2024
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/oracle-jdbc-fetch-size branch 2 times, most recently from c1d61f0 to 8515186 Compare July 15, 2024 14:25
Copy link
Copy Markdown
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

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

How about having a commit for each connector ?

@vlad-lyutenko
Copy link
Copy Markdown
Contributor Author

But not setting the value is not equivalent to setting it as 0 right ?

For Oracle :

public final void setFetchSize(int rows) throws SQLException {
        Monitor.CloseableLock lock = this.connection.acquireCloseableLock();

        try {
            this.ensureOpen();
            this.setPrefetchInternal(rows, false, true);
            this.isFetchSizeSet = rows != 0;
            this.autoTuneRowPrefetch = false;

For redshift:

public void setFetchSize(int rows) throws SQLException {
        if (RedshiftLogger.isEnable()) {
            this.connection.getLogger().logFunction(true, new Object[]{rows});
        }

        this.checkClosed();
        if (rows < 0) {
            throw new RedshiftException(GT.tr("Fetch size must be a value greater to or equal to 0.", new Object[0]), RedshiftState.INVALID_PARAMETER_VALUE);
        } else {
            this.fetchSize = rows;

and fetchsize:
this.fetchSize = 0;

for postgress:
DEFAULT_ROW_FETCH_SIZE("defaultRowFetchSize", "0", "Positive number of rows that should be fetched from the database when more rows are needed for ResultSet by each fetch iteration"),

So set 0 is equal to not set.

And latter in the code there is check:

  if (new_value == 0) {
                new_value = this.connection.getDefaultRowPrefetch();
            }

So if initial value is 0 (or not set), then DefaultRowPrefetch is applied.

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/oracle-jdbc-fetch-size branch from 8515186 to 9fe8dc7 Compare July 15, 2024 17:59
@vlad-lyutenko
Copy link
Copy Markdown
Contributor Author

vlad-lyutenko commented Jul 15, 2024

But the default value could change it is not equivalent to 10 or 0 or any number right ?

As far as I understood connector's code, setting fetch size to 0 or not setting it at all, means for connectors - use some internal default value.

@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Jul 16, 2024

@vlad-lyutenko We didn't agree with this new property on #16269 and merged #16644 instead. Could you explain more detailed context in the PR description?

@vlad-lyutenko
Copy link
Copy Markdown
Contributor Author

vlad-lyutenko commented Jul 16, 2024

@vlad-lyutenko We didn't agree with this new property on #16269 and merged #16644 instead. Could you explain more detailed context in the PR description?

Yep, but our heuristic formula is not perfect (I mean doesn't cover all corner cases), if user knows better his data layout and query type, maybe it's worth to give him ability to set explicitly fetch size, until we tune our heuristic formula to the perfect state.

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/oracle-jdbc-fetch-size branch 2 times, most recently from 05d85a6 to cbb2dde Compare July 16, 2024 12:25
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/oracle-jdbc-fetch-size branch from cbb2dde to 6ef6fde Compare July 22, 2024 16:42
Copy link
Copy Markdown
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

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

% comments

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/oracle-jdbc-fetch-size branch 3 times, most recently from b07a030 to cf93e29 Compare August 6, 2024 15:38
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/oracle-jdbc-fetch-size branch from cf93e29 to 4be2983 Compare August 7, 2024 09:27
@Praveen2112
Copy link
Copy Markdown
Member

/test-with-secrets sha=4be2983e71641333581c42607670e5266bbffe53

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 7, 2024

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/10281796499

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/oracle-jdbc-fetch-size branch from 4be2983 to d880bd4 Compare August 7, 2024 09:36
@Praveen2112
Copy link
Copy Markdown
Member

/test-with-secrets sha=d880bd47b8a25a8bded3134a7898dfd9dcabc244

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 7, 2024

The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/10284645638

@Praveen2112
Copy link
Copy Markdown
Member

@Praveen2112 Praveen2112 merged commit 3a08e1f into trinodb:master Aug 7, 2024
@github-actions github-actions bot added this to the 454 milestone Aug 7, 2024
@mosabua
Copy link
Copy Markdown
Member

mosabua commented Aug 21, 2024

This will need to be documented.

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.

7 participants