Re: SerializeParamList vs machines with strict alignment

On Sun, Sep 30, 2018 at 10:44 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Sep 10, 2018 at 7:05 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> > > On Mon, Sep 10, 2018 at 8:58 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > >> In particular, SerializeParamList does this:
> > >>
> > >> /* Write flags. */
> > >> memcpy(*start_address, &prm->pflags, sizeof(uint16));
> > >> *start_address += sizeof(uint16);
> > >>
> > >> immediately followed by this:
> > >>
> > >> datumSerialize(prm->value, prm->isnull, typByVal, typLen,
> > >> start_address);
> >
> > > datumSerialize does this:
> >
> > > memcpy(*start_address, &header, sizeof(int));
> > > *start_address += sizeof(int);
> >
> > > before calling EOH_flatten_into, so it seems to me it should be 4-byte aligned.
> >
> > But that doesn't undo the fact that you're now on an odd halfword
> > boundary. In the case I observed, EA_flatten_into was trying to
> > store an int32 through a pointer whose hex value ended in E, which
> > is explained by the "+= sizeof(uint16)".
> >
> > > Yeah, I think as suggested by you, start_address should be maxaligned.
> >
> > A localized fix would be for datumSerialize to temporarily palloc the
> > space for EOH_flatten_into to write into, and then it could memcpy
> > that to whatever random address it'd been passed.
>
> Attached is a patch along these lines, let me know if you have
> something else in mind? I have manually verified this with
> force_parallel_mode=regress configuration in my development
> environment. I don't have access to alignment-sensitive hardware, so
> can't test in such an environment. I will see if I can write a test
> as well.
>

Attached patch contains a test case as well to cover this code. It
will help us in identifying if there is any failure on
alignment-sensitive hardware in this code path. Kindly suggest what
is the best path forward, is it a good idea to commit this on HEAD
first and see if the test passes on all machines in buildfarm and then
back-patch it?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com