On Thu, Jan 18, 2024 at 10:56:09AM +0530, Ashutosh Bapat wrote:
> There is some overlap between Dtrace functionality and this
> functionality. But I see differences too. E.g. injection points offer
> deeper integration whereas dtrace provides more information to the
> probe like callstack and argument values etc. We need to assess
> whether these functionality can co-exist and whether we need both of
> them. If the answer to both of these questions is yes, it will be good
> to add documentation explaining the differences and similarities and
> also some guidance on when to use what.
Perhaps, I'm not sure how much we want to do regarding that yet,
injection points have no external dependencies and will work across
all environments as long as dlsym() (or an equivalent) is able to
work, while being cheaper because they don't spawn an external process
to trace the call.
> +
> +#ifdef USE_INJECTION_POINTS
> +static bool
> +file_exists(const char *name)
> +{
> + struct stat st;
> +
> + Assert(name != NULL);
> + if (stat(name, &st) == 0)
> + return !S_ISDIR(st.st_mode);
> + else if (!(errno == ENOENT || errno == ENOTDIR))
> + ereport(ERROR,
> + (errcode_for_file_access(),
> + errmsg("could not access file \"%s\": %m", name)));
> + return false;
> +}
>
> Shouldn't this be removed now? The code should use one from fd.c
Yep, removed that.
> Other code changes look good. I think the documentation and comments
> need some changes esp. considering the users point of view. Have
> attached two patches (0003, and 0004) with those changes to be applied
> on top of 0001 and 0002 respectively. Please review them. Might need
> some wordsmithy and language correction. Attaching the whole patch set
> to keep cibot happy.
The CF bot was perhaps happy but your 0004 has forgotten to update the
expected output. There were also a few typos, some markups and edits
required for 0002 but as a whole what you have suggested was an
improvement. Thanks.
> This is review of 0001 and 0002 only. Once we take care of these
> comments I think those patches will be ready for commit except one
> point of contention mentioned in [1]. We haven't heard any third
> opinion yet.
0001~0004 have been now applied, and I'm marking the CF entry as
committed. I'll create a new thread once I have put more energy into
the regression test improvements. Now the fun can really begin. I am
also going to switch my buildfarm animals to use the new ./configure
switch.
--
Michael