Opened 10 months ago

Closed 9 months ago

Last modified 9 months ago

#4994 closed defect (fixed)

shp2pgsql is sometimes missing the INSERT statements

Reported by: myon Owned by: robe
Priority: blocker Milestone: PostGIS 3.1.5
Component: loader/dumper Version: 2.5.x
Keywords: Cc: Bas Couwenberg, pramsey

Description

I've been chasing regression tests failures seen in 3.2 on Debian unstable: https://pgdgbuild.dus.dg-i.net/view/Binaries/job/postgis-binaries/326/architecture=amd64,distribution=sid/console The sympton is always that the diff shows some rows missing.

I managed to track it down to shp2pgsql sometimes missing the INSERT part; running it in a loop will eventually trigger the bug.

while ./loader/shp2pgsql -G  -g the_geom ./regress/loader/MultiPointZ.shp loadedshp | grep INSERT; do :; done
...
Shapefile type: MultiPointZ
Postgis type: MULTIPOINT[4]
INSERT INTO "loadedshp" (the_geom) VALUES ('01040000E0E61000000300000001010000C00000000000000000000000000000F03F0000000000000040000000000000084001010000C00000000000002240000000000000F0BF00000000000000C000000000000008C001010000C00000000000002240000000000000F0BF00000000000034C00000000000C05EC0');
Shapefile type: MultiPointZ
Postgis type: MULTIPOINT[4]
INSERT INTO "loadedshp" (the_geom) VALUES ('01040000E0E61000000300000001010000C00000000000000000000000000000F03F0000000000000040000000000000084001010000C00000000000002240000000000000F0BF00000000000000C000000000000008C001010000C00000000000002240000000000000F0BF00000000000034C00000000000C05EC0');
Shapefile type: MultiPointZ
Postgis type: MULTIPOINT[4]
  <-- no INSERT here

Change History (17)

comment:1 by robe, 10 months ago

Component: postgisloader/dumper
Milestone: PostGIS 3.1.5PostGIS 3.2.0
Owner: changed from pramsey to robe
Version: 2.5.xmaster

comment:2 by robe, 10 months ago

Milestone: PostGIS 3.2.0PostGIS 3.1.5

Myon noted he can trigger in 3.1.4 but not 3.1.3

Note from IRC:

Myon
oookay, I can trigger the bug using shp2pgsql from 3.1.4+dfsg-1.pgdg+1 on unstable
but not using 3.1.3+dfsg-1~exp1.pgdg+1+b1 on (a semi-up-to-date) bookworm

So this could be a debian unstable issue though strk recalls being able to trigger these things — but we know strk doesn't ever run bleeding edge OS.

comment:3 by myon, 10 months ago

Component: loader/dumperpostgis
Owner: changed from robe to pramsey
Version: master2.5.x

Thanks robe2 and strk on #postgis for their patience. More datapoints:

15 22:06 <Myon> oookay, I can trigger the bug using shp2pgsql from 3.1.4+dfsg-1.pgdg+1 on unstable
15 22:07 <Myon> but not using 3.1.3+dfsg-1~exp1.pgdg+1+b1 on (a semi-up-to-date) bookworm
15 22:13 <Myon> 3.1.1+dfsg-1 on bullseye seems ok
15 22:14 <Myon> 3.1.4+dfsg-1.pgdg110+1 on bullseye seems ok
15 22:18 <Myon> 3.1.4+dfsg-1.pgdg+1 on bookworm seems ok
15 22:19 <Myon> 3.2.0~alpha1+dfsg-1~exp1 from experimental installed on bookworm is also ok
15 22:19 <Myon> so it seems it might be an issue with the OS or some library on unstable
15 22:19 <Myon> ... that hasn't trickled down to testing/bookworm yet

comment:4 by robe, 9 months ago

Component: postgisloader/dumper
Owner: changed from pramsey to robe

comment:5 by robe, 9 months ago

Additional notes on #4995

comment:6 by Bas Couwenberg, 9 months ago

Cc: Bas Couwenberg added

comment:7 by strk, 9 months ago

I can reproduce on sid chroot. Valgrind says:

==38338== Conditional jump or move depends on uninitialised value(s)
==38338==    at 0x1175B6: ShpLoaderGenerateSQLRowStatement (shp2pgsql-core.c:1564)
==38338==    by 0x10BDF5: main (shp2pgsql-cli.c:397)

comment:8 by strk, 9 months ago

So if my reading is correct, the INSERT is missing when the uninitialized memory contains bits that make shp2pgsql think the record was deleted.

comment:9 by strk, 9 months ago

The valgrind report also comes up on Ubuntu 20.10

comment:10 by strk, 9 months ago

Still present, the valgrind reported error, on 64bit and with reduced switches:

$ valgrind ./loader/shp2pgsql  ./regress/loader/PointZ.shp 
...
Shapefile type: PointZ
Postgis type: POINT[4]
==49529== Conditional jump or move depends on uninitialised value(s)
==49529==    at 0x119ED1: ShpLoaderGenerateSQLRowStatement (shp2pgsql-core.c:1564)
==49529==    by 0x10E6A8: main (shp2pgsql-cli.c:397)
...

comment:11 by strk, 9 months ago

It looks like updating dbfopen.c to current master branch of https://github.com/OSGeo/shapelib fixes the issue.

comment:12 by strk, 9 months ago

Cc: pramsey added

Actually I'm wrong: using the latest dbfopen.c does fix the valgrind error but results in no records being used.

Rerting commit [595f840ebc479657adac9143cfaf890faba72f99/git] instead fixes the valgrind reported error while still passing testsuite. Paul: that commit was yours

Note that when reverting the above commit we get on stderr this printed: fread(1) failed on DBF file

comment:13 by strk, 9 months ago

Priority: mediumblocker

Paul: reverting commit [595f840ebc479657adac9143cfaf890faba72f99/git] still passes testsuite, do you remember HOW you could trigger the misbehaviour which made you push that change ? Is it not in the testsuite ?

comment:15 by Sandro Santilli <strk@…>, 9 months ago

Resolution: fixed
Status: newclosed

In 7b3f100/git:

Revert "Harmonize with old behaviour."

This reverts commit 595f840ebc479657adac9143cfaf890faba72f99.

Fixes #4994

comment:16 by myon, 9 months ago

Fwiw this needs backporting, the problem is at least also visible using shp2pgsql from 3.1.4+dfsg-1.pgdg+1 on Debian unstable.

comment:17 by Sandro Santilli <strk@…>, 9 months ago

In 7b58a46/git:

Revert "Harmonize with old behaviour."

This reverts commit 595f840ebc479657adac9143cfaf890faba72f99.

Fixes #4994 in 3.1 branch (3.1.5dev)

Note: See TracTickets for help on using tickets.