Re: Test to dump and restore objects left behind by regression

On Fri, Apr 26, 2024 at 06:38:22PM +0530, Ashutosh Bapat wrote:
> Some points for discussion:
> 1. The test still hardcodes the diffs between two dumps. Haven't found a
> better way to do it. I did consider removing the problematic objects from
> the regression database but thought against it since we would lose some
> coverage.
> 
> 2. The new code tests dump and restore of just the regression database and
> does not use pg_dumpall like pg_upgrade. Should it instead perform
> pg_dumpall? I decided against it since a. we are interested in dumping and
> restoring objects left behind by regression, b. I didn't find a way to
> provide the format option to pg_dumpall. The test could be enhanced to use
> different dump formats.
> 
> I have added it to the next commitfest.
> https://commitfest.postgresql.org/48/4956/

Ashutosh and I have discussed this patch a bit last week.  Here is a
short summary of my input, after I understood what is going on.
 
+	# We could avoid this by dumping the database loaded from original dump.
+	# But that would change the state of the objects as left behind by the
+	# regression.
+	my $expected_diff = " --
+ CREATE TABLE public.gtestxx_4 (
+-    b integer,
+-    a integer NOT NULL
++    a integer NOT NULL,
++    b integer
+ )
[...]
+	my ($stdout, $stderr) =
+		run_command([ 'diff', '-u', $dump4_file, $dump5_file]);
+	# Clear file names, line numbers from the diffs; those are not going to
+	# remain the same always. Also clear empty lines and normalize new line
+	# characters across platforms.
+	$stdout =~ s/^\@\@.*$//mg;
+	$stdout =~ s/^.*$dump4_file.*$//mg;
+	$stdout =~ s/^.*$dump5_file.*$//mg;
+	$stdout =~ s/^\s*\n//mg;
+	$stdout =~ s/\r\n/\n/g;
+	$expected_diff =~ s/\r\n/\n/g;
+	is($stdout, $expected_diff, 'old and new dumps match after dump and restore');
+}

I am not a fan of what this patch does, adding the knowledge related
to the dump filtering within 002_pg_upgrade.pl.  Please do not take
me wrong, I am not against the idea of adding that within this
pg_upgrade test to save from one full cycle of `make check` to check
the consistency of the dump.  My issue is that this logic should be
externalized, and it should be in fewer lines of code.

For the externalization part, Ashutosh and I considered a few ideas,
but one that we found tempting is to create a small .pm, say named
AdjustDump.pm.  This would share some rules with the existing
AdjustUpgrade.pm, which would be fine IMO even if there is a small
overlap, documenting the dependency between each module.  That makes
the integration with the buildfarm much simpler by not creating more
dependencies with the modules shared between core and the buildfarm
code.  For the "shorter" part, one idea that I had is to apply to the
dump a regexp that wipes out the column definitions within the
parenthesis, keeping around the CREATE TABLE and any other attributes
not impacted by the reordering.  All that should be documented in the
module, of course.

Another thing would be to improve the backend so as we are able to
a better support for physical column ordering, which would, I assume
(and correct me if I'm wrong!), prevent the reordering of the
attributes like in this inheritance case.  But that would not address
the case of dumps taken from older versions with a new version of
pg_dump, which is something that may be interesting to have more tests
for in the long-term.  Overall a module sounds like a better solution.
--
Michael