Обсуждение: [HACKERS] dtrace probes
Hi, The lwlock dtrace probes define LWLockMode as int, and the TRACE_POSTGRESQL_LWLOCK methods are called using both a variable and constant definition. This leads to a mix of argument definitions depending on the call site, as seen in probes.txt file. A fix is to explicit cast 'mode' to int such that all call sites will use the argument #2 4 signed bytes definition. Attached patch does this. I have verified all dtraces probes for their type, and only the lock__ methods doesn't aligned with its actual types. However, that would require a change to probes.d, and therefore PostgreSQL 11 material. Depending on the feedback I can add this patch to the open item list in order to fix it for PostgreSQL 10. Best regards, Jesper -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
On Tue, Apr 18, 2017 at 9:38 PM, Jesper Pedersen <jesper.pedersen@redhat.com> wrote: > Hi, > > The lwlock dtrace probes define LWLockMode as int, and the > TRACE_POSTGRESQL_LWLOCK methods are called using both a variable and > constant definition. > > This leads to a mix of argument definitions depending on the call site, as > seen in probes.txt file. > > A fix is to explicit cast 'mode' to int such that all call sites will use > the > > argument #2 4 signed bytes > > definition. Attached patch does this. > I think this fix is harmless and has some value in terms of consistency. One minor suggestion is that you should leave a space after typecasting. - TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode); + TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), (int)mode); There should be a space like "(int) mode". > I have verified all dtraces probes for their type, and only the lock__ > methods doesn't aligned with its actual types. > Do you see any problem with that? > > Depending on the feedback I can add this patch to the open item list in > order to fix it for PostgreSQL 10. > Is there any commit in PG-10 which has caused this behavior? If not, then I don't think it should be added to open items of PG-10. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Hi, On 04/20/2017 09:24 AM, Amit Kapila wrote: >> The lwlock dtrace probes define LWLockMode as int, and the >> TRACE_POSTGRESQL_LWLOCK methods are called using both a variable and >> constant definition. >> >> This leads to a mix of argument definitions depending on the call site, as >> seen in probes.txt file. >> >> A fix is to explicit cast 'mode' to int such that all call sites will use >> the >> >> argument #2 4 signed bytes >> >> definition. Attached patch does this. >> > > I think this fix is harmless and has some value in terms of > consistency. One minor suggestion is that you should leave a space > after typecasting. > > - TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode); > + TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), (int)mode); > > There should be a space like "(int) mode". > > v2 attached. >> I have verified all dtraces probes for their type, and only the lock__ >> methods doesn't aligned with its actual types. >> > > Do you see any problem with that? > Not really, but it would be more clear what the value space of each of the parameters were. >> >> Depending on the feedback I can add this patch to the open item list in >> order to fix it for PostgreSQL 10. >> > > Is there any commit in PG-10 which has caused this behavior? If not, > then I don't think it should be added to open items of PG-10. > > It is really a bug fix, so it could even be back patched. Thanks for the feedback ! Best regards, Jesper -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
On 04/20/2017 10:30 AM, Jesper Pedersen wrote: >> I think this fix is harmless and has some value in terms of >> consistency. One minor suggestion is that you should leave a space >> after typecasting. >> >> - TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode); >> + TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), (int)mode); >> >> There should be a space like "(int) mode". >> >> > > v2 attached. > I managed to attach the same patch again, so here is v3. Best regards, Jesper -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers