Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hkxPd-0000jX-8c for pgsql-hackers@arkaria.postgresql.org; Tue, 09 Jul 2019 21:12:33 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hkxPb-0007E1-Su for pgsql-hackers@arkaria.postgresql.org; Tue, 09 Jul 2019 21:12:31 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hkxPb-0006x8-2V for pgsql-hackers@lists.postgresql.org; Tue, 09 Jul 2019 21:12:31 +0000 Received: from mail-wr1-x444.google.com ([2a00:1450:4864:20::444]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hkxPW-0001as-6c for pgsql-hackers@postgresql.org; Tue, 09 Jul 2019 21:12:29 +0000 Received: by mail-wr1-x444.google.com with SMTP id c2so255171wrm.8 for ; Tue, 09 Jul 2019 14:12:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=2ndquadrant-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=ROPpk6V+K7iFyDbpkJqAMeObXSqARLfN/RU1e0Bost0=; b=Q4OUPfilp+bRj3mjtIx+WPbWU1BQyjSubNW1AfK/+u9c1dBn3aYiTfxyVqUfdWBO3D H1IkrW4CJeipmmlUUymCmXoa5BPjwRglzsh0NGbHcAQV5k3utIXkkRwJ1cbasyE1dBCO P2EnkJ46dfXq4XcMcUg+qms077eWSDXxCzUMrssyj+mmtvUGx9+H3yHFwG74aYAkFF9+ BK03JMpm5ukO+v+ZEn+vbtoL6UE4SqBlzyxlbv7hUriTR5+bAKHFnG43213miMD/RYsQ tK1MhkoH0o1U92B7b4ShmMv4Ybzr5QVZM2ABfJlWBSY89JybWKkHEz8LLgdCn4WFPW54 8ACg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=ROPpk6V+K7iFyDbpkJqAMeObXSqARLfN/RU1e0Bost0=; b=CH7lfrz9imKcFaNomZc5svNxzPzpvWVB4+mm9taWoLKrV5+pw60U8DaCPtl3eHpKM2 +KIyP70VxwbkuSeDJXCai1dcTRaK+7XwUaGvmBpLeZ0nsGYUaycQhtm1+HRgC+euU1PD 4YOXWYiVHDHiBuBU6Ctjokkystr92sK9b+ya9hZMgCsw5cM/QTi8fQ6nqD114L6Zi+Qz PiVc3mBlofqC9gSfZvFYsWZAJj7ZB9DuzPvQf4YPYyc/OBpz7O3EweLX0MxC+PhPj+ri KkCFROy+V1O1XZBBgf6LjkK0QMM3VGVvfqGktBdElNlLDOm8KPmYksO46/QqEfboL3Zk lIqQ== X-Gm-Message-State: APjAAAWv14khYCLUmK1ADdTOkZIcMVq+x3E5ETNcXYH4RRZEHLGiCJp/ rTYUEQZT/fz/DZZPUHHMDGfYpA== X-Google-Smtp-Source: APXvYqxoxsvh9e/b4/MB3WdkIe2ze/y9NUR2tQa2QEASEqflD7Y3jMDNhlwCRb9w4uJne+Bt0RuC3g== X-Received: by 2002:a5d:63c8:: with SMTP id c8mr7728264wrw.21.1562706742911; Tue, 09 Jul 2019 14:12:22 -0700 (PDT) Received: from localhost (ip-86-49-253-160.net.upcbroadband.cz. [86.49.253.160]) by smtp.gmail.com with ESMTPSA id p3sm119162wmg.15.2019.07.09.14.12.20 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Tue, 09 Jul 2019 14:12:20 -0700 (PDT) Date: Tue, 9 Jul 2019 23:12:19 +0200 From: Tomas Vondra To: Binguo Bao Cc: Andrey Borodin , Paul Ramsey , Pgsql Hackers Subject: Re: Optimize partial TOAST decompression Message-ID: <20190709211219.hbo6tyfxxcy2ek3j@development> References: <7B7E4A05-F404-4083-B7F2-267869EE2910@yandex-team.ru> <20190704174654.vnvkh7rusv25o6f5@development> <20190706152337.l64zhoj3xs3qkfc4@development> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="qixxkrpgkavq2pz4" Content-Disposition: inline In-Reply-To: <20190706152337.l64zhoj3xs3qkfc4@development> User-Agent: NeoMutt/20180716-1444-295967 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --qixxkrpgkavq2pz4 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline On Sat, Jul 06, 2019 at 05:23:37PM +0200, Tomas Vondra wrote: >On Sat, Jul 06, 2019 at 02:27:56AM +0800, Binguo Bao wrote: >>Hi, Tomas! >>Thanks for your testing and the suggestion. >> >>That's quite bizarre behavior - it does work with a prefix, but not with >>>suffix. And the exact ERROR changes after the prefix query. >> >> >>I think bug is caused by "#2 0x00000000004c3b08 in >>heap_tuple_untoast_attr_slice (attr=, sliceoffset=0, >>slicelength=-1) at tuptoaster.c:315", >>since I ignore the case where slicelength is negative, and I've appended >>some comments for heap_tuple_untoast_attr_slice for the case. >> >>FWIW the new code added to this >>>function does not adhere to our code style, and would deserve some >>>additional explanation of what it's doing/why. Same for the >>>heap_tuple_untoast_attr_slice, BTW. >> >> >>I've added more comments to explain the code's behavior. >>Besides, I also modified the macro "TOAST_COMPRESS_RAWDATA" to >>"TOAST_COMPRESS_DATA" since >>it is used to get toast compressed data rather than raw data. >> > >Thanks, this seems to address the issue - I can no longer reproduce the >crashes, allowing the benchmark to complete. I'm attaching the script I >used and spreadsheet with a summary of results. > >For the cases I've tested (100k - 10M values, different compressibility, >different prefix/length values), the results are kinda mixed - many >cases got much faster (~2x), but other cases got slower too. We're >however talking about queries taking a couple of miliseconds, so in >absolute numbers the differences are small. > >That does not mean the optimization is useless - but the example shared >at the beginning of this thread is quite extreme, as the values are >extremely compressible. Each value is ~38MB (in plaintext), but a table >with 100 such values has only ~40MB. Tha's 100:1 compression ratio, >which I think is not typical for real-world data sets. > >The data I've used are less extreme, depending on the fraction of random >data in values. > >I went through the code too. I've reworder a couple of comments and code >style issues, but there are a couple of more serious issues. > > >1) Why rename TOAST_COMPRESS_RAWDATA to TOAST_COMPRESS_DATA? > >This seems unnecessary, and it discards the clear hint that it's about >accessing the *raw* data, and the relation to TOAST_COMPRESS_RAWSIZE. >IMHO we should keep the original naming. > > >2) pglz_maximum_compressed_size signatures are confusing > >There are two places with pglz_maximum_compressed_size signature, and >those places are kinda out of sync when it comes to parameter names: > > int32 > pglz_maximum_compressed_size(int32 raw_slice_size, > int32 total_compressed_size) > > extern > int32 pglz_maximum_compressed_size(int32 raw_slice_size, > int32 raw_size); > >Also, pg_lzcompress.c has no concept of a "slice" because it only deals >with simple compression, slicing is responsibility of the tuptoaster. So >we should not mix those two, not even in comments. > > >I propose tweaks per the attached patch - I think it makes the code >clearer, and it's mostly cosmetic stuff. But I haven't tested the >changes beyond "it compiles". > > >regards > FWIW I've done another round of tests with larger values (up to ~10MB) and the larger the values the better the speedup (a bit as expected). Attached is the script I used and a spreasheet with a summary. We still need a patch addressing the review comments, though. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services --qixxkrpgkavq2pz4 Content-Type: application/x-sh Content-Disposition: attachment; filename="random-bench.sh" Content-Transfer-Encoding: quoted-printable #!/usr/bin/bash=0A=0Aecho type len rows randomlen prefix slice run time=0A= =0Afor len in 10000 100000 1000000 10000000; do=0A=0A rows=3D$((1000000000/= len))=0A=0A for a in 1 10 100 1000 10000 100000 1000000 10000000; do=0A=0A = if [ "$a" -gt "$len" ]; then=0A continue=0A fi=0A=0A b=3D$((len - a))= =0A=0A dropdb --if-exists test > /dev/null 2>&1=0A createdb test > /dev/n= ull 2>&1=0A=0A psql test -c "create unlogged table t (a text)" > /dev/null= 2>&1=0A=0A psql test -c "create extension randomstring" > /dev/null 2>&1= =0A=0A for p in 0 10 100 1000 10000 100000 1000000 10000000; do=0A=0A if= [ "$p" -gt "$len" ]; then=0A continue=0A fi=0A=0A for l in 1 10 100= 1000 10000 100000 1000000 10000000; do=0A=0A if [ "$l" -gt "$len" ]; th= en=0A continue=0A fi=0A=0A for r in `seq 1 5`; do=0A=0A psql = test -c "truncate t" > /dev/null 2>&1=0A=0A psql test -c "insert into t= select random_string($a) || repeat('a', $b) from generate_series(1,$rows) = S(i)" > /dev/null 2>&1=0A=0A psql test -c "vacuum analyze t" > /dev/nul= l 2>&1=0A=0A psql test -c "checkpoint" > /dev/null 2>&1=0A=0A psql = test > tmp.log < /dev/null 2>&1=0A creat= edb test > /dev/null 2>&1=0A=0A psql test -c "create unlogged table t (a t= ext)" > /dev/null 2>&1=0A=0A psql test -c "create extension randomstring" = > /dev/null 2>&1=0A=0A for p in 0 10 100 1000 10000 100000 1000000 1000000= 0; do=0A=0A if [ "$p" -gt "$len" ]; then=0A continue=0A fi=0A=0A = for l in 1 10 100 1000 10000 100000 1000000 10000000; d= o=0A=0A if [ "$l" -gt "$len" ]; then=0A continue=0A fi=0A=0A f= or r in `seq 1 5`; do=0A=0A psql test -c "truncate t" > /dev/null 2>&1= =0A=0A psql test -c "insert into t select repeat('a', $b) || random_str= ing($a) from generate_series(1,$rows) S(i)" > /dev/null 2>&1=0A=0A psql= test -c "vacuum analyze t" > /dev/null 2>&1=0A=0A psql test -c "checkp= oint" > /dev/null 2>&1=0A=0A psql test > tmp.log <