Index AM API changes for deferability

I am reviewing the following patch:

http://archives.postgresql.org/message-id/8e2dbb700907071138y4ebe75cw81879aa513cf82a3@mail.gmail.com

In order to provide useful feedback, I would like to reach a consensus
on a possible index AM API change to make it easier to support
deferrable constraints for index access methods that enforce the
constraints themselves.

I am trying to word this question carefully, because there is a lot of
context:
* Dean Rasheed is implementing deferrable unique constraints for BTree
(in the patch linked above)
* Kenneth Marshall has indicated that he would like to implement
unique hash indexes in a way similar to the current btree
implementation:
http://archives.postgresql.org/pgsql-hackers/2009-07/msg00812.php
http://archives.postgresql.org/pgsql-hackers/2009-07/msg00834.php
* I have a patch up for review that implements more general
constraints that are enforced outside of AM-specific code, and
therefore do not require index AM changes to support deferrable
constraints:
http://archives.postgresql.org/pgsql-hackers/2009-07/msg00302.php

The btree unique code is already a "serious failure of modularity":
http://archives.postgresql.org/pgsql-hackers/2008-06/msg00427.php

So, assuming that we support all of these features together, we have two
options that I see:
1. Extend the index AM API in a manner similar to Dean's patch.
2. Try to come up with some workaround to avoid changing the AM API

I was originally leaning toward approach #2 because I saw btree as the
only index AM that needed it, so extending the API seemed a little
excessive. However, seeing as it's potentially useful for unique hash
indexes, too, I am now leaning toward approach #1.

Also, we don't have performance numbers for either my feature or a
unique constraint implemented inside the hash index AM, so we don't know
whether that's a big win to enforce the constraint in the AM-specific
code or not.

So, should we proceed assuming an index AM API change, or try to avoid
it? If we should change the AM API, is Dean's API change acceptable?

Regards,
Jeff Davis

Jeff Davis <pgsql(at)j-davis(dot)com> writes:
> So, should we proceed assuming an index AM API change, or try to avoid
> it? If we should change the AM API, is Dean's API change acceptable?

There is no reason at all to avoid an index AM API change if one is
useful. If you look at the history, we tend to change that API every
release or two anyway.

Whether this particular design is good is a different question, and
I don't have time right now to think about that. But please don't
bend the system out of shape just to avoid an API change.

regards, tom lane

2009/7/15 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> There is no reason at all to avoid an index AM API change if one is
> useful.

Thinking about this a bit more, perhaps it would be better if I added
an out parameter to the AM for the uniqueness result, rather than
overloading the return value, which is quite ugly:

bool
index_insert(Relation indexRelation,
Datum *values,
bool *isnull,
ItemPointer heap_t_ctid,
Relation heapRelation,
IndexUniqueCheck uniqueness_check,
bool *is_unique);

This would allow me to tidy up some of the code I added to
ExecInsertIndexTuples() which is a bit of a kludge to support
the hash indexes enforcing uniqueness in the future:

http://archives.postgresql.org/pgsql-hackers/2009-07/msg00812.php

Also I could then move the ereport() for unique key violations from
_bt_check_unique() into index_insert() which would allow the
Duplicate key value error patch to be non-btree-specific:

http://archives.postgresql.org/message-id/20090403161658.AFB2.52131E4D@oss.ntt.co.jp
http://archives.postgresql.org/message-id/24655.1238885884@sss.pgh.pa.us

Thoughts?

On Sat, 2009-07-18 at 11:09 +0100, Dean Rasheed wrote:
> Thinking about this a bit more, perhaps it would be better if I added
> an out parameter to the AM for the uniqueness result, rather than
> overloading the return value, which is quite ugly:

Sounds reasonable to me.

Regards,
Jeff Davis