Обсуждение: 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.
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.
Вложения
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
Вложения
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.
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:
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 thatblind-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
Вложения
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