On Thu, Nov 16, 2023 at 5:21 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> I meant code like this
>
> memcpy(&key.rlocator, rlocator, sizeof(RelFileLocator));
> key.forknum = forknum;
> entry = blockreftable_lookup(brtab->hash, key);
Ah, I hadn't thought about that. Another way of handling that might be
to add = {0} to the declaration of key. But I can do the initializer
thing too if you think it's better. I'm not sure if there's an
argument that the initializer might optimize better.
> An output pointer, you mean :-) (Should it be const?)
I'm bad at const, but that seems to work, so sure.
> When the return value is BACK_UP_FILE_FULLY, it's not clear what happens
> to these output values; we modify some, but why? Maybe they should be
> left alone? In that case, the "if size == 0" test should move a couple
> of lines up, in the brtentry == NULL block.
OK.
> BTW, you could do the qsort() after deciding to backup the file fully if
> more than 90% needs to be replaced.
OK.
> BTW, in sendDir() why do
> lookup_path = pstrdup(pathbuf + basepathlen + 1);
> when you could do
> lookup_path = pstrdup(tarfilename);
> ?
No reason, changed.
> > If we want to inject more underscores here, my vote is to go all the
> > way and make it per_wal_range_cb.
>
> +1
Will look into this.
> Yeah, I just think that endless stream of hex chars are hard to read,
> and I've found myself following digits in the screen with my fingers in
> order to parse file names. I guess you could say thousands separators
> for regular numbers aren't needed either, but we do have them for
> readability sake.
Sigh.
> I think a new section in chapter 30 "Reliability and the Write-Ahead
> Log" is warranted. It would explain the summarization process, what the
> summary files are used for, and the deletion mechanism. I can draft
> something if you want.
Sure, if you want to take a crack at it, that's great.
> It's not clear to me if WalSummarizerCtl->pending_lsn if fulfilling some
> purpose or it's just a leftover from prior development. I see it's only
> read in an assertion ... Maybe if we think this cross-check is
> important, it should be turned into an elog? Otherwise, I'd remove it.
I've been thinking about that. One thing I'm not quite sure about
though is introspection. Maybe there should be a function that shows
summarized_tli and summarized_lsn from WalSummarizerData, and maybe it
should expose pending_lsn too.
--
Robert Haas
EDB: http://www.enterprisedb.com