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

PostgreSQL case sensitivity in listen #530

Merged
merged 14 commits into from
Feb 12, 2025
Merged

PostgreSQL case sensitivity in listen #530

merged 14 commits into from
Feb 12, 2025

Conversation

copiltembel
Copy link
Collaborator

@copiltembel copiltembel commented Feb 10, 2025

See #529

Copy link
Contributor

github-actions bot commented Feb 10, 2025

@copiltembel copiltembel changed the title Fix in pglite worker LISTEN; case sensitivity test in notify PostgreSQL case sensitivity in listen Feb 10, 2025
const caseSensitive1 = vi.fn()
await pg.listen('"tesT2"', caseSensitive1)
await pg.query(`NOTIFY "tesT2", 'paYloAd2'`)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to add a tests with spaces in the channel name:

    const quotedWithSpaces = vi.fn()
    await pg.listen('"Quoted Channel With Spaces"', quotedWithSpaces)
    await pg.query(`NOTIFY ""Quoted Channel With Spaces", 'payload1'`)


    const unquotedWithSpaces = vi.fn()
    await pg.listen('Unquoted Channel With Spaces', unquotedWithSpaces)
    // This one may need to throw?
    await pg.query(`NOTIFY "Unquoted Channel With Spaces", 'payload1'`)

Comment on lines +235 to +245
export function toPostgresName(input: string): string {
let output
if (input.startsWith('"') && input.endsWith('"')) {
// Postgres sensitive case
output = input.substring(1, input.length - 1)
} else {
// Postgres case insensitive - all to lower
output = input.toLowerCase()
}
return output
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I fear this breaks channel names with spaces.

You spotted in my original implementation I made a mistake, on the same thread I quoted the channel name, with the worker it was unquoted.

I think it's now apparent we need to decide between two options:

  1. Always quote the channel name - this forces it to be case sensitive, but means that channel names with spaces will always work: pg.listen('Channel With Spaces', ...)

  2. Never use quotes and user has to provide them to get the case sensitivity or use spaces:

  • pg.listen('"Channel With Spaces"', ...) works
  • pg.listen('Channel With Spaces', ...) throws an error

I'm not sure what the best option is.

For a reference point, Electric requires the user to provide the quotes to be case sensitive, and defaults to case insensitive.

Thinking about this now, I fear we may have a similar problem in the live.incrementalQuery api with the key column argument...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gonna take a moment to have a look at the possibilities. Will let you know.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A reason to throw if no quotes provided:

ERROR:  syntax error at or near "channel"
LINE 1: LISTEN my channel;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And I guess it's not only about spaces, but other characters as well - in an unquoted string.

ERROR:  syntax error at or near "&"
LINE 1: LISTEN my&channel;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, I think we should take the opportunity to consider what the correct pattern is here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

More issues surfaced: if the provided channel name causes the db engine to throw for whatever reason, there's no cleanup of the #notifyListeners made in the frontend. This might be the case in other parts of the code as well.

Copy link
Collaborator

@samwillis samwillis left a comment

Choose a reason for hiding this comment

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

Looks great! thanks @copiltembel

@copiltembel copiltembel merged commit 6bdd74e into main Feb 12, 2025
8 checks passed
@copiltembel copiltembel deleted the tudor/issue529 branch February 12, 2025 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants