Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-04-10 Thread David Steele
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
da...@pgmasters.net



Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-04-10 Thread Nikolay Shaplov
В письме от 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  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.

signature.asc
Description: This is a digitally signed message part.


Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-04-10 Thread David Steele
On 1/25/18 12:27 PM, Nikolay Shaplov wrote:
> В письме от 25 января 2018 11:29:34 пользователь Tom Lane написал:
>> Alvaro Herrera  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
da...@pgmasters.net



Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-01-25 Thread Peter Eisentraut
On 1/25/18 09:40, Tom Lane wrote:
> Peter Eisentraut  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



Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-01-25 Thread Nikolay Shaplov
В письме от 25 января 2018 11:29:34 пользователь Tom Lane написал:
> Alvaro Herrera  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.

signature.asc
Description: This is a digitally signed message part.


Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-01-25 Thread Tom Lane
Alvaro Herrera  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



Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-01-25 Thread Alvaro Herrera
Tom Lane wrote:
> Peter Eisentraut  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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-01-25 Thread Tom Lane
Peter Eisentraut  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



Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-01-25 Thread Peter Eisentraut
On 1/23/18 13:39, Robert Haas wrote:
> On Tue, Jan 23, 2018 at 12:03 PM, Peter Eisentraut
>  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



Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-01-23 Thread Nikolay Shaplov
В письме от 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.

signature.asc
Description: This is a digitally signed message part.


Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-01-23 Thread Robert Haas
On Tue, Jan 23, 2018 at 12:03 PM, Peter Eisentraut
 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



Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-01-23 Thread Peter Eisentraut
On 1/18/18 18:42, Tom Lane wrote:
> Robert Haas  writes:
>> On Wed, Jan 17, 2018 at 3:50 PM, Nikolay Shaplov  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



Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-01-19 Thread Nikolay Shaplov
В письме от 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.

signature.asc
Description: This is a digitally signed message part.


Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-01-18 Thread Tom Lane
Robert Haas  writes:
> On Wed, Jan 17, 2018 at 3:50 PM, Nikolay Shaplov  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



Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-01-18 Thread Robert Haas
On Wed, Jan 17, 2018 at 3:50 PM, Nikolay Shaplov  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



[PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist

2018-01-17 Thread Nikolay Shaplov

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.diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 0b4b5631..2551023 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -36,7 +36,7 @@
 /* Potentially set by pg_upgrade_support functions */
 Oid			binary_upgrade_next_toast_pg_type_oid = InvalidOid;

-static void CheckAndCreateToastTable(Oid relOid, Datum reloptions,
+static bool CheckAndCreateToastTable(Oid relOid, Datum reloptions,
 		 LOCKMODE lockmode, bool check);
 static bool create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
    Datum reloptions, LOCKMODE lockmode, bool check);
@@ -67,23 +67,26 @@ NewHeapCreateToastTable(Oid relOid, Datum reloptions, LOCKMODE lockmode)
 	CheckAndCreateToastTable(relOid, reloptions, lockmode, false);
 }

-void
+bool
 NewRelationCreateToastTable(Oid relOid, Datum reloptions)
 {
-	CheckAndCreateToastTable(relOid, reloptions, AccessExclusiveLock, false);
+	return CheckAndCreateToastTable(relOid, reloptions,
+	AccessExclusiveLock, false);
 }

-static void
+static bool
 CheckAndCreateToastTable(Oid relOid, Datum reloptions, LOCKMODE lockmode, bool check)
 {
 	Relation	rel;
+	bool		success;

 	rel = heap_open(relOid, lockmode);

 	/* create_toast_table does all the work */
-	(void) create_toast_table(rel, InvalidOid, InvalidOid, reloptions, lockmode, check);
+	success = create_toast_table(rel, InvalidOid, InvalidOid, reloptions, lockmode, check);

 	heap_close(rel, NoLock);
+	return success;
 }

 /*
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 3d82edb..7745aa5 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -89,6 +89,7 @@ create_ctas_internal(List *attrList, IntoClause *into)
 	Datum		toast_options;
 	static char *validnsps[] = HEAP_RELOPT_NAMESPACES;
 	ObjectAddress intoRelationAddr;
+	bool		toast_created;

 	/* This code supports both CREATE TABLE AS and CREATE MATERIALIZED VIEW */
 	is_matview = (into->viewQuery != NULL);
@@ -130,7 +131,15 @@ create_ctas_internal(List *attrList, IntoClause *into)

 	(void) heap_reloptions(RELKIND_TOASTVALUE, toast_options, true);

-	NewRelationCreateToastTable(intoRelationAddr.objectId, toast_options);
+	toast_created = NewRelationCreateToastTable(intoRelationAddr.objectId,
+toast_options);
+	if (!toast_created && toast_options)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("can't set TOAST relation options for a new table"),
+ errdetail("TOAST relation were not created"),
+ errhint("do not specify toast.* options, or add some variable length attributes to the table")
+ ));

 	/* Create the "view" part of a materialized view. */
 	if (is_matview)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f2a928b..a169e14 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10557,6 +10557,19 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,

 		heap_close(toastrel, NoLock);
 	}
+	else
+	{
+		newOptions = transformRelOptions((Datum) 0, defList, "toast",
+		 validnsps, false, false);
+		if (newOptions)
+			ereport(ERROR,
+	(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("can't alter TOAST relation options for \"%s\" table",
+	   RelationGetRelationName(rel)),
+	 errdetail("TOAST relation does not exist"),
+	 errhint("do not specify toast.* options, or add some variable length attributes to the table")
+	 ));
+	}

 	heap_close(pgclass, RowExclusiveLock);
 }
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index