public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 6/8] Add default_toast_compression GUC 3+ messages / 3 participants [nested] [flat]
* [PATCH 6/8] Add default_toast_compression GUC @ 2021-03-10 08:35 Dilip Kumar <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Dilip Kumar @ 2021-03-10 08:35 UTC (permalink / raw) Justin Pryzby and Dilip Kumar --- src/backend/access/common/toast_compression.c | 46 +++++++++++++++++++ src/backend/access/common/tupdesc.c | 2 +- src/backend/bootstrap/bootstrap.c | 2 +- src/backend/commands/tablecmds.c | 8 ++-- src/backend/utils/misc/guc.c | 12 +++++ src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/access/toast_compression.h | 22 ++++++++- src/test/regress/expected/compression.out | 16 +++++++ src/test/regress/expected/compression_1.out | 19 ++++++++ src/test/regress/sql/compression.sql | 8 ++++ 10 files changed, 128 insertions(+), 8 deletions(-) diff --git a/src/backend/access/common/toast_compression.c b/src/backend/access/common/toast_compression.c index db4911ce43..27fe47b201 100644 --- a/src/backend/access/common/toast_compression.c +++ b/src/backend/access/common/toast_compression.c @@ -55,6 +55,9 @@ const CompressionRoutine toast_compression[] = } }; +/* Compile-time default */ +char *default_toast_compression = DEFAULT_TOAST_COMPRESSION; + /* * pglz_cmcompress - compression routine for pglz compression method * @@ -306,3 +309,46 @@ GetCompressionRoutines(char method) { return &toast_compression[CompressionMethodToId(method)]; } + +/* check_hook: validate new default_toast_compression */ +bool +check_default_toast_compression(char **newval, void **extra, GucSource source) +{ + if (**newval == '\0') + { + GUC_check_errdetail("%s cannot be empty.", + "default_toast_compression"); + return false; + } + + if (strlen(*newval) >= NAMEDATALEN) + { + GUC_check_errdetail("%s is too long (maximum %d characters).", + "default_toast_compression", NAMEDATALEN - 1); + return false; + } + + if (!CompressionMethodIsValid(CompressionNameToMethod(*newval))) + { + /* + * When source == PGC_S_TEST, don't throw a hard error for a + * nonexistent compression method, only a NOTICE. See comments in + * guc.h. + */ + if (source == PGC_S_TEST) + { + ereport(NOTICE, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("compression method \"%s\" does not exist", + *newval))); + } + else + { + GUC_check_errdetail("Compression method \"%s\" does not exist.", + *newval); + return false; + } + } + + return true; +} diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c index a66d1113db..362a371a6c 100644 --- a/src/backend/access/common/tupdesc.c +++ b/src/backend/access/common/tupdesc.c @@ -668,7 +668,7 @@ TupleDescInitEntry(TupleDesc desc, att->attcollation = typeForm->typcollation; if (IsStorageCompressible(typeForm->typstorage)) - att->attcompression = DefaultCompressionMethod; + att->attcompression = GetDefaultToastCompression(); else att->attcompression = InvalidCompressionMethod; diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index af0c0aa77e..9be0841d08 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -733,7 +733,7 @@ DefineAttr(char *name, char *type, int attnum, int nullness) attrtypes[attnum]->atttypmod = -1; attrtypes[attnum]->attislocal = true; if (IsStorageCompressible(attrtypes[attnum]->attstorage)) - attrtypes[attnum]->attcompression = DefaultCompressionMethod; + attrtypes[attnum]->attcompression = GetDefaultToastCompression(); else attrtypes[attnum]->attcompression = InvalidCompressionMethod; diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index e20dd5f63c..a0d2af0cde 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -11971,7 +11971,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, if (!IsStorageCompressible(tform->typstorage)) attTup->attcompression = InvalidCompressionMethod; else if (!CompressionMethodIsValid(attTup->attcompression)) - attTup->attcompression = DefaultCompressionMethod; + attTup->attcompression = GetDefaultToastCompression(); } else attTup->attcompression = InvalidCompressionMethod; @@ -17847,9 +17847,9 @@ GetAttributeCompression(Form_pg_attribute att, char *compression) /* fallback to default compression if it's not specified */ if (compression == NULL) - return DefaultCompressionMethod; - - cmethod = CompressionNameToMethod(compression); + cmethod = GetDefaultToastCompression(); + else + cmethod = CompressionNameToMethod(compression); return cmethod; } diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 953837085f..e59ed5b214 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -33,6 +33,7 @@ #include "access/gin.h" #include "access/rmgr.h" #include "access/tableam.h" +#include "access/toast_compression.h" #include "access/transam.h" #include "access/twophase.h" #include "access/xact.h" @@ -3915,6 +3916,17 @@ static struct config_string ConfigureNamesString[] = check_default_table_access_method, NULL, NULL }, + { + {"default_toast_compression", PGC_USERSET, CLIENT_CONN_STATEMENT, + gettext_noop("Sets the default compression for new columns."), + NULL, + GUC_IS_NAME + }, + &default_toast_compression, + DEFAULT_TOAST_COMPRESSION, + check_default_toast_compression, NULL, NULL + }, + { {"default_tablespace", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Sets the default tablespace to create tables and indexes in."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index f46c2dd7a8..7d7a433dcc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -659,6 +659,7 @@ #temp_tablespaces = '' # a list of tablespace names, '' uses # only default tablespace #default_table_access_method = 'heap' +#default_toast_compression = 'pglz' # 'pglz' or 'lz4' #check_function_bodies = on #default_transaction_isolation = 'read committed' #default_transaction_read_only = off diff --git a/src/include/access/toast_compression.h b/src/include/access/toast_compression.h index 38800cb97a..e9d9ce5634 100644 --- a/src/include/access/toast_compression.h +++ b/src/include/access/toast_compression.h @@ -15,6 +15,14 @@ #include "postgres.h" +#include "utils/guc.h" + +/* default compression method if not specified. */ +#define DEFAULT_TOAST_COMPRESSION "pglz" + +/* GUCs */ +extern char *default_toast_compression; + /* * Built-in compression methods. pg_attribute will store this in the * attcompression column. @@ -36,8 +44,6 @@ typedef enum CompressionId LZ4_COMPRESSION_ID = 1 } CompressionId; -/* use default compression method if it is not specified. */ -#define DefaultCompressionMethod PGLZ_COMPRESSION #define IsValidCompression(cm) ((cm) != InvalidCompressionMethod) #define IsStorageCompressible(storage) ((storage) != TYPSTORAGE_PLAIN && \ @@ -67,6 +73,8 @@ typedef struct CompressionRoutine extern char CompressionNameToMethod(char *compression); extern const CompressionRoutine *GetCompressionRoutines(char method); +extern bool check_default_toast_compression(char **newval, void **extra, + GucSource source); /* * CompressionMethodToId - Convert compression method to compression id. @@ -115,5 +123,15 @@ GetCompressionMethodName(char method) return GetCompressionRoutines(method)->cmname; } +/* + * GetDefaultToastCompression -- get the current toast compression + * + * This exists to hide the use of the default_toast_compression GUC variable. + */ +static inline char +GetDefaultToastCompression(void) +{ + return CompressionNameToMethod(default_toast_compression); +} #endif /* TOAST_COMPRESSION_H */ diff --git a/src/test/regress/expected/compression.out b/src/test/regress/expected/compression.out index 6981d580dc..9ddffc8b18 100644 --- a/src/test/regress/expected/compression.out +++ b/src/test/regress/expected/compression.out @@ -231,6 +231,22 @@ CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata); NOTICE: merging column "f1" with inherited definition ERROR: column "f1" has a compression method conflict DETAIL: pglz versus lz4 +-- test default_toast_compression GUC +SET default_toast_compression = ''; +ERROR: invalid value for parameter "default_toast_compression": "" +DETAIL: default_toast_compression cannot be empty. +SET default_toast_compression = 'I do not exist compression'; +ERROR: invalid value for parameter "default_toast_compression": "I do not exist compression" +DETAIL: Compression method "I do not exist compression" does not exist. +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 | | + -- check data is ok SELECT length(f1) FROM cmdata; length diff --git a/src/test/regress/expected/compression_1.out b/src/test/regress/expected/compression_1.out index 893c18c7fa..03a7f46554 100644 --- a/src/test/regress/expected/compression_1.out +++ b/src/test/regress/expected/compression_1.out @@ -237,6 +237,25 @@ CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata); NOTICE: merging column "f1" with inherited definition ERROR: column "f1" has a compression method conflict DETAIL: pglz versus lz4 +-- test default_toast_compression GUC +SET default_toast_compression = ''; +ERROR: invalid value for parameter "default_toast_compression": "" +DETAIL: default_toast_compression cannot be empty. +SET default_toast_compression = 'I do not exist compression'; +ERROR: invalid value for parameter "default_toast_compression": "I do not exist compression" +DETAIL: Compression method "I do not exist compression" does not exist. +SET default_toast_compression = 'lz4'; +ERROR: unsupported LZ4 compression method +DETAIL: This functionality requires the server to be built with lz4 support. +HINT: You need to rebuild PostgreSQL using --with-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 | pglz | | + -- check data is ok SELECT length(f1) FROM cmdata; length diff --git a/src/test/regress/sql/compression.sql b/src/test/regress/sql/compression.sql index b085c9fbd9..6c9b24f1f7 100644 --- a/src/test/regress/sql/compression.sql +++ b/src/test/regress/sql/compression.sql @@ -103,6 +103,14 @@ SELECT pg_column_compression(f1) FROM cmpart; CREATE TABLE cminh() INHERITS(cmdata, cmdata1); CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata); +-- test default_toast_compression GUC +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 + -- check data is ok SELECT length(f1) FROM cmdata; SELECT length(f1) FROM cmdata1; -- 2.17.0 --C94crkcyjafcjHxo Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0007-Alter-table-set-compression.patch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: pgstat include expansion @ 2026-02-16 16:18 Alvaro Herrera <[email protected]> 2026-02-16 17:41 ` Re: pgstat include expansion Andres Freund <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Alvaro Herrera @ 2026-02-16 16:18 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: pgsql-hackers; Amit Kapila <[email protected]>; Michael Paquier <[email protected]> On 2026-Feb-13, Andres Freund wrote: > commit 7e638d7f509 > Author: Álvaro Herrera <[email protected]> > Date: 2025-09-25 14:45:08 +0200 > > Don't include execnodes.h in replication/conflict.h > > added an include of access/transam.h, which I don't love being included, > although it's less bad than some of the other cases. It's not actually to > blame for that though, as it shrank what was included noticeably. > diff --git a/src/include/pgstat.h b/src/include/pgstat.h > index fff7ecc2533..a2021a0e66f 100644 > --- a/src/include/pgstat.h > +++ b/src/include/pgstat.h > @@ -11,18 +11,20 @@ > #ifndef PGSTAT_H > #define PGSTAT_H > > -#include "access/transam.h" /* for FullTransactionId */ > #include "datatype/timestamp.h" > #include "portability/instr_time.h" > #include "postmaster/pgarch.h" /* for MAX_XFN_CHARS */ > -#include "replication/conflict.h" > -#include "replication/worker_internal.h" > +#include "replication/conflict.h" /* for CONFLICT_NUM_TYPES */ > #include "utils/backend_progress.h" /* for backward compatibility */ /* IWYU pragma: export */ > #include "utils/backend_status.h" /* for backward compatibility */ /* IWYU pragma: export */ > #include "utils/pgstat_kind.h" > -#include "utils/relcache.h" > #include "utils/wait_event.h" /* for backward compatibility */ /* IWYU pragma: export */ > > +/* to avoid including access/transam.h, for FullTransactionId */ > +typedef struct FullTransactionId FullTransactionId; > + > +/* to avoid including utils/relcache.h */ > +typedef struct RelationData *Relation; > > /* ---------- > * Paths for the statistics files (relative to installation's $PGDATA). Yeah, I intended not to add access/transam.h here and instead forward-declare FullTransactionId, but forgot to actually remove it before pushing. So +1 for what you're doing here with that. Same with relcache.h and most of the other changes you're doing here. I don't think the removal of pg_list.h works terribly well though, as that is something that I would expect to be pretty much everywhere, so it seems somewhat pointless to try to avoid it in this file. I'm not very sure what to do about LogicalRepWorkerType ... will follow-up to Amit's reply. I think we should in addition remove some includes that were kept during recent IWYU hacking "for backwards compatibility", and instead include them explicitly wherever they are needed. The attached does that. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "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) https://postgr.es/m/[email protected] ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: pgstat include expansion 2026-02-16 16:18 Re: pgstat include expansion Alvaro Herrera <[email protected]> @ 2026-02-16 17:41 ` Andres Freund <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Andres Freund @ 2026-02-16 17:41 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: pgsql-hackers; Amit Kapila <[email protected]>; Michael Paquier <[email protected]> Hi, On 2026-02-16 17:18:36 +0100, Alvaro Herrera wrote: > Yeah, I intended not to add access/transam.h here and instead > forward-declare FullTransactionId, but forgot to actually remove it > before pushing. So +1 for what you're doing here with that. Same with > relcache.h and most of the other changes you're doing here. Ah, great. > I don't think the removal of pg_list.h works terribly well though, as > that is something that I would expect to be pretty much everywhere, so > it seems somewhat pointless to try to avoid it in this file. Yea, I was on the fence about that one. It does seem pretty crappy that pg_list.h is so widely included and in turn includes nodes/nodes.h, which then depends on the generated nodetags.h. For one it forces an unnecessary rebuild of too much if you add a new node type, but it also just aesthetically feels wrong. But avoiding it just here does, as you say, not save very much. > I think we should in addition remove some includes that were kept during > recent IWYU hacking "for backwards compatibility", and instead include > them explicitly wherever they are needed. The attached does that. FWIW, the "for backwards compatibility" includes are from prep work towards shared memory stats. Before that all the stuff in backend_status.h, backend_progress.h and wait_event.h was in pgstat.h, which was quite unwieldy. The creation of those headers would have broken a lot of extensions if we had not provided those backward compat includes from pgstat.h. I suspect it would still break a fair number of extensions if removed those includes today. Not sure if the improvement is worth it? But even if we decide not to remove the backward compat includes, I think we should fix our own code to include the right headers directly, rather than rely on doing so via pgstat.h. FWIW, your patch that you sent subsequently doesn't seem to apply cleanly for me? I think it's perhaps based on an older tree, it doesn't know about the conflict.h include yet... And it fails to build e.g. due to miscadmin.h defines around BackendType not being indirectly included in pgstat.h anymore, but there are many other failures... Greetings, Andres Freund ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-02-16 17:41 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-10 08:35 [PATCH 6/8] Add default_toast_compression GUC Dilip Kumar <[email protected]> 2026-02-16 16:18 Re: pgstat include expansion Alvaro Herrera <[email protected]> 2026-02-16 17:41 ` Re: pgstat include expansion Andres Freund <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox