Horton

Thread

Commits

  1. Allow ALTER TYPE to change some properties of a base type.

  1. Allowing ALTER TYPE to change storage strategy

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-02-28T00:44:40Z

    Hi,

 From time to time I run into the limitation that ALTER TYPE does not
allow changing storage strategy. I've written a bunch of data types over
the years - in some cases I simply forgot to specify storage in CREATE
TYPE (so it defaulted to PLAIN) or I expected PLAIN to be sufficient and
then later wished I could enable TOAST.

Obviously, without ALTER TYPE supporting that it's rather tricky. You
either need to do dump/restore, or tweak the pg_type catalog directly.
So here is an initial patch extending ALTER TYPE to support this.

The question is why this was not implemented before - my assumption is
this is not simply because no one wanted that. We track the storage in
pg_attribute too, and ALTER TABLE allows changing that ...

My understanding is that pg_type.typstorage essentially specifies two
things: (a) default storage strategy for the attributes with this type,
and (b) whether the type implementation is prepared to handle TOAST-ed
values or not. And pg_attribute.attstorage has to respect this - when
the type says PLAIN then the attribute can't simply use strategy that
would enable TOAST.

Luckily, this is only a problem when switching typstorage to PLAIN (i.e.
when disabling TOAST for the type). The attached v1 patch checks if
there are attributes with non-PLAIN storage for this type, and errors
out if it finds one. But unfortunately that's not entirely correct,
because ALTER TABLE only sets storage for new data. A table may be
created with e.g. EXTENDED storage for an attribute, a bunch of rows may
be loaded and then the storage for the attribute may be changed to
PLAIN. That would pass the check as it's currently in the patch, yet
there may be TOAST-ed values for the type with PLAIN storage :-(

I'm not entirely sure what to do about this, but I think it's OK to just
reject changes in this direction (from non-PLAIN to PLAIN storage). I've
never needed it, and it seems pretty useless - it seems fine to just
instruct the user to do a dump/restore.

In the opposite direction - when switching from PLAIN to a TOAST-enabled
storage, or enabling/disabling compression, this is not an issue at all.
It's legal for type to specify e.g. EXTENDED but attribute to use PLAIN,
for example. So the attached v1 patch simply allows this direction.


regards

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

  2. Re: Allowing ALTER TYPE to change storage strategy

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-28T18:59:49Z

    Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> My understanding is that pg_type.typstorage essentially specifies two
> things: (a) default storage strategy for the attributes with this type,
> and (b) whether the type implementation is prepared to handle TOAST-ed
> values or not. And pg_attribute.attstorage has to respect this - when
> the type says PLAIN then the attribute can't simply use strategy that
> would enable TOAST.

Check.

> Luckily, this is only a problem when switching typstorage to PLAIN (i.e.
> when disabling TOAST for the type). The attached v1 patch checks if
> there are attributes with non-PLAIN storage for this type, and errors
> out if it finds one. But unfortunately that's not entirely correct,
> because ALTER TABLE only sets storage for new data. A table may be
> created with e.g. EXTENDED storage for an attribute, a bunch of rows may
> be loaded and then the storage for the attribute may be changed to
> PLAIN. That would pass the check as it's currently in the patch, yet
> there may be TOAST-ed values for the type with PLAIN storage :-(

> I'm not entirely sure what to do about this, but I think it's OK to just
> reject changes in this direction (from non-PLAIN to PLAIN storage).

Yeah, I think you should just reject that: once toast-capable, always
toast-capable.  Scanning pg_attribute is entirely insufficient because
of race conditions --- and while we accept such races in some other
places, this seems like a place where the risk is too high and the
value too low.

Anybody who really needs to go in that direction still has the alternative
of manually frobbing the catalogs (and taking the responsibility for
races and un-toasting whatever's stored already).

			regards, tom lane

  3. Re: Allowing ALTER TYPE to change storage strategy

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-02-29T01:25:11Z

    On Fri, Feb 28, 2020 at 01:59:49PM -0500, Tom Lane wrote:
>Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> My understanding is that pg_type.typstorage essentially specifies two
>> things: (a) default storage strategy for the attributes with this type,
>> and (b) whether the type implementation is prepared to handle TOAST-ed
>> values or not. And pg_attribute.attstorage has to respect this - when
>> the type says PLAIN then the attribute can't simply use strategy that
>> would enable TOAST.
>
>Check.
>
>> Luckily, this is only a problem when switching typstorage to PLAIN (i.e.
>> when disabling TOAST for the type). The attached v1 patch checks if
>> there are attributes with non-PLAIN storage for this type, and errors
>> out if it finds one. But unfortunately that's not entirely correct,
>> because ALTER TABLE only sets storage for new data. A table may be
>> created with e.g. EXTENDED storage for an attribute, a bunch of rows may
>> be loaded and then the storage for the attribute may be changed to
>> PLAIN. That would pass the check as it's currently in the patch, yet
>> there may be TOAST-ed values for the type with PLAIN storage :-(
>
>> I'm not entirely sure what to do about this, but I think it's OK to just
>> reject changes in this direction (from non-PLAIN to PLAIN storage).
>
>Yeah, I think you should just reject that: once toast-capable, always
>toast-capable.  Scanning pg_attribute is entirely insufficient because
>of race conditions --- and while we accept such races in some other
>places, this seems like a place where the risk is too high and the
>value too low.
>
>Anybody who really needs to go in that direction still has the alternative
>of manually frobbing the catalogs (and taking the responsibility for
>races and un-toasting whatever's stored already).
>

Yeah. Attached is v2 of the patch, simply rejecting such changes.

I think we might check if there are any attributes with the given data
type, and allow the change if there are none. That would still allow the
change when the type is used only for things like function parameters
etc. But we'd also have to check for domains (recursively).

One thing I haven't mentioned in the original message is CASCADE. It
seems useful to optionally change storage for all attributes with the
given data type. But I'm not sure it's actually a good idea, and the
amount of code seems non-trivial (it'd have to copy quite a bit of code
from ALTER TABLE). I'm also not sure what to do about domains and
attributes using those. It's more time/code than what I'm willing spend
now, so I'll laeve this as a possible future improvement.

regards

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

  4. Re: Allowing ALTER TYPE to change storage strategy

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-29T01:35:33Z

    Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> I think we might check if there are any attributes with the given data
> type, and allow the change if there are none. That would still allow the
> change when the type is used only for things like function parameters
> etc. But we'd also have to check for domains (recursively).

Still has race conditions.

> One thing I haven't mentioned in the original message is CASCADE. It
> seems useful to optionally change storage for all attributes with the
> given data type. But I'm not sure it's actually a good idea, and the
> amount of code seems non-trivial (it'd have to copy quite a bit of code
> from ALTER TABLE).

You'd need a moderately strong lock on each such table, which means
there'd be serious deadlock hazards.  I'm dubious that it's worth
troubling with.

			regards, tom lane

  5. Re: Allowing ALTER TYPE to change storage strategy

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-02-29T21:37:14Z

    On Fri, Feb 28, 2020 at 08:35:33PM -0500, Tom Lane wrote:
>Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> I think we might check if there are any attributes with the given data
>> type, and allow the change if there are none. That would still allow the
>> change when the type is used only for things like function parameters
>> etc. But we'd also have to check for domains (recursively).
>
>Still has race conditions.
>

Yeah, I have no problem believing that.

>> One thing I haven't mentioned in the original message is CASCADE. It
>> seems useful to optionally change storage for all attributes with the
>> given data type. But I'm not sure it's actually a good idea, and the
>> amount of code seems non-trivial (it'd have to copy quite a bit of code
>> from ALTER TABLE).
>
>You'd need a moderately strong lock on each such table, which means
>there'd be serious deadlock hazards.  I'm dubious that it's worth
>troubling with.
>

Yeah, I don't plan to do this in v1 (and I have no immediate plan to
work on it after that). But I wonder how is the deadlock risk any
different compared e.g. to DROP TYPE ... CASCADE?

regards

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

  6. Re: Allowing ALTER TYPE to change storage strategy

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-29T22:13:19Z

    Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> On Fri, Feb 28, 2020 at 08:35:33PM -0500, Tom Lane wrote:
>> You'd need a moderately strong lock on each such table, which means
>> there'd be serious deadlock hazards.  I'm dubious that it's worth
>> troubling with.

> Yeah, I don't plan to do this in v1 (and I have no immediate plan to
> work on it after that). But I wonder how is the deadlock risk any
> different compared e.g. to DROP TYPE ... CASCADE?

True, but dropping a type you're actively using seems pretty improbable;
whereas the whole point of the patch you're proposing is that people
*would* want to use it in production.

			regards, tom lane

  7. Re: Allowing ALTER TYPE to change storage strategy

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-02T19:11:10Z

    I started to look at this patch with fresh eyes after reading the patch
for adding binary I/O for ltree,

https://www.postgresql.org/message-id/flat/CANmj9Vxx50jOo1L7iSRxd142NyTz6Bdcgg7u9P3Z8o0=HGkYyQ@mail.gmail.com

and realizing that the only reasonable way to tackle that problem is to
provide an ALTER TYPE command that can set the binary I/O functions for
an existing type.  (One might think that it'd be acceptable to UPDATE
the pg_type row directly; but that wouldn't take care of dependencies
properly, and it also wouldn't handle the domain issues I discuss below.)
There are other properties too that can be set in CREATE TYPE but we
have no convenient way to adjust later, though it'd be reasonable to
want to do so.

I do not think that we want to invent bespoke syntax for each property.
The more such stuff we cram into ALTER TYPE, the bigger the risk of
conflicting with some future SQL extension.  Moreover, since CREATE TYPE
for base types already uses the "( keyword = value, ... )" syntax for
these properties, and we have a similar precedent in CREATE/ALTER
OPERATOR, it seems to me that the syntax we want here is

	ALTER TYPE typename SET ( keyword = value [, ... ] )

Attached is a stab at doing it that way, and implementing setting of
the binary I/O functions for good measure.  (It'd be reasonable to
add more stuff, like setting the other support functions, but this
is enough for the immediate discussion.)

The main thing I'm not too happy about is what to do about domains.
Your v2 patch supposed that it's okay to allow ALTER TYPE on domains,
but I'm not sure we want to go that way, and if we do there's certainly
a bunch more work that has to be done.  Up to now the system has
supposed that domains inherit all these properties from their base
types.  I'm not certain exactly how far that assumption has propagated,
but there's at least one place that implicitly assumes it: pg_dump has
no logic for adjusting a domain to have different storage or support
functions than the base type had.  So as v2 stands, a custom storage
option on a domain would be lost in dump/reload.

Another issue that would become a big problem if we allow domains to
have custom I/O functions is that the wire protocol transmits the
base type's OID, not the domain's OID, for an output column that
is of a domain type.  A client that expected a particular output
format on the strength of what it was told the column type was
would be in for a big surprise.

Certainly we could fix pg_dump if we had a mind to, but changing
the wire protocol for this would have unpleasant ramifications.
And I'm worried about whether there are other places in the system
that are also making this sort of assumption.

I'm also not very convinced that we *want* to allow domains to vary from
their base types in this way.  The primary use-case I can think of for
ALTER TYPE SET is in extension update scripts, and an extension would
almost surely wish for any domains over its type to automatically absorb
whatever changes of this sort it wants to make.

So I think there are two distinct paths we could take here:

* Decide that it's okay to allow domains to vary from their base type
in these properties.  Teach pg_dump to cope with that, and stand ready
to fix any other bugs we find, and have some story to tell the people
whose clients we break.  Possibly add a CASCADE option to
ALTER TYPE SET, with the semantics of adjusting dependent domains
to match.  (This is slightly less scary than the CASCADE semantics
you had in mind, because it would only affect pg_type entries not
tables.)

* Decide that we'll continue to require domains to match their base
type in all these properties.  That means refusing to allow ALTER
on a domain per se, and automatically cascading these changes to
dependent domains.

In the v3 patch below, I've ripped out the ALTER DOMAIN syntax on
the assumption that we'd do the latter; but I've not written the
cascade recursion logic, because that seemed like a lot of work
to do in advance of having consensus on it being a good idea.

I've also restricted the code to work just on base types, because
it's far from apparent to me that it makes any sense to allow any
of these operations on derived types such as composites or ranges.
Again, there's a fair amount of code that is not going to be
prepared for such a type to have properties that it could not
have at creation, and I don't see a use-case that really justifies
breaking those expectations.

Thoughts?

			regards, tom lane

  8. Re: Allowing ALTER TYPE to change storage strategy

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-03-02T22:05:31Z

    On Mon, Mar 02, 2020 at 02:11:10PM -0500, Tom Lane wrote:
>I started to look at this patch with fresh eyes after reading the patch
>for adding binary I/O for ltree,
>
>https://www.postgresql.org/message-id/flat/CANmj9Vxx50jOo1L7iSRxd142NyTz6Bdcgg7u9P3Z8o0=HGkYyQ@mail.gmail.com
>
>and realizing that the only reasonable way to tackle that problem is to
>provide an ALTER TYPE command that can set the binary I/O functions for
>an existing type.  (One might think that it'd be acceptable to UPDATE
>the pg_type row directly; but that wouldn't take care of dependencies
>properly, and it also wouldn't handle the domain issues I discuss below.)
>There are other properties too that can be set in CREATE TYPE but we
>have no convenient way to adjust later, though it'd be reasonable to
>want to do so.
>
>I do not think that we want to invent bespoke syntax for each property.
>The more such stuff we cram into ALTER TYPE, the bigger the risk of
>conflicting with some future SQL extension.  Moreover, since CREATE TYPE
>for base types already uses the "( keyword = value, ... )" syntax for
>these properties, and we have a similar precedent in CREATE/ALTER
>OPERATOR, it seems to me that the syntax we want here is
>
>	ALTER TYPE typename SET ( keyword = value [, ... ] )
>

Agreed, it seems reasonable to use the ALTER OPRATOR precedent.

>Attached is a stab at doing it that way, and implementing setting of
>the binary I/O functions for good measure.  (It'd be reasonable to
>add more stuff, like setting the other support functions, but this
>is enough for the immediate discussion.)
>
>The main thing I'm not too happy about is what to do about domains.
>Your v2 patch supposed that it's okay to allow ALTER TYPE on domains,
>but I'm not sure we want to go that way, and if we do there's certainly
>a bunch more work that has to be done.  Up to now the system has
>supposed that domains inherit all these properties from their base
>types.  I'm not certain exactly how far that assumption has propagated,
>but there's at least one place that implicitly assumes it: pg_dump has
>no logic for adjusting a domain to have different storage or support
>functions than the base type had.  So as v2 stands, a custom storage
>option on a domain would be lost in dump/reload.
>
>Another issue that would become a big problem if we allow domains to
>have custom I/O functions is that the wire protocol transmits the
>base type's OID, not the domain's OID, for an output column that
>is of a domain type.  A client that expected a particular output
>format on the strength of what it was told the column type was
>would be in for a big surprise.
>
>Certainly we could fix pg_dump if we had a mind to, but changing
>the wire protocol for this would have unpleasant ramifications.
>And I'm worried about whether there are other places in the system
>that are also making this sort of assumption.
>
>I'm also not very convinced that we *want* to allow domains to vary from
>their base types in this way.  The primary use-case I can think of for
>ALTER TYPE SET is in extension update scripts, and an extension would
>almost surely wish for any domains over its type to automatically absorb
>whatever changes of this sort it wants to make.
>
>So I think there are two distinct paths we could take here:
>
>* Decide that it's okay to allow domains to vary from their base type
>in these properties.  Teach pg_dump to cope with that, and stand ready
>to fix any other bugs we find, and have some story to tell the people
>whose clients we break.  Possibly add a CASCADE option to
>ALTER TYPE SET, with the semantics of adjusting dependent domains
>to match.  (This is slightly less scary than the CASCADE semantics
>you had in mind, because it would only affect pg_type entries not
>tables.)
>
>* Decide that we'll continue to require domains to match their base
>type in all these properties.  That means refusing to allow ALTER
>on a domain per se, and automatically cascading these changes to
>dependent domains.
>
>In the v3 patch below, I've ripped out the ALTER DOMAIN syntax on
>the assumption that we'd do the latter; but I've not written the
>cascade recursion logic, because that seemed like a lot of work
>to do in advance of having consensus on it being a good idea.
>

I do agree we should do the latter, i.e. maintain the assumption that
domains have the same properties as their base type. I can't think of a
use case for allowing them to differ, it just didn't occur to me there
is this implicit assumption when writing the patch.

>I've also restricted the code to work just on base types, because
>it's far from apparent to me that it makes any sense to allow any
>of these operations on derived types such as composites or ranges.
>Again, there's a fair amount of code that is not going to be
>prepared for such a type to have properties that it could not
>have at creation, and I don't see a use-case that really justifies
>breaking those expectations.
>

Yeah, that makes sense too, I think.


regards

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

  9. Re: Allowing ALTER TYPE to change storage strategy

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-04T18:39:58Z

    Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> On Mon, Mar 02, 2020 at 02:11:10PM -0500, Tom Lane wrote:
>> In the v3 patch below, I've ripped out the ALTER DOMAIN syntax on
>> the assumption that we'd do the latter; but I've not written the
>> cascade recursion logic, because that seemed like a lot of work
>> to do in advance of having consensus on it being a good idea.

> I do agree we should do the latter, i.e. maintain the assumption that
> domains have the same properties as their base type. I can't think of a
> use case for allowing them to differ, it just didn't occur to me there
> is this implicit assumption when writing the patch.

Here's a v4 that is rebased over HEAD + the OPAQUE-ectomy that I
proposed at <4110.1583255415@sss.pgh.pa.us>, plus it adds recursion
to domains, and I also added the ability to set typmod I/O and
analyze functions, which seems like functionality that somebody
could possibly wish to add to a type after-the-fact much like
binary I/O.

I thought about allowing the basic I/O functions to be replaced as
well, but I couldn't really convince myself that there's a use-case
for that.  In practice you'd probably always just change the
behavior of the existing I/O functions, not want to sub in new ones.

(I kind of wonder, actually, whether there's a use-case for the
NONE options here at all.  When would you remove a support function?)

Of the remaining CREATE TYPE options, "category" and "preferred"
could perhaps be changeable but I couldn't get excited about them.
All the others seem like there are gotchas --- for example,
changing a type's collatable property is much harder than it
looks because it'd affect stored views.  So this seems like a
reasonable stopping point.

I think this is committable --- how about you?

I've included the OPAQUE-ectomy patches below so that the cfbot
can test this, but they're the same as in the other thread.

			regards, tom lane

  10. Re: Allowing ALTER TYPE to change storage strategy

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-04T23:15:28Z

    I wrote:
> I think this is committable --- how about you?

... or not.  I just noticed that the typcache tracks each type's
typstorage setting, and there's no provision for flushing/reloading
that.

As far as I can find, there is only one place where the cached
value is used, and that's in rangetypes.c which needs to know
whether the range element type is toastable.  (It doesn't actually
need to know the exact value of typstorage, only whether it is or
isn't PLAIN.)

We have a number of possible fixes for that:

1. Upgrade typcache.c to support flushing and rebuilding this data.
That seems fairly expensive; while we may be forced into that someday,
I'm hesitant to do it for a fairly marginal feature like this one.

2. Stop using the typcache for this particular purpose in rangetypes.c.
That seems rather undesirable from a performance standpoint, too.

3. Drop the ability for ALTER TYPE to promote from PLAIN to not-PLAIN
typstorage, and adjust the typcache so that it only remembers boolean
toastability not the specific toasting strategy.  Then the cache is
still immutable so no need for update logic.

I'm kind of liking #3, ugly as it sounds, because I'm not sure how
much of a use-case there is for the upgrade-from-PLAIN case.
Particularly now that TOAST is so ingrained in the system, it seems
rather unlikely that a production-grade data type wouldn't have
been designed to be toastable from the beginning, if there could be
any advantage to that.  Either #1 or #2 seem like mighty high prices
to pay for offering an option that might have no real-world uses.

			regards, tom lane

  11. Re: Allowing ALTER TYPE to change storage strategy

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-04T23:56:42Z

    I wrote:
> 3. Drop the ability for ALTER TYPE to promote from PLAIN to not-PLAIN
> typstorage, and adjust the typcache so that it only remembers boolean
> toastability not the specific toasting strategy.  Then the cache is
> still immutable so no need for update logic.
>
> I'm kind of liking #3, ugly as it sounds, because I'm not sure how
> much of a use-case there is for the upgrade-from-PLAIN case.
> Particularly now that TOAST is so ingrained in the system, it seems
> rather unlikely that a production-grade data type wouldn't have
> been designed to be toastable from the beginning, if there could be
> any advantage to that.  Either #1 or #2 seem like mighty high prices
> to pay for offering an option that might have no real-world uses.

Here's a v5 based on that approach.  I also added some comments about
the potential race conditions involved in recursing to domains.

			regards, tom lane

  12. Re: Allowing ALTER TYPE to change storage strategy

    David G. Johnston <david.g.johnston@gmail.com> — 2020-03-05T00:00:58Z

    On Wed, Mar 4, 2020 at 4:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> > I think this is committable --- how about you?
>
> ... or not.  I just noticed that the typcache tracks each type's
> typstorage setting, and there's no provision for flushing/reloading
> that.
>
> As far as I can find, there is only one place where the cached
> value is used, and that's in rangetypes.c which needs to know
> whether the range element type is toastable.  (It doesn't actually
> need to know the exact value of typstorage, only whether it is or
> isn't PLAIN.)
>
> [...]



>
> 3. Drop the ability for ALTER TYPE to promote from PLAIN to not-PLAIN
> typstorage, and adjust the typcache so that it only remembers boolean
> toastability not the specific toasting strategy.  Then the cache is
> still immutable so no need for update logic.
>
> I'm kind of liking #3, ugly as it sounds, because I'm not sure how
> much of a use-case there is for the upgrade-from-PLAIN case.
> Particularly now that TOAST is so ingrained in the system, it seems
> rather unlikely that a production-grade data type wouldn't have
> been designed to be toastable from the beginning, if there could be
> any advantage to that.  Either #1 or #2 seem like mighty high prices
> to pay for offering an option that might have no real-world uses.
>

Tomas' opening paragraph for this thread indicated this was motivated by
the plain-to-toast change but I'm not in a position to provide independent
insight.

Without that piece this is mainly about being able to specify a type's
preference for when and how it can be toasted.  That seems like sufficient
motivation, though that functionality seems basic enough that I'm wondering
why it hasn't come up before now (this seems like a different topic of
wonder than what Tomas mentioned in the OP).

Is there also an issue with whether the type has implemented compression or
not - i.e., should the x->e and m->e paths be forbidden too?  Or is it
always the case a non-plain type is compressible and the other non-plain
options just switch between preferences (so External just says "while I can
be compressed, please don't")?
Separately...

Can you please include an edit to [1] indicating that "e" is the
abbreviation for External and "x" is Extended (spelling out the other two
as well).  Might be worth a comment at [2] as well.

[1] https://www.postgresql.org/docs/12/catalog-pg-type.html
[2] https://www.postgresql.org/docs/12/storage-toast.html

Thanks!

David J.

  13. Re: Allowing ALTER TYPE to change storage strategy

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-03-05T19:05:13Z

    On Wed, Mar 04, 2020 at 06:56:42PM -0500, Tom Lane wrote:
>I wrote:
>> 3. Drop the ability for ALTER TYPE to promote from PLAIN to not-PLAIN
>> typstorage, and adjust the typcache so that it only remembers boolean
>> toastability not the specific toasting strategy.  Then the cache is
>> still immutable so no need for update logic.
>>
>> I'm kind of liking #3, ugly as it sounds, because I'm not sure how
>> much of a use-case there is for the upgrade-from-PLAIN case.
>> Particularly now that TOAST is so ingrained in the system, it seems
>> rather unlikely that a production-grade data type wouldn't have
>> been designed to be toastable from the beginning, if there could be
>> any advantage to that.  Either #1 or #2 seem like mighty high prices
>> to pay for offering an option that might have no real-world uses.
>
>Here's a v5 based on that approach.  I also added some comments about
>the potential race conditions involved in recursing to domains.
>

Well, I don't know what to say, really. This very thread started with me
explaining how I've repeatedly needed a way to upgrade from PLAIN, so I
don't quite agree with your claim that there's no use case for that.

Granted, the cases may be my faults - sometimes I have not expected the
type to need TOAST initially, and then later realizing I've been wrong.
In other cases I simply failed to realize PLAIN is the default value
even for varlena types (yes, it's a silly mistake).

FWIW I'm not suggesting you go and implement #1 or #2 for me, that'd be
up to me I guess. But I disagree there's no use case for it, and #3
makes this featuer useless for me.

regards

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

  14. Re: Allowing ALTER TYPE to change storage strategy

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-05T19:52:44Z

    Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> FWIW I'm not suggesting you go and implement #1 or #2 for me, that'd be
> up to me I guess. But I disagree there's no use case for it, and #3
> makes this featuer useless for me.

OK, then we need to do something else.  Do you have ideas for other
alternatives?

If not, we probably should bite the bullet and go for #1, since
I have little doubt that we'll need that someday anyway.
The trick will be to keep down the cache invalidation overhead...

			regards, tom lane

  15. Re: Allowing ALTER TYPE to change storage strategy

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-03-05T21:17:03Z

    On Thu, Mar 05, 2020 at 02:52:44PM -0500, Tom Lane wrote:
>Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> FWIW I'm not suggesting you go and implement #1 or #2 for me, that'd be
>> up to me I guess. But I disagree there's no use case for it, and #3
>> makes this featuer useless for me.
>
>OK, then we need to do something else.  Do you have ideas for other
>alternatives?
>

I don't have any other ideas, unfortunately. And I think if I had one,
it'd probably be some sort of ugly hack anyway :-/

>If not, we probably should bite the bullet and go for #1, since
>I have little doubt that we'll need that someday anyway.
>The trick will be to keep down the cache invalidation overhead...
>

Yeah, I agree #1 seems like the cleanest/best option. Are you worried
about the overhead due to the extra complexity, or overhead due to
cache getting invalidated for this particular reason?


regards

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

  16. Re: Allowing ALTER TYPE to change storage strategy

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-05T22:46:44Z

    I wrote:
> If not, we probably should bite the bullet and go for #1, since
> I have little doubt that we'll need that someday anyway.
> The trick will be to keep down the cache invalidation overhead...

Here's a version that does it like that.  I'm less worried about the
overhead than I was before, because I realized that we already had
a syscache callback for pg_type there.  And it was being pretty
stupid about which entries it reset, too, so this version might
actually net out as less overhead (in some workloads anyway).

For ease of review I just added the new TCFLAGS value out of
sequence, but I'd be inclined to renumber the bits back into
sequence before committing.

			regards, tom lane

  17. Re: Allowing ALTER TYPE to change storage strategy

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-05T23:08:27Z

    Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> Yeah, I agree #1 seems like the cleanest/best option. Are you worried
> about the overhead due to the extra complexity, or overhead due to
> cache getting invalidated for this particular reason?

The overhead is basically a hash_seq_search traversal over the typcache
each time we get a pg_type inval event, which there could be a lot of.
On the other hand we have a lot of inval overhead already, so this might
not amount to anything noticeable.

			regards, tom lane

  18. Re: Allowing ALTER TYPE to change storage strategy

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-03-06T13:42:18Z

    On Thu, Mar 05, 2020 at 05:46:44PM -0500, Tom Lane wrote:
>I wrote:
>> If not, we probably should bite the bullet and go for #1, since
>> I have little doubt that we'll need that someday anyway.
>> The trick will be to keep down the cache invalidation overhead...
>
>Here's a version that does it like that.  I'm less worried about the
>overhead than I was before, because I realized that we already had
>a syscache callback for pg_type there.  And it was being pretty
>stupid about which entries it reset, too, so this version might
>actually net out as less overhead (in some workloads anyway).
>
>For ease of review I just added the new TCFLAGS value out of
>sequence, but I'd be inclined to renumber the bits back into
>sequence before committing.
>

LGTM. If I had to nitpick, I'd say that the example in docs should be 

   ALTER TYPE mytype SET (
       SEND = mytypesend,
       RECEIVE = mytyperecv
   );

i.e. with uppercase SEND/RECEIVE, because that's how we spell it in
other examples in CREATE TYPE etc.


regards

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

  19. Re: Allowing ALTER TYPE to change storage strategy

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-06T17:20:43Z

    Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> On Thu, Mar 05, 2020 at 05:46:44PM -0500, Tom Lane wrote:
>> For ease of review I just added the new TCFLAGS value out of
>> sequence, but I'd be inclined to renumber the bits back into
>> sequence before committing.

> LGTM. If I had to nitpick, I'd say that the example in docs should be 
>    ALTER TYPE mytype SET (
>        SEND = mytypesend,
>        RECEIVE = mytyperecv
>    );
> i.e. with uppercase SEND/RECEIVE, because that's how we spell it in
> other examples in CREATE TYPE etc.

OK, pushed with those changes and some other docs-polishing.

			regards, tom lane

  20. Re: Allowing ALTER TYPE to change storage strategy

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-06T17:23:58Z

    "David G. Johnston" <david.g.johnston@gmail.com> writes:
> Is there also an issue with whether the type has implemented compression or
> not - i.e., should the x->e and m->e paths be forbidden too?  Or is it
> always the case a non-plain type is compressible and the other non-plain
> options just switch between preferences (so External just says "while I can
> be compressed, please don't")?

Yeah, the only relevant issue here is "can it be toasted, or not?".  A
data type doesn't have direct control of which toasting options can be
applied, nor does it need to, as long as the C functions apply the
correct detoast macros.

> Can you please include an edit to [1] indicating that "e" is the
> abbreviation for External and "x" is Extended (spelling out the other two
> as well).  Might be worth a comment at [2] as well.
> [1] https://www.postgresql.org/docs/12/catalog-pg-type.html
> [2] https://www.postgresql.org/docs/12/storage-toast.html

Done in [1]; I didn't see much point in changing [2].

			regards, tom lane