Skip to content

fix(postgresql): Make pg error clear with table name#5733

Closed
yevgenypats wants to merge 2 commits intomainfrom
fix/pg_make_batch_err_clear
Closed

fix(postgresql): Make pg error clear with table name#5733
yevgenypats wants to merge 2 commits intomainfrom
fix/pg_make_batch_err_clear

Conversation

@yevgenypats
Copy link
Copy Markdown
Contributor

Sometimes we can have bugs in our type systems and/or destination which we need to find and fix as fast as we can. In PostgreSQL case we are using batch which makes it impossible to know which table and/or query caused it understand what is the bug. This finds the violating query/table before exiting and prints it so users can report us the issue.

@yevgenypats yevgenypats requested review from a team, bbernays and erezrokah and removed request for a team and bbernays December 17, 2022 20:09
Copy link
Copy Markdown
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @yevgenypats can you explain how getting the error from the pgError instance doesn't work? We already log it in

c.logger.Error().Err(pgErr).Str("table", pgErr.TableName).Msg("failed to execute batch with pgerror")

Also do we want to stop the sync on a batch failure? Before we kept syncing so at least you got some data. Stopping and reporting the offending table does makes sense as users can chose to skip the table and re-sync.

// maybe in the future we can provide a flag to switch batching mechanism.
func (c *Client) findErrorInBatch(ctx context.Context, items []*batchItem) error {
for _, item := range items {
_, err := c.conn.Exec(ctx, item.sql, item.arguments...)
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.

Are queries guaranteed to behave the same the second time we execute them? I'm concerned this can generated unrelated errors

}
atomic.AddUint64(&c.metrics.Errors, 1)
c.logger.Error().Err(pgErr).Str("table", pgErr.TableName).Msg("failed to execute batch with pgerror")
return fmt.Errorf("failed to execute batch and was unable to pinpoint table: %w", batchErr)
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.

Doesn't this mean everything was successfully inserted? Maybe log it and not return an error

@yevgenypats
Copy link
Copy Markdown
Contributor Author

yevgenypats commented Dec 18, 2022

@erezrokah Good catch! Seems you are correct. apparently there are two types of errors. One that is happening on Postgres side and for that we should usually have the TableName. Anotherone is happening on the client side while preparing the "Prepared statements" . I opened a a fix for pgx - jackc/pgx#1441 .

In the meanwhile i'll close this one and open PR to update pg to pgx v5 as the fix I opened is for v5 (already have a PR in place).

Follow-up PR here: #5757

kodiakhq bot pushed a commit that referenced this pull request Dec 18, 2022
This is a follow-up to #5733 (see discussion there). and needed so we can incorporate this nice bit: jackc/pgx#1441

The jackc/pgx#1441 is not a pre-requirement for this to go through. even better we should prob ship this first and see that `v5` works for us and for our users and if not we can do the same fix for `v4` (we will just have to fork it as I don't think `v4` accepts anymore contributions)
@hermanschaaf hermanschaaf deleted the fix/pg_make_batch_err_clear branch November 21, 2023 08:48
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.

3 participants