public inbox for [email protected]
help / color / mirror / Atom feedFrom: Konstantin Knizhnik <[email protected]>
To: Heikki Linnakangas <[email protected]>
To: Michael Paquier <[email protected]>
To: Srinath Reddy Sadipiralla <[email protected]>
Cc: Matthias van de Meent <[email protected]>
Cc: Imran Zaheer <[email protected]>
Cc: Alexander Lakhin <[email protected]>
Cc: PostgreSQL mailing lists <[email protected]>
Subject: Re: BUG #19519: REPACK can fail due to missing chunk for toast value
Date: Thu, 9 Jul 2026 08:17:06 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CAFC+b6rLNvDzzYxrfLdq7CWbbouHcuOxi-mjEOW1r3qy2uAPMQ@mail.gmail.com>
<CA+UBfan1QukfNDPvb2YpPG-ME4vUduBfbbe5uWptXDryZbtwVA@mail.gmail.com>
<[email protected]>
<CAEze2WhCRX86ZY2QN7vuGc3wsAyOJbArFSc0i5KtMVnsguA7Kw@mail.gmail.com>
<[email protected]>
<CAEze2WgBjhf6KTKuJ-UqHHGj9Z66f62grmJgZ1EazvMH9HU+XA@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAFC+b6rOdx6rMi_numCrK8cM4z_HWQKrAcBY0dfrRcRWTRpsBQ@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
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
view thread (21+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #19519: REPACK can fail due to missing chunk for toast value
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox