public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 6/8] Add default_toast_compression GUC 5+ messages / 4 participants [nested] [flat]
* [PATCH 6/8] Add default_toast_compression GUC @ 2021-03-10 08:35 Dilip Kumar <[email protected]> 0 siblings, 0 replies; 5+ 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 3463b42438..e33f92687c 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 --cvVnyQ+4j833TQvp Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0007-Alter-table-set-compression.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Don't trust unvalidated xl_tot_len. @ 2023-11-11 11:53 Christoph Berg <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Christoph Berg @ 2023-11-11 11:53 UTC (permalink / raw) To: Thomas Munro <[email protected]>; [email protected] Re: Thomas Munro > In the 13 branch we see that's in the new scan_server_header() > subroutine where it tries to open the header, after asking pg_config > --includedir-server where it lives. Hmm... It's no ok to use pg_config at test time since `make install` might not have been called yet: https://www.postgresql.org/message-id/[email protected] https://www.postgresql.org/message-id/YqkV/[email protected] Christoph ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Don't trust unvalidated xl_tot_len. @ 2023-11-11 18:17 Thomas Munro <[email protected]> parent: Christoph Berg <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Thomas Munro @ 2023-11-11 18:17 UTC (permalink / raw) To: Christoph Berg <[email protected]>; +Cc: [email protected]; Michael Paquier <[email protected]> On Sun, Nov 12, 2023 at 12:53 AM Christoph Berg <[email protected]> wrote: > Re: Thomas Munro > > In the 13 branch we see that's in the new scan_server_header() > > subroutine where it tries to open the header, after asking pg_config > > --includedir-server where it lives. Hmm... > > It's no ok to use pg_config at test time since `make install` might > not have been called yet: > > https://www.postgresql.org/message-id/[email protected] > https://www.postgresql.org/message-id/YqkV/[email protected] [CC'ing Michael who was involved in that analysis and who also wrote those bits of this commit] We don't have an installation into the final --prefix, but we have tmp_install, surely? And the tests are run with PATH set to point to tmp_install's bin directory. It looks like it did actually find a pg_config executable because otherwise we'd have hit die "could not execute pg_config" and failed sooner. So now I'm wondering if the pg_config it found gives the wrong answer for --includedir-server, because of Debian's local patches that insert a major version into some paths. For example, maybe it's trying to look for access/xlog_internal.h under tmp_install/usr/include/postgresql/server when it's really under tmp_install/usr/include/postgresql/13/server, or vice versa. But then why does that only happen on the salsa build, not on the apt.postgresql.org one? ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Don't trust unvalidated xl_tot_len. @ 2023-11-11 18:26 Christoph Berg <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Christoph Berg @ 2023-11-11 18:26 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: [email protected]; Michael Paquier <[email protected]> Re: Thomas Munro > But then why does that only happen on the salsa build, > not on the apt.postgresql.org one? The build chroots there have postgresql-NN already installed so extension builds don't have to download 7 PG versions over and over. My guess would be that that's the difference and it's using some pg_config from /usr/bin or /usr/lib/postgresql/*/bin. I can confirm that it's also failing in my local chroots if none of the postgresql-* packages are preinstalled. Christoph ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Don't trust unvalidated xl_tot_len. @ 2023-11-11 19:43 Andres Freund <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Andres Freund @ 2023-11-11 19:43 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Christoph Berg <[email protected]>; [email protected]; Michael Paquier <[email protected]> Hi, On 2023-11-12 07:17:02 +1300, Thomas Munro wrote: > On Sun, Nov 12, 2023 at 12:53 AM Christoph Berg <[email protected]> wrote: > > Re: Thomas Munro > > > In the 13 branch we see that's in the new scan_server_header() > > > subroutine where it tries to open the header, after asking pg_config > > > --includedir-server where it lives. Hmm... > > > > It's no ok to use pg_config at test time since `make install` might > > not have been called yet: > > > > https://www.postgresql.org/message-id/[email protected] > > https://www.postgresql.org/message-id/YqkV/[email protected] > > [CC'ing Michael who was involved in that analysis and who also wrote > those bits of this commit] > > We don't have an installation into the final --prefix, but we have > tmp_install, surely? Yea, that should work and does work locally. I guess it'd fail though, if you somehow ran the tests with NO_TEMP_INSTALL=1 or such. > And the tests are run with PATH set to point to > tmp_install's bin directory. It looks like it did actually find a > pg_config executable because otherwise we'd have hit die "could not > execute pg_config" and failed sooner. So now I'm wondering if the > pg_config it found gives the wrong answer for --includedir-server, > because of Debian's local patches that insert a major version into > some paths. From the second link above, the problem might more be that debian pg_config is patched to not be relocatable (huh?) - so it'd return an absolute path into the final non-DESTDIR path. Which would fail, because the file isn't installed yet. If that's the case, does that also mean that all the tests that are selectively enabled using check_pg_config() don't work in the debian context? I assume the reason to not use the source-tree access/xlog_internal.h here was just that it's nontrivial to find the top of the source tree form tap tests? It's not hard to make it available... And as we already pass in top_builddir, it doesn't actually measurably further weaken usability of the tap framework in the pgxs context. > For example, maybe it's trying to look for access/xlog_internal.h under > tmp_install/usr/include/postgresql/server when it's really under > tmp_install/usr/include/postgresql/13/server, or vice versa. But then why > does that only happen on the salsa build, not on the apt.postgresql.org one? Christoph, can you apply a patch to emit a bit more information in that environment? diff --git i/src/test/perl/PostgreSQL/Test/Utils.pm w/src/test/perl/PostgreSQL/Test/Utils.pm index cd86897580c..3c588a41755 100644 --- i/src/test/perl/PostgreSQL/Test/Utils.pm +++ w/src/test/perl/PostgreSQL/Test/Utils.pm @@ -722,7 +722,8 @@ sub scan_server_header chomp($stdout); $stdout =~ s/\r$//; - open my $header_h, '<', "$stdout/$header_path" or die "$!"; + my $fname = "$stdout/$header_path"; + open my $header_h, '<', "$fname" or die "could not open $fname: $!"; my @match = undef; while (<$header_h>) would be helpful. I guess we should also apply something like that to our tree - printing a stringified errno without even a path/filename isn't very useful. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-11-11 19:43 UTC | newest] Thread overview: 5+ 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]> 2023-11-11 11:53 Re: pgsql: Don't trust unvalidated xl_tot_len. Christoph Berg <[email protected]> 2023-11-11 18:17 ` Re: pgsql: Don't trust unvalidated xl_tot_len. Thomas Munro <[email protected]> 2023-11-11 18:26 ` Re: pgsql: Don't trust unvalidated xl_tot_len. Christoph Berg <[email protected]> 2023-11-11 19:43 ` Re: pgsql: Don't trust unvalidated xl_tot_len. 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