Tom Lane napsal(a):
Zdenek Kotala <[EMAIL PROTECTED]> writes:
I performed review and I prepared own patch which contains only probes
without any issue. I suggest commit this patch because the rest of
patch is independent and it can be committed next commit fest after
rework.

I looked at this patch a little bit.  In addition to the comments Alvaro
made, I have a couple more issues:

* The probes that pass buffer tag elements are already broken by the
pending "relation forks" patch: there is soon going to be another field
in buffer tags. Perhaps it'd be feasible to pass the buffer tag as a single probe argument to make that a bit more future-proof? I'm not
sure if that would complicate the use of the probe so much as to be
counterproductive.

It is difficult to say. If you pass only pointer to a structure you can mine all data from them but it is little bit complicated. It should be possible to use typedef and cast pointer to correct structure. However main problem is that you cannot use easily any indirect data in predicates. It needs extra magic. By my opinion best approach is use mix of approaches. Important data should be pass directly as a argument and rest as a pointer to data structure.

* I find this to be truly bletcherous:

<snip>

> What doesn't work fine is #include "postgres.h", which would
> be the ideal way to suck in TransactionId and other needed typedefs.

Whats about #include "c.h"? Does it work or does it have same issue?


What I suggest might be a reasonable compromise is to copy needed
typedefs directly into the probes.d file:

        typedef unsigned int LocalTransactionId;

        provider postgresql {

        probe transaction__start(LocalTransactionId);

This at least makes it possible to declare the probes cleanly,
and it's fairly obvious what to fix if the principal definition of
LocalTransactionId ever changes.  I don't have Solaris to test on, but
on OS X this seems to behave the way we'd like: the typedef itself isn't
copied into the emitted probes.h file, but the emitted extern
declarations use it.

It works on Solaris as well. However, I think that better solution is to put this typedef to the separate header file. DTrace script allows to use typedef or include header with typedefs. It would be better to have header file which could be used for probe.d and for any other DTrace script.


                Zdenek


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to