Re: INSERT ... ON CONFLICT {UPDATE | IGNORE} 2.0 - Mailing list pgsql-hackers

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-insertsfor 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