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

Don't create versions that would be identical to previous version #35

Conversation

nathanielobrown
Copy link
Contributor

I would find it much more useful if the "Ignore updates without actual change" would apply to no actual change to the version history, rather than no change to the source table. Why?

  • Then you can have columns in the source table that create no new versions if modified by not including those columns in the history table
  • Why would you want duplicate, identical rows in the history table anyway?

This is my first time editing plpgsql and it took me a while so I'm kinda opening this for feedback and to start a discussion. Even if this shouldn't be merged (it's backwards incompatible, after all) I would really appreciate any feedback as I'm going to go ahead and use it myself 😬. Maybe this should be controlled by a different option, maybe there's a better way to do this check... just throwing this out there, not saying it should be merged 🙂

@simoneb
Copy link
Member

simoneb commented Oct 11, 2022

@nathanielobrown thanks for your contribution!

@simoneb simoneb merged commit 9255241 into nearform:master Oct 11, 2022
@nathanielobrown
Copy link
Contributor Author

@simoneb, I'm happy you like it! Maybe we should update the README so it no longer says:

By default this extension creates a record in the history table for every update that occurs in the versioned table, regardless of any change actually happening.

@simoneb
Copy link
Member

simoneb commented Oct 11, 2022

@simoneb, I'm happy you like it! Maybe we should update the README so it no longer says:

By default this extension creates a record in the history table for every update that occurs in the versioned table, regardless of any change actually happening.

@washingtonsoares can you take care of this please?

@washingtonsoares
Copy link
Contributor

@simoneb, I'm happy you like it! Maybe we should update the README so it no longer says:

By default this extension creates a record in the history table for every update that occurs in the versioned table, regardless of any change actually happening.

@nathanielobrown Actually I think this section still makes sense. Because even after the change you proposed, by default this extension will keep creating a new record in the history with each update regardless of whether there was in fact a change, unless we passed the fourth parameter to disable it (I believe that this PR will only work if we are dealing with this fourth parameter, right?)

@nathanielobrown
Copy link
Contributor Author

@washingtonsoares, you are right, my bad. I quoted the wrong sentence from the this section. I think this sentence should be removed or modified:

It's worth noting that the actual change is checked only in the source table, so if the history table only has a subset of the columns this extension will still add a new record to history even if the changed column is not present in the history table.

You are right, you must pass the fourth parameter as TRUE for this to be active

@paolochiodi
Copy link
Member

@simoneb I'm late to the party, but I think that inclusion of this change in the latest release should have triggered a major as this change is not backward compatible.

@simoneb
Copy link
Member

simoneb commented Oct 12, 2022

We have discussed this extensively and while it may be true that this should have been considered as a breaking change, for most practical purposes the impact on existing users is extremely limited because it addresses a very specific scenario, that of the source and history tables having a different schema. Nevertheless, because this versioning is quite loose (i.e there's no automatic dependency upgrade or anything like that), we didn't think too much about semver. Thanks for bringing it up though

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.

4 participants