Refactoring speculative insertion with unique indexes a little
Currently, speculative insertion (the INSERT ... ON CONFLICT DO UPDATE
executor/storage infrastructure) uses checkUnique ==
UNIQUE_CHECK_PARTIAL for unique indexes, which is a constant
originally only used by deferred unique constraints. It occurred to me
that this has a number of disadvantages:
* It confuses separation of concerns. Pushing down this information to
the nbtree AM makes it clear why it's slightly special from a
speculative insertion point of view. For example, the nbtree AM does
not actually require "livelock insurance" (for unique indexes,
although in principle not for nbtree-based exclusion constraints,
which are possible).
* UNIQUE_CHECK_PARTIAL is not only not the same thing as
UNIQUE_CHECK_SPECULATIVE (a new constant for the enum). It's also
naturally mutually exclusive with it (since we do not and cannot
support deferred unique constraints as arbiters). Let's represent this
directly.
* It makes a conflict not detected by the pre-check always insert an
index tuple, even though that occurs after a point where it's already
been established that the pointed-to TID is doomed -- it must go on to
be super deleted. Why bother bloating the index?
I'm actually not really motivated by wanting to reduce bloat here
(that was always something that I thought was a non-issue with *any*
implemented speculative insertion prototype [1]https://wiki.postgresql.org/wiki/Value_locking#.232._.22Promise.22_heap_tuples_.28Heikki_Linnakangas.29 -- Peter Geoghegan). Rather, by actually
physically inserting an index tuple unnecessarily, the implication is
that it makes sense to do so (perhaps for roughly the same reason it
makes sense with deferred unique constraints, or some other
complicated and subtle reason.). AFAICT that implication is incorrect,
though; I see no reason why inserting that index tuple serves any
purpose, and it clearly *can* be avoided with little effort.
Attached patch updates speculative insertion along these lines.
In passing, I have make ExecInsertIndexTuples() give up when a
speculative insertion conflict is detected. Again, this is not about
bloat prevention; it's about making the code easier to understand by
not doing something that is unnecessary, and in avoiding that also
avoiding the implication that it is necessary. There are already
enough complicated interactions that *are* necessary (e.g. "livelock
avoidance" for exclusion constraints). Let us make our intent clearer.
The patch also updates the AM interface documentation (the part
covering unique indexes). It seems quite natural to me to document the
theory of operation for speculative insertion there.
Thoughts?
[1]: https://wiki.postgresql.org/wiki/Value_locking#.232._.22Promise.22_heap_tuples_.28Heikki_Linnakangas.29 -- Peter Geoghegan
--
Peter Geoghegan
Attachments:
0001-Refactor-speculative-insertion-with-unique-indexes.patchtext/x-patch; charset=US-ASCII; name=0001-Refactor-speculative-insertion-with-unique-indexes.patchDownload
From 693dc7b53d311b6a739fea0eea9c2f7147673caf Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <peter.geoghegan86@gmail.com>
Date: Tue, 2 Jun 2015 17:34:16 -0700
Subject: [PATCH] Refactor speculative insertion with unique indexes
Add a dedicated IndexUniqueCheck constant that relates to the
speculative insertion case: UNIQUE_CHECK_SPECULATIVE. This allows
btinsert() (and, in principle, any amcanunique aminsert function) to
avoid physically inserting an IndexTuple in the event of detecting a
conflict during speculative insertion's second phase. With btree, this
avoidance can occur at the critical point in _bt_doinsert() immediately
after conclusively establishing that there is a conflict, but
immediately before actually calling _bt_insertonpg() to proceed with
physical IndexTuple insertion.
It only makes sense to soldier on in the hope that the conflicts can
later be resolved for deferrable unique constraints -- speculative
inserters have no chance of working out a way to proceed without first
deleting the would-be-pointed-to heap tuple already physically inserted.
This isn't really about bloat, which seems unlikely to be a significant
problem with speculative insertion in practice. Rather, it's about
bringing clarity to how btree (or future amcanunique AMs) handle
speculative insertion. It seems quite natural to avoid a patently
unnecessary unique index physical IndexTuple insertion in the rare case
where that can currently happen. Structuring the code so that the btree
access method has some knowledge of speculative insertion callers feels
like a good way of representing its contract with the executor for
speculative insertion. Besides, it was inaccurate to assert that
UNIQUE_CHECK_PARTIAL only concerned deferrable unique indexes, and never
speculative insertion; the AM interface documentation and various
comments now recognize speculative insertion callers. Allowing the
access method documentation to falsely claim that UNIQUE_CHECK_PARTIAL
only concerned deferrable constraints could conceivably cause bugs in
third party access method code (back when UNIQUE_CHECK_PARTIAL *did*
include speculative insertion, which, of course, is no longer the case
with this commit). The access method documentation has been changed to
clarify matters, including describing the new and slightly distinct
UNIQUE_CHECK_SPECULATIVE case.
---
doc/src/sgml/indexam.sgml | 107 +++++++++++++++++++++++++++++----
src/backend/access/nbtree/nbtinsert.c | 49 +++++++++------
src/backend/executor/execIndexing.c | 36 +++++++++--
src/backend/executor/nodeModifyTable.c | 2 +-
src/include/access/genam.h | 8 +++
5 files changed, 163 insertions(+), 39 deletions(-)
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index 1c09bae..0c09977 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -199,10 +199,13 @@ aminsert (Relation indexRelation,
<para>
The function's Boolean result value is significant only when
- <literal>checkUnique</> is <literal>UNIQUE_CHECK_PARTIAL</>.
- In this case a TRUE result means the new entry is known unique, whereas
- FALSE means it might be non-unique (and a deferred uniqueness check must
- be scheduled). For other cases a constant FALSE result is recommended.
+ <literal>checkUnique</> is <literal>UNIQUE_CHECK_PARTIAL</> or
+ <literal>UNIQUE_CHECK_SPECULATIVE</>. In this case a TRUE result
+ means the new entry is known unique, whereas FALSE means it might
+ be non-unique (and a deferred uniqueness check must be scheduled,
+ or for <literal>UNIQUE_CHECK_SPECULATIVE</>, speculative insertion
+ will restart from scratch, before its conflict pre-check). For
+ other cases a constant FALSE result is recommended.
</para>
<para>
@@ -804,6 +807,34 @@ amrestrpos (IndexScanDesc scan);
for that transaction to commit or abort, and then repeat the test.
</para>
</listitem>
+ <listitem>
+ <para>
+ It may be necessary to wait on a <quote>speculative token</>
+ rather than a transaction, regardless of whether or not the
+ current insertion is a <firstterm>speculative insertion</>.
+ The procedure for handling waiting on a conflict with a
+ speculative token is largely the same as the
+ waiting-on-transaction case.
+ </para>
+ <para>
+ Speculative insertion is an executor infrastructure that allows
+ <literal>INSERT ... ON CONFLICT DO UPDATE/NOTHING</> to safely
+ maintain useful guarantees about the avoidance of
+ <quote>unprincipled deadlocks</> under high concurrency (among
+ other things). These are deadlocks that arise due to a mutual
+ dependency that is not user visible. By definition,
+ unprincipled deadlocks cannot be prevented by the user
+ reordering lock acquisition in client code, because the
+ implementation level lock acquisitions are not under the user's
+ direct control. The speculative insertion token lock is an
+ identifier that is not tied to its holders transaction
+ duration; rather, it is held (usually for only an instant)
+ during its holder's speculative insertion. Without this
+ precaution, under high concurrency <literal>INSERT ... ON
+ CONFLICT DO UPDATE</> sessions could deadlock with each other,
+ which would not be acceptable.
+ </para>
+ </listitem>
</itemizedlist>
</para>
@@ -832,15 +863,20 @@ amrestrpos (IndexScanDesc scan);
<para>
If the unique constraint is deferrable, there is additional complexity:
we need to be able to insert an index entry for a new row, but defer any
- uniqueness-violation error until end of statement or even later. To
- avoid unnecessary repeat searches of the index, the index access method
- should do a preliminary uniqueness check during the initial insertion.
- If this shows that there is definitely no conflicting live tuple, we
- are done. Otherwise, we schedule a recheck to occur when it is time to
- enforce the constraint. If, at the time of the recheck, both the inserted
- tuple and some other tuple with the same key are live, then the error
- must be reported. (Note that for this purpose, <quote>live</> actually
- means <quote>any tuple in the index entry's HOT chain is live</>.)
+ uniqueness-violation error until end of statement or even later.
+ Speculative insertion similarly avoids uniquess-violation errors.
+ To avoid unnecessary repeat searches of the index, the index access
+ method should do a preliminary uniqueness check during the initial
+ insertion for deferrable unique constraint indexes (the speculative
+ insertion case has a pre-check in advance of index insertion). If
+ this shows that there is definitely no conflicting live tuple, we
+ are done. Otherwise, we schedule a recheck to occur when it is
+ time to enforce the constraint (or for the speculative insertion
+ case, a new attempt begins, starting with a new pre-check). If, at
+ the time of the recheck, both the inserted tuple and some other
+ tuple with the same key are live, then the error must be reported.
+ (Note that for this purpose, <quote>live</> actually means
+ <quote>any tuple in the index entry's HOT chain is live</>.)
To implement this, the <function>aminsert</> function is passed a
<literal>checkUnique</> parameter having one of the following values:
@@ -901,6 +937,51 @@ amrestrpos (IndexScanDesc scan);
for the same tuple values as were used in the original insertion.
</para>
</listitem>
+ <listitem>
+ <para>
+ <literal>UNIQUE_CHECK_SPECULATIVE</> is very similar to
+ <literal>UNIQUE_CHECK_PARTIAL</>, but indicates an optimistic
+ insertion in the second stage of speculative insertion. (The
+ first stage involves the pre-check.)
+ </para>
+
+ <para>
+ <literal>UNIQUE_CHECK_SPECULATIVE</> allows the access method
+ to not proceed with <structname>IndexTuple</structname>
+ insertion when it is already clear that that cannot be useful
+ (i.e. when <function>aminsert</> is set to return FALSE,
+ making caller immediately delete its would-be inserted TID).
+ Proceeding with insertion of the caller's
+ <structname>IndexTuple</structname> regardless is harmless but
+ unnecessary. It must be harmless because there could be more
+ than one <function>aminsert</>
+ <literal>UNIQUE_CHECK_SPECULATIVE</> call for each of multiple
+ unique indexes, with all calls concerning the same TID —
+ in this case each unique index arbitrates insertion, and so can
+ individually veto the TID's insertion.
+ </para>
+
+ <para>
+ Note that speculative insertion pre-checks are not particularly
+ influenced by what might have occurred in previous iterations
+ (that is, previous unsuccessful attempts to insert or to take
+ alternative path for a user-proposed row). There is no
+ <literal>UNIQUE_CHECK_EXISTING</> style check for the
+ <literal>UNIQUE_CHECK_SPECULATIVE</> case, because speculative
+ insertion pre-checks occur without <emphasis>any</>
+ <function>aminsert</> call (at least in the first iteration of
+ a speculative insertion), and because the second stage
+ (involving an <literal>UNIQUE_CHECK_SPECULATIVE</>
+ <function>aminsert</> call) is optimistic; unlike the
+ <literal>UNIQUE_CHECK_PARTIAL</> case, the caller's TID will
+ never go on to play a useful role in locking or conflict
+ avoidance/detection. The caller will immediately resign itself
+ to a restart, having made no useful progress for its TID (or,
+ at a higher level, for the row proposed for insertion).
+ <literal>UNIQUE_CHECK_SPECULATIVE</> is otherwise equivalent to
+ <literal>UNIQUE_CHECK_PARTIAL</>.
+ </para>
+ </listitem>
</itemizedlist>
</para>
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 77c2fdf..aa5b621 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -91,17 +91,21 @@ static void _bt_vacuum_one_page(Relation rel, Buffer buffer, Relation heapRel);
* This routine is called by the public interface routine, btinsert.
* By here, itup is filled in, including the TID.
*
- * If checkUnique is UNIQUE_CHECK_NO or UNIQUE_CHECK_PARTIAL, this
- * will allow duplicates. Otherwise (UNIQUE_CHECK_YES or
- * UNIQUE_CHECK_EXISTING) it will throw error for a duplicate.
- * For UNIQUE_CHECK_EXISTING we merely run the duplicate check, and
- * don't actually insert.
+ * If checkUnique is UNIQUE_CHECK_NO, UNIQUE_CHECK_PARTIAL or
+ * UNIQUE_CHECK_SPECULATIVE, this will allow duplicates. Otherwise
+ * (UNIQUE_CHECK_YES or UNIQUE_CHECK_EXISTING) it will throw error for a
+ * duplicate. For UNIQUE_CHECK_EXISTING we merely run the duplicate
+ * check, and don't actually insert.
*
- * The result value is only significant for UNIQUE_CHECK_PARTIAL:
- * it must be TRUE if the entry is known unique, else FALSE.
- * (In the current implementation we'll also return TRUE after a
- * successful UNIQUE_CHECK_YES or UNIQUE_CHECK_EXISTING call, but
- * that's just a coding artifact.)
+ * The result value is only significant for UNIQUE_CHECK_PARTIAL
+ * and UNIQUE_CHECK_SPECULATIVE: it must be TRUE if the entry is
+ * known unique, else FALSE. (In the current implementation
+ * we'll also return TRUE after a successful UNIQUE_CHECK_YES or
+ * UNIQUE_CHECK_EXISTING call, but that's just a coding
+ * artifact.)
+ *
+ * Note that UNIQUE_CHECK_SPECULATIVE calls that return false
+ * don't actually proceed with insertion of the index tuple.
*/
bool
_bt_doinsert(Relation rel, IndexTuple itup,
@@ -188,7 +192,8 @@ top:
}
}
- if (checkUnique != UNIQUE_CHECK_EXISTING)
+ if (checkUnique != UNIQUE_CHECK_EXISTING &&
+ (checkUnique != UNIQUE_CHECK_SPECULATIVE || is_unique))
{
/*
* The only conflict predicate locking cares about for indexes is when
@@ -230,10 +235,11 @@ top:
* progress, *speculativeToken is set to non-zero, and the caller can wait for
* the verdict on the insertion using SpeculativeInsertionWait().
*
- * However, if checkUnique == UNIQUE_CHECK_PARTIAL, we always return
- * InvalidTransactionId because we don't want to wait. In this case we
- * set *is_unique to false if there is a potential conflict, and the
- * core code must redo the uniqueness check later.
+ * However, if checkUnique is UNIQUE_CHECK_PARTIAL or
+ * UNIQUE_CHECK_SPECULATIVE, we always return InvalidTransactionId because we
+ * don't want to wait. In this case we set *is_unique to false if there is a
+ * potential conflict, and the core code must redo the uniqueness check later
+ * or restart speculative insertion.
*/
static TransactionId
_bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,
@@ -331,12 +337,15 @@ _bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,
/*
* It is a duplicate. If we are only doing a partial
- * check, then don't bother checking if the tuple is being
- * updated in another transaction. Just return the fact
- * that it is a potential conflict and leave the full
- * check till later.
+ * check or speculative insertion, then don't bother
+ * checking if the tuple is being updated in another
+ * transaction. Just return the fact that it is a potential
+ * conflict. Partial check callers can leave the full check
+ * till later. Speculative insertion callers will need to
+ * restart.
*/
- if (checkUnique == UNIQUE_CHECK_PARTIAL)
+ if (checkUnique == UNIQUE_CHECK_PARTIAL ||
+ checkUnique == UNIQUE_CHECK_SPECULATIVE)
{
if (nbuf != InvalidBuffer)
_bt_relbuf(rel, nbuf);
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index bf38508..1163f1a 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -259,6 +259,14 @@ ExecCloseIndices(ResultRelInfo *resultRelInfo)
* the same is done for non-deferred constraints, but report
* if conflict was speculative or deferred conflict to caller)
*
+ * Note that the implementation does not bother with inserting
+ * into all indexes in the event of detecting a speculative
+ * insertion conflict. This should not matter, because when
+ * *specConflict is set to true, caller's heap tuple will be
+ * super-deleted; a complete list of indexes with conflicts
+ * is not useful (in fact, even the actual source of the
+ * speculative insertion conflict is not of interest).
+ *
* CAUTION: this must not be called for a HOT update.
* We can't defend against that here for lack of info.
* Should we change the API to make it safer?
@@ -374,7 +382,7 @@ ExecInsertIndexTuples(TupleTableSlot *slot,
if (!indexRelation->rd_index->indisunique)
checkUnique = UNIQUE_CHECK_NO;
else if (noDupErr && (arbiterIndexes == NIL || arbiter))
- checkUnique = UNIQUE_CHECK_PARTIAL;
+ checkUnique = UNIQUE_CHECK_SPECULATIVE;
else if (indexRelation->rd_index->indimmediate)
checkUnique = UNIQUE_CHECK_YES;
else
@@ -432,18 +440,31 @@ ExecInsertIndexTuples(TupleTableSlot *slot,
}
if ((checkUnique == UNIQUE_CHECK_PARTIAL ||
+ checkUnique == UNIQUE_CHECK_SPECULATIVE ||
indexInfo->ii_ExclusionOps != NULL) &&
!satisfiesConstraint)
{
+ if (indexRelation->rd_index->indimmediate && specConflict)
+ {
+ Assert(noDupErr);
+
+ /*
+ * Speculative conflict found, necessitating new loop for
+ * caller. Caller will now not care about specifics of
+ * conflicts if there was also a deferred constraint
+ * conflict.
+ */
+ list_free(result);
+ *specConflict = true;
+ return NIL;
+ }
+
/*
* The tuple potentially violates the uniqueness or exclusion
* constraint, so make a note of the index so that we can re-check
- * it later. Speculative inserters are told if there was a
- * speculative conflict, since that always requires a restart.
+ * it later
*/
result = lappend_oid(result, RelationGetRelid(indexRelation));
- if (indexRelation->rd_index->indimmediate && specConflict)
- *specConflict = true;
}
}
@@ -540,6 +561,11 @@ ExecCheckIndexConstraints(TupleTableSlot *slot,
errtableconstraint(heapRelation,
RelationGetRelationName(indexRelation))));
+ /*
+ * Set checkedIndex here, since partial unique index will still count
+ * as a found arbiter index despite being skipped due to predicate not
+ * being satisfied
+ */
checkedIndex = true;
/* Check for partial index */
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 874ca6a..7545e61 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -436,7 +436,7 @@ ExecInsert(ModifyTableState *mtstate,
*/
if (specConflict)
{
- list_free(recheckIndexes);
+ Assert(recheckIndexes == NIL);
goto vlock;
}
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index d86590a..bd8d344 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -98,6 +98,13 @@ typedef struct SysScanDescData *SysScanDesc;
* known unique, false if it is possibly non-unique. In the "true" case
* it is safe to omit the later recheck.
*
+ * UNIQUE_CHECK_SPECULATIVE is very similar to UNIQUE_CHECK_PARTIAL, but is
+ * used for speculative insertion (which does not support deferrable unique
+ * constraints as arbiters). The only difference is that it allows the
+ * implementation to avoid pointlessly inserting an index tuple after
+ * establishing that there is a speculative insertion conflict, where the heap
+ * tuple must be super-deleted anyway.
+ *
* When it is time to recheck the deferred constraint, a pseudo-insertion
* call is made with UNIQUE_CHECK_EXISTING. The tuple is already in the
* index in this case, so it should not be inserted again. Rather, just
@@ -108,6 +115,7 @@ typedef enum IndexUniqueCheck
UNIQUE_CHECK_NO, /* Don't do any uniqueness checking */
UNIQUE_CHECK_YES, /* Enforce uniqueness at insertion time */
UNIQUE_CHECK_PARTIAL, /* Test uniqueness, but no error */
+ UNIQUE_CHECK_SPECULATIVE, /* Speculative Insertion */
UNIQUE_CHECK_EXISTING /* Check if existing tuple is unique */
} IndexUniqueCheck;
--
1.9.1
On 06/11/2015 02:19 AM, Peter Geoghegan wrote:
Currently, speculative insertion (the INSERT ... ON CONFLICT DO UPDATE
executor/storage infrastructure) uses checkUnique ==
UNIQUE_CHECK_PARTIAL for unique indexes, which is a constant
originally only used by deferred unique constraints. It occurred to me
that this has a number of disadvantages:* It confuses separation of concerns. Pushing down this information to
the nbtree AM makes it clear why it's slightly special from a
speculative insertion point of view. For example, the nbtree AM does
not actually require "livelock insurance" (for unique indexes,
although in principle not for nbtree-based exclusion constraints,
which are possible).* UNIQUE_CHECK_PARTIAL is not only not the same thing as
UNIQUE_CHECK_SPECULATIVE (a new constant for the enum). It's also
naturally mutually exclusive with it (since we do not and cannot
support deferred unique constraints as arbiters). Let's represent this
directly.* It makes a conflict not detected by the pre-check always insert an
index tuple, even though that occurs after a point where it's already
been established that the pointed-to TID is doomed -- it must go on to
be super deleted. Why bother bloating the index?I'm actually not really motivated by wanting to reduce bloat here
(that was always something that I thought was a non-issue with *any*
implemented speculative insertion prototype [1]). Rather, by actually
physically inserting an index tuple unnecessarily, the implication is
that it makes sense to do so (perhaps for roughly the same reason it
makes sense with deferred unique constraints, or some other
complicated and subtle reason.). AFAICT that implication is incorrect,
though; I see no reason why inserting that index tuple serves any
purpose, and it clearly *can* be avoided with little effort.
I agree it would be cleaner to have a separate CHECK_UNIQUE_XXX code for
speculative insertions. You've defined CHECK_UNIQUE_SPECULATIVE as "like
CHECK_UNIQUE_PARTIAL, but you don't have to insert the index tuple if
there's a conflict". I think it'd be better to define it as "like
CHECK_UNIQUE_YES, but return FALSE instead of throwing an error on
conflict". The difference is that the aminsert would not be allowed to
return FALSE when there is no conflict.
Actually, even without this patch, a dummy implementation of aminsert
that always returns FALSE in CHECK_UNIQUE_PARTIAL mode would be legal
per the docs, but would loop forever. So that would be nice to fix or
document away, even though it's not a problem for B-tree currently.
Attached patch updates speculative insertion along these lines.
In passing, I have make ExecInsertIndexTuples() give up when a
speculative insertion conflict is detected. Again, this is not about
bloat prevention; it's about making the code easier to understand by
not doing something that is unnecessary, and in avoiding that also
avoiding the implication that it is necessary. There are already
enough complicated interactions that *are* necessary (e.g. "livelock
avoidance" for exclusion constraints). Let us make our intent clearer.
Hmm, not sure I like those changes. ExecInsertIndexTuples's behaviour
now depends on whether specConflict is passed, but that seems weird as
specConflict is an output parameter.
The patch also updates the AM interface documentation (the part
covering unique indexes). It seems quite natural to me to document the
theory of operation for speculative insertion there.
Yeah, although I find the explanation pretty long-winded and difficult
to understand ;-).
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jun 30, 2015 at 11:19 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I agree it would be cleaner to have a separate CHECK_UNIQUE_XXX code for
speculative insertions. You've defined CHECK_UNIQUE_SPECULATIVE as "like
CHECK_UNIQUE_PARTIAL, but you don't have to insert the index tuple if
there's a conflict". I think it'd be better to define it as "like
CHECK_UNIQUE_YES, but return FALSE instead of throwing an error on
conflict". The difference is that the aminsert would not be allowed to
return FALSE when there is no conflict.
Suppose we do it that way. Then what's the difference between
CHECK_UNIQUE_SPECULATIVE and CHECK_UNIQUE_PARTIAL? You've just
effectively required the CHECK_UNIQUE_YES case to not physically
insert a physical tuple before throwing an error, which does not seem
essential to the existing definition of CHECK_UNIQUE_YES -- you've
redefined CHECK_UNIQUE_YES in a way that nbtree happens to meet at the
moment. If we had an amcanunique AM that worked a bit like exclusion
constraints, this new obligation for CHECK_UNIQUE_YES might make it
impossible for that to work.
I'm not necessarily in disagreement. I just didn't do it that way for
that reason.
Actually, even without this patch, a dummy implementation of aminsert that
always returns FALSE in CHECK_UNIQUE_PARTIAL mode would be legal per the
docs, but would loop forever. So that would be nice to fix or document away,
even though it's not a problem for B-tree currently.
UNIQUE_CHECK_PARTIAL calls to an aminsert function are allowed to
report false positives (of not being unique, by returning FALSE as you
say), where there may not have been a conflict, but it's not clear
(e.g. because the conflicting xact has yet to commit/abort). You still
need to insert, though, so that the recheck at end-of-xact works for
deferred constraints. At that point, it's really not too different to
ordinary unique enforcement, except that the other guy is guaranteed
to notice you, too.
You can construct a theoretical case where lock starvation occurs with
unique constraint enforcement. I think it helps with nbtree here that
someone will reliably *not* see a conflict when concurrently
inserting, because unique index "value locking" exists (by locking the
first leaf page a value could be on with a buffer lock). But even if
that wasn't the case, the insert + recheck thing would be safe, just
as with exclusion constraints...provided you insert to begin with,
that is.
In passing, I have make ExecInsertIndexTuples() give up when a
speculative insertion conflict is detected. Again, this is not about
bloat prevention; it's about making the code easier to understand by
not doing something that is unnecessary, and in avoiding that also
avoiding the implication that it is necessary. There are already
enough complicated interactions that *are* necessary (e.g. "livelock
avoidance" for exclusion constraints). Let us make our intent clearer.Hmm, not sure I like those changes. ExecInsertIndexTuples's behaviour now
depends on whether specConflict is passed, but that seems weird as
specConflict is an output parameter.
Your statement is ambiguous and confusing to me. Are you objecting to
the idea of returning from ExecInsertIndexTuples() "early" in general,
or to one narrow aspect of how that was proposed -- the fact that it
occurs due to "specConflict != NULL" rather than "noDupErr"? Obviously
if it's just the latter, then that's a small adjustment. But AFAICT
your remark about specConflict could one detail of a broader complaint
about an idea that you dislike generally.
The patch also updates the AM interface documentation (the part
covering unique indexes). It seems quite natural to me to document the
theory of operation for speculative insertion there.Yeah, although I find the explanation pretty long-winded and difficult to
understand ;-).
I don't know what you mean. You say things like this to me fairly
regularly, but I'm always left wondering what the take-away should be.
Please be more specific.
Long-winded generally means that more words than necessary were used.
I think that the documentation proposed is actually very information
dense, if anything. As always, I aimed to keep it consistent with the
surrounding documentation.
I understand that the material might be a little hard to follow, but
it concerns how someone can externally implement a Postgres index
access method that is "amcanunique". That's a pretty esoteric subject
-- so far, we've had exactly zero takers (even without the
"amcanunique" part). It's damn hard to make these ideas accessible.
I feel that I have an obligation to share information about (say) how
the speculative insertion mechanism works -- the "theory of operation"
seems very useful. Like any one of us, I might not be around to
provide input on something like that in the future, so it makes sense
to be thorough and unambiguous. There are very few people (3?) who
have a good sense of how it works currently, and there are some very
subtle aspects to it that I tried to point out.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jul 1, 2015 at 10:45 AM, Peter Geoghegan <pg@heroku.com> wrote:
You can construct a theoretical case where lock starvation occurs with
unique constraint enforcement. I think it helps with nbtree here that
someone will reliably *not* see a conflict when concurrently
inserting, because unique index "value locking" exists (by locking the
first leaf page a value could be on with a buffer lock). But even if
that wasn't the case, the insert + recheck thing would be safe, just
as with exclusion constraints...provided you insert to begin with,
that is.
Of course, the fact that UNIQUE_CHECK_PARTIAL aminsert callers
(including deferred constraint inserters and, currently, speculative
inserters) can rely on this is very useful to UPSERT. It guarantees
progress of one session without messy exclusion constraint style fixes
(for deadlock and livelock avoidance). You cannot talk about
speculative insertion without talking about unprincipled deadlocks
(and maybe livelocks).
If I had to bet where we might find some bugs in the executor parts of
UPSERT, my first guess would be the exclusion constraint edge-case
handling (livelocking stuff). Those are probably relatively benign
bugs, but recent bugs in exclusion constraints (in released branches)
show that they can hide for a long time.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jul 1, 2015 at 10:45 AM, Peter Geoghegan <pg@heroku.com> wrote:
On Tue, Jun 30, 2015 at 11:19 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I agree it would be cleaner to have a separate CHECK_UNIQUE_XXX code for
speculative insertions. You've defined CHECK_UNIQUE_SPECULATIVE as "like
CHECK_UNIQUE_PARTIAL, but you don't have to insert the index tuple if
there's a conflict". I think it'd be better to define it as "like
CHECK_UNIQUE_YES, but return FALSE instead of throwing an error on
conflict". The difference is that the aminsert would not be allowed to
return FALSE when there is no conflict.Suppose we do it that way. Then what's the difference between
CHECK_UNIQUE_SPECULATIVE and CHECK_UNIQUE_PARTIAL? You've just
effectively required the CHECK_UNIQUE_YES case to not physically
insert a physical tuple before throwing an error, which does not seem
essential to the existing definition of CHECK_UNIQUE_YES -- you've
redefined CHECK_UNIQUE_YES in a way that nbtree happens to meet at the
moment. If we had an amcanunique AM that worked a bit like exclusion
constraints, this new obligation for CHECK_UNIQUE_YES might make it
impossible for that to work.
Another more obvious and important thing: CHECK_UNIQUE_YES waits for
conflicts to be resolved before returning to its caller. If you don't
get an error, you're done. CHECK_UNIQUE_PARTIAL never waits, and if we
add a CHECK_UNIQUE_SPECULATIVE, it ought to not wait either.
Sure, if a speculative inserter detects a conflict, it still has to
wait. But not in the aminsert call, and not until it cleans up its
physical insertion (by super-deleting). Clearly a
CHECK_UNIQUE_SPECULATIVE would have to be much closer to
CHECK_UNIQUE_PARTIAL than to CHECK_UNIQUE_YES.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 07/01/2015 09:19 PM, Peter Geoghegan wrote:
On Wed, Jul 1, 2015 at 10:45 AM, Peter Geoghegan <pg@heroku.com> wrote:
On Tue, Jun 30, 2015 at 11:19 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I agree it would be cleaner to have a separate CHECK_UNIQUE_XXX code for
speculative insertions. You've defined CHECK_UNIQUE_SPECULATIVE as "like
CHECK_UNIQUE_PARTIAL, but you don't have to insert the index tuple if
there's a conflict". I think it'd be better to define it as "like
CHECK_UNIQUE_YES, but return FALSE instead of throwing an error on
conflict". The difference is that the aminsert would not be allowed to
return FALSE when there is no conflict.Suppose we do it that way. Then what's the difference between
CHECK_UNIQUE_SPECULATIVE and CHECK_UNIQUE_PARTIAL? You've just
effectively required the CHECK_UNIQUE_YES case to not physically
insert a physical tuple before throwing an error, which does not seem
essential to the existing definition of CHECK_UNIQUE_YES -- you've
redefined CHECK_UNIQUE_YES in a way that nbtree happens to meet at the
moment. If we had an amcanunique AM that worked a bit like exclusion
constraints, this new obligation for CHECK_UNIQUE_YES might make it
impossible for that to work.Another more obvious and important thing: CHECK_UNIQUE_YES waits for
conflicts to be resolved before returning to its caller. If you don't
get an error, you're done. CHECK_UNIQUE_PARTIAL never waits, and if we
add a CHECK_UNIQUE_SPECULATIVE, it ought to not wait either.Sure, if a speculative inserter detects a conflict, it still has to
wait. But not in the aminsert call, and not until it cleans up its
physical insertion (by super-deleting). Clearly a
CHECK_UNIQUE_SPECULATIVE would have to be much closer to
CHECK_UNIQUE_PARTIAL than to CHECK_UNIQUE_YES.
Why is it not OK for aminsert to do the waiting, with
CHECK_UNIQUE_SPECULATIVE?
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jul 2, 2015 at 1:30 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Sure, if a speculative inserter detects a conflict, it still has to
wait. But not in the aminsert call, and not until it cleans up its
physical insertion (by super-deleting). Clearly a
CHECK_UNIQUE_SPECULATIVE would have to be much closer to
CHECK_UNIQUE_PARTIAL than to CHECK_UNIQUE_YES.Why is it not OK for aminsert to do the waiting, with
CHECK_UNIQUE_SPECULATIVE?
Well, waiting means getting a ShareLock on the other session's XID.
You can't do that without first releasing your locks, unless you're
okay with unprincipled deadlocks.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jul 2, 2015 at 10:49 AM, Peter Geoghegan <pg@heroku.com> wrote:
Well, waiting means getting a ShareLock on the other session's XID.
You can't do that without first releasing your locks, unless you're
okay with unprincipled deadlocks.
Besides, if the other session wins the race and inserts a physical
heap tuple, but then aborts its xact, we might have to insert again
(although not before super-deleting), with that insert going on to be
successful.
As with row locking, with insertion, if there is a conflict, any
outcome (UPDATE or INSERT) is then possible.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jul 2, 2015 at 2:58 PM, Peter Geoghegan <pg@heroku.com> wrote:
As with row locking, with insertion, if there is a conflict, any
outcome (UPDATE or INSERT) is then possible.
Where are we on this? It would be nice to get this out of the way
before a 9.5 beta is put out.
Thanks
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
On 2015-06-10 16:19:27 -0700, Peter Geoghegan wrote:
Currently, speculative insertion (the INSERT ... ON CONFLICT DO UPDATE
executor/storage infrastructure) uses checkUnique ==
UNIQUE_CHECK_PARTIAL for unique indexes, which is a constant
originally only used by deferred unique constraints. It occurred to me
that this has a number of disadvantages:
...
Attached patch updates speculative insertion along these lines.
...
The patch also updates the AM interface documentation (the part
covering unique indexes). It seems quite natural to me to document the
theory of operation for speculative insertion there.
I'm not arguing against any of this - but I don't think this needs to be
on the 9.5 open items list. I plan to remove from there.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Oct 3, 2015 at 6:38 AM, Andres Freund <andres@anarazel.de> wrote:
I'm not arguing against any of this - but I don't think this needs to be
on the 9.5 open items list. I plan to remove from there.
Obviously I don't think that this is a critical fix. I do think that
it would be nice to keep the branches in sync, and that might become a
bit more difficult after 9.5 is released.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Oct 4, 2015 at 2:07 AM, Peter Geoghegan <pg@heroku.com> wrote:
On Sat, Oct 3, 2015 at 6:38 AM, Andres Freund <andres@anarazel.de> wrote:
I'm not arguing against any of this - but I don't think this needs to be
on the 9.5 open items list. I plan to remove from there.Obviously I don't think that this is a critical fix. I do think that
it would be nice to keep the branches in sync, and that might become a
bit more difficult after 9.5 is released.
(A couple of months later)
This is not an actual fix, but an optimization, no?
UNIQUE_CHECK_SPECULATIVE is just used to optimize a couple of code
paths in the case of a insert conflicting during btree insertion..
In any case, at this point 9.5 is really aimed to be stabilized, so
targeting only master is a far saner approach IMO for this patch.
Pushing that in 9.5 a couple of months back may have given enough
reason to do so... But well life is life.
+ * it later
Missing a dot here :)
+ * Set checkedIndex here, since partial unique index
will still count
+ * as a found arbiter index despite being skipped due
to predicate not
+ * being satisfied
Ditto.
--
Michael--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Dec 16, 2015 at 11:20 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
(A couple of months later)
This is not an actual fix, but an optimization, no?
UNIQUE_CHECK_SPECULATIVE is just used to optimize a couple of code
paths in the case of a insert conflicting during btree insertion..In any case, at this point 9.5 is really aimed to be stabilized, so
targeting only master is a far saner approach IMO for this patch.
Pushing that in 9.5 a couple of months back may have given enough
reason to do so... But well life is life.
No, this really isn't an optimization at all. It has nothing to do
with performance, since I think that unnecessarily inserting an index
tuple in practice has very little or no appreciable overhead.
The point of avoiding that is that it makes no sense, while doing it
implies that it does make sense. (i.e. It does not make sense to
insert into a B-Tree leaf page an IndexTuple pointing to a speculative
heap tuple with the certain knowledge that we'll have to super-delete
the speculative heap tuple in the end anyway).
This is 100% about clarifying the intent and design of the code.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Dec 16, 2015 at 11:44 PM, Peter Geoghegan <pg@heroku.com> wrote:
In any case, at this point 9.5 is really aimed to be stabilized, so
targeting only master is a far saner approach IMO for this patch.
Pushing that in 9.5 a couple of months back may have given enough
reason to do so... But well life is life.No, this really isn't an optimization at all.
I should add: I think that the chances of this patch destabilizing the
code are very slim, once it receives the proper review. Certainly, I
foresee no possible downside to not inserting the doomed IndexTuple,
since it's guaranteed to have its heap tuple super-deleted immediately
afterwards.
That's the only real behavioral change proposed here. So, I would
prefer it if we got this in before the first stable release of 9.5.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Dec 17, 2015 at 4:55 PM, Peter Geoghegan <pg@heroku.com> wrote:
On Wed, Dec 16, 2015 at 11:44 PM, Peter Geoghegan <pg@heroku.com> wrote:
In any case, at this point 9.5 is really aimed to be stabilized, so
targeting only master is a far saner approach IMO for this patch.
Pushing that in 9.5 a couple of months back may have given enough
reason to do so... But well life is life.No, this really isn't an optimization at all.
I should add: I think that the chances of this patch destabilizing the
code are very slim, once it receives the proper review. Certainly, I
foresee no possible downside to not inserting the doomed IndexTuple,
since it's guaranteed to have its heap tuple super-deleted immediately
afterwards.
I am no committer, just a guy giving an opinion about this patch and I
have to admit that I have not enough studied the speculative insertion
code to have a clear technical point of view on the matter, but even
if the chances of destabilizing the code are slim, it does not seem a
wise idea to me to do that post-rc1 and before a GA as there are
people testing the code as it is now. Now per the two points listed in
the first sentence in this paragraph, perhaps this opinion is fine as
moot :) I didn't get much involved in the development of this code
after all.
That's the only real behavioral change proposed here. So, I would
prefer it if we got this in before the first stable release of 9.5.
Yeah, I got this one.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Dec 17, 2015 at 11:06 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
I should add: I think that the chances of this patch destabilizing the
code are very slim, once it receives the proper review. Certainly, I
foresee no possible downside to not inserting the doomed IndexTuple,
since it's guaranteed to have its heap tuple super-deleted immediately
afterwards.I am no committer, just a guy giving an opinion about this patch and I
have to admit that I have not enough studied the speculative insertion
code to have a clear technical point of view on the matter, but even
if the chances of destabilizing the code are slim, it does not seem a
wise idea to me to do that post-rc1 and before a GA as there are
people testing the code as it is now.
You can express doubt about almost anything. In this case, you haven't
voiced any particular concern about any particular risk. The risk of
not proceeding is that 9.5 will remain in a divergent state for no
reason, with substantial differences in the documentation of the AM
interface, and that has a cost. Why should the risk of destabilizing
9.5 become more palatable when 9.5 has been out for 6 months or a
year? You can take it that this will probably be now-or-never.
This is mostly just comment changes, and changes to documentation. If
it comes down to it, we could leave the existing 9.5 code intact (i.e.
still unnecessarily insert the IndexTuple), while commenting that it
is unnecessary, and still changing everything else. That would have an
unquantifiably tiny risk, certainly smaller than the risk of
committing the patch as-is (which, to reiterate, is a risk that I
think is very small).
FWIW, I tend to doubt that users have tested speculative insertion/ON
CONFLICT much this whole time. There were a couple of bug reports, but
that's it.
Now per the two points listed in
the first sentence in this paragraph, perhaps this opinion is fine as
moot :) I didn't get much involved in the development of this code
after all.
I'm concerned that Heikki's apparent unavailability will become a
blocker to this.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Dec 18, 2015 at 4:31 PM, Peter Geoghegan <pg@heroku.com> wrote:
On Thu, Dec 17, 2015 at 11:06 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:Now per the two points listed in
the first sentence in this paragraph, perhaps this opinion is fine as
moot :) I didn't get much involved in the development of this code
after all.I'm concerned that Heikki's apparent unavailability will become a
blocker to this.
Yeah, not only this patch... The second committer with enough
background on the matter is Andres.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Dec 17, 2015 at 2:55 AM, Peter Geoghegan <pg@heroku.com> wrote:
On Wed, Dec 16, 2015 at 11:44 PM, Peter Geoghegan <pg@heroku.com> wrote:
In any case, at this point 9.5 is really aimed to be stabilized, so
targeting only master is a far saner approach IMO for this patch.
Pushing that in 9.5 a couple of months back may have given enough
reason to do so... But well life is life.No, this really isn't an optimization at all.
I should add: I think that the chances of this patch destabilizing the
code are very slim, once it receives the proper review. Certainly, I
foresee no possible downside to not inserting the doomed IndexTuple,
since it's guaranteed to have its heap tuple super-deleted immediately
afterwards.That's the only real behavioral change proposed here. So, I would
prefer it if we got this in before the first stable release of 9.5.
No, it's far too late to be pushing this into 9.5. We are at RC1 now
and hoping to cut a final release right after Christmas. I think it's
quite wrong to argue that these changes have no risk of destabilizing
9.5. Nobody is exempt from having bugs in their code - not me, not
you, not Tom Lane. But quite apart from that, there seems to be no
compelling benefit to having these changes in 9.5. You say that the
branches will diverge needlessly, but the whole point of having
branches is that we do need things to diverge. The question isn't
"why shouldn't these go into 9.5?" but "do these fix something that is
clearly broken in 9.5 and must be fixed to avoid hurting users?".
Andres has said clearly that he doesn't think so, and Heikki didn't
seem convinced that we wanted the changes at all. I've read over the
thread and I think that even if all the good things you say about this
patch are 100% true, it doesn't amount to a good reason to back-patch.
Code that does something possibly non-sensical or sub-optimal isn't a
reason to back-patch in the absence of a clear, user-visible
consequence.
I think it's a shame that we haven't gotten this patch dealt with just
because when somebody submits a patch in June, it's not very nice for
it to still be pending in December, but since this stuff is even
further outside my area of expertise than the sorting stuff, and since
me and my split personalities only have so many hours in the day, I'm
going to have to leave it to somebody else to pick up anyhow. But
that's a separate issue from whether this should be back-patched.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Dec 18, 2015 at 8:58 AM, Robert Haas <robertmhaas@gmail.com> wrote:
No, it's far too late to be pushing this into 9.5. We are at RC1 now
and hoping to cut a final release right after Christmas. I think it's
quite wrong to argue that these changes have no risk of destabilizing
9.5. Nobody is exempt from having bugs in their code - not me, not
you, not Tom Lane. But quite apart from that, there seems to be no
compelling benefit to having these changes in 9.5. You say that the
branches will diverge needlessly, but the whole point of having
branches is that we do need things to diverge. The question isn't
"why shouldn't these go into 9.5?" but "do these fix something that is
clearly broken in 9.5 and must be fixed to avoid hurting users?".
Andres has said clearly that he doesn't think so, and Heikki didn't
seem convinced that we wanted the changes at all.
It isn't true that Heikki was not basically in favor of this. This
should have been committed as part of the original patch, really.
I hope to avoid needless confusion about the documented (by the
official documentation) AM interface. Yes, that is
I think it's a shame that we haven't gotten this patch dealt with just
because when somebody submits a patch in June, it's not very nice for
it to still be pending in December, but since this stuff is even
further outside my area of expertise than the sorting stuff, and since
me and my split personalities only have so many hours in the day, I'm
going to have to leave it to somebody else to pick up anyhow. But
that's a separate issue from whether this should be back-patched.
Note that I've already proposed a compromise, even though I don't
think my original position was at all unreasonable. There'd be zero
real changes (only the addition of the new constant name,
documentation updates, comment updates, etc) under that compromise (as
against one change.).
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Dec 18, 2015 at 2:04 PM, Peter Geoghegan <pg@heroku.com> wrote:
It isn't true that Heikki was not basically in favor of this. This
should have been committed as part of the original patch, really.
Maybe he wasn't against the whole thing, but he's posted two messages
to this thread and they can't be read as unequivocally in favor of
these changes. He clearly didn't like at least some of it.
I hope to avoid needless confusion about the documented (by the
official documentation) AM interface. Yes, that is
Something maybe got cut off here?
I think it's a shame that we haven't gotten this patch dealt with just
because when somebody submits a patch in June, it's not very nice for
it to still be pending in December, but since this stuff is even
further outside my area of expertise than the sorting stuff, and since
me and my split personalities only have so many hours in the day, I'm
going to have to leave it to somebody else to pick up anyhow. But
that's a separate issue from whether this should be back-patched.Note that I've already proposed a compromise, even though I don't
think my original position was at all unreasonable. There'd be zero
real changes (only the addition of the new constant name,
documentation updates, comment updates, etc) under that compromise (as
against one change.).
I only see one patch version on the thread.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Dec 18, 2015 at 12:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Dec 18, 2015 at 2:04 PM, Peter Geoghegan <pg@heroku.com> wrote:
It isn't true that Heikki was not basically in favor of this. This
should have been committed as part of the original patch, really.Maybe he wasn't against the whole thing, but he's posted two messages
to this thread and they can't be read as unequivocally in favor of
these changes. He clearly didn't like at least some of it.
The issues were very trivial.
I only see one patch version on the thread.
I'm not going to post a revision until I thrash out the tiny issues
with Heikki. He kind of trailed off. So maybe that kills it
immediately, which is a shame.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Dec 19, 2015 at 1:58 AM, Robert Haas wrote:
No, it's far too late to be pushing this into 9.5. We are at RC1 now
and hoping to cut a final release right after Christmas. I think it's
quite wrong to argue that these changes have no risk of destabilizing
9.5. Nobody is exempt from having bugs in their code - not me, not
you, not Tom Lane. But quite apart from that, there seems to be no
compelling benefit to having these changes in 9.5. You say that the
branches will diverge needlessly, but the whole point of having
branches is that we do need things to diverge. The question isn't
"why shouldn't these go into 9.5?" but "do these fix something that is
clearly broken in 9.5 and must be fixed to avoid hurting users?".
Andres has said clearly that he doesn't think so, and Heikki didn't
seem convinced that we wanted the changes at all. I've read over the
thread and I think that even if all the good things you say about this
patch are 100% true, it doesn't amount to a good reason to back-patch.
Code that does something possibly non-sensical or sub-optimal isn't a
reason to back-patch in the absence of a clear, user-visible
consequence.
+1. There are folks around doing tests using 9.5 now, it is not
correct to impact the effort they have been putting on it until now.
I think it's a shame that we haven't gotten this patch dealt with just
because when somebody submits a patch in June, it's not very nice for
it to still be pending in December, but since this stuff is even
further outside my area of expertise than the sorting stuff, and since
me and my split personalities only have so many hours in the day, I'm
going to have to leave it to somebody else to pick up anyhow. But
that's a separate issue from whether this should be back-patched.
Many patches wait in the queue for months, that's not new. Some of
them wait even longer than that.
For those reasons, and because you are willing visibly to still work
on it, I am moving this patch to next CF.
Regards,
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Dec 19, 2015 at 3:26 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
+1. There are folks around doing tests using 9.5 now, it is not
correct to impact the effort they have been putting on it until now.
This is a total misrepresentation of what I've said.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Geoghegan wrote:
On Fri, Dec 18, 2015 at 12:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:
I only see one patch version on the thread.
I'm not going to post a revision until I thrash out the tiny issues
with Heikki. He kind of trailed off. So maybe that kills it
immediately, which is a shame.
If you refuse to post an updated version of the patch until Heikki
weighs in some more, and given that Heikki has (for the purposes of this
patch) completely vanished, I think we should mark this rejected.
If somebody else is open to reviewing the patch, I think that'd be
another way to move forward, but presumably they would start from a
version with the discussed changes already fixed. Otherwise it's a
waste of time.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jan 18, 2016 at 8:36 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
If you refuse to post an updated version of the patch until Heikki
weighs in some more, and given that Heikki has (for the purposes of this
patch) completely vanished, I think we should mark this rejected.
I don't refuse. I just don't want to waste anyone's time. I will
follow all of Heikki's feedback immediately, except this:
"I think it'd be better to define it as "like CHECK_UNIQUE_YES, but
return FALSE instead of throwing an error on conflict". The difference
is that the aminsert would not be allowed to return FALSE when there
is no conflict".
That's because I believe this is quite broken, as already pointed out.
If somebody else is open to reviewing the patch, I think that'd be
another way to move forward, but presumably they would start from a
version with the discussed changes already fixed. Otherwise it's a
waste of time.
Your premise here is that what Heikki said in passing months ago is
incontrovertibly the right approach. That's ridiculous. I think Heikki
and I could work this out quite quickly, if he engaged, but for
whatever reason he appears unable to. I doubt that Heikki thinks that
about what he said, so why do you?
The point about CHECK_UNIQUE_YES I highlighted above felt like a
temporary misunderstanding to me, and not even what you might call a
real disagreement. It wasn't as if Heikki was insistent at the time. I
pointed out that what he said was broken according to an established
definition of broken (it would result in unprincipled deadlocks). He
didn't respond to that point. I think he didn't get back quickly in
part because I gave him something to think about.
If any other committer wants to engage with me on this, then I will of
course work with them. But that will not be predicated on my first
revising the patch in a way that this other committer does not
understand. That would be profoundly unfair.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Geoghegan wrote:
On Mon, Jan 18, 2016 at 8:36 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:If you refuse to post an updated version of the patch until Heikki
weighs in some more, and given that Heikki has (for the purposes of this
patch) completely vanished, I think we should mark this rejected.I don't refuse. I just don't want to waste anyone's time. I will
follow all of Heikki's feedback immediately, except this:"I think it'd be better to define it as "like CHECK_UNIQUE_YES, but
return FALSE instead of throwing an error on conflict". The difference
is that the aminsert would not be allowed to return FALSE when there
is no conflict".That's because I believe this is quite broken, as already pointed out.
I think I like your approach better.
If somebody else is open to reviewing the patch, I think that'd be
another way to move forward, but presumably they would start from a
version with the discussed changes already fixed. Otherwise it's a
waste of time.Your premise here is that what Heikki said in passing months ago is
incontrovertibly the right approach. That's ridiculous. I think Heikki
and I could work this out quite quickly, if he engaged, but for
whatever reason he appears unable to. I doubt that Heikki thinks that
about what he said, so why do you?
I don't -- I just think you could have sent a patch that addressed all
the other points, leave this one as initially submitted, and note that
the new submission left it unaddressed because you disagreed.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jan 18, 2016 at 11:56 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
That's because I believe this is quite broken, as already pointed out.
I think I like your approach better.
That makes things far simpler, then.
Your premise here is that what Heikki said in passing months ago is
incontrovertibly the right approach. That's ridiculous. I think Heikki
and I could work this out quite quickly, if he engaged, but for
whatever reason he appears unable to. I doubt that Heikki thinks that
about what he said, so why do you?I don't -- I just think you could have sent a patch that addressed all
the other points, leave this one as initially submitted, and note that
the new submission left it unaddressed because you disagreed.
I'll try to do that soon. I've got a very busy schedule over the next
couple of weeks, though.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jan 18, 2016 at 3:14 PM, Peter Geoghegan <pg@heroku.com> wrote:
On Mon, Jan 18, 2016 at 11:56 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:That's because I believe this is quite broken, as already pointed out.
I think I like your approach better.
That makes things far simpler, then.
Your premise here is that what Heikki said in passing months ago is
incontrovertibly the right approach. That's ridiculous. I think Heikki
and I could work this out quite quickly, if he engaged, but for
whatever reason he appears unable to. I doubt that Heikki thinks that
about what he said, so why do you?I don't -- I just think you could have sent a patch that addressed all
the other points, leave this one as initially submitted, and note that
the new submission left it unaddressed because you disagreed.I'll try to do that soon. I've got a very busy schedule over the next
couple of weeks, though.
This patch was reviewed during CF 2016-01 and has not been updated for
CF 2016-03. I think we should mark it Returned with Feedback.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Mar 11, 2016 at 5:26 AM, Robert Haas <robertmhaas@gmail.com> wrote:
This patch was reviewed during CF 2016-01 and has not been updated for
CF 2016-03. I think we should mark it Returned with Feedback.
I have a full plate at the moment, Robert, both as a reviewer and as a
patch author. This patch is basically uncontroversial, and is built to
make the AM interface clearer, and the design of speculative insertion
easier to understand. It's clear we should have it. I'll get around to
revising it before too long.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Mar 12, 2016 at 11:24 PM, Peter Geoghegan <pg@heroku.com> wrote:
On Fri, Mar 11, 2016 at 5:26 AM, Robert Haas <robertmhaas@gmail.com> wrote:
This patch was reviewed during CF 2016-01 and has not been updated for
CF 2016-03. I think we should mark it Returned with Feedback.I have a full plate at the moment, Robert, both as a reviewer and as a
patch author. This patch is basically uncontroversial, and is built to
make the AM interface clearer, and the design of speculative insertion
easier to understand. It's clear we should have it. I'll get around to
revising it before too long.
Only one version of this patch has been sent at the beginning of this
thread, and Heikki has clearly expressed his disagreement about at
least a portion of it at the beginning of this thread, so I find it
hard to define it as an "uncontroversial" thing and something that is
clear to have as things stand. Seeing a new version soon would be a
good next step I guess.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Mar 12, 2016 at 2:43 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
Only one version of this patch has been sent at the beginning of this
thread, and Heikki has clearly expressed his disagreement about at
least a portion of it at the beginning of this thread, so I find it
hard to define it as an "uncontroversial" thing and something that is
clear to have as things stand. Seeing a new version soon would be a
good next step I guess.
What is the point in saying this, Michael? What purpose does it serve?
I said "basically uncontroversial", not "uncontroversial". That is a
perfectly accurate characterization of the patch, and if you disagree
than I suggest you re-read the thread. Andres and Heikki were both in
favor of this patch. Heikki and I discussed one particular aspect of
it, and then it trailed off. The only thing that Heikki categorically
stated was that he disliked one narrow aspect of the style of one
thing in one function. I've already said I'm happy to do that.
As things stand, the documentation for amcanunique methods, and the
way they are described internally, is fairly misleading.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Mar 12, 2016 at 2:53 PM, Peter Geoghegan <pg@heroku.com> wrote:
I said "basically uncontroversial", not "uncontroversial". That is a
perfectly accurate characterization of the patch, and if you disagree
than I suggest you re-read the thread.
In particular, note that Alvaro eventually sided with me against the
thing that Heikki argued for:
/messages/by-id/20160118195643.GA117199@alvherre.pgsql
Describing what happened that way is unfair on Heikki, because I don't
think he was at all firm in what he said about making the new
UNIQUE_CHECK_SPECULATIVE "like CHECK_UNIQUE_YES, but return FALSE
instead of throwing an error on conflict". We were working through the
design, and it didn't actually come to any kind of impasse.
It's surprising and disappointing to me that this supposed
disagreement has been blown out of all proportion.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Mar 12, 2016 at 5:53 PM, Peter Geoghegan <pg@heroku.com> wrote:
On Sat, Mar 12, 2016 at 2:43 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:Only one version of this patch has been sent at the beginning of this
thread, and Heikki has clearly expressed his disagreement about at
least a portion of it at the beginning of this thread, so I find it
hard to define it as an "uncontroversial" thing and something that is
clear to have as things stand. Seeing a new version soon would be a
good next step I guess.What is the point in saying this, Michael? What purpose does it serve?
Gee, I think Michael is right on target. What purpose does writing
him an email that sounds annoyed serve?
There hasn't been a new version of this patch in 9 months, you're
clearly not in a hurry to produce one, and nobody else seems to feel
strongly that this is something that needs to be done at all. I think
we could just let this go and be just fine, but instead of doing that
and moving onto patches that people do feel strongly about, we're
arguing about this. Bummer.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Mar 14, 2016 at 5:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
There hasn't been a new version of this patch in 9 months, you're
clearly not in a hurry to produce one, and nobody else seems to feel
strongly that this is something that needs to be done at all. I think
we could just let this go and be just fine, but instead of doing that
and moving onto patches that people do feel strongly about, we're
arguing about this. Bummer.
I'm busy working on fixing an OpenSSL bug affecting all released
versions right at the moment, but have a number of complex 9.6 patches
to review, most of which are in need of support. I'm very busy.
I said that I'd get to this patch soon. I might be kicking the can
down the road a little with this patch; if so, I'm sorry. I suggest we
leave it at that, until the CF is almost over or until I produce a
revision.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-03-14 17:17:02 -0700, Peter Geoghegan wrote:
On Mon, Mar 14, 2016 at 5:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
There hasn't been a new version of this patch in 9 months, you're
clearly not in a hurry to produce one, and nobody else seems to feel
strongly that this is something that needs to be done at all. I think
we could just let this go and be just fine, but instead of doing that
and moving onto patches that people do feel strongly about, we're
arguing about this. Bummer.I'm busy working on fixing an OpenSSL bug affecting all released
versions right at the moment, but have a number of complex 9.6 patches
to review, most of which are in need of support. I'm very busy.
So? You're not the only one. I don't see why we shouldn't move this to
'returned with feedback' until there's a new version.
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Mar 14, 2016 at 5:21 PM, Andres Freund <andres@anarazel.de> wrote:
So? You're not the only one. I don't see why we shouldn't move this to
'returned with feedback' until there's a new version.
I don't see any point in that; I intend to get a revision in to the
ongoing CF. But fine.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Mar 14, 2016 at 8:17 PM, Peter Geoghegan <pg@heroku.com> wrote:
On Mon, Mar 14, 2016 at 5:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
There hasn't been a new version of this patch in 9 months, you're
clearly not in a hurry to produce one, and nobody else seems to feel
strongly that this is something that needs to be done at all. I think
we could just let this go and be just fine, but instead of doing that
and moving onto patches that people do feel strongly about, we're
arguing about this. Bummer.I'm busy working on fixing an OpenSSL bug affecting all released
versions right at the moment, but have a number of complex 9.6 patches
to review, most of which are in need of support. I'm very busy.I said that I'd get to this patch soon. I might be kicking the can
down the road a little with this patch; if so, I'm sorry. I suggest we
leave it at that, until the CF is almost over or until I produce a
revision.
Sure, and if everybody does that, then there will be 40 patches that
get updated in the last 2 days if the CommitFest, and that will be
impossible. Come on. You're demanding a degree of preferential
treatment which is unsupportable. You're also assuming that anybody's
going to be willing to commit that revision when you produce it, which
looks to me like an unproven hypothesis.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Mar 16, 2016 at 11:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Sure, and if everybody does that, then there will be 40 patches that
get updated in the last 2 days if the CommitFest, and that will be
impossible. Come on. You're demanding a degree of preferential
treatment which is unsupportable.
It's unexpected that an entirely maintenance-orientated patch like
this would be received this way. I'm not demanding anything, or
applying any real pressure. Let's just get on with it.
I attach a revision, that makes all the changes that Heikki suggested,
except one. As already noted several times, following this suggestion
would have added a bug. Alvaro preferred my original approach here in
any case. I refer to my original approach of making the new
UNIQUE_CHECK_SPECULATIVE case minimally different from the existing
UNIQUE_CHECK_PARTIAL case currently used for deferred unique
constraints and speculative insertion, as opposed to making the new
UNIQUE_CHECK_SPECULATIVE "like CHECK_UNIQUE_YES, but return FALSE
instead of throwing an error on conflict". That was broken because
CHECK_UNIQUE_YES waits for the outcome of an xact, which
UNIQUE_CHECK_PARTIAL never does, and so UNIQUE_CHECK_SPECULATIVE must
never do.
Any and all waits happen in the first phase of speculative insertion,
and never the seconds. I could give a complicated explanation for why,
involving a deadlock scenario, but a simple explanation will do: it
has always worked that way, and was tested to work that way.
Feedback from Heikki led to these changes for this revision:
* The use of arguments within ExecInsert() was simplified.
* More concise AM documentation.
The docs essentially describe two new concepts:
- What unique index insertion needs to know about speculative
insertion in general. This doesn't just apply to speculative inserters
themselves, of course.
- What speculative insertion is. Why it exists (why we don't just wait
on xact). In other words, what "unprincipled deadlocks" are, and how
they are avoided (from a relatively high level).
I feel like I have a responsibility to make sure that this mechanism
is well documented, especially given that not that many people were
involved in its design. It's possible that no more than the 3 original
authors of UPSERT fully understand speculative insertion -- it's easy
to miss some of the subtleties.
I do not pursue something like this without good reason. I'm
optimistic that the patch will be accepted if it is carefully
considered.
--
Peter Geoghegan
Attachments:
0001-Refactor-speculative-insertion-into-unique-indexes.patchtext/x-patch; charset=US-ASCII; name=0001-Refactor-speculative-insertion-into-unique-indexes.patchDownload
From 2b2a4c40a5e60ac1f28a75f11204ce88eb48cc73 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <peter.geoghegan86@gmail.com>
Date: Tue, 2 Jun 2015 17:34:16 -0700
Subject: [PATCH] Refactor speculative insertion into unique indexes
Add a dedicated IndexUniqueCheck constant for the speculative insertion
case, UNIQUE_CHECK_SPECULATIVE, rather than reusing
UNIQUE_CHECK_PARTIAL, which should now only be used for deferrable
unique constraints.
This change allows btinsert() (and, in principle, any amcanunique
aminsert function) to avoid physically inserting an IndexTuple in the
event of detecting a conflict during speculative insertion's second
phase. With nbtree, this avoidance now occurs at the critical point in
_bt_doinsert() immediately after establishing that there is a conflict,
but immediately before actually calling _bt_insertonpg() to proceed with
physical IndexTuple insertion.
At that point during UNIQUE_CHECK_PARTIAL insertion it makes sense to
soldier on, because the possibility remains that the conflict will later
go away and everything will happen to work out because the conflicting
insertion's transaction aborted. Speculative inserters, in contrast,
have no chance of working out a way to proceed without first deleting
the would-be-pointed-to heap tuple already physically inserted. For the
current row proposed for insertion, no useful progress will have been
made at this point.
This patch has nothing to do with performance; it is intended to clarify
how amcanunique AMs perform speculative insertion, and the general
theory of operation. It is natural to avoid an unnecessary index tuple
insertion. That that could happen before was quite misleading, because
it implied that it was necessary, and didn't acknowledge the differing
requirements in each case.
---
doc/src/sgml/indexam.sgml | 101 ++++++++++++++++++++++++++-------
src/backend/access/nbtree/nbtinsert.c | 49 +++++++++-------
src/backend/executor/execIndexing.c | 34 +++++++++--
src/backend/executor/nodeModifyTable.c | 2 +-
src/include/access/genam.h | 8 +++
5 files changed, 148 insertions(+), 46 deletions(-)
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index 5f7befb..1b26dd0 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -271,10 +271,13 @@ aminsert (Relation indexRelation,
<para>
The function's Boolean result value is significant only when
- <literal>checkUnique</> is <literal>UNIQUE_CHECK_PARTIAL</>.
- In this case a TRUE result means the new entry is known unique, whereas
- FALSE means it might be non-unique (and a deferred uniqueness check must
- be scheduled). For other cases a constant FALSE result is recommended.
+ <literal>checkUnique</> is <literal>UNIQUE_CHECK_PARTIAL</> or
+ <literal>UNIQUE_CHECK_SPECULATIVE</>. In this case a TRUE result
+ means the new entry is known unique, whereas FALSE means it might
+ be non-unique (and a deferred uniqueness check must be scheduled,
+ or for <literal>UNIQUE_CHECK_SPECULATIVE</>, speculative insertion
+ will restart from its first phase, conflict prechecking). For
+ other cases a constant FALSE result is recommended.
</para>
<para>
@@ -875,11 +878,15 @@ amrestrpos (IndexScanDesc scan);
<listitem>
<para>
If a conflicting row has been inserted by an as-yet-uncommitted
- transaction, the would-be inserter must wait to see if that transaction
- commits. If it rolls back then there is no conflict. If it commits
- without deleting the conflicting row again, there is a uniqueness
- violation. (In practice we just wait for the other transaction to
- end and then redo the visibility check in toto.)
+ transaction, the would-be inserter must wait to see if that
+ transaction commits (or if its <firstterm>speculative
+ insertion</> succeeds). If it rolls back (or the speculative
+ insertion does not complete) then there is no conflict. If it
+ commits without deleting the conflicting row again, there is a
+ uniqueness violation. (In practice we just wait for the other
+ transaction to end and then redo the visibility check in toto;
+ if a speculative insertion was conflicting, it may now be found
+ to be confirmed pending transaction commit.)
</para>
</listitem>
<listitem>
@@ -903,15 +910,17 @@ amrestrpos (IndexScanDesc scan);
</para>
<para>
- We require the index access method to apply these tests itself, which
- means that it must reach into the heap to check the commit status of
- any row that is shown to have a duplicate key according to the index
- contents. This is without a doubt ugly and non-modular, but it saves
- redundant work: if we did a separate probe then the index lookup for
- a conflicting row would be essentially repeated while finding the place to
- insert the new row's index entry. What's more, there is no obvious way
- to avoid race conditions unless the conflict check is an integral part
- of insertion of the new index entry.
+ We require the index access method to apply these tests itself,
+ which means that it must reach into the heap to check the commit
+ status of any row (possibly including a speculative insertion
+ token) that is shown to have a duplicate key according to the index
+ contents. This is without a doubt ugly and non-modular, but it
+ saves redundant work: if we did a separate probe then the index
+ lookup for a conflicting row would be essentially repeated while
+ finding the place to insert the new row's index entry. What's
+ more, there is no obvious way to avoid race conditions unless the
+ conflict check is an integral part of insertion of the new index
+ entry.
</para>
<para>
@@ -926,8 +935,36 @@ amrestrpos (IndexScanDesc scan);
tuple and some other tuple with the same key are live, then the error
must be reported. (Note that for this purpose, <quote>live</> actually
means <quote>any tuple in the index entry's HOT chain is live</>.)
- To implement this, the <function>aminsert</> function is passed a
- <literal>checkUnique</> parameter having one of the following values:
+ </para>
+
+ <para>
+ Speculative insertion is an executor infrastructure used by
+ <literal>INSERT ... ON CONFLICT DO UPDATE/NOTHING</>. A major goal
+ of the infrastructure is to consistently maintain a documented
+ user-visible guarantee: the avoidance of <quote>unprincipled</>
+ deadlocks. Unprincipled deadlocks are deadlocks that arise due to
+ a mutual dependency that is purely an implementation detail, that
+ users cannot avoid by being consistent about the order of lock
+ acquisition. A speculative insertion token lock is not held for
+ its transaction's entire duration; rather, it is held during its
+ holder's speculative insertion, typically for only an instant.
+ Without this precaution, under high concurrency <literal>INSERT ...
+ ON CONFLICT DO UPDATE</> sessions could raise non-avoidable
+ deadlocks, which would be inconsistent with documented behavior.
+ Speculative insertion conflicts are handled in a similar manner to
+ deferred constraint conflicts, in that no error is initially
+ thrown. However, rather than rechecking for a conflict at a
+ deferred point, the speculative insertion attempt is cancelled.
+ The executor restarts at the first phase, the precheck, which is
+ the only point at which unique index speculative insertion may wait
+ for an outcome of or in another transaction. Note that deferrable
+ constraints are not supported.
+ </para>
+
+ <para>
+ To implement unique enforcement, the <function>aminsert</> function
+ is passed a <literal>checkUnique</> parameter having one of the
+ following values:
<itemizedlist>
<listitem>
@@ -986,6 +1023,30 @@ amrestrpos (IndexScanDesc scan);
for the same tuple values as were used in the original insertion.
</para>
</listitem>
+ <listitem>
+ <para>
+ <literal>UNIQUE_CHECK_SPECULATIVE</> is very similar to
+ <literal>UNIQUE_CHECK_PARTIAL</>, but indicates an optimistic
+ insertion in the second stage of speculative insertion.
+ </para>
+
+ <para>
+ <literal>UNIQUE_CHECK_SPECULATIVE</> allows the access method
+ to not proceed with <structname>IndexTuple</structname>
+ insertion when it is already clear that that cannot be useful
+ (i.e. when <function>aminsert</> is set to return FALSE,
+ making caller immediately delete its physically inserted heap
+ tuple). Proceeding with insertion of the caller's
+ <structname>IndexTuple</structname> regardless is harmless but
+ unnecessary. When <literal>UNIQUE_CHECK_SPECULATIVE</>
+ insertion detects a conflict, the caller (executor) will cancel
+ its speculative insertion, immediately preventing its heap
+ tuple from being observed even by uniqueness checking. The
+ executor then restarts from the very beginning for the row
+ proposed for insertion. There should be no side-effects
+ associated with the cancelled attempt.
+ </para>
+ </listitem>
</itemizedlist>
</para>
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index e3c55eb..3b6620f 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -91,17 +91,21 @@ static void _bt_vacuum_one_page(Relation rel, Buffer buffer, Relation heapRel);
* This routine is called by the public interface routine, btinsert.
* By here, itup is filled in, including the TID.
*
- * If checkUnique is UNIQUE_CHECK_NO or UNIQUE_CHECK_PARTIAL, this
- * will allow duplicates. Otherwise (UNIQUE_CHECK_YES or
- * UNIQUE_CHECK_EXISTING) it will throw error for a duplicate.
- * For UNIQUE_CHECK_EXISTING we merely run the duplicate check, and
- * don't actually insert.
+ * If checkUnique is UNIQUE_CHECK_NO, UNIQUE_CHECK_PARTIAL or
+ * UNIQUE_CHECK_SPECULATIVE, this will allow duplicates. Otherwise
+ * (UNIQUE_CHECK_YES or UNIQUE_CHECK_EXISTING) it will throw error for a
+ * duplicate. For UNIQUE_CHECK_EXISTING we merely run the duplicate
+ * check, and don't actually insert.
*
- * The result value is only significant for UNIQUE_CHECK_PARTIAL:
- * it must be TRUE if the entry is known unique, else FALSE.
- * (In the current implementation we'll also return TRUE after a
- * successful UNIQUE_CHECK_YES or UNIQUE_CHECK_EXISTING call, but
- * that's just a coding artifact.)
+ * The result value is only significant for UNIQUE_CHECK_PARTIAL
+ * and UNIQUE_CHECK_SPECULATIVE: it must be TRUE if the entry is
+ * known unique, else FALSE. (In the current implementation
+ * we'll also return TRUE after a successful UNIQUE_CHECK_YES or
+ * UNIQUE_CHECK_EXISTING call, but that's just a coding
+ * artifact.)
+ *
+ * Note that UNIQUE_CHECK_SPECULATIVE calls that return FALSE
+ * don't actually proceed with insertion of the index tuple.
*/
bool
_bt_doinsert(Relation rel, IndexTuple itup,
@@ -188,7 +192,8 @@ top:
}
}
- if (checkUnique != UNIQUE_CHECK_EXISTING)
+ if (checkUnique != UNIQUE_CHECK_EXISTING &&
+ (checkUnique != UNIQUE_CHECK_SPECULATIVE || is_unique))
{
/*
* The only conflict predicate locking cares about for indexes is when
@@ -230,10 +235,11 @@ top:
* progress, *speculativeToken is set to non-zero, and the caller can wait for
* the verdict on the insertion using SpeculativeInsertionWait().
*
- * However, if checkUnique == UNIQUE_CHECK_PARTIAL, we always return
- * InvalidTransactionId because we don't want to wait. In this case we
- * set *is_unique to false if there is a potential conflict, and the
- * core code must redo the uniqueness check later.
+ * However, if checkUnique is UNIQUE_CHECK_PARTIAL or
+ * UNIQUE_CHECK_SPECULATIVE, we always return InvalidTransactionId because we
+ * don't want to wait. In this case we set *is_unique to false if there is a
+ * potential conflict, and the core code must redo the uniqueness check later
+ * or restart speculative insertion.
*/
static TransactionId
_bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,
@@ -331,12 +337,15 @@ _bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,
/*
* It is a duplicate. If we are only doing a partial
- * check, then don't bother checking if the tuple is being
- * updated in another transaction. Just return the fact
- * that it is a potential conflict and leave the full
- * check till later.
+ * check or speculative insertion, then don't bother
+ * checking if the tuple is being updated in another
+ * transaction. Just return the fact that it is a potential
+ * conflict. Partial check callers can leave the full check
+ * till later. Speculative insertion callers will need to
+ * restart.
*/
- if (checkUnique == UNIQUE_CHECK_PARTIAL)
+ if (checkUnique == UNIQUE_CHECK_PARTIAL ||
+ checkUnique == UNIQUE_CHECK_SPECULATIVE)
{
if (nbuf != InvalidBuffer)
_bt_relbuf(rel, nbuf);
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 5d553d5..f37c423 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -259,6 +259,14 @@ ExecCloseIndices(ResultRelInfo *resultRelInfo)
* the same is done for non-deferred constraints, but report
* if conflict was speculative or deferred conflict to caller)
*
+ * Note that the implementation does not bother with inserting
+ * into all indexes in the event of detecting a speculative
+ * insertion conflict. This should not matter, because when
+ * *specConflict is set to true, caller's heap tuple will be
+ * super-deleted; a complete list of indexes with conflicts
+ * is not useful (in fact, even the actual source of the
+ * speculative insertion conflict is not of interest).
+ *
* CAUTION: this must not be called for a HOT update.
* We can't defend against that here for lack of info.
* Should we change the API to make it safer?
@@ -320,6 +328,7 @@ ExecInsertIndexTuples(TupleTableSlot *slot,
/* Record if speculative insertion arbiter */
arbiter = list_member_oid(arbiterIndexes,
indexRelation->rd_index->indexrelid);
+ Assert(!arbiter || indexRelation->rd_index->indimmediate);
/* If the index is marked as read-only, ignore it */
if (!indexInfo->ii_ReadyForInserts)
@@ -374,7 +383,7 @@ ExecInsertIndexTuples(TupleTableSlot *slot,
if (!indexRelation->rd_index->indisunique)
checkUnique = UNIQUE_CHECK_NO;
else if (noDupErr && (arbiterIndexes == NIL || arbiter))
- checkUnique = UNIQUE_CHECK_PARTIAL;
+ checkUnique = UNIQUE_CHECK_SPECULATIVE;
else if (indexRelation->rd_index->indimmediate)
checkUnique = UNIQUE_CHECK_YES;
else
@@ -432,18 +441,29 @@ ExecInsertIndexTuples(TupleTableSlot *slot,
}
if ((checkUnique == UNIQUE_CHECK_PARTIAL ||
+ checkUnique == UNIQUE_CHECK_SPECULATIVE ||
indexInfo->ii_ExclusionOps != NULL) &&
!satisfiesConstraint)
{
+ if (noDupErr && indexRelation->rd_index->indimmediate)
+ {
+ /*
+ * Speculative conflict found, so caller must restart
+ * speculative insertion. Caller will now not care about
+ * specifics of any other deferred constraint conflict
+ * that may have already been noted.
+ */
+ list_free(result);
+ *specConflict = true;
+ return NIL;
+ }
+
/*
* The tuple potentially violates the uniqueness or exclusion
* constraint, so make a note of the index so that we can re-check
- * it later. Speculative inserters are told if there was a
- * speculative conflict, since that always requires a restart.
+ * it later
*/
result = lappend_oid(result, RelationGetRelid(indexRelation));
- if (indexRelation->rd_index->indimmediate && specConflict)
- *specConflict = true;
}
}
@@ -540,6 +560,10 @@ ExecCheckIndexConstraints(TupleTableSlot *slot,
errtableconstraint(heapRelation,
RelationGetRelationName(indexRelation))));
+ /*
+ * Set checkedIndex here, since partial unique index will still
+ * count as found
+ */
checkedIndex = true;
/* Check for partial index */
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 27051e8..d0d52f6 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -441,7 +441,7 @@ ExecInsert(ModifyTableState *mtstate,
*/
if (specConflict)
{
- list_free(recheckIndexes);
+ Assert(recheckIndexes == NIL);
goto vlock;
}
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index 81907d5..2254716 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -98,6 +98,13 @@ typedef struct SysScanDescData *SysScanDesc;
* known unique, false if it is possibly non-unique. In the "true" case
* it is safe to omit the later recheck.
*
+ * UNIQUE_CHECK_SPECULATIVE is very similar to UNIQUE_CHECK_PARTIAL, but is
+ * used for speculative insertion (which does not support deferrable unique
+ * constraints as arbiters). The only difference is that it allows the
+ * implementation to avoid pointlessly inserting an index tuple after
+ * establishing that there is a speculative insertion conflict, where the heap
+ * tuple must be super-deleted anyway.
+ *
* When it is time to recheck the deferred constraint, a pseudo-insertion
* call is made with UNIQUE_CHECK_EXISTING. The tuple is already in the
* index in this case, so it should not be inserted again. Rather, just
@@ -108,6 +115,7 @@ typedef enum IndexUniqueCheck
UNIQUE_CHECK_NO, /* Don't do any uniqueness checking */
UNIQUE_CHECK_YES, /* Enforce uniqueness at insertion time */
UNIQUE_CHECK_PARTIAL, /* Test uniqueness, but no error */
+ UNIQUE_CHECK_SPECULATIVE, /* Speculative Insertion */
UNIQUE_CHECK_EXISTING /* Check if existing tuple is unique */
} IndexUniqueCheck;
--
1.9.1
On Wed, Mar 16, 2016 at 7:43 PM, Peter Geoghegan <pg@heroku.com> wrote:
On Wed, Mar 16, 2016 at 11:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Sure, and if everybody does that, then there will be 40 patches that
get updated in the last 2 days if the CommitFest, and that will be
impossible. Come on. You're demanding a degree of preferential
treatment which is unsupportable.It's unexpected that an entirely maintenance-orientated patch like
this would be received this way. I'm not demanding anything, or
applying any real pressure. Let's just get on with it.I attach a revision, that makes all the changes that Heikki suggested,
except one. As already noted several times, following this suggestion
would have added a bug. Alvaro preferred my original approach here in
any case. I refer to my original approach of making the new
UNIQUE_CHECK_SPECULATIVE case minimally different from the existing
UNIQUE_CHECK_PARTIAL case currently used for deferred unique
constraints and speculative insertion, as opposed to making the new
UNIQUE_CHECK_SPECULATIVE "like CHECK_UNIQUE_YES, but return FALSE
instead of throwing an error on conflict". That was broken because
CHECK_UNIQUE_YES waits for the outcome of an xact, which
UNIQUE_CHECK_PARTIAL never does, and so UNIQUE_CHECK_SPECULATIVE must
never do.Any and all waits happen in the first phase of speculative insertion,
and never the seconds. I could give a complicated explanation for why,
involving a deadlock scenario, but a simple explanation will do: it
has always worked that way, and was tested to work that way.Feedback from Heikki led to these changes for this revision:
* The use of arguments within ExecInsert() was simplified.
* More concise AM documentation.
The docs essentially describe two new concepts:
- What unique index insertion needs to know about speculative
insertion in general. This doesn't just apply to speculative inserters
themselves, of course.- What speculative insertion is. Why it exists (why we don't just wait
on xact). In other words, what "unprincipled deadlocks" are, and how
they are avoided (from a relatively high level).I feel like I have a responsibility to make sure that this mechanism
is well documented, especially given that not that many people were
involved in its design. It's possible that no more than the 3 original
authors of UPSERT fully understand speculative insertion -- it's easy
to miss some of the subtleties.I do not pursue something like this without good reason. I'm
optimistic that the patch will be accepted if it is carefully
considered.
This patch has attracted no reviewers. Any volunteers?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Apr 5, 2016 at 9:47 PM, Robert Haas <robertmhaas@gmail.com> wrote:
I do not pursue something like this without good reason. I'm
optimistic that the patch will be accepted if it is carefully
considered.This patch has attracted no reviewers. Any volunteers?
Since nobody has emerged to review this, I have moved it to the next CommitFest.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 03/17/2016 01:43 AM, Peter Geoghegan wrote:
I attach a revision, that makes all the changes that Heikki suggested,
except one. As already noted several times, following this suggestion
would have added a bug. Alvaro preferred my original approach here in
any case. I refer to my original approach of making the new
UNIQUE_CHECK_SPECULATIVE case minimally different from the existing
UNIQUE_CHECK_PARTIAL case currently used for deferred unique
constraints and speculative insertion, as opposed to making the new
UNIQUE_CHECK_SPECULATIVE "like CHECK_UNIQUE_YES, but return FALSE
instead of throwing an error on conflict". That was broken because
CHECK_UNIQUE_YES waits for the outcome of an xact, which
UNIQUE_CHECK_PARTIAL never does, and so UNIQUE_CHECK_SPECULATIVE must
never do.Any and all waits happen in the first phase of speculative insertion,
and never the seconds. I could give a complicated explanation for why,
involving a deadlock scenario, but a simple explanation will do: it
has always worked that way, and was tested to work that way.Feedback from Heikki led to these changes for this revision:
* The use of arguments within ExecInsert() was simplified.
* More concise AM documentation.
The docs essentially describe two new concepts:
- What unique index insertion needs to know about speculative
insertion in general. This doesn't just apply to speculative inserters
themselves, of course.- What speculative insertion is. Why it exists (why we don't just wait
on xact). In other words, what "unprincipled deadlocks" are, and how
they are avoided (from a relatively high level).I feel like I have a responsibility to make sure that this mechanism
is well documented, especially given that not that many people were
involved in its design. It's possible that no more than the 3 original
authors of UPSERT fully understand speculative insertion -- it's easy
to miss some of the subtleties.
Thanks for working on this, and sorry for disappearing and dropping this
on the floor earlier. This doesn't apply anymore, thanks to 9c810a2e.
Shouldn't be hard to fix.
I was in support of this earlier in general, even though I had some
questions on the details. But now that I look at the patch again, I'm
not so sure anymore. I don't think this actually makes things clearer.
It adds new cases that the code needs to deal with. Index AM writers now
have to care about the difference between a UNIQUE_CHECK_PARTIAL and
UNIQUE_CHECK_SPECULATIVE. You can have the exact same implementation for
both, but at the very least the index AM author needs to read the
paragraph you added to the docs to understand the difference. That adds
some cognitive load. Likewise, in ExecInsertIndexTuples(), this makes
the deferred-index case work slightly differently from speculative
insertion. It's not a big difference, but it again forces you to keep
one more scenario in mind, when reading the code.
So overall, I think we should just drop this. Maybe a comment somewhere
would be in order, to point out that ExecInsertIndexTuples() and
index_insert might perform some unnecessary work, by inserting index
tuples for a doomed heap tuple, if a speculative insertion fails. But
that's all.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 20, 2016 at 8:55 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Thanks for working on this, and sorry for disappearing and dropping this on
the floor earlier. This doesn't apply anymore, thanks to 9c810a2e. Shouldn't
be hard to fix.
Thanks for looking at it again.
I was in support of this earlier in general, even though I had some
questions on the details. But now that I look at the patch again, I'm not so
sure anymore.
Honestly, I almost withdrew this patch myself, simply because it has
dragged on so long. It has become ridiculous.
I don't think this actually makes things clearer. It adds new
cases that the code needs to deal with. Index AM writers now have to care
about the difference between a UNIQUE_CHECK_PARTIAL and
UNIQUE_CHECK_SPECULATIVE. You can have the exact same implementation for
both, but at the very least the index AM author needs to read the paragraph
you added to the docs to understand the difference. That adds some cognitive
load.
I think it gives us the opportunity to discuss the differences, and in
particular to explain why this "speculative insertion" thing exists at
all. Besides, we can imagine an amcanunique implementation in which
the difference might matter. Honestly, since it is highly unlikely
that there ever will be another amcanunique access method, the
cognitive burden to implementers of new amcanunique AMs is not a
concern to me. Rather, the concern with that part of the patch is
educating people about how the whole speculative insertion thing works
in general, and drawing specific attention to it in a few key places
in the code.
Speculative insertion is subtle and complicated enough that I feel
that that should be well described, as I've said many times. Remember
how hard it was for us to come to agreement on the basic requirements
in the first place!
Likewise, in ExecInsertIndexTuples(), this makes the deferred-index
case work slightly differently from speculative insertion. It's not a big
difference, but it again forces you to keep one more scenario in mind, when
reading the code
This actually does have useful practical consequences, although I
didn't point that out earlier (though I should have). To see what I
mean, consider the complaint in this recent thread, which is based on
an actual user application problem:
/messages/by-id/1472134358.649524557@f146.i.mail.ru
I go on to explain how this patch represents a partial solution to
that [1]/messages/by-id/CAM3SWZTFTbK_Y=7uWJaXYLHnYQ99pV4KFpmjTKbNJR5_+QThzA@mail.gmail.com -- Peter Geoghegan. That's what I mean by "useful practical consequences". As I
say in [1]/messages/by-id/CAM3SWZTFTbK_Y=7uWJaXYLHnYQ99pV4KFpmjTKbNJR5_+QThzA@mail.gmail.com -- Peter Geoghegan, I think we could even get a full solution, if we applied
this patch and *also* made the ordering in which the relcache returns
a list of index OIDs more useful (it should still be based on
something stable, to avoid deadlocks, but more than just OID order.
Instead, relcache.c can sort indexes such that we insert into primary
keys first, then unique indexes, then all other indexes. This also
avoids bloat if there is a unique violation, by getting unique indexes
out of the way first during ExecInsert()).
So overall, I think we should just drop this. Maybe a comment somewhere
would be in order, to point out that ExecInsertIndexTuples() and
index_insert might perform some unnecessary work, by inserting index tuples
for a doomed heap tuple, if a speculative insertion fails. But that's all.
Perhaps. I'm curious about what your thoughts are on what I've said
about "useful practical consequences" of finishing insertion earlier
for speculative inserters. If you're still not convinced about this
patch, having considered that as well, then I will drop the patch (or
maybe we just add some comments, as you suggest).
[1]: /messages/by-id/CAM3SWZTFTbK_Y=7uWJaXYLHnYQ99pV4KFpmjTKbNJR5_+QThzA@mail.gmail.com -- Peter Geoghegan
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 09/24/2016 02:34 PM, Peter Geoghegan wrote:
On Tue, Sep 20, 2016 at 8:55 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Likewise, in ExecInsertIndexTuples(), this makes the deferred-index
case work slightly differently from speculative insertion. It's not a big
difference, but it again forces you to keep one more scenario in mind, when
reading the codeThis actually does have useful practical consequences, although I
didn't point that out earlier (though I should have). To see what I
mean, consider the complaint in this recent thread, which is based on
an actual user application problem:/messages/by-id/1472134358.649524557@f146.i.mail.ru
I go on to explain how this patch represents a partial solution to
that [1]. That's what I mean by "useful practical consequences". As I
say in [1], I think we could even get a full solution, if we applied
this patch and *also* made the ordering in which the relcache returns
a list of index OIDs more useful (it should still be based on
something stable, to avoid deadlocks, but more than just OID order.
Instead, relcache.c can sort indexes such that we insert into primary
keys first, then unique indexes, then all other indexes. This also
avoids bloat if there is a unique violation, by getting unique indexes
out of the way first during ExecInsert()).
Hmm, yeah, that'd be nice to fix. I'd like to see a patch specifically
to fix that. I'm not convinced that we need all the complications of
this patch, to get that fixed. (In particular, indexam's still wouldn't
need to care about the different between CHECK_UNIQUE_PARTIAL and
CHECK_UNIQUE_SPECULATIVE, I think.)
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 27, 2016 at 9:10 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Hmm, yeah, that'd be nice to fix. I'd like to see a patch specifically to
fix that. I'm not convinced that we need all the complications of this
patch, to get that fixed. (In particular, indexam's still wouldn't need to
care about the different between CHECK_UNIQUE_PARTIAL and
CHECK_UNIQUE_SPECULATIVE, I think.)
But you are convinced that everything else I describe would do it?
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 09/27/2016 11:22 AM, Peter Geoghegan wrote:
On Tue, Sep 27, 2016 at 9:10 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Hmm, yeah, that'd be nice to fix. I'd like to see a patch specifically to
fix that. I'm not convinced that we need all the complications of this
patch, to get that fixed. (In particular, indexam's still wouldn't need to
care about the different between CHECK_UNIQUE_PARTIAL and
CHECK_UNIQUE_SPECULATIVE, I think.)But you are convinced that everything else I describe would do it?
I didn't think very hard about it, but yeah, think so..
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 27, 2016 at 9:25 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I didn't think very hard about it, but yeah, think so..
Do you think it's worth a backpatch? Or, too early to tell?
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 09/27/2016 11:38 AM, Peter Geoghegan wrote:
On Tue, Sep 27, 2016 at 9:25 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I didn't think very hard about it, but yeah, think so..
Do you think it's worth a backpatch? Or, too early to tell?
Too early to tell..
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 09/24/2016 02:34 PM, Peter Geoghegan wrote:
I go on to explain how this patch represents a partial solution to
that [1]. That's what I mean by "useful practical consequences". As I
say in [1], I think we could even get a full solution, if we applied
this patch and *also* made the ordering in which the relcache returns
a list of index OIDs more useful (it should still be based on
something stable, to avoid deadlocks, but more than just OID order.
Instead, relcache.c can sort indexes such that we insert into primary
keys first, then unique indexes, then all other indexes. This also
avoids bloat if there is a unique violation, by getting unique indexes
out of the way first during ExecInsert()).
Hmm, yeah, that'd be nice to fix. I'd like to see a patch specifically
to fix that. I'm not convinced that we need all the complications of
this patch, to get that fixed. (In particular, indexam's still wouldn't
need to care about the different between CHECK_UNIQUE_PARTIAL and
CHECK_UNIQUE_SPECULATIVE, I think.)
I can see the value of processing unique indexes before non-unique ones.
I'm pretty suspicious of trying to prioritize primary keys first, though,
because (a) it's not clear why bother, and (b) PG is a tad squishy about
whether an index is a primary key or not, so that I'd be worried about
different backends sometimes choosing different orders. I'd simplify
this to "uniques in OID order then non-uniques in OID order".
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 27, 2016 at 2:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I can see the value of processing unique indexes before non-unique ones.
I'm pretty suspicious of trying to prioritize primary keys first, though,
because (a) it's not clear why bother, and (b) PG is a tad squishy about
whether an index is a primary key or not, so that I'd be worried about
different backends sometimes choosing different orders. I'd simplify
this to "uniques in OID order then non-uniques in OID order".
I see your point. A more considered ordering of indexes for processing
by the executor (prepared for it by the relcache), including something
more that goes further than your proposal is useful in the context of
fixing the bug I mentioned [1]/messages/by-id/CAM3SWZTFTbK_Y=7uWJaXYLHnYQ99pV4KFpmjTKbNJR5_+QThzA@mail.gmail.com -- Peter Geoghegan, but for non-obvious reasons. I would
like to clarify what I meant there specifically. I am repeating
myself, but maybe I just wasn't clear before.
The theory of putting the PK first there is that we then have a
well-defined (uh, better defined) user-visible ordering *across unique
indexes* such that the problem case would *reliably* be fixed. With
only this refactoring patch applied (and no change to the relcache
ordering thing), it is then only a sheer accident of OID assignment
ordering that the INSERT ON CONFLICT problem case happens to take the
alternative path on the OP's *inferred* index (which, as it happens,
was the PK for him), rather than the other unique index that was
involved (the one that is not actually inferred, and yet is equivalent
to the PK, UPSERT-semantics-wise). So, the reporter of the bug [1]/messages/by-id/CAM3SWZTFTbK_Y=7uWJaXYLHnYQ99pV4KFpmjTKbNJR5_+QThzA@mail.gmail.com -- Peter Geoghegan is
happy with his exact case now working, at least.
You might now counter: "But why prefer one convention over the other?
Prioritizing the PK would reliably fix that particular problem case,
but that's still pretty arbitrary."
It's true that it's somewhat arbitrary to always (speculatively)
insert into the PK first. But, I think that it's more likely that the
PK is inferred in general, and so it's more likely that users will
fall on the right side of that in practice. Besides, at least we now
have a consistent behavior.
You might also reasonably ask: "But what if there are multiple unique
indexes, none of which happen to be the PK? Isn't that subject to the
same vagaries of OID ordering anyway?"
I must admit that it is. But I don't really know where to draw the
line here. Is it worth contemplating a more complicated scheme still?
For example, trigger-style ordering; a sort order that considers index
name as a "secondary attribute", in order to ensure perfectly
consistent behavior? I must admit that I don't really have a clue
whether or not that's a good idea. It's an idea.
[1]: /messages/by-id/CAM3SWZTFTbK_Y=7uWJaXYLHnYQ99pV4KFpmjTKbNJR5_+QThzA@mail.gmail.com -- Peter Geoghegan
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers