From f13c4f1a473c6ad56f73af7e51d4532003f9e9ac Mon Sep 17 00:00:00 2001 From: Elvis Pranskevichus Date: Mon, 9 Dec 2024 18:04:25 -0800 Subject: [PATCH] Add patches for edgedb/postgres#3 (#127) --- ...dgedb__no_table_rewite_on_domains-16.patch | 98 +++++++++++++++++++ ...dgedb__no_table_rewite_on_domains-17.patch | 98 +++++++++++++++++++ 2 files changed, 196 insertions(+) create mode 100644 edgedbpkg/postgresql/patches/postgresql-edgedb__no_table_rewite_on_domains-16.patch create mode 100644 edgedbpkg/postgresql/patches/postgresql-edgedb__no_table_rewite_on_domains-17.patch diff --git a/edgedbpkg/postgresql/patches/postgresql-edgedb__no_table_rewite_on_domains-16.patch b/edgedbpkg/postgresql/patches/postgresql-edgedb__no_table_rewite_on_domains-16.patch new file mode 100644 index 0000000..6c2c125 --- /dev/null +++ b/edgedbpkg/postgresql/patches/postgresql-edgedb__no_table_rewite_on_domains-16.patch @@ -0,0 +1,98 @@ +From 4ee9a1380c1f8e6060bbbe5888a25eee8c049cf8 Mon Sep 17 00:00:00 2001 +From: Elvis Pranskevichus +Date: Fri, 6 Dec 2024 22:12:07 -0800 +Subject: [PATCH] Avoid table rewrite when adding nullable columns of + constrained domains (#3) + +Postgres has an optimization whereby it skips a full table rewrite if a +default was specified or the column is nullable, however this +optimization is skipped if a domain has constraints defined on it, +because `CHECK` constraints get invoked on NULL values and there is no +way for Postgres to know how that NULL value affects the constraint +without evaluating it. Unfortunatly, there's no direct way of +evaluating domain constraints from that part of `ALTER TABLE` +processing. However, EdgeQL does not support checking for an empty +value in scalar constraints, so we can safely skip the table rewrite on +nullable columns even if there are constraints (but still keep it for +domains marked as `NOT NULL` just in case). +--- + src/backend/commands/tablecmds.c | 15 ++++++++++++--- + src/backend/utils/cache/typcache.c | 15 +++++++++++++++ + src/include/utils/typcache.h | 2 ++ + 3 files changed, 29 insertions(+), 3 deletions(-) + +diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c +index 90a5238c4d4..37c27f395a1 100644 +--- a/src/backend/commands/tablecmds.c ++++ b/src/backend/commands/tablecmds.c +@@ -7092,6 +7092,18 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, + else + defval = (Expr *) build_column_default(rel, attribute.attnum); + ++ /* ++ * EdgeQL does not allow for NULL checks in domain constraints, ++ * so we can safely skip an expensive table rewrite (though keep ++ * it for domains declared as NOT NULL just in case something ++ * somewhere uses them outside of EdgeQL schema). ++ */ ++ if ((!defval && DomainIsStrict(typeOid)) ++ || (defval && DomainHasConstraints(typeOid))) ++ { ++ tab->rewrite |= AT_REWRITE_DEFAULT_VAL; ++ } ++ + if (!defval && DomainHasConstraints(typeOid)) + { + Oid baseTypeId; +@@ -7126,9 +7138,6 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, + tab->newvals = lappend(tab->newvals, newval); + } + +- if (DomainHasConstraints(typeOid)) +- tab->rewrite |= AT_REWRITE_DEFAULT_VAL; +- + if (!TupleDescAttr(rel->rd_att, attribute.attnum - 1)->atthasmissing) + { + /* +diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c +index 608cd5e8e43..c00c6ec12dc 100644 +--- a/src/backend/utils/cache/typcache.c ++++ b/src/backend/utils/cache/typcache.c +@@ -1407,6 +1407,21 @@ DomainHasConstraints(Oid type_id) + return (typentry->domainData != NULL); + } + ++/* ++ * DomainIsStrict --- routine to check if a domain has a NOT NULL constraint ++ */ ++bool ++DomainIsStrict(Oid type_id) ++{ ++ HeapTuple tup; ++ Form_pg_type typTup; ++ ++ tup = SearchSysCacheCopy1(TYPEOID, ObjectIdGetDatum(type_id)); ++ if (!HeapTupleIsValid(tup)) ++ elog(ERROR, "cache lookup failed for type %u", type_id); ++ typTup = (Form_pg_type) GETSTRUCT(tup); ++ return typTup->typnotnull; ++} + + /* + * array_element_has_equality and friends are helper routines to check +diff --git a/src/include/utils/typcache.h b/src/include/utils/typcache.h +index 95f3a9ee308..48ba3836b16 100644 +--- a/src/include/utils/typcache.h ++++ b/src/include/utils/typcache.h +@@ -183,6 +183,8 @@ extern void UpdateDomainConstraintRef(DomainConstraintRef *ref); + + extern bool DomainHasConstraints(Oid type_id); + ++extern bool DomainIsStrict(Oid type_id); ++ + extern TupleDesc lookup_rowtype_tupdesc(Oid type_id, int32 typmod); + + extern TupleDesc lookup_rowtype_tupdesc_noerror(Oid type_id, int32 typmod, +-- +2.45.2 + diff --git a/edgedbpkg/postgresql/patches/postgresql-edgedb__no_table_rewite_on_domains-17.patch b/edgedbpkg/postgresql/patches/postgresql-edgedb__no_table_rewite_on_domains-17.patch new file mode 100644 index 0000000..22b06e9 --- /dev/null +++ b/edgedbpkg/postgresql/patches/postgresql-edgedb__no_table_rewite_on_domains-17.patch @@ -0,0 +1,98 @@ +From f776c770cde68e2fd4002e51599f1480f8265096 Mon Sep 17 00:00:00 2001 +From: Elvis Pranskevichus +Date: Fri, 6 Dec 2024 22:12:07 -0800 +Subject: [PATCH] Avoid table rewrite when adding nullable columns of + constrained domains (#3) + +Postgres has an optimization whereby it skips a full table rewrite if a +default was specified or the column is nullable, however this +optimization is skipped if a domain has constraints defined on it, +because `CHECK` constraints get invoked on NULL values and there is no +way for Postgres to know how that NULL value affects the constraint +without evaluating it. Unfortunatly, there's no direct way of +evaluating domain constraints from that part of `ALTER TABLE` +processing. However, EdgeQL does not support checking for an empty +value in scalar constraints, so we can safely skip the table rewrite on +nullable columns even if there are constraints (but still keep it for +domains marked as `NOT NULL` just in case). +--- + src/backend/commands/tablecmds.c | 15 ++++++++++++--- + src/backend/utils/cache/typcache.c | 15 +++++++++++++++ + src/include/utils/typcache.h | 2 ++ + 3 files changed, 29 insertions(+), 3 deletions(-) + +diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c +index 36717ffcb0a..6300119c1bc 100644 +--- a/src/backend/commands/tablecmds.c ++++ b/src/backend/commands/tablecmds.c +@@ -7291,6 +7291,18 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, + else + defval = (Expr *) build_column_default(rel, attribute->attnum); + ++ /* ++ * EdgeQL does not allow for NULL checks in domain constraints, ++ * so we can safely skip an expensive table rewrite (though keep ++ * it for domains declared as NOT NULL just in case something ++ * somewhere uses them outside of EdgeQL schema). ++ */ ++ if ((!defval && DomainIsStrict(attribute->atttypid)) ++ || (defval && DomainHasConstraints(attribute->atttypid))) ++ { ++ tab->rewrite |= AT_REWRITE_DEFAULT_VAL; ++ } ++ + if (!defval && DomainHasConstraints(attribute->atttypid)) + { + Oid baseTypeId; +@@ -7325,9 +7337,6 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, + tab->newvals = lappend(tab->newvals, newval); + } + +- if (DomainHasConstraints(attribute->atttypid)) +- tab->rewrite |= AT_REWRITE_DEFAULT_VAL; +- + if (!TupleDescAttr(rel->rd_att, attribute->attnum - 1)->atthasmissing) + { + /* +diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c +index aa4720cb598..c539853e056 100644 +--- a/src/backend/utils/cache/typcache.c ++++ b/src/backend/utils/cache/typcache.c +@@ -1410,6 +1410,21 @@ DomainHasConstraints(Oid type_id) + return (typentry->domainData != NULL); + } + ++/* ++ * DomainIsStrict --- routine to check if a domain has a NOT NULL constraint ++ */ ++bool ++DomainIsStrict(Oid type_id) ++{ ++ HeapTuple tup; ++ Form_pg_type typTup; ++ ++ tup = SearchSysCacheCopy1(TYPEOID, ObjectIdGetDatum(type_id)); ++ if (!HeapTupleIsValid(tup)) ++ elog(ERROR, "cache lookup failed for type %u", type_id); ++ typTup = (Form_pg_type) GETSTRUCT(tup); ++ return typTup->typnotnull; ++} + + /* + * array_element_has_equality and friends are helper routines to check +diff --git a/src/include/utils/typcache.h b/src/include/utils/typcache.h +index f506cc4aa35..174a946f79b 100644 +--- a/src/include/utils/typcache.h ++++ b/src/include/utils/typcache.h +@@ -184,6 +184,8 @@ extern void UpdateDomainConstraintRef(DomainConstraintRef *ref); + + extern bool DomainHasConstraints(Oid type_id); + ++extern bool DomainIsStrict(Oid type_id); ++ + extern TupleDesc lookup_rowtype_tupdesc(Oid type_id, int32 typmod); + + extern TupleDesc lookup_rowtype_tupdesc_noerror(Oid type_id, int32 typmod, +-- +2.45.2 +