Horton

Thread

Commits

  1. tableam: Move heap-specific logic from needs_toast_table below tableam.

  1. Create TOAST table only if AM needs

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-05-17T18:26:29Z

    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.

  2. Re: Create TOAST table only if AM needs

    Andres Freund <andres@anarazel.de> — 2019-05-17T18:34:21Z

    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

  3. Re: Create TOAST table only if AM needs

    Robert Haas <robertmhaas@gmail.com> — 2019-05-17T19:13:50Z

    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

  4. Re: Create TOAST table only if AM needs

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-17T19:26:38Z

    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

  5. Re: Create TOAST table only if AM needs

    Andres Freund <andres@anarazel.de> — 2019-05-17T19:28:12Z

    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

  6. Re: Create TOAST table only if AM needs

    Andres Freund <andres@anarazel.de> — 2019-05-17T19:31:38Z

    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

  7. Re: Create TOAST table only if AM needs

    Robert Haas <robertmhaas@gmail.com> — 2019-05-17T20:47:34Z

    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

  8. Re: Create TOAST table only if AM needs

    Andres Freund <andres@anarazel.de> — 2019-05-17T20:51:54Z

    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

  9. Re: Create TOAST table only if AM needs

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-05-17T21:12:12Z

    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.

  10. Re: Create TOAST table only if AM needs

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-05-17T21:20:44Z

    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.

  11. Re: Create TOAST table only if AM needs

    Robert Haas <robertmhaas@gmail.com> — 2019-05-17T21:41:48Z

    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

  12. Re: Create TOAST table only if AM needs

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-05-17T21:54:50Z

    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.

  13. Re: Create TOAST table only if AM needs

    Greg Stark <stark@mit.edu> — 2019-05-19T14:03:23Z

    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.
>

  14. Re: Create TOAST table only if AM needs

    Robert Haas <robertmhaas@gmail.com> — 2019-05-20T14:29:29Z

    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

  15. Re: Create TOAST table only if AM needs

    Andres Freund <andres@anarazel.de> — 2019-05-21T15:53:50Z

    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

  16. Re: Create TOAST table only if AM needs

    Robert Haas <robertmhaas@gmail.com> — 2019-05-21T16:04:22Z

    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