Hi,
On 2016-04-14 13:11:43 -0400, Tom Lane wrote:
> Well, yeah, because if Size is wider than int32 then that memcpy isn't
> enough to initialize it.
Yes, I wasn't suggesting this is a proper patch, just wanted to know
whether this actually is the issue.
> 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.
'data' cannot be aligned arbitrarily. It's palloc aligned, and the
buffer is used separately for everything is loaded from disk. I.e. the
data pointer passed to ReorderBufferRestoreChange is aligned to a
MAXALIGN boundary. The alignment issues come into play for data *after*
the ReorderBufferDiskChange.
> 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.
Afaics all cases but the ->newtuple branch showing the errors ought to
be ok because of the above.
> 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.
Yea, I wasn't sure about keeping that.
> 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.
Well, then Christoph's build wouldn't have caught the issue either?
Coverage isn't perfect, but it's not bad:
http://coverage.postgresql.org/src/backend/replication/logical/reorderbuffer.c.gcov.html
Greetings,
Andres Freund