Skip to content

Allow access to Postgres partitioned tables#10403

Merged
hashhar merged 1 commit intotrinodb:masterfrom
guyco33:pg_partitioned_table
Jan 10, 2022
Merged

Allow access to Postgres partitioned tables#10403
hashhar merged 1 commit intotrinodb:masterfrom
guyco33:pg_partitioned_table

Conversation

@guyco33
Copy link
Copy Markdown
Member

@guyco33 guyco33 commented Dec 23, 2021

Fixes #10400

Postgres Jdbc 42.2.14+ added a new PARTITIONED TABLE type that must be included in PgDatabaseMetaData.getTables function arguments to retrieve partitioned tables

@cla-bot cla-bot bot added the cla-signed label Dec 23, 2021
@guyco33 guyco33 requested a review from hashhar December 23, 2021 17:32
Comment on lines 212 to 213
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.

Please use TestTable. Also, it would be nice to test SELECT query as well.

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 actual partition tables should be created before data inserts, so TestTable can't be used with rows.
Meanwhile, I added the SELECT tests with explicit CREATE and INSERT statements

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.

TestTable allows creating tables without rows.

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.

Ye, it's much cleaner

Copy link
Copy Markdown
Member

@ebyhr ebyhr Dec 23, 2021

Choose a reason for hiding this comment

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

Please don't upgrade the version here. You can introduce base test class and run tests on two versions likes BaseSqlServerConnectorSmokeTest.

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.

9.6 is already unsupported so I think it's the proper time to upgrade

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 would like to use old version here, so it should be 10.x if you want to upgrade. The version 10 is enough for testing partitioned tables as far as I know.
Also, please update postgresql.rst that has minimum required version.

@guyco33 guyco33 force-pushed the pg_partitioned_table branch 4 times, most recently from b3aedc6 to bc2ea1e Compare December 26, 2021 06:08
Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Looks good % comments. Also add the description from the PR into the commit message - it's useful context.

Thanks for the fix.

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.

I think timestamps are useful sometimes on CI - for example when a certain test appears slow or stuck.

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.

Agree, and that's why they added to the log in Postgres 10+

@guyco33 guyco33 force-pushed the pg_partitioned_table branch 2 times, most recently from e4ec0e5 to 94936d2 Compare December 30, 2021 12:14
Fixes trinodb#10400
Postgres Jdbc 42.2.14+ added a new PARTITIONED TABLE type that must be included in PgDatabaseMetaData.getTables function arguments to retrieve partitioned tables
@guyco33 guyco33 force-pushed the pg_partitioned_table branch from 94936d2 to b59e4dd Compare January 3, 2022 20:30
{
// Use the oldest supported PostgreSQL version
dockerContainer = new PostgreSQLContainer<>("postgres:9.6")
dockerContainer = new PostgreSQLContainer<>("postgres:10")
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.

Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Looks good to me. Postgres 9.x hasn't been supported since November 2021 - we can add a smoke test using 9.x if you still feel we should test with 9.x for a bit more @findepi .

@findepi
Copy link
Copy Markdown
Member

findepi commented Jan 10, 2022

if it's EOL'd, then it's fine.

@hashhar hashhar merged commit 92e5ba6 into trinodb:master Jan 10, 2022
@hashhar hashhar mentioned this pull request Jan 10, 2022
@github-actions github-actions bot added this to the 368 milestone Jan 10, 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.

Postgres tables created with declarative partitioning option are not accessible

4 participants