longfin and tamandua aren't too happy but I'm not sure why

On Tue, Sep 27, 2022 at 02:55:18PM -0400, Robert Haas wrote:

Both animals are running with -fsanitize=alignment and it's not
difficult to believe that the commit mentioned above could have
introduced an alignment problem where we didn't have one before, but
without a stack backtrace I don't know how to track it down. I tried
running those tests locally with -fsanitize=alignment and they passed.

There's one here:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=kestrel&dt=2022-09-27%2018%3A43%3A06

/mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/access/rmgrdesc/xactdesc.c:102:30: runtime error: member access within misaligned address 0x000004125074 for type 'xl_xact_invals' (aka 'struct xl_xact_invals'), which requires 8 byte alignment

#0 0x5b6702 in ParseCommitRecord /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/access/rmgrdesc/xactdesc.c:102:30
#1 0xb5264d in xact_decode /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/replication/logical/decode.c:201:5
#2 0xb521ac in LogicalDecodingProcessRecord /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/replication/logical/decode.c:119:3
#3 0xb5e868 in pg_logical_slot_get_changes_guts /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/replication/logical/logicalfuncs.c:271:5
#4 0xb5e25f in pg_logical_slot_get_changes /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/replication/logical/logicalfuncs.c:338:9
#5 0x896bba in ExecMakeTableFunctionResult /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/executor/execSRF.c:234:13
#6 0x8c7660 in FunctionNext /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/executor/nodeFunctionscan.c:95:5
#7 0x899048 in ExecScanFetch /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/executor/execScan.c:133:9
#8 0x89896b in ExecScan /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/executor/execScan.c:199:10
#9 0x8c6892 in ExecFunctionScan /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/executor/nodeFunctionscan.c:270:9
#10 0x892f42 in ExecProcNodeFirst /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/executor/execProcnode.c:464:9
#11 0x8802dd in ExecProcNode /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/include/executor/executor.h:259:9
#12 0x8802dd in ExecutePlan /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/executor/execMain.c:1636:10
#13 0x8802dd in standard_ExecutorRun /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/executor/execMain.c:363:3
#14 0x87ffbb in ExecutorRun /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/executor/execMain.c:307:3
#15 0xc36c07 in PortalRunSelect /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/tcop/pquery.c:924:4
#16 0xc364ca in PortalRun /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/tcop/pquery.c:768:18
#17 0xc34138 in exec_simple_query /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/tcop/postgres.c:1238:10
#18 0xc30953 in PostgresMain /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/tcop/postgres.c
#19 0xb27e3f in BackendRun /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:4482:2
#20 0xb2738d in BackendStartup /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:4210:3
#21 0xb2738d in ServerLoop /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:1804:7
#22 0xb24312 in PostmasterMain /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:1476:11
#23 0x953694 in main /mnt/resource/bf/build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/main/main.c:197:3
#24 0x7f834e39a209 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#25 0x7f834e39a2bb in __libc_start_main csu/../csu/libc-start.c:389:3
#26 0x4a40a0 in _start (/mnt/resource/bf/build/kestrel/HEAD/pgsql.build/tmp_install/mnt/resource/bf/build/kestrel/HEAD/inst/bin/postgres+0x4a40a0)

Note that cfbot is warning for a different reason now:
https://cirrus-ci.com/task/5794615155490816

Justin Pryzby <pryzby@telsasoft.com> writes:

On Tue, Sep 27, 2022 at 02:55:18PM -0400, Robert Haas wrote:

Both animals are running with -fsanitize=alignment and it's not
difficult to believe that the commit mentioned above could have
introduced an alignment problem where we didn't have one before, but
without a stack backtrace I don't know how to track it down. I tried
running those tests locally with -fsanitize=alignment and they passed.

There's one here:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=kestrel&amp;dt=2022-09-27%2018%3A43%3A06

On longfin's host, the test_decoding run produces two core files.
One has a backtrace like this:

* frame #0: 0x000000010a36af8c postgres`ParseCommitRecord(info='\x80', xlrec=0x00007fa0678a8090, parsed=0x00007ff7b5c50e78) at xactdesc.c:102:30
frame #1: 0x000000010a765f9e postgres`xact_decode(ctx=0x00007fa0680d9118, buf=0x00007ff7b5c51000) at decode.c:201:5 [opt]
frame #2: 0x000000010a765d17 postgres`LogicalDecodingProcessRecord(ctx=0x00007fa0680d9118, record=<unavailable>) at decode.c:119:3 [opt]
frame #3: 0x000000010a76d890 postgres`pg_logical_slot_get_changes_guts(fcinfo=<unavailable>, confirm=true, binary=false) at logicalfuncs.c:271:5 [opt]
frame #4: 0x000000010a76d320 postgres`pg_logical_slot_get_changes(fcinfo=<unavailable>) at logicalfuncs.c:338:9 [opt]
frame #5: 0x000000010a5a521d postgres`ExecMakeTableFunctionResult(setexpr=<unavailable>, econtext=0x00007fa068098f50, argContext=<unavailable>, expectedDesc=0x00007fa06701ba38, randomAccess=<unavailable>) at execSRF.c:234:13 [opt]
frame #6: 0x000000010a5c405b postgres`FunctionNext(node=0x00007fa068098d40) at nodeFunctionscan.c:95:5 [opt]
frame #7: 0x000000010a5a61b9 postgres`ExecScan(node=0x00007fa068098d40, accessMtd=(postgres`FunctionNext at nodeFunctionscan.c:61), recheckMtd=(postgres`FunctionRecheck at nodeFunctionscan.c:251)) at execScan.c:199:10 [opt]
frame #8: 0x000000010a596ee0 postgres`standard_ExecutorRun [inlined] ExecProcNode(node=0x00007fa068098d40) at executor.h:259:9 [opt]
frame #9: 0x000000010a596eb8 postgres`standard_ExecutorRun [inlined] ExecutePlan(estate=<unavailable>, planstate=0x00007fa068098d40, use_parallel_mode=<unavailable>, operation=CMD_SELECT, sendTuples=<unavailable>, numberTuples=0, direction=1745456112, dest=0x00007fa067023848, execute_once=<unavailable>) at execMain.c:1636:10 [opt]
frame #10: 0x000000010a596e2a postgres`standard_ExecutorRun(queryDesc=<unavailable>, direction=1745456112, count=0, execute_once=<unavailable>) at execMain.c:363:3 [opt]

and the other

* frame #0: 0x000000010a36af8c postgres`ParseCommitRecord(info='\x80', xlrec=0x00007fa06783a090, parsed=0x00007ff7b5c50040) at xactdesc.c:102:30
frame #1: 0x000000010a3cd24d postgres`xact_redo(record=0x00007fa0670096c8) at xact.c:6161:3
frame #2: 0x000000010a41770d postgres`ApplyWalRecord(xlogreader=0x00007fa0670096c8, record=0x00007fa06783a060, replayTLI=0x00007ff7b5c507f0) at xlogrecovery.c:1897:2
frame #3: 0x000000010a4154be postgres`PerformWalRecovery at xlogrecovery.c:1728:4
frame #4: 0x000000010a3e0dc7 postgres`StartupXLOG at xlog.c:5473:3
frame #5: 0x000000010a7498a0 postgres`StartupProcessMain at startup.c:267:2 [opt]
frame #6: 0x000000010a73e2cb postgres`AuxiliaryProcessMain(auxtype=StartupProcess) at auxprocess.c:141:4 [opt]
frame #7: 0x000000010a745b97 postgres`StartChildProcess(type=StartupProcess) at postmaster.c:5408:3 [opt]
frame #8: 0x000000010a7487e2 postgres`PostmasterStateMachine at postmaster.c:4006:16 [opt]
frame #9: 0x000000010a745804 postgres`reaper(postgres_signal_arg=<unavailable>) at postmaster.c:3256:2 [opt]
frame #10: 0x00007ff815b16dfd libsystem_platform.dylib`_sigtramp + 29
frame #11: 0x00007ff815accd5b libsystem_kernel.dylib`__select + 11
frame #12: 0x000000010a74689c postgres`ServerLoop at postmaster.c:1768:13 [opt]
frame #13: 0x000000010a743fbb postgres`PostmasterMain(argc=<unavailable>, argv=0x00006000006480a0) at postmaster.c:1476:11 [opt]
frame #14: 0x000000010a61c775 postgres`main(argc=8, argv=<unavailable>) at main.c:197:3 [opt]

Looks like it might be the same bug, but perhaps not.

I recompiled access/transam and access/rmgrdesc at -O0 to get the accurate
line numbers shown for those files. Let me know if you need any more
info; I can add -O0 in more places, or poke around in the cores.

regards, tom lane

I wrote:

* frame #0: 0x000000010a36af8c postgres`ParseCommitRecord(info='\x80', xlrec=0x00007fa0678a8090, parsed=0x00007ff7b5c50e78) at xactdesc.c:102:30

Okay, so the problem is this: by widening RelFileNumber to 64 bits,
you have increased the alignment requirement of struct RelFileLocator,
and thereby also SharedInvalidationMessage, to 8 bytes where it had
been 4. longfin's alignment check is therefore expecting that
xl_xact_twophase will likewise be 8-byte-aligned, but it isn't:

(lldb) p data
(char *) $0 = 0x00007fa06783a0a4 "\U00000001"

I'm not sure whether the code that generates commit WAL records is
breaking a contract it should maintain, or xactdesc.c needs to be
taught to not assume that this data is adequately aligned.

There is a second problem that I am going to hold your feet to the
fire about:

(lldb) p sizeof(SharedInvalidationMessage)
(unsigned long) $1 = 24

We have sweated a good deal for a long time to keep that struct
to 16 bytes. I do not think 50% bloat is acceptable.

regards, tom lane

On Tue, Sep 27, 2022 at 4:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

* frame #0: 0x000000010a36af8c postgres`ParseCommitRecord(info='\x80', xlrec=0x00007fa0678a8090, parsed=0x00007ff7b5c50e78) at xactdesc.c:102:30

Okay, so the problem is this: by widening RelFileNumber to 64 bits,
you have increased the alignment requirement of struct RelFileLocator,
and thereby also SharedInvalidationMessage, to 8 bytes where it had
been 4. longfin's alignment check is therefore expecting that
xl_xact_twophase will likewise be 8-byte-aligned, but it isn't:

Yeah, I reached the same conclusion.

There is a second problem that I am going to hold your feet to the
fire about:

(lldb) p sizeof(SharedInvalidationMessage)
(unsigned long) $1 = 24

We have sweated a good deal for a long time to keep that struct
to 16 bytes. I do not think 50% bloat is acceptable.

I noticed that problem, too.

The attached patch, which perhaps you can try out, fixes the alignment
issue and also reduces the size of SharedInvalidationMessage from 24
bytes back to 20 bytes. I do not really see a way to do better than
that. We use 1 type byte, 3 bytes for the backend ID, 4 bytes for the
database OID, and 4 bytes for the tablespace OID. Previously, we then
used 4 bytes for the relfilenode, but now we need 7, and there's no
place from which we can plausibly steal those bits, at least not as
far as I can see. If you have ideas, I'm all ears.

Also, I don't really know what problem you think it's going to cause
if that structure gets bigger. If we increased the size from 16 bytes
even all the way to 32 or 64 bytes, what negative impact do you think
that would have? It would use a little bit more shared memory, but on
modern systems I doubt it would be enough to get excited about. The
bigger impact would probably be that it would make commit records a
bit bigger since those carry invalidations as payload. That is not
great, but I think it only affects commit records for transactions
that do DDL, so I'm struggling to see that as a big performance
problem.

--
Robert Haas
EDB: http://www.enterprisedb.com

... also, lapwing's not too happy [1]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&amp;dt=2022-09-27%2018%3A40%3A18. The alter_table test
expects this to yield zero rows, but it doesn't:

SELECT m.* FROM filenode_mapping m LEFT JOIN pg_class c ON c.oid = m.oid
WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;

I've reproduced that symptom in a 32-bit FreeBSD VM building with clang,
so I suspect that it'll occur on any 32-bit build. mamba is a couple
hours away from offering a confirmatory data point, though.

(BTW, is that test case sane at all? I'm bemused by the symmetrical
NOT NULL tests on a fundamentally not-symmetrical left join; what
are those supposed to accomplish? Also, the fact that it doesn't
deign to show any fields from "c" is sure making it hard to tell
what's wrong.)

regards, tom lane

[1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&amp;dt=2022-09-27%2018%3A40%3A18

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Sep 27, 2022 at 4:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

There is a second problem that I am going to hold your feet to the
fire about:
(lldb) p sizeof(SharedInvalidationMessage)
(unsigned long) $1 = 24

Also, I don't really know what problem you think it's going to cause
if that structure gets bigger. If we increased the size from 16 bytes
even all the way to 32 or 64 bytes, what negative impact do you think
that would have?

Maybe it wouldn't have any great impact. I don't know, but I don't
think it's incumbent on me to measure that. You or the patch author
should have had a handle on that question *before* committing.

regards, tom lane

On Wed, Sep 28, 2022 at 2:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

... also, lapwing's not too happy [1]. The alter_table test
expects this to yield zero rows, but it doesn't:

By looking at regression diff as shown below, it seems that we are
able to get the relfilenode from the Oid using
pg_relation_filenode(oid) but the reverse mapping
pg_filenode_relation(reltablespace, relfilenode) returned NULL.

I am not sure but by looking at the code it is somehow related to
alignment padding while computing the hash key size in the 32-bit
machine in the function InitializeRelfilenumberMap(). I am still
looking into this and will provide updates on this.

+  oid  | mapped_oid | reltablespace | relfilenode |
 relname
+-------+------------+---------------+-------------+------------------------------------------------
+ 16385 |            |             0 |      100000 | char_tbl
+ 16388 |            |             0 |      100001 | float8_tbl
+ 16391 |            |             0 |      100002 | int2_tbl
+ 16394 |            |             0 |      100003 | int4_tbl

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

wrasse is also failing with a bus error, but I cannot get the stack
trace. So it seems it is hitting some alignment issues during startup
[1]: 2022-09-28 03:19:26.228 CEST [180:4] LOG: redo starts at 0/30FE9D8 2022-09-28 03:19:27.674 CEST [177:3] LOG: startup process (PID 180) was terminated by signal 10: Bus Error 2022-09-28 03:19:27.674 CEST [177:4] LOG: terminating any other active server processes 2022-09-28 03:19:27.677 CEST [177:5] LOG: shutting down due to startup process failure 2022-09-28 03:19:27.681 CEST [177:6] LOG: database system is shut down

[1]: 2022-09-28 03:19:26.228 CEST [180:4] LOG: redo starts at 0/30FE9D8 2022-09-28 03:19:27.674 CEST [177:3] LOG: startup process (PID 180) was terminated by signal 10: Bus Error 2022-09-28 03:19:27.674 CEST [177:4] LOG: terminating any other active server processes 2022-09-28 03:19:27.677 CEST [177:5] LOG: shutting down due to startup process failure 2022-09-28 03:19:27.681 CEST [177:6] LOG: database system is shut down
2022-09-28 03:19:26.228 CEST [180:4] LOG: redo starts at 0/30FE9D8
2022-09-28 03:19:27.674 CEST [177:3] LOG: startup process (PID 180)
was terminated by signal 10: Bus Error
2022-09-28 03:19:27.674 CEST [177:4] LOG: terminating any other
active server processes
2022-09-28 03:19:27.677 CEST [177:5] LOG: shutting down due to
startup process failure
2022-09-28 03:19:27.681 CEST [177:6] LOG: database system is shut down

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Dilip Kumar <dilipbalaut@gmail.com> writes:

wrasse is also failing with a bus error,

Yeah. At this point I think it's time to call for this patch
to get reverted. It should get tested *off line* on some
non-Intel, non-64-bit, alignment-picky architectures before
the rest of us have to deal with it any more.

There may be a larger conversation to be had here about how
much our CI infrastructure should be detecting. There seems
to be a depressingly large gap between what that found and
what the buildfarm is finding --- not only in portability
issues, but in things like cpluspluscheck failures, which
I had supposed CI would find.

regards, tom lane

On Wed, Sep 28, 2022 at 6:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

There may be a larger conversation to be had here about how
much our CI infrastructure should be detecting. There seems
to be a depressingly large gap between what that found and
what the buildfarm is finding --- not only in portability
issues, but in things like cpluspluscheck failures, which
I had supposed CI would find.

+1, Andres had some sanitizer flags in the works (stopped by a weird
problem to be resolved first), and 32 bit CI would clearly be good.
It also seems that ARM is now available to us via CI (either Amazon's
or a Mac), which IIRC is more SIGBUS-y about alignment than x86?

FTR CI reported that cpluspluscheck failure and more[1]https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3711, so perhaps we
just need to get clearer agreement on the status of CI, ie a policy
that CI had better be passing before you get to the next stage. It's
still pretty new...

[1]: https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3711

On Wed, Sep 28, 2022 at 10:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Dilip Kumar <dilipbalaut@gmail.com> writes:

wrasse is also failing with a bus error,

Yeah. At this point I think it's time to call for this patch
to get reverted. It should get tested *off line* on some
non-Intel, non-64-bit, alignment-picky architectures before
the rest of us have to deal with it any more.

There may be a larger conversation to be had here about how
much our CI infrastructure should be detecting. There seems
to be a depressingly large gap between what that found and
what the buildfarm is finding --- not only in portability
issues, but in things like cpluspluscheck failures, which
I had supposed CI would find.

Okay.

Btw, I think the reason for the bus error on wrasse is the same as
what is creating failure on longfin[1], I mean this unaligned access
is causing Bus error during startup, IMHO.

frame #0: 0x000000010a36af8c postgres`ParseCommitRecord(info='\x80',
xlrec=0x00007fa06783a090, parsed=0x00007ff7b5c50040) at
xactdesc.c:102:30
frame #1: 0x000000010a3cd24d
postgres`xact_redo(record=0x00007fa0670096c8) at xact.c:6161:3
frame #2: 0x000000010a41770d
postgres`ApplyWalRecord(xlogreader=0x00007fa0670096c8,
record=0x00007fa06783a060, replayTLI=0x00007ff7b5c507f0) at
xlogrecovery.c:1897:2

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Dilip Kumar <dilipbalaut@gmail.com> writes:

Btw, I think the reason for the bus error on wrasse is the same as
what is creating failure on longfin[1], I mean this unaligned access
is causing Bus error during startup, IMHO.

Maybe, but there's not a lot of evidence for that. wrasse got
through the test_decoding check where longfin, tamandua, kestrel,
and now skink are failing. It's evidently not the same issue
that the 32-bit animals are choking on, either. Looks like yet
a third bug to me.

regards, tom lane

On Wed, Sep 28, 2022 at 9:40 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Wed, Sep 28, 2022 at 2:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

... also, lapwing's not too happy [1]. The alter_table test
expects this to yield zero rows, but it doesn't:

By looking at regression diff as shown below, it seems that we are
able to get the relfilenode from the Oid using
pg_relation_filenode(oid) but the reverse mapping
pg_filenode_relation(reltablespace, relfilenode) returned NULL.

It was a silly mistake, I used the F_OIDEQ function instead of
F_INT8EQ. Although this was correct on the 0003 patch where we have
removed the tablespace from key, but got missed in this :(

I have locally reproduced this in a 32 bit machine consistently and
the attached patch is fixing the issue for me.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Wed, Sep 28, 2022 at 9:26 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

It was a silly mistake, I used the F_OIDEQ function instead of
F_INT8EQ. Although this was correct on the 0003 patch where we have
removed the tablespace from key, but got missed in this :(

I have locally reproduced this in a 32 bit machine consistently and
the attached patch is fixing the issue for me.

I tested this with an armhf (32 bit) toolchain, and it passes
check-world, and was failing before.

Robert's patch isn't needed on this system. I didn't look into this
subject for long but it seems that SIGBUS on misaligned access (as
typically seen on eg SPARC) requires a 32 bit Linux/ARM kernel, but I
was testing with 32 bit processes and a 64 bit kernel. Apparently 32
bit Linux/ARM has a control /proc/cpu/alignment to select behaviour
(options include emulation, SIGBUS) but 64 bit kernels don't have it
and are happy with misaligned access.

On Wed, Sep 28, 2022 at 11:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Dilip Kumar <dilipbalaut@gmail.com> writes:

Btw, I think the reason for the bus error on wrasse is the same as
what is creating failure on longfin[1], I mean this unaligned access
is causing Bus error during startup, IMHO.

Maybe, but there's not a lot of evidence for that. wrasse got
through the test_decoding check where longfin, tamandua, kestrel,
and now skink are failing. It's evidently not the same issue
that the 32-bit animals are choking on, either. Looks like yet
a third bug to me.

I think the reason is that "longfin" is configured with the
-fsanitize=alignment option so it will report the failure for any
unaligned access. Whereas "wrasse" actually generates the "Bus error"
due to architecture. So the difference is that with
-fsanitize=alignment, it will always complain for any unaligned access
but all unaligned access will not end up in the "Bus error", and I
think that could be the reason "wrasse" is not failing in the test
decoding.

Yeah but anyway this is just a theory behind why failing at different
places but we still do not have evidence/call stack to prove that.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Wed, Sep 28, 2022 at 1:48 AM Thomas Munro <thomas.munro@gmail.com> wrote:

FTR CI reported that cpluspluscheck failure and more[1], so perhaps we
just need to get clearer agreement on the status of CI, ie a policy
that CI had better be passing before you get to the next stage. It's
still pretty new...

Yeah, I suppose I have to get in the habit of looking at CI before
committing anything. It's sort of annoying to me, though. Here's a
list of the follow-up fixes I've so far committed:

1. headerscheck
2. typos
3. pg_buffercache's meson.build
4. compiler warning
5. alignment problem
6. F_INTEQ/F_OIDEQ problem

CI caught (1), (3), and (4). The buildfarm caught (1), (5), and (6).
The number of buildfarm failures that I would have avoided by checking
CI is less than the number of extra things I had to fix to keep CI
happy, and the serious problems were caught by the buildfarm, not by
CI. It's not even clear to me how I was supposed to know that every
future Makefile change is going to require adjusting a meson.build
file as well. It's not like that was mentioned in the commit message
for the meson build system, which also has no README anywhere in the
source tree. I found the wiki page by looking up the commit and
finding the URL in the commit message, but, you know, that wiki page
ALSO doesn't mention the need to now update meson.build files going
forward. So I guess the way you're supposed to know that you need to
update meson.build that is by looking at CI, but CI is also the only
reason it's necessary to carry about meson.build in the first place. I
feel like CI has not really made it in any easier to not break the
buildfarm -- it's just provided a second buildfarm that you can break
independently of the first one.

And like the existing buildfarm, it's severely under-documented.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Sep 28, 2022 at 1:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Dilip Kumar <dilipbalaut@gmail.com> writes:

wrasse is also failing with a bus error,

Yeah. At this point I think it's time to call for this patch
to get reverted. It should get tested *off line* on some
non-Intel, non-64-bit, alignment-picky architectures before
the rest of us have to deal with it any more.

I don't really understand how you expect me or Dilip to do this. Is
every PostgreSQL hacker supposed to have a bunch of antiquated servers
in their basement so that they can test this stuff? I don't think I
have had easy access to non-Intel, non-64-bit, alignment-picky
hardware in probably 25 years, unless my old Raspberry Pi counts.

I admit that I should have checked the CI results before pushing this
commit, but as you say yourself, that missed a bunch of stuff, and I'd
say it was the important stuff. Unless and until CI is able to check
all the same configurations that the buildfarm can check, it's not
going to be possible to get test results on some of these platforms
except by checking the code in and seeing what happens. If I revert
this, I'm just going to be sitting here not knowing where any of the
problems are and having no way to find them.

Maybe I'm missing something here. Apart from visual inspection of the
code and missing fewer mistakes than I did, how would you have avoided
these problems in one of your commits?

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Sep 27, 2022 at 5:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

... also, lapwing's not too happy [1]. The alter_table test
expects this to yield zero rows, but it doesn't:

SELECT m.* FROM filenode_mapping m LEFT JOIN pg_class c ON c.oid = m.oid
WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;

I've reproduced that symptom in a 32-bit FreeBSD VM building with clang,
so I suspect that it'll occur on any 32-bit build. mamba is a couple
hours away from offering a confirmatory data point, though.

(BTW, is that test case sane at all? I'm bemused by the symmetrical
NOT NULL tests on a fundamentally not-symmetrical left join; what
are those supposed to accomplish? Also, the fact that it doesn't
deign to show any fields from "c" is sure making it hard to tell
what's wrong.)

This was added by:

commit f3fdd257a430ff581090740570af9f266bb893e3
Author: Noah Misch <noah@leadboat.com>
Date: Fri Jun 13 19:57:59 2014 -0400

Harden pg_filenode_relation test against concurrent DROP TABLE.

Per buildfarm member prairiedog. Back-patch to 9.4, where the test was
introduced.

Reviewed by Tom Lane.

There seems to be a comment in that commit which explains the intent
of those funny-looking NULL tests.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Sep 27, 2022 at 5:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Maybe it wouldn't have any great impact. I don't know, but I don't
think it's incumbent on me to measure that. You or the patch author
should have had a handle on that question *before* committing.

I agree. I should have gone through and checked that every place where
RelFileLocator got embedded in some larger struct, there was no
problem with making it bigger and increasing the alignment
requirement. I'll go back and do that as soon as the immediate
problems are fixed. This case would have stood out as something
needing attention.

Some of the cases are pretty subtle, though. tamandua is still unhappy
even after pushing that fix, because xl_xact_relfilelocators embeds a
RelFileLocator which now requires 8-byte alignment, and
ParseCommitRecord has an undocumented assumption that none of the
things embedded in a commit record require more than 4-byte alignment.
Really, if it's critical for a struct to never require more than
4-byte alignment, there ought to be a comment about that *on the
struct itself*. Having a comment on a function that does something
with that struct is probably not really good enough, and we don't even
have that.

--
Robert Haas
EDB: http://www.enterprisedb.com