pgsql: Remove some dead code in selfuncs.c

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