Remaining 9.5 open items
Well, it's December nearly, and we don't seem to be making much progress
towards pushing out 9.5.0. I see the following items on
https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items
* Open Row-Level Security Issues
Seems like what's left here is only documentation fixes, but they still
need to get done.
* DDL deparsing testing module should have detected that transforms were not supported, but it failed to notice that
Is this really a release blocker? As a testing matter, it seems like any
fix would go into HEAD only.
* Foreign join pushdown vs EvalPlanQual
Is this fixed by 5fc4c26db? If not, what remains to do?
* pg_rewind exiting with error code 1 when source and target are on the same timeline
Is this a new-in-9.5 bug, or a pre-existing problem? If the latter,
I'm not sure it's a release blocker.
* psql extended wrapped format off by one error in line wrapping
There's a submitted patch, so I'll take a look at whether it's pushable.
* Finish multixact truncation rework
We're not seriously going to push something this large into 9.5 at this
point, are we?
* another strange behavior with track_commit_timestamp
Where are we on this?
* Relation files of unlogged relation for btree and spgist indexes not initialized after promotion
Again, is this a release blocker? It's evidently a very old bug.
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 2015-11-30 14:43:59 -0500, Tom Lane wrote:
* pg_rewind exiting with error code 1 when source and target are on the same timeline
Is this a new-in-9.5 bug, or a pre-existing problem? If the latter,
I'm not sure it's a release blocker.
pg_rewind was only introduced in 9.5, no?
* Finish multixact truncation rework
We're not seriously going to push something this large into 9.5 at this
point, are we?
To my knowledge this mostly comment changes. And some, actually
independent, improvements. More blocked on procedual disagreements -
with Noah wanting to revert the existing commits, fixup some stuff, then
reapply them - and me just wanting to do the improvements independently.
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
Tom Lane wrote:
* DDL deparsing testing module should have detected that transforms were not supported, but it failed to notice that
Is this really a release blocker? As a testing matter, it seems like any
fix would go into HEAD only.
Not a blocker as far as I'm concerned.
* another strange behavior with track_commit_timestamp
Where are we on this?
I will test the patch I proposed more thoroughly, then push. I intend
to add a few test cases on top of the recovery testing patch Michael
submitted (but that's branch master only), to make sure I don't
re-introduce bugs already fixed.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
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:
Well, it's December nearly, and we don't seem to be making much progress
towards pushing out 9.5.0. I see the following items on
https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items* Open Row-Level Security Issues
Seems like what's left here is only documentation fixes, but they still
need to get done.
These are mainly just documentation improvements which I'm working on,
though the docs were recently updated and I need to incorporate Peter's
changes which I wasn't exactly anticipating.
The non-documentation question is around DROP OWNED. We need to either
have policies dropped by DROP OWNED (well, roles removed, unless it's
the last one, in which case the policy should be dropped), or update the
documentation to reflect that they don't. I had been thinking we'd
fix DROP OWNED to deal with the policies, but if folks feel it's too
late for that kind of a change, then we can simply document it. I don't
believe that's unreasonable for a new feature and we can work to get it
addressed in 9.6.
I'm starting to think that just documenting it makes sense for 9.5. I
doubt it's going to be a serious issue during 9.5's lifetime.
Thanks!
Stephen
Stephen Frost wrote:
The non-documentation question is around DROP OWNED. We need to either
have policies dropped by DROP OWNED (well, roles removed, unless it's
the last one, in which case the policy should be dropped), or update the
documentation to reflect that they don't. I had been thinking we'd
fix DROP OWNED to deal with the policies, but if folks feel it's too
late for that kind of a change, then we can simply document it. I don't
believe that's unreasonable for a new feature and we can work to get it
addressed in 9.6.
DROP OWNED is documented as a mechanism to help you drop the role, so
it should do whatever is needed for that. I don't think documenting the
fact that it leaves the user as part of policies is good enough.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Stephen Frost wrote:
The non-documentation question is around DROP OWNED. We need to either
have policies dropped by DROP OWNED (well, roles removed, unless it's
the last one, in which case the policy should be dropped), or update the
documentation to reflect that they don't. I had been thinking we'd
fix DROP OWNED to deal with the policies, but if folks feel it's too
late for that kind of a change, then we can simply document it. I don't
believe that's unreasonable for a new feature and we can work to get it
addressed in 9.6.DROP OWNED is documented as a mechanism to help you drop the role, so
it should do whatever is needed for that. I don't think documenting the
fact that it leaves the user as part of policies is good enough.
We already can't take care of everything with DROP OWNED though, since
we can't do cross-database queries, and the overall process almost
certainly requires additional effort (to reassign objects, etc...), so
while I'd be happier if policies were handled by it, I don't think it's
as serious of an issue.
Still, I'll get a patch worked up for it and then we can discuss the
merits of that patch going in to 9.5 now versus just into HEAD.
Thanks!
Stephen
Stephen Frost wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Stephen Frost wrote:
The non-documentation question is around DROP OWNED. We need to either
have policies dropped by DROP OWNED (well, roles removed, unless it's
the last one, in which case the policy should be dropped), or update the
documentation to reflect that they don't. I had been thinking we'd
fix DROP OWNED to deal with the policies, but if folks feel it's too
late for that kind of a change, then we can simply document it. I don't
believe that's unreasonable for a new feature and we can work to get it
addressed in 9.6.DROP OWNED is documented as a mechanism to help you drop the role, so
it should do whatever is needed for that. I don't think documenting the
fact that it leaves the user as part of policies is good enough.We already can't take care of everything with DROP OWNED though, since
we can't do cross-database queries, and the overall process almost
certainly requires additional effort (to reassign objects, etc...), so
while I'd be happier if policies were handled by it, I don't think it's
as serious of an issue.
Yes, the documentation says to apply a combination of REASSIGN OWNED
plus DROP OWNED to each database. Sure, it's not a single command, but
if you additionally put the burden that the policies must be taken care
of separately, then the whole process is made a little worse.
Still, I'll get a patch worked up for it and then we can discuss the
merits of that patch going in to 9.5 now versus just into HEAD.
Cool.
In the past, we've made a bunch of changes to DROP OWNED in order to
deal with object types that caused errors, even in minor releases. I
think this is just another case of the same problem.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Dec 1, 2015 at 4:43 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
* pg_rewind exiting with error code 1 when source and target are on the same timeline
Is this a new-in-9.5 bug, or a pre-existing problem? If the latter,
I'm not sure it's a release blocker.
pg_rewind has been introduced in 9.5. It would be good to get
something consistent before GA.
* Relation files of unlogged relation for btree and spgist indexes not initialized after promotion
Again, is this a release blocker? It's evidently a very old bug.
Actually for 9.5 it could be said so. The approach proposed by Andres
and that I have patched requires a bump of the WAL format. It would be
nice to get that into 9.5 tree. Or 9.5 will have to use a solution
similar to the back branches, which would be something like syncing
unconditionally INIT_FORKNUM at replay after replaying its FPW.
Thoughts welcome on the dedicated thread.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Nov 30, 2015 at 2:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
* Foreign join pushdown vs EvalPlanQual
Is this fixed by 5fc4c26db? If not, what remains to do?
Unfortunately, no. That commit allows FDWs to do proper EPQ handling
for plain table scans, but it proves to be inadequate for EPQ handling
for joins. Solving that problem will require another patch, and,
modulo a bunch of cosmetic issues, I'm reasonably happy with KaiGai
Kohei's latest submission. I'll respond in more detail on that
thread, but the question I want to raise here is: do we want to
back-patch those changes to 9.5 at this late date?
If we don't, then join pushdown won't be usable in 9.5 for queries
that have locking clauses; it will crash. However, it may be that
nobody's going to try to do that anyway. And if they do, they have
the PlannerInfo available when generating paths, so they can just not
push down any joins when there are row marks, which doesn't sound like
the worst thing that ever happened to anybody. It's also possible
that the fix isn't really correct and we won't find that out until
after release, at which point it'll be too late to tinker with the API
(if it isn't already). On the other hand, what do we get out of
shipping an API that we know to be a few bricks short of a load? I
think the risk of collateral damage is low. If there's a problem, I
expect it to be that the join-pushdown-vs-EPQ problem is still not
solved, not that anything else stops working.
From the point of view of existing FDWs, the most noticeable effect of
the patch is that you'll have to pass one more NULL argument to
functions make_foreignscan(). This will break compiles, but it should
be a more trivial repair than what 5fc4c26db demanded. So maybe it's
OK.
I could go either way on this. What do others think?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Nov 30, 2015 at 8:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
* Finish multixact truncation rework
We're not seriously going to push something this large into 9.5 at this
point, are we?
I don't know all the details here, so my apologies if any of this is
incorrect/stupid/misinformed.
Given all the quite serious data corruption issues related to
multixact, wouldn't it be motivated to wait releasing 9.5.0 until this
much needed multixct truncation rework has been finished?
This is of course not an issue for users already on >=9.3, since the
problems started in 9.3 and those already on 9.3 and 9.4 already face
the risk.
But users who will consider upgrading from previous versions, will
have to ask themselves if all the new features in 9.5 are worth the
extra risk of data corruption due to the multixact issues.
We at Trustly are still running 9.1 and have been waiting for the
multixact problems to be fixed, which is why we didn't upgrade to 9.4,
and now when I read this I feel really sad we probably have to wait
for 9.6, unless we can accept some increase risk of data corruption.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-12-02 12:14:42 +0100, Joel Jacobson wrote:
On Mon, Nov 30, 2015 at 8:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
* Finish multixact truncation rework
We're not seriously going to push something this large into 9.5 at this
point, are we?I don't know all the details here, so my apologies if any of this is
incorrect/stupid/misinformed.Given all the quite serious data corruption issues related to
multixact, wouldn't it be motivated to wait releasing 9.5.0 until this
much needed multixct truncation rework has been finished?
The significant changes are in 9.5.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Dec 2, 2015 at 12:19 PM, Andres Freund <andres@anarazel.de> wrote:
The significant changes are in 9.5.
Will multixact truncations be WAL logged in 9.5?
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-12-02 12:25:37 +0100, Joel Jacobson wrote:
On Wed, Dec 2, 2015 at 12:19 PM, Andres Freund <andres@anarazel.de> wrote:
The significant changes are in 9.5.
Will multixact truncations be WAL logged in 9.5?
Yes.
C.f. the release notes:
* Rework truncation of the multixact commit log to be properly WAL-logged
(Andres Freund)
This makes things substantially simpler and more robust.
The relevant commits are
commit aa29c1ccd9f785f9365809f5133e5491acc7ae53
Author: Andres Freund <andres@anarazel.de>
Date: 2015-09-26 19:04:25 +0200
Remove legacy multixact truncation support.
...
Backpatch: 9.5
and
commit 4f627f897367f15702d59973f75f6391d5d3e06f
Author: Andres Freund <andres@anarazel.de>
Date: 2015-09-26 19:04:25 +0200
Rework the way multixact truncations work.
...
Backpatch: 9.5
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Dec 2, 2015 at 12:36 PM, Andres Freund <andres@anarazel.de> wrote:
On 2015-12-02 12:25:37 +0100, Joel Jacobson wrote:
On Wed, Dec 2, 2015 at 12:19 PM, Andres Freund <andres@anarazel.de> wrote:
The significant changes are in 9.5.
Will multixact truncations be WAL logged in 9.5?
Yes.
C.f. the release notes
Excellent! :-) Many many thanks for your efforts! By far the most
important change in 9.5. So much looking forward to upgrade.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Nov 30, 2015 at 4:55 PM, Stephen Frost <sfrost@snowman.net> wrote:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Well, it's December nearly, and we don't seem to be making much progress
towards pushing out 9.5.0. I see the following items on
https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items* Open Row-Level Security Issues
Seems like what's left here is only documentation fixes, but they still
need to get done.These are mainly just documentation improvements which I'm working on,
though the docs were recently updated and I need to incorporate Peter's
changes which I wasn't exactly anticipating.
So, when do you foresee this documentation stuff getting taken care
of? We kinda need to do a release here. Is this really a blocker?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 12/02/2015 05:27 AM, Robert Haas wrote:
On Mon, Nov 30, 2015 at 4:55 PM, Stephen Frost <sfrost@snowman.net> wrote:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Well, it's December nearly, and we don't seem to be making much progress
towards pushing out 9.5.0. I see the following items on
https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items* Open Row-Level Security Issues
Seems like what's left here is only documentation fixes, but they still
need to get done.These are mainly just documentation improvements which I'm working on,
though the docs were recently updated and I need to incorporate Peter's
changes which I wasn't exactly anticipating.So, when do you foresee this documentation stuff getting taken care
of? We kinda need to do a release here. Is this really a blocker?
A feature does not exist without documentation.
JD
--
Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Announcing "I'm offended" is basically telling the world you can't
control your own emotions, so everyone else should do it for you.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-12-02 08:25:13 -0800, Joshua D. Drake wrote:
A feature does not exist without documentation.
Uh, you do realize there's actually documentation about RLS? The issues
mentioned here are some small adjustments, not entirely new docs.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 12/02/2015 08:39 AM, Andres Freund wrote:
On 2015-12-02 08:25:13 -0800, Joshua D. Drake wrote:
A feature does not exist without documentation.
Uh, you do realize there's actually documentation about RLS? The issues
mentioned here are some small adjustments, not entirely new docs.
No I didn't. I apologize for the noise.
JD
--
Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Announcing "I'm offended" is basically telling the world you can't
control your own emotions, so everyone else should do it for you.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Dec 01, 2015 at 11:05:47AM -0500, Robert Haas wrote:
On Mon, Nov 30, 2015 at 2:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
* Foreign join pushdown vs EvalPlanQual
Is this fixed by 5fc4c26db? If not, what remains to do?
Unfortunately, no. That commit allows FDWs to do proper EPQ handling
for plain table scans, but it proves to be inadequate for EPQ handling
for joins. Solving that problem will require another patch, and,
modulo a bunch of cosmetic issues, I'm reasonably happy with KaiGai
Kohei's latest submission. I'll respond in more detail on that
thread, but the question I want to raise here is: do we want to
back-patch those changes to 9.5 at this late date?
Yes. If 9.5 added a bad interface, better to fix the interface even now than
to live with the bad one.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Dec 2, 2015 at 5:27 AM, Robert Haas <robertmhaas@gmail.com> wrote:
These are mainly just documentation improvements which I'm working on,
though the docs were recently updated and I need to incorporate Peter's
changes which I wasn't exactly anticipating.So, when do you foresee this documentation stuff getting taken care
of? We kinda need to do a release here. Is this really a blocker?
If it's a blocker, I can write the documentation myself. Let's just
fix it, rather than discussing whether or not it needs to be a blocker
-- it will take less time.
Stephen?
--
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
On 2015/12/04 11:51, Noah Misch wrote:
On Tue, Dec 01, 2015 at 11:05:47AM -0500, Robert Haas wrote:
On Mon, Nov 30, 2015 at 2:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
* Foreign join pushdown vs EvalPlanQual
Is this fixed by 5fc4c26db? If not, what remains to do?
Unfortunately, no. That commit allows FDWs to do proper EPQ handling
for plain table scans, but it proves to be inadequate for EPQ handling
for joins. Solving that problem will require another patch, and,
modulo a bunch of cosmetic issues, I'm reasonably happy with KaiGai
Kohei's latest submission. I'll respond in more detail on that
thread, but the question I want to raise here is: do we want to
back-patch those changes to 9.5 at this late date?
Yes. If 9.5 added a bad interface, better to fix the interface even now than
to live with the bad one.
I'd vote for fixing this.
I think the latest version of the patch for this is in good shape, but
that would need some changes as proposed on that thread. So, if there
are no objections, I'll update the patch.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1 December 2015 at 17:05, Robert Haas <robertmhaas@gmail.com> wrote:
do we want to
back-patch those changes to 9.5 at this late date?
Surely the whole point of a release process is to fix issues in the
release. If we don't ever dare put something in the release, we may as well
have released it earlier.
I'm thinking about a two stage release process...
Stage 1 - released, but with caveats and some parts marked
experimental/beta whatever
Stage 2 - released, all caveats resolved
Not sure what to call that.
--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Simon Riggs <simon@2ndQuadrant.com> writes:
On 1 December 2015 at 17:05, Robert Haas <robertmhaas@gmail.com> wrote:
do we want to
back-patch those changes to 9.5 at this late date?
Surely the whole point of a release process is to fix issues in the
release. If we don't ever dare put something in the release, we may as well
have released it earlier.
I'm thinking about a two stage release process...
Stage 1 - released, but with caveats and some parts marked
experimental/beta whatever
Stage 2 - released, all caveats resolved
Not sure what to call that.
9.5beta3. If you're saying we are not ready for an RC, then it's a beta.
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 4 December 2015 at 16:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Simon Riggs <simon@2ndQuadrant.com> writes:
On 1 December 2015 at 17:05, Robert Haas <robertmhaas@gmail.com> wrote:
do we want to
back-patch those changes to 9.5 at this late date?Surely the whole point of a release process is to fix issues in the
release. If we don't ever dare put something in the release, we may aswell
have released it earlier.
I'm thinking about a two stage release process...
Stage 1 - released, but with caveats and some parts marked
experimental/beta whateverStage 2 - released, all caveats resolved
Not sure what to call that.
9.5beta3. If you're saying we are not ready for an RC, then it's a beta.
I guess I really meant "in the future" or "next time", but perhaps that
could apply now.
The main issue is that most of these things are pretty trivial and hardly
worth delaying the release process of The World's Most Advanced Open Source
Database for; its not like the Saturn V will fail to fly.
We don't seem to put any negative weighting on delay; even the smallest
issues are enough to delay us. That's daft because we know we'll hit a raft
of bugs soon after release, we just don't know where they are yet - so
fooling ourselves that it needs to be perfect before release of 9.5.0
doesn't help anybody.
Do we think they ever launched a Saturn V that didn't have some marginal
flashing lights somewhere?
I accept there are open items. I'd like a way to indicate to people they
can start using it with a safety, apart from the listed caveats.
--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 4 December 2015 at 15:50, Simon Riggs <simon@2ndquadrant.com> wrote:
Do we think they ever launched a Saturn V that didn't have some marginal
flashing lights somewhere?
Almost certainly. They had triple-redundant systems that were certified
for correctness. You don't knowingly send rockets into space with dubious
control systems.
I accept there are open items. I'd like a way to indicate to people they
can start using it with a safety, apart from the listed caveats.
Just to add my .2c worth...
T
hat's what betas are for.
There's an implied open-source contract
that anyone who wants to use a feature in the next version will invest a
little of their time making sure that the feature works for them in the
beta before it gets released.
The developer side of that contract is that you fix the bugs people found
in the beta before release, because otherwise next time they won't bother.
And if you start pushing out full releases that you *know *introduce bugs
that weren't in the previous release, "with caveats" or not, you end up in
a situation where people won't upgrade until the second or third point
release.
Geoff
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Stephen Frost wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Stephen Frost wrote:
The non-documentation question is around DROP OWNED. We need to either
have policies dropped by DROP OWNED (well, roles removed, unless it's
the last one, in which case the policy should be dropped), or update the
documentation to reflect that they don't. I had been thinking we'd
fix DROP OWNED to deal with the policies, but if folks feel it's too
late for that kind of a change, then we can simply document it. I don't
believe that's unreasonable for a new feature and we can work to get it
addressed in 9.6.DROP OWNED is documented as a mechanism to help you drop the role, so
it should do whatever is needed for that. I don't think documenting the
fact that it leaves the user as part of policies is good enough.We already can't take care of everything with DROP OWNED though, since
we can't do cross-database queries, and the overall process almost
certainly requires additional effort (to reassign objects, etc...), so
while I'd be happier if policies were handled by it, I don't think it's
as serious of an issue.Yes, the documentation says to apply a combination of REASSIGN OWNED
plus DROP OWNED to each database. Sure, it's not a single command, but
if you additionally put the burden that the policies must be taken care
of separately, then the whole process is made a little worse.Still, I'll get a patch worked up for it and then we can discuss the
merits of that patch going in to 9.5 now versus just into HEAD.Cool.
In the past, we've made a bunch of changes to DROP OWNED in order to
deal with object types that caused errors, even in minor releases. I
think this is just another case of the same problem.
Patch attached for review/comment.
I noticed in passing that the role removal documentation should really
also discuss shared objects (as the DROP OWNED BY reference page does).
Thanks!
Stephen
Attachments:
drop-owned-policies.v1.patchtext/x-diff; charset=us-asciiDownload
From e3f59f57add2afbb27b68486865274db44e38ab6 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Fri, 4 Dec 2015 11:00:52 -0500
Subject: [PATCH] Handle policies during DROP OWNED BY
DROP OWNED BY handled GRANT-based ACLs but was not removing roles from
policies. Fix that by having DROP OWNED BY remove the role specified
from the list of roles the policy (or policies) apply to, or the entire
policy (or policies) if it only applied to the role specified.
As with ACLs, the DROP OWNED BY caller must have permission to modify
the policy or a WARNING is thrown and no change is made to the policy.
---
src/backend/catalog/pg_shdepend.c | 13 ++
src/backend/commands/policy.c | 246 ++++++++++++++++++++++++++++++
src/include/commands/policy.h | 2 +
src/test/regress/expected/rowsecurity.out | 14 ++
src/test/regress/sql/rowsecurity.sql | 19 +++
5 files changed, 294 insertions(+)
diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index 43076c9..eeb231b 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -50,6 +50,7 @@
#include "commands/defrem.h"
#include "commands/event_trigger.h"
#include "commands/extension.h"
+#include "commands/policy.h"
#include "commands/proclang.h"
#include "commands/schemacmds.h"
#include "commands/tablecmds.h"
@@ -1245,6 +1246,18 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
sdepForm->classid,
sdepForm->objid);
break;
+ case SHARED_DEPENDENCY_POLICY:
+ /* If unable to remove role from policy, remove policy. */
+ if (!RemoveRoleFromObjectPolicy(roleid,
+ sdepForm->classid,
+ sdepForm->objid))
+ {
+ obj.classId = sdepForm->classid;
+ obj.objectId = sdepForm->objid;
+ obj.objectSubId = sdepForm->objsubid;
+ add_exact_object_address(&obj, deleteobjs);
+ }
+ break;
case SHARED_DEPENDENCY_OWNER:
/* If a local object, save it for deletion below */
if (sdepForm->dbid == MyDatabaseId)
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 8851fe7..7bdc1a4 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -408,6 +408,252 @@ RemovePolicyById(Oid policy_id)
}
/*
+ * RemoveRoleFromObjectPolicy -
+ * remove a role from a policy by its OID. If the role is not a member of
+ * the policy then an error is raised. False is returned to indicate that
+ * the role could not be removed due to being the only role on the policy
+ * and therefore the entire policy should be removed.
+ *
+ * Note that a warning will be thrown and true will be returned on a
+ * permission error, as the policy should not be removed in that case.
+ *
+ * roleid - the oid of the role to remove
+ * classid - should always be PolicyRelationId
+ * policy_id - the oid of the policy.
+ */
+bool
+RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
+{
+ Relation pg_policy_rel;
+ SysScanDesc sscan;
+ ScanKeyData skey[1];
+ HeapTuple tuple;
+ Oid relid;
+ Relation rel;
+ ArrayType *policy_roles;
+ int num_roles;
+ Datum roles_datum;
+ bool attr_isnull;
+ bool noperm = true;
+
+ Assert(classid == PolicyRelationId);
+
+ pg_policy_rel = heap_open(PolicyRelationId, RowExclusiveLock);
+
+ /*
+ * Find the policy to update.
+ */
+ ScanKeyInit(&skey[0],
+ ObjectIdAttributeNumber,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(policy_id));
+
+ sscan = systable_beginscan(pg_policy_rel, PolicyOidIndexId, true,
+ NULL, 1, skey);
+
+ tuple = systable_getnext(sscan);
+
+ /* Raise an error if we don't find the policy. */
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "could not find tuple for policy %u", policy_id);
+
+ /*
+ * Open and exclusive-lock the relation the policy belongs to.
+ */
+ relid = ((Form_pg_policy) GETSTRUCT(tuple))->polrelid;
+
+ rel = relation_open(relid, AccessExclusiveLock);
+
+ if (rel->rd_rel->relkind != RELKIND_RELATION)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is not a table",
+ RelationGetRelationName(rel))));
+
+ if (!allowSystemTableMods && IsSystemRelation(rel))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: \"%s\" is a system catalog",
+ RelationGetRelationName(rel))));
+
+ /* Get the current set of roles */
+ roles_datum = heap_getattr(tuple,
+ Anum_pg_policy_polroles,
+ RelationGetDescr(pg_policy_rel),
+ &attr_isnull);
+
+ Assert(!attr_isnull);
+
+ policy_roles = DatumGetArrayTypePCopy(roles_datum);
+
+ /* We should be removing exactly one entry from the roles array */
+ num_roles = ARR_DIMS(policy_roles)[0] - 1;
+
+ Assert(num_roles >= 0);
+
+ /* Must own relation. */
+ if (pg_class_ownercheck(relid, GetUserId()))
+ noperm = false; /* user is allowed to modify this policy */
+ else
+ ereport(WARNING,
+ (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
+ errmsg("role \"%s\" could not be removed from policy \"%s\" on \"%s\"",
+ GetUserNameFromId(roleid, false),
+ NameStr(((Form_pg_policy) GETSTRUCT(tuple))->polname),
+ RelationGetRelationName(rel))));
+
+ /*
+ * If multiple roles exist on this policy, then remove the one we were
+ * asked to and leave the rest.
+ */
+ if (!noperm && num_roles > 0)
+ {
+ int i, j;
+ Oid *roles = (Oid *) ARR_DATA_PTR(policy_roles);
+ Datum *role_oids;
+ char *qual_value;
+ Node *qual_expr;
+ ParseState *qual_pstate;
+ char *with_check_value;
+ Node *with_check_qual;
+ ParseState *with_check_pstate;
+ Datum values[Natts_pg_policy];
+ bool isnull[Natts_pg_policy];
+ bool replaces[Natts_pg_policy];
+ Datum value_datum;
+ ArrayType *role_ids;
+ HeapTuple new_tuple;
+ ObjectAddress target;
+ ObjectAddress myself;
+
+ /* zero-clear */
+ memset(values, 0, sizeof(values));
+ memset(replaces, 0, sizeof(replaces));
+ memset(isnull, 0, sizeof(isnull));
+
+ /*
+ * All of the dependencies will be removed from the policy and then
+ * re-added. In order to get them correct, we need to extract out
+ * the expressions in the policy and construct a parsestate just
+ * enough to build the range table(s) to then pass to
+ * recordDependencyOnExpr().
+ */
+
+ /* Get policy qual, to update dependencies */
+ value_datum = heap_getattr(tuple, Anum_pg_policy_polqual,
+ RelationGetDescr(pg_policy_rel), &attr_isnull);
+ if (!attr_isnull)
+ {
+ /* parsestate is built just to build the range table */
+ qual_pstate = make_parsestate(NULL);
+
+ qual_value = TextDatumGetCString(value_datum);
+ qual_expr = stringToNode(qual_value);
+
+ /* Add this rel to the parsestate's rangetable, for dependencies */
+ addRangeTableEntryForRelation(qual_pstate, rel, NULL, false, false);
+ }
+ else
+ qual_expr = NULL;
+
+ /* Get WITH CHECK qual, to update dependencies */
+ value_datum = heap_getattr(tuple, Anum_pg_policy_polwithcheck,
+ RelationGetDescr(pg_policy_rel), &attr_isnull);
+ if (!attr_isnull)
+ {
+ /* parsestate is built just to build the range table */
+ with_check_pstate = make_parsestate(NULL);
+
+ with_check_value = TextDatumGetCString(value_datum);
+ with_check_qual = stringToNode(with_check_value);
+
+ /* Add this rel to the parsestate's rangetable, for dependencies */
+ addRangeTableEntryForRelation(with_check_pstate, rel, NULL, false,
+ false);
+ }
+ else
+ with_check_qual = NULL;
+
+ /* Rebuild the roles array to then update the pg_policy tuple with */
+ role_oids = (Datum *) palloc(num_roles * sizeof(Datum));
+ for (i = 0, j = 0; i < ARR_DIMS(policy_roles)[0]; i++)
+ /* Copy over all of the roles which are not the one being removed */
+ if (roles[i] != roleid)
+ role_oids[j++] = ObjectIdGetDatum(roles[i]);
+
+ /* We should have only removed the one role */
+ Assert(j == num_roles);
+
+ /* This is the array for the new tuple */
+ role_ids = construct_array(role_oids, num_roles, OIDOID,
+ sizeof(Oid), true, 'i');
+
+ replaces[Anum_pg_policy_polroles - 1] = true;
+ values[Anum_pg_policy_polroles - 1] = PointerGetDatum(role_ids);
+
+ new_tuple = heap_modify_tuple(tuple,
+ RelationGetDescr(pg_policy_rel),
+ values, isnull, replaces);
+ simple_heap_update(pg_policy_rel, &new_tuple->t_self, new_tuple);
+
+ /* Update Catalog Indexes */
+ CatalogUpdateIndexes(pg_policy_rel, new_tuple);
+
+ /* Remove all old dependencies. */
+ deleteDependencyRecordsFor(PolicyRelationId, policy_id, false);
+
+ /* Record the new set of dependencies */
+ target.classId = RelationRelationId;
+ target.objectId = relid;
+ target.objectSubId = 0;
+
+ myself.classId = PolicyRelationId;
+ myself.objectId = policy_id;
+ myself.objectSubId = 0;
+
+ recordDependencyOn(&myself, &target, DEPENDENCY_AUTO);
+
+ if (qual_expr)
+ recordDependencyOnExpr(&myself, qual_expr, qual_pstate->p_rtable,
+ DEPENDENCY_NORMAL);
+
+ if (with_check_qual)
+ recordDependencyOnExpr(&myself, with_check_qual,
+ with_check_pstate->p_rtable,
+ DEPENDENCY_NORMAL);
+
+ /* Remove all the old shared dependencies (roles) */
+ deleteSharedDependencyRecordsFor(PolicyRelationId, policy_id, 0);
+
+ /* Record the new shared dependencies (roles) */
+ target.classId = AuthIdRelationId;
+ target.objectSubId = 0;
+ for (i = 0; i < num_roles; i++)
+ {
+ target.objectId = DatumGetObjectId(role_oids[i]);
+ /* no need for dependency on the public role */
+ if (target.objectId != ACL_ID_PUBLIC)
+ recordSharedDependencyOn(&myself, &target,
+ SHARED_DEPENDENCY_POLICY);
+ }
+
+ InvokeObjectPostAlterHook(PolicyRelationId, policy_id, 0);
+
+ heap_freetuple(new_tuple);
+
+ /* Invalidate Relation Cache */
+ CacheInvalidateRelcache(rel);
+ }
+
+ /* Clean up. */
+ systable_endscan(sscan);
+ relation_close(rel, AccessExclusiveLock);
+ heap_close(pg_policy_rel, RowExclusiveLock);
+
+ return(noperm || num_roles > 0);
+}
+
+/*
* CreatePolicy -
* handles the execution of the CREATE POLICY command.
*
diff --git a/src/include/commands/policy.h b/src/include/commands/policy.h
index be00043..4b15887 100644
--- a/src/include/commands/policy.h
+++ b/src/include/commands/policy.h
@@ -23,6 +23,8 @@ extern void RelationBuildRowSecurity(Relation relation);
extern void RemovePolicyById(Oid policy_id);
+extern bool RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid objid);
+
extern ObjectAddress CreatePolicy(CreatePolicyStmt *stmt);
extern ObjectAddress AlterPolicy(AlterPolicyStmt *stmt);
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 0f91ebb..3965fe4 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -3246,6 +3246,20 @@ SET row_security = on;
UPDATE r1 SET a = 30 RETURNING *;
ERROR: new row violates row-level security policy for table "r1"
DROP TABLE r1;
+-- DROP OWNED BY testing
+RESET SESSION AUTHORIZATION;
+CREATE ROLE dob_role1;
+CREATE ROLE dob_role2;
+CREATE TABLE dob_t1 (c1 int);
+CREATE POLICY p1 ON dob_t1 TO dob_role1 USING (true);
+DROP OWNED BY dob_role1;
+DROP POLICY p1 ON dob_t1; -- should fail, already gone
+ERROR: policy "p1" for table "dob_t1" does not exist
+CREATE POLICY p1 ON dob_t1 TO dob_role1,dob_role2 USING (true);
+DROP OWNED BY dob_role1;
+DROP POLICY p1 ON dob_t1; -- should succeed
+DROP USER dob_role1;
+DROP USER dob_role2;
--
-- Clean up objects
--
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index b230a0f..9ff400a 100644
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -1490,6 +1490,25 @@ UPDATE r1 SET a = 30 RETURNING *;
DROP TABLE r1;
+-- DROP OWNED BY testing
+RESET SESSION AUTHORIZATION;
+
+CREATE ROLE dob_role1;
+CREATE ROLE dob_role2;
+
+CREATE TABLE dob_t1 (c1 int);
+
+CREATE POLICY p1 ON dob_t1 TO dob_role1 USING (true);
+DROP OWNED BY dob_role1;
+DROP POLICY p1 ON dob_t1; -- should fail, already gone
+
+CREATE POLICY p1 ON dob_t1 TO dob_role1,dob_role2 USING (true);
+DROP OWNED BY dob_role1;
+DROP POLICY p1 ON dob_t1; -- should succeed
+
+DROP USER dob_role1;
+DROP USER dob_role2;
+
--
-- Clean up objects
--
--
2.5.0
Stephen Frost <sfrost@snowman.net> writes:
I noticed in passing that the role removal documentation should really
also discuss shared objects (as the DROP OWNED BY reference page does).
If you're speaking of section 20.4, that text is all my fault ... but
I'm not clear on what you think needs to be added? The first DROP OWNED
BY will take care of any privileges on shared objects, so I didn't really
think we need to burden the recipe with that detail.
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:
I noticed in passing that the role removal documentation should really
also discuss shared objects (as the DROP OWNED BY reference page does).If you're speaking of section 20.4, that text is all my fault ... but
I'm not clear on what you think needs to be added? The first DROP OWNED
BY will take care of any privileges on shared objects, so I didn't really
think we need to burden the recipe with that detail.
Specifically this:
----
Once any valuable objects have been transferred to new owners, any
remaining objects owned by the role-to-be-dropped can be dropped with
the DROP OWNED command. Again, this command can only access objects in
the current database, so it is necessary to run it in each database that
contains objects owned by the role.
----
Isn't quite right, as databases which are owned by the role you're
trying to get rid of won't be dropped. The "Again," does pay it some
back-handed service but it felt to me like it'd be better if it was more
explicit about shared objects, which won't be dropped even if you do go
through and connect to each database and issue the command. Perhaps
that's a bit excessive as, really, the only kinds of 'owned, shared'
objects currently are databases and maybe it's clear enough that you
have to manually drop databases owned by the role you are trying to
drop, if you don't reassign the ownership. That's what I was referring
to, anyway.
Thanks!
Stephen
Stephen Frost <sfrost@snowman.net> writes:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
If you're speaking of section 20.4, that text is all my fault ... but
I'm not clear on what you think needs to be added? The first DROP OWNED
BY will take care of any privileges on shared objects, so I didn't really
think we need to burden the recipe with that detail.
Specifically this:
...
Isn't quite right, as databases which are owned by the role you're
trying to get rid of won't be dropped.
Ah, good point. I'll add something about that. I'm not sure that we
should talk about shared objects in general, since as you say databases
are the only instance. It would feel like handwaving I think. The point
of that section IMO is to be as concrete as we can be about how to drop
a role.
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
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Stephen Frost wrote:
Still, I'll get a patch worked up for it and then we can discuss the
merits of that patch going in to 9.5 now versus just into HEAD.Cool.
While working on the DROP OWNED BY patch, and part of what took me a bit
longer with it, I came to the realiziation that ALTER POLICY wasn't
handling dependencies quite right. All of the policy's dependencies
would be dropped, but then only those objects referred to in the ALTER
POLICY command would have dependencies recreated for them.
The attached patch fixes that (using the same approach that I used in
the DROP OWNED BY patch).
Comments welcome, as always.
I'll plan to apply these two patches in a couple of days.
Thanks!
Stephen
Attachments:
rls-alter-policy-dep.v1.patchtext/x-diff; charset=us-asciiDownload
From c3d396f0ca6b5caedb96cd3d00f15e271fda08a3 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Fri, 4 Dec 2015 14:01:23 -0500
Subject: [PATCH] Handle dependencies properly in ALTER POLICY
ALTER POLICY hadn't fully considered partial policy alternation
(eg: change just the roles on the policy, or just change one of
the expressions) when rebuilding the dependencies. Instead, it
would happily remove all dependencies which existed for the
policy and then only recreate the dependencies for the objects
referred to in the specific ALTER POLICY command.
Correct that by extracting and building the dependencies for all
objects referenced by the policy, regardless of if they were
provided as part of the ALTER POLICY command or were already in
place as part of the pre-existing policy.
---
src/backend/commands/policy.c | 97 +++++++++++++++++++++++++++++++
src/test/regress/expected/rowsecurity.out | 43 ++++++++++++++
src/test/regress/sql/rowsecurity.sql | 31 ++++++++++
3 files changed, 171 insertions(+)
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 8851fe7..2db8125 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -766,6 +766,35 @@ AlterPolicy(AlterPolicyStmt *stmt)
replaces[Anum_pg_policy_polroles - 1] = true;
values[Anum_pg_policy_polroles - 1] = PointerGetDatum(role_ids);
}
+ else
+ {
+ Oid *roles;
+ Datum roles_datum;
+ bool attr_isnull;
+ ArrayType *policy_roles;
+
+ /*
+ * We need to pull the set of roles this policy applies to from
+ * what's in the catalog, so that we can recreate the dependencies
+ * correctly for the policy.
+ */
+
+ roles_datum = heap_getattr(policy_tuple, Anum_pg_policy_polroles,
+ RelationGetDescr(pg_policy_rel),
+ &attr_isnull);
+ Assert(!attr_isnull);
+
+ policy_roles = DatumGetArrayTypePCopy(roles_datum);
+
+ roles = (Oid *) ARR_DATA_PTR(policy_roles);
+
+ nitems = ARR_DIMS(policy_roles)[0];
+
+ role_oids = (Datum *) palloc(nitems * sizeof(Datum));
+
+ for (i = 0; i < nitems; i++)
+ role_oids[i] = ObjectIdGetDatum(roles[i]);
+ }
if (qual != NULL)
{
@@ -773,6 +802,40 @@ AlterPolicy(AlterPolicyStmt *stmt)
values[Anum_pg_policy_polqual - 1]
= CStringGetTextDatum(nodeToString(qual));
}
+ else
+ {
+ Datum value_datum;
+ bool attr_isnull;
+
+ /*
+ * We need to pull the USING expression and build the range table for
+ * the policy from what's in the catalog, so that we can recreate
+ * the dependencies correctly for the policy.
+ */
+
+ /* Check if the policy has a USING expr */
+ value_datum = heap_getattr(policy_tuple, Anum_pg_policy_polqual,
+ RelationGetDescr(pg_policy_rel),
+ &attr_isnull);
+ if (!attr_isnull)
+ {
+ char *qual_value;
+ ParseState *qual_pstate = make_parsestate(NULL);
+
+ /* parsestate is built just to build the range table */
+ qual_pstate = make_parsestate(NULL);
+
+ qual_value = TextDatumGetCString(value_datum);
+ qual = stringToNode(qual_value);
+
+ /* Add this rel to the parsestate's rangetable, for dependencies */
+ addRangeTableEntryForRelation(qual_pstate, target_table, NULL,
+ false, false);
+
+ qual_parse_rtable = qual_pstate->p_rtable;
+ free_parsestate(qual_pstate);
+ }
+ }
if (with_check_qual != NULL)
{
@@ -780,6 +843,40 @@ AlterPolicy(AlterPolicyStmt *stmt)
values[Anum_pg_policy_polwithcheck - 1]
= CStringGetTextDatum(nodeToString(with_check_qual));
}
+ else
+ {
+ Datum value_datum;
+ bool attr_isnull;
+
+ /*
+ * We need to pull the WITH CHECK expression and build the range table
+ * for the policy from what's in the catalog, so that we can recreate
+ * the dependencies correctly for the policy.
+ */
+
+ /* Check if the policy has a WITH CHECK expr */
+ value_datum = heap_getattr(policy_tuple, Anum_pg_policy_polwithcheck,
+ RelationGetDescr(pg_policy_rel),
+ &attr_isnull);
+ if (!attr_isnull)
+ {
+ char *with_check_value;
+ ParseState *with_check_pstate = make_parsestate(NULL);
+
+ /* parsestate is built just to build the range table */
+ with_check_pstate = make_parsestate(NULL);
+
+ with_check_value = TextDatumGetCString(value_datum);
+ with_check_qual = stringToNode(with_check_value);
+
+ /* Add this rel to the parsestate's rangetable, for dependencies */
+ addRangeTableEntryForRelation(with_check_pstate, target_table, NULL,
+ false, false);
+
+ with_check_parse_rtable = with_check_pstate->p_rtable;
+ free_parsestate(with_check_pstate);
+ }
+ }
new_tuple = heap_modify_tuple(policy_tuple,
RelationGetDescr(pg_policy_rel),
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 0f91ebb..e7b6ff4 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -3246,6 +3246,49 @@ SET row_security = on;
UPDATE r1 SET a = 30 RETURNING *;
ERROR: new row violates row-level security policy for table "r1"
DROP TABLE r1;
+-- Check dependency handling
+RESET SESSION AUTHORIZATION;
+CREATE TABLE dep1 (c1 int);
+CREATE TABLE dep2 (c1 int);
+CREATE POLICY dep_p1 ON dep1 TO rls_regress_user1 USING (c1 > (select max(dep2.c1) from dep2));
+ALTER POLICY dep_p1 ON dep1 TO rls_regress_user1,rls_regress_user2;
+-- Should return one
+SELECT count(*) = 1 FROM pg_depend
+ WHERE objid = (SELECT oid FROM pg_policy WHERE polname = 'dep_p1')
+ AND refobjid = (SELECT oid FROM pg_class WHERE relname = 'dep2');
+ ?column?
+----------
+ t
+(1 row)
+
+ALTER POLICY dep_p1 ON dep1 USING (true);
+-- Should return one
+SELECT count(*) = 1 FROM pg_shdepend
+ WHERE objid = (SELECT oid FROM pg_policy WHERE polname = 'dep_p1')
+ AND refobjid = (SELECT oid FROM pg_authid WHERE rolname = 'rls_regress_user1');
+ ?column?
+----------
+ t
+(1 row)
+
+-- Should return one
+SELECT count(*) = 1 FROM pg_shdepend
+ WHERE objid = (SELECT oid FROM pg_policy WHERE polname = 'dep_p1')
+ AND refobjid = (SELECT oid FROM pg_authid WHERE rolname = 'rls_regress_user2');
+ ?column?
+----------
+ t
+(1 row)
+
+-- Should return zero
+SELECT count(*) = 0 FROM pg_depend
+ WHERE objid = (SELECT oid FROM pg_policy WHERE polname = 'dep_p1')
+ AND refobjid = (SELECT oid FROM pg_class WHERE relname = 'dep2');
+ ?column?
+----------
+ t
+(1 row)
+
--
-- Clean up objects
--
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index b230a0f..b06a206 100644
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -1490,6 +1490,37 @@ UPDATE r1 SET a = 30 RETURNING *;
DROP TABLE r1;
+-- Check dependency handling
+RESET SESSION AUTHORIZATION;
+CREATE TABLE dep1 (c1 int);
+CREATE TABLE dep2 (c1 int);
+
+CREATE POLICY dep_p1 ON dep1 TO rls_regress_user1 USING (c1 > (select max(dep2.c1) from dep2));
+ALTER POLICY dep_p1 ON dep1 TO rls_regress_user1,rls_regress_user2;
+
+-- Should return one
+SELECT count(*) = 1 FROM pg_depend
+ WHERE objid = (SELECT oid FROM pg_policy WHERE polname = 'dep_p1')
+ AND refobjid = (SELECT oid FROM pg_class WHERE relname = 'dep2');
+
+ALTER POLICY dep_p1 ON dep1 USING (true);
+
+-- Should return one
+SELECT count(*) = 1 FROM pg_shdepend
+ WHERE objid = (SELECT oid FROM pg_policy WHERE polname = 'dep_p1')
+ AND refobjid = (SELECT oid FROM pg_authid WHERE rolname = 'rls_regress_user1');
+
+-- Should return one
+SELECT count(*) = 1 FROM pg_shdepend
+ WHERE objid = (SELECT oid FROM pg_policy WHERE polname = 'dep_p1')
+ AND refobjid = (SELECT oid FROM pg_authid WHERE rolname = 'rls_regress_user2');
+
+-- Should return zero
+SELECT count(*) = 0 FROM pg_depend
+ WHERE objid = (SELECT oid FROM pg_policy WHERE polname = 'dep_p1')
+ AND refobjid = (SELECT oid FROM pg_class WHERE relname = 'dep2');
+
+
--
-- Clean up objects
--
--
2.5.0
On Thu, Dec 3, 2015 at 9:51 PM, Noah Misch <noah@leadboat.com> wrote:
On Tue, Dec 01, 2015 at 11:05:47AM -0500, Robert Haas wrote:
On Mon, Nov 30, 2015 at 2:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
* Foreign join pushdown vs EvalPlanQual
Is this fixed by 5fc4c26db? If not, what remains to do?
Unfortunately, no. That commit allows FDWs to do proper EPQ handling
for plain table scans, but it proves to be inadequate for EPQ handling
for joins. Solving that problem will require another patch, and,
modulo a bunch of cosmetic issues, I'm reasonably happy with KaiGai
Kohei's latest submission. I'll respond in more detail on that
thread, but the question I want to raise here is: do we want to
back-patch those changes to 9.5 at this late date?Yes. If 9.5 added a bad interface, better to fix the interface even now than
to live with the bad one.
OK, I've pushed the latest patch for that issue to master and 9.5.
I'm not completely positive we've killed this problem dead, but I hope
so.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015/12/09 2:56, Robert Haas wrote:
On Thu, Dec 3, 2015 at 9:51 PM, Noah Misch <noah@leadboat.com> wrote:
On Tue, Dec 01, 2015 at 11:05:47AM -0500, Robert Haas wrote:
On Mon, Nov 30, 2015 at 2:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
* Foreign join pushdown vs EvalPlanQual
Is this fixed by 5fc4c26db? If not, what remains to do?
Unfortunately, no. That commit allows FDWs to do proper EPQ handling
for plain table scans, but it proves to be inadequate for EPQ handling
for joins. Solving that problem will require another patch, and,
modulo a bunch of cosmetic issues, I'm reasonably happy with KaiGai
Kohei's latest submission. I'll respond in more detail on that
thread, but the question I want to raise here is: do we want to
back-patch those changes to 9.5 at this late date?
Yes. If 9.5 added a bad interface, better to fix the interface even now than
to live with the bad one.
OK, I've pushed the latest patch for that issue to master and 9.5.
I'm not completely positive we've killed this problem dead, but I hope
so.
Thank you for committing the patch!
Sorry, I overlooked a typo in docs: s/more that one/more than one/
Please find attached a patch.
Best regards,
Etsuro Fujita
Attachments:
RecheckForeignScan-doc-typo.patchapplication/x-patch; name=RecheckForeignScan-doc-typo.patchDownload
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 0090e24..dc2d890 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -793,7 +793,7 @@ RecheckForeignScan (ForeignScanState *node, TupleTableSlot *slot);
<literal>ForeignScan</>. When a recheck is required, this subplan
can be executed and the resulting tuple can be stored in the slot.
This plan need not be efficient since no base table will return more
- that one row; for example, it may implement all joins as nested loops.
+ than one row; for example, it may implement all joins as nested loops.
</para>
</sect2>
On Wed, Dec 9, 2015 at 2:52 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
Thank you for committing the patch!
Sorry, I overlooked a typo in docs: s/more that one/more than one/ Please
find attached a patch.
Committed, thanks.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015/12/11 1:18, Robert Haas wrote:
On Wed, Dec 9, 2015 at 2:52 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:Thank you for committing the patch!
Sorry, I overlooked a typo in docs: s/more that one/more than one/ Please
find attached a patch.
Committed, thanks.
Thanks!
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Dec 4, 2015 at 3:22 PM, Stephen Frost <sfrost@snowman.net> wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Stephen Frost wrote:
Still, I'll get a patch worked up for it and then we can discuss the
merits of that patch going in to 9.5 now versus just into HEAD.Cool.
While working on the DROP OWNED BY patch, and part of what took me a bit
longer with it, I came to the realiziation that ALTER POLICY wasn't
handling dependencies quite right. All of the policy's dependencies
would be dropped, but then only those objects referred to in the ALTER
POLICY command would have dependencies recreated for them.The attached patch fixes that (using the same approach that I used in
the DROP OWNED BY patch).Comments welcome, as always.
I'll plan to apply these two patches in a couple of days.
It's been a week?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Robert Haas (robertmhaas@gmail.com) wrote:
On Fri, Dec 4, 2015 at 3:22 PM, Stephen Frost <sfrost@snowman.net> wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Stephen Frost wrote:
Still, I'll get a patch worked up for it and then we can discuss the
merits of that patch going in to 9.5 now versus just into HEAD.Cool.
While working on the DROP OWNED BY patch, and part of what took me a bit
longer with it, I came to the realiziation that ALTER POLICY wasn't
handling dependencies quite right. All of the policy's dependencies
would be dropped, but then only those objects referred to in the ALTER
POLICY command would have dependencies recreated for them.The attached patch fixes that (using the same approach that I used in
the DROP OWNED BY patch).Comments welcome, as always.
I'll plan to apply these two patches in a couple of days.
It's been a week?
I've pushed these now.
Thanks!
Stephen