Обсуждение: pgsql: Remove some dead code in selfuncs.c
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(-)
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
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
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)
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