Horton

Thread

Commits

  1. Remove forced toast recompression in VACUUM FULL/CLUSTER

  2. Rethink definition of pg_attribute.attcompression.

  3. Fix memory leak when de-toasting compressed values in VACUUM FULL/CLUSTER

  4. Re-order pg_attribute columns to eliminate some padding space.

  5. Add more TAP tests for pg_dump with attribute compression

  1. Move pg_attribute.attcompression to earlier in struct for reduced size?

    Andres Freund <andres@anarazel.de> — 2021-05-17T20:48:03Z

    Hi,

pg_attribute is one of the biggest table in a new cluster, and often the
biggest table in production clusters. Its size is also quite relevant in
memory, due to all the TupleDescs we allocate.

I just noticed that the new attcompression increased the size not just
by 1 byte, but by 4, due to padding. While an increase from 112 to 116
bytes isn't the end of the world, it does seem worth considering using
existing unused bytes instead?

If we moved attcompression to all the other bool/char fields, we'd avoid
that size increase, as there's an existing 2 byte hole.

Of course there's the argument that we shouldn't change the column order
for existing SELECT * queries, but the existing placement already does
(the CATALOG_VARLEN columns follow).

Greetings,

Andres Freund

  2. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Justin Pryzby <pryzby@telsasoft.com> — 2021-05-17T21:05:44Z

    On Mon, May 17, 2021 at 01:48:03PM -0700, Andres Freund wrote:
> pg_attribute is one of the biggest table in a new cluster, and often the
> biggest table in production clusters. Its size is also quite relevant in
> memory, due to all the TupleDescs we allocate.
> 
> I just noticed that the new attcompression increased the size not just
> by 1 byte, but by 4, due to padding. While an increase from 112 to 116
> bytes isn't the end of the world, it does seem worth considering using
> existing unused bytes instead?

+1

FYI: attcompression was an OID until a few weeks before the feature was merged,
and there were several issues related to that:
  aa25d1089 - fixed two issues
  226e2be38

-- 
Justin

  3. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-17T21:06:32Z

    Andres Freund <andres@anarazel.de> writes:
> If we moved attcompression to all the other bool/char fields, we'd avoid
> that size increase, as there's an existing 2 byte hole.

+1.  Looks to me like its existing placement was according to the good
old "add new things at the end" anti-pattern.  It certainly isn't
related to the adjacent fields.

Putting it just after attalign seems like a reasonably sane choice
from the standpoint of grouping things affecting physical storage;
and as you say, that wins from the standpoint of using up alignment
padding rather than adding more.

Personally I'd think the most consistent order in that area would
be attbyval, attalign, attstorage, attcompression; but perhaps it's
too late to swap the order of attstorage and attalign.

			regards, tom lane

  4. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Andres Freund <andres@anarazel.de> — 2021-05-17T21:28:57Z

    Hi,

On 2021-05-17 17:06:32 -0400, Tom Lane wrote:
> Putting it just after attalign seems like a reasonably sane choice
> from the standpoint of grouping things affecting physical storage;
> and as you say, that wins from the standpoint of using up alignment
> padding rather than adding more.

Makes sense to me.


> Personally I'd think the most consistent order in that area would
> be attbyval, attalign, attstorage, attcompression; but perhaps it's
> too late to swap the order of attstorage and attalign.

Given that we've put in new fields in various positions on a fairly
regular basis, I don't think swapping around attalign, attstorage would
cause a meaningful amount of additional pain.  Personally I don't have a
preference for how these are ordered.

Greetings,

Andres Freund

  5. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-05-18T01:24:36Z

    On Mon, May 17, 2021 at 02:28:57PM -0700, Andres Freund wrote:
> On 2021-05-17 17:06:32 -0400, Tom Lane wrote:
>> Putting it just after attalign seems like a reasonably sane choice
>> from the standpoint of grouping things affecting physical storage;
>> and as you say, that wins from the standpoint of using up alignment
>> padding rather than adding more.
> 
> Makes sense to me.

+1.

>> Personally I'd think the most consistent order in that area would
>> be attbyval, attalign, attstorage, attcompression; but perhaps it's
>> too late to swap the order of attstorage and attalign.
> 
> Given that we've put in new fields in various positions on a fairly
> regular basis, I don't think swapping around attalign, attstorage would
> cause a meaningful amount of additional pain.  Personally I don't have a
> preference for how these are ordered.

If you switch attcompression, I'd say to go for the others while on
it.  It would not be the first time in history there is a catalog
version bump between betas.
--
Michael

  6. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-05-21T06:32:05Z

    On Tue, May 18, 2021 at 10:24:36AM +0900, Michael Paquier wrote:
> If you switch attcompression, I'd say to go for the others while on
> it.  It would not be the first time in history there is a catalog
> version bump between betas.

This is still an open item.  FWIW, I can get behind the reordering
proposed by Tom for the consistency gained with pg_type, leading to
the attached to reduce the size of FormData_pg_attribute from 116b to
112b.
--
Michael

  7. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-21T06:55:11Z

    On Fri, May 21, 2021 at 12:02 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, May 18, 2021 at 10:24:36AM +0900, Michael Paquier wrote:
> > If you switch attcompression, I'd say to go for the others while on
> > it.  It would not be the first time in history there is a catalog
> > version bump between betas.
>
> This is still an open item.  FWIW, I can get behind the reordering
> proposed by Tom for the consistency gained with pg_type, leading to
> the attached to reduce the size of FormData_pg_attribute from 116b to
> 112b.

This makes sense, thanks for working on this.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  8. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-21T15:01:03Z

    Michael Paquier <michael@paquier.xyz> writes:
> This is still an open item.  FWIW, I can get behind the reordering
> proposed by Tom for the consistency gained with pg_type, leading to
> the attached to reduce the size of FormData_pg_attribute from 116b to
> 112b.

I think we need to do more than that.  It's certainly not okay to
leave catalogs.sgml out of sync with reality.  And maybe I'm just
an overly anal-retentive sort, but I think that code that manipulates
tuples ought to match the declared field order if there's not some
specific reason to do otherwise.  So that led me to the attached.

It was a good thing I went through this code, too, because I noticed
one serious bug (attcompression not checked in equalTupleDescs) and
another thing that looks like a bug: there are two places that set
up attcompression depending on

    if (rel->rd_rel->relkind == RELKIND_RELATION ||
        rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)

This seems fairly nuts; in particular, why are matviews excluded?

			regards, tom lane

  9. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Andres Freund <andres@anarazel.de> — 2021-05-21T20:54:20Z

    Hi,

On 2021-05-21 11:01:03 -0400, Tom Lane wrote:
> It was a good thing I went through this code, too, because I noticed
> one serious bug (attcompression not checked in equalTupleDescs) and
> another thing that looks like a bug: there are two places that set
> up attcompression depending on
>
>     if (rel->rd_rel->relkind == RELKIND_RELATION ||
>         rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
>
> This seems fairly nuts; in particular, why are matviews excluded?

Yea, that doesn't seem right. I was confused why this appears to work at
all right now. It only does because REFRESH always inserts into a
staging table first - which is created as a normal table.  For
non-concurrent refresh that relation's relfilenode is swapped with the
MV's. For concurrent refresh we actually do insert into the MV - but we
never need to compress a datum at that point, because it'll already have
been compressed during the insert into the temp table.

I think there might something slightly off with concurrent refresh - the
TEMPORARY diff table that is created doesn't use the matview's
compression settings. Which means all tuples need to be recompressed
unnecessarily, if default_toast_compression differs from a column in the
materialized view.

SET default_toast_compression = 'lz4';
DROP MATERIALIZED VIEW IF EXISTS wide_mv;
CREATE MATERIALIZED VIEW wide_mv AS SELECT 1::int4 AS key, random() || string_agg(i::text, '') data FROM generate_series(1, 10000) g(i);CREATE UNIQUE INDEX ON wide_mv(key);
ALTER MATERIALIZED VIEW wide_mv ALTER COLUMN data SET COMPRESSION pglz;
REFRESH MATERIALIZED VIEW CONCURRENTLY wide_mv;

With the SET COMPRESSION pglz I see the following compression calls:
1) pglz in refresh_matview_datafill
2) lz4 during temp table CREATE TEMP TABLE AS
3) pglz during the INSERT into the matview

Without I only see 1) and 2).

Greetings,

Andres Freund

  10. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Andres Freund <andres@anarazel.de> — 2021-05-21T21:19:29Z

    Hi,

On 2021-05-21 11:01:03 -0400, Tom Lane wrote:
> It was a good thing I went through this code, too, because I noticed
> one serious bug (attcompression not checked in equalTupleDescs) and
> another thing that looks like a bug:

Grepping for attcompression while trying to understand the issue Tom
reported I found a substantial, but transient, memory leak:

During VACUUM FULL reform_and_rewrite_tuple() detoasts the old value if
it was compressed with a different method, while in
TopTransactionContext. There's nothing freeing that until
TopTransactionContext ends - obviously not great for a large relation
being VACUUM FULLed.

SET default_toast_compression = 'lz4';
DROP TABLE IF EXISTS wide CASCADE;
CREATE TABLE wide(data text not null);
INSERT INTO wide(data) SELECT random() || (SELECT string_agg(i::text, '') data FROM generate_series(1, 100000) g(i)) FROM generate_series(1, 1000);

\c

SET client_min_messages = 'log';
SET log_statement_stats = on;
VACUUM FULL wide;
...
DETAIL:  ! system usage stats:
!	0.836638 s user, 0.375344 s system, 1.268705 s elapsed
!	[2.502369 s user, 0.961681 s system total]
!	18052 kB max resident size
!	0/1789088 [0/3530048] filesystem blocks in/out
!	0/277 [0/205655] page faults/reclaims, 0 [0] swaps
!	0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
!	22/1 [55/6] voluntary/involuntary context switches
LOCATION:  ShowUsage, postgres.c:4886
VACUUM
Time: 1269.029 ms (00:01.269)

\c
ALTER TABLE wide ALTER COLUMN data SET COMPRESSION pglz;
SET client_min_messages = 'log';
SET log_statement_stats = on;
VACUUM FULL wide;
...
DETAIL:  ! system usage stats:
!	19.816867 s user, 0.493233 s system, 20.320711 s elapsed
!	[19.835995 s user, 0.493233 s system total]
!	491588 kB max resident size
!	0/656032 [0/656048] filesystem blocks in/out
!	0/287363 [0/287953] page faults/reclaims, 0 [0] swaps
!	0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
!	1/24 [13/26] voluntary/involuntary context switches

Note the drastically different "max resident size". This is with huge
pages (removing s_b from RSS), but it's visible even without.


Random fun note:
time for VACUUM FULL wide with recompression:
pglz->lz4: 3.2s
lz4->pglz: 20.3s

Greetings,

Andres Freund

  11. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-23T16:25:10Z

    I wrote:
> I think we need to do more than that.  It's certainly not okay to
> leave catalogs.sgml out of sync with reality.  And maybe I'm just
> an overly anal-retentive sort, but I think that code that manipulates
> tuples ought to match the declared field order if there's not some
> specific reason to do otherwise.  So that led me to the attached.

Pushed that after another round of review.

> ... there are two places that set
> up attcompression depending on
>     if (rel->rd_rel->relkind == RELKIND_RELATION ||
>         rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> This seems fairly nuts; in particular, why are matviews excluded?

While I've not actually tested this, it seems to me that we could
just drop these relkind tests altogether.  It won't hurt anything
to set up attcompression in relation descriptors where it'll never
be consulted.

However, the more I looked at that code the less I liked it.
I think the way that compression selection is handled for indexes,
ie consult default_toast_compression on-the-fly, is *far* saner
than what is currently implemented for tables.  So I think we
should redefine attcompression as "ID of a compression method
to use, or \0 to select the prevailing default.  Ignored if
attstorage does not permit the use of compression".  This would
result in approximately 99.44% of all columns just having zero
attcompression, greatly simplifying the tupdesc setup code, and
also making it much easier to flip an installation over to a
different preferred compression method.

I'm happy to prepare a patch if that sketch sounds sane.

(Note that the existing comment claiming that attcompression
"Must be InvalidCompressionMethod if and only if typstorage is
'plain' or 'external'" is a flat out lie in any case; *both*
directions of that claim are wrong.)

			regards, tom lane

  12. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-05-24T04:09:18Z

    On Fri, May 21, 2021 at 02:19:29PM -0700, Andres Freund wrote:
> During VACUUM FULL reform_and_rewrite_tuple() detoasts the old value if
> it was compressed with a different method, while in
> TopTransactionContext. There's nothing freeing that until
> TopTransactionContext ends - obviously not great for a large relation
> being VACUUM FULLed.

Yeah, that's not good.  The confusion comes from the fact that we'd
just overwrite the values without freeing them out if recompressed, so
something like the attached would be fine?
--
Michael

  13. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-05-24T04:42:26Z

    On Sun, May 23, 2021 at 12:25:10PM -0400, Tom Lane wrote:
> While I've not actually tested this, it seems to me that we could
> just drop these relkind tests altogether.  It won't hurt anything
> to set up attcompression in relation descriptors where it'll never
> be consulted.

Wouldn't it be confusing to set up attcompression for relkinds without
storage, like views?

> However, the more I looked at that code the less I liked it.
> I think the way that compression selection is handled for indexes,
> ie consult default_toast_compression on-the-fly, is *far* saner
> than what is currently implemented for tables.  So I think we
> should redefine attcompression as "ID of a compression method
> to use, or \0 to select the prevailing default.  Ignored if
> attstorage does not permit the use of compression".  This would
> result in approximately 99.44% of all columns just having zero
> attcompression, greatly simplifying the tupdesc setup code, and
> also making it much easier to flip an installation over to a
> different preferred compression method.

Would there be any impact when it comes to CTAS or matviews where the
current code assumes that the same compression method as the one from
the original value gets used, making the creation of the new relation
cheaper because there is less de-toasting and re-toasting?
--
Michael

  14. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-24T05:05:07Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Sun, May 23, 2021 at 12:25:10PM -0400, Tom Lane wrote:
>> While I've not actually tested this, it seems to me that we could
>> just drop these relkind tests altogether.  It won't hurt anything
>> to set up attcompression in relation descriptors where it'll never
>> be consulted.

> Wouldn't it be confusing to set up attcompression for relkinds without
> storage, like views?

No more so than setting up attstorage, surely.

>> ... I think we
>> should redefine attcompression as "ID of a compression method
>> to use, or \0 to select the prevailing default.  Ignored if
>> attstorage does not permit the use of compression".  This would
>> result in approximately 99.44% of all columns just having zero
>> attcompression, greatly simplifying the tupdesc setup code, and
>> also making it much easier to flip an installation over to a
>> different preferred compression method.

> Would there be any impact when it comes to CTAS or matviews where the
> current code assumes that the same compression method as the one from
> the original value gets used, making the creation of the new relation
> cheaper because there is less de-toasting and re-toasting?

I'd still envision copying the source attcompression setting in such
cases.  I guess the question is (a) does that code path actually
recompress values that have the "wrong" compression, and (b) if it
does, is that wrong?  If you think (a) is correct behavior, then
I don't see why refreshing after changing default_toast_compression
shouldn't cause that to happen.

			regards, tom lane

  15. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-24T05:55:18Z

    On Fri, May 21, 2021 at 8:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>
>     if (rel->rd_rel->relkind == RELKIND_RELATION ||
>         rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
>
> This seems fairly nuts; in particular, why are matviews excluded?

The matviews are excluded only in "ATExecAddColumn()" right?  But we
can not ALTER TABLE ADD COLUMN to matviews right?  I agree that even
if we don't skip matview it will not create any issue as matview will
not reach here.  Am I missing something?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  16. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-24T06:02:22Z

    On Mon, May 24, 2021 at 9:39 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, May 21, 2021 at 02:19:29PM -0700, Andres Freund wrote:
> > During VACUUM FULL reform_and_rewrite_tuple() detoasts the old value if
> > it was compressed with a different method, while in
> > TopTransactionContext. There's nothing freeing that until
> > TopTransactionContext ends - obviously not great for a large relation
> > being VACUUM FULLed.
>
> Yeah, that's not good.  The confusion comes from the fact that we'd
> just overwrite the values without freeing them out if recompressed, so
> something like the attached would be fine?

  /* Be sure to null out any dropped columns */
  for (i = 0; i < newTupDesc->natts; i++)
  {
+ tup_values[i] = values[i];
+
  if (TupleDescAttr(newTupDesc, i)->attisdropped)
  isnull[i] = true;

I think you don't need to initialize tup_values[i] with the
values[i];, other than that looks fine to me.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  17. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-05-24T08:53:51Z

    On Mon, May 24, 2021 at 11:32:22AM +0530, Dilip Kumar wrote:
> I think you don't need to initialize tup_values[i] with the
> values[i];, other than that looks fine to me.

You mean because heap_deform_tuple() does this job, right?  Sure.
--
Michael

  18. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-24T09:16:11Z

    On Mon, May 24, 2021 at 2:23 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, May 24, 2021 at 11:32:22AM +0530, Dilip Kumar wrote:
> > I think you don't need to initialize tup_values[i] with the
> > values[i];, other than that looks fine to me.
>
> You mean because heap_deform_tuple() does this job, right?  Sure.

Sorry, I just noticed that my statement was incomplete in last mail,
what I wanted to say is that if the attisdropped then we can avoid
"tup_values[i] = values[i]", so in short we can move "tup_values[i] =
values[i]" in the else part of " if (TupleDescAttr(newTupDesc,
i)->attisdropped)" check.

Like this.
  if (TupleDescAttr(newTupDesc, i)->attisdropped)
     isnull[i] = true;
  else
     tup_values[i] = values[i];

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  19. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-05-25T05:46:27Z

    On Mon, May 24, 2021 at 02:46:11PM +0530, Dilip Kumar wrote:
> Like this.
>   if (TupleDescAttr(newTupDesc, i)->attisdropped)
>      isnull[i] = true;
>   else
>      tup_values[i] = values[i];

That would work.  Your suggestion, as I understood it first, makes the
code simpler by not using tup_values at all as the set of values[] is
filled when the values and nulls are extracted.  So I have gone with
this simplification, and applied the patch (moved a bit the comments
while on it).
--
Michael

  20. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-25T06:15:27Z

    On Tue, 25 May 2021 at 11:16 AM, Michael Paquier <michael@paquier.xyz>
wrote:

> On Mon, May 24, 2021 at 02:46:11PM +0530, Dilip Kumar wrote:
> > Like this.
> >   if (TupleDescAttr(newTupDesc, i)->attisdropped)
> >      isnull[i] = true;
> >   else
> >      tup_values[i] = values[i];
>
> That would work.  Your suggestion, as I understood it first, makes the
> code simpler by not using tup_values at all as the set of values[] is
> filled when the values and nulls are extracted.  So I have gone with
> this simplification, and applied the patch (moved a bit the comments
> while on it).


Perfect.  That looks much better.

>

  21. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Justin Pryzby <pryzby@telsasoft.com> — 2021-05-26T01:33:47Z

    On Sun, May 23, 2021 at 12:25:10PM -0400, Tom Lane wrote:
> However, the more I looked at that code the less I liked it.
> I think the way that compression selection is handled for indexes,
> ie consult default_toast_compression on-the-fly, is *far* saner
> than what is currently implemented for tables.  So I think we
> should redefine attcompression as "ID of a compression method
> to use, or \0 to select the prevailing default.  Ignored if
> attstorage does not permit the use of compression".

+1

It reminds me of reltablespace, which is stored as 0 to mean the database's
default tablespace.

Also, values are currently retoasted during vacuum full if their column's
current compression method doesn't match the value's old compression.

But it doesn't rewrite the column if the it used to use the default
compression, and the default was changed.  I think your idea would handle that.

-- 
Justin

PS. I just ran into the memory leak that Andres reported and Michael fixed.

  22. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-05-26T01:57:00Z

    On Tue, May 25, 2021 at 08:33:47PM -0500, Justin Pryzby wrote:
> It reminds me of reltablespace, which is stored as 0 to mean the database's
> default tablespace.
> 
> Also, values are currently retoasted during vacuum full if their column's
> current compression method doesn't match the value's old compression.
> 
> But it doesn't rewrite the column if the it used to use the default
> compression, and the default was changed.  I think your idea would handle that.

Ah, the parallel with reltablespace and default_tablespace at database
level is a very good point.  It is true that currently the code would
assign attcompression to a non-zero value once the relation is defined
depending on default_toast_compression set for the database, but
setting it to 0 in this case would be really helpful to change the
compression methods of all the relations if doing something as crazy
as a VACUUM FULL for this database.  Count me as convinced.
--
Michael

  23. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-26T15:13:46Z

    Michael Paquier <michael@paquier.xyz> writes:
> Ah, the parallel with reltablespace and default_tablespace at database
> level is a very good point.  It is true that currently the code would
> assign attcompression to a non-zero value once the relation is defined
> depending on default_toast_compression set for the database, but
> setting it to 0 in this case would be really helpful to change the
> compression methods of all the relations if doing something as crazy
> as a VACUUM FULL for this database.  Count me as convinced.

Here's a draft patch series to address this.

0001 removes the relkind checks I was questioning originally.
As expected, this results in zero changes in check-world results.

0002 is the main change in the semantics of attcompression.
This does change the results of compression.sql, but in what
seem to me to be expected ways: a column's compression option
is now shown in \d+ output only if you explicitly set it.

0003 further removes pg_dump's special handling of
default_toast_compression.  I don't think we need that anymore.
AFAICS its only effect would be to override the receiving server's
default_toast_compression setting for dumped/re-loaded data, which
does not seem like a behavior that anyone would want.

Loose ends:

* I've not reviewed the docs fully; there are likely some more
things that need updated.

* As things stand here, once you've applied ALTER ... SET COMPRESSION
to select a specific method, there is no way to undo that and go
back to the use-the-default setting.  All you can do is change to
explicitly select the other method.  Should we invent "ALTER ...
SET COMPRESSION default" or the like to cover that?  (Since
DEFAULT is a reserved word, that exact syntax might be a bit of
a pain to implement, but maybe we could think of another word.)

* I find GetDefaultToastCompression() annoying.  I do not think
it is project style to invent trivial wrapper functions around
GUC variable references: it buys nothing while requiring readers
to remember one more name than they would otherwise.  Since there
are only two uses remaining, maybe this isn't very important either
way, but I'm still inclined to flush it.

Comments?

			regards, tom lane

  24. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Robert Haas <robertmhaas@gmail.com> — 2021-05-26T15:17:22Z

    On Wed, May 26, 2021 at 11:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> * As things stand here, once you've applied ALTER ... SET COMPRESSION
> to select a specific method, there is no way to undo that and go
> back to the use-the-default setting.  All you can do is change to
> explicitly select the other method.  Should we invent "ALTER ...
> SET COMPRESSION default" or the like to cover that?  (Since
> DEFAULT is a reserved word, that exact syntax might be a bit of
> a pain to implement, but maybe we could think of another word.)

Yes. Irreversible catalog changes are bad.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  25. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-26T19:31:28Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, May 26, 2021 at 11:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * As things stand here, once you've applied ALTER ... SET COMPRESSION
>> to select a specific method, there is no way to undo that and go
>> back to the use-the-default setting.  All you can do is change to
>> explicitly select the other method.  Should we invent "ALTER ...
>> SET COMPRESSION default" or the like to cover that?

> Yes. Irreversible catalog changes are bad.

Here's an add-on 0004 that does that, and takes care of assorted
silliness in the grammar and docs --- did you know that this patch
caused
	alter table foo alter column bar set ;
to be allowed?

I think this is about ready to commit now (though I didn't yet nuke
GetDefaultToastCompression).

			regards, tom lane

  26. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Justin Pryzby <pryzby@telsasoft.com> — 2021-05-26T19:32:38Z

    On Wed, May 26, 2021 at 11:13:46AM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > Ah, the parallel with reltablespace and default_tablespace at database
> > level is a very good point.  It is true that currently the code would
> > assign attcompression to a non-zero value once the relation is defined
> > depending on default_toast_compression set for the database, but
> > setting it to 0 in this case would be really helpful to change the
> > compression methods of all the relations if doing something as crazy
> > as a VACUUM FULL for this database.  Count me as convinced.
> 
> Here's a draft patch series to address this.
> 
> 0001 removes the relkind checks I was questioning originally.
> As expected, this results in zero changes in check-world results.
> 
> 0002 is the main change in the semantics of attcompression.
> This does change the results of compression.sql, but in what
> seem to me to be expected ways: a column's compression option
> is now shown in \d+ output only if you explicitly set it.
> 
> 0003 further removes pg_dump's special handling of
> default_toast_compression.  I don't think we need that anymore.
> AFAICS its only effect would be to override the receiving server's
> default_toast_compression setting for dumped/re-loaded data, which
> does not seem like a behavior that anyone would want.
> 
> Loose ends:
> 
> * I've not reviewed the docs fully; there are likely some more
> things that need updated.
> 
> * As things stand here, once you've applied ALTER ... SET COMPRESSION
> to select a specific method, there is no way to undo that and go
> back to the use-the-default setting.  All you can do is change to
> explicitly select the other method.  Should we invent "ALTER ...
> SET COMPRESSION default" or the like to cover that?  (Since
> DEFAULT is a reserved word, that exact syntax might be a bit of
> a pain to implement, but maybe we could think of another word.)
> 
> * I find GetDefaultToastCompression() annoying.  I do not think
> it is project style to invent trivial wrapper functions around
> GUC variable references: it buys nothing while requiring readers
> to remember one more name than they would otherwise.  Since there
> are only two uses remaining, maybe this isn't very important either
> way, but I'm still inclined to flush it.

+1

It existed when default_toast_compression was a text string.  Since e5595de03,
it's an enum/int/char, and serves no purpose.

-- 
Justin

  27. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-26T20:11:46Z

    I wrote:
> I think this is about ready to commit now (though I didn't yet nuke
> GetDefaultToastCompression).

Here's a bundled-up final version, in case anybody would prefer
to review it that way.

			regards, tom lane

  28. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-26T22:08:20Z

    On 2021-May-26, Tom Lane wrote:

> I wrote:
> > I think this is about ready to commit now (though I didn't yet nuke
> > GetDefaultToastCompression).
> 
> Here's a bundled-up final version, in case anybody would prefer
> to review it that way.

Looks good to me.

I tested the behavior with partitioned tables and it seems OK.

It would be good to have a test case in src/bin/pg_dump/t/002_pg_dump.pl
for the case ... and I find it odd that we don't seem to have anything
for the "CREATE TABLE foo (LIKE sometab INCLUDING stuff)" form of the
command ... but neither of those seem the fault of this patch, and they
both work as [I think] is intended.

-- 
Álvaro Herrera       Valdivia, Chile
"Estoy de acuerdo contigo en que la verdad absoluta no existe...
El problema es que la mentira sí existe y tu estás mintiendo" (G. Lama)

  29. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-26T22:21:43Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Looks good to me.
> I tested the behavior with partitioned tables and it seems OK.

Thanks for reviewing/testing!

> It would be good to have a test case in src/bin/pg_dump/t/002_pg_dump.pl
> for the case

Personally I won't touch 002_pg_dump.pl with a 10-foot pole, but if
somebody else wants to, have at it.

> ... and I find it odd that we don't seem to have anything
> for the "CREATE TABLE foo (LIKE sometab INCLUDING stuff)" form of the
> command ... but neither of those seem the fault of this patch, and they
> both work as [I think] is intended.

Hm, there's this in compression.sql:

-- test LIKE INCLUDING COMPRESSION
CREATE TABLE cmdata2 (LIKE cmdata1 INCLUDING COMPRESSION);
\d+ cmdata2

Or did you mean the case with a partitioned table specifically?

			regards, tom lane

  30. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-26T23:44:03Z

    On 2021-May-26, Tom Lane wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

> > It would be good to have a test case in src/bin/pg_dump/t/002_pg_dump.pl
> > for the case
> 
> Personally I won't touch 002_pg_dump.pl with a 10-foot pole, but if
> somebody else wants to, have at it.

Nod.

> > ... and I find it odd that we don't seem to have anything
> > for the "CREATE TABLE foo (LIKE sometab INCLUDING stuff)" form of the
> > command ... but neither of those seem the fault of this patch, and they
> > both work as [I think] is intended.
> 
> Hm, there's this in compression.sql:
> 
> -- test LIKE INCLUDING COMPRESSION
> CREATE TABLE cmdata2 (LIKE cmdata1 INCLUDING COMPRESSION);
> \d+ cmdata2
> 
> Or did you mean the case with a partitioned table specifically?

Ah, I guess that's sufficient.  (The INCLUDING clause cannot be used to
create a partition, actually.)

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"Now I have my system running, not a byte was off the shelf;
It rarely breaks and when it does I fix the code myself.
It's stable, clean and elegant, and lightning fast as well,
And it doesn't cost a nickel, so Bill Gates can go to hell."

  31. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-05-27T00:13:30Z

    On Wed, May 26, 2021 at 07:44:03PM -0400, Alvaro Herrera wrote:
> On 2021-May-26, Tom Lane wrote:
>> Personally I won't touch 002_pg_dump.pl with a 10-foot pole, but if
>> somebody else wants to, have at it.
> 
> Nod.

Yeah, having an extra test for partitioned tables would be a good
idea.

>> Hm, there's this in compression.sql:
>> 
>> -- test LIKE INCLUDING COMPRESSION
>> CREATE TABLE cmdata2 (LIKE cmdata1 INCLUDING COMPRESSION);
>> \d+ cmdata2
>> 
>> Or did you mean the case with a partitioned table specifically?
> 
> Ah, I guess that's sufficient.  (The INCLUDING clause cannot be used to
> create a partition, actually.)

+column_compression:
+           COMPRESSION ColId                       { $$ = $2; }
+           | COMPRESSION DEFAULT                   { $$ =
pstrdup("default"); }
Could it be possible to have some tests for COMPRESSION DEFAULT?  It
seems to me that this should be documented as a supported keyword for
CREATE/ALTER TABLE.

 --changing column storage should not impact the compression method
 --but the data should not be compressed
 ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE varchar;
+ALTER TABLE cmdata2 ALTER COLUMN f1 SET COMPRESSION pglz;
This comment needs a refresh?
--
Michael

  32. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-27T00:29:50Z

    Michael Paquier <michael@paquier.xyz> writes:
> Yeah, having an extra test for partitioned tables would be a good
> idea.

We do have some coverage already via the pg_upgrade test.

> Could it be possible to have some tests for COMPRESSION DEFAULT?  It
> seems to me that this should be documented as a supported keyword for
> CREATE/ALTER TABLE.

Uh, I did do both of those, no?  (The docs treat "default" as another
possible value, not a keyword, even though it's a keyword internally.)

>  --changing column storage should not impact the compression method
>  --but the data should not be compressed
>  ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE varchar;
> +ALTER TABLE cmdata2 ALTER COLUMN f1 SET COMPRESSION pglz;
> This comment needs a refresh?

It's correct AFAICS.  Maybe it needs a bit of editing for clarity,
but I'm not sure how to make it better.  The point is that the
SET STORAGE just below disables compression of new values, no
matter what SET COMPRESSION says.

			regards, tom lane

  33. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Andres Freund <andres@anarazel.de> — 2021-05-27T00:31:44Z

    Hi,

On 2021-05-25 14:46:27 +0900, Michael Paquier wrote:
> That would work.  Your suggestion, as I understood it first, makes the
> code simpler by not using tup_values at all as the set of values[] is
> filled when the values and nulls are extracted.  So I have gone with
> this simplification, and applied the patch (moved a bit the comments
> while on it).

Hm. memsetting values_free() to zero repeatedly isn't quite free, nor is
iterating over all columns one more time. Note that values/isnull are
passed in, and allocated with an accurate size, so it's a bit odd to
then do a pessimally sized stack allocation. Efficiency aside, that just
seems a bit weird?

The efficiency bit is probably going to be swamped by the addition of
the compression handling, given the amount of additional work we're now
doing in in reform_and_rewrite_tuple(). I wonder if we should check how
much slower a VACUUM FULL of a table with a few varlena columns has
gotten vs 13.

Greetings,

Andres Freund

  34. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-27T00:35:46Z

    Andres Freund <andres@anarazel.de> writes:
> The efficiency bit is probably going to be swamped by the addition of
> the compression handling, given the amount of additional work we're now
> doing in in reform_and_rewrite_tuple().

Only if the user has explicitly requested a change of compression, no?

			regards, tom lane

  35. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-05-27T01:00:16Z

    On Wed, May 26, 2021 at 08:35:46PM -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > The efficiency bit is probably going to be swamped by the addition of
> > the compression handling, given the amount of additional work we're now
> > doing in in reform_and_rewrite_tuple().
> 
> Only if the user has explicitly requested a change of compression, no?

Andres' point is that we'd still initialize and run through
values_free at the end of reform_and_rewrite_tuple() for each tuple
even if there no need to do so.  Well, we could control the
initialization and the free() checks at the end of the routine if we
know that there has been at least one detoasted value, at the expense
of making the code a bit less clear, of course.
--
Michael

  36. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Andres Freund <andres@anarazel.de> — 2021-05-27T01:54:15Z

    Hi,

On 2021-05-26 20:35:46 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > The efficiency bit is probably going to be swamped by the addition of
> > the compression handling, given the amount of additional work we're now
> > doing in in reform_and_rewrite_tuple().
> 
> Only if the user has explicitly requested a change of compression, no?

Oh, it'll definitely be more expensive in that case - but that seems
fair game. What I was wondering about was whether VACUUM FULL would be
measurably slower, because we'll now call toast_get_compression_id() on
each varlena datum. It's pretty easy for VACUUM FULL to be CPU bound
already, and presumably this'll add a bit.

Greetings,

Andres Freund

  37. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-05-27T02:07:53Z

    On Wed, May 26, 2021 at 06:54:15PM -0700, Andres Freund wrote:
> Oh, it'll definitely be more expensive in that case - but that seems
> fair game. What I was wondering about was whether VACUUM FULL would be
> measurably slower, because we'll now call toast_get_compression_id() on
> each varlena datum. It's pretty easy for VACUUM FULL to be CPU bound
> already, and presumably this'll add a bit.

This depends on the number of attributes, but I do see an extra 0.5%
__memmove_avx_unaligned_erms in reform_and_rewrite_tuple() for a
normal VACUUM FULL with a 1-int-column relation on a perf profile,
with rewrite_heap_tuple eating most of it as in the past, so that's
within the noise bandwidth if you measure the runtime.  What would be
the worst case here, a table with one text column made of non-NULL
still very short values?
--
Michael

  38. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Andres Freund <andres@anarazel.de> — 2021-05-27T02:14:02Z

    Hi,

On 2021-05-26 18:54:15 -0700, Andres Freund wrote:
> On 2021-05-26 20:35:46 -0400, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > The efficiency bit is probably going to be swamped by the addition of
> > > the compression handling, given the amount of additional work we're now
> > > doing in in reform_and_rewrite_tuple().
> >
> > Only if the user has explicitly requested a change of compression, no?
>
> Oh, it'll definitely be more expensive in that case - but that seems
> fair game. What I was wondering about was whether VACUUM FULL would be
> measurably slower, because we'll now call toast_get_compression_id() on
> each varlena datum. It's pretty easy for VACUUM FULL to be CPU bound
> already, and presumably this'll add a bit.
>

CREATE UNLOGGED TABLE vacme_text(t01 text not null default 't01',t02 text not null default 't02',t03 text not null default 't03',t04 text not null default 't04',t05 text not null default 't05',t06 text not null default 't06',t07 text not null default 't07',t08 text not null default 't08',t09 text not null default 't09',t10 text not null default 't10');
CREATE UNLOGGED TABLE vacme_int(i1 int not null default '1',i2 int not null default '2',i3 int not null default '3',i4 int not null default '4',i5 int not null default '5',i6 int not null default '6',i7 int not null default '7',i8 int not null default '8',i9 int not null default '9',i10 int not null default '10');
INSERT INTO vacme_text SELECT FROM generate_series(1, 10000000);
INSERT INTO vacme_int SELECT FROM generate_series(1, 10000000);

I ran 10 VACUUM FULLs on each, chose the shortest time:

unmodified
text:   3562ms
int:    3037ms

after ifdefing out the compression handling:
text:   3175ms (x 0.88)
int:    2894ms (x 0.95)

That's not *too* bad, but also not nothing....

Greetings,

Andres Freund

  39. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Andres Freund <andres@anarazel.de> — 2021-05-27T02:24:12Z

    Hi,

On 2021-05-27 11:07:53 +0900, Michael Paquier wrote:
> This depends on the number of attributes, but I do see an extra 0.5%
> __memmove_avx_unaligned_erms in reform_and_rewrite_tuple() for a
> normal VACUUM FULL with a 1-int-column relation on a perf profile,
> with rewrite_heap_tuple eating most of it as in the past, so that's
> within the noise bandwidth if you measure the runtime.
> What would be the worst case here, a table with one text column made
> of non-NULL still very short values?

I think you need a bunch of columns to see it, like in the benchmark I
just posted - I didn't test any other number of columns than 10 though.

Greetings,

Andres Freund

  40. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-27T02:43:42Z

    Andres Freund <andres@anarazel.de> writes:
> That's not *too* bad, but also not nothing....

The memsets seem to be easy to get rid of.  memset the array
to zeroes *once* before entering the per-tuple loop.  Then,
in the loop that looks for stuff to pfree, reset any entries
that are found to be set, thereby returning the array to all
zeroes for the next iteration.

I"m having a hard time though believing that the memset is the
main problem.  I'd think the pfree search loop is at least as
expensive.  Maybe skip that when not useful, by having a single
bool flag remembering whether any columns got detoasted in this
row?

			regards, tom lane

  41. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Andres Freund <andres@anarazel.de> — 2021-05-27T03:21:05Z

    Hi,

On 2021-05-26 22:43:42 -0400, Tom Lane wrote:
> The memsets seem to be easy to get rid of.  memset the array
> to zeroes *once* before entering the per-tuple loop.  Then,
> in the loop that looks for stuff to pfree, reset any entries
> that are found to be set, thereby returning the array to all
> zeroes for the next iteration.

> I"m having a hard time though believing that the memset is the
> main problem.  I'd think the pfree search loop is at least as
> expensive.  Maybe skip that when not useful, by having a single
> bool flag remembering whether any columns got detoasted in this
> row?

Yea, I tested that - it does help in the integer case. But the bigger
contributors are the loop over the attributes, and especially the access
to the datum's compression method. Particularly the latter seems hard to
avoid.

Greetings,

Andres Freund

  42. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-27T03:34:53Z

    Andres Freund <andres@anarazel.de> writes:
> Yea, I tested that - it does help in the integer case. But the bigger
> contributors are the loop over the attributes, and especially the access
> to the datum's compression method. Particularly the latter seems hard to
> avoid.

So maybe we should just dump the promise that VACUUM FULL will recompress
everything?  I'd be in favor of that actually, because it seems 100%
outside the charter of either VACUUM FULL or CLUSTER.

			regards, tom lane

  43. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-05-27T04:04:38Z

    On Wed, May 26, 2021 at 11:34:53PM -0400, Tom Lane wrote:
> So maybe we should just dump the promise that VACUUM FULL will recompress
> everything?  I'd be in favor of that actually, because it seems 100%
> outside the charter of either VACUUM FULL or CLUSTER.

Hmm.  You are right that by default this may not be worth the extra
cost.  We could make that easily an option, though, for users ready to
accept this cost.  And that could be handy when it comes to a
database-wise VACUUM.
--
Michael

  44. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-27T04:11:38Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Wed, May 26, 2021 at 11:34:53PM -0400, Tom Lane wrote:
>> So maybe we should just dump the promise that VACUUM FULL will recompress
>> everything?  I'd be in favor of that actually, because it seems 100%
>> outside the charter of either VACUUM FULL or CLUSTER.

> Hmm.  You are right that by default this may not be worth the extra
> cost.  We could make that easily an option, though, for users ready to
> accept this cost.  And that could be handy when it comes to a
> database-wise VACUUM.

AFAIR, there are zero promises about how effective, or when effective,
changes in SET STORAGE will be.  And the number of complaints about
that has also been zero.  So I'm not sure why we need to do more for
SET COMPRESSION.  Especially since I'm unconvinced that recompressing
everything just to recompress everything would *ever* be worthwhile.

			regards, tom lane

  45. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Robert Haas <robertmhaas@gmail.com> — 2021-05-27T11:58:33Z

    On Thu, May 27, 2021 at 12:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> AFAIR, there are zero promises about how effective, or when effective,
> changes in SET STORAGE will be.  And the number of complaints about
> that has also been zero.  So I'm not sure why we need to do more for
> SET COMPRESSION.  Especially since I'm unconvinced that recompressing
> everything just to recompress everything would *ever* be worthwhile.

I think it is good to have *some* way of ensuring that what you want
the system to do, it is actually doing. If we have not a single
operation in the system anywhere that can force recompression, someone
who actually cares will be left with no option but a dump and reload.
That is probably both a whole lot slower than something in the server
itself and also a pretty silly thing to have to tell people to do.

If it helps, I'd be perfectly fine with having this be an *optional*
behavior for CLUSTER or VACUUM FULL, depending on some switch. Or we
can devise another way for the user to make it happen. But we
shouldn't just be setting a policy that users are not allowed to care
whether their data is actually compressed using the compression method
they specified.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  46. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-27T13:34:08Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, May 27, 2021 at 12:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> AFAIR, there are zero promises about how effective, or when effective,
>> changes in SET STORAGE will be.  And the number of complaints about
>> that has also been zero.  So I'm not sure why we need to do more for
>> SET COMPRESSION.  Especially since I'm unconvinced that recompressing
>> everything just to recompress everything would *ever* be worthwhile.

> I think it is good to have *some* way of ensuring that what you want
> the system to do, it is actually doing. If we have not a single
> operation in the system anywhere that can force recompression, someone
> who actually cares will be left with no option but a dump and reload.
> That is probably both a whole lot slower than something in the server
> itself and also a pretty silly thing to have to tell people to do.

[ shrug... ]  I think the history of the SET STORAGE option teaches us
that there is no such requirement, and you're inventing a scenario that
doesn't exist in the real world.

			regards, tom lane

  47. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-27T14:18:09Z

    On Thu, May 27, 2021 at 7:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Thu, May 27, 2021 at 12:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> AFAIR, there are zero promises about how effective, or when effective,
> >> changes in SET STORAGE will be.  And the number of complaints about
> >> that has also been zero.  So I'm not sure why we need to do more for
> >> SET COMPRESSION.  Especially since I'm unconvinced that recompressing
> >> everything just to recompress everything would *ever* be worthwhile.
>
> > I think it is good to have *some* way of ensuring that what you want
> > the system to do, it is actually doing. If we have not a single
> > operation in the system anywhere that can force recompression, someone
> > who actually cares will be left with no option but a dump and reload.
> > That is probably both a whole lot slower than something in the server
> > itself and also a pretty silly thing to have to tell people to do.
>
> [ shrug... ]  I think the history of the SET STORAGE option teaches us
> that there is no such requirement, and you're inventing a scenario that
> doesn't exist in the real world.

But can we compare SET STORAGE with SET compression?  I mean storage
just controls how the data are stored internally and there is no
external dependency.  But if we see the compression it will have a
dependency on the external library.  So if the user wants to get rid
of the dependency on the external library then IMHO, there should be
some way to do it by recompressing all the data.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  48. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Robert Haas <robertmhaas@gmail.com> — 2021-05-27T14:25:17Z

    On Thu, May 27, 2021 at 10:18 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > [ shrug... ]  I think the history of the SET STORAGE option teaches us
> > that there is no such requirement, and you're inventing a scenario that
> > doesn't exist in the real world.
>
> But can we compare SET STORAGE with SET compression?  I mean storage
> just controls how the data are stored internally and there is no
> external dependency.  But if we see the compression it will have a
> dependency on the external library.  So if the user wants to get rid
> of the dependency on the external library then IMHO, there should be
> some way to do it by recompressing all the data.

TBH, I'm more concerned about the other direction. Surely someone who
upgrades from an existing release to v14 and sets their compression
method to lz4 is going to want a way of actually converting their data
to using lz4. To say that nobody cares about that is to deem the
feature useless. Maybe that's what Tom thinks, but it's not what I
think.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  49. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-27T14:39:51Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, May 27, 2021 at 10:18 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>>> [ shrug... ]  I think the history of the SET STORAGE option teaches us
>>> that there is no such requirement, and you're inventing a scenario that
>>> doesn't exist in the real world.

>> But can we compare SET STORAGE with SET compression?  I mean storage
>> just controls how the data are stored internally and there is no
>> external dependency.  But if we see the compression it will have a
>> dependency on the external library.  So if the user wants to get rid
>> of the dependency on the external library then IMHO, there should be
>> some way to do it by recompressing all the data.

> TBH, I'm more concerned about the other direction. Surely someone who
> upgrades from an existing release to v14 and sets their compression
> method to lz4 is going to want a way of actually converting their data
> to using lz4. To say that nobody cares about that is to deem the
> feature useless. Maybe that's what Tom thinks, but it's not what I
> think.

What I'm hearing is a whole lot of hypothesizing and zero evidence of
actual field requirements.  On the other side of the coin, we've already
wasted significant person-hours on fixing this feature's memory leakage,
and now people are proposing to expend more effort on solving^Wpapering
over its performance issues by adding yet more user-visible complication.
It's already adding too much user-visible complication IMO --- I know
because I was just copy-editing the documentation about that yesterday.

I say it's time to stop the bleeding and rip it out.  When and if
there are actual field requests to have a way to do this, we can
discuss what's the best way to respond to those requests.  Hacking
VACUUM probably isn't the best answer, anyway.  But right now,
we are past feature freeze, and I think we ought to jettison this
one rather than quickly kluge something.

			regards, tom lane

  50. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Robert Haas <robertmhaas@gmail.com> — 2021-05-27T18:21:00Z

    On Thu, May 27, 2021 at 10:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> What I'm hearing is a whole lot of hypothesizing and zero evidence of
> actual field requirements.  On the other side of the coin, we've already
> wasted significant person-hours on fixing this feature's memory leakage,
> and now people are proposing to expend more effort on solving^Wpapering
> over its performance issues by adding yet more user-visible complication.
> It's already adding too much user-visible complication IMO --- I know
> because I was just copy-editing the documentation about that yesterday.
>
> I say it's time to stop the bleeding and rip it out.  When and if
> there are actual field requests to have a way to do this, we can
> discuss what's the best way to respond to those requests.  Hacking
> VACUUM probably isn't the best answer, anyway.  But right now,
> we are past feature freeze, and I think we ought to jettison this
> one rather than quickly kluge something.

Thanks for sharing your thoughts. -1 from me.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  51. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-27T19:34:21Z

    On 2021-May-27, Tom Lane wrote:

> I say it's time to stop the bleeding and rip it out.  When and if
> there are actual field requests to have a way to do this, we can
> discuss what's the best way to respond to those requests.  Hacking
> VACUUM probably isn't the best answer, anyway.  But right now,
> we are past feature freeze, and I think we ought to jettison this
> one rather than quickly kluge something.

Sorry, I'm unclear on exactly what are you proposing.  Are you proposing
to rip out the fact that VACUUM FULL promises to recompress everything,
or are you proposing to rip out the whole attcompression feature?

Absolute -1 on the latter from me.  Pluggable compression has taken
years to get to this point, it certainly won't do to give that up.

Now about the former.  If we do think that recompressing causes an
unacceptable 10% slowdown for every single VACUUM FULLs, then yeah we
should discuss changing that behavior -- maybe remove promises of
recompression and wait for pg15 to add "VACUUM (RECOMPRESS)" or
similar.

If it's a 10% slowdown of the only best times (variability unspecified)
and only in corner cases (unlogged tables with no indexes that fit in
shared buffers), then I don't think we should bother.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"You're _really_ hosed if the person doing the hiring doesn't understand
relational systems: you end up with a whole raft of programmers, none of
whom has had a Date with the clue stick."              (Andrew Sullivan)

  52. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Peter Geoghegan <pg@bowt.ie> — 2021-05-27T19:57:38Z

    On Thu, May 27, 2021 at 7:25 AM Robert Haas <robertmhaas@gmail.com> wrote:
> TBH, I'm more concerned about the other direction. Surely someone who
> upgrades from an existing release to v14 and sets their compression
> method to lz4 is going to want a way of actually converting their data
> to using lz4.

Your argument would be more convincing (at least to me) if we really
did expect users to want to pick and choose, based on natural
variations in datasets that make switching to *either* potentially
yield a real benefit. It is my understanding that lz4 is pretty much
superior to pglz by every relevant measure, though, so I'm not sure
that that argument can be made. At the same time, users tend to only
care specifically about things that are real step changes -- which I
don't think this qualifies as. Users will go out of their way to get one of
those, but otherwise won't bother.

Perhaps there is a practical argument in favor of VACUUM FULL reliably
recompressing using lz4 on upgrade, where that's the user's stated
preference. It's not self-evident that VACUUM FULL must or even should
do that, at least to me. I'm not suggesting that there must not be
such an argument. Just that I don't think that anybody has made such
an argument.

> To say that nobody cares about that is to deem the
> feature useless. Maybe that's what Tom thinks, but it's not what I
> think.

I don't think that follows at all.


--
Peter Geoghegan

  53. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-27T20:10:04Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Sorry, I'm unclear on exactly what are you proposing.  Are you proposing
> to rip out the fact that VACUUM FULL promises to recompress everything,
> or are you proposing to rip out the whole attcompression feature?

Just the former.

			regards, tom lane

  54. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-27T20:17:58Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Now about the former.  If we do think that recompressing causes an
> unacceptable 10% slowdown for every single VACUUM FULLs, then yeah we
> should discuss changing that behavior -- maybe remove promises of
> recompression and wait for pg15 to add "VACUUM (RECOMPRESS)" or
> similar.
> If it's a 10% slowdown of the only best times (variability unspecified)
> and only in corner cases (unlogged tables with no indexes that fit in
> shared buffers), then I don't think we should bother.

BTW, perhaps I should clarify my goal here: it's to cut off expending
further effort on this feature during v14.  If we can decide that the
existing performance situation is acceptable, I'm content with that
decision.  But if we're to start designing new user-visible behavior to
satisfy performance objections, then I'd prefer to remove this VACUUM
behavior altogether for now.

			regards, tom lane

  55. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-27T20:29:44Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Thu, May 27, 2021 at 7:25 AM Robert Haas <robertmhaas@gmail.com> wrote:
>> To say that nobody cares about that is to deem the
>> feature useless. Maybe that's what Tom thinks, but it's not what I
>> think.

> I don't think that follows at all.

Yeah.  My belief here is that users might bother to change
default_toast_compression, or that we might do it for them in a few
years, but the gains from doing so are going to be only incremental.
That being the case, most DBAs will be content to allow the older
compression method to age out of their databases through routine row
updates.  The idea that somebody is going to be excited enough about
this to run a downtime-inducing VACUUM FULL doesn't really pass the
smell test.

That doesn't make LZ4 compression useless, by any means, but it does
suggest that we shouldn't be adding overhead to VACUUM FULL for the
purpose of easing instantaneous switchovers.

I'll refrain from re-telling old war stories about JPEG/GIF/PNG, but
I do have real-world experience with compression algorithm changes.
IME you need an integer-multiples type of improvement to get peoples'
attention, and LZ4 isn't going to offer that, except maybe in
cherry-picked examples.

			regards, tom lane

  56. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Justin Pryzby <pryzby@telsasoft.com> — 2021-05-27T22:10:19Z

    On Tue, May 25, 2021 at 08:33:47PM -0500, Justin Pryzby wrote:
> On Sun, May 23, 2021 at 12:25:10PM -0400, Tom Lane wrote:
> > However, the more I looked at that code the less I liked it.
> > I think the way that compression selection is handled for indexes,
> > ie consult default_toast_compression on-the-fly, is *far* saner
> > than what is currently implemented for tables.  So I think we
> > should redefine attcompression as "ID of a compression method
> > to use, or \0 to select the prevailing default.  Ignored if
> > attstorage does not permit the use of compression".
> 
> +1
> 
> It reminds me of reltablespace, which is stored as 0 to mean the database's
> default tablespace.

I was surprised to realize that I made this same suggestion last month...
https://www.postgresql.org/message-id/20210320074420.GR11765@telsasoft.com
|..unless we changed attcompression='\0' to mean (for varlena) "the default
|compression".  Rather than "resolving" to the default compression at the time
|the table is created, columns without an explicit compression set would "defer"
|to the GUC (of course, that only affects newly-inserted data).

The original reason for that suggestion Michael handled differently in
63db0ac3f9e6bae313da67f640c95c0045b7f0ee

-- 
Justin

  57. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Peter Geoghegan <pg@bowt.ie> — 2021-05-27T22:52:06Z

    On Thu, May 27, 2021 at 1:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah.  My belief here is that users might bother to change
> default_toast_compression, or that we might do it for them in a few
> years, but the gains from doing so are going to be only incremental.
> That being the case, most DBAs will be content to allow the older
> compression method to age out of their databases through routine row
> updates.  The idea that somebody is going to be excited enough about
> this to run a downtime-inducing VACUUM FULL doesn't really pass the
> smell test.

That was my original understanding of your position, FWIW. I agree
with all of this.

> That doesn't make LZ4 compression useless, by any means, but it does
> suggest that we shouldn't be adding overhead to VACUUM FULL for the
> purpose of easing instantaneous switchovers.

Right. More generally, there often seems to be value in
under-specifying what a compression option does. Or in treating them
as advisory.

You mentioned the history of SET STORAGE, which seems very relevant. I
am reminded of the example of B-Tree deduplication with unique
indexes, where we selectively apply the optimization based on
page-level details. Deduplication isn't usually useful in unique
indexes (for the obvious reason), though occasionally it is extremely
useful. I think that there might be a variety of things that work a
little like that. It can help with avoiding unnecessary dump and
reload hazards, too.

I am interested in hearing the *principle* behind Robert's position.
This whole area seems like something that might have at least a couple
of different schools of thought. If it is then I'd sincerely like to
hear the other side of the argument.

-- 
Peter Geoghegan

  58. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-06-02T02:32:51Z

    On Thu, May 27, 2021 at 04:17:58PM -0400, Tom Lane wrote:
> BTW, perhaps I should clarify my goal here: it's to cut off expending
> further effort on this feature during v14.

No disagreement here.

> If we can decide that the
> existing performance situation is acceptable, I'm content with that
> decision.  But if we're to start designing new user-visible behavior to
> satisfy performance objections, then I'd prefer to remove this VACUUM
> behavior altogether for now.

After putting a PGDATA on a tmpfs, I have looked at the run time of
VACUUM FULL with tables full of text columns, with that:
CREATE OR REPLACE FUNCTION create_cols(tabname text, num_cols int)
RETURNS VOID AS
$func$
DECLARE
  query text;
BEGIN
  query := 'CREATE TABLE ' || tabname || ' (';
  FOR i IN 1..num_cols LOOP
    query := query || 'a_' || i::text || ' text NOT NULL DEFAULT ' || i::text;
    IF i != num_cols THEN
      query := query || ', ';
    END IF;
  END LOOP;
  query := query || ')';
  EXECUTE format(query);
  query := 'INSERT INTO ' || tabname || ' SELECT FROM generate_series(1,1000000)';
  EXECUTE format(query);
END
$func$ LANGUAGE plpgsql;

After 12 runs of VACUUM FULL on my laptop, I have removed the two
highest and the two lowest to remove some noise, and did an average of
the rest:
- HEAD, 100 text columns: 5720ms
- REL_13_STABLE, 100 text columns: 4308ms
- HEAD, 200 text columns: 10020ms
- REL_13_STABLE, 200 text columns:  8319ms

So yes, that looks much visible to me, and an argument in favor of the
removal of the forced recompression on HEAD when rewriting tuples.
--
Michael

  59. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-06-02T03:25:08Z

    On Thu, May 27, 2021 at 03:52:06PM -0700, Peter Geoghegan wrote:
> On Thu, May 27, 2021 at 1:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yeah.  My belief here is that users might bother to change
>> default_toast_compression, or that we might do it for them in a few
>> years, but the gains from doing so are going to be only incremental.
>> That being the case, most DBAs will be content to allow the older
>> compression method to age out of their databases through routine row
>> updates.  The idea that somebody is going to be excited enough about
>> this to run a downtime-inducing VACUUM FULL doesn't really pass the
>> smell test.
> 
> That was my original understanding of your position, FWIW. I agree
> with all of this.

If one wishes to enforce a compression method on a table, the only way
I could see through here, able to bypass the downtime constraint, is 
by using logical replication.  Anybody willing to enforce a new
default compression may accept the cost of setting up instances for
that.
--
Michael

  60. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-06-03T16:04:48Z

    On 2021-Jun-02, Michael Paquier wrote:

> After 12 runs of VACUUM FULL on my laptop, I have removed the two
> highest and the two lowest to remove some noise, and did an average of
> the rest:
> - HEAD, 100 text columns: 5720ms
> - REL_13_STABLE, 100 text columns: 4308ms
> - HEAD, 200 text columns: 10020ms
> - REL_13_STABLE, 200 text columns:  8319ms
> 
> So yes, that looks much visible to me, and an argument in favor of the
> removal of the forced recompression on HEAD when rewriting tuples.

Just to be clear -- that's the time to vacuum the table without changing
the compression algorithm, right?  So the overhead is just the check for
whether the recompression is needed, not the recompression itself?

If the check for recompression is this expensive, then yeah I agree that
we should take it out.  If recompression is actually occurring, then I
don't think this is a good test :-)

-- 
Álvaro Herrera       Valdivia, Chile

  61. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-06-03T23:54:48Z

    On Thu, Jun 03, 2021 at 12:04:48PM -0400, Alvaro Herrera wrote:
> If the check for recompression is this expensive, then yeah I agree that
> we should take it out.  If recompression is actually occurring, then I
> don't think this is a good test :-)

I have done no recompression here, so I was just stressing the extra
test for the recompression.  Sorry for the confusion.
--
Michael

  62. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-06-04T05:24:51Z

    On Fri, Jun 04, 2021 at 08:54:48AM +0900, Michael Paquier wrote:
> I have done no recompression here, so I was just stressing the extra
> test for the recompression.  Sorry for the confusion.

I am not sure yet which way we are going, but cleaning up this code
involves a couple of things:
- Clean up the docs.
- Update one of the tests of compression.sql, with its alternate
output.
- Clean up of reform_and_rewrite_tuple() where the rewrite is done.

So that would give the attached.
--
Michael

  63. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-06-04T22:42:57Z

    So I tried running vacuum full in pgbench of your 10-column table,
max_wal_size=32GB.  I didn't move pgdata to an in-memory pgdata, but
this is on NVMe so pretty fast anyway.

pgbench -c1 -t30 -n -f vacuumfull.sql.

Current master:
latency average = 2885.550 ms
latency stddev = 1771.170 ms
tps = 0.346554 (without initial connection time)

With the recompression code ifdef'ed out (pretty much like in your
patch):
latency average = 2481.336 ms
latency stddev = 1011.738 ms
tps = 0.403008 (without initial connection time)

With toast_get_compression_id as a static inline, like in the attach
patch:
latency average = 2520.982 ms
latency stddev = 1043.042 ms
tps = 0.396671 (without initial connection time)

It seems to me that most of the overhead is the function call for
toast_get_compression_id(), so we should get rid of that.


Now, while this patch does seem to work correctly, it raises a number of
weird cpluspluscheck warnings, which I think are attributable to the
new macro definitions.  I didn't look into it closely, but I suppose it
should be fixable given sufficient effort:

In file included from /tmp/cpluspluscheck.yuQqS5/test.cpp:2:
/pgsql/source/master//src/include/access/toast_compression.h: In function ‘ToastCompressionId toast_get_compression_id(varlena*)’:
/pgsql/source/master//src/include/postgres.h:392:46: warning: comparison of integer expressions of different signedness: ‘uint32’ {aka ‘unsigned int’} and ‘int32’ {aka ‘int’} [-Wsign-compare]
  (VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer) < \
/pgsql/source/master//src/include/access/toast_compression.h:109:7: note: in expansion of macro ‘VARATT_EXTERNAL_IS_COMPRESSED’
   if (VARATT_EXTERNAL_IS_COMPRESSED(toast_pointer))
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/pgsql/source/master//src/include/postgres.h:374:30: error: invalid conversion from ‘uint32’ {aka ‘unsigned int’} to ‘ToastCompressionId’ [-fpermissive]
  ((toast_pointer).va_extinfo >> VARLENA_EXTSIZE_BITS)
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
/pgsql/source/master//src/include/access/toast_compression.h:110:11: note: in expansion of macro ‘VARATT_EXTERNAL_GET_COMPRESS_METHOD’
    cmid = VARATT_EXTERNAL_GET_COMPRESS_METHOD(toast_pointer);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/pgsql/source/master//src/include/postgres.h:368:53: error: invalid conversion from ‘uint32’ {aka ‘unsigned int’} to ‘ToastCompressionId’ [-fpermissive]
  (((varattrib_4b *) (PTR))->va_compressed.va_tcinfo >> VARLENA_EXTSIZE_BITS)
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
/pgsql/source/master//src/include/access/toast_compression.h:113:10: note: in expansion of macro ‘VARDATA_COMPRESSED_GET_COMPRESS_METHOD’
   cmid = VARDATA_COMPRESSED_GET_COMPRESS_METHOD(attr);
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/cpluspluscheck.yuQqS5/test.cpp:2:
/pgsql/source/master/src/include/access/toast_compression.h: In function ‘ToastCompressionId toast_get_compression_id(varlena*)’:
/pgsql/source/master//src/include/postgres.h:392:46: warning: comparison of integer expressions of different signedness: ‘uint32’ {aka ‘unsigned int’} and ‘int32’ {aka ‘int’} [-Wsign-compare]
  (VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer) < \
/pgsql/source/master/src/include/access/toast_compression.h:109:7: note: in expansion of macro ‘VARATT_EXTERNAL_IS_COMPRESSED’
   if (VARATT_EXTERNAL_IS_COMPRESSED(toast_pointer))
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/pgsql/source/master//src/include/postgres.h:374:30: error: invalid conversion from ‘uint32’ {aka ‘unsigned int’} to ‘ToastCompressionId’ [-fpermissive]
  ((toast_pointer).va_extinfo >> VARLENA_EXTSIZE_BITS)
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
/pgsql/source/master/src/include/access/toast_compression.h:110:11: note: in expansion of macro ‘VARATT_EXTERNAL_GET_COMPRESS_METHOD’
    cmid = VARATT_EXTERNAL_GET_COMPRESS_METHOD(toast_pointer);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/pgsql/source/master//src/include/postgres.h:368:53: error: invalid conversion from ‘uint32’ {aka ‘unsigned int’} to ‘ToastCompressionId’ [-fpermissive]
  (((varattrib_4b *) (PTR))->va_compressed.va_tcinfo >> VARLENA_EXTSIZE_BITS)
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
/pgsql/source/master/src/include/access/toast_compression.h:113:10: note: in expansion of macro ‘VARDATA_COMPRESSED_GET_COMPRESS_METHOD’
   cmid = VARDATA_COMPRESSED_GET_COMPRESS_METHOD(attr);
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

-- 
Álvaro Herrera       Valdivia, Chile

  64. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-04T22:51:00Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> It seems to me that most of the overhead is the function call for
> toast_get_compression_id(), so we should get rid of that.

Nice result.  I'm willing to live with 1.5% slowdown ... IME that's
usually below the noise threshold anyway.

> Now, while this patch does seem to work correctly, it raises a number of
> weird cpluspluscheck warnings, which I think are attributable to the
> new macro definitions.  I didn't look into it closely, but I suppose it
> should be fixable given sufficient effort:

Didn't test, but the first one is certainly fixable by adding a cast,
and I guess the others might be as well.

			regards, tom lane

  65. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-06-04T23:21:11Z

    On 2021-Jun-04, Tom Lane wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

> > Now, while this patch does seem to work correctly, it raises a number of
> > weird cpluspluscheck warnings, which I think are attributable to the
> > new macro definitions.  I didn't look into it closely, but I suppose it
> > should be fixable given sufficient effort:
> 
> Didn't test, but the first one is certainly fixable by adding a cast,
> and I guess the others might be as well.

I get no warnings with this one.  I'm a bit wary of leaving
VARDATA_COMPRESSED_GET_EXTSIZE unchanged, but at least nothing in this
patch requires a cast there.

-- 
Álvaro Herrera       Valdivia, Chile

  66. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-06-06T03:07:35Z

    On Fri, Jun 04, 2021 at 07:21:11PM -0400, Alvaro Herrera wrote:
> On 2021-Jun-04, Tom Lane wrote:
> 
> > Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> 
> > > Now, while this patch does seem to work correctly, it raises a number of
> > > weird cpluspluscheck warnings, which I think are attributable to the
> > > new macro definitions.  I didn't look into it closely, but I suppose it
> > > should be fixable given sufficient effort:
> > 
> > Didn't test, but the first one is certainly fixable by adding a cast,
> > and I guess the others might be as well.
> 
> I get no warnings with this one.  I'm a bit wary of leaving
> VARDATA_COMPRESSED_GET_EXTSIZE unchanged, but at least nothing in this
> patch requires a cast there.

I have done the same test as previously, with the following
configuration to be clear:
- No assertion, non-debug build.
- No autovacuum.
- No recompression involved.
- Data put in a tmpfs.
- One relation with 200 columns of NOT NULL text with default values,
using that:
https://postgr.es/m/YLbt02A+IDnFhwIp@paquier.xyz
- 1M rows.
- 15 VACUUM FULL runs, discarding the 3 lowest and the 3 highest run
times to remove most of the noise, then did an average of the
remaining 9 runs.

The test has been done with four configurations, and here are the
results:
1) HEAD: 9659ms
2) REL_13_STABLE: 8310ms.
3) Alvaro's patch, as of
https://postgr.es/m/202106042321.6jx54yliy2l6@alvherre.pgsql: 9521ms.
4) My patch applied on HEAD, as of
https://postgr.es/m/YLm5I9MCGz4SnPdX@paquier.xyz: 8304ms.

This case is a kind of worst-case configuration, but it seems to me
that there is still a large difference with that :/
--
Michael

  67. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-06-08T14:39:24Z

    On 2021-Jun-06, Michael Paquier wrote:

> On Fri, Jun 04, 2021 at 07:21:11PM -0400, Alvaro Herrera wrote:

> The test has been done with four configurations, and here are the
> results:
> 1) HEAD: 9659ms
> 2) REL_13_STABLE: 8310ms.
> 3) Alvaro's patch, as of
> https://postgr.es/m/202106042321.6jx54yliy2l6@alvherre.pgsql: 9521ms.
> 4) My patch applied on HEAD, as of
> https://postgr.es/m/YLm5I9MCGz4SnPdX@paquier.xyz: 8304ms.

Hmm, ok.  Trying to figure out what is happening would require more time
than I can devote to this at present.

My unverified guess is that this code causes too many pipeline stalls
while executing the big per-column loop.  Maybe it would be better to
scan the attribute array twice: one to collect all data from
Form_pg_attribute for each column into nicely packed arrays, then in a
second loop process all the recompressions together ... the idea being
that the first loop can run without stalling.

Maybe at this point reverting is the only solution.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)

  68. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-06-09T03:24:57Z

    On Tue, Jun 08, 2021 at 10:39:24AM -0400, Alvaro Herrera wrote:
> My unverified guess is that this code causes too many pipeline stalls
> while executing the big per-column loop.  Maybe it would be better to
> scan the attribute array twice: one to collect all data from
> Form_pg_attribute for each column into nicely packed arrays, then in a
> second loop process all the recompressions together ... the idea being
> that the first loop can run without stalling.

You mean for attlen and attcompression, right?  I agree that it would
help.

A extra set of things worth it here would be to move the allocation
and memset(0) of values_free from reform_and_rewrite_tuple(), and do
the round of pfree() calls if at least one value has been detoasted.

> Maybe at this point reverting is the only solution.

That's a safe bet at this point.  It would be good to conclude this
one by beta2 IMO.
--
Michael

  69. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-09T03:32:21Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Jun 08, 2021 at 10:39:24AM -0400, Alvaro Herrera wrote:
>> Maybe at this point reverting is the only solution.

> That's a safe bet at this point.  It would be good to conclude this
> one by beta2 IMO.

I still think it's a really dubious argument that anybody would want to
incur a VACUUM FULL to force conversion to a different compression method.

I can imagine sometime in the future where we need to get rid of all
instances of pglz so we can reassign that compression code to something
else.  But would we insist on a mass VACUUM FULL to make that happen?
Doubt it.  You'd want a tool that would make that happen over time,
in the background; like the mechanisms that have been discussed for
enabling checksums on-the-fly.

In the meantime I'm +1 for dropping this logic from VACUUM FULL.
I don't even want to spend enough more time on it to confirm the
different overhead measurements that have been reported.

			regards, tom lane

  70. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-06-10T02:09:52Z

    On Tue, Jun 08, 2021 at 11:32:21PM -0400, Tom Lane wrote:
> I can imagine sometime in the future where we need to get rid of all
> instances of pglz so we can reassign that compression code to something
> else.  But would we insist on a mass VACUUM FULL to make that happen?
> Doubt it.  You'd want a tool that would make that happen over time,
> in the background; like the mechanisms that have been discussed for
> enabling checksums on-the-fly.

Well, I can imagine that some people could afford being more
aggressive here even if it implies some downtime and if they are not
willing to afford the deployment of a second instance for a
dump/restore or a logirep setup.

(The parallel with data checksums is partially true, as you can do a
switch of checksums with physical replication as the page's checksums
are only written when pushed out of shared buffers, not when they are
written into WAL.  This needs a second instance, of course.)

> In the meantime I'm +1 for dropping this logic from VACUUM FULL.
> I don't even want to spend enough more time on it to confirm the
> different overhead measurements that have been reported.

Agreed.  It looks like we are heading toward doing just that for this
release.
--
Michael

  71. Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

    Michael Paquier <michael@paquier.xyz> — 2021-06-14T00:27:16Z

    On Thu, Jun 10, 2021 at 11:09:52AM +0900, Michael Paquier wrote:
> On Tue, Jun 08, 2021 at 11:32:21PM -0400, Tom Lane wrote:
>> In the meantime I'm +1 for dropping this logic from VACUUM FULL.
>> I don't even want to spend enough more time on it to confirm the
>> different overhead measurements that have been reported.
> 
> Agreed.  It looks like we are heading toward doing just that for this
> release.

Hearing nothing, done this way.
--
Michael