Subject: Re: Fix brin_form_tuple to properly detoast data
To: Alvaro Herrera <alvherre@alvh.no-ip.org>,
 Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: pgsql-hackers@postgresql.org
References: <20201105171717.GA29029@alvherre.pgsql>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <21cacd39-1ca2-54ef-4f3e-ac493238832c@enterprisedb.com>
Date: Thu, 5 Nov 2020 18:42:10 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.3.1
MIME-Version: 1.0
In-Reply-To: <20201105171717.GA29029@alvherre.pgsql>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 11/5/20 6:17 PM, Alvaro Herrera wrote:
> On 2020-Nov-04, Tomas Vondra wrote:
> 
>> The first test is fairly trivial - it simply builds index on toasted data
>> and then shows how an insert and select fail. There's a caveat, that this
>> requires a DELETE + VACUUM, and the VACUUM actually has to cleanup the rows.
>> So there must be no concurrent transactions that might need the rows, which
>> is unlikely in regression tests. So this requires waiting for all running
>> transactions to finish - I did that by building an index concurrently. It's
>> a bit strange, but it's better than any other solution I could think of
>> (timeout or some custom wait for xacts).
> 
> 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.

>> The second test is a bit redundant - it merely checks that both CREATE INDEX
>> and INSERT INTO fail the same way when the index tuple gets too large.
>> Before the fix there were some inconsistencies - the CREATE INDEX succeeded
>> because it used TOASTed data. So ultimately this tests the same thing, but
>> from a different perspective.
> 
> Hmm.  This one shows page size in the error messages, so it'll fail on
> nonstandard builds.  I think we try to stay away from introducing those,
> so I'd leave this test out.

Hmm, OK. I don't think having "redundant" test is a big deal, but I 
haven't thought about builds with different block sizes. I'll leave this 
out.

> 
> The code fix looks all right -- I'd just move the #include lines to
> their place.  Patch 0002.
> 

OK

> You add this comment:
> 
>> +			/*
>> +			 * Do nothing if value is not of varlena type. We don't need to
>> +			 * care about NULL values here, thanks to bv_allnulls above.
>> +			 *
>> +			 * If value is stored EXTERNAL, must fetch it so we are not
>> +			 * depending on outside storage.
>> +			 *
>> +			 * XXX Is this actually true? Could it be that the summary is
>> +			 * NULL even for range with non-NULL data? E.g. degenerate bloom
>> +			 * filter may be thrown away, etc.
>> +			 */
> 
> I think the XXX comment points to a bug that we don't have right now,
> since neither minmax nor inclusion can end up with a NULL summary
> starting from non-NULL data.  But if the comment about bloom is correct,
> then surely it'll become a bug when bloom is added.
> 

Yeah, but that'd be something for the bloom patch to fix, I think.

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.

> I don't think we need the second part of this comment:
> 
>> +/*
>> + * 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.

> 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?


regards

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