On Thu, Sep 29, 2011 at 4:52 PM, Kohei KaiGai <kai...@kaigai.gr.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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers