public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v24 05/10] fixups.patch
7+ messages / 5 participants
[nested] [flat]
* [PATCH v24 05/10] fixups.patch
@ 2021-02-10 21:56 Robert Haas <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Robert Haas @ 2021-02-10 21:56 UTC (permalink / raw)
---
doc/src/sgml/ddl.sgml | 3 ---
doc/src/sgml/ref/create_table.sgml | 15 +++++++++++----
src/backend/access/common/detoast.c | 5 ++---
src/backend/access/compression/compress_lz4.c | 10 +++++-----
src/backend/executor/nodeModifyTable.c | 2 +-
src/include/nodes/execnodes.h | 4 ++--
6 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 4d7ed698b9..1e9a4625cc 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3762,9 +3762,6 @@ CREATE TABLE measurement (
<productname>PostgreSQL</productname>
tables (or, possibly, foreign tables). It is possible to specify a
tablespace and storage parameters for each partition separately.
- By default, each column in a partition inherits the compression method
- from parent table's column, however a different compression method can be
- set for each partition.
</para>
<para>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 51a7a977a5..a4b297340f 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -997,10 +997,17 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<term><literal>COMPRESSION <replaceable class="parameter">compression_method</replaceable></literal></term>
<listitem>
<para>
- This sets the compression method for a 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>. The default
+ 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
+ has no direct effect, because such tables have no storage of their own,
+ but the configured value is 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>.
</para>
</listitem>
diff --git a/src/backend/access/common/detoast.c b/src/backend/access/common/detoast.c
index b78d49167b..95d5b1c12a 100644
--- a/src/backend/access/common/detoast.c
+++ b/src/backend/access/common/detoast.c
@@ -477,8 +477,8 @@ toast_get_compression_oid(struct varlena *attr)
return InvalidOid;
/*
- * Just fetch the toast compress header to know the compression method
- * in the compressed data.
+ * Fetch just enough of the value to examine the compression header,
+ * so that we can find out the compression method.
*/
attr = toast_fetch_datum_slice(attr, 0, VARHDRSZ_COMPRESS);
}
@@ -503,7 +503,6 @@ toast_get_compression_handler(struct varlena *attr)
cmid = TOAST_COMPRESS_METHOD(attr);
- /* Get the handler routines for the compression method */
switch (cmid)
{
case PGLZ_COMPRESSION_ID:
diff --git a/src/backend/access/compression/compress_lz4.c b/src/backend/access/compression/compress_lz4.c
index 1856cf7df7..3079eff7eb 100644
--- a/src/backend/access/compression/compress_lz4.c
+++ b/src/backend/access/compression/compress_lz4.c
@@ -23,7 +23,7 @@
/*
* lz4_cmcompress - compression routine for lz4 compression method
*
- * Compresses source into dest using the default strategy. Returns the
+ * Compresses source into dest using the LZ4 defaults. Returns the
* compressed varlena, or NULL if compression fails.
*/
static struct varlena *
@@ -42,8 +42,8 @@ lz4_cmcompress(const struct varlena *value)
valsize = VARSIZE_ANY_EXHDR(value);
/*
- * Get maximum size of the compressed data that lz4 compression may output
- * and allocate the memory for holding the compressed data and the header.
+ * Figure out the maximum possible size of the LZ4 output, add the bytes
+ * that will be needed for varlena overhead, and allocate that amount.
*/
max_size = LZ4_compressBound(valsize);
tmp = (struct varlena *) palloc(max_size + VARHDRSZ_COMPRESS);
@@ -83,10 +83,10 @@ lz4_cmdecompress(const struct varlena *value)
int32 rawsize;
struct varlena *result;
- /* allocate memory for holding the uncompressed data */
+ /* allocate memory for the uncompressed data */
result = (struct varlena *) palloc(VARRAWSIZE_4B_C(value) + VARHDRSZ);
- /* decompress data using lz4 routine */
+ /* decompress the data */
rawsize = LZ4_decompress_safe((char *) value + VARHDRSZ_COMPRESS,
VARDATA(result),
VARSIZE(value) - VARHDRSZ_COMPRESS,
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index f77deee399..920d9dd0d5 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2117,7 +2117,7 @@ CompareCompressionMethodAndDecompress(TupleTableSlot *slot,
TupleTableSlot *newslot = *outslot;
/*
- * If the called has passed an invalid slot then create a new slot.
+ * If the caller has passed an invalid slot then create a new slot.
* Otherwise, just clear the existing tuple from the slot. This slot
* should be stored by the caller so that it can be reused for
* decompressing the subsequent tuples.
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 18e8ecfd90..d8483c36b3 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1183,8 +1183,8 @@ typedef struct ModifyTableState
TupleTableSlot *mt_root_tuple_slot;
/*
- * Slot for storing the modified tuple, incase the target attribute's
- * compression method doesn't match with the source table.
+ * Slot for storing the modified tuple, in case the target attribute's
+ * compression method doesn't match that of the source table.
*/
TupleTableSlot *mt_decompress_tuple_slot;
--
2.17.0
--m51xatjYGsM+13rf
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v24-0006-alter-table-set-compression.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: RFI: Extending the TOAST Pointer
@ 2023-05-18 10:51 Aleksander Alekseev <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Aleksander Alekseev @ 2023-05-18 10:51 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>; +Cc: Nikita Malakhov <[email protected]>
Hi Nikita,
> this part of the PostgreSQL screams to be revised and improved
I completely agree. The problem with TOAST pointers is that they are
not extendable at the moment which prevents adding new compression
algorithms (e.g. ZSTD), new features like compression dictionaries
[1], etc. I suggest we add extensibility in order to solve this
problem for the foreseeable future for everyone.
> where Custom TOAST Pointer is distinguished from Regular one by va_flag field
> which is a part of varlena header
I don't think that varlena header is the best place to distinguish a
classical TOAST pointer from an extended one. On top of that I don't
see any free bits that would allow adding such a flag to the on-disk
varlena representation [2].
The current on-disk TOAST pointer representation is following:
```
typedef struct varatt_external
{
int32 va_rawsize; /* Original data size (includes header) */
uint32 va_extinfo; /* External saved size (without header) and
* compression method */
Oid va_valueid; /* Unique ID of value within TOAST table */
Oid va_toastrelid; /* RelID of TOAST table containing it */
} varatt_external;
```
Note that currently only 2 compression methods are supported:
```
typedef enum ToastCompressionId
{
TOAST_PGLZ_COMPRESSION_ID = 0,
TOAST_LZ4_COMPRESSION_ID = 1,
TOAST_INVALID_COMPRESSION_ID = 2
} ToastCompressionId;
```
I suggest adding a new flag that will mark an extended TOAST format:
```
typedef enum ToastCompressionId
{
TOAST_PGLZ_COMPRESSION_ID = 0,
TOAST_LZ4_COMPRESSION_ID = 1,
TOAST_RESERVED_COMPRESSION_ID = 2,
TOAST_HAS_EXTENDED_FORMAT = 3,
} ToastCompressionId;
```
For an extended format we add a varint (utf8-like) bitmask right after
varatt_external that marks the features supported in this particular
instance of the pointer. The rest of the data is interpreted depending
on the bits set. This will allow us to extend the pointers
indefinitely.
Note that the proposed approach doesn't require running any
migrations. Note also that I described only the on-disk
representation. We can tweak the in-memory representation as we want
without affecting the end user.
Thoughts?
[1]: https://commitfest.postgresql.org/43/3626/
[2]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/include/postgres.h;h=0446daa0e61722...
--
Best regards,
Aleksander Alekseev
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: RFI: Extending the TOAST Pointer
@ 2023-05-18 12:05 Matthias van de Meent <[email protected]>
parent: Aleksander Alekseev <[email protected]>
0 siblings, 2 replies; 7+ messages in thread
From: Matthias van de Meent @ 2023-05-18 12:05 UTC (permalink / raw)
To: Aleksander Alekseev <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Nikita Malakhov <[email protected]>
On Thu, 18 May 2023 at 12:52, Aleksander Alekseev
<[email protected]> wrote:
>
> Hi Nikita,
>
> > this part of the PostgreSQL screams to be revised and improved
>
> I completely agree. The problem with TOAST pointers is that they are
> not extendable at the moment which prevents adding new compression
> algorithms (e.g. ZSTD), new features like compression dictionaries
> [1], etc. I suggest we add extensibility in order to solve this
> problem for the foreseeable future for everyone.
>
> > where Custom TOAST Pointer is distinguished from Regular one by va_flag field
> > which is a part of varlena header
>
> I don't think that varlena header is the best place to distinguish a
> classical TOAST pointer from an extended one. On top of that I don't
> see any free bits that would allow adding such a flag to the on-disk
> varlena representation [2].
>
> The current on-disk TOAST pointer representation is following:
>
> ```
> typedef struct varatt_external
> {
> int32 va_rawsize; /* Original data size (includes header) */
> uint32 va_extinfo; /* External saved size (without header) and
> * compression method */
> Oid va_valueid; /* Unique ID of value within TOAST table */
> Oid va_toastrelid; /* RelID of TOAST table containing it */
> } varatt_external;
> ```
No, that's inaccurate. The complete on-disk representation of a varatt is
{
uint8 va_header; /* Always 0x80 or 0x01 */
uint8 va_tag; /* Type of datum */
char va_data[FLEXIBLE_ARRAY_MEMBER]; /* Type-dependent
data, for toasted values that's currently only a varatt_external */
} varattrib_1b_e;
With va_tag being filled with one of the vartag_external values:
typedef enum vartag_external
{
VARTAG_INDIRECT = 1,
VARTAG_EXPANDED_RO = 2,
VARTAG_EXPANDED_RW = 3,
VARTAG_ONDISK = 18
} vartag_external;
This enum still has many options to go before it exceeds the maximum
of the uint8 va_tag field. Therefore, I don't think we have no disk
representations left, nor do I think we'll need to add another option
to the ToastCompressionId enum.
As an example, we can add another VARTAG option for dictionary-enabled
external toast; like what the pluggable toast patch worked on. I think
we can salvage some ideas from that patch, even if the main idea got
stuck.
Kind regards,
Matthias van de Meent
Neon Inc.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: RFI: Extending the TOAST Pointer
@ 2023-05-18 12:57 Aleksander Alekseev <[email protected]>
parent: Matthias van de Meent <[email protected]>
1 sibling, 0 replies; 7+ messages in thread
From: Aleksander Alekseev @ 2023-05-18 12:57 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>; +Cc: Matthias van de Meent <[email protected]>; Nikita Malakhov <[email protected]>
Hi,
> No, that's inaccurate. The complete on-disk representation of a varatt is
>
> {
> uint8 va_header; /* Always 0x80 or 0x01 */
> uint8 va_tag; /* Type of datum */
> char va_data[FLEXIBLE_ARRAY_MEMBER]; /* Type-dependent
> data, for toasted values that's currently only a varatt_external */
> } varattrib_1b_e;
>
> With va_tag being filled with one of the vartag_external values:
>
> typedef enum vartag_external
> {
> VARTAG_INDIRECT = 1,
> VARTAG_EXPANDED_RO = 2,
> VARTAG_EXPANDED_RW = 3,
> VARTAG_ONDISK = 18
> } vartag_external;
>
> This enum still has many options to go before it exceeds the maximum
> of the uint8 va_tag field. Therefore, I don't think we have no disk
> representations left, nor do I think we'll need to add another option
> to the ToastCompressionId enum.
> As an example, we can add another VARTAG option for dictionary-enabled
> external toast; like what the pluggable toast patch worked on. I think
> we can salvage some ideas from that patch, even if the main idea got
> stuck.
The problem here is that the comments are ambiguous regarding what to
call "TOAST pointer" exactly. I proposed a patch for this but it was
not accepted [1].
So the exact on-disk representation of a TOAST'ed value (for
little-endian machines) is:
0b00000001, 18 (va_tag), (varatt_external here)
Where 18 is sizeof(varatt_external) + 2, because the length includes
the length of the header.
I agree that va_tag can have another use. But since we are going to
make varatt_external variable in size (otherwise I don't see how it
could be really **extendable**) I don't think this is the right
approach.
Also I agree that this particular statement is incorrect:
> This will allow us to extend the pointers indefinitely.
varatt_external is going to be limited to 255. But it seems to be a
reasonable limitation for the nearest 10-20 years or so.
[1]: https://commitfest.postgresql.org/39/3820/
--
Best regards,
Aleksander Alekseev
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: RFI: Extending the TOAST Pointer
@ 2023-05-23 16:33 Robert Haas <[email protected]>
parent: Matthias van de Meent <[email protected]>
1 sibling, 1 reply; 7+ messages in thread
From: Robert Haas @ 2023-05-23 16:33 UTC (permalink / raw)
To: Matthias van de Meent <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; PostgreSQL Hackers <[email protected]>; Nikita Malakhov <[email protected]>
On Thu, May 18, 2023 at 8:06 AM Matthias van de Meent
<[email protected]> wrote:
> This enum still has many options to go before it exceeds the maximum
> of the uint8 va_tag field. Therefore, I don't think we have no disk
> representations left, nor do I think we'll need to add another option
> to the ToastCompressionId enum.
> As an example, we can add another VARTAG option for dictionary-enabled
> external toast; like what the pluggable toast patch worked on. I think
> we can salvage some ideas from that patch, even if the main idea got
> stuck.
Adding another VARTAG option is somewhat different from adding another
ToastCompressionId. I think that right now we have embedded in various
places the idea that VARTAG_EXTERNAL is the only thing that shows up
on disk, and we'd need to track down all such places and adjust them
if we add other VARTAG types in the future. Depending on how it is to
be used, adding a new ToastCompressionId might be less work. However,
I don't think we can use the possibility of adding a new VARTAG value
as a reason why it's OK to use up the last possible ToastCompressionId
value for something non-extensible.
For projects like this, the details matter a lot. If the goal is to
add a new compression type that behaves like the existing compression
types, more or less, then I think we should allocate the last
ToastCompressionId bit to mean "some other compression ID" and add a
1-byte header that says which one is in use. But if the new feature
being added is enough different from the other compression methods,
then it might be better to do it in some other way e.g. a new VARTAG.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: RFI: Extending the TOAST Pointer
@ 2023-05-24 00:17 Michael Paquier <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Michael Paquier @ 2023-05-24 00:17 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Matthias van de Meent <[email protected]>; Aleksander Alekseev <[email protected]>; PostgreSQL Hackers <[email protected]>; Nikita Malakhov <[email protected]>
On Tue, May 23, 2023 at 12:33:50PM -0400, Robert Haas wrote:
> For projects like this, the details matter a lot. If the goal is to
> add a new compression type that behaves like the existing compression
> types, more or less, then I think we should allocate the last
> ToastCompressionId bit to mean "some other compression ID" and add a
> 1-byte header that says which one is in use. But if the new feature
> being added is enough different from the other compression methods,
> then it might be better to do it in some other way e.g. a new VARTAG.
Agreed. While the compression argument and the possibility to add
more options to toast pointers are very appealing, FWIW, I'd like to
think that the primary target is the 4-byte OID assignment limit of
where backends loop infinitely until a OID can be found, which can be
a real pain for users with a large number of blobs or just enough
toast data to trigger it.
Saying that even if I sent the patch for zstd on toast..
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../ZG1XjI5meIvQNS%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: RFI: Extending the TOAST Pointer
@ 2023-05-24 09:16 Nikita Malakhov <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Nikita Malakhov @ 2023-05-24 09:16 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Matthias van de Meent <[email protected]>; Aleksander Alekseev <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi!
I've made a WIP patch that uses 64-bit TOAST value ID instead of 32-bit,
and sent it as a part of discussion, but there was no feedback on such a
solution. There was a link to that discussion at the top of this thread.
Also, I have to note that, based on our work on Pluggable TOAST - extending
TOAST pointer with additional structures would require review of the logical
replication engine, currently it is not suitable for any custom TOAST
pointers.
Currently we have no final solution for problems with logical replication
for
custom TOAST pointers.
--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2023-05-24 09:16 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-02-10 21:56 [PATCH v24 05/10] fixups.patch Robert Haas <[email protected]>
2023-05-18 10:51 Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-18 12:05 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
2023-05-18 12:57 ` Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-23 16:33 ` Re: RFI: Extending the TOAST Pointer Robert Haas <[email protected]>
2023-05-24 00:17 ` Re: RFI: Extending the TOAST Pointer Michael Paquier <[email protected]>
2023-05-24 09:16 ` Re: RFI: Extending the TOAST Pointer Nikita Malakhov <[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