Re: INSERT ... ON CONFLICT {UPDATE | IGNORE} 2.0

On 2/9/15 6:21 PM, Peter Geoghegan wrote:
> Thanks for taking a look at it. That's somewhat cleaned up in the
> attached patchseries - V2.2.

In patch 1, "sepgsql is also affected by this commit. Note that this commit
necessitates an initdb, since stored ACLs are broken."

Doesn't that warrant bumping catversion?

Patch 2
+ * When killing a speculatively-inserted tuple, we set xmin to invalid
and
+if (!(xlrec->flags & XLOG_HEAP_KILLED_SPECULATIVE_TUPLE))

When doing this, should we also set the HEAP_XMIN_INVALID hint bit?

<reads more of patch>

Ok, I see we're not doing this because the check for a super deleted
tuple is already cheap. Probably worth mentioning that in the comment...

ExecInsert():
+ * We don't suppress the effects (or, perhaps, side-effects) of
+ * BEFORE ROW INSERT triggers. This isn't ideal, but then we
+ * cannot proceed with even considering uniqueness violations until
+ * these triggers fire on the one hand, but on the other hand they
+ * have the ability to execute arbitrary user-defined code which
+ * may perform operations entirely outside the system's ability to
+ * nullify.

I'm a bit confused as to why we're calling BEFORE triggers out here...
hasn't this always been true for both BEFORE *and* AFTER triggers? The
comment makes me wonder if there's some magic that happens with AFTER...

+spec != SPEC_NONE? HEAP_INSERT_SPECULATIVE:0
Non-standard formatting. Given the size of the patch and work already
into it I'd just leave it for the next formatting run; I only mention it
in case someone has some compelling reason not to.

ExecLockUpdateTuple():
+ * Try to lock tuple for update as part of speculative insertion. If
+ * a qual originating from ON CONFLICT UPDATE is satisfied, update
+ * (but still lock row, even though it may not satisfy estate's
+ * snapshot).

I find this confusing... which row is being locked? The previously
inserted one? Perhaps a better wording would be "satisfied, update. Lock
the original row even if it doesn't satisfy estate's snapshot."

infer_unique_index():
+/*
+ * We need not lock the relation since it was already locked, either by
+ * the rewriter or when expand_inherited_rtentry() added it to the query's
+ * rangetable.
+ */
+relationObjectId = rt_fetch(parse->resultRelation, parse->rtable)->relid;
+
+relation = heap_open(relationObjectId, NoLock);

Seems like there should be an Assert here. Also, the comment should
probably go before the heap_open call.

heapam_xlog.h:
+/* reuse xl_heap_multi_insert-only bit for xl_heap_delete */
I wish this would mention why it's safe to do this. Also, the comment
mentions xl_heap_delete when the #define is for
XLOG_HEAP_KILLED_SPECULATIVE_TUPLE; presumably that's wrong. Perhaps:
/*
* reuse XLOG_HEAP_LAST_MULTI_INSERT bit for
* XLOG_HEAP_KILLED_SPECULATIVE_TUPLE. This is safe because we never do
* multi-inserts for INSERT ON CONFLICT.
*/

I'll review the remaining patches later.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com