Обсуждение: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

Hi!
This patch is part of a bigger patch I've offered before
https://www.postgresql.org/message-id/flat/2146419.veIEZdk4E4@x200m#2146419.veIEZdk4E4@x200m
as we agreed I am trying to commit it by smaller bits

This patch raises error if user tries o set or change toast.* option for a 
table that does not have a TOST relation.

I believe it is the only right thing to do, as now if you set toast reloption 
for table that does not  have TOAST table, the value of this option will be 
lost without any warning. You will not get it back with pg_dump, it will not 
be effective when you add varlen attributes to the table later.

So you offer DB some value to store, it accepts it without errors, and 
immediately loses it. I would consider it a bad behavior.

I also think that we should not change this error to warning, as toast.* 
options are usually used by very experienced users for precised DB tunning. I 
hardly expect them to do TOAST tuning for tables without TOAST relations. So 
chances to get problem with existing SQL code are minimal.

So I would suggest to throw an error in this case.

Possible flaws: I tied to write error messages according to guide lines. But I 
suppose it is still not prefect enough as I am not so good with English. May 
be somebody who knows the language well, can make it better.  

-- 
Do code for fun.

On Wed, Jan 17, 2018 at 3:50 PM, Nikolay Shaplov <dhyan@nataraj.su> wrote:
> This patch raises error if user tries o set or change toast.* option for a
> table that does not have a TOST relation.
>
> I believe it is the only right thing to do, as now if you set toast reloption
> for table that does not  have TOAST table, the value of this option will be
> lost without any warning. You will not get it back with pg_dump, it will not
> be effective when you add varlen attributes to the table later.
>
> So you offer DB some value to store, it accepts it without errors, and
> immediately loses it. I would consider it a bad behavior.
>
> I also think that we should not change this error to warning, as toast.*
> options are usually used by very experienced users for precised DB tunning. I
> hardly expect them to do TOAST tuning for tables without TOAST relations. So
> chances to get problem with existing SQL code are minimal.
>
> So I would suggest to throw an error in this case.

I think there is a problem with this idea, which is that the rules for
whether or not a given table has an associated TOAST table
occasionally change from one major release to the next.  Suppose that,
in release X, a particular table definition causes a TOAST table to be
created, but in release X+1, it does not.  If a table with that
definition has a toast.* option set, then upgrading from release X to
release X+1 via pg_dump and restore will fail.  That's bad.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Jan 17, 2018 at 3:50 PM, Nikolay Shaplov <dhyan@nataraj.su> wrote:
>> This patch raises error if user tries o set or change toast.* option for a
>> table that does not have a TOST relation.

> I think there is a problem with this idea, which is that the rules for
> whether or not a given table has an associated TOAST table
> occasionally change from one major release to the next.  Suppose that,
> in release X, a particular table definition causes a TOAST table to be
> created, but in release X+1, it does not.  If a table with that
> definition has a toast.* option set, then upgrading from release X to
> release X+1 via pg_dump and restore will fail.  That's bad.

Yeah --- and it doesn't even have to be a major version change; the
same version on different hardware might make different choices too.
Not to mention that there is discussion out there about making the
toasting rules more configurable.

There might be a case for raising a warning in this situation,
but I would disagree with making it a hard error in any case.
All that's going to accomplish is to break people's scripts and
get them mad at you.

            regards, tom lane

В письме от 18 января 2018 18:42:01 пользователь Tom Lane написал:
> >> This patch raises error if user tries o set or change toast.* option for
> >> a
> >> table that does not have a TOST relation.
> >
> > I think there is a problem with this idea, which is that the rules for
> > whether or not a given table has an associated TOAST table
> > occasionally change from one major release to the next.  Suppose that,
> > in release X, a particular table definition causes a TOAST table to be
> > created, but in release X+1, it does not.  If a table with that
> > definition has a toast.* option set, then upgrading from release X to
> > release X+1 via pg_dump and restore will fail.  That's bad.
>
> Yeah --- and it doesn't even have to be a major version change; the
> same version on different hardware might make different choices too.
> Not to mention that there is discussion out there about making the
> toasting rules more configurable.
>
> There might be a case for raising a warning in this situation,
> but I would disagree with making it a hard error in any case.
> All that's going to accomplish is to break people's scripts and
> get them mad at you.

These all sound very reasonable, but still does not solve problem with loosing
toast option values when you set one for table without TOAST relation.

May be, if reasons for existence TOAST relation is no so much fixed thing (I
thought it is almost as fixed as a rock), may be the solution would be to
create a TOAST relation anyway if toast options is set, and gave a warning
that may be setting this option is not the thing the user really want?
This way we will not loose option values. And it would be 100% backward
compatible.

The main misfeature here is that we will have empty TOAST relation in this
case. But first I do not think it will really harm anybody, and second, we
would WARN that it is not the best way to do things, so DB user will be able
to find way around.

What do you think about it?

--
Do code for fun.

On 1/18/18 18:42, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, Jan 17, 2018 at 3:50 PM, Nikolay Shaplov <dhyan@nataraj.su> wrote:
>>> This patch raises error if user tries o set or change toast.* option for a
>>> table that does not have a TOST relation.

> There might be a case for raising a warning in this situation,
> but I would disagree with making it a hard error in any case.
> All that's going to accomplish is to break people's scripts and
> get them mad at you.

It might also be useful to set these reloptions before you add a new
column to a table.

So I think this change is not desirable.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jan 23, 2018 at 12:03 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> It might also be useful to set these reloptions before you add a new
> column to a table.

I don't understand.  AAUI, it is currently the case that if you set
the options before the TOAST table exists, they are lost.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

В письме от 23 января 2018 12:03:50 пользователь Peter Eisentraut написал:

> >>> This patch raises error if user tries o set or change toast.* option for
> >>> a table that does not have a TOST relation.
> >
> > There might be a case for raising a warning in this situation,
> > but I would disagree with making it a hard error in any case.
> > All that's going to accomplish is to break people's scripts and
> > get them mad at you.
>
> It might also be useful to set these reloptions before you add a new
> column to a table.
As Robert have just said, this will not work with current master.

If we, as I've suggested in previous letter, will force TOAST creation if
toast.* option is set, then your use case will also work.

--
Do code for fun.

On 1/23/18 13:39, Robert Haas wrote:
> On Tue, Jan 23, 2018 at 12:03 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> It might also be useful to set these reloptions before you add a new
>> column to a table.
> 
> I don't understand.  AAUI, it is currently the case that if you set
> the options before the TOAST table exists, they are lost.

Well, that's also weird.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 1/23/18 13:39, Robert Haas wrote:
>> I don't understand.  AAUI, it is currently the case that if you set
>> the options before the TOAST table exists, they are lost.

> Well, that's also weird.

Well, maybe the right answer is to address that.  It's clear to me
why that would happen if we store these things as reloptions on the
toast table, but can't they be stored on the parent table?

Backward compatibility might be an issue, but I doubt that there
are enough people using these options that we couldn't get away
with breaking things, if we're unable to find a decent automatic
conversion method.

            regards, tom lane

Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > On 1/23/18 13:39, Robert Haas wrote:
> >> I don't understand.  AAUI, it is currently the case that if you set
> >> the options before the TOAST table exists, they are lost.
> 
> > Well, that's also weird.
> 
> Well, maybe the right answer is to address that.  It's clear to me
> why that would happen if we store these things as reloptions on the
> toast table, but can't they be stored on the parent table?

Actually, Nikolay provided a possible solution: if you execute ALTER
TABLE SET (toast.foobar = xyz), and a toast table doesn't exist, create
one at that point.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Tom Lane wrote:
>> Well, maybe the right answer is to address that.  It's clear to me
>> why that would happen if we store these things as reloptions on the
>> toast table, but can't they be stored on the parent table?

> Actually, Nikolay provided a possible solution: if you execute ALTER
> TABLE SET (toast.foobar = xyz), and a toast table doesn't exist, create
> one at that point.

That adds a lot of overhead if you never actually need the toast table.

Still, maybe it's an appropriate amount of effort compared to the size
of the use-case for this.

            regards, tom lane

В письме от 25 января 2018 11:29:34 пользователь Tom Lane написал:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > Tom Lane wrote:
> >> Well, maybe the right answer is to address that.  It's clear to me
> >> why that would happen if we store these things as reloptions on the
> >> toast table, but can't they be stored on the parent table?
> >
> > Actually, Nikolay provided a possible solution: if you execute ALTER
> > TABLE SET (toast.foobar = xyz), and a toast table doesn't exist, create
> > one at that point.
>
> That adds a lot of overhead if you never actually need the toast table.
I think this overhead case is not that terrible if it is properly warned ;-)

> Still, maybe it's an appropriate amount of effort compared to the size
> of the use-case for this.
If you came to some final conclustion, please close the commiffest item with
"Return with feedback" resolution, and I write another patch...

--
Do code for fun.

On 1/25/18 09:40, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> On 1/23/18 13:39, Robert Haas wrote:
>>> I don't understand.  AAUI, it is currently the case that if you set
>>> the options before the TOAST table exists, they are lost.
> 
>> Well, that's also weird.
> 
> Well, maybe the right answer is to address that.  It's clear to me
> why that would happen if we store these things as reloptions on the
> toast table, but can't they be stored on the parent table?

I don't think there is a case where this can currently be a problem with
the current options set, but here is what I was thinking about:  Imagine
there were a setting toast.fillfactor or something like that that
affects the storage layout of the TOAST table.  If you ran

ALTER TABLE mytbl ADD COLUMN foo text DEFAULT somelongvalue();

then you would conceivably want to set TOAST-related reloptions before
the TOAST table is created and have them apply as the TOAST table is
being first populated.

Again, currently not a problem, I think.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 1/25/18 12:27 PM, Nikolay Shaplov wrote:
> В письме от 25 января 2018 11:29:34 пользователь Tom Lane написал:
>> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>>> Tom Lane wrote:
>>>> Well, maybe the right answer is to address that.  It's clear to me
>>>> why that would happen if we store these things as reloptions on the
>>>> toast table, but can't they be stored on the parent table?
>>>
>>> Actually, Nikolay provided a possible solution: if you execute ALTER
>>> TABLE SET (toast.foobar = xyz), and a toast table doesn't exist, create
>>> one at that point.
>>
>> That adds a lot of overhead if you never actually need the toast table.
> I think this overhead case is not that terrible if it is properly warned ;-)
> 
>> Still, maybe it's an appropriate amount of effort compared to the size
>> of the use-case for this.
> If you came to some final conclustion, please close the commiffest item with 
> "Return with feedback" resolution, and I write another patch... 

I think this patch should be marked Returned with Feedback since it
appears there is no consensus on whether it is useful or correct, so I
have done that.

If I got it wrong I'm happy to move it to the next CF in Waiting for
Author state instead.

Thanks,
-- 
-David
david@pgmasters.net

Nikolay Shaplov wrote:
> В письме от 25 января 2018 11:29:34 пользователь Tom Lane написал:
> > Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

> > > Actually, Nikolay provided a possible solution: if you execute ALTER
> > > TABLE SET (toast.foobar = xyz), and a toast table doesn't exist, create
> > > one at that point.
> > 
> > That adds a lot of overhead if you never actually need the toast table.
>
> I think this overhead case is not that terrible if it is properly warned ;-)

Let's go with this idea, which seems to me better than the statu quo.
There are a couple of other proposals, but they seem to require a lot of
work in exchange for unclear benefits.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

В письме от 10 апреля 2018 08:55:52 пользователь David Steele написал:
> On 1/25/18 12:27 PM, Nikolay Shaplov wrote:
> > В письме от 25 января 2018 11:29:34 пользователь Tom Lane написал:
> >> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> >>> Tom Lane wrote:
> >>>> Well, maybe the right answer is to address that.  It's clear to me
> >>>> why that would happen if we store these things as reloptions on the
> >>>> toast table, but can't they be stored on the parent table?
> >>>
> >>> Actually, Nikolay provided a possible solution: if you execute ALTER
> >>> TABLE SET (toast.foobar = xyz), and a toast table doesn't exist, create
> >>> one at that point.
> >>
> >> That adds a lot of overhead if you never actually need the toast table.
> >
> > I think this overhead case is not that terrible if it is properly warned
> > ;-)>
> >> Still, maybe it's an appropriate amount of effort compared to the size
> >> of the use-case for this.
> >
> > If you came to some final conclustion, please close the commiffest item
> > with "Return with feedback" resolution, and I write another patch...
>
> I think this patch should be marked Returned with Feedback since it
> appears there is no consensus on whether it is useful or correct, so I
> have done that.
Exactly!

But I'd like to know what kind of feedback is it :-)
What conclusion have been reached (I did not got it)

Otherwise I would not know how to rewrite this patch.

I would suggest: create a TOAST relation whenever toast.* options is set, but
give a warning it this relation will not be used for data storage (i.e. there
is no toastable columns in the table)

But I need some confirmation, in order not to write patch in vain again :-)

>
> If I got it wrong I'm happy to move it to the next CF in Waiting for
> Author state instead.
>
> Thanks,

--
Do code for fun.

Nikolay Shaplov wrote:

> But I need some confirmation, in order not to write patch in vain again :-)

Don't worry, rest assured that you will still write *many* patches in
vain, not just this one.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 4/10/18 9:17 AM, Alvaro Herrera wrote:
> Nikolay Shaplov wrote:
> 
>> But I need some confirmation, in order not to write patch in vain again :-)
> 
> Don't worry, rest assured that you will still write *many* patches in
> vain, not just this one.

Despite the rather dubious pep talk, Álvaro is right.  You will get more
response with a new patch and a new thread.

But everyone is pretty fatigued right now, so I would recommend waiting
a while.  If you would like to pursue this, plan to have a new patch
ready in August for the next CF.  It sounds like you have an idea about
what needs to be done.

Regards,
-- 
-David
david@pgmasters.net