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

Support marshalling inferred updates of NULL values #71

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Jawshua
Copy link

@Jawshua Jawshua commented Dec 23, 2021

Within an UPDATE operation, downstream consumers currently cannot differentiate between an unchanged column and an update from NULL to not NULL . This is of course only relevant for tables that have set REPLICA IDENTITY FULL.

This is due to an interaction quirk that arises from the following details:

  • The test_decoding output omits NULL values from old-key.
  • The old key being omitted from the published output represents an unchanged column.

This PR attempts to address this by adding a new infer-updated-nulls setting, which causes the marshaller to inject a NULL value for each column missing from the decoded old-key.

Another way I'd considered solving this was by adding a setting to always output the old value, even if the value was unchanged, but this seems like the more efficient approach.

@stlava
Copy link
Member

stlava commented Dec 23, 2021

Hi @Jawshua, thank you for contributing. The change sounds reasonable. Could you also please add an integration test since this is fairly important change. Also, I'm wondering if the user should be able to specify how they want to differentiate nulls as. eg "null", empty string, or something else just in case the word "null" is valid in their use case.

@Jawshua
Copy link
Author

Jawshua commented Mar 5, 2022

Hello again. Apologies - only just getting back to this now after finally grabbing some free cycles.

I've added an integration test for the new flag behaviour, and tweaked some existing ones the tweak I made to the unchanged toast datum detection. I had to bump the test dependencies in order to get the suite running on an M1 MacBook - I hope that is ok.

Re: allowing users to provide their own values - this is already covered by the database with the quoted property. An entry of {"quoted":"true", "value":"null"} represents the string literal "null", whereas the entry {"quoted":"false", "value":"null"} represents the absent value marker NULL.

Anyone that is currently checking value for "null" and ignoring quoted is probably going to be running into erroneous behaviour already. Such a bug is currently present (and fixed in this MR) within the unchanged toast value detection - it currently doesn't differentiate between an unchanged toast value and the string literal "unchanged-toast-datum".

@Jawshua Jawshua marked this pull request as ready for review March 5, 2022 17:52
@@ -1,6 +1,6 @@
FROM postgres:10.13
FROM postgres:13.6
Copy link
Member

Choose a reason for hiding this comment

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

I ran the integration tests and I'm having a couple related issues because postgres takes a little longer to startup. First the docker build of the postgres container times out because bootstrap.sh times out waiting for postgres to start. Increasing the loop counter in bootstrap.sh helped. The second issue I had was when I ran the integration tests pg-bifrost errored creating the replication slot because postgres wasn't ready yet. I think the create function could use a retrier or loop.

   bifrost                           | time="2022-03-07T22:00:43Z" level=error msg="Unable to create replication slot 'pg_bifrost'" package=app
   bifrost                           | time="2022-03-07T22:00:43Z" level=fatal msg="failed to connect to `host=postgres user=replication database=postgres`: server error (FATAL: the database system is starting up (SQLSTATE 57P03))" package=app

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