public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v5] ALTER tbl rewrite loses CLUSTER ON index
44+ messages / 13 participants
[nested] [flat]
* [PATCH v5] ALTER tbl rewrite loses CLUSTER ON index
@ 2020-02-06 09:14 Amit Langote <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Amit Langote @ 2020-02-06 09:14 UTC (permalink / raw)
---
src/backend/commands/tablecmds.c | 39 +++++++++++++++++++++++
src/test/regress/expected/alter_table.out | 34 ++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 16 +++++++++-
3 files changed, 88 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 02a7c04fdb..6b2469bd09 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -490,6 +490,7 @@ static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
static void RebuildConstraintComment(AlteredTableInfo *tab, int pass,
Oid objid, Relation rel, List *domname,
const char *conname);
+static void PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid);
static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
static void TryReuseForeignKey(Oid oldId, Constraint *con);
static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
@@ -11838,6 +11839,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
newcmd->def = (Node *) stmt;
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, oldId);
}
else if (IsA(stm, AlterTableStmt))
{
@@ -11874,6 +11878,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
rel,
NIL,
indstmt->idxname);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, indoid);
}
else if (cmd->subtype == AT_AddConstraint)
{
@@ -11996,6 +12003,38 @@ RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
}
+/*
+ * For a table's index that is to be recreated due to PostAlterType
+ * processing, preserve its indisclustered property by issuing ALTER TABLE
+ * CLUSTER ON command on the table that will run after the command to recreate
+ * the index.
+ */
+static void
+PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid)
+{
+ HeapTuple indexTuple;
+ Form_pg_index indexForm;
+
+ Assert(OidIsValid(indoid));
+ Assert(pass == AT_PASS_OLD_INDEX);
+
+ indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indoid));
+ if (!HeapTupleIsValid(indexTuple))
+ elog(ERROR, "cache lookup failed for index %u", indoid);
+ indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+
+ if (indexForm->indisclustered)
+ {
+ AlterTableCmd *newcmd = makeNode(AlterTableCmd);
+
+ newcmd->subtype = AT_ClusterOn;
+ newcmd->name = get_rel_name(indoid);
+ tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
+ }
+
+ ReleaseSysCache(indexTuple);
+}
+
/*
* Subroutine for ATPostAlterTypeParse(). Calls out to CheckIndexCompatible()
* for the real analysis, then mutates the IndexStmt based on that verdict.
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index fb6d86a269..a01c6d6ec5 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4296,3 +4296,37 @@ create trigger xtrig
update bar1 set a = a + 1;
INFO: a=1, b=1
/* End test case for bug #16242 */
+-- alter type rewrite/rebuild should preserve cluster marking on index
+create table alttype_cluster (a int);
+create index alttype_cluster_a on alttype_cluster (a);
+alter table alttype_cluster cluster on alttype_cluster_a;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered
+----------------
+ t
+(1 row)
+
+alter table alttype_cluster alter a type bigint;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered
+----------------
+ t
+(1 row)
+
+drop index alttype_cluster_a;
+alter table alttype_cluster add primary key (a);
+alter table alttype_cluster cluster on alttype_cluster_pkey;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered
+----------------
+ t
+(1 row)
+
+alter table alttype_cluster alter a type int;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered
+----------------
+ t
+(1 row)
+
+drop table alttype_cluster;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 3801f19c58..6e9048bbec 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2802,7 +2802,6 @@ drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
-
/* Test case for bug #16242 */
-- We create a parent and child where the child has missing
@@ -2840,3 +2839,18 @@ create trigger xtrig
update bar1 set a = a + 1;
/* End test case for bug #16242 */
+
+-- alter type rewrite/rebuild should preserve cluster marking on index
+create table alttype_cluster (a int);
+create index alttype_cluster_a on alttype_cluster (a);
+alter table alttype_cluster cluster on alttype_cluster_a;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+alter table alttype_cluster alter a type bigint;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+drop index alttype_cluster_a;
+alter table alttype_cluster add primary key (a);
+alter table alttype_cluster cluster on alttype_cluster_pkey;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+alter table alttype_cluster alter a type int;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+drop table alttype_cluster;
--
2.17.0
--bg08WKrSYDhXBjb5--
^ permalink raw reply [nested|flat] 44+ messages in thread
* [PATCH v7 1/2] ALTER tbl rewrite loses CLUSTER ON index
@ 2020-03-16 07:01 Amit Langote <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Amit Langote @ 2020-03-16 07:01 UTC (permalink / raw)
On Fri, Mar 13, 2020 at 2:19 AM Tom Lane <[email protected]> wrote:
> Justin Pryzby <[email protected]> writes:
> > @cfbot: resending with only Amit's 0001, since Michael pushed a variation on
> > 0002.
Thank you for taking a look at it.
> Boy, I really dislike this patch. ATPostAlterTypeParse is documented as
> using the supplied definition string, and nothing else, to reconstruct
> the index. This breaks that without even the courtesy of documenting
> the breakage. Moreover, the reason why it's designed like that is to
> avoid requiring the old index objects to still be accessible. So I'm
> surprised that this hack works at all. I don't think it would have
> worked at the time the code was first written, and I think it's imposing
> a constraint we'll have problems with (again?) in future.
Okay, so maybe in the middle of ATPostAlterTypeParse() is not a place
to do it, but don't these arguments apply to
RebuildConstraintComment(), which I based the patch on?
> The right way to fix this is to note the presence of the indisclustered
> flag when we're examining the index earlier, and include a suitable
> command in the definition string. So probably pg_get_indexdef_string()
> is what needs to change. It doesn't look like that's used anywhere
> else, so we can just redefine its behavior as needed.
I came across a commit that recently went in:
commit 1cc9c2412cc9a2fbe6a381170097d315fd40ccca
Author: Peter Eisentraut <[email protected]>
Date: Fri Mar 13 11:28:11 2020 +0100
Preserve replica identity index across ALTER TABLE rewrite
which fixes something very similar to what we are trying to with this
patch. The way it's done looks to me very close to what you are
telling. I have updated the patch to be similar to the above fix.
--
Thank you,
Amit
---
src/backend/commands/tablecmds.c | 49 +++++++++++++++++++++++
src/backend/utils/cache/lsyscache.c | 23 +++++++++++
src/include/utils/lsyscache.h | 1 +
src/test/regress/expected/alter_table.out | 34 ++++++++++++++++
src/test/regress/sql/alter_table.sql | 15 +++++++
5 files changed, 122 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8c33b67c1b..73d9392878 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -177,6 +177,7 @@ typedef struct AlteredTableInfo
List *changedIndexOids; /* OIDs of indexes to rebuild */
List *changedIndexDefs; /* string definitions of same */
char *replicaIdentityIndex; /* index to reset as REPLICA IDENTITY */
+ char *clusterOnIndex; /* index to CLUSTER ON */
} AlteredTableInfo;
/* Struct describing one new constraint to check in Phase 3 scan */
@@ -11579,9 +11580,28 @@ RememberReplicaIdentityForRebuilding(Oid indoid, AlteredTableInfo *tab)
tab->replicaIdentityIndex = get_rel_name(indoid);
}
+/*
+ * Subroutine for ATExecAlterColumnType: remember any clustered index.
+ */
+static void
+RememberClusterOnForRebuilding(Oid indoid, AlteredTableInfo *tab)
+{
+ if (!get_index_isclustered(indoid))
+ return;
+
+ if (tab->clusterOnIndex)
+ elog(ERROR, "relation %u has multiple clustered indexes", tab->relid);
+
+ tab->clusterOnIndex = get_rel_name(indoid);
+}
+
/*
* Subroutine for ATExecAlterColumnType: remember that a constraint needs
* to be rebuilt (which we might already know).
+ *
+ * For constraint's index (if any), also remember if it's the table's replica
+ * identity or its clustered index, so that ATPostAlterTypeCleanup() can
+ * queue up commands necessary to restore that property.
*/
static void
RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab)
@@ -11604,9 +11624,18 @@ RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab)
tab->changedConstraintDefs = lappend(tab->changedConstraintDefs,
defstring);
+ /*
+ * For constraint's index (if any), remember if it's the table's
+ * replica identity or its clustered index, so that
+ * ATPostAlterTypeCleanup() can queue up commands necessary to restore
+ * that property.
+ */
indoid = get_constraint_index(conoid);
if (OidIsValid(indoid))
+ {
RememberReplicaIdentityForRebuilding(indoid, tab);
+ RememberClusterOnForRebuilding(indoid, tab);
+ }
}
}
@@ -11650,7 +11679,13 @@ RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab)
tab->changedIndexDefs = lappend(tab->changedIndexDefs,
defstring);
+ /*
+ * Remember if it's the table's replica identity or its clustered
+ * index, so that ATPostAlterTypeCleanup() can queue up commands
+ * necessary to restore that property.
+ */
RememberReplicaIdentityForRebuilding(indoid, tab);
+ RememberClusterOnForRebuilding(indoid, tab);
}
}
}
@@ -11777,6 +11812,20 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
}
+ /*
+ * Queue up command to restore clustered index marking
+ */
+ if (tab->clusterOnIndex)
+ {
+ AlterTableCmd *cmd = makeNode(AlterTableCmd);
+
+ cmd->subtype = AT_ClusterOn;
+ cmd->name = tab->clusterOnIndex;
+
+ /* do it after indexes and constraints */
+ tab->subcmds[AT_PASS_OLD_CONSTR] =
+ lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
+ }
/*
* It should be okay to use DROP_RESTRICT here, since nothing else should
* be depending on these objects.
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 27bbb58f56..9c5b806c22 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -3299,3 +3299,26 @@ get_index_isvalid(Oid index_oid)
return isvalid;
}
+
+/*
+ * get_index_isclustered
+ *
+ * Given the index OID, return pg_index.indisclustered.
+ */
+bool
+get_index_isclustered(Oid index_oid)
+{
+ bool isclustered;
+ HeapTuple tuple;
+ Form_pg_index rd_index;
+
+ tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(index_oid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for index %u", index_oid);
+
+ rd_index = (Form_pg_index) GETSTRUCT(tuple);
+ isclustered = rd_index->indisclustered;
+ ReleaseSysCache(tuple);
+
+ return isclustered;
+}
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 4e646c55e9..9459c6a94f 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -184,6 +184,7 @@ extern Oid get_range_collation(Oid rangeOid);
extern Oid get_index_column_opclass(Oid index_oid, int attno);
extern bool get_index_isreplident(Oid index_oid);
extern bool get_index_isvalid(Oid index_oid);
+extern bool get_index_isclustered(Oid index_oid);
#define type_is_array(typid) (get_element_type(typid) != InvalidOid)
/* type_is_array_domain accepts both plain arrays and domains over arrays */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index fb6d86a269..a01c6d6ec5 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4296,3 +4296,37 @@ create trigger xtrig
update bar1 set a = a + 1;
INFO: a=1, b=1
/* End test case for bug #16242 */
+-- alter type rewrite/rebuild should preserve cluster marking on index
+create table alttype_cluster (a int);
+create index alttype_cluster_a on alttype_cluster (a);
+alter table alttype_cluster cluster on alttype_cluster_a;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered
+----------------
+ t
+(1 row)
+
+alter table alttype_cluster alter a type bigint;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered
+----------------
+ t
+(1 row)
+
+drop index alttype_cluster_a;
+alter table alttype_cluster add primary key (a);
+alter table alttype_cluster cluster on alttype_cluster_pkey;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered
+----------------
+ t
+(1 row)
+
+alter table alttype_cluster alter a type int;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered
+----------------
+ t
+(1 row)
+
+drop table alttype_cluster;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 3801f19c58..4eeec24e58 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2840,3 +2840,18 @@ create trigger xtrig
update bar1 set a = a + 1;
/* End test case for bug #16242 */
+
+-- alter type rewrite/rebuild should preserve cluster marking on index
+create table alttype_cluster (a int);
+create index alttype_cluster_a on alttype_cluster (a);
+alter table alttype_cluster cluster on alttype_cluster_a;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+alter table alttype_cluster alter a type bigint;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+drop index alttype_cluster_a;
+alter table alttype_cluster add primary key (a);
+alter table alttype_cluster cluster on alttype_cluster_pkey;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+alter table alttype_cluster alter a type int;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+drop table alttype_cluster;
--
2.17.0
--aZoGpuMECXJckB41
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0002-Use-get_index_isclustered-in-cluster.c.patch"
^ permalink raw reply [nested|flat] 44+ messages in thread
* [PATCH v25 8/9] Row pattern recognition patch (typedefs.list).
@ 2024-12-21 06:19 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Tatsuo Ishii @ 2024-12-21 06:19 UTC (permalink / raw)
---
src/tools/pgindent/typedefs.list | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 971a150dbb..bf78e162bc 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1690,6 +1690,7 @@ NamedLWLockTrancheRequest
NamedTuplestoreScan
NamedTuplestoreScanState
NamespaceInfo
+NavigationInfo
NestLoop
NestLoopParam
NestLoopState
@@ -2316,6 +2317,9 @@ RI_CompareKey
RI_ConstraintInfo
RI_QueryHashEntry
RI_QueryKey
+RPCommonSyntax
+RPSkipTo
+RPSubsetItem
RTEKind
RTEPermissionInfo
RWConflict
@@ -2670,6 +2674,7 @@ SimpleStringList
SimpleStringListCell
SingleBoundSortItem
Size
+SkipContext
SkipPages
SlabBlock
SlabContext
@@ -2761,6 +2766,7 @@ StreamStopReason
String
StringInfo
StringInfoData
+StringSet
StripnullState
SubLink
SubLinkType
@@ -3085,6 +3091,7 @@ VarString
VarStringSortSupport
Variable
VariableAssignHook
+VariablePos
VariableSetKind
VariableSetStmt
VariableShowStmt
--
2.25.1
----Next_Part(Sat_Dec_21_18_20_04_2024_526)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v25-0009-Allow-to-print-raw-parse-tree.patch"
^ permalink raw reply [nested|flat] 44+ messages in thread
* [PATCH v25 8/9] Row pattern recognition patch (typedefs.list).
@ 2024-12-21 06:19 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Tatsuo Ishii @ 2024-12-21 06:19 UTC (permalink / raw)
---
src/tools/pgindent/typedefs.list | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 971a150dbb..bf78e162bc 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1690,6 +1690,7 @@ NamedLWLockTrancheRequest
NamedTuplestoreScan
NamedTuplestoreScanState
NamespaceInfo
+NavigationInfo
NestLoop
NestLoopParam
NestLoopState
@@ -2316,6 +2317,9 @@ RI_CompareKey
RI_ConstraintInfo
RI_QueryHashEntry
RI_QueryKey
+RPCommonSyntax
+RPSkipTo
+RPSubsetItem
RTEKind
RTEPermissionInfo
RWConflict
@@ -2670,6 +2674,7 @@ SimpleStringList
SimpleStringListCell
SingleBoundSortItem
Size
+SkipContext
SkipPages
SlabBlock
SlabContext
@@ -2761,6 +2766,7 @@ StreamStopReason
String
StringInfo
StringInfoData
+StringSet
StripnullState
SubLink
SubLinkType
@@ -3085,6 +3091,7 @@ VarString
VarStringSortSupport
Variable
VariableAssignHook
+VariablePos
VariableSetKind
VariableSetStmt
VariableShowStmt
--
2.25.1
----Next_Part(Sat_Dec_21_18_20_04_2024_526)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v25-0009-Allow-to-print-raw-parse-tree.patch"
^ permalink raw reply [nested|flat] 44+ messages in thread
* [PATCH v25 8/9] Row pattern recognition patch (typedefs.list).
@ 2024-12-21 06:19 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Tatsuo Ishii @ 2024-12-21 06:19 UTC (permalink / raw)
---
src/tools/pgindent/typedefs.list | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 971a150dbb..bf78e162bc 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1690,6 +1690,7 @@ NamedLWLockTrancheRequest
NamedTuplestoreScan
NamedTuplestoreScanState
NamespaceInfo
+NavigationInfo
NestLoop
NestLoopParam
NestLoopState
@@ -2316,6 +2317,9 @@ RI_CompareKey
RI_ConstraintInfo
RI_QueryHashEntry
RI_QueryKey
+RPCommonSyntax
+RPSkipTo
+RPSubsetItem
RTEKind
RTEPermissionInfo
RWConflict
@@ -2670,6 +2674,7 @@ SimpleStringList
SimpleStringListCell
SingleBoundSortItem
Size
+SkipContext
SkipPages
SlabBlock
SlabContext
@@ -2761,6 +2766,7 @@ StreamStopReason
String
StringInfo
StringInfoData
+StringSet
StripnullState
SubLink
SubLinkType
@@ -3085,6 +3091,7 @@ VarString
VarStringSortSupport
Variable
VariableAssignHook
+VariablePos
VariableSetKind
VariableSetStmt
VariableShowStmt
--
2.25.1
----Next_Part(Sat_Dec_21_18_20_04_2024_526)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v25-0009-Allow-to-print-raw-parse-tree.patch"
^ permalink raw reply [nested|flat] 44+ messages in thread
* [PATCH v26 8/9] Row pattern recognition patch (typedefs.list).
@ 2024-12-30 12:44 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Tatsuo Ishii @ 2024-12-30 12:44 UTC (permalink / raw)
---
src/tools/pgindent/typedefs.list | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index e1c4f913f8..72906d0fc1 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1691,6 +1691,7 @@ NamedLWLockTrancheRequest
NamedTuplestoreScan
NamedTuplestoreScanState
NamespaceInfo
+NavigationInfo
NestLoop
NestLoopParam
NestLoopState
@@ -2317,6 +2318,9 @@ RI_CompareKey
RI_ConstraintInfo
RI_QueryHashEntry
RI_QueryKey
+RPCommonSyntax
+RPSkipTo
+RPSubsetItem
RTEKind
RTEPermissionInfo
RWConflict
@@ -2673,6 +2677,7 @@ SimpleStringList
SimpleStringListCell
SingleBoundSortItem
Size
+SkipContext
SkipPages
SlabBlock
SlabContext
@@ -2764,6 +2769,7 @@ StreamStopReason
String
StringInfo
StringInfoData
+StringSet
StripnullState
SubLink
SubLinkType
@@ -3088,6 +3094,7 @@ VarString
VarStringSortSupport
Variable
VariableAssignHook
+VariablePos
VariableSetKind
VariableSetStmt
VariableShowStmt
--
2.25.1
----Next_Part(Mon_Dec_30_22_37_18_2024_171)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v26-0009-Allow-to-print-raw-parse-tree.patch"
^ permalink raw reply [nested|flat] 44+ messages in thread
* [PATCH v26 8/9] Row pattern recognition patch (typedefs.list).
@ 2024-12-30 12:44 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Tatsuo Ishii @ 2024-12-30 12:44 UTC (permalink / raw)
---
src/tools/pgindent/typedefs.list | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index e1c4f913f8..72906d0fc1 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1691,6 +1691,7 @@ NamedLWLockTrancheRequest
NamedTuplestoreScan
NamedTuplestoreScanState
NamespaceInfo
+NavigationInfo
NestLoop
NestLoopParam
NestLoopState
@@ -2317,6 +2318,9 @@ RI_CompareKey
RI_ConstraintInfo
RI_QueryHashEntry
RI_QueryKey
+RPCommonSyntax
+RPSkipTo
+RPSubsetItem
RTEKind
RTEPermissionInfo
RWConflict
@@ -2673,6 +2677,7 @@ SimpleStringList
SimpleStringListCell
SingleBoundSortItem
Size
+SkipContext
SkipPages
SlabBlock
SlabContext
@@ -2764,6 +2769,7 @@ StreamStopReason
String
StringInfo
StringInfoData
+StringSet
StripnullState
SubLink
SubLinkType
@@ -3088,6 +3094,7 @@ VarString
VarStringSortSupport
Variable
VariableAssignHook
+VariablePos
VariableSetKind
VariableSetStmt
VariableShowStmt
--
2.25.1
----Next_Part(Mon_Dec_30_22_37_18_2024_171)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v26-0009-Allow-to-print-raw-parse-tree.patch"
^ permalink raw reply [nested|flat] 44+ messages in thread
* [PATCH v26 8/9] Row pattern recognition patch (typedefs.list).
@ 2024-12-30 12:44 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Tatsuo Ishii @ 2024-12-30 12:44 UTC (permalink / raw)
---
src/tools/pgindent/typedefs.list | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index e1c4f913f8..72906d0fc1 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1691,6 +1691,7 @@ NamedLWLockTrancheRequest
NamedTuplestoreScan
NamedTuplestoreScanState
NamespaceInfo
+NavigationInfo
NestLoop
NestLoopParam
NestLoopState
@@ -2317,6 +2318,9 @@ RI_CompareKey
RI_ConstraintInfo
RI_QueryHashEntry
RI_QueryKey
+RPCommonSyntax
+RPSkipTo
+RPSubsetItem
RTEKind
RTEPermissionInfo
RWConflict
@@ -2673,6 +2677,7 @@ SimpleStringList
SimpleStringListCell
SingleBoundSortItem
Size
+SkipContext
SkipPages
SlabBlock
SlabContext
@@ -2764,6 +2769,7 @@ StreamStopReason
String
StringInfo
StringInfoData
+StringSet
StripnullState
SubLink
SubLinkType
@@ -3088,6 +3094,7 @@ VarString
VarStringSortSupport
Variable
VariableAssignHook
+VariablePos
VariableSetKind
VariableSetStmt
VariableShowStmt
--
2.25.1
----Next_Part(Mon_Dec_30_22_37_18_2024_171)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v26-0009-Allow-to-print-raw-parse-tree.patch"
^ permalink raw reply [nested|flat] 44+ messages in thread
* [PATCH v27 8/9] Row pattern recognition patch (typedefs.list).
@ 2024-12-30 23:53 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Tatsuo Ishii @ 2024-12-30 23:53 UTC (permalink / raw)
---
src/tools/pgindent/typedefs.list | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index e1c4f913f8..72906d0fc1 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1691,6 +1691,7 @@ NamedLWLockTrancheRequest
NamedTuplestoreScan
NamedTuplestoreScanState
NamespaceInfo
+NavigationInfo
NestLoop
NestLoopParam
NestLoopState
@@ -2317,6 +2318,9 @@ RI_CompareKey
RI_ConstraintInfo
RI_QueryHashEntry
RI_QueryKey
+RPCommonSyntax
+RPSkipTo
+RPSubsetItem
RTEKind
RTEPermissionInfo
RWConflict
@@ -2673,6 +2677,7 @@ SimpleStringList
SimpleStringListCell
SingleBoundSortItem
Size
+SkipContext
SkipPages
SlabBlock
SlabContext
@@ -2764,6 +2769,7 @@ StreamStopReason
String
StringInfo
StringInfoData
+StringSet
StripnullState
SubLink
SubLinkType
@@ -3088,6 +3094,7 @@ VarString
VarStringSortSupport
Variable
VariableAssignHook
+VariablePos
VariableSetKind
VariableSetStmt
VariableShowStmt
--
2.25.1
----Next_Part(Tue_Dec_31_08_57_07_2024_963)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v27-0009-Allow-to-print-raw-parse-tree.patch"
^ permalink raw reply [nested|flat] 44+ messages in thread
* [PATCH v27 8/9] Row pattern recognition patch (typedefs.list).
@ 2024-12-30 23:53 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Tatsuo Ishii @ 2024-12-30 23:53 UTC (permalink / raw)
---
src/tools/pgindent/typedefs.list | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index e1c4f913f8..72906d0fc1 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1691,6 +1691,7 @@ NamedLWLockTrancheRequest
NamedTuplestoreScan
NamedTuplestoreScanState
NamespaceInfo
+NavigationInfo
NestLoop
NestLoopParam
NestLoopState
@@ -2317,6 +2318,9 @@ RI_CompareKey
RI_ConstraintInfo
RI_QueryHashEntry
RI_QueryKey
+RPCommonSyntax
+RPSkipTo
+RPSubsetItem
RTEKind
RTEPermissionInfo
RWConflict
@@ -2673,6 +2677,7 @@ SimpleStringList
SimpleStringListCell
SingleBoundSortItem
Size
+SkipContext
SkipPages
SlabBlock
SlabContext
@@ -2764,6 +2769,7 @@ StreamStopReason
String
StringInfo
StringInfoData
+StringSet
StripnullState
SubLink
SubLinkType
@@ -3088,6 +3094,7 @@ VarString
VarStringSortSupport
Variable
VariableAssignHook
+VariablePos
VariableSetKind
VariableSetStmt
VariableShowStmt
--
2.25.1
----Next_Part(Tue_Dec_31_08_57_07_2024_963)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v27-0009-Allow-to-print-raw-parse-tree.patch"
^ permalink raw reply [nested|flat] 44+ messages in thread
* [PATCH v27 8/9] Row pattern recognition patch (typedefs.list).
@ 2024-12-30 23:53 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Tatsuo Ishii @ 2024-12-30 23:53 UTC (permalink / raw)
---
src/tools/pgindent/typedefs.list | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index e1c4f913f8..72906d0fc1 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1691,6 +1691,7 @@ NamedLWLockTrancheRequest
NamedTuplestoreScan
NamedTuplestoreScanState
NamespaceInfo
+NavigationInfo
NestLoop
NestLoopParam
NestLoopState
@@ -2317,6 +2318,9 @@ RI_CompareKey
RI_ConstraintInfo
RI_QueryHashEntry
RI_QueryKey
+RPCommonSyntax
+RPSkipTo
+RPSubsetItem
RTEKind
RTEPermissionInfo
RWConflict
@@ -2673,6 +2677,7 @@ SimpleStringList
SimpleStringListCell
SingleBoundSortItem
Size
+SkipContext
SkipPages
SlabBlock
SlabContext
@@ -2764,6 +2769,7 @@ StreamStopReason
String
StringInfo
StringInfoData
+StringSet
StripnullState
SubLink
SubLinkType
@@ -3088,6 +3094,7 @@ VarString
VarStringSortSupport
Variable
VariableAssignHook
+VariablePos
VariableSetKind
VariableSetStmt
VariableShowStmt
--
2.25.1
----Next_Part(Tue_Dec_31_08_57_07_2024_963)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v27-0009-Allow-to-print-raw-parse-tree.patch"
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
@ 2025-02-12 16:02 Tom Lane <[email protected]>
2025-02-12 16:46 ` Re: Small memory fixes for pg_createsubcriber Ranier Vilela <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
0 siblings, 2 replies; 44+ messages in thread
From: Tom Lane @ 2025-02-12 16:02 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
Andres Freund <[email protected]> writes:
> On 2025-02-11 13:32:32 -0300, Euler Taveira wrote:
>> There is no bug. They are the same behind the scenes.
> That *is* a bug. On windows the allocator that a shared library (i.e. libpq)
> uses, may *not* be the same as the one that an executable
> (i.e. pg_createsubscriber). It's not correct to free memory allocated in a
> shared library just with free, it has to go through the library's free.
Indeed. This is particularly pernicious because it will work even on
Windows under common scenarios (which no doubt explains the lack of
field reports). From the last time we discussed this [1]:
It seems to work fine as long as a debug-readline is paired with a
debug-psql or a release-readline is paired with a release-psql.
I wish we had some way to detect misuses automatically ...
This seems like the sort of bug that Coverity could detect if only it
knew to look, but I have no idea if it could be configured that way.
Maybe some weird lashup with a debugging malloc library would be
another way.
regards, tom lane
[1] https://www.postgresql.org/message-id/[email protected]
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
@ 2025-02-12 16:46 ` Ranier Vilela <[email protected]>
1 sibling, 0 replies; 44+ messages in thread
From: Ranier Vilela @ 2025-02-12 16:46 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; pgsql-hackers
Hi.
Em qua., 12 de fev. de 2025 às 13:02, Tom Lane <[email protected]> escreveu:
> Andres Freund <[email protected]> writes:
> > On 2025-02-11 13:32:32 -0300, Euler Taveira wrote:
> >> There is no bug. They are the same behind the scenes.
>
> > That *is* a bug. On windows the allocator that a shared library (i.e.
> libpq)
> > uses, may *not* be the same as the one that an executable
> > (i.e. pg_createsubscriber). It's not correct to free memory allocated
> in a
> > shared library just with free, it has to go through the library's free.
>
> Indeed. This is particularly pernicious because it will work even on
> Windows under common scenarios (which no doubt explains the lack of
> field reports). From the last time we discussed this [1]:
>
> It seems to work fine as long as a debug-readline is paired with a
> debug-psql or a release-readline is paired with a release-psql.
>
> I wish we had some way to detect misuses automatically ...
>
Unfortunately, for now, there is only manual search.
In fact, after your post, I did a new search and found two other
occurrences.
(src/bin/pg_amcheck/pg_amcheck.c)
One fixed in the patch attached.
Another *dat->amcheck_schema* is it not worth the effort,
since the memory is not released at any point?
Trivial patch attached.
best regards,
Ranier Vilela
Attachments:
[application/octet-stream] fix-api-misuse-pg_amcheck.patch (371B, ../../CAEudQArD_nKSnYCNUZiPPsJ2tNXgRmLbXGSOrH1vpOF_XtP0Vg@mail.gmail.com/3-fix-api-misuse-pg_amcheck.patch)
download | inline diff:
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index c5ec25be22..dda8b1b8ad 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -560,7 +560,7 @@ main(int argc, char *argv[])
executeCommand(conn, install_sql, opts.echo);
pfree(install_sql);
- pfree(schema);
+ PQfreemem(schema);
}
/*
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
@ 2025-02-12 17:00 ` Andres Freund <[email protected]>
2025-02-12 17:17 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 23:56 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
1 sibling, 2 replies; 44+ messages in thread
From: Andres Freund @ 2025-02-12 17:00 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
Hi,
On 2025-02-12 11:02:04 -0500, Tom Lane wrote:
> I wish we had some way to detect misuses automatically ...
>
> This seems like the sort of bug that Coverity could detect if only it
> knew to look, but I have no idea if it could be configured that way.
> Maybe some weird lashup with a debugging malloc library would be
> another way.
Recent gcc actually has a fairly good way to detect this kind of issue:
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-malloc-function-attribute
in particular, the variant of the attribute with "deallocator".
I'd started on a patch to add those, but ran out of cycles for 18.
I quickly checked out my branch and added the relevant attributes to
PQescapeLiteral(), PQescapeIdentifier() and that indeed finds the issue
pointed out in this thread:
../../../../../home/andres/src/postgresql/src/bin/pg_basebackup/pg_createsubscriber.c: In function 'create_logical_replication_slot':
../../../../../home/andres/src/postgresql/src/bin/pg_basebackup/pg_createsubscriber.c:1332:9: warning: 'pg_free' called on pointer returned from a mismatched allocation function [-Wmismatched-dealloc]
1332 | pg_free(slot_name_esc);
| ^~~~~~~~~~~~~~~~~~~~~~
../../../../../home/andres/src/postgresql/src/bin/pg_basebackup/pg_createsubscriber.c:1326:25: note: returned from 'PQescapeLiteral'
1326 | slot_name_esc = PQescapeLiteral(conn, slot_name, strlen(slot_name));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...
but also finds one more:
[62/214 42 28%] Compiling C object src/bin/pg_amcheck/pg_amcheck.p/pg_amcheck.c.o
../../../../../home/andres/src/postgresql/src/bin/pg_amcheck/pg_amcheck.c: In function 'main':
../../../../../home/andres/src/postgresql/src/bin/pg_amcheck/pg_amcheck.c:563:25: warning: 'pfree' called on pointer returned from a mismatched allocation function [-Wmismatched-dealloc]
563 | pfree(schema);
| ^~~~~~~~~~~~~
../../../../../home/andres/src/postgresql/src/bin/pg_amcheck/pg_amcheck.c:556:34: note: returned from 'PQescapeIdentifier'
556 | schema = PQescapeIdentifier(conn, opts.install_schema,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
557 | strlen(opts.install_schema));
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Note that that doesn't just require adding the attributes to
PQescapeIdentifier() etc, but also to pg_malloc(), as the latter is how gcc
knows that pg_pfree() is a deallocating function.
Particularly for something like libpq it's not quitetrivial to add
attributes like this, of course. We can't even depend on pg_config.h.
One way would be to define them in libpq-fe.h, guarded by an #ifdef, that's
"armed" by a commandline -D flag, if the compiler is supported?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
@ 2025-02-12 17:17 ` Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 17:23 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
1 sibling, 1 reply; 44+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2025-02-12 17:17 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
Andres Freund <[email protected]> writes:
> Hi,
>
> On 2025-02-12 11:02:04 -0500, Tom Lane wrote:
>> I wish we had some way to detect misuses automatically ...
>>
>> This seems like the sort of bug that Coverity could detect if only it
>> knew to look, but I have no idea if it could be configured that way.
>> Maybe some weird lashup with a debugging malloc library would be
>> another way.
>
> Recent gcc actually has a fairly good way to detect this kind of issue:
> https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-malloc-function-attribute
> in particular, the variant of the attribute with "deallocator".
[...]
> Note that that doesn't just require adding the attributes to
> PQescapeIdentifier() etc, but also to pg_malloc(), as the latter is how gcc
> knows that pg_pfree() is a deallocating function.
>
>
> Particularly for something like libpq it's not quitetrivial to add
> attributes like this, of course. We can't even depend on pg_config.h.
>
> One way would be to define them in libpq-fe.h, guarded by an #ifdef, that's
> "armed" by a commandline -D flag, if the compiler is supported?
Does it need a -D flag, wouldn't __has_attribute(malloc) (with the
fallback definition in c.h) be enough?
- ilmari
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
2025-02-12 17:17 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
@ 2025-02-12 17:23 ` Tom Lane <[email protected]>
2025-02-12 17:29 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
0 siblings, 1 reply; 44+ messages in thread
From: Tom Lane @ 2025-02-12 17:23 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <[email protected]> writes:
> Andres Freund <[email protected]> writes:
>> Particularly for something like libpq it's not quitetrivial to add
>> attributes like this, of course. We can't even depend on pg_config.h.
>> One way would be to define them in libpq-fe.h, guarded by an #ifdef, that's
>> "armed" by a commandline -D flag, if the compiler is supported?
> Does it need a -D flag, wouldn't __has_attribute(malloc) (with the
> fallback definition in c.h) be enough?
libpq-fe.h has to be compilable by application code that has never
heard of pg_config.h let alone c.h, so we'd have to tread carefully
about not breaking that property. But it seems like this would be
worth looking into.
regards, tom lane
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
2025-02-12 17:17 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 17:23 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
@ 2025-02-12 17:29 ` Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 17:38 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
0 siblings, 1 reply; 44+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2025-02-12 17:29 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
Tom Lane <[email protected]> writes:
> =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <[email protected]> writes:
>> Andres Freund <[email protected]> writes:
>>> Particularly for something like libpq it's not quitetrivial to add
>>> attributes like this, of course. We can't even depend on pg_config.h.
>>> One way would be to define them in libpq-fe.h, guarded by an #ifdef, that's
>>> "armed" by a commandline -D flag, if the compiler is supported?
>
>> Does it need a -D flag, wouldn't __has_attribute(malloc) (with the
>> fallback definition in c.h) be enough?
>
> libpq-fe.h has to be compilable by application code that has never
> heard of pg_config.h let alone c.h, so we'd have to tread carefully
> about not breaking that property. But it seems like this would be
> worth looking into.
The fallback code isn't exactly complicated, so we could just duplicate
it in libpq-fe.h:
#ifndef __has_attribute
#define __has_attribute(attribute) 0
#endif
And then do something like this:
#if __has_attribute (malloc)
#define pg_attribute_malloc __attribute__((malloc))
#define pg_attribute_deallocator(...) __attribute__((malloc(__VA_ARGS__)))
#else
#define pg_attribute_malloc
#define pg_attribute_deallocator(...)
#endif
> regards, tom lane
- ilmari
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
2025-02-12 17:17 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 17:23 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:29 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
@ 2025-02-12 17:38 ` Tom Lane <[email protected]>
2025-02-12 18:35 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 18:49 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
0 siblings, 2 replies; 44+ messages in thread
From: Tom Lane @ 2025-02-12 17:38 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <[email protected]> writes:
> Tom Lane <[email protected]> writes:
>> libpq-fe.h has to be compilable by application code that has never
>> heard of pg_config.h let alone c.h, so we'd have to tread carefully
>> about not breaking that property. But it seems like this would be
>> worth looking into.
> The fallback code isn't exactly complicated, so we could just duplicate
> it in libpq-fe.h:
> #ifndef __has_attribute
> #define __has_attribute(attribute) 0
> #endif
The problem with that approach is the likelihood of stomping on
symbols that a calling application will use later. I think we
really need a controlling #ifdef check on some PG_FOO symbol
that we can be sure no outside application will have defined.
Roughly speaking,
#ifdef PG_USE_ALLOCATOR_CHECKS
#define pg_attribute_malloc __attribute__((malloc))
...
#else
#define pg_attribute_malloc
...
#endif
and then we could make definition of PG_USE_ALLOCATOR_CHECKS
be conditional on having the right compiler behavior, rather
than trusting that a nest of #ifdef checks is sufficient to
detect that.
regards, tom lane
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
2025-02-12 17:17 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 17:23 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:29 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 17:38 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
@ 2025-02-12 18:35 ` Tom Lane <[email protected]>
2025-02-12 23:59 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
1 sibling, 1 reply; 44+ messages in thread
From: Tom Lane @ 2025-02-12 18:35 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
I experimented with the other approach: hack libpq.so to depend on
dmalloc, leaving the rest of the system alone, so that libpq's
allocations could not be freed elsewhere nor vice versa.
It could not even get through initdb, crashing here:
replace_guc_value(char **lines, const char *guc_name, const char *guc_value,
bool mark_as_comment)
{
PQExpBuffer newline = createPQExpBuffer();
...
free(newline); /* but don't free newline->data */
which upon investigation is expected, because createPQExpBuffer is
exported by libpq and therefore is returning space allocated within
the shlib. Diking out that particular free() call just allows it
to crash a bit later on, because initdb is totally full of direct
manipulations of PQExpBuffer contents.
This indicates to me that we have a *far* larger problem than
we thought, if we'd like to be totally clean on this point.
Realistically, it's not going to happen that way; it's just
not worth the trouble and notational mess. I think if we're
honest we should just document that such-and-such combinations
of libpq and our client programs will work on Windows.
For amusement's sake, totally dirty hack-and-slash patch attached.
(I tested this on macOS, with dmalloc from MacPorts; adjust SHLIB_LINK
to suit on other platforms.)
regards, tom lane
Attachments:
[text/x-diff] use-dmalloc-within-libpq-hack-hack-hack.patch (2.5K, ../../[email protected]/2-use-dmalloc-within-libpq-hack-hack-hack.patch)
download | inline diff:
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index 701810a272a..91dc2e14d99 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -81,7 +81,7 @@ endif
# that are built correctly for use in a shlib.
SHLIB_LINK_INTERNAL = -lpgcommon_shlib -lpgport_shlib
ifneq ($(PORTNAME), win32)
-SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket -lnsl -lresolv -lintl -lm, $(LIBS)) $(LDAP_LIBS_FE) $(PTHREAD_LIBS)
+SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket -lnsl -lresolv -lintl -lm, $(LIBS)) $(LDAP_LIBS_FE) -L/opt/local/lib -ldmalloc $(PTHREAD_LIBS)
else
SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi32 -lssl -lsocket -lnsl -lresolv -lintl -lm $(PTHREAD_LIBS), $(LIBS)) $(LDAP_LIBS_FE)
endif
@@ -116,11 +116,6 @@ backend_src = $(top_srcdir)/src/backend
# coding rule.
libpq-refs-stamp: $(shlib)
ifneq ($(enable_coverage), yes)
-ifeq (,$(filter solaris,$(PORTNAME)))
- @if nm -A -u $< 2>/dev/null | grep -v -e __cxa_atexit -e __tsan_func_exit | grep exit; then \
- echo 'libpq must not be calling any function which invokes exit'; exit 1; \
- fi
-endif
endif
touch $@
diff --git a/src/interfaces/libpq/fe-auth-sasl.h b/src/interfaces/libpq/fe-auth-sasl.h
index f06f547c07d..1abafe8d34b 100644
--- a/src/interfaces/libpq/fe-auth-sasl.h
+++ b/src/interfaces/libpq/fe-auth-sasl.h
@@ -146,7 +146,7 @@ typedef struct pg_fe_sasl_mech
* state: The opaque mechanism state returned by init()
*--------
*/
- void (*free) (void *state);
+ void (*saslfree) (void *state);
} pg_fe_sasl_mech;
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 85d1ca2864f..56ff7bc3b9d 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -573,7 +573,7 @@ pqDropConnection(PGconn *conn, bool flushInput)
#endif
if (conn->sasl_state)
{
- conn->sasl->free(conn->sasl_state);
+ conn->sasl->saslfree(conn->sasl_state);
conn->sasl_state = NULL;
}
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 2546f9f8a50..7296d00b34f 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -20,6 +20,8 @@
#ifndef LIBPQ_INT_H
#define LIBPQ_INT_H
+#include <dmalloc.h>
+
/* We assume libpq-fe.h has already been included. */
#include "libpq-events.h"
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
2025-02-12 17:17 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 17:23 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:29 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 17:38 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 18:35 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
@ 2025-02-12 23:59 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Michael Paquier @ 2025-02-12 23:59 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
On Wed, Feb 12, 2025 at 01:35:20PM -0500, Tom Lane wrote:
> For amusement's sake, totally dirty hack-and-slash patch attached.
> (I tested this on macOS, with dmalloc from MacPorts; adjust SHLIB_LINK
> to suit on other platforms.)
Ugh. Fun one.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
2025-02-12 17:17 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 17:23 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:29 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 17:38 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
@ 2025-02-12 18:49 ` Dagfinn Ilmari Mannsåker <[email protected]>
1 sibling, 0 replies; 44+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2025-02-12 18:49 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
Tom Lane <[email protected]> writes:
> =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <[email protected]> writes:
>> Tom Lane <[email protected]> writes:
>>> libpq-fe.h has to be compilable by application code that has never
>>> heard of pg_config.h let alone c.h, so we'd have to tread carefully
>>> about not breaking that property. But it seems like this would be
>>> worth looking into.
>
>> The fallback code isn't exactly complicated, so we could just duplicate
>> it in libpq-fe.h:
>
>> #ifndef __has_attribute
>> #define __has_attribute(attribute) 0
>> #endif
>
> The problem with that approach is the likelihood of stomping on
> symbols that a calling application will use later. I think we
> really need a controlling #ifdef check on some PG_FOO symbol
> that we can be sure no outside application will have defined.
> Roughly speaking,
>
> #ifdef PG_USE_ALLOCATOR_CHECKS
As long as we're not checking for too many attributes, we could avoid
introducing the __has_attribute symbol by doing:
#if defined(__has_attribute) && __has_attribute(malloc)
> #define pg_attribute_malloc __attribute__((malloc))
> ...
> #else
> #define pg_attribute_malloc
> ...
> #endif
>
> and then we could make definition of PG_USE_ALLOCATOR_CHECKS
> be conditional on having the right compiler behavior, rather
> than trusting that a nest of #ifdef checks is sufficient to
> detect that.
But I just looked at the clang attribute docs
(https://clang.llvm.org/docs/AttributeReference.html#malloc), and it
only has the argumentless version, not the one that that declares the
matching deallocator, so you're right we need a more comprehensive
check.
> regards, tom lane
- ilmari
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
@ 2025-02-12 23:56 ` Michael Paquier <[email protected]>
2025-02-13 00:08 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
1 sibling, 1 reply; 44+ messages in thread
From: Michael Paquier @ 2025-02-12 23:56 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
On Wed, Feb 12, 2025 at 12:00:03PM -0500, Andres Freund wrote:
> Particularly for something like libpq it's not quitetrivial to add
> attributes like this, of course. We can't even depend on pg_config.h.
>
> One way would be to define them in libpq-fe.h, guarded by an #ifdef, that's
> "armed" by a commandline -D flag, if the compiler is supported?
Interesting set of tricks.
I have looked at bit at the uses of PQescapeLiteral() and
PQescapeIdentifier() in the tree. On top of the one in pg_amcheck you
are just pointing to, there is an inconsistency in pg_upgrade.c for
set_locale_and_encoding() where datlocale_literal may be allocated
with a pg_strdup() or a PQescapeLiteral() depending on the path. The
code has been using PQfreemem() for the pg_strdup() allocation, which
is logically incorrect.
The pg_upgrade path is fancy as designed this way, for sure, but
that's less invasive than hardcoding three times "NULL". Any thoughts
about backpatching something like that for both of them?
--
Michael
Attachments:
[text/x-diff] libpq-escape-fixes.patch (847B, ../../[email protected]/2-libpq-escape-fixes.patch)
download | inline diff:
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index c5ec25be22b..dda8b1b8ade 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -560,7 +560,7 @@ main(int argc, char *argv[])
executeCommand(conn, install_sql, opts.echo);
pfree(install_sql);
- pfree(schema);
+ PQfreemem(schema);
}
/*
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 36c7f3879d5..d854ad110ee 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -465,7 +465,10 @@ set_locale_and_encoding(void)
PQfreemem(datcollate_literal);
PQfreemem(datctype_literal);
- PQfreemem(datlocale_literal);
+ if (locale->db_locale)
+ PQfreemem(datlocale_literal);
+ else
+ pg_free(datlocale_literal);
PQfinish(conn_new_template1);
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
2025-02-12 23:56 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
@ 2025-02-13 00:08 ` Tom Lane <[email protected]>
2025-02-13 04:50 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
2025-02-25 12:39 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
0 siblings, 2 replies; 44+ messages in thread
From: Tom Lane @ 2025-02-13 00:08 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
Michael Paquier <[email protected]> writes:
> I have looked at bit at the uses of PQescapeLiteral() and
> PQescapeIdentifier() in the tree. On top of the one in pg_amcheck you
> are just pointing to, there is an inconsistency in pg_upgrade.c for
> set_locale_and_encoding() where datlocale_literal may be allocated
> with a pg_strdup() or a PQescapeLiteral() depending on the path. The
> code has been using PQfreemem() for the pg_strdup() allocation, which
> is logically incorrect.
Yeah, I suspected there would be places like that. It just hasn't
mattered in practice up to now. (I have a vague recollection that
Windows used to be pickier about this, but evidently not in recent
years.)
I spent a little time earlier today seeing what I could do with the
use-dmalloc patch I posted earlier. It turns out you can get through
initdb after s/free/PQfreemem/ in just two places, and then the
backend works fine. But psql is a frickin' disaster --- there's
free's of strings made with PQExpBuffer all over its backslash-command
handling, and no easy way to clean it up. Maybe other clients will
be less of a mess, but I'm not betting on that.
regards, tom lane
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
2025-02-12 23:56 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
2025-02-13 00:08 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
@ 2025-02-13 04:50 ` Michael Paquier <[email protected]>
2025-02-25 06:02 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
1 sibling, 1 reply; 44+ messages in thread
From: Michael Paquier @ 2025-02-13 04:50 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
On Wed, Feb 12, 2025 at 07:08:31PM -0500, Tom Lane wrote:
> I spent a little time earlier today seeing what I could do with the
> use-dmalloc patch I posted earlier. It turns out you can get through
> initdb after s/free/PQfreemem/ in just two places, and then the
> backend works fine. But psql is a frickin' disaster --- there's
> free's of strings made with PQExpBuffer all over its backslash-command
> handling, and no easy way to clean it up. Maybe other clients will
> be less of a mess, but I'm not betting on that.
Hmm. Okay. It sounds like it would be better to group everything
that has been pointed together towards what should be a more generic
solution than what I have posted. So I am holding on touching
anything.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
2025-02-12 23:56 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
2025-02-13 00:08 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-13 04:50 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
@ 2025-02-25 06:02 ` Michael Paquier <[email protected]>
2025-02-25 11:54 ` Re: Small memory fixes for pg_createsubcriber Ranier Vilela <[email protected]>
0 siblings, 1 reply; 44+ messages in thread
From: Michael Paquier @ 2025-02-25 06:02 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
On Thu, Feb 13, 2025 at 01:50:29PM +0900, Michael Paquier wrote:
> On Wed, Feb 12, 2025 at 07:08:31PM -0500, Tom Lane wrote:
>> I spent a little time earlier today seeing what I could do with the
>> use-dmalloc patch I posted earlier. It turns out you can get through
>> initdb after s/free/PQfreemem/ in just two places, and then the
>> backend works fine. But psql is a frickin' disaster --- there's
>> free's of strings made with PQExpBuffer all over its backslash-command
>> handling, and no easy way to clean it up. Maybe other clients will
>> be less of a mess, but I'm not betting on that.
>
> Hmm. Okay. It sounds like it would be better to group everything
> that has been pointed together towards what should be a more generic
> solution than what I have posted. So I am holding on touching
> anything.
Two weeks later. Would there be any objections for doing something in
the lines of [1] for pg_upgrade and pg_amcheck? The pg_upgrade bit is
a bit strange, for sure, still it's better than the current state of
things.
[1]: https://www.postgresql.org/message-id/[email protected]
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
2025-02-12 23:56 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
2025-02-13 00:08 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-13 04:50 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
2025-02-25 06:02 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
@ 2025-02-25 11:54 ` Ranier Vilela <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Ranier Vilela @ 2025-02-25 11:54 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Euler Taveira <[email protected]>; pgsql-hackers
Em ter., 25 de fev. de 2025 às 03:02, Michael Paquier <[email protected]>
escreveu:
> On Thu, Feb 13, 2025 at 01:50:29PM +0900, Michael Paquier wrote:
> > On Wed, Feb 12, 2025 at 07:08:31PM -0500, Tom Lane wrote:
> >> I spent a little time earlier today seeing what I could do with the
> >> use-dmalloc patch I posted earlier. It turns out you can get through
> >> initdb after s/free/PQfreemem/ in just two places, and then the
> >> backend works fine. But psql is a frickin' disaster --- there's
> >> free's of strings made with PQExpBuffer all over its backslash-command
> >> handling, and no easy way to clean it up. Maybe other clients will
> >> be less of a mess, but I'm not betting on that.
> >
> > Hmm. Okay. It sounds like it would be better to group everything
> > that has been pointed together towards what should be a more generic
> > solution than what I have posted. So I am holding on touching
> > anything.
>
> Two weeks later. Would there be any objections for doing something in
> the lines of [1] for pg_upgrade and pg_amcheck?
I prefer not to mix api.
We already have a branch for decision.
Let's use it then.
diff --git a/src/bin/pg_upgrade/pg_upgrade.c
b/src/bin/pg_upgrade/pg_upgrade.c
index d95c491fb5..b2c8e8e420 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -455,7 +455,9 @@ set_locale_and_encoding(void)
locale->db_locale,
strlen(locale->db_locale));
else
- datlocale_literal = pg_strdup("NULL");
+ datlocale_literal = PQescapeLiteral(conn_new_template1,
+ "NULL",
+ strlen("NULL"));
best regards,
Ranier Vilela
Attachments:
[application/octet-stream] avoid-mix-api-pg_upgrade.patch (571B, ../../CAEudQAqsjfhfJYp8CWTJk3u-7h4C2LL38StJgcrFvcnc6+cOCA@mail.gmail.com/3-avoid-mix-api-pg_upgrade.patch)
download | inline diff:
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index d95c491fb5..b2c8e8e420 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -455,7 +455,9 @@ set_locale_and_encoding(void)
locale->db_locale,
strlen(locale->db_locale));
else
- datlocale_literal = pg_strdup("NULL");
+ datlocale_literal = PQescapeLiteral(conn_new_template1,
+ "NULL",
+ strlen("NULL"));
/* update template0 in new cluster */
if (GET_MAJOR_VERSION(new_cluster.major_version) >= 1700)
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Small memory fixes for pg_createsubcriber
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
2025-02-12 23:56 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
2025-02-13 00:08 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
@ 2025-02-25 12:39 ` Andres Freund <[email protected]>
1 sibling, 0 replies; 44+ messages in thread
From: Andres Freund @ 2025-02-25 12:39 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
Hi,
On 2025-02-12 19:08:31 -0500, Tom Lane wrote:
> Michael Paquier <[email protected]> writes:
> > I have looked at bit at the uses of PQescapeLiteral() and
> > PQescapeIdentifier() in the tree. On top of the one in pg_amcheck you
> > are just pointing to, there is an inconsistency in pg_upgrade.c for
> > set_locale_and_encoding() where datlocale_literal may be allocated
> > with a pg_strdup() or a PQescapeLiteral() depending on the path. The
> > code has been using PQfreemem() for the pg_strdup() allocation, which
> > is logically incorrect.
>
> Yeah, I suspected there would be places like that. It just hasn't
> mattered in practice up to now. (I have a vague recollection that
> Windows used to be pickier about this, but evidently not in recent
> years.)
It's a question of compiler / link options. If you e.g. have a debug psql
runtime linked against a non-debug libpq, the allocators for both will
differ. Or if you link an application statically while a library is built for
a shared C runtime. Or a few other such things.
But when building in the BF / CI we'll not typically encounter this issue
between libraries and binaries in our source tree, because they'll all be
built the same.
However, we have more recently hit this with external libraries we link
to. While it's not recommended, we commonly build (in BF/CI) a debug postgres
against production openssl, zstd etc. Which means they don't share
allocators, file descriptors etc.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
@ 2025-06-17 15:37 Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 14:47 ` Re: Add CASEFOLD() function. Peter Eisentraut <[email protected]>
0 siblings, 2 replies; 44+ messages in thread
From: Vik Fearing @ 2025-06-17 15:37 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; +Cc: pgsql-hackers; Peter Eisentraut <[email protected]>
On 16/12/2024 18:49, Jeff Davis wrote:
> One question I have is whether we want this function to normalize the
> output.
Yes, we do.
I am sorry that I am so late to the party, but I am currently writing
the Change Proposal for the SQL Standard for this function.
For <fold> (which includes LOWER() and UPPER()), the text says in
Section 6.35 GR 7.e:
If the character set of <character factor> is UTF8, UTF16, or UTF32,
then FR is replaced by
Case:
i) If the <search condition> S IS NORMALIZED evaluates to True,
then NORMALIZE (FR)
ii) Otherwise, FR.
Here, FR is the result of the function and S is its argument.
It does not appear to me that our LOWER and UPPER functions obey this
rule, so there is a valid argument that we should continue to ignore it.
Or, we can say that we have at least one of three compliant.
--
Vik Fearing
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
@ 2025-06-17 18:14 ` Jeff Davis <[email protected]>
2025-06-18 17:09 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
1 sibling, 1 reply; 44+ messages in thread
From: Jeff Davis @ 2025-06-17 18:14 UTC (permalink / raw)
To: Vik Fearing <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; +Cc: pgsql-hackers; Peter Eisentraut <[email protected]>
On Tue, 2025-06-17 at 17:37 +0200, Vik Fearing wrote:
> If the character set of <character factor> is UTF8, UTF16, or UTF32,
> then FR is replaced by
> Case:
> i) If the <search condition> S IS NORMALIZED evaluates to
> True, then NORMALIZE (FR)
> ii) Otherwise, FR.
I read that as "if the input is normalized, then the output should be
normalized", IOW preserve the normalization. But does it mean "preserve
whatever the input normal form is" or "preserve NFC if the input is
NFC, otherwise the normalization is undefined"?
The above wording seems to mean "preserve NFC if the input is NFC",
because that's what NORMALIZE(FR) does when the normal form is
unspecified.
> It does not appear to me that our LOWER and UPPER functions obey this
> rule,
You are correct:
WITH s(t) AS
(SELECT NORMALIZE(U&'\00C1\00DF\0301' COLLATE "en-US-x-icu"))
SELECT UPPER(t) = NORMALIZE(UPPER(t)) FROM s;
?column?
----------
f
> so there is a valid argument that we should continue to ignore it.
> Or, we can say that we have at least one of three compliant.
What do other databases do?
Given how costly normalization can be, imposing that on every caller
seems like a bit much. And favoring NFC for the user unconditionally
might not be the best thing. Then again, NFC is good most of the time,
and there are patches to speed up normalization.
I tend to think that a lot of users who want casefolding would also
want normalization, but it's hard to weigh that against the performance
cost. It might not matter outside of a few edge cases, though I'm not
sure exactly how many.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
@ 2025-06-18 17:09 ` Vik Fearing <[email protected]>
2025-06-19 02:53 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
0 siblings, 1 reply; 44+ messages in thread
From: Vik Fearing @ 2025-06-18 17:09 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; +Cc: pgsql-hackers; Peter Eisentraut <[email protected]>
On 17/06/2025 20:14, Jeff Davis wrote:
> On Tue, 2025-06-17 at 17:37 +0200, Vik Fearing wrote:
>> If the character set of <character factor> is UTF8, UTF16, or UTF32,
>> then FR is replaced by
>> Case:
>> i) If the <search condition> S IS NORMALIZED evaluates to
>> True, then NORMALIZE (FR)
>> ii) Otherwise, FR.
> I read that as "if the input is normalized, then the output should be
> normalized", IOW preserve the normalization. But does it mean "preserve
> whatever the input normal form is" or "preserve NFC if the input is
> NFC, otherwise the normalization is undefined"?
>
> The above wording seems to mean "preserve NFC if the input is NFC",
> because that's what NORMALIZE(FR) does when the normal form is
> unspecified.
Yes, and that is also the default for <normalized predicate>.
>> It does not appear to me that our LOWER and UPPER functions obey this
>> rule,
> You are correct:
>
> WITH s(t) AS
> (SELECT NORMALIZE(U&'\00C1\00DF\0301' COLLATE "en-US-x-icu"))
> SELECT UPPER(t) = NORMALIZE(UPPER(t)) FROM s;
> ?column?
> ----------
> f
>
>> so there is a valid argument that we should continue to ignore it.
>> Or, we can say that we have at least one of three compliant.
> What do other databases do?
I don't know. I am just pointing out what the Standard says. I think
we should either comply, or say that we don't do it for LOWER and UPPER
so let's keep things implementation-consistent.
> Given how costly normalization can be, imposing that on every caller
> seems like a bit much.
How much does it cost to check for NFC? I honestly don't know the
answer to that question, but that is the only case where we need to
maintain normalization.
> And favoring NFC for the user unconditionally
> might not be the best thing. Then again, NFC is good most of the time,
> and there are patches to speed up normalization.
It's not unconditionally, it's only if the input was NFC.
> I tend to think that a lot of users who want casefolding would also
> want normalization, but it's hard to weigh that against the performance
> cost. It might not matter outside of a few edge cases, though I'm not
> sure exactly how many.
I defer to you and others in the thread to make this decision.
--
Vik Fearing
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-18 17:09 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
@ 2025-06-19 02:53 ` Jeff Davis <[email protected]>
2025-06-19 04:03 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
0 siblings, 1 reply; 44+ messages in thread
From: Jeff Davis @ 2025-06-19 02:53 UTC (permalink / raw)
To: Vik Fearing <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; +Cc: pgsql-hackers; Peter Eisentraut <[email protected]>
On Wed, 2025-06-18 at 19:09 +0200, Vik Fearing wrote:
> I don't know. I am just pointing out what the Standard says. I
> think
> we should either comply, or say that we don't do it for LOWER and
> UPPER
> so let's keep things implementation-consistent.
For the standard, I see two potential philosophies:
I. CASEFOLD() is another variant of LOWER()/UPPER(), and it should
preserve NFC in the same way.
II. CASEFOLD() is not like LOWER()/UPPER(); it returns a semi-opaque
text value that is useful for caseless matching, but should not
ordinarily be used for display or sent to the application (those things
would be allowed, just not encouraged). For normalization, either:
(A) Follow Unicode Default Caseless Matching (16.0 3.13.5 D144), and
don't require any kind of normalization; or
(B) Follow Unicode Canonical Caseless Matching (D145), and require
that the input and output are normalized appropriately, but leave the
precise normal form as implementation-defined.
The current implementation could either be seen as philosophy (I) where
we've chosen to ignore the normalization part for the sake of
consistency with LOWER()/UPPER(); or it could be seen as philosophy
(II)(A).
> How much does it cost to check for NFC? I honestly don't know the
> answer to that question, but that is the only case where we need to
> maintain normalization.
I attached a very rough patch and ran a very simple test on strings
averaging 36 bytes in length, all already in NFC and the result is also
NFC. Before the patch, doing a CASEFOLD() on 10M tuples took about 3
seconds, afterward about 8.
There's a patch to optimize some of the normalization paths, which I
haven't had a chance to review yet. So those numbers might come down.
>
> It's not unconditionally, it's only if the input was NFC.
Optimizing the case where the input is _not_ NFC seems strange to me.
If we are normalizing the output, I'd say we should just make the
output always NFC. Being more strict, this seems likely to comply with
the eventual standard.
Additionally, if we are normalizing the output, then we should also do
the input fixup for U+0345, which would make the result usable for
Canonical Caseless Matching. Again, this seems likely to comply with
the eventual standard.
>
So I only see two reasonable implementations:
1. The current CASEFOLD() implementation.
2. Do the input fixup for U+0345 and unconditionally normalize the
output in NFC.
If there's a case to be made for both implementations, we could also
consider having two functions, say, CASEFOLD() for #1 and NCASEFOLD()
for #2. I'm not sure whether we'd want to standardize one or both of
those functions.
And if you think there's likely to be a collision with the standard
that's hard to anticipate and fix now, then we should consider
reverting CASEFOLD() for 18 and wait for more progress on the
standardization. What's the likelihood that the name changes or
something like that?
Regards,
Jeff Davis
Attachments:
[text/x-patch] casefold_normalize.diff (1.9K, ../../[email protected]/2-casefold_normalize.diff)
download | inline diff:
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index 5bd1e01f7e4..12e688acec6 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -79,6 +79,7 @@
#include "common/int.h"
#include "common/unicode_case.h"
#include "common/unicode_category.h"
+#include "common/unicode_norm.h"
#include "mb/pg_wchar.h"
#include "nodes/miscnodes.h"
#include "parser/scansup.h"
@@ -1866,6 +1867,9 @@ str_casefold(const char *buff, size_t nbytes, Oid collid)
size_t dstsize;
char *dst;
size_t needed;
+ int mblen, i;
+ unsigned char *p;
+ pg_wchar *decoded;
/* first try buffer of equal size plus terminating NUL */
dstsize = srclen + 1;
@@ -1882,7 +1886,54 @@ str_casefold(const char *buff, size_t nbytes, Oid collid)
}
Assert(dst[needed] == '\0');
- result = dst;
+
+ /* convert to pg_wchar */
+ mblen = pg_mbstrlen_with_len(dst, needed);
+ decoded = palloc((mblen + 1) * sizeof(pg_wchar));
+ p = (unsigned char *) dst;
+ for (i = 0; i < mblen; i++)
+ {
+ decoded[i] = utf8_to_unicode(p);
+ p += pg_utf_mblen(p);
+ }
+ decoded[i] = (pg_wchar) '\0';
+
+ if (unicode_is_normalized_quickcheck(UNICODE_NFC, decoded) == UNICODE_NORM_QC_YES)
+ {
+ pfree(decoded);
+ result = dst;
+ }
+ else
+ {
+ pg_wchar *normalized;
+ unsigned char *normalized_utf8;
+
+ normalized = unicode_normalize(UNICODE_NFC, decoded);
+ pfree(decoded);
+
+ /* convert back to UTF-8 string */
+ mblen = 0;
+ for (pg_wchar *wp = normalized; *wp; wp++)
+ {
+ unsigned char buf[4];
+
+ unicode_to_utf8(*wp, buf);
+ mblen += pg_utf_mblen(buf);
+ }
+
+ normalized_utf8 = palloc(mblen + 1);
+
+ p = normalized_utf8;
+ for (pg_wchar *wp = normalized; *wp; wp++)
+ {
+ unicode_to_utf8(*wp, p);
+ p += pg_utf_mblen(p);
+ }
+ *p = '\0';
+ pfree(normalized);
+
+ result = (char *) normalized_utf8;
+ }
}
return result;
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-18 17:09 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 02:53 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
@ 2025-06-19 04:03 ` Thom Brown <[email protected]>
2025-06-19 04:26 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 14:51 ` Re: Add CASEFOLD() function. Peter Eisentraut <[email protected]>
0 siblings, 2 replies; 44+ messages in thread
From: Thom Brown @ 2025-06-19 04:03 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Vik Fearing <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; pgsql-hackers; Peter Eisentraut <[email protected]>
On Thu, 19 Jun 2025, 03:53 Jeff Davis, <[email protected]> wrote:
> On Wed, 2025-06-18 at 19:09 +0200, Vik Fearing wrote:
> > I don't know. I am just pointing out what the Standard says. I
> > think
> > we should either comply, or say that we don't do it for LOWER and
> > UPPER
> > so let's keep things implementation-consistent.
>
> For the standard, I see two potential philosophies:
>
> I. CASEFOLD() is another variant of LOWER()/UPPER(), and it should
> preserve NFC in the same way.
>
> II. CASEFOLD() is not like LOWER()/UPPER(); it returns a semi-opaque
> text value that is useful for caseless matching, but should not
> ordinarily be used for display or sent to the application (those things
> would be allowed, just not encouraged). For normalization, either:
> (A) Follow Unicode Default Caseless Matching (16.0 3.13.5 D144), and
> don't require any kind of normalization; or
> (B) Follow Unicode Canonical Caseless Matching (D145), and require
> that the input and output are normalized appropriately, but leave the
> precise normal form as implementation-defined.
>
>
> The current implementation could either be seen as philosophy (I) where
> we've chosen to ignore the normalization part for the sake of
> consistency with LOWER()/UPPER(); or it could be seen as philosophy
> (II)(A).
>
> > How much does it cost to check for NFC? I honestly don't know the
> > answer to that question, but that is the only case where we need to
> > maintain normalization.
>
> I attached a very rough patch and ran a very simple test on strings
> averaging 36 bytes in length, all already in NFC and the result is also
> NFC. Before the patch, doing a CASEFOLD() on 10M tuples took about 3
> seconds, afterward about 8.
>
> There's a patch to optimize some of the normalization paths, which I
> haven't had a chance to review yet. So those numbers might come down.
>
> >
> > It's not unconditionally, it's only if the input was NFC.
>
> Optimizing the case where the input is _not_ NFC seems strange to me.
> If we are normalizing the output, I'd say we should just make the
> output always NFC. Being more strict, this seems likely to comply with
> the eventual standard.
>
> Additionally, if we are normalizing the output, then we should also do
> the input fixup for U+0345, which would make the result usable for
> Canonical Caseless Matching. Again, this seems likely to comply with
> the eventual standard.
>
> >
>
> So I only see two reasonable implementations:
>
> 1. The current CASEFOLD() implementation.
>
> 2. Do the input fixup for U+0345 and unconditionally normalize the
> output in NFC.
>
> If there's a case to be made for both implementations, we could also
> consider having two functions, say, CASEFOLD() for #1 and NCASEFOLD()
> for #2. I'm not sure whether we'd want to standardize one or both of
> those functions.
>
> And if you think there's likely to be a collision with the standard
> that's hard to anticipate and fix now, then we should consider
> reverting CASEFOLD() for 18 and wait for more progress on the
> standardization. What's the likelihood that the name changes or
> something like that?
>
Late to the party, but is there an argument for porting this to the citext
type? Or supplementing the extension with an additional type ("cftext"?
*shrug*). It currently uses lower(), so our current recommendation for
dealing with all unicode characters is to use nondeterministic collations.
Thom
>
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-18 17:09 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 02:53 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 04:03 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
@ 2025-06-19 04:26 ` Jeff Davis <[email protected]>
1 sibling, 0 replies; 44+ messages in thread
From: Jeff Davis @ 2025-06-19 04:26 UTC (permalink / raw)
To: Thom Brown <[email protected]>; +Cc: Vik Fearing <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; pgsql-hackers; Peter Eisentraut <[email protected]>
On Thu, 2025-06-19 at 05:03 +0100, Thom Brown wrote:
> Late to the party, but is there an argument for porting this to the
> citext type? Or supplementing the extension with an additional type
> ("cftext"? *shrug*).
CASEFOLD() addresses a lot of the problems with using LOWER(), so that
sounds like a good idea. I'd be interested to hear from users of
citext.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-18 17:09 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 02:53 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 04:03 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
@ 2025-06-19 14:51 ` Peter Eisentraut <[email protected]>
2025-06-19 15:36 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
1 sibling, 1 reply; 44+ messages in thread
From: Peter Eisentraut @ 2025-06-19 14:51 UTC (permalink / raw)
To: Thom Brown <[email protected]>; Jeff Davis <[email protected]>; +Cc: Vik Fearing <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; pgsql-hackers
On 19.06.25 06:03, Thom Brown wrote:
> Late to the party, but is there an argument for porting this to the
> citext type? Or supplementing the extension with an additional type
> ("cftext"? *shrug*). It currently uses lower(), so our current
> recommendation for dealing with all unicode characters is to use
> nondeterministic collations.
What is the motivation for wanting a citext variant instead of using
nondeterministic collations?
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-18 17:09 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 02:53 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 04:03 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 14:51 ` Re: Add CASEFOLD() function. Peter Eisentraut <[email protected]>
@ 2025-06-19 15:36 ` Thom Brown <[email protected]>
2025-06-19 16:15 ` Re: Add CASEFOLD() function. Robert Treat <[email protected]>
2025-06-19 16:33 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
0 siblings, 2 replies; 44+ messages in thread
From: Thom Brown @ 2025-06-19 15:36 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Jeff Davis <[email protected]>; Vik Fearing <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; pgsql-hackers
On Thu, 19 Jun 2025 at 15:51, Peter Eisentraut <[email protected]> wrote:
>
> On 19.06.25 06:03, Thom Brown wrote:
> > Late to the party, but is there an argument for porting this to the
> > citext type? Or supplementing the extension with an additional type
> > ("cftext"? *shrug*). It currently uses lower(), so our current
> > recommendation for dealing with all unicode characters is to use
> > nondeterministic collations.
>
> What is the motivation for wanting a citext variant instead of using
> nondeterministic collations?
Ease of use, perhaps. It seems easier to use:
column_name cftext
rather than:
CREATE COLLATION case_insensitive_collation (
PROVIDER = icu,
LOCALE = 'und-u-ks-level2',
DETERMINISTIC = FALSE
);
column_name text COLLATE case_insensitive_collation
But I see the arguments against it. It creates an unnecessary
dependency on an extension, and if someone wants to ignore both case
and accents, they may resort to using 2 extensions (citext + unaccent)
when none are needed. I guess I don't feel strongly about it either
way.
Thom
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-18 17:09 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 02:53 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 04:03 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 14:51 ` Re: Add CASEFOLD() function. Peter Eisentraut <[email protected]>
2025-06-19 15:36 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
@ 2025-06-19 16:15 ` Robert Treat <[email protected]>
1 sibling, 0 replies; 44+ messages in thread
From: Robert Treat @ 2025-06-19 16:15 UTC (permalink / raw)
To: Thom Brown <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Jeff Davis <[email protected]>; Vik Fearing <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; pgsql-hackers
On Thu, Jun 19, 2025 at 11:37 AM Thom Brown <[email protected]> wrote:
> On Thu, 19 Jun 2025 at 15:51, Peter Eisentraut <[email protected]> wrote:
> > On 19.06.25 06:03, Thom Brown wrote:
> > > Late to the party, but is there an argument for porting this to the
> > > citext type? Or supplementing the extension with an additional type
> > > ("cftext"? *shrug*). It currently uses lower(), so our current
> > > recommendation for dealing with all unicode characters is to use
> > > nondeterministic collations.
> >
> > What is the motivation for wanting a citext variant instead of using
> > nondeterministic collations?
>
> Ease of use, perhaps. It seems easier to use:
>
> column_name cftext
>
> rather than:
>
> CREATE COLLATION case_insensitive_collation (
> PROVIDER = icu,
> LOCALE = 'und-u-ks-level2',
> DETERMINISTIC = FALSE
> );
>
> column_name text COLLATE case_insensitive_collation
>
> But I see the arguments against it. It creates an unnecessary
> dependency on an extension, and if someone wants to ignore both case
> and accents, they may resort to using 2 extensions (citext + unaccent)
> when none are needed. I guess I don't feel strongly about it either
> way.
Don't forget, if you have a defined insensitive / normalized
collations, you also enable on-the-fly collation based matching, a la
"SELECT 'Å' = 'A' COLLATE ignore_accent_case;" regardless of the
provided collations (which I think is much more common certain in
other databases)
Robert Treat
https://xzilla.net
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-18 17:09 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 02:53 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 04:03 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 14:51 ` Re: Add CASEFOLD() function. Peter Eisentraut <[email protected]>
2025-06-19 15:36 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
@ 2025-06-19 16:33 ` Jeff Davis <[email protected]>
2025-06-19 16:51 ` Re: Add CASEFOLD() function. Robert Treat <[email protected]>
2025-06-19 16:59 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
1 sibling, 2 replies; 44+ messages in thread
From: Jeff Davis @ 2025-06-19 16:33 UTC (permalink / raw)
To: Thom Brown <[email protected]>; Peter Eisentraut <[email protected]>; +Cc: Vik Fearing <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; pgsql-hackers
On Thu, 2025-06-19 at 16:36 +0100, Thom Brown wrote:
> Ease of use, perhaps. It seems easier to use:
>
> column_name cftext
>
> rather than:
>
> CREATE COLLATION case_insensitive_collation (
> PROVIDER = icu,
> LOCALE = 'und-u-ks-level2',
> DETERMINISTIC = FALSE
> );
We could auto-create such a collation at initdb time for ICU-enabled
builds.
> But I see the arguments against it. It creates an unnecessary
> dependency on an extension, and if someone wants to ignore both case
> and accents, they may resort to using 2 extensions (citext +
> unaccent)
> when none are needed.
There are at least three ways to do case insensitivity (or other kinds
of equivalence):
* Explicit function calls in queries, as well as index and constraint
definitions. E.g. expression index on LOWER(), queries that explicitly
do "LOWER(x) = ..."
* Wrap those function calls up in a separate data type, like citext.
* Non-deterministic collations.
Given that we have collations, which are a way of organizing alternate
behaviors for existing data types, I'm not sure I see the need for
creating an entirely separate data type.
> I guess I don't feel strongly about it either
> way.
Are you a user of citext? I'm genuinely interested in the use cases,
and whether the separate-data-type approach has merits that are missing
in the other approaches.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-18 17:09 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 02:53 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 04:03 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 14:51 ` Re: Add CASEFOLD() function. Peter Eisentraut <[email protected]>
2025-06-19 15:36 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 16:33 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
@ 2025-06-19 16:51 ` Robert Treat <[email protected]>
1 sibling, 0 replies; 44+ messages in thread
From: Robert Treat @ 2025-06-19 16:51 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Thom Brown <[email protected]>; Peter Eisentraut <[email protected]>; Vik Fearing <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; pgsql-hackers
On Thu, Jun 19, 2025 at 12:33 PM Jeff Davis <[email protected]> wrote:
>
> On Thu, 2025-06-19 at 16:36 +0100, Thom Brown wrote:
> > Ease of use, perhaps. It seems easier to use:
> >
> > column_name cftext
> >
> > rather than:
> >
> > CREATE COLLATION case_insensitive_collation (
> > PROVIDER = icu,
> > LOCALE = 'und-u-ks-level2',
> > DETERMINISTIC = FALSE
> > );
>
> We could auto-create such a collation at initdb time for ICU-enabled
> builds.
>
Providing a generic insensitive/non-deterministic collation by default
would solve a number of different use cases, so +1 on the idea from
me.
And TBH I usually build --without-icu but this would likely cause me
to change that.
> > But I see the arguments against it. It creates an unnecessary
> > dependency on an extension, and if someone wants to ignore both case
> > and accents, they may resort to using 2 extensions (citext +
> > unaccent)
> > when none are needed.
>
> There are at least three ways to do case insensitivity (or other kinds
> of equivalence):
>
> * Explicit function calls in queries, as well as index and constraint
> definitions. E.g. expression index on LOWER(), queries that explicitly
> do "LOWER(x) = ..."
>
> * Wrap those function calls up in a separate data type, like citext.
>
> * Non-deterministic collations.
>
> Given that we have collations, which are a way of organizing alternate
> behaviors for existing data types, I'm not sure I see the need for
> creating an entirely separate data type.
>
> > I guess I don't feel strongly about it either
> > way.
>
> Are you a user of citext? I'm genuinely interested in the use cases,
> and whether the separate-data-type approach has merits that are missing
> in the other approaches.
>
Yeah, I'd be interested to hear if there is some missing bit that
existing users have concerns over; as a former user of citext, it was
a great workaround at the time, but there are "better ways" to handle
those things now (imho).
Robert Treat
https://xzilla.net
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-18 17:09 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 02:53 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 04:03 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 14:51 ` Re: Add CASEFOLD() function. Peter Eisentraut <[email protected]>
2025-06-19 15:36 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 16:33 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
@ 2025-06-19 16:59 ` Thom Brown <[email protected]>
2025-06-19 17:38 ` Re: Add CASEFOLD() function. David E. Wheeler <[email protected]>
1 sibling, 1 reply; 44+ messages in thread
From: Thom Brown @ 2025-06-19 16:59 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Vik Fearing <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; pgsql-hackers
On Thu, 19 Jun 2025, 17:33 Jeff Davis, <[email protected]> wrote:
> On Thu, 2025-06-19 at 16:36 +0100, Thom Brown wrote:
> > Ease of use, perhaps. It seems easier to use:
> >
> > column_name cftext
> >
> > rather than:
> >
> > CREATE COLLATION case_insensitive_collation (
> > PROVIDER = icu,
> > LOCALE = 'und-u-ks-level2',
> > DETERMINISTIC = FALSE
> > );
>
> We could auto-create such a collation at initdb time for ICU-enabled
> builds.
>
> > But I see the arguments against it. It creates an unnecessary
> > dependency on an extension, and if someone wants to ignore both case
> > and accents, they may resort to using 2 extensions (citext +
> > unaccent)
> > when none are needed.
>
> There are at least three ways to do case insensitivity (or other kinds
> of equivalence):
>
> * Explicit function calls in queries, as well as index and constraint
> definitions. E.g. expression index on LOWER(), queries that explicitly
> do "LOWER(x) = ..."
>
> * Wrap those function calls up in a separate data type, like citext.
>
> * Non-deterministic collations.
>
> Given that we have collations, which are a way of organizing alternate
> behaviors for existing data types, I'm not sure I see the need for
> creating an entirely separate data type.
>
> > I guess I don't feel strongly about it either
> > way.
>
> Are you a user of citext? I'm genuinely interested in the use cases,
> and whether the separate-data-type approach has merits that are missing
> in the other approaches.
>
No. But given the options, I would personally choose nondeterministic
collations now that they are available. I just wish they were more
user-friendly as I suspect the majority of people either won't know about
them, or won't know how to use them. But like you say, maybe having a set
of predefined nd-collections would help. As it stands, I'm just bringing up
the consideration of citext in case it has any value, which it doesn't
appear to. In fact it's probably even an argument to begin the process of
deprecation.
Thom
>
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-18 17:09 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 02:53 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 04:03 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 14:51 ` Re: Add CASEFOLD() function. Peter Eisentraut <[email protected]>
2025-06-19 15:36 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 16:33 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 16:59 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
@ 2025-06-19 17:38 ` David E. Wheeler <[email protected]>
2025-06-19 18:55 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
0 siblings, 1 reply; 44+ messages in thread
From: David E. Wheeler @ 2025-06-19 17:38 UTC (permalink / raw)
To: Thom Brown <[email protected]>; +Cc: Jeff Davis <[email protected]>; Peter Eisentraut <[email protected]>; Vik Fearing <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; pgsql-hackers
On Jun 19, 2025, at 12:59, Thom Brown <[email protected]> wrote:
> No. But given the options, I would personally choose nondeterministic collations now that they are available. I just wish they were more user-friendly as I suspect the majority of people either won't know about them, or won't know how to use them.
I suspect there are a lot of uses of citext for databases created before nondeterministic collations existed and people are unaware of them or unclear on the migration path from one to the other, let alone implications for any infrastructure they built around cutest (like function signatures and return values). As long as citext conteinues to be maintained there and there’s no super clear path to migrate, I’d bet good money that few would bother to switch.
Best,
David
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-18 17:09 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 02:53 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 04:03 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 14:51 ` Re: Add CASEFOLD() function. Peter Eisentraut <[email protected]>
2025-06-19 15:36 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 16:33 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 16:59 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 17:38 ` Re: Add CASEFOLD() function. David E. Wheeler <[email protected]>
@ 2025-06-19 18:55 ` Thom Brown <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Thom Brown @ 2025-06-19 18:55 UTC (permalink / raw)
To: David E. Wheeler <[email protected]>; +Cc: Jeff Davis <[email protected]>; Peter Eisentraut <[email protected]>; Vik Fearing <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; pgsql-hackers
On Thu, 19 Jun 2025 at 18:39, David E. Wheeler <[email protected]> wrote:
>
> On Jun 19, 2025, at 12:59, Thom Brown <[email protected]> wrote:
>
> > No. But given the options, I would personally choose nondeterministic collations now that they are available. I just wish they were more user-friendly as I suspect the majority of people either won't know about them, or won't know how to use them.
>
> I suspect there are a lot of uses of citext for databases created before nondeterministic collations existed and people are unaware of them or unclear on the migration path from one to the other, let alone implications for any infrastructure they built around cutest (like function signatures and return values). As long as citext conteinues to be maintained there and there’s no super clear path to migrate, I’d bet good money that few would bother to switch.
Maybe the citext doc page should explain how to get unhooked from it.
Something like:
ALTER TABLE mytable
ALTER COLUMN ci_column
SET DATA TYPE TEXT COLLATE case_insensitive_collation;
or
CREATE DOMAIN ci_text AS text
COLLATE case_insensitive_collation;
ALTER TABLE mytable
ALTER COLUMN ci_column
SET DATA TYPE ci_text;
And because they're binary-compatible, they should also be free. No
doubt a procedure could do this to every instance in the database,
although I guess it gets trickier when it comes to functions that
accept citext as a parameter type, and other similar examples.
Thom
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
@ 2025-06-19 14:47 ` Peter Eisentraut <[email protected]>
2025-06-19 16:21 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
1 sibling, 1 reply; 44+ messages in thread
From: Peter Eisentraut @ 2025-06-19 14:47 UTC (permalink / raw)
To: Vik Fearing <[email protected]>; Jeff Davis <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; +Cc: pgsql-hackers
On 17.06.25 17:37, Vik Fearing wrote:
> For <fold> (which includes LOWER() and UPPER()), the text says in
> Section 6.35 GR 7.e:
>
>
> If the character set of <character factor> is UTF8, UTF16, or UTF32,
> then FR is replaced by
> Case:
> i) If the <search condition> S IS NORMALIZED evaluates to True,
> then NORMALIZE (FR)
> ii) Otherwise, FR.
>
>
> Here, FR is the result of the function and S is its argument.
>
>
> It does not appear to me that our LOWER and UPPER functions obey this
> rule, so there is a valid argument that we should continue to ignore it.
> Or, we can say that we have at least one of three compliant.
The SQL standard also says in a few other places that normalization
should be applied, and we do none of those, so this is probably not a
reason to change CASEFOLD at this point.
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 14:47 ` Re: Add CASEFOLD() function. Peter Eisentraut <[email protected]>
@ 2025-06-19 16:21 ` Vik Fearing <[email protected]>
2025-06-19 16:52 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
0 siblings, 1 reply; 44+ messages in thread
From: Vik Fearing @ 2025-06-19 16:21 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; Jeff Davis <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; +Cc: pgsql-hackers
On 19/06/2025 16:47, Peter Eisentraut wrote:
> On 17.06.25 17:37, Vik Fearing wrote:
>> For <fold> (which includes LOWER() and UPPER()), the text says in
>> Section 6.35 GR 7.e:
>>
>>
>> If the character set of <character factor> is UTF8, UTF16, or UTF32,
>> then FR is replaced by
>> Case:
>> i) If the <search condition> S IS NORMALIZED evaluates to
>> True, then NORMALIZE (FR)
>> ii) Otherwise, FR.
>>
>>
>> Here, FR is the result of the function and S is its argument.
>>
>>
>> It does not appear to me that our LOWER and UPPER functions obey this
>> rule, so there is a valid argument that we should continue to ignore
>> it. Or, we can say that we have at least one of three compliant.
>
> The SQL standard also says in a few other places that normalization
> should be applied, and we do none of those, so this is probably not a
> reason to change CASEFOLD at this point.
>
Works for me.
--
Vik Fearing.
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add CASEFOLD() function.
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 14:47 ` Re: Add CASEFOLD() function. Peter Eisentraut <[email protected]>
2025-06-19 16:21 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
@ 2025-06-19 16:52 ` Jeff Davis <[email protected]>
0 siblings, 0 replies; 44+ messages in thread
From: Jeff Davis @ 2025-06-19 16:52 UTC (permalink / raw)
To: Vik Fearing <[email protected]>; Peter Eisentraut <[email protected]>; Joe Conway <[email protected]>; Ian Lawrence Barwick <[email protected]>; +Cc: pgsql-hackers
On Thu, 2025-06-19 at 18:21 +0200, Vik Fearing wrote:
> >
> > The SQL standard also says in a few other places that normalization
> > should be applied, and we do none of those, so this is probably not
> > a
> > reason to change CASEFOLD at this point.
> >
>
> Works for me.
Sounds good. We can document compatibility notes around this point.
If normalization becomes important, we can take the time to work out
the performance implications more carefully, and potentially introduce
an NCASEFOLD() if needed.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 44+ messages in thread
end of thread, other threads:[~2025-06-19 18:55 UTC | newest]
Thread overview: 44+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-02-06 09:14 [PATCH v5] ALTER tbl rewrite loses CLUSTER ON index Amit Langote <[email protected]>
2020-03-16 07:01 [PATCH v7 1/2] ALTER tbl rewrite loses CLUSTER ON index Amit Langote <[email protected]>
2024-12-21 06:19 [PATCH v25 8/9] Row pattern recognition patch (typedefs.list). Tatsuo Ishii <[email protected]>
2024-12-21 06:19 [PATCH v25 8/9] Row pattern recognition patch (typedefs.list). Tatsuo Ishii <[email protected]>
2024-12-21 06:19 [PATCH v25 8/9] Row pattern recognition patch (typedefs.list). Tatsuo Ishii <[email protected]>
2024-12-30 12:44 [PATCH v26 8/9] Row pattern recognition patch (typedefs.list). Tatsuo Ishii <[email protected]>
2024-12-30 12:44 [PATCH v26 8/9] Row pattern recognition patch (typedefs.list). Tatsuo Ishii <[email protected]>
2024-12-30 12:44 [PATCH v26 8/9] Row pattern recognition patch (typedefs.list). Tatsuo Ishii <[email protected]>
2024-12-30 23:53 [PATCH v27 8/9] Row pattern recognition patch (typedefs.list). Tatsuo Ishii <[email protected]>
2024-12-30 23:53 [PATCH v27 8/9] Row pattern recognition patch (typedefs.list). Tatsuo Ishii <[email protected]>
2024-12-30 23:53 [PATCH v27 8/9] Row pattern recognition patch (typedefs.list). Tatsuo Ishii <[email protected]>
2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 16:46 ` Re: Small memory fixes for pg_createsubcriber Ranier Vilela <[email protected]>
2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
2025-02-12 17:17 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 17:23 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 17:29 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 17:38 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 18:35 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-12 23:59 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
2025-02-12 18:49 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]>
2025-02-12 23:56 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
2025-02-13 00:08 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]>
2025-02-13 04:50 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
2025-02-25 06:02 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]>
2025-02-25 11:54 ` Re: Small memory fixes for pg_createsubcriber Ranier Vilela <[email protected]>
2025-02-25 12:39 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]>
2025-06-17 15:37 Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-17 18:14 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-18 17:09 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 02:53 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 04:03 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 04:26 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 14:51 ` Re: Add CASEFOLD() function. Peter Eisentraut <[email protected]>
2025-06-19 15:36 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 16:15 ` Re: Add CASEFOLD() function. Robert Treat <[email protected]>
2025-06-19 16:33 ` Re: Add CASEFOLD() function. Jeff Davis <[email protected]>
2025-06-19 16:51 ` Re: Add CASEFOLD() function. Robert Treat <[email protected]>
2025-06-19 16:59 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 17:38 ` Re: Add CASEFOLD() function. David E. Wheeler <[email protected]>
2025-06-19 18:55 ` Re: Add CASEFOLD() function. Thom Brown <[email protected]>
2025-06-19 14:47 ` Re: Add CASEFOLD() function. Peter Eisentraut <[email protected]>
2025-06-19 16:21 ` Re: Add CASEFOLD() function. Vik Fearing <[email protected]>
2025-06-19 16:52 ` Re: Add CASEFOLD() function. Jeff Davis <[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