Re: Bus error in pg_logical_slot_get_changes (9.4.7, sparc)

Christoph Berg <myon@debian.org> writes:
> Re: Tom Lane 2016-04-14 <423.1460653903@sss.pgh.pa.us>
>> Proposed patch attached, but I'm still wondering why the alignment-picky
>> buildfarm members aren't all failing on this.  It seems to strongly
>> suggest that the regression tests' coverage is lacking here.

> This patch works both on sparc and amd64.  Thanks!
> (Tested on 9.4.7 only.)

I've been looking around and testing some more.  I noticed that the only
caller of ReorderBufferRestoreChange always passes a maxaligned buffer,
so most of the changes I suggested are unnecessary.  AFAICT, it's only
the second fetch of t_len that is at any risk.  Even there, at least in
HEAD, I cannot construct a test case in which the first tuple's t_len is
not a multiple of 4.  It's hard to tell what is causing that, because
surely heap_form_tuple guarantees no such thing.  I have a suspicion that
something in the impenetrable thicket that passes for prefix/suffix
optimization in log_heap_update is forcing the WAL-logged tuple length to
be rounded off to sizeof(int) (*not* MAXALIGN).

It might be that 9.4 acts differently here, but the lack of crashes in our
buildfarm suggests otherwise.  The best theory I can come up with is that
somehow, your Sparc compiler is deciding that "data" is 8-aligned at this
point and generating a code sequence that depends on that, even though
it's only being asked to fetch a uint32.  If the machine were a 64-bit
machine, the compiler could legitimately make such a deduction because of
the presence of a pointer field in HeapTupleData; but I dunno how it gets
to that conclusion if the architecture is 32-bit.  Anyway, there are few
enough architectures where it could make sense to use an 8-aligned
instruction to fetch a 4-byte value that it's not so surprising we've not
seen it crash elsewhere.

So I now think my previous patch is overkill, and we should instead use
something like the attached.

            regards, tom lane

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 52c6986..4520708 100644
*** a/src/backend/replication/logical/reorderbuffer.c
--- b/src/backend/replication/logical/reorderbuffer.c
*************** ReorderBufferRestoreChanges(ReorderBuffe
*** 2444,2449 ****
--- 2444,2453 ----
  /*
   * Convert change from its on-disk format to in-memory format and queue it onto
   * the TXN's ->changes list.
+  *
+  * Note: although "data" is declared char*, at entry it points to a
+  * maxalign'd buffer, making it safe in most of this function to assume
+  * that the pointed-to data is suitably aligned for direct access.
   */
  static void
  ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
*************** ReorderBufferRestoreChange(ReorderBuffer
*** 2471,2477 ****
          case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT:
              if (change->data.tp.oldtuple)
              {
!                 Size        tuplelen = ((HeapTuple) data)->t_len;

                  change->data.tp.oldtuple =
                      ReorderBufferGetTupleBuf(rb, tuplelen - SizeofHeapTupleHeader);
--- 2475,2481 ----
          case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT:
              if (change->data.tp.oldtuple)
              {
!                 uint32        tuplelen = ((HeapTuple) data)->t_len;

                  change->data.tp.oldtuple =
                      ReorderBufferGetTupleBuf(rb, tuplelen - SizeofHeapTupleHeader);
*************** ReorderBufferRestoreChange(ReorderBuffer
*** 2492,2498 ****

              if (change->data.tp.newtuple)
              {
!                 Size        tuplelen = ((HeapTuple) data)->t_len;

                  change->data.tp.newtuple =
                      ReorderBufferGetTupleBuf(rb, tuplelen - SizeofHeapTupleHeader);
--- 2496,2506 ----

              if (change->data.tp.newtuple)
              {
!                 /* here, data might not be suitably aligned! */
!                 uint32        tuplelen;
!
!                 memcpy(&tuplelen, data + offsetof(HeapTupleData, t_len),
!                        sizeof(uint32));

                  change->data.tp.newtuple =
                      ReorderBufferGetTupleBuf(rb, tuplelen - SizeofHeapTupleHeader);