Opened 19 months ago

Closed 19 months ago

Last modified 12 months ago

#5261 closed defect (fixed)

PostgreSQL 16 GUC changes breaks compile, removal of Abs breaks compile

Reported by: robe Owned by: robe
Priority: blocker Milestone: PostGIS 3.4.0
Component: postgis Version: master
Keywords: PG16 Cc:

Description

Both berrie32 and debbie's PG16 run are showing this error:

22:06:19 make[1]: Entering directory '/var/lib/jenkins/workspace/postgis/branches/3.4/libpgcommon'
22:06:19 gcc -I./../liblwgeom -I../liblwgeom  -I/var/lib/jenkins/workspace/geos/rel-3.11w64/include   -I/usr/include/libxml2 -I/usr/include -I/usr/include/json-c  -DNDEBUG  -std=gnu99 -g -O2 -fno-math-errno -fno-signed-zeros -Wall -I/var/lib/jenkins/workspace/pg/rel/pg16w64/include/postgresql/server   -fPIC -DPIC  -c -o gserialized_gist.o gserialized_gist.c
22:06:19 gcc -I./../liblwgeom -I../liblwgeom  -I/var/lib/jenkins/workspace/geos/rel-3.11w64/include   -I/usr/include/libxml2 -I/usr/include -I/usr/include/json-c  -DNDEBUG  -std=gnu99 -g -O2 -fno-math-errno -fno-signed-zeros -Wall -I/var/lib/jenkins/workspace/pg/rel/pg16w64/include/postgresql/server   -fPIC -DPIC  -c -o lwgeom_transform.o lwgeom_transform.c
22:06:20 gcc -I./../liblwgeom -I../liblwgeom  -I/var/lib/jenkins/workspace/geos/rel-3.11w64/include   -I/usr/include/libxml2 -I/usr/include -I/usr/include/json-c  -DNDEBUG  -std=gnu99 -g -O2 -fno-math-errno -fno-signed-zeros -Wall -I/var/lib/jenkins/workspace/pg/rel/pg16w64/include/postgresql/server   -fPIC -DPIC  -c -o lwgeom_cache.o lwgeom_cache.c
22:06:20 gcc -I./../liblwgeom -I../liblwgeom  -I/var/lib/jenkins/workspace/geos/rel-3.11w64/include   -I/usr/include/libxml2 -I/usr/include -I/usr/include/json-c  -DNDEBUG  -std=gnu99 -g -O2 -fno-math-errno -fno-signed-zeros -Wall -I/var/lib/jenkins/workspace/pg/rel/pg16w64/include/postgresql/server   -fPIC -DPIC  -c -o lwgeom_pg.o lwgeom_pg.c
22:06:20 lwgeom_pg.c: In function ‘postgis_guc_find_option’:
22:06:20 lwgeom_pg.c:505:13: error: too few arguments to function ‘get_guc_variables’
22:06:20   505 |    (void *) get_guc_variables(),
22:06:20       |             ^~~~~~~~~~~~~~~~~
22:06:20 In file included from lwgeom_pg.c:30:
22:06:20 /var/lib/jenkins/workspace/pg/rel/pg16w64/include/postgresql/server/utils/guc_tables.h:296:32: note: declared here
22:06:20   296 | extern struct config_generic **get_guc_variables(int *num_vars);
22:06:20       |                                ^~~~~~~~~~~~~~~~~
22:06:20 lwgeom_pg.c:506:4: warning: implicit declaration of function ‘GetNumConfigOptions’; did you mean ‘GetConfigOption’? [-Wimplicit-function-declaration]
22:06:20   506 |    GetNumConfigOptions(),
22:06:20       |    ^~~~~~~~~~~~~~~~~~~
22:06:20       |    GetConfigOption
22:06:20 make[1]: *** [Makefile:69: lwgeom_pg.o] Error 1
22:06:20 make[1]: Leaving directory '/var/lib/jenkins/workspace/postgis/branches/3.4/libpgcommon'
22:06:20 make: *** [GNUmakefile:25: all] Error 1
22:06:21 Build step 'Execute shell' marked build as failure
22:06:21 Triggering a new build of PostgreSQL stop
22:06:21 Finished: FAILURE

Change History (9)

comment:1 by robe, 19 months ago

Owner: changed from pramsey to robe

comment:2 by robe, 19 months ago

Okay quite a few commits around gucs around 6 days ago.

I suspect this one is the culprit

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3057465acfbea2f3dd7a914a1478064022c6eecd

Replace the sorted array of GUC variables with a hash table.

This gets rid of bsearch() in favor of hashed lookup.  The main
advantage is that it becomes far cheaper to add new GUCs, since
we needn't re-sort the pointer array.  Adding N new GUCs had
been O(N^2 log N), but now it's closer to O(N).  We need to
sort only in SHOW ALL and equivalent functions, which are
hopefully not performance-critical to anybody.

Also, merge GetNumConfigOptions() into get_guc_variables(),
because in a world where the set of GUCs isn't fairly static
you really want to consider those two results as tied together
not independent.

Discussion: https://postgr.es/m/2982579.1662416866@sss.pgh.pa.us

But might have also been changed later on: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=407b50f2d421bca5b134a0033176ea8f8c68dc6b

Store GUC data in a memory context, instead of using malloc().

The only real argument for using malloc directly was that we needed
the ability to not throw error on OOM; but mcxt.c grew that feature
awhile ago.

Keeping the data in a memory context improves accountability and
debuggability --- for example, without this it's almost impossible
to detect memory leaks in the GUC code with anything less costly
than valgrind.  Moreover, the next patch in this series will add a
hash table for GUC lookup, and it'd be pretty silly to be using
palloc-dependent hash facilities alongside malloc'd storage of the
underlying data.

This is a bit invasive though, in particular causing an API break
for GUC check hooks that want to modify the GUC's value or use an
"extra" data structure.  They must now use guc_malloc() and
guc_free() instead of malloc() and free().  Failure to change
affected code will result in assertion failures or worse; but
thanks to recent effort in the mcxt infrastructure, it shouldn't
be too hard to diagnose such oversights (at least in assert-enabled
builds).

One note is that this changes ParseLongOption() to return short-lived
palloc'd not malloc'd data.  There wasn't any caller for which the
previous definition was better.

Discussion: https://postgr.es/m/2982579.1662416866@sss.pgh.pa.us

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=f13b2088fa2d4455936e65459b77698a4452f932

Add auxiliary lists to GUC data structures for better performance.

The previous patch made addition of new GUCs cheap, but other GUC
operations aren't improved and indeed get a bit slower, because
hash_seq_search() is slower than just scanning a pointer array.

However, most performance-critical GUC operations only need
to touch a relatively small fraction of the GUCs; especially
so for AtEOXact_GUC().  We can improve matters at the cost
of a bit more space by adding dlist or slist links to the
GUC data structures.  This patch invents lists that track

(1) all GUCs with non-default "source";

(2) all GUCs with nonempty state stack (implying they've
been changed in the current transaction);

(3) all GUCs due for reporting to the client.

All of guc.c's performance-critical cases can make use of one or
another of these lists to avoid searching the whole hash table.
In particular, the stack list means that transaction end
doesn't take time proportional to the number of GUCs, but
only to the number changed in the current transaction.

Discussion: https://postgr.es/m/2982579.1662416866@sss.pgh.pa.us

comment:3 by robe, 19 months ago

Summary: PostgreSQL 16 GUC changes breaks compilePostgreSQL 16 GUC changes breaks compile, removal of Abs breaks compile

Okay I think this one I have under control maybe, but that just allowed stumbling into yet another break

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=a699b7a7aa9f34c19aa7632b3f037f24c8ee7720

Remove Abs()
author	Peter Eisentraut <peter@eisentraut.org>	
	Wed, 12 Oct 2022 04:36:12 +0000 (06:36 +0200)
committer	Peter Eisentraut <peter@eisentraut.org>	
	Wed, 12 Oct 2022 04:53:47 +0000 (06:53 +0200)
commit	a699b7a7aa9f34c19aa7632b3f037f24c8ee7720
tree	f1b387b4cddc0e2857cb4ed80f98ab6bb31096e5	tree
parent	4574eb9d38c69f4c90b5468f740ff22519752066	commit | diff
Remove Abs()

All callers have been replaced by standard C library functions.

Reviewed-by: Zhang Mingli <zmlpostgres@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/4beb42b5-216b-bce8-d452-d924d5794c63%40enterprisedb.com

Which is causing a break in

gserialized_gist_2d.o: In function `gserialized_gist_picksplit_2d':
C:\ming64gcc81\projects\postgis\postgis-git\postgis/gserialized_gist_2d.c:2057: undefined reference to `Abs'
collect2.exe: error: ld returned 1 exit status
make[1]: *** [C:/MING64~1/projects/POSTGR~1/rel/PG16W6~3/lib/pgxs/src/MAKEFI~1/../../src/Makefile.shlib:339: postgis-3.4.dll] Error 1
make[1]: Leaving directory '/projects/postgis/postgis-git/postgis'

I'm thinking we should just replace this one with abs — can't think of a reason to keep it.

comment:4 by Regina Obe <lr@…>, 19 months ago

Resolution: fixed
Status: newclosed

In 1e96c92/git:

FIX compile against PostgreSQL 16
1) get_guc_variables() function now requires number variables reference.
Change to not use this function at all and replace with find_option,
which does much the same but with less code

2) Abs function removed in PG16. As noted in Discussion:
https://www.postgresql.org/message-id/flat/4beb42b5-216b-bce8-d452-d924d5794c63%40enterprisedb.com
We should be using C lib abs or fabs. Replaced this left over use with
fabs, which we are already using elsewhere

Closes #5261 for PostGIS 3.4.0

comment:5 by Regina Obe <lr@…>, 19 months ago

In 87ad535/git:

Fix typo in last commit. References #5261 for PostGIS 3.4.0

comment:6 by Regina Obe <lr@…>, 19 months ago

In 7f54d8c/git:

Get rid of unused variable accidentally added. References #5261 for PostGIS 3.4.0

comment:7 by Regina Obe <lr@…>, 18 months ago

In 0b49bc5/git:

Fix error: assignment to 'struct config_generic ' from incompatible pointer type 'struct config_generic *'
References #5261 for PostGIS 3.4.0

comment:8 by Regina Obe <lr@…>, 18 months ago

In bcbbaff/git:

Fix 'const char *' but argument is of type 'const char *' for PG16
References #5261 for PostGIS 3.4.0

comment:9 by Regina Obe <lr@…>, 12 months ago

In 76f44de/git:

Support build on Pg16devel for PostGIS 3.3.3

Closes #5257
Closes #5261
Closes #5277
Closes #5308
Closes #5374
Closes https://github.com/postgis/postgis/pull/716

Note: See TracTickets for help on using tickets.