Re: Avoid index rebuilds for no-rewrite ALTER TABLE ALTER TYPE

Previous version (at7-alt-index-opfamily.patch):
http://archives.postgresql.org/message-id/20110113230124.GA18733@tornado.gateway.2wire.net
Design:
http://archives.postgresql.org/message-id/20110524104029.GB18831@tornado.gateway.2wire.net

Patches committed in 2011CF1 allow ALTER TABLE ... ALTER ... SET DATA TYPE to
avoid rewriting the table in certain cases. We still rebuild all indexes and
recheck all foreign key constraints dependent on the changing column. This
patch avoids the rebuild for some indexes, using the operator family facility
to identify when that is safe.

Changes since last version:

* Instead of noting in comments that the operator family contracts do not make
the guarantees I desire and opining that the code is fine in practice anyway,
I amend those contracts in the documentation. See the aforementioned design
explanation, but note that its third and fifth sentences are incorrect. I
settled on requiring identical behavior among operators in the family after
implicit casts (of any castmethod) or binary coercion casts (of any
castcontext) on operands. I really only need the requirement for binary
coercion casts, but implicit casts seem like a obvious extension.

* Update for per-column collations. Index collations need to match.

* We don't assign meaning to GIN or GiST operator family groupings. For
access methods other than btree and hash, require an index rebuild unless the
operator classes match exactly. Even if we could do otherwise, it would
currently be academic; GIN "array_ops" is the only such family with more than
one constituent operator class. The only practical case I've observed to be
helped here is a conversion like varchar(2)[] -> varchar(4)[] with a GIN index
on the column; operator class comparison covers that fine.

* Remove support for avoiding foreign key constraint revalidation. This is
orthogonal to the index case, though they share many principles. If we get
this committed, I will submit a small patch for foreign keys at a later date.

* Original patch was submitted in two forms for comment on the relative
merits. The first form had tablecmds.c updating catalog entries pertaining to
an old index until it matched how the index would be recreated with the new
column data type. The second form actually dropped and recreated the index
using the usual facilities, then reattached the old RelFileNode to the "new"
index. Nobody commented, but I've come around to thinking the second approach
is clearly better. Therefore, I'm submitting only that.

* Change the division of labor between tablecmds.c and indexcmds.c.
* Test cases removed per 2011CF1 discussion.
* Sync with variable names changed since last submission.

This patch is fully independent of the "Eliding no-op varchar length
coercions" patch I've posted recently, though they do have synergy.

Thanks,
nm

On Wed, Jun 15, 2011 at 1:03 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> [patch to avoid index rebuilds]

With respect to the documentation hunks, it seems to me that the first
hunk might be made clearer by leaving the paragraph of which it is a
part as-is, and adding another paragraph afterwards beginning with the
words "In addition". I am not sure whether the second hunk is
necessary at all. Doesn't the existing language cover the same
territory as what you've added?

I think that the variables in ATPostAlterTypeCleanup() could be better
named. They appear to be values, when in fact they are ListCells.
Honestly I'd probably just use l1 and l2, but if you want to insist on
some more mnemonic naming it should probably be something that sounds
vaguely list-ish.

As you no doubt expected, my eyes was immediately drawn to the
index-resurrection hack. Reviewing the thread, I see that you asked
about that in January and never got feedback. I have to say that what
you've done here looks like a pretty vile hack, but it's hard to say
for sure without knowing what to compare it against. You made
reference to this being smaller and simpler than updating the index
definition in place - can you give a sketch of what would need to be
done if we went that route instead?

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

On Mon, Jun 27, 2011 at 03:45:43PM -0400, Robert Haas wrote:
> On Wed, Jun 15, 2011 at 1:03 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > [patch to avoid index rebuilds]
>
> With respect to the documentation hunks, it seems to me that the first
> hunk might be made clearer by leaving the paragraph of which it is a
> part as-is, and adding another paragraph afterwards beginning with the
> words "In addition".

The added restriction elaborates on the transitivity requirement, so I wanted to
keep the new language adjacent to that.

> I am not sure whether the second hunk is
> necessary at all. Doesn't the existing language cover the same
> territory as what you've added?

The first hunk updates the contract for btree families, and the second updates
the contract for hash families. I kept the second instance a bit terse since it
follows soon after the similar text for B-tree.

> I think that the variables in ATPostAlterTypeCleanup() could be better
> named. They appear to be values, when in fact they are ListCells.
> Honestly I'd probably just use l1 and l2, but if you want to insist on
> some more mnemonic naming it should probably be something that sounds
> vaguely list-ish.

Okay; I'll do that in the next version. Either l1/l2 or maybe oid_item/def_item
like we use in postgres.c.

> As you no doubt expected, my eyes was immediately drawn to the
> index-resurrection hack. Reviewing the thread, I see that you asked
> about that in January and never got feedback. I have to say that what
> you've done here looks like a pretty vile hack, but it's hard to say
> for sure without knowing what to compare it against. You made
> reference to this being smaller and simpler than updating the index
> definition in place - can you give a sketch of what would need to be
> done if we went that route instead?

In "at7-index-opfamily.patch" attached to
http://archives.postgresql.org/message-id/20110113230124.GA18733@tornado.gateway.2wire.net
check out the code following the comment "/* The old index is compatible.
Update catalogs. */" until the end of the function. That code would need
updates for per-column collations, and it incorrectly reuses
values/nulls/replace arrays. It probably does not belong in tablecmds.c,
either. However, it gives the right general outline.

It would be valuable to avoid introducing a second chunk of code that knows
everything about the catalog entries behind an index. That's what led me to the
put forward the most recent version as best. What do you find vile about that
approach? I wasn't comfortable with it at first, because I suspected the checks
in RelationPreserveStorage() might be important for correctness. Having studied
it some more, though, I think they just reflect the narrower needs of its
current sole user.

Thanks,
nm

On Mon, Jun 27, 2011 at 10:43 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Mon, Jun 27, 2011 at 03:45:43PM -0400, Robert Haas wrote:
>> On Wed, Jun 15, 2011 at 1:03 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> > [patch to avoid index rebuilds]
>>
>> With respect to the documentation hunks, it seems to me that the first
>> hunk might be made clearer by leaving the paragraph of which it is a
>> part as-is, and adding another paragraph afterwards beginning with the
>> words "In addition".
>
> The added restriction elaborates on the transitivity requirement, so I wanted to
> keep the new language adjacent to that.

That makes sense, but it reads a bit awkwardly to me. Maybe it's just
that the sentence itself is so complicated that I have difficulty
understanding it. I guess it's the same problem as with the text you
added about hash indexes: without thinking about it, it's hard to
understand what territory is covered by the new sentence that is not
covered by what's already there. In the case of the hash indexes, I
think I have it figured out: we already know that we must get
compatible hash values out of logically equal values of different
datatypes. But it's possible that the inter-type cast changes the
value in some arbitrary way and then compensates for it by defining
the hash function in such a way as to compensate. Similarly, for
btrees, we need the relative ordering of A and B to remain the same
after casting within the opfamily, not to be rearranged somehow.
Maybe the text you've got is OK to explain this, but I wonder if
there's a way to do it more simply.

>> As you no doubt expected, my eyes was immediately drawn to the
>> index-resurrection hack.  Reviewing the thread, I see that you asked
>> about that in January and never got feedback.  I have to say that what
>> you've done here looks like a pretty vile hack, but it's hard to say
>> for sure without knowing what to compare it against.  You made
>> reference to this being smaller and simpler than updating the index
>> definition in place - can you give a sketch of what would need to be
>> done if we went that route instead?
>
> In "at7-index-opfamily.patch" attached to
> http://archives.postgresql.org/message-id/20110113230124.GA18733@tornado.gateway.2wire.net
> check out the code following the comment "/* The old index is compatible.
> Update catalogs. */" until the end of the function.  That code would need
> updates for per-column collations, and it incorrectly reuses
> values/nulls/replace arrays.  It probably does not belong in tablecmds.c,
> either.  However, it gives the right general outline.
>
> It would be valuable to avoid introducing a second chunk of code that knows
> everything about the catalog entries behind an index.  That's what led me to the
> put forward the most recent version as best.  What do you find vile about that
> approach?  I wasn't comfortable with it at first, because I suspected the checks
> in RelationPreserveStorage() might be important for correctness.  Having studied
> it some more, though, I think they just reflect the narrower needs of its
> current sole user.

Maybe vile is a strong word, but it seems like a modularity violation:
we're basically letting DefineIndex() do some stuff we don't really
want done, and then backing it out parts of it that we don't really
want done after all. It seems like it would be better to provide
DefineIndex with enough information not to do the wrong thing in the
first place. Could we maybe pass stmt->oldNode to DefineIndex(), and
let it work out the details?

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

On Tue, Jun 28, 2011 at 02:11:11PM -0400, Robert Haas wrote:
> On Mon, Jun 27, 2011 at 10:43 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > On Mon, Jun 27, 2011 at 03:45:43PM -0400, Robert Haas wrote:
> >> On Wed, Jun 15, 2011 at 1:03 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> >> > [patch to avoid index rebuilds]
> >>
> >> With respect to the documentation hunks, it seems to me that the first
> >> hunk might be made clearer by leaving the paragraph of which it is a
> >> part as-is, and adding another paragraph afterwards beginning with the
> >> words "In addition".
> >
> > The added restriction elaborates on the transitivity requirement, so I wanted to
> > keep the new language adjacent to that.
>
> That makes sense, but it reads a bit awkwardly to me. Maybe it's just
> that the sentence itself is so complicated that I have difficulty
> understanding it. I guess it's the same problem as with the text you
> added about hash indexes: without thinking about it, it's hard to
> understand what territory is covered by the new sentence that is not
> covered by what's already there. In the case of the hash indexes, I
> think I have it figured out: we already know that we must get
> compatible hash values out of logically equal values of different
> datatypes. But it's possible that the inter-type cast changes the
> value in some arbitrary way and then compensates for it by defining
> the hash function in such a way as to compensate. Similarly, for
> btrees, we need the relative ordering of A and B to remain the same
> after casting within the opfamily, not to be rearranged somehow.
> Maybe the text you've got is OK to explain this, but I wonder if
> there's a way to do it more simply.

That's basically right, I think. Presently, there is no connection between
casts and operator family notions of equality. For example, a cast can change
the hash value. In general, that's not wrong. However, I wish to forbid it
when some hash operator family covers both the source and destination types of
the cast. Note that, I don't especially care whether the stored bits changed or
not. I just want casts to preserve equality when an operator family defines
equality across the types involved in the cast. The specific way of
articulating that is probably vulnerable to improvement.

> > It would be valuable to avoid introducing a second chunk of code that knows
> > everything about the catalog entries behind an index. ?That's what led me to the
> > put forward the most recent version as best. ?What do you find vile about that
> > approach? ?I wasn't comfortable with it at first, because I suspected the checks
> > in RelationPreserveStorage() might be important for correctness. ?Having studied
> > it some more, though, I think they just reflect the narrower needs of its
> > current sole user.
>
> Maybe vile is a strong word, but it seems like a modularity violation:
> we're basically letting DefineIndex() do some stuff we don't really
> want done, and then backing it out parts of it that we don't really
> want done after all. It seems like it would be better to provide
> DefineIndex with enough information not to do the wrong thing in the
> first place. Could we maybe pass stmt->oldNode to DefineIndex(), and
> let it work out the details?

True. I initially shied away from that, because we assume somewhat deep into
the stack that the new relation will have pg_class.oid = pg_class.relfilenode.
Here's the call stack in question:

RelationBuildLocalRelation
heap_create
index_create
DefineIndex
ATExecAddIndex

Looking at it again, it wouldn't bring the end of the world to add a relfilenode
argument to each. None of those have more than four callers. ATExecAddIndex()
would then call RelationPreserveStorage() before calling DefineIndex(), which
would in turn put things in a correct state from the start. Does that seem
appropriate? Offhand, I do like it better than what I had.

Thanks,
nm

On Tue, Jun 28, 2011 at 3:40 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Tue, Jun 28, 2011 at 02:11:11PM -0400, Robert Haas wrote:
>> On Mon, Jun 27, 2011 at 10:43 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> > On Mon, Jun 27, 2011 at 03:45:43PM -0400, Robert Haas wrote:
>> >> On Wed, Jun 15, 2011 at 1:03 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> >> > [patch to avoid index rebuilds]
>> >>
>> >> With respect to the documentation hunks, it seems to me that the first
>> >> hunk might be made clearer by leaving the paragraph of which it is a
>> >> part as-is, and adding another paragraph afterwards beginning with the
>> >> words "In addition".
>> >
>> > The added restriction elaborates on the transitivity requirement, so I wanted to
>> > keep the new language adjacent to that.
>>
>> That makes sense, but it reads a bit awkwardly to me.  Maybe it's just
>> that the sentence itself is so complicated that I have difficulty
>> understanding it.  I guess it's the same problem as with the text you
>> added about hash indexes: without thinking about it, it's hard to
>> understand what territory is covered by the new sentence that is not
>> covered by what's already there.  In the case of the hash indexes, I
>> think I have it figured out: we already know that we must get
>> compatible hash values out of logically equal values of different
>> datatypes.  But it's possible that the inter-type cast changes the
>> value in some arbitrary way and then compensates for it by defining
>> the hash function in such a way as to compensate.  Similarly, for
>> btrees, we need the relative ordering of A and B to remain the same
>> after casting within the opfamily, not to be rearranged somehow.
>> Maybe the text you've got is OK to explain this, but I wonder if
>> there's a way to do it more simply.
>
> That's basically right, I think.  Presently, there is no connection between
> casts and operator family notions of equality.  For example, a cast can change
> the hash value.  In general, that's not wrong.  However, I wish to forbid it
> when some hash operator family covers both the source and destination types of
> the cast.  Note that, I don't especially care whether the stored bits changed or
> not.  I just want casts to preserve equality when an operator family defines
> equality across the types involved in the cast.  The specific way of
> articulating that is probably vulnerable to improvement.
>
>> > It would be valuable to avoid introducing a second chunk of code that knows
>> > everything about the catalog entries behind an index. ?That's what led me to the
>> > put forward the most recent version as best. ?What do you find vile about that
>> > approach? ?I wasn't comfortable with it at first, because I suspected the checks
>> > in RelationPreserveStorage() might be important for correctness. ?Having studied
>> > it some more, though, I think they just reflect the narrower needs of its
>> > current sole user.
>>
>> Maybe vile is a strong word, but it seems like a modularity violation:
>> we're basically letting DefineIndex() do some stuff we don't really
>> want done, and then backing it out parts of it that we don't really
>> want done after all.  It seems like it would be better to provide
>> DefineIndex with enough information not to do the wrong thing in the
>> first place.  Could we maybe pass stmt->oldNode to DefineIndex(), and
>> let it work out the details?
>
> True.  I initially shied away from that, because we assume somewhat deep into
> the stack that the new relation will have pg_class.oid = pg_class.relfilenode.
> Here's the call stack in question:
>
>        RelationBuildLocalRelation
>        heap_create
>        index_create
>        DefineIndex
>        ATExecAddIndex
>
> Looking at it again, it wouldn't bring the end of the world to add a relfilenode
> argument to each. None of those have more than four callers.

Yeah. Those functions take an awful lot of arguments, which suggests
that some refactoring might be in order, but I still think it's
cleaner to add another argument than to change the state around
after-the-fact.

> ATExecAddIndex()
> would then call RelationPreserveStorage() before calling DefineIndex(), which
> would in turn put things in a correct state from the start.  Does that seem
> appropriate?  Offhand, I do like it better than what I had.

I wish we could avoid the whole death-and-resurrection thing
altogether, but off-hand I'm not seeing a real clean way to do that.
At the very least we had better comment it to death.

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

On Wed, Jun 29, 2011 at 09:42:06AM -0400, Robert Haas wrote:
> On Tue, Jun 28, 2011 at 3:40 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:

> > Here's the call stack in question:
> >
> > ? ? ? ?RelationBuildLocalRelation
> > ? ? ? ?heap_create
> > ? ? ? ?index_create
> > ? ? ? ?DefineIndex
> > ? ? ? ?ATExecAddIndex
> >
> > Looking at it again, it wouldn't bring the end of the world to add a relfilenode
> > argument to each. None of those have more than four callers.
>
> Yeah. Those functions take an awful lot of arguments, which suggests
> that some refactoring might be in order, but I still think it's
> cleaner to add another argument than to change the state around
> after-the-fact.
>
> > ATExecAddIndex()
> > would then call RelationPreserveStorage() before calling DefineIndex(), which
> > would in turn put things in a correct state from the start. ?Does that seem
> > appropriate? ?Offhand, I do like it better than what I had.
>
> I wish we could avoid the whole death-and-resurrection thing
> altogether, but off-hand I'm not seeing a real clean way to do that.
> At the very least we had better comment it to death.

I couldn't think of a massive amount to say about that, but see what you think
of this level of commentary.

Looking at this again turned up a live bug in the previous version: if the old
index file were created in the current transaction, we would wrongly remove its
delete-at-abort entry as well as its delete-at-commit entry. This leaked the
disk file. Fixed by adding an argument to RelationPreserveStorage() indicating
which kind to remove. Test case:

BEGIN;
CREATE TABLE t AS SELECT * FROM generate_series(1,100000) t(n);
CREATE INDEX ti ON t(n);
SELECT pg_relation_filepath('ti');
ALTER TABLE t ALTER n TYPE int;
ROLLBACK;
CHECKPOINT;
-- file named above should be gone

I also updated the ATPostAlterTypeCleanup() variable names per discussion and
moved IndexStmt.oldNode to a more-natural position in the structure.

Thanks,
nm

On Thu, Jun 30, 2011 at 11:55 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Wed, Jun 29, 2011 at 09:42:06AM -0400, Robert Haas wrote:
>> On Tue, Jun 28, 2011 at 3:40 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>
>> > Here's the call stack in question:
>> >
>> > ? ? ? ?RelationBuildLocalRelation
>> > ? ? ? ?heap_create
>> > ? ? ? ?index_create
>> > ? ? ? ?DefineIndex
>> > ? ? ? ?ATExecAddIndex
>> >
>> > Looking at it again, it wouldn't bring the end of the world to add a relfilenode
>> > argument to each. None of those have more than four callers.
>>
>> Yeah.  Those functions take an awful lot of arguments, which suggests
>> that some refactoring might be in order, but I still think it's
>> cleaner to add another argument than to change the state around
>> after-the-fact.
>>
>> > ATExecAddIndex()
>> > would then call RelationPreserveStorage() before calling DefineIndex(), which
>> > would in turn put things in a correct state from the start. ?Does that seem
>> > appropriate? ?Offhand, I do like it better than what I had.
>>
>> I wish we could avoid the whole death-and-resurrection thing
>> altogether, but off-hand I'm not seeing a real clean way to do that.
>> At the very least we had better comment it to death.
>
> I couldn't think of a massive amount to say about that, but see what you think
> of this level of commentary.
>
> Looking at this again turned up a live bug in the previous version: if the old
> index file were created in the current transaction, we would wrongly remove its
> delete-at-abort entry as well as its delete-at-commit entry.  This leaked the
> disk file.  Fixed by adding an argument to RelationPreserveStorage() indicating
> which kind to remove.  Test case:
>
>        BEGIN;
>        CREATE TABLE t AS SELECT * FROM generate_series(1,100000) t(n);
>        CREATE INDEX ti ON t(n);
>        SELECT pg_relation_filepath('ti');
>        ALTER TABLE t ALTER n TYPE int;
>        ROLLBACK;
>        CHECKPOINT;
>        -- file named above should be gone
>
> I also updated the ATPostAlterTypeCleanup() variable names per discussion and
> moved IndexStmt.oldNode to a more-natural position in the structure.

On first blush, that looks a whole lot cleaner. I'll try to find some
time for a more detailed review soon.

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

On Thu, Jun 30, 2011 at 1:02 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On first blush, that looks a whole lot cleaner.  I'll try to find some
> time for a more detailed review soon.

This seems not to compile for me:

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wformat-security
-fno-strict-aliasing -fwrapv -g -Werror -I../../../src/include
-I/opt/local/include -c -o index.o index.c -MMD -MP -MF
.deps/index.Po
index.c:692: error: conflicting types for ‘index_create’
../../../src/include/catalog/index.h:53: error: previous declaration
of ‘index_create’ was here
cc1: warnings being treated as errors
index.c: In function ‘index_create’:
index.c:821: warning: passing argument 5 of ‘heap_create’ makes
integer from pointer without a cast
index.c:821: warning: passing argument 6 of ‘heap_create’ makes
pointer from integer without a cast
index.c:821: error: too few arguments to function ‘heap_create’

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

On Wed, Jul 06, 2011 at 09:55:01AM -0400, Robert Haas wrote:
> On Thu, Jun 30, 2011 at 1:02 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On first blush, that looks a whole lot cleaner. ?I'll try to find some
> > time for a more detailed review soon.
>
> This seems not to compile for me:
>
> gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels -Wformat-security
> -fno-strict-aliasing -fwrapv -g -Werror -I../../../src/include
> -I/opt/local/include -c -o index.o index.c -MMD -MP -MF
> .deps/index.Po
> index.c:692: error: conflicting types for ?index_create?
> ../../../src/include/catalog/index.h:53: error: previous declaration
> of ?index_create? was here
> cc1: warnings being treated as errors
> index.c: In function ?index_create?:
> index.c:821: warning: passing argument 5 of ?heap_create? makes
> integer from pointer without a cast
> index.c:821: warning: passing argument 6 of ?heap_create? makes
> pointer from integer without a cast
> index.c:821: error: too few arguments to function ?heap_create?

Drat; fixed in this version. My local branches contain a large test battery
that I filter out of the diff before posting. This time, that filter also
removed an essential part of the patch.

Thanks,
nm

On Wed, Jul 6, 2011 at 2:50 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
> Drat; fixed in this version.  My local branches contain a large test battery
> that I filter out of the diff before posting.  This time, that filter also
> removed an essential part of the patch.

OK, I'm pretty happy with this version, with the following minor caveats:

1. I still think the documentation changes could use some
word-smithing. If I end up being the one who commits this, I'll take
a look at that as part of the commit.

2. I think it would be helpful to add a comment explaining why
CheckIndexCompatible() is calling

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

On Thu, Jul 7, 2011 at 2:43 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jul 6, 2011 at 2:50 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
>> Drat; fixed in this version.  My local branches contain a large test battery
>> that I filter out of the diff before posting.  This time, that filter also
>> removed an essential part of the patch.
>
> OK, I'm pretty happy with this version, with the following minor caveats:
>
> 1. I still think the documentation changes could use some
> word-smithing.  If I end up being the one who commits this, I'll take
> a look at that as part of the commit.
>
> 2. I think it would be helpful to add a comment explaining why
> CheckIndexCompatible() is calling

Woops, sorry. Hit send too soon.

...why CheckIndexCompatible() is calling ComputeIndexAttrs().

Rather than committing this immediately, I'm going to mark this "Ready
for Committer", just in case Tom or someone else wants to look this
over and weigh in.

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

On Thu, Jul 07, 2011 at 02:44:49PM -0400, Robert Haas wrote:
> On Thu, Jul 7, 2011 at 2:43 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Wed, Jul 6, 2011 at 2:50 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
> >> Drat; fixed in this version.  My local branches contain a large test battery
> >> that I filter out of the diff before posting.  This time, that filter also
> >> removed an essential part of the patch.
> >
> > OK, I'm pretty happy with this version, with the following minor caveats:
> >
> > 1. I still think the documentation changes could use some
> > word-smithing.  If I end up being the one who commits this, I'll take
> > a look at that as part of the commit.
> >
> > 2. I think it would be helpful to add a comment explaining why
> > CheckIndexCompatible() is calling
>
> Woops, sorry. Hit send too soon.
>
> ...why CheckIndexCompatible() is calling ComputeIndexAttrs().

CheckIndexCompatible() calls ComputeIndexAttrs() to resolve the new operator
classes, collations and exclusion operators for each index column. It then
checks those against the existing values for the same. I figured that was
obvious enough, but do you want a new version noting that?

> Rather than committing this immediately, I'm going to mark this "Ready
> for Committer", just in case Tom or someone else wants to look this
> over and weigh in.

Great. Thanks for reviewing.

On Thu, Jul 7, 2011 at 2:55 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
> CheckIndexCompatible() calls ComputeIndexAttrs() to resolve the new operator
> classes, collations and exclusion operators for each index column.  It then
> checks those against the existing values for the same.  I figured that was
> obvious enough, but do you want a new version noting that?

I guess one question I had was... are we depending on the fact that
ComputeIndexAttrs() performs a bunch of internal sanity checks? Or
are we just expecting those to always pass, and we're going to examine
the outputs after the fact?

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

On Thu, Jul 07, 2011 at 03:06:46PM -0400, Robert Haas wrote:
> On Thu, Jul 7, 2011 at 2:55 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
> > CheckIndexCompatible() calls ComputeIndexAttrs() to resolve the new operator
> > classes, collations and exclusion operators for each index column.  It then
> > checks those against the existing values for the same.  I figured that was
> > obvious enough, but do you want a new version noting that?
>
> I guess one question I had was... are we depending on the fact that
> ComputeIndexAttrs() performs a bunch of internal sanity checks? Or
> are we just expecting those to always pass, and we're going to examine
> the outputs after the fact?

Those checks can fail; consider an explicit operator class or collation that
does not support the destination type. At that stage, we neither rely on those
checks nor mind if they do fire. If we somehow miss the problem at that stage,
DefineIndex() will detect it later. Likewise, if we hit an error in
CheckIndexCompatible(), we would also hit it later in DefineIndex().

On Thu, Jul 7, 2011 at 3:21 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
> On Thu, Jul 07, 2011 at 03:06:46PM -0400, Robert Haas wrote:
>> On Thu, Jul 7, 2011 at 2:55 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
>> > CheckIndexCompatible() calls ComputeIndexAttrs() to resolve the new operator
>> > classes, collations and exclusion operators for each index column.  It then
>> > checks those against the existing values for the same.  I figured that was
>> > obvious enough, but do you want a new version noting that?
>>
>> I guess one question I had was... are we depending on the fact that
>> ComputeIndexAttrs() performs a bunch of internal sanity checks?  Or
>> are we just expecting those to always pass, and we're going to examine
>> the outputs after the fact?
>
> Those checks can fail; consider an explicit operator class or collation that
> does not support the destination type.  At that stage, we neither rely on those
> checks nor mind if they do fire.  If we somehow miss the problem at that stage,
> DefineIndex() will detect it later.  Likewise, if we hit an error in
> CheckIndexCompatible(), we would also hit it later in DefineIndex().

OK.

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

On Thu, Jul 7, 2011 at 3:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jul 7, 2011 at 3:21 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
>> On Thu, Jul 07, 2011 at 03:06:46PM -0400, Robert Haas wrote:
>>> On Thu, Jul 7, 2011 at 2:55 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
>>> > CheckIndexCompatible() calls ComputeIndexAttrs() to resolve the new operator
>>> > classes, collations and exclusion operators for each index column.  It then
>>> > checks those against the existing values for the same.  I figured that was
>>> > obvious enough, but do you want a new version noting that?
>>>
>>> I guess one question I had was... are we depending on the fact that
>>> ComputeIndexAttrs() performs a bunch of internal sanity checks?  Or
>>> are we just expecting those to always pass, and we're going to examine
>>> the outputs after the fact?
>>
>> Those checks can fail; consider an explicit operator class or collation that
>> does not support the destination type.  At that stage, we neither rely on those
>> checks nor mind if they do fire.  If we somehow miss the problem at that stage,
>> DefineIndex() will detect it later.  Likewise, if we hit an error in
>> CheckIndexCompatible(), we would also hit it later in DefineIndex().
>
> OK.

Committed with minor comment and documentation changes.

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

On mån, 2011-07-18 at 11:05 -0400, Robert Haas wrote:
> On Thu, Jul 7, 2011 at 3:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Thu, Jul 7, 2011 at 3:21 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
> >> On Thu, Jul 07, 2011 at 03:06:46PM -0400, Robert Haas wrote:
> >>> On Thu, Jul 7, 2011 at 2:55 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
> >>> > CheckIndexCompatible() calls ComputeIndexAttrs() to resolve the new operator
> >>> > classes, collations and exclusion operators for each index column. It then
> >>> > checks those against the existing values for the same. I figured that was
> >>> > obvious enough, but do you want a new version noting that?
> >>>
> >>> I guess one question I had was... are we depending on the fact that
> >>> ComputeIndexAttrs() performs a bunch of internal sanity checks? Or
> >>> are we just expecting those to always pass, and we're going to examine
> >>> the outputs after the fact?
> >>
> >> Those checks can fail; consider an explicit operator class or collation that
> >> does not support the destination type. At that stage, we neither rely on those
> >> checks nor mind if they do fire. If we somehow miss the problem at that stage,
> >> DefineIndex() will detect it later. Likewise, if we hit an error in
> >> CheckIndexCompatible(), we would also hit it later in DefineIndex().
> >
> > OK.
>
> Committed with minor comment and documentation changes.

Please review and fix this compiler warning:

indexcmds.c: In function ‘CheckIndexCompatible’:
indexcmds.c:126:15: warning: variable ‘amoptions’ set but not used [-Wunused-but-set-variable]

On Tue, Jul 19, 2011 at 12:24 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> Please review and fix this compiler warning:
>
> indexcmds.c: In function ‘CheckIndexCompatible’:
> indexcmds.c:126:15: warning: variable ‘amoptions’ set but not used [-Wunused-but-set-variable]

I have removed the offending variable.

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