public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nikita Malakhov <[email protected]>
To: Teodor Sigaev <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Pluggable toaster
Date: Thu, 10 Mar 2022 11:47:32 +0300
Message-ID: <CAN-LCVORMn85jAL4GCWpQf+H+eZ3vFoO2r+tOEn7k47uw2A9gg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAF4Au4zsdZ3t=f+YAaq12-SzqfCxfOTc1NanjBcUsw+5L-0iiA@mail.gmail.com>
<[email protected]>
<CA+TgmoYOZ0Ofoo1DJ+iRa6Mg8MACu0eb8tr-uggfNMZmD6bpkA@mail.gmail.com>
<[email protected]>
Hi Hackers,
In addition to original patch set for Pluggable Toaster, we have two more
patches
(actually, small, but important fixes), authored by Nikita Glukhov:
1) 0001-Fix-toast_tuple_externalize.patch
This patch fixes freeing memory in case of new toasted value is the same as
old one,
this seems incorrect, and in this case the function just returns instead of
freeing old value;
2) 0002-Fix-alignment-of-custom-TOAST-pointers.patch
This patch adds data alignment for new varatt_custom data structure in
building tuples,
since varatt_custom must be aligned for custom toasters (in particular,
this fix is very
important to JSONb Toaster).
These patches must be applied on top of the latter
4_bytea_appendable_toaster_v1.patch.
Please consider them in reviewing Pluggable Toaster.
Regards.
On Wed, Feb 2, 2022 at 10:35 AM Teodor Sigaev <[email protected]> wrote:
> > I agree ... but I'm also worried about what happens when we have
> > multiple table AMs. One can imagine a new table AM that is
> > specifically optimized for TOAST which can be used with an existing
> > heap table. One can imagine a new table AM for the main table that
> > wants to use something different for TOAST. So, I don't think it's
> > right to imagine that the choice of TOASTer depends solely on the
> > column data type. I'm not really sure how this should work exactly ...
> > but it needs careful thought.
>
> Right. that's why we propose a validate method (may be, it's a wrong
> name, but I don't known better one) which accepts several arguments, one
> of which is table AM oid. If that method returns false then toaster
> isn't useful with current TAM, storage or/and compression kinds, etc.
>
> --
> Teodor Sigaev E-mail: [email protected]
> WWW: http://www.sigaev.ru/
>
>
>
--
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/
Attachments:
[application/octet-stream] 0001-Fix-toast_tuple_externalize.patch (851B, ../CAN-LCVORMn85jAL4GCWpQf+H+eZ3vFoO2r+tOEn7k47uw2A9gg@mail.gmail.com/3-0001-Fix-toast_tuple_externalize.patch)
download | inline diff:
From c16114bf17cee5b5649d3dfa462c4a8b594fb2f0 Mon Sep 17 00:00:00 2001
From: Nikita Glukhov <[email protected]>
Date: Tue, 22 Feb 2022 20:52:45 +0300
Subject: [PATCH 1/2] Fix toast_tuple_externalize()
---
src/backend/access/table/toast_helper.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/backend/access/table/toast_helper.c b/src/backend/access/table/toast_helper.c
index bfbe941d6ac..11d870535e1 100644
--- a/src/backend/access/table/toast_helper.c
+++ b/src/backend/access/table/toast_helper.c
@@ -345,6 +345,9 @@ toast_tuple_externalize(ToastTupleContext *ttc, int attribute, int maxDataLen,
maxDataLen, options)
);
+ if (*value == old_value)
+ return;
+
if ((attr->tai_colflags & TOASTCOL_NEEDS_FREE) != 0)
pfree(DatumGetPointer(old_value));
attr->tai_colflags |= TOASTCOL_NEEDS_FREE;
--
2.25.1
[application/octet-stream] 0002-Fix-alignment-of-custom-TOAST-pointers.patch (1.5K, ../CAN-LCVORMn85jAL4GCWpQf+H+eZ3vFoO2r+tOEn7k47uw2A9gg@mail.gmail.com/4-0002-Fix-alignment-of-custom-TOAST-pointers.patch)
download | inline diff:
From 6db6630943be39eee31cb0411e2a849ec97bc66c Mon Sep 17 00:00:00 2001
From: Nikita Glukhov <[email protected]>
Date: Wed, 12 Jan 2022 01:23:18 +0300
Subject: [PATCH 2/2] Fix alignment of custom TOAST pointers
---
src/backend/access/common/heaptuple.c | 5 ++++-
src/include/access/tupmacs.h | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index 0b56b0fa5a9..47c808d462f 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -242,7 +242,10 @@ fill_val(Form_pg_attribute att,
else
{
*infomask |= HEAP_HASEXTERNAL;
- /* no alignment, since it's short by definition */
+ if (VARATT_IS_CUSTOM(val))
+ data = (char *) att_align_nominal(data,
+ att->attalign);
+ /* else no alignment, since it's short by definition */
data_length = VARSIZE_EXTERNAL(val);
memcpy(data, val, data_length);
}
diff --git a/src/include/access/tupmacs.h b/src/include/access/tupmacs.h
index 65ac1ef3fc8..ddb164ef2ba 100644
--- a/src/include/access/tupmacs.h
+++ b/src/include/access/tupmacs.h
@@ -104,7 +104,7 @@
*/
#define att_align_datum(cur_offset, attalign, attlen, attdatum) \
( \
- ((attlen) == -1 && VARATT_IS_SHORT(DatumGetPointer(attdatum))) ? \
+ ((attlen) == -1 && (!VARATT_IS_CUSTOM(DatumGetPointer(attdatum)) && VARATT_IS_SHORT(DatumGetPointer(attdatum)))) ? \
(uintptr_t) (cur_offset) : \
att_align_nominal(cur_offset, attalign) \
)
--
2.25.1
view thread (39+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Pluggable toaster
In-Reply-To: <CAN-LCVORMn85jAL4GCWpQf+H+eZ3vFoO2r+tOEn7k47uw2A9gg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox