Skip to content
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

Document postgresql.include-system-tables config property #18574

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

Ankan0011
Copy link
Member

@Ankan0011 Ankan0011 commented Aug 7, 2023

Description

Added in general postgreSQL connector configuration in the documentation for support for reading PostgreSQL system tables, e.g., pg_catalog relations. The functionality is disabled by default.

Additional context and related issues

Fixes #15424
Relates to https://trinodb.slack.com/archives/CGB0QHWSW/p1671165424335919

Release notes

(X) 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.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot
Copy link

cla-bot bot commented Aug 7, 2023

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@@ -51,3 +51,6 @@ connector:
JDBC query. Using a large timeout can potentially result in more detailed
dynamic filters. However, it can also increase latency for some queries.
- ``20s``
* - ``postgresql.include-system-tables``
Copy link
Member

Choose a reason for hiding this comment

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

This file is jdbc-common-configurations.fragment which is used from several connectors. Please move to postgresql.rst.

@@ -51,3 +51,6 @@ connector:
JDBC query. Using a large timeout can potentially result in more detailed
dynamic filters. However, it can also increase latency for some queries.
- ``20s``
* - ``postgresql.include-system-tables``
- Supports access to postgres system catalog tables(pg_catalog schemas).
Copy link
Member

Choose a reason for hiding this comment

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

"Supports reading PostgreSQL system tables, e.g., pg_catalog relations."

@@ -51,3 +51,6 @@ connector:
JDBC query. Using a large timeout can potentially result in more detailed
dynamic filters. However, it can also increase latency for some queries.
- ``20s``
* - ``postgresql.include-system-tables``
- Supports access to postgres system catalog tables(pg_catalog schemas).
- ``false``
Copy link
Member

Choose a reason for hiding this comment

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

Please update the commit title as "Document postgresql.include-system-tables config property"
https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

@ebyhr ebyhr changed the title #15424 Documentation for postgres general connector config. Document postgresql.include-system-tables config property Aug 7, 2023
@github-actions github-actions bot added the docs label Aug 7, 2023
@cla-bot
Copy link

cla-bot bot commented Aug 8, 2023

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@@ -285,7 +285,9 @@ The following values are accepted for this property:
Querying PostgreSQL
-------------------

The PostgreSQL connector provides a schema for every PostgreSQL schema.
The PostgreSQL connector supports reading PostgreSQL system tables, e.g., ``pg_catalog`` relations.
Copy link
Member

Choose a reason for hiding this comment

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

Please rebase on master to resolve conflicts. The file was converted to markdown.

@cla-bot
Copy link

cla-bot bot commented Aug 8, 2023

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

1 similar comment
@cla-bot
Copy link

cla-bot bot commented Aug 8, 2023

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@Ankan0011 Ankan0011 requested a review from ebyhr August 9, 2023 16:16
@cla-bot
Copy link

cla-bot bot commented Aug 9, 2023

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@ebyhr ebyhr requested review from mosabua and removed request for ebyhr August 9, 2023 23:40
@@ -291,7 +291,9 @@ The following values are accepted for this property:

## Querying PostgreSQL

The PostgreSQL connector provides a schema for every PostgreSQL schema.
The PostgreSQL connector provides a schema for every PostgreSQL schema.
It also supports reading PostgreSQL system tables, e.g., `pg_catalog` relations.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It also supports reading PostgreSQL system tables, e.g., `pg_catalog` relations.
It also supports reading PostgreSQL system tables, such as `pg_catalog`.

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

This content should be moved up into the Configuration section .. maybe under a separate subtitle "Access to system tables"

The PostgreSQL connector provides a schema for every PostgreSQL schema.
The PostgreSQL connector provides a schema for every PostgreSQL schema.
It also supports reading PostgreSQL system tables, e.g., `pg_catalog` relations.
The functionality is disabled by default but this can be enabled using the ``postgresql.include-system-tables`` configuration property.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The functionality is disabled by default but this can be enabled using the ``postgresql.include-system-tables`` configuration property.
The functionality is disabled by default, and can be enabled using the ``postgresql.include-system-tables`` configuration property.

@mosabua
Copy link
Member

mosabua commented Aug 11, 2023

Also .. please rebase and send a signed CLA.

@cla-bot
Copy link

cla-bot bot commented Aug 11, 2023

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@Ankan0011
Copy link
Member Author

Also .. please rebase and send a signed CLA.

I have send the CLA multiple times. I have changed my email and sent again.

@mosabua
Copy link
Member

mosabua commented Aug 11, 2023

Also .. please rebase and send a signed CLA.

I have send the CLA multiple times. I have changed my email and sent again.

Looks like it was not yet added to https://github.com/trinodb/cla/blob/master/contributors
Did you receive the email @martint ?

docs/src/main/sphinx/connector/postgresql.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/postgresql.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/postgresql.md Outdated Show resolved Hide resolved
@cla-bot
Copy link

cla-bot bot commented Aug 12, 2023

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@Ankan0011 Ankan0011 force-pushed the #15424_issue_doc_update branch from eb9bd24 to 3974b73 Compare August 19, 2023 22:17
@cla-bot cla-bot bot added the cla-signed label Aug 19, 2023
```

More details about the PostgreSQL system tables, see the [PostgreSQL system
catalog documentation](https://www.postgresql.org/docs/current/catalogs.html).
Copy link
Member

Choose a reason for hiding this comment

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

According to that URL there is no pg_tables table...

@Ankan0011 Ankan0011 force-pushed the #15424_issue_doc_update branch from 3974b73 to fc69718 Compare August 20, 2023 10:15
@mosabua mosabua force-pushed the #15424_issue_doc_update branch from fc69718 to 4fe0877 Compare August 21, 2023 16:44
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Fixed the last issues and the commit message. Merging.

@mosabua mosabua merged commit 5d8824e into trinodb:master Aug 21, 2023
@github-actions github-actions bot added this to the 425 milestone Aug 21, 2023
@Ankan0011 Ankan0011 deleted the #15424_issue_doc_update branch August 21, 2023 19:25
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.

Document postgresql.include-system-tables config property
3 participants