[PATCH] remove redundant ownership checks

The patch was not attached...

(2009/12/16 15:15), KaiGai Kohei wrote:

It is a cleanup patch apart from SELinux and security framework.

Now, EnableDisableRule() checks ownership of the relation which
owns the rewrite rule to be enabled/disabled.

But it has the following call path, and this check is already done
in the ATPrepCmd().

ATExecCmd()
-> ATExecEnableDisableRule()
-> EnableDisableRule()

This patch removes redundant permission checks.
No need to check same things twice.

Also see the related discussions:
http://archives.postgresql.org/pgsql-hackers/2009-09/msg01593.php
http://archives.postgresql.org/pgsql-hackers/2009-09/msg01839.php
http://archives.postgresql.org/pgsql-hackers/2009-09/msg01840.php

Thanks,

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

KaiGai,

* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:

The patch was not attached...

This patch either does too much, or not enough.

I would either leave the Assert() in-place as a double-check (I presume
that's why it was there in the first place, and if that Assert() fails
then our assumption about the permissions check being already done on
the object in question would be wrong, since the check is done against
the passed-in 'rel' and the assert is that 'rel' and 'ruletup->ev_class'
are the same; if they're not, then we might need to do perms checking on
ruletup->ev_class)

Or

Remove the now-unused variable eventRelationOid.

Overall, I agree with removing this check as it's already done by
ATSimplePermissions() and we don't double-check the permissions in the
other things called through ATExecCmd() (though there are cases where
specific commands have to do *additional* checks beyond what
ATSimplePermissions() does.. it might be worth looking into what those
are and thinking about if we should move/consolidate/etc them, or if it
makes sense to leave them where they are).

Thanks,

Stephen

(2009/12/18 6:38), Stephen Frost wrote:

KaiGai,

* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:

The patch was not attached...

This patch either does too much, or not enough.

I would either leave the Assert() in-place as a double-check (I presume
that's why it was there in the first place, and if that Assert() fails
then our assumption about the permissions check being already done on
the object in question would be wrong, since the check is done against
the passed-in 'rel' and the assert is that 'rel' and 'ruletup->ev_class'
are the same; if they're not, then we might need to do perms checking on
ruletup->ev_class)

Or

Remove the now-unused variable eventRelationOid.

My preference is the later option, because the pg_rewrite entry to be
checked is fetched using RULERELNAME syscache which takes OID of the
relation and name of the rule.
If this Assert() failed, it implies syscache mechanism has problem,
not only integrity of pg_rewrite system catalog.

The attached patch is an revised one.

Thanks,

Overall, I agree with removing this check as it's already done by
ATSimplePermissions() and we don't double-check the permissions in the
other things called through ATExecCmd() (though there are cases where
specific commands have to do *additional* checks beyond what
ATSimplePermissions() does.. it might be worth looking into what those
are and thinking about if we should move/consolidate/etc them, or if it
makes sense to leave them where they are).

Thanks,

Stephen

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:

[ patch to remove EnableDisableRule's permissions check ]

I don't particularly like this patch, mainly because I disagree with
randomly removing permissions checks without any sort of plan about
where they ought to go. There are two principal entry points in
rewriteDefine.c (the other one being DefineQueryRewrite), and currently
they both do permissions checks. There is also a third major function
RenameRewriteRule, currently ifdef'd out because it's not used, which
is commented as "Note that it lacks a permissions check", indicating
that somebody (possibly me, I don't recall for sure) thought it was
surprising that there wasn't such a check there. This is sensible if
you suppose that this file implements rule utility commands that are
more or less directly called by the user, which is in fact the case for
DefineQueryRewrite, and it's not obvious why it wouldn't be the case for
EnableDisableRule. Since that's a globally exposed function, it's quite
possible that there's third-party code calling it and expecting it to do
the appropriate permissions check. (A quick look at Slony, in
particular, would be a good idea here.)

If we're going to start moving these checks around we need a very
well-defined notion of where permissions checks should be made, so that
everyone knows what to expect. I have not seen any plan for that.
Removing one check at a time because it appears to not be necessary
in the code paths you've looked at is not a plan.

regards, tom lane

On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

If we're going to start moving these checks around we need a very
well-defined notion of where permissions checks should be made, so that
everyone knows what to expect.  I have not seen any plan for that.
Removing one check at a time because it appears to not be necessary
in the code paths you've looked at is not a plan.

I'm not completely familiar with the existing code structure here, but
it sort of seems like in general you might want to divide up the
processing of a statement into a parse analysis phase, a permissions
checking phase, and an execution phase. The parse analysis seems to
be mostly separated out into transformXyz() functions, but the
permissions checking is mixed in with the execution. Disentangling
that seems like a job and a half.

...Robert

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

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:

[ patch to remove EnableDisableRule's permissions check ]

I don't particularly like this patch, mainly because I disagree with
randomly removing permissions checks without any sort of plan about
where they ought to go.

The goal of this was to increase consistancy with the rest of the code,
in particular, ATPrepCmd checks ownership rights on the table, and
anything which wants to check permissions beyond that has to do it
independently. Do I like that? No, not really.

There are two principal entry points in
rewriteDefine.c (the other one being DefineQueryRewrite), and currently
they both do permissions checks.

DefineQueryRewrite gets called out of tcop/utility.c (either under a
CREATE VIEW or a CREATE RULE). tcop/utility.c expects the functions it
calls to to handle permissions checking (except in the one case it
doesn't call a function- T_IndexStmt). Interestingly, DefineIndex,
while it handles a number of other permissions checks, doesn't do checks
to ensure the caller is the table owner- it expects callers to have done
that (which happens both in tcop/utility.c for CREATE INDEX and in
ATPrepCmd for ALTER TABLE ...).

There is also a third major function
RenameRewriteRule, currently ifdef'd out because it's not used, which
is commented as "Note that it lacks a permissions check", indicating
that somebody (possibly me, I don't recall for sure) thought it was
surprising that there wasn't such a check there. This is sensible if
you suppose that this file implements rule utility commands that are
more or less directly called by the user, which is in fact the case for
DefineQueryRewrite, and it's not obvious why it wouldn't be the case for
EnableDisableRule. Since that's a globally exposed function, it's quite
possible that there's third-party code calling it and expecting it to do
the appropriate permissions check. (A quick look at Slony, in
particular, would be a good idea here.)

Personally I find it suprising that things called from ATExecCmd()
expect "some" permissions checks to have been done already.

If we're going to start moving these checks around we need a very
well-defined notion of where permissions checks should be made, so that
everyone knows what to expect. I have not seen any plan for that.
Removing one check at a time because it appears to not be necessary
in the code paths you've looked at is not a plan.

What I suggested previously, though it may be naive, is to do the
permissions checks when you actually have enough information to do them.
At the moment we do a 'simple' check in ATPrepCmd (essentially,
ownership of the relation) and then any more complicated checks have to
be done by the function which actually understands what's going on
enough to know what else needs to be checked (eg:
ATAddForeignKeyConstraint).

As I see it, you've mainly got three steps:

Parsing the command (syntax, basic understanding)
Validation (check for object existance, get object info, etc)
Implementation (do the requested action)

I would put the permissions checking between "Validation" and
"Implementation", ideally at the 'top' of "Implementation". This, imv,
is pretty similar to how we handle DML commands today-
parsing/validation are done first but then the permissions checks aren't
done until we actually go to run the command (of course, this also deals
with prepared queries and the like). Right now what we have are a bunch
of checks scattered around during Validation, as we come across things
we think should be checked (gee, we know the table the user referenced,
let's check if he owns it now).

Of course, this patch isn't doing that because it was intended to make
the existing code consistant, not institute a new policy for how
permissions checking should be done. The big downside about trying to
define a new policy is that it's alot more difficult to get agreement
on, and there are likely to be exceptions brought out that might make
the policy appear to be ill-formed.

Do people think this is a sensible approach? Are there known exceptions
where this won't work or would cause problems? Is it possible to make
that kind of deliniation in general? Should we work up an example patch
which moves some of these checks in that direction?

Thanks for your comments.

Stephen

* Robert Haas (robertmhaas@gmail.com) wrote:

Disentangling that seems like a job and a half.

Indeed it will be, but I think it would be a good thing to actually have
a defined point at which permissions checking is to be done. Trying to
read the code and figure out what permissions you need to perform
certain actions, when some of those checks are done as 'prep work' far
up the tree, isn't fun. Makes validation of the checks we say we do in
the documentation more difficult too. Not to mention that if we want to
allow more granular permission granting for certain operations, it gets
even uglier..

Thanks,

Stephen

On Thu, Dec 17, 2009 at 10:14 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Robert Haas (robertmhaas@gmail.com) wrote:

Disentangling that seems like a job and a half.

Indeed it will be, but I think it would be a good thing to actually have
a defined point at which permissions checking is to be done.

Completely agree...

...Robert

(2009/12/18 9:19), Tom Lane wrote:

KaiGai Kohei<kaigai@ak.jp.nec.com> writes:

[ patch to remove EnableDisableRule's permissions check ]

I don't particularly like this patch, mainly because I disagree with
randomly removing permissions checks without any sort of plan about
where they ought to go. There are two principal entry points in
rewriteDefine.c (the other one being DefineQueryRewrite), and currently
they both do permissions checks. There is also a third major function
RenameRewriteRule, currently ifdef'd out because it's not used, which
is commented as "Note that it lacks a permissions check", indicating
that somebody (possibly me, I don't recall for sure) thought it was
surprising that there wasn't such a check there. This is sensible if
you suppose that this file implements rule utility commands that are
more or less directly called by the user, which is in fact the case for
DefineQueryRewrite, and it's not obvious why it wouldn't be the case for
EnableDisableRule. Since that's a globally exposed function, it's quite
possible that there's third-party code calling it and expecting it to do
the appropriate permissions check. (A quick look at Slony, in
particular, would be a good idea here.)

If we consider this permission check is a part of specification in
the EnableDisableRule(), it is a viewpoint/standpoint.

However, if we adopt this standpoint, we should skip ATSimplePermissions()
for ENABLE/DISABLE RULE options in ATPrepCmd(), because ATExecCmd() calls
EnableDisableRule() which applies permission checks according to the
specification.

We don't need to apply same checks twice. It will be enough with either
of them. But I don't think skipping ATSimplePermissions() is a right
direction, because the existing PG model obviously handles rewrite rules
as properties of relation, although it is not stored within pg_class.
So, it is quite natural to check permission to modify properties of
relations in ATPrepCmd().

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:

[ patch to remove EnableDisableRule's permissions check ]

I don't particularly like this patch, mainly because I disagree with
randomly removing permissions checks without any sort of plan about
where they ought to go.

So where ought they to go?

If we're going to start moving these checks around we need a very
well-defined notion of where permissions checks should be made, so that
everyone knows what to expect.  I have not seen any plan for that.

This seems to me to get right the heart of the matter. When I
submitted my machine-readable explain patch, you critiqued it for
implementing half of an abstraction layer, and it seems to me that our
current permissions-checking logic has precisely the same issue. We
consistently write code that starts by checking permissions and then
moves right along to implementing the action. Those two things need
to be severed. I see two ways to do this. Given a function that (A)
does some prep work, (B) checks permissions, and (C) performs the
action, we could either:

1. Make the existing function do (A) and (B) and then call another
function to do (C), or
2. Make the existing function do (A), call another function to do (B),
and then do (C) itself.

I'm not sure which will work better, but I think making a decision
about which way to do it and how to name the functions would be a big
step towards having a coherent plan for this project.

A related issue is where parse analysis should be performed. We're
not completely consistent about this right now. Most of it seems to
be done by code in the "parser" directory, but there are several
exceptions, including DefineRule().

...Robert

(2009/12/21 12:53), Robert Haas wrote:

On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:

KaiGai Kohei<kaigai@ak.jp.nec.com> writes:

[ patch to remove EnableDisableRule's permissions check ]

I don't particularly like this patch, mainly because I disagree with
randomly removing permissions checks without any sort of plan about
where they ought to go.

So where ought they to go?

If we're going to start moving these checks around we need a very
well-defined notion of where permissions checks should be made, so that
everyone knows what to expect. I have not seen any plan for that.

This seems to me to get right the heart of the matter. When I
submitted my machine-readable explain patch, you critiqued it for
implementing half of an abstraction layer, and it seems to me that our
current permissions-checking logic has precisely the same issue. We
consistently write code that starts by checking permissions and then
moves right along to implementing the action. Those two things need
to be severed. I see two ways to do this. Given a function that (A)
does some prep work, (B) checks permissions, and (C) performs the
action, we could either:

1. Make the existing function do (A) and (B) and then call another
function to do (C), or
2. Make the existing function do (A), call another function to do (B),
and then do (C) itself.

I'm not sure which will work better, but I think making a decision
about which way to do it and how to name the functions would be a big
step towards having a coherent plan for this project.

We have mixed policy in the current implementation.

The point is what database object shall be handled in this function.

If we consider a rewrite rule as a database object, not a property of
the relation, it seems to me a correct manner to apply permission checks
in the EnableDisableRule(), because it handles a given rewrite rule.

If we consider a rewrite rule as a property of a relation, not an independent
database object, it seems to me we should apply permission checks in ATPrepCmd()
which handles a relation, rather than EnableDisableRule().

My patch stands on the later perspective, because pg_rewrite entries don't
have its own ownership and access privileges, and it is always owned by
a certain relation.

Thanks,

A related issue is where parse analysis should be performed. We're
not completely consistent about this right now. Most of it seems to
be done by code in the "parser" directory, but there are several
exceptions, including DefineRule().

...Robert

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

2009/12/22 KaiGai Kohei <kaigai@ak.jp.nec.com>:

(2009/12/21 12:53), Robert Haas wrote:

On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane<tgl@sss.pgh.pa.us>  wrote:

KaiGai Kohei<kaigai@ak.jp.nec.com>  writes:

[ patch to remove EnableDisableRule's permissions check ]

I don't particularly like this patch, mainly because I disagree with
randomly removing permissions checks without any sort of plan about
where they ought to go.

So where ought they to go?

If we're going to start moving these checks around we need a very
well-defined notion of where permissions checks should be made, so that
everyone knows what to expect.  I have not seen any plan for that.

This seems to me to get right the heart of the matter.  When I
submitted my machine-readable explain patch, you critiqued it for
implementing half of an abstraction layer, and it seems to me that our
current permissions-checking logic has precisely the same issue.  We
consistently write code that starts by checking permissions and then
moves right along to implementing the action.  Those two things need
to be severed.  I see two ways to do this.  Given a function that (A)
does some prep work, (B) checks permissions, and (C) performs the
action, we could either:

1. Make the existing function do (A) and (B) and then call another
function to do (C), or
2. Make the existing function do (A), call another function to do (B),
and then do (C) itself.

I'm not sure which will work better, but I think making a decision
about which way to do it and how to name the functions would be a big
step towards having a coherent plan for this project.

We have mixed policy in the current implementation.

The point is what database object shall be handled in this function.

If we consider a rewrite rule as a database object, not a property of
the relation, it seems to me a correct manner to apply permission checks
in the EnableDisableRule(), because it handles a given rewrite rule.

If we consider a rewrite rule as a property of a relation, not an independent
database object, it seems to me we should apply permission checks in ATPrepCmd()
which handles a relation, rather than EnableDisableRule().

My patch stands on the later perspective, because pg_rewrite entries don't
have its own ownership and access privileges, and it is always owned by
a certain relation.

That's somewhat separate from the point I was making, but it's a good
point all the same.

...Robert

On Sun, Dec 20, 2009 at 10:53 PM, I wrote:

On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:

[ patch to remove EnableDisableRule's permissions check ]

I don't particularly like this patch, mainly because I disagree with
randomly removing permissions checks without any sort of plan about
where they ought to go.

So where ought they to go?

I have looked this over a little bit and I guess I don't see why the
lack of a grand plan for how to organize all of our permissions checks
ought to keep us from removing this one on the grounds of redundancy.
We have to attack this problem in small pieces if we're going to make
any progress, and the pieces aren't going to get any smaller than
this.

Per Tom's suggestion, I did a quick grep of the Slony source code for
EnableDisableRule and got no hits. I think the appropriate level of
concern about the possibility that third-party code might be calling
this function is to (1) add a comment to the function noting that it
is the caller's responsibility to check permissions, and (2) if we're
*really* concerned that someone might miss that, release-note it. It
is very unclear that it's worth even that much effort, though, since
we have zero evidence that there is any third-party code using this
function directly. A quick Google search on EnableDisableRule hits
mostly this thread.

With respect to cleaning up permissions more generally, it seems to me
that Stephen Frost hit the nail on the upthread when he remarked that
the current coding, where we're expecting certain functions in
tablecmds.c to be called with PART of the permissions checking done,
is fairly counterintuitive. I think it would be interesting to see if
someone could propose a refactoring that eliminates this and puts all
the permissions checking in a single, appropriate location. But ISTM
that this patch would be a subset of any such more comprehensive
patch, so that doesn't seem like a reason to hold off on applying this
either.

So I am inclined to go ahead and apply this with the addition of the
comment discussed above.

...Robert

Robert Haas <robertmhaas@gmail.com> writes:

I have looked this over a little bit and I guess I don't see why the
lack of a grand plan for how to organize all of our permissions checks
ought to keep us from removing this one on the grounds of redundancy.
We have to attack this problem in small pieces if we're going to make
any progress, and the pieces aren't going to get any smaller than
this.

I would turn that argument around: given the lack of a grand plan,
why should we remove this particular check at all? Nobody has argued
that there would be a significant, or even measurable, performance gain.
When and if we do have a plan, we might find ourselves putting this
check back.

Even if you are right in your unsubstantiated hypothesis that this
change will be a subset of any future change that is made with some plan
in mind, I don't see that incremental revisions of the permissions check
placement are a good way to approach the problem. What I fear will
result from that is gaps in permissions checking, depending on what
combination of revisions of core and third-party code happen to get used
in a given installation.

I think we need a plan first, not random patches first.

regards, tom lane

On Sun, Jan 10, 2010 at 4:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I have looked this over a little bit and I guess I don't see why the
lack of a grand plan for how to organize all of our permissions checks
ought to keep us from removing this one on the grounds of redundancy.
We have to attack this problem in small pieces if we're going to make
any progress, and the pieces aren't going to get any smaller than
this.

I would turn that argument around: given the lack of a grand plan,
why should we remove this particular check at all? Nobody has argued
that there would be a significant, or even measurable, performance gain.
When and if we do have a plan, we might find ourselves putting this
check back.

You're arguing against a straw man - there's clearly no argument here
from performance. We generally do not choose to litter the code with
redundant or irrelevant checks because it makes the code difficult to
maintain and understand. Sometimes it also hurts performance, but
that's not a necessary criterion for removal. Nor are we generally in
the habit of keeping redundant code around because a hypothetical
future refactoring might by chance end up putting exactly the same
code back.

Even if you are right in your unsubstantiated hypothesis that this
change will be a subset of any future change that is made with some plan
in mind, I don't see that incremental revisions of the permissions check
placement are a good way to approach the problem.  What I fear will
result from that is gaps in permissions checking, depending on what
combination of revisions of core and third-party code happen to get used
in a given installation.

I think we need a plan first, not random patches first.

I think the issue at hand is completely severable from the issue of a
more general plan. Again, when we find that we find that the code
does the same work twice, that's typically something we would want to
fix, independent of what else might or might not come later. That
having been said, I'm 100% in favor of talking about what a more
general plan should look like; in fact, I asked for your opinion here:

http://archives.postgresql.org/pgsql-hackers/2009-12/msg01824.php

...Robert

Robert Haas wrote:

On Sun, Jan 10, 2010 at 4:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I have looked this over a little bit and I guess I don't see why the
lack of a grand plan for how to organize all of our permissions checks
ought to keep us from removing this one on the grounds of redundancy.
We have to attack this problem in small pieces if we're going to make
any progress, and the pieces aren't going to get any smaller than
this.

I would turn that argument around: given the lack of a grand plan,
why should we remove this particular check at all? Nobody has argued
that there would be a significant, or even measurable, performance gain.
When and if we do have a plan, we might find ourselves putting this
check back.

You're arguing against a straw man - there's clearly no argument here
from performance. We generally do not choose to litter the code with
redundant or irrelevant checks because it makes the code difficult to
maintain and understand. Sometimes it also hurts performance, but
that's not a necessary criterion for removal. Nor are we generally in
the habit of keeping redundant code around because a hypothetical
future refactoring might by chance end up putting exactly the same
code back.

I agree. Why are arbitrary restrictions being placed on code
improvements? If code has no purpose, why not remove it, or at least
mark it as NOT_USED.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

(2010/01/12 10:27), Bruce Momjian wrote:

Robert Haas wrote:

On Sun, Jan 10, 2010 at 4:54 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:

Robert Haas<robertmhaas@gmail.com> writes:

I have looked this over a little bit and I guess I don't see why the
lack of a grand plan for how to organize all of our permissions checks
ought to keep us from removing this one on the grounds of redundancy.
We have to attack this problem in small pieces if we're going to make
any progress, and the pieces aren't going to get any smaller than
this.

I would turn that argument around: given the lack of a grand plan,
why should we remove this particular check at all? Nobody has argued
that there would be a significant, or even measurable, performance gain.
When and if we do have a plan, we might find ourselves putting this
check back.

You're arguing against a straw man - there's clearly no argument here
from performance. We generally do not choose to litter the code with
redundant or irrelevant checks because it makes the code difficult to
maintain and understand. Sometimes it also hurts performance, but
that's not a necessary criterion for removal. Nor are we generally in
the habit of keeping redundant code around because a hypothetical
future refactoring might by chance end up putting exactly the same
code back.

I agree. Why are arbitrary restrictions being placed on code
improvements? If code has no purpose, why not remove it, or at least
mark it as NOT_USED.

The attached patch adds a source code comment which informs developers
that its own permission check had gone at the v8.5 release.

I also think we don't need to note it on the release-note. If we would
describe all the specification changes in external functions, is it
really valuable as a summary? It seems to me too details.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

On Mon, Jan 11, 2010 at 8:27 PM, Bruce Momjian <bruce@momjian.us> wrote:

Robert Haas wrote:

On Sun, Jan 10, 2010 at 4:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I have looked this over a little bit and I guess I don't see why the
lack of a grand plan for how to organize all of our permissions checks
ought to keep us from removing this one on the grounds of redundancy.
We have to attack this problem in small pieces if we're going to make
any progress, and the pieces aren't going to get any smaller than
this.

I would turn that argument around: given the lack of a grand plan,
why should we remove this particular check at all?  Nobody has argued
that there would be a significant, or even measurable, performance gain.
When and if we do have a plan, we might find ourselves putting this
check back.

You're arguing against a straw man - there's clearly no argument here
from performance.  We generally do not choose to litter the code with
redundant or irrelevant checks because it makes the code difficult to
maintain and understand.  Sometimes it also hurts performance, but
that's not a necessary criterion for removal.  Nor are we generally in
the habit of keeping redundant code around because a hypothetical
future refactoring might by chance end up putting exactly the same
code back.

I agree.  Why are arbitrary restrictions being placed on code
improvements?  If code has no purpose, why not remove it, or at least
mark it as NOT_USED.

So, where do we go from here? Any other opinions? I'm not sure how
much it's really worth fighting over a six line patch, but there's
something in me that rails against the idea of telling someone who
took the trouble to write a patch "no" when the only argument against
it is that we might change our mind at some point in the future. Of
course, I will accept the consensus of the community whatever it is,
but the only people who have expressed a clear opinion on this version
of the patch are Tom, Bruce, and myself, and 2-1 is not a consensus.

...Robert

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Jan 11, 2010 at 8:27 PM, Bruce Momjian <bruce@momjian.us> wrote:

I agree. �Why are arbitrary restrictions being placed on code
improvements? �If code has no purpose, why not remove it, or at least
mark it as NOT_USED.

So, where do we go from here? Any other opinions? I'm not sure how
much it's really worth fighting over a six line patch, but there's
something in me that rails against the idea of telling someone who
took the trouble to write a patch "no" when the only argument against
it is that we might change our mind at some point in the future. Of
course, I will accept the consensus of the community whatever it is,
but the only people who have expressed a clear opinion on this version
of the patch are Tom, Bruce, and myself, and 2-1 is not a consensus.

I still say that this patch is putting the cart before the horse.
What we need before we do any significant amount of rearrangement
of security checks is to have a plan for where they should go.
Making a change here and a change there without a plan isn't an
improvement, it just risks creating bugs.

If I thought this patch represented incremental movement in the
direction of a better security-check factorization, I'd be fine with it,
but that's not clear either. The argument for it is that these checks
are redundant with some other ones, but why should we remove these and
not the other ones instead? And there would still be still some checks
left in this file, so it doesn't seem to me that we'd have actually made
any progress towards clarity of where the checks should be.

Plan first, then code, please.

regards, tom lane