Date: Sat, 7 Nov 2020 00:45:29 +0100
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 pgsql-hackers@postgresql.org
Subject: Re: Fix brin_form_tuple to properly detoast data
Message-ID: <20201107004529.1b16417d@enterprisedb.com>
In-Reply-To: <20201105211604.GA3054@alvherre.pgsql>
References: <21cacd39-1ca2-54ef-4f3e-ac493238832c@enterprisedb.com>
	<20201105211604.GA3054@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 5 Nov 2020 18:16:04 -0300
Alvaro Herrera <alvherre@alvh.no-ip.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