fix(batch-exports): Try to handle missing Postgres permissions #27437
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tomasfarias
reviewed
Jan 10, 2025
Comment on lines
683
to
687
| await internal_logger.awarning( | ||
| "Insufficient privileges to get table columns for table '%s.%s'; will assume all columns are present", | ||
| schema=inputs.schema, | ||
| table_name=inputs.table_name, | ||
| ) |
There was a problem hiding this comment.
We could probably make this a public log if we indicate users how to resolve this:
- Grant us permissions.
- Ensure table is updated to latest schema.
There was a problem hiding this comment.
yeah that's a good idea, thanks
tomasfarias
reviewed
Jan 10, 2025
Comment on lines
684
to
685
| schema=inputs.schema, | ||
| table_name=inputs.table_name, |
There was a problem hiding this comment.
Shouldn't these two be positional parameters if we are using the formatting %s?
There was a problem hiding this comment.
>>> logger.info("%s.%s", schema="test", table="test")
2025-01-10 14:33:10 [info ] %s.%s schema=test table=test
>>> logger.info("%s.%s", "test", "test")
2025-01-10 14:33:21 [info ] test.test
I assume we expect the second result.
There was a problem hiding this comment.
ah yes, sorry, my bad
tomasfarias
reviewed
Jan 10, 2025
| "Insufficient privileges to get table columns for table '%s.%s'; " | ||
| "will assume all columns are present. If this results in an error, please grant SELECT " | ||
| "permissions on this table or ensure the destination table is using the latest schema " | ||
| "as described in the docs.", |
There was a problem hiding this comment.
Could we link the docs here?
There was a problem hiding this comment.
good point, have added now 👍
tomasfarias
approved these changes
Jan 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
We try to fetch a list of columns for the destination table in case it doesn't contain all the columns present in the record batch. However this can sometimes fail with permissions errors if we don't have
SELECTpermissions on the destination table.Changes
Catch permissions errors and assume all columns are present. Note that this means existing batch exports could still fail during the merge if the destination table is missing columns present in the record batch.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Existing tests