Skip to content

Fix misleading error in batch for live migration#1764

Merged
makalaaneesh merged 7 commits intomainfrom
aneesh/live-migration-misleading-error
Oct 21, 2024
Merged

Fix misleading error in batch for live migration#1764
makalaaneesh merged 7 commits intomainfrom
aneesh/live-migration-misleading-error

Conversation

@makalaaneesh
Copy link
Copy Markdown
Collaborator

When using pgx SendBatch, there can be two types of errors thrown:

  1. Error while preparing the statement - this is preprocessing (parsing, preparinng statements, etc) that pgx will do before sending the batch. Examples - syntax error.
    No matter which statement in the batch has an issue, the error will be thrown on calling br.Exec() for the first time.
  2. Error while executing the statement - this is the actual execution of the statement, and the error comes from the DB.
    Examples - constraint violation, etc.
    In this case, we get the error on the appropriate br.Exec() call associated with the statement that failed.

Therefore, if error is thrown on the first br.Exec() call, it could be either of the above cases. Modified the logs to indicate this as part of this PR.

Reference - jackc/pgx#872
This ideally needs to be fixed in pgx library, and has been partially fixed in jackc/pgx#1441 (pgx v5)

// This ideally needs to be fixed in pgx library.
errorMsg := ""
if i == 0 {
errorMsg = fmt.Sprintf("error preparing statements for events in batch (%s) or when executing event with vsn(%d)", batch.ID(), batch.Events[i].Vsn)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is printing start and end of the batch enough here? or should we print the VSNs of all events in that batch.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, thought of this @sanyamsinghal , but I plan to add debug logs for every statement in a subsequent PR, which I thought would be more valuable than simply listing all the VSNs here.

Let me know if you think I should still add it here as well!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, i think we can add it here also, since it won't flood the logs, just the last line in case of failures.
We can use debug logs for more level of details(like logging particular events)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@makalaaneesh makalaaneesh merged commit 531a66f into main Oct 21, 2024
@makalaaneesh makalaaneesh deleted the aneesh/live-migration-misleading-error branch October 21, 2024 10:59
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