Re: [PATCH] remove redundant ownership checks

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> Some of ALTER TABLE operations take multiple permission checks, not only
> ownership of the relation to be altered.

Yes, exactly my point. Those places typically just presume that the
owner check has already been done.

> For example, ALTER TABLE with SET TABLESPACE option also need ACL_CREATE
> permission on the new tablespace, not only ownership of the relation.
> It means we need to gather two information before whole of the permission
> checks. (1) OID of the relation to be altered. (2) OID of the tablespace
> to be set.

Right. I would say that we should wait until we have all the necessary
information to do the permissions checking, and then do it all at that
point, similar to how we handle DML today.

> In my understanding, Stephen suggests that we should, ideally, rip out
> permission logic from the code closely-combined with the steps of
> implementation.

This might be a confusion due to language, but I think of the
"implementation" as being "the work". The check on permissions on the
tablespace that you're describing above is done right before "the work".
For this case, specifically, ATPrepSetTableSpace() takes the action on
line 6754: tab->newTableSpace = tablespaceId;
Prior to that, it checks the tablespace permissions (but not the table
permissions, since they've been checked already). I would suggest we
add a call to ATSimplePermissions() in ATPrepSetTableSpace at line 6745-
right after the /* Check its permissions */ comment (which would be
changed to "check permissions for ALTER TABLE x SET TABLESPACE y").

> Of course, it does not mean all the checks should be
> moved just before simple_heap_update().

No, I would have it earlier than simple_heap_update(), we don't need to
go building the structures and whatnot needed to call
simple_heap_update(). For this specific case though, I'm a bit torn by
the fact that the work associated with changing the tablespace can
actually happen in two distinct places- either through
ATExecSetTableSpace, or in ATRewriteTables directly.
ATExecSetTableSpace would actually be a good candidate rather than in
the 'prep' stage, if all tablespace changes were done there. The 'prep'
stage worries me a bit since I'm not sure if all permissions checking
is currently, or coulde be, done at that point, and I'd prefer that we
use the same approach for permissions checking throughout the code- for
example, it's either done in 'phase 3' (where we're going through the
subcommands) or all done in 'phase 1/2', where we're setting things up.

> However, it is a separate topic for this patch.
> This patch just tries to remove redundant checks, and integrate them
> into a common place where most of ALTER TABLE option applies its
> permission checks on.

I don't believe we can make the case that it's a distinct topic based on
the feedback received.

Thanks,

Stephen