Message-ID: <937634ae-e562-690a-8a5e-a964ba39f188@pgmasters.net>
Date: Thu, 19 Oct 2023 15:18:20 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.15.1
Subject: Re: trying again to get incremental backup
To: Robert Haas <robertmhaas@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>
Cc: Andres Freund <andres@anarazel.de>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
References: <CA+TgmoYOYZfMCyOXFyC-P+-mdrZqm5pP2N7S-r0z3_402h9rsA@mail.gmail.com>
 <20230614194717.jyuw3okxup4cvtbt@awork3.anarazel.de>
 <CA+TgmoaF-TnXK9D5m3m1pTj6p5u0DiyzKuPZM1pqPGKzXhQw1w@mail.gmail.com>
 <20230614204056.bhjq6hozxkjtbz7s@awork3.anarazel.de>
 <CA+TgmoaTZPKzcTW0pf30n+_uQtrEKRWB8qoOgkmnVh+QK2Wn_A@mail.gmail.com>
 <20230619155151.grygznxxnazgu6qm@awork3.anarazel.de>
 <CA+TgmoZoHNqx=zconLLbV+8=gNhv7g5ocXd-Mee97wvTkKc-6A@mail.gmail.com>
 <CAFiTN-sx4JS-2tKe-Vv8otWHAp3MaSTt678a7d4EdaSzBJgpBg@mail.gmail.com>
 <CA+Tgmoa_Gq2Qc9dh-WiZEUyF-nk3XULuQR-zFY2vOGNWRCZjkQ@mail.gmail.com>
 <CA+TgmoYw=8CnKEPQX9qk2estSTCaB7OGcnVM8oa-hAePG2O4Yw@mail.gmail.com>
 <CA+TgmoZNVeBzoqDL8xvr-nkaepq815jtDR4nJzPew7=3iEuM1g@mail.gmail.com>
Content-Language: en-US
From: David Steele <david@pgmasters.net>
In-Reply-To: <CA+TgmoZNVeBzoqDL8xvr-nkaepq815jtDR4nJzPew7=3iEuM1g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/937634ae-e562-690a-8a5e-a964ba39f188%40pgmasters.net>
Precedence: bulk

On 10/19/23 12:05, Robert Haas wrote:
> On Wed, Oct 4, 2023 at 4:08 PM Robert Haas <robertmhaas@gmail.com> wrote:
>> Clearly there's a good amount of stuff to sort out here, but we've
>> still got quite a bit of time left before feature freeze so I'd like
>> to have a go at it. Please let me know your thoughts, if you have any.
> 
> Apparently, nobody has any thoughts, but here's an updated patch set
> anyway. The main change, other than rebasing, is that I did a bunch
> more documentation work on the main patch (0005). I'm much happier
> with it now, although I expect it may need more adjustments here and
> there as outstanding design questions get settled.
> 
> After some thought, I think that it should be fine to commit 0001 and
> 0002 as independent refactoring patches, and I plan to go ahead and do
> that pretty soon unless somebody objects.

0001 looks pretty good to me. The only thing I find a little troublesome 
is the repeated construction of file names with/without segment numbers 
in ResetUnloggedRelationsInDbspaceDir(), .e.g.:

+			if (segno == 0)
+				snprintf(dstpath, sizeof(dstpath), "%s/%u",
+						 dbspacedirname, relNumber);
+			else
+				snprintf(dstpath, sizeof(dstpath), "%s/%u.%u",
+						 dbspacedirname, relNumber, segno);


If this happened three times I'd definitely want a helper function, but 
even with two I think it would be a bit nicer.

0002 is definitely a good idea. FWIW pgBackRest does this conversion but 
also errors if it does not succeed. We have never seen a report of this 
error happening in the wild, so I think it must be pretty rare if it 
does happen.

Regards,
-David