public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v29 4/6] Track collation versions for indexes. 19+ messages / 8 participants [nested] [flat]
* [PATCH v29 4/6] Track collation versions for indexes. @ 2019-05-28 18:16 Thomas Munro <[email protected]> 0 siblings, 0 replies; 19+ messages in thread From: Thomas Munro @ 2019-05-28 18:16 UTC (permalink / raw) Record the current version of dependent collations in pg_depend when creating or rebuilding an index. The version is checked against the current version whenever we call get_relation_info for an index or open the parent table during non-full VACUUM or ANALYZE. Warn that the index may be corrupted if the versions don't match. Add a new flag is added to RelationData to record that the check has already been done to avoid repetition. Thanks to Doug Doole, Peter Eisentraut, Christoph Berg, Laurenz Albe, Michael Paquier, Robert Haas, Tom Lane and others for design discussion. Author: Thomas Munro <[email protected]> Author: Julien Rouhaud <[email protected]> Reviewed-by: Peter Eisentraut <[email protected]> Reviewed-by: Laurenz Albe <[email protected]> Discussion: https://postgr.es/m/CAEepm%3D0uEQCpfq_%2BLYFBdArCe4Ot98t1aR4eYiYTe%3DyavQygiQ%40mail.gmail.com --- doc/src/sgml/func.sgml | 3 +- doc/src/sgml/ref/pgupgrade.sgml | 18 ++ doc/src/sgml/ref/reindex.sgml | 6 + src/backend/catalog/dependency.c | 218 +++++++++++++-- src/backend/catalog/heap.c | 7 +- src/backend/catalog/index.c | 264 +++++++++++++++++- src/backend/catalog/pg_constraint.c | 2 +- src/backend/catalog/pg_depend.c | 208 +++++++++++++- src/backend/catalog/pg_type.c | 69 +++++ src/backend/commands/collationcmds.c | 22 +- src/backend/commands/vacuum.c | 32 +++ src/backend/optimizer/util/plancat.c | 9 + src/backend/utils/adt/pg_locale.c | 52 +++- src/backend/utils/adt/pg_upgrade_support.c | 25 ++ src/backend/utils/cache/relcache.c | 2 + src/bin/pg_dump/Makefile | 2 + src/bin/pg_dump/pg_backup.h | 1 + src/bin/pg_dump/pg_dump.c | 186 +++++++++++- src/bin/pg_dump/pg_dump.h | 3 + src/bin/pg_dump/t/002_pg_dump.pl | 248 ++++++++++++---- src/bin/pg_upgrade/dump.c | 4 +- src/bin/pg_upgrade/option.c | 7 + src/bin/pg_upgrade/pg_upgrade.h | 2 + src/include/catalog/dependency.h | 31 +- src/include/catalog/index.h | 7 + src/include/catalog/pg_proc.dat | 4 + src/include/catalog/pg_type.h | 2 + src/include/utils/pg_locale.h | 2 +- src/include/utils/rel.h | 1 + src/test/Makefile | 5 +- src/test/locale/.gitignore | 1 + src/test/locale/Makefile | 7 + src/test/locale/t/001_index.pl | 76 +++++ src/test/perl/PostgresNode.pm | 6 +- .../regress/expected/collate.icu.utf8.out | 157 +++++++++++ src/test/regress/expected/create_index.out | 8 +- src/test/regress/sql/collate.icu.utf8.sql | 106 +++++++ 37 files changed, 1655 insertions(+), 148 deletions(-) create mode 100644 src/test/locale/t/001_index.pl diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index ea2be6d819..bcddca822c 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -25483,7 +25483,8 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); </para> <para> Returns the actual version of the collation object as it is currently - installed in the operating system. + installed in the operating system. An empty string is returned if the + version is unknown. </para></entry> </row> diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml index b59c5697a3..527c847e27 100644 --- a/doc/src/sgml/ref/pgupgrade.sgml +++ b/doc/src/sgml/ref/pgupgrade.sgml @@ -215,6 +215,24 @@ PostgreSQL documentation </listitem> </varlistentry> + <varlistentry> + <term><option>--collation-binary-compatible</option></term> + <listitem> + <para> + After upgrading from a release of <productname>PostgreSQL</productname> + before 14, warnings may be reported when collation-dependent + indexes are first accessed. This is because + <application>pg_upgrade</application> records the collation versions + as unknown, so <productname>PostgreSQL</productname> considers the + indexes to be potentially corrupted. If you're certain that the + collation definitions used by your operating system or ICU haven't + changed since all indexes were created, you can use this flag + to record that the indexes match the currently installed collations. + Otherwise, see <xref linkend="sql-reindex"/>. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><option>-?</option></term> <term><option>--help</option></term> diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 33af4ae02a..cbac0284c7 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -39,6 +39,12 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN <itemizedlist> <listitem> + <para> + The index depends on the sort order of a collation, and the definition + of the collation has changed. This can cause an index to fail to + find a key that is present in the index. On some systems, collation + versions are tracked in order to generate warnings when this happens. + </para> <para> An index has become corrupted, and no longer contains valid data. Although in theory this should never happen, in diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index 1a927377e7..d6a3cae303 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -76,6 +76,7 @@ #include "rewrite/rewriteRemove.h" #include "storage/lmgr.h" #include "utils/acl.h" +#include "utils/builtins.h" #include "utils/fmgroids.h" #include "utils/guc.h" #include "utils/lsyscache.h" @@ -136,6 +137,9 @@ typedef struct { ObjectAddresses *addrs; /* addresses being accumulated */ List *rtables; /* list of rangetables to resolve Vars */ + bool track_version; /* whether caller asked to track dependency + * versions */ + NodeTag type; /* nodetag of the current node */ } find_expr_references_context; /* @@ -434,6 +438,81 @@ performMultipleDeletions(const ObjectAddresses *objects, table_close(depRel, RowExclusiveLock); } +/* + * Call a function for all objects that depend on 'object'. If the function + * returns a non-NULL pointer to a new version string, update the version. + */ +void +visitDependentObjects(const ObjectAddress *object, + VisitDependentObjectsFun callback, + void *userdata) +{ + Relation depRel; + ScanKeyData key[3]; + SysScanDesc scan; + HeapTuple tup; + ObjectAddress otherObject; + + ScanKeyInit(&key[0], + Anum_pg_depend_classid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(object->classId)); + ScanKeyInit(&key[1], + Anum_pg_depend_objid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(object->objectId)); + ScanKeyInit(&key[2], + Anum_pg_depend_objsubid, + BTEqualStrategyNumber, F_INT4EQ, + Int32GetDatum(object->objectSubId)); + + depRel = table_open(DependRelationId, RowExclusiveLock); + scan = systable_beginscan(depRel, DependDependerIndexId, true, + NULL, 3, key); + + while (HeapTupleIsValid(tup = systable_getnext(scan))) + { + Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(tup); + char *cur_version, + *new_version; + Datum depversion; + bool isnull; + + otherObject.classId = foundDep->refclassid; + otherObject.objectId = foundDep->refobjid; + otherObject.objectSubId = foundDep->refobjsubid; + + depversion = heap_getattr(tup, Anum_pg_depend_refobjversion, + RelationGetDescr(depRel), &isnull); + + cur_version = isnull ? NULL : TextDatumGetCString(depversion); + + new_version = callback(&otherObject, cur_version, userdata); + if (new_version) + { + Datum values[Natts_pg_depend]; + bool nulls[Natts_pg_depend]; + bool replaces[Natts_pg_depend]; + + memset(values, 0, sizeof(values)); + memset(nulls, false, sizeof(nulls)); + memset(replaces, false, sizeof(replaces)); + + values[Anum_pg_depend_refobjversion - 1] = + CStringGetTextDatum(new_version); + replaces[Anum_pg_depend_refobjversion - 1] = true; + + tup = heap_modify_tuple(tup, RelationGetDescr(depRel), values, + nulls, replaces); + CatalogTupleUpdate(depRel, &tup->t_self, tup); + + heap_freetuple(tup); + } + } + systable_endscan(scan); + table_close(depRel, RowExclusiveLock); +} + /* * findDependentObjects - find all objects that depend on 'object' * @@ -1566,6 +1645,36 @@ ReleaseDeletionLock(const ObjectAddress *object) AccessExclusiveLock); } +/* + * Given a list of collations, record a dependency on its underlying collation + * version. + */ +void +recordDependencyOnCollations(ObjectAddress *myself, + List *collations, + bool record_version) +{ + ObjectAddresses *addrs; + ListCell *lc; + + if (list_length(collations) == 0) + return; + + addrs = new_object_addresses(); + + foreach(lc, collations) + { + ObjectAddress referenced; + + ObjectAddressSet(referenced, CollationRelationId, lfirst_oid(lc)); + + add_exact_object_address(&referenced, addrs); + } + + recordMultipleDependencies(myself, addrs->refs, addrs->numrefs, + DEPENDENCY_NORMAL, record_version); +} + /* * recordDependencyOnExpr - find expression dependencies * @@ -1588,6 +1697,10 @@ recordDependencyOnExpr(const ObjectAddress *depender, find_expr_references_context context; context.addrs = new_object_addresses(); + if (expr) + context.type = expr->type; + else + context.type = T_Invalid; /* Set up interpretation for Vars at varlevelsup = 0 */ context.rtables = list_make1(rtable); @@ -1602,8 +1715,8 @@ recordDependencyOnExpr(const ObjectAddress *depender, recordMultipleDependencies(depender, context.addrs->refs, context.addrs->numrefs, - NULL, - behavior); + behavior, + false); free_object_addresses(context.addrs); } @@ -1630,12 +1743,18 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender, Node *expr, Oid relId, DependencyType behavior, DependencyType self_behavior, - bool reverse_self) + bool reverse_self, + bool track_version) { find_expr_references_context context; RangeTblEntry rte; context.addrs = new_object_addresses(); + context.track_version = track_version; + if (expr) + context.type = expr->type; + else + context.type = T_Invalid; /* We gin up a rather bogus rangetable list to handle Vars */ MemSet(&rte, 0, sizeof(rte)); @@ -1691,8 +1810,8 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender, recordMultipleDependencies(depender, self_addrs->refs, self_addrs->numrefs, - NULL, - self_behavior); + self_behavior, + track_version); else { /* Can't use recordMultipleDependencies, so do it the hard way */ @@ -1713,8 +1832,8 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender, recordMultipleDependencies(depender, context.addrs->refs, context.addrs->numrefs, - NULL, - behavior); + behavior, + track_version); free_object_addresses(context.addrs); } @@ -1736,8 +1855,13 @@ static bool find_expr_references_walker(Node *node, find_expr_references_context *context) { + NodeTag parent_type = context->type; + if (node == NULL) return false; + + context->type = node->type; + if (IsA(node, Var)) { Var *var = (Var *) node; @@ -1770,6 +1894,49 @@ find_expr_references_walker(Node *node, /* If it's a plain relation, reference this column */ add_object_address(OCLASS_CLASS, rte->relid, var->varattno, context->addrs); + + /* + * Record collations from the type itself, or underlying in case + * of complex type. Note that if the direct parent is a + * CollateExpr node, there's no need to record the type underlying + * collation if any. A dependency already exists for the owning + * relation, and a change in the collation sort order wouldn't + * cause any harm as the collation isn't used at all in such case. + */ + if (parent_type != T_CollateExpr) + { + /* type's collation if valid */ + if (OidIsValid(var->varcollid)) + { + add_object_address(OCLASS_COLLATION, var->varcollid, 0, + context->addrs); + } + + /* + * otherwise, it may be a composite type having underlying + * collations + */ + else if (var->vartype >= FirstNormalObjectId) + { + List *collations = NIL; + ListCell *lc; + + collations = GetTypeCollations(var->vartype, false); + + foreach(lc, collations) + { + Oid coll = lfirst_oid(lc); + + if (OidIsValid(coll) && + (coll != DEFAULT_COLLATION_OID || + context->track_version) + ) + add_object_address(OCLASS_COLLATION, + lfirst_oid(lc), 0, + context->addrs); + } + } + } } /* @@ -1795,10 +1962,12 @@ find_expr_references_walker(Node *node, * We must also depend on the constant's collation: it could be * different from the datatype's, if a CollateExpr was const-folded to * a simple constant. However we can save work in the most common - * case where the collation is "default", since we know that's pinned. + * case where the collation is "default", since we know that's pinned, + * if the caller didn't ask to track dependency versions. */ if (OidIsValid(con->constcollid) && - con->constcollid != DEFAULT_COLLATION_OID) + (con->constcollid != DEFAULT_COLLATION_OID || + context->track_version)) add_object_address(OCLASS_COLLATION, con->constcollid, 0, context->addrs); @@ -1888,7 +2057,8 @@ find_expr_references_walker(Node *node, context->addrs); /* and its collation, just as for Consts */ if (OidIsValid(param->paramcollid) && - param->paramcollid != DEFAULT_COLLATION_OID) + (param->paramcollid != DEFAULT_COLLATION_OID || + context->track_version)) add_object_address(OCLASS_COLLATION, param->paramcollid, 0, context->addrs); } @@ -1976,7 +2146,8 @@ find_expr_references_walker(Node *node, context->addrs); /* the collation might not be referenced anywhere else, either */ if (OidIsValid(fselect->resultcollid) && - fselect->resultcollid != DEFAULT_COLLATION_OID) + (fselect->resultcollid != DEFAULT_COLLATION_OID || + context->track_version)) add_object_address(OCLASS_COLLATION, fselect->resultcollid, 0, context->addrs); } @@ -2007,7 +2178,8 @@ find_expr_references_walker(Node *node, context->addrs); /* the collation might not be referenced anywhere else, either */ if (OidIsValid(relab->resultcollid) && - relab->resultcollid != DEFAULT_COLLATION_OID) + (relab->resultcollid != DEFAULT_COLLATION_OID || + context->track_version)) add_object_address(OCLASS_COLLATION, relab->resultcollid, 0, context->addrs); } @@ -2020,7 +2192,8 @@ find_expr_references_walker(Node *node, context->addrs); /* the collation might not be referenced anywhere else, either */ if (OidIsValid(iocoerce->resultcollid) && - iocoerce->resultcollid != DEFAULT_COLLATION_OID) + (iocoerce->resultcollid != DEFAULT_COLLATION_OID || + context->track_version)) add_object_address(OCLASS_COLLATION, iocoerce->resultcollid, 0, context->addrs); } @@ -2033,7 +2206,8 @@ find_expr_references_walker(Node *node, context->addrs); /* the collation might not be referenced anywhere else, either */ if (OidIsValid(acoerce->resultcollid) && - acoerce->resultcollid != DEFAULT_COLLATION_OID) + (acoerce->resultcollid != DEFAULT_COLLATION_OID || + context->track_version)) add_object_address(OCLASS_COLLATION, acoerce->resultcollid, 0, context->addrs); /* fall through to examine arguments */ @@ -2122,7 +2296,8 @@ find_expr_references_walker(Node *node, add_object_address(OCLASS_PROC, wc->endInRangeFunc, 0, context->addrs); if (OidIsValid(wc->inRangeColl) && - wc->inRangeColl != DEFAULT_COLLATION_OID) + (wc->inRangeColl != DEFAULT_COLLATION_OID || + context->track_version)) add_object_address(OCLASS_COLLATION, wc->inRangeColl, 0, context->addrs); /* fall through to examine substructure */ @@ -2267,7 +2442,9 @@ find_expr_references_walker(Node *node, { Oid collid = lfirst_oid(ct); - if (OidIsValid(collid) && collid != DEFAULT_COLLATION_OID) + if (OidIsValid(collid) && + (collid != DEFAULT_COLLATION_OID || + context->track_version)) add_object_address(OCLASS_COLLATION, collid, 0, context->addrs); } @@ -2289,7 +2466,9 @@ find_expr_references_walker(Node *node, { Oid collid = lfirst_oid(ct); - if (OidIsValid(collid) && collid != DEFAULT_COLLATION_OID) + if (OidIsValid(collid) && + (collid != DEFAULT_COLLATION_OID || + context->track_version)) add_object_address(OCLASS_COLLATION, collid, 0, context->addrs); } @@ -2685,8 +2864,9 @@ record_object_address_dependencies(const ObjectAddress *depender, { eliminate_duplicate_dependencies(referenced); recordMultipleDependencies(depender, - referenced->refs, referenced->numrefs, NULL, - behavior); + referenced->refs, referenced->numrefs, + behavior, + false); } /* diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 67144aa3c9..fc5140daa6 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2336,7 +2336,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum, */ recordDependencyOnSingleRelExpr(&colobject, expr, RelationGetRelid(rel), DEPENDENCY_AUTO, - DEPENDENCY_AUTO, false); + DEPENDENCY_AUTO, false, false); } else { @@ -2346,7 +2346,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum, */ recordDependencyOnSingleRelExpr(&defobject, expr, RelationGetRelid(rel), DEPENDENCY_NORMAL, - DEPENDENCY_NORMAL, false); + DEPENDENCY_NORMAL, false, false); } /* @@ -3665,7 +3665,8 @@ StorePartitionKey(Relation rel, RelationGetRelid(rel), DEPENDENCY_NORMAL, DEPENDENCY_INTERNAL, - true /* reverse the self-deps */ ); + true /* reverse the self-deps */ , + false /* don't track versions */ ); /* * We must invalidate the relcache so that the next diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 117e3fdef7..a30e83ca39 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -53,6 +53,7 @@ #include "catalog/pg_trigger.h" #include "catalog/pg_type.h" #include "catalog/storage.h" +#include "commands/defrem.h" #include "commands/event_trigger.h" #include "commands/progress.h" #include "commands/tablecmds.h" @@ -75,6 +76,7 @@ #include "utils/guc.h" #include "utils/inval.h" #include "utils/lsyscache.h" +#include "utils/pg_locale.h" #include "utils/memutils.h" #include "utils/pg_rusage.h" #include "utils/rel.h" @@ -1020,6 +1022,10 @@ index_create(Relation heapRelation, ObjectAddress myself, referenced; ObjectAddresses *addrs; + ListCell *lc; + List *colls = NIL, + *determ_colls = NIL, + *nondeterm_colls = NIL; ObjectAddressSet(myself, RelationRelationId, indexRelationId); @@ -1106,20 +1112,80 @@ index_create(Relation heapRelation, /* placeholder for normal dependencies */ addrs = new_object_addresses(); - /* Store dependency on collations */ - - /* The default collation is pinned, so don't bother recording it */ + /* First, get all dependencies on collations for all index keys. */ for (i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++) { - if (OidIsValid(collationObjectId[i]) && - collationObjectId[i] != DEFAULT_COLLATION_OID) + Oid colloid = collationObjectId[i]; + + if (OidIsValid(colloid)) + colls = lappend_oid(colls, colloid); + else { - ObjectAddressSet(referenced, CollationRelationId, - collationObjectId[i]); - add_exact_object_address(&referenced, addrs); + Form_pg_attribute att = TupleDescAttr(indexTupDesc, i); + + Assert(i < indexTupDesc->natts); + + colls = list_concat(colls, + GetTypeCollations(att->atttypid, false)); } } + /* + * Then split the dependencies on whether they're deterministic or + * not, removing any duplicates. + */ + foreach(lc, colls) + { + Oid c = lfirst_oid(lc); + + if (!OidIsValid(c)) + continue; + + if (get_collation_isdeterministic(c)) + determ_colls = list_append_unique_oid(determ_colls, c); + else + nondeterm_colls = list_append_unique_oid(nondeterm_colls, c); + } + + /* + * For deterministic collation, we always track the dependency on the + * collation but only track the version if the AM relies on a stable + * ordering. + */ + if (determ_colls) + { + IndexAmRoutine *routine; + bool track_version; + + routine = GetIndexAmRoutineByAmId(accessMethodObjectId, false); + track_version = !routine->amnostablecollorder; + + recordDependencyOnCollations(&myself, determ_colls, track_version); + + /* + * Advance the command counter so that later calls to + * recordMultipleDependencies calls can see the newly-entered + * pg_depend catalog tuples for the index. + */ + CommandCounterIncrement(); + } + + /* + * We always record the version for dependency on non-deterministic + * collations. + */ + if (nondeterm_colls) + { + recordDependencyOnCollations(&myself, nondeterm_colls, true); + + /* + * Advance the command counter so that later calls to + * recordMultipleDependencies calls can see the newly-entered + * pg_depend catalog tuples for the index. + */ + CommandCounterIncrement(); + } + /* Store dependency on operator classes */ for (i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++) { @@ -1133,21 +1199,32 @@ index_create(Relation heapRelation, /* Store dependencies on anything mentioned in index expressions */ if (indexInfo->ii_Expressions) { + /* + * recordDependencyOnSingleRelExpr gets rid of duplicated entries + */ recordDependencyOnSingleRelExpr(&myself, (Node *) indexInfo->ii_Expressions, heapRelationId, DEPENDENCY_NORMAL, - DEPENDENCY_AUTO, false); + DEPENDENCY_AUTO, false, true); + + /* + * Advance the command counter so that later + * recordMultipleDependencies calls can see the newly-entered + * pg_depend catalog tuples for the index. + */ + CommandCounterIncrement(); } /* Store dependencies on anything mentioned in predicate */ if (indexInfo->ii_Predicate) { + /* recordDependencyOnSingleRelExpr get rid of duplicated entries */ recordDependencyOnSingleRelExpr(&myself, (Node *) indexInfo->ii_Predicate, heapRelationId, DEPENDENCY_NORMAL, - DEPENDENCY_AUTO, false); + DEPENDENCY_AUTO, false, true); } } else @@ -1226,6 +1303,108 @@ index_create(Relation heapRelation, return indexRelationId; } +/* index_check_collation_version + * Raise a warning if the recorded and current collation version don't + * match. +*/ +static char * +index_check_collation_version(const ObjectAddress *otherObject, + const char *version, + void *userdata) +{ + Oid relid = *(Oid *) userdata; + char *current_version; + + /* We only care about dependencies on collations. */ + if (otherObject->classId != CollationRelationId) + return NULL; + + /* Compare with the current version. */ + current_version = get_collation_version_for_oid(otherObject->objectId); + + /* XXX should we warn about "disappearing" versions? */ + if (current_version) + { + /* + * We now support versioning for the underlying collation library on + * this system, or previous version is unknown. + */ + if (!version || (strcmp(version, "") == 0 && strcmp(current_version, + "") != 0)) + { + ereport(WARNING, + (errmsg("index \"%s\" depends on collation \"%s\" with an unknown version, and the current version is \"%s\"", + get_rel_name(relid), + get_collation_name(otherObject->objectId), + current_version), + errdetail("The index may be corrupted due to changes in sort order."), + errhint("REINDEX to avoid the risk of corruption."))); + } + else if (strcmp(current_version, version) != 0) + { + ereport(WARNING, + (errmsg("index \"%s\" depends on collation \"%s\" version \"%s\", but the current version is \"%s\"", + get_rel_name(relid), + get_collation_name(otherObject->objectId), + version, + current_version), + errdetail("The index may be corrupted due to changes in sort order."), + errhint("REINDEX to avoid the risk of corruption."))); + } + } + + return NULL; +} + +/* index_check_collation_versions + * Check the collation version for all dependencies on the given object. + */ +void +index_check_collation_versions(Oid relid) +{ + ObjectAddress object; + + object.classId = RelationRelationId; + object.objectId = relid; + object.objectSubId = 0; + visitDependentObjects(&object, &index_check_collation_version, &relid); +} + +/* index_update_collation_version + * Return the current collation version for the given object. + */ +static char * +index_update_collation_version(const ObjectAddress *otherObject, + const char *version, + void *userdata) +{ + char *current_version = (char *) userdata; + + /* We only care about dependencies on collations. */ + if (otherObject->classId != CollationRelationId) + return NULL; + + current_version = get_collation_version_for_oid(otherObject->objectId); + return current_version; +} + +/* index_update_collation_versions + * Record the current collation versions of all dependencies on the given + * object. + */ +void +index_update_collation_versions(Oid relid) +{ + ObjectAddress object; + NameData current_version; + + object.classId = RelationRelationId; + object.objectId = relid; + object.objectSubId = 0; + visitDependentObjects(&object, &index_update_collation_version, + ¤t_version); +} + /* * index_concurrently_create_copy * @@ -3000,6 +3179,68 @@ index_build(Relation heapRelation, SetUserIdAndSecContext(save_userid, save_sec_context); } +static char * +index_force_collation_version(const ObjectAddress *otherObject, + const char *version, + void *userdata) +{ + NewCollationVersionDependency *forced_dependency; + + forced_dependency = (NewCollationVersionDependency *) userdata; + + /* We only care about dependencies on collations. */ + if (otherObject->classId != CollationRelationId) + return NULL; + + /* + * We only care about dependencies on a specific collation if a valid Oid + * was given. + */ + if (OidIsValid(forced_dependency->oid) && + otherObject->objectId != forced_dependency->oid) + return NULL; + + return forced_dependency->version; +} + +/* index_force_collation_versions + * + * Override collation version dependencies for the given index. If no + * collation identifier is specified, all dependent collation should be + * reset to an unknown version dependency, and no version should be provided + * either. + */ +void +index_force_collation_versions(Oid indexid, Oid coll, char *version) +{ + Relation index; + ObjectAddress object; + NewCollationVersionDependency forced_dependency; + + Assert(version); + + index = relation_open(indexid, AccessExclusiveLock); + + if (index->rd_rel->relkind != RELKIND_INDEX) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + (errmsg("\"%s\" is not an index", get_rel_name(indexid))))); + + forced_dependency.oid = InvalidOid; + forced_dependency.version = version; + + object.classId = RelationRelationId; + object.objectId = indexid; + object.objectSubId = 0; + visitDependentObjects(&object, &index_force_collation_version, + &forced_dependency); + + /* Invalidate the index relcache */ + CacheInvalidateRelcache(index); + + relation_close(index, NoLock); +} + /* * IndexCheckExclusion - verify that a new exclusion constraint is satisfied * @@ -3646,6 +3887,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence, /* Close rels, but keep locks */ index_close(iRel, NoLock); table_close(heapRelation, NoLock); + + /* Record the current versions of all depended-on collations. */ + index_update_collation_versions(indexId); } /* diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 0d70cb0c3c..93774c9d21 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -362,7 +362,7 @@ CreateConstraintEntry(const char *constraintName, */ recordDependencyOnSingleRelExpr(&conobject, conExpr, relId, DEPENDENCY_NORMAL, - DEPENDENCY_NORMAL, false); + DEPENDENCY_NORMAL, false, true); } /* Post creation hook for new constraint */ diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c index 09c30b13e8..2bd9401e6a 100644 --- a/src/backend/catalog/pg_depend.c +++ b/src/backend/catalog/pg_depend.c @@ -19,6 +19,7 @@ #include "access/table.h" #include "catalog/dependency.h" #include "catalog/indexing.h" +#include "catalog/pg_collation.h" #include "catalog/pg_constraint.h" #include "catalog/pg_depend.h" #include "catalog/pg_extension.h" @@ -27,9 +28,14 @@ #include "utils/builtins.h" #include "utils/fmgroids.h" #include "utils/lsyscache.h" +#include "utils/pg_locale.h" #include "utils/rel.h" +static bool dependencyExists(const ObjectAddress *depender, + const ObjectAddress *referenced); +static char *getDependencyVersion(const ObjectAddress *depender, + const ObjectAddress *referenced); static bool isObjectPinned(const ObjectAddress *object, Relation rel); @@ -45,19 +51,24 @@ recordDependencyOn(const ObjectAddress *depender, const ObjectAddress *referenced, DependencyType behavior) { - recordMultipleDependencies(depender, referenced, 1, NULL, behavior); + recordMultipleDependencies(depender, referenced, 1, behavior, false); } /* * Record multiple dependencies (of the same kind) for a single dependent * object. This has a little less overhead than recording each separately. + * + * If track_version is true, then a record could be added even if the referenced + * dependency is pinned, and the dependency version will be retrieved according + * to the referenced object kind. + * For now, only collation version is supported. */ void recordMultipleDependencies(const ObjectAddress *depender, const ObjectAddress *referenced, int nreferenced, - const char *version, - DependencyType behavior) + DependencyType behavior, + bool track_version) { Relation dependDesc; CatalogIndexState indstate; @@ -66,6 +77,7 @@ recordMultipleDependencies(const ObjectAddress *depender, max_slots, slot_init_count, slot_stored_count; + char *version = NULL; if (nreferenced <= 0) return; /* nothing to do */ @@ -96,12 +108,49 @@ recordMultipleDependencies(const ObjectAddress *depender, slot_init_count = 0; for (i = 0; i < nreferenced; i++, referenced++) { + bool ignore_systempin = false; + + if (track_version) + { + /* Only dependency on a collation is supported. */ + if (referenced->classId == CollationRelationId) + { + /* C and POSIX collations don't require tracking the version */ + if (referenced->objectId == C_COLLATION_OID || + referenced->objectId == POSIX_COLLATION_OID) + continue; + + /* + * We don't want to record redundant dependencies that are used + * to track versions to avoid redundant warnings in case of + * non-matching versions when those are checked. Note that + * callers have to take care of removing duplicated entries + * and calling CommandCounterIncrement() if the dependencies + * are registered in multiple calls. + */ + if (dependencyExists(depender, referenced)) + continue; + + /* + * Default collation is pinned, so we need to force recording + * the dependency to store the version. + */ + if (referenced->objectId == DEFAULT_COLLATION_OID) + ignore_systempin = true; + version = get_collation_version_for_oid(referenced->objectId); + Assert(version); + } + } + else + Assert(!version); + /* * If the referenced object is pinned by the system, there's no real - * need to record dependencies on it. This saves lots of space in - * pg_depend, so it's worth the time taken to check. + * need to record dependencies on it, unless we need to record a + * version. This saves lots of space in pg_depend, so it's worth the + * time taken to check. */ - if (isObjectPinned(referenced, dependDesc)) + if (!ignore_systempin && isObjectPinned(referenced, dependDesc)) continue; if (slot_init_count < max_slots) @@ -493,7 +542,8 @@ changeDependencyFor(Oid classId, Oid objectId, } /* - * Adjust all dependency records to come from a different object of the same type + * Adjust all dependency records to come from a different object of the same + * type, optionally preserving the original referenced version. * * classId/oldObjectId specify the old referencing object. * newObjectId is the new referencing object (must be of class classId). @@ -501,8 +551,7 @@ changeDependencyFor(Oid classId, Oid objectId, * Returns the number of records updated. */ long -changeDependenciesOf(Oid classId, Oid oldObjectId, - Oid newObjectId) +changeDependenciesOf(Oid classId, Oid oldObjectId, Oid newObjectId) { long count = 0; Relation depRel; @@ -526,14 +575,47 @@ changeDependenciesOf(Oid classId, Oid oldObjectId, while (HeapTupleIsValid((tup = systable_getnext(scan)))) { - Form_pg_depend depform; + Form_pg_depend depform = (Form_pg_depend) GETSTRUCT(tup); + Datum values[Natts_pg_depend]; + bool nulls[Natts_pg_depend]; + bool replaces[Natts_pg_depend]; + bool isnull = true; + + memset(values, 0, sizeof(values)); + memset(nulls, 0, sizeof(nulls)); + memset(replaces, 0, sizeof(replaces)); + + values[Anum_pg_depend_objid - 1] = newObjectId; + replaces[Anum_pg_depend_objid - 1] = true; - /* make a modifiable copy */ - tup = heap_copytuple(tup); - depform = (Form_pg_depend) GETSTRUCT(tup); + /* + * We assume that a version would exist for both the old and new + * object or none. + */ + heap_getattr(tup, Anum_pg_depend_refobjversion, + RelationGetDescr(depRel), &isnull); - depform->objid = newObjectId; + if (!isnull) + { + ObjectAddress depender, + referenced; + char *version; + + ObjectAddressSubSet(depender, depform->classid, + newObjectId, depform->objsubid); + ObjectAddressSubSet(referenced, depform->refclassid, + depform->refobjid, depform->refobjsubid); + + version = getDependencyVersion(&depender, &referenced); + if (version) + values[Anum_pg_depend_refobjversion - 1] = CStringGetTextDatum(version); + else + nulls[Anum_pg_depend_refobjversion - 1] = true; + replaces[Anum_pg_depend_refobjversion - 1] = true; + } + tup = heap_modify_tuple(tup, RelationGetDescr(depRel), values, + nulls, replaces); CatalogTupleUpdate(depRel, &tup->t_self, tup); heap_freetuple(tup); @@ -636,6 +718,104 @@ changeDependenciesOn(Oid refClassId, Oid oldRefObjectId, return count; } +/* dependencyExists() + * + * Test if a record exists for the given depender and referenced addresses. + */ +static bool +dependencyExists(const ObjectAddress *depender, + const ObjectAddress *referenced) +{ + Relation depRel; + ScanKeyData key[3]; + SysScanDesc scan; + HeapTuple tup; + bool ret = false; + + ScanKeyInit(&key[0], + Anum_pg_depend_classid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(depender->classId)); + ScanKeyInit(&key[1], + Anum_pg_depend_objid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(depender->objectId)); + ScanKeyInit(&key[2], + Anum_pg_depend_objsubid, + BTEqualStrategyNumber, F_INT4EQ, + Int32GetDatum(depender->objectSubId)); + + depRel = table_open(DependRelationId, RowExclusiveLock); + scan = systable_beginscan(depRel, DependDependerIndexId, true, + NULL, 3, key); + + while (HeapTupleIsValid(tup = systable_getnext(scan))) + { + Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(tup); + + if (foundDep->refclassid == referenced->classId && + foundDep->refobjid == referenced->objectId && + foundDep->refobjsubid == referenced->objectSubId) + { + ret = true; + break; + } + } + systable_endscan(scan); + table_close(depRel, RowExclusiveLock); + + return ret; +} + +static char * +getDependencyVersion(const ObjectAddress *depender, + const ObjectAddress *referenced) +{ + Relation depRel; + ScanKeyData key[3]; + SysScanDesc scan; + HeapTuple tup; + Datum depversion; + char *version = NULL; + + ScanKeyInit(&key[0], + Anum_pg_depend_classid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(depender->classId)); + ScanKeyInit(&key[1], + Anum_pg_depend_objid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(depender->objectId)); + ScanKeyInit(&key[2], + Anum_pg_depend_objsubid, + BTEqualStrategyNumber, F_INT4EQ, + Int32GetDatum(depender->objectSubId)); + + depRel = table_open(DependRelationId, RowExclusiveLock); + scan = systable_beginscan(depRel, DependDependerIndexId, true, + NULL, 3, key); + + while (HeapTupleIsValid(tup = systable_getnext(scan))) + { + Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(tup); + bool isnull; + + if (foundDep->refclassid == referenced->classId && + foundDep->refobjid == referenced->objectId && + foundDep->refobjsubid == referenced->objectSubId) + { + depversion = heap_getattr(tup, Anum_pg_depend_refobjversion, + RelationGetDescr(depRel), &isnull); + version = isnull ? NULL : TextDatumGetCString(depversion); + break; + } + } + systable_endscan(scan); + table_close(depRel, RowExclusiveLock); + + return version; +} + /* * isObjectPinned() * diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c index 0b04dff773..f2af6212e5 100644 --- a/src/backend/catalog/pg_type.c +++ b/src/backend/catalog/pg_type.c @@ -15,6 +15,7 @@ #include "postgres.h" #include "access/htup_details.h" +#include "access/relation.h" #include "access/table.h" #include "access/xact.h" #include "catalog/binary_upgrade.h" @@ -512,6 +513,74 @@ TypeCreate(Oid newTypeOid, return address; } +/* Get a list of all distinct collations oid that the given type depends on. */ +List * +GetTypeCollations(Oid typeoid, bool non_deterministic_only) +{ + List *result = NIL; + HeapTuple tuple; + Form_pg_type typeTup; + + tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(typeoid)); + if (!HeapTupleIsValid(tuple)) + elog(ERROR, "cache lookup failed for type %u", typeoid); + typeTup = (Form_pg_type) GETSTRUCT(tuple); + + if (OidIsValid(typeTup->typcollation)) + { + if (!non_deterministic_only || + !get_collation_isdeterministic(typeTup->typcollation)) + result = list_append_unique_oid(result, typeTup->typcollation); + } + else if (typeTup->typtype == TYPTYPE_COMPOSITE) + { + Relation rel = relation_open(typeTup->typrelid, AccessShareLock); + TupleDesc desc = RelationGetDescr(rel); + + for (int i = 0; i < RelationGetNumberOfAttributes(rel); i++) + { + Form_pg_attribute att = TupleDescAttr(desc, i); + + if (OidIsValid(att->attcollation)) + { + if (!non_deterministic_only || + !get_collation_isdeterministic(att->attcollation)) + result = list_append_unique_oid(result, att->attcollation); + } + else + result = list_concat_unique_oid(result, + GetTypeCollations(att->atttypid, + non_deterministic_only)); + } + + relation_close(rel, NoLock); + } + else if (typeTup->typtype == TYPTYPE_DOMAIN) + { + Assert(OidIsValid(typeTup->typbasetype)); + + result = list_concat_unique_oid(result, + GetTypeCollations(typeTup->typbasetype, + non_deterministic_only)); + } + else if (typeTup->typtype == TYPTYPE_RANGE) + { + Oid rangeid = get_range_subtype(typeTup->oid); + + Assert(OidIsValid(rangeid)); + + result = list_concat_unique_oid(result, + GetTypeCollations(rangeid, non_deterministic_only)); + } + else if (OidIsValid(typeTup->typelem)) + result = list_concat_unique_oid(result, + GetTypeCollations(typeTup->typelem, non_deterministic_only)); + + ReleaseSysCache(tuple); + + return result; +} + /* * GenerateTypeDependencies: build the dependencies needed for a type * diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c index 5ad8886e60..519f7a7df3 100644 --- a/src/backend/commands/collationcmds.c +++ b/src/backend/commands/collationcmds.c @@ -270,28 +270,12 @@ Datum pg_collation_actual_version(PG_FUNCTION_ARGS) { Oid collid = PG_GETARG_OID(0); - HeapTuple tp; - char *collcollate; - char collprovider; char *version; - tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(collid)); - if (!HeapTupleIsValid(tp)) - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("collation with OID %u does not exist", collid))); - - collcollate = pstrdup(NameStr(((Form_pg_collation) GETSTRUCT(tp))->collcollate)); - collprovider = ((Form_pg_collation) GETSTRUCT(tp))->collprovider; - - ReleaseSysCache(tp); - - version = get_collation_actual_version(collprovider, collcollate); + version = get_collation_version_for_oid(collid); + Assert(version); - if (version) - PG_RETURN_TEXT_P(cstring_to_text(version)); - else - PG_RETURN_NULL(); + PG_RETURN_TEXT_P(cstring_to_text(version)); } diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index ddeec870d8..e1016609dc 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -31,6 +31,8 @@ #include "access/tableam.h" #include "access/transam.h" #include "access/xact.h" +#include "catalog/catalog.h" +#include "catalog/index.h" #include "catalog/namespace.h" #include "catalog/pg_database.h" #include "catalog/pg_inherits.h" @@ -632,6 +634,36 @@ vacuum_open_relation(Oid relid, RangeVar *relation, int options, rel_lock = false; } + /* + * Perform version sanity checks on the relation underlying indexes if + * it's not a VACUUM FULL, as VACUUM FULL will recreate the index and + * update the recorded collation version. + */ + if (!(options & VACOPT_FULL) && onerel && !IsSystemRelation(onerel) && + onerel->rd_rel->relhasindex) + { + List *indexoidlist; + ListCell *l; + + indexoidlist = RelationGetIndexList(onerel); + foreach(l, indexoidlist) + { + Oid indexoid = lfirst_oid(l); + Relation indexRelation; + + indexRelation = index_open(indexoid, AccessShareLock); + + /* Warn if any dependent collations' versions have moved. */ + if (!indexRelation->rd_version_checked) + { + index_check_collation_versions(indexoid); + indexRelation->rd_version_checked = true; + } + + index_close(indexRelation, NoLock); + } + } + /* if relation is opened, leave */ if (onerel) return onerel; diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index f9d0d67aa7..3a20dcdee5 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -28,6 +28,7 @@ #include "catalog/catalog.h" #include "catalog/dependency.h" #include "catalog/heap.h" +#include "catalog/index.h" #include "catalog/pg_am.h" #include "catalog/pg_proc.h" #include "catalog/pg_statistic_ext.h" @@ -198,6 +199,14 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent, indexRelation = index_open(indexoid, lmode); index = indexRelation->rd_index; + /* Warn if any dependent collations' versions have moved. */ + if (!IsSystemRelation(relation) && + !indexRelation->rd_version_checked) + { + index_check_collation_versions(indexoid); + indexRelation->rd_version_checked = true; + } + /* * Ignore invalid indexes, since they can't safely be used for * queries. Note that this is OK because the data structure we diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index 514e0fa0af..7e5fcb77df 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -57,7 +57,9 @@ #include "access/htup_details.h" #include "catalog/pg_collation.h" #include "catalog/pg_control.h" +#include "catalog/pg_database.h" #include "mb/pg_wchar.h" +#include "miscadmin.h" #include "utils/builtins.h" #include "utils/formatting.h" #include "utils/hsearch.h" @@ -139,6 +141,9 @@ static char *IsoLocaleName(const char *); /* MSVC specific */ static void icu_set_collation_attributes(UCollator *collator, const char *loc); #endif +static char *get_collation_actual_version(char collprovider, + const char *collcollate); + /* * pg_perm_setlocale * @@ -1630,7 +1635,7 @@ pg_newlocale_from_collation(Oid collid) * Get provider-specific collation version string for the given collation from * the operating system/library. */ -char * +static char * get_collation_actual_version(char collprovider, const char *collcollate) { char *collversion = NULL; @@ -1712,6 +1717,51 @@ get_collation_actual_version(char collprovider, const char *collcollate) return collversion; } +/* + * Get provider-specific collation version string given a collation OID. + * + * As this version is used in index dependency tracking, an empty string is + * returned when the version is unknown, to distinguish from dependencies where + * the version should not be tracked, represented by a NULL version. + */ +char * +get_collation_version_for_oid(Oid oid) +{ + HeapTuple tp; + char *version = NULL; + + Assert(oid != C_COLLATION_OID && oid != POSIX_COLLATION_OID); + + if (oid == DEFAULT_COLLATION_OID) + { + Form_pg_database dbform; + + tp = SearchSysCache1(DATABASEOID, ObjectIdGetDatum(MyDatabaseId)); + if (!HeapTupleIsValid(tp)) + elog(ERROR, "cache lookup failed for database %u", MyDatabaseId); + dbform = (Form_pg_database) GETSTRUCT(tp); + version = get_collation_actual_version(COLLPROVIDER_LIBC, + NameStr(dbform->datcollate)); + } + else + { + Form_pg_collation collform; + + tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(oid)); + if (!HeapTupleIsValid(tp)) + elog(ERROR, "cache lookup failed for collation %u", oid); + collform = (Form_pg_collation) GETSTRUCT(tp); + version = get_collation_actual_version(collform->collprovider, + NameStr(collform->collcollate)); + } + + ReleaseSysCache(tp); + + if (!version) + return ""; + else + return version; +} #ifdef USE_ICU /* diff --git a/src/backend/utils/adt/pg_upgrade_support.c b/src/backend/utils/adt/pg_upgrade_support.c index 14d9eb2b5b..4ba8c8c637 100644 --- a/src/backend/utils/adt/pg_upgrade_support.c +++ b/src/backend/utils/adt/pg_upgrade_support.c @@ -13,6 +13,7 @@ #include "catalog/binary_upgrade.h" #include "catalog/heap.h" +#include "catalog/index.h" #include "catalog/namespace.h" #include "catalog/pg_type.h" #include "commands/extension.h" @@ -197,3 +198,27 @@ binary_upgrade_set_missing_value(PG_FUNCTION_ARGS) PG_RETURN_VOID(); } + +Datum +binary_upgrade_set_index_coll_version(PG_FUNCTION_ARGS) +{ + Oid relid; + Oid coll; + char *version; + + CHECK_IS_BINARY_UPGRADE; + + relid = PG_GETARG_OID(0); + + /* Detect if a collation is specified */ + if (PG_ARGISNULL(1)) + coll = InvalidOid; + else + coll = PG_GETARG_OID(1); + + version = TextDatumGetCString(PG_GETARG_TEXT_PP(2)); + + index_force_collation_versions(relid, coll, version); + + PG_RETURN_VOID(); +} diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 96ecad02dd..96f8c67831 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -42,6 +42,7 @@ #include "access/xact.h" #include "access/xlog.h" #include "catalog/catalog.h" +#include "catalog/index.h" #include "catalog/indexing.h" #include "catalog/namespace.h" #include "catalog/partition.h" @@ -5927,6 +5928,7 @@ load_relcache_init_file(bool shared) rel->rd_idattr = NULL; rel->rd_pubactions = NULL; rel->rd_statvalid = false; + rel->rd_version_checked = false; rel->rd_statlist = NIL; rel->rd_fkeyvalid = false; rel->rd_fkeylist = NIL; diff --git a/src/bin/pg_dump/Makefile b/src/bin/pg_dump/Makefile index 2532d9183a..c58ebaa681 100644 --- a/src/bin/pg_dump/Makefile +++ b/src/bin/pg_dump/Makefile @@ -16,6 +16,8 @@ subdir = src/bin/pg_dump top_builddir = ../../.. include $(top_builddir)/src/Makefile.global +export with_icu + override CPPFLAGS := -I$(libpq_srcdir) $(CPPFLAGS) LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport) diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h index 1017abbbe5..a94fbafec3 100644 --- a/src/bin/pg_dump/pg_backup.h +++ b/src/bin/pg_dump/pg_backup.h @@ -171,6 +171,7 @@ typedef struct _dumpOptions int sequence_data; /* dump sequence data even in schema-only mode */ int do_nothing; + int unknown_coll_compat; } DumpOptions; /* diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 77139355e0..90535936ae 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -46,6 +46,7 @@ #include "catalog/pg_attribute_d.h" #include "catalog/pg_cast_d.h" #include "catalog/pg_class_d.h" +#include "catalog/pg_collation_d.h" #include "catalog/pg_default_acl_d.h" #include "catalog/pg_largeobject_d.h" #include "catalog/pg_largeobject_metadata_d.h" @@ -286,6 +287,8 @@ static void binary_upgrade_extension_member(PQExpBuffer upgrade_buffer, static const char *getAttrName(int attrnum, TableInfo *tblInfo); static const char *fmtCopyColumnList(const TableInfo *ti, PQExpBuffer buffer); static bool nonemptyReloptions(const char *reloptions); +static void appendIndexCollationVersion(PQExpBuffer buffer, IndxInfo *indxinfo, + int enc, int unknown_coll_compat); static void appendReloptionsArrayAH(PQExpBuffer buffer, const char *reloptions, const char *prefix, Archive *fout); static char *get_synchronized_snapshot(Archive *fout); @@ -387,6 +390,7 @@ main(int argc, char **argv) {"on-conflict-do-nothing", no_argument, &dopt.do_nothing, 1}, {"rows-per-insert", required_argument, NULL, 10}, {"include-foreign-data", required_argument, NULL, 11}, + {"unknown-collations-binary-compatible", no_argument, &dopt.unknown_coll_compat, 1}, {NULL, 0, NULL, 0} }; @@ -714,6 +718,10 @@ main(int argc, char **argv) if (archiveFormat != archDirectory && numWorkers > 1) fatal("parallel backup only supported by the directory format"); + /* Unknown collation versions can only be ignored in binary upgrade mode */ + if (dopt.unknown_coll_compat && !dopt.binary_upgrade) + fatal("option --unknown-collations-binary-compatible only works in binary upgrade mode"); + /* Open the output file */ fout = CreateArchive(filename, archiveFormat, compressLevel, dosync, archiveMode, setupDumpWorker); @@ -7034,7 +7042,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) i_tablespace, i_indreloptions, i_indstatcols, - i_indstatvals; + i_indstatvals, + i_inddependoids, + i_inddependversions; int ntups; for (i = 0; i < numTables; i++) @@ -7070,7 +7080,62 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) * is not. */ resetPQExpBuffer(query); - if (fout->remoteVersion >= 110000) + if (fout->remoteVersion >= 140000) + { + appendPQExpBuffer(query, + "SELECT t.tableoid, t.oid, " + "t.relname AS indexname, " + "inh.inhparent AS parentidx, " + "pg_catalog.pg_get_indexdef(i.indexrelid) AS indexdef, " + "i.indnkeyatts AS indnkeyatts, " + "i.indnatts AS indnatts, " + "i.indkey, i.indisclustered, " + "i.indisreplident, " + "c.contype, c.conname, " + "c.condeferrable, c.condeferred, " + "c.tableoid AS contableoid, " + "c.oid AS conoid, " + "pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, " + "(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, " + "t.reloptions AS indreloptions, " + "(SELECT pg_catalog.array_agg(attnum ORDER BY attnum) " + " FROM pg_catalog.pg_attribute " + " WHERE attrelid = i.indexrelid AND " + " attstattarget >= 0) AS indstatcols," + "(SELECT pg_catalog.array_agg(attstattarget ORDER BY attnum) " + " FROM pg_catalog.pg_attribute " + " WHERE attrelid = i.indexrelid AND " + " attstattarget >= 0) AS indstatvals, " + "(SELECT pg_catalog.array_agg(refobjid ORDER BY refobjid) " + " FROM pg_catalog.pg_depend " + " WHERE classid = " CppAsString2(RelationRelationId) " AND " + " objid = i.indexrelid AND " + " objsubid = 0 AND " + " refclassid = " CppAsString2(CollationRelationId) " AND " + " refobjversion IS NOT NULL) AS inddependoids, " + "(SELECT pg_catalog.array_agg(quote_literal(refobjversion) ORDER BY refobjid) " + " FROM pg_catalog.pg_depend " + " WHERE classid = " CppAsString2(RelationRelationId) " AND " + " objid = i.indexrelid AND " + " objsubid = 0 AND " + " refclassid = " CppAsString2(CollationRelationId) " AND " + " refobjversion IS NOT NULL) AS inddependversions " + "FROM pg_catalog.pg_index i " + "JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) " + "JOIN pg_catalog.pg_class t2 ON (t2.oid = i.indrelid) " + "LEFT JOIN pg_catalog.pg_constraint c " + "ON (i.indrelid = c.conrelid AND " + "i.indexrelid = c.conindid AND " + "c.contype IN ('p','u','x')) " + "LEFT JOIN pg_catalog.pg_inherits inh " + "ON (inh.inhrelid = indexrelid) " + "WHERE i.indrelid = '%u'::pg_catalog.oid " + "AND (i.indisvalid OR t2.relkind = 'p') " + "AND i.indisready " + "ORDER BY indexname", + tbinfo->dobj.catId.oid); + } + else if (fout->remoteVersion >= 110000) { appendPQExpBuffer(query, "SELECT t.tableoid, t.oid, " @@ -7095,7 +7160,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) "(SELECT pg_catalog.array_agg(attstattarget ORDER BY attnum) " " FROM pg_catalog.pg_attribute " " WHERE attrelid = i.indexrelid AND " - " attstattarget >= 0) AS indstatvals " + " attstattarget >= 0) AS indstatvals, " + "' ' AS inddependoids, " + "' ' AS inddependversions " "FROM pg_catalog.pg_index i " "JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) " "JOIN pg_catalog.pg_class t2 ON (t2.oid = i.indrelid) " @@ -7134,7 +7201,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) "(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, " "t.reloptions AS indreloptions, " "'' AS indstatcols, " - "'' AS indstatvals " + "'' AS indstatvals, " + "' ' AS inddependoids, " + "' ' AS inddependversions " "FROM pg_catalog.pg_index i " "JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) " "LEFT JOIN pg_catalog.pg_constraint c " @@ -7169,7 +7238,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) "(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, " "t.reloptions AS indreloptions, " "'' AS indstatcols, " - "'' AS indstatvals " + "'' AS indstatvals, " + "' ' AS inddependoids, " + "' ' AS inddependversions " "FROM pg_catalog.pg_index i " "JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) " "LEFT JOIN pg_catalog.pg_constraint c " @@ -7200,7 +7271,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) "(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, " "t.reloptions AS indreloptions, " "'' AS indstatcols, " - "'' AS indstatvals " + "'' AS indstatvals, " + "' ' AS inddependoids, " + "' ' AS inddependversions " "FROM pg_catalog.pg_index i " "JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) " "LEFT JOIN pg_catalog.pg_depend d " @@ -7234,7 +7307,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) "(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, " "null AS indreloptions, " "'' AS indstatcols, " - "'' AS indstatvals " + "'' AS indstatvals, " + "' ' AS inddependoids, " + "' ' AS inddependversions " "FROM pg_catalog.pg_index i " "JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) " "LEFT JOIN pg_catalog.pg_depend d " @@ -7274,6 +7349,8 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) i_indreloptions = PQfnumber(res, "indreloptions"); i_indstatcols = PQfnumber(res, "indstatcols"); i_indstatvals = PQfnumber(res, "indstatvals"); + i_inddependoids = PQfnumber(res, "inddependoids"); + i_inddependversions = PQfnumber(res, "inddependversions"); tbinfo->indexes = indxinfo = (IndxInfo *) pg_malloc(ntups * sizeof(IndxInfo)); @@ -7299,6 +7376,8 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) indxinfo[j].indreloptions = pg_strdup(PQgetvalue(res, j, i_indreloptions)); indxinfo[j].indstatcols = pg_strdup(PQgetvalue(res, j, i_indstatcols)); indxinfo[j].indstatvals = pg_strdup(PQgetvalue(res, j, i_indstatvals)); + indxinfo[j].inddependoids = pg_strdup(PQgetvalue(res, j, i_inddependoids)); + indxinfo[j].inddependversions = pg_strdup(PQgetvalue(res, j, i_inddependversions)); indxinfo[j].indkeys = (Oid *) pg_malloc(indxinfo[j].indnattrs * sizeof(Oid)); parseOidArray(PQgetvalue(res, j, i_indkey), indxinfo[j].indkeys, indxinfo[j].indnattrs); @@ -16360,7 +16439,8 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo) /* * If there's an associated constraint, don't dump the index per se, but - * do dump any comment for it. (This is safe because dependency ordering + * do dump any comment, or in binary upgrade mode dependency on a + * collation version for it. (This is safe because dependency ordering * will have ensured the constraint is emitted first.) Note that the * emitted comment has to be shown as depending on the constraint, not the * index, in such cases. @@ -16427,6 +16507,10 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo) "pg_catalog.pg_class", "INDEX", qqindxname); + if (dopt->binary_upgrade) + appendIndexCollationVersion(q, indxinfo, fout->encoding, + dopt->unknown_coll_compat); + /* If the index defines identity, we need to record that. */ if (indxinfo->indisreplident) { @@ -16455,6 +16539,21 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo) if (indstatvalsarray) free(indstatvalsarray); } + else if (dopt->binary_upgrade) + { + appendIndexCollationVersion(q, indxinfo, fout->encoding, + dopt->unknown_coll_compat); + + if (indxinfo->dobj.dump & DUMP_COMPONENT_DEFINITION) + ArchiveEntry(fout, indxinfo->dobj.catId, indxinfo->dobj.dumpId, + ARCHIVE_OPTS(.tag = indxinfo->dobj.name, + .namespace = tbinfo->dobj.namespace->dobj.name, + .tablespace = indxinfo->tablespace, + .owner = tbinfo->rolname, + .description = "INDEX", + .section = SECTION_POST_DATA, + .createStmt = q->data)); + } /* Dump Index Comments */ if (indxinfo->dobj.dump & DUMP_COMPONENT_COMMENT) @@ -18441,6 +18540,77 @@ nonemptyReloptions(const char *reloptions) return (reloptions != NULL && strlen(reloptions) > 2); } +/* + * Format inddependoids and inddependversions arrays and append it to the given + * buffer in the form of binary_upgrade_set_index_coll_version() calls. + */ +static void +appendIndexCollationVersion(PQExpBuffer buffer, IndxInfo *indxinfo, int enc, + int unknown_coll_compat) +{ + char *inddependoids = indxinfo->inddependoids; + char *inddependversions = indxinfo->inddependversions; + char **inddependoidsarray = NULL; + char **inddependversionsarray = NULL; + int ninddependoids; + int ninddependversions; + int i; + + /* + * for older versions that don't record the collation depndency, issue a + * statement to mark the collation version as unknown + */ + if (strcmp(inddependoids, " ") == 0) + { + /* + * do not issue UNKNOWN VERSION if caller specified that those are + * compatible + */ + if (unknown_coll_compat) + return; + + Assert(strcmp(inddependversions, " ") == 0); + + appendPQExpBufferStr(buffer, "\n-- For binary upgrade, restore dependent collation version.\n"); + appendPQExpBuffer(buffer, "SELECT " + "pg_catalog.binary_upgrade_set_index_coll_version(%d, NULL, '');", + indxinfo->dobj.catId.oid); + return; + } + + parsePGArray(inddependoids, &inddependoidsarray, &ninddependoids); + parsePGArray(inddependversions, &inddependversionsarray, &ninddependversions); + + Assert(ninddependoids == ninddependversions); + + for (i = 0; i < ninddependoids; i++) + { + /* + * If there was an unknown version dependency recorded for this + * collation and the caller asked to mark those as depending on the + * current version, don't emit a binary_upgrade_set_index_coll_version + * function call. + */ + if ((strcmp(inddependversionsarray[i], "''")) == 0 + && unknown_coll_compat) + { + continue; + } + + appendPQExpBufferStr(buffer, "\n-- For binary upgrade, restore dependent collation version.\n"); + appendPQExpBuffer(buffer, "SELECT " + "pg_catalog.binary_upgrade_set_index_coll_version(%d, %s, %s);", + indxinfo->dobj.catId.oid, + inddependoidsarray[i], + inddependversionsarray[i]); + } + + if (inddependoidsarray) + free(inddependoidsarray); + if (inddependversionsarray) + free(inddependversionsarray); +} + /* * Format a reloptions array and append it to the given buffer. * diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index e0b42e8391..623814d1c5 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -366,6 +366,9 @@ typedef struct _indxInfo int indnattrs; /* total number of index attributes */ Oid *indkeys; /* In spite of the name 'indkeys' this field * contains both key and nonkey attributes */ + char *inddependoids; /* oids of collation this index depends on */ + char *inddependversions; /* version of collation this index depends + * on */ bool indisclustered; bool indisreplident; Oid parentidx; /* if a partition, parent index OID */ diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl index ec63662060..5f323efb1f 100644 --- a/src/bin/pg_dump/t/002_pg_dump.pl +++ b/src/bin/pg_dump/t/002_pg_dump.pl @@ -53,6 +53,24 @@ my %pgdump_runs = ( "$tempdir/binary_upgrade.dump", ], }, + binary_coll_compatible => { + dump_cmd => [ + 'pg_dump', + '--no-sync', + '--format=custom', + "--file=$tempdir/binary_coll_compatible.dump", + '-w', + '--schema-only', + '--binary-upgrade', + '--unknown-collations-binary-compatible', + '-d', 'postgres', # alternative way to specify database + ], + restore_cmd => [ + 'pg_restore', '-Fc', '--verbose', + "--file=$tempdir/binary_coll_compatible.sql", + "$tempdir/binary_coll_compatible.dump", + ], + }, clean => { dump_cmd => [ 'pg_dump', @@ -387,6 +405,7 @@ my %dump_test_schema_runs = ( # are flags used to exclude specific items (ACLs, blobs, etc). my %full_runs = ( binary_upgrade => 1, + binary_coll_compatible => 1, clean => 1, clean_if_exists => 1, createdb => 1, @@ -920,9 +939,10 @@ my %tests = ( test_schema_plus_blobs => 1, }, unlike => { - binary_upgrade => 1, - no_blobs => 1, - schema_only => 1, + binary_upgrade => 1, + binary_coll_compatible => 1, + no_blobs => 1, + schema_only => 1, }, }, @@ -1184,6 +1204,7 @@ my %tests = ( }, unlike => { binary_upgrade => 1, + binary_coll_compatible => 1, exclude_dump_test_schema => 1, exclude_test_table => 1, exclude_test_table_data => 1, @@ -1209,6 +1230,7 @@ my %tests = ( }, unlike => { binary_upgrade => 1, + binary_coll_compatible => 1, exclude_dump_test_schema => 1, schema_only => 1, }, @@ -1244,6 +1266,7 @@ my %tests = ( }, unlike => { binary_upgrade => 1, + binary_coll_compatible => 1, exclude_dump_test_schema => 1, schema_only => 1, }, @@ -1266,6 +1289,7 @@ my %tests = ( }, unlike => { binary_upgrade => 1, + binary_coll_compatible => 1, exclude_dump_test_schema => 1, schema_only => 1, }, @@ -1287,6 +1311,7 @@ my %tests = ( }, unlike => { binary_upgrade => 1, + binary_coll_compatible => 1, exclude_dump_test_schema => 1, schema_only => 1, }, @@ -1308,6 +1333,7 @@ my %tests = ( }, unlike => { binary_upgrade => 1, + binary_coll_compatible => 1, exclude_dump_test_schema => 1, schema_only => 1, }, @@ -1674,6 +1700,7 @@ my %tests = ( { %full_runs, %dump_test_schema_runs, section_pre_data => 1, }, unlike => { binary_upgrade => 1, + binary_coll_compatible => 1, exclude_dump_test_schema => 1, }, }, @@ -1688,7 +1715,7 @@ my %tests = ( \n.*^ \QALTER TYPE dump_test.planets ADD VALUE 'mars';\E \n/xms, - like => { binary_upgrade => 1, }, + like => { binary_upgrade => 1, binary_coll_compatible => 1, }, }, 'CREATE TYPE dump_test.textrange AS RANGE' => { @@ -2356,6 +2383,7 @@ my %tests = ( { %full_runs, %dump_test_schema_runs, section_pre_data => 1, }, unlike => { binary_upgrade => 1, + binary_coll_compatible => 1, exclude_dump_test_schema => 1, }, }, @@ -2549,6 +2577,7 @@ my %tests = ( }, unlike => { binary_upgrade => 1, + binary_coll_compatible => 1, exclude_dump_test_schema => 1, }, }, @@ -2617,6 +2646,7 @@ my %tests = ( /xm, like => { binary_upgrade => 1, + binary_coll_compatible => 1, clean => 1, clean_if_exists => 1, createdb => 1, @@ -2688,6 +2718,7 @@ my %tests = ( /xm, like => { binary_upgrade => 1, + binary_coll_compatible => 1, clean => 1, clean_if_exists => 1, createdb => 1, @@ -3155,6 +3186,7 @@ my %tests = ( { %full_runs, %dump_test_schema_runs, section_post_data => 1, }, unlike => { binary_upgrade => 1, + binary_coll_compatible => 1, exclude_dump_test_schema => 1, schema_only => 1, }, @@ -3170,6 +3202,7 @@ my %tests = ( { %full_runs, %dump_test_schema_runs, section_post_data => 1, }, unlike => { binary_upgrade => 1, + binary_coll_compatible => 1, exclude_dump_test_schema => 1, schema_only => 1, }, @@ -3302,16 +3335,53 @@ my %tests = ( %full_runs, %dump_test_schema_runs, section_pre_data => 1, }, unlike => { exclude_dump_test_schema => 1 }, + }, + + "binary_upgrade_set_index_coll_version(oid, oid, 'not_a_version')" => { + create_order => 101, + create_sql => ' + CREATE TABLE dump_test.regress_table_coll(id integer, val text); + CREATE INDEX regress_coll_idx1 ON dump_test.regress_table_coll(val COLLATE "fr-x-icu"); + UPDATE pg_depend SET refobjversion = \'not_a_version\' WHERE refobjversion IS NOT NULL AND objid::regclass::text = \'dump_test.regress_coll_idx1\';', + regexp => qr/^ + \QCREATE INDEX regress_coll_idx1 ON dump_test.regress_table_coll USING btree (val COLLATE "fr-x-icu");\E\n + \n + \Q-- For binary upgrade, restore dependent collation version.\E\n + \QSELECT pg_catalog.binary_upgrade_set_index_coll_version\E \(\d+,\ \d+,\ 'not_a_version'\);/xm, + like => { binary_upgrade => 1, binary_coll_compatible => 1, }, + icu => 1, + }, + "binary_upgrade_set_index_coll_version(?, ?, '')" => { + create_order => 102, + create_sql => ' + CREATE TABLE dump_test.regress_table_coll_no_ver(id integer, val text); + CREATE INDEX regress_coll_no_ver_idx1 ON dump_test.regress_table_coll_no_ver(val COLLATE "fr-x-icu"); + UPDATE pg_depend SET refobjversion = \'\' WHERE refobjversion IS NOT NULL AND objid::regclass::text = \'dump_test.regress_coll_no_ver_idx1\';', + regexp => qr/SELECT pg_catalog.binary_upgrade_set_index_coll_version\(\d+, \d+, ''\)/, + like => { binary_upgrade => 1}, + # should not appear in binary_coll_compatible case! + unlike => { binary_coll_compatible => 1}, + icu => 1, }); ######################################### # Create a PG instance to test actually dumping from -my $node = get_new_node('main'); -$node->init; -$node->start; +my $main_node = get_new_node('main'); +$main_node->init; +$main_node->start; + +my $port = $main_node->port; + +# And another instance to validate the binary dump +my $bin_node = get_new_node('binary'); +$bin_node->init; +$bin_node->start; -my $port = $node->port; +my $bin_port = $bin_node->port; + +# and add a $node variable pointing to main_node for now +my $node = $main_node; # We need to see if this system supports CREATE COLLATION or not # If it doesn't then we will skip all the COLLATION-related tests. @@ -3335,6 +3405,10 @@ $node->psql('postgres', 'create database regress_pg_dump_test;'); # command_fails_like is actually 2 tests) my $num_tests = 12; +# 4 more tests for restoring globals and binary_upgrade dump, dumping it again +# and regenerating the sql file +$num_tests+= 4; + foreach my $run (sort keys %pgdump_runs) { my $test_key = $run; @@ -3385,16 +3459,29 @@ foreach my $run (sort keys %pgdump_runs) next; } + # Skip any icu-related commands if there is no icu support + if ($ENV{with_icu} ne 'yes' && defined($tests{$test}->{icu})) + { + next; + } + # If there is a like entry, but no unlike entry, then we will test the like case if ($tests{$test}->{like}->{$test_key} && !defined($tests{$test}->{unlike}->{$test_key})) { $num_tests++; + + # binary_upgrade tests are also run after being restored and + # re-dumped. + $num_tests++ if ($test_key eq 'binary_upgrade'); } else { # We will test everything that isn't a 'like' $num_tests++; + # binary_upgrade tests are also run after being restored and + # re-dumped. + $num_tests++ if ($test_key eq 'binary_upgrade'); } } } @@ -3442,6 +3529,12 @@ foreach my $test ( next; } + # Skip any icu-related commands if there is no icu support + if ($ENV{with_icu} ne 'yes' && defined($tests{$test}->{icu})) + { + next; + } + # Add terminating semicolon $create_sql{$test_db} .= $tests{$test}->{create_sql} . ";"; } @@ -3495,79 +3588,116 @@ command_fails_like( ######################################### # Run all runs -foreach my $run (sort keys %pgdump_runs) +foreach my $pass (1, 2) { - my $test_key = $run; - my $run_db = 'postgres'; - - $node->command_ok(\@{ $pgdump_runs{$run}->{dump_cmd} }, - "$run: pg_dump runs"); - - if ($pgdump_runs{$run}->{restore_cmd}) + foreach my $run (sort keys %pgdump_runs) { - $node->command_ok(\@{ $pgdump_runs{$run}->{restore_cmd} }, - "$run: pg_restore runs"); - } - - if ($pgdump_runs{$run}->{test_key}) - { - $test_key = $pgdump_runs{$run}->{test_key}; - } - - my $output_file = slurp_file("$tempdir/${run}.sql"); + my $test_key = $run; + my $run_db = 'postgres'; - ######################################### - # Run all tests where this run is included - # as either a 'like' or 'unlike' test. + # we only test binary upgrade on the 2nd pass + next if ($pass == 2 and $test_key ne 'binary_upgrade'); - foreach my $test (sort keys %tests) - { - my $test_db = 'postgres'; + $node->command_ok(\@{ $pgdump_runs{$run}->{dump_cmd} }, + "$run: pg_dump runs"); - if (defined($pgdump_runs{$run}->{database})) + if ($pgdump_runs{$run}->{restore_cmd}) { - $run_db = $pgdump_runs{$run}->{database}; + $node->command_ok(\@{ $pgdump_runs{$run}->{restore_cmd} }, + "$run: pg_restore runs"); } - if (defined($tests{$test}->{database})) + if ($pgdump_runs{$run}->{test_key}) { - $test_db = $tests{$test}->{database}; + $test_key = $pgdump_runs{$run}->{test_key}; } - # Skip any collation-related commands if there is no collation support - if (!$collation_support && defined($tests{$test}->{collation})) - { - next; - } + my $output_file = slurp_file("$tempdir/${run}.sql"); - if ($run_db ne $test_db) - { - next; - } + ######################################### + # Run all tests where this run is included + # as either a 'like' or 'unlike' test. - # Run the test listed as a like, unless it is specifically noted - # as an unlike (generally due to an explicit exclusion or similar). - if ($tests{$test}->{like}->{$test_key} - && !defined($tests{$test}->{unlike}->{$test_key})) + foreach my $test (sort keys %tests) { - if (!ok($output_file =~ $tests{$test}->{regexp}, - "$run: should dump $test")) + my $test_db = 'postgres'; + + if (defined($pgdump_runs{$run}->{database})) { - diag("Review $run results in $tempdir"); + $run_db = $pgdump_runs{$run}->{database}; } - } - else - { - if (!ok($output_file !~ $tests{$test}->{regexp}, - "$run: should not dump $test")) + + if (defined($tests{$test}->{database})) + { + $test_db = $tests{$test}->{database}; + } + + # Skip any collation-related commands if there is no collation support + if (!$collation_support && defined($tests{$test}->{collation})) + { + next; + } + + # Skip any icu-related commands if there is no icu support + if ($ENV{with_icu} ne 'yes' && defined($tests{$test}->{icu})) + { + next; + } + + if ($run_db ne $test_db) + { + next; + } + + # Run the test listed as a like, unless it is specifically noted + # as an unlike (generally due to an explicit exclusion or similar). + if ($tests{$test}->{like}->{$test_key} + && !defined($tests{$test}->{unlike}->{$test_key})) { - diag("Review $run results in $tempdir"); + if (!ok($output_file =~ $tests{$test}->{regexp}, + "$run: should dump $test")) + { + diag("Review $run results in $tempdir"); + } + } + else + { + if (!ok($output_file !~ $tests{$test}->{regexp}, + "$run: should not dump $test")) + { + diag("Review $run results in $tempdir"); + } } } } + + # After all dump have been generated, restore the binary_upgrade dump with + # the required global objects on a suitable node, and continue with the 2nd + # pass. + if ($pass == 1) + { + # Stop the original database instance as we don't need it anymore. + $node->stop('fast'); + + $bin_node->command_ok(\@{['psql', + "-d", "postgres", "-f", "$tempdir/pg_dumpall_globals.sql"]}, + "Restore globals"); + + $bin_node->stop('fast'); + $bin_node->start(binary_start => 1); + $bin_node->command_ok(\@{['pg_restore', '-p', $bin_port, + '-d', 'postgres', + "$tempdir/binary_upgrade.dump"]}, + "Restore the binary_upgrade dump"); + $bin_node->stop('fast'); + $bin_node->start; + + # And change $node to point to the freshly restored node. + $node = $bin_node; + } } ######################################### # Stop the database instance, which will be removed at the end of the tests. -$node->stop('fast'); +$bin_node->stop('fast'); diff --git a/src/bin/pg_upgrade/dump.c b/src/bin/pg_upgrade/dump.c index 4d730adfe2..672ecda169 100644 --- a/src/bin/pg_upgrade/dump.c +++ b/src/bin/pg_upgrade/dump.c @@ -52,9 +52,11 @@ generate_old_dump(void) parallel_exec_prog(log_file_name, NULL, "\"%s/pg_dump\" %s --schema-only --quote-all-identifiers " - "--binary-upgrade --format=custom %s --file=\"%s\" %s", + "--binary-upgrade --format=custom %s %s --file=\"%s\" %s", new_cluster.bindir, cluster_conn_opts(&old_cluster), log_opts.verbose ? "--verbose" : "", + user_opts.coll_compat ? + "--unknown-collations-binary-compatible" : "", sql_file_name, escaped_connstr.data); termPQExpBuffer(&escaped_connstr); diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index aca1ee8b48..c7e291f7e7 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -56,6 +56,7 @@ parseCommandLine(int argc, char *argv[]) {"socketdir", required_argument, NULL, 's'}, {"verbose", no_argument, NULL, 'v'}, {"clone", no_argument, NULL, 1}, + {"collation-binary-compatible", no_argument, NULL, 2}, {NULL, 0, NULL, 0} }; @@ -203,6 +204,10 @@ parseCommandLine(int argc, char *argv[]) user_opts.transfer_mode = TRANSFER_MODE_CLONE; break; + case 2: + user_opts.coll_compat = true; + break; + default: fprintf(stderr, _("Try \"%s --help\" for more information.\n"), os_info.progname); @@ -307,6 +312,8 @@ usage(void) printf(_(" -v, --verbose enable verbose internal logging\n")); printf(_(" -V, --version display version information, then exit\n")); printf(_(" --clone clone instead of copying files to new cluster\n")); + printf(_(" --collation-binary-compatible mark collations as depending on current collation\n" + " versions rather than unknown if they're unknown\n")); printf(_(" -?, --help show this help, then exit\n")); printf(_("\n" "Before running pg_upgrade you must:\n" diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h index 8b90cefbe0..68e637ce8a 100644 --- a/src/bin/pg_upgrade/pg_upgrade.h +++ b/src/bin/pg_upgrade/pg_upgrade.h @@ -292,6 +292,8 @@ typedef struct transferMode transfer_mode; /* copy files or link them? */ int jobs; /* number of processes/threads to use */ char *socketdir; /* directory to use for Unix sockets */ + bool coll_compat; /* should we skip marking index collations as + * unknown version */ } UserOpts; typedef struct diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h index 3baa5e498a..2966339498 100644 --- a/src/include/catalog/dependency.h +++ b/src/include/catalog/dependency.h @@ -130,6 +130,13 @@ typedef enum ObjectClass #define LAST_OCLASS OCLASS_TRANSFORM +/* Struct describing one forced collation version dependency */ +typedef struct NewCollationVersionDependency +{ + char *version; /* forced collation version */ + Oid oid; /* target collation oid */ +} NewCollationVersionDependency; + /* flag bits for performDeletion/performMultipleDeletions: */ #define PERFORM_DELETION_INTERNAL 0x0001 /* internal action */ #define PERFORM_DELETION_CONCURRENTLY 0x0002 /* concurrent drop */ @@ -160,7 +167,8 @@ extern void recordDependencyOnSingleRelExpr(const ObjectAddress *depender, Node *expr, Oid relId, DependencyType behavior, DependencyType self_behavior, - bool reverse_self); + bool reverse_self, + bool track_version); extern ObjectClass getObjectClass(const ObjectAddress *object); @@ -180,17 +188,29 @@ extern void sort_object_addresses(ObjectAddresses *addrs); extern void free_object_addresses(ObjectAddresses *addrs); +typedef char *(*VisitDependentObjectsFun) (const ObjectAddress *otherObject, + const char *version, + void *userdata); + +extern void visitDependentObjects(const ObjectAddress *object, + VisitDependentObjectsFun callback, + void *userdata); + /* in pg_depend.c */ extern void recordDependencyOn(const ObjectAddress *depender, const ObjectAddress *referenced, DependencyType behavior); +extern void recordDependencyOnCollations(ObjectAddress *myself, + List *collations, + bool record_version); + extern void recordMultipleDependencies(const ObjectAddress *depender, const ObjectAddress *referenced, int nreferenced, - const char *version, - DependencyType behavior); + DependencyType behavior, + bool track_version); extern void recordDependencyOnCurrentExtension(const ObjectAddress *object, bool isReplace); @@ -209,10 +229,9 @@ extern long changeDependencyFor(Oid classId, Oid objectId, Oid refClassId, Oid oldRefObjectId, Oid newRefObjectId); -extern long changeDependenciesOf(Oid classId, Oid oldObjectId, +long changeDependenciesOf(Oid classId, Oid oldObjectId, Oid newObjectId); - -extern long changeDependenciesOn(Oid refClassId, Oid oldRefObjectId, +long changeDependenciesOn(Oid refClassId, Oid oldRefObjectId, Oid newRefObjectId); extern Oid getExtensionOfObject(Oid classId, Oid objectId); diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h index f58e8675f3..9b4de26514 100644 --- a/src/include/catalog/index.h +++ b/src/include/catalog/index.h @@ -121,6 +121,11 @@ extern void FormIndexDatum(IndexInfo *indexInfo, Datum *values, bool *isnull); +extern void index_check_collation_versions(Oid relid); + +extern void index_force_collation_versions(Oid indexid, Oid coll, + char *version); + extern void index_build(Relation heapRelation, Relation indexRelation, IndexInfo *indexInfo, @@ -131,6 +136,8 @@ extern void validate_index(Oid heapId, Oid indexId, Snapshot snapshot); extern void index_set_state_flags(Oid indexId, IndexStateFlagsAction action); +extern void index_update_collation_versions(Oid relid); + extern Oid IndexGetRelation(Oid indexId, bool missing_ok); extern void reindex_index(Oid indexId, bool skip_constraint_checks, diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 687509ba92..84466ab577 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -10367,6 +10367,10 @@ proname => 'binary_upgrade_set_missing_value', provolatile => 'v', proparallel => 'u', prorettype => 'void', proargtypes => 'oid text text', prosrc => 'binary_upgrade_set_missing_value' }, +{ oid => '8178', descr => 'for use by pg_upgrade', + proname => 'binary_upgrade_set_index_coll_version', provolatile => 'v', + proparallel => 'u', prorettype => 'void', proargtypes => 'oid oid text', + prosrc => 'binary_upgrade_set_index_coll_version' }, # conversion functions { oid => '4302', diff --git a/src/include/catalog/pg_type.h b/src/include/catalog/pg_type.h index 7b37562648..2d511c5cba 100644 --- a/src/include/catalog/pg_type.h +++ b/src/include/catalog/pg_type.h @@ -361,6 +361,8 @@ extern void GenerateTypeDependencies(HeapTuple typeTuple, bool isDependentType, bool rebuild); +extern List *GetTypeCollations(Oid typeObjectid, bool non_deterministic_only); + extern void RenameTypeInternal(Oid typeOid, const char *newTypeName, Oid typeNamespace); diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h index 9cb7d91ddf..96da132c03 100644 --- a/src/include/utils/pg_locale.h +++ b/src/include/utils/pg_locale.h @@ -103,7 +103,7 @@ typedef struct pg_locale_struct *pg_locale_t; extern pg_locale_t pg_newlocale_from_collation(Oid collid); -extern char *get_collation_actual_version(char collprovider, const char *collcollate); +extern char *get_collation_version_for_oid(Oid collid); #ifdef USE_ICU extern int32_t icu_to_uchar(UChar **buff_uchar, const char *buff, size_t nbytes); diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index 0b5957ba02..f79667d651 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -63,6 +63,7 @@ typedef struct RelationData bool rd_indexvalid; /* is rd_indexlist valid? (also rd_pkindex and * rd_replidindex) */ bool rd_statvalid; /* is rd_statlist valid? */ + bool rd_version_checked; /* has version check being done yet? */ /*---------- * rd_createSubid is the ID of the highest subtransaction the rel has diff --git a/src/test/Makefile b/src/test/Makefile index efb206aa75..4ef0d11702 100644 --- a/src/test/Makefile +++ b/src/test/Makefile @@ -12,7 +12,8 @@ subdir = src/test top_builddir = ../.. include $(top_builddir)/src/Makefile.global -SUBDIRS = perl regress isolation modules authentication recovery subscription +SUBDIRS = perl regress isolation modules authentication recovery subscription \ + locale # Test suites that are not safe by default but can be run if selected # by the user via the whitespace-separated list in variable @@ -37,7 +38,7 @@ endif # clean" etc to recurse into them. (We must filter out those that we # have conditionally included into SUBDIRS above, else there will be # make confusion.) -ALWAYS_SUBDIRS = $(filter-out $(SUBDIRS),examples kerberos ldap locale thread ssl) +ALWAYS_SUBDIRS = $(filter-out $(SUBDIRS),examples kerberos ldap thread ssl) # We want to recurse to all subdirs for all standard targets, except that # installcheck and install should not recurse into the subdirectory "modules". diff --git a/src/test/locale/.gitignore b/src/test/locale/.gitignore index 620d3df425..64e1bf2a80 100644 --- a/src/test/locale/.gitignore +++ b/src/test/locale/.gitignore @@ -1 +1,2 @@ /test-ctype +/tmp_check/ diff --git a/src/test/locale/Makefile b/src/test/locale/Makefile index 22a45b65f2..73495cf16b 100644 --- a/src/test/locale/Makefile +++ b/src/test/locale/Makefile @@ -4,6 +4,7 @@ subdir = src/test/locale top_builddir = ../../.. include $(top_builddir)/src/Makefile.global +export with_icu PROGS = test-ctype DIRS = de_DE.ISO8859-1 gr_GR.ISO8859-7 koi8-r koi8-to-win1251 @@ -19,3 +20,9 @@ clean distclean maintainer-clean: # These behave like installcheck targets. check-%: all @$(MAKE) -C `echo $@ | sed 's/^check-//'` test + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/src/test/locale/t/001_index.pl b/src/test/locale/t/001_index.pl new file mode 100644 index 0000000000..468fbb63b6 --- /dev/null +++ b/src/test/locale/t/001_index.pl @@ -0,0 +1,76 @@ +use strict; +use warnings; + +use Config; +use PostgresNode; +use TestLib; +use Test::More; + +if ($ENV{with_icu} eq 'yes') +{ + plan tests => 12; +} +else +{ + plan skip_all => 'ICU not supported by this build'; +} + +#### Set up the server + +note "setting up data directory"; +my $node = get_new_node('main'); +$node->init; + +$ENV{PGHOST} = $node->host; +$ENV{PGPORT} = $node->port; +$node->start; + +sub test_index +{ + my ($err_like, $err_comm) = @_; + + my ($ret, $out, $err) = $node->psql('postgres', + 'SET enable_seqscan = 0;' + . "EXPLAIN SELECT val FROM icu1 WHERE val = '0'"); + + is($ret, 0, 'EXPLAIN should succeed.'); + like($out, qr/icu1_fr/, 'Index icu1_fr should be used.'); + like($err, $err_like, $err_comm); +} + +$node->safe_psql('postgres', + 'CREATE TABLE icu1(val text);' + . 'INSERT INTO icu1 SELECT i::text FROM generate_series(1, 10000) i;' + . 'CREATE INDEX icu1_fr ON icu1 (val COLLATE "fr-x-icu");'); +$node->safe_psql('postgres', 'VACUUM ANALYZE icu1;'); + +test_index(qr/^$/, 'No warning should be raised'); + +# Simulate different collation version +$node->safe_psql('postgres', + "UPDATE pg_depend SET refobjversion = 'not_a_version'" + . " WHERE refobjversion IS NOT NULL" + . " AND objid::regclass::text = 'icu1_fr';"); + +test_index(qr/index "icu1_fr" depends on collation "fr-x-icu" version "not_a_version", but the current version is/, + 'Different collation version warning should be raised.'); + +# Simulate unknown collation version +$node->safe_psql('postgres', + "UPDATE pg_depend SET refobjversion = ''" + . " WHERE refobjversion IS NOT NULL" + . " AND objid::regclass::text = 'icu1_fr';"); + +test_index(qr/index "icu1_fr" depends on collation "fr-x-icu" with an unknown version, and the current version is/, + 'Unknown collation version warning should be raised.'); + +# Simulate previously unhandled collation versioning +$node->safe_psql('postgres', + "UPDATE pg_depend SET refobjversion = NULL" + . " WHERE refobjversion IS NOT NULL" + . " AND objid::regclass::text = 'icu1_fr';"); + +test_index(qr/index "icu1_fr" depends on collation "fr-x-icu" with an unknown version, and the current version is/, + 'Unknown collation version warning should be raised.'); + +$node->stop; diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm index 1488bffa2b..8c54e12268 100644 --- a/src/test/perl/PostgresNode.pm +++ b/src/test/perl/PostgresNode.pm @@ -765,10 +765,14 @@ sub start local %ENV = %ENV; delete $ENV{PGAPPNAME}; + my $options = "--cluster-name=$name"; + + $options .= ' -b' if ($params{binary_start}); + # Note: We set the cluster_name here, not in postgresql.conf (in # sub init) so that it does not get copied to standbys. $ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l', - $self->logfile, '-o', "--cluster-name=$name", 'start'); + $self->logfile, '-o', $options, 'start'); } if ($ret != 0) diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out index 60d9263a2f..db386c1b09 100644 --- a/src/test/regress/expected/collate.icu.utf8.out +++ b/src/test/regress/expected/collate.icu.utf8.out @@ -1897,6 +1897,163 @@ SELECT (SELECT count(*) FROM test33_0) <> (SELECT count(*) FROM test33_1); t (1 row) +-- collation versioning support +CREATE TYPE t_en_fr AS (fr text COLLATE "fr-x-icu", en text COLLATE "en-x-icu"); +CREATE DOMAIN d_en_fr AS t_en_fr; +CREATE DOMAIN d_es AS text COLLATE "es-x-icu"; +CREATE TYPE t_en_fr_ga AS (en_fr t_en_fr, ga text COLLATE "ga-x-icu"); +CREATE DOMAIN d_en_fr_ga AS t_en_fr_ga; +CREATE TYPE t_custom AS (meh text, meh2 text); +CREATE DOMAIN d_custom AS t_custom; +CREATE COLLATION custom ( + LOCALE = 'fr-x-icu', PROVIDER = 'icu' +); +CREATE TYPE myrange AS range (subtype = text); +CREATE TYPE myrange_en_fr_ga AS range(subtype = t_en_fr_ga); +CREATE TYPE mood AS ENUM ('sad', 'ok', 'happy'); +CREATE TABLE collate_test ( + id integer, + t_en_fr t_en_fr, + d_en_fr d_en_fr, + d_es d_es, + t_en_fr_ga t_en_fr_ga, + d_en_fr_ga d_en_fr_ga, + d_en_fr_ga_arr d_en_fr_ga[], + myrange myrange, + myrange_en_fr_ga myrange_en_fr_ga, + mood mood +); +CREATE INDEX icuidx01_t_en_fr__d_es ON collate_test (t_en_fr, d_es); +CREATE INDEX icuidx02_d_en_fr ON collate_test (d_en_fr); +CREATE INDEX icuidx03_t_en_fr_ga ON collate_test (t_en_fr_ga); +CREATE INDEX icuidx04_d_en_fr_ga ON collate_test (d_en_fr_ga); +CREATE INDEX icuidx05_d_en_fr_ga_arr ON collate_test (d_en_fr_ga_arr); +CREATE INDEX icuidx06_d_en_fr_ga ON collate_test(id) WHERE (d_en_fr_ga).en_fr.fr = 'foo'; +CREATE INDEX icuidx07_d_en_fr_ga ON collate_test(id) WHERE (d_en_fr_ga).ga = 'foo'; +CREATE INDEX icuidx08_d_en_fr_ga ON collate_test(id) WHERE (t_en_fr_ga) = ('foo', 'bar', 'baz'); +CREATE INDEX icuidx09_d_en_fr_ga ON collate_test(id) WHERE (d_en_fr_ga) = ('foo', 'bar', 'baz'); +CREATE INDEX icuidx10_d_en_fr_ga_es ON collate_test(id) WHERE (d_en_fr_ga) = ('foo', 'bar', 'baz' COLLATE "es-x-icu"); +CREATE INDEX icuidx11_d_es ON collate_test(id) WHERE (d_es) = ('foo'); +CREATE INDEX icuidx12_custom ON collate_test(id) WHERE ('foo', 'bar')::d_custom = ('foo', 'bar' collate custom)::d_custom; +CREATE INDEX icuidx13_custom ON collate_test(id) WHERE ('foo' collate custom, 'bar')::d_custom = ('foo', 'bar')::d_custom; +CREATE INDEX icuidx14_myrange ON collate_test(myrange); +CREATE INDEX icuidx15_myrange_en_fr_ga ON collate_test USING gist (myrange_en_fr_ga); +CREATE INDEX icuidx16_mood ON collate_test(id) WHERE mood > 'ok' collate "fr-x-icu"; +CREATE TABLE collate_part(id integer, val text COLLATE "en-x-icu") PARTITION BY range(id); +CREATE TABLE collate_part_0 PARTITION OF collate_part FOR VALUES FROM (0) TO (1); +CREATE TABLE collate_part_1 PARTITION OF collate_part FOR VALUES FROM (1) TO (1000000); +CREATE INDEX icuidx17_part ON collate_part_1 (val); +-- for key columns, hash indexes should record dependency on the collation but +-- not the version +CREATE INDEX icuidx18_hash_d_es ON collate_test USING hash (d_es); +CREATE INDEX icuidx19_hash_id_d_es_eq ON collate_test USING hash (id) WHERE (d_es) = 'foo'; +CREATE INDEX icuidx20_hash_id_d_es_lt ON collate_test USING hash (id) WHERE (d_es) < 'foo'; +SELECT objid::regclass, refobjid::regcollation, +CASE +WHEN refobjversion IS NULL THEN 'version not tracked' +ELSE CASE + WHEN refobjversion = pg_collation_actual_version(refobjid) THEN 'up to date' + ELSE 'out of date' + END +END AS version +FROM pg_depend d +LEFT JOIN pg_class c ON c.oid = d.objid +WHERE refclassid = 'pg_collation'::regclass +AND coalesce(relkind, 'i') = 'i' +AND relname LIKE 'icuidx%' +ORDER BY 1, 2; + objid | refobjid | version +---------------------------+------------+--------------------- + icuidx01_t_en_fr__d_es | "en-x-icu" | up to date + icuidx01_t_en_fr__d_es | "es-x-icu" | up to date + icuidx01_t_en_fr__d_es | "fr-x-icu" | up to date + icuidx02_d_en_fr | "en-x-icu" | up to date + icuidx02_d_en_fr | "fr-x-icu" | up to date + icuidx03_t_en_fr_ga | "en-x-icu" | up to date + icuidx03_t_en_fr_ga | "fr-x-icu" | up to date + icuidx03_t_en_fr_ga | "ga-x-icu" | up to date + icuidx04_d_en_fr_ga | "en-x-icu" | up to date + icuidx04_d_en_fr_ga | "fr-x-icu" | up to date + icuidx04_d_en_fr_ga | "ga-x-icu" | up to date + icuidx05_d_en_fr_ga_arr | "en-x-icu" | up to date + icuidx05_d_en_fr_ga_arr | "fr-x-icu" | up to date + icuidx05_d_en_fr_ga_arr | "ga-x-icu" | up to date + icuidx06_d_en_fr_ga | "default" | up to date + icuidx06_d_en_fr_ga | "en-x-icu" | up to date + icuidx06_d_en_fr_ga | "fr-x-icu" | up to date + icuidx06_d_en_fr_ga | "ga-x-icu" | up to date + icuidx07_d_en_fr_ga | "default" | up to date + icuidx07_d_en_fr_ga | "en-x-icu" | up to date + icuidx07_d_en_fr_ga | "fr-x-icu" | up to date + icuidx07_d_en_fr_ga | "ga-x-icu" | up to date + icuidx08_d_en_fr_ga | "en-x-icu" | up to date + icuidx08_d_en_fr_ga | "fr-x-icu" | up to date + icuidx08_d_en_fr_ga | "ga-x-icu" | up to date + icuidx09_d_en_fr_ga | "en-x-icu" | up to date + icuidx09_d_en_fr_ga | "fr-x-icu" | up to date + icuidx09_d_en_fr_ga | "ga-x-icu" | up to date + icuidx10_d_en_fr_ga_es | "en-x-icu" | up to date + icuidx10_d_en_fr_ga_es | "es-x-icu" | up to date + icuidx10_d_en_fr_ga_es | "fr-x-icu" | up to date + icuidx10_d_en_fr_ga_es | "ga-x-icu" | up to date + icuidx11_d_es | "default" | up to date + icuidx11_d_es | "es-x-icu" | up to date + icuidx12_custom | "default" | up to date + icuidx12_custom | custom | up to date + icuidx13_custom | "default" | up to date + icuidx13_custom | custom | up to date + icuidx14_myrange | "default" | up to date + icuidx15_myrange_en_fr_ga | "en-x-icu" | up to date + icuidx15_myrange_en_fr_ga | "fr-x-icu" | up to date + icuidx15_myrange_en_fr_ga | "ga-x-icu" | up to date + icuidx16_mood | "fr-x-icu" | up to date + icuidx17_part | "en-x-icu" | up to date + icuidx18_hash_d_es | "es-x-icu" | version not tracked + icuidx19_hash_id_d_es_eq | "default" | up to date + icuidx19_hash_id_d_es_eq | "es-x-icu" | up to date + icuidx20_hash_id_d_es_lt | "default" | up to date + icuidx20_hash_id_d_es_lt | "es-x-icu" | up to date +(49 rows) + +-- Validate that REINDEX will update the stored version. +UPDATE pg_depend SET refobjversion = 'not a version' +WHERE refclassid = 'pg_collation'::regclass +AND objid::regclass::text LIKE 'icuidx%' +AND refobjversion IS NOT NULL; +REINDEX TABLE collate_test; +REINDEX TABLE collate_part_0; +REINDEX TABLE collate_part_1; +SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version'; + objid +------- +(0 rows) + +-- Validate that REINDEX CONCURRENTLY will update the stored version. +UPDATE pg_depend SET refobjversion = 'not a version' +WHERE refclassid = 'pg_collation'::regclass +AND objid::regclass::text LIKE 'icuidx%' +AND refobjversion IS NOT NULL; +REINDEX TABLE CONCURRENTLY collate_test; +REINDEX TABLE CONCURRENTLY collate_part_0; +REINDEX INDEX CONCURRENTLY icuidx17_part; +SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version'; + objid +------- +(0 rows) + +-- Validate that VACUUM FULL will update the stored version. +UPDATE pg_depend SET refobjversion = 'not a version' +WHERE refclassid = 'pg_collation'::regclass +AND objid::regclass::text LIKE 'icuidx%' +AND refobjversion IS NOT NULL; +VACUUM FULL collate_test; +VACUUM FULL collate_part_0; +VACUUM FULL collate_part_1; +SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version'; + objid +------- +(0 rows) + -- cleanup RESET search_path; SET client_min_messages TO warning; diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out index 64c0c66859..d883ee58d1 100644 --- a/src/test/regress/expected/create_index.out +++ b/src/test/regress/expected/create_index.out @@ -2065,14 +2065,16 @@ WHERE classid = 'pg_class'::regclass AND obj | objref | deptype ------------------------------------------+------------------------------------------------------------+--------- index concur_reindex_ind1 | constraint concur_reindex_ind1 on table concur_reindex_tab | i + index concur_reindex_ind2 | collation "default" | n index concur_reindex_ind2 | column c2 of table concur_reindex_tab | a index concur_reindex_ind3 | column c1 of table concur_reindex_tab | a index concur_reindex_ind3 | table concur_reindex_tab | a + index concur_reindex_ind4 | collation "default" | n index concur_reindex_ind4 | column c1 of table concur_reindex_tab | a index concur_reindex_ind4 | column c2 of table concur_reindex_tab | a materialized view concur_reindex_matview | schema public | n table concur_reindex_tab | schema public | n -(8 rows) +(10 rows) REINDEX INDEX CONCURRENTLY concur_reindex_ind1; REINDEX TABLE CONCURRENTLY concur_reindex_tab; @@ -2092,14 +2094,16 @@ WHERE classid = 'pg_class'::regclass AND obj | objref | deptype ------------------------------------------+------------------------------------------------------------+--------- index concur_reindex_ind1 | constraint concur_reindex_ind1 on table concur_reindex_tab | i + index concur_reindex_ind2 | collation "default" | n index concur_reindex_ind2 | column c2 of table concur_reindex_tab | a index concur_reindex_ind3 | column c1 of table concur_reindex_tab | a index concur_reindex_ind3 | table concur_reindex_tab | a + index concur_reindex_ind4 | collation "default" | n index concur_reindex_ind4 | column c1 of table concur_reindex_tab | a index concur_reindex_ind4 | column c2 of table concur_reindex_tab | a materialized view concur_reindex_matview | schema public | n table concur_reindex_tab | schema public | n -(8 rows) +(10 rows) -- Check that comments are preserved CREATE TABLE testcomment (i int); diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql index 35acf91fbf..e93530af55 100644 --- a/src/test/regress/sql/collate.icu.utf8.sql +++ b/src/test/regress/sql/collate.icu.utf8.sql @@ -716,6 +716,112 @@ INSERT INTO test33 VALUES (2, 'DEF'); -- they end up in the same partition (but it's platform-dependent which one) SELECT (SELECT count(*) FROM test33_0) <> (SELECT count(*) FROM test33_1); +-- collation versioning support +CREATE TYPE t_en_fr AS (fr text COLLATE "fr-x-icu", en text COLLATE "en-x-icu"); +CREATE DOMAIN d_en_fr AS t_en_fr; +CREATE DOMAIN d_es AS text COLLATE "es-x-icu"; +CREATE TYPE t_en_fr_ga AS (en_fr t_en_fr, ga text COLLATE "ga-x-icu"); +CREATE DOMAIN d_en_fr_ga AS t_en_fr_ga; +CREATE TYPE t_custom AS (meh text, meh2 text); +CREATE DOMAIN d_custom AS t_custom; + +CREATE COLLATION custom ( + LOCALE = 'fr-x-icu', PROVIDER = 'icu' +); + +CREATE TYPE myrange AS range (subtype = text); +CREATE TYPE myrange_en_fr_ga AS range(subtype = t_en_fr_ga); + +CREATE TYPE mood AS ENUM ('sad', 'ok', 'happy'); + +CREATE TABLE collate_test ( + id integer, + t_en_fr t_en_fr, + d_en_fr d_en_fr, + d_es d_es, + t_en_fr_ga t_en_fr_ga, + d_en_fr_ga d_en_fr_ga, + d_en_fr_ga_arr d_en_fr_ga[], + myrange myrange, + myrange_en_fr_ga myrange_en_fr_ga, + mood mood +); + +CREATE INDEX icuidx01_t_en_fr__d_es ON collate_test (t_en_fr, d_es); +CREATE INDEX icuidx02_d_en_fr ON collate_test (d_en_fr); +CREATE INDEX icuidx03_t_en_fr_ga ON collate_test (t_en_fr_ga); +CREATE INDEX icuidx04_d_en_fr_ga ON collate_test (d_en_fr_ga); +CREATE INDEX icuidx05_d_en_fr_ga_arr ON collate_test (d_en_fr_ga_arr); +CREATE INDEX icuidx06_d_en_fr_ga ON collate_test(id) WHERE (d_en_fr_ga).en_fr.fr = 'foo'; +CREATE INDEX icuidx07_d_en_fr_ga ON collate_test(id) WHERE (d_en_fr_ga).ga = 'foo'; +CREATE INDEX icuidx08_d_en_fr_ga ON collate_test(id) WHERE (t_en_fr_ga) = ('foo', 'bar', 'baz'); +CREATE INDEX icuidx09_d_en_fr_ga ON collate_test(id) WHERE (d_en_fr_ga) = ('foo', 'bar', 'baz'); +CREATE INDEX icuidx10_d_en_fr_ga_es ON collate_test(id) WHERE (d_en_fr_ga) = ('foo', 'bar', 'baz' COLLATE "es-x-icu"); +CREATE INDEX icuidx11_d_es ON collate_test(id) WHERE (d_es) = ('foo'); +CREATE INDEX icuidx12_custom ON collate_test(id) WHERE ('foo', 'bar')::d_custom = ('foo', 'bar' collate custom)::d_custom; +CREATE INDEX icuidx13_custom ON collate_test(id) WHERE ('foo' collate custom, 'bar')::d_custom = ('foo', 'bar')::d_custom; +CREATE INDEX icuidx14_myrange ON collate_test(myrange); +CREATE INDEX icuidx15_myrange_en_fr_ga ON collate_test USING gist (myrange_en_fr_ga); +CREATE INDEX icuidx16_mood ON collate_test(id) WHERE mood > 'ok' collate "fr-x-icu"; + +CREATE TABLE collate_part(id integer, val text COLLATE "en-x-icu") PARTITION BY range(id); +CREATE TABLE collate_part_0 PARTITION OF collate_part FOR VALUES FROM (0) TO (1); +CREATE TABLE collate_part_1 PARTITION OF collate_part FOR VALUES FROM (1) TO (1000000); +CREATE INDEX icuidx17_part ON collate_part_1 (val); +-- for key columns, hash indexes should record dependency on the collation but +-- not the version +CREATE INDEX icuidx18_hash_d_es ON collate_test USING hash (d_es); +CREATE INDEX icuidx19_hash_id_d_es_eq ON collate_test USING hash (id) WHERE (d_es) = 'foo'; +CREATE INDEX icuidx20_hash_id_d_es_lt ON collate_test USING hash (id) WHERE (d_es) < 'foo'; + +SELECT objid::regclass, refobjid::regcollation, +CASE +WHEN refobjversion IS NULL THEN 'version not tracked' +ELSE CASE + WHEN refobjversion = pg_collation_actual_version(refobjid) THEN 'up to date' + ELSE 'out of date' + END +END AS version +FROM pg_depend d +LEFT JOIN pg_class c ON c.oid = d.objid +WHERE refclassid = 'pg_collation'::regclass +AND coalesce(relkind, 'i') = 'i' +AND relname LIKE 'icuidx%' +ORDER BY 1, 2; + +-- Validate that REINDEX will update the stored version. +UPDATE pg_depend SET refobjversion = 'not a version' +WHERE refclassid = 'pg_collation'::regclass +AND objid::regclass::text LIKE 'icuidx%' +AND refobjversion IS NOT NULL; + +REINDEX TABLE collate_test; +REINDEX TABLE collate_part_0; +REINDEX TABLE collate_part_1; + +SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version'; + +-- Validate that REINDEX CONCURRENTLY will update the stored version. +UPDATE pg_depend SET refobjversion = 'not a version' +WHERE refclassid = 'pg_collation'::regclass +AND objid::regclass::text LIKE 'icuidx%' +AND refobjversion IS NOT NULL; +REINDEX TABLE CONCURRENTLY collate_test; +REINDEX TABLE CONCURRENTLY collate_part_0; +REINDEX INDEX CONCURRENTLY icuidx17_part; + +SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version'; + +-- Validate that VACUUM FULL will update the stored version. +UPDATE pg_depend SET refobjversion = 'not a version' +WHERE refclassid = 'pg_collation'::regclass +AND objid::regclass::text LIKE 'icuidx%' +AND refobjversion IS NOT NULL; +VACUUM FULL collate_test; +VACUUM FULL collate_part_0; +VACUUM FULL collate_part_1; + +SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version'; -- cleanup RESET search_path; -- 2.20.1 --9jxsPFA5p3P2qPhR Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="v29-0005-Add-ALTER-INDEX-.-ALTER-COLLATION-.-REFRESH-VERS.patch" ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify @ 2024-07-02 12:59 Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Heikki Linnakangas @ 2024-07-02 12:59 UTC (permalink / raw) To: Paul Jungwirth <[email protected]>; PostgreSQL Hackers <[email protected]> On 28/06/2024 01:01, Paul Jungwirth wrote: > If it seems good to let people use SupportRequestSimplify to make their SRFs be inlineable, I'm > happy to add tests and docs. We should really document the idea of inlined functions in general, so > I'll do that too. > > Another approach I considered is using a separate support request, e.g. SupportRequestInlineSRF, and > just calling it from inline_set_returning_function. I didn't like having two support requests that > did almost exactly the same thing. OTOH my current approach means you'll get an error if you do this: > > ``` > postgres=# select temporal_semijoin('employees', 'id', 'valid_at', 'positions', 'employee_id', > 'valid_at'); > ERROR: unrecognized node type: 66 > ``` > > I'll look into ways to fix that. If the support function returns a Query, we end up having a FuncExpr with a Query in the tree. A Query isnt an Expr, which is why you get that error, and it seems like a recipe for confusion in general. Perhaps returning a SubLink would be better. I think we should actually add an assertion after the call to the SupportRequestSimplify support function, to check that it returned an Expr node. +1 to the general feature of letting SRFs be simplified by the support function. > I think SupportRequestSimplify is a really cool feature. It is nearly like having macros. > I'm dreaming about other ways I can (ab)use it. :-D -- Heikki Linnakangas Neon (https://neon.tech) ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> @ 2024-07-26 18:58 ` Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Tom Lane @ 2024-07-26 18:58 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Paul Jungwirth <[email protected]>; PostgreSQL Hackers <[email protected]> Heikki Linnakangas <[email protected]> writes: > On 28/06/2024 01:01, Paul Jungwirth wrote: >> Another approach I considered is using a separate support request, e.g. SupportRequestInlineSRF, and >> just calling it from inline_set_returning_function. I didn't like having two support requests that >> did almost exactly the same thing. OTOH my current approach means you'll get an error if you do this: >> >> ``` >> postgres=# select temporal_semijoin('employees', 'id', 'valid_at', 'positions', 'employee_id', >> 'valid_at'); >> ERROR: unrecognized node type: 66 >> ``` >> >> I'll look into ways to fix that. I like this idea, but I like exactly nothing about this implementation. The right thing is to have a separate SupportRequestInlineSRF request that is called directly by inline_set_returning_function. It might be "almost the same thing" as SupportRequestSimplify, but "almost" only counts in horseshoes and hand grenades. In particular, returning a Query node is simply broken for SupportRequestSimplify (as your example demonstrates), whereas it's the only correct result for SupportRequestInlineSRF. You could imagine keeping it to one support request by adding a boolean field to the request struct to show which behavior is wanted, but I think the principal result of that would be to break extensions that weren't expecting such calls. The defined mechanism for extending the SupportRequest protocol is to add new support request codes, not to whack around the APIs of existing ones. > I think we should actually add an assertion after the call to the > SupportRequestSimplify support function, to check that it returned an > Expr node. Um ... IsA(node, Expr) isn't going to work, and I'm not sure that it'd be useful to try to enumerate the set of Expr subtypes that should be allowed there. But possibly it'd be worth asserting that it's not a Query, just in case anyone gets confused about the difference between SupportRequestSimplify and SupportRequestInlineSRF. It would be good to have an in-core test case for this request type, but I don't really see any built-in SRFs for which expansion as a sub-SELECT would be an improvement. regards, tom lane ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> @ 2024-08-30 16:26 ` Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Paul Jungwirth @ 2024-08-30 16:26 UTC (permalink / raw) To: Tom Lane <[email protected]>; Heikki Linnakangas <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On 7/26/24 11:58, Tom Lane wrote: > Heikki Linnakangas <[email protected]> writes: >> On 28/06/2024 01:01, Paul Jungwirth wrote: >>> Another approach I considered is using a separate support request, e.g. SupportRequestInlineSRF, and >>> just calling it from inline_set_returning_function. I didn't like having two support requests that >>> did almost exactly the same thing. OTOH my current approach means you'll get an error if you do this: >>> >>> ``` >>> postgres=# select temporal_semijoin('employees', 'id', 'valid_at', 'positions', 'employee_id', >>> 'valid_at'); >>> ERROR: unrecognized node type: 66 >>> ``` >>> >>> I'll look into ways to fix that. > > I like this idea, but I like exactly nothing about this implementation. > The right thing is to have a separate SupportRequestInlineSRF request > that is called directly by inline_set_returning_function. Here are new patches using a new SupportRequestInlineSRF request type. They include patches and documentation. The patches handle this: SELECT * FROM srf(); but not this: SELECT srf(); In the latter case, Postgres always calls the function in "materialized mode" and gets the whole result up front, so inline_set_returning_function is never called, even for SQL functions. For tests I added a `foo_from_bar(colname, tablename, filter)` PL/pgSQL function that does `SELECT $colname FROM $tablename [WHERE $colname = $filter]`, then the support function generates the same SQL and turns it into a Query node. This matches how I want to use the feature for my temporal_semijoin etc functions. If you give a non-NULL filter, you get a Query with a Var node, so we are testing something that isn't purely Const. The SupportRequestSimplify type has some comments about supporting operators, but I don't think you can have a set-returning operator, so I didn't repeat those comments for this new type. I split things up into three patch files because I couldn't get git to gracefully handle shifting a large block of code into an if statement. The first two patches have no changes except that indentation (and initializing one variable to NULL). They aren't meant to be committed separately. Rebased to a83a944e9f. Yours, -- Paul ~{:-) [email protected] Attachments: [text/x-patch] v2-0001-Add-indented-section.patch (3.2K, ../../[email protected]/2-v2-0001-Add-indented-section.patch) download | inline diff: From 591de3a06ec97cf1bff10d603946f0bac176b9d9 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <[email protected]> Date: Thu, 29 Aug 2024 18:17:55 -0700 Subject: [PATCH v2 1/3] Add indented section --- src/backend/optimizer/util/clauses.c | 74 +++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index b4e085e9d4b..ef8282ded49 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5069,7 +5069,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) TupleDesc rettupdesc; List *raw_parsetree_list; List *querytree_list; - Query *querytree; + Query *querytree = NULL; Assert(rte->rtekind == RTE_FUNCTION); @@ -5235,6 +5235,78 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) goto fail; querytree = linitial(querytree_list); } + if (!querytree) + { + /* Fetch the function body */ + tmp = SysCacheGetAttrNotNull(PROCOID, func_tuple, Anum_pg_proc_prosrc); + src = TextDatumGetCString(tmp); + + /* + * Setup error traceback support for ereport(). This is so that we can + * finger the function that bad information came from. + */ + callback_arg.proname = NameStr(funcform->proname); + callback_arg.prosrc = src; + + sqlerrcontext.callback = sql_inline_error_callback; + sqlerrcontext.arg = (void *) &callback_arg; + sqlerrcontext.previous = error_context_stack; + error_context_stack = &sqlerrcontext; + + /* If we have prosqlbody, pay attention to that not prosrc */ + tmp = SysCacheGetAttr(PROCOID, + func_tuple, + Anum_pg_proc_prosqlbody, + &isNull); + if (!isNull) + { + Node *n; + + n = stringToNode(TextDatumGetCString(tmp)); + if (IsA(n, List)) + querytree_list = linitial_node(List, castNode(List, n)); + else + querytree_list = list_make1(n); + if (list_length(querytree_list) != 1) + goto fail; + querytree = linitial(querytree_list); + + /* Acquire necessary locks, then apply rewriter. */ + AcquireRewriteLocks(querytree, true, false); + querytree_list = pg_rewrite_query(querytree); + if (list_length(querytree_list) != 1) + goto fail; + querytree = linitial(querytree_list); + } + else + { + /* + * Set up to handle parameters while parsing the function body. We + * can use the FuncExpr just created as the input for + * prepare_sql_fn_parse_info. + */ + pinfo = prepare_sql_fn_parse_info(func_tuple, + (Node *) fexpr, + fexpr->inputcollid); + + /* + * Parse, analyze, and rewrite (unlike inline_function(), we can't + * skip rewriting here). We can fail as soon as we find more than one + * query, though. + */ + raw_parsetree_list = pg_parse_query(src); + if (list_length(raw_parsetree_list) != 1) + goto fail; + + querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), + src, + (ParserSetupHook) sql_fn_parser_setup, + pinfo, NULL); + if (list_length(querytree_list) != 1) + goto fail; + querytree = linitial(querytree_list); + } + } /* * Also resolve the actual function result tupdesc, if composite. If we -- 2.42.0 [text/x-patch] v2-0002-Remove-unindented-section.patch (2.9K, ../../[email protected]/3-v2-0002-Remove-unindented-section.patch) download | inline diff: From 7a9ee15d17bee6a6743bf18e7428e6594e17be35 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <[email protected]> Date: Thu, 29 Aug 2024 18:18:20 -0700 Subject: [PATCH v2 2/3] Remove unindented section --- src/backend/optimizer/util/clauses.c | 69 ---------------------------- 1 file changed, 69 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index ef8282ded49..d2d8f7eb0c0 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5166,75 +5166,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) ALLOCSET_DEFAULT_SIZES); oldcxt = MemoryContextSwitchTo(mycxt); - /* Fetch the function body */ - tmp = SysCacheGetAttrNotNull(PROCOID, func_tuple, Anum_pg_proc_prosrc); - src = TextDatumGetCString(tmp); - - /* - * Setup error traceback support for ereport(). This is so that we can - * finger the function that bad information came from. - */ - callback_arg.proname = NameStr(funcform->proname); - callback_arg.prosrc = src; - - sqlerrcontext.callback = sql_inline_error_callback; - sqlerrcontext.arg = (void *) &callback_arg; - sqlerrcontext.previous = error_context_stack; - error_context_stack = &sqlerrcontext; - - /* If we have prosqlbody, pay attention to that not prosrc */ - tmp = SysCacheGetAttr(PROCOID, - func_tuple, - Anum_pg_proc_prosqlbody, - &isNull); - if (!isNull) - { - Node *n; - - n = stringToNode(TextDatumGetCString(tmp)); - if (IsA(n, List)) - querytree_list = linitial_node(List, castNode(List, n)); - else - querytree_list = list_make1(n); - if (list_length(querytree_list) != 1) - goto fail; - querytree = linitial(querytree_list); - - /* Acquire necessary locks, then apply rewriter. */ - AcquireRewriteLocks(querytree, true, false); - querytree_list = pg_rewrite_query(querytree); - if (list_length(querytree_list) != 1) - goto fail; - querytree = linitial(querytree_list); - } - else - { - /* - * Set up to handle parameters while parsing the function body. We - * can use the FuncExpr just created as the input for - * prepare_sql_fn_parse_info. - */ - pinfo = prepare_sql_fn_parse_info(func_tuple, - (Node *) fexpr, - fexpr->inputcollid); - - /* - * Parse, analyze, and rewrite (unlike inline_function(), we can't - * skip rewriting here). We can fail as soon as we find more than one - * query, though. - */ - raw_parsetree_list = pg_parse_query(src); - if (list_length(raw_parsetree_list) != 1) - goto fail; - - querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), - src, - (ParserSetupHook) sql_fn_parser_setup, - pinfo, NULL); - if (list_length(querytree_list) != 1) - goto fail; - querytree = linitial(querytree_list); - } if (!querytree) { /* Fetch the function body */ -- 2.42.0 [text/x-patch] v2-0003-Inline-non-SQL-SRFs-using-SupportRequestInlineSRF.patch (22.9K, ../../[email protected]/4-v2-0003-Inline-non-SQL-SRFs-using-SupportRequestInlineSRF.patch) download | inline diff: From 4ed50cad16901c5b7b3447bad7105de11a81773a Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <[email protected]> Date: Thu, 29 Aug 2024 18:20:47 -0700 Subject: [PATCH v2 3/3] Inline non-SQL SRFs using SupportRequestInlineSRF By inlining functions written in PL/pgSQL (or other languages), we let users compose dynamic queries without paying the overhead of function invocations. Formerly this was only possible for SQL functions. If the support request returns a Query node, then inline_set_returning_function will use it in place of the FuncExpr. The Query node can reference the original function's arguments as Var nodes. We only try to inline SRFs (even SQL SRFs) when they are used as RTEs, so inlining happens for `SELECT * FROM foo()` but not for `SELECT foo()`. --- doc/src/sgml/xfunc.sgml | 14 ++ src/backend/optimizer/util/clauses.c | 63 +++++- src/include/nodes/supportnodes.h | 22 ++ src/test/regress/expected/misc_functions.out | 212 +++++++++++++++++++ src/test/regress/regress.c | 91 ++++++++ src/test/regress/sql/misc_functions.sql | 87 ++++++++ 6 files changed, 487 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 9bc23a9a938..311a87b14a5 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -4076,6 +4076,20 @@ supportfn(internal) returns internal expression and an actual execution of the target function. </para> + <para> + Similarly, a <link linkend="queries-tablefunction">set-returning function</link> + can implement <literal>SupportRequestInlineSRF</literal> to return a + <literal>Query</literal> node, which the planner will try to inline into + the outer query, just as <productname>PostgreSQL</productname> inlines + SQL functions. Normallly only SQL functions can be inlined, but this support + request allows a function in <link linkend="plpgsql">PL/pgSQL</link> + or another language to build a dynamic SQL query and have it inlined too. + It is the responsibility of the support function to return + a node that matches the parent function's implementation. One way to do this is + to build a SQL string then parse it with <literal>pg_parse_query</literal>. + See <literal>regress.c</literal> for an example. + </para> + <para> For target functions that return <type>boolean</type>, it is often useful to estimate the fraction of rows that will be selected by a <literal>WHERE</literal> clause using that diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index d2d8f7eb0c0..9ee5831ed0e 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5134,7 +5134,8 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) funcform = (Form_pg_proc) GETSTRUCT(func_tuple); /* - * Forget it if the function is not SQL-language or has other showstopper + * Forget it unless the function is SQL-language or provides a support function. + * Also check for other showstopper * properties. In particular it mustn't be declared STRICT, since we * couldn't enforce that. It also mustn't be VOLATILE, because that is * supposed to cause it to be executed with its own snapshot, rather than @@ -5143,7 +5144,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) * of the function's last SELECT, which should not happen in that case. * (Rechecking prokind, proretset, and pronargs is just paranoia.) */ - if (funcform->prolang != SQLlanguageId || + if ((funcform->prolang != SQLlanguageId && !funcform->prosupport) || funcform->prokind != PROKIND_FUNCTION || funcform->proisstrict || funcform->provolatile == PROVOLATILE_VOLATILE || @@ -5166,6 +5167,64 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) ALLOCSET_DEFAULT_SIZES); oldcxt = MemoryContextSwitchTo(mycxt); + /* + * If the function has a SupportRequestInlineSRF support function, + * see if it can produce a Query node we can inline. + * This can be mutually-exclusive with SQL functions. + * Those are inlineable already, so it doesn't make sense to + * attach such a support request to them. + */ + if (funcform->prolang != SQLlanguageId && funcform->prosupport) { + SupportRequestInlineSRF req; + FuncExpr dummy_fexpr; + Node *newnode; + + /* + * We don't need the whole sqlerrcontext, + * but make sure we restore it correctly + * if we goto fail. + */ + sqlerrcontext.previous = error_context_stack; + + /* + * Build a SupportRequestInlineSRF node to pass to the support + * function, pointing to a dummy FuncExpr node containing the + * simplified arg list. We use this approach to present a uniform + * interface to the support function regardless of how the target + * function is actually being invoked. + */ + + dummy_fexpr.xpr.type = T_FuncExpr; + dummy_fexpr.funcid = func_oid; + dummy_fexpr.funcresulttype = funcform->prorettype; + dummy_fexpr.funcretset = funcform->proretset; + dummy_fexpr.funcvariadic = OidIsValid(funcform->provariadic); + dummy_fexpr.funcformat = COERCE_EXPLICIT_CALL; + dummy_fexpr.funccollid = InvalidOid; + dummy_fexpr.inputcollid = InvalidOid; + dummy_fexpr.args = fexpr->args; + dummy_fexpr.location = -1; + + req.type = T_SupportRequestInlineSRF; + req.fcall = &dummy_fexpr; + + newnode = (Node *) + DatumGetPointer(OidFunctionCall1(funcform->prosupport, + PointerGetDatum(&req))); + + if (!newnode) + goto fail; + + if (!IsA(newnode, Query)) + elog(ERROR, + "Got unexpected node type %d from SupportRequestInlineSRF for function %s", + newnode->type, NameStr(funcform->proname)); + + querytree = (Query *) newnode; + querytree_list = list_make1(querytree); + } + + if (!querytree) { /* Fetch the function body */ diff --git a/src/include/nodes/supportnodes.h b/src/include/nodes/supportnodes.h index 5f7bcde891c..bc0b5cccaf3 100644 --- a/src/include/nodes/supportnodes.h +++ b/src/include/nodes/supportnodes.h @@ -69,6 +69,28 @@ typedef struct SupportRequestSimplify FuncExpr *fcall; /* Function call to be simplified */ } SupportRequestSimplify; +/* + * The InlineSRF request allows the support function to perform plan-time + * simplification of a call to its target set-returning function. For + * example a plpgsql function could build a dynamic SQL query and execute it. + * Normally only SQL functions can be inlined, but with this support function + * the dynamic query can be inlined as well. + * + * "fcall" will be a FuncExpr invoking the support function's target + * function. + * + * The result should be a semantically-equivalent transformed node tree, + * or NULL if no simplification could be performed. Do *not* return or + * modify *fcall, as it isn't really a separately allocated Node. But + * it's okay to use fcall->args, or parts of it, in the result tree. + */ +typedef struct SupportRequestInlineSRF +{ + NodeTag type; + + FuncExpr *fcall; /* Function call to be simplified */ +} SupportRequestInlineSRF; + /* * The Selectivity request allows the support function to provide a * selectivity estimate for a function appearing at top level of a WHERE diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index 35fb72f302b..c5bac6d43ea 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -595,6 +595,11 @@ CREATE FUNCTION test_support_func(internal) RETURNS internal AS :'regresslib', 'test_support_func' LANGUAGE C STRICT; +-- With a support function that inlines SRFs +CREATE FUNCTION test_inline_srf_support_func(internal) + RETURNS internal + AS :'regresslib', 'test_inline_srf_support_func' + LANGUAGE C STRICT; ALTER FUNCTION my_int_eq(int, int) SUPPORT test_support_func; EXPLAIN (COSTS OFF) SELECT * FROM tenk1 a JOIN tenk1 b ON a.unique1 = b.unique1 @@ -706,6 +711,213 @@ false, true, false, true); -- the support function. SELECT * FROM generate_series(TIMESTAMPTZ '2024-02-01', TIMESTAMPTZ '2024-03-01', INTERVAL '0 day') g(s); ERROR: step size cannot equal zero +-- +-- Test inlining PL/pgSQL functions +-- +-- RETURNS SETOF TEXT: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF TEXT +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo_from_bar +-------------- + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo_from_bar +-------------- + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; +-- RETURNS SETOF RECORD: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF RECORD +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + QUERY PLAN +-------------------------------------------------------------------------- + Function Scan on foo_from_bar bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + QUERY PLAN +-------------------------------------------------------------------------- + Function Scan on foo_from_bar bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; +-- RETURNS TABLE: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS TABLE(foo TEXT) +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); ok diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index 14aad5a0c6e..9f97ad6ad45 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -29,6 +29,7 @@ #include "commands/sequence.h" #include "commands/trigger.h" #include "executor/executor.h" +#include "executor/functions.h" #include "executor/spi.h" #include "funcapi.h" #include "mb/pg_wchar.h" @@ -39,11 +40,13 @@ #include "parser/parse_coerce.h" #include "port/atomics.h" #include "storage/spin.h" +#include "tcop/tcopprot.h" #include "utils/array.h" #include "utils/builtins.h" #include "utils/geo_decls.h" #include "utils/memutils.h" #include "utils/rel.h" +#include "utils/syscache.h" #include "utils/typcache.h" #define EXPECT_TRUE(expr) \ @@ -989,6 +992,94 @@ test_support_func(PG_FUNCTION_ARGS) PG_RETURN_POINTER(ret); } +PG_FUNCTION_INFO_V1(test_inline_srf_support_func); +Datum +test_inline_srf_support_func(PG_FUNCTION_ARGS) +{ + Node *rawreq = (Node *) PG_GETARG_POINTER(0); + + if (IsA(rawreq, SupportRequestInlineSRF)) + { + /* + * Assume that the target is foo_from_bar; that's safe as long + * as we don't attach this to any other set-returning function. + */ + SupportRequestInlineSRF *req = (SupportRequestInlineSRF *) rawreq; + StringInfoData sql; + FuncExpr *expr = req->fcall; + Node *node; + Const *c; + char *colname; + char *tablename; + HeapTuple func_tuple; + SQLFunctionParseInfoPtr pinfo; + List *raw_parsetree_list; + List *querytree_list; + Query *querytree; + + if (list_length(expr->args) != 3) + ereport(ERROR, (errmsg("test_inline_srf_support_func called with %d args but expected 3", list_length(expr->args)))); + + /* Get colname */ + node = linitial(expr->args); + if (!IsA(node, Const)) + ereport(ERROR, (errmsg("test_inline_srf_support_func called with non-Const parameters"))); + c = (Const *) node; + if (c->consttype != TEXTOID) + ereport(ERROR, (errmsg("test_inline_srf_support_func called with non-TEXT parameters"))); + colname = TextDatumGetCString(c->constvalue); + + /* Get tablename */ + node = lsecond(expr->args); + if (!IsA(node, Const)) + ereport(ERROR, (errmsg("test_inline_srf_support_func called with non-Const parameters"))); + c = (Const *) node; + if (c->consttype != TEXTOID) + ereport(ERROR, (errmsg("test_inline_srf_support_func called with non-TEXT parameters"))); + tablename = TextDatumGetCString(c->constvalue); + + initStringInfo(&sql); + appendStringInfo(&sql, "SELECT %s::text FROM %s", quote_identifier(colname), quote_identifier(tablename)); + + /* Get filter if present */ + node = lthird(expr->args); + if (!(IsA(node, Const) && ((Const *) node)->constisnull)) + { + appendStringInfo(&sql, " WHERE %s::text = $3", quote_identifier(colname)); + } + + func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(expr->funcid)); + if (!HeapTupleIsValid(func_tuple)) + elog(ERROR, "cache lookup failed for function %u", expr->funcid); + + pinfo = prepare_sql_fn_parse_info(func_tuple, + (Node *) expr, + expr->inputcollid); + + ReleaseSysCache(func_tuple); + + raw_parsetree_list = pg_parse_query(sql.data); + if (list_length(raw_parsetree_list) != 1) + elog(ERROR, "test_inline_srf_support_func parsed to more than one node"); + + querytree_list = pg_analyze_and_rewrite_withcb( + linitial(raw_parsetree_list), + sql.data, + (ParserSetupHook) sql_fn_parser_setup, + pinfo, NULL); + if (list_length(querytree_list) != 1) + elog(ERROR, "test_inline_srf_support_func rewrote to more than one node"); + + querytree = linitial(querytree_list); + if (!IsA(querytree, Query)) + ereport(ERROR, (errmsg("test_inline_srf_support_func didn't parse to a Query"), + errdetail("Got this instead: %s", nodeToString(querytree)))); + PG_RETURN_POINTER(querytree); + } + + PG_RETURN_POINTER(NULL); +} + PG_FUNCTION_INFO_V1(test_opclass_options_func); Datum test_opclass_options_func(PG_FUNCTION_ARGS) diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index e570783453c..bf692edd0b7 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -247,6 +247,12 @@ CREATE FUNCTION test_support_func(internal) AS :'regresslib', 'test_support_func' LANGUAGE C STRICT; +-- With a support function that inlines SRFs +CREATE FUNCTION test_inline_srf_support_func(internal) + RETURNS internal + AS :'regresslib', 'test_inline_srf_support_func' + LANGUAGE C STRICT; + ALTER FUNCTION my_int_eq(int, int) SUPPORT test_support_func; EXPLAIN (COSTS OFF) @@ -265,6 +271,8 @@ SELECT * FROM tenk1 a JOIN my_gen_series(1,1000) g ON a.unique1 = g; EXPLAIN (COSTS OFF) SELECT * FROM tenk1 a JOIN my_gen_series(1,10) g ON a.unique1 = g; + + -- -- Test the SupportRequestRows support function for generate_series_timestamp() -- @@ -310,6 +318,85 @@ false, true, false, true); -- the support function. SELECT * FROM generate_series(TIMESTAMPTZ '2024-02-01', TIMESTAMPTZ '2024-03-01', INTERVAL '0 day') g(s); +-- +-- Test inlining PL/pgSQL functions +-- +-- RETURNS SETOF TEXT: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF TEXT +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +DROP FUNCTION foo_from_bar; +-- RETURNS SETOF RECORD: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF RECORD +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +DROP FUNCTION foo_from_bar; +-- RETURNS TABLE: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS TABLE(foo TEXT) +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +DROP FUNCTION foo_from_bar; + -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); SELECT count(*) > 0 AS ok FROM pg_control_init(); -- 2.42.0 ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> @ 2024-09-03 16:42 ` Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Tom Lane @ 2024-09-03 16:42 UTC (permalink / raw) To: Paul Jungwirth <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; PostgreSQL Hackers <[email protected]> Paul Jungwirth <[email protected]> writes: > Here are new patches using a new SupportRequestInlineSRF request type. They include patches and > documentation. I took a look through this. I feel like we're still some way away from having something committable. I've got two main complaint areas: 1. It doesn't seem like integrating this into inline_set_returning_function was the right thing after all, or maybe just the way you did it isn't right. That function is pretty opinionated about what it is doing, and a lot of what it is doing doesn't seem appropriate for a support-function-driven substitution. As an example, it rejects WITH ORDINALITY, but who's to say that a support function couldn't handle that? More generally, I'm not sure if it's appropriate to make any tests on the function's properties, rather than assuming the support function knows what it's doing. I see you already hacked up the test on prolang, but the others in the same if-clause seem equally dubious from here. I'm also unsure whether it's our business to reject volatile functions or subplans in the function arguments. (Maybe it is, but not sure.) There is also stuff towards the bottom of the function, particularly check_sql_fn_retval and parameter substitution, that I do not think makes sense to apply to a non-SQL-language function; but if I'm reading this right you run all that code on the support function's result. It does make sense to require there to be just one RangeTblFunction in the RTE, since it's not at all clear how we could combine the results if there's more than one. But I wonder if we should just pass the RTE node to the support function, and let it make its own decision about rte->funcordinality. Or if that seems like a bad idea, pass the RangeTblFunction node. I think it's essential to do one of those things rather than fake up a FuncExpr, because a support function for a function returning RECORD would likely need access to the column definition list to figure out what to do. I notice that in the case of non-SRF function inlining, we handle support-function calling in a totally separate function (simplify_function) rather than try to integrate it into the code that does SQL function inlining (inline_function). Maybe a similar approach should be adopted here. We could have a wrapper function that implements the parts worth sharing, such as looking up the target function's pg_proc entry and doing the permissions check. Or perhaps put that stuff into the sole caller, preprocess_function_rtes. If we do keep this in inline_set_returning_function, we need to pay more than zero attention to updating that function's header comment. 2. The documentation needs to be a great deal more explicit about what the function is supposed to return. It needs to be a SELECT Query node that has been through parse analysis and rewriting. I don't think pointing to a regression test function is adequate, or even appropriate. The test function is a pretty bad example as-is, too. It aggressively disregards the API recommendation in supportnodes.h: * Support functions must return a NULL pointer, not fail, if they do not * recognize the request node type or cannot handle the given case; this * allows for future extensions of the set of request cases. As a more minor nit, I think SupportRequestInlineSRF should include "struct PlannerInfo *root", for the same reasons that SupportRequestSimplify does. > I split things up into three patch files because I couldn't get git to gracefully handle shifting a > large block of code into an if statement. The first two patches have no changes except that > indentation (and initializing one variable to NULL). They aren't meant to be committed separately. A hack I've used in the past is to have the main patch just add + if (...) + { ... + } around the to-be-reindented code, and then apply pgindent as a separate patch step. (We used to just leave it to the committer to run pgindent, but I think nowadays the cfbot will whine at you if you submit not-pgindented code.) I think that's easier to review since the reviewer can mechanically verify the pgindent patch. This problem may be moot for this patch once we detangle the support function call from SQL-function inlining, though. regards, tom lane ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> @ 2025-06-30 21:42 ` Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Paul Jungwirth @ 2025-06-30 21:42 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; PostgreSQL Hackers <[email protected]> On 9/3/24 09:42, Tom Lane wrote: > Paul Jungwirth <[email protected]> writes: >> Here are new patches using a new SupportRequestInlineSRF request type. They include patches and >> documentation. > > I took a look through this. I feel like we're still some way away > from having something committable. I've got two main complaint > areas: > > 1. It doesn't seem like integrating this into > inline_set_returning_function was the right thing after all, or > maybe just the way you did it isn't right. > ... > 2. The documentation needs to be a great deal more explicit > about what the function is supposed to return. Thanks for the review . . . and your patience waiting for an update! I tried a few refactoring approaches but the nicest seemed to be to keep the shared parts in inline_set_returning_function, but have it call out to either inline_sql_set_returning_function or inline_set_returning_function_with_support. The first patch just refactors but doesn't yet add inline_set_returning_function_with_support, then the second patch adds the new functionality. The refactor lets us share lots of pre-condition checks, as well as parameter substitution into the Query result. In some cases the refactor changes the order of things, but all of those changes looked safe to me. I didn't love passing a SysCache HeapTuple into another function, but it does make the cleanup a little easier, since now we can always release it in the same place. The first patch gave me a wacky diff, but I couldn't get git to make something less fragmented. The idea is simple though: move part of inline_set_returning_function into inline_sql_set_returning_function, and call that instead. Rebased to 0836683a89. Yours, -- Paul ~{:-) [email protected] Attachments: [text/x-patch] v2-0001-Move-some-things-outside-of-inline_set_returning_.patch (10.0K, ../../[email protected]/2-v2-0001-Move-some-things-outside-of-inline_set_returning_.patch) download | inline diff: From 2a0d2bb6cc6fe25cab1065411ed4db17cc394c53 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <[email protected]> Date: Tue, 24 Jun 2025 19:10:15 -0700 Subject: [PATCH v2 1/2] Move some things outside of inline_set_returning_function. Added a new inline_sql_set_returning_function in preparation for inline_set_returning_function_with_support. Then inline_set_returning_function can call both, handling their shared needs itself. Author: Paul A. Jungwirth <[email protected]> --- src/backend/optimizer/util/clauses.c | 212 +++++++++++++++------------ 1 file changed, 122 insertions(+), 90 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 26a3e050086..a0dc5dfcd61 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5049,29 +5049,21 @@ evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod, /* - * inline_set_returning_function - * Attempt to "inline" a set-returning function in the FROM clause. - * - * "rte" is an RTE_FUNCTION rangetable entry. If it represents a call of a - * set-returning SQL function that can safely be inlined, expand the function - * and return the substitute Query structure. Otherwise, return NULL. + * inline_sql_set_returning_function * - * We assume that the RTE's expression has already been put through - * eval_const_expressions(), which among other things will take care of - * default arguments and named-argument notation. + * This implements inline_set_returning_function for sql-language functions. + * It parses the body (or uses the pre-parsed body if available). + * It allocates its own temporary MemoryContext for the parsing, then copies + * the result into the caller's context. * - * This has a good deal of similarity to inline_function(), but that's - * for the non-set-returning case, and there are enough differences to - * justify separate functions. + * Returns NULL if the function couldn't be inlined. */ -Query * -inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) +static Query * +inline_sql_set_returning_function(PlannerInfo *root, RangeTblEntry *rte, + RangeTblFunction *rtfunc, + FuncExpr *fexpr, Oid func_oid, HeapTuple func_tuple, + Form_pg_proc funcform) { - RangeTblFunction *rtfunc; - FuncExpr *fexpr; - Oid func_oid; - HeapTuple func_tuple; - Form_pg_proc funcform; char *src; Datum tmp; bool isNull; @@ -5088,37 +5080,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) Assert(rte->rtekind == RTE_FUNCTION); - /* - * It doesn't make a lot of sense for a SQL SRF to refer to itself in its - * own FROM clause, since that must cause infinite recursion at runtime. - * It will cause this code to recurse too, so check for stack overflow. - * (There's no need to do more.) - */ - check_stack_depth(); - - /* Fail if the RTE has ORDINALITY - we don't implement that here. */ - if (rte->funcordinality) - return NULL; - - /* Fail if RTE isn't a single, simple FuncExpr */ - if (list_length(rte->functions) != 1) - return NULL; - rtfunc = (RangeTblFunction *) linitial(rte->functions); - - if (!IsA(rtfunc->funcexpr, FuncExpr)) - return NULL; - fexpr = (FuncExpr *) rtfunc->funcexpr; - - func_oid = fexpr->funcid; - - /* - * The function must be declared to return a set, else inlining would - * change the results if the contained SELECT didn't return exactly one - * row. - */ - if (!fexpr->funcretset) - return NULL; - /* * Refuse to inline if the arguments contain any volatile functions or * sub-selects. Volatile functions are rejected because inlining may @@ -5132,22 +5093,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) contain_subplans((Node *) fexpr->args)) return NULL; - /* Check permission to call function (fail later, if not) */ - if (object_aclcheck(ProcedureRelationId, func_oid, GetUserId(), ACL_EXECUTE) != ACLCHECK_OK) - return NULL; - - /* Check whether a plugin wants to hook function entry/exit */ - if (FmgrHookIsNeeded(func_oid)) - return NULL; - - /* - * OK, let's take a look at the function's pg_proc entry. - */ - func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(func_oid)); - if (!HeapTupleIsValid(func_tuple)) - elog(ERROR, "cache lookup failed for function %u", func_oid); - funcform = (Form_pg_proc) GETSTRUCT(func_tuple); - /* * Forget it if the function is not SQL-language or has other showstopper * properties. In particular it mustn't be declared STRICT, since we @@ -5168,7 +5113,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) list_length(fexpr->args) != funcform->pronargs || !heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL)) { - ReleaseSysCache(func_tuple); return NULL; } @@ -5177,7 +5121,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) * that parsing might create. */ mycxt = AllocSetContextCreate(CurrentMemoryContext, - "inline_set_returning_function", + "inline_sql_set_returning_function", ALLOCSET_DEFAULT_SIZES); oldcxt = MemoryContextSwitchTo(mycxt); @@ -5267,13 +5211,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) else functypclass = get_expr_result_type((Node *) fexpr, NULL, &rettupdesc); - /* - * The single command must be a plain SELECT. - */ - if (!IsA(querytree, Query) || - querytree->commandType != CMD_SELECT) - goto fail; - /* * Make sure the function (still) returns what it's declared to. This * will raise an error if wrong, but that's okay since the function would @@ -5302,13 +5239,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) */ querytree = linitial_node(Query, querytree_list); - /* - * Looks good --- substitute parameters into the query. - */ - querytree = substitute_actual_srf_parameters(querytree, - funcform->pronargs, - fexpr->args); - /* * Copy the modified query out of the temporary memory context, and clean * up. @@ -5319,7 +5249,112 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) MemoryContextDelete(mycxt); error_context_stack = sqlerrcontext.previous; - ReleaseSysCache(func_tuple); + + return querytree; + + /* Here if func is not inlinable: release temp memory and return NULL */ +fail: + MemoryContextSwitchTo(oldcxt); + MemoryContextDelete(mycxt); + error_context_stack = sqlerrcontext.previous; + + return NULL; +} + +/* + * inline_set_returning_function + * Attempt to "inline" an SQL set-returning function in the FROM clause. + * + * "rte" is an RTE_FUNCTION rangetable entry. If it represents a call of a + * set-returning SQL function that can safely be inlined, expand the function + * and return the substitute Query structure. Otherwise, return NULL. + * + * We assume that the RTE's expression has already been put through + * eval_const_expressions(), which among other things will take care of + * default arguments and named-argument notation. + * + * This has a good deal of similarity to inline_function(), but that's + * for the non-set-returning case, and there are enough differences to + * justify separate functions. + */ +Query * +inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) +{ + RangeTblFunction *rtfunc; + FuncExpr *fexpr; + Oid func_oid; + HeapTuple func_tuple; + Form_pg_proc funcform; + Query *funcquery; + + Assert(rte->rtekind == RTE_FUNCTION); + + /* + * It doesn't make a lot of sense for a SRF to refer to itself in its own + * FROM clause, since that must cause infinite recursion at runtime. It + * will cause this code to recurse too, so check for stack overflow. + * (There's no need to do more.) + */ + check_stack_depth(); + + /* Fail if the RTE has ORDINALITY - we don't implement that here. */ + if (rte->funcordinality) + return NULL; + + /* Fail if RTE isn't a single, simple FuncExpr */ + if (list_length(rte->functions) != 1) + return NULL; + rtfunc = (RangeTblFunction *) linitial(rte->functions); + + if (!IsA(rtfunc->funcexpr, FuncExpr)) + return NULL; + fexpr = (FuncExpr *) rtfunc->funcexpr; + + func_oid = fexpr->funcid; + + /* + * The function must be declared to return a set, else inlining would + * change the results if the contained SELECT didn't return exactly one + * row. + */ + if (!fexpr->funcretset) + return NULL; + + /* Check permission to call function (fail later, if not) */ + if (object_aclcheck(ProcedureRelationId, func_oid, GetUserId(), ACL_EXECUTE) != ACLCHECK_OK) + return NULL; + + /* Check whether a plugin wants to hook function entry/exit */ + if (FmgrHookIsNeeded(func_oid)) + return NULL; + + /* + * OK, let's take a look at the function's pg_proc entry. + */ + func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(func_oid)); + if (!HeapTupleIsValid(func_tuple)) + elog(ERROR, "cache lookup failed for function %u", func_oid); + funcform = (Form_pg_proc) GETSTRUCT(func_tuple); + + funcquery = inline_sql_set_returning_function(root, rte, rtfunc, fexpr, + func_oid, func_tuple, funcform); + + if (!funcquery) + goto fail; + + /* + * The single command must be a plain SELECT. + */ + if (!IsA(funcquery, Query) || + funcquery->commandType != CMD_SELECT) + goto fail; + + /* + * Looks good --- substitute parameters into the query. + */ + funcquery = substitute_actual_srf_parameters(funcquery, + funcform->pronargs, + fexpr->args); /* * We don't have to fix collations here because the upper query is already @@ -5336,18 +5371,15 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) * We must also notice if the inserted query adds a dependency on the * calling role due to RLS quals. */ - if (querytree->hasRowSecurity) + if (funcquery->hasRowSecurity) root->glob->dependsOnRole = true; - return querytree; + ReleaseSysCache(func_tuple); + + return funcquery; - /* Here if func is not inlinable: release temp memory and return NULL */ fail: - MemoryContextSwitchTo(oldcxt); - MemoryContextDelete(mycxt); - error_context_stack = sqlerrcontext.previous; ReleaseSysCache(func_tuple); - return NULL; } -- 2.39.5 [text/x-patch] v2-0002-Add-SupportRequestInlineSRF.patch (27.2K, ../../[email protected]/3-v2-0002-Add-SupportRequestInlineSRF.patch) download | inline diff: From c48655640c56055ee5d10fd6feb804a88d7eee29 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <[email protected]> Date: Tue, 24 Jun 2025 21:30:02 -0700 Subject: [PATCH v2 2/2] Add SupportRequestInlineSRF If a set-returning function has an attached support function that can handle SupportRequestInlineSRF, then we replace the FuncExpr with a Query node built by the support function. Then the planner can rewrite the Query as if it were from a SQL-language function, merging it with the outer query. Author: Paul A. Jungwirth <[email protected]> --- doc/src/sgml/xfunc.sgml | 101 +++++++++ src/backend/optimizer/util/clauses.c | 65 +++++- src/include/nodes/supportnodes.h | 33 +++ src/test/regress/expected/misc_functions.out | 212 +++++++++++++++++++ src/test/regress/regress.c | 122 +++++++++++ src/test/regress/sql/misc_functions.sql | 86 ++++++++ 6 files changed, 614 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 2d81afce8cb..ef41ce49e65 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -4166,6 +4166,107 @@ supportfn(internal) returns internal expression and an actual execution of the target function. </para> + <para> + Similarly, a <link linkend="queries-tablefunctions">set-returning function</link> + can implement <literal>SupportRequestInlineSRF</literal> to return a + <literal>Query</literal> node, which the planner will try to inline into + the outer query, just as <productname>PostgreSQL</productname> inlines + SQL functions. Normallly only SQL functions can be inlined, but this support + request allows a function in <link linkend="plpgsql">PL/pgSQL</link> + or another language to build a dynamic SQL query and have it inlined too. + The <literal>Query</literal> node must be a <literal>SELECT</literal> query + that has gone through parse analysis and rewriting. + You may include <literal>Param</literal> nodes referencing the original function's + parameters, and <productname>PostgreSQL</productname> will map those appropriately + to the arguments passed by the caller. + It is the responsibility of the support function to return + a node that matches the parent function's implementation. + We make no guarantee that <productname>PostgreSQL</productname> will + never call the target function in cases that the support function could + simplify. Functions called in <literal>SELECT</literal> are not simplified. + Or if the <literal>RangeTblEntry</literal> has more than one + <literal>RangeTblFunction</literal> (such as when using + <literal>ROWS FROM</literal>), the function will not be simplified. + Ensure rigorous equivalence between the simplified expression and an actual + execution of the target function. + </para> + + <para> + One way to implement a <literal>SupportRequestInlineSRF</literal> support function + is to build a SQL string then parse it with <literal>pg_parse_query</literal>. + The outline of such a function might look like this: +<programlisting> +PG_FUNCTION_INFO_V1(my_support_function); +Datum +my_support_function(PG_FUNCTION_ARGS) +{ + Node *rawreq = (Node *) PG_GETARG_POINTER(0); + SupportRequestInlineSRF *req + RangeTblFunction *rtfunc; + FuncExpr *expr; + Query *querytree; + StringInfoData sql; + HeapTuple func_tuple; + SQLFunctionParseInfoPtr pinfo; + List *raw_parsetree_list; + + /* Return if it's not a type we handle. */ + if (!IsA(rawreq, SupportRequestInlineSRF)) + PG_RETURN_POINTER(NULL); + + /* Get things we need off the support request node. */ + req = (SupportRequestInlineSRF *) rawreq; + rtfunc = req->rtfunc; + expr = (FuncExpr *) rtfunc->funcexpr; + + /* Generate the SQL string. */ + initStringInfo(&sql); + appendStringInfo(&sql, "SELECT ..."); + + /* Build a SQLFunctionParseInfo. */ + func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(expr->funcid)); + if (!HeapTupleIsValid(func_tuple)) + { + ereport(WARNING, (errmsg("cache lookup failed for function %u", expr->funcid))); + PG_RETURN_POINTER(NULL); + } + pinfo = prepare_sql_fn_parse_info(func_tuple, + (Node *) expr, + expr->inputcollid); + ReleaseSysCache(func_tuple); + + /* Parse the SQL. */ + raw_parsetree_list = pg_parse_query(sql.data); + if (list_length(raw_parsetree_list) != 1) + { + ereport(WARNING, (errmsg("my_support_func parsed to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + /* Analyze the parse tree as if it were a SQL-language body. */ + querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), + sql.data, + (ParserSetupHook) sql_fn_parser_setup, + pinfo, NULL); + if (list_length(querytree_list) != 1) + { + ereport(WARNING, (errmsg("my_support_func rewrote to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + querytree = linitial(querytree_list); + if (!IsA(querytree, Query)) + { + ereport(WARNING, (errmsg("my_support_func didn't parse to a Query"), + errdetail("Got this instead: %s", nodeToString(querytree)))); + PG_RETURN_POINTER(NULL); + } + + PG_RETURN_POINTER(querytree); +} +</programlisting> + </para> + <para> For target functions that return <type>boolean</type>, it is often useful to estimate the fraction of rows that will be selected by a <literal>WHERE</literal> clause using that diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index a0dc5dfcd61..c9304c2b8fb 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5048,6 +5048,50 @@ evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod, } +/* + * inline_set_returning_function_with_support + * + * This implements inline_set_returning_function for functions with + * a support function that can handle SupportRequestInlineSRF. + * We let the support function make its own decisions about what it can + * handle. For instance we don't forbid a VOLATILE function or volatile + * arguments. + */ +static Query * +inline_set_returning_function_with_support(PlannerInfo *root, RangeTblEntry *rte, + RangeTblFunction *rtfunc, + FuncExpr *fexpr, Form_pg_proc funcform) +{ + SupportRequestInlineSRF req; + Node *newnode; + + /* It must have a support function. */ + Assert(funcform->prosupport); + + req.root = root; + req.type = T_SupportRequestInlineSRF; + req.rtfunc = rtfunc; + req.proc = funcform; + + /* + * XXX: wrap this in its own memory context, as + * inline_sql_set_returning_function does below? + */ + newnode = (Node *) + DatumGetPointer(OidFunctionCall1(funcform->prosupport, + PointerGetDatum(&req))); + + if (!newnode) + return NULL; + + if (!IsA(newnode, Query)) + elog(ERROR, + "Got unexpected node type %d from %s for function %s", + newnode->type, "SupportRequestInlineSRF", NameStr(funcform->proname)); + + return (Query *) newnode; +} + /* * inline_sql_set_returning_function * @@ -5263,10 +5307,10 @@ fail: /* * inline_set_returning_function - * Attempt to "inline" an SQL set-returning function in the FROM clause. + * Attempt to "inline" a set-returning function in the FROM clause. * * "rte" is an RTE_FUNCTION rangetable entry. If it represents a call of a - * set-returning SQL function that can safely be inlined, expand the function + * set-returning function that can safely be inlined, expand the function * and return the substitute Query structure. Otherwise, return NULL. * * We assume that the RTE's expression has already been put through @@ -5285,7 +5329,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) Oid func_oid; HeapTuple func_tuple; Form_pg_proc funcform; - Query *funcquery; + Query *funcquery = NULL; Assert(rte->rtekind == RTE_FUNCTION); @@ -5336,8 +5380,19 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) elog(ERROR, "cache lookup failed for function %u", func_oid); funcform = (Form_pg_proc) GETSTRUCT(func_tuple); - funcquery = inline_sql_set_returning_function(root, rte, rtfunc, fexpr, - func_oid, func_tuple, funcform); + /* + * If the function has an attached support function that can handle + * SupportRequestInlineSRF, then attempt to inline with that. Return the + * result if we get one, otherwise proceed. + */ + if (funcform->prosupport) + funcquery = inline_set_returning_function_with_support(root, rte, rtfunc, fexpr, + funcform); + + /* Try to inline automatically */ + if (!funcquery) + funcquery = inline_sql_set_returning_function(root, rte, rtfunc, fexpr, + func_oid, func_tuple, funcform); if (!funcquery) goto fail; diff --git a/src/include/nodes/supportnodes.h b/src/include/nodes/supportnodes.h index 9c047cc401b..2016e8def65 100644 --- a/src/include/nodes/supportnodes.h +++ b/src/include/nodes/supportnodes.h @@ -33,6 +33,7 @@ #ifndef SUPPORTNODES_H #define SUPPORTNODES_H +#include "catalog/pg_proc.h" #include "nodes/plannodes.h" struct PlannerInfo; /* avoid including pathnodes.h here */ @@ -69,6 +70,38 @@ typedef struct SupportRequestSimplify FuncExpr *fcall; /* Function call to be simplified */ } SupportRequestSimplify; +/* + * The InlineSRF request allows the support function to perform plan-time + * simplification of a call to its target set-returning function. For + * example a PL/pgSQL function could build a dynamic SQL query and execute it. + * Normally only SQL functions can be inlined, but with this support function + * the dynamic query can be inlined as well. + * + * The planner's PlannerInfo "root" is typically not needed, but can be + * consulted if it's necessary to obtain info about Vars present in + * the given node tree. Beware that root could be NULL in some usages. + * + * "rtfunc" will be a RangeTblFunction node for the function being replaced. + * The support function is only called if rtfunc->functions contains a + * single FuncExpr node. (ROWS FROM is one way to get more than one.) + * + * "proc" will be the Form_pg_proc record for the function being replaced. + * + * The result should be a semantically-equivalent transformed node tree, + * or NULL if no simplification could be performed. It should be allocated + * in the CurrentMemoryContext. Do *not* return or modify the FuncExpr node + * tree, as it isn't really a separately allocated Node. But it's okay to + * use its args, or parts of it, in the result tree. + */ +typedef struct SupportRequestInlineSRF +{ + NodeTag type; + + struct PlannerInfo *root; /* Planner's infrastructure */ + RangeTblFunction *rtfunc; /* Function call to be simplified */ + Form_pg_proc proc; /* Function definition */ +} SupportRequestInlineSRF; + /* * The Selectivity request allows the support function to provide a * selectivity estimate for a function appearing at top level of a WHERE diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index c3b2b9d8603..13388ebc715 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -601,6 +601,11 @@ CREATE FUNCTION test_support_func(internal) RETURNS internal AS :'regresslib', 'test_support_func' LANGUAGE C STRICT; +-- With a support function that inlines SRFs +CREATE FUNCTION test_inline_srf_support_func(internal) + RETURNS internal + AS :'regresslib', 'test_inline_srf_support_func' + LANGUAGE C STRICT; ALTER FUNCTION my_int_eq(int, int) SUPPORT test_support_func; EXPLAIN (COSTS OFF) SELECT * FROM tenk1 a JOIN tenk1 b ON a.unique1 = b.unique1 @@ -777,6 +782,213 @@ false, true, false, true); Function Scan on generate_series g (cost=N..N rows=1000 width=N) (1 row) +-- +-- Test inlining PL/pgSQL functions +-- +-- RETURNS SETOF TEXT: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF TEXT +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo_from_bar +-------------- + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo_from_bar +-------------- + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; +-- RETURNS SETOF RECORD: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF RECORD +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + QUERY PLAN +-------------------------------------------------------------------------- + Function Scan on foo_from_bar bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + QUERY PLAN +-------------------------------------------------------------------------- + Function Scan on foo_from_bar bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; +-- RETURNS TABLE: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS TABLE(foo TEXT) +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); ok diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index 3dbba069024..626ec3a1a71 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -28,6 +28,7 @@ #include "commands/sequence.h" #include "commands/trigger.h" #include "executor/executor.h" +#include "executor/functions.h" #include "executor/spi.h" #include "funcapi.h" #include "mb/pg_wchar.h" @@ -39,11 +40,13 @@ #include "port/atomics.h" #include "postmaster/postmaster.h" /* for MAX_BACKENDS */ #include "storage/spin.h" +#include "tcop/tcopprot.h" #include "utils/array.h" #include "utils/builtins.h" #include "utils/geo_decls.h" #include "utils/memutils.h" #include "utils/rel.h" +#include "utils/syscache.h" #include "utils/typcache.h" #define EXPECT_TRUE(expr) \ @@ -803,6 +806,125 @@ test_support_func(PG_FUNCTION_ARGS) PG_RETURN_POINTER(ret); } +PG_FUNCTION_INFO_V1(test_inline_srf_support_func); +Datum +test_inline_srf_support_func(PG_FUNCTION_ARGS) +{ + Node *rawreq = (Node *) PG_GETARG_POINTER(0); + Query *querytree = NULL; + + if (IsA(rawreq, SupportRequestInlineSRF)) + { + /* + * Assume that the target is foo_from_bar; that's safe as long as we + * don't attach this to any other set-returning function. + */ + SupportRequestInlineSRF *req = (SupportRequestInlineSRF *) rawreq; + StringInfoData sql; + RangeTblFunction *rtfunc = req->rtfunc; + FuncExpr *expr = (FuncExpr *) rtfunc->funcexpr; + Node *node; + Const *c; + char *colname; + char *tablename; + HeapTuple func_tuple; + SQLFunctionParseInfoPtr pinfo; + List *raw_parsetree_list; + List *querytree_list; + + if (list_length(expr->args) != 3) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with %d args but expected 3", list_length(expr->args)))); + PG_RETURN_POINTER(NULL); + } + + /* Get colname */ + node = linitial(expr->args); + if (!IsA(node, Const)) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-Const parameters"))); + PG_RETURN_POINTER(NULL); + } + + c = (Const *) node; + if (c->consttype != TEXTOID) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-TEXT parameters"))); + PG_RETURN_POINTER(NULL); + } + colname = TextDatumGetCString(c->constvalue); + + /* Get tablename */ + node = lsecond(expr->args); + if (!IsA(node, Const)) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-Const parameters"))); + PG_RETURN_POINTER(NULL); + } + + c = (Const *) node; + if (c->consttype != TEXTOID) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-TEXT parameters"))); + PG_RETURN_POINTER(NULL); + } + tablename = TextDatumGetCString(c->constvalue); + + initStringInfo(&sql); + appendStringInfo(&sql, "SELECT %s::text FROM %s", quote_identifier(colname), quote_identifier(tablename)); + + /* Get filter if present */ + node = lthird(expr->args); + if (!(IsA(node, Const) && ((Const *) node)->constisnull)) + { + appendStringInfo(&sql, " WHERE %s::text = $3", quote_identifier(colname)); + } + + /* Build a SQLFunctionParseInfo. */ + func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(expr->funcid)); + if (!HeapTupleIsValid(func_tuple)) + { + ereport(WARNING, (errmsg("cache lookup failed for function %u", expr->funcid))); + PG_RETURN_POINTER(NULL); + } + + pinfo = prepare_sql_fn_parse_info(func_tuple, + (Node *) expr, + expr->inputcollid); + + ReleaseSysCache(func_tuple); + + /* Parse the SQL. */ + raw_parsetree_list = pg_parse_query(sql.data); + if (list_length(raw_parsetree_list) != 1) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func parsed to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + /* Analyze the parse tree as if it were a SQL-language body. */ + querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), + sql.data, + (ParserSetupHook) sql_fn_parser_setup, + pinfo, NULL); + if (list_length(querytree_list) != 1) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func rewrote to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + querytree = linitial(querytree_list); + if (!IsA(querytree, Query)) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func didn't parse to a Query"), + errdetail("Got this instead: %s", nodeToString(querytree)))); + PG_RETURN_POINTER(NULL); + } + } + + PG_RETURN_POINTER(querytree); +} + PG_FUNCTION_INFO_V1(test_opclass_options_func); Datum test_opclass_options_func(PG_FUNCTION_ARGS) diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index 23792c4132a..56c6e7a8a88 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -248,6 +248,12 @@ CREATE FUNCTION test_support_func(internal) AS :'regresslib', 'test_support_func' LANGUAGE C STRICT; +-- With a support function that inlines SRFs +CREATE FUNCTION test_inline_srf_support_func(internal) + RETURNS internal + AS :'regresslib', 'test_inline_srf_support_func' + LANGUAGE C STRICT; + ALTER FUNCTION my_int_eq(int, int) SUPPORT test_support_func; EXPLAIN (COSTS OFF) @@ -349,6 +355,86 @@ SELECT explain_mask_costs($$ SELECT * FROM generate_series(25.0, 2.0, 0.0) g(s);$$, false, true, false, true); +-- +-- Test inlining PL/pgSQL functions +-- + +-- RETURNS SETOF TEXT: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF TEXT +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +DROP FUNCTION foo_from_bar; +-- RETURNS SETOF RECORD: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF RECORD +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +DROP FUNCTION foo_from_bar; +-- RETURNS TABLE: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS TABLE(foo TEXT) +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +DROP FUNCTION foo_from_bar; + -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); SELECT count(*) > 0 AS ok FROM pg_control_init(); -- 2.39.5 ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> @ 2025-07-14 21:21 ` Tom Lane <[email protected]> 2025-08-08 21:46 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul A Jungwirth <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Tom Lane @ 2025-07-14 21:21 UTC (permalink / raw) To: Paul Jungwirth <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; PostgreSQL Hackers <[email protected]> Paul Jungwirth <[email protected]> writes: > I tried a few refactoring approaches but the nicest seemed to be to keep the shared parts in > inline_set_returning_function, but have it call out to either inline_sql_set_returning_function or > inline_set_returning_function_with_support. The first patch just refactors but doesn't yet add > inline_set_returning_function_with_support, then the second patch adds the new functionality. I got around to looking at this again. I generally agree with your approach to the refactoring in clauses.c, with minor nitpicks: * I don't like postponing the early exit for its-not-a-SELECT; as coded, this wastes a pretty decent number of cycles transforming a querytree that won't be used (not to mention that I'm not sure that our usage of check_sql_fn_retval won't fail on a non-SELECT). So I think we should keep this bit where it is: - /* - * The single command must be a plain SELECT. - */ - if (!IsA(querytree, Query) || - querytree->commandType != CMD_SELECT) - goto fail; and then in the other path, simply Assert that those two conditions hold for anything the support function might try to give back. * I'm inclined to think that the test for "it must be declared to return a set" should stay in inline_sql_set_returning_function. In the case of a support-function-supported function, it's okay either if the function returns a set or if it is guaranteed to return exactly one row (including edge cases such as null function arguments). The support function either knows that already or can take the responsibility for checking it. But if we do it like this, we foreclose the possibility of supporting the latter class of functions. * But on the other hand, I wonder if this bit shouldn't move to the outer function: /* * Refuse to inline if the arguments contain any volatile functions or * sub-selects. Volatile functions are rejected because inlining may * result in the arguments being evaluated multiple times, risking a * change in behavior. Sub-selects are rejected partly for implementation * reasons (pushing them down another level might change their behavior) * and partly because they're likely to be expensive and so multiple * evaluation would be bad. */ if (contain_volatile_functions((Node *) fexpr->args) || contain_subplans((Node *) fexpr->args)) return NULL; I am not really convinced that any support function could safely ignore those restrictions, and I do fear that a lot would omit the enforcement and thereby produce wrong queries in such cases. Another thing that likely needs to be in the outer wrapper is the check that pg_proc_proconfig is empty, because that doesn't seem like a case that support functions could skip over either. * I don't like the memory management. I think creation/destruction of the temp context should occur at the outer level, and in particular that we want to perform substitute_actual_srf_parameters() while still working in the temp context, and copy out only the final form of the query tree. This addresses your XXX comment in v2-0002, and also saves support functions from having to re-invent that wheel. > I didn't love passing a SysCache HeapTuple into another function, No, that's perfectly common; see for example prepare_sql_fn_parse_info. In fact, one thing I don't like in v2-0002 is that you should pass the pg_proc entry to the support function as a HeapTuple not Form_pg_proc. It's possible to get the Form_pg_proc pointer from the HeapTuple but not vice versa, while the Form_pg_proc does not allow access to varlena fields, which makes it useless for many cases. Even your own example function is forced to re-fetch the syscache entry because of this. One other comment on v2-0002 is that this bit doesn't look right: + /* Get filter if present */ + node = lthird(expr->args); + if (!(IsA(node, Const) && ((Const *) node)->constisnull)) + { + appendStringInfo(&sql, " WHERE %s::text = $3", quote_identifier(colname)); + } It's not actually doing anything with the "node" value. Backing up to a higher level, it seems like we still have no answer for how to build a valid support function result besides "construct an equivalent SQL query string and feed it through parse analysis and rewrite". That seems both restrictive and expensive. In particular it begs the question of why the target function couldn't just have been written as a SQL function to begin with. So I still have kind of a low estimate of this feature's usefulness. Is there a way to do better? regards, tom lane ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> @ 2025-08-08 21:46 ` Paul A Jungwirth <[email protected]> 2025-11-23 00:44 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Paul A Jungwirth @ 2025-08-08 21:46 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Jul 14, 2025 at 2:21 PM Tom Lane <[email protected]> wrote: > I got around to looking at this again. I generally agree with your > approach to the refactoring in clauses.c, with minor nitpicks: Thanks for taking another look! Revisions attached. > * I don't like postponing the early exit for its-not-a-SELECT; > as coded, this wastes a pretty decent number of cycles transforming > a querytree that won't be used (not to mention that I'm not sure > that our usage of check_sql_fn_retval won't fail on a non-SELECT). > So I think we should keep this bit where it is: > > - /* > - * The single command must be a plain SELECT. > - */ > - if (!IsA(querytree, Query) || > - querytree->commandType != CMD_SELECT) > - goto fail; > > and then in the other path, simply Assert that those two conditions > hold for anything the support function might try to give back. Okay. > * I'm inclined to think that the test for "it must be declared to > return a set" should stay in inline_sql_set_returning_function. > In the case of a support-function-supported function, it's okay either > if the function returns a set or if it is guaranteed to return exactly > one row (including edge cases such as null function arguments). > The support function either knows that already or can take the > responsibility for checking it. But if we do it like this, we > foreclose the possibility of supporting the latter class of functions. Makes sense. Done. > * But on the other hand, I wonder if this bit shouldn't move to > the outer function: > > /* > * Refuse to inline if the arguments contain any volatile functions or > * sub-selects. Volatile functions are rejected because inlining may > * result in the arguments being evaluated multiple times, risking a > * change in behavior. Sub-selects are rejected partly for implementation > * reasons (pushing them down another level might change their behavior) > * and partly because they're likely to be expensive and so multiple > * evaluation would be bad. > */ > if (contain_volatile_functions((Node *) fexpr->args) || > contain_subplans((Node *) fexpr->args)) > return NULL; > > I am not really convinced that any support function could safely > ignore those restrictions, and I do fear that a lot would omit the > enforcement and thereby produce wrong queries in such cases. Another > thing that likely needs to be in the outer wrapper is the check that > pg_proc_proconfig is empty, because that doesn't seem like a case > that support functions could skip over either. Agreed, done. > * I don't like the memory management. I think creation/destruction > of the temp context should occur at the outer level, and in particular > that we want to perform substitute_actual_srf_parameters() while still > working in the temp context, and copy out only the final form of the > query tree. This addresses your XXX comment in v2-0002, and also > saves support functions from having to re-invent that wheel. Okay, done. I was trying to defer creating a memory context past as many checks as possible. It's not an expensive thing to do? > > I didn't love passing a SysCache HeapTuple into another function, > > No, that's perfectly common; see for example > prepare_sql_fn_parse_info. In fact, one thing I don't like in v2-0002 > is that you should pass the pg_proc entry to the support function as a > HeapTuple not Form_pg_proc. It's possible to get the Form_pg_proc > pointer from the HeapTuple but not vice versa, while the Form_pg_proc > does not allow access to varlena fields, which makes it useless for > many cases. Even your own example function is forced to re-fetch > the syscache entry because of this. Okay, thanks for explaining! Done. > One other comment on v2-0002 is that this bit doesn't look right: > > + /* Get filter if present */ > + node = lthird(expr->args); > + if (!(IsA(node, Const) && ((Const *) node)->constisnull)) > + { > + appendStringInfo(&sql, " WHERE %s::text = $3", quote_identifier(colname)); > + } > > It's not actually doing anything with the "node" value. This is correct, but I added a comment. The idea is that $3 will get the value of "node". > Backing up to a higher level, it seems like we still have no answer > for how to build a valid support function result besides "construct an > equivalent SQL query string and feed it through parse analysis and > rewrite". That seems both restrictive and expensive. In particular > it begs the question of why the target function couldn't just have > been written as a SQL function to begin with. So I still have kind > of a low estimate of this feature's usefulness. Is there a way to > do better? Parsing the function body is no more expensive than what we'd do to execute it separately, right? And by inlining we only do it once. But if it was too much for someone, perhaps they could keep a cache of node trees based on the function arguments and/or hash of the SQL string, not unlike how foreign keys cache query plans. Or they could build the node tree directly, without parsing. But parsing the equivalent string is easy and covers most uses. I can even imagine a way to eventually semi-automate it, e.g. creating a general-purpose support function that you can attach to (many) PL/pgSQL functions that end in `EXECUTE format(...)`. The reason for supporting more than SQL functions is to let you construct the query dynamically, e.g. with user-supplied table/column names, or to only include some expensive filters if needed. This would be great for building functions that implement temporal outer/semi/antijoin. Another use-case I personally have, which I think is quite common, is building "parameterized views" for permissions checks, e.g. visible_sales(user). In that case we may only need to include certain joins if the user belongs to certain roles (e.g. a third-party sales rep). Rebased to 04b7ff3cd3. Yours, -- Paul ~{:-) [email protected] Attachments: [application/octet-stream] v3-0001-Move-some-things-outside-of-inline_set_returning_.patch (12.9K, ../../CA+renyXxanvX8rnrqTNkZt1r9=rm59MHSG4DDVfsV2aUGe3dUQ@mail.gmail.com/2-v3-0001-Move-some-things-outside-of-inline_set_returning_.patch) download | inline diff: From c74e3fea09f44df39c62299777527e28616e0345 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <[email protected]> Date: Tue, 24 Jun 2025 19:10:15 -0700 Subject: [PATCH v3 1/2] Move some things outside of inline_set_returning_function. Added a new inline_sql_set_returning_function in preparation for inline_set_returning_function_with_support. Then inline_set_returning_function can call both, handling their shared needs itself. Author: Paul A. Jungwirth <[email protected]> --- src/backend/optimizer/util/clauses.c | 273 +++++++++++++++------------ 1 file changed, 157 insertions(+), 116 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 6f0b338d2cd..77f48ff4069 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5146,36 +5146,21 @@ evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod, /* - * inline_set_returning_function - * Attempt to "inline" a set-returning function in the FROM clause. - * - * "rte" is an RTE_FUNCTION rangetable entry. If it represents a call of a - * set-returning SQL function that can safely be inlined, expand the function - * and return the substitute Query structure. Otherwise, return NULL. + * inline_sql_set_returning_function * - * We assume that the RTE's expression has already been put through - * eval_const_expressions(), which among other things will take care of - * default arguments and named-argument notation. + * This implements inline_set_returning_function for sql-language functions. + * It parses the body (or uses the pre-parsed body if available). * - * This has a good deal of similarity to inline_function(), but that's - * for the non-set-returning case, and there are enough differences to - * justify separate functions. + * Returns NULL if the function couldn't be inlined. */ -Query * -inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) +static Query * +inline_sql_set_returning_function(PlannerInfo *root, RangeTblEntry *rte, + RangeTblFunction *rtfunc, + FuncExpr *fexpr, Oid func_oid, HeapTuple func_tuple, + Form_pg_proc funcform, char *src) { - RangeTblFunction *rtfunc; - FuncExpr *fexpr; - Oid func_oid; - HeapTuple func_tuple; - Form_pg_proc funcform; - char *src; - Datum tmp; + Datum sqlbody; bool isNull; - MemoryContext oldcxt; - MemoryContext mycxt; - inline_error_callback_arg callback_arg; - ErrorContextCallback sqlerrcontext; SQLFunctionParseInfoPtr pinfo; TypeFuncClass functypclass; TupleDesc rettupdesc; @@ -5185,29 +5170,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) Assert(rte->rtekind == RTE_FUNCTION); - /* - * It doesn't make a lot of sense for a SQL SRF to refer to itself in its - * own FROM clause, since that must cause infinite recursion at runtime. - * It will cause this code to recurse too, so check for stack overflow. - * (There's no need to do more.) - */ - check_stack_depth(); - - /* Fail if the RTE has ORDINALITY - we don't implement that here. */ - if (rte->funcordinality) - return NULL; - - /* Fail if RTE isn't a single, simple FuncExpr */ - if (list_length(rte->functions) != 1) - return NULL; - rtfunc = (RangeTblFunction *) linitial(rte->functions); - - if (!IsA(rtfunc->funcexpr, FuncExpr)) - return NULL; - fexpr = (FuncExpr *) rtfunc->funcexpr; - - func_oid = fexpr->funcid; - /* * The function must be declared to return a set, else inlining would * change the results if the contained SELECT didn't return exactly one @@ -5216,35 +5178,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) if (!fexpr->funcretset) return NULL; - /* - * Refuse to inline if the arguments contain any volatile functions or - * sub-selects. Volatile functions are rejected because inlining may - * result in the arguments being evaluated multiple times, risking a - * change in behavior. Sub-selects are rejected partly for implementation - * reasons (pushing them down another level might change their behavior) - * and partly because they're likely to be expensive and so multiple - * evaluation would be bad. - */ - if (contain_volatile_functions((Node *) fexpr->args) || - contain_subplans((Node *) fexpr->args)) - return NULL; - - /* Check permission to call function (fail later, if not) */ - if (object_aclcheck(ProcedureRelationId, func_oid, GetUserId(), ACL_EXECUTE) != ACLCHECK_OK) - return NULL; - - /* Check whether a plugin wants to hook function entry/exit */ - if (FmgrHookIsNeeded(func_oid)) - return NULL; - - /* - * OK, let's take a look at the function's pg_proc entry. - */ - func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(func_oid)); - if (!HeapTupleIsValid(func_tuple)) - elog(ERROR, "cache lookup failed for function %u", func_oid); - funcform = (Form_pg_proc) GETSTRUCT(func_tuple); - /* * Forget it if the function is not SQL-language or has other showstopper * properties. In particular it mustn't be declared STRICT, since we @@ -5262,61 +5195,34 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) funcform->prorettype == VOIDOID || funcform->prosecdef || !funcform->proretset || - list_length(fexpr->args) != funcform->pronargs || - !heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL)) + list_length(fexpr->args) != funcform->pronargs) { - ReleaseSysCache(func_tuple); return NULL; } - /* - * Make a temporary memory context, so that we don't leak all the stuff - * that parsing might create. - */ - mycxt = AllocSetContextCreate(CurrentMemoryContext, - "inline_set_returning_function", - ALLOCSET_DEFAULT_SIZES); - oldcxt = MemoryContextSwitchTo(mycxt); - - /* Fetch the function body */ - tmp = SysCacheGetAttrNotNull(PROCOID, func_tuple, Anum_pg_proc_prosrc); - src = TextDatumGetCString(tmp); - - /* - * Setup error traceback support for ereport(). This is so that we can - * finger the function that bad information came from. - */ - callback_arg.proname = NameStr(funcform->proname); - callback_arg.prosrc = src; - - sqlerrcontext.callback = sql_inline_error_callback; - sqlerrcontext.arg = &callback_arg; - sqlerrcontext.previous = error_context_stack; - error_context_stack = &sqlerrcontext; - /* If we have prosqlbody, pay attention to that not prosrc */ - tmp = SysCacheGetAttr(PROCOID, - func_tuple, - Anum_pg_proc_prosqlbody, - &isNull); + sqlbody = SysCacheGetAttr(PROCOID, + func_tuple, + Anum_pg_proc_prosqlbody, + &isNull); if (!isNull) { Node *n; - n = stringToNode(TextDatumGetCString(tmp)); + n = stringToNode(TextDatumGetCString(sqlbody)); if (IsA(n, List)) querytree_list = linitial_node(List, castNode(List, n)); else querytree_list = list_make1(n); if (list_length(querytree_list) != 1) - goto fail; + return NULL; querytree = linitial(querytree_list); /* Acquire necessary locks, then apply rewriter. */ AcquireRewriteLocks(querytree, true, false); querytree_list = pg_rewrite_query(querytree); if (list_length(querytree_list) != 1) - goto fail; + return NULL; querytree = linitial(querytree_list); } else @@ -5337,14 +5243,14 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) */ raw_parsetree_list = pg_parse_query(src); if (list_length(raw_parsetree_list) != 1) - goto fail; + return NULL; querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), src, (ParserSetupHook) sql_fn_parser_setup, pinfo, NULL); if (list_length(querytree_list) != 1) - goto fail; + return NULL; querytree = linitial(querytree_list); } @@ -5369,7 +5275,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) */ if (!IsA(querytree, Query) || querytree->commandType != CMD_SELECT) - goto fail; + return NULL; /* * Make sure the function (still) returns what it's declared to. This @@ -5391,7 +5297,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) (functypclass == TYPEFUNC_COMPOSITE || functypclass == TYPEFUNC_COMPOSITE_DOMAIN || functypclass == TYPEFUNC_RECORD)) - goto fail; /* reject not-whole-tuple-result cases */ + return NULL; /* reject not-whole-tuple-result cases */ /* * check_sql_fn_retval might've inserted a projection step, but that's @@ -5399,6 +5305,141 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) */ querytree = linitial_node(Query, querytree_list); + return querytree; +} + +/* + * inline_set_returning_function + * Attempt to "inline" an SQL set-returning function in the FROM clause. + * + * "rte" is an RTE_FUNCTION rangetable entry. If it represents a call of a + * set-returning SQL function that can safely be inlined, expand the function + * and return the substitute Query structure. Otherwise, return NULL. + * + * We assume that the RTE's expression has already been put through + * eval_const_expressions(), which among other things will take care of + * default arguments and named-argument notation. + * + * This has a good deal of similarity to inline_function(), but that's + * for the non-set-returning case, and there are enough differences to + * justify separate functions. + * + * It allocates its own temporary MemoryContext for the parsing, then copies + * the result into the caller's context. + */ +Query * +inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) +{ + RangeTblFunction *rtfunc; + FuncExpr *fexpr; + Oid func_oid; + HeapTuple func_tuple; + Form_pg_proc funcform; + Datum tmp; + char *src; + inline_error_callback_arg callback_arg; + ErrorContextCallback sqlerrcontext; + MemoryContext oldcxt; + MemoryContext mycxt; + Query *querytree; + + Assert(rte->rtekind == RTE_FUNCTION); + + /* + * It doesn't make a lot of sense for a SRF to refer to itself in its own + * FROM clause, since that must cause infinite recursion at runtime. It + * will cause this code to recurse too, so check for stack overflow. + * (There's no need to do more.) + */ + check_stack_depth(); + + /* Fail if the RTE has ORDINALITY - we don't implement that here. */ + if (rte->funcordinality) + return NULL; + + /* Fail if RTE isn't a single, simple FuncExpr */ + if (list_length(rte->functions) != 1) + return NULL; + rtfunc = (RangeTblFunction *) linitial(rte->functions); + + if (!IsA(rtfunc->funcexpr, FuncExpr)) + return NULL; + fexpr = (FuncExpr *) rtfunc->funcexpr; + + func_oid = fexpr->funcid; + + /* + * Refuse to inline if the arguments contain any volatile functions or + * sub-selects. Volatile functions are rejected because inlining may + * result in the arguments being evaluated multiple times, risking a + * change in behavior. Sub-selects are rejected partly for implementation + * reasons (pushing them down another level might change their behavior) + * and partly because they're likely to be expensive and so multiple + * evaluation would be bad. + */ + if (contain_volatile_functions((Node *) fexpr->args) || + contain_subplans((Node *) fexpr->args)) + return NULL; + + /* Check permission to call function (fail later, if not) */ + if (object_aclcheck(ProcedureRelationId, func_oid, GetUserId(), ACL_EXECUTE) != ACLCHECK_OK) + return NULL; + + /* Check whether a plugin wants to hook function entry/exit */ + if (FmgrHookIsNeeded(func_oid)) + return NULL; + + /* + * OK, let's take a look at the function's pg_proc entry. + */ + func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(func_oid)); + if (!HeapTupleIsValid(func_tuple)) + elog(ERROR, "cache lookup failed for function %u", func_oid); + funcform = (Form_pg_proc) GETSTRUCT(func_tuple); + + /* + * Make a temporary memory context, so that we don't leak all the stuff + * that parsing might create. + */ + mycxt = AllocSetContextCreate(CurrentMemoryContext, + "inline_set_returning_function", + ALLOCSET_DEFAULT_SIZES); + oldcxt = MemoryContextSwitchTo(mycxt); + + /* Fetch the function body */ + tmp = SysCacheGetAttrNotNull(PROCOID, func_tuple, Anum_pg_proc_prosrc); + src = TextDatumGetCString(tmp); + + /* + * Setup error traceback support for ereport(). This is so that we can + * finger the function that bad information came from. + */ + callback_arg.proname = NameStr(funcform->proname); + callback_arg.prosrc = src; + + sqlerrcontext.callback = sql_inline_error_callback; + sqlerrcontext.arg = &callback_arg; + sqlerrcontext.previous = error_context_stack; + error_context_stack = &sqlerrcontext; + + /* + * If the function SETs configuration parameters, inlining would cause us + * to skip those changes. + */ + if (!heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL)) + goto fail; + + querytree = inline_sql_set_returning_function(root, rte, rtfunc, fexpr, + func_oid, func_tuple, funcform, + src); + + if (!querytree) + goto fail; + + /* Only SELECTs are permitted */ + Assert(IsA(querytree, Query)); + Assert(querytree->commandType == CMD_SELECT); + /* * Looks good --- substitute parameters into the query. */ -- 2.45.0 [application/octet-stream] v3-0002-Add-SupportRequestInlineSRF.patch (27.5K, ../../CA+renyXxanvX8rnrqTNkZt1r9=rm59MHSG4DDVfsV2aUGe3dUQ@mail.gmail.com/3-v3-0002-Add-SupportRequestInlineSRF.patch) download | inline diff: From d6409f6d61019e54ae5c1a49ef57b7bded80de7b Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <[email protected]> Date: Tue, 24 Jun 2025 21:30:02 -0700 Subject: [PATCH v3 2/2] Add SupportRequestInlineSRF If a set-returning function has an attached support function that can handle SupportRequestInlineSRF, then we replace the FuncExpr with a Query node built by the support function. Then the planner can rewrite the Query as if it were from a SQL-language function, merging it with the outer query. Author: Paul A. Jungwirth <[email protected]> --- doc/src/sgml/xfunc.sgml | 101 +++++++++ src/backend/optimizer/util/clauses.c | 63 +++++- src/include/nodes/supportnodes.h | 34 +++ src/test/regress/expected/misc_functions.out | 212 +++++++++++++++++++ src/test/regress/regress.c | 114 ++++++++++ src/test/regress/sql/misc_functions.sql | 86 ++++++++ src/tools/pgindent/typedefs.list | 1 + 7 files changed, 605 insertions(+), 6 deletions(-) diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 30219f432d9..ecd7ced8be6 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -4166,6 +4166,107 @@ supportfn(internal) returns internal expression and an actual execution of the target function. </para> + <para> + Similarly, a <link linkend="queries-tablefunctions">set-returning function</link> + can implement <literal>SupportRequestInlineSRF</literal> to return a + <literal>Query</literal> node, which the planner will try to inline into + the outer query, just as <productname>PostgreSQL</productname> inlines + SQL functions. Normallly only SQL functions can be inlined, but this support + request allows a function in <link linkend="plpgsql">PL/pgSQL</link> + or another language to build a dynamic SQL query and have it inlined too. + The <literal>Query</literal> node must be a <literal>SELECT</literal> query + that has gone through parse analysis and rewriting. + You may include <literal>Param</literal> nodes referencing the original function's + parameters, and <productname>PostgreSQL</productname> will map those appropriately + to the arguments passed by the caller. + It is the responsibility of the support function to return + a node that matches the parent function's implementation. + We make no guarantee that <productname>PostgreSQL</productname> will + never call the target function in cases that the support function could + simplify. Functions called in <literal>SELECT</literal> are not simplified. + Or if the <literal>RangeTblEntry</literal> has more than one + <literal>RangeTblFunction</literal> (such as when using + <literal>ROWS FROM</literal>), the function will not be simplified. + Ensure rigorous equivalence between the simplified expression and an actual + execution of the target function. + </para> + + <para> + One way to implement a <literal>SupportRequestInlineSRF</literal> support function + is to build a SQL string then parse it with <literal>pg_parse_query</literal>. + The outline of such a function might look like this: +<programlisting> +PG_FUNCTION_INFO_V1(my_support_function); +Datum +my_support_function(PG_FUNCTION_ARGS) +{ + Node *rawreq = (Node *) PG_GETARG_POINTER(0); + SupportRequestInlineSRF *req + RangeTblFunction *rtfunc; + FuncExpr *expr; + Query *querytree; + StringInfoData sql; + HeapTuple func_tuple; + SQLFunctionParseInfoPtr pinfo; + List *raw_parsetree_list; + + /* Return if it's not a type we handle. */ + if (!IsA(rawreq, SupportRequestInlineSRF)) + PG_RETURN_POINTER(NULL); + + /* Get things we need off the support request node. */ + req = (SupportRequestInlineSRF *) rawreq; + rtfunc = req->rtfunc; + expr = (FuncExpr *) rtfunc->funcexpr; + + /* Generate the SQL string. */ + initStringInfo(&sql); + appendStringInfo(&sql, "SELECT ..."); + + /* Build a SQLFunctionParseInfo. */ + func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(expr->funcid)); + if (!HeapTupleIsValid(func_tuple)) + { + ereport(WARNING, (errmsg("cache lookup failed for function %u", expr->funcid))); + PG_RETURN_POINTER(NULL); + } + pinfo = prepare_sql_fn_parse_info(func_tuple, + (Node *) expr, + expr->inputcollid); + ReleaseSysCache(func_tuple); + + /* Parse the SQL. */ + raw_parsetree_list = pg_parse_query(sql.data); + if (list_length(raw_parsetree_list) != 1) + { + ereport(WARNING, (errmsg("my_support_func parsed to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + /* Analyze the parse tree as if it were a SQL-language body. */ + querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), + sql.data, + (ParserSetupHook) sql_fn_parser_setup, + pinfo, NULL); + if (list_length(querytree_list) != 1) + { + ereport(WARNING, (errmsg("my_support_func rewrote to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + querytree = linitial(querytree_list); + if (!IsA(querytree, Query)) + { + ereport(WARNING, (errmsg("my_support_func didn't parse to a Query"), + errdetail("Got this instead: %s", nodeToString(querytree)))); + PG_RETURN_POINTER(NULL); + } + + PG_RETURN_POINTER(querytree); +} +</programlisting> + </para> + <para> For target functions that return <type>boolean</type>, it is often useful to estimate the fraction of rows that will be selected by a <literal>WHERE</literal> clause using that diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 77f48ff4069..312237a5e13 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5145,6 +5145,47 @@ evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod, } +/* + * inline_set_returning_function_with_support + * + * This implements inline_set_returning_function for functions with + * a support function that can handle SupportRequestInlineSRF. + * We check fewer things than inline_sql_set_returning_function, + * so that support functions can make their own decisions about what + * to handle. For instance we don't forbid a VOLATILE function. + */ +static Query * +inline_set_returning_function_with_support(PlannerInfo *root, RangeTblEntry *rte, + RangeTblFunction *rtfunc, + FuncExpr *fexpr, HeapTuple func_tuple, + Form_pg_proc funcform) +{ + SupportRequestInlineSRF req; + Node *newnode; + + /* It must have a support function. */ + Assert(funcform->prosupport); + + req.root = root; + req.type = T_SupportRequestInlineSRF; + req.rtfunc = rtfunc; + req.proc = func_tuple; + + newnode = (Node *) + DatumGetPointer(OidFunctionCall1(funcform->prosupport, + PointerGetDatum(&req))); + + if (!newnode) + return NULL; + + if (!IsA(newnode, Query)) + elog(ERROR, + "Got unexpected node type %d from %s for function %s", + newnode->type, "SupportRequestInlineSRF", NameStr(funcform->proname)); + + return (Query *) newnode; +} + /* * inline_sql_set_returning_function * @@ -5310,10 +5351,10 @@ inline_sql_set_returning_function(PlannerInfo *root, RangeTblEntry *rte, /* * inline_set_returning_function - * Attempt to "inline" an SQL set-returning function in the FROM clause. + * Attempt to "inline" a set-returning function in the FROM clause. * * "rte" is an RTE_FUNCTION rangetable entry. If it represents a call of a - * set-returning SQL function that can safely be inlined, expand the function + * set-returning function that can safely be inlined, expand the function * and return the substitute Query structure. Otherwise, return NULL. * * We assume that the RTE's expression has already been put through @@ -5341,7 +5382,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) ErrorContextCallback sqlerrcontext; MemoryContext oldcxt; MemoryContext mycxt; - Query *querytree; + Query *querytree = NULL; Assert(rte->rtekind == RTE_FUNCTION); @@ -5429,9 +5470,19 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) if (!heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL)) goto fail; - querytree = inline_sql_set_returning_function(root, rte, rtfunc, fexpr, - func_oid, func_tuple, funcform, - src); + /* + * If the function has an attached support function that can handle + * SupportRequestInlineSRF, then attempt to inline with that. Return the + * result if we get one, otherwise proceed. + */ + if (funcform->prosupport) + querytree = inline_set_returning_function_with_support(root, rte, rtfunc, fexpr, + func_tuple, funcform); + + /* Try to inline automatically */ + if (!querytree) + querytree = inline_sql_set_returning_function(root, rte, rtfunc, fexpr, + func_oid, func_tuple, funcform, src); if (!querytree) goto fail; diff --git a/src/include/nodes/supportnodes.h b/src/include/nodes/supportnodes.h index 9c047cc401b..94f8a52de2b 100644 --- a/src/include/nodes/supportnodes.h +++ b/src/include/nodes/supportnodes.h @@ -33,6 +33,7 @@ #ifndef SUPPORTNODES_H #define SUPPORTNODES_H +#include "catalog/pg_proc.h" #include "nodes/plannodes.h" struct PlannerInfo; /* avoid including pathnodes.h here */ @@ -69,6 +70,39 @@ typedef struct SupportRequestSimplify FuncExpr *fcall; /* Function call to be simplified */ } SupportRequestSimplify; +/* + * The InlineSRF request allows the support function to perform plan-time + * simplification of a call to its target set-returning function. For + * example a PL/pgSQL function could build a dynamic SQL query and execute it. + * Normally only SQL functions can be inlined, but with this support function + * the dynamic query can be inlined as well. + * + * The planner's PlannerInfo "root" is typically not needed, but can be + * consulted if it's necessary to obtain info about Vars present in + * the given node tree. Beware that root could be NULL in some usages. + * + * "rtfunc" will be a RangeTblFunction node for the function being replaced. + * The support function is only called if rtfunc->functions contains a + * single FuncExpr node. (ROWS FROM is one way to get more than one.) + * + * "proc" will be the HeapTuple for the pg_proc record of the function being + * replaced. + * + * The result should be a semantically-equivalent transformed node tree, + * or NULL if no simplification could be performed. It should be allocated + * in the CurrentMemoryContext. Do *not* return or modify the FuncExpr node + * tree, as it isn't really a separately allocated Node. But it's okay to + * use its args, or parts of it, in the result tree. + */ +typedef struct SupportRequestInlineSRF +{ + NodeTag type; + + struct PlannerInfo *root; /* Planner's infrastructure */ + RangeTblFunction *rtfunc; /* Function call to be simplified */ + HeapTuple proc; /* Function definition from pg_proc */ +} SupportRequestInlineSRF; + /* * The Selectivity request allows the support function to provide a * selectivity estimate for a function appearing at top level of a WHERE diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index c3b2b9d8603..13388ebc715 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -601,6 +601,11 @@ CREATE FUNCTION test_support_func(internal) RETURNS internal AS :'regresslib', 'test_support_func' LANGUAGE C STRICT; +-- With a support function that inlines SRFs +CREATE FUNCTION test_inline_srf_support_func(internal) + RETURNS internal + AS :'regresslib', 'test_inline_srf_support_func' + LANGUAGE C STRICT; ALTER FUNCTION my_int_eq(int, int) SUPPORT test_support_func; EXPLAIN (COSTS OFF) SELECT * FROM tenk1 a JOIN tenk1 b ON a.unique1 = b.unique1 @@ -777,6 +782,213 @@ false, true, false, true); Function Scan on generate_series g (cost=N..N rows=1000 width=N) (1 row) +-- +-- Test inlining PL/pgSQL functions +-- +-- RETURNS SETOF TEXT: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF TEXT +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo_from_bar +-------------- + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo_from_bar +-------------- + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; +-- RETURNS SETOF RECORD: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF RECORD +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + QUERY PLAN +-------------------------------------------------------------------------- + Function Scan on foo_from_bar bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + QUERY PLAN +-------------------------------------------------------------------------- + Function Scan on foo_from_bar bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; +-- RETURNS TABLE: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS TABLE(foo TEXT) +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +---------------------------------------------------------------------- + Function Scan on foo_from_bar (cost=0.25..10.25 rows=1000 width=32) +(1 row) + +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + foo +------------------- + doh! + hi de ho neighbor +(2 rows) + +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + foo +------ + doh! +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=2 width=32) +(1 row) + +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); + QUERY PLAN +--------------------------------------------------------- + Seq Scan on text_tbl (cost=0.00..1.02 rows=1 width=32) + Filter: (f1 = 'doh!'::text) +(2 rows) + +DROP FUNCTION foo_from_bar; -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); ok diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index 3dbba069024..eadb40369d4 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -28,6 +28,7 @@ #include "commands/sequence.h" #include "commands/trigger.h" #include "executor/executor.h" +#include "executor/functions.h" #include "executor/spi.h" #include "funcapi.h" #include "mb/pg_wchar.h" @@ -39,11 +40,13 @@ #include "port/atomics.h" #include "postmaster/postmaster.h" /* for MAX_BACKENDS */ #include "storage/spin.h" +#include "tcop/tcopprot.h" #include "utils/array.h" #include "utils/builtins.h" #include "utils/geo_decls.h" #include "utils/memutils.h" #include "utils/rel.h" +#include "utils/syscache.h" #include "utils/typcache.h" #define EXPECT_TRUE(expr) \ @@ -803,6 +806,117 @@ test_support_func(PG_FUNCTION_ARGS) PG_RETURN_POINTER(ret); } +PG_FUNCTION_INFO_V1(test_inline_srf_support_func); +Datum +test_inline_srf_support_func(PG_FUNCTION_ARGS) +{ + Node *rawreq = (Node *) PG_GETARG_POINTER(0); + Query *querytree = NULL; + + if (IsA(rawreq, SupportRequestInlineSRF)) + { + /* + * Assume that the target is foo_from_bar; that's safe as long as we + * don't attach this to any other set-returning function. + */ + SupportRequestInlineSRF *req = (SupportRequestInlineSRF *) rawreq; + StringInfoData sql; + RangeTblFunction *rtfunc = req->rtfunc; + FuncExpr *expr = (FuncExpr *) rtfunc->funcexpr; + Node *node; + Const *c; + char *colname; + char *tablename; + SQLFunctionParseInfoPtr pinfo; + List *raw_parsetree_list; + List *querytree_list; + + if (list_length(expr->args) != 3) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with %d args but expected 3", list_length(expr->args)))); + PG_RETURN_POINTER(NULL); + } + + /* Get colname */ + node = linitial(expr->args); + if (!IsA(node, Const)) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-Const parameters"))); + PG_RETURN_POINTER(NULL); + } + + c = (Const *) node; + if (c->consttype != TEXTOID) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-TEXT parameters"))); + PG_RETURN_POINTER(NULL); + } + colname = TextDatumGetCString(c->constvalue); + + /* Get tablename */ + node = lsecond(expr->args); + if (!IsA(node, Const)) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-Const parameters"))); + PG_RETURN_POINTER(NULL); + } + + c = (Const *) node; + if (c->consttype != TEXTOID) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func called with non-TEXT parameters"))); + PG_RETURN_POINTER(NULL); + } + tablename = TextDatumGetCString(c->constvalue); + + initStringInfo(&sql); + appendStringInfo(&sql, "SELECT %s::text FROM %s", quote_identifier(colname), quote_identifier(tablename)); + + /* Get filter if present */ + node = lthird(expr->args); + if (!(IsA(node, Const) && ((Const *) node)->constisnull)) + { + /* Only filter if $3 is Const */ + appendStringInfo(&sql, " WHERE %s::text = $3", quote_identifier(colname)); + } + + /* Build a SQLFunctionParseInfo. */ + + pinfo = prepare_sql_fn_parse_info(req->proc, + (Node *) expr, + expr->inputcollid); + + /* Parse the SQL. */ + raw_parsetree_list = pg_parse_query(sql.data); + if (list_length(raw_parsetree_list) != 1) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func parsed to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + /* Analyze the parse tree as if it were a SQL-language body. */ + querytree_list = pg_analyze_and_rewrite_withcb(linitial(raw_parsetree_list), + sql.data, + (ParserSetupHook) sql_fn_parser_setup, + pinfo, NULL); + if (list_length(querytree_list) != 1) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func rewrote to more than one node"))); + PG_RETURN_POINTER(NULL); + } + + querytree = linitial(querytree_list); + if (!IsA(querytree, Query)) + { + ereport(WARNING, (errmsg("test_inline_srf_support_func didn't parse to a Query"), + errdetail("Got this instead: %s", nodeToString(querytree)))); + PG_RETURN_POINTER(NULL); + } + } + + PG_RETURN_POINTER(querytree); +} + PG_FUNCTION_INFO_V1(test_opclass_options_func); Datum test_opclass_options_func(PG_FUNCTION_ARGS) diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index 23792c4132a..56c6e7a8a88 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -248,6 +248,12 @@ CREATE FUNCTION test_support_func(internal) AS :'regresslib', 'test_support_func' LANGUAGE C STRICT; +-- With a support function that inlines SRFs +CREATE FUNCTION test_inline_srf_support_func(internal) + RETURNS internal + AS :'regresslib', 'test_inline_srf_support_func' + LANGUAGE C STRICT; + ALTER FUNCTION my_int_eq(int, int) SUPPORT test_support_func; EXPLAIN (COSTS OFF) @@ -349,6 +355,86 @@ SELECT explain_mask_costs($$ SELECT * FROM generate_series(25.0, 2.0, 0.0) g(s);$$, false, true, false, true); +-- +-- Test inlining PL/pgSQL functions +-- + +-- RETURNS SETOF TEXT: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF TEXT +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +DROP FUNCTION foo_from_bar; +-- RETURNS SETOF RECORD: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF RECORD +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL) AS bar(foo TEXT); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!') AS bar(foo TEXT); +DROP FUNCTION foo_from_bar; +-- RETURNS TABLE: +CREATE OR REPLACE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS TABLE(foo TEXT) +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE LEAKPROOF; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SUPPORT test_inline_srf_support_func; +SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); +EXPLAIN SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +DROP FUNCTION foo_from_bar; + -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); SELECT count(*) > 0 AS ok FROM pg_control_init(); diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index e6f2e93b2d6..8b681e9e44e 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2900,6 +2900,7 @@ SubscriptionRelState SummarizerReadLocalXLogPrivate SupportRequestCost SupportRequestIndexCondition +SupportRequestInlineSRF SupportRequestModifyInPlace SupportRequestOptimizeWindowClause SupportRequestRows -- 2.45.0 ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-08-08 21:46 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul A Jungwirth <[email protected]> @ 2025-11-23 00:44 ` Tom Lane <[email protected]> 2025-12-02 05:52 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Tom Lane @ 2025-11-23 00:44 UTC (permalink / raw) To: Paul A Jungwirth <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; PostgreSQL Hackers <[email protected]> Paul A Jungwirth <[email protected]> writes: > The reason for supporting more than SQL functions is to let you > construct the query dynamically, e.g. with user-supplied table/column > names, or to only include some expensive filters if needed. This would > be great for building functions that implement temporal > outer/semi/antijoin. Another use-case I personally have, which I think > is quite common, is building "parameterized views" for permissions > checks, e.g. visible_sales(user). In that case we may only need to > include certain joins if the user belongs to certain roles (e.g. a > third-party sales rep). I went through this again, and committed it with a bunch of mostly-cosmetic changes. In particular, it seemed like talking about inlining "set-returning functions" is no longer really on-point, since this mechanism is perfectly capable of inlining non-SRFs. (The reason we haven't done that for SQL functions is mainly that we didn't feel like doing the analysis necessary to prove that a SELECT will return exactly one row, which would be necessary to maintain semantic equivalence for a non-SRF after inlining. The easy cases of that, such as "SELECT expression", are already sufficiently handled by regular inlining.) So after some thought I renamed inline_set_returning_function to inline_function_in_from, and made a bunch of other changes in names and comments to line up with that. Thanks for working on this! I know it's been a long slog. regards, tom lane ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-08-08 21:46 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul A Jungwirth <[email protected]> 2025-11-23 00:44 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> @ 2025-12-02 05:52 ` Pavel Stehule <[email protected]> 2025-12-02 06:05 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-12-20 04:23 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?ISO-8859-1?B?emVuZ21hbg==?= <[email protected]> 0 siblings, 2 replies; 19+ messages in thread From: Pavel Stehule @ 2025-12-02 05:52 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Paul A Jungwirth <[email protected]>; Heikki Linnakangas <[email protected]>; PostgreSQL Hackers <[email protected]> Hi ne 23. 11. 2025 v 1:45 odesílatel Tom Lane <[email protected]> napsal: > Paul A Jungwirth <[email protected]> writes: > > The reason for supporting more than SQL functions is to let you > > construct the query dynamically, e.g. with user-supplied table/column > > names, or to only include some expensive filters if needed. This would > > be great for building functions that implement temporal > > outer/semi/antijoin. Another use-case I personally have, which I think > > is quite common, is building "parameterized views" for permissions > > checks, e.g. visible_sales(user). In that case we may only need to > > include certain joins if the user belongs to certain roles (e.g. a > > third-party sales rep). > > I went through this again, and committed it with a bunch of > mostly-cosmetic changes. In particular, it seemed like talking > about inlining "set-returning functions" is no longer really on-point, > since this mechanism is perfectly capable of inlining non-SRFs. > (The reason we haven't done that for SQL functions is mainly that > we didn't feel like doing the analysis necessary to prove that a > SELECT will return exactly one row, which would be necessary to > maintain semantic equivalence for a non-SRF after inlining. > The easy cases of that, such as "SELECT expression", are already > sufficiently handled by regular inlining.) > > So after some thought I renamed inline_set_returning_function to > inline_function_in_from, and made a bunch of other changes in names > and comments to line up with that. > > Thanks for working on this! I know it's been a long slog. > I checked this patch, and I think so using the body of the function foo_from_bar is very confusing. If I understand this patch and functionality, then the function is never executed (when the support function has support for SupportRequestInlineInFrom). +CREATE FUNCTION test_inline_in_from_support_func(internal) + RETURNS internal + AS :'regresslib', 'test_inline_in_from_support_func' + LANGUAGE C STRICT; +CREATE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +RETURNS SETOF TEXT +LANGUAGE plpgsql +AS $function$ +DECLARE + sql TEXT; +BEGIN + sql := format('SELECT %I::text FROM %I', colname, tablename); + IF filter IS NOT NULL THEN + sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); + END IF; + RETURN QUERY EXECUTE sql USING filter; +END; +$function$ STABLE; More correct form should be BEGIN RAISE EXCEPTION 'halt - should not be executed directly'; END; Regards Pavel > > regards, tom lane > > > ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-08-08 21:46 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul A Jungwirth <[email protected]> 2025-11-23 00:44 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-12-02 05:52 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> @ 2025-12-02 06:05 ` Tom Lane <[email protected]> 2025-12-02 06:14 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 1 sibling, 1 reply; 19+ messages in thread From: Tom Lane @ 2025-12-02 06:05 UTC (permalink / raw) To: Pavel Stehule <[email protected]>; +Cc: Paul A Jungwirth <[email protected]>; Heikki Linnakangas <[email protected]>; PostgreSQL Hackers <[email protected]> Pavel Stehule <[email protected]> writes: > ne 23. 11. 2025 v 1:45 odesílatel Tom Lane <[email protected]> napsal: >> So after some thought I renamed inline_set_returning_function to >> inline_function_in_from, and made a bunch of other changes in names >> and comments to line up with that. > I checked this patch, and I think so using the body of the > function foo_from_bar is very confusing. Perhaps that example could use more documentation effort ... > More correct form should be > RAISE EXCEPTION 'halt - should not be executed directly'; ... but I don't agree with this at all. We specifically do not guarantee that replacement via SupportRequestSimplify will be honored, and I think the same must be true for SupportRequestInlineInFrom. Otherwise we risk breaking who-knows-how-many third-party tools, as well as cases where the optimizer declines to inline (eg, because there are volatile arguments). Besides, what's the value? regards, tom lane ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-08-08 21:46 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul A Jungwirth <[email protected]> 2025-11-23 00:44 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-12-02 05:52 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-02 06:05 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> @ 2025-12-02 06:14 ` Pavel Stehule <[email protected]> 0 siblings, 0 replies; 19+ messages in thread From: Pavel Stehule @ 2025-12-02 06:14 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Paul A Jungwirth <[email protected]>; Heikki Linnakangas <[email protected]>; PostgreSQL Hackers <[email protected]> út 2. 12. 2025 v 7:05 odesílatel Tom Lane <[email protected]> napsal: > Pavel Stehule <[email protected]> writes: > > ne 23. 11. 2025 v 1:45 odesílatel Tom Lane <[email protected]> napsal: > >> So after some thought I renamed inline_set_returning_function to > >> inline_function_in_from, and made a bunch of other changes in names > >> and comments to line up with that. > > > I checked this patch, and I think so using the body of the > > function foo_from_bar is very confusing. > > Perhaps that example could use more documentation effort ... > > > More correct form should be > > RAISE EXCEPTION 'halt - should not be executed directly'; > > ... but I don't agree with this at all. We specifically do > not guarantee that replacement via SupportRequestSimplify > will be honored, and I think the same must be true for > SupportRequestInlineInFrom. Otherwise we risk breaking > who-knows-how-many third-party tools, as well as cases > where the optimizer declines to inline (eg, because there > are volatile arguments). Besides, what's the value? > My note is just for this test. I understand that the planner function can handle only some specific variants - and all others will be done by real execution. But this is really confusing - I read the blog https://www.thatguyfromdelhi.com/2025/11/teaching-query-planner-to-see-inside-c.html and I was very interested in what is magic in forward, and the reality is very simple. I think this feature can be very practical - but can be very messy for people who don't understand well to complete the picture. And it is a second level of overloading (what is byself dangerous for someone). Regards Pavel > > regards, tom lane > ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-08-08 21:46 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul A Jungwirth <[email protected]> 2025-11-23 00:44 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-12-02 05:52 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> @ 2025-12-20 04:23 ` =?ISO-8859-1?B?emVuZ21hbg==?= <[email protected]> 2025-12-20 06:57 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-20 21:37 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 1 sibling, 2 replies; 19+ messages in thread From: =?ISO-8859-1?B?emVuZ21hbg==?= @ 2025-12-20 04:23 UTC (permalink / raw) To: =?ISO-8859-1?B?UGF2ZWwgU3RlaHVsZQ==?= <[email protected]>; =?ISO-8859-1?B?VG9tIExhbmU=?= <[email protected]>; +Cc: =?ISO-8859-1?B?cGdzcWwtaGFja2Vycw==?= <[email protected]> Hi everyone, I've noticed this interesting feature and would like to modify this test case to clarify that we fall back to the original logic when inlining fails. This is a small change that doesn't touch core code. What do you all think? Regards Man Zeng Attachments: [application/octet-stream] 0001-misc_functions-Verify-Function-Scan-fallback-on-inli.patch (3.7K, ../../[email protected]/2-0001-misc_functions-Verify-Function-Scan-fallback-on-inli.patch) download | inline diff: From 3b55e316c45a1f309864b384d147bd85c19a9579 Mon Sep 17 00:00:00 2001 From: Man Zeng <[email protected]> Date: Sat, 20 Dec 2025 12:09:07 +0800 Subject: [PATCH] misc_functions: Verify Function Scan fallback on inline failure Execution plan falls back to Function Scan (run original PL/pgSQL logic) when inlining fails Signed-off-by: Man Zeng <[email protected]> --- src/test/regress/expected/misc_functions.out | 21 +++++++++++++++++++- src/test/regress/sql/misc_functions.sql | 8 +++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index d7d965d884a..212de3e2783 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -815,13 +815,17 @@ CREATE FUNCTION test_inline_in_from_support_func(internal) RETURNS internal AS :'regresslib', 'test_inline_in_from_support_func' LANGUAGE C STRICT; -CREATE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +CREATE FUNCTION foo_from_bar(colname TEXT DEFAULT NULL, tablename TEXT DEFAULT NULL, filter TEXT DEFAULT NULL) RETURNS SETOF TEXT LANGUAGE plpgsql AS $function$ DECLARE sql TEXT; BEGIN + IF tablename IS NULL AND colname IS NULL THEN + tablename = 'text_tbl'; + colname = 'f1'; + END IF; sql := format('SELECT %I::text FROM %I', colname, tablename); IF filter IS NOT NULL THEN sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); @@ -844,6 +848,14 @@ SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); doh! (1 row) +SELECT * FROM foo_from_bar(); +WARNING: test_inline_in_from_support_func called with non-TEXT parameters + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); QUERY PLAN ---------------------- @@ -857,6 +869,13 @@ EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); Filter: (f1 = 'doh!'::text) (2 rows) +EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar(); +WARNING: test_inline_in_from_support_func called with non-TEXT parameters + QUERY PLAN +------------------------------- + Function Scan on foo_from_bar +(1 row) + DROP FUNCTION foo_from_bar; -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index 0fc20fbb6b4..a4078ee3dfc 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -369,13 +369,17 @@ CREATE FUNCTION test_inline_in_from_support_func(internal) AS :'regresslib', 'test_inline_in_from_support_func' LANGUAGE C STRICT; -CREATE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +CREATE FUNCTION foo_from_bar(colname TEXT DEFAULT NULL, tablename TEXT DEFAULT NULL, filter TEXT DEFAULT NULL) RETURNS SETOF TEXT LANGUAGE plpgsql AS $function$ DECLARE sql TEXT; BEGIN + IF tablename IS NULL AND colname IS NULL THEN + tablename = 'text_tbl'; + colname = 'f1'; + END IF; sql := format('SELECT %I::text FROM %I', colname, tablename); IF filter IS NOT NULL THEN sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); @@ -389,8 +393,10 @@ ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +SELECT * FROM foo_from_bar(); EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar(); DROP FUNCTION foo_from_bar; -- 2.45.2 ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-08-08 21:46 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul A Jungwirth <[email protected]> 2025-11-23 00:44 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-12-02 05:52 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-20 04:23 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?ISO-8859-1?B?emVuZ21hbg==?= <[email protected]> @ 2025-12-20 06:57 ` Pavel Stehule <[email protected]> 2025-12-20 10:53 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?gb18030?B?emVuZ21hbg==?= <[email protected]> 1 sibling, 1 reply; 19+ messages in thread From: Pavel Stehule @ 2025-12-20 06:57 UTC (permalink / raw) To: zengman <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers <[email protected]> so 20. 12. 2025 v 5:23 odesílatel zengman <[email protected]> napsal: > Hi everyone, > > I've noticed this interesting feature and would like to modify this test > case to clarify that we fall back to the original logic when inlining > fails. > This is a small change that doesn't touch core code. What do you all think? > +1 I think so message of warning should be enhanced - "test_inline_in_from_support_func called with non-TEXT parameters" is messy little bit. Regards Pavel > > Regards > Man Zeng ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-08-08 21:46 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul A Jungwirth <[email protected]> 2025-11-23 00:44 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-12-02 05:52 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-20 04:23 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?ISO-8859-1?B?emVuZ21hbg==?= <[email protected]> 2025-12-20 06:57 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> @ 2025-12-20 10:53 ` =?gb18030?B?emVuZ21hbg==?= <[email protected]> 2025-12-20 11:36 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: =?gb18030?B?emVuZ21hbg==?= @ 2025-12-20 10:53 UTC (permalink / raw) To: =?gb18030?B?UGF2ZWwgU3RlaHVsZQ==?= <[email protected]>; +Cc: =?gb18030?B?cGdzcWwtaGFja2Vycw==?= <[email protected]> I share this opinion. In fact, in the existing tests, if test_inline_in_from_support_func is triggered, the input parameters are necessarily of type TEXT—so there’s no need for an additional type check. How about changing it to the following? ``` diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index b7a926c6f01..adb8b9793ba 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -849,9 +849,9 @@ test_inline_in_from_support_func(PG_FUNCTION_ARGS) } c = (Const *) node; - if (c->consttype != TEXTOID || c->constisnull) + if (c->constisnull) { - ereport(WARNING, (errmsg("test_inline_in_from_support_func called with non-TEXT parameters"))); + ereport(WARNING, (errmsg("test_inline_in_from_support_func called with null parameter for colname"))); PG_RETURN_POINTER(NULL); } colname = TextDatumGetCString(c->constvalue); @@ -865,9 +865,9 @@ test_inline_in_from_support_func(PG_FUNCTION_ARGS) } c = (Const *) node; - if (c->consttype != TEXTOID || c->constisnull) + if (c->constisnull) { - ereport(WARNING, (errmsg("test_inline_in_from_support_func called with non-TEXT parameters"))); + ereport(WARNING, (errmsg("test_inline_in_from_support_func called with null parameter for tablename"))); PG_RETURN_POINTER(NULL); } tablename = TextDatumGetCString(c->constvalue); ``` Regards Man Zeng ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-08-08 21:46 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul A Jungwirth <[email protected]> 2025-11-23 00:44 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-12-02 05:52 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-20 04:23 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?ISO-8859-1?B?emVuZ21hbg==?= <[email protected]> 2025-12-20 06:57 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-20 10:53 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?gb18030?B?emVuZ21hbg==?= <[email protected]> @ 2025-12-20 11:36 ` Pavel Stehule <[email protected]> 2025-12-20 12:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?ISO-8859-1?B?emVuZ21hbg==?= <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Pavel Stehule @ 2025-12-20 11:36 UTC (permalink / raw) To: zengman <[email protected]>; +Cc: pgsql-hackers <[email protected]> so 20. 12. 2025 v 11:53 odesílatel zengman <[email protected]> napsal: > I share this opinion. In fact, in the existing tests, if > test_inline_in_from_support_func is triggered, > the input parameters are necessarily of type TEXT—so there’s no need for > an additional type check. > > How about changing it to the following? > ``` > diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c > index b7a926c6f01..adb8b9793ba 100644 > --- a/src/test/regress/regress.c > +++ b/src/test/regress/regress.c > @@ -849,9 +849,9 @@ test_inline_in_from_support_func(PG_FUNCTION_ARGS) > } > > c = (Const *) node; > - if (c->consttype != TEXTOID || c->constisnull) > + if (c->constisnull) > { > - ereport(WARNING, > (errmsg("test_inline_in_from_support_func called with non-TEXT > parameters"))); > + ereport(WARNING, > (errmsg("test_inline_in_from_support_func called with null parameter for > colname"))); > PG_RETURN_POINTER(NULL); > } > colname = TextDatumGetCString(c->constvalue); > @@ -865,9 +865,9 @@ test_inline_in_from_support_func(PG_FUNCTION_ARGS) > } > > c = (Const *) node; > - if (c->consttype != TEXTOID || c->constisnull) > + if (c->constisnull) > { > - ereport(WARNING, > (errmsg("test_inline_in_from_support_func called with non-TEXT > parameters"))); > + ereport(WARNING, > (errmsg("test_inline_in_from_support_func called with null parameter for > tablename"))); > PG_RETURN_POINTER(NULL); > } > tablename = TextDatumGetCString(c->constvalue); > +1 Pavel > ``` > > Regards > Man Zeng ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-08-08 21:46 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul A Jungwirth <[email protected]> 2025-11-23 00:44 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-12-02 05:52 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-20 04:23 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?ISO-8859-1?B?emVuZ21hbg==?= <[email protected]> 2025-12-20 06:57 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-20 10:53 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?gb18030?B?emVuZ21hbg==?= <[email protected]> 2025-12-20 11:36 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> @ 2025-12-20 12:21 ` =?ISO-8859-1?B?emVuZ21hbg==?= <[email protected]> 0 siblings, 0 replies; 19+ messages in thread From: =?ISO-8859-1?B?emVuZ21hbg==?= @ 2025-12-20 12:21 UTC (permalink / raw) To: =?ISO-8859-1?B?UGF2ZWwgU3RlaHVsZQ==?= <[email protected]>; +Cc: =?ISO-8859-1?B?cGdzcWwtaGFja2Vycw==?= <[email protected]>; =?ISO-8859-1?B?VG9tIExhbmU=?= <[email protected]> Thank you for your guidance. Attached is the updated patch. I wonder if there are any other adjustments needed and look forward to more feedback. -- Regards Man Zeng www.openhalo.org Attachments: [application/octet-stream] 0002-misc_functions-Verify-Function-Scan-fallback-on-inli.patch (4.9K, ../../[email protected]/2-0002-misc_functions-Verify-Function-Scan-fallback-on-inli.patch) download | inline diff: From b7f1ded3512993cb2b71270dd0124af51d2e1936 Mon Sep 17 00:00:00 2001 From: Man Zeng <[email protected]> Date: Sat, 20 Dec 2025 12:09:07 +0800 Subject: [PATCH] misc_functions: Verify inline fallback & fix TEXT check/warnings Verify inline failure falls back to Function Scan (original PL/pgSQL logic) Clean up redundant TEXT check and adjust warnings Signed-off-by: Man Zeng <[email protected]> --- src/test/regress/expected/misc_functions.out | 21 +++++++++++++++++++- src/test/regress/regress.c | 8 ++++---- src/test/regress/sql/misc_functions.sql | 8 +++++++- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index d7d965d884a..44e4450cacd 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -815,13 +815,17 @@ CREATE FUNCTION test_inline_in_from_support_func(internal) RETURNS internal AS :'regresslib', 'test_inline_in_from_support_func' LANGUAGE C STRICT; -CREATE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +CREATE FUNCTION foo_from_bar(colname TEXT DEFAULT NULL, tablename TEXT DEFAULT NULL, filter TEXT DEFAULT NULL) RETURNS SETOF TEXT LANGUAGE plpgsql AS $function$ DECLARE sql TEXT; BEGIN + IF tablename IS NULL AND colname IS NULL THEN + tablename = 'text_tbl'; + colname = 'f1'; + END IF; sql := format('SELECT %I::text FROM %I', colname, tablename); IF filter IS NOT NULL THEN sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); @@ -844,6 +848,14 @@ SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); doh! (1 row) +SELECT * FROM foo_from_bar(); +WARNING: test_inline_in_from_support_func called with null parameter for colname + foo_from_bar +------------------- + doh! + hi de ho neighbor +(2 rows) + EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); QUERY PLAN ---------------------- @@ -857,6 +869,13 @@ EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); Filter: (f1 = 'doh!'::text) (2 rows) +EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar(); +WARNING: test_inline_in_from_support_func called with null parameter for colname + QUERY PLAN +------------------------------- + Function Scan on foo_from_bar +(1 row) + DROP FUNCTION foo_from_bar; -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index b7a926c6f01..adb8b9793ba 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -849,9 +849,9 @@ test_inline_in_from_support_func(PG_FUNCTION_ARGS) } c = (Const *) node; - if (c->consttype != TEXTOID || c->constisnull) + if (c->constisnull) { - ereport(WARNING, (errmsg("test_inline_in_from_support_func called with non-TEXT parameters"))); + ereport(WARNING, (errmsg("test_inline_in_from_support_func called with null parameter for colname"))); PG_RETURN_POINTER(NULL); } colname = TextDatumGetCString(c->constvalue); @@ -865,9 +865,9 @@ test_inline_in_from_support_func(PG_FUNCTION_ARGS) } c = (Const *) node; - if (c->consttype != TEXTOID || c->constisnull) + if (c->constisnull) { - ereport(WARNING, (errmsg("test_inline_in_from_support_func called with non-TEXT parameters"))); + ereport(WARNING, (errmsg("test_inline_in_from_support_func called with null parameter for tablename"))); PG_RETURN_POINTER(NULL); } tablename = TextDatumGetCString(c->constvalue); diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index 0fc20fbb6b4..a4078ee3dfc 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -369,13 +369,17 @@ CREATE FUNCTION test_inline_in_from_support_func(internal) AS :'regresslib', 'test_inline_in_from_support_func' LANGUAGE C STRICT; -CREATE FUNCTION foo_from_bar(colname TEXT, tablename TEXT, filter TEXT) +CREATE FUNCTION foo_from_bar(colname TEXT DEFAULT NULL, tablename TEXT DEFAULT NULL, filter TEXT DEFAULT NULL) RETURNS SETOF TEXT LANGUAGE plpgsql AS $function$ DECLARE sql TEXT; BEGIN + IF tablename IS NULL AND colname IS NULL THEN + tablename = 'text_tbl'; + colname = 'f1'; + END IF; sql := format('SELECT %I::text FROM %I', colname, tablename); IF filter IS NOT NULL THEN sql := CONCAT(sql, format(' WHERE %I::text = $1', colname)); @@ -389,8 +393,10 @@ ALTER FUNCTION foo_from_bar(TEXT, TEXT, TEXT) SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +SELECT * FROM foo_from_bar(); EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', NULL); EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); +EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar(); DROP FUNCTION foo_from_bar; -- 2.45.2 ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-08-08 21:46 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul A Jungwirth <[email protected]> 2025-11-23 00:44 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-12-02 05:52 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-20 04:23 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?ISO-8859-1?B?emVuZ21hbg==?= <[email protected]> @ 2025-12-20 21:37 ` Tom Lane <[email protected]> 2025-12-21 02:48 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?gb18030?B?emVuZ21hbg==?= <[email protected]> 1 sibling, 1 reply; 19+ messages in thread From: Tom Lane @ 2025-12-20 21:37 UTC (permalink / raw) To: =?ISO-8859-1?B?emVuZ21hbg==?= <[email protected]>; +Cc: =?ISO-8859-1?B?UGF2ZWwgU3RlaHVsZQ==?= <[email protected]>; =?ISO-8859-1?B?cGdzcWwtaGFja2Vycw==?= <[email protected]> "=?ISO-8859-1?B?emVuZ21hbg==?=" <[email protected]> writes: > I've noticed this interesting feature and would like to modify this test case to clarify that we fall back to the original logic when inlining fails. > This is a small change that doesn't touch core code. What do you all think? I think this looks like a waste of test cycles. AFAICS it won't add even a single line of code coverage for inline_function_in_from(). Why do you think we need it? regards, tom lane ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Inline non-SQL SRFs using SupportRequestSimplify 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-08-08 21:46 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul A Jungwirth <[email protected]> 2025-11-23 00:44 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-12-02 05:52 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-20 04:23 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?ISO-8859-1?B?emVuZ21hbg==?= <[email protected]> 2025-12-20 21:37 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> @ 2025-12-21 02:48 ` =?gb18030?B?emVuZ21hbg==?= <[email protected]> 0 siblings, 0 replies; 19+ messages in thread From: =?gb18030?B?emVuZ21hbg==?= @ 2025-12-21 02:48 UTC (permalink / raw) To: =?gb18030?B?VG9tIExhbmU=?= <[email protected]>; +Cc: =?gb18030?B?UGF2ZWwgU3RlaHVsZQ==?= <[email protected]>; =?gb18030?B?cGdzcWwtaGFja2Vycw==?= <[email protected]> Well, from my perspective, the test logic for this feature seems a bit odd: even if you directly modify the function body to return NULL; or return 1/0;, none of this execution logic takes effect at all—it serves no purpose whatsoever as long as inlining succeeds. This could be unfriendly to developers who are new to this feature, so I added a test for this scenario to verify that our logic falls back to the function body when inlining fails. This may just be my personal opinion, though; if you think everything is fine, we can leave it as it is. -- Regards, Man Zeng www.openhalo.org ^ permalink raw reply [nested|flat] 19+ messages in thread
end of thread, other threads:[~2025-12-21 02:48 UTC | newest] Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-05-28 18:16 [PATCH v29 4/6] Track collation versions for indexes. Thomas Munro <[email protected]> 2024-07-02 12:59 Re: Inline non-SQL SRFs using SupportRequestSimplify Heikki Linnakangas <[email protected]> 2024-07-26 18:58 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2024-08-30 16:26 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2024-09-03 16:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-06-30 21:42 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul Jungwirth <[email protected]> 2025-07-14 21:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-08-08 21:46 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Paul A Jungwirth <[email protected]> 2025-11-23 00:44 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-12-02 05:52 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-02 06:05 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-12-02 06:14 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-20 04:23 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?ISO-8859-1?B?emVuZ21hbg==?= <[email protected]> 2025-12-20 06:57 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-20 10:53 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?gb18030?B?emVuZ21hbg==?= <[email protected]> 2025-12-20 11:36 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Pavel Stehule <[email protected]> 2025-12-20 12:21 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?ISO-8859-1?B?emVuZ21hbg==?= <[email protected]> 2025-12-20 21:37 ` Re: Inline non-SQL SRFs using SupportRequestSimplify Tom Lane <[email protected]> 2025-12-21 02:48 ` Re: Inline non-SQL SRFs using SupportRequestSimplify =?gb18030?B?emVuZ21hbg==?= <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox