Fix ResultsExist

[frioux]

Looks like when I "fixed" ResultsExist I broke it :(

Here's some info:

11:24:33     Perlkonig | https://metacpan.org/source/FREW/DBIx-Class-Helpers-2.031000/lib/DBIx/Class/Helper/ResultSet/Shortcut/ResultsExist.pm <- heh, and
                       | there's ->first again :-)
11:25:30    @ribasushi | frew: ^^ that's indeed odd
11:25:47         @frew | not totally sure I wrote that
11:25:54         @frew | the Shortcuts were mostly contributed
11:26:01    @ribasushi | frew: I'd go for rows => 1 + next, so it implicitly exhausts
11:26:19    @ribasushi | ( in addition to the EXISTS )
11:26:20         @frew | but fwiw, there was a LONG time where I heard a lot of conflicting information about first, next, and single
11:27:23     Perlkonig | wait, what SQL does that even generate? "SELECT EXISTS (SELECT ...);"?
11:27:39     Perlkonig | no, that's way off
11:29:08     Perlkonig | "SELECT * FROM table WHERE EXISTS (SELECT ...);"?
11:29:23         @frew | something along those lins
11:29:29         @frew | you could run it and see :)
11:29:32     Perlkonig | so that would return either no rows, or all rows
11:29:35         @frew | no
11:29:50         @frew | the query would, but we don't fetch it all
11:30:21         @frew | Perlkonig: https://metacpan.org/pod/DBIx::Class::Helper::ResultSet::Shortcut#results_exist
11:30:24         @frew | the doc might help?
11:31:41     Perlkonig | oh, I thought it was undocumented
11:31:52     Perlkonig | I saw it in https://metacpan.org/release/DBIx-Class-Helpers but it had no POD
11:32:03         @frew | yeah it's sorta internal to ::Shortcut
11:32:12     Perlkonig | I did run it
11:32:14     Perlkonig | I was right
11:32:21         @frew | I figured ::Shortcut wouldb e easier if everything were documnted in one place
11:32:24     Perlkonig | it also runs out of memory because it fetches all rows
11:32:25         @frew | Perlkonig: do you use postgres?
11:32:43     Perlkonig | in general, yes, but this app also needs to access a mysql db
11:32:44         @frew | sounds like you are a pg user expecting all dbd's to be as silly as yours :)
11:32:51    @ribasushi | errrrr
11:32:52             * | ribasushi looks closer at the thing
11:33:03    @ribasushi | frew: this is actually entirely incorrect (putting it in where)
11:33:05     Perlkonig | so yeah, ResultsExist is completely broken
11:33:32             * | frew looks at test
11:33:36    @ribasushi | you want to SELECT EXISTS (....) FROM <something, doesn't matter what)
11:33:53    @ribasushi | which will always return 1 row, with a boolean in the 1st col
11:34:11     Perlkonig | or just do the { rows => 1 } thing again
11:34:20     Perlkonig | hmm, but that would still have to transfer all columns
11:34:49         @frew | patches welcome?
11:34:53         @frew | I haven't actually used this helper

11:34:53    @ribasushi | frew: sorry I didn't catch that back when you wrote it, I should have seen that :/
11:35:02         @frew | ribasushi: again: I didn't write it :)
11:35:06         @frew | it was contributed
11:35:09    @ribasushi | hm... was it oalders maybe?
11:35:12         @frew | yessir
11:35:12             * | ribasushi fuzzy
11:35:22         @frew | https://github.com/frioux/DBIx-Class-Helpers/blame/master/lib/DBIx/Class/Helper/ResultSet/Shortcut/ResultsExist.pm
11:35:32    @ribasushi | oalders: ^^ read past ~15 lines
11:35:37       oalders | ok
11:35:43     Perlkonig | - Add ::Shortcut::ResultsExist RS helper (Olaf Alders)
11:35:44         @frew | anyway I'm totally interested in fixing it, I just won't take the blame :)
11:35:46     Perlkonig | oh, too late
11:36:07       oalders | ribasushi: that's a copy/paste of some code you gave me
11:36:08         @frew | hm
11:36:11       oalders | but it seems to work for me
11:36:16         @frew | did I break it somehow? https://github.com/frioux/DBIx-Class-Helpers/commit/fd4024322fb7601352f2caf1ab6c5873f6ea6263
11:36:29         @frew | oalders: they are claiming that while it may work, it pulls in the whole table
11:36:36    @ribasushi | oalders: it works, it's just very inefficient
11:36:43       oalders | yeah, that's not at all what we wanted
11:36:44         @frew | I'm theorizing that it works for everyting but pg
11:36:48     Perlkonig | it works if your table is small enough to fit into memory
11:36:49         @frew | but I don't really know
11:37:28       oalders | i'm using it on Pg. obviously didn't realize there's an issue
11:37:38    @ribasushi | frew: yeah - the original https://github.com/frioux/DBIx-Class-Helpers/commit/fd402432 has the select => ...
11:37:46         @frew | iirc that actually failed tests
11:37:52         @frew | or I'dve left it alone
11:38:01     Perlkonig | I'm running out of memory on a mysql table with 6917243 rows :-)
11:38:10    @ribasushi | right - but the fix changed the concept entirely ;)
11:38:12         @frew | Perlkonig: at the client or the server?
11:38:40       oalders | https://github.com/frioux/DBIx-Class-Helpers/pull/25
11:38:51     Perlkonig | frew: client
11:38:54         @frew | huh
11:39:07     Perlkonig | perl gets oom-killed or runs into ulimits
11:39:11         @frew | sure sure
11:39:58         @frew | ribasushi: so yeah, with his original you get a SQL syntax error
11:40:02    @ribasushi | Perlkonig: problem has been identified, if frew and oalders doesn't have time to fixxxor it - perhaps you can lend a hand? :)
11:40:02         @frew | maybe a bug in core dbic?
11:40:12         @frew | DBIx::Class::Storage::DBI::_prepare_sth(): DBI Exception: DBD::SQLite::db prepare_cached failed: near "(": syntax error [for Statement
                       | "SELECT EXISTS( (SELECT "me"."id", "me"."bar_id" FROM "Foo" "me" WHERE ( "id" > ? )) ) FROM "Foo" "me""] at
                       | t/ResultSet/Shortcut/ResultsExist.t line 16
11:40:41     Perlkonig | that doesn't look right to me either
11:40:43    @ribasushi | frew: sqlite is choking on (( ))
11:40:48     Perlkonig | you don't want to select from Foo
11:40:55         @frew | oalders: fwiw, I could change it back to what you had and only test against pg
11:41:10         @frew | I have that capability in the helpers, and there are some helpers that don't support some engines
11:41:38    @ribasushi | Perlkonig: one needs to select from "somewhere" on most engines
11:41:50             * | frew could regex out the () but that seems perilous
11:41:57         @frew | ribasushi: thoughts re the parens?
11:42:19     Perlkonig | ribasushi: yeah, but then you need some kind of restriction (like rows => 1!), or you'll get as many results as there are rows in the
                       | table
11:42:47    @ribasushi | Perlkonig: yes that's a fair point too
11:43:01     Perlkonig | this is like  (map { exists $foo{key} } keys %foo)
11:43:02     Perlkonig | sort of
11:43:20    @ribasushi | frew: I'd say most expedient is 'EXISTS ' . ${$asq}->[0]
11:44:13         @frew | bummer
11:44:21     Perlkonig | ... in that case, why even bother with exists?
11:44:30     Perlkonig | select 1 from Foo limit 1  
11:44:49         @frew | Perlkonig: well the whole thing is if your db engine needs some other way of checking existence
11:45:00         @frew | I mean, for some people I think doing a limit at all can be pricy?
11:45:05         @frew | it varies from engine to engine
11:45:10    @ribasushi | Perlkonig: because something needs to apply all the $rs conditions
11:45:11         @frew | ribasushi might know better
11:45:33     Perlkonig | ah, true
11:46:04    @ribasushi | Perlkonig: basically the original thinking is to have a proper subquery and hint the optimizer "I just want to know if there's
                       | anything" for portaility
11:46:27    @ribasushi | instead of descending into the nightmares that are limits, and various codepaths depending on the $rs attributes
11:46:40    @ribasushi | nevertheless - given the amount of fail, it may be warranted
11:47:00    @ribasushi | s/warranted/necessary/
11:47:09     Perlkonig | I don't see how you can do this portably without a generic 1-row relation to select from
11:47:10         @frew | s/necesarry/wantarray/
11:47:29    @ribasushi | Perlkonig: in the current form - it won't work this is true  
11:47:56    @ribasushi | Perlkonig: but there may be a portable-ish FROM <> spec or somesuch
11:48:05    @ribasushi | someone needs to do the research basically
11:48:27             * | ribasushi is braincycle-booked for the next week or so, thus someone else stepping up would be awesome
11:50:34     Perlkonig | what do I need to do if I want to write my own dbic component? 
11:50:49         @frew | subclass something, maybe less
11:51:14     Perlkonig | ... huh?
11:51:22         @frew | Perlkonig: like, a component is just a perl package
11:51:44         @frew | you set your base class so that C3 knows what to do (likely DBIx::Class::Core or DBIx::Class::ResultSet)
11:51:53         @frew | and then load the component with load_components
11:52:33             * | frew adds DBICH issue re ResultsExist

[frioux]

cc: @oalders

[oalders]

[13:02:19]  <@frew> our best bet at this point is to add a limit => 1 or do research on if there is a standard dummy empty table
[13:02:26]  <@frew> or something like that
[13:02:39]  <@frew> I think the end result needs to be like
[13:02:50]  <@frew> SELECT 1 FROM DUMMY where EXISTENCE CHECK
[13:02:53]  <@frew> more or less
[13:03:07]  <@frew> and actually dummy should have 1 row

[frioux]

Seemingly unrelated conversation that could resolve our issue:

14:56:44      xinming_ | mst: Is it possible to make DBIC to do soemthing like  SELECT 1 FROM table WHERE ...?
14:57:10          @mst | step back. explain what you're trying to achieve.
14:57:18      xinming_ | What I mean is something like  SELECT EXISTS (SELECT ... FROM ...);
14:57:39      xinming_ | I want to test wether we have results in for a ResultSet
14:58:25      xinming_ | so we don't need to use count to access all rows. Since in postgres, It'll try to check all rows matched.
14:59:10      xinming_ | so something like  'SELECT 1 FROM table WHERE rs filtering clauses'
15:01:06          @mst | columns => [ { foo => \1 } ] ?
15:02:06      xinming_ | $self->search($query, { rows => 1, columns => [\1] })->single;    <--- Something like this?
15:02:18      xinming_ | Let me ry.
15:02:19      xinming_ | try
15:06:55       tharkun | How can I do an INSERT ... SELECT ... with DBIx::Class ? man page will suffice
15:07:54     @castaway | tharkun: ->as_query
15:08:10       tharkun | castaway: Thanks
15:09:39      xinming_ | Thanks
15:09:41      xinming_ | got it
15:10:12      xinming_ | $rs->search(undef, { columns => { exists => \1 }, rows => 1 })->single->get_column('exists'); worked

[frioux]

Got started on this and sadly the answer @shadowcat-mst gave to @xinming fails tests and always returns true.

[frioux]

Oh the branch I started on was here if anyone wants to try out themselves.

[ilmari]

The simplest thing would be $self->search(undef, { rows => 1 })->count, but this bit in DBIx::Class::ResultSet::Count defeats that:

  # this is a little optimization - it is faster to do the limit
  # adjustments in software, instead of a subquery
  my ($rows, $offset) = delete @{$attrs}{qw/rows offset/};

It might be the case for some databases without built-in LIMIT (or equivalent) and have to do horrible ROW_COUNT subqueries, but PostgreSQL definitely benefits from leaving the LIMIT in.

[karenetheridge]

Has anyone looked at this in a while? it would be great to not have a dangling cursor from the ->first in the current implementation on cpan.

[frioux]

Nope; help welcome :)

[karenetheridge]

@ribasushi's comments here (https://github.com/frioux/DBIx-Class-Helpers/commit/85702318c13da517d0c9d6b997ff346fdc47abba) seemed sane, but this is way too far out of my depth to be able to add on to that.

[karenetheridge]

FWIW, here is a postgres-specific implementation:

# generates: select exists (SELECT 1 FROM $table WHERE ... )
sub exists {
    my $self = shift;

    my $inner = $self->search(undef, { select => [ \1 ] })->as_query;

    my $statement = 'select exists ' . $inner->$*->[0];
    my @binds = map { $_->[1] } $inner->$*->@[1 .. $inner->$*->$#*];

    my ($exists) = $self->result_source->schema->storage->dbh_do(sub {
        my ($storage, $dbh) = @_;

        # cribbed from DBIx::Class::Storage::DBI::_format_for_trace
        $storage->debugobj->query_start($statement, map { defined($_) ? qq{'$_'} : q{NULL} } @binds)
            if $storage->debug;

        return $dbh->selectrow_array($statement, undef, @binds);
    });

    return $exists;
}

[frioux]

Fixed by #98

[karenetheridge]

Given the changes to the query that I see in https://gist.github.com/karenetheridge/750f774d1e4dea2692d80d9babecb568, I think I will stick with the original implementation I had. The addition of all columns in the original source, twice, can become quite inefficient.

[ribasushi]

@karenetheridge something locally in your codebase ( some other extension? ) is interfering with the generation of the column list. You need to debug this. The shipped version does not do what you are describing:

rabbit@Ahasver:~$ cpanm --look DBIx::Class::Helpers
--> Working on DBIx::Class::Helpers
Fetching http://www.cpan.org/authors/id/F/FR/FREW/DBIx-Class-Helpers-2.034001.tar.gz ... OK
Entering /home/rabbit/.cpanm/work/1575914802.6172/DBIx-Class-Helpers-2.034001 with /bin/bash

rabbit@Ahasver:~/.cpanm/work/1575914802.6172/DBIx-Class-Helpers-2.034001$ DBIC_TRACE=1 prove -l t/ResultSet/Shortcut/ResultsExist.t 
t/ResultSet/Shortcut/ResultsExist.t .. 
...
SELECT * FROM ( SELECT EXISTS (SELECT 42 FROM "Foo" "me" WHERE ( "id" > ? )) ): '0'
t/ResultSet/Shortcut/ResultsExist.t .. 1/? SELECT * FROM ( SELECT EXISTS (SELECT 42 FROM "Foo" "me" WHERE ( "id" < ? )) ): '0'
SELECT ( SELECT EXISTS (SELECT 42 FROM "Foo" "correlation" WHERE ( ( "correlation"."id" > "me"."id" AND "id" > ? ) )) ), ( SELECT EXISTS (SELECT 42 FROM "Foo" "correlation" WHERE ( ( "correlation"."id" < "me"."id" AND "id" > ? ) )) ), "me"."id" FROM "Foo" "me" WHERE ( "id" > ? )  ORDER BY "id": '0', '0', '0'
t/ResultSet/Shortcut/ResultsExist.t .. ok   
All tests successful.
Files=1, Tests=3,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.56 cusr  0.01 csys =  0.59 CPU)
Result: PASS

[karenetheridge]

@ribasushi much to my embarrassment I discovered that I was testing on a machine where I hadn't updated ::Helpers to 2.034001, which contain the reworded exists. :/

However, this leads to a worse problem.. the reworked query doesn't work at all. :(

(This is with PostgreSQL 10.10 on x86_64-apple-darwin17.7.0, compiled by Apple LLVM version 9.1.0 (clang-902.0.39.2), 64-bit)

my code:

my $exists = $schema->resultset('build')->search({ name => 'my first build' })->results_exist;

output, with DBIC_TRACE enabled:

SELECT * FROM ( SELECT EXISTS (SELECT 42 FROM build me WHERE ( name = ? )) ): 'my first build'
DBI Exception: DBD::Pg::st execute failed: ERROR:  subquery in FROM must have an alias
LINE 1: SELECT * FROM ( SELECT EXISTS (SELECT 42 FROM build me WHERE...
                      ^
HINT:  For example, FROM (SELECT ...) [AS] foo. [for Statement "SELECT * FROM ( SELECT EXISTS (SELECT 42 FROM build me WHERE ( name = ? )) )" with ParamValues: 1='my first build'] at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/DBIx/Class/Schema.pm line 1118.
    DBIx::Class::Schema::throw_exception(Conch::DB=HASH(0x7fc41b349a28), "DBI Exception: DBD::Pg::st execute failed: ERROR:  subquery i"...) called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/DBIx/Class/Storage.pm line 113
    DBIx::Class::Storage::throw_exception(DBIx::Class::Storage::DBI::Pg=HASH(0x7fc41b348e40), "DBI Exception: DBD::Pg::st execute failed: ERROR:  subquery i"...) called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/DBIx/Class/Storage/DBI.pm line 1501
    DBIx::Class::Storage::DBI::__ANON__("DBD::Pg::st execute failed: ERROR:  subquery in FROM must hav"..., DBI::st=HASH(0x7fc41bc53e50), undef) called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/DBIx/Class/Storage/DBI.pm line 1836
    DBIx::Class::Storage::DBI::_dbh_execute(DBIx::Class::Storage::DBI::Pg=HASH(0x7fc41b348e40), DBI::db=HASH(0x7fc41e4af7b0), "SELECT * FROM ( SELECT EXISTS (SELECT 42 FROM build me WHERE "..., ARRAY(0x7fc41b349ab8), ARRAY(0x7fc41bd15d70)) called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/DBIx/Class/Storage/DBI.pm line 856
    DBIx::Class::Storage::DBI::__ANON__() called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/DBIx/Class/Storage/BlockRunner.pm line 130
    DBIx::Class::Storage::BlockRunner::try {...} () called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/Try/Tiny.pm line 98
    eval {...} called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/Try/Tiny.pm line 93
    Try::Tiny::try(CODE(0x7fc41bc54150), Try::Tiny::Catch=REF(0x7fc41bd2f610)) called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/DBIx/Class/Storage/BlockRunner.pm line 134
    DBIx::Class::Storage::BlockRunner::__ANON__() called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/Context/Preserve.pm line 33
    Context::Preserve::preserve_context(CODE(0x7fc41d617918), "replace", CODE(0x7fc41f1d0038)) called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/DBIx/Class/Storage/BlockRunner.pm line 213
    DBIx::Class::Storage::BlockRunner::_run(DBIx::Class::Storage::BlockRunner=HASH(0x7fc41e423ed8), CODE(0x7fc41e4aed60)) called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/DBIx/Class/Storage/BlockRunner.pm line 105
    DBIx::Class::Storage::BlockRunner::run(DBIx::Class::Storage::BlockRunner=HASH(0x7fc41e423ed8), CODE(0x7fc41e4aed60)) called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/DBIx/Class/Storage/DBI.pm line 857
    DBIx::Class::Storage::DBI::dbh_do(undef, undef, "SELECT * FROM ( SELECT EXISTS (SELECT 42 FROM build me WHERE "..., ARRAY(0x7fc41b349ab8), ARRAY(0x7fc41bd15d70)) called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/DBIx/Class/Storage/DBI.pm line 1817
    DBIx::Class::Storage::DBI::_execute(DBIx::Class::Storage::DBI::Pg=HASH(0x7fc41b348e40), "select", REF(0x7fc41bd0e320), SCALAR(0x7fc41e356be0), HASH(0x7fc41e4b0d88), HASH(0x7fc41f19e9b0)) called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/DBIx/Class/Storage/DBI.pm line 2409
    DBIx::Class::Storage::DBI::_select(DBIx::Class::Storage::DBI::Pg=HASH(0x7fc41b348e40), REF(0x7fc41bd0e320), SCALAR(0x7fc41e356be0), HASH(0x7fc41e4b0d88), HASH(0x7fc41b349110)) called at /Users/ether/.perlbrew/libs/26.3@std/lib/perl5/DBIx/Class/Helper/ResultSet/Shortcut/ResultsExist.pm line 27
    DBIx::Class::Helper::ResultSet::Shortcut::ResultsExist::results_exist(Conch::DB::ResultSet::Build=HASH(0x7fc41f190a50)) called at lib/Conch/DB/Helper/ResultSet/ResultsExist.pm line 39
    Conch::DB::Helper::ResultSet::ResultsExist::exists(Conch::DB::ResultSet::Build=HASH(0x7fc41f190a50)) called at t/resultsexist.t line 14

[ribasushi]

@karenetheridge indeed, I screwed up an aliasing step when writing up the patch for @frioux. Something like this should do it, but I will not have time to properly test/PR this:

--- lib/DBIx/Class/Helper/ResultSet/Shortcut/ResultsExist.pm.orig   2019-12-09 19:44:10.666520779 +0100
+++ lib/DBIx/Class/Helper/ResultSet/Shortcut/ResultsExist.pm    2019-12-09 19:47:21.834153731 +0100
@@ -24,11 +24,14 @@
 sub results_exist {
    my $self = shift;

+   my $inner_q = $self->results_exist_as_query;
+   $$inner_q->[0] .= " AS _existence_subq";
+
    my( undef, $sth ) = $self->result_source
                              ->schema
                               ->storage
                                ->_select(
-                                  $self->results_exist_as_query,
+                                  $inner_q,
                                   \'*',
                                   {},
                                   {},

[karenetheridge]

That appears to work -- the new query is SELECT * FROM ( SELECT EXISTS (SELECT 42 FROM build me WHERE ( name = ? )) ) AS _existence_subq: 'my first build' which successfully evaluates.

I can PR this up for @frioux, but I am also unsure of the ramifications to other db drivers.

[karenetheridge]

for my internal use, I've boiled this down to:

sub exists ($self) {
    my $inner_q = $self->search(undef, { select => [ \1 ] })->as_query;
    $$inner_q->[0] = '( select exists '.$$inner_q->[0].' ) as _existence_subq';
    my (undef, $sth) = $self->result_source
                        ->schema
                        ->storage
                        ->_select(
                            $inner_q,
                            \'*',
                            {},
                            {},
                        );

    return $sth->fetchall_arrayref->[0][0];
}

[ribasushi]

much to my embarrassment I discovered that I was testing on a machine where I hadn't updated

@karenetheridge apparently you also have not looked at the open bugreports :unamused: https://github.com/frioux/DBIx-Class-Helpers/pull/99

[karenetheridge]

much to my embarrassment I discovered that I was testing on a machine where I hadn't updated

@karenetheridge apparently you also have not looked at the open bugreports 😒

99

You didn't seem to know about the bug report before yesterday either :)

[abraxxa]

This actually broke results_exist on Oracle:

ORA-00936: missing expression (DBD ERROR: error possibly near <*> indicator at char 23 in 'SELECT * FROM ( SELECT <*>EXISTS (SELECT 42 FROM (

[abraxxa]

https://stackoverflow.com/questions/16562162/how-to-use-select-exists-in-oracle