public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 3/5] Add default_toast_compression GUC
5+ messages / 4 participants
[nested] [flat]
* [PATCH 3/5] 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 503d64df38..a4b34ec570 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 45e1cfa56c..def0ad1dcf 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 d295d85cd2..745dfe9570 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11931,7 +11931,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;
@@ -17745,9 +17745,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 855076b1fd..321d2eb21e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -30,6 +30,7 @@
#include <unistd.h>
#include "access/commit_ts.h"
+#include "access/toast_compression.h"
#include "access/gin.h"
#include "access/rmgr.h"
#include "access/tableam.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 1de3ae2646..12ab404900 100644
--- a/src/test/regress/expected/compression.out
+++ b/src/test/regress/expected/compression.out
@@ -211,6 +211,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 2999b3bb79..cc9b913418 100644
--- a/src/test/regress/expected/compression_1.out
+++ b/src/test/regress/expected/compression_1.out
@@ -208,6 +208,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 ab35f3b4ae..0d5a2231d0 100644
--- a/src/test/regress/sql/compression.sql
+++ b/src/test/regress/sql/compression.sql
@@ -92,6 +92,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
--nHwqXXcoX0o6fKCv
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0004-Alter-table-set-compression.patch"
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: The documentation for READ COMMITTED may be incomplete or wrong
@ 2023-05-19 12:53 Amit Langote <[email protected]>
2023-05-19 16:22 ` Re: The documentation for READ COMMITTED may be incomplete or wrong Tom Lane <[email protected]>
2023-05-19 18:33 ` Re: The documentation for READ COMMITTED may be incomplete or wrong Tom Lane <[email protected]>
0 siblings, 2 replies; 5+ messages in thread
From: Amit Langote @ 2023-05-19 12:53 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Aleksander Alekseev <[email protected]>; PostgreSQL Hackers <[email protected]>
Thanks for the patch.
On Fri, May 19, 2023 at 8:57 AM Tom Lane <[email protected]> wrote:
> I wrote:
> > Debian Code Search doesn't know of any outside code touching
> > relsubs_done, so I think we are safe in dropping that code in
> > ExecScanReScan. It seems quite pointless anyway considering
> > that up to now, EvalPlanQualBegin has always zeroed the whole
> > array.
>
> Oh, belay that. What I'd forgotten is that it's possible that
> the target relation is on the inside of a nestloop, meaning that
> we might need to fetch the EPQ substitute tuple more than once.
> So there are three possible states: blocked (never return a
> tuple), ready to return a tuple, and done returning a tuple
> for this scan. ExecScanReScan needs to reset "done" to "ready",
> but not touch the "blocked" state. The attached v2 mechanizes
> that using two bool arrays.
Aha, that's clever. So ExecScanReScan() would only reset the
relsubs_done[] entry for the currently active ("unblocked") target
relation, because that would be the only one "unblocked" during a
given EvalPlanQual() invocation.
+ * Initialize per-relation EPQ tuple states. Result relations, if any,
+ * get marked as blocked; others as not-fetched.
Would it be helpful to clarify that "blocked" means blocked for a
given EvalPlanQual() cycle?
+ /*
+ * relsubs_blocked[scanrelid - 1] is true if there is no EPQ tuple for
+ * this target relation.
+ */
+ bool *relsubs_blocked;
Similarly, maybe say "no EPQ tuple for this target relation in a given
EvalPlanQual() invocation" here?
BTW, I didn't quite understand why EPQ involving resultRelations must
behave in this new way but not the EPQ during LockRows?
> What I'm thinking about doing to back-patch this is to replace
> one of the pointer fields in EPQState with a pointer to a
> subsidiary palloc'd structure, where we can put the new fields
> along with the cannibalized old one. We've done something
> similar before, and it seems a lot safer than having basically
> different logic in v16 than earlier branches.
+1.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: The documentation for READ COMMITTED may be incomplete or wrong
2023-05-19 12:53 Re: The documentation for READ COMMITTED may be incomplete or wrong Amit Langote <[email protected]>
@ 2023-05-19 16:22 ` Tom Lane <[email protected]>
1 sibling, 0 replies; 5+ messages in thread
From: Tom Lane @ 2023-05-19 16:22 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Aleksander Alekseev <[email protected]>; PostgreSQL Hackers <[email protected]>
Amit Langote <[email protected]> writes:
> On Fri, May 19, 2023 at 8:57 AM Tom Lane <[email protected]> wrote:
> + * Initialize per-relation EPQ tuple states. Result relations, if any,
> + * get marked as blocked; others as not-fetched.
> Would it be helpful to clarify that "blocked" means blocked for a
> given EvalPlanQual() cycle?
Probably best to put that with the data structure's comments. I changed
those to look like
/*
* relsubs_done[scanrelid - 1] is true if there is no EPQ tuple for this
* target relation or it has already been fetched in the current scan of
* this target relation within the current EvalPlanQual test.
*/
bool *relsubs_done;
/*
* relsubs_blocked[scanrelid - 1] is true if there is no EPQ tuple for
* this target relation during the current EvalPlanQual test. We keep
* these flags set for all relids listed in resultRelations, but
* transiently clear the one for the relation whose tuple is actually
* passed to EvalPlanQual().
*/
bool *relsubs_blocked;
> BTW, I didn't quite understand why EPQ involving resultRelations must
> behave in this new way but not the EPQ during LockRows?
LockRows doesn't have a bug: it always fills all the EPQ tuple slots
it's responsible for, and it doesn't use EvalPlanQual() anyway.
In the name of simplicity I kept the behavior exactly the same for
callers other than nodeModifyTable.
Perhaps replication/logical/worker.c could use a closer look here.
It's not entirely clear to me that the EPQ state it sets up is ever
used; but if it is I think it is okay as I have it here, because it
looks like those invocations always have just one result relation in
the plan, so there aren't any "extra" result rels that need to be
blocked.
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: The documentation for READ COMMITTED may be incomplete or wrong
2023-05-19 12:53 Re: The documentation for READ COMMITTED may be incomplete or wrong Amit Langote <[email protected]>
@ 2023-05-19 18:33 ` Tom Lane <[email protected]>
2023-05-19 18:42 ` Re: The documentation for READ COMMITTED may be incomplete or wrong Aleksander Alekseev <[email protected]>
1 sibling, 1 reply; 5+ messages in thread
From: Tom Lane @ 2023-05-19 18:33 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Aleksander Alekseev <[email protected]>; PostgreSQL Hackers <[email protected]>
Amit Langote <[email protected]> writes:
> On Fri, May 19, 2023 at 8:57 AM Tom Lane <[email protected]> wrote:
>> What I'm thinking about doing to back-patch this is to replace
>> one of the pointer fields in EPQState with a pointer to a
>> subsidiary palloc'd structure, where we can put the new fields
>> along with the cannibalized old one. We've done something
>> similar before, and it seems a lot safer than having basically
>> different logic in v16 than earlier branches.
> +1.
Done that way. I chose to replace the tuple_table field, because
it was in a convenient spot and it seemed like the field least
likely to have any outside code referencing it.
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: The documentation for READ COMMITTED may be incomplete or wrong
2023-05-19 12:53 Re: The documentation for READ COMMITTED may be incomplete or wrong Amit Langote <[email protected]>
2023-05-19 18:33 ` Re: The documentation for READ COMMITTED may be incomplete or wrong Tom Lane <[email protected]>
@ 2023-05-19 18:42 ` Aleksander Alekseev <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Aleksander Alekseev @ 2023-05-19 18:42 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>; +Cc: Tom Lane <[email protected]>; Amit Langote <[email protected]>; Nathan Bossart <[email protected]>
Hi Tom,
> Done that way. I chose to replace the tuple_table field, because
> it was in a convenient spot and it seemed like the field least
> likely to have any outside code referencing it.
Many thanks!
If it's not too much trouble could you please recommend good entry
points to learn more about the internals of this part of the system
and accompanying edge cases? Perhaps there is an experiment or two an
extension author can do in order to lower the entry threshold and/or
known bugs, limitations or wanted features one could start with?
--
Best regards,
Aleksander Alekseev
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-05-19 18:42 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 3/5] Add default_toast_compression GUC Dilip Kumar <[email protected]>
2023-05-19 12:53 Re: The documentation for READ COMMITTED may be incomplete or wrong Amit Langote <[email protected]>
2023-05-19 16:22 ` Re: The documentation for READ COMMITTED may be incomplete or wrong Tom Lane <[email protected]>
2023-05-19 18:33 ` Re: The documentation for READ COMMITTED may be incomplete or wrong Tom Lane <[email protected]>
2023-05-19 18:42 ` Re: The documentation for READ COMMITTED may be incomplete or wrong Aleksander Alekseev <[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