Re: restructuring "alter table" privilege checks

(2010/01/24 12:40), KaiGai Kohei wrote:
> Perhaps, it may be a good idea to make two conceptual patches both head of
> the ATPrepCmd() and ATExec*(). They will make clear good/bad points between
> two approaches.

I tried to make two conceptual patches.

* pgsql-at-rework-prep.1.patch

It adds ATPermCmd(Relation rel, AlterTableCmd *cmd) that is called from the
head of ATPrepCmd(). This function enables to divide the logic of permission
checks depending on cmd->subtype from ATPrepCmd().
In most of subcommand (it does not check permission except for ownership of
the relation to be altered), it calls ATSimplePermissions() or similar.
Or, it does not anything at the stage for rest of exceptions.

Good: Here is only one entrypoint to call ATPermCmd().
Bad: Although most of logics are consolidated into ATPremCmd(), we need to
put individual checks on some of exception cases.

Was it matching with what you suggested? Or, am I missing something?

* pgsql-at-rework-exec.2.patch

It moves permission checks into the head (or just after all needed information
was gathered) of ATExec*() functions. The ATPrepCmd() checks only correctness
of relkind and ensure the relation is not system catalog.
This basis/concept can be applied to ALTER TABLE RENAME TO/SET SCHEMA cases also.

Good: Concept is clear, and less exceptions.
Good: We can apply this concept (just before execution) on other database
objects which does not have explicit preparation stage.
Bad: All the ATExec*() function has to call the permission checks.

My preference is the later approach. Indeed, it has larger number of entrypoints
compared to the ATPermCom() functions, but its concept is clear and we can also
apply same basis on the code path that does not go through ATPrepCmd().

P.S In the right way, this patch should also touch CheckRelationOwnership() or
DefineIndex() logic, but I omitted them because of simplifies.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>