Thread: Add TOAST support for more system tables
Hi, hackers! I've tried sending this patch to community before, let me try it second time. Patch is revised and improved compared to previous version. This patch adds TOAST support for system tables pg_class, pg_attribute and pg_largeobject_metadata, as they include ACL columns, which may be potentially large in size. Patch fixes possible pg_upgrade bug (problem with seeing a non-empty new cluster). During code developing it turned out that heap_inplace_update function is not suitable for use with TOAST, so its work could lead to wrong statistics update (for example, during VACUUM). This problem is fixed by adding new heap_inplace_update_prepare_tuple function -- we assume TOASTed attributes are never changed by in-place update, and just replace them with old values. I also added pg_catalog_toast1 test that does check for "invalid tupple length" error when creating index with toasted pg_class. Test grants and drops roles on certain table many times to make ACL column long and then creates index on this table. I wonder what other bugs can happen there, but if anyone can give me a hint, I'll try to fix them. Anyway, in PostgresPro we didn't encounter any problems with this feature. First attempt here: https://www.postgresql.org/message-id/1643112264.186902312@f325.i.mail.ru This time I'll do it better -- Sofia Kopikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Attachment
Sofia Kopikova <s.kopikova@postgrespro.ru> writes:
> This patch adds TOAST support for system tables pg_class,
> pg_attribute and pg_largeobject_metadata, as they include ACL columns,
> which may be potentially large in size.
We have been around on this topic before, cf discussion leading up to
commit 96cdeae07. Allowing toasted data in pg_class or pg_attribute
seems quite scary to me because of the potential for recursive access,
particularly during cache-flush scenarios. (That is, you need to be
able to read those catalogs on the way to fetching a toasted value,
so how can you be sure that doesn't devolve into an infinite loop?)
I wonder whether we'd be better off shoving the ACL data out of
these catalogs and putting it somewhere else (compare pg_attrdef).
regards, tom lane
On Tue, 18 Jul 2023 at 10:31, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wonder whether we'd be better off shoving the ACL data out of > these catalogs and putting it somewhere else (compare pg_attrdef). relpartbound is another column that could cause a pg_class row to grow too large. I did have a patch [1] to move that column into pg_partition. I imagine it's very bit rotted now. David [1] https://postgr.es/m/CAKJS1f9QjUwQrio20Pi%3DyCHmnouf4z3SfN8sqXaAcwREG6k0zQ%40mail.gmail.com
On Mon, Jul 17, 2023 at 06:31:04PM -0400, Tom Lane wrote:
> Sofia Kopikova <s.kopikova@postgrespro.ru> writes:
> > This patch adds TOAST support for system tables pg_class,
> > pg_attribute and pg_largeobject_metadata, as they include ACL columns,
> > which may be potentially large in size.
>
> We have been around on this topic before, cf discussion leading up to
> commit 96cdeae07. Allowing toasted data in pg_class or pg_attribute
> seems quite scary to me because of the potential for recursive access,
> particularly during cache-flush scenarios. (That is, you need to be
> able to read those catalogs on the way to fetching a toasted value,
> so how can you be sure that doesn't devolve into an infinite loop?)
Yep. I have something to add here. The last time I poked at that, I
was wondering about two code paths that have specific comments on this
matter. Based on my notes:
1) finish_heap_swap() in cluster.c:
* pg_class doesn't have a toast relation, so we don't need to update the
* corresponding toast relation. Not that there's little point moving all
* relfrozenxid updates here since swap_relation_files() needs to write to
* pg_class for non-mapped relations anyway.
2) extract_autovac_opts() in autovacuum.c:
* we acquired the pg_class row. If pg_class had a TOAST table, this would
* be a risk; fortunately, it doesn't.
What has been posted makes zero adjustments in these areas.
--
Michael
Attachment
On Mon, Jul 17, 2023 at 06:31:04PM -0400, Tom Lane wrote: > Sofia Kopikova <s.kopikova@postgrespro.ru> writes: >> This patch adds TOAST support for system tables pg_class, >> pg_attribute and pg_largeobject_metadata, as they include ACL columns, >> which may be potentially large in size. > We have been around on this topic before, cf discussion leading up to > commit 96cdeae07. Allowing toasted data in pg_class or pg_attribute > seems quite scary to me because of the potential for recursive access, > particularly during cache-flush scenarios. (That is, you need to be > able to read those catalogs on the way to fetching a toasted value, > so how can you be sure that doesn't devolve into an infinite loop?) Many thanks for your reviews. I'm gonna do research and revise this feature thoroughly. I'll set status of the patch to "Waiting on author" for now. -- Sofia Kopikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company