Allowing ALTER TYPE to change storage strategy

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

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

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

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

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

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

/messages/by-id/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

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,

/messages/by-id/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

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

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

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

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]https://www.postgresql.org/docs/12/catalog-pg-type.html 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]https://www.postgresql.org/docs/12/storage-toast.html 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.

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

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

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

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

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

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

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

"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