Обсуждение: s_lock_test no longer works

I just discovered that doing "make -C src/backend/storage/lmgr check" no
longer works, because commit 92daeca45df6 ("Add wait event for
pg_usleep() in perform_spin_delay()") added a requirement for
my_wait_event_info to be present at link time:

$ LC_ALL=C make -C src/backend/storage/lmgr/ s_lock_test
make: Entering directory '/home/alvherre/Code/pgsql-build/master/src/backend/storage/lmgr'
gcc -I. -I../../../../src/include -I/pgsql/source/master/src/include  -D_GNU_SOURCE  -Wall -Wmissing-prototypes
-Wpointer-arith-Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute
-Wimplicit-fallthrough=3-Wcast-function-type -Wshadow=compatible-local -Wformat-security -fno-strict-aliasing -fwrapv
-fexcess-precision=standard-Wno-format-truncation -Wno-stringop-truncation -g -O2 -DS_LOCK_TEST=1
/pgsql/source/master/src/backend/storage/lmgr/s_lock.c\
 
     -L ../../../../src/common -lpgcommon \
    -L ../../../../src/port -lpgport -lm -o s_lock_test
/usr/bin/ld: /run/user/1000/alvherre-tmp/ccMaAvVj.o: warning: relocation against `my_wait_event_info' in read-only
section`.text'
 
/usr/bin/ld: /run/user/1000/alvherre-tmp/ccMaAvVj.o: in function `pgstat_report_wait_start':
/pgsql/source/master/src/include/utils/wait_event.h:94: undefined reference to `my_wait_event_info'
/usr/bin/ld: /run/user/1000/alvherre-tmp/ccMaAvVj.o: in function `pgstat_report_wait_end':
/pgsql/source/master/src/include/utils/wait_event.h:107: undefined reference to `my_wait_event_info'
/usr/bin/ld: warning: creating DT_TEXTREL in a PIE
collect2: error: ld returned 1 exit status
make: *** [Makefile:35: s_lock_test] Error 1
make: Leaving directory '/home/alvherre/Code/pgsql-build/master/src/backend/storage/lmgr'


This is after I added -lm, to fix these other problems:

/home/alvherre/Code/pgsql-build/master/src/common/../../../../../../../pgsql/source/master/src/common/pg_prng.c:269:
undefinedreference to `log'
 
/usr/bin/ld:
/home/alvherre/Code/pgsql-build/master/src/common/../../../../../../../pgsql/source/master/src/common/pg_prng.c:269:
undefinedreference to `sin'
 
/usr/bin/ld:
/home/alvherre/Code/pgsql-build/master/src/common/../../../../../../../pgsql/source/master/src/common/pg_prng.c:269:
undefinedreference to `sqrt'
 


On my machine, it's enough to patch s_lock_test.c to have a local definition
for the missing symbol.  Since the file already has a test mode, it
turns out to be quite simple -- attached.


I do wonder if we want to keep this around, given that it's been more
than one year broken and nobody seems to have noticed, and the Meson
build does not support the test as a target.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
 Are you not unsure you want to delete Firefox?
       [Not unsure]     [Not not unsure]    [Cancel]
                   http://smylers.hates-software.com/2008/01/03/566e45b2.html

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I do wonder if we want to keep this around, given that it's been more
> than one year broken and nobody seems to have noticed, and the Meson
> build does not support the test as a target.

The last time it was broken, it took us multiple years to notice, too.
I'm not sure that that's a reason to remove the test scaffolding,
though.  You'd probably only really use it to smoke-test some new
spinlock assembly code, and how often does anyone do that anymore?

            regards, tom lane

Hi,

On 2024-01-24 12:14:17 +0100, Alvaro Herrera wrote:
> I do wonder if we want to keep this around, given that it's been more
> than one year broken and nobody seems to have noticed, and the Meson
> build does not support the test as a target.

Perhaps we should just make the test built and run by default instead?  OTOH,
regress.c:test_spinlock() actually covers about as much as the standalone
test...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2024-01-24 12:14:17 +0100, Alvaro Herrera wrote:
>> I do wonder if we want to keep this around, given that it's been more
>> than one year broken and nobody seems to have noticed, and the Meson
>> build does not support the test as a target.

> Perhaps we should just make the test built and run by default instead?  OTOH,
> regress.c:test_spinlock() actually covers about as much as the standalone
> test...

If your spinlocks aren't working, it's unlikely you'll get as far as
being able to run test_spinlock().  I think the standalone test does
have some value; it's just that it's not needed very often these days.

            regards, tom lane

Hi,

On 2024-01-24 15:05:12 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2024-01-24 12:14:17 +0100, Alvaro Herrera wrote:
> >> I do wonder if we want to keep this around, given that it's been more
> >> than one year broken and nobody seems to have noticed, and the Meson
> >> build does not support the test as a target.
> 
> > Perhaps we should just make the test built and run by default instead?  OTOH,
> > regress.c:test_spinlock() actually covers about as much as the standalone
> > test...
> 
> If your spinlocks aren't working, it's unlikely you'll get as far as
> being able to run test_spinlock().  I think the standalone test does
> have some value; it's just that it's not needed very often these days.

As long as the uncontended case works, you can get surprisingly far... But
still, fair enough. If so, I think we should just rig things so the standalone
test gets built and run by default. It's not like that'd be a measurably
expensive thing to do.

Greetings,

Andres Freund