Обсуждение: pgsql: Remove some dead code in selfuncs.c

Поиск
Список
Период
Сортировка

pgsql: Remove some dead code in selfuncs.c

От
Alvaro Herrera
Дата:
Remove some dead code in selfuncs.c

RelOptInfo.userid is the same for all relations in a given inheritance
tree, so the code in examine_variable() and example_simple_variable()
that repeats the ACL checks on the root parent rel instead of a given
leaf child relations need not recompute userid too.

Author: Amit Langote <amitlangote09@gmail.com>
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/20221210201753.GA27893@telsasoft.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/438e6b7240905c8055f9e221187f2ac818876169

Modified Files
--------------
src/backend/optimizer/util/relnode.c |  1 -
src/backend/utils/adt/selfuncs.c     | 42 ++++++++++++++----------------------
2 files changed, 16 insertions(+), 27 deletions(-)


Re: pgsql: Remove some dead code in selfuncs.c

От
Tom Lane
Дата:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Remove some dead code in selfuncs.c
> RelOptInfo.userid is the same for all relations in a given inheritance
> tree, so the code in examine_variable() and example_simple_variable()
> that repeats the ACL checks on the root parent rel instead of a given
> leaf child relations need not recompute userid too.

This change seems rather ill-advised.  The premise is false:

regression=# create user joe;
CREATE ROLE
regression=# \c - joe
You are now connected to database "regression" as user "joe".
regression=> create table joestable(f1 int) partition by range (f1);
CREATE TABLE
regression=> \c - postgres
regression=# create table joeschild partition of joestable for values from (1) to (10);
CREATE TABLE
regression=# select relname, relowner from pg_class where relname like 'joe%';
  relname  | relowner
-----------+----------
 joeschild |       10
 joestable |    39822
(2 rows)

I didn't read the actual code, so perhaps it's okay, but not if
it's doing what your commit message says.

            regards, tom lane



Re: pgsql: Remove some dead code in selfuncs.c

От
Tom Lane
Дата:
I wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> Remove some dead code in selfuncs.c
>> RelOptInfo.userid is the same for all relations in a given inheritance
>> tree, so the code in examine_variable() and example_simple_variable()
>> that repeats the ACL checks on the root parent rel instead of a given
>> leaf child relations need not recompute userid too.

> This change seems rather ill-advised.

Ah, sorry, -ENOCAFFEINE.  It's talking about the access-as-user field,
not the relation's owner.  I agree that as querytrees are currently
built, this is probably a safe optimization.  But do we really want
to hard-wire such a subtle assumption to gain a microscopic speed
benefit?  It's not as though GetUserId() is expensive.

            regards, tom lane



Re: pgsql: Remove some dead code in selfuncs.c

От
Alvaro Herrera
Дата:
On 2023-Jan-19, Tom Lane wrote:

> I wrote:
> > Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> >> Remove some dead code in selfuncs.c
> >> RelOptInfo.userid is the same for all relations in a given inheritance
> >> tree, so the code in examine_variable() and example_simple_variable()
> >> that repeats the ACL checks on the root parent rel instead of a given
> >> leaf child relations need not recompute userid too.
> 
> > This change seems rather ill-advised.
> 
> Ah, sorry, -ENOCAFFEINE.  It's talking about the access-as-user field,
> not the relation's owner.  I agree that as querytrees are currently
> built, this is probably a safe optimization.  But do we really want
> to hard-wire such a subtle assumption to gain a microscopic speed
> benefit?  It's not as though GetUserId() is expensive.

Well, I didn't see it as an optimization but rather a removal of
confusing code.  Given the current RTEPermissionInfo representation,
it's just not possible for an "otherrel" to have a different
access-as-user value than what "the relation mentioned in the query"
has -- by construction, they share the same RTEPermissionInfo.

If we wanted to decouple selfuncs.c from that knowledge, then what we
should be doing is obtain the RTEPermissionInfo for the relation, and
use the userid from there.  But the code I deleted wasn't doing that, it
was just using the same 'onerel' all the time.  It's not difficult (or
expensive) to do otherwise, but it seems somewhat pointless.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"El destino baraja y nosotros jugamos" (A. Schopenhauer)



Re: pgsql: Remove some dead code in selfuncs.c

От
Tom Lane
Дата:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Jan-19, Tom Lane wrote:
>> Ah, sorry, -ENOCAFFEINE.  It's talking about the access-as-user field,
>> not the relation's owner.  I agree that as querytrees are currently
>> built, this is probably a safe optimization.  But do we really want
>> to hard-wire such a subtle assumption to gain a microscopic speed
>> benefit?  It's not as though GetUserId() is expensive.

> Well, I didn't see it as an optimization but rather a removal of
> confusing code.  Given the current RTEPermissionInfo representation,
> it's just not possible for an "otherrel" to have a different
> access-as-user value than what "the relation mentioned in the query"
> has -- by construction, they share the same RTEPermissionInfo.

> If we wanted to decouple selfuncs.c from that knowledge, then what we
> should be doing is obtain the RTEPermissionInfo for the relation, and
> use the userid from there.  But the code I deleted wasn't doing that, it
> was just using the same 'onerel' all the time.  It's not difficult (or
> expensive) to do otherwise, but it seems somewhat pointless.

Hmm.  Point taken, and I agree that it's not worth adding complication
that wasn't there before to track this.  No further objection.

            regards, tom lane