public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 4/5] Fix a bug of insertion into an internal partition.
15+ messages / 6 participants
[nested] [flat]
* [PATCH 4/7] Fix a bug of insertion into an internal partition.
@ 2016-12-13 06:07 amit <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: amit @ 2016-12-13 06:07 UTC (permalink / raw)
Since implicit partition constraints are not inherited, an internal
partition's constraint was not being enforced when targeted directly.
So, include such constraint when setting up leaf partition result
relations for tuple-routing.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/commands/copy.c | 1 -
src/backend/commands/tablecmds.c | 1 -
src/backend/executor/execMain.c | 41 ++++++++++++++++++++++++++++--------
src/include/executor/executor.h | 1 -
src/test/regress/expected/insert.out | 6 ++++++
src/test/regress/sql/insert.sql | 5 +++++
6 files changed, 43 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index afbfb9f6e8..5aa4449e3e 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2429,7 +2429,6 @@ CopyFrom(CopyState cstate)
InitResultRelInfo(resultRelInfo,
cstate->rel,
1, /* dummy rangetable index */
- true, /* do load partition check expression */
NULL,
0);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3c08551d38..2e51124eb3 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1323,7 +1323,6 @@ ExecuteTruncate(TruncateStmt *stmt)
InitResultRelInfo(resultRelInfo,
rel,
0, /* dummy rangetable index */
- false,
NULL,
0);
resultRelInfo++;
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 6a82c18571..c991a18ce8 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -824,10 +824,10 @@ InitPlan(QueryDesc *queryDesc, int eflags)
resultRelationOid = getrelid(resultRelationIndex, rangeTable);
resultRelation = heap_open(resultRelationOid, RowExclusiveLock);
+
InitResultRelInfo(resultRelInfo,
resultRelation,
resultRelationIndex,
- true,
NULL,
estate->es_instrument);
resultRelInfo++;
@@ -1218,10 +1218,11 @@ void
InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
Relation partition_root,
int instrument_options)
{
+ List *partition_check = NIL;
+
MemSet(resultRelInfo, 0, sizeof(ResultRelInfo));
resultRelInfo->type = T_ResultRelInfo;
resultRelInfo->ri_RangeTableIndex = resultRelationIndex;
@@ -1257,14 +1258,38 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_ConstraintExprs = NULL;
resultRelInfo->ri_junkFilter = NULL;
resultRelInfo->ri_projectReturning = NULL;
- if (load_partition_check)
- resultRelInfo->ri_PartitionCheck =
- RelationGetPartitionQual(resultRelationDesc);
/*
- * The following gets set to NULL unless we are initializing leaf
- * partitions for tuple-routing.
+ * If partition_root has been specified, that means we are builiding the
+ * ResultRelationInfo for one of its leaf partitions. In that case, we
+ * need *not* initialize the leaf partition's constraint, but rather the
+ * the partition_root's (if any). We must do that explicitly like this,
+ * because implicit partition constraints are not inherited like user-
+ * defined constraints and would fail to be enforced by ExecConstraints()
+ * after a tuple is routed to a leaf partition.
*/
+ if (partition_root)
+ {
+ /*
+ * Root table itself may or may not be a partition; partition_check
+ * would be NIL in the latter case.
+ */
+ partition_check = RelationGetPartitionQual(partition_root);
+
+ /*
+ * This is not our own partition constraint, but rather an ancestor's.
+ * So any Vars in it bear the ancestor's attribute numbers. We must
+ * switch them to our own.
+ */
+ if (partition_check != NIL)
+ partition_check = map_partition_varattnos(partition_check,
+ resultRelationDesc,
+ partition_root);
+ }
+ else
+ partition_check = RelationGetPartitionQual(resultRelationDesc);
+
+ resultRelInfo->ri_PartitionCheck = partition_check;
resultRelInfo->ri_PartitionRoot = partition_root;
}
@@ -1328,7 +1353,6 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
InitResultRelInfo(rInfo,
rel,
0, /* dummy rangetable index */
- true,
NULL,
estate->es_instrument);
estate->es_trig_target_relations =
@@ -3133,7 +3157,6 @@ ExecSetupPartitionTupleRouting(Relation rel,
InitResultRelInfo(leaf_part_rri,
partrel,
1, /* dummy */
- false,
rel,
0);
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 70ecf108a3..4fed4e101f 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -189,7 +189,6 @@ extern void CheckValidResultRel(Relation resultRel, CmdType operation);
extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
Relation partition_root,
int instrument_options);
extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index b120954997..6a6aac5a88 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -339,6 +339,12 @@ alter table p add constraint check_b check (b = 3);
insert into p values (1, 2);
ERROR: new row for relation "p11" violates check constraint "check_b"
DETAIL: Failing row contains (1, 2).
+-- check that inserting into an internal partition successfully results in
+-- checking its partition constraint before inserting into the leaf partition
+-- selected by tuple-routing
+insert into p1 (a, b) values (2, 3);
+ERROR: new row for relation "p11" violates partition constraint
+DETAIL: Failing row contains (3, 2).
-- cleanup
drop table p cascade;
NOTICE: drop cascades to 2 other objects
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 3d2fdb92c5..171196df6d 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -200,5 +200,10 @@ alter table p add constraint check_b check (b = 3);
-- after "(1, 2)" is routed to it
insert into p values (1, 2);
+-- check that inserting into an internal partition successfully results in
+-- checking its partition constraint before inserting into the leaf partition
+-- selected by tuple-routing
+insert into p1 (a, b) values (2, 3);
+
-- cleanup
drop table p cascade;
--
2.11.0
--------------C187C3B1305137A7AECB8485
Content-Type: text/x-diff;
name="0005-Add-some-more-tests-for-tuple-routing.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="0005-Add-some-more-tests-for-tuple-routing.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH 4/5] Fix a bug of insertion into an internal partition.
@ 2016-12-13 06:07 amit <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: amit @ 2016-12-13 06:07 UTC (permalink / raw)
Since implicit partition constraints are not inherited, an internal
partition's constraint was not being enforced when targeted directly.
So, include such constraint when setting up leaf partition result
relations for tuple-routing.
---
src/backend/commands/copy.c | 19 +++++++++++++++++--
src/backend/commands/tablecmds.c | 2 +-
src/backend/executor/execMain.c | 22 +++++++++++++++-------
src/backend/executor/nodeModifyTable.c | 12 +++++++++++-
src/include/executor/executor.h | 3 +--
5 files changed, 45 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e9177491e2..b05055808f 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1411,6 +1411,7 @@ BeginCopy(ParseState *pstate,
int i,
num_parted;
ResultRelInfo *leaf_part_rri;
+ List *partcheck = NIL;
/* Get the tuple-routing information and lock partitions */
cstate->partition_dispatch_info =
@@ -1426,6 +1427,15 @@ BeginCopy(ParseState *pstate,
palloc0(cstate->num_partitions *
sizeof(TupleConversionMap *));
+ /*
+ * If the main target rel is a partition, ExecConstraints() as
+ * applied to each leaf partition must consider its partition
+ * constraint, because unlike explicit constraints, an implicit
+ * partition constraint is not inherited.
+ */
+ if (rel->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(rel, true);
+
leaf_part_rri = cstate->partitions;
i = 0;
foreach(cell, leaf_parts)
@@ -1449,7 +1459,7 @@ BeginCopy(ParseState *pstate,
InitResultRelInfo(leaf_part_rri,
partrel,
1, /* dummy */
- false, /* no partition constraint check */
+ partcheck,
0);
/* Open partition indices */
@@ -2335,6 +2345,7 @@ CopyFrom(CopyState cstate)
uint64 processed = 0;
bool useHeapMultiInsert;
int nBufferedTuples = 0;
+ List *partcheck = NIL;
#define MAX_BUFFERED_TUPLES 1000
HeapTuple *bufferedTuples = NULL; /* initialize to silence warning */
@@ -2451,6 +2462,10 @@ CopyFrom(CopyState cstate)
hi_options |= HEAP_INSERT_FROZEN;
}
+ /* Don't forget the partition constraints */
+ if (cstate->rel->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(cstate->rel, true);
+
/*
* We need a ResultRelInfo so we can use the regular executor's
* index-entry-making machinery. (There used to be a huge amount of code
@@ -2460,7 +2475,7 @@ CopyFrom(CopyState cstate)
InitResultRelInfo(resultRelInfo,
cstate->rel,
1, /* dummy rangetable index */
- true, /* do load partition check expression */
+ partcheck,
0);
ExecOpenIndices(resultRelInfo, false);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f94cab60a3..48adeedbe0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1329,7 +1329,7 @@ ExecuteTruncate(TruncateStmt *stmt)
InitResultRelInfo(resultRelInfo,
rel,
0, /* dummy rangetable index */
- false,
+ NIL, /* No need for partition constraints */
0);
resultRelInfo++;
}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index d43a204808..963cd2ae43 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -820,13 +820,19 @@ InitPlan(QueryDesc *queryDesc, int eflags)
Index resultRelationIndex = lfirst_int(l);
Oid resultRelationOid;
Relation resultRelation;
+ List *partcheck = NIL;
resultRelationOid = getrelid(resultRelationIndex, rangeTable);
resultRelation = heap_open(resultRelationOid, RowExclusiveLock);
+
+ /* Don't forget the partition constraint */
+ if (resultRelation->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(resultRelation, true);
+
InitResultRelInfo(resultRelInfo,
resultRelation,
resultRelationIndex,
- true,
+ partcheck,
estate->es_instrument);
resultRelInfo++;
}
@@ -1216,7 +1222,7 @@ void
InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
+ List *partition_check,
int instrument_options)
{
MemSet(resultRelInfo, 0, sizeof(ResultRelInfo));
@@ -1254,10 +1260,7 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_ConstraintExprs = NULL;
resultRelInfo->ri_junkFilter = NULL;
resultRelInfo->ri_projectReturning = NULL;
- if (load_partition_check)
- resultRelInfo->ri_PartitionCheck =
- RelationGetPartitionQual(resultRelationDesc,
- true);
+ resultRelInfo->ri_PartitionCheck = partition_check;
}
/*
@@ -1284,6 +1287,7 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
ListCell *l;
Relation rel;
MemoryContext oldcontext;
+ List *partcheck = NIL;
/* First, search through the query result relations */
rInfo = estate->es_result_relations;
@@ -1312,6 +1316,10 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
*/
rel = heap_open(relid, NoLock);
+ /* Don't forget the partition constraint */
+ if (rel->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(rel, true);
+
/*
* Make the new entry in the right context.
*/
@@ -1320,7 +1328,7 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
InitResultRelInfo(rInfo,
rel,
0, /* dummy rangetable index */
- true,
+ partcheck,
estate->es_instrument);
estate->es_trig_target_relations =
lappend(estate->es_trig_target_relations, rInfo);
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index ec440b353d..e236c0e0a7 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1724,6 +1724,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
List *leaf_parts;
ListCell *cell;
ResultRelInfo *leaf_part_rri;
+ List *partcheck = NIL;
/* Get the tuple-routing information and lock partitions */
mtstate->mt_partition_dispatch_info =
@@ -1739,6 +1740,15 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
palloc0(mtstate->mt_num_partitions *
sizeof(TupleConversionMap *));
+ /*
+ * If the main target rel is a partition, ExecConstraints() as
+ * applied to each leaf partition must consider its partition
+ * constraint, because unlike explicit constraints, an implicit
+ * partition constraint is not inherited.
+ */
+ if (rel->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(rel, true);
+
leaf_part_rri = mtstate->mt_partitions;
i = j = 0;
foreach(cell, leaf_parts)
@@ -1762,7 +1772,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
InitResultRelInfo(leaf_part_rri,
partrel,
1, /* dummy */
- false, /* no partition constraint checks */
+ partcheck,
eflags);
/* Open partition indices (note: ON CONFLICT unsupported)*/
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index b4d09f9564..74213da078 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -73,7 +73,6 @@
#define ExecEvalExpr(expr, econtext, isNull, isDone) \
((*(expr)->evalfunc) (expr, econtext, isNull, isDone))
-
/* Hook for plugins to get control in ExecutorStart() */
typedef void (*ExecutorStart_hook_type) (QueryDesc *queryDesc, int eflags);
extern PGDLLIMPORT ExecutorStart_hook_type ExecutorStart_hook;
@@ -189,7 +188,7 @@ extern void CheckValidResultRel(Relation resultRel, CmdType operation);
extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
+ List *partition_check,
int instrument_options);
extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids);
--
2.11.0
--------------87B7BEABA41A98E25D2BEF30
Content-Type: text/x-diff;
name="0005-Fix-a-tuple-routing-bug-in-multi-level-partitioned-t.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="0005-Fix-a-tuple-routing-bug-in-multi-level-partitioned-t.pa";
filename*1="tch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH 4/5] Fix a bug of insertion into an internal partition.
@ 2016-12-13 06:07 amit <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: amit @ 2016-12-13 06:07 UTC (permalink / raw)
Since implicit partition constraints are not inherited, an internal
partition's constraint was not being enforced when targeted directly.
So, include such constraint when setting up leaf partition result
relations for tuple-routing.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/commands/copy.c | 1 -
src/backend/commands/tablecmds.c | 1 -
src/backend/executor/execMain.c | 41 ++++++++++++++++++++++++++++--------
src/include/executor/executor.h | 1 -
src/test/regress/expected/insert.out | 6 ++++++
src/test/regress/sql/insert.sql | 5 +++++
6 files changed, 43 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index a20e22daaf..2cb89190e7 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2419,7 +2419,6 @@ CopyFrom(CopyState cstate)
InitResultRelInfo(resultRelInfo,
cstate->rel,
1, /* dummy rangetable index */
- true, /* do load partition check expression */
NULL,
0);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3db830f0c7..096ca181be 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1323,7 +1323,6 @@ ExecuteTruncate(TruncateStmt *stmt)
InitResultRelInfo(resultRelInfo,
rel,
0, /* dummy rangetable index */
- false,
NULL,
0);
resultRelInfo++;
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 6954ba8d32..3e2c1cb790 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -824,10 +824,10 @@ InitPlan(QueryDesc *queryDesc, int eflags)
resultRelationOid = getrelid(resultRelationIndex, rangeTable);
resultRelation = heap_open(resultRelationOid, RowExclusiveLock);
+
InitResultRelInfo(resultRelInfo,
resultRelation,
resultRelationIndex,
- true,
NULL,
estate->es_instrument);
resultRelInfo++;
@@ -1218,10 +1218,11 @@ void
InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
Relation partition_root,
int instrument_options)
{
+ List *partition_check = NIL;
+
MemSet(resultRelInfo, 0, sizeof(ResultRelInfo));
resultRelInfo->type = T_ResultRelInfo;
resultRelInfo->ri_RangeTableIndex = resultRelationIndex;
@@ -1257,14 +1258,38 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_ConstraintExprs = NULL;
resultRelInfo->ri_junkFilter = NULL;
resultRelInfo->ri_projectReturning = NULL;
- if (load_partition_check)
- resultRelInfo->ri_PartitionCheck =
- RelationGetPartitionQual(resultRelationDesc);
/*
- * The following gets set to NULL unless we are initializing leaf
- * partitions for tuple-routing.
+ * If partition_root has been specified, that means we are builiding the
+ * ResultRelationInfo for one of its leaf partitions. In that case, we
+ * need *not* initialize the leaf partition's constraint, but rather the
+ * the partition_root's (if any). We must do that explicitly like this,
+ * because implicit partition constraints are not inherited like user-
+ * defined constraints and would fail to be enforced by ExecConstraints()
+ * after a tuple is routed to a leaf partition.
*/
+ if (partition_root)
+ {
+ /*
+ * Root table itself may or may not be a partition; partition_check
+ * would be NIL in the latter case.
+ */
+ partition_check = RelationGetPartitionQual(partition_root);
+
+ /*
+ * This is not our own partition constraint, but rather an ancestor's.
+ * So any Vars in it bear the ancestor's attribute numbers. We must
+ * switch them to our own.
+ */
+ if (partition_check != NIL)
+ partition_check = map_partition_varattnos(partition_check,
+ resultRelationDesc,
+ partition_root);
+ }
+ else
+ partition_check = RelationGetPartitionQual(resultRelationDesc);
+
+ resultRelInfo->ri_PartitionCheck = partition_check;
resultRelInfo->ri_PartitionRoot = partition_root;
}
@@ -1328,7 +1353,6 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
InitResultRelInfo(rInfo,
rel,
0, /* dummy rangetable index */
- true,
NULL,
estate->es_instrument);
estate->es_trig_target_relations =
@@ -3137,7 +3161,6 @@ ExecSetupPartitionTupleRouting(Relation rel,
InitResultRelInfo(leaf_part_rri,
partrel,
1, /* dummy */
- false,
rel,
0);
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index f85b26b00a..9c7878f847 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -189,7 +189,6 @@ extern void CheckValidResultRel(Relation resultRel, CmdType operation);
extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
Relation partition_root,
int instrument_options);
extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index b120954997..6a6aac5a88 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -339,6 +339,12 @@ alter table p add constraint check_b check (b = 3);
insert into p values (1, 2);
ERROR: new row for relation "p11" violates check constraint "check_b"
DETAIL: Failing row contains (1, 2).
+-- check that inserting into an internal partition successfully results in
+-- checking its partition constraint before inserting into the leaf partition
+-- selected by tuple-routing
+insert into p1 (a, b) values (2, 3);
+ERROR: new row for relation "p11" violates partition constraint
+DETAIL: Failing row contains (3, 2).
-- cleanup
drop table p cascade;
NOTICE: drop cascades to 2 other objects
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 3d2fdb92c5..171196df6d 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -200,5 +200,10 @@ alter table p add constraint check_b check (b = 3);
-- after "(1, 2)" is routed to it
insert into p values (1, 2);
+-- check that inserting into an internal partition successfully results in
+-- checking its partition constraint before inserting into the leaf partition
+-- selected by tuple-routing
+insert into p1 (a, b) values (2, 3);
+
-- cleanup
drop table p cascade;
--
2.11.0
--------------57363B45666F3B09901F4C0C
Content-Type: text/x-diff;
name="0005-Add-some-more-tests-for-tuple-routing.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="0005-Add-some-more-tests-for-tuple-routing.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH 4/7] Fix a bug of insertion into an internal partition.
@ 2016-12-13 06:07 amit <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: amit @ 2016-12-13 06:07 UTC (permalink / raw)
Since implicit partition constraints are not inherited, an internal
partition's constraint was not being enforced when targeted directly.
So, include such constraint when setting up leaf partition result
relations for tuple-routing.
InitResultRelInfo()'s API changes with this. Instead of passing
a boolean telling whether or not to load the partition constraint,
callers now need to pass the exact constraint expression to use
as ri_PartitionCheck or NIL.
---
src/backend/commands/copy.c | 7 +++++-
src/backend/commands/tablecmds.c | 2 +-
src/backend/executor/execMain.c | 43 ++++++++++++++++++++++++++++++------
src/include/executor/executor.h | 3 +--
src/test/regress/expected/insert.out | 4 ++++
src/test/regress/sql/insert.sql | 3 +++
6 files changed, 51 insertions(+), 11 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index d5901651db..a0eb4241e2 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2294,6 +2294,7 @@ CopyFrom(CopyState cstate)
uint64 processed = 0;
bool useHeapMultiInsert;
int nBufferedTuples = 0;
+ List *partcheck = NIL;
#define MAX_BUFFERED_TUPLES 1000
HeapTuple *bufferedTuples = NULL; /* initialize to silence warning */
@@ -2410,6 +2411,10 @@ CopyFrom(CopyState cstate)
hi_options |= HEAP_INSERT_FROZEN;
}
+ /* Don't forget the partition constraints */
+ if (cstate->rel->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(cstate->rel);
+
/*
* We need a ResultRelInfo so we can use the regular executor's
* index-entry-making machinery. (There used to be a huge amount of code
@@ -2419,7 +2424,7 @@ CopyFrom(CopyState cstate)
InitResultRelInfo(resultRelInfo,
cstate->rel,
1, /* dummy rangetable index */
- true, /* do load partition check expression */
+ partcheck,
0);
ExecOpenIndices(resultRelInfo, false);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d2e4cfc365..4bd4ec4e8c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1323,7 +1323,7 @@ ExecuteTruncate(TruncateStmt *stmt)
InitResultRelInfo(resultRelInfo,
rel,
0, /* dummy rangetable index */
- false,
+ NIL,
0);
resultRelInfo++;
}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index b3cedadce4..5627378a35 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -821,13 +821,19 @@ InitPlan(QueryDesc *queryDesc, int eflags)
Index resultRelationIndex = lfirst_int(l);
Oid resultRelationOid;
Relation resultRelation;
+ List *partcheck = NIL;
resultRelationOid = getrelid(resultRelationIndex, rangeTable);
resultRelation = heap_open(resultRelationOid, RowExclusiveLock);
+
+ /* Don't forget the partition constraint */
+ if (resultRelation->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(resultRelation);
+
InitResultRelInfo(resultRelInfo,
resultRelation,
resultRelationIndex,
- true,
+ partcheck,
estate->es_instrument);
resultRelInfo++;
}
@@ -1217,7 +1223,7 @@ void
InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
+ List *partition_check,
int instrument_options)
{
MemSet(resultRelInfo, 0, sizeof(ResultRelInfo));
@@ -1255,9 +1261,7 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_ConstraintExprs = NULL;
resultRelInfo->ri_junkFilter = NULL;
resultRelInfo->ri_projectReturning = NULL;
- if (load_partition_check)
- resultRelInfo->ri_PartitionCheck =
- RelationGetPartitionQual(resultRelationDesc);
+ resultRelInfo->ri_PartitionCheck = partition_check;
}
/*
@@ -1284,6 +1288,7 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
ListCell *l;
Relation rel;
MemoryContext oldcontext;
+ List *partcheck = NIL;
/* First, search through the query result relations */
rInfo = estate->es_result_relations;
@@ -1312,6 +1317,10 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
*/
rel = heap_open(relid, NoLock);
+ /* Don't forget the partition constraint */
+ if (rel->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(rel);
+
/*
* Make the new entry in the right context.
*/
@@ -1320,7 +1329,7 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
InitResultRelInfo(rInfo,
rel,
0, /* dummy rangetable index */
- true,
+ partcheck,
estate->es_instrument);
estate->es_trig_target_relations =
lappend(estate->es_trig_target_relations, rInfo);
@@ -3032,6 +3041,7 @@ ExecSetupPartitionTupleRouting(Relation rel,
ListCell *cell;
int i;
ResultRelInfo *leaf_part_rri;
+ List *partcheck = NIL;
/* Get the tuple-routing information and lock partitions */
*pd = RelationGetPartitionDispatchInfo(rel, RowExclusiveLock, num_parted,
@@ -3042,12 +3052,22 @@ ExecSetupPartitionTupleRouting(Relation rel,
*tup_conv_maps = (TupleConversionMap **) palloc0(*num_partitions *
sizeof(TupleConversionMap *));
+ /*
+ * If the main target rel is a partition, ExecConstraints() as applied to
+ * each leaf partition must consider its partition constraint, because
+ * unlike explicit constraints, an implicit partition constraint is not
+ * inherited.
+ */
+ if (rel->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(rel);
+
leaf_part_rri = *partitions;
i = 0;
foreach(cell, leaf_parts)
{
Relation partrel;
TupleDesc part_tupdesc;
+ List *my_check = NIL;
/*
* We locked all the partitions above including the leaf partitions.
@@ -3069,10 +3089,19 @@ ExecSetupPartitionTupleRouting(Relation rel,
(*tup_conv_maps)[i] = convert_tuples_by_name(tupDesc, part_tupdesc,
gettext_noop("could not convert row type"));
+ /*
+ * This is the parent's partition constraint, so any Vars in
+ * it bear the its attribute numbers. We must switch them to
+ * the leaf partition's, which is possible with the
+ * reverse_map's attribute-number map.
+ */
+ if (partcheck)
+ my_check = map_partition_varattnos(partcheck, partrel, rel);
+
InitResultRelInfo(leaf_part_rri,
partrel,
1, /* dummy */
- false,
+ my_check,
0);
/* Open partition indices */
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index bd1bc6bb6e..8bcc876809 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -73,7 +73,6 @@
#define ExecEvalExpr(expr, econtext, isNull, isDone) \
((*(expr)->evalfunc) (expr, econtext, isNull, isDone))
-
/* Hook for plugins to get control in ExecutorStart() */
typedef void (*ExecutorStart_hook_type) (QueryDesc *queryDesc, int eflags);
extern PGDLLIMPORT ExecutorStart_hook_type ExecutorStart_hook;
@@ -189,7 +188,7 @@ extern void CheckValidResultRel(Relation resultRel, CmdType operation);
extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
+ List *partition_check,
int instrument_options);
extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids);
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 561cefa3c4..95a7c4da7a 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -285,6 +285,10 @@ select tableoid::regclass, * from list_parted;
part_ee_ff2 | EE | 10
(8 rows)
+-- fail due to partition constraint failure
+insert into part_ee_ff values ('gg', 1);
+ERROR: new row for relation "part_ee_ff1" violates partition constraint
+DETAIL: Failing row contains (gg, 1).
-- cleanup
drop table range_parted cascade;
NOTICE: drop cascades to 4 other objects
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 846bb5897a..77577682ac 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -167,6 +167,9 @@ insert into list_parted values ('EE', 1);
insert into part_ee_ff values ('EE', 10);
select tableoid::regclass, * from list_parted;
+-- fail due to partition constraint failure
+insert into part_ee_ff values ('gg', 1);
+
-- cleanup
drop table range_parted cascade;
drop table list_parted cascade;
--
2.11.0
--------------487C465F02616E3296F6B86F
Content-Type: text/x-diff;
name="0005-Fix-oddities-of-tuple-routing-and-TupleTableSlots.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="0005-Fix-oddities-of-tuple-routing-and-TupleTableSlots.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH 4/7] Fix a bug of insertion into an internal partition.
@ 2016-12-13 06:07 amit <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: amit @ 2016-12-13 06:07 UTC (permalink / raw)
Since implicit partition constraints are not inherited, an internal
partition's constraint was not being enforced when targeted directly.
So, include such constraint when setting up leaf partition result
relations for tuple-routing.
InitResultRelInfo()'s API changes with this. Instead of passing
a boolean telling whether or not to load the partition constraint,
callers now need to pass the exact constraint expression to use
as ri_PartitionCheck or NIL.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/commands/copy.c | 7 +++++-
src/backend/commands/tablecmds.c | 2 +-
src/backend/executor/execMain.c | 42 ++++++++++++++++++++++++++++++------
src/include/executor/executor.h | 3 +--
src/test/regress/expected/insert.out | 4 ++++
src/test/regress/sql/insert.sql | 3 +++
6 files changed, 50 insertions(+), 11 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index d5901651db..a0eb4241e2 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2294,6 +2294,7 @@ CopyFrom(CopyState cstate)
uint64 processed = 0;
bool useHeapMultiInsert;
int nBufferedTuples = 0;
+ List *partcheck = NIL;
#define MAX_BUFFERED_TUPLES 1000
HeapTuple *bufferedTuples = NULL; /* initialize to silence warning */
@@ -2410,6 +2411,10 @@ CopyFrom(CopyState cstate)
hi_options |= HEAP_INSERT_FROZEN;
}
+ /* Don't forget the partition constraints */
+ if (cstate->rel->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(cstate->rel);
+
/*
* We need a ResultRelInfo so we can use the regular executor's
* index-entry-making machinery. (There used to be a huge amount of code
@@ -2419,7 +2424,7 @@ CopyFrom(CopyState cstate)
InitResultRelInfo(resultRelInfo,
cstate->rel,
1, /* dummy rangetable index */
- true, /* do load partition check expression */
+ partcheck,
0);
ExecOpenIndices(resultRelInfo, false);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d2e4cfc365..4bd4ec4e8c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1323,7 +1323,7 @@ ExecuteTruncate(TruncateStmt *stmt)
InitResultRelInfo(resultRelInfo,
rel,
0, /* dummy rangetable index */
- false,
+ NIL,
0);
resultRelInfo++;
}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index b3cedadce4..1b19bc38ef 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -821,13 +821,19 @@ InitPlan(QueryDesc *queryDesc, int eflags)
Index resultRelationIndex = lfirst_int(l);
Oid resultRelationOid;
Relation resultRelation;
+ List *partcheck = NIL;
resultRelationOid = getrelid(resultRelationIndex, rangeTable);
resultRelation = heap_open(resultRelationOid, RowExclusiveLock);
+
+ /* Don't forget the partition constraint */
+ if (resultRelation->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(resultRelation);
+
InitResultRelInfo(resultRelInfo,
resultRelation,
resultRelationIndex,
- true,
+ partcheck,
estate->es_instrument);
resultRelInfo++;
}
@@ -1217,7 +1223,7 @@ void
InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
+ List *partition_check,
int instrument_options)
{
MemSet(resultRelInfo, 0, sizeof(ResultRelInfo));
@@ -1255,9 +1261,7 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_ConstraintExprs = NULL;
resultRelInfo->ri_junkFilter = NULL;
resultRelInfo->ri_projectReturning = NULL;
- if (load_partition_check)
- resultRelInfo->ri_PartitionCheck =
- RelationGetPartitionQual(resultRelationDesc);
+ resultRelInfo->ri_PartitionCheck = partition_check;
}
/*
@@ -1284,6 +1288,7 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
ListCell *l;
Relation rel;
MemoryContext oldcontext;
+ List *partcheck = NIL;
/* First, search through the query result relations */
rInfo = estate->es_result_relations;
@@ -1312,6 +1317,10 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
*/
rel = heap_open(relid, NoLock);
+ /* Don't forget the partition constraint */
+ if (rel->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(rel);
+
/*
* Make the new entry in the right context.
*/
@@ -1320,7 +1329,7 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
InitResultRelInfo(rInfo,
rel,
0, /* dummy rangetable index */
- true,
+ partcheck,
estate->es_instrument);
estate->es_trig_target_relations =
lappend(estate->es_trig_target_relations, rInfo);
@@ -3032,6 +3041,7 @@ ExecSetupPartitionTupleRouting(Relation rel,
ListCell *cell;
int i;
ResultRelInfo *leaf_part_rri;
+ List *partcheck = NIL;
/* Get the tuple-routing information and lock partitions */
*pd = RelationGetPartitionDispatchInfo(rel, RowExclusiveLock, num_parted,
@@ -3042,12 +3052,22 @@ ExecSetupPartitionTupleRouting(Relation rel,
*tup_conv_maps = (TupleConversionMap **) palloc0(*num_partitions *
sizeof(TupleConversionMap *));
+ /*
+ * If the main target rel is a partition, ExecConstraints() as applied to
+ * each leaf partition must consider its partition constraint, because
+ * unlike explicit constraints, an implicit partition constraint is not
+ * inherited.
+ */
+ if (rel->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(rel);
+
leaf_part_rri = *partitions;
i = 0;
foreach(cell, leaf_parts)
{
Relation partrel;
TupleDesc part_tupdesc;
+ List *my_check = NIL;
/*
* We locked all the partitions above including the leaf partitions.
@@ -3069,10 +3089,18 @@ ExecSetupPartitionTupleRouting(Relation rel,
(*tup_conv_maps)[i] = convert_tuples_by_name(tupDesc, part_tupdesc,
gettext_noop("could not convert row type"));
+ /*
+ * This is the parent's partition constraint, so any Vars in
+ * it bear the its attribute numbers. We must switch them to
+ * the leaf partition's.
+ */
+ if (partcheck)
+ my_check = map_partition_varattnos(partcheck, partrel, rel);
+
InitResultRelInfo(leaf_part_rri,
partrel,
1, /* dummy */
- false,
+ my_check,
0);
/* Open partition indices */
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index b74fa5eb5d..c8e42ae2eb 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -73,7 +73,6 @@
#define ExecEvalExpr(expr, econtext, isNull, isDone) \
((*(expr)->evalfunc) (expr, econtext, isNull, isDone))
-
/* Hook for plugins to get control in ExecutorStart() */
typedef void (*ExecutorStart_hook_type) (QueryDesc *queryDesc, int eflags);
extern PGDLLIMPORT ExecutorStart_hook_type ExecutorStart_hook;
@@ -189,7 +188,7 @@ extern void CheckValidResultRel(Relation resultRel, CmdType operation);
extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
+ List *partition_check,
int instrument_options);
extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids);
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 561cefa3c4..95a7c4da7a 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -285,6 +285,10 @@ select tableoid::regclass, * from list_parted;
part_ee_ff2 | EE | 10
(8 rows)
+-- fail due to partition constraint failure
+insert into part_ee_ff values ('gg', 1);
+ERROR: new row for relation "part_ee_ff1" violates partition constraint
+DETAIL: Failing row contains (gg, 1).
-- cleanup
drop table range_parted cascade;
NOTICE: drop cascades to 4 other objects
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 846bb5897a..77577682ac 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -167,6 +167,9 @@ insert into list_parted values ('EE', 1);
insert into part_ee_ff values ('EE', 10);
select tableoid::regclass, * from list_parted;
+-- fail due to partition constraint failure
+insert into part_ee_ff values ('gg', 1);
+
-- cleanup
drop table range_parted cascade;
drop table list_parted cascade;
--
2.11.0
--------------6BBEDC11B506E66A7D75776C
Content-Type: text/x-diff;
name="0005-Fix-oddities-of-tuple-routing-and-TupleTableSlots-2.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="0005-Fix-oddities-of-tuple-routing-and-TupleTableSlots-2.pat";
filename*1="ch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH 4/7] Fix a bug of insertion into an internal partition.
@ 2016-12-13 06:07 amit <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: amit @ 2016-12-13 06:07 UTC (permalink / raw)
Since implicit partition constraints are not inherited, an internal
partition's constraint was not being enforced when targeted directly.
So, include such constraint when setting up leaf partition result
relations for tuple-routing.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/commands/copy.c | 1 -
src/backend/commands/tablecmds.c | 1 -
src/backend/executor/execMain.c | 41 ++++++++++++++++++++++++++++--------
src/include/executor/executor.h | 1 -
src/test/regress/expected/insert.out | 6 ++++++
src/test/regress/sql/insert.sql | 5 +++++
6 files changed, 43 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index afbfb9f6e8..5aa4449e3e 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2429,7 +2429,6 @@ CopyFrom(CopyState cstate)
InitResultRelInfo(resultRelInfo,
cstate->rel,
1, /* dummy rangetable index */
- true, /* do load partition check expression */
NULL,
0);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3c08551d38..2e51124eb3 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1323,7 +1323,6 @@ ExecuteTruncate(TruncateStmt *stmt)
InitResultRelInfo(resultRelInfo,
rel,
0, /* dummy rangetable index */
- false,
NULL,
0);
resultRelInfo++;
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 6a82c18571..c991a18ce8 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -824,10 +824,10 @@ InitPlan(QueryDesc *queryDesc, int eflags)
resultRelationOid = getrelid(resultRelationIndex, rangeTable);
resultRelation = heap_open(resultRelationOid, RowExclusiveLock);
+
InitResultRelInfo(resultRelInfo,
resultRelation,
resultRelationIndex,
- true,
NULL,
estate->es_instrument);
resultRelInfo++;
@@ -1218,10 +1218,11 @@ void
InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
Relation partition_root,
int instrument_options)
{
+ List *partition_check = NIL;
+
MemSet(resultRelInfo, 0, sizeof(ResultRelInfo));
resultRelInfo->type = T_ResultRelInfo;
resultRelInfo->ri_RangeTableIndex = resultRelationIndex;
@@ -1257,14 +1258,38 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_ConstraintExprs = NULL;
resultRelInfo->ri_junkFilter = NULL;
resultRelInfo->ri_projectReturning = NULL;
- if (load_partition_check)
- resultRelInfo->ri_PartitionCheck =
- RelationGetPartitionQual(resultRelationDesc);
/*
- * The following gets set to NULL unless we are initializing leaf
- * partitions for tuple-routing.
+ * If partition_root has been specified, that means we are builiding the
+ * ResultRelationInfo for one of its leaf partitions. In that case, we
+ * need *not* initialize the leaf partition's constraint, but rather the
+ * the partition_root's (if any). We must do that explicitly like this,
+ * because implicit partition constraints are not inherited like user-
+ * defined constraints and would fail to be enforced by ExecConstraints()
+ * after a tuple is routed to a leaf partition.
*/
+ if (partition_root)
+ {
+ /*
+ * Root table itself may or may not be a partition; partition_check
+ * would be NIL in the latter case.
+ */
+ partition_check = RelationGetPartitionQual(partition_root);
+
+ /*
+ * This is not our own partition constraint, but rather an ancestor's.
+ * So any Vars in it bear the ancestor's attribute numbers. We must
+ * switch them to our own.
+ */
+ if (partition_check != NIL)
+ partition_check = map_partition_varattnos(partition_check,
+ resultRelationDesc,
+ partition_root);
+ }
+ else
+ partition_check = RelationGetPartitionQual(resultRelationDesc);
+
+ resultRelInfo->ri_PartitionCheck = partition_check;
resultRelInfo->ri_PartitionRoot = partition_root;
}
@@ -1328,7 +1353,6 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
InitResultRelInfo(rInfo,
rel,
0, /* dummy rangetable index */
- true,
NULL,
estate->es_instrument);
estate->es_trig_target_relations =
@@ -3133,7 +3157,6 @@ ExecSetupPartitionTupleRouting(Relation rel,
InitResultRelInfo(leaf_part_rri,
partrel,
1, /* dummy */
- false,
rel,
0);
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 70ecf108a3..4fed4e101f 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -189,7 +189,6 @@ extern void CheckValidResultRel(Relation resultRel, CmdType operation);
extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
Relation partition_root,
int instrument_options);
extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index b120954997..6a6aac5a88 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -339,6 +339,12 @@ alter table p add constraint check_b check (b = 3);
insert into p values (1, 2);
ERROR: new row for relation "p11" violates check constraint "check_b"
DETAIL: Failing row contains (1, 2).
+-- check that inserting into an internal partition successfully results in
+-- checking its partition constraint before inserting into the leaf partition
+-- selected by tuple-routing
+insert into p1 (a, b) values (2, 3);
+ERROR: new row for relation "p11" violates partition constraint
+DETAIL: Failing row contains (3, 2).
-- cleanup
drop table p cascade;
NOTICE: drop cascades to 2 other objects
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 3d2fdb92c5..171196df6d 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -200,5 +200,10 @@ alter table p add constraint check_b check (b = 3);
-- after "(1, 2)" is routed to it
insert into p values (1, 2);
+-- check that inserting into an internal partition successfully results in
+-- checking its partition constraint before inserting into the leaf partition
+-- selected by tuple-routing
+insert into p1 (a, b) values (2, 3);
+
-- cleanup
drop table p cascade;
--
2.11.0
--------------AEE611E30AEE235CC4F15C3B
Content-Type: text/x-diff;
name="0005-Add-some-more-tests-for-tuple-routing.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="0005-Add-some-more-tests-for-tuple-routing.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v5 3/3] Implement CLUSTER of partitioned table..
@ 2020-06-07 21:58 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Justin Pryzby @ 2020-06-07 21:58 UTC (permalink / raw)
This requires either specification of a partitioned index on which to cluster,
or that an partitioned index was previously set clustered.
---
src/backend/commands/cluster.c | 139 +++++++++++++++++++-------
src/bin/psql/tab-complete.c | 1 +
src/test/regress/expected/cluster.out | 23 ++++-
src/test/regress/sql/cluster.sql | 12 ++-
4 files changed, 133 insertions(+), 42 deletions(-)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 04d12a7ece..7409c17b0b 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -33,6 +33,7 @@
#include "catalog/namespace.h"
#include "catalog/objectaccess.h"
#include "catalog/pg_am.h"
+#include "catalog/pg_inherits.h"
#include "catalog/toasting.h"
#include "commands/cluster.h"
#include "commands/progress.h"
@@ -72,6 +73,9 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
bool verbose, bool *pSwapToastByContent,
TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
static List *get_tables_to_cluster(MemoryContext cluster_context);
+static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
+ Oid indexOid);
+static void cluster_multiple_rels(List *rvs, int options);
/*---------------------------------------------------------------------------
@@ -124,14 +128,6 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot cluster temporary tables of other sessions")));
- /*
- * Reject clustering a partitioned table.
- */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot cluster a partitioned table")));
-
if (stmt->indexname == NULL)
{
ListCell *index;
@@ -169,8 +165,37 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
/* close relation, keep lock till commit */
table_close(rel, NoLock);
- /* Do the job. */
- cluster_rel(tableOid, indexOid, stmt->options);
+ if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+ {
+ /* Do the job. */
+ cluster_rel(tableOid, indexOid, stmt->options);
+ } else {
+ List *rvs;
+ MemoryContext cluster_context;
+
+ /* Check index directly since cluster_rel isn't called for partitioned table */
+ check_index_is_clusterable(rel, indexOid, true, AccessExclusiveLock);
+
+ /* Refuse to hold strong locks in a user transaction */
+ PreventInTransactionBlock(isTopLevel, "CLUSTER");
+
+ cluster_context = AllocSetContextCreate(PortalContext,
+ "Cluster",
+ ALLOCSET_DEFAULT_SIZES);
+
+ rvs = get_tables_to_cluster_partitioned(cluster_context, indexOid);
+ cluster_multiple_rels(rvs, stmt->options);
+
+ /* Start a new transaction for the cleanup work. */
+ StartTransactionCommand();
+
+ rel = table_open(tableOid, ShareUpdateExclusiveLock);
+ mark_index_clustered(rel, indexOid, true);
+ table_close(rel, NoLock);
+
+ /* Clean up working storage */
+ MemoryContextDelete(cluster_context);
+ }
}
else
{
@@ -180,7 +205,6 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
*/
MemoryContext cluster_context;
List *rvs;
- ListCell *rv;
/*
* We cannot run this form of CLUSTER inside a user transaction block;
@@ -204,25 +228,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
*/
rvs = get_tables_to_cluster(cluster_context);
- /* Commit to get out of starting transaction */
- PopActiveSnapshot();
- CommitTransactionCommand();
-
- /* Ok, now that we've got them all, cluster them one by one */
- foreach(rv, rvs)
- {
- RelToCluster *rvtc = (RelToCluster *) lfirst(rv);
-
- /* Start a new transaction for each relation. */
- StartTransactionCommand();
- /* functions in indexes may want a snapshot set */
- PushActiveSnapshot(GetTransactionSnapshot());
- /* Do the job. */
- cluster_rel(rvtc->tableOid, rvtc->indexOid,
- stmt->options | CLUOPT_RECHECK);
- PopActiveSnapshot();
- CommitTransactionCommand();
- }
+ cluster_multiple_rels(rvs, stmt->options);
/* Start a new transaction for the cleanup work. */
StartTransactionCommand();
@@ -483,12 +489,6 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
Relation pg_index;
ListCell *index;
- /* Disallow applying to a partitioned table */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot mark index clustered in partitioned table")));
-
/*
* If the index is already marked clustered, no need to do anything.
*/
@@ -1557,3 +1557,70 @@ get_tables_to_cluster(MemoryContext cluster_context)
return rvs;
}
+
+/*
+ * Return a List of tables and associated index, where each index is a
+ * partition of the given index
+ */
+static List *
+get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
+{
+ List *inhoids;
+ ListCell *lc;
+ List *rvs = NIL;
+
+ MemoryContext old_context = MemoryContextSwitchTo(cluster_context);
+
+ inhoids = find_all_inheritors(indexOid, NoLock, NULL);
+ foreach(lc, inhoids)
+ {
+ Oid indexrelid = lfirst_oid(lc);
+ Oid relid = IndexGetRelation(indexrelid, false);
+ RelToCluster *rvtc;
+
+ /*
+ * We have a full list of direct and indirect children, so skip
+ * partitioned tables and just handle their children.
+ */
+ if (get_rel_relkind(relid) == RELKIND_PARTITIONED_TABLE)
+ continue;
+
+ rvtc = (RelToCluster *) palloc(sizeof(RelToCluster));
+ rvtc->tableOid = relid;
+ rvtc->indexOid = indexrelid;
+ rvs = lappend(rvs, rvtc);
+ }
+
+ MemoryContextSwitchTo(old_context);
+ return rvs;
+}
+
+/* Cluster each relation in a separate transaction */
+static void
+cluster_multiple_rels(List *rvs, int options)
+{
+ ListCell *lc;
+
+ /* Commit to get out of starting transaction */
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+
+ /* Ok, now that we've got them all, cluster them one by one */
+ foreach(lc, rvs)
+ {
+ RelToCluster *rvtc = (RelToCluster *) lfirst(lc);
+
+ /* Start a new transaction for each relation. */
+ StartTransactionCommand();
+
+ /* functions in indexes may want a snapshot set */
+ PushActiveSnapshot(GetTransactionSnapshot());
+
+ /* Do the job. */
+ cluster_rel(rvtc->tableOid, rvtc->indexOid,
+ options | CLUOPT_RECHECK);
+
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+ }
+}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index c4af40bfa9..e68808218a 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -587,6 +587,7 @@ static const SchemaQuery Query_for_list_of_clusterables = {
.catname = "pg_catalog.pg_class c",
.selcondition =
"c.relkind IN (" CppAsString2(RELKIND_RELATION) ", "
+ CppAsString2(RELKIND_PARTITIONED_TABLE) ", "
CppAsString2(RELKIND_MATVIEW) ")",
.viscondition = "pg_catalog.pg_table_is_visible(c.oid)",
.namespace = "c.relnamespace",
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index bdae8fe00c..21b28fde6e 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -439,13 +439,28 @@ select * from clstr_temp;
drop table clstr_temp;
RESET SESSION AUTHORIZATION;
--- Check that partitioned tables cannot be clustered
+-- Check that partitioned tables can be clustered
CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart1 PARTITION OF clstrpart FOR VALUES FROM (1)TO(10) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart11 PARTITION OF clstrpart1 FOR VALUES FROM (1)TO(10);
+CREATE TABLE clstrpart12 PARTITION OF clstrpart1 FOR VALUES FROM (10)TO(20) PARTITION BY RANGE(a);
+CREATE TABLE clstrpart2 PARTITION OF clstrpart FOR VALUES FROM (20)TO(30);
CREATE INDEX clstrpart_idx ON clstrpart (a);
-ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
-ERROR: cannot mark index clustered in partitioned table
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
CLUSTER clstrpart USING clstrpart_idx;
-ERROR: cannot cluster a partitioned table
+CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitioned
+CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
+CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
+\d clstrpart
+ Partitioned table "public.clstrpart"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition key: RANGE (a)
+Indexes:
+ "clstrpart_idx" btree (a) CLUSTER
+Number of partitions: 2 (Use \d+ to list them.)
+
DROP TABLE clstrpart;
-- Test CLUSTER with external tuplesorting
create table clstr_4 as select * from tenk1;
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 188183647c..4a76848213 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -196,11 +196,19 @@ drop table clstr_temp;
RESET SESSION AUTHORIZATION;
--- Check that partitioned tables cannot be clustered
+-- Check that partitioned tables can be clustered
CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart1 PARTITION OF clstrpart FOR VALUES FROM (1)TO(10) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart11 PARTITION OF clstrpart1 FOR VALUES FROM (1)TO(10);
+CREATE TABLE clstrpart12 PARTITION OF clstrpart1 FOR VALUES FROM (10)TO(20) PARTITION BY RANGE(a);
+CREATE TABLE clstrpart2 PARTITION OF clstrpart FOR VALUES FROM (20)TO(30);
CREATE INDEX clstrpart_idx ON clstrpart (a);
-ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
CLUSTER clstrpart USING clstrpart_idx;
+CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitioned
+CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
+CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
+\d clstrpart
DROP TABLE clstrpart;
-- Test CLUSTER with external tuplesorting
--
2.17.0
--hYooF8G/hrfVAmum--
^ permalink raw reply [nested|flat] 15+ messages in thread
* may be a buffer overflow problem
@ 2024-06-14 07:38 Winter Loo <[email protected]>
2024-06-14 07:55 ` Re: may be a buffer overflow problem Daniel Gustafsson <[email protected]>
2024-06-14 08:04 ` Re: may be a buffer overflow problem Laurenz Albe <[email protected]>
0 siblings, 2 replies; 15+ messages in thread
From: Winter Loo @ 2024-06-14 07:38 UTC (permalink / raw)
To: [email protected]
Hi hackers,
I am using gcc version 11.3.0 to compile postgres source code. Gcc complains about the following line:
```c
strncpy(sqlca->sqlstate, "YE001", sizeof(sqlca->sqlstate));
```
with error as:
misc.c:529:17: error: ‘strncpy’ output truncated before terminating nul copying 5 bytes from a string of the same length [-Werror=stringop-truncation]
I find the definition of `sqlca->sqlstate` and it has only 5 bytes. When the statement
```c
strncpy(sqlca->sqlstate, "YE001", sizeof(sqlca->sqlstate));
```
get executed, `sqlca->sqlstate` will have no '\0' byte which makes me anxious when someone prints that as a string. Indeed, I found the code(in src/interfaces/ecpg/ecpglib/misc.c) does that,
```c
fprintf(debugstream, "[NO_PID]: sqlca: code: %ld, state: %s\n",
sqlca->sqlcode, sqlca->sqlstate);
```
Is there any chance to fix the code?
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: may be a buffer overflow problem
2024-06-14 07:38 may be a buffer overflow problem Winter Loo <[email protected]>
@ 2024-06-14 07:55 ` Daniel Gustafsson <[email protected]>
2024-06-14 08:06 ` Re: may be a buffer overflow problem Laurenz Albe <[email protected]>
2024-06-14 14:39 ` Re: may be a buffer overflow problem Tom Lane <[email protected]>
1 sibling, 2 replies; 15+ messages in thread
From: Daniel Gustafsson @ 2024-06-14 07:55 UTC (permalink / raw)
To: Winter Loo <[email protected]>; +Cc: pgsql-hackers <[email protected]>
> On 14 Jun 2024, at 09:38, Winter Loo <[email protected]> wrote:
> I find the definition of `sqlca->sqlstate` and it has only 5 bytes. When the statement
>
> ```c
> strncpy(sqlca->sqlstate, "YE001", sizeof(sqlca->sqlstate));
> ```
>
> get executed, `sqlca->sqlstate` will have no '\0' byte which makes me anxious when someone prints that as a string.
sqlstate is defined as not being unterminated fixed-length, leaving the callers
to handle termination.
> Indeed, I found the code(in src/interfaces/ecpg/ecpglib/misc.c) does that,
>
> fprintf(debugstream, "[NO_PID]: sqlca: code: %ld, state: %s\n",
> sqlca->sqlcode, sqlca->sqlstate);
This is indeed buggy and need to take the length into account, as per the
attached. This only happens when in the undocumented regression test debug
mode which may be why it's gone unnoticed.
--
Daniel Gustafsson
Attachments:
[application/octet-stream] ecgp_sqlstate.diff (639B, ../../[email protected]/2-ecgp_sqlstate.diff)
download | inline diff:
diff --git a/src/interfaces/ecpg/ecpglib/misc.c b/src/interfaces/ecpg/ecpglib/misc.c
index 2ae989e3e5..110f73d3dd 100644
--- a/src/interfaces/ecpg/ecpglib/misc.c
+++ b/src/interfaces/ecpg/ecpglib/misc.c
@@ -274,8 +274,8 @@ ecpg_log(const char *format,...)
/* dump out internal sqlca variables */
if (ecpg_internal_regression_mode && sqlca != NULL)
{
- fprintf(debugstream, "[NO_PID]: sqlca: code: %ld, state: %s\n",
- sqlca->sqlcode, sqlca->sqlstate);
+ fprintf(debugstream, "[NO_PID]: sqlca: code: %ld, state: %.*s\n",
+ sqlca->sqlcode, (int) sizeof(sqlca->sqlstate), sqlca->sqlstate);
}
fflush(debugstream);
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: may be a buffer overflow problem
2024-06-14 07:38 may be a buffer overflow problem Winter Loo <[email protected]>
2024-06-14 07:55 ` Re: may be a buffer overflow problem Daniel Gustafsson <[email protected]>
@ 2024-06-14 08:06 ` Laurenz Albe <[email protected]>
2024-06-14 08:10 ` Re: may be a buffer overflow problem Daniel Gustafsson <[email protected]>
1 sibling, 1 reply; 15+ messages in thread
From: Laurenz Albe @ 2024-06-14 08:06 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; Winter Loo <[email protected]>; +Cc: pgsql-hackers <[email protected]>
On Fri, 2024-06-14 at 09:55 +0200, Daniel Gustafsson wrote:
> > On 14 Jun 2024, at 09:38, Winter Loo <[email protected]> wrote:
>
> > I find the definition of `sqlca->sqlstate` and it has only 5 bytes. When the statement
> >
> > ```c
> > strncpy(sqlca->sqlstate, "YE001", sizeof(sqlca->sqlstate));
> > ```
> >
> > get executed, `sqlca->sqlstate` will have no '\0' byte which makes me anxious when someone prints that as a string.
>
> sqlstate is defined as not being unterminated fixed-length, leaving the callers
> to handle termination.
>
> > Indeed, I found the code(in src/interfaces/ecpg/ecpglib/misc.c) does that,
> >
> > fprintf(debugstream, "[NO_PID]: sqlca: code: %ld, state: %s\n",
> > sqlca->sqlcode, sqlca->sqlstate);
>
> This is indeed buggy and need to take the length into account, as per the
> attached. This only happens when in the undocumented regression test debug
> mode which may be why it's gone unnoticed.
So you think we should ignore that compiler warning?
What about using memcpy() instead of strncpy()?
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: may be a buffer overflow problem
2024-06-14 07:38 may be a buffer overflow problem Winter Loo <[email protected]>
2024-06-14 07:55 ` Re: may be a buffer overflow problem Daniel Gustafsson <[email protected]>
2024-06-14 08:06 ` Re: may be a buffer overflow problem Laurenz Albe <[email protected]>
@ 2024-06-14 08:10 ` Daniel Gustafsson <[email protected]>
2024-06-14 08:29 ` Re: may be a buffer overflow problem Laurenz Albe <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Daniel Gustafsson @ 2024-06-14 08:10 UTC (permalink / raw)
To: Laurenz Albe <[email protected]>; +Cc: Winter Loo <[email protected]>; pgsql-hackers <[email protected]>
> On 14 Jun 2024, at 10:06, Laurenz Albe <[email protected]> wrote:
> So you think we should ignore that compiler warning?
We already do using this in meson.build:
# Similarly disable useless truncation warnings from gcc 8+
'format-truncation',
'stringop-truncation',
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: may be a buffer overflow problem
2024-06-14 07:38 may be a buffer overflow problem Winter Loo <[email protected]>
2024-06-14 07:55 ` Re: may be a buffer overflow problem Daniel Gustafsson <[email protected]>
2024-06-14 08:06 ` Re: may be a buffer overflow problem Laurenz Albe <[email protected]>
2024-06-14 08:10 ` Re: may be a buffer overflow problem Daniel Gustafsson <[email protected]>
@ 2024-06-14 08:29 ` Laurenz Albe <[email protected]>
2024-06-14 08:39 ` Re: may be a buffer overflow problem Daniel Gustafsson <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Laurenz Albe @ 2024-06-14 08:29 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; +Cc: Winter Loo <[email protected]>; pgsql-hackers <[email protected]>
On Fri, 2024-06-14 at 10:10 +0200, Daniel Gustafsson wrote:
> > On 14 Jun 2024, at 10:06, Laurenz Albe <[email protected]> wrote:
>
> > So you think we should ignore that compiler warning?
>
> We already do using this in meson.build:
>
> # Similarly disable useless truncation warnings from gcc 8+
> 'format-truncation',
> 'stringop-truncation',
Right; and I see that -Wno-stringop-truncation is also set if you build
with "make". So your patch is good.
I wonder how Winter Loo got to see that warning...
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: may be a buffer overflow problem
2024-06-14 07:38 may be a buffer overflow problem Winter Loo <[email protected]>
2024-06-14 07:55 ` Re: may be a buffer overflow problem Daniel Gustafsson <[email protected]>
2024-06-14 08:06 ` Re: may be a buffer overflow problem Laurenz Albe <[email protected]>
2024-06-14 08:10 ` Re: may be a buffer overflow problem Daniel Gustafsson <[email protected]>
2024-06-14 08:29 ` Re: may be a buffer overflow problem Laurenz Albe <[email protected]>
@ 2024-06-14 08:39 ` Daniel Gustafsson <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Gustafsson @ 2024-06-14 08:39 UTC (permalink / raw)
To: Laurenz Albe <[email protected]>; +Cc: Winter Loo <[email protected]>; pgsql-hackers <[email protected]>
> On 14 Jun 2024, at 10:29, Laurenz Albe <[email protected]> wrote:
>
> On Fri, 2024-06-14 at 10:10 +0200, Daniel Gustafsson wrote:
>>> On 14 Jun 2024, at 10:06, Laurenz Albe <[email protected]> wrote:
>>
>>> So you think we should ignore that compiler warning?
>>
>> We already do using this in meson.build:
>>
>> # Similarly disable useless truncation warnings from gcc 8+
>> 'format-truncation',
>> 'stringop-truncation',
>
> Right; and I see that -Wno-stringop-truncation is also set if you build
> with "make". So your patch is good.
Thanks for looking! I will apply it backpatched all the way down as this has
been wrong since 2006.
> I wonder how Winter Loo got to see that warning...
And it would be interesting to know if that was the only warning, since error.c
in ECPG performs the exact same string copy.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: may be a buffer overflow problem
2024-06-14 07:38 may be a buffer overflow problem Winter Loo <[email protected]>
2024-06-14 07:55 ` Re: may be a buffer overflow problem Daniel Gustafsson <[email protected]>
@ 2024-06-14 14:39 ` Tom Lane <[email protected]>
1 sibling, 0 replies; 15+ messages in thread
From: Tom Lane @ 2024-06-14 14:39 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; +Cc: Winter Loo <[email protected]>; pgsql-hackers <[email protected]>
Daniel Gustafsson <[email protected]> writes:
> This is indeed buggy and need to take the length into account, as per the
> attached. This only happens when in the undocumented regression test debug
> mode which may be why it's gone unnoticed.
Seeing that this code is exercised thousands of times a day in the
regression tests and has had a failure rate of exactly zero (and
yes, the tests do check the output), there must be some reason
why it's okay.
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: may be a buffer overflow problem
2024-06-14 07:38 may be a buffer overflow problem Winter Loo <[email protected]>
@ 2024-06-14 08:04 ` Laurenz Albe <[email protected]>
1 sibling, 0 replies; 15+ messages in thread
From: Laurenz Albe @ 2024-06-14 08:04 UTC (permalink / raw)
To: Winter Loo <[email protected]>; [email protected]
On Fri, 2024-06-14 at 15:38 +0800, Winter Loo wrote:
> I am using gcc version 11.3.0 to compile postgres source code. Gcc complains about the following line:
>
> strncpy(sqlca->sqlstate, "YE001", sizeof(sqlca->sqlstate));
>
> with error as:
>
> misc.c:529:17: error: ‘strncpy’ output truncated before terminating nul
> copying 5 bytes from a string of the same length [-Werror=stringop-truncation]
>
> I find the definition of `sqlca->sqlstate` and it has only 5 bytes. When the statement
>
> strncpy(sqlca->sqlstate, "YE001", sizeof(sqlca->sqlstate));
>
> get executed, `sqlca->sqlstate` will have no '\0' byte which makes me anxious
> when someone prints that as a string. Indeed, I found the code(in src/interfaces/ecpg/ecpglib/misc.c) does that,
>
> fprintf(debugstream, "[NO_PID]: sqlca: code: %ld, state: %s\n",
> sqlca->sqlcode, sqlca->sqlstate);
>
> Is there any chance to fix the code?
I agree that that is wrong.
We could either use memcpy() to avoid the warning and use a format string
with %.*s in fprintf(), or we could make the "sqlstate" one byte longer.
I think that the second option would be less error-prone.
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2024-06-14 14:39 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2016-12-13 06:07 [PATCH 4/7] Fix a bug of insertion into an internal partition. amit <[email protected]>
2016-12-13 06:07 [PATCH 4/5] Fix a bug of insertion into an internal partition. amit <[email protected]>
2016-12-13 06:07 [PATCH 4/5] Fix a bug of insertion into an internal partition. amit <[email protected]>
2016-12-13 06:07 [PATCH 4/7] Fix a bug of insertion into an internal partition. amit <[email protected]>
2016-12-13 06:07 [PATCH 4/7] Fix a bug of insertion into an internal partition. amit <[email protected]>
2016-12-13 06:07 [PATCH 4/7] Fix a bug of insertion into an internal partition. amit <[email protected]>
2020-06-07 21:58 [PATCH v5 3/3] Implement CLUSTER of partitioned table.. Justin Pryzby <[email protected]>
2024-06-14 07:38 may be a buffer overflow problem Winter Loo <[email protected]>
2024-06-14 07:55 ` Re: may be a buffer overflow problem Daniel Gustafsson <[email protected]>
2024-06-14 08:06 ` Re: may be a buffer overflow problem Laurenz Albe <[email protected]>
2024-06-14 08:10 ` Re: may be a buffer overflow problem Daniel Gustafsson <[email protected]>
2024-06-14 08:29 ` Re: may be a buffer overflow problem Laurenz Albe <[email protected]>
2024-06-14 08:39 ` Re: may be a buffer overflow problem Daniel Gustafsson <[email protected]>
2024-06-14 14:39 ` Re: may be a buffer overflow problem Tom Lane <[email protected]>
2024-06-14 08:04 ` Re: may be a buffer overflow problem Laurenz Albe <[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