MIME-Version: 1.0
References: <ZQhYYRhUxpW3PSf9@telsasoft.com> <CAMbWs48X+cK2pEaZDVVjd+nOJzb1dN1CO1OKz+6D0FX2OTCGbg@mail.gmail.com>
 <CAApHDvpJJigQRW29TppTOPYp+Aui0mtd3MpfRxyKv=N-tB62jQ@mail.gmail.com>
In-Reply-To: <CAApHDvpJJigQRW29TppTOPYp+Aui0mtd3MpfRxyKv=N-tB62jQ@mail.gmail.com>
From: David Rowley <dgrowleyml@gmail.com>
Date: Tue, 3 Oct 2023 20:16:07 +1300
Message-ID: <CAApHDvo7RzcQYw-gnkZr6QCijCqf8vJLkJ4XFk-KawvyAw109Q@mail.gmail.com>
Subject: Re: pg16: XX000: could not find pathkey item to sort
To: Richard Guo <guofenglinux@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@lists.postgresql.org
Content-Type: multipart/mixed; boundary="000000000000ce2ee00606caaa6e"
Archived-At: <https://www.postgresql.org/message-id/CAApHDvo7RzcQYw-gnkZr6QCijCqf8vJLkJ4XFk-KawvyAw109Q%40mail.gmail.com>
Precedence: bulk

--000000000000ce2ee00606caaa6e
Content-Type: text/plain; charset="UTF-8"

On Tue, 3 Oct 2023 at 09:11, David Rowley <dgrowleyml@gmail.com> wrote:
> I'm concerned that this patch will be too much overhead when creating
> paths when a PathKey's EquivalenceClass has a large number of members
> from partitioned tables.

I just tried out the patch to see how much it affects the performance
of the planner.  I think we need to find a better way to strip off the
pathkeys for the columns that have been aggregated.

Setup:
create table lp (a int, b int) partition by list (a);
select 'create table lp'||x||' partition of lp for values in('||x||')'
from generate_series(0,999)x;
\gexec

\pset pager off
set enable_partitionwise_aggregate=1;

Benchmark query:
explain (summary on) select a,count(*) from lp group by a;

Master:
Planning Time: 23.945 ms
Planning Time: 23.887 ms
Planning Time: 23.927 ms

perf top:
   7.39%  libc.so.6         [.] __memmove_avx_unaligned_erms
   6.98%  [kernel]          [k] clear_page_rep
   5.69%  postgres          [.] bms_is_subset
   5.07%  postgres          [.] fetch_upper_rel
   4.41%  postgres          [.] bms_equal

Patched:
Planning Time: 41.410 ms
Planning Time: 41.474 ms
Planning Time: 41.488 ms

perf top:
 19.02%  postgres          [.] bms_is_subset
   6.91%  postgres          [.] find_ec_member_matching_expr
   5.93%  libc.so.6         [.] __memmove_avx_unaligned_erms
   5.55%  [kernel]          [k] clear_page_rep
   4.07%  postgres          [.] fetch_upper_rel
   3.46%  postgres          [.] bms_equal

> I wondered if we should instead just check
> if the subpath's pathkeys match root->group_pathkeys and if they do
> set the AggPath's pathkeys to list_copy_head(subpath->pathkeys,
> root->num_groupby_pathkeys),  that'll be much cheaper, but it just
> feels a bit too much like a special case.

I tried this approach (patch attached) and it does perform better than
the other patch:

create_agg_path_fix2.patch:
Planning Time: 24.357 ms
Planning Time: 24.293 ms
Planning Time: 24.259 ms

   7.45%  libc.so.6         [.] __memmove_avx_unaligned_erms
   6.90%  [kernel]          [k] clear_page_rep
   5.56%  postgres          [.] bms_is_subset
   5.38%  postgres          [.] bms_equal

I wonder if the attached patch is too much of a special case fix.  I
guess from the lack of complaints previously that there are no other
cases where we could possibly have pathkeys that belong to columns
that are aggregated.  I've not gone to much effort to see if I can
craft a case that hits this without the ORDER BY/DISTINCT aggregate
optimisation, however.

David

--000000000000ce2ee00606caaa6e
Content-Type: application/octet-stream; name="create_agg_path_fix2.patch"
Content-Disposition: attachment; filename="create_agg_path_fix2.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_ln9zh9nj0>
X-Attachment-Id: f_ln9zh9nj0

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL29wdGltaXplci91dGlsL3BhdGhub2RlLmMgYi9zcmMv
YmFja2VuZC9vcHRpbWl6ZXIvdXRpbC9wYXRobm9kZS5jCmluZGV4IDIxMWJhNjUzODkuLjI0Njlm
NDM4MTkgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL29wdGltaXplci91dGlsL3BhdGhub2RlLmMK
KysrIGIvc3JjL2JhY2tlbmQvb3B0aW1pemVyL3V0aWwvcGF0aG5vZGUuYwpAQCAtMzEzMywxMCAr
MzEzMywxOCBAQCBjcmVhdGVfYWdnX3BhdGgoUGxhbm5lckluZm8gKnJvb3QsCiAJcGF0aG5vZGUt
PnBhdGgucGFyYWxsZWxfc2FmZSA9IHJlbC0+Y29uc2lkZXJfcGFyYWxsZWwgJiYKIAkJc3VicGF0
aC0+cGFyYWxsZWxfc2FmZTsKIAlwYXRobm9kZS0+cGF0aC5wYXJhbGxlbF93b3JrZXJzID0gc3Vi
cGF0aC0+cGFyYWxsZWxfd29ya2VyczsKKwogCWlmIChhZ2dzdHJhdGVneSA9PSBBR0dfU09SVEVE
KQotCQlwYXRobm9kZS0+cGF0aC5wYXRoa2V5cyA9IHN1YnBhdGgtPnBhdGhrZXlzOwkvKiBwcmVz
ZXJ2ZXMgb3JkZXIgKi8KKwl7CisJCWlmIChwYXRoa2V5c19jb250YWluZWRfaW4oc3VicGF0aC0+
cGF0aGtleXMsIHJvb3QtPmdyb3VwX3BhdGhrZXlzKSkKKwkJCXBhdGhub2RlLT5wYXRoLnBhdGhr
ZXlzID0gbGlzdF9jb3B5X2hlYWQoc3VicGF0aC0+cGF0aGtleXMsCisJCQkJCQkJCQkJCQkJIHJv
b3QtPm51bV9ncm91cGJ5X3BhdGhrZXlzKTsKKwkJZWxzZQorCQkJcGF0aG5vZGUtPnBhdGgucGF0
aGtleXMgPSBzdWJwYXRoLT5wYXRoa2V5czsgLyogcHJlc2VydmVzIG9yZGVyICovCisJfQogCWVs
c2UKIAkJcGF0aG5vZGUtPnBhdGgucGF0aGtleXMgPSBOSUw7CS8qIG91dHB1dCBpcyB1bm9yZGVy
ZWQgKi8KKwogCXBhdGhub2RlLT5zdWJwYXRoID0gc3VicGF0aDsKIAogCXBhdGhub2RlLT5hZ2dz
dHJhdGVneSA9IGFnZ3N0cmF0ZWd5Owo=
--000000000000ce2ee00606caaa6e--