Обсуждение: 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

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

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