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

Add On-demand WAL Download to logicalfuncs #7960

Merged
merged 5 commits into from
Jun 12, 2024

Conversation

save-buffer
Copy link
Contributor

@save-buffer save-buffer commented Jun 4, 2024

We implemented on-demand WAL download for walsender, but other things that may want to read the WAL from safekeepers don't do that yet. This PR makes it do that by adding the same set of hooks to logicalfuncs.

Addresses #7959

Also relies on:
neondatabase/postgres#438
neondatabase/postgres#437
neondatabase/postgres#436

@save-buffer save-buffer requested review from a team as code owners June 4, 2024 18:12
@save-buffer save-buffer force-pushed the sasha_ondemand_wal_logicalfuncs branch from 3daf643 to 602475b Compare June 4, 2024 18:14
@skyzh skyzh removed their request for review June 4, 2024 18:50
Copy link

github-actions bot commented Jun 4, 2024

3204 tests run: 3062 passed, 0 failed, 142 skipped (full report)


Flaky tests (2)

Postgres 16

  • test_scrubber_tenant_snapshot[4]: release
  • test_storage_controller_smoke: debug

Code coverage* (full report)

  • functions: 31.6% (6625 of 20990 functions)
  • lines: 48.6% (51490 of 106045 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
93c79c3 at 2024-06-11T21:13:39.625Z :recycle:

@jcsp
Copy link
Collaborator

jcsp commented Jun 5, 2024

I'm a reviewer on this PR, but I have no idea what it's about. Please always write a description that fills out the PR template -- that's helpful even if a linked issue is provided, but in this case the link goes to a PR that has an empty description too.

@save-buffer
Copy link
Contributor Author

Sorry, I did have an issue that links to a slack thread but I didn't paste correctly. I've added a description

@jcsp jcsp removed their request for review June 5, 2024 16:56
@kelvich
Copy link
Contributor

kelvich commented Jun 6, 2024

@save-buffer that PR would benefit from a test that can reproduce the issue without a fix and would pass with fix :-)

@kelvich
Copy link
Contributor

kelvich commented Jun 6, 2024

Error message looks like that:

PG:2024-06-06 16:22:46.537 GMT ttid=1d880e086b24349586dc1ee10ebea34e/1921f4934124c56e8e937fefffe88bfa
sqlstate=XX000 [6370]
ERROR:  invalid magic number 0000 in log segment 00000001000000000000000F, offset 12206080

(posting here so that search on the error message could find this issue; originally i went in https://github.com/neondatabase/cloud/issues/12992).

@kelvich
Copy link
Contributor

kelvich commented Jun 6, 2024

Also double-checked that user faces invalid magic number 0000 in log segment when calling pg_logical_slot_peek_binary_changes, which is turn calls pg_logical_slot_get_changes_guts. So everything checks out. https://neonprod.grafana.net/goto/D-SdG_sIR?orgId=1

@save-buffer
Copy link
Contributor Author

Added a test

@save-buffer
Copy link
Contributor Author

Without callbacks:

psycopg2.errors.InternalError_: invalid record length at 0/2ADA620: wanted 24, got 0

With callbacks: test passes

@save-buffer save-buffer requested a review from kelvich June 10, 2024 20:12
@kelvich
Copy link
Contributor

kelvich commented Jun 10, 2024

@knizhnik and/or @ars pls take a look too as you are marked as code owners here

@save-buffer save-buffer force-pushed the sasha_ondemand_wal_logicalfuncs branch from 5e58980 to 7af9ae3 Compare June 10, 2024 20:28
Copy link
Contributor

@arssher arssher left a comment

Choose a reason for hiding this comment

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

Thanks! Let's also run pgindent on C part.

pgxn/neon/neon.c Outdated Show resolved Hide resolved
test_runner/regress/test_logical_replication.py Outdated Show resolved Hide resolved
@save-buffer save-buffer force-pushed the sasha_ondemand_wal_logicalfuncs branch from 3617612 to 93c79c3 Compare June 11, 2024 20:22
@save-buffer save-buffer enabled auto-merge (squash) June 11, 2024 21:00
@save-buffer save-buffer merged commit b7a0c2b into main Jun 12, 2024
64 checks passed
@save-buffer save-buffer deleted the sasha_ondemand_wal_logicalfuncs branch June 12, 2024 00:59
VladLazar pushed a commit that referenced this pull request Jun 12, 2024
We implemented on-demand WAL download for walsender, but other things
that may want to read the WAL from safekeepers don't do that yet. This
PR makes it do that by adding the same set of hooks to logicalfuncs.

Addresses #7959

Also relies on:
neondatabase/postgres#438
neondatabase/postgres#437
neondatabase/postgres#436
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.

5 participants