On Tue, Feb 2, 2015 at 01:06 AM, Andres Freund <and...@2ndquadrant.com> wrote:
> A first (not actually that quick :() look through the patches to see
> what actually happened in the last months. I didn't keep up with the
> thread.
So, let me get this out of the way: This is the first in-depth
technical review that this work has had in a long time. Thank you for
your help here.
> Generally the split into the individual commits doesn't seem to make
> much sense to me. The commits individually (except the first) aren't
> indivdiually commitable and aren't even meaningful. Splitting off the
> internal docs, tests and such actually just seems to make reviewing
> harder because you miss context. Splitting it so that individual piece
> are committable and reviewable makes sense, but... I have no problem
> doing the user docs later. If you split of RLS support, you need to
> throw an error before it's implemented.
I mostly agree. Basically, I did not intend for all of the patches to
be individually committed. The mechanism by which EXCLUDED.*
expressions are added is somewhat novel, and deserves to be
independently *considered*. I'm trying to show how the parts fit
together more so than breaking things down in to smaller commits (as
you picked up on, 0001 is the exception - that is genuinely intended
to be committed early). Also, those commit messages give me the
opportunity to put those parts in their appropriate context vis-a-vis
our discussions. They refer to the Wiki, for example, or reasons why
pg_stat_statements shouldn't care about ExcludedExpr. Obviously the
final commit messages won't look that way.
> 0001:
> * References INSERT with ON CONFLICT UPDATE, can thus not be committed
> independently. I don't think those references really are needed.
> * I'm not a fan of the increased code duplication in
> ExecCheckRTEPerms(). Can you look into cleaning that up?
> * Also the comments inside the ACL_INSERT case still reference UPDATE.
>
> Other than that I think we can just go ahead and commit this ahead of
> time. Mentioning ON CONFLICT UPDATE (OCU henceforth) in the commit
> message only.
Cool. Attached revision makes those changes.
> 0007:
> * "AMs" alone isn't particularly unique.
> * Without the context of the discussion "unprincipled deadlocks" aren't
> well defined.
> * Too many "" words.
> * Waiting "too long" isn't defined. Neither is why that'd imply
> unprincipled deadlocks. Somewhat understandable with the context of
> the discussion, but surely not a couple years down the road.
> * As is I don't find the README entry super helpful. It should state
> what the reason for doing this is cleary, start at the higher level,
> and then move to the details.
> * Misses details about the speculative heavyweight locking of tuples.
Fair points. I'll work through that feedback.
Actually, I think we should memorialize that "unprincipled deadlocks"
should be avoided in some more general way, since they are after all a
general problem that we've seen elsewhere. I'm not sure about how to
go about doing that, though.
> 0002:
> * Tentatively I'd say that killspeculative should be done via a separate
> function instead of heap_delete()
Really? I guess if that were to happen, it would entail refactoring
heap_delete() to call a static function, which was also called by a
new kill_speculative() function that does this. Otherwise, you'd have
far too much duplication.
> * I think we should, as you ponder in a comment, do the OCU specific
> stuff lazily and/or in a separate function from BuildIndexInfo(). That
> function is already quite visible in profiles, and the additional work
> isn't entirely trivial.
Okay.
> * I doubt logical decoding works with the patch as it stands.
I thought so. Perhaps you could suggest a better use of the available
XLOG_HEAP_* bits. I knew I needed to consider that more carefully
(hence the XXX comment), but didn't get around to it.
> * The added ereport (i.e. user facing) error message in
> ExecInsertIndexTuples won't help a user at all.
So, this:
>> /* Skip this index-update if the predicate isn't satisfied */
>> if (!ExecQual(predicate, econtext, false))
>> + {
>> + if (arbiterIdx == indexRelation->rd_index->indexrelid)
>> + ereport(ERROR,
>> + (errcode(ERRCODE_TRIGGERED_ACTION_EXCEPTION),
>> + errmsg("partial arbiter unique index has predicate
>> that does not cover tuple proposed for insertion"),
>> + errdetail("ON CONFLICT inference clause implies that
>> the tuple proposed for insertion actually be covered by partial predicate
>> for index \"%s\".",
>> +
>> RelationGetRelationName(indexRelation)),
>> + errhint("ON CONFLICT inference clause must infer a
>> unique index that covers the final tuple, after BEFORE ROW INSERT triggers
>> fire."),
>> + errtableconstraint(heapRelation,
>> +
>> RelationGetRelationName(indexRelation))));
>> continue;
>> + }
Yeah, that isn't a great error message. This happens here because you
are using a partial unique index (and so you must have had an
inference specification with a "WHERE" to get here). However, what you
actually went to insert would not be covered by this partial unique
index, and so couldn't ever take the alternative path, and so is
likely not thought out. Maybe it would be better to silently always
let the INSERT succeed as an INSERT. *That* actually wasn't really
discussed - this is all my idea.
> * Personally I don't care one iota for comments like "Get information
> from the result relation info structure.". Yes, one of these already
> exists, but ...
Okay.
> * If a arbiter index is passed to ExecCheckIndexConstraints(), can't we
> abort the loop after checking it? Also, do we really have to iterate
> over indexes for that case? How about moving the loop contents to a
> separate function and using that separately for the arbiter cases?
Well, the failure to do that implies very few extra cycles, but sure.
I'll add a new reason to break at the end, when
check_exclusion_or_unique_constraint() is called in respect of a
particular (inferred) arbiter unique index.
> * Don't like the comment above check_exclusion_or_unique_constraint()'s
> much. Makes too much of a special case of OCU
I guess I should just refer to speculative insertion.
> * ItemPointerIsValid
What about it?
> * ExecCheckHeapTupleVisible's comment "It is not acceptable to proceed "
> sounds like you're talking with a child or so ;)
Fair point. I should say "It would not be consistent with the
guarantees of the higher isolation levels..."
> * ExecCheckHeapTupleVisible()'s errhint() sounds like an
> argument/excuse (actually like a code comment). That's not going to
> help a user at all.
Really? I thought it might be less than intuitive that higher
isolation levels cannot decide to do nothing on the basis of something
not in their MVCC snapshot. But come to think of it, yeah, that
errhint() isn't adding much over the main error message.
> * I find the modified control flow in ExecInsert() pretty darn ugly. I
> think this needs to be cleaned up. The speculative case should imo be
> a separate function or something.
Hmm. I'm not quite sold on that. Basically, if we did that, we'd still
have a function that was more or less a strict superset of
ExecInsert(). What have we saved?
What I do agree with is that ExecInsert() should be refactored to make
the common case (a vanilla insert) look like the common case, whereas
the uncommon case (an upsert) should have that dealt with specially.
There is room for improvement. Is that a fair compromise?
> * /*
> * This may occur when an instantaneously invisible tuple is blamed
> * as a conflict because multiple rows are inserted with the same
> * constrained values.
> How can this happen? We don't insert multiple rows with the same
> command id?
This is a cardinality violation [1]. It can definitely happen - just
try the examples you see on the Wiki. This is possible because I
modified heap_lock_tuple() to return HeapTupleInvisible (and not just
complain directly when HeapTupleSatisfiesUpdate() returns
"HeapTupleInvisible"). It's also possible because we're using a
DirtySnapshot at various points. This is sort of like how ExecUpdate()
handles a return value of "HeapTupleSelfUpdated" from heap_update().
Not quite though, because 1. ) I prefer to throw an error (rather than
silently not UPDATE that slot), and 2. ) we're not dealing with MVCC
semantics, so the return values are different in both cases. The
*nature* of the situation handled is similar between UPSERTs (in
ExecLockUpdatedTuple()) and vanilla UPDATEs (in ExecUpdate()), though.
Does that make sense?
> * ExecLockUpdatedTuple() has (too?) many comments, but little overview
> of what/why it is doing what it does on a higher level.
Fair point. Seems like material for a better worked out executor README.
> * plan_speculative_use_index: "Use the planner to decide speculative
> insertion arbiter index" - Huh? " rel is the target to undergo ON
> CONFLICT UPDATE/IGNORE." - Which rel?
Sorry, that's an obsolete comment (the function signature changed). It
should refer to the target of the Query being planned.
> * formulations as "fundamental nexus" are hard to understand imo.
I'm trying to suggest that INSERT ... ON CONFLICT UPDATE is not quite
two separate top-level commands, and yet is also not a new, distinct
type of top-level command. This is mostly a high level design decision
that maximizes code reuse.
> * Perhaps it has previously been discussed but I'm not convinced by the
> reasoning for not looking at opclasses in infer_unique_index(). This
> seems like it'd prohibit ever having e.g. case insensitive opclasses -
> something surely worthwile.
I don't think anyone gave that idea the thumbs-up. However, I really
don't see the problem. Sure, we could have case insensitive opclasses
in the future, and you may want to make a unique index using one. But
then it becomes a matter of whatever unique indexes are available. The
limitation is only that you cannot explicitly indicate that you want a
certain opclass. It comes down to whatever unique indexes happen to be
available, since of course taking the alternative path is arbitrated
by a would-be unique violation. It's a bit odd that we're leaving it
up to the available indexes to decide on semantics like that, but the
problem is so narrow and the solution so involved that I'd argue it's
acceptable.
> * Doesn't infer_unique_index() have to look for indisvalid? This isn't
> going to work well with a invalid (not to speak for a !ready) index.
It does (check IndexIsValid()). I think the mistake I made here was
not checking IndexIsReady(), since that is an additional concern above
what the similar get_relation_info() function must consider.
> * Is ->relation in the UpdateStmt generated in transformInsertStmt ever
> used for anything? If so, it'd possibly generate some possible
> nastyness due to repeated name lookups. Looks like it'll be used in
> transformUpdateStmt
What, you mean security issues, for example? I have a hard time seeing
how that could work in practice, given that the one and only target
RTE is marked with the appropriate updatedCols originating from
transformUpdateStmt(). Still, it is a concern generally - better safe
than sorry. I was thinking of plugging it by ensuring that the
Relations matched, but that might not be good enough. Maybe it would
be better to bite the bullet and have transformUpdateStmt() use the
same Relation directly, which is something I hoped to avoid (better to
have transformUpdateStmt() know as little as possible about this, I'd
say).
> * What's the reason for the !pstate->p_parent? Also why the parens?
> pstate->p_is_speculative = (pstate->parentParseState &&
> (!pstate->p_parent_cte &&
> pstate->parentParseState->p_is_insert &&
> pstate->parentParseState->p_is_speculative));
You mean the "!pstate->p_parent_cte"? That's there because you can get
queries to segfault if this logic doesn't consider that a
data-modifying CTE can have an UPDATE that appears within a CTE
referenced from an INSERT. :-)
> * Why did you need to make %nonassoc DISTINCT and ON nonassoc in the
> grammar?
To prevent a shift/reduce conflict, I changed the associativity.
Without this, here are the details of State 700, which has the
conflict (from gram.output):
"""""
State 700
1465 opt_distinct: DISTINCT .
1466 | DISTINCT . ON '(' expr_list ')'
ON shift, and go to state 1094
ON [reduce using rule 1465 (opt_distinct)]
$default reduce using rule 1465 (opt_distinct)
"""""
> * The whole speculative insert logic isn't really well documented. Why,
> for example, do we actually need the token? And why are there no
> issues with overflow? And where is it documented that a 0 means
> there's no token? ...
Fair enough. Presumably it's okay that overflow theoretically could
occur, because a race is all but impossible. The token represents a
particular attempt by some backend at inserting a tuple, that needs to
be waited on specifically only if it is their active attempt (and the
xact is still running). Otherwise, you get unprincipled deadlocks.
Even if by some incredibly set of circumstances it wraps around, worst
case scenario you get an unprinciped deadlock, which is hardly the end
of the world given the immense number of insertions required, and the
immense unlikelihood that things would work out that way - it'd be
basically impossible.
I'll document the "0" thing.
> * Isn't "SpecType" a awfully generic (and nondescriptive) name?
OK. That'll be changed.
> * /* XXX: Make sure that re-use of bits is safe here */ - no, not
> unless you change existing checks.
I think that this is a restatement of your remarks on logical decoding. No?
> * /*
> * Immediately VACUUM "super-deleted" tuples
> */
> if (TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
> InvalidTransactionId))
> return HEAPTUPLE_DEAD;
> Is that branch really needed? Shouldn't it just be happening as a
> consequence of the already existing code? Same in SatisfiesMVCC. If
> you actually needed that block, it'd need to be done in SatisfiesSelf
> as well, no? You have a comment about a possible loop - but that seems
> wrong to me, implying that HEAP_XMIN_COMMITTED was set invalidly.
Indeed, this code is kind of odd. While I think the omission within
SatisfiesSelf() may be problematic too, if you really want to know why
this code is needed, uncomment it and run Jeff's stress-test. It will
reliably break.
This code:
"""""
if (HeapTupleHeaderXminInvalid(tuple))
return HEAPTUPLE_DEAD;
"""""
and this code:
"""""
/*
* Immediately VACUUM "super-deleted" tuples
*/
if (TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
InvalidTransactionId))
return HEAPTUPLE_DEAD;
"""""
are not equivalent (nor is the latter a superset of the former). Maybe
they should be, but they're not. What's more, heap tuple header raw
xmin has never been able to change, and I don't think there is any
reason for it to be InvalidTransactionId. See my new comments within
EvalPlanQualFetch() remarking on how it's now possible for that to
change (before, the comment claimed that it wasn't possible).
> Ok, I can't focus at all any further at this point. But there's enough
> comments here that some even might make sense ;)
Most do. :-)
Thanks.
[1]
https://wiki.postgresql.org/wiki/UPSERT#.22Cardinality_violation.22_errors_in_detail
--
Peter Geoghegan
From ce390514f6ac94fdb30f5930a658c92a6987e371 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <p...@heroku.com>
Date: Tue, 26 Aug 2014 21:28:40 -0700
Subject: [PATCH 1/8] Make UPDATE privileges distinct from INSERT privileges in
RTEs
Previously, relation range table entries used a single Bitmapset field
representing which columns required either UPDATE or INSERT privileges,
despite the fact that INSERT and UPDATE privileges are separately
cataloged, and may be independently held. This worked because
ExecCheckRTEPerms() was called with a ACL_INSERT or ACL_UPDATE
requiredPerms, and based on that it was evident which type of
optimizable statement was under consideration. Since historically no
type of optimizable statement could directly INSERT and UPDATE at the
same time, there was no ambiguity as to which privileges were required.
This largely mechanical commit is required infrastructure for the
INSERT...ON CONFLICT UPDATE feature, which introduces an optimizable
statement that may be subject to both INSERT and UPDATE permissions
enforcement. Tests follow in a later commit.
sepgsql is also affected by this commit. Note that this commit
necessitates an initdb, since stored ACLs are broken.
---
contrib/sepgsql/dml.c | 31 ++++++---
src/backend/commands/copy.c | 2 +-
src/backend/commands/createas.c | 2 +-
src/backend/commands/trigger.c | 22 +++---
src/backend/executor/execMain.c | 110 +++++++++++++++++++-----------
src/backend/nodes/copyfuncs.c | 3 +-
src/backend/nodes/equalfuncs.c | 3 +-
src/backend/nodes/outfuncs.c | 3 +-
src/backend/nodes/readfuncs.c | 3 +-
src/backend/optimizer/plan/setrefs.c | 6 +-
src/backend/optimizer/prep/prepsecurity.c | 6 +-
src/backend/optimizer/prep/prepunion.c | 8 ++-
src/backend/parser/analyze.c | 4 +-
src/backend/parser/parse_relation.c | 21 ++++--
src/backend/rewrite/rewriteHandler.c | 52 ++++++++------
src/include/nodes/parsenodes.h | 14 ++--
16 files changed, 176 insertions(+), 114 deletions(-)
diff --git a/contrib/sepgsql/dml.c b/contrib/sepgsql/dml.c
index 36c6a37..4a71753 100644
--- a/contrib/sepgsql/dml.c
+++ b/contrib/sepgsql/dml.c
@@ -145,7 +145,8 @@ fixup_inherited_columns(Oid parentId, Oid childId, Bitmapset *columns)
static bool
check_relation_privileges(Oid relOid,
Bitmapset *selected,
- Bitmapset *modified,
+ Bitmapset *inserted,
+ Bitmapset *updated,
uint32 required,
bool abort_on_violation)
{
@@ -231,8 +232,9 @@ check_relation_privileges(Oid relOid,
* Check permissions on the columns
*/
selected = fixup_whole_row_references(relOid, selected);
- modified = fixup_whole_row_references(relOid, modified);
- columns = bms_union(selected, modified);
+ inserted = fixup_whole_row_references(relOid, inserted);
+ updated = fixup_whole_row_references(relOid, updated);
+ columns = bms_union(selected, bms_union(inserted, updated));
while ((index = bms_first_member(columns)) >= 0)
{
@@ -241,13 +243,16 @@ check_relation_privileges(Oid relOid,
if (bms_is_member(index, selected))
column_perms |= SEPG_DB_COLUMN__SELECT;
- if (bms_is_member(index, modified))
+ if (bms_is_member(index, inserted))
{
- if (required & SEPG_DB_TABLE__UPDATE)
- column_perms |= SEPG_DB_COLUMN__UPDATE;
if (required & SEPG_DB_TABLE__INSERT)
column_perms |= SEPG_DB_COLUMN__INSERT;
}
+ if (bms_is_member(index, updated))
+ {
+ if (required & SEPG_DB_TABLE__UPDATE)
+ column_perms |= SEPG_DB_COLUMN__UPDATE;
+ }
if (column_perms == 0)
continue;
@@ -304,7 +309,7 @@ sepgsql_dml_privileges(List *rangeTabls, bool abort_on_violation)
required |= SEPG_DB_TABLE__INSERT;
if (rte->requiredPerms & ACL_UPDATE)
{
- if (!bms_is_empty(rte->modifiedCols))
+ if (!bms_is_empty(rte->updatedCols))
required |= SEPG_DB_TABLE__UPDATE;
else
required |= SEPG_DB_TABLE__LOCK;
@@ -333,7 +338,8 @@ sepgsql_dml_privileges(List *rangeTabls, bool abort_on_violation)
{
Oid tableOid = lfirst_oid(li);
Bitmapset *selectedCols;
- Bitmapset *modifiedCols;
+ Bitmapset *insertedCols;
+ Bitmapset *updatedCols;
/*
* child table has different attribute numbers, so we need to fix
@@ -341,15 +347,18 @@ sepgsql_dml_privileges(List *rangeTabls, bool abort_on_violation)
*/
selectedCols = fixup_inherited_columns(rte->relid, tableOid,
rte->selectedCols);
- modifiedCols = fixup_inherited_columns(rte->relid, tableOid,
- rte->modifiedCols);
+ insertedCols = fixup_inherited_columns(rte->relid, tableOid,
+ rte->insertedCols);
+ updatedCols = fixup_inherited_columns(rte->relid, tableOid,
+ rte->updatedCols);
/*
* check permissions on individual tables
*/
if (!check_relation_privileges(tableOid,
selectedCols,
- modifiedCols,
+ insertedCols,
+ updatedCols,
required, abort_on_violation))
return false;
}
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 92ff632..d2996fb 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -847,7 +847,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
FirstLowInvalidHeapAttributeNumber;
if (is_from)
- rte->modifiedCols = bms_add_member(rte->modifiedCols, attno);
+ rte->insertedCols = bms_add_member(rte->insertedCols, attno);
else
rte->selectedCols = bms_add_member(rte->selectedCols, attno);
}
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index c961429..bf2235d 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -433,7 +433,7 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
rte->requiredPerms = ACL_INSERT;
for (attnum = 1; attnum <= intoRelationDesc->rd_att->natts; attnum++)
- rte->modifiedCols = bms_add_member(rte->modifiedCols,
+ rte->insertedCols = bms_add_member(rte->insertedCols,
attnum - FirstLowInvalidHeapAttributeNumber);
ExecCheckRTPerms(list_make1(rte), true);
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 5c1c1be..7defe80 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -71,8 +71,8 @@ static int MyTriggerDepth = 0;
* it uses, so we let them be duplicated. Be sure to update both if one needs
* to be changed, however.
*/
-#define GetModifiedColumns(relinfo, estate) \
- (rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->modifiedCols)
+#define GetUpdatedColumns(relinfo, estate) \
+ (rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->updatedCols)
/* Local function prototypes */
static void ConvertTriggerToFK(CreateTrigStmt *stmt, Oid funcoid);
@@ -2343,7 +2343,7 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
TriggerDesc *trigdesc;
int i;
TriggerData LocTriggerData;
- Bitmapset *modifiedCols;
+ Bitmapset *updatedCols;
trigdesc = relinfo->ri_TrigDesc;
@@ -2352,7 +2352,7 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
if (!trigdesc->trig_update_before_statement)
return;
- modifiedCols = GetModifiedColumns(relinfo, estate);
+ updatedCols = GetUpdatedColumns(relinfo, estate);
LocTriggerData.type = T_TriggerData;
LocTriggerData.tg_event = TRIGGER_EVENT_UPDATE |
@@ -2373,7 +2373,7 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
TRIGGER_TYPE_UPDATE))
continue;
if (!TriggerEnabled(estate, relinfo, trigger, LocTriggerData.tg_event,
- modifiedCols, NULL, NULL))
+ updatedCols, NULL, NULL))
continue;
LocTriggerData.tg_trigger = trigger;
@@ -2398,7 +2398,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
if (trigdesc && trigdesc->trig_update_after_statement)
AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE,
false, NULL, NULL, NIL,
- GetModifiedColumns(relinfo, estate));
+ GetUpdatedColumns(relinfo, estate));
}
TupleTableSlot *
@@ -2416,7 +2416,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
HeapTuple oldtuple;
TupleTableSlot *newSlot;
int i;
- Bitmapset *modifiedCols;
+ Bitmapset *updatedCols;
Bitmapset *keyCols;
LockTupleMode lockmode;
@@ -2425,10 +2425,10 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
* been modified, then we can use a weaker lock, allowing for better
* concurrency.
*/
- modifiedCols = GetModifiedColumns(relinfo, estate);
+ updatedCols = GetUpdatedColumns(relinfo, estate);
keyCols = RelationGetIndexAttrBitmap(relinfo->ri_RelationDesc,
INDEX_ATTR_BITMAP_KEY);
- if (bms_overlap(keyCols, modifiedCols))
+ if (bms_overlap(keyCols, updatedCols))
lockmode = LockTupleExclusive;
else
lockmode = LockTupleNoKeyExclusive;
@@ -2482,7 +2482,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
TRIGGER_TYPE_UPDATE))
continue;
if (!TriggerEnabled(estate, relinfo, trigger, LocTriggerData.tg_event,
- modifiedCols, trigtuple, newtuple))
+ updatedCols, trigtuple, newtuple))
continue;
LocTriggerData.tg_trigtuple = trigtuple;
@@ -2552,7 +2552,7 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE,
true, trigtuple, newtuple, recheckIndexes,
- GetModifiedColumns(relinfo, estate));
+ GetUpdatedColumns(relinfo, estate));
if (trigtuple != fdw_trigtuple)
heap_freetuple(trigtuple);
}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 20b3188..f20f1e8 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -82,6 +82,9 @@ static void ExecutePlan(EState *estate, PlanState *planstate,
ScanDirection direction,
DestReceiver *dest);
static bool ExecCheckRTEPerms(RangeTblEntry *rte);
+static bool ExecCheckRTEPermsModified(Oid relOid, Oid userid,
+ Bitmapset *modifiedCols,
+ AclMode requiredPerms);
static void ExecCheckXactReadOnly(PlannedStmt *plannedstmt);
static char *ExecBuildSlotValueDescription(Oid reloid,
TupleTableSlot *slot,
@@ -97,8 +100,10 @@ static void EvalPlanQualStart(EPQState *epqstate, EState *parentestate,
* it uses, so we let them be duplicated. Be sure to update both if one needs
* to be changed, however.
*/
-#define GetModifiedColumns(relinfo, estate) \
- (rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->modifiedCols)
+#define GetUpdatedColumns(relinfo, estate) \
+ (rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->updatedCols)
+#define GetInsertedColumns(relinfo, estate) \
+ (rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->insertedCols)
/* end of local decls */
@@ -559,7 +564,6 @@ ExecCheckRTEPerms(RangeTblEntry *rte)
AclMode remainingPerms;
Oid relOid;
Oid userid;
- int col;
/*
* Only plain-relation RTEs need to be checked here. Function RTEs are
@@ -597,6 +601,8 @@ ExecCheckRTEPerms(RangeTblEntry *rte)
remainingPerms = requiredPerms & ~relPerms;
if (remainingPerms != 0)
{
+ int col = -1;
+
/*
* If we lack any permissions that exist only as relation permissions,
* we can fail straight away.
@@ -625,7 +631,6 @@ ExecCheckRTEPerms(RangeTblEntry *rte)
return false;
}
- col = -1;
while ((col = bms_next_member(rte->selectedCols, col)) >= 0)
{
/* bit #s are offset by FirstLowInvalidHeapAttributeNumber */
@@ -648,43 +653,63 @@ ExecCheckRTEPerms(RangeTblEntry *rte)
}
/*
- * Basically the same for the mod columns, with either INSERT or
- * UPDATE privilege as specified by remainingPerms.
+ * Basically the same for the mod columns, for both INSERT and UPDATE
+ * privilege as specified by remainingPerms.
*/
- remainingPerms &= ~ACL_SELECT;
- if (remainingPerms != 0)
- {
- /*
- * When the query doesn't explicitly change any columns, allow the
- * query if we have permission on any column of the rel. This is
- * to handle SELECT FOR UPDATE as well as possible corner cases in
- * INSERT and UPDATE.
- */
- if (bms_is_empty(rte->modifiedCols))
- {
- if (pg_attribute_aclcheck_all(relOid, userid, remainingPerms,
- ACLMASK_ANY) != ACLCHECK_OK)
- return false;
- }
+ if (remainingPerms & ACL_INSERT && !ExecCheckRTEPermsModified(relOid,
+ userid,
+ rte->insertedCols,
+ ACL_INSERT))
+ return false;
- col = -1;
- while ((col = bms_next_member(rte->modifiedCols, col)) >= 0)
- {
- /* bit #s are offset by FirstLowInvalidHeapAttributeNumber */
- AttrNumber attno = col + FirstLowInvalidHeapAttributeNumber;
+ if (remainingPerms & ACL_UPDATE && !ExecCheckRTEPermsModified(relOid,
+ userid,
+ rte->updatedCols,
+ ACL_UPDATE))
+ return false;
+ }
+ return true;
+}
- if (attno == InvalidAttrNumber)
- {
- /* whole-row reference can't happen here */
- elog(ERROR, "whole-row update is not implemented");
- }
- else
- {
- if (pg_attribute_aclcheck(relOid, attno, userid,
- remainingPerms) != ACLCHECK_OK)
- return false;
- }
- }
+/*
+ * ExecCheckRTEPermsModified
+ * Check INSERT or UPDATE access permissions for a single RTE (these
+ * are processed uniformly).
+ */
+static bool
+ExecCheckRTEPermsModified(Oid relOid, Oid userid, Bitmapset *modifiedCols,
+ AclMode requiredPerms)
+{
+ int col = -1;
+
+ /*
+ * When the query doesn't explicitly update any columns, allow the
+ * query if we have permission on any column of the rel. This is
+ * to handle SELECT FOR UPDATE as well as possible corner cases in
+ * UPDATE.
+ */
+ if (bms_is_empty(modifiedCols))
+ {
+ if (pg_attribute_aclcheck_all(relOid, userid, requiredPerms,
+ ACLMASK_ANY) != ACLCHECK_OK)
+ return false;
+ }
+
+ while ((col = bms_next_member(modifiedCols, col)) >= 0)
+ {
+ /* bit #s are offset by FirstLowInvalidHeapAttributeNumber */
+ AttrNumber attno = col + FirstLowInvalidHeapAttributeNumber;
+
+ if (attno == InvalidAttrNumber)
+ {
+ /* whole-row reference can't happen here */
+ elog(ERROR, "whole-row update is not implemented");
+ }
+ else
+ {
+ if (pg_attribute_aclcheck(relOid, attno, userid,
+ requiredPerms) != ACLCHECK_OK)
+ return false;
}
}
return true;
@@ -1623,7 +1648,8 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
char *val_desc;
Bitmapset *modifiedCols;
- modifiedCols = GetModifiedColumns(resultRelInfo, estate);
+ modifiedCols = GetUpdatedColumns(resultRelInfo, estate);
+ modifiedCols = bms_union(modifiedCols, GetInsertedColumns(resultRelInfo, estate));
val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
slot,
tupdesc,
@@ -1649,7 +1675,8 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
char *val_desc;
Bitmapset *modifiedCols;
- modifiedCols = GetModifiedColumns(resultRelInfo, estate);
+ modifiedCols = GetUpdatedColumns(resultRelInfo, estate);
+ modifiedCols = bms_union(modifiedCols, GetInsertedColumns(resultRelInfo, estate));
val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
slot,
tupdesc,
@@ -1708,7 +1735,8 @@ ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
char *val_desc;
Bitmapset *modifiedCols;
- modifiedCols = GetModifiedColumns(resultRelInfo, estate);
+ modifiedCols = GetUpdatedColumns(resultRelInfo, estate);
+ modifiedCols = bms_union(modifiedCols, GetInsertedColumns(resultRelInfo, estate));
val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
slot,
tupdesc,
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index f1a24f5..00ffe4a 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2028,7 +2028,8 @@ _copyRangeTblEntry(const RangeTblEntry *from)
COPY_SCALAR_FIELD(requiredPerms);
COPY_SCALAR_FIELD(checkAsUser);
COPY_BITMAPSET_FIELD(selectedCols);
- COPY_BITMAPSET_FIELD(modifiedCols);
+ COPY_BITMAPSET_FIELD(insertedCols);
+ COPY_BITMAPSET_FIELD(updatedCols);
COPY_NODE_FIELD(securityQuals);
return newnode;
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 6e8b308..79035b2 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2345,7 +2345,8 @@ _equalRangeTblEntry(const RangeTblEntry *a, const RangeTblEntry *b)
COMPARE_SCALAR_FIELD(requiredPerms);
COMPARE_SCALAR_FIELD(checkAsUser);
COMPARE_BITMAPSET_FIELD(selectedCols);
- COMPARE_BITMAPSET_FIELD(modifiedCols);
+ COMPARE_BITMAPSET_FIELD(insertedCols);
+ COMPARE_BITMAPSET_FIELD(updatedCols);
COMPARE_NODE_FIELD(securityQuals);
return true;
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index dd1278b..b4a2667 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2456,7 +2456,8 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
WRITE_UINT_FIELD(requiredPerms);
WRITE_OID_FIELD(checkAsUser);
WRITE_BITMAPSET_FIELD(selectedCols);
- WRITE_BITMAPSET_FIELD(modifiedCols);
+ WRITE_BITMAPSET_FIELD(insertedCols);
+ WRITE_BITMAPSET_FIELD(updatedCols);
WRITE_NODE_FIELD(securityQuals);
}
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index ae24d05..dbc162a 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1253,7 +1253,8 @@ _readRangeTblEntry(void)
READ_UINT_FIELD(requiredPerms);
READ_OID_FIELD(checkAsUser);
READ_BITMAPSET_FIELD(selectedCols);
- READ_BITMAPSET_FIELD(modifiedCols);
+ READ_BITMAPSET_FIELD(insertedCols);
+ READ_BITMAPSET_FIELD(updatedCols);
READ_NODE_FIELD(securityQuals);
READ_DONE();
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 7703946..5d865b0 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -368,9 +368,9 @@ flatten_rtes_walker(Node *node, PlannerGlobal *glob)
*
* In the flat rangetable, we zero out substructure pointers that are not
* needed by the executor; this reduces the storage space and copying cost
- * for cached plans. We keep only the alias and eref Alias fields, which
- * are needed by EXPLAIN, and the selectedCols and modifiedCols bitmaps,
- * which are needed for executor-startup permissions checking and for
+ * for cached plans. We keep only the alias and eref Alias fields, which are
+ * needed by EXPLAIN, and the selectedCols, insertedCols and updatedCols
+ * bitmaps, which are needed for executor-startup permissions checking and for
* trigger event checking.
*/
static void
diff --git a/src/backend/optimizer/prep/prepsecurity.c b/src/backend/optimizer/prep/prepsecurity.c
index af3ee61..f86e792 100644
--- a/src/backend/optimizer/prep/prepsecurity.c
+++ b/src/backend/optimizer/prep/prepsecurity.c
@@ -115,7 +115,8 @@ expand_security_quals(PlannerInfo *root, List *tlist)
rte->requiredPerms = 0;
rte->checkAsUser = InvalidOid;
rte->selectedCols = NULL;
- rte->modifiedCols = NULL;
+ rte->insertedCols = NULL;
+ rte->updatedCols = NULL;
/*
* For the most part, Vars referencing the original relation
@@ -213,7 +214,8 @@ expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
rte->requiredPerms = 0;
rte->checkAsUser = InvalidOid;
rte->selectedCols = NULL;
- rte->modifiedCols = NULL;
+ rte->insertedCols = NULL;
+ rte->updatedCols = NULL;
/*
* Now deal with any PlanRowMark on this RTE by requesting a lock
diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index 05f601e..1e28363 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -1367,14 +1367,16 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
* if this is the parent table, leave copyObject's result alone.
*
* Note: we need to do this even though the executor won't run any
- * permissions checks on the child RTE. The modifiedCols bitmap may
- * be examined for trigger-firing purposes.
+ * permissions checks on the child RTE. The insertedCols/updatedCols
+ * bitmaps may be examined for trigger-firing purposes.
*/
if (childOID != parentOID)
{
childrte->selectedCols = translate_col_privs(rte->selectedCols,
appinfo->translated_vars);
- childrte->modifiedCols = translate_col_privs(rte->modifiedCols,
+ childrte->insertedCols = translate_col_privs(rte->insertedCols,
+ appinfo->translated_vars);
+ childrte->updatedCols = translate_col_privs(rte->updatedCols,
appinfo->translated_vars);
}
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index a68f2e8..df89065 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -733,7 +733,7 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt)
false);
qry->targetList = lappend(qry->targetList, tle);
- rte->modifiedCols = bms_add_member(rte->modifiedCols,
+ rte->insertedCols = bms_add_member(rte->insertedCols,
attr_num - FirstLowInvalidHeapAttributeNumber);
icols = lnext(icols);
@@ -2002,7 +2002,7 @@ transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt)
origTarget->location);
/* Mark the target column as requiring update permissions */
- target_rte->modifiedCols = bms_add_member(target_rte->modifiedCols,
+ target_rte->updatedCols = bms_add_member(target_rte->updatedCols,
attrno - FirstLowInvalidHeapAttributeNumber);
origTargetList = lnext(origTargetList);
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 8d4f79f..d2820d8 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -1052,7 +1052,8 @@ addRangeTableEntry(ParseState *pstate,
rte->requiredPerms = ACL_SELECT;
rte->checkAsUser = InvalidOid; /* not set-uid by default, either */
rte->selectedCols = NULL;
- rte->modifiedCols = NULL;
+ rte->insertedCols = NULL;
+ rte->updatedCols = NULL;
/*
* Add completed RTE to pstate's range table list, but not to join list
@@ -1105,7 +1106,8 @@ addRangeTableEntryForRelation(ParseState *pstate,
rte->requiredPerms = ACL_SELECT;
rte->checkAsUser = InvalidOid; /* not set-uid by default, either */
rte->selectedCols = NULL;
- rte->modifiedCols = NULL;
+ rte->insertedCols = NULL;
+ rte->updatedCols = NULL;
/*
* Add completed RTE to pstate's range table list, but not to join list
@@ -1183,7 +1185,8 @@ addRangeTableEntryForSubquery(ParseState *pstate,
rte->requiredPerms = 0;
rte->checkAsUser = InvalidOid;
rte->selectedCols = NULL;
- rte->modifiedCols = NULL;
+ rte->insertedCols = NULL;
+ rte->updatedCols = NULL;
/*
* Add completed RTE to pstate's range table list, but not to join list
@@ -1437,7 +1440,8 @@ addRangeTableEntryForFunction(ParseState *pstate,
rte->requiredPerms = 0;
rte->checkAsUser = InvalidOid;
rte->selectedCols = NULL;
- rte->modifiedCols = NULL;
+ rte->insertedCols = NULL;
+ rte->updatedCols = NULL;
/*
* Add completed RTE to pstate's range table list, but not to join list
@@ -1509,7 +1513,8 @@ addRangeTableEntryForValues(ParseState *pstate,
rte->requiredPerms = 0;
rte->checkAsUser = InvalidOid;
rte->selectedCols = NULL;
- rte->modifiedCols = NULL;
+ rte->insertedCols = NULL;
+ rte->updatedCols = NULL;
/*
* Add completed RTE to pstate's range table list, but not to join list
@@ -1577,7 +1582,8 @@ addRangeTableEntryForJoin(ParseState *pstate,
rte->requiredPerms = 0;
rte->checkAsUser = InvalidOid;
rte->selectedCols = NULL;
- rte->modifiedCols = NULL;
+ rte->insertedCols = NULL;
+ rte->updatedCols = NULL;
/*
* Add completed RTE to pstate's range table list, but not to join list
@@ -1677,7 +1683,8 @@ addRangeTableEntryForCTE(ParseState *pstate,
rte->requiredPerms = 0;
rte->checkAsUser = InvalidOid;
rte->selectedCols = NULL;
- rte->modifiedCols = NULL;
+ rte->insertedCols = NULL;
+ rte->updatedCols = NULL;
/*
* Add completed RTE to pstate's range table list, but not to join list
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index b8e6e7a..fab2948 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1403,7 +1403,8 @@ ApplyRetrieveRule(Query *parsetree,
rte->requiredPerms = 0;
rte->checkAsUser = InvalidOid;
rte->selectedCols = NULL;
- rte->modifiedCols = NULL;
+ rte->insertedCols = NULL;
+ rte->updatedCols = NULL;
/*
* For the most part, Vars referencing the view should remain as
@@ -1466,12 +1467,14 @@ ApplyRetrieveRule(Query *parsetree,
subrte->requiredPerms = rte->requiredPerms;
subrte->checkAsUser = rte->checkAsUser;
subrte->selectedCols = rte->selectedCols;
- subrte->modifiedCols = rte->modifiedCols;
+ subrte->insertedCols = rte->insertedCols;
+ subrte->updatedCols = rte->updatedCols;
rte->requiredPerms = 0; /* no permission check on subquery itself */
rte->checkAsUser = InvalidOid;
rte->selectedCols = NULL;
- rte->modifiedCols = NULL;
+ rte->insertedCols = NULL;
+ rte->updatedCols = NULL;
/*
* If FOR [KEY] UPDATE/SHARE of view, mark all the contained tables as
@@ -2584,9 +2587,9 @@ rewriteTargetView(Query *parsetree, Relation view)
/*
* For INSERT/UPDATE the modified columns must all be updatable. Note that
* we get the modified columns from the query's targetlist, not from the
- * result RTE's modifiedCols set, since rewriteTargetListIU may have added
- * additional targetlist entries for view defaults, and these must also be
- * updatable.
+ * result RTE's insertedCols and/or updatedCols set, since
+ * rewriteTargetListIU may have added additional targetlist entries for
+ * view defaults, and these must also be updatable.
*/
if (parsetree->commandType != CMD_DELETE)
{
@@ -2723,26 +2726,31 @@ rewriteTargetView(Query *parsetree, Relation view)
*
* Initially, new_rte contains selectedCols permission check bits for all
* base-rel columns referenced by the view, but since the view is a SELECT
- * query its modifiedCols is empty. We set modifiedCols to include all
- * the columns the outer query is trying to modify, adjusting the column
- * numbers as needed. But we leave selectedCols as-is, so the view owner
- * must have read permission for all columns used in the view definition,
- * even if some of them are not read by the outer query. We could try to
- * limit selectedCols to only columns used in the transformed query, but
- * that does not correspond to what happens in ordinary SELECT usage of a
- * view: all referenced columns must have read permission, even if
- * optimization finds that some of them can be discarded during query
- * transformation. The flattening we're doing here is an optional
- * optimization, too. (If you are unpersuaded and want to change this,
- * note that applying adjust_view_column_set to view_rte->selectedCols is
- * clearly *not* the right answer, since that neglects base-rel columns
- * used in the view's WHERE quals.)
+ * query its insertedCols/updatedCols is empty. We set insertedCols and
+ * updatedCols to include all the columns the outer query is trying to
+ * modify, adjusting the column numbers as needed. But we leave
+ * selectedCols as-is, so the view owner must have read permission for all
+ * columns used in the view definition, even if some of them are not read
+ * by the outer query. We could try to limit selectedCols to only columns
+ * used in the transformed query, but that does not correspond to what
+ * happens in ordinary SELECT usage of a view: all referenced columns must
+ * have read permission, even if optimization finds that some of them can
+ * be discarded during query transformation. The flattening we're doing
+ * here is an optional optimization, too. (If you are unpersuaded and want
+ * to change this, note that applying adjust_view_column_set to
+ * view_rte->selectedCols is clearly *not* the right answer, since that
+ * neglects base-rel columns used in the view's WHERE quals.)
*
* This step needs the modified view targetlist, so we have to do things
* in this order.
*/
- Assert(bms_is_empty(new_rte->modifiedCols));
- new_rte->modifiedCols = adjust_view_column_set(view_rte->modifiedCols,
+ Assert(bms_is_empty(new_rte->insertedCols) &&
+ bms_is_empty(new_rte->updatedCols));
+
+ new_rte->insertedCols = adjust_view_column_set(view_rte->insertedCols,
+ view_targetlist);
+
+ new_rte->updatedCols = adjust_view_column_set(view_rte->updatedCols,
view_targetlist);
/*
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index b1dfa85..86d1c07 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -717,11 +717,12 @@ typedef struct XmlSerialize
* For SELECT/INSERT/UPDATE permissions, if the user doesn't have
* table-wide permissions then it is sufficient to have the permissions
* on all columns identified in selectedCols (for SELECT) and/or
- * modifiedCols (for INSERT/UPDATE; we can tell which from the query type).
- * selectedCols and modifiedCols are bitmapsets, which cannot have negative
- * integer members, so we subtract FirstLowInvalidHeapAttributeNumber from
- * column numbers before storing them in these fields. A whole-row Var
- * reference is represented by setting the bit for InvalidAttrNumber.
+ * insertedCols and/or updatedCols (INSERT with ON CONFLICT UPDATE may
+ * have all 3). selectedCols, insertedCols and updatedCols are
+ * bitmapsets, which cannot have negative integer members, so we subtract
+ * FirstLowInvalidHeapAttributeNumber from column numbers before storing
+ * them in these fields. A whole-row Var reference is represented by
+ * setting the bit for InvalidAttrNumber.
*--------------------
*/
typedef enum RTEKind
@@ -816,7 +817,8 @@ typedef struct RangeTblEntry
AclMode requiredPerms; /* bitmask of required access permissions */
Oid checkAsUser; /* if valid, check access as this role */
Bitmapset *selectedCols; /* columns needing SELECT permission */
- Bitmapset *modifiedCols; /* columns needing INSERT/UPDATE permission */
+ Bitmapset *insertedCols; /* columns needing INSERT permission */
+ Bitmapset *updatedCols; /* columns needing UPDATE permission */
List *securityQuals; /* any security barrier quals to apply */
} RangeTblEntry;
--
1.9.1
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
