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: Remove extraneous break condition in logical slot advance function @ 2023-10-21 18:10 Tom Lane <[email protected]> 2023-10-22 18:29 ` Re: Remove extraneous break condition in logical slot advance function Bharath Rupireddy <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Tom Lane @ 2023-10-21 18:10 UTC (permalink / raw) To: Gurjeet Singh <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> Gurjeet Singh <[email protected]> writes: > On Fri, Oct 20, 2023 at 7:30 PM Bharath Rupireddy > <[email protected]> wrote: >> There exists an extraneous break condition in >> pg_logical_replication_slot_advance(). When the end of WAL or moveto >> LSN is reached, the main while condition helps to exit the loop, so no >> separate break condition is needed. Attached patch removes it. > The only advantage I see of the code as it stands right now is that it > avoids one last call to CHECK_FOR_INTERRUPTS() by break'ing early. I > don't think we'd lose much in terms of performance by making one (very > cheap, in common case) extra call of this macro. Agreed, bypassing the last CHECK_FOR_INTERRUPTS() shouldn't save anything noticeable. Could there be a correctness argument for it though? Can't see what. We should assume that CFIs might happen down inside LogicalDecodingProcessRecord. I wondered why the code looks like this, and whether there used to be more of a reason for it. "git blame" reveals the probable answer: when this code was added, in 9c7d06d60, the loop condition was different so the break was necessary. 38a957316 simplified the loop condition to what we see today, but didn't notice that the break was thereby made pointless. While we're here ... the comment above the loop seems wrong already, and this makes it more so. I suggest something like - /* Decode at least one record, until we run out of records */ + /* Decode records until we reach the requested target */ while (ctx->reader->EndRecPtr < moveto) regards, tom lane ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Remove extraneous break condition in logical slot advance function 2023-10-21 18:10 Re: Remove extraneous break condition in logical slot advance function Tom Lane <[email protected]> @ 2023-10-22 18:29 ` Bharath Rupireddy <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Bharath Rupireddy @ 2023-10-22 18:29 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Gurjeet Singh <[email protected]>; PostgreSQL Hackers <[email protected]> On Sat, Oct 21, 2023 at 11:40 PM Tom Lane <[email protected]> wrote: > > Gurjeet Singh <[email protected]> writes: > > On Fri, Oct 20, 2023 at 7:30 PM Bharath Rupireddy > > <[email protected]> wrote: > >> There exists an extraneous break condition in > >> pg_logical_replication_slot_advance(). When the end of WAL or moveto > >> LSN is reached, the main while condition helps to exit the loop, so no > >> separate break condition is needed. Attached patch removes it. > > > The only advantage I see of the code as it stands right now is that it > > avoids one last call to CHECK_FOR_INTERRUPTS() by break'ing early. I > > don't think we'd lose much in terms of performance by making one (very > > cheap, in common case) extra call of this macro. > > Agreed, bypassing the last CHECK_FOR_INTERRUPTS() shouldn't save > anything noticeable. Could there be a correctness argument for it > though? Can't see what. We should assume that CFIs might happen > down inside LogicalDecodingProcessRecord. AFAICS, there's no correctness argument for breaking before CFI. As rightly said, CFIs can happen before the break condition either down inside LogicalDecodingProcessRecord or XLogReadRecord (page_read callbacks for instance). Having said that, what may happen if CFI happens and interrupts are processed before the break condition is that the decoding occurs again which IMV is not a big problem. An idea to keep all of XLogReadRecord() - LogicalDecodingProcessRecord() loops consistent is by having CFI at the start of the loops before the XLogReadRecord(). > I wondered why the code looks like this, and whether there used > to be more of a reason for it. "git blame" reveals the probable > answer: when this code was added, in 9c7d06d60, the loop > condition was different so the break was necessary. > 38a957316 simplified the loop condition to what we see today, > but didn't notice that the break was thereby made pointless. Right. Thanks for these references. > While we're here ... the comment above the loop seems wrong > already, and this makes it more so. I suggest something like > > - /* Decode at least one record, until we run out of records */ > + /* Decode records until we reach the requested target */ > while (ctx->reader->EndRecPtr < moveto) +1 and done so in the attached v2 patch. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com Attachments: [application/octet-stream] v2-0001-Remove-extraneous-break-condition-in-logical-slot.patch (1.5K, ../../CALj2ACWbyogkFFeLQe5Hs4umqd-igffg0YpoosvniC+G8ukzXQ@mail.gmail.com/2-v2-0001-Remove-extraneous-break-condition-in-logical-slot.patch) download | inline diff: From 3f86c6138f45f49f09380ec17fc25901f3f6784f Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Sun, 22 Oct 2023 06:59:05 +0000 Subject: [PATCH v2] Remove extraneous break condition in logical slot advance function There exists an extraneous break condition in pg_logical_replication_slot_advance(). When end of WAL or moveto LSN is reached, the main while condition helps to exit the loop, so no separate break condition is needed. In passing, modify the comment atop the while loop by specifying that the loop runs until the requested target LSN has been reached. --- src/backend/replication/slotfuncs.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c index 6035cf4816..4b694a03d0 100644 --- a/src/backend/replication/slotfuncs.c +++ b/src/backend/replication/slotfuncs.c @@ -500,7 +500,7 @@ pg_logical_replication_slot_advance(XLogRecPtr moveto) /* invalidate non-timetravel entries */ InvalidateSystemCaches(); - /* Decode at least one record, until we run out of records */ + /* Decode records until we reach the requested target */ while (ctx->reader->EndRecPtr < moveto) { char *errm = NULL; @@ -523,10 +523,6 @@ pg_logical_replication_slot_advance(XLogRecPtr moveto) if (record) LogicalDecodingProcessRecord(ctx, ctx->reader); - /* Stop once the requested target has been reached */ - if (moveto <= ctx->reader->EndRecPtr) - break; - CHECK_FOR_INTERRUPTS(); } -- 2.34.1 ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2023-10-22 18:29 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]> 2023-10-21 18:10 Re: Remove extraneous break condition in logical slot advance function Tom Lane <[email protected]> 2023-10-22 18:29 ` Re: Remove extraneous break condition in logical slot advance function Bharath Rupireddy <[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