Re: RFC: replace pg_stat_activity.waiting with something more descriptive

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Ildus Kurbangaliev <i(dot)kurbangaliev(at)postgrespro(dot)ru>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Alexander Korotkov <aekorotkov(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>
Subject: Re: RFC: replace pg_stat_activity.waiting with something more descriptive
Date: 2015-08-05 06:53:13
Message-ID: CAA4eK1KdeC1Tm5ya9gkV85Vtn4qqsRxzKJrU-tu70G_tL1xkFA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Tue, Aug 4, 2015 at 5:45 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Tue, Jul 28, 2015 at 3:28 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
wrote:
> > * The patch requires that the LWLOCK_INDIVIDUAL_NAMES array is kept in
sync
> > with the list of individual locks in lwlock.h. Sooner or later someone
will
> > add an LWLock and forget to update the names-array. That needs to be
made
> > less error-prone, so that the names are maintained in the same place as
the
> > #defines. Perhaps something like rmgrlist.h.
>
> This is a good idea, but it's not easy to do in the style of
> rmgrlist.h, because I don't believe there's any way to define a macro
> that expands to a preprocessor directive. Attached is a patch that
> instead generates the list of macros from a text file, and also
> generates an array inside lwlock.c with the lock names that gets used
> by the Trace_lwlocks stuff where applicable.
>
> Any objections to this solution to the problem? If not, I'd like to
> go ahead and push this much. I can't test the Windows changes
> locally, though, so it would be helpful if someone could check that
> out.
>

Okay, I have check it on Windows and found below issues which I
have fixed in patch attached with this mail.

1. Got below error while running mkvcbuild.pl

Use of uninitialized value in numeric lt (<) at Solution.pm line 103.
Could not open src/backend/storage/lmgr/lwlocknames.h at Mkvcbuild.pm line
674.

+ if (IsNewer(
+ 'src/backend/storage/lmgr/lwlocknames.txt',
'src/include/storage/lwlocknames.h'))

I think the usage of old and new filenames in IsNewer is reversed here.

2.
+ print "Generating lwlocknames.c and fmgroids.h...\n";

Here it should be lwlocknames.h instead of fmgroids.h

3.
+ if (IsNewer(
+ 'src/include/storage/lwlocknames.h',
+
'src/backend/storage/lmgr/fmgroids.h'))

Here, it seems lwlocknames.h needs to compared instead of fmgroids.h

4. Got below error while running mkvcbuild.pl
Generating lwlocknames.c and fmgroids.h...
/* autogenerated from src/backend/storage/lmgr/lwlocknames.txt, do not edit
*/
/* there is deliberately not an #ifndef LWLOCKNAMES_H here */
rename: lwlocknames.h.tmp3180: Permission denied at generate-lwlocknames.pl
line
59, <$lwlocknames> line 47.

In generate-lwlocknames.pl, below line is causing problem.

rename($htmp, 'lwlocknames.h') || die "rename: $htmp: $!";

The reason is that closing of tmp files is missing.

Some other general observations

5.
On running perl mkvcbuild.pl in Windows, below message is generated.

Generating lwlocknames.c and lwlocknames.h...
/* autogenerated from src/backend/storage/lmgr/lwlocknames.txt, do not edit
*/
/* there is deliberately not an #ifndef LWLOCKNAMES_H here */

Following message gets displayed, which looks slightly odd, displaying
it in C comments makes it look out of place and other similar .pl files
don't generate such message. Having said that, I think it displays useful
information, so it might be okay to retain it.

6.
+
+maintainer-clean: clean
+ rm -f lwlocknames.h

Here, don't we need to clean lwlocknames.c?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Attachment Content-Type Size
lwlocknames-v2.patch application/octet-stream 14.1 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Ashutosh Bapat 2015-08-05 07:33:25 Re: Dependency between bgw_notify_pid and bgw_flags
Previous Message Piotr Stefaniak 2015-08-05 05:20:48 Re: [sqlsmith] Failed assertion in joinrels.c