Re: Bus error in pg_logical_slot_get_changes (9.4.7, sparc)

Christoph Berg <myon@debian.org> writes:
>> ... the patch worked indeed, thanks!

> Duh, the patch does work on sparc, but it breaks amd64:

Well, yeah, because if Size is wider than int32 then that memcpy isn't
enough to initialize it.

But looking around a bit, I think that ReorderBufferRestoreChange is
almost completely broken on alignment-picky machines, if the expectation
is that the input data could be aligned arbitrarily.  Aside from this
particular problem, it contains multiple places where we cast "data"
to something other than char*, and that is sufficient to allow a
spec-compliant compiler to decide that "data" is aligned on more than
a char boundary and hence generate code that depends on such alignment.

I also noticed that the INTERNAL_SNAPSHOT case doesn't bother to advance
"data" past the data read, where all the other switch cases do.  In the
attached, I made it do likewise, but it looks like you could equally
easily just drop the final update of "data" in each switch case,
because nothing is looking at it after the switch.

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.

            regards, tom lane

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 52c6986..b6f83f8 100644
*** a/src/backend/replication/logical/reorderbuffer.c
--- b/src/backend/replication/logical/reorderbuffer.c
*************** static void
*** 2449,2463 ****
  ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
                             char *data)
  {
-     ReorderBufferDiskChange *ondisk;
      ReorderBufferChange *change;

-     ondisk = (ReorderBufferDiskChange *) data;
-
      change = ReorderBufferGetChange(rb);

      /* copy static part */
!     memcpy(change, &ondisk->change, sizeof(ReorderBufferChange));

      data += sizeof(ReorderBufferDiskChange);

--- 2449,2461 ----
  ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
                             char *data)
  {
      ReorderBufferChange *change;

      change = ReorderBufferGetChange(rb);

      /* copy static part */
!     memcpy(change, data + offsetof(ReorderBufferDiskChange, change),
!            sizeof(ReorderBufferChange));

      data += sizeof(ReorderBufferDiskChange);

*************** 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);
--- 2469,2479 ----
          case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT:
              if (change->data.tp.oldtuple)
              {
!                 uint32        tuplelen;
!
!                 /* Must get tuplelen the hard way in case it's misaligned */
!                 memcpy(&tuplelen, data + offsetof(HeapTupleData, t_len),
!                        sizeof(uint32));

                  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);
--- 2494,2504 ----

              if (change->data.tp.newtuple)
              {
!                 uint32        tuplelen;
!
!                 /* Must get tuplelen the hard way in case it's misaligned */
!                 memcpy(&tuplelen, data + offsetof(HeapTupleData, t_len),
!                        sizeof(uint32));

                  change->data.tp.newtuple =
                      ReorderBufferGetTupleBuf(rb, tuplelen - SizeofHeapTupleHeader);
*************** ReorderBufferRestoreChange(ReorderBuffer
*** 2538,2552 ****
              }
          case REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT:
              {
!                 Snapshot    oldsnap;
                  Snapshot    newsnap;
                  Size        size;

!                 oldsnap = (Snapshot) data;

                  size = sizeof(SnapshotData) +
!                     sizeof(TransactionId) * oldsnap->xcnt +
!                     sizeof(TransactionId) * (oldsnap->subxcnt + 0);

                  change->data.snapshot = MemoryContextAllocZero(rb->context, size);

--- 2544,2558 ----
              }
          case REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT:
              {
!                 SnapshotData oldsnap;
                  Snapshot    newsnap;
                  Size        size;

!                 memcpy(&oldsnap, data, sizeof(SnapshotData));

                  size = sizeof(SnapshotData) +
!                     sizeof(TransactionId) * oldsnap.xcnt +
!                     sizeof(TransactionId) * oldsnap.subxcnt;

                  change->data.snapshot = MemoryContextAllocZero(rb->context, size);

*************** ReorderBufferRestoreChange(ReorderBuffer
*** 2557,2562 ****
--- 2563,2570 ----
                      (((char *) newsnap) + sizeof(SnapshotData));
                  newsnap->subxip = newsnap->xip + newsnap->xcnt;
                  newsnap->copied = true;
+
+                 data += size;
                  break;
              }
              /* the base struct contains all the data, easy peasy */