Обсуждение: Move pg_attribute.attcompression to earlier in struct for reduced size?
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
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
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
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
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
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
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
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 6d06ad22b9..8aebc4d12f 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1236,6 +1236,15 @@
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>attalign</structfield> <type>char</type>
+ </para>
+ <para>
+ A copy of <literal>pg_type.typalign</literal> of this column's type
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>attstorage</structfield> <type>char</type>
@@ -1249,10 +1258,13 @@
<row>
<entry role="catalog_table_entry"><para role="column_definition">
- <structfield>attalign</structfield> <type>char</type>
+ <structfield>attcompression</structfield> <type>char</type>
</para>
<para>
- A copy of <literal>pg_type.typalign</literal> of this column's type
+ The current compression method of the column. If it is an invalid
+ compression method (<literal>'\0'</literal>) then column data will not
+ be compressed. Otherwise, <literal>'p'</literal> = pglz compression or
+ <literal>'l'</literal> = <productname>LZ4</productname> compression.
</para></entry>
</row>
@@ -1355,18 +1367,6 @@
</para></entry>
</row>
- <row>
- <entry role="catalog_table_entry"><para role="column_definition">
- <structfield>attcompression</structfield> <type>char</type>
- </para>
- <para>
- The current compression method of the column. If it is an invalid
- compression method (<literal>'\0'</literal>) then column data will not
- be compressed. Otherwise, <literal>'p'</literal> = pglz compression or
- <literal>'l'</literal> = <productname>LZ4</productname> compression.
- </para></entry>
- </row>
-
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>attacl</structfield> <type>aclitem[]</type>
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index ffb275afbe..e03e5c9909 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -440,9 +440,11 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
return false;
if (attr1->attbyval != attr2->attbyval)
return false;
+ if (attr1->attalign != attr2->attalign)
+ return false;
if (attr1->attstorage != attr2->attstorage)
return false;
- if (attr1->attalign != attr2->attalign)
+ if (attr1->attcompression != attr2->attcompression)
return false;
if (attr1->attnotnull != attr2->attnotnull)
return false;
@@ -460,7 +462,7 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
return false;
if (attr1->attcollation != attr2->attcollation)
return false;
- /* attacl, attoptions and attfdwoptions are not even present... */
+ /* variable-length fields are not even present... */
}
if (tupdesc1->constr != NULL)
@@ -639,12 +641,11 @@ TupleDescInitEntry(TupleDesc desc,
att->attbyval = typeForm->typbyval;
att->attalign = typeForm->typalign;
att->attstorage = typeForm->typstorage;
- att->attcollation = typeForm->typcollation;
-
if (IsStorageCompressible(typeForm->typstorage))
att->attcompression = GetDefaultToastCompression();
else
att->attcompression = InvalidCompressionMethod;
+ att->attcollation = typeForm->typcollation;
ReleaseSysCache(tuple);
}
@@ -709,6 +710,7 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
att->attbyval = false;
att->attalign = TYPALIGN_INT;
att->attstorage = TYPSTORAGE_EXTENDED;
+ att->attcompression = InvalidCompressionMethod; /* don't care */
att->attcollation = DEFAULT_COLLATION_OID;
break;
@@ -717,6 +719,7 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
att->attbyval = true;
att->attalign = TYPALIGN_CHAR;
att->attstorage = TYPSTORAGE_PLAIN;
+ att->attcompression = InvalidCompressionMethod;
att->attcollation = InvalidOid;
break;
@@ -725,6 +728,7 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
att->attbyval = true;
att->attalign = TYPALIGN_INT;
att->attstorage = TYPSTORAGE_PLAIN;
+ att->attcompression = InvalidCompressionMethod;
att->attcollation = InvalidOid;
break;
@@ -733,6 +737,7 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
att->attbyval = FLOAT8PASSBYVAL;
att->attalign = TYPALIGN_DOUBLE;
att->attstorage = TYPSTORAGE_PLAIN;
+ att->attcompression = InvalidCompressionMethod;
att->attcollation = InvalidOid;
break;
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 8d99c9b762..9ff280a252 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -165,8 +165,8 @@ fillTypeDesc(SpGistTypeDesc *desc, Oid type)
typtup = (Form_pg_type) GETSTRUCT(tp);
desc->attlen = typtup->typlen;
desc->attbyval = typtup->typbyval;
- desc->attstorage = typtup->typstorage;
desc->attalign = typtup->typalign;
+ desc->attstorage = typtup->typstorage;
ReleaseSysCache(tp);
}
@@ -304,8 +304,8 @@ getSpGistTupleDesc(Relation index, SpGistTypeDesc *keyType)
att->attalign = keyType->attalign;
att->attstorage = keyType->attstorage;
/* We shouldn't need to bother with making these valid: */
- att->attcollation = InvalidOid;
att->attcompression = InvalidCompressionMethod;
+ att->attcollation = InvalidOid;
/* In case we changed typlen, we'd better reset following offsets */
for (int i = spgFirstIncludeColumn; i < outTupDesc->natts; i++)
TupleDescAttr(outTupDesc, i)->attcacheoff = -1;
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index b155237488..217b4cd001 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -699,8 +699,8 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
attrtypes[attnum]->atttypid = Ap->am_oid;
attrtypes[attnum]->attlen = Ap->am_typ.typlen;
attrtypes[attnum]->attbyval = Ap->am_typ.typbyval;
- attrtypes[attnum]->attstorage = Ap->am_typ.typstorage;
attrtypes[attnum]->attalign = Ap->am_typ.typalign;
+ attrtypes[attnum]->attstorage = Ap->am_typ.typstorage;
attrtypes[attnum]->attcollation = Ap->am_typ.typcollation;
/* if an array type, assume 1-dimensional attribute */
if (Ap->am_typ.typelem != InvalidOid && Ap->am_typ.typlen < 0)
@@ -713,8 +713,8 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
attrtypes[attnum]->atttypid = TypInfo[typeoid].oid;
attrtypes[attnum]->attlen = TypInfo[typeoid].len;
attrtypes[attnum]->attbyval = TypInfo[typeoid].byval;
- attrtypes[attnum]->attstorage = TypInfo[typeoid].storage;
attrtypes[attnum]->attalign = TypInfo[typeoid].align;
+ attrtypes[attnum]->attstorage = TypInfo[typeoid].storage;
attrtypes[attnum]->attcollation = TypInfo[typeoid].collation;
/* if an array type, assume 1-dimensional attribute */
if (TypInfo[typeoid].elem != InvalidOid &&
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 63a907d50d..112b3affdf 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -897,8 +897,11 @@ sub morph_row_for_pgattr
$row->{atttypid} = $type->{oid};
$row->{attlen} = $type->{typlen};
$row->{attbyval} = $type->{typbyval};
- $row->{attstorage} = $type->{typstorage};
$row->{attalign} = $type->{typalign};
+ $row->{attstorage} = $type->{typstorage};
+
+ $row->{attcompression} =
+ $type->{typstorage} ne 'p' && $type->{typstorage} ne 'e' ? 'p' : '\0';
# set attndims if it's an array type
$row->{attndims} = $type->{typcategory} eq 'A' ? '1' : '0';
@@ -907,9 +910,6 @@ sub morph_row_for_pgattr
$row->{attcollation} =
$type->{typcollation} ne '0' ? $C_COLLATION_OID : 0;
- $row->{attcompression} =
- $type->{typstorage} ne 'p' && $type->{typstorage} ne 'e' ? 'p' : '\0';
-
if (defined $attr->{forcenotnull})
{
$row->{attnotnull} = 't';
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 431e62e389..3b2cd203c1 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -146,7 +146,8 @@ static Node *cookConstraint(ParseState *pstate,
/*
* The initializers below do not include trailing variable length fields,
* but that's OK - we're never going to reference anything beyond the
- * fixed-size portion of the structure anyway.
+ * fixed-size portion of the structure anyway. Fields that can default
+ * to zeroes are also not mentioned.
*/
static const FormData_pg_attribute a1 = {
@@ -157,8 +158,8 @@ static const FormData_pg_attribute a1 = {
.attcacheoff = -1,
.atttypmod = -1,
.attbyval = false,
- .attstorage = TYPSTORAGE_PLAIN,
.attalign = TYPALIGN_SHORT,
+ .attstorage = TYPSTORAGE_PLAIN,
.attnotnull = true,
.attislocal = true,
};
@@ -171,8 +172,8 @@ static const FormData_pg_attribute a2 = {
.attcacheoff = -1,
.atttypmod = -1,
.attbyval = true,
- .attstorage = TYPSTORAGE_PLAIN,
.attalign = TYPALIGN_INT,
+ .attstorage = TYPSTORAGE_PLAIN,
.attnotnull = true,
.attislocal = true,
};
@@ -185,8 +186,8 @@ static const FormData_pg_attribute a3 = {
.attcacheoff = -1,
.atttypmod = -1,
.attbyval = true,
- .attstorage = TYPSTORAGE_PLAIN,
.attalign = TYPALIGN_INT,
+ .attstorage = TYPSTORAGE_PLAIN,
.attnotnull = true,
.attislocal = true,
};
@@ -199,8 +200,8 @@ static const FormData_pg_attribute a4 = {
.attcacheoff = -1,
.atttypmod = -1,
.attbyval = true,
- .attstorage = TYPSTORAGE_PLAIN,
.attalign = TYPALIGN_INT,
+ .attstorage = TYPSTORAGE_PLAIN,
.attnotnull = true,
.attislocal = true,
};
@@ -213,8 +214,8 @@ static const FormData_pg_attribute a5 = {
.attcacheoff = -1,
.atttypmod = -1,
.attbyval = true,
- .attstorage = TYPSTORAGE_PLAIN,
.attalign = TYPALIGN_INT,
+ .attstorage = TYPSTORAGE_PLAIN,
.attnotnull = true,
.attislocal = true,
};
@@ -233,8 +234,8 @@ static const FormData_pg_attribute a6 = {
.attcacheoff = -1,
.atttypmod = -1,
.attbyval = true,
- .attstorage = TYPSTORAGE_PLAIN,
.attalign = TYPALIGN_INT,
+ .attstorage = TYPSTORAGE_PLAIN,
.attnotnull = true,
.attislocal = true,
};
@@ -779,8 +780,9 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
slot[slotCount]->tts_values[Anum_pg_attribute_attcacheoff - 1] = Int32GetDatum(-1);
slot[slotCount]->tts_values[Anum_pg_attribute_atttypmod - 1] = Int32GetDatum(attrs->atttypmod);
slot[slotCount]->tts_values[Anum_pg_attribute_attbyval - 1] = BoolGetDatum(attrs->attbyval);
- slot[slotCount]->tts_values[Anum_pg_attribute_attstorage - 1] = CharGetDatum(attrs->attstorage);
slot[slotCount]->tts_values[Anum_pg_attribute_attalign - 1] = CharGetDatum(attrs->attalign);
+ slot[slotCount]->tts_values[Anum_pg_attribute_attstorage - 1] = CharGetDatum(attrs->attstorage);
+ slot[slotCount]->tts_values[Anum_pg_attribute_attcompression - 1] = CharGetDatum(attrs->attcompression);
slot[slotCount]->tts_values[Anum_pg_attribute_attnotnull - 1] = BoolGetDatum(attrs->attnotnull);
slot[slotCount]->tts_values[Anum_pg_attribute_atthasdef - 1] = BoolGetDatum(attrs->atthasdef);
slot[slotCount]->tts_values[Anum_pg_attribute_atthasmissing - 1] = BoolGetDatum(attrs->atthasmissing);
@@ -790,7 +792,6 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
slot[slotCount]->tts_values[Anum_pg_attribute_attislocal - 1] = BoolGetDatum(attrs->attislocal);
slot[slotCount]->tts_values[Anum_pg_attribute_attinhcount - 1] = Int32GetDatum(attrs->attinhcount);
slot[slotCount]->tts_values[Anum_pg_attribute_attcollation - 1] = ObjectIdGetDatum(attrs->attcollation);
- slot[slotCount]->tts_values[Anum_pg_attribute_attcompression - 1] = CharGetDatum(attrs->attcompression);
if (attoptions && attoptions[natts] != (Datum) 0)
slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attoptions[natts];
else
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 0f8cfae4ec..30b900aac6 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -345,8 +345,8 @@ ConstructTupleDescriptor(Relation heapRelation,
to->attndims = from->attndims;
to->atttypmod = from->atttypmod;
to->attbyval = from->attbyval;
- to->attstorage = from->attstorage;
to->attalign = from->attalign;
+ to->attstorage = from->attstorage;
to->attcompression = from->attcompression;
}
else
@@ -373,10 +373,10 @@ ConstructTupleDescriptor(Relation heapRelation,
*/
to->atttypid = keyType;
to->attlen = typeTup->typlen;
+ to->atttypmod = exprTypmod(indexkey);
to->attbyval = typeTup->typbyval;
- to->attstorage = typeTup->typstorage;
to->attalign = typeTup->typalign;
- to->atttypmod = exprTypmod(indexkey);
+ to->attstorage = typeTup->typstorage;
/*
* For expression columns, set attcompression invalid, since
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ebc62034d2..e28d6e4253 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6593,12 +6593,19 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
attribute.atttypid = typeOid;
attribute.attstattarget = (newattnum > 0) ? -1 : 0;
attribute.attlen = tform->typlen;
- attribute.atttypmod = typmod;
attribute.attnum = newattnum;
- attribute.attbyval = tform->typbyval;
attribute.attndims = list_length(colDef->typeName->arrayBounds);
- attribute.attstorage = tform->typstorage;
+ attribute.atttypmod = typmod;
+ attribute.attbyval = tform->typbyval;
attribute.attalign = tform->typalign;
+ attribute.attstorage = tform->typstorage;
+ /* do not set compression in views etc */
+ if (rel->rd_rel->relkind == RELKIND_RELATION ||
+ rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ attribute.attcompression = GetAttributeCompression(&attribute,
+ colDef->compression);
+ else
+ attribute.attcompression = InvalidCompressionMethod;
attribute.attnotnull = colDef->is_not_null;
attribute.atthasdef = false;
attribute.atthasmissing = false;
@@ -6609,17 +6616,6 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
attribute.attinhcount = colDef->inhcount;
attribute.attcollation = collOid;
- /*
- * lookup attribute's compression method and store it in the
- * attr->attcompression.
- */
- if (rel->rd_rel->relkind == RELKIND_RELATION ||
- rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- attribute.attcompression = GetAttributeCompression(&attribute,
- colDef->compression);
- else
- attribute.attcompression = InvalidCompressionMethod;
-
/* attribute.attacl is handled by InsertPgAttributeTuples() */
ReleaseSysCache(typeTuple);
diff --git a/src/include/access/spgist_private.h b/src/include/access/spgist_private.h
index ba3da5b540..40d3b71b89 100644
--- a/src/include/access/spgist_private.h
+++ b/src/include/access/spgist_private.h
@@ -137,8 +137,8 @@ typedef struct SpGistTypeDesc
Oid type;
int16 attlen;
bool attbyval;
- char attstorage;
char attalign;
+ char attstorage;
} SpGistTypeDesc;
typedef struct SpGistState
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 560f8f00bb..1e26016214 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -111,6 +111,12 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
*/
bool attbyval;
+ /*
+ * attalign is a copy of the typalign field from pg_type for this
+ * attribute. See atttypid comments above.
+ */
+ char attalign;
+
/*----------
* attstorage tells for VARLENA attributes, what the heap access
* methods can do to it if a given tuple doesn't fit into a page.
@@ -120,10 +126,10 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
char attstorage;
/*
- * attalign is a copy of the typalign field from pg_type for this
- * attribute. See atttypid comments above.
+ * Compression method. Must be InvalidCompressionMethod if and only if
+ * typstorage is 'plain' or 'external'.
*/
- char attalign;
+ char attcompression BKI_DEFAULT('\0');
/* This flag represents the "NOT NULL" constraint */
bool attnotnull;
@@ -160,12 +166,6 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
/* attribute's collation, if any */
Oid attcollation BKI_LOOKUP_OPT(pg_collation);
- /*
- * compression method. Must be InvalidCompressionMethod if and only if
- * typstorage is 'plain' or 'external'.
- */
- char attcompression BKI_DEFAULT('\0');
-
#ifdef CATALOG_VARLEN /* variable-length fields start here */
/* NOTE: The following fields are not present in tuple descriptors. */
@@ -190,10 +190,10 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
* ATTRIBUTE_FIXED_PART_SIZE is the size of the fixed-layout,
* guaranteed-not-null part of a pg_attribute row. This is in fact as much
* of the row as gets copied into tuple descriptors, so don't expect you
- * can access fields beyond attcollation except in a real tuple!
+ * can access the variable-length fields except in a real tuple!
*/
#define ATTRIBUTE_FIXED_PART_SIZE \
- (offsetof(FormData_pg_attribute,attcompression) + sizeof(char))
+ (offsetof(FormData_pg_attribute,attcollation) + sizeof(Oid))
/* ----------------
* Form_pg_attribute corresponds to a pointer to a tuple with
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
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)) FROMgenerate_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
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
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
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
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
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
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
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
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
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.
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.
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
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 11e91c4ad3..69b1839fd7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -897,17 +897,8 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
if (colDef->generated)
attr->attgenerated = colDef->generated;
- /*
- * lookup attribute's compression method and store it in the
- * attr->attcompression.
- */
- if (relkind == RELKIND_RELATION ||
- relkind == RELKIND_PARTITIONED_TABLE ||
- relkind == RELKIND_MATVIEW)
- attr->attcompression =
- GetAttributeCompression(attr, colDef->compression);
- else
- attr->attcompression = InvalidCompressionMethod;
+ attr->attcompression = GetAttributeCompression(attr,
+ colDef->compression);
}
/*
@@ -6602,13 +6593,8 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
attribute.attbyval = tform->typbyval;
attribute.attalign = tform->typalign;
attribute.attstorage = tform->typstorage;
- /* do not set compression in views etc */
- if (rel->rd_rel->relkind == RELKIND_RELATION ||
- rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- attribute.attcompression = GetAttributeCompression(&attribute,
- colDef->compression);
- else
- attribute.attcompression = InvalidCompressionMethod;
+ attribute.attcompression = GetAttributeCompression(&attribute,
+ colDef->compression);
attribute.attnotnull = colDef->is_not_null;
attribute.atthasdef = false;
attribute.atthasmissing = false;
@@ -12299,23 +12285,10 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
attTup->attbyval = tform->typbyval;
attTup->attalign = tform->typalign;
attTup->attstorage = tform->typstorage;
-
- /* Setup attribute compression */
- if (rel->rd_rel->relkind == RELKIND_RELATION ||
- rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- {
- /*
- * No compression for plain/external storage, otherwise, default
- * compression method if it is not already set, refer comments atop
- * attcompression parameter in pg_attribute.h.
- */
- if (!IsStorageCompressible(tform->typstorage))
- attTup->attcompression = InvalidCompressionMethod;
- else if (!CompressionMethodIsValid(attTup->attcompression))
- attTup->attcompression = GetDefaultToastCompression();
- }
- else
+ if (!IsStorageCompressible(tform->typstorage))
attTup->attcompression = InvalidCompressionMethod;
+ else if (!CompressionMethodIsValid(attTup->attcompression))
+ attTup->attcompression = GetDefaultToastCompression();
ReleaseSysCache(typeTuple);
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index b26b872a06..16493209c6 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1261,10 +1261,14 @@
<structfield>attcompression</structfield> <type>char</type>
</para>
<para>
- The current compression method of the column. If it is an invalid
- compression method (<literal>'\0'</literal>) then column data will not
- be compressed. Otherwise, <literal>'p'</literal> = pglz compression or
- <literal>'l'</literal> = <productname>LZ4</productname> compression.
+ The current compression method of the column. Typically this is
+ <literal>'\0'</literal> to specify use of the current default setting
+ (see <xref linkend="guc-default-toast-compression"/>). Otherwise,
+ <literal>'p'</literal> selects pglz compression, while
+ <literal>'l'</literal> selects <productname>LZ4</productname>
+ compression. However, this field is ignored
+ whenever <structfield>attstorage</structfield> does not allow
+ compression.
</para></entry>
</row>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 7e32b0686c..95a302ffee 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8239,10 +8239,11 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
<para>
This variable sets the default
<link linkend="storage-toast">TOAST</link>
- compression method for columns of newly-created tables. The
- <command>CREATE TABLE</command> statement can override this default
- by specifying the <literal>COMPRESSION</literal> column option.
-
+ compression method for values of compressible columns.
+ (This can be overridden for individual columns by setting
+ the <literal>COMPRESSION</literal> column option in
+ <command>CREATE TABLE</command> or
+ <command>ALTER TABLE</command>.)
The supported compression methods are <literal>pglz</literal> and,
if <productname>PostgreSQL</productname> was compiled with
<literal>--with-lz4</literal>, <literal>lz4</literal>.
diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c
index ee05372f79..09e563b1f0 100644
--- a/src/backend/access/brin/brin_tuple.c
+++ b/src/backend/access/brin/brin_tuple.c
@@ -232,11 +232,10 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple,
* same compression method. Otherwise we have to use the
* default method.
*/
- if (att->atttypid == atttype->type_id &&
- CompressionMethodIsValid(att->attcompression))
+ if (att->atttypid == atttype->type_id)
compression = att->attcompression;
else
- compression = GetDefaultToastCompression();
+ compression = InvalidCompressionMethod;
cvalue = toast_compress_datum(value, compression);
diff --git a/src/backend/access/common/indextuple.c b/src/backend/access/common/indextuple.c
index 5212560411..8df882da7a 100644
--- a/src/backend/access/common/indextuple.c
+++ b/src/backend/access/common/indextuple.c
@@ -104,18 +104,9 @@ index_form_tuple(TupleDesc tupleDescriptor,
att->attstorage == TYPSTORAGE_MAIN))
{
Datum cvalue;
- char compression = att->attcompression;
- /*
- * If the compression method is not valid, use the default. We
- * don't expect this to happen for regular index columns, which
- * inherit the setting from the corresponding table column, but we
- * do expect it to happen whenever an expression is indexed.
- */
- if (!CompressionMethodIsValid(compression))
- compression = GetDefaultToastCompression();
-
- cvalue = toast_compress_datum(untoasted_values[i], compression);
+ cvalue = toast_compress_datum(untoasted_values[i],
+ att->attcompression);
if (DatumGetPointer(cvalue) != NULL)
{
diff --git a/src/backend/access/common/toast_internals.c b/src/backend/access/common/toast_internals.c
index 8d2a9964c3..4d60a56a59 100644
--- a/src/backend/access/common/toast_internals.c
+++ b/src/backend/access/common/toast_internals.c
@@ -53,10 +53,12 @@ toast_compress_datum(Datum value, char cmethod)
Assert(!VARATT_IS_EXTERNAL(DatumGetPointer(value)));
Assert(!VARATT_IS_COMPRESSED(DatumGetPointer(value)));
- Assert(CompressionMethodIsValid(cmethod));
-
valsize = VARSIZE_ANY_EXHDR(DatumGetPointer(value));
+ /* If the compression method is not valid, use the default */
+ if (!CompressionMethodIsValid(cmethod))
+ cmethod = GetDefaultToastCompression();
+
/*
* Call appropriate compression routine for the compression method.
*/
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index affbc509bd..4c63bd4dc6 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -642,10 +642,7 @@ TupleDescInitEntry(TupleDesc desc,
att->attbyval = typeForm->typbyval;
att->attalign = typeForm->typalign;
att->attstorage = typeForm->typstorage;
- if (IsStorageCompressible(typeForm->typstorage))
- att->attcompression = GetDefaultToastCompression();
- else
- att->attcompression = InvalidCompressionMethod;
+ att->attcompression = InvalidCompressionMethod;
att->attcollation = typeForm->typcollation;
ReleaseSysCache(tuple);
@@ -711,7 +708,7 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
att->attbyval = false;
att->attalign = TYPALIGN_INT;
att->attstorage = TYPSTORAGE_EXTENDED;
- att->attcompression = GetDefaultToastCompression();
+ att->attcompression = InvalidCompressionMethod;
att->attcollation = DEFAULT_COLLATION_OID;
break;
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 8e6e8d5169..d1f4c21f94 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2483,10 +2483,10 @@ reform_and_rewrite_tuple(HeapTuple tuple,
* perform the compression here; we just need to decompress. That
* will trigger recompression later on.
*/
-
struct varlena *new_value;
ToastCompressionId cmid;
char cmethod;
+ char targetmethod;
new_value = (struct varlena *) DatumGetPointer(values[i]);
cmid = toast_get_compression_id(new_value);
@@ -2495,7 +2495,7 @@ reform_and_rewrite_tuple(HeapTuple tuple,
if (cmid == TOAST_INVALID_COMPRESSION_ID)
continue;
- /* convert compression id to compression method */
+ /* convert existing compression id to compression method */
switch (cmid)
{
case TOAST_PGLZ_COMPRESSION_ID:
@@ -2506,10 +2506,16 @@ reform_and_rewrite_tuple(HeapTuple tuple,
break;
default:
elog(ERROR, "invalid compression method id %d", cmid);
+ cmethod = '\0'; /* keep compiler quiet */
}
+ /* figure out what the target method is */
+ targetmethod = TupleDescAttr(newTupDesc, i)->attcompression;
+ if (!CompressionMethodIsValid(targetmethod))
+ targetmethod = GetDefaultToastCompression();
+
/* if compression method doesn't match then detoast the value */
- if (TupleDescAttr(newTupDesc, i)->attcompression != cmethod)
+ if (targetmethod != cmethod)
{
values[i] = PointerGetDatum(detoast_attr(new_value));
values_free[i] = true;
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 62abd008cc..94ab5ca095 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -701,6 +701,7 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
attrtypes[attnum]->attbyval = Ap->am_typ.typbyval;
attrtypes[attnum]->attalign = Ap->am_typ.typalign;
attrtypes[attnum]->attstorage = Ap->am_typ.typstorage;
+ attrtypes[attnum]->attcompression = InvalidCompressionMethod;
attrtypes[attnum]->attcollation = Ap->am_typ.typcollation;
/* if an array type, assume 1-dimensional attribute */
if (Ap->am_typ.typelem != InvalidOid && Ap->am_typ.typlen < 0)
@@ -715,6 +716,7 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
attrtypes[attnum]->attbyval = TypInfo[typeoid].byval;
attrtypes[attnum]->attalign = TypInfo[typeoid].align;
attrtypes[attnum]->attstorage = TypInfo[typeoid].storage;
+ attrtypes[attnum]->attcompression = InvalidCompressionMethod;
attrtypes[attnum]->attcollation = TypInfo[typeoid].collation;
/* if an array type, assume 1-dimensional attribute */
if (TypInfo[typeoid].elem != InvalidOid &&
@@ -724,11 +726,6 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
attrtypes[attnum]->attndims = 0;
}
- if (IsStorageCompressible(attrtypes[attnum]->attstorage))
- attrtypes[attnum]->attcompression = GetDefaultToastCompression();
- else
- attrtypes[attnum]->attcompression = InvalidCompressionMethod;
-
/*
* If a system catalog column is collation-aware, force it to use C
* collation, so that its behavior is independent of the database's
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 112b3affdf..f893ae4f45 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -899,9 +899,7 @@ sub morph_row_for_pgattr
$row->{attbyval} = $type->{typbyval};
$row->{attalign} = $type->{typalign};
$row->{attstorage} = $type->{typstorage};
-
- $row->{attcompression} =
- $type->{typstorage} ne 'p' && $type->{typstorage} ne 'e' ? 'p' : '\0';
+ $row->{attcompression} = '\0';
# set attndims if it's an array type
$row->{attndims} = $type->{typcategory} eq 'A' ? '1' : '0';
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 3dfe2e8a56..afa830d924 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1719,8 +1719,6 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
/* Unset this so no one tries to look up the generation expression */
attStruct->attgenerated = '\0';
- attStruct->attcompression = InvalidCompressionMethod;
-
/*
* Change the column name to something that isn't likely to conflict
*/
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 69b1839fd7..e87d9cda93 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -601,7 +601,7 @@ static void refuseDupeIndexAttach(Relation parentIdx, Relation partIdx,
Relation partitionTbl);
static List *GetParentedForeignKeyRefs(Relation partition);
static void ATDetachCheckNoForeignKeyRefs(Relation partition);
-static char GetAttributeCompression(Form_pg_attribute att, char *compression);
+static char GetAttributeCompression(Oid atttypid, char *compression);
/* ----------------------------------------------------------------
@@ -897,8 +897,9 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
if (colDef->generated)
attr->attgenerated = colDef->generated;
- attr->attcompression = GetAttributeCompression(attr,
- colDef->compression);
+ if (colDef->compression)
+ attr->attcompression = GetAttributeCompression(attr->atttypid,
+ colDef->compression);
}
/*
@@ -6593,7 +6594,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
attribute.attbyval = tform->typbyval;
attribute.attalign = tform->typalign;
attribute.attstorage = tform->typstorage;
- attribute.attcompression = GetAttributeCompression(&attribute,
+ attribute.attcompression = GetAttributeCompression(typeOid,
colDef->compression);
attribute.attnotnull = colDef->is_not_null;
attribute.atthasdef = false;
@@ -12285,10 +12286,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
attTup->attbyval = tform->typbyval;
attTup->attalign = tform->typalign;
attTup->attstorage = tform->typstorage;
- if (!IsStorageCompressible(tform->typstorage))
- attTup->attcompression = InvalidCompressionMethod;
- else if (!CompressionMethodIsValid(attTup->attcompression))
- attTup->attcompression = GetDefaultToastCompression();
+ attTup->attcompression = InvalidCompressionMethod;
ReleaseSysCache(typeTuple);
@@ -15586,7 +15584,6 @@ ATExecSetCompression(AlteredTableInfo *tab,
Form_pg_attribute atttableform;
AttrNumber attnum;
char *compression;
- char typstorage;
char cmethod;
ObjectAddress address;
@@ -15611,17 +15608,11 @@ ATExecSetCompression(AlteredTableInfo *tab,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot alter system column \"%s\"", column)));
- typstorage = get_typstorage(atttableform->atttypid);
-
- /* prevent from setting compression methods for uncompressible type */
- if (!IsStorageCompressible(typstorage))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("column data type %s does not support compression",
- format_type_be(atttableform->atttypid))));
-
- /* get the attribute compression method. */
- cmethod = GetAttributeCompression(atttableform, compression);
+ /*
+ * Check that column type is compressible, then get the attribute
+ * compression method code
+ */
+ cmethod = GetAttributeCompression(atttableform->atttypid, compression);
/* update pg_attribute entry */
atttableform->attcompression = cmethod;
@@ -18585,29 +18576,26 @@ ATDetachCheckNoForeignKeyRefs(Relation partition)
* resolve column compression specification to compression method.
*/
static char
-GetAttributeCompression(Form_pg_attribute att, char *compression)
+GetAttributeCompression(Oid atttypid, char *compression)
{
- char typstorage = get_typstorage(att->atttypid);
+ char typstorage;
char cmethod;
+ if (compression == NULL)
+ return InvalidCompressionMethod;
+
/*
- * No compression for plain/external storage, refer comments atop
- * attcompression parameter in pg_attribute.h
+ * To specify a nondefault method, the column data type must be
+ * compressible. Note this says nothing about whether the column's
+ * attstorage setting permits compression; we intentionally allow
+ * attstorage and attcompression to be independent.
*/
+ typstorage = get_typstorage(atttypid);
if (!IsStorageCompressible(typstorage))
- {
- if (compression == NULL)
- return InvalidCompressionMethod;
-
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("column data type %s does not support compression",
- format_type_be(att->atttypid))));
- }
-
- /* fallback to default compression if it's not specified */
- if (compression == NULL)
- return GetDefaultToastCompression();
+ format_type_be(atttypid))));
cmethod = CompressionNameToMethod(compression);
if (!CompressionMethodIsValid(cmethod))
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ee731044b6..87bc688704 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4651,13 +4651,13 @@ static struct config_enum ConfigureNamesEnum[] =
{
{"default_toast_compression", PGC_USERSET, CLIENT_CONN_STATEMENT,
- gettext_noop("Sets the default compression for new columns."),
- NULL,
- GUC_IS_NAME
+ gettext_noop("Sets the default compression method for compressible values."),
+ NULL
},
&default_toast_compression,
TOAST_PGLZ_COMPRESSION,
- default_toast_compression_options, NULL, NULL
+ default_toast_compression_options,
+ NULL, NULL, NULL
},
{
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 339c393718..b063e00cd4 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -16421,7 +16421,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
tbinfo->attfdwoptions[j]);
/*
- * Dump per-column compression, if different from default.
+ * Dump per-column compression, if it's been set.
*/
if (!dopt->no_toast_compression)
{
@@ -16440,9 +16440,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
break;
}
- if (cmname != NULL &&
- (fout->default_toast_compression == NULL ||
- strcmp(cmname, fout->default_toast_compression) != 0))
+ if (cmname != NULL)
appendPQExpBuffer(q, "ALTER %sTABLE ONLY %s ALTER COLUMN %s SET COMPRESSION %s;\n",
foreign, qualrelname,
fmtId(tbinfo->attnames[j]),
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 3e39fdb545..e461bcefb9 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2055,7 +2055,7 @@ describeOneTableDetails(const char *schemaname,
appendPQExpBufferStr(&buf, ",\n a.attstorage");
attstorage_col = cols++;
- /* compression info */
+ /* compression info, if relevant to relkind */
if (pset.sversion >= 140000 &&
!pset.hide_compression &&
(tableinfo.relkind == RELKIND_RELATION ||
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 1e26016214..603392fe81 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -126,8 +126,12 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
char attstorage;
/*
- * Compression method. Must be InvalidCompressionMethod if and only if
- * typstorage is 'plain' or 'external'.
+ * attcompression sets the current compression method of the attribute.
+ * Typically this is InvalidCompressionMethod ('\0') to specify use of the
+ * current default setting (see default_toast_compression). Otherwise,
+ * 'p' selects pglz compression, while 'l' selects LZ4 compression.
+ * However, this field is ignored whenever attstorage does not allow
+ * compression.
*/
char attcompression BKI_DEFAULT('\0');
diff --git a/src/test/regress/expected/compression.out b/src/test/regress/expected/compression.out
index 61e97cb33c..79e929673a 100644
--- a/src/test/regress/expected/compression.out
+++ b/src/test/regress/expected/compression.out
@@ -53,7 +53,7 @@ SELECT * INTO cmmove1 FROM cmdata;
Table "public.cmmove1"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | text | | | | extended | pglz | |
+ f1 | text | | | | extended | | |
SELECT pg_column_compression(f1) FROM cmmove1;
pg_column_compression
@@ -146,7 +146,7 @@ ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE varchar;
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+-------------------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | character varying | | | | extended | pglz | |
+ f1 | character varying | | | | extended | | |
ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE int USING f1::integer;
\d+ cmdata2
@@ -162,14 +162,14 @@ ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE varchar;
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+-------------------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | character varying | | | | extended | pglz | |
+ f1 | character varying | | | | extended | | |
ALTER TABLE cmdata2 ALTER COLUMN f1 SET STORAGE plain;
\d+ cmdata2
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+-------------------+-----------+----------+---------+---------+-------------+--------------+-------------
- f1 | character varying | | | | plain | pglz | |
+ f1 | character varying | | | | plain | | |
INSERT INTO cmdata2 VALUES (repeat('123456789', 800));
SELECT pg_column_compression(f1) FROM cmdata2;
@@ -184,7 +184,7 @@ CREATE MATERIALIZED VIEW mv(x) AS SELECT * FROM cmdata1;
Materialized view "public.mv"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+------+-----------+----------+---------+----------+-------------+--------------+-------------
- x | text | | | | extended | pglz | |
+ x | text | | | | extended | | |
View definition:
SELECT cmdata1.f1 AS x
FROM cmdata1;
@@ -245,7 +245,7 @@ CREATE TABLE cmdata2 (f1 text);
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | text | | | | extended | lz4 | |
+ f1 | text | | | | extended | | |
SET default_toast_compression = 'pglz';
-- test alter compression method
diff --git a/src/test/regress/expected/compression_1.out b/src/test/regress/expected/compression_1.out
index d03d6255ae..2a9fc81b51 100644
--- a/src/test/regress/expected/compression_1.out
+++ b/src/test/regress/expected/compression_1.out
@@ -50,7 +50,7 @@ SELECT * INTO cmmove1 FROM cmdata;
Table "public.cmmove1"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | text | | | | extended | pglz | |
+ f1 | text | | | | extended | | |
SELECT pg_column_compression(f1) FROM cmmove1;
pg_column_compression
@@ -144,7 +144,7 @@ ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE varchar;
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+-------------------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | character varying | | | | extended | pglz | |
+ f1 | character varying | | | | extended | | |
ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE int USING f1::integer;
\d+ cmdata2
@@ -160,14 +160,14 @@ ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE varchar;
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+-------------------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | character varying | | | | extended | pglz | |
+ f1 | character varying | | | | extended | | |
ALTER TABLE cmdata2 ALTER COLUMN f1 SET STORAGE plain;
\d+ cmdata2
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+-------------------+-----------+----------+---------+---------+-------------+--------------+-------------
- f1 | character varying | | | | plain | pglz | |
+ f1 | character varying | | | | plain | | |
INSERT INTO cmdata2 VALUES (repeat('123456789', 800));
SELECT pg_column_compression(f1) FROM cmdata2;
@@ -240,7 +240,7 @@ CREATE TABLE cmdata2 (f1 text);
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | text | | | | extended | pglz | |
+ f1 | text | | | | extended | | |
SET default_toast_compression = 'pglz';
-- test alter compression method
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index fc054af5ba..3c1cd858a8 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -208,8 +208,6 @@ typedef struct Archive
/* other important stuff */
char *searchpath; /* search_path to set during restore */
- char *default_toast_compression; /* default TOAST compression to
- * set during restore */
char *use_role; /* Issue SET ROLE to this */
/* error handling */
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 86de26a4bf..6b046e7734 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -86,7 +86,6 @@ static void _selectTableAccessMethod(ArchiveHandle *AH, const char *tableam);
static void processEncodingEntry(ArchiveHandle *AH, TocEntry *te);
static void processStdStringsEntry(ArchiveHandle *AH, TocEntry *te);
static void processSearchPathEntry(ArchiveHandle *AH, TocEntry *te);
-static void processToastCompressionEntry(ArchiveHandle *AH, TocEntry *te);
static int _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH);
static RestorePass _tocEntryRestorePass(TocEntry *te);
static bool _tocEntryIsACL(TocEntry *te);
@@ -2638,8 +2637,6 @@ ReadToc(ArchiveHandle *AH)
processStdStringsEntry(AH, te);
else if (strcmp(te->desc, "SEARCHPATH") == 0)
processSearchPathEntry(AH, te);
- else if (strcmp(te->desc, "TOASTCOMPRESSION") == 0)
- processToastCompressionEntry(AH, te);
}
}
@@ -2697,29 +2694,6 @@ processSearchPathEntry(ArchiveHandle *AH, TocEntry *te)
AH->public.searchpath = pg_strdup(te->defn);
}
-static void
-processToastCompressionEntry(ArchiveHandle *AH, TocEntry *te)
-{
- /* te->defn should have the form SET default_toast_compression = 'x'; */
- char *defn = pg_strdup(te->defn);
- char *ptr1;
- char *ptr2 = NULL;
-
- ptr1 = strchr(defn, '\'');
- if (ptr1)
- ptr2 = strchr(++ptr1, '\'');
- if (ptr2)
- {
- *ptr2 = '\0';
- AH->public.default_toast_compression = pg_strdup(ptr1);
- }
- else
- fatal("invalid TOASTCOMPRESSION item: %s",
- te->defn);
-
- free(defn);
-}
-
static void
StrictNamesCheck(RestoreOptions *ropt)
{
@@ -2779,8 +2753,7 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
/* These items are treated specially */
if (strcmp(te->desc, "ENCODING") == 0 ||
strcmp(te->desc, "STDSTRINGS") == 0 ||
- strcmp(te->desc, "SEARCHPATH") == 0 ||
- strcmp(te->desc, "TOASTCOMPRESSION") == 0)
+ strcmp(te->desc, "SEARCHPATH") == 0)
return REQ_SPECIAL;
/*
@@ -3103,11 +3076,6 @@ _doSetFixedOutputState(ArchiveHandle *AH)
if (AH->public.searchpath)
ahprintf(AH, "%s", AH->public.searchpath);
- /* Select the dump-time default_toast_compression */
- if (AH->public.default_toast_compression)
- ahprintf(AH, "SET default_toast_compression = '%s';\n",
- AH->public.default_toast_compression);
-
/* Make sure function checking is disabled */
ahprintf(AH, "SET check_function_bodies = false;\n");
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index b063e00cd4..e9a86ed512 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -276,7 +276,6 @@ static void dumpDatabaseConfig(Archive *AH, PQExpBuffer outbuf,
static void dumpEncoding(Archive *AH);
static void dumpStdStrings(Archive *AH);
static void dumpSearchPath(Archive *AH);
-static void dumpToastCompression(Archive *AH);
static void binary_upgrade_set_type_oids_by_type_oid(Archive *fout,
PQExpBuffer upgrade_buffer,
Oid pg_type_oid,
@@ -925,13 +924,11 @@ main(int argc, char **argv)
*/
/*
- * First the special entries for ENCODING, STDSTRINGS, SEARCHPATH and
- * TOASTCOMPRESSION.
+ * First the special entries for ENCODING, STDSTRINGS, and SEARCHPATH.
*/
dumpEncoding(fout);
dumpStdStrings(fout);
dumpSearchPath(fout);
- dumpToastCompression(fout);
/* The database items are always next, unless we don't want them at all */
if (dopt.outputCreateDB)
@@ -3398,58 +3395,6 @@ dumpSearchPath(Archive *AH)
destroyPQExpBuffer(path);
}
-/*
- * dumpToastCompression: save the dump-time default TOAST compression in the
- * archive
- */
-static void
-dumpToastCompression(Archive *AH)
-{
- char *toast_compression;
- PQExpBuffer qry;
-
- if (AH->dopt->no_toast_compression)
- {
- /* we don't intend to dump the info, so no need to fetch it either */
- return;
- }
-
- if (AH->remoteVersion < 140000)
- {
- /* pre-v14, the only method was pglz */
- toast_compression = pg_strdup("pglz");
- }
- else
- {
- PGresult *res;
-
- res = ExecuteSqlQueryForSingleRow(AH, "SHOW default_toast_compression");
- toast_compression = pg_strdup(PQgetvalue(res, 0, 0));
- PQclear(res);
- }
-
- qry = createPQExpBuffer();
- appendPQExpBufferStr(qry, "SET default_toast_compression = ");
- appendStringLiteralAH(qry, toast_compression, AH);
- appendPQExpBufferStr(qry, ";\n");
-
- pg_log_info("saving default_toast_compression = %s", toast_compression);
-
- ArchiveEntry(AH, nilCatalogId, createDumpId(),
- ARCHIVE_OPTS(.tag = "TOASTCOMPRESSION",
- .description = "TOASTCOMPRESSION",
- .section = SECTION_PRE_DATA,
- .createStmt = qry->data));
-
- /*
- * Also save it in AH->default_toast_compression, in case we're doing
- * plain text dump.
- */
- AH->default_toast_compression = toast_compression;
-
- destroyPQExpBuffer(qry);
-}
-
/*
* getBlobs:
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
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
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 95a302ffee..9f7f42c4aa 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8242,11 +8242,11 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
compression method for values of compressible columns.
(This can be overridden for individual columns by setting
the <literal>COMPRESSION</literal> column option in
- <command>CREATE TABLE</command> or
+ <command>CREATE TABLE</command> or
<command>ALTER TABLE</command>.)
- The supported compression methods are <literal>pglz</literal> and,
- if <productname>PostgreSQL</productname> was compiled with
- <literal>--with-lz4</literal>, <literal>lz4</literal>.
+ The supported compression methods are <literal>pglz</literal> and
+ (if <productname>PostgreSQL</productname> was compiled with
+ <option>--with-lz4</option>) <literal>lz4</literal>.
The default is <literal>pglz</literal>.
</para>
</listitem>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 3a21129021..08b07f561e 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -26253,10 +26253,10 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
<primary>pg_column_compression</primary>
</indexterm>
<function>pg_column_compression</function> ( <type>"any"</type> )
- <returnvalue>integer</returnvalue>
+ <returnvalue>text</returnvalue>
</para>
<para>
- Shows the compression algorithm that was used to compress a
+ Shows the compression algorithm that was used to compress
an individual variable-length value. Returns <literal>NULL</literal>
if the value is not compressed.
</para></entry>
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1431d2649b..939d3fe273 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -104,7 +104,6 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( <replaceable>sequence_options</replaceable> ) ] |
UNIQUE <replaceable class="parameter">index_parameters</replaceable> |
PRIMARY KEY <replaceable class="parameter">index_parameters</replaceable> |
- COMPRESSION <replaceable class="parameter">compression_method</replaceable> |
REFERENCES <replaceable class="parameter">reftable</replaceable> [ ( <replaceable
class="parameter">refcolumn</replaceable>) ] [ MATCH FULL | MATCH PARTIAL | MATCH SIMPLE ]
[ ON DELETE <replaceable class="parameter">referential_action</replaceable> ] [ ON UPDATE <replaceable
class="parameter">referential_action</replaceable>] }
[ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
@@ -391,24 +390,27 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
</term>
<listitem>
<para>
- This sets the compression method to be used for data inserted into a column.
-
+ This form sets the compression method for a column, determining how
+ values inserted in future will be compressed (if the storage mode
+ permits compression at all).
This does not cause the table to be rewritten, so existing data may still
be compressed with other compression methods. If the table is rewritten with
<command>VACUUM FULL</command> or <command>CLUSTER</command>, or restored
- with <application>pg_restore</application>, then all tuples are rewritten
- with the configured compression methods.
-
- Also, note that when data is inserted from another relation (for example,
- by <command>INSERT ... SELECT</command>), tuples from the source data are
- not necessarily detoasted, and any previously compressed data is retained
- with its existing compression method, rather than recompressing with the
- compression methods of the target columns.
-
+ with <application>pg_restore</application>, then all values are rewritten
+ with the configured compression method.
+ However, when data is inserted from another relation (for example,
+ by <command>INSERT ... SELECT</command>), values from the source table are
+ not necessarily detoasted, so any previously compressed data may retain
+ its existing compression method, rather than being recompressed with the
+ compression method of the target column.
The supported compression
methods are <literal>pglz</literal> and <literal>lz4</literal>.
- <literal>lz4</literal> is available only if <literal>--with-lz4</literal>
- was used when building <productname>PostgreSQL</productname>.
+ (<literal>lz4</literal> is available only if <option>--with-lz4</option>
+ was used when building <productname>PostgreSQL</productname>.) In
+ addition, <replaceable class="parameter">compression_method</replaceable>
+ can be <literal>default</literal>, which selects the default behavior of
+ consulting the <xref linkend="guc-default-toast-compression"/> setting
+ at the time of data insertion to determine the method to use.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index a8c5e4028a..c6d0a35e50 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable
class="parameter">table_name</replaceable>( [
- { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [
COLLATE<replaceable>collation</replaceable> ] [ COMPRESSION <replaceable>compression_method</replaceable> ] [
<replaceableclass="parameter">column_constraint</replaceable> [ ... ] ]
+ { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [
COMPRESSION<replaceable>compression_method</replaceable> ] [ COLLATE <replaceable>collation</replaceable> ] [
<replaceableclass="parameter">column_constraint</replaceable> [ ... ] ]
| <replaceable>table_constraint</replaceable>
| LIKE <replaceable>source_table</replaceable> [ <replaceable>like_option</replaceable> ... ] }
[, ... ]
@@ -293,17 +293,22 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<listitem>
<para>
The <literal>COMPRESSION</literal> clause sets the compression method
- for a column. Compression is supported only for variable-width data
- types, and is used only for columns whose storage type is main or
- extended. (See <xref linkend="sql-altertable"/> for information on
- column storage types.) Setting this property for a partitioned table
+ for the column. Compression is supported only for variable-width data
+ types, and is used only when the column's storage mode
+ is <literal>main</literal> or <literal>extended</literal>.
+ (See <xref linkend="sql-altertable"/> for information on
+ column storage modes.) Setting this property for a partitioned table
has no direct effect, because such tables have no storage of their own,
- but the configured value is inherited by newly-created partitions.
+ but the configured value will be inherited by newly-created partitions.
The supported compression methods are <literal>pglz</literal> and
- <literal>lz4</literal>. <literal>lz4</literal> is available only if
- <literal>--with-lz4</literal> was used when building
- <productname>PostgreSQL</productname>. The default
- is <literal>pglz</literal>.
+ <literal>lz4</literal>. (<literal>lz4</literal> is available only if
+ <option>--with-lz4</option> was used when building
+ <productname>PostgreSQL</productname>.) In addition,
+ <replaceable class="parameter">compression_method</replaceable>
+ can be <literal>default</literal> to explicitly specify the default
+ behavior, which is to consult the
+ <xref linkend="guc-default-toast-compression"/> setting at the time of
+ data insertion to determine the method to use.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 67c2cbbec6..5ddadc11f2 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -975,8 +975,8 @@ PostgreSQL documentation
<para>
Do not output commands to set <acronym>TOAST</acronym> compression
methods.
- With this option, all objects will be created using whichever
- compression method is the default during restore.
+ With this option, all columns will be restored with the default
+ compression setting.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 805c47d5c1..ddffbf85ed 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -464,12 +464,12 @@ PostgreSQL documentation
<para>
Do not output commands to set <acronym>TOAST</acronym> compression
methods.
- With this option, all objects will be created using whichever
- compression method is the default during restore.
+ With this option, all columns will be restored with the default
+ compression setting.
</para>
</listitem>
</varlistentry>
-
+
<varlistentry>
<term><option>--no-unlogged-table-data</option></term>
<listitem>
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index bfccda77af..7136bbe7a3 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -376,6 +376,16 @@ but the varlena header does not tell whether it has occurred —
the content of the <acronym>TOAST</acronym> pointer tells that, instead.
</para>
+<para>
+The compression technique used for either in-line or out-of-line compressed
+data can be selected for each column by setting
+the <literal>COMPRESSION</literal> column option in <command>CREATE
+TABLE</command> or <command>ALTER TABLE</command>. The default for columns
+with no explicit setting is to consult the
+<xref linkend="guc-default-toast-compression"/> parameter at the time data is
+inserted.
+</para>
+
<para>
As mentioned, there are multiple types of <acronym>TOAST</acronym> pointer datums.
The oldest and most common type is a pointer to out-of-line data stored in
@@ -392,13 +402,6 @@ useful for avoiding copying and redundant processing of large data values.
Further details appear in <xref linkend="storage-toast-inmemory"/>.
</para>
-<para>
-The compression technique used for either in-line or out-of-line compressed
-data can be selected using the <literal>COMPRESSION</literal> option on a per-column
-basis when creating a table. The default for columns with no explicit setting
-is taken from the value of <xref linkend="guc-default-toast-compression" />.
-</para>
-
<sect2 id="storage-toast-ondisk">
<title>Out-of-Line, On-Disk TOAST Storage</title>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e87d9cda93..e14039e971 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7982,23 +7982,24 @@ ATExecSetOptions(Relation rel, const char *colName, Node *options,
/*
* Helper function for ATExecSetStorage and ATExecSetCompression
*
- * Set the attcompression and/or attstorage for the respective index attribute
- * if the respective input values are valid.
+ * Set the attstorage and/or attcompression fields for index columns
+ * associated with the specified table column.
*/
static void
SetIndexStorageProperties(Relation rel, Relation attrelation,
- AttrNumber attnum, char newcompression,
- char newstorage, LOCKMODE lockmode)
+ AttrNumber attnum,
+ bool setstorage, char newstorage,
+ bool setcompression, char newcompression,
+ LOCKMODE lockmode)
{
- HeapTuple tuple;
ListCell *lc;
- Form_pg_attribute attrtuple;
foreach(lc, RelationGetIndexList(rel))
{
Oid indexoid = lfirst_oid(lc);
Relation indrel;
AttrNumber indattnum = 0;
+ HeapTuple tuple;
indrel = index_open(indexoid, lockmode);
@@ -8021,14 +8022,14 @@ SetIndexStorageProperties(Relation rel, Relation attrelation,
if (HeapTupleIsValid(tuple))
{
- attrtuple = (Form_pg_attribute) GETSTRUCT(tuple);
+ Form_pg_attribute attrtuple = (Form_pg_attribute) GETSTRUCT(tuple);
- if (CompressionMethodIsValid(newcompression))
- attrtuple->attcompression = newcompression;
-
- if (newstorage != '\0')
+ if (setstorage)
attrtuple->attstorage = newstorage;
+ if (setcompression)
+ attrtuple->attcompression = newcompression;
+
CatalogTupleUpdate(attrelation, &tuple->t_self, tuple);
InvokeObjectPostAlterHook(RelationRelationId,
@@ -8121,8 +8122,9 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc
* matching behavior of index.c ConstructTupleDescriptor()).
*/
SetIndexStorageProperties(rel, attrelation, attnum,
- InvalidCompressionMethod,
- newstorage, lockmode);
+ true, newstorage,
+ false, 0,
+ lockmode);
table_close(attrelation, RowExclusiveLock);
@@ -15626,7 +15628,10 @@ ATExecSetCompression(AlteredTableInfo *tab,
* Apply the change to indexes as well (only for simple index columns,
* matching behavior of index.c ConstructTupleDescriptor()).
*/
- SetIndexStorageProperties(rel, attrel, attnum, cmethod, '\0', lockmode);
+ SetIndexStorageProperties(rel, attrel, attnum,
+ false, 0,
+ true, cmethod,
+ lockmode);
heap_freetuple(tuple);
@@ -18581,7 +18586,7 @@ GetAttributeCompression(Oid atttypid, char *compression)
char typstorage;
char cmethod;
- if (compression == NULL)
+ if (compression == NULL || strcmp(compression, "default") == 0)
return InvalidCompressionMethod;
/*
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index aaf1a51f68..9ee90e3f13 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -561,6 +561,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <node> TableConstraint TableLikeClause
%type <ival> TableLikeOptionList TableLikeOption
+%type <str> column_compression opt_column_compression
%type <list> ColQualList
%type <node> ColConstraint ColConstraintElem ConstraintAttr
%type <ival> key_actions key_delete key_match key_update key_action
@@ -609,7 +610,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <list> hash_partbound
%type <defelt> hash_partbound_elem
-%type <str> optColumnCompression
/*
* Non-keyword token types. These are hard-wired into the "flex" lexer.
@@ -2302,6 +2302,15 @@ alter_table_cmd:
n->def = (Node *) makeString($6);
$$ = (Node *)n;
}
+ /* ALTER TABLE <name> ALTER [COLUMN] <colname> SET COMPRESSION <cm> */
+ | ALTER opt_column ColId SET column_compression
+ {
+ AlterTableCmd *n = makeNode(AlterTableCmd);
+ n->subtype = AT_SetCompression;
+ n->name = $3;
+ n->def = (Node *) makeString($5);
+ $$ = (Node *)n;
+ }
/* ALTER TABLE <name> ALTER [COLUMN] <colname> ADD GENERATED ... AS IDENTITY ... */
| ALTER opt_column ColId ADD_P GENERATED generated_when AS IDENTITY_P OptParenthesizedSeqOptList
{
@@ -2346,15 +2355,6 @@ alter_table_cmd:
n->missing_ok = true;
$$ = (Node *)n;
}
- /* ALTER TABLE <name> ALTER [COLUMN] <colname> SET (COMPRESSION <cm>) */
- | ALTER opt_column ColId SET optColumnCompression
- {
- AlterTableCmd *n = makeNode(AlterTableCmd);
- n->subtype = AT_SetCompression;
- n->name = $3;
- n->def = (Node *) makeString($5);
- $$ = (Node *)n;
- }
/* ALTER TABLE <name> DROP [COLUMN] IF EXISTS <colname> [RESTRICT|CASCADE] */
| DROP opt_column IF_P EXISTS ColId opt_drop_behavior
{
@@ -3462,7 +3462,7 @@ TypedTableElement:
| TableConstraint { $$ = $1; }
;
-columnDef: ColId Typename optColumnCompression create_generic_options ColQualList
+columnDef: ColId Typename opt_column_compression create_generic_options ColQualList
{
ColumnDef *n = makeNode(ColumnDef);
n->colname = $1;
@@ -3522,13 +3522,15 @@ columnOptions: ColId ColQualList
}
;
-optColumnCompression:
- COMPRESSION name
- {
- $$ = $2;
- }
- | /*EMPTY*/ { $$ = NULL; }
- ;
+column_compression:
+ COMPRESSION ColId { $$ = $2; }
+ | COMPRESSION DEFAULT { $$ = pstrdup("default"); }
+ ;
+
+opt_column_compression:
+ column_compression { $$ = $1; }
+ | /*EMPTY*/ { $$ = NULL; }
+ ;
ColQualList:
ColQualList ColConstraint { $$ = lappend($1, $2); }
diff --git a/src/test/regress/expected/compression.out b/src/test/regress/expected/compression.out
index 79e929673a..5c645e4650 100644
--- a/src/test/regress/expected/compression.out
+++ b/src/test/regress/expected/compression.out
@@ -158,18 +158,19 @@ ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE int USING f1::integer;
--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;
\d+ cmdata2
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+-------------------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | character varying | | | | extended | | |
+ f1 | character varying | | | | extended | pglz | |
ALTER TABLE cmdata2 ALTER COLUMN f1 SET STORAGE plain;
\d+ cmdata2
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+-------------------+-----------+----------+---------+---------+-------------+--------------+-------------
- f1 | character varying | | | | plain | | |
+ f1 | character varying | | | | plain | pglz | |
INSERT INTO cmdata2 VALUES (repeat('123456789', 800));
SELECT pg_column_compression(f1) FROM cmdata2;
@@ -179,9 +180,9 @@ SELECT pg_column_compression(f1) FROM cmdata2;
(1 row)
-- test compression with materialized view
-CREATE MATERIALIZED VIEW mv(x) AS SELECT * FROM cmdata1;
-\d+ mv
- Materialized view "public.mv"
+CREATE MATERIALIZED VIEW compressmv(x) AS SELECT * FROM cmdata1;
+\d+ compressmv
+ Materialized view "public.compressmv"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+------+-----------+----------+---------+----------+-------------+--------------+-------------
x | text | | | | extended | | |
@@ -196,7 +197,7 @@ SELECT pg_column_compression(f1) FROM cmdata1;
lz4
(2 rows)
-SELECT pg_column_compression(x) FROM mv;
+SELECT pg_column_compression(x) FROM compressmv;
pg_column_compression
-----------------------
lz4
@@ -222,7 +223,7 @@ SELECT pg_column_compression(f1) FROM cmpart2;
pglz
(1 row)
--- test compression with inheritence, error
+-- test compression with inheritance, error
CREATE TABLE cminh() INHERITS(cmdata, cmdata1);
NOTICE: merging multiple inherited definitions of column "f1"
ERROR: column "f1" has a compression method conflict
@@ -239,14 +240,6 @@ SET default_toast_compression = 'I do not exist compression';
ERROR: invalid value for parameter "default_toast_compression": "I do not exist compression"
HINT: Available values: pglz, lz4.
SET default_toast_compression = 'lz4';
-DROP TABLE cmdata2;
-CREATE TABLE cmdata2 (f1 text);
-\d+ cmdata2
- Table "public.cmdata2"
- Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
---------+------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | text | | | | extended | | |
-
SET default_toast_compression = 'pglz';
-- test alter compression method
ALTER TABLE cmdata ALTER COLUMN f1 SET COMPRESSION lz4;
@@ -266,10 +259,17 @@ SELECT pg_column_compression(f1) FROM cmdata;
lz4
(2 rows)
--- test alter compression method for the materialized view
-ALTER MATERIALIZED VIEW mv ALTER COLUMN x SET COMPRESSION lz4;
-\d+ mv
- Materialized view "public.mv"
+ALTER TABLE cmdata2 ALTER COLUMN f1 SET COMPRESSION default;
+\d+ cmdata2
+ Table "public.cmdata2"
+ Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
+--------+-------------------+-----------+----------+---------+---------+-------------+--------------+-------------
+ f1 | character varying | | | | plain | | |
+
+-- test alter compression method for materialized views
+ALTER MATERIALIZED VIEW compressmv ALTER COLUMN x SET COMPRESSION lz4;
+\d+ compressmv
+ Materialized view "public.compressmv"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+------+-----------+----------+---------+----------+-------------+--------------+-------------
x | text | | | | extended | lz4 | |
@@ -277,7 +277,7 @@ View definition:
SELECT cmdata1.f1 AS x
FROM cmdata1;
--- test alter compression method for the partitioned table
+-- test alter compression method for partitioned tables
ALTER TABLE cmpart1 ALTER COLUMN f1 SET COMPRESSION pglz;
ALTER TABLE cmpart2 ALTER COLUMN f1 SET COMPRESSION lz4;
-- new data should be compressed with the current compression method
diff --git a/src/test/regress/expected/compression_1.out b/src/test/regress/expected/compression_1.out
index 2a9fc81b51..aac96037fc 100644
--- a/src/test/regress/expected/compression_1.out
+++ b/src/test/regress/expected/compression_1.out
@@ -156,18 +156,19 @@ ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE int USING f1::integer;
--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;
\d+ cmdata2
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+-------------------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | character varying | | | | extended | | |
+ f1 | character varying | | | | extended | pglz | |
ALTER TABLE cmdata2 ALTER COLUMN f1 SET STORAGE plain;
\d+ cmdata2
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+-------------------+-----------+----------+---------+---------+-------------+--------------+-------------
- f1 | character varying | | | | plain | | |
+ f1 | character varying | | | | plain | pglz | |
INSERT INTO cmdata2 VALUES (repeat('123456789', 800));
SELECT pg_column_compression(f1) FROM cmdata2;
@@ -177,18 +178,18 @@ SELECT pg_column_compression(f1) FROM cmdata2;
(1 row)
-- test compression with materialized view
-CREATE MATERIALIZED VIEW mv(x) AS SELECT * FROM cmdata1;
+CREATE MATERIALIZED VIEW compressmv(x) AS SELECT * FROM cmdata1;
ERROR: relation "cmdata1" does not exist
-LINE 1: CREATE MATERIALIZED VIEW mv(x) AS SELECT * FROM cmdata1;
- ^
-\d+ mv
+LINE 1: ...TE MATERIALIZED VIEW compressmv(x) AS SELECT * FROM cmdata1;
+ ^
+\d+ compressmv
SELECT pg_column_compression(f1) FROM cmdata1;
ERROR: relation "cmdata1" does not exist
LINE 1: SELECT pg_column_compression(f1) FROM cmdata1;
^
-SELECT pg_column_compression(x) FROM mv;
-ERROR: relation "mv" does not exist
-LINE 1: SELECT pg_column_compression(x) FROM mv;
+SELECT pg_column_compression(x) FROM compressmv;
+ERROR: relation "compressmv" does not exist
+LINE 1: SELECT pg_column_compression(x) FROM compressmv;
^
-- test compression with partition
CREATE TABLE cmpart(f1 text COMPRESSION lz4) PARTITION BY HASH(f1);
@@ -217,7 +218,7 @@ SELECT pg_column_compression(f1) FROM cmpart2;
-----------------------
(0 rows)
--- test compression with inheritence, error
+-- test compression with inheritance, error
CREATE TABLE cminh() INHERITS(cmdata, cmdata1);
ERROR: relation "cmdata1" does not exist
CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata);
@@ -234,14 +235,6 @@ HINT: Available values: pglz.
SET default_toast_compression = 'lz4';
ERROR: invalid value for parameter "default_toast_compression": "lz4"
HINT: Available values: pglz.
-DROP TABLE cmdata2;
-CREATE TABLE cmdata2 (f1 text);
-\d+ cmdata2
- Table "public.cmdata2"
- Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
---------+------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | text | | | | extended | | |
-
SET default_toast_compression = 'pglz';
-- test alter compression method
ALTER TABLE cmdata ALTER COLUMN f1 SET COMPRESSION lz4;
@@ -264,11 +257,18 @@ SELECT pg_column_compression(f1) FROM cmdata;
pglz
(2 rows)
--- test alter compression method for the materialized view
-ALTER MATERIALIZED VIEW mv ALTER COLUMN x SET COMPRESSION lz4;
-ERROR: relation "mv" does not exist
-\d+ mv
--- test alter compression method for the partitioned table
+ALTER TABLE cmdata2 ALTER COLUMN f1 SET COMPRESSION default;
+\d+ cmdata2
+ Table "public.cmdata2"
+ Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
+--------+-------------------+-----------+----------+---------+---------+-------------+--------------+-------------
+ f1 | character varying | | | | plain | | |
+
+-- test alter compression method for materialized views
+ALTER MATERIALIZED VIEW compressmv ALTER COLUMN x SET COMPRESSION lz4;
+ERROR: relation "compressmv" does not exist
+\d+ compressmv
+-- test alter compression method for partitioned tables
ALTER TABLE cmpart1 ALTER COLUMN f1 SET COMPRESSION pglz;
ERROR: relation "cmpart1" does not exist
ALTER TABLE cmpart2 ALTER COLUMN f1 SET COMPRESSION lz4;
diff --git a/src/test/regress/sql/compression.sql b/src/test/regress/sql/compression.sql
index 76d1776d83..35557c1f7d 100644
--- a/src/test/regress/sql/compression.sql
+++ b/src/test/regress/sql/compression.sql
@@ -69,6 +69,7 @@ ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE int USING f1::integer;
--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;
\d+ cmdata2
ALTER TABLE cmdata2 ALTER COLUMN f1 SET STORAGE plain;
\d+ cmdata2
@@ -76,10 +77,10 @@ INSERT INTO cmdata2 VALUES (repeat('123456789', 800));
SELECT pg_column_compression(f1) FROM cmdata2;
-- test compression with materialized view
-CREATE MATERIALIZED VIEW mv(x) AS SELECT * FROM cmdata1;
-\d+ mv
+CREATE MATERIALIZED VIEW compressmv(x) AS SELECT * FROM cmdata1;
+\d+ compressmv
SELECT pg_column_compression(f1) FROM cmdata1;
-SELECT pg_column_compression(x) FROM mv;
+SELECT pg_column_compression(x) FROM compressmv;
-- test compression with partition
CREATE TABLE cmpart(f1 text COMPRESSION lz4) PARTITION BY HASH(f1);
@@ -92,7 +93,7 @@ INSERT INTO cmpart VALUES (repeat('123456789', 4004));
SELECT pg_column_compression(f1) FROM cmpart1;
SELECT pg_column_compression(f1) FROM cmpart2;
--- test compression with inheritence, error
+-- test compression with inheritance, error
CREATE TABLE cminh() INHERITS(cmdata, cmdata1);
CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata);
@@ -100,9 +101,6 @@ CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata);
SET default_toast_compression = '';
SET default_toast_compression = 'I do not exist compression';
SET default_toast_compression = 'lz4';
-DROP TABLE cmdata2;
-CREATE TABLE cmdata2 (f1 text);
-\d+ cmdata2
SET default_toast_compression = 'pglz';
-- test alter compression method
@@ -111,11 +109,14 @@ INSERT INTO cmdata VALUES (repeat('123456789', 4004));
\d+ cmdata
SELECT pg_column_compression(f1) FROM cmdata;
--- test alter compression method for the materialized view
-ALTER MATERIALIZED VIEW mv ALTER COLUMN x SET COMPRESSION lz4;
-\d+ mv
+ALTER TABLE cmdata2 ALTER COLUMN f1 SET COMPRESSION default;
+\d+ cmdata2
+
+-- test alter compression method for materialized views
+ALTER MATERIALIZED VIEW compressmv ALTER COLUMN x SET COMPRESSION lz4;
+\d+ compressmv
--- test alter compression method for the partitioned table
+-- test alter compression method for partitioned tables
ALTER TABLE cmpart1 ALTER COLUMN f1 SET COMPRESSION pglz;
ALTER TABLE cmpart2 ALTER COLUMN f1 SET COMPRESSION lz4;
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
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
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index b26b872a06..16493209c6 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1261,10 +1261,14 @@
<structfield>attcompression</structfield> <type>char</type>
</para>
<para>
- The current compression method of the column. If it is an invalid
- compression method (<literal>'\0'</literal>) then column data will not
- be compressed. Otherwise, <literal>'p'</literal> = pglz compression or
- <literal>'l'</literal> = <productname>LZ4</productname> compression.
+ The current compression method of the column. Typically this is
+ <literal>'\0'</literal> to specify use of the current default setting
+ (see <xref linkend="guc-default-toast-compression"/>). Otherwise,
+ <literal>'p'</literal> selects pglz compression, while
+ <literal>'l'</literal> selects <productname>LZ4</productname>
+ compression. However, this field is ignored
+ whenever <structfield>attstorage</structfield> does not allow
+ compression.
</para></entry>
</row>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 7e32b0686c..9f7f42c4aa 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8239,13 +8239,14 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
<para>
This variable sets the default
<link linkend="storage-toast">TOAST</link>
- compression method for columns of newly-created tables. The
- <command>CREATE TABLE</command> statement can override this default
- by specifying the <literal>COMPRESSION</literal> column option.
-
- The supported compression methods are <literal>pglz</literal> and,
- if <productname>PostgreSQL</productname> was compiled with
- <literal>--with-lz4</literal>, <literal>lz4</literal>.
+ compression method for values of compressible columns.
+ (This can be overridden for individual columns by setting
+ the <literal>COMPRESSION</literal> column option in
+ <command>CREATE TABLE</command> or
+ <command>ALTER TABLE</command>.)
+ The supported compression methods are <literal>pglz</literal> and
+ (if <productname>PostgreSQL</productname> was compiled with
+ <option>--with-lz4</option>) <literal>lz4</literal>.
The default is <literal>pglz</literal>.
</para>
</listitem>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 3a21129021..08b07f561e 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -26253,10 +26253,10 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
<primary>pg_column_compression</primary>
</indexterm>
<function>pg_column_compression</function> ( <type>"any"</type> )
- <returnvalue>integer</returnvalue>
+ <returnvalue>text</returnvalue>
</para>
<para>
- Shows the compression algorithm that was used to compress a
+ Shows the compression algorithm that was used to compress
an individual variable-length value. Returns <literal>NULL</literal>
if the value is not compressed.
</para></entry>
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1431d2649b..939d3fe273 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -104,7 +104,6 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( <replaceable>sequence_options</replaceable> ) ] |
UNIQUE <replaceable class="parameter">index_parameters</replaceable> |
PRIMARY KEY <replaceable class="parameter">index_parameters</replaceable> |
- COMPRESSION <replaceable class="parameter">compression_method</replaceable> |
REFERENCES <replaceable class="parameter">reftable</replaceable> [ ( <replaceable
class="parameter">refcolumn</replaceable>) ] [ MATCH FULL | MATCH PARTIAL | MATCH SIMPLE ]
[ ON DELETE <replaceable class="parameter">referential_action</replaceable> ] [ ON UPDATE <replaceable
class="parameter">referential_action</replaceable>] }
[ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
@@ -391,24 +390,27 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
</term>
<listitem>
<para>
- This sets the compression method to be used for data inserted into a column.
-
+ This form sets the compression method for a column, determining how
+ values inserted in future will be compressed (if the storage mode
+ permits compression at all).
This does not cause the table to be rewritten, so existing data may still
be compressed with other compression methods. If the table is rewritten with
<command>VACUUM FULL</command> or <command>CLUSTER</command>, or restored
- with <application>pg_restore</application>, then all tuples are rewritten
- with the configured compression methods.
-
- Also, note that when data is inserted from another relation (for example,
- by <command>INSERT ... SELECT</command>), tuples from the source data are
- not necessarily detoasted, and any previously compressed data is retained
- with its existing compression method, rather than recompressing with the
- compression methods of the target columns.
-
+ with <application>pg_restore</application>, then all values are rewritten
+ with the configured compression method.
+ However, when data is inserted from another relation (for example,
+ by <command>INSERT ... SELECT</command>), values from the source table are
+ not necessarily detoasted, so any previously compressed data may retain
+ its existing compression method, rather than being recompressed with the
+ compression method of the target column.
The supported compression
methods are <literal>pglz</literal> and <literal>lz4</literal>.
- <literal>lz4</literal> is available only if <literal>--with-lz4</literal>
- was used when building <productname>PostgreSQL</productname>.
+ (<literal>lz4</literal> is available only if <option>--with-lz4</option>
+ was used when building <productname>PostgreSQL</productname>.) In
+ addition, <replaceable class="parameter">compression_method</replaceable>
+ can be <literal>default</literal>, which selects the default behavior of
+ consulting the <xref linkend="guc-default-toast-compression"/> setting
+ at the time of data insertion to determine the method to use.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index a8c5e4028a..c6d0a35e50 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable
class="parameter">table_name</replaceable>( [
- { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [
COLLATE<replaceable>collation</replaceable> ] [ COMPRESSION <replaceable>compression_method</replaceable> ] [
<replaceableclass="parameter">column_constraint</replaceable> [ ... ] ]
+ { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [
COMPRESSION<replaceable>compression_method</replaceable> ] [ COLLATE <replaceable>collation</replaceable> ] [
<replaceableclass="parameter">column_constraint</replaceable> [ ... ] ]
| <replaceable>table_constraint</replaceable>
| LIKE <replaceable>source_table</replaceable> [ <replaceable>like_option</replaceable> ... ] }
[, ... ]
@@ -293,17 +293,22 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<listitem>
<para>
The <literal>COMPRESSION</literal> clause sets the compression method
- for a column. Compression is supported only for variable-width data
- types, and is used only for columns whose storage type is main or
- extended. (See <xref linkend="sql-altertable"/> for information on
- column storage types.) Setting this property for a partitioned table
+ for the column. Compression is supported only for variable-width data
+ types, and is used only when the column's storage mode
+ is <literal>main</literal> or <literal>extended</literal>.
+ (See <xref linkend="sql-altertable"/> for information on
+ column storage modes.) Setting this property for a partitioned table
has no direct effect, because such tables have no storage of their own,
- but the configured value is inherited by newly-created partitions.
+ but the configured value will be inherited by newly-created partitions.
The supported compression methods are <literal>pglz</literal> and
- <literal>lz4</literal>. <literal>lz4</literal> is available only if
- <literal>--with-lz4</literal> was used when building
- <productname>PostgreSQL</productname>. The default
- is <literal>pglz</literal>.
+ <literal>lz4</literal>. (<literal>lz4</literal> is available only if
+ <option>--with-lz4</option> was used when building
+ <productname>PostgreSQL</productname>.) In addition,
+ <replaceable class="parameter">compression_method</replaceable>
+ can be <literal>default</literal> to explicitly specify the default
+ behavior, which is to consult the
+ <xref linkend="guc-default-toast-compression"/> setting at the time of
+ data insertion to determine the method to use.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 67c2cbbec6..5ddadc11f2 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -975,8 +975,8 @@ PostgreSQL documentation
<para>
Do not output commands to set <acronym>TOAST</acronym> compression
methods.
- With this option, all objects will be created using whichever
- compression method is the default during restore.
+ With this option, all columns will be restored with the default
+ compression setting.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 805c47d5c1..ddffbf85ed 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -464,12 +464,12 @@ PostgreSQL documentation
<para>
Do not output commands to set <acronym>TOAST</acronym> compression
methods.
- With this option, all objects will be created using whichever
- compression method is the default during restore.
+ With this option, all columns will be restored with the default
+ compression setting.
</para>
</listitem>
</varlistentry>
-
+
<varlistentry>
<term><option>--no-unlogged-table-data</option></term>
<listitem>
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index bfccda77af..7136bbe7a3 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -376,6 +376,16 @@ but the varlena header does not tell whether it has occurred —
the content of the <acronym>TOAST</acronym> pointer tells that, instead.
</para>
+<para>
+The compression technique used for either in-line or out-of-line compressed
+data can be selected for each column by setting
+the <literal>COMPRESSION</literal> column option in <command>CREATE
+TABLE</command> or <command>ALTER TABLE</command>. The default for columns
+with no explicit setting is to consult the
+<xref linkend="guc-default-toast-compression"/> parameter at the time data is
+inserted.
+</para>
+
<para>
As mentioned, there are multiple types of <acronym>TOAST</acronym> pointer datums.
The oldest and most common type is a pointer to out-of-line data stored in
@@ -392,13 +402,6 @@ useful for avoiding copying and redundant processing of large data values.
Further details appear in <xref linkend="storage-toast-inmemory"/>.
</para>
-<para>
-The compression technique used for either in-line or out-of-line compressed
-data can be selected using the <literal>COMPRESSION</literal> option on a per-column
-basis when creating a table. The default for columns with no explicit setting
-is taken from the value of <xref linkend="guc-default-toast-compression" />.
-</para>
-
<sect2 id="storage-toast-ondisk">
<title>Out-of-Line, On-Disk TOAST Storage</title>
diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c
index ee05372f79..09e563b1f0 100644
--- a/src/backend/access/brin/brin_tuple.c
+++ b/src/backend/access/brin/brin_tuple.c
@@ -232,11 +232,10 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple,
* same compression method. Otherwise we have to use the
* default method.
*/
- if (att->atttypid == atttype->type_id &&
- CompressionMethodIsValid(att->attcompression))
+ if (att->atttypid == atttype->type_id)
compression = att->attcompression;
else
- compression = GetDefaultToastCompression();
+ compression = InvalidCompressionMethod;
cvalue = toast_compress_datum(value, compression);
diff --git a/src/backend/access/common/indextuple.c b/src/backend/access/common/indextuple.c
index 5212560411..8df882da7a 100644
--- a/src/backend/access/common/indextuple.c
+++ b/src/backend/access/common/indextuple.c
@@ -104,18 +104,9 @@ index_form_tuple(TupleDesc tupleDescriptor,
att->attstorage == TYPSTORAGE_MAIN))
{
Datum cvalue;
- char compression = att->attcompression;
- /*
- * If the compression method is not valid, use the default. We
- * don't expect this to happen for regular index columns, which
- * inherit the setting from the corresponding table column, but we
- * do expect it to happen whenever an expression is indexed.
- */
- if (!CompressionMethodIsValid(compression))
- compression = GetDefaultToastCompression();
-
- cvalue = toast_compress_datum(untoasted_values[i], compression);
+ cvalue = toast_compress_datum(untoasted_values[i],
+ att->attcompression);
if (DatumGetPointer(cvalue) != NULL)
{
diff --git a/src/backend/access/common/toast_internals.c b/src/backend/access/common/toast_internals.c
index 8d2a9964c3..c7b9ade574 100644
--- a/src/backend/access/common/toast_internals.c
+++ b/src/backend/access/common/toast_internals.c
@@ -53,10 +53,12 @@ toast_compress_datum(Datum value, char cmethod)
Assert(!VARATT_IS_EXTERNAL(DatumGetPointer(value)));
Assert(!VARATT_IS_COMPRESSED(DatumGetPointer(value)));
- Assert(CompressionMethodIsValid(cmethod));
-
valsize = VARSIZE_ANY_EXHDR(DatumGetPointer(value));
+ /* If the compression method is not valid, use the current default */
+ if (!CompressionMethodIsValid(cmethod))
+ cmethod = default_toast_compression;
+
/*
* Call appropriate compression routine for the compression method.
*/
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index affbc509bd..4c63bd4dc6 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -642,10 +642,7 @@ TupleDescInitEntry(TupleDesc desc,
att->attbyval = typeForm->typbyval;
att->attalign = typeForm->typalign;
att->attstorage = typeForm->typstorage;
- if (IsStorageCompressible(typeForm->typstorage))
- att->attcompression = GetDefaultToastCompression();
- else
- att->attcompression = InvalidCompressionMethod;
+ att->attcompression = InvalidCompressionMethod;
att->attcollation = typeForm->typcollation;
ReleaseSysCache(tuple);
@@ -711,7 +708,7 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
att->attbyval = false;
att->attalign = TYPALIGN_INT;
att->attstorage = TYPSTORAGE_EXTENDED;
- att->attcompression = GetDefaultToastCompression();
+ att->attcompression = InvalidCompressionMethod;
att->attcollation = DEFAULT_COLLATION_OID;
break;
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 8e6e8d5169..e2cd79ec54 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2483,10 +2483,10 @@ reform_and_rewrite_tuple(HeapTuple tuple,
* perform the compression here; we just need to decompress. That
* will trigger recompression later on.
*/
-
struct varlena *new_value;
ToastCompressionId cmid;
char cmethod;
+ char targetmethod;
new_value = (struct varlena *) DatumGetPointer(values[i]);
cmid = toast_get_compression_id(new_value);
@@ -2495,7 +2495,7 @@ reform_and_rewrite_tuple(HeapTuple tuple,
if (cmid == TOAST_INVALID_COMPRESSION_ID)
continue;
- /* convert compression id to compression method */
+ /* convert existing compression id to compression method */
switch (cmid)
{
case TOAST_PGLZ_COMPRESSION_ID:
@@ -2506,10 +2506,16 @@ reform_and_rewrite_tuple(HeapTuple tuple,
break;
default:
elog(ERROR, "invalid compression method id %d", cmid);
+ cmethod = '\0'; /* keep compiler quiet */
}
+ /* figure out what the target method is */
+ targetmethod = TupleDescAttr(newTupDesc, i)->attcompression;
+ if (!CompressionMethodIsValid(targetmethod))
+ targetmethod = default_toast_compression;
+
/* if compression method doesn't match then detoast the value */
- if (TupleDescAttr(newTupDesc, i)->attcompression != cmethod)
+ if (targetmethod != cmethod)
{
values[i] = PointerGetDatum(detoast_attr(new_value));
values_free[i] = true;
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 62abd008cc..94ab5ca095 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -701,6 +701,7 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
attrtypes[attnum]->attbyval = Ap->am_typ.typbyval;
attrtypes[attnum]->attalign = Ap->am_typ.typalign;
attrtypes[attnum]->attstorage = Ap->am_typ.typstorage;
+ attrtypes[attnum]->attcompression = InvalidCompressionMethod;
attrtypes[attnum]->attcollation = Ap->am_typ.typcollation;
/* if an array type, assume 1-dimensional attribute */
if (Ap->am_typ.typelem != InvalidOid && Ap->am_typ.typlen < 0)
@@ -715,6 +716,7 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
attrtypes[attnum]->attbyval = TypInfo[typeoid].byval;
attrtypes[attnum]->attalign = TypInfo[typeoid].align;
attrtypes[attnum]->attstorage = TypInfo[typeoid].storage;
+ attrtypes[attnum]->attcompression = InvalidCompressionMethod;
attrtypes[attnum]->attcollation = TypInfo[typeoid].collation;
/* if an array type, assume 1-dimensional attribute */
if (TypInfo[typeoid].elem != InvalidOid &&
@@ -724,11 +726,6 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
attrtypes[attnum]->attndims = 0;
}
- if (IsStorageCompressible(attrtypes[attnum]->attstorage))
- attrtypes[attnum]->attcompression = GetDefaultToastCompression();
- else
- attrtypes[attnum]->attcompression = InvalidCompressionMethod;
-
/*
* If a system catalog column is collation-aware, force it to use C
* collation, so that its behavior is independent of the database's
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 112b3affdf..f893ae4f45 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -899,9 +899,7 @@ sub morph_row_for_pgattr
$row->{attbyval} = $type->{typbyval};
$row->{attalign} = $type->{typalign};
$row->{attstorage} = $type->{typstorage};
-
- $row->{attcompression} =
- $type->{typstorage} ne 'p' && $type->{typstorage} ne 'e' ? 'p' : '\0';
+ $row->{attcompression} = '\0';
# set attndims if it's an array type
$row->{attndims} = $type->{typcategory} eq 'A' ? '1' : '0';
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 3dfe2e8a56..afa830d924 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1719,8 +1719,6 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
/* Unset this so no one tries to look up the generation expression */
attStruct->attgenerated = '\0';
- attStruct->attcompression = InvalidCompressionMethod;
-
/*
* Change the column name to something that isn't likely to conflict
*/
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 11e91c4ad3..e14039e971 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -601,7 +601,7 @@ static void refuseDupeIndexAttach(Relation parentIdx, Relation partIdx,
Relation partitionTbl);
static List *GetParentedForeignKeyRefs(Relation partition);
static void ATDetachCheckNoForeignKeyRefs(Relation partition);
-static char GetAttributeCompression(Form_pg_attribute att, char *compression);
+static char GetAttributeCompression(Oid atttypid, char *compression);
/* ----------------------------------------------------------------
@@ -897,17 +897,9 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
if (colDef->generated)
attr->attgenerated = colDef->generated;
- /*
- * lookup attribute's compression method and store it in the
- * attr->attcompression.
- */
- if (relkind == RELKIND_RELATION ||
- relkind == RELKIND_PARTITIONED_TABLE ||
- relkind == RELKIND_MATVIEW)
- attr->attcompression =
- GetAttributeCompression(attr, colDef->compression);
- else
- attr->attcompression = InvalidCompressionMethod;
+ if (colDef->compression)
+ attr->attcompression = GetAttributeCompression(attr->atttypid,
+ colDef->compression);
}
/*
@@ -6602,13 +6594,8 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
attribute.attbyval = tform->typbyval;
attribute.attalign = tform->typalign;
attribute.attstorage = tform->typstorage;
- /* do not set compression in views etc */
- if (rel->rd_rel->relkind == RELKIND_RELATION ||
- rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- attribute.attcompression = GetAttributeCompression(&attribute,
- colDef->compression);
- else
- attribute.attcompression = InvalidCompressionMethod;
+ attribute.attcompression = GetAttributeCompression(typeOid,
+ colDef->compression);
attribute.attnotnull = colDef->is_not_null;
attribute.atthasdef = false;
attribute.atthasmissing = false;
@@ -7995,23 +7982,24 @@ ATExecSetOptions(Relation rel, const char *colName, Node *options,
/*
* Helper function for ATExecSetStorage and ATExecSetCompression
*
- * Set the attcompression and/or attstorage for the respective index attribute
- * if the respective input values are valid.
+ * Set the attstorage and/or attcompression fields for index columns
+ * associated with the specified table column.
*/
static void
SetIndexStorageProperties(Relation rel, Relation attrelation,
- AttrNumber attnum, char newcompression,
- char newstorage, LOCKMODE lockmode)
+ AttrNumber attnum,
+ bool setstorage, char newstorage,
+ bool setcompression, char newcompression,
+ LOCKMODE lockmode)
{
- HeapTuple tuple;
ListCell *lc;
- Form_pg_attribute attrtuple;
foreach(lc, RelationGetIndexList(rel))
{
Oid indexoid = lfirst_oid(lc);
Relation indrel;
AttrNumber indattnum = 0;
+ HeapTuple tuple;
indrel = index_open(indexoid, lockmode);
@@ -8034,14 +8022,14 @@ SetIndexStorageProperties(Relation rel, Relation attrelation,
if (HeapTupleIsValid(tuple))
{
- attrtuple = (Form_pg_attribute) GETSTRUCT(tuple);
-
- if (CompressionMethodIsValid(newcompression))
- attrtuple->attcompression = newcompression;
+ Form_pg_attribute attrtuple = (Form_pg_attribute) GETSTRUCT(tuple);
- if (newstorage != '\0')
+ if (setstorage)
attrtuple->attstorage = newstorage;
+ if (setcompression)
+ attrtuple->attcompression = newcompression;
+
CatalogTupleUpdate(attrelation, &tuple->t_self, tuple);
InvokeObjectPostAlterHook(RelationRelationId,
@@ -8134,8 +8122,9 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc
* matching behavior of index.c ConstructTupleDescriptor()).
*/
SetIndexStorageProperties(rel, attrelation, attnum,
- InvalidCompressionMethod,
- newstorage, lockmode);
+ true, newstorage,
+ false, 0,
+ lockmode);
table_close(attrelation, RowExclusiveLock);
@@ -12299,23 +12288,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
attTup->attbyval = tform->typbyval;
attTup->attalign = tform->typalign;
attTup->attstorage = tform->typstorage;
-
- /* Setup attribute compression */
- if (rel->rd_rel->relkind == RELKIND_RELATION ||
- rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- {
- /*
- * No compression for plain/external storage, otherwise, default
- * compression method if it is not already set, refer comments atop
- * attcompression parameter in pg_attribute.h.
- */
- if (!IsStorageCompressible(tform->typstorage))
- attTup->attcompression = InvalidCompressionMethod;
- else if (!CompressionMethodIsValid(attTup->attcompression))
- attTup->attcompression = GetDefaultToastCompression();
- }
- else
- attTup->attcompression = InvalidCompressionMethod;
+ attTup->attcompression = InvalidCompressionMethod;
ReleaseSysCache(typeTuple);
@@ -15613,7 +15586,6 @@ ATExecSetCompression(AlteredTableInfo *tab,
Form_pg_attribute atttableform;
AttrNumber attnum;
char *compression;
- char typstorage;
char cmethod;
ObjectAddress address;
@@ -15638,17 +15610,11 @@ ATExecSetCompression(AlteredTableInfo *tab,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot alter system column \"%s\"", column)));
- typstorage = get_typstorage(atttableform->atttypid);
-
- /* prevent from setting compression methods for uncompressible type */
- if (!IsStorageCompressible(typstorage))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("column data type %s does not support compression",
- format_type_be(atttableform->atttypid))));
-
- /* get the attribute compression method. */
- cmethod = GetAttributeCompression(atttableform, compression);
+ /*
+ * Check that column type is compressible, then get the attribute
+ * compression method code
+ */
+ cmethod = GetAttributeCompression(atttableform->atttypid, compression);
/* update pg_attribute entry */
atttableform->attcompression = cmethod;
@@ -15662,7 +15628,10 @@ ATExecSetCompression(AlteredTableInfo *tab,
* Apply the change to indexes as well (only for simple index columns,
* matching behavior of index.c ConstructTupleDescriptor()).
*/
- SetIndexStorageProperties(rel, attrel, attnum, cmethod, '\0', lockmode);
+ SetIndexStorageProperties(rel, attrel, attnum,
+ false, 0,
+ true, cmethod,
+ lockmode);
heap_freetuple(tuple);
@@ -18612,29 +18581,26 @@ ATDetachCheckNoForeignKeyRefs(Relation partition)
* resolve column compression specification to compression method.
*/
static char
-GetAttributeCompression(Form_pg_attribute att, char *compression)
+GetAttributeCompression(Oid atttypid, char *compression)
{
- char typstorage = get_typstorage(att->atttypid);
+ char typstorage;
char cmethod;
+ if (compression == NULL || strcmp(compression, "default") == 0)
+ return InvalidCompressionMethod;
+
/*
- * No compression for plain/external storage, refer comments atop
- * attcompression parameter in pg_attribute.h
+ * To specify a nondefault method, the column data type must be
+ * compressible. Note this says nothing about whether the column's
+ * attstorage setting permits compression; we intentionally allow
+ * attstorage and attcompression to be independent.
*/
+ typstorage = get_typstorage(atttypid);
if (!IsStorageCompressible(typstorage))
- {
- if (compression == NULL)
- return InvalidCompressionMethod;
-
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("column data type %s does not support compression",
- format_type_be(att->atttypid))));
- }
-
- /* fallback to default compression if it's not specified */
- if (compression == NULL)
- return GetDefaultToastCompression();
+ format_type_be(atttypid))));
cmethod = CompressionNameToMethod(compression);
if (!CompressionMethodIsValid(cmethod))
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index aaf1a51f68..9ee90e3f13 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -561,6 +561,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <node> TableConstraint TableLikeClause
%type <ival> TableLikeOptionList TableLikeOption
+%type <str> column_compression opt_column_compression
%type <list> ColQualList
%type <node> ColConstraint ColConstraintElem ConstraintAttr
%type <ival> key_actions key_delete key_match key_update key_action
@@ -609,7 +610,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <list> hash_partbound
%type <defelt> hash_partbound_elem
-%type <str> optColumnCompression
/*
* Non-keyword token types. These are hard-wired into the "flex" lexer.
@@ -2302,6 +2302,15 @@ alter_table_cmd:
n->def = (Node *) makeString($6);
$$ = (Node *)n;
}
+ /* ALTER TABLE <name> ALTER [COLUMN] <colname> SET COMPRESSION <cm> */
+ | ALTER opt_column ColId SET column_compression
+ {
+ AlterTableCmd *n = makeNode(AlterTableCmd);
+ n->subtype = AT_SetCompression;
+ n->name = $3;
+ n->def = (Node *) makeString($5);
+ $$ = (Node *)n;
+ }
/* ALTER TABLE <name> ALTER [COLUMN] <colname> ADD GENERATED ... AS IDENTITY ... */
| ALTER opt_column ColId ADD_P GENERATED generated_when AS IDENTITY_P OptParenthesizedSeqOptList
{
@@ -2346,15 +2355,6 @@ alter_table_cmd:
n->missing_ok = true;
$$ = (Node *)n;
}
- /* ALTER TABLE <name> ALTER [COLUMN] <colname> SET (COMPRESSION <cm>) */
- | ALTER opt_column ColId SET optColumnCompression
- {
- AlterTableCmd *n = makeNode(AlterTableCmd);
- n->subtype = AT_SetCompression;
- n->name = $3;
- n->def = (Node *) makeString($5);
- $$ = (Node *)n;
- }
/* ALTER TABLE <name> DROP [COLUMN] IF EXISTS <colname> [RESTRICT|CASCADE] */
| DROP opt_column IF_P EXISTS ColId opt_drop_behavior
{
@@ -3462,7 +3462,7 @@ TypedTableElement:
| TableConstraint { $$ = $1; }
;
-columnDef: ColId Typename optColumnCompression create_generic_options ColQualList
+columnDef: ColId Typename opt_column_compression create_generic_options ColQualList
{
ColumnDef *n = makeNode(ColumnDef);
n->colname = $1;
@@ -3522,13 +3522,15 @@ columnOptions: ColId ColQualList
}
;
-optColumnCompression:
- COMPRESSION name
- {
- $$ = $2;
- }
- | /*EMPTY*/ { $$ = NULL; }
- ;
+column_compression:
+ COMPRESSION ColId { $$ = $2; }
+ | COMPRESSION DEFAULT { $$ = pstrdup("default"); }
+ ;
+
+opt_column_compression:
+ column_compression { $$ = $1; }
+ | /*EMPTY*/ { $$ = NULL; }
+ ;
ColQualList:
ColQualList ColConstraint { $$ = lappend($1, $2); }
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ee731044b6..87bc688704 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4651,13 +4651,13 @@ static struct config_enum ConfigureNamesEnum[] =
{
{"default_toast_compression", PGC_USERSET, CLIENT_CONN_STATEMENT,
- gettext_noop("Sets the default compression for new columns."),
- NULL,
- GUC_IS_NAME
+ gettext_noop("Sets the default compression method for compressible values."),
+ NULL
},
&default_toast_compression,
TOAST_PGLZ_COMPRESSION,
- default_toast_compression_options, NULL, NULL
+ default_toast_compression_options,
+ NULL, NULL, NULL
},
{
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index fc054af5ba..3c1cd858a8 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -208,8 +208,6 @@ typedef struct Archive
/* other important stuff */
char *searchpath; /* search_path to set during restore */
- char *default_toast_compression; /* default TOAST compression to
- * set during restore */
char *use_role; /* Issue SET ROLE to this */
/* error handling */
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 86de26a4bf..6b046e7734 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -86,7 +86,6 @@ static void _selectTableAccessMethod(ArchiveHandle *AH, const char *tableam);
static void processEncodingEntry(ArchiveHandle *AH, TocEntry *te);
static void processStdStringsEntry(ArchiveHandle *AH, TocEntry *te);
static void processSearchPathEntry(ArchiveHandle *AH, TocEntry *te);
-static void processToastCompressionEntry(ArchiveHandle *AH, TocEntry *te);
static int _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH);
static RestorePass _tocEntryRestorePass(TocEntry *te);
static bool _tocEntryIsACL(TocEntry *te);
@@ -2638,8 +2637,6 @@ ReadToc(ArchiveHandle *AH)
processStdStringsEntry(AH, te);
else if (strcmp(te->desc, "SEARCHPATH") == 0)
processSearchPathEntry(AH, te);
- else if (strcmp(te->desc, "TOASTCOMPRESSION") == 0)
- processToastCompressionEntry(AH, te);
}
}
@@ -2697,29 +2694,6 @@ processSearchPathEntry(ArchiveHandle *AH, TocEntry *te)
AH->public.searchpath = pg_strdup(te->defn);
}
-static void
-processToastCompressionEntry(ArchiveHandle *AH, TocEntry *te)
-{
- /* te->defn should have the form SET default_toast_compression = 'x'; */
- char *defn = pg_strdup(te->defn);
- char *ptr1;
- char *ptr2 = NULL;
-
- ptr1 = strchr(defn, '\'');
- if (ptr1)
- ptr2 = strchr(++ptr1, '\'');
- if (ptr2)
- {
- *ptr2 = '\0';
- AH->public.default_toast_compression = pg_strdup(ptr1);
- }
- else
- fatal("invalid TOASTCOMPRESSION item: %s",
- te->defn);
-
- free(defn);
-}
-
static void
StrictNamesCheck(RestoreOptions *ropt)
{
@@ -2779,8 +2753,7 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
/* These items are treated specially */
if (strcmp(te->desc, "ENCODING") == 0 ||
strcmp(te->desc, "STDSTRINGS") == 0 ||
- strcmp(te->desc, "SEARCHPATH") == 0 ||
- strcmp(te->desc, "TOASTCOMPRESSION") == 0)
+ strcmp(te->desc, "SEARCHPATH") == 0)
return REQ_SPECIAL;
/*
@@ -3103,11 +3076,6 @@ _doSetFixedOutputState(ArchiveHandle *AH)
if (AH->public.searchpath)
ahprintf(AH, "%s", AH->public.searchpath);
- /* Select the dump-time default_toast_compression */
- if (AH->public.default_toast_compression)
- ahprintf(AH, "SET default_toast_compression = '%s';\n",
- AH->public.default_toast_compression);
-
/* Make sure function checking is disabled */
ahprintf(AH, "SET check_function_bodies = false;\n");
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 339c393718..e9a86ed512 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -276,7 +276,6 @@ static void dumpDatabaseConfig(Archive *AH, PQExpBuffer outbuf,
static void dumpEncoding(Archive *AH);
static void dumpStdStrings(Archive *AH);
static void dumpSearchPath(Archive *AH);
-static void dumpToastCompression(Archive *AH);
static void binary_upgrade_set_type_oids_by_type_oid(Archive *fout,
PQExpBuffer upgrade_buffer,
Oid pg_type_oid,
@@ -925,13 +924,11 @@ main(int argc, char **argv)
*/
/*
- * First the special entries for ENCODING, STDSTRINGS, SEARCHPATH and
- * TOASTCOMPRESSION.
+ * First the special entries for ENCODING, STDSTRINGS, and SEARCHPATH.
*/
dumpEncoding(fout);
dumpStdStrings(fout);
dumpSearchPath(fout);
- dumpToastCompression(fout);
/* The database items are always next, unless we don't want them at all */
if (dopt.outputCreateDB)
@@ -3398,58 +3395,6 @@ dumpSearchPath(Archive *AH)
destroyPQExpBuffer(path);
}
-/*
- * dumpToastCompression: save the dump-time default TOAST compression in the
- * archive
- */
-static void
-dumpToastCompression(Archive *AH)
-{
- char *toast_compression;
- PQExpBuffer qry;
-
- if (AH->dopt->no_toast_compression)
- {
- /* we don't intend to dump the info, so no need to fetch it either */
- return;
- }
-
- if (AH->remoteVersion < 140000)
- {
- /* pre-v14, the only method was pglz */
- toast_compression = pg_strdup("pglz");
- }
- else
- {
- PGresult *res;
-
- res = ExecuteSqlQueryForSingleRow(AH, "SHOW default_toast_compression");
- toast_compression = pg_strdup(PQgetvalue(res, 0, 0));
- PQclear(res);
- }
-
- qry = createPQExpBuffer();
- appendPQExpBufferStr(qry, "SET default_toast_compression = ");
- appendStringLiteralAH(qry, toast_compression, AH);
- appendPQExpBufferStr(qry, ";\n");
-
- pg_log_info("saving default_toast_compression = %s", toast_compression);
-
- ArchiveEntry(AH, nilCatalogId, createDumpId(),
- ARCHIVE_OPTS(.tag = "TOASTCOMPRESSION",
- .description = "TOASTCOMPRESSION",
- .section = SECTION_PRE_DATA,
- .createStmt = qry->data));
-
- /*
- * Also save it in AH->default_toast_compression, in case we're doing
- * plain text dump.
- */
- AH->default_toast_compression = toast_compression;
-
- destroyPQExpBuffer(qry);
-}
-
/*
* getBlobs:
@@ -16421,7 +16366,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
tbinfo->attfdwoptions[j]);
/*
- * Dump per-column compression, if different from default.
+ * Dump per-column compression, if it's been set.
*/
if (!dopt->no_toast_compression)
{
@@ -16440,9 +16385,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
break;
}
- if (cmname != NULL &&
- (fout->default_toast_compression == NULL ||
- strcmp(cmname, fout->default_toast_compression) != 0))
+ if (cmname != NULL)
appendPQExpBuffer(q, "ALTER %sTABLE ONLY %s ALTER COLUMN %s SET COMPRESSION %s;\n",
foreign, qualrelname,
fmtId(tbinfo->attnames[j]),
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 3e39fdb545..e461bcefb9 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2055,7 +2055,7 @@ describeOneTableDetails(const char *schemaname,
appendPQExpBufferStr(&buf, ",\n a.attstorage");
attstorage_col = cols++;
- /* compression info */
+ /* compression info, if relevant to relkind */
if (pset.sversion >= 140000 &&
!pset.hide_compression &&
(tableinfo.relkind == RELKIND_RELATION ||
diff --git a/src/include/access/toast_compression.h b/src/include/access/toast_compression.h
index 9e2c1cbe1a..4d5ca57dfb 100644
--- a/src/include/access/toast_compression.h
+++ b/src/include/access/toast_compression.h
@@ -23,16 +23,16 @@
extern int default_toast_compression;
/*
- * Built-in compression method-id. The toast compression header will store
+ * Built-in compression method ID. The toast compression header will store
* this in the first 2 bits of the raw length. These built-in compression
- * method-id are directly mapped to the built-in compression methods.
+ * method IDs are directly mapped to the built-in compression methods.
*
* Don't use these values for anything other than understanding the meaning
* of the raw bits from a varlena; in particular, if the goal is to identify
* a compression method, use the constants TOAST_PGLZ_COMPRESSION, etc.
* below. We might someday support more than 4 compression methods, but
* we can never have more than 4 values in this enum, because there are
- * only 2 bits available in the places where this is used.
+ * only 2 bits available in the places where this is stored.
*/
typedef enum ToastCompressionId
{
@@ -42,8 +42,9 @@ typedef enum ToastCompressionId
} ToastCompressionId;
/*
- * Built-in compression methods. pg_attribute will store this in the
- * attcompression column.
+ * Built-in compression methods. pg_attribute will store these in the
+ * attcompression column. In attcompression, InvalidCompressionMethod
+ * denotes the default behavior.
*/
#define TOAST_PGLZ_COMPRESSION 'p'
#define TOAST_LZ4_COMPRESSION 'l'
@@ -51,19 +52,10 @@ typedef enum ToastCompressionId
#define CompressionMethodIsValid(cm) ((cm) != InvalidCompressionMethod)
+/* Test whether a storage mode allows compression to be applied */
#define IsStorageCompressible(storage) ((storage) != TYPSTORAGE_PLAIN && \
(storage) != TYPSTORAGE_EXTERNAL)
-/*
- * GetDefaultToastCompression -- get the default toast compression method
- *
- * This exists to hide the use of the default_toast_compression GUC variable.
- */
-static inline char
-GetDefaultToastCompression(void)
-{
- return (char) default_toast_compression;
-}
/* pglz compression/decompression routines */
extern struct varlena *pglz_compress_datum(const struct varlena *value);
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 1e26016214..603392fe81 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -126,8 +126,12 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
char attstorage;
/*
- * Compression method. Must be InvalidCompressionMethod if and only if
- * typstorage is 'plain' or 'external'.
+ * attcompression sets the current compression method of the attribute.
+ * Typically this is InvalidCompressionMethod ('\0') to specify use of the
+ * current default setting (see default_toast_compression). Otherwise,
+ * 'p' selects pglz compression, while 'l' selects LZ4 compression.
+ * However, this field is ignored whenever attstorage does not allow
+ * compression.
*/
char attcompression BKI_DEFAULT('\0');
diff --git a/src/test/regress/expected/compression.out b/src/test/regress/expected/compression.out
index 61e97cb33c..5c645e4650 100644
--- a/src/test/regress/expected/compression.out
+++ b/src/test/regress/expected/compression.out
@@ -53,7 +53,7 @@ SELECT * INTO cmmove1 FROM cmdata;
Table "public.cmmove1"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | text | | | | extended | pglz | |
+ f1 | text | | | | extended | | |
SELECT pg_column_compression(f1) FROM cmmove1;
pg_column_compression
@@ -146,7 +146,7 @@ ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE varchar;
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+-------------------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | character varying | | | | extended | pglz | |
+ f1 | character varying | | | | extended | | |
ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE int USING f1::integer;
\d+ cmdata2
@@ -158,6 +158,7 @@ ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE int USING f1::integer;
--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;
\d+ cmdata2
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
@@ -179,12 +180,12 @@ SELECT pg_column_compression(f1) FROM cmdata2;
(1 row)
-- test compression with materialized view
-CREATE MATERIALIZED VIEW mv(x) AS SELECT * FROM cmdata1;
-\d+ mv
- Materialized view "public.mv"
+CREATE MATERIALIZED VIEW compressmv(x) AS SELECT * FROM cmdata1;
+\d+ compressmv
+ Materialized view "public.compressmv"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+------+-----------+----------+---------+----------+-------------+--------------+-------------
- x | text | | | | extended | pglz | |
+ x | text | | | | extended | | |
View definition:
SELECT cmdata1.f1 AS x
FROM cmdata1;
@@ -196,7 +197,7 @@ SELECT pg_column_compression(f1) FROM cmdata1;
lz4
(2 rows)
-SELECT pg_column_compression(x) FROM mv;
+SELECT pg_column_compression(x) FROM compressmv;
pg_column_compression
-----------------------
lz4
@@ -222,7 +223,7 @@ SELECT pg_column_compression(f1) FROM cmpart2;
pglz
(1 row)
--- test compression with inheritence, error
+-- test compression with inheritance, error
CREATE TABLE cminh() INHERITS(cmdata, cmdata1);
NOTICE: merging multiple inherited definitions of column "f1"
ERROR: column "f1" has a compression method conflict
@@ -239,14 +240,6 @@ SET default_toast_compression = 'I do not exist compression';
ERROR: invalid value for parameter "default_toast_compression": "I do not exist compression"
HINT: Available values: pglz, lz4.
SET default_toast_compression = 'lz4';
-DROP TABLE cmdata2;
-CREATE TABLE cmdata2 (f1 text);
-\d+ cmdata2
- Table "public.cmdata2"
- Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
---------+------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | text | | | | extended | lz4 | |
-
SET default_toast_compression = 'pglz';
-- test alter compression method
ALTER TABLE cmdata ALTER COLUMN f1 SET COMPRESSION lz4;
@@ -266,10 +259,17 @@ SELECT pg_column_compression(f1) FROM cmdata;
lz4
(2 rows)
--- test alter compression method for the materialized view
-ALTER MATERIALIZED VIEW mv ALTER COLUMN x SET COMPRESSION lz4;
-\d+ mv
- Materialized view "public.mv"
+ALTER TABLE cmdata2 ALTER COLUMN f1 SET COMPRESSION default;
+\d+ cmdata2
+ Table "public.cmdata2"
+ Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
+--------+-------------------+-----------+----------+---------+---------+-------------+--------------+-------------
+ f1 | character varying | | | | plain | | |
+
+-- test alter compression method for materialized views
+ALTER MATERIALIZED VIEW compressmv ALTER COLUMN x SET COMPRESSION lz4;
+\d+ compressmv
+ Materialized view "public.compressmv"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+------+-----------+----------+---------+----------+-------------+--------------+-------------
x | text | | | | extended | lz4 | |
@@ -277,7 +277,7 @@ View definition:
SELECT cmdata1.f1 AS x
FROM cmdata1;
--- test alter compression method for the partitioned table
+-- test alter compression method for partitioned tables
ALTER TABLE cmpart1 ALTER COLUMN f1 SET COMPRESSION pglz;
ALTER TABLE cmpart2 ALTER COLUMN f1 SET COMPRESSION lz4;
-- new data should be compressed with the current compression method
diff --git a/src/test/regress/expected/compression_1.out b/src/test/regress/expected/compression_1.out
index d03d6255ae..aac96037fc 100644
--- a/src/test/regress/expected/compression_1.out
+++ b/src/test/regress/expected/compression_1.out
@@ -50,7 +50,7 @@ SELECT * INTO cmmove1 FROM cmdata;
Table "public.cmmove1"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | text | | | | extended | pglz | |
+ f1 | text | | | | extended | | |
SELECT pg_column_compression(f1) FROM cmmove1;
pg_column_compression
@@ -144,7 +144,7 @@ ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE varchar;
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+-------------------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | character varying | | | | extended | pglz | |
+ f1 | character varying | | | | extended | | |
ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE int USING f1::integer;
\d+ cmdata2
@@ -156,6 +156,7 @@ ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE int USING f1::integer;
--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;
\d+ cmdata2
Table "public.cmdata2"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
@@ -177,18 +178,18 @@ SELECT pg_column_compression(f1) FROM cmdata2;
(1 row)
-- test compression with materialized view
-CREATE MATERIALIZED VIEW mv(x) AS SELECT * FROM cmdata1;
+CREATE MATERIALIZED VIEW compressmv(x) AS SELECT * FROM cmdata1;
ERROR: relation "cmdata1" does not exist
-LINE 1: CREATE MATERIALIZED VIEW mv(x) AS SELECT * FROM cmdata1;
- ^
-\d+ mv
+LINE 1: ...TE MATERIALIZED VIEW compressmv(x) AS SELECT * FROM cmdata1;
+ ^
+\d+ compressmv
SELECT pg_column_compression(f1) FROM cmdata1;
ERROR: relation "cmdata1" does not exist
LINE 1: SELECT pg_column_compression(f1) FROM cmdata1;
^
-SELECT pg_column_compression(x) FROM mv;
-ERROR: relation "mv" does not exist
-LINE 1: SELECT pg_column_compression(x) FROM mv;
+SELECT pg_column_compression(x) FROM compressmv;
+ERROR: relation "compressmv" does not exist
+LINE 1: SELECT pg_column_compression(x) FROM compressmv;
^
-- test compression with partition
CREATE TABLE cmpart(f1 text COMPRESSION lz4) PARTITION BY HASH(f1);
@@ -217,7 +218,7 @@ SELECT pg_column_compression(f1) FROM cmpart2;
-----------------------
(0 rows)
--- test compression with inheritence, error
+-- test compression with inheritance, error
CREATE TABLE cminh() INHERITS(cmdata, cmdata1);
ERROR: relation "cmdata1" does not exist
CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata);
@@ -234,14 +235,6 @@ HINT: Available values: pglz.
SET default_toast_compression = 'lz4';
ERROR: invalid value for parameter "default_toast_compression": "lz4"
HINT: Available values: pglz.
-DROP TABLE cmdata2;
-CREATE TABLE cmdata2 (f1 text);
-\d+ cmdata2
- Table "public.cmdata2"
- Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
---------+------+-----------+----------+---------+----------+-------------+--------------+-------------
- f1 | text | | | | extended | pglz | |
-
SET default_toast_compression = 'pglz';
-- test alter compression method
ALTER TABLE cmdata ALTER COLUMN f1 SET COMPRESSION lz4;
@@ -264,11 +257,18 @@ SELECT pg_column_compression(f1) FROM cmdata;
pglz
(2 rows)
--- test alter compression method for the materialized view
-ALTER MATERIALIZED VIEW mv ALTER COLUMN x SET COMPRESSION lz4;
-ERROR: relation "mv" does not exist
-\d+ mv
--- test alter compression method for the partitioned table
+ALTER TABLE cmdata2 ALTER COLUMN f1 SET COMPRESSION default;
+\d+ cmdata2
+ Table "public.cmdata2"
+ Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
+--------+-------------------+-----------+----------+---------+---------+-------------+--------------+-------------
+ f1 | character varying | | | | plain | | |
+
+-- test alter compression method for materialized views
+ALTER MATERIALIZED VIEW compressmv ALTER COLUMN x SET COMPRESSION lz4;
+ERROR: relation "compressmv" does not exist
+\d+ compressmv
+-- test alter compression method for partitioned tables
ALTER TABLE cmpart1 ALTER COLUMN f1 SET COMPRESSION pglz;
ERROR: relation "cmpart1" does not exist
ALTER TABLE cmpart2 ALTER COLUMN f1 SET COMPRESSION lz4;
diff --git a/src/test/regress/sql/compression.sql b/src/test/regress/sql/compression.sql
index 76d1776d83..35557c1f7d 100644
--- a/src/test/regress/sql/compression.sql
+++ b/src/test/regress/sql/compression.sql
@@ -69,6 +69,7 @@ ALTER TABLE cmdata2 ALTER COLUMN f1 TYPE int USING f1::integer;
--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;
\d+ cmdata2
ALTER TABLE cmdata2 ALTER COLUMN f1 SET STORAGE plain;
\d+ cmdata2
@@ -76,10 +77,10 @@ INSERT INTO cmdata2 VALUES (repeat('123456789', 800));
SELECT pg_column_compression(f1) FROM cmdata2;
-- test compression with materialized view
-CREATE MATERIALIZED VIEW mv(x) AS SELECT * FROM cmdata1;
-\d+ mv
+CREATE MATERIALIZED VIEW compressmv(x) AS SELECT * FROM cmdata1;
+\d+ compressmv
SELECT pg_column_compression(f1) FROM cmdata1;
-SELECT pg_column_compression(x) FROM mv;
+SELECT pg_column_compression(x) FROM compressmv;
-- test compression with partition
CREATE TABLE cmpart(f1 text COMPRESSION lz4) PARTITION BY HASH(f1);
@@ -92,7 +93,7 @@ INSERT INTO cmpart VALUES (repeat('123456789', 4004));
SELECT pg_column_compression(f1) FROM cmpart1;
SELECT pg_column_compression(f1) FROM cmpart2;
--- test compression with inheritence, error
+-- test compression with inheritance, error
CREATE TABLE cminh() INHERITS(cmdata, cmdata1);
CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata);
@@ -100,9 +101,6 @@ CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata);
SET default_toast_compression = '';
SET default_toast_compression = 'I do not exist compression';
SET default_toast_compression = 'lz4';
-DROP TABLE cmdata2;
-CREATE TABLE cmdata2 (f1 text);
-\d+ cmdata2
SET default_toast_compression = 'pglz';
-- test alter compression method
@@ -111,11 +109,14 @@ INSERT INTO cmdata VALUES (repeat('123456789', 4004));
\d+ cmdata
SELECT pg_column_compression(f1) FROM cmdata;
--- test alter compression method for the materialized view
-ALTER MATERIALIZED VIEW mv ALTER COLUMN x SET COMPRESSION lz4;
-\d+ mv
+ALTER TABLE cmdata2 ALTER COLUMN f1 SET COMPRESSION default;
+\d+ cmdata2
+
+-- test alter compression method for materialized views
+ALTER MATERIALIZED VIEW compressmv ALTER COLUMN x SET COMPRESSION lz4;
+\d+ compressmv
--- test alter compression method for the partitioned table
+-- test alter compression method for partitioned tables
ALTER TABLE cmpart1 ALTER COLUMN f1 SET COMPRESSION pglz;
ALTER TABLE cmpart2 ALTER COLUMN f1 SET COMPRESSION lz4;
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)
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
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."
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
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
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
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
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
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
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
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 notnull 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 intnot null default '4',i5 int not null default '5',i6 int not null default '6',i7 int not null default '7',i8 int not nulldefault '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
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
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
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
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
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
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
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
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
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
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
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
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
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)
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Peter Geoghegan
Дата:
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
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
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
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
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
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Peter Geoghegan
Дата:
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
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
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
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
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
Вложения
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
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
Вложения
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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.
Hi Alvaro.
Please, would you mind testing with these changes.
I'm curious to see if anything improves or not.
1. Add a const to the attr parameter.
2. Remove the cmid variable (and store it).
3. Add tail cut.
regards,
Ranier Vilela
Вложения
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
Em sáb., 5 de jun. de 2021 às 11:16, Ranier Vilela <ranier.vf@gmail.com> escreveu:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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.Hi Alvaro.
Please, would you mind testing with these changes.
I'm curious to see if anything improves or not.
1. Add a const to the attr parameter.
2. Remove the cmid variable (and store it).
3. Add tail cut.
still has space for improvements.
If attr->attcompression is invalid, it doesn't matter, it's better to decompress.
Besides if it is invalid, currently default_toast_compression is not stored in attr->attcompression.
I believe this version should be a little faster.
Since it won't double-scan the attributes if it's not really necessary.
Since it won't double-scan the attributes if it's not really necessary.
regards,
Ranier Vilela
Вложения
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)
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
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
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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
Вложения
Re: Move pg_attribute.attcompression to earlier in struct for reduced size?
От
Michael Paquier
Дата:
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