fix(pgcdc): fix uppercase-identifier of pgcdc #17754

KeXiangWang · 2024-07-19T02:28:10Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

fix #17752
Change the sql from using ::regclass to using schemaName and tableName

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

StrikeW · 2024-07-19T02:47:38Z

...onnector-service/src/main/java/com/risingwave/connector/source/common/PostgresValidator.java

@@ -180,7 +180,8 @@ private void validateTableSchema() throws SQLException {
        // check primary key
        // reference: https://wiki.postgresql.org/wiki/Retrieve_primary_key_columns
        try (var stmt = jdbcConnection.prepareStatement(ValidatorUtils.getSql("postgres.pk"))) {
-            stmt.setString(1, this.schemaName + "." + this.tableName);


Why not just quote the object name?
String.format("\"%s\".\"%s\"", this.schemaName, this.tableName)

Can be another choice. I noticed that other sql statements use schemaName and tableName, so I think we should keep them consistent. Do you prefer "\"%s\".\"%s\""?

The change to the validate sql needs to join two more system tables, I think it can be avoided.

Slightly prefer to keep consistent with other places, unless the cost of join is high.

Strictly speaking the manual quoting above is wrong, but I do believe no one would challenge this in practice:

test=# create table "my_test""table" ("Foo" int); -- two quotes required CREATE TABLE test=# select relname, quote_ident(relname) from pg_class where relname ^@ 'my_te'; relname | quote_ident ---------------+------------------ my_test"table | "my_test""table" (1 row)

(pgdoc)

Embedded quotes are properly doubled

yuhao-su · 2024-07-19T03:16:07Z

I thought the name should only be quoted by " when used as an identifier. cc @xiangjinwu to confirm

StrikeW · 2024-07-19T03:20:36Z

I thought the name should only be quoted by " when used as an identifier. cc @xiangjinwu to confirm

I think so, IIRC. any concern?

...onnector-service/src/main/java/com/risingwave/connector/source/common/PostgresValidator.java

StrikeW

LGTM

yuhao-su · 2024-07-19T03:33:49Z

any concern?

All good. Never mind.

StrikeW · 2024-07-19T03:36:56Z

e2e_test/source/cdc/cdc.load.slt

+ port = '${PGPORT:5432}',
+ username = '${PGUSER:$USER}',
+ password = '${PGPASSWORD:}',
+ database.name = '${PGDATABASE:postgres}',


BTW, we better to test whether uppercase in database.name can work.

Tested. It works. But I also found a weird bug #17754 (comment). After some attempts, it's sometimes not reproduciable: only several special tables can trigger this bug. I'm still not aware of the exact condition. To merge this PR, I temporarily delete the tests about database.name. Will create an issue later to track and fix it.

ok, we didn't get bug report from our users yet. Agree to create a issue to track it.

gitguardian · 2024-07-19T04:01:57Z

⚠️ GitGuardian has uncovered 8 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
9425213	Triggered	Generic Password	`0670570`	ci/scripts/e2e-source-test.sh	View secret
9425213	Triggered	Generic Password	`0670570`	ci/scripts/e2e-source-test.sh	View secret
9425213	Triggered	Generic Password	`0670570`	ci/scripts/e2e-source-test.sh	View secret
9425213	Triggered	Generic Password	`7cf41c9`	ci/scripts/e2e-source-test.sh	View secret
9425213	Triggered	Generic Password	`7cf41c9`	ci/scripts/e2e-source-test.sh	View secret
9425213	Triggered	Generic Password	`938f610`	ci/scripts/e2e-source-test.sh	View secret
9425213	Triggered	Generic Password	`938f610`	ci/scripts/e2e-source-test.sh	View secret
9425213	Triggered	Generic Password	`938f610`	ci/scripts/e2e-source-test.sh	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

tabVersion

either approach lgtm

KeXiangWang · 2024-07-20T02:32:25Z

Accidentally find a bug: if there's a new db is created in the upstream pg after RW setup a cdc connector, RW will crash. Investigating.

ci/scripts/e2e-source-test.sh

fix(cdc): fix uppercase-identifier of pgcdc

167e938

KeXiangWang requested review from tabVersion and StrikeW July 19, 2024 02:28

github-actions bot added the type/fix Bug fix label Jul 19, 2024

KeXiangWang requested a review from yuhao-su July 19, 2024 02:28

add test

3ea9d5b

StrikeW reviewed Jul 19, 2024

View reviewed changes

yuhao-su requested a review from xiangjinwu July 19, 2024 02:52

xiangjinwu reviewed Jul 19, 2024

View reviewed changes

...onnector-service/src/main/java/com/risingwave/connector/source/common/PostgresValidator.java Outdated Show resolved Hide resolved

StrikeW approved these changes Jul 19, 2024

View reviewed changes

StrikeW reviewed Jul 19, 2024

View reviewed changes

KeXiangWang force-pushed the wkx/fix-pgcdc-uppercase-identifier branch 2 times, most recently from cd1a646 to d1c95b2 Compare July 19, 2024 04:12

use uppercase identifier

0670570

KeXiangWang force-pushed the wkx/fix-pgcdc-uppercase-identifier branch from d1c95b2 to 0670570 Compare July 19, 2024 04:18

tabVersion approved these changes Jul 19, 2024

View reviewed changes

fix test

7cf41c9

yuhao-su approved these changes Jul 22, 2024

View reviewed changes

yuhao-su reviewed Jul 22, 2024

View reviewed changes

ci/scripts/e2e-source-test.sh Outdated Show resolved Hide resolved

KeXiangWang added 4 commits July 22, 2024 17:44

Merge branch 'main' into wkx/fix-pgcdc-uppercase-identifier

bce6b62

fix test

938f610

fix test

4c3b194

fix test

c20713b

KeXiangWang force-pushed the wkx/fix-pgcdc-uppercase-identifier branch 2 times, most recently from 9d92915 to 7374402 Compare July 23, 2024 01:09

fix test

8aa28b0

KeXiangWang force-pushed the wkx/fix-pgcdc-uppercase-identifier branch from 7374402 to 8aa28b0 Compare July 23, 2024 01:25

KeXiangWang added this pull request to the merge queue Jul 23, 2024

Merged via the queue into main with commit 5bd3e3e Jul 23, 2024
28 of 30 checks passed

KeXiangWang deleted the wkx/fix-pgcdc-uppercase-identifier branch July 23, 2024 02:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(pgcdc): fix uppercase-identifier of pgcdc #17754

fix(pgcdc): fix uppercase-identifier of pgcdc #17754

KeXiangWang commented Jul 19, 2024

StrikeW Jul 19, 2024

KeXiangWang Jul 19, 2024

StrikeW Jul 19, 2024

KeXiangWang Jul 19, 2024

xiangjinwu Jul 19, 2024 •

edited

Loading

yuhao-su commented Jul 19, 2024

StrikeW commented Jul 19, 2024

StrikeW left a comment

yuhao-su commented Jul 19, 2024

StrikeW Jul 19, 2024

KeXiangWang Jul 22, 2024 •

edited

Loading

StrikeW Jul 23, 2024

gitguardian bot commented Jul 19, 2024 •

edited

Loading

tabVersion left a comment

KeXiangWang commented Jul 20, 2024 •

edited

Loading

fix(pgcdc): fix uppercase-identifier of pgcdc #17754

fix(pgcdc): fix uppercase-identifier of pgcdc #17754

Conversation

KeXiangWang commented Jul 19, 2024

What's changed and what's your intention?

Checklist

Documentation

Release note

StrikeW Jul 19, 2024

Choose a reason for hiding this comment

KeXiangWang Jul 19, 2024

Choose a reason for hiding this comment

StrikeW Jul 19, 2024

Choose a reason for hiding this comment

KeXiangWang Jul 19, 2024

Choose a reason for hiding this comment

xiangjinwu Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

yuhao-su commented Jul 19, 2024

StrikeW commented Jul 19, 2024

StrikeW left a comment

Choose a reason for hiding this comment

yuhao-su commented Jul 19, 2024

StrikeW Jul 19, 2024

Choose a reason for hiding this comment

KeXiangWang Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

StrikeW Jul 23, 2024

Choose a reason for hiding this comment

gitguardian bot commented Jul 19, 2024 • edited Loading

⚠️ GitGuardian has uncovered 8 secrets following the scan of your pull request.

tabVersion left a comment

Choose a reason for hiding this comment

KeXiangWang commented Jul 20, 2024 • edited Loading

xiangjinwu Jul 19, 2024 •

edited

Loading

KeXiangWang Jul 22, 2024 •

edited

Loading

gitguardian bot commented Jul 19, 2024 •

edited

Loading

KeXiangWang commented Jul 20, 2024 •

edited

Loading