Thread: Using LibPq in TAP tests via FFI
Over at [1] Andres expressed enthusiasm for enabling TAP tests to call LibPQ directly via FFI, and there was some support from others as well. Attached is a very rough POC for just that.There are two perl modules, one which wraps libpq (or almost all of it) in perl, and another which uses that module to create a session object that can be used to run SQL. Also in the patch is a modification of one TAP test (arbitrarily chosen as src/bin/pg_amcheck/t/004_verify_heapam.p) to use the new interface, so it doesn't use psql at all. There's a bunch of work to do here, but for a morning's work it's not too bad :-) Luckily I had most of the first file already to hand. Next I plan to look at some of the recovery tests and other uses of background_psql, which might be more challenging,a dn require extension of the session API. Also there's a lot of error checking and documentation that need to be added. cheers andrew [1] https://postgr.es/m/20240612152812.ixz3eiz2p475gose@awork3.anarazel.de -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2024-06-14 Fr 11:09, Andrew Dunstan wrote: > Over at [1] Andres expressed enthusiasm for enabling TAP tests to call > LibPQ directly via FFI, and there was some support from others as > well. Attached is a very rough POC for just that.There are two perl > modules, one which wraps libpq (or almost all of it) in perl, and > another which uses that module to create a session object that can be > used to run SQL. Also in the patch is a modification of one TAP test > (arbitrarily chosen as src/bin/pg_amcheck/t/004_verify_heapam.p) to > use the new interface, so it doesn't use psql at all. > > There's a bunch of work to do here, but for a morning's work it's not > too bad :-) Luckily I had most of the first file already to hand. > > Next I plan to look at some of the recovery tests and other uses of > background_psql, which might be more challenging,a dn require > extension of the session API. Also there's a lot of error checking and > documentation that need to be added. > > > cheers > > > andrew > > > [1] > https://postgr.es/m/20240612152812.ixz3eiz2p475gose@awork3.anarazel.de > > And here's the patch cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Attachment
On Fri, Jun 14, 2024 at 11:11 AM Andrew Dunstan <andrew@dunslane.net> wrote: > And here's the patch I haven't reviewed the patch, but a big +1 for the idea. Not only this might cut down on the resource costs of running the tests in CI, as Andres has pointed out a few times, but it also could lead to much nicer user interfaces. For instance, right now, we have a number of TAP tests that are parsing psql output to recover the values returned by queries. Perhaps eventually - or maybe already, again I haven't looked at the code - you'll be able to do something like $resultset->[0][0] to pull the first column out of the first row. That kind of thing could substantially improve the readability and maintainability of some of our tests. -- Robert Haas EDB: http://www.enterprisedb.com
Hi, On June 14, 2024 8:09:43 AM PDT, Andrew Dunstan <andrew@dunslane.net> wrote: >Over at [1] Andres expressed enthusiasm for enabling TAP tests to call LibPQ directly via FFI, and there was some supportfrom others as well. Attached is a very rough POC for just that.There are two perl modules, one which wraps libpq(or almost all of it) in perl, and another which uses that module to create a session object that can be used to runSQL. Also in the patch is a modification of one TAP test (arbitrarily chosen as src/bin/pg_amcheck/t/004_verify_heapam.p)to use the new interface, so it doesn't use psql at all. > >There's a bunch of work to do here, but for a morning's work it's not too bad :-) Luckily I had most of the first file alreadyto hand. Yay! >Next I plan to look at some of the recovery tests and other uses of background_psql, which might be more challenging,a dnrequire extension of the session API. Also there's a lot of error checking and documentation that need to be added. I'd suggest trying to convert the various looping constructs first, they're responsible for a large number of spawned shells.And I vaguely recall that there were none/very few that depend on actually being run via psql. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Hi, On 2024-06-14 11:11:38 -0400, Andrew Dunstan wrote: > On 2024-06-14 Fr 11:09, Andrew Dunstan wrote: > > Over at [1] Andres expressed enthusiasm for enabling TAP tests to call > > LibPQ directly via FFI, and there was some support from others as well. > > Attached is a very rough POC for just that.There are two perl modules, > > one which wraps libpq (or almost all of it) in perl, and another which > > uses that module to create a session object that can be used to run SQL. What are your current thoughts about a fallback for this? It seems possible to implement the session module ontop of BackgroundPsql.pm, if necessary. But I suspect we'll eventually get to a point where that gets less and less convenient. How much of a dependency is FFI::Platypus, compared to requiring perl headers to be installed? In case FFI::Platypus is a complicted dependency, a small XS wrapper could be an alternative. Greetings, Andres Freund
On Sat, Jun 15, 2024 at 3:33 AM Robert Haas <robertmhaas@gmail.com> wrote: > I haven't reviewed the patch, but a big +1 for the idea. Not only this > might cut down on the resource costs of running the tests in CI, as It would be good to keep some context between the threads here. For the archives' sake, here is where the potential savings were reported, and this and other ideas were discussed: https://www.postgresql.org/message-id/flat/CA%2BhUKGJoEO33K%3DZynsH%3DxkiEyfBMZjOoqBK%2BgouBdTGW2-woig%40mail.gmail.com
On Fri, Jun 14, 2024 at 11:40 AM Andres Freund <andres@anarazel.de> wrote:
Hi,
On June 14, 2024 8:09:43 AM PDT, Andrew Dunstan <andrew@dunslane.net> wrote:
>Over at [1] Andres expressed enthusiasm for enabling TAP tests to call LibPQ directly via FFI, and there was some support from others as well. Attached is a very rough POC for just that.There are two perl modules, one which wraps libpq (or almost all of it) in perl, and another which uses that module to create a session object that can be used to run SQL. Also in the patch is a modification of one TAP test (arbitrarily chosen as src/bin/pg_amcheck/t/004_verify_heapam.p) to use the new interface, so it doesn't use psql at all.
>
>There's a bunch of work to do here, but for a morning's work it's not too bad :-) Luckily I had most of the first file already to hand.
Yay!
>Next I plan to look at some of the recovery tests and other uses of background_psql, which might be more challenging,a dn require extension of the session API. Also there's a lot of error checking and documentation that need to be added.
I'd suggest trying to convert the various looping constructs first, they're responsible for a large number of spawned shells. And I vaguely recall that there were none/very few that depend on actually being run via psql.
Yeah, here's a new version with a few more scripts modified, and also poll_query_until() adjusted. That seems to be the biggest looping construct.
The biggest remaining unadjusted script users of psql are all in the subscription and recovery tests.
cheers
andrew
Attachment
On Fri, Jun 14, 2024 at 7:42 PM Thomas Munro <thomas.munro@gmail.com> wrote:
On Sat, Jun 15, 2024 at 3:33 AM Robert Haas <robertmhaas@gmail.com> wrote:
> I haven't reviewed the patch, but a big +1 for the idea. Not only this
> might cut down on the resource costs of running the tests in CI, as
It would be good to keep some context between the threads here. For
the archives' sake, here is where the potential savings were reported,
and this and other ideas were discussed:
https://www.postgresql.org/message-id/flat/CA%2BhUKGJoEO33K%3DZynsH%3DxkiEyfBMZjOoqBK%2BgouBdTGW2-woig%40mail.gmail.com
Yeah thanks for adding that context.
cheers
andrew
On Fri, Jun 14, 2024 at 12:25 PM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2024-06-14 11:11:38 -0400, Andrew Dunstan wrote:
> On 2024-06-14 Fr 11:09, Andrew Dunstan wrote:
> > Over at [1] Andres expressed enthusiasm for enabling TAP tests to call
> > LibPQ directly via FFI, and there was some support from others as well.
> > Attached is a very rough POC for just that.There are two perl modules,
> > one which wraps libpq (or almost all of it) in perl, and another which
> > uses that module to create a session object that can be used to run SQL.
What are your current thoughts about a fallback for this? It seems possible
to implement the session module ontop of BackgroundPsql.pm, if necessary. But
I suspect we'll eventually get to a point where that gets less and less
convenient.
I guess it's a question of how widely available FFI::Platypus is. I know it's available pretty much out of the box on Strawberry Perl and Msys2' ucrt perl. It works fine on my Ubuntu ARM64 instance. On my Mac I had to install it via cpan, but that worked fine. For the moment CYgwin has me beat, but I believe it's possible to make it work - at least the docs suggest it is. Not sure about other platforms.
I agree with you that falling back on BackgroundPsql is not a terribly satisfactory solution.
How much of a dependency is FFI::Platypus, compared to requiring perl headers
to be installed? In case FFI::Platypus is a complicted dependency, a small XS
wrapper could be an alternative.
Sure we could look at it. I might need to enlist some assistance there :-). Using FFI is really nice because it does so much of the work for you.
cheers
andrew
Hi, On 2024-06-16 17:43:05 -0400, Andrew Dunstan wrote: > On Fri, Jun 14, 2024 at 12:25 PM Andres Freund <andres@anarazel.de> wrote: > I guess it's a question of how widely available FFI::Platypus is. I know > it's available pretty much out of the box on Strawberry Perl and Msys2' > ucrt perl. FWIW I hacked a bit on CI, trying to make it work. Took a bit, partially because CI uses an older strawberry perl without FFI::Platypus. And FFI::Platypus didn't build with that. Updating that to 5.38 causes some complaints about LANG that I haven't hunted down, just avoided by unsetting LANG. As-is your patch didn't work, because it has "systempath => []", which caused libpq to not load, because it depended on things in the system path... What's the reason for that? After commenting that out, all but one tests passed: [20:21:31.137] ------------------------------------- 8< ------------------------------------- [20:21:31.137] stderr: [20:21:31.137] # Failed test 'psql connect success' [20:21:31.137] # at C:/cirrus/src/test/recovery/t/041_checkpoint_at_promote.pl line 161. [20:21:31.137] # got: '2' [20:21:31.137] # expected: '0' [20:21:31.137] # Failed test 'psql select 1' [20:21:31.137] # at C:/cirrus/src/test/recovery/t/041_checkpoint_at_promote.pl line 162. [20:21:31.137] # got: '' [20:21:31.137] # expected: '1' [20:21:31.137] # Looks like you failed 2 tests of 6. [20:21:31.137] [20:21:31.137] (test program exited with status code 2) [20:21:31.137] ------------------------------------------------------------------------------ [20:21:31.137] Due to concurrency and run-to-run variance I wouldn't bet too much on this, but the modified tests do have improved test times: before: [19:40:47.468] 135/296 postgresql:pg_amcheck / pg_amcheck/004_verify_heapam OK 7.70s 32 subtests passed [19:43:40.853] 232/296 postgresql:amcheck / amcheck/001_verify_heapam OK 36.50s 272 subtests passed after: [20:22:55.495] 133/296 postgresql:pg_amcheck / pg_amcheck/004_verify_heapam OK 4.60s 32 subtests passed [20:25:13.641] 212/296 postgresql:amcheck / amcheck/001_verify_heapam OK 4.87s 272 subtests passed I looked at a few past runs and there never were instances of amcheck/001_verify_heapam that were even close to as fast as this. The overall tests time did improve some, but that is hard to weigh due to the test failure. > I agree with you that falling back on BackgroundPsql is not a terribly > satisfactory solution. I'm somewhat doubtful we'll just agree on making FFI::Platypus a hard dependency, but if we agree to do so... Greetings, Andres Freund
On Mon, Jun 17, 2024 at 10:38 AM Andres Freund <andres@anarazel.de> wrote: > before: > > [19:40:47.468] 135/296 postgresql:pg_amcheck / pg_amcheck/004_verify_heapam OK 7.70s 32 subtests passed > [19:43:40.853] 232/296 postgresql:amcheck / amcheck/001_verify_heapam OK 36.50s 272 subtests passed > > after: > [20:22:55.495] 133/296 postgresql:pg_amcheck / pg_amcheck/004_verify_heapam OK 4.60s 32 subtests passed > [20:25:13.641] 212/296 postgresql:amcheck / amcheck/001_verify_heapam OK 4.87s 272 subtests passed Nice! > > I agree with you that falling back on BackgroundPsql is not a terribly > > satisfactory solution. > > I'm somewhat doubtful we'll just agree on making FFI::Platypus a hard > dependency, but if we agree to do so... Why can't we just do that? I mean, do we have any concrete reason to think that it'll block a supported platform? I'm personally willing to test/validate on the full set of non-Linux Unixen and write up the install instructions to help eg build farm animal owners adjust. Really this is mostly about libffi, which is super widely ported, and it is required by Python which we already soft-depend on, and will hard-depend on if we drop autoconf. The rest is presumably just Perl xs glue to drive it, which, if it doesn't work on some niche platform, you'd think should be easy enough to fix...
Thomas Munro <thomas.munro@gmail.com> writes:
> On Mon, Jun 17, 2024 at 10:38 AM Andres Freund <andres@anarazel.de> wrote:
>> I'm somewhat doubtful we'll just agree on making FFI::Platypus a hard
>> dependency, but if we agree to do so...
> Why can't we just do that? I mean, do we have any concrete reason to
> think that it'll block a supported platform?
IIUC, this would only be a hard dependency if you want to run certain
TAP tests (maybe eventually all of them). Seems like not that much of
a roadblock for somebody that's just trying to build PG for
themselves. I agree we'd want it on most buildfarm animals
eventually, but they pretty much all have python installed ...
regards, tom lane
Hi, On 2024-06-16 19:07:49 -0400, Tom Lane wrote: > Thomas Munro <thomas.munro@gmail.com> writes: > > On Mon, Jun 17, 2024 at 10:38 AM Andres Freund <andres@anarazel.de> wrote: > >> I'm somewhat doubtful we'll just agree on making FFI::Platypus a hard > >> dependency, but if we agree to do so... > > > Why can't we just do that? I mean, do we have any concrete reason to > > think that it'll block a supported platform? > > IIUC, this would only be a hard dependency if you want to run certain > TAP tests (maybe eventually all of them). I think it'd be all of them within a very short timeframe. IMO we'd want to convert a bunch of the code in Cluster.pm to use psql-less connections to maximize the benefit across all tests, without needing to modify all of them. Greetings, Andres Freund
On Mon, Jun 17, 2024 at 10:57 AM Thomas Munro <thomas.munro@gmail.com> wrote: > I'm personally willing to test/validate on the full set of non-Linux > Unixen and write up the install instructions to help eg build farm > animal owners adjust. I created a page where we can log "works/doesn't work" and "installed how" information: https://wiki.postgresql.org/wiki/Platypus I'll go and test the BSDs and hopefully illumos. And then maybe Macs if Tom doesn't beat me to it.
Hi, On 2024-06-17 12:03:28 +1200, Thomas Munro wrote: > And then maybe Macs if Tom doesn't beat me to it. macports even has a platypus package, so that should be easy. For CI it should suffice to add p5.34-ffi-platypus to the list of packages in macos' setup_additional_packages_script, they then should get automatically cached. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2024-06-17 12:03:28 +1200, Thomas Munro wrote:
>> And then maybe Macs if Tom doesn't beat me to it.
> macports even has a platypus package, so that should be easy.
Less easy if you don't want to depend on macports or homebrew.
However, I see something a bit promising-looking in the base system:
$ ls -l /usr/lib/*ffi*
-rwxr-xr-x 1 root wheel 100720 May 7 03:01 /usr/lib/libffi-trampolines.dylib
regards, tom lane
Thomas Munro <thomas.munro@gmail.com> writes:
> Really this is mostly about libffi, which is
> super widely ported, and it is required by Python
BTW, what form does that "requirement" take exactly? I see no
evidence that the core python3 executable is linked to libffi
on any of the machines I checked.
regards, tom lane
I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> macports even has a platypus package, so that should be easy.
> Less easy if you don't want to depend on macports or homebrew.
I tried "sudo cpan install FFI::Platypus" against macOS Sonoma's
base-system perl. It seemed to compile all right, but a nontrivial
fraction of its self-tests fail:
Files=72, Tests=296, 7 wallclock secs ( 0.10 usr 0.07 sys + 4.96 cusr 1.34 csys = 6.47 CPU)
Result: FAIL
Failed 33/72 test programs. 87/296 subtests failed.
make: *** [test_dynamic] Error 3
PLICEASE/FFI-Platypus-2.08.tar.gz
/usr/bin/make test -- NOT OK
No energy for digging deeper tonight.
regards, tom lane
On Mon, Jun 17, 2024 at 12:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > Really this is mostly about libffi, which is
> > super widely ported, and it is required by Python
>
> BTW, what form does that "requirement" take exactly? I see no
> evidence that the core python3 executable is linked to libffi
> on any of the machines I checked.
There is another library in between:
$ ldd /usr/local/lib/python3.11/lib-dynload/_ctypes.cpython-311.so
/usr/local/lib/python3.11/lib-dynload/_ctypes.cpython-311.so:
libffi.so.8 => /usr/local/lib/libffi.so.8 (0x214865b76000)
libdl.so.1 => /usr/lib/libdl.so.1 (0x214864bcc000)
libthr.so.3 => /lib/libthr.so.3 (0x214866862000)
libc.so.7 => /lib/libc.so.7 (0x214863e03000)
Perhaps it's technically possible to build Python without the ctypes
module, but I'm not sure and I don't see anywhere that describes it
explicitly as optional.
On Mon, Jun 17, 2024 at 1:36 PM Thomas Munro <thomas.munro@gmail.com> wrote: > Perhaps it's technically possible to build Python without the ctypes > module, but I'm not sure and I don't see anywhere that describes it > explicitly as optional. One clue is that they used to bundle their own copy of libffi before Python 3.7. You had a choice of that or --with-system-ffi, but I don't see an option for none. I might be missing something about their build system, though.
On 2024-Jun-16, Andrew Dunstan wrote:
> +sub query_oneval
> +{
> + my $self = shift;
> + my $sql = shift;
> + my $missing_ok = shift; # default is not ok
> + my $conn = $self->{conn};
> + my $result = PQexec($conn, $sql);
> + my $ok = $result && (PQresultStatus($result) == PGRES_TUPLES_OK);
> + unless ($ok)
> + {
> + PQclear($result) if $result;
> + return undef;
> + }
> + my $ntuples = PQntuples($result);
> + return undef if ($missing_ok && !$ntuples);
> + my $nfields = PQnfields($result);
> + die "$ntuples tuples != 1 or $nfields fields != 1"
> + if $ntuples != 1 || $nfields != 1;
> + my $val = PQgetvalue($result, 0, 0);
> + if ($val eq "")
> + {
> + $val = undef if PGgetisnull($result, 0, 0);
> + }
> + PQclear($result);
> + return $val;
> +}
Hmm, here you use PGgetisnull, is that a typo for PQgetisnull? If it
is, then I wonder why doesn't this fail in some obvious way? Is this
part dead code maybe?
> +# return tuples like psql's -A -t mode.
> +
> +sub query_tuples
> +{
> + my $self = shift;
> + my @results;
> + foreach my $sql (@_)
> + {
> + my $res = $self->query($sql);
> + # join will render undef as an empty string here
> + no warnings qw(uninitialized);
> + my @tuples = map { join('|', @$_); } @{$res->{rows}};
> + push(@results, join("\n",@tuples));
> + }
> + return join("\n",@results);
> +}
You made this function join the tuples from multiple queries together,
but the output format doesn't show anything for queries that return
empty. I think this strategy doesn't cater for the case of comparing
results from multiple queries very well, because it might lead to sets
of queries that return empty result for different queries reported as
identical when they aren't. Maybe add a separator line between the
results from each query, when there's more than one? (Perhaps just
"join('--\n', @results)" in that last line does the trick?)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"The Postgresql hackers have what I call a "NASA space shot" mentality.
Quite refreshing in a world of "weekend drag racer" developers."
(Scott Marlowe)
On 2024-06-17 Mo 5:07 AM, Alvaro Herrera wrote:
> On 2024-Jun-16, Andrew Dunstan wrote:
>
>
>> +sub query_oneval
>> +{
>> + my $self = shift;
>> + my $sql = shift;
>> + my $missing_ok = shift; # default is not ok
>> + my $conn = $self->{conn};
>> + my $result = PQexec($conn, $sql);
>> + my $ok = $result && (PQresultStatus($result) == PGRES_TUPLES_OK);
>> + unless ($ok)
>> + {
>> + PQclear($result) if $result;
>> + return undef;
>> + }
>> + my $ntuples = PQntuples($result);
>> + return undef if ($missing_ok && !$ntuples);
>> + my $nfields = PQnfields($result);
>> + die "$ntuples tuples != 1 or $nfields fields != 1"
>> + if $ntuples != 1 || $nfields != 1;
>> + my $val = PQgetvalue($result, 0, 0);
>> + if ($val eq "")
>> + {
>> + $val = undef if PGgetisnull($result, 0, 0);
>> + }
>> + PQclear($result);
>> + return $val;
>> +}
> Hmm, here you use PGgetisnull, is that a typo for PQgetisnull? If it
> is, then I wonder why doesn't this fail in some obvious way? Is this
> part dead code maybe?
It's not dead, just not exercised ATM. I should maybe include a test
scripts for the two new modules.
As you rightly suggest, it's a typo. If it had been called it would have
aborted the test.
>
>> +# return tuples like psql's -A -t mode.
>> +
>> +sub query_tuples
>> +{
>> + my $self = shift;
>> + my @results;
>> + foreach my $sql (@_)
>> + {
>> + my $res = $self->query($sql);
>> + # join will render undef as an empty string here
>> + no warnings qw(uninitialized);
>> + my @tuples = map { join('|', @$_); } @{$res->{rows}};
>> + push(@results, join("\n",@tuples));
>> + }
>> + return join("\n",@results);
>> +}
> You made this function join the tuples from multiple queries together,
> but the output format doesn't show anything for queries that return
> empty. I think this strategy doesn't cater for the case of comparing
> results from multiple queries very well, because it might lead to sets
> of queries that return empty result for different queries reported as
> identical when they aren't. Maybe add a separator line between the
> results from each query, when there's more than one? (Perhaps just
> "join('--\n', @results)" in that last line does the trick?)
>
psql doesn't do that, and this is designed to mimic psql's behaviour. We
could change that of course. I suspect none of the uses expect empty
resultsets, so it's probably somewhat moot.
Thanks for looking.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On 2024-Jun-17, Andrew Dunstan wrote:
> On 2024-06-17 Mo 5:07 AM, Alvaro Herrera wrote:
> > You made this function join the tuples from multiple queries together,
> > but the output format doesn't show anything for queries that return
> > empty. I think this strategy doesn't cater for the case of comparing
> > results from multiple queries very well, because it might lead to sets
> > of queries that return empty result for different queries reported as
> > identical when they aren't. Maybe add a separator line between the
> > results from each query, when there's more than one? (Perhaps just
> > "join('--\n', @results)" in that last line does the trick?)
>
> psql doesn't do that, and this is designed to mimic psql's behaviour. We
> could change that of course. I suspect none of the uses expect empty
> resultsets, so it's probably somewhat moot.
True -- I guess my comment should really be directed to the original
coding of the test in test_index_replay. I think adding the separator
line makes it more trustworthy.
Probably you're right that the current code of in-core tests don't care
about this, but if we export this technique to the world, I'm sure
somebody somewhere is going to care.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"The Gord often wonders why people threaten never to come back after they've
been told never to return" (www.actsofgord.com)
On 2024-06-16 Su 6:38 PM, Andres Freund wrote: > Hi, > > On 2024-06-16 17:43:05 -0400, Andrew Dunstan wrote: >> On Fri, Jun 14, 2024 at 12:25 PM Andres Freund <andres@anarazel.de> wrote: >> I guess it's a question of how widely available FFI::Platypus is. I know >> it's available pretty much out of the box on Strawberry Perl and Msys2' >> ucrt perl. > FWIW I hacked a bit on CI, trying to make it work. Took a bit, partially > because CI uses an older strawberry perl without FFI::Platypus. And > FFI::Platypus didn't build with that. > > > Updating that to 5.38 causes some complaints about LANG that I haven't hunted > down, just avoided by unsetting LANG. > > > As-is your patch didn't work, because it has "systempath => []", which caused > libpq to not load, because it depended on things in the system path... > > > What's the reason for that? Not sure, that code was written months ago. I just checked the FFI::CheckLib code and libpath is searched before systempath, so there shouldn't be any reason not to use the default load path. > > After commenting that out, all but one tests passed: > > [20:21:31.137] ------------------------------------- 8< ------------------------------------- > [20:21:31.137] stderr: > [20:21:31.137] # Failed test 'psql connect success' > [20:21:31.137] # at C:/cirrus/src/test/recovery/t/041_checkpoint_at_promote.pl line 161. > [20:21:31.137] # got: '2' > [20:21:31.137] # expected: '0' > [20:21:31.137] # Failed test 'psql select 1' > [20:21:31.137] # at C:/cirrus/src/test/recovery/t/041_checkpoint_at_promote.pl line 162. > [20:21:31.137] # got: '' > [20:21:31.137] # expected: '1' > [20:21:31.137] # Looks like you failed 2 tests of 6. > [20:21:31.137] > [20:21:31.137] (test program exited with status code 2) > [20:21:31.137] ------------------------------------------------------------------------------ > [20:21:31.137] Yeah, the recovery tests were using poll_query_until in a rather funky way. That's fixed in this latest version. > > > >> I agree with you that falling back on BackgroundPsql is not a terribly >> satisfactory solution. > I'm somewhat doubtful we'll just agree on making FFI::Platypus a hard > dependency, but if we agree to do so... > > Maybe not. If so your other suggestion of a small XS wrapper might make sense. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Attachment
On 2024-06-17 Mo 10:01 AM, Andrew Dunstan wrote: > >> >>> I agree with you that falling back on BackgroundPsql is not a terribly >>> satisfactory solution. >> I'm somewhat doubtful we'll just agree on making FFI::Platypus a hard >> dependency, but if we agree to do so... >> >> > > Maybe not. If so your other suggestion of a small XS wrapper might > make sense. Here's the latest version of this patch. It removes all use of background_psql(). Instead it uses libpq's async interface, which seems to me far more robust. There is one remaining use of interactive_psql(), but that's reasonable as it's used for testing psql itself. I spent yesterday creating an XS wrapper for just the 19 libpq functions used in Session.pm. It's pretty simple. I have it passing a very basic test, but haven't tried plugging it into Session.pm yet. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Attachment
On Wed, Jul 17, 2024 at 2:27 AM Andrew Dunstan <andrew@dunslane.net> wrote: > Here's the latest version of this patch. It removes all use of > background_psql(). Instead it uses libpq's async interface, which seems > to me far more robust. There is one remaining use of interactive_psql(), > but that's reasonable as it's used for testing psql itself. This looks really nice! Works on my local FBSD machine. I pushed it to CI, and mostly saw environmental problems unrelated to the patch, but you might be interested in the ASAN failure visible in the cores section: https://cirrus-ci.com/task/6607915962859520 Unfortunately I can't see the interesting log messages, because it detected that the logs were still being appended to and declined to upload them. I think that means there must be subprocesses not being waited for somewhere? > I spent yesterday creating an XS wrapper for just the 19 libpq functions > used in Session.pm. It's pretty simple. I have it passing a very basic > test, but haven't tried plugging it into Session.pm yet. Neat. I guess the libpq FFI/XS piece looks the same to the rest of the test framework outside that module. It does sound pretty convenient if the patch just works™ on CI/BF without any environment changes, which I assume must be doable because we already build XS stuff in sr/pl/plperl. Looking forward to trying that version.
On 2024-07-18 Th 6:51 PM, Thomas Munro wrote:
On Wed, Jul 17, 2024 at 2:27 AM Andrew Dunstan <andrew@dunslane.net> wrote:Here's the latest version of this patch. It removes all use of background_psql(). Instead it uses libpq's async interface, which seems to me far more robust. There is one remaining use of interactive_psql(), but that's reasonable as it's used for testing psql itself.This looks really nice! Works on my local FBSD machine.
cool
I pushed it to CI, and mostly saw environmental problems unrelated to the patch, but you might be interested in the ASAN failure visible in the cores section: https://cirrus-ci.com/task/6607915962859520 Unfortunately I can't see the interesting log messages, because it detected that the logs were still being appended to and declined to upload them. I think that means there must be subprocesses not being waited for somewhere?
I couldn't see anything obvious either.
I spent yesterday creating an XS wrapper for just the 19 libpq functions used in Session.pm. It's pretty simple. I have it passing a very basic test, but haven't tried plugging it into Session.pm yet.Neat. I guess the libpq FFI/XS piece looks the same to the rest of the test framework outside that module.
Yeah, that's the idea.
It does sound pretty convenient if the patch just works™ on CI/BF without any environment changes, which I assume must be doable because we already build XS stuff in sr/pl/plperl. Looking forward to trying that version.
Still working on it. Meanwhile, here's a new version. It has some cleanup and also tries to use Session objects instead of psql in simple cases for safe_psql().
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com