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

Register custom xlog reader callbacks for on-demand WAL download in StartupDecodingContext #9007

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

knizhnik
Copy link
Contributor

@knizhnik knizhnik commented Sep 16, 2024

Problem

See #8931
On-demand WAL download are not set in all cases where WAL is accessed by logical replication

Summary of changes

Set customer xlog reader handles in StartupDecodingContext

Related changes in Postgres modules:

neondatabase/postgres#495
neondatabase/postgres#496
neondatabase/postgres#497
neondatabase/postgres#498

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

Copy link

@orca-security-us orca-security-us bot left a comment

Choose a reason for hiding this comment

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

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Infrastructure as Code high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Vulnerabilities high 0   medium 0   low 0   info 0 View in Orca

Copy link

github-actions bot commented Sep 16, 2024

4994 tests run: 4830 passed, 0 failed, 164 skipped (full report)


Flaky tests (8)

Postgres 17

Postgres 16

Postgres 15

Code coverage* (full report)

  • functions: 31.8% (7415 of 23294 functions)
  • lines: 49.9% (59571 of 119380 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
f3d520e at 2024-09-16T18:52:49.065Z :recycle:

@hlinnaka
Copy link
Contributor

Some of the callers of CreateDecodingContext and CreateInitDecodingContext pass read_local_xlog_page, and some pass logical_read_xlog_page. Does NeonOnDemandXLogReaderRoutines work as a replacement for both?

CreateReplicationSlot() passes .segment_open = WalSndSegmentOpen. WalSndSegmentOpen has some code to deal with historic (postgres) timelines. I'm not sure if that code is reachable on Neon, but this makes us skip it.

I think I would be more comfortable if NeonOnDemandXLogReaderRoutines() was just a wrapper around the original routines, so that the original routines would still actually do the opening and reading of the file, after NeonOnDemandXLogReaderRoutines() has performed the download.

Needs tests. In particular, tests for the cases where we were not doing on-demand download without this PR.

@knizhnik knizhnik force-pushed the on_demand_wal_downoad_for_logical_replication branch from da082d1 to e81eb03 Compare September 16, 2024 12:13
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