Hi, On 2025-04-01 08:11:59 -0700, Noah Misch wrote: > On Mon, Mar 31, 2025 at 08:41:39PM -0400, Andres Freund wrote: > > updated version > > All non-write patches (1-7) are ready for commit, though I have some cosmetic > recommendations below. I've marked the commitfest entry Ready for Committer.
Thanks! I haven't yet pushed the changes, but will work on that in the afternoon. I plan to afterwards close the CF entry and will eventually create a new one for write support, although probably only rebasing onto https://postgr.es/m/stj36ea6yyhoxtqkhpieia2z4krnam7qyetc57rfezgk4zgapf%40gcnactj4z56m and addressing some of the locking issues. WRT the locking issues, I've been wondering whether we could make LWLockWaitForVar() work that purpose, but I doubt it's the right approach. Probably better to get rid of the LWLock*Var functions and go for the approach I had in v1, namely a version of LWLockAcquire() with a callback that gets called between LWLockQueueSelf() and PGSemaphoreLock(), which can cause the lock acquisition to abort. > This comment is a copy of the previous test's comment. While the comment is > not false, consider changing it to: > > # Check one read reporting multiple invalid blocks. > > + # Then test zeroing vio zero_damaged_pages > > s/vio/via/ > These make sense. > > +# Verify checksum handling when creating database from an invalid database. > > +# This also serves as a minimal check that cross-database IO is handled > > +# reasonably. > > To me, "invalid database" is a term of art from the message "cannot connect to > invalid database". Hence, I would change "invalid database" to "database w/ > invalid block" or similar, here and below. (Alternatively, just delete "from > an invalid database". It's clear from the context.) Yea, I agree, this is easy to misunderstand when stepping back. I went for "with an invalid block". > > + if (corrupt_checksum) > > + { > > + bool successfully_corrupted = 0; > > + > > + /* > > + * Any single modification of the checksum could just end up > > being > > + * valid again. To be sure > > + */ > > Unfinished sentence. Oops. See below. > That said, I'm not following why we'd need this loop. If this test code > were changing the input to the checksum, it's true that an input bit flip > might reach the same pd_checksum. The test case is changing pd_checksum, > not the input bits. We might be changing the input, due to the zero/corrupt_header options. Or we might be called on a page that is *already* corrupted. I did encounter that situation once while writing tests, where the tests only passed if I made the + 1 a + 2. Which was, uh, rather confusing and left me feel like I was cursed that day. > I don't see how changing pd_checksum could leave the > page still valid. There's only one valid pd_checksum value for a given > input page. I updated the comment to: /* * Any single modification of the checksum could just end up being * valid again, due to e.g. corrupt_header changing the data in a way * that'd result in the "corrupted" checksum, or the checksum already * being invalid. Retry in that, unlikely, case. */ > > + /* > > + * The underlying IO actually completed OK, and thus > > the "invalid" > > + * portion of the IOV actually contains valid data. > > That can hide > > + * a lot of problems, e.g. if we were to wrongly mark a > > buffer, > > + * that wasn't read according to the shortened-read, IO > > as valid, > > + * the contents would look valid and we might miss a > > bug. > > Minimally s/read, IO/read IO,/ but I'd edit a bit further: > > * a lot of problems, e.g. if we were to wrongly > mark-valid a > * buffer that wasn't read according to the > shortened-read IO, the > * contents would look valid and we might miss a bug. Adopted. > > Subject: [PATCH v2.15 05/18] md: Add comment & assert to buffer-zeroing path > > in md[start]readv() > > > The zero_damaged_pages path is incomplete, as as missing segments are not > > s/as as/as/ > > > For now, put an Assert(false) comments documenting this choice into > > mdreadv() > > s/comments/and comments/ > > > + * For PG 18, we are putting an Assert(false) > > in into > > + * mdreadv() (triggering failures in > > assertion-enabled builds, > > s/in into/in/ > > Subject: [PATCH v2.15 06/18] aio: comment polishing > > > + * - Partial reads need to be handle by the caller re-issuing IO for the > > + * unread blocks > > s/handle/handled/ All adopted. I'm sorry that you had to see so much of tiredness-enhanced dyslexia :(. Greetings, Andres Freund
