Re: pgsql: Add support for multivariate MCV lists

From: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
To: Peter Geoghegan <pg(at)bowt(dot)ie>
Cc: Andres Freund <andres(at)anarazel(dot)de>, Tomas Vondra <tomas(dot)vondra(at)postgresql(dot)org>, pgsql-committers <pgsql-committers(at)lists(dot)postgresql(dot)org>
Subject: Re: pgsql: Add support for multivariate MCV lists
Date: 2019-03-29 18:20:13
Message-ID: 20190329182013.GB8733@development
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers

On Thu, Mar 28, 2019 at 08:37:11PM +0100, Tomas Vondra wrote:
>On Thu, Mar 28, 2019 at 07:33:36PM +0100, Tomas Vondra wrote:
>>On Thu, Mar 28, 2019 at 11:29:12AM -0700, Peter Geoghegan wrote:
>>>On Wed, Mar 27, 2019 at 6:27 PM Tomas Vondra
>>><tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>>>>It's a bit too late for pushing emergency fixes over here, so I'll do
>>>>more testing tomorrow and then push.
>>>
>>>The buildfarm is still almost all-red now. Can you estimate how long
>>>it will take to push a fix?
>>>
>>
>>Half an hour, at most. I have a fix and I'm running tests on it to make
>>sure it does break something else.
>>
>
>OK, I've pushed the fix. As explained in the commit message, the
>deserialization was borked in two ways. Firstly, it was vulnerable to
>use-after-free. Secondly, the serialization/deserialization of data for
>by-value types did not work for bigendian systems.
>
>I believe this should fix prion (which was tripping on the first issue,
>due to using -DRELCACHE_FORCE_RELEASE -DCATCACHE_FORCE_RELEASE) and at
>least some of the bigendian boxes (I've tested it on s390x).
>

OK, this apparently worked fine for all x86 and s390 machines.

>
>I do think there's one remaining issue - the deserialized value is
>allocated as a single chunk, and is then "sliced" into smaller buffers.
>But the code ignores alignment, which I think may trigger SIGBUS on some
>platforms - for example grison, skate or gull fail like this, and those
>are ARMv7 and sparc machines.
>
>I do have a fix for that too, but I decided not to push it yet before
>testing it a bit more.
>

I've pushed a fix for this. The short version is that the serialized
representation was not respecting memory alignment requirements, which was
causing issues in machines sensitive to this (ia64, sparc, hppa). It's a
blind attempt, as I currently don't have access to any such machine.

FWIW I've pushed this now so that the machines can test the other stuff,
like jsonpath. I think the serialization/deserialization would deserve a
second look, and I'm wondering if it should work more like ArrayType (i.e.
relying on att_align_* instead of explicit MAXALIGN).

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Peter Geoghegan 2019-03-29 19:06:26 Re: pgsql: Add support for multivariate MCV lists
Previous Message Tomas Vondra 2019-03-29 18:08:16 pgsql: Fix memory alignment in pg_mcv_list serialization