Re: libpq object hooks

On Fri, May 16, 2008 at 10:59 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Merlin Moncure" <mmoncure(at)gmail(dot)com> writes:
>>> typedef void *(*PGobjectEventProc)(PGobjectEventId evtId, ...);
>>> int PQregisterObjectEventProc(PGconn*, PGobjectEventProc);
>>> void *PQeventData(PGconn *, PGobjectEventProc);
>>> void *PQresultEventData(PGresult *, PGobjectEventProc);
>
>> This provides what we need...a key to lookup the hook data without
>> using a string. Also, it reduces the number of exports (it's a little
>> easier for us, while not essential, to not have to register each
>> callback individually). Also, this AFAICT does not have any ABI
>> issues (no struct), and adds less exports which is nice. We don't
>> have to 'look up' the data inside the callbacks..it's properly passed
>> through as an argument. While vararg callbacks may be considered
>> unsafe in some scenarios, I think it's a good fit here.
>
> I don't think varargs callbacks are a good idea at all. Please adjust
> this to something that doesn't do that. Also, haven't you forgotten
> the passthrough void *?

We didn't...one of the functions (the init) doesn't need it so we
didn't expose it to the fixed arguments...we would just va_arg it off
in the other cases. Regardless, your comments below make that moot:

> If you really want only one callback function, perhaps what would work
> is
>
> typedef void (*PGeventProc) (PGeventId eventId, const void *eventInfo,
> void *passthrough);
>
> int PQregisterEventProc(PGconn *conn, PGeventProc proc, void *passthrough);
>
> where eventInfo will point to one of several exported struct types
> depending on the value of eventId. With this approach, you can add
> more fields to the end of any one such struct type without creating
> ABI issues. I have little confidence in being able to make similar
> changes in a varargs environment.

this is fine.

> Also, even if varargs are safe they'd be notationally unpleasant
> in the extreme. varargs are just a PITA to work with --- you'd have
> to do all the decoding in the first-level hook routine, even for
> items you weren't going to use. With something like the above
> all you need is a switch() and some pointer casts.

Switch, plus struct (basically a union) will do the trick nicely. Can
it be a formal union, or is it better as a void*?

The main issue was how what we called the 'hook data' was passed back
and forth. We'll get a patch up.

merlin