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.186902...@f325.i.mail.ru
This time I'll do it better
--
Sofia Kopikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
From 6812e6cd47c03e22f953732a24223411d80d6c95 Mon Sep 17 00:00:00 2001
From: Sofia Kopikova <s.kopik...@postgrespro.ru>
Date: Mon, 17 Jul 2023 11:56:25 +0300
Subject: [PATCH] Add TOAST support for several system tables
ACL lists may have large size. Using TOAST for system tables pg_class,
pg_attribute and pg_largeobject_metadata with aclitem[] columns.
In 96cdeae07f9 some system catalogs were excluded from adding TOATS to
them due to several possible bugs. Here bug with pg_upgrade seeing a
non-empty new cluster is fixed. A workaround is added to
heap_inplace_update function for its correct work with TOAST. Also test
for "invalig tupple length" error case when creating index with toasted
pg_class is added.
---
src/backend/access/heap/heapam.c | 64 +++++++++++++++++--
src/backend/catalog/catalog.c | 2 +
src/bin/pg_upgrade/check.c | 3 +-
src/include/catalog/pg_attribute.h | 2 +
src/include/catalog/pg_class.h | 2 +
src/include/catalog/pg_largeobject_metadata.h | 2 +
src/test/regress/expected/misc_sanity.out | 30 ++++-----
.../regress/expected/pg_catalog_toast1.out | 25 ++++++++
src/test/regress/parallel_schedule | 3 +
src/test/regress/sql/misc_sanity.sql | 10 +--
src/test/regress/sql/pg_catalog_toast1.sql | 20 ++++++
11 files changed, 134 insertions(+), 29 deletions(-)
create mode 100644 src/test/regress/expected/pg_catalog_toast1.out
create mode 100644 src/test/regress/sql/pg_catalog_toast1.sql
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 7ed72abe597..e043a8e4401 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5860,6 +5860,53 @@ heap_abort_speculative(Relation relation, ItemPointer tid)
pgstat_count_heap_delete(relation);
}
+/*
+ * Prepare tuple for inplace update. TOASTed attributes can't be modified
+ * by in-place upgrade. Simultaneously, new tuple is flatten. So, we just
+ * replace TOASTed attributes with their values of old tuple.
+ */
+static HeapTuple
+heap_inplace_update_prepare_tuple(Relation relation,
+ HeapTuple oldtup,
+ HeapTuple newtup)
+{
+ TupleDesc desc = relation->rd_att;
+ HeapTuple result;
+ Datum *oldvals,
+ *newvals;
+ bool *oldnulls,
+ *newnulls;
+ int i,
+ natts = desc->natts;
+
+ oldvals = (Datum *) palloc(sizeof(Datum) * natts);
+ newvals = (Datum *) palloc(sizeof(Datum) * natts);
+ oldnulls = (bool *) palloc(sizeof(bool) * natts);
+ newnulls = (bool *) palloc(sizeof(bool) * natts);
+
+ heap_deform_tuple(oldtup, desc, oldvals, oldnulls);
+ heap_deform_tuple(newtup, desc, newvals, newnulls);
+
+ for (i = 0; i < natts; i++)
+ {
+ Form_pg_attribute att = &desc->attrs[i];
+
+ if (att->attlen == -1 &&
+ !oldnulls[i] &&
+ VARATT_IS_EXTENDED(oldvals[i]))
+ {
+ Assert(!newnulls[i]);
+ newvals[i] = oldvals[i];
+ }
+ }
+
+ result = heap_form_tuple(desc, newvals, newnulls);
+
+ result->t_self = newtup->t_self;
+
+ return result;
+}
+
/*
* heap_inplace_update - update a tuple "in place" (ie, overwrite it)
*
@@ -5889,6 +5936,8 @@ heap_inplace_update(Relation relation, HeapTuple tuple)
HeapTupleHeader htup;
uint32 oldlen;
uint32 newlen;
+ HeapTupleData oldtup;
+ HeapTuple newtup;
/*
* For now, we don't allow parallel updates. Unlike a regular update,
@@ -5914,16 +5963,23 @@ heap_inplace_update(Relation relation, HeapTuple tuple)
htup = (HeapTupleHeader) PageGetItem(page, lp);
+ oldtup.t_tableOid = RelationGetRelid(relation);
+ oldtup.t_data = htup;
+ oldtup.t_len = ItemIdGetLength(lp);
+ oldtup.t_self = tuple->t_self;
+
+ newtup = heap_inplace_update_prepare_tuple(relation, &oldtup, tuple);
+
oldlen = ItemIdGetLength(lp) - htup->t_hoff;
- newlen = tuple->t_len - tuple->t_data->t_hoff;
- if (oldlen != newlen || htup->t_hoff != tuple->t_data->t_hoff)
+ newlen = newtup->t_len - newtup->t_data->t_hoff;
+ if (oldlen != newlen || htup->t_hoff != newtup->t_data->t_hoff)
elog(ERROR, "wrong tuple length");
/* NO EREPORT(ERROR) from here till changes are logged */
START_CRIT_SECTION();
memcpy((char *) htup + htup->t_hoff,
- (char *) tuple->t_data + tuple->t_data->t_hoff,
+ (char *) newtup->t_data + newtup->t_data->t_hoff,
newlen);
MarkBufferDirty(buffer);
@@ -5934,7 +5990,7 @@ heap_inplace_update(Relation relation, HeapTuple tuple)
xl_heap_inplace xlrec;
XLogRecPtr recptr;
- xlrec.offnum = ItemPointerGetOffsetNumber(&tuple->t_self);
+ xlrec.offnum = ItemPointerGetOffsetNumber(&newtup->t_self);
XLogBeginInsert();
XLogRegisterData((char *) &xlrec, SizeOfHeapInplace);
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 1bf6c5633cd..2f463f9216d 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -297,6 +297,8 @@ IsSharedRelation(Oid relationId)
relationId == PgShseclabelToastIndex ||
relationId == PgSubscriptionToastTable ||
relationId == PgSubscriptionToastIndex ||
+ relationId == PgDatabaseToastTable ||
+ relationId == PgDatabaseToastIndex ||
relationId == PgTablespaceToastTable ||
relationId == PgTablespaceToastIndex)
return true;
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 64024e3b9ec..e9994367995 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -358,7 +358,8 @@ check_new_cluster_is_empty(void)
relnum++)
{
/* pg_largeobject and its index should be skipped */
- if (strcmp(rel_arr->rels[relnum].nspname, "pg_catalog") != 0)
+ if (strcmp(rel_arr->rels[relnum].nspname, "pg_catalog") != 0 &&
+ strcmp(rel_arr->rels[relnum].nspname, "pg_toast") != 0)
pg_fatal("New cluster database \"%s\" is not empty: found relation \"%s.%s\"",
new_cluster.dbarr.dbs[dbnum].db_name,
rel_arr->rels[relnum].nspname,
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index f00df488ce7..e932cb82836 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -208,6 +208,8 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
*/
typedef FormData_pg_attribute *Form_pg_attribute;
+DECLARE_TOAST(pg_attribute, 9001, 9002);
+
DECLARE_UNIQUE_INDEX(pg_attribute_relid_attnam_index, 2658, AttributeRelidNameIndexId, on pg_attribute using btree(attrelid oid_ops, attname name_ops));
DECLARE_UNIQUE_INDEX_PKEY(pg_attribute_relid_attnum_index, 2659, AttributeRelidNumIndexId, on pg_attribute using btree(attrelid oid_ops, attnum int2_ops));
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 2d1bb7af3a9..9bdb058fe78 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -152,6 +152,8 @@ CATALOG(pg_class,1259,RelationRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83,Relat
*/
typedef FormData_pg_class *Form_pg_class;
+DECLARE_TOAST(pg_class, 9003, 9004);
+
DECLARE_UNIQUE_INDEX_PKEY(pg_class_oid_index, 2662, ClassOidIndexId, on pg_class using btree(oid oid_ops));
DECLARE_UNIQUE_INDEX(pg_class_relname_nsp_index, 2663, ClassNameNspIndexId, on pg_class using btree(relname name_ops, relnamespace oid_ops));
DECLARE_INDEX(pg_class_tblspc_relfilenode_index, 3455, ClassTblspcRelfilenodeIndexId, on pg_class using btree(reltablespace oid_ops, relfilenode oid_ops));
diff --git a/src/include/catalog/pg_largeobject_metadata.h b/src/include/catalog/pg_largeobject_metadata.h
index 80db926079f..c0b99f5b295 100644
--- a/src/include/catalog/pg_largeobject_metadata.h
+++ b/src/include/catalog/pg_largeobject_metadata.h
@@ -46,6 +46,8 @@ CATALOG(pg_largeobject_metadata,2995,LargeObjectMetadataRelationId)
*/
typedef FormData_pg_largeobject_metadata *Form_pg_largeobject_metadata;
+DECLARE_TOAST(pg_largeobject_metadata, 9040, 9041);
+
DECLARE_UNIQUE_INDEX_PKEY(pg_largeobject_metadata_oid_index, 2996, LargeObjectMetadataOidIndexId, on pg_largeobject_metadata using btree(oid oid_ops));
#endif /* PG_LARGEOBJECT_METADATA_H */
diff --git a/src/test/regress/expected/misc_sanity.out b/src/test/regress/expected/misc_sanity.out
index a57fd142a94..2275c48ad3a 100644
--- a/src/test/regress/expected/misc_sanity.out
+++ b/src/test/regress/expected/misc_sanity.out
@@ -35,11 +35,11 @@ WHERE refclassid = 0 OR refobjid = 0 OR
-- Look for system tables with varlena columns but no toast table. All
-- system tables with toastable columns should have toast tables, with
-- the following exceptions:
--- 1. pg_class, pg_attribute, and pg_index, due to fear of recursive
--- dependencies as toast tables depend on them.
--- 2. pg_largeobject and pg_largeobject_metadata. Large object catalogs
--- and toast tables are mutually exclusive and large object data is handled
--- as user data by pg_upgrade, which would cause failures.
+-- 1. pg_index.
+-- 2. pg_largeobject.
+-- These and some other tables were excluded in PostgreSQL for various reasons,
+-- but in Postgres Pro Enterprise we added toast tables for system tables with
+-- ACL columns.
SELECT relname, attname, atttypid::regtype
FROM pg_class c JOIN pg_attribute a ON c.oid = attrelid
WHERE c.oid < 16384 AND
@@ -47,20 +47,12 @@ WHERE c.oid < 16384 AND
relkind = 'r' AND
attstorage != 'p'
ORDER BY 1, 2;
- relname | attname | atttypid
--------------------------+---------------+--------------
- pg_attribute | attacl | aclitem[]
- pg_attribute | attfdwoptions | text[]
- pg_attribute | attmissingval | anyarray
- pg_attribute | attoptions | text[]
- pg_class | relacl | aclitem[]
- pg_class | reloptions | text[]
- pg_class | relpartbound | pg_node_tree
- pg_index | indexprs | pg_node_tree
- pg_index | indpred | pg_node_tree
- pg_largeobject | data | bytea
- pg_largeobject_metadata | lomacl | aclitem[]
-(11 rows)
+ relname | attname | atttypid
+----------------+----------+--------------
+ pg_index | indexprs | pg_node_tree
+ pg_index | indpred | pg_node_tree
+ pg_largeobject | data | bytea
+(3 rows)
-- system catalogs without primary keys
--
diff --git a/src/test/regress/expected/pg_catalog_toast1.out b/src/test/regress/expected/pg_catalog_toast1.out
new file mode 100644
index 00000000000..1918351fede
--- /dev/null
+++ b/src/test/regress/expected/pg_catalog_toast1.out
@@ -0,0 +1,25 @@
+CREATE OR REPLACE FUNCTION toast_acl()
+RETURNS BOOLEAN AS $$
+DECLARE
+ I INTEGER :=0;
+BEGIN
+ SET LOCAL CLIENT_MIN_MESSAGES=WARNING;
+ EXECUTE 'DROP TABLE IF EXISTS test2510';
+ EXECUTE 'CREATE TABLE test2510 (a INT)';
+ FOR I IN 1..4096 LOOP
+ EXECUTE 'DROP ROLE IF EXISTS role' || I;
+ EXECUTE 'CREATE ROLE role' || I;
+ EXECUTE 'GRANT ALL ON test2510 TO role' || I;
+ END LOOP;
+ RESET CLIENT_MIN_MESSAGES;
+ RETURN TRUE;
+END;
+$$
+LANGUAGE PLPGSQL;
+SELECT toast_acl();
+ toast_acl
+-----------
+ t
+(1 row)
+
+CREATE INDEX ON test2510 USING BTREE(a);
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 4df9d8503b9..06620d79ec8 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -132,3 +132,6 @@ test: fast_default
# run tablespace test at the end because it drops the tablespace created during
# setup that other tests may use.
test: tablespace
+
+#check for "invalig tupple length" error when creating index with toasted pg_class
+test: pg_catalog_toast1
diff --git a/src/test/regress/sql/misc_sanity.sql b/src/test/regress/sql/misc_sanity.sql
index 2c0f87a651f..658f17fef99 100644
--- a/src/test/regress/sql/misc_sanity.sql
+++ b/src/test/regress/sql/misc_sanity.sql
@@ -38,11 +38,11 @@ WHERE refclassid = 0 OR refobjid = 0 OR
-- Look for system tables with varlena columns but no toast table. All
-- system tables with toastable columns should have toast tables, with
-- the following exceptions:
--- 1. pg_class, pg_attribute, and pg_index, due to fear of recursive
--- dependencies as toast tables depend on them.
--- 2. pg_largeobject and pg_largeobject_metadata. Large object catalogs
--- and toast tables are mutually exclusive and large object data is handled
--- as user data by pg_upgrade, which would cause failures.
+-- 1. pg_index.
+-- 2. pg_largeobject.
+-- These and some other tables were excluded in PostgreSQL for various reasons,
+-- but in Postgres Pro Enterprise we added toast tables for system tables with
+-- ACL columns.
SELECT relname, attname, atttypid::regtype
FROM pg_class c JOIN pg_attribute a ON c.oid = attrelid
diff --git a/src/test/regress/sql/pg_catalog_toast1.sql b/src/test/regress/sql/pg_catalog_toast1.sql
new file mode 100644
index 00000000000..d33122214fe
--- /dev/null
+++ b/src/test/regress/sql/pg_catalog_toast1.sql
@@ -0,0 +1,20 @@
+CREATE OR REPLACE FUNCTION toast_acl()
+RETURNS BOOLEAN AS $$
+DECLARE
+ I INTEGER :=0;
+BEGIN
+ SET LOCAL CLIENT_MIN_MESSAGES=WARNING;
+ EXECUTE 'DROP TABLE IF EXISTS test2510';
+ EXECUTE 'CREATE TABLE test2510 (a INT)';
+ FOR I IN 1..4096 LOOP
+ EXECUTE 'DROP ROLE IF EXISTS role' || I;
+ EXECUTE 'CREATE ROLE role' || I;
+ EXECUTE 'GRANT ALL ON test2510 TO role' || I;
+ END LOOP;
+ RESET CLIENT_MIN_MESSAGES;
+ RETURN TRUE;
+END;
+$$
+LANGUAGE PLPGSQL;
+SELECT toast_acl();
+CREATE INDEX ON test2510 USING BTREE(a);
--
2.20.1
