hi.
* The attached patch makes foreign keys with PERIOD fail if any of the
foreign key columns is "generated columns".
* The following queries will cause segmentation fault. not sure the
best way to fix it. the reason
in LINE: numpks = transformColumnNameList(RelationGetRelid(pkrel),
fkconstraint->pk_attrs, pkattnum, pktypoid);
begin;
drop table if exists temporal3,temporal_fk_rng2rng;
CREATE TABLE temporal3 (id int4range,valid_at tsrange,
CONSTRAINT temporal3_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS));
CREATE TABLE temporal_fk_rng2rng (
id int4range,valid_at tsrange,parent_id int4range,
CONSTRAINT temporal_fk_rng2rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS),
CONSTRAINT temporal_fk_rng2rng_fk FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES temporal3 (id, valid_at)
);
* change the function FindFKComparisonOperators's "eqstrategy" to
make pg_constraint record correct {conpfeqop,conppeqop,conffeqop}.
* fix the ON DELETE SET NULL/DEFAULT (columnlist). Now the following
queries error will be more consistent.
ALTER TABLE temporal_fk_rng2rng DROP CONSTRAINT temporal_fk_rng2rng_fk,
ADD CONSTRAINT temporal_fk_rng2rng_fk
FOREIGN KEY (parent_id, PERIOD valid_at) REFERENCES temporal_rng
ON DELETE SET DEFAULT(valid_at);
--ON DELETE SET NULL(valid_at);
* refactor restrict_cascading_range function.
* you did if (numfks != numpks) before if (is_temporal) {numfks +=
1;}, So I changed the code order to make the error report more
consistent.
anyway, I put it in one patch. please check the attached.
From 7dc2194f9bd46cfee7cfc774a2e52a2b64d465ea Mon Sep 17 00:00:00 2001
From: pgaddict <[email protected]>
Date: Sun, 29 Oct 2023 14:42:57 +0800
Subject: [PATCH v1 1/1] various small fixes.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* The attached patch makes foreign keys with PERIOD fail if any of the foreign key columns is "generated columns".
* change the function FindFKComparisonOperators's "eqstrategy" to make pg_constraint record correct {conpfeqop,conppeqop,conffeqop}.
* refactor restrict_cascading_range function.
* first (is_temporal) {numfks += 1;} then (numfks != numpks) validation.
* variable comment fix.
---
src/backend/commands/tablecmds.c | 96 ++++++++++++++++-------------
src/backend/utils/adt/ri_triggers.c | 32 +++++-----
2 files changed, 69 insertions(+), 59 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5ae0f113..86e99c81 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -522,7 +522,7 @@ static ObjectAddress addFkRecurseReferenced(List **wqueue, Constraint *fkconstra
bool is_temporal);
static void validateFkOnDeleteSetColumns(int numfks, const int16 *fkattnums,
int numfksetcols, const int16 *fksetcolsattnums,
- List *fksetcols);
+ const int16 *fkperiodattnums, List *fksetcols);
static void addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint,
Relation rel, Relation pkrel, Oid indexOid, Oid parentConstr,
int numfks, int16 *pkattnum, int16 *fkattnum,
@@ -10292,6 +10292,7 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
fkdelsetcols, NULL);
validateFkOnDeleteSetColumns(numfks, fkattnum,
numfkdelsetcols, fkdelsetcols,
+ fkperiodattnums,
fkconstraint->fk_del_set_cols);
/*
@@ -10325,6 +10326,43 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
opclasses);
}
+ /*
+ * On the strength of a previous constraint, we might avoid scanning
+ * tables to validate this one. See below.
+ */
+ old_check_ok = (fkconstraint->old_conpfeqop != NIL);
+ Assert(!old_check_ok || numfks == list_length(fkconstraint->old_conpfeqop));
+
+ for (i = 0; i < numpks; i++)
+ {
+ FindFKComparisonOperators(
+ fkconstraint, tab, i, fkattnum,
+ &old_check_ok, &old_pfeqop_item,
+ pktypoid[i], fktypoid[i], opclasses[i],
+ is_temporal, false,
+ &pfeqoperators[i], &ppeqoperators[i], &ffeqoperators[i]);
+ }
+ /* set pfeq, ppeq, ffeq operators for withoutoverlaps constraint.
+ * this also assume overlaps is the last key columns in constraint.
+ *
+ */
+ if (is_temporal)
+ {
+ pkattnum[numpks] = pkperiodattnums[0];
+ pktypoid[numpks] = pkperiodtypoids[0];
+ fkattnum[numpks] = fkperiodattnums[0];
+ fktypoid[numpks] = fkperiodtypoids[0];
+
+ FindFKComparisonOperators(
+ fkconstraint, tab, numpks, fkattnum,
+ &old_check_ok, &old_pfeqop_item,
+ pkperiodtypoids[0], fkperiodtypoids[0], opclasses[numpks],
+ is_temporal, true,
+ &pfeqoperators[numpks], &ppeqoperators[numpks], &ffeqoperators[numpks]);
+ numfks += 1;
+ numpks += 1;
+ }
+
/*
* Now we can check permissions.
*/
@@ -10371,38 +10409,6 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
(errcode(ERRCODE_INVALID_FOREIGN_KEY),
errmsg("number of referencing and referenced columns for foreign key disagree")));
- /*
- * On the strength of a previous constraint, we might avoid scanning
- * tables to validate this one. See below.
- */
- old_check_ok = (fkconstraint->old_conpfeqop != NIL);
- Assert(!old_check_ok || numfks == list_length(fkconstraint->old_conpfeqop));
-
- for (i = 0; i < numpks; i++)
- {
- FindFKComparisonOperators(
- fkconstraint, tab, i, fkattnum,
- &old_check_ok, &old_pfeqop_item,
- pktypoid[i], fktypoid[i], opclasses[i],
- is_temporal, false,
- &pfeqoperators[i], &ppeqoperators[i], &ffeqoperators[i]);
- }
- if (is_temporal) {
- pkattnum[numpks] = pkperiodattnums[0];
- pktypoid[numpks] = pkperiodtypoids[0];
- fkattnum[numpks] = fkperiodattnums[0];
- fktypoid[numpks] = fkperiodtypoids[0];
-
- FindFKComparisonOperators(
- fkconstraint, tab, numpks, fkattnum,
- &old_check_ok, &old_pfeqop_item,
- pkperiodtypoids[0], fkperiodtypoids[0], opclasses[numpks],
- is_temporal, true,
- &pfeqoperators[numpks], &ppeqoperators[numpks], &ffeqoperators[numpks]);
- numfks += 1;
- numpks += 1;
- }
-
/*
* Create all the constraint and trigger objects, recursing to partitions
* as necessary. First handle the referenced side.
@@ -10455,11 +10461,13 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
void
validateFkOnDeleteSetColumns(int numfks, const int16 *fkattnums,
int numfksetcols, const int16 *fksetcolsattnums,
- List *fksetcols)
+ const int16 *fkperiodattnums, List *fksetcols)
{
for (int i = 0; i < numfksetcols; i++)
{
int16 setcol_attnum = fksetcolsattnums[i];
+ /* assume only one PERIOD key column in foreign key */
+ int16 fkperiod_attnum = fkperiodattnums[0];
bool seen = false;
for (int j = 0; j < numfks; j++)
@@ -10469,6 +10477,14 @@ validateFkOnDeleteSetColumns(int numfks, const int16 *fkattnums,
seen = true;
break;
}
+ if (fkperiod_attnum == setcol_attnum)
+ {
+ char *col = strVal(list_nth(fksetcols, i));
+
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+ errmsg("column \"%s\" referenced in ON DELETE SET action cannot be PERIOD ", col)));
+ }
}
if (!seen)
@@ -11502,7 +11518,7 @@ FindFKComparisonOperators(Constraint *fkconstraint,
* For the non-overlaps parts, we want either RTEqualStrategyNumber (without btree_gist)
* or BTEqualStrategyNumber (with btree_gist). We'll try the latter first.
*/
- eqstrategy = for_overlaps ? RTOverlapStrategyNumber : BTEqualStrategyNumber;
+ eqstrategy = for_overlaps ? RTOverlapStrategyNumber : RTEqualStrategyNumber;
}
else
{
@@ -11521,19 +11537,11 @@ FindFKComparisonOperators(Constraint *fkconstraint,
/*
* There had better be a primary equality operator for the index.
* We'll use it for PK = PK comparisons.
+ * This apply to gist and btree index method.
*/
ppeqop = get_opfamily_member(opfamily, opcintype, opcintype,
eqstrategy);
- /* Fall back to RTEqualStrategyNumber for temporal overlaps */
- if (is_temporal && !for_overlaps && !OidIsValid(ppeqop))
- {
- eqstrategy = RTEqualStrategyNumber;
- ppeqop = get_opfamily_member(opfamily, opcintype, opcintype,
- eqstrategy);
- }
-
-
if (!OidIsValid(ppeqop))
elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
eqstrategy, opcintype, opcintype, opfamily);
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index d0027918..08b46536 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -245,7 +245,7 @@ static void ri_ReportViolation(const RI_ConstraintInfo *riinfo,
int queryno, bool partgone) pg_attribute_noreturn();
static bool fpo_targets_pk_range(const ForPortionOfState *tg_temporal,
const RI_ConstraintInfo *riinfo);
-static Datum restrict_cascading_range(const ForPortionOfState *tg_temporal,
+static Datum get_portion_intersect_range(const ForPortionOfState *tg_temporal,
const RI_ConstraintInfo *riinfo,
TupleTableSlot *oldslot);
@@ -816,7 +816,7 @@ ri_restrict(TriggerData *trigdata, bool is_no_action)
if (trigdata->tg_temporal)
{
targetRangeParam = riinfo->nkeys - 1;
- targetRange = restrict_cascading_range(trigdata->tg_temporal, riinfo, oldslot);
+ targetRange = get_portion_intersect_range(trigdata->tg_temporal, riinfo, oldslot);
}
ri_PerformCheck(riinfo, &qkey, qplan,
@@ -1435,7 +1435,7 @@ TRI_FKey_cascade_del(PG_FUNCTION_ARGS)
* Don't delete than more than the PK's duration,
* trimmed by an original FOR PORTION OF if necessary.
*/
- targetRange = restrict_cascading_range(trigdata->tg_temporal, riinfo, oldslot);
+ targetRange = get_portion_intersect_range(trigdata->tg_temporal, riinfo, oldslot);
if (SPI_connect() != SPI_OK_CONNECT)
elog(ERROR, "SPI_connect failed");
@@ -1480,7 +1480,7 @@ TRI_FKey_cascade_del(PG_FUNCTION_ARGS)
quoteOneName(attname,
RIAttName(fk_rel, riinfo->fk_attnums[i]));
- sprintf(paramname, "$%d", i + 1);
+ snprintf(paramname, sizeof(paramname), "$%d", i + 1);
ri_GenerateQual(&querybuf, querysep,
paramname, pk_type,
riinfo->pf_eq_oprs[i],
@@ -1596,7 +1596,7 @@ TRI_FKey_cascade_upd(PG_FUNCTION_ARGS)
* Don't delete than more than the PK's duration,
* trimmed by an original FOR PORTION OF if necessary.
*/
- targetRange = restrict_cascading_range(trigdata->tg_temporal, riinfo, oldslot);
+ targetRange = get_portion_intersect_range(trigdata->tg_temporal, riinfo, oldslot);
if (SPI_connect() != SPI_OK_CONNECT)
elog(ERROR, "SPI_connect failed");
@@ -1621,9 +1621,11 @@ TRI_FKey_cascade_upd(PG_FUNCTION_ARGS)
* UPDATE [ONLY] <fktable>
* FOR PORTION OF $fkatt FROM lower(${2n+1}) TO upper(${2n+1})
* SET fkatt1 = $1, [, ...]
- * WHERE $n = fkatt1 [AND ...]
+ * WHERE $n + 1 = fkatt1 [AND ...]
* The type id's for the $ parameters are those of the
- * corresponding PK attributes. Note that we are assuming
+ * corresponding PK attributes. ri_PerformCheck need fillin
+ * oldslot and newslot key values. so we put targetRange(Portion)
+ * to ${2n+1}. Note that we are assuming
* there is an assignment cast from the PK to the FK type;
* else the parser will fail.
* ----------
@@ -1658,7 +1660,7 @@ TRI_FKey_cascade_upd(PG_FUNCTION_ARGS)
"%s %s = $%d",
querysep, attname, i + 1);
- sprintf(paramname, "$%d", j + 1);
+ snprintf(paramname, sizeof(paramname), "$%d", j + 1);
ri_GenerateQual(&qualbuf, qualsep,
paramname, pk_type,
riinfo->pf_eq_oprs[i],
@@ -1793,7 +1795,7 @@ tri_set(TriggerData *trigdata, bool is_set_null, int tgkind)
* Don't SET NULL/DEFAULT than more than the PK's duration,
* trimmed by an original FOR PORTION OF if necessary.
*/
- targetRange = restrict_cascading_range(trigdata->tg_temporal, riinfo, oldslot);
+ targetRange = get_portion_intersect_range(trigdata->tg_temporal, riinfo, oldslot);
if (SPI_connect() != SPI_OK_CONNECT)
elog(ERROR, "SPI_connect failed");
@@ -1910,7 +1912,7 @@ tri_set(TriggerData *trigdata, bool is_set_null, int tgkind)
quoteOneName(attname,
RIAttName(fk_rel, riinfo->fk_attnums[i]));
- sprintf(paramname, "$%d", i + 1);
+ snprintf(paramname, sizeof(paramname), "$%d", i + 1);
ri_GenerateQual(&querybuf, qualsep,
paramname, pk_type,
riinfo->pf_eq_oprs[i],
@@ -3877,17 +3879,17 @@ fpo_targets_pk_range(const ForPortionOfState *tg_temporal, const RI_ConstraintIn
}
/*
- * restrict_cascading_range -
+ * get_portion_intersect_range -
*
* Returns a Datum of RangeTypeP holding the appropriate timespan
* to target child records when we CASCADE/SET NULL/SET DEFAULT.
*
- * In a normal UPDATE/DELETE this should be the parent's own valid time,
- * but if there was a FOR PORTION OF clause, then we should use that to
- * trim down the parent's span further.
+ * In a normal UPDATE/DELETE this should be the parent's own valid range,
+ * but if there was a FOR PORTION OF clause, then we should
+ * get the intersect of old valid range and FOR PORTION OF clause's range.
*/
static Datum
-restrict_cascading_range(const ForPortionOfState *tg_temporal, const RI_ConstraintInfo *riinfo, TupleTableSlot *oldslot)
+get_portion_intersect_range(const ForPortionOfState *tg_temporal, const RI_ConstraintInfo *riinfo, TupleTableSlot *oldslot)
{
Datum pkRecordRange;
bool isnull;
--
2.34.1
