On Thu, Feb 6, 2025 at 11:32 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Feb 05, 2025 at 03:28:04PM +0900, Michael Paquier wrote:
> > Hmm. I was reading through the patch and there is something that
> > clearly stands out IMO: the new compare_dumps(). It is in Utils.pm,
> > and it acts as a wrapper of `diff` with its formalized output format.
> > It is not really about dumps, but about file comparisons. This should
> > be renamed compare_files(), with internals adjusted as such, and
> > reused in all the existing tests. Good idea to use that in
> > 027_stream_regress.pl, actually. I'll go extract that first, to
> > reduce the presence of `diff` in the whole set of TAP tests.
>
> The result of this part is pretty neat, resulting in 0001 where it is
> possible to use the refactored routine as well in pg_combinebackup
> where there is a piece comparing dumps. There are three more tests
> with diff commands and assumptions of their own, that I've left out.
> This has the merit of unifying the output generated should any diffs
> show up, while removing a nice chunk from the main patch.
Sorry for replying late here. The refactored code in
002_compare_backups.pl has a potential to cause confusion even without
this refactoring. The differences in tablespace paths are adjusted in
compare_files() and not in the actual dump outputs. In case there's a
difference other than paths, diff between the dump outputs is reported
which will also show the differences in paths. That might mislead
developers in thinking that the differences in paths are also not
expected. Am I right?
I will address other comments soon, but the answer to this question
has some impact there.
--
Best Wishes,
Ashutosh Bapat
