Create TOAST table only if AM needs
Currently TOAST table is always created (if needed based on data type
properties) independent of table AM. How toasting is handled seems
should be AM responsibility. Generic code shouldn't force the use of
the separate table for the same. Like for Zedstore we store toasted
chunks in separate blocks but within the table file itself and don't
need separate toast table. Some other AM may implement the
functionality differently. So, similar to relation forks, usage of
toast table should be optional and left to AM to handle.
Wish to discuss ways on how best to achieve it. Attaching patch just
to showcase a way could be done. The patch adds property to
TableAmRoutine to convey if AM uses separate Toast table or not.
Other possibility could be with some refactoring move toast table
creation inside relation_set_new_filenode callback or provide separate
callback for Toast Table creation to AM.
Attachments:
0001-Create-toast-table-only-if-AM-needs-it.patchtext/x-patch; charset=US-ASCII; name=0001-Create-toast-table-only-if-AM-needs-it.patchDownload
From fcbf6b8187ef8389cf38cd185b709bcb6ef1152a Mon Sep 17 00:00:00 2001
From: Ashwin Agrawal <aagrawal@pivotal.io>
Date: Fri, 10 May 2019 17:42:06 -0700
Subject: [PATCH 1/1] Create toast table only if AM needs it.
All AM may not need toast table, hence add property to TableAmRoutine
to define if uses toast table or not. Only for AM using the toast
table create the toast table, otherwise skip the same. Heap AM sets
the property as true.
---
src/backend/access/heap/heapam_handler.c | 1 +
src/backend/catalog/toasting.c | 5 +++++
src/include/access/tableam.h | 7 +++++++
3 files changed, 13 insertions(+)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 00505ec3f4d..243a221c923 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2515,6 +2515,7 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
static const TableAmRoutine heapam_methods = {
.type = T_TableAmRoutine,
+ .uses_toast_table = true,
.slot_callbacks = heapam_slot_callbacks,
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 2276d3c5d36..2345ef8ee84 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -407,6 +407,11 @@ needs_toast_table(Relation rel)
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
return false;
+ Assert(rel->rd_tableam != NULL);
+ /* No need to create TOAST table if AM doesn't need one */
+ if (!table_uses_toast_table(rel))
+ return false;
+
/*
* We cannot allow toasting a shared relation after initdb (because
* there's no way to mark it toasted in other databases' pg_class).
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index ebfa0d51855..ed116e18f10 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -142,6 +142,8 @@ typedef struct TableAmRoutine
{
/* this must be set to T_TableAmRoutine */
NodeTag type;
+ /* does AM need separate TOAST table */
+ bool uses_toast_table;
/* ------------------------------------------------------------------------
@@ -657,6 +659,11 @@ typedef struct TableAmRoutine
} TableAmRoutine;
+static inline bool
+table_uses_toast_table(Relation relation)
+{
+ return relation->rd_tableam->uses_toast_table;
+}
/* ----------------------------------------------------------------------------
* Slot functions.
--
2.19.1
Hi,
On 2019-05-17 11:26:29 -0700, Ashwin Agrawal wrote:
Currently TOAST table is always created (if needed based on data type
properties) independent of table AM. How toasting is handled seems
should be AM responsibility. Generic code shouldn't force the use of
the separate table for the same. Like for Zedstore we store toasted
chunks in separate blocks but within the table file itself and don't
need separate toast table. Some other AM may implement the
functionality differently. So, similar to relation forks, usage of
toast table should be optional and left to AM to handle.
Yea, Robert is also working on this. In fact, we were literally chatting
about it a few minutes ago. He'll probably chime in too.
+static inline bool +table_uses_toast_table(Relation relation) +{ + return relation->rd_tableam->uses_toast_table; +}
Don't think this is sufficient - imo it needs to be a callback to look
at the columns etc.
My inclination is that it's too late for 12 to do anything about
this. There are many known limitations, and we'll discover many more, of
the current tableam interface. If we try to fix them for 12, we'll never
get anywhere. It'll take a while to iron out all those wrinkles...
Greetings,
Andres Freund
On Fri, May 17, 2019 at 2:34 PM Andres Freund <andres@anarazel.de> wrote:
Yea, Robert is also working on this. In fact, we were literally chatting
about it a few minutes ago. He'll probably chime in too.
Yeah, I'm aiming to post a patch set very soon that does a bunch of
refactoring of the TOAST stuff to make life easier for new AMs - maybe
today, or else Monday. I think your use case of wanting to suppress
TOAST table creation altogether is a valid one, but I want to do go a
little further and make it easier for non-heap AMs to implement
heap-like toasting based on their own page format.
Generally, I would say that the state of play with respect to table
AMs and toasting is annoyingly bad right now:
- If your AM uses some system other than TOAST to store large values,
you are out of luck. You will get TOAST tables whether you want them
or not.
- If your AM uses some page or tuple format that results in a
different maximum tuple size, you are out of luck. You will get TOAST
tables based on whether a heap table with the same set of columns
would need one.
- If your AM would like to use the heap for TOAST data, you are out of
luck. The AM used for TOAST data must be the same as the AM used for
the main table.
- If your AM would like to use itself to store TOAST data, you are
also out of luck, because all of the existing TOAST code works with
heap tuples.
- Even if you copy all of tuptoaster.c/h - which is a lot of code -
and change everything that is different for your AM than for the
regular heap, you are still out of luck, because code that knows
nothing about tableam is going to call heap_tuple_untoast_attr() to
detoast stuff, and that code is only going to be happy if you've used
the same chunk size that we use for the regular heap, and that chunk
size has a good chance of being mildly to severely suboptimal if your
heap has made any sort of page format changes.
So I think this basically just doesn't work right now. I am
sympathetic to Andres's position that we shouldn't go whacking the
code around too much at this late date, and he's probably right that
we're going to find lots of other problems with tableam as well and
you have to draw the line someplace, but on the other hand given your
experience and mine, it's probably pretty likely that anybody who
tries to use tableam for anything is going to run into this problem,
so maybe it's not crazy to think about a few last-minute changes.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
So I think this basically just doesn't work right now. I am
sympathetic to Andres's position that we shouldn't go whacking the
code around too much at this late date, and he's probably right that
we're going to find lots of other problems with tableam as well and
you have to draw the line someplace, but on the other hand given your
experience and mine, it's probably pretty likely that anybody who
tries to use tableam for anything is going to run into this problem,
so maybe it's not crazy to think about a few last-minute changes.
It seems to me that the entire tableam project is still very much WIP,
and if anybody is able to do anything actually useful with a different
AM right at the moment, that's just mighty good fortune for them.
It's way too late to be making destabilizing changes in v12 in order
to move the frontier of what can be done in a new AM. I'm all for
the sorts of changes you're describing here --- but as v13 material.
We should be looking at v12 as something we're trying to get out
the door soon, with as few bugs as possible. "I can't do X in an
external AM" is not a bug, not for v12 anyway.
regards, tom lane
Hi,
On 2019-05-17 15:13:50 -0400, Robert Haas wrote:
On Fri, May 17, 2019 at 2:34 PM Andres Freund <andres@anarazel.de> wrote:
Yea, Robert is also working on this. In fact, we were literally chatting
about it a few minutes ago. He'll probably chime in too.Yeah, I'm aiming to post a patch set very soon that does a bunch of
refactoring of the TOAST stuff to make life easier for new AMs - maybe
today, or else Monday. I think your use case of wanting to suppress
TOAST table creation altogether is a valid one, but I want to do go a
little further and make it easier for non-heap AMs to implement
heap-like toasting based on their own page format.Generally, I would say that the state of play with respect to table
AMs and toasting is annoyingly bad right now:- If your AM uses some system other than TOAST to store large values,
you are out of luck. You will get TOAST tables whether you want them
or not.
Which is aesthetically and indode usage wise annoying, but not
*terrible*. You get a a bunch of useless pg_class/pg_index entries and a
few close-to-empty relfilenodes.
- If your AM uses some page or tuple format that results in a
different maximum tuple size, you are out of luck. You will get TOAST
tables based on whether a heap table with the same set of columns
would need one.
- If your AM would like to use the heap for TOAST data, you are out of
luck. The AM used for TOAST data must be the same as the AM used for
the main table.- If your AM would like to use itself to store TOAST data, you are
also out of luck, because all of the existing TOAST code works with
heap tuples.- Even if you copy all of tuptoaster.c/h - which is a lot of code -
and change everything that is different for your AM than for the
regular heap, you are still out of luck, because code that knows
nothing about tableam is going to call heap_tuple_untoast_attr() to
detoast stuff, and that code is only going to be happy if you've used
the same chunk size that we use for the regular heap, and that chunk
size has a good chance of being mildly to severely suboptimal if your
heap has made any sort of page format changes.
Well, I don't *quite* buy the suboptimal. As far as I can tell, the
current chunking size doesn't have much going for it for heap either -
and while a few people (me including) have complained about it, it's not
that many people either. My impression is that the current chunking is
essentially a randomly chosen choice without much to go for it, and so
it's not going to be much different for other AMs.
So I think this basically just doesn't work right now.
I mean, the zheap on tableam code copies more toast code than I'm happy
about, and it's chunking is somewhat suboptimal, but that's not
*terrible*. There's no if(zheap) branches outside of zheap related to
toasting.
Greetings,
Andres Freund
On 2019-05-17 15:26:38 -0400, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
So I think this basically just doesn't work right now. I am
sympathetic to Andres's position that we shouldn't go whacking the
code around too much at this late date, and he's probably right that
we're going to find lots of other problems with tableam as well and
you have to draw the line someplace, but on the other hand given your
experience and mine, it's probably pretty likely that anybody who
tries to use tableam for anything is going to run into this problem,
so maybe it's not crazy to think about a few last-minute changes.It seems to me that the entire tableam project is still very much WIP,
Agreed on that front.
and if anybody is able to do anything actually useful with a different
AM right at the moment, that's just mighty good fortune for them.
I think this is too negative. Yes, there's a warts, but you can write
something like zheap without tableam related code modifications (undo
however...). You can write something like zedstore, and it will works,
with a few warts. Yes, a bit of code duplication, and a few efficiency
losses are to be expected. But that's different from it being impossible
to write an AM.
"I can't do X in an external AM" is not a bug, not for v12 anyway.
Indeed.
Greetings,
Andres Freund
On Fri, May 17, 2019 at 3:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
It seems to me that the entire tableam project is still very much WIP,
and if anybody is able to do anything actually useful with a different
AM right at the moment, that's just mighty good fortune for them.
It's way too late to be making destabilizing changes in v12 in order
to move the frontier of what can be done in a new AM.
What about non-destabilizing changes? It seems to me that we could do
some good with a pretty simple patch that just moves most of the logic
from needs_toast_table() below tableam, as in the attached. Then we
could leave the broader refactoring for v13.
Maybe this is still too much, but it seems pretty simple so I thought I'd ask.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
0001-tableam-Move-heap-specific-logic-from-needs_toast_ta.patchapplication/octet-stream; name=0001-tableam-Move-heap-specific-logic-from-needs_toast_ta.patchDownload
From 6ab459b23824e7ff270be360eea0163f4dc16b3a Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Fri, 17 May 2019 16:01:47 -0400
Subject: [PATCH] tableam: Move heap-specific logic from needs_toast_table
below tableam.
This allows table AMs to completely suppress TOAST table creation, or
to modify the conditions under which they are created.
---
src/backend/access/heap/heapam_handler.c | 59 ++++++++++++++++++++++++
src/backend/catalog/toasting.c | 50 ++------------------
src/include/access/tableam.h | 30 ++++++++++++
3 files changed, 92 insertions(+), 47 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 00505ec3f4..b73e9cd6f1 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -29,6 +29,7 @@
#include "access/rewriteheap.h"
#include "access/tableam.h"
#include "access/tsmapi.h"
+#include "access/tuptoaster.h"
#include "access/xact.h"
#include "catalog/catalog.h"
#include "catalog/index.h"
@@ -1975,6 +1976,62 @@ heapam_scan_get_blocks_done(HeapScanDesc hscan)
}
+/* ------------------------------------------------------------------------
+ * TOAST related callbacks for the heap AM
+ * ------------------------------------------------------------------------
+ */
+
+/*
+ * Check to see whether the table needs a TOAST table. It does only if
+ * (1) there are any toastable attributes, and (2) the maximum length
+ * of a tuple could exceed TOAST_TUPLE_THRESHOLD. (We don't want to
+ * create a toast table for something like "f1 varchar(20)".)
+ */
+static bool
+heapam_needs_toast_table(Relation rel)
+{
+ int32 data_length = 0;
+ bool maxlength_unknown = false;
+ bool has_toastable_attrs = false;
+ TupleDesc tupdesc = rel->rd_att;
+ int32 tuple_length;
+ int i;
+
+ for (i = 0; i < tupdesc->natts; i++)
+ {
+ Form_pg_attribute att = TupleDescAttr(tupdesc, i);
+
+ if (att->attisdropped)
+ continue;
+ data_length = att_align_nominal(data_length, att->attalign);
+ if (att->attlen > 0)
+ {
+ /* Fixed-length types are never toastable */
+ data_length += att->attlen;
+ }
+ else
+ {
+ int32 maxlen = type_maximum_size(att->atttypid,
+ att->atttypmod);
+
+ if (maxlen < 0)
+ maxlength_unknown = true;
+ else
+ data_length += maxlen;
+ if (att->attstorage != 'p')
+ has_toastable_attrs = true;
+ }
+ }
+ if (!has_toastable_attrs)
+ return false; /* nothing to toast? */
+ if (maxlength_unknown)
+ return true; /* any unlimited-length attrs? */
+ tuple_length = MAXALIGN(SizeofHeapTupleHeader +
+ BITMAPLEN(tupdesc->natts)) +
+ MAXALIGN(data_length);
+ return (tuple_length > TOAST_TUPLE_THRESHOLD);
+}
+
/* ------------------------------------------------------------------------
* Planner related callbacks for the heap AM
@@ -2558,6 +2615,8 @@ static const TableAmRoutine heapam_methods = {
.relation_estimate_size = heapam_estimate_rel_size,
+ .needs_toast_table = heapam_needs_toast_table,
+
.scan_bitmap_next_block = heapam_scan_bitmap_next_block,
.scan_bitmap_next_tuple = heapam_scan_bitmap_next_tuple,
.scan_sample_next_block = heapam_scan_sample_next_block,
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 2276d3c5d3..be0a397d9d 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -15,7 +15,6 @@
#include "postgres.h"
#include "access/heapam.h"
-#include "access/tuptoaster.h"
#include "access/xact.h"
#include "catalog/binary_upgrade.h"
#include "catalog/catalog.h"
@@ -386,21 +385,11 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
}
/*
- * Check to see whether the table needs a TOAST table. It does only if
- * (1) there are any toastable attributes, and (2) the maximum length
- * of a tuple could exceed TOAST_TUPLE_THRESHOLD. (We don't want to
- * create a toast table for something like "f1 varchar(20)".)
+ * Check to see whether the table needs a TOAST table.
*/
static bool
needs_toast_table(Relation rel)
{
- int32 data_length = 0;
- bool maxlength_unknown = false;
- bool has_toastable_attrs = false;
- TupleDesc tupdesc;
- int32 tuple_length;
- int i;
-
/*
* No need to create a TOAST table for partitioned tables.
*/
@@ -423,39 +412,6 @@ needs_toast_table(Relation rel)
if (IsCatalogRelation(rel) && !IsBootstrapProcessingMode())
return false;
- tupdesc = rel->rd_att;
-
- for (i = 0; i < tupdesc->natts; i++)
- {
- Form_pg_attribute att = TupleDescAttr(tupdesc, i);
-
- if (att->attisdropped)
- continue;
- data_length = att_align_nominal(data_length, att->attalign);
- if (att->attlen > 0)
- {
- /* Fixed-length types are never toastable */
- data_length += att->attlen;
- }
- else
- {
- int32 maxlen = type_maximum_size(att->atttypid,
- att->atttypmod);
-
- if (maxlen < 0)
- maxlength_unknown = true;
- else
- data_length += maxlen;
- if (att->attstorage != 'p')
- has_toastable_attrs = true;
- }
- }
- if (!has_toastable_attrs)
- return false; /* nothing to toast? */
- if (maxlength_unknown)
- return true; /* any unlimited-length attrs? */
- tuple_length = MAXALIGN(SizeofHeapTupleHeader +
- BITMAPLEN(tupdesc->natts)) +
- MAXALIGN(data_length);
- return (tuple_length > TOAST_TUPLE_THRESHOLD);
+ /* Otherwise, let the AM decide. */
+ return table_needs_toast_table(rel);
}
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index ebfa0d5185..ac0913c579 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -540,6 +540,21 @@ typedef struct TableAmRoutine
struct ValidateIndexState *state);
+ /* ------------------------------------------------------------------------
+ * TOAST support.
+ * ------------------------------------------------------------------------
+ */
+
+ /*
+ * This callback should return true if the relation requires a TOAST table
+ * and false if it does not. It may wish to examine the relation's
+ * tuple descriptor before making a decision, but if it uses some other
+ * method of storing large values (or if it does not support them) it can
+ * simply return false.
+ */
+ bool (*needs_toast_table) (Relation rel);
+
+
/* ------------------------------------------------------------------------
* Planner related functions.
* ------------------------------------------------------------------------
@@ -1503,6 +1518,21 @@ table_index_validate_scan(Relation heap_rel,
}
+/* ----------------------------------------------------------------------------
+ * TOAST
+ * ----------------------------------------------------------------------------
+ */
+
+/*
+ * table_needs_toast_table - does this relation need a toast table?
+ */
+static inline bool
+table_needs_toast_table(Relation rel)
+{
+ return rel->rd_tableam->needs_toast_table(rel);
+}
+
+
/* ----------------------------------------------------------------------------
* Planner related functionality
* ----------------------------------------------------------------------------
--
2.17.2 (Apple Git-113)
Hi,
+ +/* + * Check to see whether the table needs a TOAST table. It does only if + * (1) there are any toastable attributes, and (2) the maximum length + * of a tuple could exceed TOAST_TUPLE_THRESHOLD. (We don't want to + * create a toast table for something like "f1 varchar(20)".) + */ +static bool +heapam_needs_toast_table(Relation rel) +{ + int32 data_length = 0; + bool maxlength_unknown = false; + bool has_toastable_attrs = false; + TupleDesc tupdesc = rel->rd_att; + int32 tuple_length; + int i; + + for (i = 0; i < tupdesc->natts; i++) + { + Form_pg_attribute att = TupleDescAttr(tupdesc, i); + + if (att->attisdropped) + continue; + data_length = att_align_nominal(data_length, att->attalign); + if (att->attlen > 0) + { + /* Fixed-length types are never toastable */ + data_length += att->attlen; + } + else + { + int32 maxlen = type_maximum_size(att->atttypid, + att->atttypmod); + + if (maxlen < 0) + maxlength_unknown = true; + else + data_length += maxlen; + if (att->attstorage != 'p') + has_toastable_attrs = true; + } + } + if (!has_toastable_attrs) + return false; /* nothing to toast? */ + if (maxlength_unknown) + return true; /* any unlimited-length attrs? */ + tuple_length = MAXALIGN(SizeofHeapTupleHeader + + BITMAPLEN(tupdesc->natts)) + + MAXALIGN(data_length); + return (tuple_length > TOAST_TUPLE_THRESHOLD); +}
I'm ok with adding something roughly like this.
/* ------------------------------------------------------------------------
* Planner related callbacks for the heap AM
@@ -2558,6 +2615,8 @@ static const TableAmRoutine heapam_methods = {.relation_estimate_size = heapam_estimate_rel_size,
+ .needs_toast_table = heapam_needs_toast_table, +
I'd rather see this have a relation_ prefix.
Greetings,
Andres Freund
On Fri, May 17, 2019 at 1:51 PM Andres Freund <andres@anarazel.de> wrote:
/*
------------------------------------------------------------------------
* Planner related callbacks for the heap AM
@@ -2558,6 +2615,8 @@ static const TableAmRoutine heapam_methods = {.relation_estimate_size = heapam_estimate_rel_size,
+ .needs_toast_table = heapam_needs_toast_table, +I'd rather see this have a relation_ prefix.
+1 to overall patch with that comment incorporated. This seems simple
enough to incorporate for v12. Though stating that blind-folded with what
else is remaining to be must done for v12.
On Fri, May 17, 2019 at 11:34 AM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2019-05-17 11:26:29 -0700, Ashwin Agrawal wrote:
Currently TOAST table is always created (if needed based on data type
properties) independent of table AM. How toasting is handled seems
should be AM responsibility. Generic code shouldn't force the use of
the separate table for the same. Like for Zedstore we store toasted
chunks in separate blocks but within the table file itself and don't
need separate toast table. Some other AM may implement the
functionality differently. So, similar to relation forks, usage of
toast table should be optional and left to AM to handle.Yea, Robert is also working on this. In fact, we were literally chatting
about it a few minutes ago. He'll probably chime in too.
Thank You.
My inclination is that it's too late for 12 to do anything about
this. There are many known limitations, and we'll discover many more, of
the current tableam interface. If we try to fix them for 12, we'll never
get anywhere. It'll take a while to iron out all those wrinkles...
Agree on that, most of the stuff would be enhancements. And enhancements
can and will be made as we find them. Plus, will get added to the version
active in development that time. Intent is to start the discussion, and not
to convey a bug or has to be fixed in v12.
On Fri, May 17, 2019 at 3:28 PM Andres Freund <andres@anarazel.de> wrote:
- If your AM uses some system other than TOAST to store large values,
you are out of luck. You will get TOAST tables whether you want them
or not.Which is aesthetically and indode usage wise annoying, but not
*terrible*. You get a a bunch of useless pg_class/pg_index entries and a
few close-to-empty relfilenodes.
OK, that's fair.
Well, I don't *quite* buy the suboptimal. As far as I can tell, the
current chunking size doesn't have much going for it for heap either -
and while a few people (me including) have complained about it, it's not
that many people either. My impression is that the current chunking is
essentially a randomly chosen choice without much to go for it, and so
it's not going to be much different for other AMs.
I don't think that's really quite fair. The size is carefully chosen
so that you can fit 4 rows on a page with no free space left over.
The wisdom of that particular choice is debatable, but think how sad
you'd be if your AM had 4 bytes less free space available on every
page (because, idk, you stored the epoch in the special space, or
whatever). If you could somehow get the system to store your TOAST
chunks in your side table, you'd end up only being able to fit 3 toast
chunks per page, because the remaining space after you put in 3 chunks
would be 4 bytes too small for another chunk. That is really the
pits, because now your toast table is going to be 33% larger than it
would have been otherwise.
So I think this basically just doesn't work right now.
I mean, the zheap on tableam code copies more toast code than I'm happy
about, and it's chunking is somewhat suboptimal, but that's not
*terrible*. There's no if(zheap) branches outside of zheap related to
toasting.
I admit to not having studied that terribly closely, so maybe the
situation is not as bad as I think. In any case, it bears saying that
tableam is a remarkable accomplishment regardless of whatever
shortcomings it has in this area or elsewhere. And it's not really
any skin off my neck whether we do anything to improve this for v12 or
not, because no table AM written by me is likely to get deployed
against PostgreSQL 12, so why am I even arguing about this? Am I just
a naturally argumentative person?
Don't answer that...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Fri, May 17, 2019 at 2:42 PM Robert Haas <robertmhaas@gmail.com> wrote:
In any case, it bears saying that
tableam is a remarkable accomplishment regardless of whatever
shortcomings it has in this area or elsewhere.
Big +1 to this.
Just throwing this out there.... Perhaps we should just disable toasting
for non-heap tables entirely for now?
That way at least people can use it and storage plugins just have to be
able to deal with large datums in their own (or throw errors).
On Fri., May 17, 2019, 5:56 p.m. Ashwin Agrawal, <aagrawal@pivotal.io>
wrote:
Show quoted text
On Fri, May 17, 2019 at 2:42 PM Robert Haas <robertmhaas@gmail.com> wrote:
In any case, it bears saying that
tableam is a remarkable accomplishment regardless of whatever
shortcomings it has in this area or elsewhere.Big +1 to this.
On Fri, May 17, 2019 at 5:12 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
I'd rather see this have a relation_ prefix.
+1 to overall patch with that comment incorporated. This seems simple enough to incorporate for v12. Though stating that blind-folded with what else is remaining to be must done for v12.
Rebased and updated patch attached.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
v2-0001-tableam-Move-heap-specific-logic-from-needs_toast.patchapplication/octet-stream; name=v2-0001-tableam-Move-heap-specific-logic-from-needs_toast.patchDownload
From 4e361bfe51810d7c637bf57968da2dfea4197701 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Fri, 17 May 2019 16:01:47 -0400
Subject: [PATCH v2] tableam: Move heap-specific logic from needs_toast_table
below tableam.
This allows table AMs to completely suppress TOAST table creation, or
to modify the conditions under which they are created.
Patch by me. Reviewed by Andres Freund.
Discussion: http://postgr.es/m/CALfoeitE+P8UGii8=BsGQLpHch2EZWJhq4M+D-jfaj8YCa_FSw@mail.gmail.com
---
src/backend/access/heap/heapam_handler.c | 53 ++++++++++++++++++++++++
src/backend/catalog/toasting.c | 50 ++--------------------
src/include/access/tableam.h | 20 +++++++++
3 files changed, 76 insertions(+), 47 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 8d8161fd97..56b2abda5f 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -29,6 +29,7 @@
#include "access/rewriteheap.h"
#include "access/tableam.h"
#include "access/tsmapi.h"
+#include "access/tuptoaster.h"
#include "access/xact.h"
#include "catalog/catalog.h"
#include "catalog/index.h"
@@ -2009,6 +2010,57 @@ heapam_relation_size(Relation rel, ForkNumber forkNumber)
return nblocks * BLCKSZ;
}
+/*
+ * Check to see whether the table needs a TOAST table. It does only if
+ * (1) there are any toastable attributes, and (2) the maximum length
+ * of a tuple could exceed TOAST_TUPLE_THRESHOLD. (We don't want to
+ * create a toast table for something like "f1 varchar(20)".)
+ */
+static bool
+heapam_relation_needs_toast_table(Relation rel)
+{
+ int32 data_length = 0;
+ bool maxlength_unknown = false;
+ bool has_toastable_attrs = false;
+ TupleDesc tupdesc = rel->rd_att;
+ int32 tuple_length;
+ int i;
+
+ for (i = 0; i < tupdesc->natts; i++)
+ {
+ Form_pg_attribute att = TupleDescAttr(tupdesc, i);
+
+ if (att->attisdropped)
+ continue;
+ data_length = att_align_nominal(data_length, att->attalign);
+ if (att->attlen > 0)
+ {
+ /* Fixed-length types are never toastable */
+ data_length += att->attlen;
+ }
+ else
+ {
+ int32 maxlen = type_maximum_size(att->atttypid,
+ att->atttypmod);
+
+ if (maxlen < 0)
+ maxlength_unknown = true;
+ else
+ data_length += maxlen;
+ if (att->attstorage != 'p')
+ has_toastable_attrs = true;
+ }
+ }
+ if (!has_toastable_attrs)
+ return false; /* nothing to toast? */
+ if (maxlength_unknown)
+ return true; /* any unlimited-length attrs? */
+ tuple_length = MAXALIGN(SizeofHeapTupleHeader +
+ BITMAPLEN(tupdesc->natts)) +
+ MAXALIGN(data_length);
+ return (tuple_length > TOAST_TUPLE_THRESHOLD);
+}
+
/* ------------------------------------------------------------------------
* Planner related callbacks for the heap AM
@@ -2592,6 +2644,7 @@ static const TableAmRoutine heapam_methods = {
.index_validate_scan = heapam_index_validate_scan,
.relation_size = heapam_relation_size,
+ .relation_needs_toast_table = heapam_relation_needs_toast_table,
.relation_estimate_size = heapam_estimate_rel_size,
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 2276d3c5d3..cf20ddb4e9 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -15,7 +15,6 @@
#include "postgres.h"
#include "access/heapam.h"
-#include "access/tuptoaster.h"
#include "access/xact.h"
#include "catalog/binary_upgrade.h"
#include "catalog/catalog.h"
@@ -386,21 +385,11 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
}
/*
- * Check to see whether the table needs a TOAST table. It does only if
- * (1) there are any toastable attributes, and (2) the maximum length
- * of a tuple could exceed TOAST_TUPLE_THRESHOLD. (We don't want to
- * create a toast table for something like "f1 varchar(20)".)
+ * Check to see whether the table needs a TOAST table.
*/
static bool
needs_toast_table(Relation rel)
{
- int32 data_length = 0;
- bool maxlength_unknown = false;
- bool has_toastable_attrs = false;
- TupleDesc tupdesc;
- int32 tuple_length;
- int i;
-
/*
* No need to create a TOAST table for partitioned tables.
*/
@@ -423,39 +412,6 @@ needs_toast_table(Relation rel)
if (IsCatalogRelation(rel) && !IsBootstrapProcessingMode())
return false;
- tupdesc = rel->rd_att;
-
- for (i = 0; i < tupdesc->natts; i++)
- {
- Form_pg_attribute att = TupleDescAttr(tupdesc, i);
-
- if (att->attisdropped)
- continue;
- data_length = att_align_nominal(data_length, att->attalign);
- if (att->attlen > 0)
- {
- /* Fixed-length types are never toastable */
- data_length += att->attlen;
- }
- else
- {
- int32 maxlen = type_maximum_size(att->atttypid,
- att->atttypmod);
-
- if (maxlen < 0)
- maxlength_unknown = true;
- else
- data_length += maxlen;
- if (att->attstorage != 'p')
- has_toastable_attrs = true;
- }
- }
- if (!has_toastable_attrs)
- return false; /* nothing to toast? */
- if (maxlength_unknown)
- return true; /* any unlimited-length attrs? */
- tuple_length = MAXALIGN(SizeofHeapTupleHeader +
- BITMAPLEN(tupdesc->natts)) +
- MAXALIGN(data_length);
- return (tuple_length > TOAST_TUPLE_THRESHOLD);
+ /* Otherwise, let the AM decide. */
+ return table_relation_needs_toast_table(rel);
}
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 06eae2337a..53b58c51da 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -573,6 +573,16 @@ typedef struct TableAmRoutine
uint64 (*relation_size) (Relation rel, ForkNumber forkNumber);
+ /*
+ * This callback should return true if the relation requires a TOAST table
+ * and false if it does not. It may wish to examine the relation's
+ * tuple descriptor before making a decision, but if it uses some other
+ * method of storing large values (or if it does not support them) it can
+ * simply return false.
+ */
+ bool (*relation_needs_toast_table) (Relation rel);
+
+
/* ------------------------------------------------------------------------
* Planner related functions.
* ------------------------------------------------------------------------
@@ -1585,6 +1595,16 @@ table_relation_size(Relation rel, ForkNumber forkNumber)
return rel->rd_tableam->relation_size(rel, forkNumber);
}
+/*
+ * table_needs_toast_table - does this relation need a toast table?
+ */
+static inline bool
+table_relation_needs_toast_table(Relation rel)
+{
+ return rel->rd_tableam->relation_needs_toast_table(rel);
+}
+
+
/* ----------------------------------------------------------------------------
* Planner related functionality
* ----------------------------------------------------------------------------
--
2.17.2 (Apple Git-113)
Hi,
On 2019-05-20 10:29:29 -0400, Robert Haas wrote:
From 4e361bfe51810d7c637bf57968da2dfea4197701 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Fri, 17 May 2019 16:01:47 -0400
Subject: [PATCH v2] tableam: Move heap-specific logic from needs_toast_table
below tableam.
Assuming you didn't sneakily change the content of
heapam_relation_needs_toast_table from its previous behaviour, this
looks good to me ;)
Greetings,
Andres Freund
On Tue, May 21, 2019 at 11:53 AM Andres Freund <andres@anarazel.de> wrote:
Assuming you didn't sneakily change the content of
heapam_relation_needs_toast_table from its previous behaviour, this
looks good to me ;)
git diff --color-moved=zebra says I didn't.
Committed; thanks for the review.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company