pgsql: RLS refactoring

Stephen Frost <sfrost@snowman.net> writes:

RLS refactoring

It looks to me like this changed the representation of stored rules, so it
should have included a catversion bump. This is particularly relevant to
the 9.5 branch where people already have alpha installations.

regards, tom lane

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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

RLS refactoring

It looks to me like this changed the representation of stored rules, so it
should have included a catversion bump. This is particularly relevant to
the 9.5 branch where people already have alpha installations.

I had considererd if a bump was needed and figured it wasn't.

The WithCheckOption node which was changed doesn't ever end up in the
catalog, I don't believe; certainly not in pg_policy which just stores
the expressions which come from transformWhereClause, which haven't
changed.

I don't mind doing a bump if we feel it's necessary and maybe I'm
missing that there's a way to cause that node type to end up in the
catalog, but I don't think so, as we only ever build WithCheckOption
nodes in the rewriter.

Thanks!

Stephen

Stephen Frost wrote:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

It looks to me like this changed the representation of stored rules, so it
should have included a catversion bump. This is particularly relevant to
the 9.5 branch where people already have alpha installations.

I had considererd if a bump was needed and figured it wasn't.

The WithCheckOption node which was changed doesn't ever end up in the
catalog, I don't believe; certainly not in pg_policy which just stores
the expressions which come from transformWhereClause, which haven't
changed.

Uhm, so why is it in readfuncs.c? If you create a view "WITH CHECK
OPTION", the pg_rewrite row says ":withCheckOptions <>". Does that not
change with your commit?

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

It looks to me like this changed the representation of stored rules, so it
should have included a catversion bump. This is particularly relevant to
the 9.5 branch where people already have alpha installations.

I had considererd if a bump was needed and figured it wasn't.

I don't mind doing a bump if we feel it's necessary and maybe I'm
missing that there's a way to cause that node type to end up in the
catalog, but I don't think so, as we only ever build WithCheckOption
nodes in the rewriter.

Oh, I see. In that case you should remove WithCheckOption from the set of
node types supported by readfuncs.c, both because it's dead code and to
clarify that the node is not meant to ever end up on disk.

(outfuncs.c support is useful for debugging though, so keep that.)

regards, tom lane

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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

It looks to me like this changed the representation of stored rules, so it
should have included a catversion bump. This is particularly relevant to
the 9.5 branch where people already have alpha installations.

I had considererd if a bump was needed and figured it wasn't.

I don't mind doing a bump if we feel it's necessary and maybe I'm
missing that there's a way to cause that node type to end up in the
catalog, but I don't think so, as we only ever build WithCheckOption
nodes in the rewriter.

Oh, I see. In that case you should remove WithCheckOption from the set of
node types supported by readfuncs.c, both because it's dead code and to
clarify that the node is not meant to ever end up on disk.

Yeah, I was just thinking the same.

(outfuncs.c support is useful for debugging though, so keep that.)

Right, makes sense.

I should be able to get to that tomorrow afternoon, til then I'm pretty
tied up with PostgresOpen.

Thanks!

Stephen

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Stephen Frost wrote:

The WithCheckOption node which was changed doesn't ever end up in the
catalog, I don't believe; certainly not in pg_policy which just stores
the expressions which come from transformWhereClause, which haven't
changed.

Uhm, so why is it in readfuncs.c? If you create a view "WITH CHECK
OPTION", the pg_rewrite row says ":withCheckOptions <>". Does that not
change with your commit?

That field would always be NIL in a query produced by the parser;
it's only ever filled by the rewriter. But if this is documented
anywhere, I couldn't find it, and the placement of the field in struct
Query seems designed to be as confusing as possible about that. I'd have
put it down near the end myself, and certainly have documented that it is
NOT the parse-time representation of a WITH CHECK OPTION clause. For that
matter I don't even find it to be named very well, because it's impossible
to avoid that impression with the name as-is. Perhaps something like
insertedCheckClauses would have been better.

regards, tom lane

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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Stephen Frost wrote:

The WithCheckOption node which was changed doesn't ever end up in the
catalog, I don't believe; certainly not in pg_policy which just stores
the expressions which come from transformWhereClause, which haven't
changed.

Uhm, so why is it in readfuncs.c? If you create a view "WITH CHECK
OPTION", the pg_rewrite row says ":withCheckOptions <>". Does that not
change with your commit?

That field would always be NIL in a query produced by the parser;

Right.

it's only ever filled by the rewriter. But if this is documented
anywhere, I couldn't find it, and the placement of the field in struct
Query seems designed to be as confusing as possible about that. I'd have
put it down near the end myself, and certainly have documented that it is
NOT the parse-time representation of a WITH CHECK OPTION clause. For that
matter I don't even find it to be named very well, because it's impossible
to avoid that impression with the name as-is. Perhaps something like
insertedCheckClauses would have been better.

Agreed. I will see about improving on that situation with at least
documentation changes. If we want to remove it completely then we'd
need to bump catversion.. Not against doing that if we want to though.
Might be better that way.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

Agreed. I will see about improving on that situation with at least
documentation changes. If we want to remove it completely then we'd
need to bump catversion.. Not against doing that if we want to though.
Might be better that way.

readfuncs.c doesn't actually stop to verify that the field name in stored
rules is what it expects. So I believe (but you'd better check) that
renaming the field could be done without initdb. If we wanted to change
its placement, we'd need initdb --- unless we wanted to move it in the
struct definition but not in _outQuery/_readQuery, which I wouldn't do
in HEAD but it might be acceptable in back branches.

So the limiting factor here is not initdb but avoiding an API/ABI break
for extensions that look at struct Query. It's clearly too late to do any
such thing in 9.4, but we still could accept API breaks for 9.5 I think.

My vote would be to rename and reposition the field in HEAD and 9.5
and accept the corresponding initdb. We already forced an initdb
since alpha2, so it's basically free as far as testers are concerned,
and keeping 9.5 in sync with HEAD in this area seems like a really
good idea for awhile to come.

regards, tom lane

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

On 2015-09-15 19:16:06 -0400, Tom Lane wrote:

readfuncs.c doesn't actually stop to verify that the field name in stored
rules is what it expects.

This reminds me: Is there a reason for that? ISTM that adding checks for
the field names would make error messages about borked stored trees much
easier to understand?

Greetings,

Andres Freund

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

Andres Freund <andres@anarazel.de> writes:

On 2015-09-15 19:16:06 -0400, Tom Lane wrote:

readfuncs.c doesn't actually stop to verify that the field name in stored
rules is what it expects.

This reminds me: Is there a reason for that? ISTM that adding checks for
the field names would make error messages about borked stored trees much
easier to understand?

Dunno, how often have you seen such an error message, and how much would
it help anyway? I'm not sure it'd be worth the code and cycles to check.

regards, tom lane

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

Tom, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

My vote would be to rename and reposition the field in HEAD and 9.5
and accept the corresponding initdb. We already forced an initdb
since alpha2, so it's basically free as far as testers are concerned,
and keeping 9.5 in sync with HEAD in this area seems like a really
good idea for awhile to come.

Alright, attached is an attempt at doing these renames. I went a bit
farther since it seemed to make sense to me (at least at the time, I'm
wondering a bit about it now), so, please provide any thoughts or
feedback you have regarding these changes.

Thanks!

Stephen

All,

and this totally should have gone to -hackers instead, sorry about that.
Please ignore the one to -committers, if possible.

Tom, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

My vote would be to rename and reposition the field in HEAD and 9.5
and accept the corresponding initdb. We already forced an initdb
since alpha2, so it's basically free as far as testers are concerned,
and keeping 9.5 in sync with HEAD in this area seems like a really
good idea for awhile to come.

Alright, attached is an attempt at doing these renames. I went a bit
farther since it seemed to make sense to me (at least at the time, I'm
wondering a bit about it now), so, please provide any thoughts or
feedback you have regarding these changes.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

Alright, attached is an attempt at doing these renames. I went a bit
farther since it seemed to make sense to me (at least at the time, I'm
wondering a bit about it now), so, please provide any thoughts or
feedback you have regarding these changes.

Further, rename 'withCheckOptions' in Query and ModifyTable to
'insertedCheckClauses', ExecWithCheckOption to ExecCheckClauses,
ri_WithCheckOptions and ri_WithCheckOptionExprs to
ri_InsertedCheckClauses and ri_InsertedCheckExprs, respectively, all to
make it clear that these are the check clauses which were added by the
rewriter, not the actual WITH CHECK OPTION indication for a view (which
is stored in reloptions for the view) nor the WITH CHECK expressions for

That commitlog entry doesn't seem to quite square with the patch,
wherein I see

- *		WithCheckOptions		list of WithCheckOption's to be checked
- *		WithCheckOptionExprs	list of WithCheckOption expr states
+ *		InsertedCheckClauses	list of WithCheckOption's to be checked
+ *		InsertedCheckClauseExprs	list of WithCheckOption expr states

-	List	   *ri_WithCheckOptions;
-	List	   *ri_WithCheckOptionExprs;
+	List	   *ri_InsertedCheckClauses;
+	List	   *ri_InsertedCheckClauseExprs;

The distinction between a "clause" and an "expr" is not very obvious,
and certainly most other places in the code use those terms pretty
interchangeably, so I find both the old and new names unclear here.
How about ri_InsertedCheckClauseStates instead for the second list?
And similarly if you're using "Expr" to mean ExprState anywhere else.

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

I wrote:

-	List	   *ri_WithCheckOptions;
-	List	   *ri_WithCheckOptionExprs;
+	List	   *ri_InsertedCheckClauses;
+	List	   *ri_InsertedCheckClauseExprs;

The distinction between a "clause" and an "expr" is not very obvious,
and certainly most other places in the code use those terms pretty
interchangeably, so I find both the old and new names unclear here.
How about ri_InsertedCheckClauseStates instead for the second list?
And similarly if you're using "Expr" to mean ExprState anywhere else.

Actually ... does struct ResultRelInfo need to carry the original WCO
clauses at all, rather than just the exprstate list? In most places
we do not store expr and exprstate lists in the same node in the first
place, so we can get away with using the same field name for corresponding
lists in plan and planstate nodes. That's why we don't already have a
convention like "fooStates" for such lists.

Another thought is that as long as these are lists specifically of
WithCheckOption nodes, and not arbitrary expressions, "clause" isn't an
especially good term for them; it implies generality that isn't there.
And CheckClauses invites confusion with, for example, CHECK clauses of
domain types. So maybe better names would be "ri_InsertedCheckOptions"
(and "ri_InsertedCheckOptionStates" if you still need that). Or maybe
"ri_InsertedWCOClauses" and "ri_InsertedWCOClauseStates". I'm less sure
about whether this is an improvement, though.

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

On 24 September 2015 at 21:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

-     List       *ri_WithCheckOptions;
-     List       *ri_WithCheckOptionExprs;
+     List       *ri_InsertedCheckClauses;
+     List       *ri_InsertedCheckClauseExprs;

The distinction between a "clause" and an "expr" is not very obvious,
and certainly most other places in the code use those terms pretty
interchangeably, so I find both the old and new names unclear here.
How about ri_InsertedCheckClauseStates instead for the second list?
And similarly if you're using "Expr" to mean ExprState anywhere else.

Actually ... does struct ResultRelInfo need to carry the original WCO
clauses at all, rather than just the exprstate list? In most places
we do not store expr and exprstate lists in the same node in the first
place, so we can get away with using the same field name for corresponding
lists in plan and planstate nodes. That's why we don't already have a
convention like "fooStates" for such lists.

Another thought is that as long as these are lists specifically of
WithCheckOption nodes, and not arbitrary expressions, "clause" isn't an
especially good term for them; it implies generality that isn't there.
And CheckClauses invites confusion with, for example, CHECK clauses of
domain types. So maybe better names would be "ri_InsertedCheckOptions"
(and "ri_InsertedCheckOptionStates" if you still need that). Or maybe
"ri_InsertedWCOClauses" and "ri_InsertedWCOClauseStates". I'm less sure
about whether this is an improvement, though.

None of this renaming seems like an improvement to me.

ri_InsertedCheckClauses is misleading because they're not clauses,
they're WithCheckOption nodes carrying various pieces of information,
only one of which is the clause to check.

ri_InsertedCheckOptions is a bit better from that perspective, but it
still seems messy for the executor to carry the knowledge that these
checks were inserted by the rewriter. In the executor they're just
checks to be executed, and "WithCheck" reflects their original source
better than "InsertedCheck".

ri_InsertedCheckOptionStates is inconsistent with the names of the
other lists of expr states on that node -- ri_TrigWhenExprs and
ri_ConstraintExprs.

Also, these were added in 9.4, so introducing this many differences
between 9.4 and 9.5+ will make back-patching harder.

The original objection to the name WithCheckOptions was in relation to
the Query struct, and the fact that this field isn't the parsed
representation of WITH CHECK OPTION's on the query. I think that can
be cured with a suitable comment.

Regards,
Dean

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

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

None of this renaming seems like an improvement to me.

I have to admit that I'm not entirely sure it's improving things either.

Certainly, we shouldn't be dumping out the withCheckOptions node and
perhaps we should move its place in Query and add comments to it, but
I'm not sure about the other changes.

ri_InsertedCheckClauses is misleading because they're not clauses,
they're WithCheckOption nodes carrying various pieces of information,
only one of which is the clause to check.

ri_InsertedCheckOptions is a bit better from that perspective, but it
still seems messy for the executor to carry the knowledge that these
checks were inserted by the rewriter. In the executor they're just
checks to be executed, and "WithCheck" reflects their original source
better than "InsertedCheck".

Yeah, the actual structure is 'WithCheckOption' and everything is pretty
consistent around that. One possibly confusing bit is that we have a
ViewCheckOption enum in ViewStmt called "withCheckOption". Perhaps
that's what we should change? Semes like it's either that or rename the
structure itself to NewTupleCheck (or similar) and rename everything to
go along with that instead.

Also, these were added in 9.4, so introducing this many differences
between 9.4 and 9.5+ will make back-patching harder.

That's certainly true, but we don't want current or future hackers to be
confused either.

The original objection to the name WithCheckOptions was in relation to
the Query struct, and the fact that this field isn't the parsed
representation of WITH CHECK OPTION's on the query. I think that can
be cured with a suitable comment.

There's also the issue that outfuncs is including that node when it
really shouldn't be. That can be fixed independnetly of this
discussion, of course.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

Also, these were added in 9.4, so introducing this many differences
between 9.4 and 9.5+ will make back-patching harder.

That's certainly true, but we don't want current or future hackers to be
confused either.

Yes. I do not think that we should stick with badly chosen names just
because of back-patching concerns. By that argument, we should never
fix any erroneous comments either.

Whether these particular names are improvements is, of course, a fit
subject for debate. I have to agree that I don't feel like we've quite
hit on le mot juste yet.

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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

Also, these were added in 9.4, so introducing this many differences
between 9.4 and 9.5+ will make back-patching harder.

That's certainly true, but we don't want current or future hackers to be
confused either.

Yes. I do not think that we should stick with badly chosen names just
because of back-patching concerns. By that argument, we should never
fix any erroneous comments either.

Whether these particular names are improvements is, of course, a fit
subject for debate. I have to agree that I don't feel like we've quite
hit on le mot juste yet.

I've gone ahead and at least removed the withCheckOptions empty-list
from being written out as part of Query for 9.5 and HEAD, and bumped
catversion accordingly.

I came to realize that ModifyTable actually is planned to be used for
parallel query and therefore the list for that needs to stay, along with
support for reading the WithCheckOption node in.

Thanks!

Stephen