Обсуждение: A separate table level option to control compression

On 2/6/19 2:32 AM, Pavan Deolasee wrote:
> Hello,
>
> Currently either the table level option `toast_tuple_target` or the
> compile time default `TOAST_TUPLE_TARGET` is used to decide whether a
> new tuple should be compressed or not. While this works reasonably
> well for most situations, at times the user may not want to pay the
> overhead of toasting, yet take benefits of inline compression.
>
> I would like to propose a new table level option,
> compress_tuple_target, which can be set independently of
> toast_tuple_target, and is checked while deciding whether to compress
> the new tuple or not.
>
> For example,
>
> CREATE TABLE compresstest250 (a int, b text) WITH
> (compress_tuple_target = 250);
> CREATE TABLE compresstest2040 (a int, b text) WITH
> (compress_tuple_target = 2040);
>
> -- shouldn't get compressed nor toasted
> INSERT INTO compresstest250 VALUES (1, repeat('1234567890',20));
>
> -- should get compressed, but not toasted
> INSERT INTO compresstest250 VALUES (2, repeat('1234567890',30));
>
> -- shouldn't get compressed nor toasted
> INSERT INTO compresstest2040 VALUES (1, repeat('1234567890',20));
> INSERT INTO compresstest2040 VALUES (2, repeat('1234567890',30));
>
> Without this patch, the second INSERT will not compress the tuple
> since its length is less than the toast threshold. With the patch and
> after setting table level option, one can compress such tuples.
>
> The attached patch implements this idea. 
>


This is a nice idea, and I'm a bit surprised it hasn't got more
attention. The patch itself seems very simple and straightforward,
although it could probably do with having several sets of eyeballs on it.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Mar 5, 2019 at 5:30 PM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:
> This is a nice idea, and I'm a bit surprised it hasn't got more
> attention. The patch itself seems very simple and straightforward,
> although it could probably do with having several sets of eyeballs on it.

I haven't needed this for anything, but it seems reasonable to me.
The documentation seems to need some wordsmithing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Feb 6, 2019 at 4:32 PM Pavan Deolasee <pavan.deolasee@gmail.com> wrote:
>
> Hello,
>
> Currently either the table level option `toast_tuple_target` or the compile time default `TOAST_TUPLE_TARGET` is used
todecide whether a new tuple should be compressed or not. While this works reasonably well for most situations, at
timesthe user may not want to pay the overhead of toasting, yet take benefits of inline compression. 
>
> I would like to propose a new table level option, compress_tuple_target, which can be set independently of
toast_tuple_target,and is checked while deciding whether to compress the new tuple or not. 
>
> For example,
>
> CREATE TABLE compresstest250 (a int, b text) WITH (compress_tuple_target = 250);
> CREATE TABLE compresstest2040 (a int, b text) WITH (compress_tuple_target = 2040);
>
> -- shouldn't get compressed nor toasted
> INSERT INTO compresstest250 VALUES (1, repeat('1234567890',20));
>
> -- should get compressed, but not toasted
> INSERT INTO compresstest250 VALUES (2, repeat('1234567890',30));
>
> -- shouldn't get compressed nor toasted
> INSERT INTO compresstest2040 VALUES (1, repeat('1234567890',20));
> INSERT INTO compresstest2040 VALUES (2, repeat('1234567890',30));
>
> Without this patch, the second INSERT will not compress the tuple since its length is less than the toast threshold.
Withthe patch and after setting table level option, one can compress such tuples. 
>
> The attached patch implements this idea.
>

I like this idea.

The patch seems to need update the part describing on-disk toast
storage in storage.sgml.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On 3/11/19 2:23 AM, Masahiko Sawada wrote:
> On Wed, Feb 6, 2019 at 4:32 PM Pavan Deolasee <pavan.deolasee@gmail.com> wrote:
>> Hello,
>>
>> Currently either the table level option `toast_tuple_target` or the compile time default `TOAST_TUPLE_TARGET` is
usedto decide whether a new tuple should be compressed or not. While this works reasonably well for most situations, at
timesthe user may not want to pay the overhead of toasting, yet take benefits of inline compression.
 
>>
>> I would like to propose a new table level option, compress_tuple_target, which can be set independently of
toast_tuple_target,and is checked while deciding whether to compress the new tuple or not.
 
>>
>> For example,
>>
>> CREATE TABLE compresstest250 (a int, b text) WITH (compress_tuple_target = 250);
>> CREATE TABLE compresstest2040 (a int, b text) WITH (compress_tuple_target = 2040);
>>
>> -- shouldn't get compressed nor toasted
>> INSERT INTO compresstest250 VALUES (1, repeat('1234567890',20));
>>
>> -- should get compressed, but not toasted
>> INSERT INTO compresstest250 VALUES (2, repeat('1234567890',30));
>>
>> -- shouldn't get compressed nor toasted
>> INSERT INTO compresstest2040 VALUES (1, repeat('1234567890',20));
>> INSERT INTO compresstest2040 VALUES (2, repeat('1234567890',30));
>>
>> Without this patch, the second INSERT will not compress the tuple since its length is less than the toast threshold.
Withthe patch and after setting table level option, one can compress such tuples.
 
>>
>> The attached patch implements this idea.
>>
> I like this idea.
>
> The patch seems to need update the part describing on-disk toast
> storage in storage.sgml.
>


Yeah. Meanwhile, here's a rebased version of the patch to keep the cfbot
happy.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Pavan,

On 3/12/19 4:38 PM, Andrew Dunstan wrote:
> On 3/11/19 2:23 AM, Masahiko Sawada wrote:

>> I like this idea.
>>
>> The patch seems to need update the part describing on-disk toast
>> storage in storage.sgml.
> 
> Yeah. Meanwhile, here's a rebased version of the patch to keep the cfbot
> happy.

Looks like you need to update the documentation in storage.sgml?

Regards,
-- 
-David
david@pgmasters.net

Jumping in here, please be gentle. :)

Contents & Purpose
==================

This appears to be a patch to add a new table storage option similar to
`toast_tuple_target` but geared toward compression. As a result, it's been
named `compress_tuple_target`, and allows modifying the threshold where
inline tuples actually become compressed. If we're going to make the toast
threshold configurable, it tends to make sense we'd do the same for the
compression threshold.

The patch includes necessary documentation to describe the storage parameter
along with limitations and fallback operating modes. Several tests are also
included.

Verification Procedure
======================

The patch applied clean to HEAD, which was at commit 28988a84c... by Peter
Eisentraut, at the time of this review.

Build succeeded without issue, as did `make check` and `make installcheck`.
In addition, I also performed the following manual verification steps
using table page count and `pgstattuple` page distribution for a 10-row
table with a junk column in these scenarios:

* A standard table with a 1000-byte junk column not using this option:
  2 pages at 66% density
* A table with a 1000-byte junk and `compress_tuple_target` set to 512:
  1 page at 6% density; the low threshold activated compression
* A table with a 8120-byte junk and `compress_tuple_target` +
  `toast_tuple_target` set to 8160. Further, junk column was set to
  `main` storage to prevent standard toast thresholds from interfering:
  10 pages at 99.5% density; no compression activated despite large column
* A table with a 8140-byte junk and `compress_tuple_target` +
  `toast_tuple_target` set to 8180. Further, junk column was set to
  `main` storage to prevent standard toast thresholds from interfering:
  1 page at 16% density; compression threshold > 8160 ignored as documented.
  Additionally, neither `compress_tuple_target` or `toast_tuple_target`
  were saved in `pg_class`.

I also performed a `pg_dump` of a table using `compress_tuple_target`,
and dump faithfully preserved the option in the expected location before
the DATA portion.

Discussion
==========

Generally this ended about as I expected. I suspect much of the existing
code was cribbed from the implementation of `toast_tuple_target` given
the similar entrypoints and the already existing hard-coded compression
thresholds.

I can't really speak for the discussion related to `storage.sgml`, but
I based my investigation on the existing patch to `create_table.sgml`.
About the only thing I would suggest there is to possibly tweak the
wording.

* "The compress_tuple_target ... " for example should probably read
  "The toast_tuple_target parameter ...".
* "the (blocksize - header)" can drop "the".
* "If the value is set to a value" redundant wording should be rephrased;
  "If the specified value is greater than toast_tuple_target, then
  we will substitute the current setting of toast_tuple_target instead."
  would work.
* I'd recommend a short discussion on what negative consequences can be
  expected by playing with this value. As an example in my tests, setting
  it very high may result in extremely sparse pages that could have an
  adverse impact on HOT updates.

Still, those are just minor nitpicks, and I don't expect that to affect the
quality of the patch implementation.

Good show, gents!

-- 
Shaun M Thomas - 2ndQuadrant
PostgreSQL Training, Services and Support
shaun.thomas@2ndquadrant.com | www.2ndQuadrant.com

> Setting compress_tuple_target to a higher value won't be negative because the
> toast_tuple_target is used for compression anyways when compress_tuple_target
> is higher than toast_tuple_target.

Ack, you're right. Got my wires crossed. I must have gotten confused by my later
tests that enforced main storage and pushed out the toast tuple target
to prevent
out-of-line storage.

> I added a small discussion about negative effects of setting  compress_tuple_target
> lower though, per your suggestion.

Fair enough. I like the addition since it provides a very concrete
reason not to abuse
the parameter. :shipit:

Vaya con Queso

-- 
Shaun M Thomas - 2ndQuadrant
PostgreSQL Training, Services and Support
shaun.thomas@2ndquadrant.com | www.2ndQuadrant.com

On Wed, Mar 27, 2019 at 3:38 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Mar 21, 2019 at 10:40 PM Pavan Deolasee
> <pavan.deolasee@gmail.com> wrote:
> >
> > Hi,
> >
> > On Thu, Mar 21, 2019 at 3:10 AM Shaun Thomas <shaun.thomas@2ndquadrant.com> wrote:
> >>
> >>
> >> I can't really speak for the discussion related to `storage.sgml`, but
> >> I based my investigation on the existing patch to `create_table.sgml`.
> >> About the only thing I would suggest there is to possibly tweak the
> >> wording.
> >>
> >> * "The compress_tuple_target ... " for example should probably read
> >>   "The toast_tuple_target parameter ...".
> >> * "the (blocksize - header)" can drop "the".
> >> * "If the value is set to a value" redundant wording should be rephrased;
> >>   "If the specified value is greater than toast_tuple_target, then
> >>   we will substitute the current setting of toast_tuple_target instead."
> >>   would work.
> >
> >
> > Thanks Shaun. Attached patch makes these adjustments.
> >
> >>
> >> * I'd recommend a short discussion on what negative consequences can be
> >>   expected by playing with this value. As an example in my tests, setting
> >>   it very high may result in extremely sparse pages that could have an
> >>   adverse impact on HOT updates.
> >
> >
> > Setting compress_tuple_target to a higher value won't be negative because the toast_tuple_target is used for
compressionanyways when  compress_tuple_target is higher than toast_tuple_target. May be some discussion in the
paragraphrelated to toast_tuple_target can be added to explain the negative impact of the high value. 
> >
> > I added a small discussion about negative effects of setting  compress_tuple_target lower though, per your
suggestion.
> >
> > Also added some details in storage.sgml as recommended by Sawada-san. Hope this helps.
> >
>
> Thank you for updating the patch!
>
> The patch looks good to me but the <para> of the following change
> seems to break indents a bit. Please check it.
>
> +    <term><literal>compress_tuple_target</literal>
> (<type>integer</type>)</term>
> +    <listitem>
> +     <para>
> +     The compress_tuple_target parameter specifies the minimum tuple
> length required before
> +     we try to compress columns marked as Extended or Main and applies only to
> +     new tuples - there is no effect on existing rows.
> +     By default this parameter is set to allow at least 4 tuples per block,
> +     which with the default blocksize will be 2040 bytes. Valid values are
> +     between 128 bytes and (blocksize - header), by default 8160 bytes.
> +     If the specified value is greater than
> +     <literal>toast_tuple_target</literal>, then
> +     we will use the current setting of <literal>toast_tuple_target</literal>
> +     for <literal>compress_tuple_target</literal>.
> +     Note that the default setting is often close to optimal. If the value is
> +     set too low then the <acronym>TOAST</acronym> may get invoked too often.
> +     If the compressibility of
> +     the field values is not good, then compression and decompression can add
> +     significant computation overhead without corresponding savings in storage
> +     consumption.
> +    </para>
> +    <para>
> +     This parameter cannot be set for TOAST tables.
> +     </para>
> +    </listitem>
> +   </varlistentry>
>
> If there is no comment from other reviews, I'll mark this patch as
> Ready for Committer.
>

Marked.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Tue, Apr 02, 2019 at 11:37:56AM +0900, Masahiko Sawada wrote:
> Marked.

+   compress_tuple_threshold = RelationGetCompressTupleTarget(relation,
+                               toast_tuple_threshold);
+   compress_tuple_threshold = Min(compress_tuple_threshold,
+                                  toast_tuple_threshold);
All the callers of RelationGetCompressTupleTarget directly compile the
minimum between compress_tuple_threshold and toast_tuple_threshold,
and also call RelationGetToastTupleTarget() beforehand.  Wouldn't it
be better to merge all that in a common routine?  The same calculation
method is duplicated 5 times.

+   /*
+    * Get the limit at which we should apply compression. This will be same as
+    * maxDataLen unless overridden by the user explicitly.
+    */
+   maxCompressLen = RelationGetCompressTupleTarget(rel, maxDataLen) - hoff;

Is that fine?  hoff gets substracted twice.

+     This parameter cannot be set for TOAST tables.
Missing <acronym> markup here for TOAST.

The format of the whole new documentation paragraph is a bit weird.
Could it be possible to adjust it 72-80 characters per line?

The same boundary logic applies to compress_tuple_target and
toast_tuple_target.  Perhaps it would make sense to remove the 4-tuple
limit-per-block from the new paragraph, and mention that the same
limits as for toast_tuple_target apply to compress_tuple_target.

+-- expect 0 blocks
+select 0 = pg_relation_size('pg_toast.pg_toast_'||(select oid from
pg_class where relname =
'compresstest250'))/current_setting('block_size')::integer as blocks;
+select 0 = pg_relation_size('pg_toast.pg_toast_'||(select oid from
pg_class where relname =
'compresstest2040'))/current_setting('block_size')::integer as blocks;

This is unreadable.  Cannot pg_class.reltoastrelid be used instead?
--
Michael

On Tue, Apr 02, 2019 at 02:35:19PM +0900, Michael Paquier wrote:
> +   compress_tuple_threshold = RelationGetCompressTupleTarget(relation,
> +                               toast_tuple_threshold);
> +   compress_tuple_threshold = Min(compress_tuple_threshold,
> +                                  toast_tuple_threshold);
> All the callers of RelationGetCompressTupleTarget directly compile the
> minimum between compress_tuple_threshold and toast_tuple_threshold,
> and also call RelationGetToastTupleTarget() beforehand.  Wouldn't it
> be better to merge all that in a common routine?  The same calculation
> method is duplicated 5 times.

I have been looking at this patch more, and here are some notes:
- The tests can be really simplified using directly reltoastrelid, so
I changed the queries this way.  I am aware that the surroundings
hardcode directly the relation name, but that's not really elegant in
my opinion.  And I am really tempted to adjust these as well to
directly use reltoastrelid.
- The docs had a weird indentation.
- I think that we should be careful with the portability of
pg_column_size(), so I have added comparisons instead of the direct
values in a way which does not change the meaning of the tests nor
their coverage.
- Having RelationGetCompressTupleTarget use directly
toast_tuple_threshold as default argument is I think kind of
confusing, so let's use a different static value, named
COMPRESS_TUPLE_TARGET in the attached.  This is similar to
TOAST_TUPLE_TARGET for the toast tuple threshold.
- The comments in tuptoaster.h need to be updated to outline the
difference between the compression invocation and the toast invocation
thresholds.  The wording could be better though.
- Better to avoid comments in the middle of the else/if blocks in my
opinion.

Also, the previous versions of the patch do that when doing a heap
insertion (heapam.c and rewriteheap.c):
+    toast_tuple_threshold = RelationGetToastTupleTarget(relation,
+                                    TOAST_TUPLE_THRESHOLD);
+        compress_tuple_threshold = RelationGetCompressTupleTarget(relation,
+                                    toast_tuple_threshold);
+        compress_tuple_threshold = Min(compress_tuple_threshold,
+                                       toast_tuple_threshold);
[...]
    need_toast = (HeapTupleHasExternal(&oldtup) ||
                  HeapTupleHasExternal(newtup) ||
-                newtup->t_len > TOAST_TUPLE_THRESHOLD);
+                newtup->t_len > compress_tuple_threshold);

This means that the original code always uses the compilation-time,
default value of toast_tuple_target for all relations.  But this gets
changed so as we would use the value set at relation level for
toast_tuple_target if the reloption is changed, without touching
compress_tuple_threshold.  This is out of the scope of this patch, but
shouldn't we always use the relation-level value instead of the
compiled one?  Perhaps there is something I am missing?
--
Michael

On Wed, Apr 03, 2019 at 11:40:57AM +0530, Pavan Deolasee wrote:
> On Wed, Apr 3, 2019 at 10:19 AM Michael Paquier <michael@paquier.xyz> wrote:
>> - The comments in tuptoaster.h need to be updated to outline the
>> difference between the compression invocation and the toast invocation
>> thresholds.  The wording could be better though.
>
> I see that you've done this already. But please let me if more is needed.

If you have a better idea of wording for that part...  Please feel
free.

> Yeah, this is an issue with the existing code. Even though we allow setting
> toast_tuple_target to a value less than compile-time TOAST_TUPLE_THRESHOLD,
> the code doesn't really honour a value less than TOAST_TUPLE_THRESHOLD. In
> other words, setting toast_tuple_target lesser than TOAST_TUPLE_THRESHOLD
> doesn't have any effect. We don't even create a toast table if the
> estimated length of tuple is not greater than TOAST_TUPLE_THRESHOLD.
>
> The change introduced by this patch will now trigger the tuptoaster code
> when the compression or toast threshold is set to a value lower than
> TOAST_TUPLE_THRESHOLD. But as far as I can see, that doesn't have any bad
> effect on the toasting since toast_insert_or_update() is capable of
> handling the case when the toast table is missing. There will be a
> behavioural change though. e.g.

It seems to me that c251336 should have done all those things from the
start...  In other terms, isn't that a bug and something that we
should fix and back-patch?  I'll begin a new thread about that to
catch more attention, with Simon and Andrew in CC.
--
Michael

On Wed, Apr 03, 2019 at 03:23:33PM +0900, Michael Paquier wrote:
> It seems to me that c251336 should have done all those things from the
> start...  In other terms, isn't that a bug and something that we
> should fix and back-patch?  I'll begin a new thread about that to
> catch more attention, with Simon and Andrew in CC.

For what it's worth, I have dropped a new thread on the matter here:
https://www.postgresql.org/message-id/20190403063759.GF3298@paquier.xyz

It seems to me that it is sensible to conclude on the other thread
first before acting on what is proposed here.  As we are only a couple
of days away from the feature freeze, are there any objections to mark
this patch as returned with feedback?
--
Michael

On Fri, Apr 5, 2019 at 2:58 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Apr 03, 2019 at 03:23:33PM +0900, Michael Paquier wrote:
> > It seems to me that c251336 should have done all those things from the
> > start...  In other terms, isn't that a bug and something that we
> > should fix and back-patch?  I'll begin a new thread about that to
> > catch more attention, with Simon and Andrew in CC.
>
> For what it's worth, I have dropped a new thread on the matter here:
> https://www.postgresql.org/message-id/20190403063759.GF3298@paquier.xyz
>
> It seems to me that it is sensible to conclude on the other thread
> first before acting on what is proposed here.  As we are only a couple
> of days away from the feature freeze, are there any objections to mark
> this patch as returned with feedback?


Well, that would be a bit sad. ISTM if we conclude that the current
behaviour is a bug we could commit the current patch and backpatch a
fix to honor a lower toast_tuple_threshold. But yes, time is tight.

cheers

andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Apr 05, 2019 at 09:10:31AM -0400, Andrew Dunstan wrote:
> Well, that would be a bit sad. ISTM if we conclude that the current
> behaviour is a bug we could commit the current patch and backpatch a
> fix to honor a lower toast_tuple_threshold. But yes, time is tight.

48 hours remain, which is very tight.  Let's see but the chances are
small :(

If we think that lowering toast_tuple_threshold should be supported
then the patch on the other thread should be used first, perhaps
back-patched (it lacks pieces with ALTER TABLE as pointed out as
well).  If we don't use the other patch, then what's proposed on this
thread is actually wrong and should be reworked.  In any case,
something is wrong.
--
Michael

On Sat, Apr 6, 2019 at 3:18 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Apr 05, 2019 at 09:10:31AM -0400, Andrew Dunstan wrote:
> > Well, that would be a bit sad. ISTM if we conclude that the current
> > behaviour is a bug we could commit the current patch and backpatch a
> > fix to honor a lower toast_tuple_threshold. But yes, time is tight.
>
> 48 hours remain, which is very tight.  Let's see but the chances are
> small :(
>
> If we think that lowering toast_tuple_threshold should be supported
> then the patch on the other thread should be used first, perhaps
> back-patched (it lacks pieces with ALTER TABLE as pointed out as
> well).  If we don't use the other patch, then what's proposed on this
> thread is actually wrong and should be reworked.  In any case,
> something is wrong.

Yeah, I'd hoped for some more opinions. I agree we've run out of time :-(

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Apr 06, 2019 at 09:18:18PM -0400, Andrew Dunstan wrote:
> Yeah, I'd hoped for some more opinions. I agree we've run out of time :-(

We are a couple of hours away from the freeze, so I have marked the
patch as returned with feedback.  Let's see if we can still do
something for the other item in v11 or even only v12, still this is
not likely an issue which would prevent v12 to be released.
--
Michael