Re: [v9.2] Object access hooks with arguments support (v1)

On Thu, Sep 29, 2011 at 4:52 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> I noticed that the previous revision does not provide any way to inform
> the modules name of foreign server, even if foreign table was created,
> on the OAT_POST_CREATE hook.
> So, I modified the invocation at heap_create_with_catalog to deliver
> this information to the modules.
>
> Rest of parts were unchanged, except for rebasing to the latest git
> master.

I've never really been totally sanguine with the idea of making object
access hooks take arguments, and all of my general concerns seem to
apply to the way you've set this patch up. In particular:

1. Type safety goes out the window. What you're essentially proposing
here is that we should have one hook function can be used for almost
anything at all and can be getting up to 9 arguments of any type
whatsoever. The hook will need to know how to interpret all of its
arguments depending on the context in which it was called. The
compiler will be totally unable to do any static type-checking, so
there will be absolutely no warning if the interface is changed
incompatibly on either side. Instead, you'll probably just crash the
database server at runtime.

2. The particular choice of data being passed to the object access
hooks appears capricious and arbitrary. Here's an example:

/* Post creation hook for new relation */
- InvokeObjectAccessHook(OAT_POST_CREATE, RelationRelationId, relid, 0);
+ InvokeObjectAccessHook4(OAT_POST_CREATE,
+
RelationRelationId, relid, 0,
+
PointerGetDatum(new_rel_tup),
+
PointerGetDatum(tupdesc),
+
BoolGetDatum(is_select_into),
+
CStringGetDatum(fdw_srvname));

Now, I am sure that you have good reasons for wanting to pass those
particular things to the object access hook rather than any other
local variable or argument that might happen to be lying around at
this point in the code, but they are not documented. If someone adds
a new argument to this function, or removes an argument that's being
passed, they will have no idea what to do about this. Moreover, if
you did document it, I think it would boil down to "this is what
sepgsql happens to need", and I don't think that's an acceptable
answer. We have repeatedly refused to adopt that approach in the
past.

(This particular case is worse than average, because new_rel_tup is
coming from InsertPgClassTuple, which therefore has to be modified,
along with AddNewRelationTuple and index_create, to get the tuple back
up to the call site where, apparently, it is needed.)

I am not exactly sure what the right way to solve this problem is, but
I don't think this is it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company