Re: Allowing ALTER TYPE to change storage strategy

On Fri, Feb 28, 2020 at 01:59:49PM -0500, Tom Lane wrote:
>Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)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