Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit f24b156

Browse filesBrowse files
tglsfdctglsfdc
committed
Rethink extraction of collation dependencies.
As it stands, find_expr_references_walker() pays attention to leaf-node collation fields while ignoring the input collations of actual function and operator nodes. That seems exactly backwards from a semantic standpoint, and it leads to reporting dependencies on collations that really have nothing to do with the expression's behavior. Hence, rewrite to look at function input collations instead. This isn't completely perfect either; it fails to account for the behavior of record_eq and its siblings. (The previous coding at least gave an approximation of that, though I think it could be fooled pretty easily into considering the columns of irrelevant composite types.) We may be able to improve on this later, but for now this should satisfy the buildfarm members that didn't like ef387be. In passing fix some oversights in GetTypeCollations(), and get rid of its duplicative de-duplications. (I'm worried that it's still potentially O(N^2) or worse, but this makes it a little better.) Discussion: https://postgr.es/m/3564817.1618420687@sss.pgh.pa.us
1 parent 8a2df44 commit f24b156
Copy full SHA for f24b156

File tree

Expand file treeCollapse file tree

4 files changed

+72
-86
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+72
-86
lines changed
Open diff view settings
Collapse file

‎src/backend/catalog/dependency.c‎

Copy file name to clipboardExpand all lines: src/backend/catalog/dependency.c
+50-54Lines changed: 50 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1835,8 +1835,17 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
18351835
* the datatype. However we do need a type dependency if there is no such
18361836
* indirect dependency, as for example in Const and CoerceToDomain nodes.
18371837
*
1838-
* Similarly, we don't need to create dependencies on collations except where
1839-
* the collation is being freshly introduced to the expression.
1838+
* Collations are handled primarily by recording the inputcollid's of node
1839+
* types that have them, as those are the ones that are semantically
1840+
* significant during expression evaluation. We also record the collation of
1841+
* CollateExpr nodes, since those will be needed to print such nodes even if
1842+
* they don't really affect semantics. Collations of leaf nodes such as Vars
1843+
* can be ignored on the grounds that if they're not default, they came from
1844+
* the referenced object (e.g., a table column), so the dependency on that
1845+
* object is enough. (Note: in a post-const-folding expression tree, a
1846+
* CollateExpr's collation could have been absorbed into a Const or
1847+
* RelabelType node. While ruleutils.c prints such collations for clarity,
1848+
* we may ignore them here as they have no semantic effect.)
18401849
*/
18411850
static bool
18421851
find_expr_references_walker(Node *node,
@@ -1876,29 +1885,6 @@ find_expr_references_walker(Node *node,
18761885
/* If it's a plain relation, reference this column */
18771886
add_object_address(OCLASS_CLASS, rte->relid, var->varattno,
18781887
context->addrs);
1879-
1880-
/* Top-level collation if valid */
1881-
if (OidIsValid(var->varcollid))
1882-
add_object_address(OCLASS_COLLATION, var->varcollid, 0,
1883-
context->addrs);
1884-
/* Otherwise, it may be a type with internal collations */
1885-
else if (var->vartype >= FirstNormalObjectId)
1886-
{
1887-
List *collations;
1888-
ListCell *lc;
1889-
1890-
collations = GetTypeCollations(var->vartype);
1891-
1892-
foreach(lc, collations)
1893-
{
1894-
Oid coll = lfirst_oid(lc);
1895-
1896-
if (OidIsValid(coll))
1897-
add_object_address(OCLASS_COLLATION,
1898-
lfirst_oid(lc), 0,
1899-
context->addrs);
1900-
}
1901-
}
19021888
}
19031889

19041890
/*
@@ -1920,15 +1906,6 @@ find_expr_references_walker(Node *node,
19201906
add_object_address(OCLASS_TYPE, con->consttype, 0,
19211907
context->addrs);
19221908

1923-
/*
1924-
* We must also depend on the constant's collation: it could be
1925-
* different from the datatype's, if a CollateExpr was const-folded to
1926-
* a simple constant.
1927-
*/
1928-
if (OidIsValid(con->constcollid))
1929-
add_object_address(OCLASS_COLLATION, con->constcollid, 0,
1930-
context->addrs);
1931-
19321909
/*
19331910
* If it's a regclass or similar literal referring to an existing
19341911
* object, add a reference to that object. (Currently, only the
@@ -2013,17 +1990,16 @@ find_expr_references_walker(Node *node,
20131990
/* A parameter must depend on the parameter's datatype */
20141991
add_object_address(OCLASS_TYPE, param->paramtype, 0,
20151992
context->addrs);
2016-
/* and its collation, just as for Consts */
2017-
if (OidIsValid(param->paramcollid))
2018-
add_object_address(OCLASS_COLLATION, param->paramcollid, 0,
2019-
context->addrs);
20201993
}
20211994
else if (IsA(node, FuncExpr))
20221995
{
20231996
FuncExpr *funcexpr = (FuncExpr *) node;
20241997

20251998
add_object_address(OCLASS_PROC, funcexpr->funcid, 0,
20261999
context->addrs);
2000+
if (OidIsValid(funcexpr->inputcollid))
2001+
add_object_address(OCLASS_COLLATION, funcexpr->inputcollid, 0,
2002+
context->addrs);
20272003
/* fall through to examine arguments */
20282004
}
20292005
else if (IsA(node, OpExpr))
@@ -2032,6 +2008,9 @@ find_expr_references_walker(Node *node,
20322008

20332009
add_object_address(OCLASS_OPERATOR, opexpr->opno, 0,
20342010
context->addrs);
2011+
if (OidIsValid(opexpr->inputcollid))
2012+
add_object_address(OCLASS_COLLATION, opexpr->inputcollid, 0,
2013+
context->addrs);
20352014
/* fall through to examine arguments */
20362015
}
20372016
else if (IsA(node, DistinctExpr))
@@ -2040,6 +2019,9 @@ find_expr_references_walker(Node *node,
20402019

20412020
add_object_address(OCLASS_OPERATOR, distinctexpr->opno, 0,
20422021
context->addrs);
2022+
if (OidIsValid(distinctexpr->inputcollid))
2023+
add_object_address(OCLASS_COLLATION, distinctexpr->inputcollid, 0,
2024+
context->addrs);
20432025
/* fall through to examine arguments */
20442026
}
20452027
else if (IsA(node, NullIfExpr))
@@ -2048,6 +2030,9 @@ find_expr_references_walker(Node *node,
20482030

20492031
add_object_address(OCLASS_OPERATOR, nullifexpr->opno, 0,
20502032
context->addrs);
2033+
if (OidIsValid(nullifexpr->inputcollid))
2034+
add_object_address(OCLASS_COLLATION, nullifexpr->inputcollid, 0,
2035+
context->addrs);
20512036
/* fall through to examine arguments */
20522037
}
20532038
else if (IsA(node, ScalarArrayOpExpr))
@@ -2056,6 +2041,9 @@ find_expr_references_walker(Node *node,
20562041

20572042
add_object_address(OCLASS_OPERATOR, opexpr->opno, 0,
20582043
context->addrs);
2044+
if (OidIsValid(opexpr->inputcollid))
2045+
add_object_address(OCLASS_COLLATION, opexpr->inputcollid, 0,
2046+
context->addrs);
20592047
/* fall through to examine arguments */
20602048
}
20612049
else if (IsA(node, Aggref))
@@ -2064,6 +2052,9 @@ find_expr_references_walker(Node *node,
20642052

20652053
add_object_address(OCLASS_PROC, aggref->aggfnoid, 0,
20662054
context->addrs);
2055+
if (OidIsValid(aggref->inputcollid))
2056+
add_object_address(OCLASS_COLLATION, aggref->inputcollid, 0,
2057+
context->addrs);
20672058
/* fall through to examine arguments */
20682059
}
20692060
else if (IsA(node, WindowFunc))
@@ -2072,6 +2063,9 @@ find_expr_references_walker(Node *node,
20722063

20732064
add_object_address(OCLASS_PROC, wfunc->winfnoid, 0,
20742065
context->addrs);
2066+
if (OidIsValid(wfunc->inputcollid))
2067+
add_object_address(OCLASS_COLLATION, wfunc->inputcollid, 0,
2068+
context->addrs);
20752069
/* fall through to examine arguments */
20762070
}
20772071
else if (IsA(node, SubscriptingRef))
@@ -2116,10 +2110,6 @@ find_expr_references_walker(Node *node,
21162110
else
21172111
add_object_address(OCLASS_TYPE, fselect->resulttype, 0,
21182112
context->addrs);
2119-
/* the collation might not be referenced anywhere else, either */
2120-
if (OidIsValid(fselect->resultcollid))
2121-
add_object_address(OCLASS_COLLATION, fselect->resultcollid, 0,
2122-
context->addrs);
21232113
}
21242114
else if (IsA(node, FieldStore))
21252115
{
@@ -2146,10 +2136,6 @@ find_expr_references_walker(Node *node,
21462136
/* since there is no function dependency, need to depend on type */
21472137
add_object_address(OCLASS_TYPE, relab->resulttype, 0,
21482138
context->addrs);
2149-
/* the collation might not be referenced anywhere else, either */
2150-
if (OidIsValid(relab->resultcollid))
2151-
add_object_address(OCLASS_COLLATION, relab->resultcollid, 0,
2152-
context->addrs);
21532139
}
21542140
else if (IsA(node, CoerceViaIO))
21552141
{
@@ -2158,10 +2144,6 @@ find_expr_references_walker(Node *node,
21582144
/* since there is no exposed function, need to depend on type */
21592145
add_object_address(OCLASS_TYPE, iocoerce->resulttype, 0,
21602146
context->addrs);
2161-
/* the collation might not be referenced anywhere else, either */
2162-
if (OidIsValid(iocoerce->resultcollid))
2163-
add_object_address(OCLASS_COLLATION, iocoerce->resultcollid, 0,
2164-
context->addrs);
21652147
}
21662148
else if (IsA(node, ArrayCoerceExpr))
21672149
{
@@ -2170,10 +2152,6 @@ find_expr_references_walker(Node *node,
21702152
/* as above, depend on type */
21712153
add_object_address(OCLASS_TYPE, acoerce->resulttype, 0,
21722154
context->addrs);
2173-
/* the collation might not be referenced anywhere else, either */
2174-
if (OidIsValid(acoerce->resultcollid))
2175-
add_object_address(OCLASS_COLLATION, acoerce->resultcollid, 0,
2176-
context->addrs);
21772155
/* fall through to examine arguments */
21782156
}
21792157
else if (IsA(node, ConvertRowtypeExpr))
@@ -2213,6 +2191,24 @@ find_expr_references_walker(Node *node,
22132191
add_object_address(OCLASS_OPFAMILY, lfirst_oid(l), 0,
22142192
context->addrs);
22152193
}
2194+
foreach(l, rcexpr->inputcollids)
2195+
{
2196+
Oid inputcollid = lfirst_oid(l);
2197+
2198+
if (OidIsValid(inputcollid))
2199+
add_object_address(OCLASS_COLLATION, inputcollid, 0,
2200+
context->addrs);
2201+
}
2202+
/* fall through to examine arguments */
2203+
}
2204+
else if (IsA(node, MinMaxExpr))
2205+
{
2206+
MinMaxExpr *mmexpr = (MinMaxExpr *) node;
2207+
2208+
/* minmaxtype will match one of the inputs, so no need to record it */
2209+
if (OidIsValid(mmexpr->inputcollid))
2210+
add_object_address(OCLASS_COLLATION, mmexpr->inputcollid, 0,
2211+
context->addrs);
22162212
/* fall through to examine arguments */
22172213
}
22182214
else if (IsA(node, CoerceToDomain))
Collapse file

‎src/backend/catalog/pg_type.c‎

Copy file name to clipboardExpand all lines: src/backend/catalog/pg_type.c
+19-10Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -535,8 +535,12 @@ GetTypeCollations(Oid typeoid)
535535
elog(ERROR, "cache lookup failed for type %u", typeoid);
536536
typeTup = (Form_pg_type) GETSTRUCT(tuple);
537537

538+
/*
539+
* If the type has a typcollation attribute, report that and we're done.
540+
* Otherwise, it could be a container type that we should recurse into.
541+
*/
538542
if (OidIsValid(typeTup->typcollation))
539-
result = list_append_unique_oid(result, typeTup->typcollation);
543+
result = list_make1_oid(typeTup->typcollation);
540544
else if (typeTup->typtype == TYPTYPE_COMPOSITE)
541545
{
542546
Relation rel = relation_open(typeTup->typrelid, AccessShareLock);
@@ -546,6 +550,8 @@ GetTypeCollations(Oid typeoid)
546550
{
547551
Form_pg_attribute att = TupleDescAttr(desc, i);
548552

553+
if (att->attisdropped)
554+
continue;
549555
if (OidIsValid(att->attcollation))
550556
result = list_append_unique_oid(result, att->attcollation);
551557
else
@@ -558,21 +564,24 @@ GetTypeCollations(Oid typeoid)
558564
else if (typeTup->typtype == TYPTYPE_DOMAIN)
559565
{
560566
Assert(OidIsValid(typeTup->typbasetype));
561-
562-
result = list_concat_unique_oid(result,
563-
GetTypeCollations(typeTup->typbasetype));
567+
result = GetTypeCollations(typeTup->typbasetype);
564568
}
565569
else if (typeTup->typtype == TYPTYPE_RANGE)
566570
{
567-
Oid rangeid = get_range_subtype(typeTup->oid);
571+
Oid rangecoll = get_range_collation(typeTup->oid);
568572

569-
Assert(OidIsValid(rangeid));
573+
if (OidIsValid(rangecoll))
574+
result = list_make1_oid(rangecoll);
575+
else
576+
{
577+
Oid rangeid = get_range_subtype(typeTup->oid);
570578

571-
result = list_concat_unique_oid(result, GetTypeCollations(rangeid));
579+
Assert(OidIsValid(rangeid));
580+
result = GetTypeCollations(rangeid);
581+
}
572582
}
573-
else if (OidIsValid(typeTup->typelem))
574-
result = list_concat_unique_oid(result,
575-
GetTypeCollations(typeTup->typelem));
583+
else if (IsTrueArrayType(typeTup))
584+
result = GetTypeCollations(typeTup->typelem);
576585

577586
ReleaseSysCache(tuple);
578587

Collapse file

‎src/test/regress/expected/collate.icu.utf8.out‎

Copy file name to clipboardExpand all lines: src/test/regress/expected/collate.icu.utf8.out
+2-21Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1958,7 +1958,7 @@ CREATE DOMAIN d_custom AS t_custom;
19581958
CREATE COLLATION custom (
19591959
LOCALE = 'fr-x-icu', PROVIDER = 'icu'
19601960
);
1961-
CREATE TYPE myrange AS range (subtype = text);
1961+
CREATE TYPE myrange AS range (subtype = text, collation = "POSIX");
19621962
CREATE TYPE myrange_en_fr_ga AS range(subtype = t_en_fr_ga);
19631963
CREATE TABLE collate_test (
19641964
id integer,
@@ -2054,36 +2054,17 @@ ORDER BY 1, 2, 3;
20542054
icuidx05_d_en_fr_ga_arr | "en-x-icu" | up to date
20552055
icuidx05_d_en_fr_ga_arr | "fr-x-icu" | up to date
20562056
icuidx05_d_en_fr_ga_arr | "ga-x-icu" | up to date
2057-
icuidx06_d_en_fr_ga | "default" | XXX
2058-
icuidx06_d_en_fr_ga | "en-x-icu" | up to date
20592057
icuidx06_d_en_fr_ga | "fr-x-icu" | up to date
2060-
icuidx06_d_en_fr_ga | "ga-x-icu" | up to date
2061-
icuidx07_d_en_fr_ga | "default" | XXX
2062-
icuidx07_d_en_fr_ga | "en-x-icu" | up to date
2063-
icuidx07_d_en_fr_ga | "fr-x-icu" | up to date
20642058
icuidx07_d_en_fr_ga | "ga-x-icu" | up to date
2065-
icuidx08_d_en_fr_ga | "en-x-icu" | up to date
2066-
icuidx08_d_en_fr_ga | "fr-x-icu" | up to date
2067-
icuidx08_d_en_fr_ga | "ga-x-icu" | up to date
2068-
icuidx09_d_en_fr_ga | "en-x-icu" | up to date
2069-
icuidx09_d_en_fr_ga | "fr-x-icu" | up to date
2070-
icuidx09_d_en_fr_ga | "ga-x-icu" | up to date
2071-
icuidx10_d_en_fr_ga_es | "en-x-icu" | up to date
20722059
icuidx10_d_en_fr_ga_es | "es-x-icu" | up to date
2073-
icuidx10_d_en_fr_ga_es | "fr-x-icu" | up to date
2074-
icuidx10_d_en_fr_ga_es | "ga-x-icu" | up to date
2075-
icuidx11_d_es | "default" | XXX
20762060
icuidx11_d_es | "es-x-icu" | up to date
2077-
icuidx12_custom | "default" | XXX
20782061
icuidx12_custom | custom | up to date
2079-
icuidx13_custom | "default" | XXX
20802062
icuidx13_custom | custom | up to date
2081-
icuidx14_myrange | "default" | XXX
20822063
icuidx15_myrange_en_fr_ga | "en-x-icu" | up to date
20832064
icuidx15_myrange_en_fr_ga | "fr-x-icu" | up to date
20842065
icuidx15_myrange_en_fr_ga | "ga-x-icu" | up to date
20852066
icuidx17_part | "en-x-icu" | up to date
2086-
(57 rows)
2067+
(38 rows)
20872068

20882069
-- Validate that REINDEX will update the stored version.
20892070
UPDATE pg_depend SET refobjversion = 'not a version'
Collapse file

‎src/test/regress/sql/collate.icu.utf8.sql‎

Copy file name to clipboardExpand all lines: src/test/regress/sql/collate.icu.utf8.sql
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -755,7 +755,7 @@ CREATE COLLATION custom (
755755
LOCALE = 'fr-x-icu', PROVIDER = 'icu'
756756
);
757757

758-
CREATE TYPE myrange AS range (subtype = text);
758+
CREATE TYPE myrange AS range (subtype = text, collation = "POSIX");
759759
CREATE TYPE myrange_en_fr_ga AS range(subtype = t_en_fr_ga);
760760

761761
CREATE TABLE collate_test (

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.