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

Fixes online migration with composite primary key #879

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hanefi
Copy link
Contributor

@hanefi hanefi commented Sep 2, 2024

Refactor code to skip executing empty UPDATE statements

The logical_message_metadata_should_skip_statement function has been added to check if the logical message metadata should be skipped. This is necessary due to a bug in the wall2json output plugin that creates an UPDATE statement with an empty SET clause and places the actual SET clause in the WHERE clause. The function checks for this specific pattern and returns true if found, indicating that the statement should be skipped during execution.

In passing fixes a double free bug.

Fixes: #750

Refactor code to skip executing empty UPDATE statements

The logical_message_metadata_should_skip_statement function has been
added to check if the logical message metadata should be skipped. This
is necessary due to a bug in the wall2json output plugin that creates an
UPDATE statement with an empty SET clause and places the actual SET
clause in the WHERE clause. The function checks for this specific
pattern and returns true if found, indicating that the statement should
be skipped during execution.

In passing fixes a double free bug.

Fixes: dimitri#750
@dimitri
Copy link
Owner

dimitri commented Sep 2, 2024

I am confused here:

  1. In Migration failed while update for schema containing composite primary key #750 the reporter is using test_decoding, not wal2json.
  2. The comment here is mentioning a bug in wal2json: can we link to the wal2json issue?
  3. The PR does not address why it is safe to skip the UPDATE statement when we fail to format it -- one way to show that it's safe would be for the unit test to also run a data compare after the end of the CDC apply.

Also the PR uses the wrong spelling wall2json in multiple places.

From issue #750:

15:22:30.588 8683 ERROR  Failed to parse decoding message for UPDATE on table public.test_002: SET clause columns not found
15:22:30.589 8683 ERROR  Failed to parse UPDATE new-tuple columns for logical message table public.test_002: UPDATE: id[integer]:1 name[text]:'postgres'
15:22:30.589 8683 ERROR  Failed to parse test_decoding UPDATE message: table public.test_002: UPDATE: id[integer]:1 name[text]:'postgres'
15:22:30.589 8683 ERROR  Failed to parse test_decoding message, see above for details

@arajkumar
Copy link
Contributor

arajkumar commented Sep 24, 2024

@hanefi Looks like this PR and #883 addresses same issue?

As mentioned in the PR #883, this is not a problem in the plugin, instead the logic from ld_transform which skips columns from SET clause having same values as in WHERE clause leading to removal of all SET clause items.

@@ -2066,7 +2066,6 @@ pgsql_sync_pipeline(PGSQL *pgsql)
if (!is_response_ok(res))
{
(void) pgcopy_log_error(pgsql, res, "Failed to receive pipeline sync");
PQclear(res);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we fix the double free in a separate PR?

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.

Migration failed while update for schema containing composite primary key
4 participants