Skip to content

Avoid invoking PublicationManager from SchemaReconciler when there are no diverged relations - #3108

Merged
alco merged 2 commits into
mainfrom
alco/noop-publication-manager-update
Sep 22, 2025
Merged

alco merged 2 commits into
mainfrom
alco/noop-publication-manager-update

Conversation

@alco

@alco alco commented Sep 8, 2025

Copy link
Copy Markdown
Contributor

Just getting rid of a no-op function invocation that pollutes the log with periodic log entries.

@alco
alco requested a review from msfstef September 8, 2025 11:05
@codecov

codecov Bot commented Sep 8, 2025 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.87879% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.06%. Comparing base (a3cef79) to head (a29029f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...vice/lib/electric/replication/schema_reconciler.ex 70.00% 3 Missing ⚠️
...ce/lib/electric/replication/publication_manager.ex 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3108      +/-   ##
==========================================
- Coverage   77.53%   77.06%   -0.47%     
==========================================
  Files         169      172       +3     
  Lines        8931     9145     +214     
  Branches      242      296      +54     
==========================================
+ Hits         6925     7048     +123     
- Misses       2004     2095      +91     
  Partials        2        2              
Flag Coverage Δ
elixir 75.73% <87.87%> (+0.04%) ⬆️
elixir-client 74.40% <ø> (+0.53%) ⬆️
packages/experimental 87.73% <ø> (ø)
packages/react-hooks 86.48% <ø> (ø)
packages/typescript-client 93.49% <ø> (ø)
packages/y-electric 55.12% <ø> (?)
postgres-140000 75.62% <87.87%> (-0.07%) ⬇️
postgres-150000 75.75% <87.87%> (+0.18%) ⬆️
postgres-170000 75.77% <87.87%> (-0.04%) ⬇️
sync-service 75.87% <87.87%> (-0.02%) ⬇️
typescript 85.79% <ø> (-6.26%) ⬇️
unit-tests 77.06% <87.87%> (-0.47%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@netlify

netlify Bot commented Sep 9, 2025 •

Copy link
Copy Markdown

✅ Deploy Preview for electric-next ready!

Name Link
🔨 Latest commit a29029f
🔍 Latest deploy log https://app.netlify.com/projects/electric-next/deploys/68d1390d6574f200086d5a61
😎 Deploy Preview https://deploy-preview-3108--electric-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@alco

alco commented Sep 9, 2025

Copy link
Copy Markdown
Contributor Author

As the failing integration test has shown, the change in the 1st commit alone wasn't enough. I have pushed another change that moves the responsibility of refreshing the publication with forced?=true every minute into the publication manager itself, since it's not the responsibility of SchemaReconciler.

As for the need to fetch the publication state from the database once every minute, it could be debated. One thing it does for us is repair any meddling with the publication in Postgres outside of Electric. For example, if the admin adds a table to the publication that Electric can't read from (or can't update the replica identity off), the periodic refresh_publication call will remove that table from the publication since it's not used by any shape.

@alco
alco force-pushed the alco/noop-publication-manager-update branch from bce197b to 5491d2c Compare September 9, 2025 21:14
@alco
alco changed the base branch from main to msfstef/simplify-publication-manager September 9, 2025 21:14
@alco

alco commented Sep 9, 2025

Copy link
Copy Markdown
Contributor Author

Rebased the branch on top of #3119

Base automatically changed from msfstef/simplify-publication-manager to main September 11, 2025 10:36
So far SchemaReconciler has been responsible both for updating the
publication when it detected schema changes as well as when there are no
schema changes. This latter periodic update is useful to keep the
publication up-to-date with the filters but it's not really the
responsibility of SchemaReconciler.
@alco
alco force-pushed the alco/noop-publication-manager-update branch from 5491d2c to a29029f Compare September 22, 2025 11:54
@alco

alco commented Sep 22, 2025

Copy link
Copy Markdown
Contributor Author

Rebased once more on top of #3152.

@alco
alco merged commit 97752f9 into main Sep 22, 2025
45 of 47 checks passed
@alco
alco deleted the alco/noop-publication-manager-update branch September 22, 2025 12:06
msfstef added a commit that referenced this pull request Sep 25, 2025
Fixes #3167

We started seeing this error with an update made on 22nd of September,
and until then one of the relevant changes included is this:
#3108

Since @alco has introduced periodic forced publication updates anyway,
and upon reconciling the schema we also trigger dropping relevant shapes
(which would remove them from the publication on cleanup), we no longer
need to refresh the publication manually IMO - not much to be gained
other than unnecessary dependencies between our processes.

It's still worth investigating why we're seeing these timeouts (but was
harder to debug because we were missing sentry metadata, see
#3166) - but ultimately we
should simplify things.

1. Schema gets reconciled
2. Shapes using diverged/stale relations get dropped
3. Shape cleanup will involve removing them from the publication which
should cause a publication update (as all relevant shapes for a given
relation will be removed)
4. Even if (3) doesn't occur, which it will, we now periodically refresh
the publication anyway

In fact, I would go even further to say that since we're periodically
refreshing the publication and thus detecting missing relations there,
we should simply add an inspector cleanup and get rid of the schema
reconciler in its entirety - what do you think @alco @icehaunter ?
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