Re: pgsql: Don't trust unvalidated xl_tot_len. - Mailing list pgsql-hackers

Hi,

On 2023-11-12 07:17:02 +1300, Thomas Munro wrote:
> On Sun, Nov 12, 2023 at 12:53 AM Christoph Berg <myon@debian.org> wrote:
> > Re: Thomas Munro
> > > In the 13 branch we see that's in the new scan_server_header()
> > > subroutine where it tries to open the header, after asking pg_config
> > > --includedir-server where it lives.  Hmm...
> >
> > It's no ok to use pg_config at test time since `make install` might
> > not have been called yet:
> >
> > https://www.postgresql.org/message-id/2023925.1644591595@sss.pgh.pa.us
> > https://www.postgresql.org/message-id/YqkV/hoi2SX91it8@paquier.xyz
> 
> [CC'ing Michael who was involved in that analysis and who also wrote
> those bits of this commit]
> 
> We don't have an installation into the final --prefix, but we have
> tmp_install, surely?

Yea, that should work and does work locally.  I guess it'd fail though, if you
somehow ran the tests with NO_TEMP_INSTALL=1 or such.


> And the tests are run with PATH set to point to
> tmp_install's bin directory.  It looks like it did actually find a
> pg_config executable because otherwise we'd have hit die "could not
> execute pg_config" and failed sooner.  So now I'm wondering if the
> pg_config it found gives the wrong answer for --includedir-server,
> because of Debian's local patches that insert a major version into
> some paths.

From the second link above, the problem might more be that debian pg_config is
patched to not be relocatable (huh?) - so it'd return an absolute path into
the final non-DESTDIR path. Which would fail, because the file isn't installed
yet.

If that's the case, does that also mean that all the tests that are
selectively enabled using check_pg_config() don't work in the debian context?


I assume the reason to not use the source-tree access/xlog_internal.h here was
just that it's nontrivial to find the top of the source tree form tap tests?
It's not hard to make it available... And as we already pass in top_builddir,
it doesn't actually measurably further weaken usability of the tap framework
in the pgxs context.


> For example, maybe it's trying to look for access/xlog_internal.h under
> tmp_install/usr/include/postgresql/server when it's really under
> tmp_install/usr/include/postgresql/13/server, or vice versa.  But then why
> does that only happen on the salsa build, not on the apt.postgresql.org one?


Christoph, can you apply a patch to emit a bit more information in that
environment?

diff --git i/src/test/perl/PostgreSQL/Test/Utils.pm w/src/test/perl/PostgreSQL/Test/Utils.pm
index cd86897580c..3c588a41755 100644
--- i/src/test/perl/PostgreSQL/Test/Utils.pm
+++ w/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -722,7 +722,8 @@ sub scan_server_header
     chomp($stdout);
     $stdout =~ s/\r$//;
 
-    open my $header_h, '<', "$stdout/$header_path" or die "$!";
+    my $fname = "$stdout/$header_path";
+    open my $header_h, '<', "$fname" or die "could not open $fname: $!";
 
     my @match = undef;
     while (<$header_h>)

would be helpful.

I guess we should also apply something like that to our tree - printing a
stringified errno without even a path/filename isn't very useful.

Greetings,

Andres Freund