ALTER TYPE 2: skip already-provable no-work rewrites

On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch <noah@leadboat.com> wrote:

This patch removes ALTER TYPE rewrites in cases we can already prove valid.  I
add a function GetCoerceExemptions() that walks an Expr according to rules
discussed in the design thread, simplified slightly pending additions in the
next patch.  See the comment at that function for a refresher.  I use it to
populate two new bools to AlteredTableInfo, "new_bits" and "mayerror".
"new_bits" is a superset of "new_changedoids", so I subsume that.  I change
ATRewriteTable to act on those and support the notion of evaluating the
transformation expressions when we're not rewriting the table.

This helps on conversions like varchar(X)->text, xml->text, and conversions
between domains and their base types.

This certainly looks like a worthwhile thing to do, and it doesn't
seem to need a lot of code, which is great. But I confess I'm not
confident I really understand what this patch is changing or why it's
changing it. I think the problem is partly that the terminology used
is not very consistent:

+ 		if (!(exempt & COERCE_EXEMPT_NOCHANGE))
+ 			tab->new_bits = true;
+ 		if (!(exempt & COERCE_EXEMPT_NOERROR))
+ 			tab->mayerror = true;

These are the same two bits of status that are elsewhere called
always-noop and never-error. Or maybe not quite the same... but
close. A related problem is that I think only three of the four
combinations are actually interesting: if there are new bits... that
is, if !COERCE_EXEMPT_NOCHANGE... i.e. not always-noop, then the state
of the other bit is irrelevant. I think maybe this ought to just be
rephrased as an enum with three elements, describing the operation to
be performed: rewrite, check, nothing.

As unintended fallout, it's no longer an error to add oids or a column with a
default value to a table whose rowtype is used in columns elsewhere. This seems
best. Defaults on the origin table do not even apply to new inserts into such a
column, and the rowtype does not gain an OID column via its table.

I think this should be split into two patches (we can discuss both on
this thread, no need to start any new ones), one that implements just
the above improvement and another that accomplishes the main purpose
of the patch. Patches that do two or three or four things are quite a
bit harder to understand than patches that just do one thing.

On a related note, it is very helpful to avoid introducing unrelated
changes into a patch. Comment updates should reflect changes you
made, rather than editorialization on what's already there. There is
some value to the latter, but it makes it harder to understand what
the patch is doing.

Also, you need to update the ALTER TABLE documentation. The whole
notes section needs to be gone over, but the following part in
particular seems problematic, since we're proposing to break this:

# The fact that ALTER TYPE requires rewriting the whole table is
sometimes an advantage, because the rewriting process
# eliminates any dead space in the table. For example, to reclaim the
space occupied by a dropped column immediately, the
# fastest way is:
#
# ALTER TABLE table ALTER COLUMN anycol TYPE anytype;

I'm not sure what we can recommend here instead.

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

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch <noah@leadboat.com> wrote:

As unintended fallout, it's no longer an error to add oids or a column with a
default value to a table whose rowtype is used in columns elsewhere. This seems
best. Defaults on the origin table do not even apply to new inserts into such a
column, and the rowtype does not gain an OID column via its table.

I think this should be split into two patches (we can discuss both on
this thread, no need to start any new ones), one that implements just
the above improvement and another that accomplishes the main purpose
of the patch.

I haven't been paying much attention to this thread, but I happened to
read the above, and quite frankly it scares the cr*p out of me. I don't
believe that Noah even begins to be qualified to understand the
implications of adding/removing an OID column, and I clearly remember
the non-obvious bugs we've had in the past from that. Before this goes
in I want to see a convincing explanation (not an unsupported assertion)
why this isn't going to re-introduce those old bugs.

I'm also pretty unclear on why it's a good idea to let uses of a rowtype
be different from the table on which it's allegedly based. To the
extent that the current behavior allows that, isn't that a bug rather
than a feature we should extend?

# The fact that ALTER TYPE requires rewriting the whole table is
sometimes an advantage, because the rewriting process
# eliminates any dead space in the table.

I'm not sure what we can recommend here instead.

New-style VACUUM FULL. I don't think that a patch that makes it harder
to use ALTER TABLE this way is a problem in itself, now that we've got
that.

regards, tom lane

On Sun, Jan 23, 2011 at 12:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch <noah@leadboat.com> wrote:

As unintended fallout, it's no longer an error to add oids or a column with a
default value to a table whose rowtype is used in columns elsewhere.  This seems
best.  Defaults on the origin table do not even apply to new inserts into such a
column, and the rowtype does not gain an OID column via its table.

I think this should be split into two patches (we can discuss both on
this thread, no need to start any new ones), one that implements just
the above improvement and another that accomplishes the main purpose
of the patch.

I haven't been paying much attention to this thread, but I happened to
read the above, and quite frankly it scares the cr*p out of me.  I don't
believe that Noah even begins to be qualified to understand the
implications of adding/removing an OID column, and I clearly remember
the non-obvious bugs we've had in the past from that.  Before this goes
in I want to see a convincing explanation (not an unsupported assertion)
why this isn't going to re-introduce those old bugs.

Because all of our old bugs now have regression tests that will break
if we reintroduce them?

I guess that probably falls into the category of "wishful thinking".

I'm also pretty unclear on why it's a good idea to let uses of a rowtype
be different from the table on which it's allegedly based.  To the
extent that the current behavior allows that, isn't that a bug rather
than a feature we should extend?

It's not clear to me what it would mean for OIDs or default values to
propagate themselves to the table's row type. Check constraints,
foreign keys, unique constraints, etc. don't either. In fact, not
even the NOT NULL property flows through. On the surface, preventing
these details from interfering with ALTER TABLE commands that can't
actually break anything seems like removing an annoying implementation
restriction, but I guess one could make the argument that we actually
intend to make those flow through some day. But if so, this is the
first I'm hearing of it.

# The fact that ALTER TYPE requires rewriting the whole table is
sometimes an advantage, because the rewriting process
# eliminates any dead space in the table.

I'm not sure what we can recommend here instead.

New-style VACUUM FULL.  I don't think that a patch that makes it harder
to use ALTER TABLE this way is a problem in itself, now that we've got
that.

Cool. That'll reclaim space from dropped columns and stuff?

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

On Sun, Jan 23, 2011 at 12:06:43PM -0500, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch <noah@leadboat.com> wrote:

As unintended fallout, it's no longer an error to add oids or a column with a
default value to a table whose rowtype is used in columns elsewhere. This seems
best. Defaults on the origin table do not even apply to new inserts into such a
column, and the rowtype does not gain an OID column via its table.

I think this should be split into two patches (we can discuss both on
this thread, no need to start any new ones), one that implements just
the above improvement and another that accomplishes the main purpose
of the patch.

I haven't been paying much attention to this thread, but I happened to
read the above, and quite frankly it scares the cr*p out of me. I don't
believe that Noah even begins to be qualified to understand the
implications of adding/removing an OID column, and I clearly remember
the non-obvious bugs we've had in the past from that. Before this goes
in I want to see a convincing explanation (not an unsupported assertion)
why this isn't going to re-introduce those old bugs.

Turns out that we do set HEAP_HASOID and allocate space for an OID in the
composite-type datums. We don't actually assign an OID, and I can't see any way
to read it from the composite. It seems most consistent with the verdict of
commit 6d1e361 to continue rejecting these cases. Thanks for the heads-up.

I'm also pretty unclear on why it's a good idea to let uses of a rowtype
be different from the table on which it's allegedly based. To the
extent that the current behavior allows that, isn't that a bug rather
than a feature we should extend?

From the perspective of defining the behavior afresh, I'd agree. I haven't seen
any stirrings of actually implementing this, though. Like Robert, I'm doubting
there's a user benefit from rejecting these cases in preparation for the day
that they would actually require it.

nm

Robert,

Thanks for the review.

On Sat, Jan 22, 2011 at 11:28:48PM -0500, Robert Haas wrote:

This certainly looks like a worthwhile thing to do, and it doesn't
seem to need a lot of code, which is great. But I confess I'm not
confident I really understand what this patch is changing or why it's
changing it.

I think the problem is partly that the terminology used
is not very consistent:

+ 		if (!(exempt & COERCE_EXEMPT_NOCHANGE))
+ 			tab->new_bits = true;
+ 		if (!(exempt & COERCE_EXEMPT_NOERROR))
+ 			tab->mayerror = true;

These are the same two bits of status that are elsewhere called
always-noop and never-error. Or maybe not quite the same... but
close. A related problem is that I think only three of the four
combinations are actually interesting: if there are new bits... that
is, if !COERCE_EXEMPT_NOCHANGE... i.e. not always-noop, then the state
of the other bit is irrelevant. I think maybe this ought to just be
rephrased as an enum with three elements, describing the operation to
be performed: rewrite, check, nothing.

I've fixed the GetCoerceExemptions header comments to follow the #define'd
names. You are correct that only three of the four possibilities are distinct
for ALTER TABLE purposes. I've adopted the enum in tablecmds.c.

As unintended fallout, it's no longer an error to add oids or a column with a
default value to a table whose rowtype is used in columns elsewhere. This seems
best. Defaults on the origin table do not even apply to new inserts into such a
column, and the rowtype does not gain an OID column via its table.

I think this should be split into two patches (we can discuss both on
this thread, no need to start any new ones), one that implements just
the above improvement and another that accomplishes the main purpose
of the patch. Patches that do two or three or four things are quite a
bit harder to understand than patches that just do one thing.

Sounds good; done.

Also, you need to update the ALTER TABLE documentation. The whole
notes section needs to be gone over, but the following part in
particular seems problematic, since we're proposing to break this:

Done.

I'm attaching four patches:

* at1.1-default-composite.patch
Remove the error when the user adds a column with a default value to a table
whose rowtype is used in a column elsewhere.
* at1.2-doc-set-data-type.patch
The documentation used "ALTER TYPE" when it meant "SET DATA TYPE", a subform of
"ALTER TABLE" or "ALTER FOREIGN TABLE". Fixes just that.
* at1.3-tablecmds-enum.patch
Implements your suggestion of using an enum to represent the choice between
rewriting, scanning, and doing nothing. No functional changes. Most of this
patch is re-indentation, so I'm also attaching "at1.3w-tablecmds-enum.patch",
the same change under "git diff -w". I reflowed the comment blocks that became
too wide, but I did not reflow the ones that now have more width available.
* at2v2-skip-nowork.patch
The remainder of the original patch, with the updates noted above.

nm

On Mon, Jan 24, 2011 at 7:10 PM, Noah Misch <noah@leadboat.com> wrote:

* at1.1-default-composite.patch
Remove the error when the user adds a column with a default value to a table
whose rowtype is used in a column elsewhere.

Can we fix this without moving the logic around quite so much? I'm
worried that could introduce bugs.

It strikes me that the root of the problem here is that this test is
subtly wrong:

if (newrel)
find_composite_type_dependencies(oldrel->rd_rel->reltype,

RelationGetRelationName(oldrel),

NULL);

So what this is saying is: If the user has performed an operation that
requires a rewrite, then we can't carry out that operation if the
rowtype is used elsewhere, because we wouldn't be able to propagate
the rewrite to those other objects. That's correct, unless the
operation in question is one which isn't supported by composite types
anyway. We trigger a rewrite if there is a has-OIDs change or if
tab->newvals contains any elements, which can happen if either there
is a type change or if a column with a default is added. So it seems
to me that we could fix this with something like the attached.
Thoughts?

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

On Mon, Jan 24, 2011 at 7:10 PM, Noah Misch <noah@leadboat.com> wrote:

* at1.2-doc-set-data-type.patch
The documentation used "ALTER TYPE" when it meant "SET DATA TYPE", a subform of
"ALTER TABLE" or "ALTER FOREIGN TABLE".  Fixes just that.

Committed this part. For reasons involving me being tired, I
initially thought that only the first part was correct, which is why I
did it as two commits. But it's obviously right, so now it's all
committed. I back-patched the ALTER TABLE part to 9.0.X so it'll show
up in the web site docs after the next minor release.

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

On Tue, Jan 25, 2011 at 06:40:08PM -0500, Robert Haas wrote:

On Mon, Jan 24, 2011 at 7:10 PM, Noah Misch <noah@leadboat.com> wrote:

* at1.1-default-composite.patch
Remove the error when the user adds a column with a default value to a table
whose rowtype is used in a column elsewhere.

Can we fix this without moving the logic around quite so much? I'm
worried that could introduce bugs.

It strikes me that the root of the problem here is that this test is
subtly wrong:

if (newrel)
find_composite_type_dependencies(oldrel->rd_rel->reltype,

RelationGetRelationName(oldrel),

NULL);

Correct.

So it seems
to me that we could fix this with something like the attached.
Thoughts?

I'm fine with this patch. A few notes based on its context in the larger
project:

--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c

@@ -3366,14 +3367,14 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
}

/*
-	 * If we need to rewrite the table, the operation has to be propagated to
-	 * tables that use this table's rowtype as a column type.
+	 * If we change column data types or add/remove OIDs, the operation has to
+	 * be propagated to tables that use this table's rowtype as a column type.
*
* (Eventually this will probably become true for scans as well, but at
* the moment a composite type does not enforce any constraints, so it's
* not necessary/appropriate to enforce them just during ALTER.)
*/
-	if (newrel)
+	if (tab->new_changetypes || tab->new_changeoids)

The next patch removed new_changeoids, so we would instead be keeping it with
this as the only place referencing it.

find_composite_type_dependencies(oldrel->rd_rel->reltype,
RelationGetRelationName(oldrel),
NULL);
@@ -6347,6 +6348,7 @@ ATPrepAlterColumnType(List **wqueue,
newval->expr = (Expr *) transform;

tab->newvals = lappend(tab->newvals, newval);
+ tab->new_changetypes = true;

The at2v2 patch would then morph to do something like:

if (worklevel != WORK_NONE)
tab->new_changetypes = true;

That weakens the name "new_changetypes" a bit.

On Tue, Jan 25, 2011 at 10:22 PM, Noah Misch <noah@leadboat.com> wrote:

I'm fine with this patch.

OK, committed.

The next patch removed new_changeoids, so we would instead be keeping it with
this as the only place referencing it.

[...]

The at2v2 patch would then morph to do something like:

if (worklevel != WORK_NONE)
       tab->new_changetypes = true;

Well, I'm not too keen on either of those things. The second one,
especially, looks like the sense of the Boolean is clearly being
abused, so either the Boolean needs to be renamed or some other change
is required.

I'd also suggest that this big if-block you changed to a case
statement could just as well stay as an if-block. There are only
three cases, and we want to avoid rearranging things more than
necessary. It complicates both review and back-patching to no good
end.

I think you should collect up what's left of ALTER TABLE 0 and the
stuff on this thread, rebase it, and submit it as a single patch on
this thread that applies directly against the master branch. We may
decide to split it back up again in some other way, but I think the
current division isn't actually buying us much.

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

On Wed, Jan 26, 2011 at 07:31:40AM -0500, Robert Haas wrote:

I'd also suggest that this big if-block you changed to a case
statement could just as well stay as an if-block. There are only
three cases, and we want to avoid rearranging things more than
necessary. It complicates both review and back-patching to no good
end.

Okay. I've also left out the large reindent in ATRewriteTable for now. Easy to
re-add it later if desired.

I think you should collect up what's left of ALTER TABLE 0 and the
stuff on this thread, rebase it, and submit it as a single patch on
this thread that applies directly against the master branch. We may
decide to split it back up again in some other way, but I think the
current division isn't actually buying us much.

Done as attached. This preserves compatibility with our current handling of
composite type dependencies. The rest you've seen before.

Thanks,
nm

On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch <noah@leadboat.com> wrote:

Done as attached.  This preserves compatibility with our current handling of
composite type dependencies.  The rest you've seen before.

OK, so I took a look at this in more detail today. The current logic
for table rewrites looks like this:

1. If we're changing the data type of a column, or adding a column
with a default, or adding/dropping OIDs, rewrite the table. Stop.
2. Otherwise, if we're adding a constraint or NOT NULL, scan the table
and check constraints.
3. If we're changing tablespaces, copy the table block-by-block.

It seems to me that the revised logic needs to look like this:

1. If we're changing the data type of a column and the existing
contents are not binary coercible to the new contents, or if we're
adding a column with a default or adding/dropping OIDs, rewrite the
table. Stop.
2. Otherwise, if we're adding a constraint or NOT NULL, scan the table
and check constraints.
3. If we're changing the data type of a column in the table, reindex the table.
4. If we're changing tablespaces, copy the table block-by-block.

I might be missing something, but I don't see that the patch includes
step #3, which I think is necessary. For example, citext is binary
coercible to text, but you can't reuse the index because the
comparison function is different. Of course, if you're changing the
type of a column to its already-current type, you can skip #3, but if
that's the only case we can optimize, it's not much of an
accomplishment. I guess this gets back to the ALTER TYPE 7 patch,
which I haven't looked at in detail, but I have a feeling it may be
controversial.

Another problem here is that if you have to do both #2 and #3, you
might have been better off (or just as well off) doing #1, unless you
can somehow jigger things so that the same scan does both the
constraint checks and the index rebuild. That doesn't look simple.

Thoughts?

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

Robert,

Thanks for the obviously thought-out review.

On Sat, Feb 05, 2011 at 01:29:35AM -0500, Robert Haas wrote:

On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch <noah@leadboat.com> wrote:

Done as attached. ?This preserves compatibility with our current handling of
composite type dependencies. ?The rest you've seen before.

OK, so I took a look at this in more detail today. The current logic
for table rewrites looks like this:

1. If we're changing the data type of a column, or adding a column
with a default, or adding/dropping OIDs, rewrite the table. Stop.
2. Otherwise, if we're adding a constraint or NOT NULL, scan the table
and check constraints.
3. If we're changing tablespaces, copy the table block-by-block.

Correct. It's perhaps obvious, but rewriting the table will always reindex it.

It seems to me that the revised logic needs to look like this:

1. If we're changing the data type of a column and the existing
contents are not binary coercible to the new contents, or if we're
adding a column with a default or adding/dropping OIDs, rewrite the
table. Stop.
2. Otherwise, if we're adding a constraint or NOT NULL, scan the table
and check constraints.

With this patch, step 2 changes changes to "Otherwise, if we're adding a
constraint or NOT NULL, or changing a column to a binary-compatible domain with
a domain CHECK constraint, scan the table and check constraints."

3. If we're changing the data type of a column in the table, reindex the table.

Rebuild indexes that depend on a changing column. Other indexes can stay.

4. If we're changing tablespaces, copy the table block-by-block.

I might be missing something, but I don't see that the patch includes
step #3, which I think is necessary. For example, citext is binary
coercible to text, but you can't reuse the index because the
comparison function is different. Of course, if you're changing the
type of a column to its already-current type, you can skip #3, but if
that's the only case we can optimize, it's not much of an
accomplishment. I guess this gets back to the ALTER TYPE 7 patch,
which I haven't looked at in detail, but I have a feeling it may be
controversial.

It's there, but it's happening rather implicitly. ATExecAlterColumnType builds
lists of indexes and constraints that depend on changing columns. Specifically,
it stashes their OIDs and the SQL to recreate them. ATPostAlterTypeCleanup
drops those objects by OID, then parses the SQL statements, now based on the
updated table definition. ATExecAddIndex and ATExecAddConstraint use those
parsed statements to recreate the objects. The key is the skip_build logic in
ATExecAddIndex: if ATRewriteTables will rewrite the table (and therefore *all*
indexes), we skip the build at that earlier stage to avoid building the same
index twice. The only thing I had to do was update the skip_build condition so
it continues to mirror the corresponding test in ATRewriteTables.

Originally I had this patch doing a full reindex, with an eye to having the next
patch reduce the scope to dependent indexes. However, all the infrastructure
was already there, and it actually made this patch smaller to skip directly to
what it does today.

ALTER TYPE 7 additionally skips builds of indexes that depend on a changing
column but can be proven compatible. So it's in the business of, for example
figuring out that text and varchar are compatible but text and citext are not.

Another problem here is that if you have to do both #2 and #3, you
might have been better off (or just as well off) doing #1, unless you
can somehow jigger things so that the same scan does both the
constraint checks and the index rebuild. That doesn't look simple.

We have no such optimization during #1, either, so #2+#3 is never worse. In
particular, #1 scans the table (# indexes total) + 1 times, while #2+#3 scans it
(# of indexes depending on changed columns) + 1 times.

There are some nice optimization opportunities here, to be sure. As a specific
first step, teach index_build to create multiple indexes with a single scan,
then have reindex_relation use that. Probably not simple. Combining that with
the ATRewriteTable scan would be less simple still.

nm

On Sat, Feb 5, 2011 at 3:05 AM, Noah Misch <noah@leadboat.com> wrote:

Correct.  It's perhaps obvious, but rewriting the table will always reindex it.

Right.

3. If we're changing the data type of a column in the table, reindex the table.

Rebuild indexes that depend on a changing column.  Other indexes can stay.

Good point.

4. If we're changing tablespaces, copy the table block-by-block.

I might be missing something, but I don't see that the patch includes
step #3, which I think is necessary.  For example, citext is binary
coercible to text, but you can't reuse the index because the
comparison function is different.  Of course, if you're changing the
type of a column to its already-current type, you can skip #3, but if
that's the only case we can optimize, it's not much of an
accomplishment.  I guess this gets back to the ALTER TYPE 7 patch,
which I haven't looked at in detail, but I have a feeling it may be
controversial.

It's there, but it's happening rather implicitly.

I see now. So you're actually not really making any change to that
machinery. It's sufficient to just skip the rewrite of the heap when
it isn't needed, and without any particular code change the indexes
will sort themselves out.

We have no such optimization during #1, either, so #2+#3 is never worse.  In
particular, #1 scans the table (# indexes total) + 1 times, while #2+#3 scans it
(# of indexes depending on changed columns) + 1 times.

OK.

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

On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch <noah@leadboat.com> wrote:

Done as attached.

Looking at this still more, it appears that independent of any change
this patch may wish to make, there's a live bug here related to the
foreign table patch I committed back in December. Creating a foreign
table creates an eponymous rowtype, which can be used as a column in a
regular table. You can then change the data type of a column in the
foreign table, read from the regular table, and crash the server.

The simple fix for this is to just change the code in
ATPrepAlterColumnType to handle the foreign table case also:

if (tab->relkind == RELKIND_COMPOSITE_TYPE)
{
/*
* For composite types, do this check now. Tables will check
* it later when the table is being rewritten.
*/
find_composite_type_dependencies(rel->rd_rel->reltype,
NULL,
RelationGetRelationName(rel));
}

But this is a little unsatisfying, for two reasons. First, the error
message will be subtly wrong: we can make it complain about a table or
a type, but not a foreign table. At a quick look, it likes the right
fix might be to replace the second and third arguments to
find_composite_type_dependencies() with a Relation. Second, I wonder
if we shouldn't refactor things so that all the checks fire in
ATRewriteTables() rather than doing them in different places. Seems
like that might be cleaner.

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

On Sat, Feb 05, 2011 at 10:03:59AM -0500, Robert Haas wrote:

Looking at this still more, it appears that independent of any change
this patch may wish to make, there's a live bug here related to the
foreign table patch I committed back in December. Creating a foreign
table creates an eponymous rowtype, which can be used as a column in a
regular table. You can then change the data type of a column in the
foreign table, read from the regular table, and crash the server.

The simple fix for this is to just change the code in
ATPrepAlterColumnType to handle the foreign table case also:

if (tab->relkind == RELKIND_COMPOSITE_TYPE)
{
/*
* For composite types, do this check now. Tables will check
* it later when the table is being rewritten.
*/
find_composite_type_dependencies(rel->rd_rel->reltype,
NULL,
RelationGetRelationName(rel));
}

But this is a little unsatisfying, for two reasons. First, the error
message will be subtly wrong: we can make it complain about a table or
a type, but not a foreign table. At a quick look, it likes the right
fix might be to replace the second and third arguments to
find_composite_type_dependencies() with a Relation.

Seems like a clear improvement.

Second, I wonder
if we shouldn't refactor things so that all the checks fire in
ATRewriteTables() rather than doing them in different places. Seems
like that might be cleaner.

Offhand, this seems reasonable, too. I assumed there was some good reason it
couldn't be done there for non-tables, but nothing comes to mind.

On Sat, Feb 5, 2011 at 7:44 PM, Noah Misch <noah@leadboat.com> wrote:

But this is a little unsatisfying, for two reasons.  First, the error
message will be subtly wrong: we can make it complain about a table or
a type, but not a foreign table.  At a quick look, it likes the right
fix might be to replace the second and third arguments to
find_composite_type_dependencies() with a Relation.

Seems like a clear improvement.

That didn't quite work, because there's a caller in typecmds.c that
doesn't have the relation handy. So I made it take a relkind and a
name, which works fine.

Second, I wonder
if we shouldn't refactor things so that all the checks fire in
ATRewriteTables() rather than doing them in different places.  Seems
like that might be cleaner.

Offhand, this seems reasonable, too.  I assumed there was some good reason it
couldn't be done there for non-tables, but nothing comes to mind.

Actually, thinking about this more, I'm thinking if we're going to
change anything, it seems we ought to go the other way. If we ever
actually did support recursing into wherever the composite type
dependencies take us, we'd want to detect that before phase 3 and add
work-queue items for each table that we needed to futz with.

The attached patch takes this approach. It's very slightly more code,
but it reduces the amount of spooky action at a distance. The
existing coding is basically relying on the assumption that operations
which require finding composite type dependencies also require a table
rewrite. That was probably true at one point in time, but it's not
really quite right. It already requires compensating code foreign
tables and composite types (which can require this treatment even
though there's nothing to rewrite) and your proposed changes to avoid
table rewrites in cases where a type is changed to a compatible type
would break it in the opposite direction (the check would still be
needed even if the parent table doesn't need a rewrite, because the
rowtype could be indexed in some fashion that depends on the type of
the column being changed).

Comments?

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

On Sun, Feb 06, 2011 at 02:15:47AM -0500, Robert Haas wrote:

On Sat, Feb 5, 2011 at 7:44 PM, Noah Misch <noah@leadboat.com> wrote:

But this is a little unsatisfying, for two reasons. ?First, the error
message will be subtly wrong: we can make it complain about a table or
a type, but not a foreign table. ?At a quick look, it likes the right
fix might be to replace the second and third arguments to
find_composite_type_dependencies() with a Relation.

Seems like a clear improvement.

That didn't quite work, because there's a caller in typecmds.c that
doesn't have the relation handy. So I made it take a relkind and a
name, which works fine.

Hmm, indeed. In get_rels_with_domain(), it's a scalar type.

Second, I wonder
if we shouldn't refactor things so that all the checks fire in
ATRewriteTables() rather than doing them in different places. ?Seems
like that might be cleaner.

Offhand, this seems reasonable, too. ?I assumed there was some good reason it
couldn't be done there for non-tables, but nothing comes to mind.

Actually, thinking about this more, I'm thinking if we're going to
change anything, it seems we ought to go the other way. If we ever
actually did support recursing into wherever the composite type
dependencies take us, we'd want to detect that before phase 3 and add
work-queue items for each table that we needed to futz with.

The attached patch takes this approach. It's very slightly more code,
but it reduces the amount of spooky action at a distance.

Comments?

Your patch improves the code. My standard for commending a refactor-only patch
is rather high, though, and this patch doesn't reach it. The ancestral code
placement wasn't obviously correct, but neither is this. So I'd vote -0.

nm

On Sun, Feb 6, 2011 at 4:15 AM, Noah Misch <noah@leadboat.com> wrote:

That didn't quite work, because there's a caller in typecmds.c that
doesn't have the relation handy.  So I made it take a relkind and a
name, which works fine.

Hmm, indeed.  In get_rels_with_domain(), it's a scalar type.

Yeeeeeeah, that's actually a little ugly. It's actually a domain
over a composite type, not a composite type proper, IIUC. Better
ideas?

The attached patch takes this approach.  It's very slightly more code,
but it reduces the amount of spooky action at a distance.

Comments?

Your patch improves the code.  My standard for commending a refactor-only patch
is rather high, though, and this patch doesn't reach it.  The ancestral code
placement wasn't obviously correct, but neither is this.  So I'd vote -0.

Well, what's your suggestion, then? Your patch pops the test up from
ATRewriteTable() up to ATRewriteTables(), but that's not obviously
correct either, and it's an awkward place to do it because we don't
have the Relation object handy at the point where the check needs to
be done.

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

On Sun, Feb 06, 2011 at 07:54:52AM -0500, Robert Haas wrote:

On Sun, Feb 6, 2011 at 4:15 AM, Noah Misch <noah@leadboat.com> wrote:

That didn't quite work, because there's a caller in typecmds.c that
doesn't have the relation handy. ?So I made it take a relkind and a
name, which works fine.

Hmm, indeed. ?In get_rels_with_domain(), it's a scalar type.

Yeeeeeeah, that's actually a little ugly. It's actually a domain
over a composite type, not a composite type proper, IIUC. Better
ideas?

There are no domains over composite types. get_rels_with_domain() finds all
relations having columns of the (scalar) domain type. It then calls
find_composite_type_dependencies to identify uses of the composite types
discovered in the previous step.

Honestly, RELKIND_COMPOSITE_TYPE is a reasonable choice despite the technical
mismatch. One more-correct approach would be to have two arguments, a catalog
OID (pg_type or pg_class, currently) and a relkind, 0 when the catalog OID !=
pg_class. Might be an improvement, albeit a minor one.

The attached patch takes this approach. ?It's very slightly more code,
but it reduces the amount of spooky action at a distance.

Comments?

Your patch improves the code. ?My standard for commending a refactor-only patch
is rather high, though, and this patch doesn't reach it. ?The ancestral code
placement wasn't obviously correct, but neither is this. ?So I'd vote -0.

Well, what's your suggestion, then? Your patch pops the test up from
ATRewriteTable() up to ATRewriteTables(), but that's not obviously
correct either, and it's an awkward place to do it because we don't
have the Relation object handy at the point where the check needs to
be done.

Agreed. I couldn't think of any grand improvements, so my patch bore the
conceptually-smallest change I could come up with to handle that particular
issue. That is, I felt it was the change that could most easily be verified as
rejecting the same cases as the old test.

nm