Message-ID: <21477053-efd1-485a-8f72-c2e292fcf2a0@garret.ru>
Date: Thu, 9 Jul 2026 08:17:06 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: BUG #19519: REPACK can fail due to missing chunk for toast value
To: Heikki Linnakangas <hlinnaka@iki.fi>,
 Michael Paquier <michael@paquier.xyz>,
 Srinath Reddy Sadipiralla <srinath2133@gmail.com>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>,
 Imran Zaheer <imran.zhir@gmail.com>, Alexander Lakhin <exclusion@gmail.com>,
 PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
References: 
 <CAFC+b6rLNvDzzYxrfLdq7CWbbouHcuOxi-mjEOW1r3qy2uAPMQ@mail.gmail.com>
 <CA+UBfan1QukfNDPvb2YpPG-ME4vUduBfbbe5uWptXDryZbtwVA@mail.gmail.com>
 <aktM919ANQGYZcsj@paquier.xyz>
 <CAEze2WhCRX86ZY2QN7vuGc3wsAyOJbArFSc0i5KtMVnsguA7Kw@mail.gmail.com>
 <akw336Xpi9Ck9-Oz@paquier.xyz>
 <CAEze2WgBjhf6KTKuJ-UqHHGj9Z66f62grmJgZ1EazvMH9HU+XA@mail.gmail.com>
 <akxZiYmGhvNfPVvh@paquier.xyz> <akyM9G67ZIhPXjmQ@paquier.xyz>
 <CAFC+b6rOdx6rMi_numCrK8cM4z_HWQKrAcBY0dfrRcRWTRpsBQ@mail.gmail.com>
 <ak16dMkUP1pS0rCn@paquier.xyz> <ak2jc7mIhcMDkm-k@paquier.xyz>
 <bc6ec409-41ca-4889-a5c7-4e2f7a3f081c@iki.fi>
Content-Language: en-US
From: Konstantin Knizhnik <knizhnik@garret.ru>
In-Reply-To: <bc6ec409-41ca-4889-a5c7-4e2f7a3f081c@iki.fi>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/21477053-efd1-485a-8f72-c2e292fcf2a0%40garret.ru>
Precedence: bulk


On 08/07/2026 3:00 PM, Heikki Linnakangas wrote:
> On 08/07/2026 04:10, Michael Paquier wrote:
>> On Wed, Jul 08, 2026 at 07:15:16AM +0900, Michael Paquier wrote:
>>> One thing that I still don't like much in the patch as written is my
>>> use of a missing_ok argument, which feels super confusing as it
>>> applies only to the underlying external TOAST, if the relation has
>>> any.  I'd be tempted to rewrite this portion of the patch with a
>>> uint32 flags.  Even if we assign one value for now (say MISSING_TOAST,
>>> MISSING_TOAST_OK or whatever), it would allow more flexibility on ABI
>>> grounds if we want more like states in the future across this portion
>>> of the stack.  Again, I strongly doubt that we will be able to
>>> backpatch any of that.
>
> Yeah, boolean arguments in general can be confusing. Especially 
> arguments like "missing_ok" where it's hard to remember which behavior 
> is true and which is false. An IDE that shows the name of the argument 
> helps, but having the caller say "MISSING_TOAST_OK" rather than "true" 
> is much clearer.
>
> For backpatching, I think we could smuggle the flag in a global 
> variable, to avoid changing the signature of the 
> relation_fetch_toast_slice() callback. See attached patch 0002 on top 
> of your 0001 patch (which is also attached for completeness). It 
> doesn't fix the problem for hypothetical 3rd party tableam 
> implementations that implement their own relation_fetch_toast_slice() 
> callback. But do such extensions even exist? If yes, they could be 
> fixed too by also checking the global variable.
>
> I thought about adding an extra "extended" callback next to 
> relation_fetch_toast_slice(), but I don't see a way to add functions 
> to TableAmRoutine in an ABI-compatible way. For the future, we might 
> want to store sizeof(TableAmRoutine) in the struct itself, so that we 
> could add fields to it in minor versions without breaking the API, in 
> case we need something like this again.


Changing `table_relation_fetch_toast_slice` signature breaks 
compatibility with some extensions, for example duckdb.
I wonder if we can change only  `bool (*relation_fetch_toast_slice)(..., 
bool missing_ok)` callback signature, but preserve signature of 
`table_relation_fetch_toast_slice`, adding one more function 
`table_relation_try_fetch_toast_slice`:


static inline void
table_relation_fetch_toast_slice(Relation toastrel, Oid valueid,
                                                       int32 attrsize, 
int32 sliceoffset,
                                                       int32 
slicelength, varlena *result,
                                                       bool missing_ok)
{

       (void)toastrel->rd_tableam->relation_fetch_toast_slice(toastrel, 
valueid,
attrsize,
sliceoffset, slicelength,
result, false);

}

static inline bool
table_relation_try_fetch_toast_slice(Relation toastrel, Oid valueid,
                                                       int32 attrsize, 
int32 sliceoffset,
                                                       int32 
slicelength, varlena *result,
                                                       bool missing_ok)
{

     return toastrel->rd_tableam->relation_fetch_toast_slice(toastrel, 
valueid,
attrsize,
sliceoffset, slicelength,
result, missing_ok);
  }


>
>> A couple of extra notes while I do not forget about that stuff..  The
>> patch may be better split into two if we go with this approach, as the
>> index build and rewrite paths require different solutions:
>> - One for the rewrite path.  It makes little sense to do any kind of
>> aggressive early detoasting because it may be wasteful due to the
>> tuple rewrites that update their data with only copies by reference
>> (main relation tuple is rewritten, reuses the same external TOAST
>> tuple).  For workloads where UPDATEs do not touch the TOASTed
>> attributes, that would be a waste.
>
> I didn't understand this part. Do you mean when rebuilding the indexes 
> after rewriting the heap? I didn't think we support storing toasted 
> external datums in an index at all.
>
>> - One for the index build path, which is actually too aggressive with
>> its early detoasting, now that I think about it.  There should be no
>> need to perform a detoast for anything else than the attributes that
>> are used in the index definition or the attributes that are used in
>> index expressions.  So as written this patch would lead to a
>> regression.
> Yeah, although I wouldn't be too worried about the performance here. 
> The penalty would be when building an index on values that are large 
> enough to be toasted, with lots of RECENTLY_DEAD tuples. That doesn't 
> seem like a very common case.
>
> - Heikki