Grokbase › Groups › PostgreSQL › pgsql-hackers › June 2011
FAQ
Previous version (at7-alt-index-opfamily.patch):
http://archives.postgresql.org/message-id/[email protected]
Design:
http://archives.postgresql.org/message-id/[email protected]

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

Search Discussions

  • Robert Haas at Jun 27, 2011 at 7:45 pm ⇧

    On Wed, Jun 15, 2011 at 1:03 AM, Noah Misch 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
  • Noah Misch at Jun 28, 2011 at 2:43 am ⇧

    On Mon, Jun 27, 2011 at 03:45:43PM -0400, Robert Haas wrote:
    On Wed, Jun 15, 2011 at 1:03 AM, Noah Misch 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/[email protected]
    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
  • Robert Haas at Jun 28, 2011 at 6:12 pm ⇧

    On Mon, Jun 27, 2011 at 10:43 PM, Noah Misch 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 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/[email protected]
    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
  • Noah Misch at Jun 28, 2011 at 7:40 pm ⇧

    On Tue, Jun 28, 2011 at 02:11:11PM -0400, Robert Haas wrote:
    On Mon, Jun 27, 2011 at 10:43 PM, Noah Misch 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 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
  • Robert Haas at Jun 29, 2011 at 1:42 pm ⇧

    On Tue, Jun 28, 2011 at 3:40 PM, Noah Misch 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 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 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
  • Noah Misch at Jun 30, 2011 at 3:55 pm ⇧

    On Wed, Jun 29, 2011 at 09:42:06AM -0400, Robert Haas wrote:
    On Tue, Jun 28, 2011 at 3:40 PM, Noah Misch 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
  • Robert Haas at Jun 30, 2011 at 5:02 pm ⇧

    On Thu, Jun 30, 2011 at 11:55 AM, Noah Misch 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 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
  • Robert Haas at Jul 6, 2011 at 1:55 pm ⇧

    On Thu, Jun 30, 2011 at 1:02 PM, Robert Haas 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
  • Noah Misch at Jul 6, 2011 at 6:51 pm ⇧

    On Wed, Jul 06, 2011 at 09:55:01AM -0400, Robert Haas wrote:
    On Thu, Jun 30, 2011 at 1:02 PM, Robert Haas 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
  • Robert Haas at Jul 7, 2011 at 6:43 pm ⇧

    On Wed, Jul 6, 2011 at 2:50 PM, Noah Misch 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
  • Robert Haas at Jul 7, 2011 at 6:44 pm ⇧

    On Thu, Jul 7, 2011 at 2:43 PM, Robert Haas wrote:
    On Wed, Jul 6, 2011 at 2:50 PM, Noah Misch 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
  • Noah Misch at Jul 7, 2011 at 6:55 pm ⇧

    On Thu, Jul 07, 2011 at 02:44:49PM -0400, Robert Haas wrote:
    On Thu, Jul 7, 2011 at 2:43 PM, Robert Haas wrote:
    On Wed, Jul 6, 2011 at 2:50 PM, Noah Misch 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.
  • Robert Haas at Jul 7, 2011 at 7:07 pm ⇧

    On Thu, Jul 7, 2011 at 2:55 PM, Noah Misch 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
  • Noah Misch at Jul 7, 2011 at 7:21 pm ⇧

    On Thu, Jul 07, 2011 at 03:06:46PM -0400, Robert Haas wrote:
    On Thu, Jul 7, 2011 at 2:55 PM, Noah Misch 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().
  • Robert Haas at Jul 7, 2011 at 7:25 pm ⇧

    On Thu, Jul 7, 2011 at 3:21 PM, Noah Misch 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 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
  • Robert Haas at Jul 18, 2011 at 3:05 pm ⇧

    On Thu, Jul 7, 2011 at 3:25 PM, Robert Haas wrote:
    On Thu, Jul 7, 2011 at 3:21 PM, Noah Misch 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 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
  • Peter Eisentraut at Jul 19, 2011 at 4:24 am ⇧

    On mån, 2011-07-18 at 11:05 -0400, Robert Haas wrote:
    On Thu, Jul 7, 2011 at 3:25 PM, Robert Haas wrote:
    On Thu, Jul 7, 2011 at 3:21 PM, Noah Misch 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 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]
  • Robert Haas at Jul 19, 2011 at 2:31 pm ⇧

    On Tue, Jul 19, 2011 at 12:24 AM, Peter Eisentraut 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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedJun 15, '11 at 5:03a
activeJul 19, '11 at 2:31p
posts19
users4
websitepostgresql.org...
irc#postgresql

4 users in discussion

Robert Haas: 11 posts Noah Misch: 4 posts Noah Misch: 3 posts Peter Eisentraut: 1 post

Content

People

Support

Translate

site design / logo © 2023 Grokbase