Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Stephen Frost]

Peter, all,

* Peter Geoghegan (p...@heroku.com) wrote:
> I see now that commit 4f3b2a8883 changed things for UPDATE and DELETE
> statements, but not INSERT statements. I guess my unease is because
> that isn't entirely consistent with INSERT + RETURNING and the GRANT
> system. Logically, the only possible justification for our long
> standing INSERT and RETURNING behavior with GRANT (the fact that it
> requires SELECT privilege for rows returned, just like UPDATE and
> DELETE) is that before row insert triggers could do something secret
> (e.g. they could be security definer). It doesn't seem to be too much
> of a stretch to suppose the same should apply with RLS.

I had intended to address with policies what is addressed through
permissions with 7d8db3e, but the coverage for INSERT+RETURNING was only
done when ON CONFLICT was in use.

I've fixed that by applying the SELECT policies as WCOs for both the
INSERT and UPDATE RETURNING cases.  This matches the permissions system,
where we require SELECT rights on the table for an INSERT RETURNING
query.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Andres Freund]

On 2015-10-05 08:01:00 -0400, Stephen Frost wrote:
> Peter, all,
> I had intended to address with policies what is addressed through
> permissions with 7d8db3e, but the coverage for INSERT+RETURNING was only
> done when ON CONFLICT was in use.
> 
> I've fixed that by applying the SELECT policies as WCOs for both the
> INSERT and UPDATE RETURNING cases.  This matches the permissions system,
> where we require SELECT rights on the table for an INSERT RETURNING
> query.

This really needs tests verifying the behaviour...

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Stephen Frost]

Andres,

On Monday, October 5, 2015, Andres Freund  wrote:

> On 2015-10-05 08:01:00 -0400, Stephen Frost wrote:
> > Peter, all,
> > I had intended to address with policies what is addressed through
> > permissions with 7d8db3e, but the coverage for INSERT+RETURNING was only
> > done when ON CONFLICT was in use.
> >
> > I've fixed that by applying the SELECT policies as WCOs for both the
> > INSERT and UPDATE RETURNING cases.  This matches the permissions system,
> > where we require SELECT rights on the table for an INSERT RETURNING
> > query.
>
> This really needs tests verifying the behaviour...
>

Good point, will add.

Thanks!

Stephen

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Stephen Frost]

* Andres Freund (and...@anarazel.de) wrote:
> On 2015-10-05 08:01:00 -0400, Stephen Frost wrote:
> > Peter, all,
> > I had intended to address with policies what is addressed through
> > permissions with 7d8db3e, but the coverage for INSERT+RETURNING was only
> > done when ON CONFLICT was in use.
> > 
> > I've fixed that by applying the SELECT policies as WCOs for both the
> > INSERT and UPDATE RETURNING cases.  This matches the permissions system,
> > where we require SELECT rights on the table for an INSERT RETURNING
> > query.
> 
> This really needs tests verifying the behaviour...

Added.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > I had intended to address with policies what is addressed through
> > permissions with 7d8db3e, but the coverage for INSERT+RETURNING was only
> > done when ON CONFLICT was in use.
> 
> > I've fixed that by applying the SELECT policies as WCOs for both the
> > INSERT and UPDATE RETURNING cases.  This matches the permissions system,
> > where we require SELECT rights on the table for an INSERT RETURNING
> > query.
> 
> What of DELETE RETURNING?

That was handled in 7d8db3e.

Per previous discussion, UPDATE and DELETE RETURNING apply SELECT 
policies as security quals, meaning only the records visible through the
SELECT policy are eligible for consideration.  INSERT+RETURNING has only
WithCheckOptions, no security quals, which is what makes it different
from the other cases.  The INSERT+ON CONFLICT+RETURNING case had been
covered already and I had mistakenly thought it was also covering
INSERT+RETURNING.  In fixing that, I realized that Peter makes a good
point that UPDATE+RETURNING should also have SELECT policies applied as
WithCheckOptions.

I'm about to push updated regression tests, as suggested by Andres.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Tom Lane]

Stephen Frost  writes:
> I had intended to address with policies what is addressed through
> permissions with 7d8db3e, but the coverage for INSERT+RETURNING was only
> done when ON CONFLICT was in use.

> I've fixed that by applying the SELECT policies as WCOs for both the
> INSERT and UPDATE RETURNING cases.  This matches the permissions system,
> where we require SELECT rights on the table for an INSERT RETURNING
> query.

What of DELETE RETURNING?

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Andres Freund]

> My proposal in this WIP patch is to make it a bit clearer that
> 'EXCLUDED' isn't a real relation. I played around with adding a
> different rtekind, but that's too heavy a hammer. What I instead did was
> to set relkind to composite - which seems to signal pretty well that
> we're not dealing with a real relation. That immediately fixes the RLS
> issue as fireRIRrules has the following check:
>   if (rte->rtekind != RTE_RELATION ||
>   rte->relkind != RELKIND_RELATION)
>   continue;
> It also makes it relatively straightforward to fix the system column
> issue by adding an additional relkind check to scanRTEForColumn's system
> column handling.

That works, but also precludes referencing 'oid' in a WITH OIDs table
via EXCLUDED.oid - to me that looks correct since a to-be-inserted row
can't yet have an oid assigned. Differing opinions?

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Andres Freund]

On 2015-09-29 15:49:28 -0400, Stephen Frost wrote:
> From Andres' reply, it looks like this is about the EXCLUDED pseudo
> relation which comes from the INSERT'd values themselves

Right.

> in which case, I tend to agree with his assessment that it doesn't
> make sense for those to be subject to RLS policies, given that it's
> all user-provided data, as long as the USING check is done on the row
> found to be conflicting and the CHECK constraints are dealt with
> correctly for any row added, which I believe is what we had agreed was
> the correct way to handle this case in prior discussions.

Yes, that what I think as well.  At this point we'll already have
executed insert rls stuff on the EXCLUDED tuple:
/*
 * Check any RLS INSERT WITH CHECK policies
 *
 * ExecWithCheckOptions() will skip any WCOs which are not of 
the kind
 * we are looking for at this point.
 */
if (resultRelInfo->ri_WithCheckOptions != NIL)
ExecWithCheckOptions(WCO_RLS_INSERT_CHECK,
 resultRelInfo, 
slot, estate);
and before executing the actual projection we also checked the existing
tuple:
ExecWithCheckOptions(WCO_RLS_CONFLICT_CHECK, resultRelInfo,
 mtstate->mt_existing,
 mtstate->ps.state);

after the update triggers have, if applicable run, we run the the normal
checks there as well because it's just ExecUpdate()
if (resultRelInfo->ri_WithCheckOptions != NIL)
ExecWithCheckOptions(WCO_RLS_UPDATE_CHECK,
 resultRelInfo, 
slot, estate);

so I do indeed think that there's no point in layering RLS above
EXCLUDED.

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Andres Freund]

On 2015-09-29 11:52:14 -0700, Peter Geoghegan wrote:
> On Tue, Sep 29, 2015 at 8:24 AM, Andres Freund  wrote:
> > So, took a bit longer than "tomorrow. I fought for a long while with a
> > mysterious issue, which turned out to be separate bug: The excluded
> > relation was affected by row level security policies, which doesn't make
> > sense.
> 
> Why? You certainly thought that it made sense for conventional column
> permissions due to potential problems with before row insert triggers.

I don't see how those compare:

> I specifically remember discussing this with you off list (on IM,
> roughly a couple of weeks prior to initial commit). I recommended that
> we err towards a more restrictive behavior in the absence of any
> strong principle pushing us one way or the other. You seemed to agree.

I don't think this really is comparable. Comparing this with a plain
INSERT or UPDATE this would be akin to running RLS on the RETURNING
tuple - which we currently don't.

I think this is was just a bug.

> I suppose that we have a tight enough grip on the targetlist that it's
> hard to imagine anything else being introduced there spuriously. I had
> thought that the pull-up did allow useful additional
> defense/sanitization, but that may not be an excellent argument. The
> only remaining argument is that my approach is closer to RETURNING,
> but that doesn't seem like an excellent argument.

I indeed don't think this is comparable to RETURNING - the pullup there
is into an actual querytree above unrelated relations.

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Peter Geoghegan]

On Thu, Oct 1, 2015 at 3:42 AM, Andres Freund  wrote:
> Yes, that what I think as well.  At this point we'll already have
> executed insert rls stuff on the EXCLUDED tuple:
> /*
>  * Check any RLS INSERT WITH CHECK policies
>  *
>  * ExecWithCheckOptions() will skip any WCOs which are not of 
> the kind
>  * we are looking for at this point.
>  */
> if (resultRelInfo->ri_WithCheckOptions != NIL)
> ExecWithCheckOptions(WCO_RLS_INSERT_CHECK,
>  
> resultRelInfo, slot, estate);
> and before executing the actual projection we also checked the existing
> tuple:
> ExecWithCheckOptions(WCO_RLS_CONFLICT_CHECK, resultRelInfo,
>  mtstate->mt_existing,
>  mtstate->ps.state);
>
> after the update triggers have, if applicable run, we run the the normal
> checks there as well because it's just ExecUpdate()
> if (resultRelInfo->ri_WithCheckOptions != NIL)
> ExecWithCheckOptions(WCO_RLS_UPDATE_CHECK,
>  
> resultRelInfo, slot, estate);
>
> so I do indeed think that there's no point in layering RLS above
> EXCLUDED.

I see your point, I think. It might be a problem if we weren't already
making the statement error out, but we are.

However, we're checking the excluded tuple (the might-be-inserted,
might-be-excluded tuple that reflects before row insert trigger
effects) with WCO_RLS_INSERT_CHECK, not WCO_RLS_UPDATE_CHECK. The
WCO_RLS_UPDATE_CHECK applies to the tuple to be appended to the
relation (the tuple that an UPDATE makes supersede some existing
tuple, a new row version).

We all seem to be in agreement that excluded.* ought to be subject to
column-level privilege enforcement, mostly due to possible leaks with
before row insert triggers (these could be SoD; a malicious UPSERT
could be written a certain way). None of the checks in the code above
are the exact RLS equivalent of the principle we have for column
privileges, AFAICT, because update-applicable policies (everything but
insert-applicable policies, actually) are not checked against the
excluded tuple. Shouldn't select-applicable policies also be applied
to the excluded tuples, just as with UPDATE ... FROM "join from"
tables, which excluded is kinda similar to?

I'm not trying to be pedantic; I just don't grok the underlying
principles here. Couldn't a malicious WHERE clause leak the excluded.*
tuple contents (and cause the UPDATE to not proceed) before the
WCO_RLS_CONFLICT_CHECK call site was reached, while also preventing it
from ever actually being reached (with a malicious function that
returns false after stashing excluded.* elsewhere)? You can put
volatile functions in UPDATE WHERE clauses, even if it is generally a
bad idea.

Perhaps I'm simply not following you here, though. I think that this
is one challenge with having per-command policies with a system that
checks permissions dynamically (not during parse analysis). Note that
I'm not defending the status quo of the master branch -- I'm just a
little uneasy about what the ideal, least surprising behavior is here.

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Peter Geoghegan]

On Thu, Oct 1, 2015 at 3:53 AM, Andres Freund  wrote:
>> I specifically remember discussing this with you off list (on IM,
>> roughly a couple of weeks prior to initial commit). I recommended that
>> we err towards a more restrictive behavior in the absence of any
>> strong principle pushing us one way or the other. You seemed to agree.
>
> I don't think this really is comparable. Comparing this with a plain
> INSERT or UPDATE this would be akin to running RLS on the RETURNING
> tuple - which we currently don't.
>
> I think this is was just a bug.

Maybe that's the problem here; I still thought that we were planning
on changing RLS in this regard, but it actually seems we changed
course, looking at the 9.5 open items list.

I would say that that's a clear divergence between RLS and column
privileges. That might be fine, but it doesn't match my prior
understanding of RLS (or, more accurately, how it was likely to change
pre-release).

If that's the design that we want for RLS across the board, then I'm
happy to defer to that decision.

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Peter Geoghegan]

On Thu, Oct 1, 2015 at 4:17 PM, Andres Freund  wrote:
> On 2015-10-01 16:13:12 -0700, Peter Geoghegan wrote:
>> However, we're checking the excluded tuple (the might-be-inserted,
>> might-be-excluded tuple that reflects before row insert trigger
>> effects) with WCO_RLS_INSERT_CHECK, not WCO_RLS_UPDATE_CHECK. The
>> WCO_RLS_UPDATE_CHECK applies to the tuple to be appended to the
>> relation (the tuple that an UPDATE makes supersede some existing
>> tuple, a new row version).
>
> You can already see the effects of an INSERT modified by before triggers
> via RETURNING. No?

I'm not saying that I agree with the decision to not do anything
special with RLS + RETURNING in general. I'm also not going to say
that I disagree with it. As I said, I missed that decision until just
now. I agree that it's obviously true that what you propose is
consistent with what is now considered ideal behavior for RLS (that's
what I get from the wiki page comments on RLS + RETURNING).

FWIW, I think that this technically wasn't a bug, because it was based
on a deliberate design decision that I thought (not without
justification) was consistent with what we wanted for RLS across the
board. But, again, happy to go along with what you say in light of
this new information.

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Peter Geoghegan]

On Thu, Oct 1, 2015 at 4:26 PM, Peter Geoghegan  wrote:
>> You can already see the effects of an INSERT modified by before triggers
>> via RETURNING. No?
>
> I'm not saying that I agree with the decision to not do anything
> special with RLS + RETURNING in general. I'm also not going to say
> that I disagree with it. As I said, I missed that decision until just
> now. I agree that it's obviously true that what you propose is
> consistent with what is now considered ideal behavior for RLS (that's
> what I get from the wiki page comments on RLS + RETURNING).

I see now that commit 4f3b2a8883 changed things for UPDATE and DELETE
statements, but not INSERT statements. I guess my unease is because
that isn't entirely consistent with INSERT + RETURNING and the GRANT
system. Logically, the only possible justification for our long
standing INSERT and RETURNING behavior with GRANT (the fact that it
requires SELECT privilege for rows returned, just like UPDATE and
DELETE) is that before row insert triggers could do something secret
(e.g. they could be security definer). It doesn't seem to be too much
of a stretch to suppose the same should apply with RLS.


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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Andres Freund]

On 2015-10-01 16:13:12 -0700, Peter Geoghegan wrote:
> However, we're checking the excluded tuple (the might-be-inserted,
> might-be-excluded tuple that reflects before row insert trigger
> effects) with WCO_RLS_INSERT_CHECK, not WCO_RLS_UPDATE_CHECK. The
> WCO_RLS_UPDATE_CHECK applies to the tuple to be appended to the
> relation (the tuple that an UPDATE makes supersede some existing
> tuple, a new row version).

You can already see the effects of an INSERT modified by before triggers
via RETURNING. No?

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Andres Freund]

On 2015-10-01 16:26:07 -0700, Peter Geoghegan wrote:
> FWIW, I think that this technically wasn't a bug

Meh. In which scenario would do a policy applied to EXCLUDED actually
anything reasonable?


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Peter Geoghegan]

On Thu, Oct 1, 2015 at 4:49 PM, Andres Freund  wrote:
> On 2015-10-01 16:26:07 -0700, Peter Geoghegan wrote:
>> FWIW, I think that this technically wasn't a bug
>
> Meh. In which scenario would do a policy applied to EXCLUDED actually
> anything reasonable?

I agree that it's very unlikely to matter. Consistency is something
that is generally valued, though.

I'm not going to object if you want to continue with committing
something that changes excluded + RLS. I was just explaining my view
of the matter.

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Andres Freund]

On 2015-10-01 16:55:23 -0700, Peter Geoghegan wrote:
> On Thu, Oct 1, 2015 at 4:49 PM, Andres Freund  wrote:
> > On 2015-10-01 16:26:07 -0700, Peter Geoghegan wrote:
> >> FWIW, I think that this technically wasn't a bug
> >
> > Meh. In which scenario would do a policy applied to EXCLUDED actually
> > anything reasonable?
> 
> I agree that it's very unlikely to matter. Consistency is something
> that is generally valued, though.

I don't think you get my gist.

I'm can't see how the current code can do anything sensible at all. What
do you think is going to be the effect of an excluded row that doesn't
meet security quals? Even if it worked in the sense that the correct
data were accessed and every - which I doubt is completely the case as
things stands given there's no actual scan node and stuff - you'd still
have EXCLUDED.* being used in the projection for the new version of the
tuple.

As far as I can see the only correct thing you could do in that
situation is error out.

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Peter Geoghegan]

On Thu, Oct 1, 2015 at 5:12 PM, Andres Freund  wrote:
> I'm can't see how the current code can do anything sensible at all. What
> do you think is going to be the effect of an excluded row that doesn't
> meet security quals? Even if it worked in the sense that the correct
> data were accessed and every - which I doubt is completely the case as
> things stands given there's no actual scan node and stuff - you'd still
> have EXCLUDED.* being used in the projection for the new version of the
> tuple.
>
> As far as I can see the only correct thing you could do in that
> situation is error out.

I agree. I wasn't defending the current code (although that might have
been made unclear by the "technically wasn't a bug" remark).

Note that I'm not telling you what I think needs to happen. I'm just
explaining my understanding of what has happened.

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Andres Freund]

On 2015-09-24 17:25:21 +0200, Andres Freund wrote:
> Stuff I want to fix by tomorrow:
> * Whole row var references to exclude
> * wrong offsets for columns after dropped ones
> * INSTEAD DO UPDATE for tables with oids
>
> Do you know of anything else?

So, took a bit longer than "tomorrow. I fought for a long while with a
mysterious issue, which turned out to be separate bug: The excluded
relation was affected by row level security policies, which doesn't make
sense.

My proposal in this WIP patch is to make it a bit clearer that
'EXCLUDED' isn't a real relation. I played around with adding a
different rtekind, but that's too heavy a hammer. What I instead did was
to set relkind to composite - which seems to signal pretty well that
we're not dealing with a real relation. That immediately fixes the RLS
issue as fireRIRrules has the following check:
if (rte->rtekind != RTE_RELATION ||
rte->relkind != RELKIND_RELATION)
continue;
It also makes it relatively straightforward to fix the system column
issue by adding an additional relkind check to scanRTEForColumn's system
column handling.

WRT to the wholerow issue: There's currently two reasons we need a
targetlist entry for excluded wholerow vars: 1) setrefs.c errors out
without - that can relativley easily be worked around 2) ruleutils.c
expects an entry in the child tlist. That could also be worked around,
but it's a bit more verbose.  I'm inclined to not go the pullup route
but instead simply unconditionally add a wholerow var to the excluded
tlist.

Peter, what do you think?

Andres
>From f11fc12993beabf4d280b979c062682b6612c989 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Tue, 29 Sep 2015 17:08:36 +0200
Subject: [PATCH] wip-upsert

---
 src/backend/parser/analyze.c  |  76 +++
 src/backend/parser/parse_relation.c   |   7 +-
 src/test/regress/expected/insert_conflict.out | 101 ++
 src/test/regress/expected/rules.out   |  18 ++---
 src/test/regress/sql/insert_conflict.sql  |  56 ++
 src/test/regress/sql/rules.sql|   2 +-
 6 files changed, 234 insertions(+), 26 deletions(-)

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index a0dfbf9..528825a 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -891,33 +891,81 @@ transformOnConflictClause(ParseState *pstate,
 	/* Process DO UPDATE */
 	if (onConflictClause->action == ONCONFLICT_UPDATE)
 	{
+		Relation targetrel = pstate->p_target_relation;
+		Var	   *var;
+		TargetEntry *te;
+		int		attno;
+
 		/*
 		 * All INSERT expressions have been parsed, get ready for potentially
 		 * existing SET statements that need to be processed like an UPDATE.
 		 */
 		pstate->p_is_insert = false;
 
+		/*
+		 * Add range table entry for the EXCLUDED pseudo relation; relkind is
+		 * set to composite to signal that we're not dealing with an actual
+		 * relation.
+		 */
 		exclRte = addRangeTableEntryForRelation(pstate,
-pstate->p_target_relation,
+targetrel,
 makeAlias("excluded", NIL),
 false, false);
+		exclRte->relkind = RELKIND_COMPOSITE_TYPE;
 		exclRelIndex = list_length(pstate->p_rtable);
 
 		/*
-		 * Build a targetlist for the EXCLUDED pseudo relation. Out of
-		 * simplicity we do that here, because expandRelAttrs() happens to
-		 * nearly do the right thing; specifically it also works with views.
-		 * It'd be more proper to instead scan some pseudo scan node, but it
-		 * doesn't seem worth the amount of code required.
-		 *
-		 * The only caveat of this hack is that the permissions expandRelAttrs
-		 * adds have to be reset. markVarForSelectPriv() will add the exact
-		 * required permissions back.
+		 * Build a targetlist for the EXCLUDED pseudo relation. Have to be
+		 * careful to use resnos that correspond to attnos of the underlying
+		 * relation.
+		 */
+		Assert(pstate->p_next_resno == 1);
+		for (attno = 0; attno < targetrel->rd_rel->relnatts; attno++)
+		{
+			Form_pg_attribute attr = targetrel->rd_att->attrs[attno];
+			char		*name;
+
+			if (attr->attisdropped)
+			{
+/*
+ * can't use atttypid here, but it doesn't really matter
+ * what type the Const claims to be.
+ */
+var = (Var *) makeNullConst(INT4OID, -1, InvalidOid);
+name = "";
+			}
+			else
+			{
+var = makeVar(exclRelIndex, attno + 1,
+			  attr->atttypid, attr->atttypmod,
+			  attr->attcollation,
+			  0);
+var->location = -1;
+
+name = NameStr(attr->attname);
+			}
+
+			Assert(pstate->p_next_resno == attno + 1);
+			te = makeTargetEntry((Expr *) var,
+ pstate->p_next_resno++,
+ name,
+ false);
+
+			/* don't require select access yet */
+			exclRelTlist = lappend(exclRelTlist, te);
+		}
+
+		/*
+		 * Additionally add a whole row tlist entry for EXCLUDED. That's
+		 * really 

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Peter Geoghegan]

On Tue, Sep 29, 2015 at 8:24 AM, Andres Freund  wrote:
> So, took a bit longer than "tomorrow. I fought for a long while with a
> mysterious issue, which turned out to be separate bug: The excluded
> relation was affected by row level security policies, which doesn't make
> sense.

Why? You certainly thought that it made sense for conventional column
permissions due to potential problems with before row insert triggers.
Why would RLS be different? Some of my concerns with RLS were that it
is different to the existing permissions model in a way that seems a
bit arbitrary. I don't think that they were changed to do anything
special with SELECT ... FOR UPDATE, which has always required some
amount of conventional UPDATE privilege.

I specifically remember discussing this with you off list (on IM,
roughly a couple of weeks prior to initial commit). I recommended that
we err towards a more restrictive behavior in the absence of any
strong principle pushing us one way or the other. You seemed to agree.

> My proposal in this WIP patch is to make it a bit clearer that
> 'EXCLUDED' isn't a real relation. I played around with adding a
> different rtekind, but that's too heavy a hammer. What I instead did was
> to set relkind to composite - which seems to signal pretty well that
> we're not dealing with a real relation. That immediately fixes the RLS
> issue as fireRIRrules has the following check:
> if (rte->rtekind != RTE_RELATION ||
> rte->relkind != RELKIND_RELATION)
> continue;

Well, not sure that that's a good thing. Let's discuss.

> It also makes it relatively straightforward to fix the system column
> issue by adding an additional relkind check to scanRTEForColumn's system
> column handling.

That seems fine.

> WRT to the wholerow issue: There's currently two reasons we need a
> targetlist entry for excluded wholerow vars: 1) setrefs.c errors out
> without - that can relativley easily be worked around 2) ruleutils.c
> expects an entry in the child tlist. That could also be worked around,
> but it's a bit more verbose.  I'm inclined to not go the pullup route
> but instead simply unconditionally add a wholerow var to the excluded
> tlist.

I suppose that we have a tight enough grip on the targetlist that it's
hard to imagine anything else being introduced there spuriously. I had
thought that the pull-up did allow useful additional
defense/sanitization, but that may not be an excellent argument. The
only remaining argument is that my approach is closer to RETURNING,
but that doesn't seem like an excellent argument.

Basically, I think that this is fine.

However, there were a number of small stylistic tweaks made in passing
within my original patch -- minor things around consistency. Please
either restore these, or commit them separately.

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Andres Freund]

On September 29, 2015 8:52:14 PM GMT+02:00, Peter Geoghegan  
wrote:
>On Tue, Sep 29, 2015 at 8:24 AM, Andres Freund 
>wrote:
>> So, took a bit longer than "tomorrow. I fought for a long while with
>a
>> mysterious issue, which turned out to be separate bug: The excluded
>> relation was affected by row level security policies, which doesn't
>make
>> sense.
>
>Why? You certainly thought that it made sense for conventional column
>permissions due to potential problems with before row insert triggers.
>Why would RLS be different? 

What would it mean? And why would it make sense to apply rls to a values list?

nodeModify already has the necessary rls invocations, no?

Andres


--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Stephen Frost]

* Peter Geoghegan (p...@heroku.com) wrote:
> On Tue, Sep 29, 2015 at 8:24 AM, Andres Freund  wrote:
> > So, took a bit longer than "tomorrow. I fought for a long while with a
> > mysterious issue, which turned out to be separate bug: The excluded
> > relation was affected by row level security policies, which doesn't make
> > sense.
> 
> Why? You certainly thought that it made sense for conventional column
> permissions due to potential problems with before row insert triggers.
> Why would RLS be different? Some of my concerns with RLS were that it
> is different to the existing permissions model in a way that seems a
> bit arbitrary. I don't think that they were changed to do anything
> special with SELECT ... FOR UPDATE, which has always required some
> amount of conventional UPDATE privilege.

I'm just about to push a patch to address exactly the SELECT .. FOR
UPDATE case, actually, and to try and make sure that RLS is more in line
with the existing permissions model..  I admit that I've not been
entirely following this thread though, so I'm not quite sure how that's
relevant to this discussion.

From Andres' reply, it looks like this is about the EXCLUDED pseudo
relation which comes from the INSERT'd values themselves, in which case,
I tend to agree with his assessment that it doesn't make sense for those
to be subject to RLS policies, given that it's all user-provided data,
as long as the USING check is done on the row found to be conflicting
and the CHECK constraints are dealt with correctly for any row added,
which I believe is what we had agreed was the correct way to handle this
case in prior discussions.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Peter Geoghegan]

On Thu, Sep 24, 2015 at 8:25 AM, Andres Freund  wrote:
> Stuff I want to fix by tomorrow:
> * Whole row var references to exclude
> * wrong offsets for columns after dropped ones
> * INSTEAD DO UPDATE for tables with oids
>
> Do you know of anything else?

You said something in Dallas about the test case developed by Amit
Langote touching on a different bug to the regression test I came up
with. If that is the case, then you didn't list that one separately.
Otherwise, no.

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Andres Freund]

On 2015-09-19 18:40:14 -0700, Peter Geoghegan wrote:
> > An actually trivial, although not all that pretty, fix is to simply
> > accept wholerow references in fix_join_expr_mutator(), even if not in
> > the targetlist. As far as I can see the problem right now really can
> > only be hit for whole row references.
> 
> I am concerned about the risk of adding bugs to unrelated code paths
> that this could create.

How? This is a must-not-reach code path currently?

Stuff I want to fix by tomorrow:
* Whole row var references to exclude
* wrong offsets for columns after dropped ones
* INSTEAD DO UPDATE for tables with oids

Do you know of anything else?

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Andres Freund]

Hi,

To recap for other readers: There's a problem with ON CONFLICT when the
SET or ON CONFLICT ... WHERE clause references excluded.* (i.e. as a
whole row var).  The problem is that setrefs.c in
fix_join_expr_mutator() currently won't find a matching entry in the
indexed tlist and thus error out with
  elog(ERROR, "variable not found in subplan target lists");

The reason is that the targetlist we build the index list on just
contains the attributes in excluded.*.

Peter's patch upthread fixes this by pulling expressions from
onConflictSet/Where into the targetlist. I disliked this - much less
than initially - a bit because that seems a bit crufty given that we're
not actually getting data from a child node.  This is different to
RETURNING where the targetlist massaging is actually important to get
the data up the tree.

An actually trivial, although not all that pretty, fix is to simply
accept wholerow references in fix_join_expr_mutator(), even if not in
the targetlist. As far as I can see the problem right now really can
only be hit for whole row references.

A variant of the second approach is to have a fix_onconflict_expr()
mutator that has such special handler.

Any opinions on either approach?

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

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[Peter Geoghegan]

On Sat, Sep 19, 2015 at 5:11 PM, Andres Freund  wrote:
> Peter's patch upthread fixes this by pulling expressions from
> onConflictSet/Where into the targetlist. I disliked this - much less
> than initially - a bit because that seems a bit crufty given that we're
> not actually getting data from a child node.  This is different to
> RETURNING where the targetlist massaging is actually important to get
> the data up the tree.

Maybe the massaging is somewhat justified by the fact that it's just
as good a place as any to reject system columns, and that needs to
happen somewhere.  I know that you suggested that this be done during
parse analysis, but not sure how attached you are to that. Might also
be a good choke point for detecting when unexpected vars/expressions
appear in the targetlist due to unforeseen circumstances/bugs. I
actually cover a couple of "can't happen" cases at the same time,
IIRC.

Continuing to follow RETURNING may have some value, even if the
analogy is a bit more forced here.

> An actually trivial, although not all that pretty, fix is to simply
> accept wholerow references in fix_join_expr_mutator(), even if not in
> the targetlist. As far as I can see the problem right now really can
> only be hit for whole row references.

I am concerned about the risk of adding bugs to unrelated code paths
that this could create. I must admit that this concern is mostly
driven by paranoia, and not a seasoned appreciation of problems that
could arise from ordinary post-processing of join expressions.

> A variant of the second approach is to have a fix_onconflict_expr()
> mutator that has such special handler.

I suppose you could add a fix_join_expr_context field that had
fix_join_expr_mutator() avoid the special handler for post-processing
of join expressions. That might be a bit ugly too, but would involve
less code duplication.

> Any opinions on either approach?

I think that I favor my original solution, although only by a tiny
margin. I will avoid offering either a -1 or a +1 to any proposal
here, although they all sound basically reasonable to me. A more
complete targetlist representation would have been something that I'd
probably vote against, since it seems complex and invasive, but that
doesn't matter now. In short, I defer to others here.

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