Thread: Re: Large expressions in indexes can't be stored (non-TOASTable)
Nathan Bossart <nathandbossart@gmail.com> writes:
> Thanks to commit 96cdeae, only a few catalogs remain that are missing TOAST
> tables: pg_attribute, pg_class, pg_index, pg_largeobject, and
> pg_largeobject_metadata. I've attached a short patch to add one for
> pg_index, which resolves the issue cited here. This passes "check-world"
> and didn't fail for a few ad hoc tests (e.g., VACUUM FULL on pg_index). I
> haven't spent too much time investigating possible circularity issues, but
> I'll note that none of the system indexes presently use the indexprs and
> indpred columns.
Yeah, the possibility of circularity seems like the main hazard, but
I agree it's unlikely that the entries for system indexes could ever
need out-of-line storage. There are many other things that would have
to be improved before a system index could use indexprs or indpred.
regards, tom lane
On Wed, Sep 04, 2024 at 03:20:33PM -0400, Jonathan S. Katz wrote: > On 9/4/24 3:08 PM, Tom Lane wrote: >> Nathan Bossart <nathandbossart@gmail.com> writes: >> > Thanks to commit 96cdeae, only a few catalogs remain that are missing TOAST >> > tables: pg_attribute, pg_class, pg_index, pg_largeobject, and >> > pg_largeobject_metadata. I've attached a short patch to add one for >> > pg_index, which resolves the issue cited here. This passes "check-world" >> > and didn't fail for a few ad hoc tests (e.g., VACUUM FULL on pg_index). I >> > haven't spent too much time investigating possible circularity issues, but >> > I'll note that none of the system indexes presently use the indexprs and >> > indpred columns. >> >> Yeah, the possibility of circularity seems like the main hazard, but >> I agree it's unlikely that the entries for system indexes could ever >> need out-of-line storage. There are many other things that would have >> to be improved before a system index could use indexprs or indpred. > > Agreed on the unlikeliness of that, certainly in the short-to-mid term. The > impetus driving this is dealing with a data type that can be quite large, > and it's unlikely system catalogs will be dealing with anything of that > nature, or requiring very long expressions that couldn't be encapsulated in > a different way. Any objections to committing this? I've still been unable to identify any breakage, and adding it now would give us ~1 year of testing before it'd be available in a GA release. Perhaps we should at least add something to misc_sanity.sql that verifies no system indexes are using pg_index's TOAST table. -- nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Wed, Sep 04, 2024 at 03:20:33PM -0400, Jonathan S. Katz wrote:
>> On 9/4/24 3:08 PM, Tom Lane wrote:
>>> Nathan Bossart <nathandbossart@gmail.com> writes:
>>>> Thanks to commit 96cdeae, only a few catalogs remain that are missing TOAST
>>>> tables: pg_attribute, pg_class, pg_index, pg_largeobject, and
>>>> pg_largeobject_metadata. I've attached a short patch to add one for
>>>> pg_index, which resolves the issue cited here.
> Any objections to committing this?
Nope.
regards, tom lane
Hello Nathan,
18.09.2024 22:52, Nathan Bossart wrote:
18.09.2024 22:52, Nathan Bossart wrote:
Committed. I waffled on whether to add a test for system indexes that used pg_index's varlena columns, but I ended up leaving it out. I've attached it here in case anyone thinks we should add it.
I've discovered that Jonathan's initial script:
CREATE TABLE def (id int);
SELECT array_agg(n) b FROM generate_series(1,10_000) n \gset
CREATE OR REPLACE FUNCTION vec_quantizer (a int, b int[]) RETURNS bool
AS $$ SELECT true $$ LANGUAGE SQL IMMUTABLE;
CREATE INDEX ON def (vec_quantizer(id, :'b'));
completed with:
DROP INDEX CONCURRENTLY def_vec_quantizer_idx;
triggers an assertion failure:
TRAP: failed Assert("HaveRegisteredOrActiveSnapshot()"), File: "toast_internals.c", Line: 668, PID: 3723372
with the following stack trace:
ExceptionalCondition at assert.c:52:13
init_toast_snapshot at toast_internals.c:670:2
toast_delete_datum at toast_internals.c:429:60
toast_tuple_cleanup at toast_helper.c:303:30
heap_toast_insert_or_update at heaptoast.c:335:9
heap_update at heapam.c:3752:14
simple_heap_update at heapam.c:4210:11
CatalogTupleUpdate at indexing.c:324:2
index_set_state_flags at index.c:3522:2
index_concurrently_set_dead at index.c:1848:2
index_drop at index.c:2286:3
doDeletion at dependency.c:1362:5
deleteOneObject at dependency.c:1279:12
deleteObjectsInList at dependency.c:229:3
performMultipleDeletions at dependency.c:393:2
RemoveRelations at tablecmds.c:1594:2
ExecDropStmt at utility.c:2008:4
...
This class of assert failures is not new, see e. g., bugs #13809, #18127,
but this concrete instance (with index_set_state_flags()) emerged with
b52c4fc3c and may be worth fixing while on it...
Best regards,
Alexander
Hello Nathan,
19.09.2024 21:36, Nathan Bossart wrote:
> On Thu, Sep 19, 2024 at 12:00:00PM +0300, Alexander Lakhin wrote:
>> completed with:
>> DROP INDEX CONCURRENTLY def_vec_quantizer_idx;
>>
>> triggers an assertion failure:
>> TRAP: failed Assert("HaveRegisteredOrActiveSnapshot()"), File: "toast_internals.c", Line: 668, PID: 3723372
> Ha, that was fast. The attached patch seems to fix the assertion failures.
> It's probably worth checking if any of the adjacent code paths are
> affected, too.
>
Thank you for your attention to that issue!
I've found another two paths to reach that condition:
CREATE INDEX CONCURRENTLY ON def (vec_quantizer(id, :'b'));
ERROR: cannot fetch toast data without an active snapshot
REINDEX INDEX CONCURRENTLY def_vec_quantizer_idx;
(or REINDEX TABLE CONCURRENTLY def;)
TRAP: failed Assert("HaveRegisteredOrActiveSnapshot()"), File: "toast_internals.c", Line: 668, PID: 2934502
ExceptionalCondition at assert.c:52:13
init_toast_snapshot at toast_internals.c:670:2
toast_delete_datum at toast_internals.c:429:60
toast_tuple_cleanup at toast_helper.c:303:30
heap_toast_insert_or_update at heaptoast.c:335:9
heap_update at heapam.c:3752:14
simple_heap_update at heapam.c:4210:11
CatalogTupleUpdate at indexing.c:324:2
index_concurrently_swap at index.c:1649:2
ReindexRelationConcurrently at indexcmds.c:4270:3
ReindexIndex at indexcmds.c:2962:1
ExecReindex at indexcmds.c:2884:4
ProcessUtilitySlow at utility.c:1570:22
...
Perhaps it would make sense to check all CatalogTupleUpdate(pg_index, ...)
calls (I've found 10 such instances, but haven't checked them yet).
Best regards,
Alexander
On Fri, Sep 20, 2024 at 08:16:24AM +0900, Michael Paquier wrote: > Perhaps the reason why these snapshots are pushed should be documented > with a comment? Definitely. I'll add those once we are more confident that we've identified all the bugs. -- nathan
Hello Nathan, 20.09.2024 19:51, Nathan Bossart wrote: > Here's a (probably naive) attempt at fixing these, too. I'll give each > path a closer look once it feels like we've identified all the bugs. Thank you for the updated patch! I tested it with two code modifications (1st is to make each created expression index TOASTed (by prepending 1M of spaces to the indexeprs value) and 2nd to make each created index an expression index (by modifying index_elem_options in gram.y) — both modifications are kludgy so I don't dare to publish them) and found no other snapshot-related issues during `make check-world`. Best regards, Alexander
On Tue, Sep 24, 2024 at 02:26:08PM -0500, Nathan Bossart wrote: > On Tue, Sep 24, 2024 at 01:21:45PM +0900, Michael Paquier wrote: >> On Mon, Sep 23, 2024 at 10:50:21AM -0500, Nathan Bossart wrote: >>> I carefully inspected all the code paths this patch touches, and I think >>> I've got all the details right, but I would be grateful if someone else >>> could take a look. >> >> No objections from here with putting the snapshots pops and pushes >> outside the inner routines of reindex/drop concurrently, meaning that >> ReindexRelationConcurrently(), DefineIndex() and index_drop() are fine >> to do these operations. > > Great. I plan to push 0001 shortly. Committed this one. -- nathan
On Wed, Oct 16, 2024 at 09:12:31AM +0900, Michael Paquier wrote:
> - if (!ctx->rel->rd_rel->reltoastrelid)
> + if (!OidIsValid(RelationGetToastRelid(ctx->rel)))
>
> This set of diffs in 0002 is a nice cleanup. I'd wish for relying
> less on zero comparitons when assuming that InvalidOid is in use.
I'm wondering if there's any concern about this one causing back-patching
pain. If so, I can just add the macro for use in new code.
> +static inline void
> +AssertHasSnapshotForToast(Relation rel)
> +{
> + /* bootstrap mode in particular breaks this rule */
> + if (!IsNormalProcessingMode())
> + return;
> +
> + /* if the relation doesn't have a TOAST table, we are good */
> + if (!OidIsValid(RelationGetToastRelid(rel)))
> + return;
> +
> + Assert(HaveRegisteredOrActiveSnapshot());
> +}
>
> Using a separate inlined routine is indeed cleaner as you have
> documented the assumptions behind the check. Wouldn't it be better to
> use a USE_ASSERT_CHECKING block? These two checks for normal
> processing and toastrelid are cheap lookups, but we don't need them at
> all in non-assert paths, so I'd suggest to ignore them entirely for
> the non-USE_ASSERT_CHECKING case.
I assume all of this will get compiled out in non-USE_ASSERT_CHECKING
builds as-is, but I see no problem with surrounding it with an #ifdef to be
sure.
--
nathan
On Thu, Oct 31, 2024 at 08:45:15AM +0900, Michael Paquier wrote: > On Wed, Oct 30, 2024 at 03:54:32PM -0500, Nathan Bossart wrote: >> I'll manage. 0001 was a doozy to back-patch, and this obviously isn't a >> terribly pressing issue, so I plan to wait until after the November minor >> release to apply this. > > Okay by me. I took another look at 0001, and I think it still needs a bit of work. Specifically, IIUC a few of these code paths are actually fine since they do not ever touch a varlena column. For example, clear_subscription_skip_lsn() only updates subskiplsn, so unless I am missing a corner case, there is no risk of trying to access a TOAST table without a snapshot. Allowing such cases would involve adjusting the new assertions in 0003 to check for only the varlena columns during inserts/updates, which is more complicated but seems doable. Or we could just enforce that you have an active snapshot whenever you modify a catalog with a TOAST table. That's simpler, but it requires extra work in some paths (and probably comments to point out that we're only pushing an active snapshot to satisfy an assertion). >> I'm having second thoughts on 0002, so I'll >> probably leave that one uncommitted, but IMHO we definitely need 0003 to >> prevent this issue from reoccurring. > > I liked your idea behind 0002, FWIW. We do that for a lot of fields > already in a Relation. Alright, then I'll plan on proceeding with it. -- nathan
On Wed, Nov 27, 2024 at 03:20:19PM +0900, Michael Paquier wrote: > On Mon, Nov 25, 2024 at 01:29:31PM -0600, Nathan Bossart wrote: >> Or we could just enforce that you have an active snapshot whenever you >> modify a catalog with a TOAST table. That's simpler, but it requires extra >> work in some paths (and probably comments to point out that we're only >> pushing an active snapshot to satisfy an assertion). > > I may be wrong, but I suspect that enforcing the check without being > column-based is the right way to go and that this is going to catch > more errors in the long-term than being a maintenance burden. So I > would keep the snapshot check even if it's a bit aggressive, still > it's useful. And we are not talking about that may code paths that > need to be switched to require a snapshot, as well. Most of the ones > you have mentioned on this thread are really particular in the ways > they do transaction handling. I suspect that it may also catch > out-of-core issues with extensions doing direct catalog manipulations. That is useful feedback, thanks. One other thing that caught my eye is that replorigin_create() uses SnapshotDirty, so I'm unsure if PushActiveSnapshot(GetTransactionSnapshot()) is correct there. The only other example I found is RelationFindReplTupleByIndex(), which uses GetLatestSnapshot(). But I do see that CreateSubscription() calls replorigin_create(), and it seems to rely on the transaction snapshot, so maybe it's okay, at least for the purpose of TOAST table access... I'm finding myself wishing there was a bit more commentary about the proper usage of these snapshot functions. Maybe I can try to add some as a follow-up exercise. -- nathan
On Wed, 27 Nov 2024 at 21:29, Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Wed, Nov 27, 2024 at 03:20:19PM +0900, Michael Paquier wrote: > > On Mon, Nov 25, 2024 at 01:29:31PM -0600, Nathan Bossart wrote: > >> Or we could just enforce that you have an active snapshot whenever you > >> modify a catalog with a TOAST table. That's simpler, but it requires extra > >> work in some paths (and probably comments to point out that we're only > >> pushing an active snapshot to satisfy an assertion). > > > > I may be wrong, but I suspect that enforcing the check without being > > column-based is the right way to go and that this is going to catch > > more errors in the long-term than being a maintenance burden. So I > > would keep the snapshot check even if it's a bit aggressive, still > > it's useful. And we are not talking about that may code paths that > > need to be switched to require a snapshot, as well. Most of the ones > > you have mentioned on this thread are really particular in the ways > > they do transaction handling. I suspect that it may also catch > > out-of-core issues with extensions doing direct catalog manipulations. > > That is useful feedback, thanks. Michael's feedback from [1] is still pending, so I'm updating the status to "Waiting on Author." Please provide an updated patch and change the status back to "Needs Review". [1] - https://www.postgresql.org/message-id/Z0a6IwjW36af71J7%40paquier.xyz Regards, Vignesh
On Wed, Nov 27, 2024 at 9:29 PM Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Wed, Nov 27, 2024 at 03:20:19PM +0900, Michael Paquier wrote: > > On Mon, Nov 25, 2024 at 01:29:31PM -0600, Nathan Bossart wrote: > >> Or we could just enforce that you have an active snapshot whenever you > >> modify a catalog with a TOAST table. That's simpler, but it requires extra > >> work in some paths (and probably comments to point out that we're only > >> pushing an active snapshot to satisfy an assertion). > > > > I may be wrong, but I suspect that enforcing the check without being > > column-based is the right way to go and that this is going to catch > > more errors in the long-term than being a maintenance burden. So I > > would keep the snapshot check even if it's a bit aggressive, still > > it's useful. And we are not talking about that may code paths that > > need to be switched to require a snapshot, as well. Most of the ones > > you have mentioned on this thread are really particular in the ways > > they do transaction handling. I suspect that it may also catch > > out-of-core issues with extensions doing direct catalog manipulations. > > That is useful feedback, thanks. > > One other thing that caught my eye is that replorigin_create() uses > SnapshotDirty, so I'm unsure if > PushActiveSnapshot(GetTransactionSnapshot()) is correct there. > Can we dodge adding this push call if we restrict the length of the origin name to some reasonable limit like 256 or 512 and avoid the need of toast altogether? -- With Regards, Amit Kapila.
On Fri, Apr 04, 2025 at 05:16:43PM +0530, Amit Kapila wrote: > Can we dodge adding this push call if we restrict the length of the > origin name to some reasonable limit like 256 or 512 and avoid the > need of toast altogether? We did consider just removing pg_replication_origin's TOAST table earlier [0], but we decided against it at that time. Maybe it's worth reconsidering... [0] https://postgr.es/m/ZvOMBhlb5zrBCG5p%40paquier.xyz -- nathan