Re: Fix brin_form_tuple to properly detoast data

On Thu, 5 Nov 2020 18:16:04 -0300
Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:

> On 2020-Nov-05, Tomas Vondra wrote:
>
> > On 11/5/20 6:17 PM, Alvaro Herrera wrote:
>
> > > There are recent changes in vacuum for temp tables (commit
> > > 94bc27b57680?) that would maybe make this stable enough, without
> > > having to have the CIC there. At least, I tried it locally a few
> > > times and it appears to work well. This won't work for older
> > > releases though, just master. This is patch 0001 attached here.
> >
> > IIUC you're suggesting to use a temporary table in the test?
> > Unfortunately, this does not work on older releases, and IMHO the
> > test should be backpatched too. IMHO the CIC "hack" is acceptable,
> > unless there's a better solution that I'm not aware of.
>
> Oh, sure, the CIC hack is acceptable for the older branches. I'm just
> saying that you can use a temp table everywhere, and keep CIC in the
> old branches and no CIC in master.
>
> > This got me thinking though - wouldn't it be better to handle too
> > large values by treating the range as "degenerate" (i.e. store NULL
> > and consider it as matching all queries), instead of failing the
> > CREATE INDEX or DML? I find the current behavior rather annoying,
> > because it depends on the other rows in the page range, not just on
> > the one row the user deals with. Perhaps this might even be
> > considered an information leak about the other data. Of course, not
> > something this patch should deal with.
>
> Hmm. Regarding text I remember thinking we could just truncate values
> (as we do for LIKE, as I recall). I suppose that strategy would work
> even for bytea.
>
>
> > > > +/*
> > > > + * This enables de-toasting of index entries. Needed until
> > > > VACUUM is
> > > > + * smart enough to rebuild indexes from scratch.
> > > > + */
> > >
> > > ... because, surely, we're now never working on having VACUUM
> > > rebuild indexes from scratch. In fact, I wonder if we need the
> > > #define at all. I propose to remove all those #ifdef lines in
> > > your patch.
> >
> > That's a verbatim copy of a comment from indextuple.c. IMHO we
> > should keep it the same in both places.
>
> Sure, if you want to.
>
> > > The fix looks good to me. I just added a comment in 0003.
> >
> > Thanks. Any opinions on fixing this in existing clusters? Any
> > better ideas than just giving users the SQL query to list
> > possibly-affected indexes?
>
> None here.
>

OK, pushed and backpatched all the way back to 9.5. I decided not to
use the temporary table - I'd still need to use the CIC trick on older
releases, and there were enough differences already.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company