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

Robert,

I agree with it is a reasonable argument that compiler cannot raise warnings
if all the arguments are delivered as Datum. In fact, I also tried to implement
this feature with InvokeObjectAccessHook() defined as function.

The first needed point to be improved is that we hope compiler to raise
warnnings if and when number or type of arguments are changed in the
future version.
If so, how about an idea to define data types to inform modules the context
information in, such as:

struct ObjectAccessInfoData {
ObjectAccessType oa_type;
ObjectAddress oa_address;
union {
struct {
HeapTuple newtuple;
TupleDesc tupdesc; /* only if create a new relation */
:
} post_create; /* additional info for OAT_POST_CREATE event */
}
}

Even if its invocation shall be wrapped up by macro, compiler can
raise warnings when caller set values on the ObjectAccessInfoData
structure.

It will also help the second points partially. The objectaccess.h will perform
a placeholder to describe specification of the argument. That clearly informs
developers what informations are available on the hook and what was lacked.

> 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.
>
Unfortunately, I still don't have a clear answer for this point.
However, in general terms, it is impossible to design any interface without
knowledge of its usage more or less.

We have several other hooks being available to loadable modules,
but I don't believe that we designed these hooks without knowledge
of use-cases, more or less.

At least, this proposition enables modules being informed using
commonly used data type (such as HeapTuple, TupleDesc), compared
to the past approach that tried to push all the necessary information
into argument list individually.

I think the answer of this matter is on the middle-of-position between
"we should not know anything about sepgsql" and "we should know
everything about sepgsql", neither of them....

> (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.)
>
It might be a wrong design. All we want to inform was stored within
new_rel_desc in heap_create_with_catalog(). So, I should design the
hook to deliver new_rel_desc, instead of HeapTuple itself that need to
pull up from InsertPgClassTuple.

Thanks,

2011/10/12 Robert Haas <robertmhaas(at)gmail(dot)com>:
> 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.
>
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>