On Fri, Feb 13, 2015 at 7:22 PM, Jim Nasby <jim.na...@bluetreble.com> wrote: > 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?
Yes, but traditionally that is left until the last minute - when the patch is committed. That's why it's called out in the commit message (it isn't otherwise obvious - it's not a common catversion necessitating change like an addition to a system catalog). > 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... See my remarks to Heikki on this. I don't think it adds much. > 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... Yes, but the difference here is that the UPDATE path could be taken (which is sort of like when a before row insert path returns NULL). What I'm calling out is the dependency on firing before row insert triggers to decide if the alternative path must be taken. Roughly speaking, we must perform "part" of the INSERT (firing of before row insert triggers) in order to decide to do or not do an INSERT. This is, as I said, similar to when those triggers return NULL, and won't matter with idiomatic patterns of before trigger usage. Still feels worth calling out, because sometimes users do foolishly write before triggers with many external side-effects. > 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." Take a look at the executor README. We're locking the only row that can be locked when an UPSERT non-conclusively thinks to take the UPDATE path: the row that was found during our pre-check. We can only UPDATE when we find such a tuple, and then lock it without finding a row-level conflict. > 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. An Assert() of what? Note that the similar function get_relation_info() does about the same thing here. > 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. > */ It's safe, as the comment indicates, because the former is only used for xl_heap_multi_insert records, while the latter is only used for xl_heap_delete records. There is no ambiguity, because naturally we're always able to establish what type of record is under consideration before checking the bit is set. The XLOG_HEAP_* format is used for other flags there, despite the fact that other flags (like XLOG_HEAP_PREFIX_FROM_OLD) can only appear in certain context-appropriate xl_heap_* records. AFAICT, all that I've done that's new here is rely on that, since a bunch of those bits were used up in the last year or two, and the need to even consider bit reuse here is a new problem. > I'll review the remaining patches later. 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
