public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 3/6] Be less verbose on variable names 26+ messages / 4 participants [nested] [flat]
* [PATCH 3/6] Be less verbose on variable names @ 2019-11-28 22:30 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Alvaro Herrera @ 2019-11-28 22:30 UTC (permalink / raw) --- src/backend/commands/typecmds.c | 23 +++++++++------------- src/backend/utils/adt/pg_upgrade_support.c | 4 ++-- src/include/catalog/binary_upgrade.h | 4 ++-- 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c index 1b012c9cad..4ca2d3364b 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -85,8 +85,8 @@ typedef struct /* Potentially set by pg_upgrade_support functions */ Oid binary_upgrade_next_array_pg_type_oid = InvalidOid; -Oid binary_upgrade_next_multirange_pg_type_oid = InvalidOid; -Oid binary_upgrade_next_multirange_array_pg_type_oid = InvalidOid; +Oid binary_upgrade_next_mrng_pg_type_oid = InvalidOid; +Oid binary_upgrade_next_mrng_array_pg_type_oid = InvalidOid; static void makeRangeConstructors(const char *name, Oid namespace, Oid rangeOid, Oid subtype); @@ -1529,13 +1529,9 @@ DefineRange(CreateRangeStmt *stmt) /* alignment must be 'i' or 'd' for ranges */ alignment = (subtypalign == 'd') ? 'd' : 'i'; - /* Allocate OID for array type */ + /* Allocate OID for array type, its multirange and its multirange array */ rangeArrayOid = AssignTypeArrayOid(); - - /* Allocate OID for multirange type */ multirangeOid = AssignTypeMultirangeOid(); - - /* Allocate OID for multirange array type */ multirangeArrayOid = AssignTypeMultirangeArrayOid(); /* Create the pg_type entry */ @@ -1574,7 +1570,6 @@ DefineRange(CreateRangeStmt *stmt) Assert(typoid == address.objectId); /* Create the multirange that goes with it */ - multirangeTypeName = makeMultirangeTypeName(typeName, typeNamespace); mltrngaddress = @@ -2313,13 +2308,13 @@ AssignTypeMultirangeOid(void) /* Use binary-upgrade override for pg_type.oid? */ if (IsBinaryUpgrade) { - if (!OidIsValid(binary_upgrade_next_multirange_pg_type_oid)) + if (!OidIsValid(binary_upgrade_next_mrng_pg_type_oid)) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("pg_type multirange OID value not set when in binary upgrade mode"))); - type_multirange_oid = binary_upgrade_next_multirange_pg_type_oid; - binary_upgrade_next_multirange_pg_type_oid = InvalidOid; + type_multirange_oid = binary_upgrade_next_mrng_pg_type_oid; + binary_upgrade_next_mrng_pg_type_oid = InvalidOid; } else { @@ -2346,13 +2341,13 @@ AssignTypeMultirangeArrayOid(void) /* Use binary-upgrade override for pg_type.oid? */ if (IsBinaryUpgrade) { - if (!OidIsValid(binary_upgrade_next_multirange_array_pg_type_oid)) + if (!OidIsValid(binary_upgrade_next_mrng_array_pg_type_oid)) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("pg_type multirange array OID value not set when in binary upgrade mode"))); - type_multirange_array_oid = binary_upgrade_next_multirange_array_pg_type_oid; - binary_upgrade_next_multirange_array_pg_type_oid = InvalidOid; + type_multirange_array_oid = binary_upgrade_next_mrng_array_pg_type_oid; + binary_upgrade_next_mrng_array_pg_type_oid = InvalidOid; } else { diff --git a/src/backend/utils/adt/pg_upgrade_support.c b/src/backend/utils/adt/pg_upgrade_support.c index d980b96f48..418c26c81b 100644 --- a/src/backend/utils/adt/pg_upgrade_support.c +++ b/src/backend/utils/adt/pg_upgrade_support.c @@ -57,7 +57,7 @@ binary_upgrade_set_next_multirange_pg_type_oid(PG_FUNCTION_ARGS) Oid typoid = PG_GETARG_OID(0); CHECK_IS_BINARY_UPGRADE; - binary_upgrade_next_multirange_pg_type_oid = typoid; + binary_upgrade_next_mrng_pg_type_oid = typoid; PG_RETURN_VOID(); } @@ -68,7 +68,7 @@ binary_upgrade_set_next_multirange_array_pg_type_oid(PG_FUNCTION_ARGS) Oid typoid = PG_GETARG_OID(0); CHECK_IS_BINARY_UPGRADE; - binary_upgrade_next_multirange_array_pg_type_oid = typoid; + binary_upgrade_next_mrng_array_pg_type_oid = typoid; PG_RETURN_VOID(); } diff --git a/src/include/catalog/binary_upgrade.h b/src/include/catalog/binary_upgrade.h index 2b6e87bb84..ba132ddf23 100644 --- a/src/include/catalog/binary_upgrade.h +++ b/src/include/catalog/binary_upgrade.h @@ -16,8 +16,8 @@ extern PGDLLIMPORT Oid binary_upgrade_next_pg_type_oid; extern PGDLLIMPORT Oid binary_upgrade_next_array_pg_type_oid; -extern PGDLLIMPORT Oid binary_upgrade_next_multirange_pg_type_oid; -extern PGDLLIMPORT Oid binary_upgrade_next_multirange_array_pg_type_oid; +extern PGDLLIMPORT Oid binary_upgrade_next_mrng_pg_type_oid; +extern PGDLLIMPORT Oid binary_upgrade_next_mrng_array_pg_type_oid; extern PGDLLIMPORT Oid binary_upgrade_next_toast_pg_type_oid; extern PGDLLIMPORT Oid binary_upgrade_next_heap_pg_class_oid; -- 2.20.1 --+QahgC5+KEYLbs62 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0004-Protect-comment-against-pgindent.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: simplifying foreign key/RI checks @ 2022-01-18 06:30 Amit Langote <[email protected]> 2022-03-14 08:33 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Amit Langote @ 2022-01-18 06:30 UTC (permalink / raw) To: Zhihong Yu <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Alvaro Herrera <[email protected]>; vignesh C <[email protected]>; Kyotaro Horiguchi <[email protected]>; PostgreSQL Developers <[email protected]> Thanks for the review. On Tue, Dec 21, 2021 at 5:54 PM Zhihong Yu <[email protected]> wrote: > Hi, > > + int lockflags = 0; > + TM_Result test; > + > + lockflags = TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS; > > The above assignment can be meged with the line where variable lockflags is declared. Sure. > + GetUserIdAndSecContext(&save_userid, &save_sec_context); > > save_userid -> saved_userid > save_sec_context -> saved_sec_context I agree that's better though I guess I had kept the names as they were in other functions. Fixed nevertheless. > + * the transaction-snapshot mode. If we didn't push one already, do > > didn't push -> haven't pushed Done. > For ri_PerformCheck(): > > + bool source_is_pk = true; > > It seems the value of source_is_pk doesn't change - the value true can be plugged into ri_ExtractValues() calls directly. OK, done. v13 is attached. -- Amit Langote EDB: http://www.enterprisedb.com Attachments: [application/octet-stream] v13-0002-Avoid-using-SPI-for-some-RI-checks.patch (40.0K, ../../CA+HiwqEXXzqWa=RE90Ce2=R9B0vggFrvGZYrupVT3GeY-XXR-g@mail.gmail.com/2-v13-0002-Avoid-using-SPI-for-some-RI-checks.patch) download | inline diff: From e1c250eb3529e15334fc91138ea4b0e86a7e936a Mon Sep 17 00:00:00 2001 From: amitlan <[email protected]> Date: Tue, 12 Jan 2021 14:17:31 +0900 Subject: [PATCH v13 2/2] Avoid using SPI for some RI checks This modifies the subroutines called by RI trigger functions that want to check if a given referenced value exists in the referenced relation to simply scan the foreign key constraint's unique index. That replaces the current way of issuing a `SELECT 1 FROM referenced_relation WHERE ref_key = $1` query through SPI to do the same. This saves a lot of work, especially when inserting into or updating a referencing relation. This rewrite allows to fix a PK row visibility bug caused by a partition descriptor hack which requires ActiveSnapshot to be set to come up with the correct set of partitions for the RI query running under REPEATABLE READ isolation. We now set that snapshot indepedently of the snapshot to be used by the PK index scan, so the two no longer interfere. The buggy output in src/test/isolation/expected/fk-snapshot.out of the relevant test case has been corrected. --- src/backend/executor/execPartition.c | 160 +++++- src/backend/executor/nodeLockRows.c | 160 +++--- src/backend/utils/adt/ri_triggers.c | 545 +++++++++++--------- src/include/executor/execPartition.h | 6 + src/include/executor/executor.h | 9 + src/test/isolation/expected/fk-snapshot.out | 4 +- src/test/isolation/specs/fk-snapshot.spec | 5 +- 7 files changed, 568 insertions(+), 321 deletions(-) diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 90ed1485d1..72ee019330 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -175,8 +175,9 @@ static void FormPartitionKeyDatum(PartitionDispatch pd, EState *estate, Datum *values, bool *isnull); -static int get_partition_for_tuple(PartitionDispatch pd, Datum *values, - bool *isnull); +static int get_partition_for_tuple(PartitionKey key, + PartitionDesc partdesc, + Datum *values, bool *isnull); static char *ExecBuildSlotPartitionKeyDescription(Relation rel, Datum *values, bool *isnull, @@ -310,7 +311,9 @@ ExecFindPartition(ModifyTableState *mtstate, * these values, error out. */ if (partdesc->nparts == 0 || - (partidx = get_partition_for_tuple(dispatch, values, isnull)) < 0) + (partidx = get_partition_for_tuple(dispatch->key, + dispatch->partdesc, + values, isnull)) < 0) { char *val_desc; @@ -1240,12 +1243,12 @@ FormPartitionKeyDatum(PartitionDispatch pd, * found or -1 if none found. */ static int -get_partition_for_tuple(PartitionDispatch pd, Datum *values, bool *isnull) +get_partition_for_tuple(PartitionKey key, + PartitionDesc partdesc, + Datum *values, bool *isnull) { int bound_offset; int part_index = -1; - PartitionKey key = pd->key; - PartitionDesc partdesc = pd->partdesc; PartitionBoundInfo boundinfo = partdesc->boundinfo; /* Route as appropriate based on partitioning strategy. */ @@ -1337,6 +1340,151 @@ get_partition_for_tuple(PartitionDispatch pd, Datum *values, bool *isnull) return part_index; } +/* + * find_leaf_part_for_key + * Finds the leaf partition of a partitioned table 'root_rel' that might + * contain the specified key tuple containing a subset of the table's + * columns (including all of the partition key columns) + * + * 'key_natts' specifies the number columns contained in the key, + * 'key_attnums' their attribute numbers as defined in 'root_rel', and + * 'key_vals' and 'key_nulls' specify the key tuple. + * + * Returns NULL if no leaf partition is found for the key. Caller must close + * the relation. + * + * This works because the unique key defined on the root relation is required + * to contain the partition key columns of all of the ancestors that lead up to + * a given leaf partition. + */ +Relation +find_leaf_part_for_key(Relation root_rel, int key_natts, + const AttrNumber *key_attnums, + Datum *key_vals, char *key_nulls, + Oid root_idxoid, int lockmode, + Oid *leaf_idxoid) +{ + Relation rel = root_rel; + Oid constr_idxoid = root_idxoid; + + *leaf_idxoid = InvalidOid; + + /* + * Descend through partitioned parents to find the leaf partition that + * would accept a row with the provided key values, starting with the root + * parent. + */ + while (true) + { + PartitionKey partkey = RelationGetPartitionKey(rel); + PartitionDirectory partdir; + PartitionDesc partdesc; + Datum partkey_vals[PARTITION_MAX_KEYS]; + bool partkey_isnull[PARTITION_MAX_KEYS]; + AttrNumber *root_partattrs = partkey->partattrs; + int i, + j; + int partidx; + Oid partoid; + bool is_leaf; + + /* + * Collect partition key values from the unique key. + * + * Because we only have the root table's copy of pk_attnums, must map + * any non-root table's partition key attribute numbers to the root + * table's. + */ + if (rel != root_rel) + { + /* + * map->attnums will contain root table attribute numbers for each + * attribute of the current partitioned relation. + */ + AttrMap *map = build_attrmap_by_name_if_req(RelationGetDescr(root_rel), + RelationGetDescr(rel)); + + if (map) + { + root_partattrs = palloc(partkey->partnatts * + sizeof(AttrNumber)); + for (i = 0; i < partkey->partnatts; i++) + { + AttrNumber partattno = partkey->partattrs[i]; + + root_partattrs[i] = map->attnums[partattno - 1]; + } + + free_attrmap(map); + } + } + + /* + * Referenced key specification does not allow expressions, so there + * would not be expressions in the partition keys either. + */ + Assert(partkey->partexprs == NIL); + for (i = 0, j = 0; i < partkey->partnatts; i++) + { + int k; + + for (k = 0; k < key_natts; k++) + { + if (root_partattrs[i] == key_attnums[k]) + { + partkey_vals[j] = key_vals[k]; + partkey_isnull[j] = (key_nulls[k] == 'n' ? true : false); + j++; + break; + } + } + } + /* Had better have found values for all of the partition keys. */ + Assert(j == partkey->partnatts); + + if (root_partattrs != partkey->partattrs) + pfree(root_partattrs); + + /* Get the PartitionDesc using the partition directory machinery. */ + partdir = CreatePartitionDirectory(CurrentMemoryContext, true); + partdesc = PartitionDirectoryLookup(partdir, rel); + + /* Find the partition for the key. */ + partidx = get_partition_for_tuple(partkey, partdesc, + partkey_vals, partkey_isnull); + Assert(partidx < 0 || partidx < partdesc->nparts); + + /* done using the partition directory */ + DestroyPartitionDirectory(partdir); + + /* close any intermediate parents we opened */ + if (rel != root_rel) + table_close(rel, NoLock); + + /* No partition found. */ + if (partidx < 0) + return NULL; + + partoid = partdesc->oids[partidx]; + rel = table_open(partoid, lockmode); + constr_idxoid = index_get_partition(rel, constr_idxoid); + + /* + * Return if the partition is a leaf, else find its partition in the + * next iteration. + */ + is_leaf = partdesc->is_leaf[partidx]; + if (is_leaf) + { + *leaf_idxoid = constr_idxoid; + return rel; + } + } + + Assert(false); + return NULL; +} + /* * ExecBuildSlotPartitionKeyDescription * diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c index 1a9dab25dd..ab54a65e0e 100644 --- a/src/backend/executor/nodeLockRows.c +++ b/src/backend/executor/nodeLockRows.c @@ -79,10 +79,7 @@ lnext: Datum datum; bool isNull; ItemPointerData tid; - TM_FailureData tmfd; LockTupleMode lockmode; - int lockflags = 0; - TM_Result test; TupleTableSlot *markSlot; /* clear any leftover test tuple for this rel */ @@ -179,74 +176,11 @@ lnext: break; } - lockflags = TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS; - if (!IsolationUsesXactSnapshot()) - lockflags |= TUPLE_LOCK_FLAG_FIND_LAST_VERSION; - - test = table_tuple_lock(erm->relation, &tid, estate->es_snapshot, - markSlot, estate->es_output_cid, - lockmode, erm->waitPolicy, - lockflags, - &tmfd); - - switch (test) - { - case TM_WouldBlock: - /* couldn't lock tuple in SKIP LOCKED mode */ - goto lnext; - - case TM_SelfModified: - - /* - * The target tuple was already updated or deleted by the - * current command, or by a later command in the current - * transaction. We *must* ignore the tuple in the former - * case, so as to avoid the "Halloween problem" of repeated - * update attempts. In the latter case it might be sensible - * to fetch the updated tuple instead, but doing so would - * require changing heap_update and heap_delete to not - * complain about updating "invisible" tuples, which seems - * pretty scary (table_tuple_lock will not complain, but few - * callers expect TM_Invisible, and we're not one of them). So - * for now, treat the tuple as deleted and do not process. - */ - goto lnext; - - case TM_Ok: - - /* - * Got the lock successfully, the locked tuple saved in - * markSlot for, if needed, EvalPlanQual testing below. - */ - if (tmfd.traversed) - epq_needed = true; - break; - - case TM_Updated: - if (IsolationUsesXactSnapshot()) - ereport(ERROR, - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), - errmsg("could not serialize access due to concurrent update"))); - elog(ERROR, "unexpected table_tuple_lock status: %u", - test); - break; - - case TM_Deleted: - if (IsolationUsesXactSnapshot()) - ereport(ERROR, - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), - errmsg("could not serialize access due to concurrent update"))); - /* tuple was deleted so don't return it */ - goto lnext; - - case TM_Invisible: - elog(ERROR, "attempted to lock invisible tuple"); - break; - - default: - elog(ERROR, "unrecognized table_tuple_lock status: %u", - test); - } + /* skip tuple if it couldn't be locked */ + if (!ExecLockTableTuple(erm->relation, &tid, markSlot, + estate->es_snapshot, estate->es_output_cid, + lockmode, erm->waitPolicy, &epq_needed)) + goto lnext; /* Remember locked tuple's TID for EPQ testing and WHERE CURRENT OF */ erm->curCtid = tid; @@ -281,6 +215,90 @@ lnext: return slot; } +/* + * ExecLockTableTuple + * Locks tuple with the specified TID in lockmode following given wait + * policy + * + * Returns true if the tuple was successfully locked. Locked tuple is loaded + * into provided slot. + */ +bool +ExecLockTableTuple(Relation relation, ItemPointer tid, TupleTableSlot *slot, + Snapshot snapshot, CommandId cid, + LockTupleMode lockmode, LockWaitPolicy waitPolicy, + bool *epq_needed) +{ + TM_FailureData tmfd; + int lockflags = TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS; + TM_Result test; + + if (!IsolationUsesXactSnapshot()) + lockflags |= TUPLE_LOCK_FLAG_FIND_LAST_VERSION; + + test = table_tuple_lock(relation, tid, snapshot, slot, cid, lockmode, + waitPolicy, lockflags, &tmfd); + + switch (test) + { + case TM_WouldBlock: + /* couldn't lock tuple in SKIP LOCKED mode */ + return false; + + case TM_SelfModified: + /* + * The target tuple was already updated or deleted by the + * current command, or by a later command in the current + * transaction. We *must* ignore the tuple in the former + * case, so as to avoid the "Halloween problem" of repeated + * update attempts. In the latter case it might be sensible + * to fetch the updated tuple instead, but doing so would + * require changing heap_update and heap_delete to not + * complain about updating "invisible" tuples, which seems + * pretty scary (table_tuple_lock will not complain, but few + * callers expect TM_Invisible, and we're not one of them). So + * for now, treat the tuple as deleted and do not process. + */ + return false; + + case TM_Ok: + /* + * Got the lock successfully, the locked tuple saved in + * slot for EvalPlanQual, if asked by the caller. + */ + if (tmfd.traversed && epq_needed) + *epq_needed = true; + break; + + case TM_Updated: + if (IsolationUsesXactSnapshot()) + ereport(ERROR, + (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), + errmsg("could not serialize access due to concurrent update"))); + elog(ERROR, "unexpected table_tuple_lock status: %u", + test); + break; + + case TM_Deleted: + if (IsolationUsesXactSnapshot()) + ereport(ERROR, + (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), + errmsg("could not serialize access due to concurrent update"))); + /* tuple was deleted so don't return it */ + return false; + + case TM_Invisible: + elog(ERROR, "attempted to lock invisible tuple"); + return false; + + default: + elog(ERROR, "unrecognized table_tuple_lock status: %u", test); + return false; + } + + return true; +} + /* ---------------------------------------------------------------- * ExecInitLockRows * diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index c95cd32402..cbcf0754dc 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -23,22 +23,27 @@ #include "postgres.h" +#include "access/genam.h" #include "access/htup_details.h" +#include "access/skey.h" #include "access/sysattr.h" #include "access/table.h" #include "access/tableam.h" #include "access/xact.h" +#include "catalog/partition.h" #include "catalog/pg_collation.h" #include "catalog/pg_constraint.h" #include "catalog/pg_operator.h" #include "catalog/pg_type.h" #include "commands/trigger.h" +#include "executor/execPartition.h" #include "executor/executor.h" #include "executor/spi.h" #include "lib/ilist.h" #include "miscadmin.h" #include "parser/parse_coerce.h" #include "parser/parse_relation.h" +#include "partitioning/partdesc.h" #include "storage/bufmgr.h" #include "utils/acl.h" #include "utils/builtins.h" @@ -48,6 +53,7 @@ #include "utils/inval.h" #include "utils/lsyscache.h" #include "utils/memutils.h" +#include "utils/partcache.h" #include "utils/rel.h" #include "utils/rls.h" #include "utils/ruleutils.h" @@ -68,19 +74,15 @@ #define RI_KEYS_NONE_NULL 2 /* RI query type codes */ -/* these queries are executed against the PK (referenced) table: */ -#define RI_PLAN_CHECK_LOOKUPPK 1 -#define RI_PLAN_CHECK_LOOKUPPK_FROM_PK 2 -#define RI_PLAN_LAST_ON_PK RI_PLAN_CHECK_LOOKUPPK_FROM_PK /* these queries are executed against the FK (referencing) table: */ -#define RI_PLAN_CASCADE_ONDELETE 3 -#define RI_PLAN_CASCADE_ONUPDATE 4 +#define RI_PLAN_CASCADE_ONDELETE 1 +#define RI_PLAN_CASCADE_ONUPDATE 2 /* For RESTRICT, the same plan can be used for both ON DELETE and ON UPDATE triggers. */ -#define RI_PLAN_RESTRICT 5 -#define RI_PLAN_SETNULL_ONDELETE 6 -#define RI_PLAN_SETNULL_ONUPDATE 7 -#define RI_PLAN_SETDEFAULT_ONDELETE 8 -#define RI_PLAN_SETDEFAULT_ONUPDATE 9 +#define RI_PLAN_RESTRICT 3 +#define RI_PLAN_SETNULL_ONDELETE 4 +#define RI_PLAN_SETNULL_ONUPDATE 5 +#define RI_PLAN_SETDEFAULT_ONDELETE 6 +#define RI_PLAN_SETDEFAULT_ONUPDATE 7 #define MAX_QUOTED_NAME_LEN (NAMEDATALEN*2+3) #define MAX_QUOTED_REL_NAME_LEN (MAX_QUOTED_NAME_LEN*2) @@ -229,8 +231,246 @@ static void ri_ExtractValues(Relation rel, TupleTableSlot *slot, static void ri_ReportViolation(const RI_ConstraintInfo *riinfo, Relation pk_rel, Relation fk_rel, TupleTableSlot *violatorslot, TupleDesc tupdesc, - int queryno, bool partgone) pg_attribute_noreturn(); + bool on_fk, bool partgone) pg_attribute_noreturn(); +/* + * Checks whether a tuple containing the unique key as extracted from the + * tuple provided in 'slot' exists in 'pk_rel'. The key is extracted using the + * constraint's index given in 'riinfo', which is also scanned to check the + * existence of the key. + * + * If 'pk_rel' is a partitioned table, the check is performed on its leaf + * partition that would contain the key. + * + * The provided tuple is either the one being inserted into the referencing + * relation ('fk_rel' is non-NULL), or the one being deleted from the + * referenced relation, that is, 'pk_rel' ('fk_rel' is NULL). + */ +static bool +ri_ReferencedKeyExists(Relation pk_rel, Relation fk_rel, + TupleTableSlot *slot, + const RI_ConstraintInfo *riinfo) +{ + Oid constr_id = riinfo->constraint_id; + Oid idxoid; + Relation idxrel; + Relation leaf_pk_rel = NULL; + int num_pk; + int i; + bool found = false; + const Oid *eq_oprs; + Datum pk_vals[INDEX_MAX_KEYS]; + char pk_nulls[INDEX_MAX_KEYS]; + ScanKeyData skey[INDEX_MAX_KEYS]; + Snapshot snap = InvalidSnapshot; + bool pushed_latest_snapshot = false; + IndexScanDesc scan; + TupleTableSlot *outslot; + Oid saved_userid; + int saved_sec_context; + AclResult aclresult; + + /* + * Extract the unique key from the provided slot and choose the equality + * operators to use when scanning the index below. + */ + if (fk_rel) + { + ri_ExtractValues(fk_rel, slot, riinfo, false, pk_vals, pk_nulls); + /* Use PK = FK equality operator. */ + eq_oprs = riinfo->pf_eq_oprs; + + /* + * May neeed to cast each of the individual values of the foreign key + * to the corresponding PK column's type if the equality operator + * demands it. + */ + for (i = 0; i < riinfo->nkeys; i++) + { + if (pk_nulls[i] != 'n') + { + Oid eq_opr = eq_oprs[i]; + Oid typeid = RIAttType(fk_rel, riinfo->fk_attnums[i]); + RI_CompareHashEntry *entry = ri_HashCompareOp(eq_opr, typeid); + + if (OidIsValid(entry->cast_func_finfo.fn_oid)) + pk_vals[i] = FunctionCall3(&entry->cast_func_finfo, + pk_vals[i], + Int32GetDatum(-1), /* typmod */ + BoolGetDatum(false)); /* implicit coercion */ + } + } + } + else + { + ri_ExtractValues(pk_rel, slot, riinfo, true, pk_vals, pk_nulls); + /* Use PK = PK equality operator. */ + eq_oprs = riinfo->pp_eq_oprs; + } + + /* + * Switch to referenced table's owner to perform the below operations + * as. This matches what ri_PerformCheck() does. + * + * Note that as with queries done by ri_PerformCheck(), the way we select + * the referenced row below effectively bypasses any RLS policies that may + * be present on the referenced table. + */ + GetUserIdAndSecContext(&saved_userid, &saved_sec_context); + SetUserIdAndSecContext(RelationGetForm(pk_rel)->relowner, + saved_sec_context | SECURITY_LOCAL_USERID_CHANGE); + + /* + * Also check that the new user has permissions to look into the schema + * of and SELECT from the referenced table. + */ + aclresult = pg_namespace_aclcheck(RelationGetNamespace(pk_rel), + GetUserId(), ACL_USAGE); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_SCHEMA, + get_namespace_name(RelationGetNamespace(pk_rel))); + aclresult = pg_class_aclcheck(RelationGetRelid(pk_rel), GetUserId(), + ACL_SELECT); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_TABLE, + RelationGetRelationName(pk_rel)); + + /* + * In the case of scanning the PK index for ri_Check_Pk_Match(), we'd like + * to see all rows that could be interesting, even those that would not be + * visible to the transaction snapshot. To do so, force-push the latest + * snapshot. + * + * Also, increment the command counter to make the changes of the current + * command visible in all cases. + */ + CommandCounterIncrement(); + if (fk_rel == NULL) + { + snap = GetLatestSnapshot(); + PushActiveSnapshot(snap); + pushed_latest_snapshot = true; + } + else + { + snap = GetTransactionSnapshot(); + PushActiveSnapshot(snap); + } + + /* + * Open the constraint index to be scanned. + * + * If the target table is partitioned, we must look up the leaf partition + * and its corresponding unique index to search the keys in. + */ + idxoid = get_constraint_index(constr_id); + if (pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + { + Oid leaf_idxoid; + Snapshot mysnap = InvalidSnapshot; + + /* + * XXX the partition descriptor machinery has a hack that assumes that + * the queries originating in this module push the latest snapshot in + * the transaction-snapshot mode. If we haven't push one already, do + * so here. + */ + if (!pushed_latest_snapshot) + { + mysnap = GetLatestSnapshot(); + PushActiveSnapshot(mysnap); + } + + leaf_pk_rel = find_leaf_part_for_key(pk_rel, riinfo->nkeys, + riinfo->pk_attnums, + pk_vals, pk_nulls, + idxoid, RowShareLock, + &leaf_idxoid); + /* + * XXX done fiddling with the partition descriptor machinery so unset + * the active snapshot if we must. + */ + if (mysnap != InvalidSnapshot) + PopActiveSnapshot(); + + /* + * If no suitable leaf partition exists, neither can the key we're + * looking for. + */ + if (leaf_pk_rel == NULL) + { + SetUserIdAndSecContext(saved_userid, saved_sec_context); + PopActiveSnapshot(); + return false; + } + + pk_rel = leaf_pk_rel; + idxoid = leaf_idxoid; + } + idxrel = index_open(idxoid, RowShareLock); + + /* Set up ScanKeys for the index scan. */ + num_pk = IndexRelationGetNumberOfKeyAttributes(idxrel); + for (i = 0; i < num_pk; i++) + { + int pkattno = i + 1; + Oid operator = eq_oprs[i]; + Oid opfamily = idxrel->rd_opfamily[i]; + StrategyNumber strat = get_op_opfamily_strategy(operator, opfamily); + RegProcedure regop = get_opcode(operator); + + /* Initialize the scankey. */ + ScanKeyInit(&skey[i], + pkattno, + strat, + regop, + pk_vals[i]); + + skey[i].sk_collation = idxrel->rd_indcollation[i]; + + /* + * Check for null value. Should not occur, because callers currently + * take care of the cases in which they do occur. + */ + if (pk_nulls[i] == 'n') + skey[i].sk_flags |= SK_ISNULL; + } + + scan = index_beginscan(pk_rel, idxrel, snap, num_pk, 0); + index_rescan(scan, skey, num_pk, NULL, 0); + + /* Look for the tuple, and if found, try to lock it in key share mode. */ + outslot = table_slot_create(pk_rel, NULL); + if (index_getnext_slot(scan, ForwardScanDirection, outslot)) + { + /* + * If we fail to lock the tuple for whatever reason, assume it doesn't + * exist. + */ + found = ExecLockTableTuple(pk_rel, &(outslot->tts_tid), outslot, + snap, + GetCurrentCommandId(false), + LockTupleKeyShare, + LockWaitBlock, NULL); + } + + index_endscan(scan); + ExecDropSingleTupleTableSlot(outslot); + + /* Don't release lock until commit. */ + index_close(idxrel, NoLock); + + /* Close leaf partition relation if any. */ + if (leaf_pk_rel) + table_close(leaf_pk_rel, NoLock); + + /* Restore UID and security context */ + SetUserIdAndSecContext(saved_userid, saved_sec_context); + + PopActiveSnapshot(); + + return found; +} /* * RI_FKey_check - @@ -244,8 +484,6 @@ RI_FKey_check(TriggerData *trigdata) Relation fk_rel; Relation pk_rel; TupleTableSlot *newslot; - RI_QueryKey qkey; - SPIPlanPtr qplan; riinfo = ri_FetchConstraintInfo(trigdata->tg_trigger, trigdata->tg_relation, false); @@ -325,9 +563,9 @@ RI_FKey_check(TriggerData *trigdata) /* * MATCH PARTIAL - all non-null columns must match. (not - * implemented, can be done by modifying the query below - * to only include non-null columns, or by writing a - * special version here) + * implemented, can be done by modifying + * ri_ReferencedKeyExists() to only include non-null + * columns. */ break; #endif @@ -342,74 +580,12 @@ RI_FKey_check(TriggerData *trigdata) break; } - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); - - /* Fetch or prepare a saved plan for the real check */ - ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_CHECK_LOOKUPPK); - - if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL) - { - StringInfoData querybuf; - char pkrelname[MAX_QUOTED_REL_NAME_LEN]; - char attname[MAX_QUOTED_NAME_LEN]; - char paramname[16]; - const char *querysep; - Oid queryoids[RI_MAX_NUMKEYS]; - const char *pk_only; - - /* ---------- - * The query string built is - * SELECT 1 FROM [ONLY] <pktable> x WHERE pkatt1 = $1 [AND ...] - * FOR KEY SHARE OF x - * The type id's for the $ parameters are those of the - * corresponding FK attributes. - * ---------- - */ - initStringInfo(&querybuf); - pk_only = pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ? - "" : "ONLY "; - quoteRelationName(pkrelname, pk_rel); - appendStringInfo(&querybuf, "SELECT 1 FROM %s%s x", - pk_only, pkrelname); - querysep = "WHERE"; - for (int i = 0; i < riinfo->nkeys; i++) - { - Oid pk_type = RIAttType(pk_rel, riinfo->pk_attnums[i]); - Oid fk_type = RIAttType(fk_rel, riinfo->fk_attnums[i]); - - quoteOneName(attname, - RIAttName(pk_rel, riinfo->pk_attnums[i])); - sprintf(paramname, "$%d", i + 1); - ri_GenerateQual(&querybuf, querysep, - attname, pk_type, - riinfo->pf_eq_oprs[i], - paramname, fk_type); - querysep = "AND"; - queryoids[i] = fk_type; - } - appendStringInfoString(&querybuf, " FOR KEY SHARE OF x"); - - /* Prepare and save the plan */ - qplan = ri_PlanCheck(querybuf.data, riinfo->nkeys, queryoids, - &qkey, fk_rel, pk_rel); - } - - /* - * Now check that foreign key exists in PK table - * - * XXX detectNewRows must be true when a partitioned table is on the - * referenced side. The reason is that our snapshot must be fresh in - * order for the hack in find_inheritance_children() to work. - */ - ri_PerformCheck(riinfo, &qkey, qplan, - fk_rel, pk_rel, - NULL, newslot, - pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE, - SPI_OK_SELECT); - - if (SPI_finish() != SPI_OK_FINISH) - elog(ERROR, "SPI_finish failed"); + if (!ri_ReferencedKeyExists(pk_rel, fk_rel, newslot, riinfo)) + ri_ReportViolation(riinfo, + pk_rel, fk_rel, + newslot, + NULL, + true, false); table_close(pk_rel, RowShareLock); @@ -464,81 +640,10 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel, TupleTableSlot *oldslot, const RI_ConstraintInfo *riinfo) { - SPIPlanPtr qplan; - RI_QueryKey qkey; - bool result; - /* Only called for non-null rows */ Assert(ri_NullCheck(RelationGetDescr(pk_rel), oldslot, riinfo, true) == RI_KEYS_NONE_NULL); - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); - - /* - * Fetch or prepare a saved plan for checking PK table with values coming - * from a PK row - */ - ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_CHECK_LOOKUPPK_FROM_PK); - - if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL) - { - StringInfoData querybuf; - char pkrelname[MAX_QUOTED_REL_NAME_LEN]; - char attname[MAX_QUOTED_NAME_LEN]; - char paramname[16]; - const char *querysep; - const char *pk_only; - Oid queryoids[RI_MAX_NUMKEYS]; - - /* ---------- - * The query string built is - * SELECT 1 FROM [ONLY] <pktable> x WHERE pkatt1 = $1 [AND ...] - * FOR KEY SHARE OF x - * The type id's for the $ parameters are those of the - * PK attributes themselves. - * ---------- - */ - initStringInfo(&querybuf); - pk_only = pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ? - "" : "ONLY "; - quoteRelationName(pkrelname, pk_rel); - appendStringInfo(&querybuf, "SELECT 1 FROM %s%s x", - pk_only, pkrelname); - querysep = "WHERE"; - for (int i = 0; i < riinfo->nkeys; i++) - { - Oid pk_type = RIAttType(pk_rel, riinfo->pk_attnums[i]); - - quoteOneName(attname, - RIAttName(pk_rel, riinfo->pk_attnums[i])); - sprintf(paramname, "$%d", i + 1); - ri_GenerateQual(&querybuf, querysep, - attname, pk_type, - riinfo->pp_eq_oprs[i], - paramname, pk_type); - querysep = "AND"; - queryoids[i] = pk_type; - } - appendStringInfoString(&querybuf, " FOR KEY SHARE OF x"); - - /* Prepare and save the plan */ - qplan = ri_PlanCheck(querybuf.data, riinfo->nkeys, queryoids, - &qkey, fk_rel, pk_rel); - } - - /* - * We have a plan now. Run it. - */ - result = ri_PerformCheck(riinfo, &qkey, qplan, - fk_rel, pk_rel, - oldslot, NULL, - true, /* treat like update */ - SPI_OK_SELECT); - - if (SPI_finish() != SPI_OK_FINISH) - elog(ERROR, "SPI_finish failed"); - - return result; + return ri_ReferencedKeyExists(pk_rel, NULL, oldslot, riinfo); } @@ -1602,15 +1707,10 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) errtableconstraint(fk_rel, NameStr(fake_riinfo.conname)))); - /* - * We tell ri_ReportViolation we were doing the RI_PLAN_CHECK_LOOKUPPK - * query, which isn't true, but will cause it to use - * fake_riinfo.fk_attnums as we need. - */ ri_ReportViolation(&fake_riinfo, pk_rel, fk_rel, slot, tupdesc, - RI_PLAN_CHECK_LOOKUPPK, false); + true, false); ExecDropSingleTupleTableSlot(slot); } @@ -1827,7 +1927,7 @@ RI_PartitionRemove_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) fake_riinfo.pk_attnums[i] = i + 1; ri_ReportViolation(&fake_riinfo, pk_rel, fk_rel, - slot, tupdesc, 0, true); + slot, tupdesc, true, true); } if (SPI_finish() != SPI_OK_FINISH) @@ -1964,26 +2064,25 @@ ri_BuildQueryKey(RI_QueryKey *key, const RI_ConstraintInfo *riinfo, { /* * Inherited constraints with a common ancestor can share ri_query_cache - * entries for all query types except RI_PLAN_CHECK_LOOKUPPK_FROM_PK. - * Except in that case, the query processes the other table involved in - * the FK constraint (i.e., not the table on which the trigger has been - * fired), and so it will be the same for all members of the inheritance - * tree. So we may use the root constraint's OID in the hash key, rather - * than the constraint's own OID. This avoids creating duplicate SPI - * plans, saving lots of work and memory when there are many partitions - * with similar FK constraints. + * entries, because each query processes the other table involved in the + * FK constraint (i.e., not the table on which the trigger has been fired), + * and so it will be the same for all members of the inheritance tree. So + * we may use the root constraint's OID in the hash key, rather than the + * constraint's own OID. This avoids creating duplicate SPI plans, saving + * lots of work and memory when there are many partitions with similar FK + * constraints. * * (Note that we must still have a separate RI_ConstraintInfo for each * constraint, because partitions can have different column orders, * resulting in different pk_attnums[] or fk_attnums[] array contents.) * + * (Note also that for a standalone or non-inherited constraint, + * constraint_root_id is same as constraint_id.) + * * We assume struct RI_QueryKey contains no padding bytes, else we'd need * to use memset to clear them. */ - if (constr_queryno != RI_PLAN_CHECK_LOOKUPPK_FROM_PK) - key->constr_id = riinfo->constraint_root_id; - else - key->constr_id = riinfo->constraint_id; + key->constr_id = riinfo->constraint_root_id; key->constr_queryno = constr_queryno; } @@ -2254,19 +2353,11 @@ ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes, RI_QueryKey *qkey, Relation fk_rel, Relation pk_rel) { SPIPlanPtr qplan; - Relation query_rel; + /* There are currently no queries that run on PK table. */ + Relation query_rel = fk_rel; Oid save_userid; int save_sec_context; - /* - * Use the query type code to determine whether the query is run against - * the PK or FK table; we'll do the check as that table's owner - */ - if (qkey->constr_queryno <= RI_PLAN_LAST_ON_PK) - query_rel = pk_rel; - else - query_rel = fk_rel; - /* Switch to proper UID to perform check as */ GetUserIdAndSecContext(&save_userid, &save_sec_context); SetUserIdAndSecContext(RelationGetForm(query_rel)->relowner, @@ -2299,9 +2390,9 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, TupleTableSlot *oldslot, TupleTableSlot *newslot, bool detectNewRows, int expect_OK) { - Relation query_rel, - source_rel; - bool source_is_pk; + /* There are currently no queries that run on PK table. */ + Relation query_rel = fk_rel, + source_rel = pk_rel; Snapshot test_snapshot; Snapshot crosscheck_snapshot; int limit; @@ -2311,46 +2402,17 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, Datum vals[RI_MAX_NUMKEYS * 2]; char nulls[RI_MAX_NUMKEYS * 2]; - /* - * Use the query type code to determine whether the query is run against - * the PK or FK table; we'll do the check as that table's owner - */ - if (qkey->constr_queryno <= RI_PLAN_LAST_ON_PK) - query_rel = pk_rel; - else - query_rel = fk_rel; - - /* - * The values for the query are taken from the table on which the trigger - * is called - it is normally the other one with respect to query_rel. An - * exception is ri_Check_Pk_Match(), which uses the PK table for both (and - * sets queryno to RI_PLAN_CHECK_LOOKUPPK_FROM_PK). We might eventually - * need some less klugy way to determine this. - */ - if (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK) - { - source_rel = fk_rel; - source_is_pk = false; - } - else - { - source_rel = pk_rel; - source_is_pk = true; - } - /* Extract the parameters to be passed into the query */ if (newslot) { - ri_ExtractValues(source_rel, newslot, riinfo, source_is_pk, - vals, nulls); + ri_ExtractValues(source_rel, newslot, riinfo, true, vals, nulls); if (oldslot) - ri_ExtractValues(source_rel, oldslot, riinfo, source_is_pk, + ri_ExtractValues(source_rel, oldslot, riinfo, true, vals + riinfo->nkeys, nulls + riinfo->nkeys); } else { - ri_ExtractValues(source_rel, oldslot, riinfo, source_is_pk, - vals, nulls); + ri_ExtractValues(source_rel, oldslot, riinfo, true, vals, nulls); } /* @@ -2414,14 +2476,12 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, errhint("This is most likely due to a rule having rewritten the query."))); /* XXX wouldn't it be clearer to do this part at the caller? */ - if (qkey->constr_queryno != RI_PLAN_CHECK_LOOKUPPK_FROM_PK && - expect_OK == SPI_OK_SELECT && - (SPI_processed == 0) == (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK)) + if (expect_OK == SPI_OK_SELECT && SPI_processed != 0) ri_ReportViolation(riinfo, pk_rel, fk_rel, newslot ? newslot : oldslot, NULL, - qkey->constr_queryno, false); + false, false); return SPI_processed != 0; } @@ -2452,9 +2512,9 @@ ri_ExtractValues(Relation rel, TupleTableSlot *slot, /* * Produce an error report * - * If the failed constraint was on insert/update to the FK table, - * we want the key names and values extracted from there, and the error - * message to look like 'key blah is not present in PK'. + * If the failed constraint was on insert/update to the FK table (on_fk is + * true), we want the key names and values extracted from there, and the + * error message to look like 'key blah is not present in PK'. * Otherwise, the attr names and values come from the PK table and the * message looks like 'key blah is still referenced from FK'. */ @@ -2462,22 +2522,20 @@ static void ri_ReportViolation(const RI_ConstraintInfo *riinfo, Relation pk_rel, Relation fk_rel, TupleTableSlot *violatorslot, TupleDesc tupdesc, - int queryno, bool partgone) + bool on_fk, bool partgone) { StringInfoData key_names; StringInfoData key_values; - bool onfk; const int16 *attnums; Oid rel_oid; AclResult aclresult; bool has_perm = true; /* - * Determine which relation to complain about. If tupdesc wasn't passed - * by caller, assume the violator tuple came from there. + * If tupdesc wasn't passed by caller, assume the violator tuple came from + * there. */ - onfk = (queryno == RI_PLAN_CHECK_LOOKUPPK); - if (onfk) + if (on_fk) { attnums = riinfo->fk_attnums; rel_oid = fk_rel->rd_id; @@ -2579,7 +2637,7 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo, key_names.data, key_values.data, RelationGetRelationName(fk_rel)), errtableconstraint(fk_rel, NameStr(riinfo->conname)))); - else if (onfk) + else if (on_fk) ereport(ERROR, (errcode(ERRCODE_FOREIGN_KEY_VIOLATION), errmsg("insert or update on table \"%s\" violates foreign key constraint \"%s\"", @@ -2886,7 +2944,10 @@ ri_AttributesEqual(Oid eq_opr, Oid typeid, * ri_HashCompareOp - * * See if we know how to compare two values, and create a new hash entry - * if not. + * if not. The entry contains the FmgrInfo of the equality operator function + * and that of the cast function, if one is needed to convert the right + * operand (whose type OID has been passed) before passing it to the equality + * function. */ static RI_CompareHashEntry * ri_HashCompareOp(Oid eq_opr, Oid typeid) @@ -2942,8 +3003,16 @@ ri_HashCompareOp(Oid eq_opr, Oid typeid) * moment since that will never be generated for implicit coercions. */ op_input_types(eq_opr, &lefttype, &righttype); - Assert(lefttype == righttype); - if (typeid == lefttype) + + /* + * Don't need to cast if the values that will be passed to the + * operator will be of expected operand type(s). The operator can be + * cross-type (such as when called by ri_ReferencedKeyExists()), in + * which case, we only need the cast if the right operand value + * doesn't match the type expected by the operator. + */ + if ((lefttype == righttype && typeid == lefttype) || + (lefttype != righttype && typeid == righttype)) castfunc = InvalidOid; /* simplest case */ else { diff --git a/src/include/executor/execPartition.h b/src/include/executor/execPartition.h index 603d8becc4..e63dcb12f6 100644 --- a/src/include/executor/execPartition.h +++ b/src/include/executor/execPartition.h @@ -124,5 +124,11 @@ extern PartitionPruneState *ExecCreatePartitionPruneState(PlanState *planstate, extern Bitmapset *ExecFindMatchingSubPlans(PartitionPruneState *prunestate); extern Bitmapset *ExecFindInitialMatchingSubPlans(PartitionPruneState *prunestate, int nsubplans); +extern Relation find_leaf_part_for_key(Relation root_rel, + int key_natts, + const AttrNumber *key_attnums, + Datum *key_vals, char *key_nulls, + Oid root_idxoid, int lockmode, + Oid *leaf_idxoid); #endif /* EXECPARTITION_H */ diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index 344399f6a8..8f32353a44 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -241,6 +241,15 @@ extern bool ExecShutdownNode(PlanState *node); extern void ExecSetTupleBound(int64 tuples_needed, PlanState *child_node); +/* + * functions in execLockRows.c + */ + +extern bool ExecLockTableTuple(Relation relation, ItemPointer tid, TupleTableSlot *slot, + Snapshot snapshot, CommandId cid, + LockTupleMode lockmode, LockWaitPolicy waitPolicy, + bool *epq_needed); + /* ---------------------------------------------------------------- * ExecProcNode * diff --git a/src/test/isolation/expected/fk-snapshot.out b/src/test/isolation/expected/fk-snapshot.out index 5faf80d6ce..22752cc742 100644 --- a/src/test/isolation/expected/fk-snapshot.out +++ b/src/test/isolation/expected/fk-snapshot.out @@ -47,12 +47,12 @@ a step s2ifn2: INSERT INTO fk_noparted VALUES (2); step s2c: COMMIT; +ERROR: insert or update on table "fk_noparted" violates foreign key constraint "fk_noparted_a_fkey" step s2sfn: SELECT * FROM fk_noparted; a - 1 -2 -(2 rows) +(1 row) starting permutation: s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 s2c s2sfn diff --git a/src/test/isolation/specs/fk-snapshot.spec b/src/test/isolation/specs/fk-snapshot.spec index 378507fbc3..64d27f29c3 100644 --- a/src/test/isolation/specs/fk-snapshot.spec +++ b/src/test/isolation/specs/fk-snapshot.spec @@ -46,10 +46,7 @@ step s2sfn { SELECT * FROM fk_noparted; } # inserting into referencing tables in transaction-snapshot mode # PK table is non-partitioned permutation s1brr s2brc s2ip2 s1sp s2c s1sp s1ifp2 s1c s1sfp -# PK table is partitioned: buggy, because s2's serialization transaction can -# see the uncommitted row thanks to the latest snapshot taken for -# partition lookup to work correctly also ends up getting used by the PK index -# scan +# PK table is partitioned permutation s2ip2 s2brr s1brc s1ifp2 s2sfp s1c s2sfp s2ifn2 s2c s2sfn # inserting into referencing tables in up-to-date snapshot mode -- 2.24.1 [application/octet-stream] v13-0001-Add-isolation-tests-for-snapshot-behavior-in-ri_.patch (6.5K, ../../CA+HiwqEXXzqWa=RE90Ce2=R9B0vggFrvGZYrupVT3GeY-XXR-g@mail.gmail.com/3-v13-0001-Add-isolation-tests-for-snapshot-behavior-in-ri_.patch) download | inline diff: From c4be44711c0eb34ad386fc27085fa97db39526ce Mon Sep 17 00:00:00 2001 From: amitlan <[email protected]> Date: Mon, 15 Nov 2021 18:22:33 +0900 Subject: [PATCH v13 1/2] Add isolation tests for snapshot behavior in ri_triggers.c They are to check the behavior of RI_FKey_check() and ri_Check_Pk_Match(). A test case whereby RI_FKey_check() queries a partitioned PK table under REPEATABLE READ isolation produces wrong output due to a bug of the partition-descriptor logic and that is noted as such in the comment above the test. A subsequent patch will fix the bug and replace the buggy output by the correct one. --- src/test/isolation/expected/fk-snapshot.out | 124 ++++++++++++++++++++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/fk-snapshot.spec | 61 ++++++++++ 3 files changed, 186 insertions(+) create mode 100644 src/test/isolation/expected/fk-snapshot.out create mode 100644 src/test/isolation/specs/fk-snapshot.spec diff --git a/src/test/isolation/expected/fk-snapshot.out b/src/test/isolation/expected/fk-snapshot.out new file mode 100644 index 0000000000..5faf80d6ce --- /dev/null +++ b/src/test/isolation/expected/fk-snapshot.out @@ -0,0 +1,124 @@ +Parsed test spec with 2 sessions + +starting permutation: s1brr s2brc s2ip2 s1sp s2c s1sp s1ifp2 s1c s1sfp +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s2brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +(1 row) + +step s2c: COMMIT; +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +(1 row) + +step s1ifp2: INSERT INTO fk_parted_pk VALUES (2); +ERROR: insert or update on table "fk_parted_pk_2" violates foreign key constraint "fk_parted_pk_a_fkey" +step s1c: COMMIT; +step s1sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + + +starting permutation: s2ip2 s2brr s1brc s1ifp2 s2sfp s1c s2sfp s2ifn2 s2c s2sfn +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s2brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s1ifp2: INSERT INTO fk_parted_pk VALUES (2); +step s2sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + +step s1c: COMMIT; +step s2sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + +step s2ifn2: INSERT INTO fk_noparted VALUES (2); +step s2c: COMMIT; +step s2sfn: SELECT * FROM fk_noparted; +a +- +1 +2 +(2 rows) + + +starting permutation: s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 s2c s2sfn +step s1brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +(1 row) + +step s2c: COMMIT; +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +2 +(2 rows) + +step s1ifp2: INSERT INTO fk_parted_pk VALUES (2); +step s2brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + +step s1c: COMMIT; +step s1sfp: SELECT * FROM fk_parted_pk; +a +- +1 +2 +(2 rows) + +step s2ifn2: INSERT INTO fk_noparted VALUES (2); +step s2c: COMMIT; +step s2sfn: SELECT * FROM fk_noparted; +a +- +1 +2 +(2 rows) + + +starting permutation: s1brr s1dfp s1ifp1 s1c s1sfn +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1dfp: DELETE FROM fk_parted_pk WHERE a = 1; +step s1ifp1: INSERT INTO fk_parted_pk VALUES (1); +step s1c: COMMIT; +step s1sfn: SELECT * FROM fk_noparted; +a +- +1 +(1 row) + + +starting permutation: s1brc s1dfp s1ifp1 s1c s1sfn +step s1brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s1dfp: DELETE FROM fk_parted_pk WHERE a = 1; +step s1ifp1: INSERT INTO fk_parted_pk VALUES (1); +step s1c: COMMIT; +step s1sfn: SELECT * FROM fk_noparted; +a +- +1 +(1 row) + diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 99c23b16ff..90f29fd278 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -33,6 +33,7 @@ test: fk-deadlock test: fk-deadlock2 test: fk-partitioned-1 test: fk-partitioned-2 +test: fk-snapshot test: eval-plan-qual test: eval-plan-qual-trigger test: lock-update-delete diff --git a/src/test/isolation/specs/fk-snapshot.spec b/src/test/isolation/specs/fk-snapshot.spec new file mode 100644 index 0000000000..378507fbc3 --- /dev/null +++ b/src/test/isolation/specs/fk-snapshot.spec @@ -0,0 +1,61 @@ +setup +{ + CREATE TABLE pk_noparted ( + a int PRIMARY KEY + ); + + CREATE TABLE fk_parted_pk ( + a int PRIMARY KEY REFERENCES pk_noparted ON DELETE CASCADE + ) PARTITION BY LIST (a); + CREATE TABLE fk_parted_pk_1 PARTITION OF fk_parted_pk FOR VALUES IN (1); + CREATE TABLE fk_parted_pk_2 PARTITION OF fk_parted_pk FOR VALUES IN (2); + + CREATE TABLE fk_noparted ( + a int REFERENCES fk_parted_pk ON DELETE NO ACTION INITIALLY DEFERRED + ); + INSERT INTO pk_noparted VALUES (1); + INSERT INTO fk_parted_pk VALUES (1); + INSERT INTO fk_noparted VALUES (1); +} + +teardown +{ + DROP TABLE pk_noparted, fk_parted_pk, fk_noparted; +} + +session s1 +step s1brr { BEGIN ISOLATION LEVEL REPEATABLE READ; } +step s1brc { BEGIN ISOLATION LEVEL READ COMMITTED; } +step s1ifp2 { INSERT INTO fk_parted_pk VALUES (2); } +step s1ifp1 { INSERT INTO fk_parted_pk VALUES (1); } +step s1dfp { DELETE FROM fk_parted_pk WHERE a = 1; } +step s1c { COMMIT; } +step s1sfp { SELECT * FROM fk_parted_pk; } +step s1sp { SELECT * FROM pk_noparted; } +step s1sfn { SELECT * FROM fk_noparted; } + +session s2 +step s2brr { BEGIN ISOLATION LEVEL REPEATABLE READ; } +step s2brc { BEGIN ISOLATION LEVEL READ COMMITTED; } +step s2ip2 { INSERT INTO pk_noparted VALUES (2); } +step s2ifn2 { INSERT INTO fk_noparted VALUES (2); } +step s2c { COMMIT; } +step s2sfp { SELECT * FROM fk_parted_pk; } +step s2sfn { SELECT * FROM fk_noparted; } + +# inserting into referencing tables in transaction-snapshot mode +# PK table is non-partitioned +permutation s1brr s2brc s2ip2 s1sp s2c s1sp s1ifp2 s1c s1sfp +# PK table is partitioned: buggy, because s2's serialization transaction can +# see the uncommitted row thanks to the latest snapshot taken for +# partition lookup to work correctly also ends up getting used by the PK index +# scan +permutation s2ip2 s2brr s1brc s1ifp2 s2sfp s1c s2sfp s2ifn2 s2c s2sfn + +# inserting into referencing tables in up-to-date snapshot mode +permutation s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 s2c s2sfn + +# deleting a referenced row and then inserting again in the same transaction; works +# the same no matter the snapshot mode +permutation s1brr s1dfp s1ifp1 s1c s1sfn +permutation s1brc s1dfp s1ifp1 s1c s1sfn -- 2.24.1 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: simplifying foreign key/RI checks 2022-01-18 06:30 Re: simplifying foreign key/RI checks Amit Langote <[email protected]> @ 2022-03-14 08:33 ` Amit Langote <[email protected]> 2022-03-14 09:32 ` Re: simplifying foreign key/RI checks Zhihong Yu <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Amit Langote @ 2022-03-14 08:33 UTC (permalink / raw) To: Zhihong Yu <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Alvaro Herrera <[email protected]>; vignesh C <[email protected]>; Kyotaro Horiguchi <[email protected]>; PostgreSQL Developers <[email protected]> On Tue, Jan 18, 2022 at 3:30 PM Amit Langote <[email protected]> wrote: > v13 is attached. I noticed that the recent 641f3dffcdf's changes to get_constraint_index() made it basically unusable for this patch's purposes. Reading in the thread that led to 641f3dffcdf why get_constraint_index() was changed the way it was, I invented in the attached updated patch a get_fkey_constraint_index() that is local to ri_triggers.c for use by the new ri_ReferencedKeyExists(), replacing get_constraint_index() that no longer gives it the index it's looking for. -- Amit Langote EDB: http://www.enterprisedb.com Attachments: [application/octet-stream] v14-0001-Add-isolation-tests-for-snapshot-behavior-in-ri_.patch (6.5K, ../../CA+HiwqGGF=PNE6XhDcGriXGq6kEHnf7RZFRKSxeY7EGHCy=FhA@mail.gmail.com/2-v14-0001-Add-isolation-tests-for-snapshot-behavior-in-ri_.patch) download | inline diff: From 1bd70ca0c434a364e55bc16ec3edb6c810527435 Mon Sep 17 00:00:00 2001 From: amitlan <[email protected]> Date: Mon, 15 Nov 2021 18:22:33 +0900 Subject: [PATCH v14 1/2] Add isolation tests for snapshot behavior in ri_triggers.c They are to check the behavior of RI_FKey_check() and ri_Check_Pk_Match(). A test case whereby RI_FKey_check() queries a partitioned PK table under REPEATABLE READ isolation produces wrong output due to a bug of the partition-descriptor logic and that is noted as such in the comment above the test. A subsequent patch will fix the bug and replace the buggy output by the correct one. --- src/test/isolation/expected/fk-snapshot.out | 124 ++++++++++++++++++++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/fk-snapshot.spec | 61 ++++++++++ 3 files changed, 186 insertions(+) create mode 100644 src/test/isolation/expected/fk-snapshot.out create mode 100644 src/test/isolation/specs/fk-snapshot.spec diff --git a/src/test/isolation/expected/fk-snapshot.out b/src/test/isolation/expected/fk-snapshot.out new file mode 100644 index 0000000000..5faf80d6ce --- /dev/null +++ b/src/test/isolation/expected/fk-snapshot.out @@ -0,0 +1,124 @@ +Parsed test spec with 2 sessions + +starting permutation: s1brr s2brc s2ip2 s1sp s2c s1sp s1ifp2 s1c s1sfp +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s2brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +(1 row) + +step s2c: COMMIT; +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +(1 row) + +step s1ifp2: INSERT INTO fk_parted_pk VALUES (2); +ERROR: insert or update on table "fk_parted_pk_2" violates foreign key constraint "fk_parted_pk_a_fkey" +step s1c: COMMIT; +step s1sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + + +starting permutation: s2ip2 s2brr s1brc s1ifp2 s2sfp s1c s2sfp s2ifn2 s2c s2sfn +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s2brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s1ifp2: INSERT INTO fk_parted_pk VALUES (2); +step s2sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + +step s1c: COMMIT; +step s2sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + +step s2ifn2: INSERT INTO fk_noparted VALUES (2); +step s2c: COMMIT; +step s2sfn: SELECT * FROM fk_noparted; +a +- +1 +2 +(2 rows) + + +starting permutation: s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 s2c s2sfn +step s1brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +(1 row) + +step s2c: COMMIT; +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +2 +(2 rows) + +step s1ifp2: INSERT INTO fk_parted_pk VALUES (2); +step s2brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + +step s1c: COMMIT; +step s1sfp: SELECT * FROM fk_parted_pk; +a +- +1 +2 +(2 rows) + +step s2ifn2: INSERT INTO fk_noparted VALUES (2); +step s2c: COMMIT; +step s2sfn: SELECT * FROM fk_noparted; +a +- +1 +2 +(2 rows) + + +starting permutation: s1brr s1dfp s1ifp1 s1c s1sfn +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1dfp: DELETE FROM fk_parted_pk WHERE a = 1; +step s1ifp1: INSERT INTO fk_parted_pk VALUES (1); +step s1c: COMMIT; +step s1sfn: SELECT * FROM fk_noparted; +a +- +1 +(1 row) + + +starting permutation: s1brc s1dfp s1ifp1 s1c s1sfn +step s1brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s1dfp: DELETE FROM fk_parted_pk WHERE a = 1; +step s1ifp1: INSERT INTO fk_parted_pk VALUES (1); +step s1c: COMMIT; +step s1sfn: SELECT * FROM fk_noparted; +a +- +1 +(1 row) + diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 0dae483e82..2b89acb54d 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -33,6 +33,7 @@ test: fk-deadlock test: fk-deadlock2 test: fk-partitioned-1 test: fk-partitioned-2 +test: fk-snapshot test: eval-plan-qual test: eval-plan-qual-trigger test: lock-update-delete diff --git a/src/test/isolation/specs/fk-snapshot.spec b/src/test/isolation/specs/fk-snapshot.spec new file mode 100644 index 0000000000..378507fbc3 --- /dev/null +++ b/src/test/isolation/specs/fk-snapshot.spec @@ -0,0 +1,61 @@ +setup +{ + CREATE TABLE pk_noparted ( + a int PRIMARY KEY + ); + + CREATE TABLE fk_parted_pk ( + a int PRIMARY KEY REFERENCES pk_noparted ON DELETE CASCADE + ) PARTITION BY LIST (a); + CREATE TABLE fk_parted_pk_1 PARTITION OF fk_parted_pk FOR VALUES IN (1); + CREATE TABLE fk_parted_pk_2 PARTITION OF fk_parted_pk FOR VALUES IN (2); + + CREATE TABLE fk_noparted ( + a int REFERENCES fk_parted_pk ON DELETE NO ACTION INITIALLY DEFERRED + ); + INSERT INTO pk_noparted VALUES (1); + INSERT INTO fk_parted_pk VALUES (1); + INSERT INTO fk_noparted VALUES (1); +} + +teardown +{ + DROP TABLE pk_noparted, fk_parted_pk, fk_noparted; +} + +session s1 +step s1brr { BEGIN ISOLATION LEVEL REPEATABLE READ; } +step s1brc { BEGIN ISOLATION LEVEL READ COMMITTED; } +step s1ifp2 { INSERT INTO fk_parted_pk VALUES (2); } +step s1ifp1 { INSERT INTO fk_parted_pk VALUES (1); } +step s1dfp { DELETE FROM fk_parted_pk WHERE a = 1; } +step s1c { COMMIT; } +step s1sfp { SELECT * FROM fk_parted_pk; } +step s1sp { SELECT * FROM pk_noparted; } +step s1sfn { SELECT * FROM fk_noparted; } + +session s2 +step s2brr { BEGIN ISOLATION LEVEL REPEATABLE READ; } +step s2brc { BEGIN ISOLATION LEVEL READ COMMITTED; } +step s2ip2 { INSERT INTO pk_noparted VALUES (2); } +step s2ifn2 { INSERT INTO fk_noparted VALUES (2); } +step s2c { COMMIT; } +step s2sfp { SELECT * FROM fk_parted_pk; } +step s2sfn { SELECT * FROM fk_noparted; } + +# inserting into referencing tables in transaction-snapshot mode +# PK table is non-partitioned +permutation s1brr s2brc s2ip2 s1sp s2c s1sp s1ifp2 s1c s1sfp +# PK table is partitioned: buggy, because s2's serialization transaction can +# see the uncommitted row thanks to the latest snapshot taken for +# partition lookup to work correctly also ends up getting used by the PK index +# scan +permutation s2ip2 s2brr s1brc s1ifp2 s2sfp s1c s2sfp s2ifn2 s2c s2sfn + +# inserting into referencing tables in up-to-date snapshot mode +permutation s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 s2c s2sfn + +# deleting a referenced row and then inserting again in the same transaction; works +# the same no matter the snapshot mode +permutation s1brr s1dfp s1ifp1 s1c s1sfn +permutation s1brc s1dfp s1ifp1 s1c s1sfn -- 2.24.1 [application/octet-stream] v14-0002-Avoid-using-SPI-for-some-RI-checks.patch (40.7K, ../../CA+HiwqGGF=PNE6XhDcGriXGq6kEHnf7RZFRKSxeY7EGHCy=FhA@mail.gmail.com/3-v14-0002-Avoid-using-SPI-for-some-RI-checks.patch) download | inline diff: From 46ffe1d6df5eb3545666c8d19a3f7f2d977c25db Mon Sep 17 00:00:00 2001 From: amitlan <[email protected]> Date: Tue, 12 Jan 2021 14:17:31 +0900 Subject: [PATCH v14 2/2] Avoid using SPI for some RI checks This modifies the subroutines called by RI trigger functions that want to check if a given referenced value exists in the referenced relation to simply scan the foreign key constraint's unique index. That replaces the current way of issuing a `SELECT 1 FROM referenced_relation WHERE ref_key = $1` query through SPI to do the same. This saves a lot of work, especially when inserting into or updating a referencing relation. This rewrite allows to fix a PK row visibility bug caused by a partition descriptor hack which requires ActiveSnapshot to be set to come up with the correct set of partitions for the RI query running under REPEATABLE READ isolation. We now set that snapshot indepedently of the snapshot to be used by the PK index scan, so the two no longer interfere. The buggy output in src/test/isolation/expected/fk-snapshot.out of the relevant test case has been corrected. --- src/backend/executor/execPartition.c | 160 +++++- src/backend/executor/nodeLockRows.c | 160 +++--- src/backend/utils/adt/ri_triggers.c | 573 ++++++++++++-------- src/include/executor/execPartition.h | 6 + src/include/executor/executor.h | 9 + src/test/isolation/expected/fk-snapshot.out | 4 +- src/test/isolation/specs/fk-snapshot.spec | 5 +- 7 files changed, 596 insertions(+), 321 deletions(-) diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 90ed1485d1..72ee019330 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -175,8 +175,9 @@ static void FormPartitionKeyDatum(PartitionDispatch pd, EState *estate, Datum *values, bool *isnull); -static int get_partition_for_tuple(PartitionDispatch pd, Datum *values, - bool *isnull); +static int get_partition_for_tuple(PartitionKey key, + PartitionDesc partdesc, + Datum *values, bool *isnull); static char *ExecBuildSlotPartitionKeyDescription(Relation rel, Datum *values, bool *isnull, @@ -310,7 +311,9 @@ ExecFindPartition(ModifyTableState *mtstate, * these values, error out. */ if (partdesc->nparts == 0 || - (partidx = get_partition_for_tuple(dispatch, values, isnull)) < 0) + (partidx = get_partition_for_tuple(dispatch->key, + dispatch->partdesc, + values, isnull)) < 0) { char *val_desc; @@ -1240,12 +1243,12 @@ FormPartitionKeyDatum(PartitionDispatch pd, * found or -1 if none found. */ static int -get_partition_for_tuple(PartitionDispatch pd, Datum *values, bool *isnull) +get_partition_for_tuple(PartitionKey key, + PartitionDesc partdesc, + Datum *values, bool *isnull) { int bound_offset; int part_index = -1; - PartitionKey key = pd->key; - PartitionDesc partdesc = pd->partdesc; PartitionBoundInfo boundinfo = partdesc->boundinfo; /* Route as appropriate based on partitioning strategy. */ @@ -1337,6 +1340,151 @@ get_partition_for_tuple(PartitionDispatch pd, Datum *values, bool *isnull) return part_index; } +/* + * find_leaf_part_for_key + * Finds the leaf partition of a partitioned table 'root_rel' that might + * contain the specified key tuple containing a subset of the table's + * columns (including all of the partition key columns) + * + * 'key_natts' specifies the number columns contained in the key, + * 'key_attnums' their attribute numbers as defined in 'root_rel', and + * 'key_vals' and 'key_nulls' specify the key tuple. + * + * Returns NULL if no leaf partition is found for the key. Caller must close + * the relation. + * + * This works because the unique key defined on the root relation is required + * to contain the partition key columns of all of the ancestors that lead up to + * a given leaf partition. + */ +Relation +find_leaf_part_for_key(Relation root_rel, int key_natts, + const AttrNumber *key_attnums, + Datum *key_vals, char *key_nulls, + Oid root_idxoid, int lockmode, + Oid *leaf_idxoid) +{ + Relation rel = root_rel; + Oid constr_idxoid = root_idxoid; + + *leaf_idxoid = InvalidOid; + + /* + * Descend through partitioned parents to find the leaf partition that + * would accept a row with the provided key values, starting with the root + * parent. + */ + while (true) + { + PartitionKey partkey = RelationGetPartitionKey(rel); + PartitionDirectory partdir; + PartitionDesc partdesc; + Datum partkey_vals[PARTITION_MAX_KEYS]; + bool partkey_isnull[PARTITION_MAX_KEYS]; + AttrNumber *root_partattrs = partkey->partattrs; + int i, + j; + int partidx; + Oid partoid; + bool is_leaf; + + /* + * Collect partition key values from the unique key. + * + * Because we only have the root table's copy of pk_attnums, must map + * any non-root table's partition key attribute numbers to the root + * table's. + */ + if (rel != root_rel) + { + /* + * map->attnums will contain root table attribute numbers for each + * attribute of the current partitioned relation. + */ + AttrMap *map = build_attrmap_by_name_if_req(RelationGetDescr(root_rel), + RelationGetDescr(rel)); + + if (map) + { + root_partattrs = palloc(partkey->partnatts * + sizeof(AttrNumber)); + for (i = 0; i < partkey->partnatts; i++) + { + AttrNumber partattno = partkey->partattrs[i]; + + root_partattrs[i] = map->attnums[partattno - 1]; + } + + free_attrmap(map); + } + } + + /* + * Referenced key specification does not allow expressions, so there + * would not be expressions in the partition keys either. + */ + Assert(partkey->partexprs == NIL); + for (i = 0, j = 0; i < partkey->partnatts; i++) + { + int k; + + for (k = 0; k < key_natts; k++) + { + if (root_partattrs[i] == key_attnums[k]) + { + partkey_vals[j] = key_vals[k]; + partkey_isnull[j] = (key_nulls[k] == 'n' ? true : false); + j++; + break; + } + } + } + /* Had better have found values for all of the partition keys. */ + Assert(j == partkey->partnatts); + + if (root_partattrs != partkey->partattrs) + pfree(root_partattrs); + + /* Get the PartitionDesc using the partition directory machinery. */ + partdir = CreatePartitionDirectory(CurrentMemoryContext, true); + partdesc = PartitionDirectoryLookup(partdir, rel); + + /* Find the partition for the key. */ + partidx = get_partition_for_tuple(partkey, partdesc, + partkey_vals, partkey_isnull); + Assert(partidx < 0 || partidx < partdesc->nparts); + + /* done using the partition directory */ + DestroyPartitionDirectory(partdir); + + /* close any intermediate parents we opened */ + if (rel != root_rel) + table_close(rel, NoLock); + + /* No partition found. */ + if (partidx < 0) + return NULL; + + partoid = partdesc->oids[partidx]; + rel = table_open(partoid, lockmode); + constr_idxoid = index_get_partition(rel, constr_idxoid); + + /* + * Return if the partition is a leaf, else find its partition in the + * next iteration. + */ + is_leaf = partdesc->is_leaf[partidx]; + if (is_leaf) + { + *leaf_idxoid = constr_idxoid; + return rel; + } + } + + Assert(false); + return NULL; +} + /* * ExecBuildSlotPartitionKeyDescription * diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c index 1a9dab25dd..ab54a65e0e 100644 --- a/src/backend/executor/nodeLockRows.c +++ b/src/backend/executor/nodeLockRows.c @@ -79,10 +79,7 @@ lnext: Datum datum; bool isNull; ItemPointerData tid; - TM_FailureData tmfd; LockTupleMode lockmode; - int lockflags = 0; - TM_Result test; TupleTableSlot *markSlot; /* clear any leftover test tuple for this rel */ @@ -179,74 +176,11 @@ lnext: break; } - lockflags = TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS; - if (!IsolationUsesXactSnapshot()) - lockflags |= TUPLE_LOCK_FLAG_FIND_LAST_VERSION; - - test = table_tuple_lock(erm->relation, &tid, estate->es_snapshot, - markSlot, estate->es_output_cid, - lockmode, erm->waitPolicy, - lockflags, - &tmfd); - - switch (test) - { - case TM_WouldBlock: - /* couldn't lock tuple in SKIP LOCKED mode */ - goto lnext; - - case TM_SelfModified: - - /* - * The target tuple was already updated or deleted by the - * current command, or by a later command in the current - * transaction. We *must* ignore the tuple in the former - * case, so as to avoid the "Halloween problem" of repeated - * update attempts. In the latter case it might be sensible - * to fetch the updated tuple instead, but doing so would - * require changing heap_update and heap_delete to not - * complain about updating "invisible" tuples, which seems - * pretty scary (table_tuple_lock will not complain, but few - * callers expect TM_Invisible, and we're not one of them). So - * for now, treat the tuple as deleted and do not process. - */ - goto lnext; - - case TM_Ok: - - /* - * Got the lock successfully, the locked tuple saved in - * markSlot for, if needed, EvalPlanQual testing below. - */ - if (tmfd.traversed) - epq_needed = true; - break; - - case TM_Updated: - if (IsolationUsesXactSnapshot()) - ereport(ERROR, - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), - errmsg("could not serialize access due to concurrent update"))); - elog(ERROR, "unexpected table_tuple_lock status: %u", - test); - break; - - case TM_Deleted: - if (IsolationUsesXactSnapshot()) - ereport(ERROR, - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), - errmsg("could not serialize access due to concurrent update"))); - /* tuple was deleted so don't return it */ - goto lnext; - - case TM_Invisible: - elog(ERROR, "attempted to lock invisible tuple"); - break; - - default: - elog(ERROR, "unrecognized table_tuple_lock status: %u", - test); - } + /* skip tuple if it couldn't be locked */ + if (!ExecLockTableTuple(erm->relation, &tid, markSlot, + estate->es_snapshot, estate->es_output_cid, + lockmode, erm->waitPolicy, &epq_needed)) + goto lnext; /* Remember locked tuple's TID for EPQ testing and WHERE CURRENT OF */ erm->curCtid = tid; @@ -281,6 +215,90 @@ lnext: return slot; } +/* + * ExecLockTableTuple + * Locks tuple with the specified TID in lockmode following given wait + * policy + * + * Returns true if the tuple was successfully locked. Locked tuple is loaded + * into provided slot. + */ +bool +ExecLockTableTuple(Relation relation, ItemPointer tid, TupleTableSlot *slot, + Snapshot snapshot, CommandId cid, + LockTupleMode lockmode, LockWaitPolicy waitPolicy, + bool *epq_needed) +{ + TM_FailureData tmfd; + int lockflags = TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS; + TM_Result test; + + if (!IsolationUsesXactSnapshot()) + lockflags |= TUPLE_LOCK_FLAG_FIND_LAST_VERSION; + + test = table_tuple_lock(relation, tid, snapshot, slot, cid, lockmode, + waitPolicy, lockflags, &tmfd); + + switch (test) + { + case TM_WouldBlock: + /* couldn't lock tuple in SKIP LOCKED mode */ + return false; + + case TM_SelfModified: + /* + * The target tuple was already updated or deleted by the + * current command, or by a later command in the current + * transaction. We *must* ignore the tuple in the former + * case, so as to avoid the "Halloween problem" of repeated + * update attempts. In the latter case it might be sensible + * to fetch the updated tuple instead, but doing so would + * require changing heap_update and heap_delete to not + * complain about updating "invisible" tuples, which seems + * pretty scary (table_tuple_lock will not complain, but few + * callers expect TM_Invisible, and we're not one of them). So + * for now, treat the tuple as deleted and do not process. + */ + return false; + + case TM_Ok: + /* + * Got the lock successfully, the locked tuple saved in + * slot for EvalPlanQual, if asked by the caller. + */ + if (tmfd.traversed && epq_needed) + *epq_needed = true; + break; + + case TM_Updated: + if (IsolationUsesXactSnapshot()) + ereport(ERROR, + (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), + errmsg("could not serialize access due to concurrent update"))); + elog(ERROR, "unexpected table_tuple_lock status: %u", + test); + break; + + case TM_Deleted: + if (IsolationUsesXactSnapshot()) + ereport(ERROR, + (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), + errmsg("could not serialize access due to concurrent update"))); + /* tuple was deleted so don't return it */ + return false; + + case TM_Invisible: + elog(ERROR, "attempted to lock invisible tuple"); + return false; + + default: + elog(ERROR, "unrecognized table_tuple_lock status: %u", test); + return false; + } + + return true; +} + /* ---------------------------------------------------------------- * ExecInitLockRows * diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index c95cd32402..511697f2ce 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -23,22 +23,27 @@ #include "postgres.h" +#include "access/genam.h" #include "access/htup_details.h" +#include "access/skey.h" #include "access/sysattr.h" #include "access/table.h" #include "access/tableam.h" #include "access/xact.h" +#include "catalog/partition.h" #include "catalog/pg_collation.h" #include "catalog/pg_constraint.h" #include "catalog/pg_operator.h" #include "catalog/pg_type.h" #include "commands/trigger.h" +#include "executor/execPartition.h" #include "executor/executor.h" #include "executor/spi.h" #include "lib/ilist.h" #include "miscadmin.h" #include "parser/parse_coerce.h" #include "parser/parse_relation.h" +#include "partitioning/partdesc.h" #include "storage/bufmgr.h" #include "utils/acl.h" #include "utils/builtins.h" @@ -48,6 +53,7 @@ #include "utils/inval.h" #include "utils/lsyscache.h" #include "utils/memutils.h" +#include "utils/partcache.h" #include "utils/rel.h" #include "utils/rls.h" #include "utils/ruleutils.h" @@ -68,19 +74,15 @@ #define RI_KEYS_NONE_NULL 2 /* RI query type codes */ -/* these queries are executed against the PK (referenced) table: */ -#define RI_PLAN_CHECK_LOOKUPPK 1 -#define RI_PLAN_CHECK_LOOKUPPK_FROM_PK 2 -#define RI_PLAN_LAST_ON_PK RI_PLAN_CHECK_LOOKUPPK_FROM_PK /* these queries are executed against the FK (referencing) table: */ -#define RI_PLAN_CASCADE_ONDELETE 3 -#define RI_PLAN_CASCADE_ONUPDATE 4 +#define RI_PLAN_CASCADE_ONDELETE 1 +#define RI_PLAN_CASCADE_ONUPDATE 2 /* For RESTRICT, the same plan can be used for both ON DELETE and ON UPDATE triggers. */ -#define RI_PLAN_RESTRICT 5 -#define RI_PLAN_SETNULL_ONDELETE 6 -#define RI_PLAN_SETNULL_ONUPDATE 7 -#define RI_PLAN_SETDEFAULT_ONDELETE 8 -#define RI_PLAN_SETDEFAULT_ONUPDATE 9 +#define RI_PLAN_RESTRICT 3 +#define RI_PLAN_SETNULL_ONDELETE 4 +#define RI_PLAN_SETNULL_ONUPDATE 5 +#define RI_PLAN_SETDEFAULT_ONDELETE 6 +#define RI_PLAN_SETDEFAULT_ONUPDATE 7 #define MAX_QUOTED_NAME_LEN (NAMEDATALEN*2+3) #define MAX_QUOTED_REL_NAME_LEN (MAX_QUOTED_NAME_LEN*2) @@ -229,8 +231,274 @@ static void ri_ExtractValues(Relation rel, TupleTableSlot *slot, static void ri_ReportViolation(const RI_ConstraintInfo *riinfo, Relation pk_rel, Relation fk_rel, TupleTableSlot *violatorslot, TupleDesc tupdesc, - int queryno, bool partgone) pg_attribute_noreturn(); + bool on_fk, bool partgone) pg_attribute_noreturn(); +static Oid get_fkey_unique_index(Oid conoid); +/* + * Checks whether a tuple containing the unique key as extracted from the + * tuple provided in 'slot' exists in 'pk_rel'. The key is extracted using the + * constraint's index given in 'riinfo', which is also scanned to check the + * existence of the key. + * + * If 'pk_rel' is a partitioned table, the check is performed on its leaf + * partition that would contain the key. + * + * The provided tuple is either the one being inserted into the referencing + * relation ('fk_rel' is non-NULL), or the one being deleted from the + * referenced relation, that is, 'pk_rel' ('fk_rel' is NULL). + */ +static bool +ri_ReferencedKeyExists(Relation pk_rel, Relation fk_rel, + TupleTableSlot *slot, + const RI_ConstraintInfo *riinfo) +{ + Oid constr_id = riinfo->constraint_id; + Oid idxoid; + Relation idxrel; + Relation leaf_pk_rel = NULL; + int num_pk; + int i; + bool found = false; + const Oid *eq_oprs; + Datum pk_vals[INDEX_MAX_KEYS]; + char pk_nulls[INDEX_MAX_KEYS]; + ScanKeyData skey[INDEX_MAX_KEYS]; + Snapshot snap = InvalidSnapshot; + bool pushed_latest_snapshot = false; + IndexScanDesc scan; + TupleTableSlot *outslot; + Oid saved_userid; + int saved_sec_context; + AclResult aclresult; + + /* + * Extract the unique key from the provided slot and choose the equality + * operators to use when scanning the index below. + */ + if (fk_rel) + { + ri_ExtractValues(fk_rel, slot, riinfo, false, pk_vals, pk_nulls); + /* Use PK = FK equality operator. */ + eq_oprs = riinfo->pf_eq_oprs; + + /* + * May neeed to cast each of the individual values of the foreign key + * to the corresponding PK column's type if the equality operator + * demands it. + */ + for (i = 0; i < riinfo->nkeys; i++) + { + if (pk_nulls[i] != 'n') + { + Oid eq_opr = eq_oprs[i]; + Oid typeid = RIAttType(fk_rel, riinfo->fk_attnums[i]); + RI_CompareHashEntry *entry = ri_HashCompareOp(eq_opr, typeid); + + if (OidIsValid(entry->cast_func_finfo.fn_oid)) + pk_vals[i] = FunctionCall3(&entry->cast_func_finfo, + pk_vals[i], + Int32GetDatum(-1), /* typmod */ + BoolGetDatum(false)); /* implicit coercion */ + } + } + } + else + { + ri_ExtractValues(pk_rel, slot, riinfo, true, pk_vals, pk_nulls); + /* Use PK = PK equality operator. */ + eq_oprs = riinfo->pp_eq_oprs; + } + + /* + * Switch to referenced table's owner to perform the below operations + * as. This matches what ri_PerformCheck() does. + * + * Note that as with queries done by ri_PerformCheck(), the way we select + * the referenced row below effectively bypasses any RLS policies that may + * be present on the referenced table. + */ + GetUserIdAndSecContext(&saved_userid, &saved_sec_context); + SetUserIdAndSecContext(RelationGetForm(pk_rel)->relowner, + saved_sec_context | SECURITY_LOCAL_USERID_CHANGE); + + /* + * Also check that the new user has permissions to look into the schema + * of and SELECT from the referenced table. + */ + aclresult = pg_namespace_aclcheck(RelationGetNamespace(pk_rel), + GetUserId(), ACL_USAGE); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_SCHEMA, + get_namespace_name(RelationGetNamespace(pk_rel))); + aclresult = pg_class_aclcheck(RelationGetRelid(pk_rel), GetUserId(), + ACL_SELECT); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_TABLE, + RelationGetRelationName(pk_rel)); + + /* + * In the case of scanning the PK index for ri_Check_Pk_Match(), we'd like + * to see all rows that could be interesting, even those that would not be + * visible to the transaction snapshot. To do so, force-push the latest + * snapshot. + * + * Also, increment the command counter to make the changes of the current + * command visible in all cases. + */ + CommandCounterIncrement(); + if (fk_rel == NULL) + { + snap = GetLatestSnapshot(); + PushActiveSnapshot(snap); + pushed_latest_snapshot = true; + } + else + { + snap = GetTransactionSnapshot(); + PushActiveSnapshot(snap); + } + + /* + * Open the constraint index to be scanned. + * + * If the target table is partitioned, we must look up the leaf partition + * and its corresponding unique index to search the keys in. + */ + idxoid = get_fkey_unique_index(constr_id); + if (pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + { + Oid leaf_idxoid; + Snapshot mysnap = InvalidSnapshot; + + /* + * XXX the partition descriptor machinery has a hack that assumes that + * the queries originating in this module push the latest snapshot in + * the transaction-snapshot mode. If we haven't push one already, do + * so here. + */ + if (!pushed_latest_snapshot) + { + mysnap = GetLatestSnapshot(); + PushActiveSnapshot(mysnap); + } + + leaf_pk_rel = find_leaf_part_for_key(pk_rel, riinfo->nkeys, + riinfo->pk_attnums, + pk_vals, pk_nulls, + idxoid, RowShareLock, + &leaf_idxoid); + /* + * XXX done fiddling with the partition descriptor machinery so unset + * the active snapshot if we must. + */ + if (mysnap != InvalidSnapshot) + PopActiveSnapshot(); + + /* + * If no suitable leaf partition exists, neither can the key we're + * looking for. + */ + if (leaf_pk_rel == NULL) + { + SetUserIdAndSecContext(saved_userid, saved_sec_context); + PopActiveSnapshot(); + return false; + } + + pk_rel = leaf_pk_rel; + idxoid = leaf_idxoid; + } + idxrel = index_open(idxoid, RowShareLock); + + /* Set up ScanKeys for the index scan. */ + num_pk = IndexRelationGetNumberOfKeyAttributes(idxrel); + for (i = 0; i < num_pk; i++) + { + int pkattno = i + 1; + Oid operator = eq_oprs[i]; + Oid opfamily = idxrel->rd_opfamily[i]; + StrategyNumber strat = get_op_opfamily_strategy(operator, opfamily); + RegProcedure regop = get_opcode(operator); + + /* Initialize the scankey. */ + ScanKeyInit(&skey[i], + pkattno, + strat, + regop, + pk_vals[i]); + + skey[i].sk_collation = idxrel->rd_indcollation[i]; + + /* + * Check for null value. Should not occur, because callers currently + * take care of the cases in which they do occur. + */ + if (pk_nulls[i] == 'n') + skey[i].sk_flags |= SK_ISNULL; + } + + scan = index_beginscan(pk_rel, idxrel, snap, num_pk, 0); + index_rescan(scan, skey, num_pk, NULL, 0); + + /* Look for the tuple, and if found, try to lock it in key share mode. */ + outslot = table_slot_create(pk_rel, NULL); + if (index_getnext_slot(scan, ForwardScanDirection, outslot)) + { + /* + * If we fail to lock the tuple for whatever reason, assume it doesn't + * exist. + */ + found = ExecLockTableTuple(pk_rel, &(outslot->tts_tid), outslot, + snap, + GetCurrentCommandId(false), + LockTupleKeyShare, + LockWaitBlock, NULL); + } + + index_endscan(scan); + ExecDropSingleTupleTableSlot(outslot); + + /* Don't release lock until commit. */ + index_close(idxrel, NoLock); + + /* Close leaf partition relation if any. */ + if (leaf_pk_rel) + table_close(leaf_pk_rel, NoLock); + + /* Restore UID and security context */ + SetUserIdAndSecContext(saved_userid, saved_sec_context); + + PopActiveSnapshot(); + + return found; +} + +/* + * get_fkey_unique_index + * Returns the unique index used by a supposedly foreign key constraint + */ +static Oid +get_fkey_unique_index(Oid conoid) +{ + Oid result = InvalidOid; + HeapTuple tp; + + tp = SearchSysCache1(CONSTROID, ObjectIdGetDatum(conoid)); + if (HeapTupleIsValid(tp)) + { + Form_pg_constraint contup = (Form_pg_constraint) GETSTRUCT(tp); + + if (contup->contype == CONSTRAINT_FOREIGN) + result = contup->conindid; + ReleaseSysCache(tp); + } + + if (!OidIsValid(result)) + elog(ERROR, "unique index not found for foreign key constraint %u", + conoid); + + return result; +} /* * RI_FKey_check - @@ -244,8 +512,6 @@ RI_FKey_check(TriggerData *trigdata) Relation fk_rel; Relation pk_rel; TupleTableSlot *newslot; - RI_QueryKey qkey; - SPIPlanPtr qplan; riinfo = ri_FetchConstraintInfo(trigdata->tg_trigger, trigdata->tg_relation, false); @@ -325,9 +591,9 @@ RI_FKey_check(TriggerData *trigdata) /* * MATCH PARTIAL - all non-null columns must match. (not - * implemented, can be done by modifying the query below - * to only include non-null columns, or by writing a - * special version here) + * implemented, can be done by modifying + * ri_ReferencedKeyExists() to only include non-null + * columns. */ break; #endif @@ -342,74 +608,12 @@ RI_FKey_check(TriggerData *trigdata) break; } - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); - - /* Fetch or prepare a saved plan for the real check */ - ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_CHECK_LOOKUPPK); - - if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL) - { - StringInfoData querybuf; - char pkrelname[MAX_QUOTED_REL_NAME_LEN]; - char attname[MAX_QUOTED_NAME_LEN]; - char paramname[16]; - const char *querysep; - Oid queryoids[RI_MAX_NUMKEYS]; - const char *pk_only; - - /* ---------- - * The query string built is - * SELECT 1 FROM [ONLY] <pktable> x WHERE pkatt1 = $1 [AND ...] - * FOR KEY SHARE OF x - * The type id's for the $ parameters are those of the - * corresponding FK attributes. - * ---------- - */ - initStringInfo(&querybuf); - pk_only = pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ? - "" : "ONLY "; - quoteRelationName(pkrelname, pk_rel); - appendStringInfo(&querybuf, "SELECT 1 FROM %s%s x", - pk_only, pkrelname); - querysep = "WHERE"; - for (int i = 0; i < riinfo->nkeys; i++) - { - Oid pk_type = RIAttType(pk_rel, riinfo->pk_attnums[i]); - Oid fk_type = RIAttType(fk_rel, riinfo->fk_attnums[i]); - - quoteOneName(attname, - RIAttName(pk_rel, riinfo->pk_attnums[i])); - sprintf(paramname, "$%d", i + 1); - ri_GenerateQual(&querybuf, querysep, - attname, pk_type, - riinfo->pf_eq_oprs[i], - paramname, fk_type); - querysep = "AND"; - queryoids[i] = fk_type; - } - appendStringInfoString(&querybuf, " FOR KEY SHARE OF x"); - - /* Prepare and save the plan */ - qplan = ri_PlanCheck(querybuf.data, riinfo->nkeys, queryoids, - &qkey, fk_rel, pk_rel); - } - - /* - * Now check that foreign key exists in PK table - * - * XXX detectNewRows must be true when a partitioned table is on the - * referenced side. The reason is that our snapshot must be fresh in - * order for the hack in find_inheritance_children() to work. - */ - ri_PerformCheck(riinfo, &qkey, qplan, - fk_rel, pk_rel, - NULL, newslot, - pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE, - SPI_OK_SELECT); - - if (SPI_finish() != SPI_OK_FINISH) - elog(ERROR, "SPI_finish failed"); + if (!ri_ReferencedKeyExists(pk_rel, fk_rel, newslot, riinfo)) + ri_ReportViolation(riinfo, + pk_rel, fk_rel, + newslot, + NULL, + true, false); table_close(pk_rel, RowShareLock); @@ -464,81 +668,10 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel, TupleTableSlot *oldslot, const RI_ConstraintInfo *riinfo) { - SPIPlanPtr qplan; - RI_QueryKey qkey; - bool result; - /* Only called for non-null rows */ Assert(ri_NullCheck(RelationGetDescr(pk_rel), oldslot, riinfo, true) == RI_KEYS_NONE_NULL); - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); - - /* - * Fetch or prepare a saved plan for checking PK table with values coming - * from a PK row - */ - ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_CHECK_LOOKUPPK_FROM_PK); - - if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL) - { - StringInfoData querybuf; - char pkrelname[MAX_QUOTED_REL_NAME_LEN]; - char attname[MAX_QUOTED_NAME_LEN]; - char paramname[16]; - const char *querysep; - const char *pk_only; - Oid queryoids[RI_MAX_NUMKEYS]; - - /* ---------- - * The query string built is - * SELECT 1 FROM [ONLY] <pktable> x WHERE pkatt1 = $1 [AND ...] - * FOR KEY SHARE OF x - * The type id's for the $ parameters are those of the - * PK attributes themselves. - * ---------- - */ - initStringInfo(&querybuf); - pk_only = pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ? - "" : "ONLY "; - quoteRelationName(pkrelname, pk_rel); - appendStringInfo(&querybuf, "SELECT 1 FROM %s%s x", - pk_only, pkrelname); - querysep = "WHERE"; - for (int i = 0; i < riinfo->nkeys; i++) - { - Oid pk_type = RIAttType(pk_rel, riinfo->pk_attnums[i]); - - quoteOneName(attname, - RIAttName(pk_rel, riinfo->pk_attnums[i])); - sprintf(paramname, "$%d", i + 1); - ri_GenerateQual(&querybuf, querysep, - attname, pk_type, - riinfo->pp_eq_oprs[i], - paramname, pk_type); - querysep = "AND"; - queryoids[i] = pk_type; - } - appendStringInfoString(&querybuf, " FOR KEY SHARE OF x"); - - /* Prepare and save the plan */ - qplan = ri_PlanCheck(querybuf.data, riinfo->nkeys, queryoids, - &qkey, fk_rel, pk_rel); - } - - /* - * We have a plan now. Run it. - */ - result = ri_PerformCheck(riinfo, &qkey, qplan, - fk_rel, pk_rel, - oldslot, NULL, - true, /* treat like update */ - SPI_OK_SELECT); - - if (SPI_finish() != SPI_OK_FINISH) - elog(ERROR, "SPI_finish failed"); - - return result; + return ri_ReferencedKeyExists(pk_rel, NULL, oldslot, riinfo); } @@ -1602,15 +1735,10 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) errtableconstraint(fk_rel, NameStr(fake_riinfo.conname)))); - /* - * We tell ri_ReportViolation we were doing the RI_PLAN_CHECK_LOOKUPPK - * query, which isn't true, but will cause it to use - * fake_riinfo.fk_attnums as we need. - */ ri_ReportViolation(&fake_riinfo, pk_rel, fk_rel, slot, tupdesc, - RI_PLAN_CHECK_LOOKUPPK, false); + true, false); ExecDropSingleTupleTableSlot(slot); } @@ -1827,7 +1955,7 @@ RI_PartitionRemove_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) fake_riinfo.pk_attnums[i] = i + 1; ri_ReportViolation(&fake_riinfo, pk_rel, fk_rel, - slot, tupdesc, 0, true); + slot, tupdesc, true, true); } if (SPI_finish() != SPI_OK_FINISH) @@ -1964,26 +2092,25 @@ ri_BuildQueryKey(RI_QueryKey *key, const RI_ConstraintInfo *riinfo, { /* * Inherited constraints with a common ancestor can share ri_query_cache - * entries for all query types except RI_PLAN_CHECK_LOOKUPPK_FROM_PK. - * Except in that case, the query processes the other table involved in - * the FK constraint (i.e., not the table on which the trigger has been - * fired), and so it will be the same for all members of the inheritance - * tree. So we may use the root constraint's OID in the hash key, rather - * than the constraint's own OID. This avoids creating duplicate SPI - * plans, saving lots of work and memory when there are many partitions - * with similar FK constraints. + * entries, because each query processes the other table involved in the + * FK constraint (i.e., not the table on which the trigger has been fired), + * and so it will be the same for all members of the inheritance tree. So + * we may use the root constraint's OID in the hash key, rather than the + * constraint's own OID. This avoids creating duplicate SPI plans, saving + * lots of work and memory when there are many partitions with similar FK + * constraints. * * (Note that we must still have a separate RI_ConstraintInfo for each * constraint, because partitions can have different column orders, * resulting in different pk_attnums[] or fk_attnums[] array contents.) * + * (Note also that for a standalone or non-inherited constraint, + * constraint_root_id is same as constraint_id.) + * * We assume struct RI_QueryKey contains no padding bytes, else we'd need * to use memset to clear them. */ - if (constr_queryno != RI_PLAN_CHECK_LOOKUPPK_FROM_PK) - key->constr_id = riinfo->constraint_root_id; - else - key->constr_id = riinfo->constraint_id; + key->constr_id = riinfo->constraint_root_id; key->constr_queryno = constr_queryno; } @@ -2254,19 +2381,11 @@ ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes, RI_QueryKey *qkey, Relation fk_rel, Relation pk_rel) { SPIPlanPtr qplan; - Relation query_rel; + /* There are currently no queries that run on PK table. */ + Relation query_rel = fk_rel; Oid save_userid; int save_sec_context; - /* - * Use the query type code to determine whether the query is run against - * the PK or FK table; we'll do the check as that table's owner - */ - if (qkey->constr_queryno <= RI_PLAN_LAST_ON_PK) - query_rel = pk_rel; - else - query_rel = fk_rel; - /* Switch to proper UID to perform check as */ GetUserIdAndSecContext(&save_userid, &save_sec_context); SetUserIdAndSecContext(RelationGetForm(query_rel)->relowner, @@ -2299,9 +2418,9 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, TupleTableSlot *oldslot, TupleTableSlot *newslot, bool detectNewRows, int expect_OK) { - Relation query_rel, - source_rel; - bool source_is_pk; + /* There are currently no queries that run on PK table. */ + Relation query_rel = fk_rel, + source_rel = pk_rel; Snapshot test_snapshot; Snapshot crosscheck_snapshot; int limit; @@ -2311,46 +2430,17 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, Datum vals[RI_MAX_NUMKEYS * 2]; char nulls[RI_MAX_NUMKEYS * 2]; - /* - * Use the query type code to determine whether the query is run against - * the PK or FK table; we'll do the check as that table's owner - */ - if (qkey->constr_queryno <= RI_PLAN_LAST_ON_PK) - query_rel = pk_rel; - else - query_rel = fk_rel; - - /* - * The values for the query are taken from the table on which the trigger - * is called - it is normally the other one with respect to query_rel. An - * exception is ri_Check_Pk_Match(), which uses the PK table for both (and - * sets queryno to RI_PLAN_CHECK_LOOKUPPK_FROM_PK). We might eventually - * need some less klugy way to determine this. - */ - if (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK) - { - source_rel = fk_rel; - source_is_pk = false; - } - else - { - source_rel = pk_rel; - source_is_pk = true; - } - /* Extract the parameters to be passed into the query */ if (newslot) { - ri_ExtractValues(source_rel, newslot, riinfo, source_is_pk, - vals, nulls); + ri_ExtractValues(source_rel, newslot, riinfo, true, vals, nulls); if (oldslot) - ri_ExtractValues(source_rel, oldslot, riinfo, source_is_pk, + ri_ExtractValues(source_rel, oldslot, riinfo, true, vals + riinfo->nkeys, nulls + riinfo->nkeys); } else { - ri_ExtractValues(source_rel, oldslot, riinfo, source_is_pk, - vals, nulls); + ri_ExtractValues(source_rel, oldslot, riinfo, true, vals, nulls); } /* @@ -2414,14 +2504,12 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, errhint("This is most likely due to a rule having rewritten the query."))); /* XXX wouldn't it be clearer to do this part at the caller? */ - if (qkey->constr_queryno != RI_PLAN_CHECK_LOOKUPPK_FROM_PK && - expect_OK == SPI_OK_SELECT && - (SPI_processed == 0) == (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK)) + if (expect_OK == SPI_OK_SELECT && SPI_processed != 0) ri_ReportViolation(riinfo, pk_rel, fk_rel, newslot ? newslot : oldslot, NULL, - qkey->constr_queryno, false); + false, false); return SPI_processed != 0; } @@ -2452,9 +2540,9 @@ ri_ExtractValues(Relation rel, TupleTableSlot *slot, /* * Produce an error report * - * If the failed constraint was on insert/update to the FK table, - * we want the key names and values extracted from there, and the error - * message to look like 'key blah is not present in PK'. + * If the failed constraint was on insert/update to the FK table (on_fk is + * true), we want the key names and values extracted from there, and the + * error message to look like 'key blah is not present in PK'. * Otherwise, the attr names and values come from the PK table and the * message looks like 'key blah is still referenced from FK'. */ @@ -2462,22 +2550,20 @@ static void ri_ReportViolation(const RI_ConstraintInfo *riinfo, Relation pk_rel, Relation fk_rel, TupleTableSlot *violatorslot, TupleDesc tupdesc, - int queryno, bool partgone) + bool on_fk, bool partgone) { StringInfoData key_names; StringInfoData key_values; - bool onfk; const int16 *attnums; Oid rel_oid; AclResult aclresult; bool has_perm = true; /* - * Determine which relation to complain about. If tupdesc wasn't passed - * by caller, assume the violator tuple came from there. + * If tupdesc wasn't passed by caller, assume the violator tuple came from + * there. */ - onfk = (queryno == RI_PLAN_CHECK_LOOKUPPK); - if (onfk) + if (on_fk) { attnums = riinfo->fk_attnums; rel_oid = fk_rel->rd_id; @@ -2579,7 +2665,7 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo, key_names.data, key_values.data, RelationGetRelationName(fk_rel)), errtableconstraint(fk_rel, NameStr(riinfo->conname)))); - else if (onfk) + else if (on_fk) ereport(ERROR, (errcode(ERRCODE_FOREIGN_KEY_VIOLATION), errmsg("insert or update on table \"%s\" violates foreign key constraint \"%s\"", @@ -2886,7 +2972,10 @@ ri_AttributesEqual(Oid eq_opr, Oid typeid, * ri_HashCompareOp - * * See if we know how to compare two values, and create a new hash entry - * if not. + * if not. The entry contains the FmgrInfo of the equality operator function + * and that of the cast function, if one is needed to convert the right + * operand (whose type OID has been passed) before passing it to the equality + * function. */ static RI_CompareHashEntry * ri_HashCompareOp(Oid eq_opr, Oid typeid) @@ -2942,8 +3031,16 @@ ri_HashCompareOp(Oid eq_opr, Oid typeid) * moment since that will never be generated for implicit coercions. */ op_input_types(eq_opr, &lefttype, &righttype); - Assert(lefttype == righttype); - if (typeid == lefttype) + + /* + * Don't need to cast if the values that will be passed to the + * operator will be of expected operand type(s). The operator can be + * cross-type (such as when called by ri_ReferencedKeyExists()), in + * which case, we only need the cast if the right operand value + * doesn't match the type expected by the operator. + */ + if ((lefttype == righttype && typeid == lefttype) || + (lefttype != righttype && typeid == righttype)) castfunc = InvalidOid; /* simplest case */ else { diff --git a/src/include/executor/execPartition.h b/src/include/executor/execPartition.h index 603d8becc4..e63dcb12f6 100644 --- a/src/include/executor/execPartition.h +++ b/src/include/executor/execPartition.h @@ -124,5 +124,11 @@ extern PartitionPruneState *ExecCreatePartitionPruneState(PlanState *planstate, extern Bitmapset *ExecFindMatchingSubPlans(PartitionPruneState *prunestate); extern Bitmapset *ExecFindInitialMatchingSubPlans(PartitionPruneState *prunestate, int nsubplans); +extern Relation find_leaf_part_for_key(Relation root_rel, + int key_natts, + const AttrNumber *key_attnums, + Datum *key_vals, char *key_nulls, + Oid root_idxoid, int lockmode, + Oid *leaf_idxoid); #endif /* EXECPARTITION_H */ diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index 344399f6a8..8f32353a44 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -241,6 +241,15 @@ extern bool ExecShutdownNode(PlanState *node); extern void ExecSetTupleBound(int64 tuples_needed, PlanState *child_node); +/* + * functions in execLockRows.c + */ + +extern bool ExecLockTableTuple(Relation relation, ItemPointer tid, TupleTableSlot *slot, + Snapshot snapshot, CommandId cid, + LockTupleMode lockmode, LockWaitPolicy waitPolicy, + bool *epq_needed); + /* ---------------------------------------------------------------- * ExecProcNode * diff --git a/src/test/isolation/expected/fk-snapshot.out b/src/test/isolation/expected/fk-snapshot.out index 5faf80d6ce..22752cc742 100644 --- a/src/test/isolation/expected/fk-snapshot.out +++ b/src/test/isolation/expected/fk-snapshot.out @@ -47,12 +47,12 @@ a step s2ifn2: INSERT INTO fk_noparted VALUES (2); step s2c: COMMIT; +ERROR: insert or update on table "fk_noparted" violates foreign key constraint "fk_noparted_a_fkey" step s2sfn: SELECT * FROM fk_noparted; a - 1 -2 -(2 rows) +(1 row) starting permutation: s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 s2c s2sfn diff --git a/src/test/isolation/specs/fk-snapshot.spec b/src/test/isolation/specs/fk-snapshot.spec index 378507fbc3..64d27f29c3 100644 --- a/src/test/isolation/specs/fk-snapshot.spec +++ b/src/test/isolation/specs/fk-snapshot.spec @@ -46,10 +46,7 @@ step s2sfn { SELECT * FROM fk_noparted; } # inserting into referencing tables in transaction-snapshot mode # PK table is non-partitioned permutation s1brr s2brc s2ip2 s1sp s2c s1sp s1ifp2 s1c s1sfp -# PK table is partitioned: buggy, because s2's serialization transaction can -# see the uncommitted row thanks to the latest snapshot taken for -# partition lookup to work correctly also ends up getting used by the PK index -# scan +# PK table is partitioned permutation s2ip2 s2brr s1brc s1ifp2 s2sfp s1c s2sfp s2ifn2 s2c s2sfn # inserting into referencing tables in up-to-date snapshot mode -- 2.24.1 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: simplifying foreign key/RI checks 2022-01-18 06:30 Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-03-14 08:33 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> @ 2022-03-14 09:32 ` Zhihong Yu <[email protected]> 2022-03-22 04:01 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Zhihong Yu @ 2022-03-14 09:32 UTC (permalink / raw) To: Amit Langote <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Alvaro Herrera <[email protected]>; vignesh C <[email protected]>; Kyotaro Horiguchi <[email protected]>; PostgreSQL Developers <[email protected]> On Mon, Mar 14, 2022 at 1:33 AM Amit Langote <[email protected]> wrote: > On Tue, Jan 18, 2022 at 3:30 PM Amit Langote <[email protected]> > wrote: > > v13 is attached. > > I noticed that the recent 641f3dffcdf's changes to > get_constraint_index() made it basically unusable for this patch's > purposes. > > Reading in the thread that led to 641f3dffcdf why > get_constraint_index() was changed the way it was, I invented in the > attached updated patch a get_fkey_constraint_index() that is local to > ri_triggers.c for use by the new ri_ReferencedKeyExists(), replacing > get_constraint_index() that no longer gives it the index it's looking > for. > > -- > Amit Langote > EDB: http://www.enterprisedb.com Hi, + partkey_isnull[j] = (key_nulls[k] == 'n' ? true : false); The above can be shortened as: partkey_isnull[j] = key_nulls[k] == 'n'; + * May neeed to cast each of the individual values of the foreign key neeed -> need Cheers ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: simplifying foreign key/RI checks 2022-01-18 06:30 Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-03-14 08:33 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-03-14 09:32 ` Re: simplifying foreign key/RI checks Zhihong Yu <[email protected]> @ 2022-03-22 04:01 ` Amit Langote <[email protected]> 2022-04-07 01:05 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Amit Langote @ 2022-03-22 04:01 UTC (permalink / raw) To: Zhihong Yu <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Alvaro Herrera <[email protected]>; vignesh C <[email protected]>; Kyotaro Horiguchi <[email protected]>; PostgreSQL Developers <[email protected]> On Mon, Mar 14, 2022 at 6:28 PM Zhihong Yu <[email protected]> wrote: > On Mon, Mar 14, 2022 at 1:33 AM Amit Langote <[email protected]> wrote: >> On Tue, Jan 18, 2022 at 3:30 PM Amit Langote <[email protected]> wrote: >> > v13 is attached. >> >> I noticed that the recent 641f3dffcdf's changes to >> get_constraint_index() made it basically unusable for this patch's >> purposes. >> >> Reading in the thread that led to 641f3dffcdf why >> get_constraint_index() was changed the way it was, I invented in the >> attached updated patch a get_fkey_constraint_index() that is local to >> ri_triggers.c for use by the new ri_ReferencedKeyExists(), replacing >> get_constraint_index() that no longer gives it the index it's looking >> for. >> > > Hi, > + partkey_isnull[j] = (key_nulls[k] == 'n' ? true : false); > > The above can be shortened as: > > partkey_isnull[j] = key_nulls[k] == 'n'; > > + * May neeed to cast each of the individual values of the foreign key > > neeed -> need Both fixed, thanks. -- Amit Langote EDB: http://www.enterprisedb.com Attachments: [application/octet-stream] v15-0001-Add-isolation-tests-for-snapshot-behavior-in-ri_.patch (6.5K, ../../CA+HiwqEGATLvKk8DCT3zm9AtZ02N_M0D5kiFe-+mC6LqFBc8qA@mail.gmail.com/2-v15-0001-Add-isolation-tests-for-snapshot-behavior-in-ri_.patch) download | inline diff: From 9824d2801d6082afdff1e0a0091d0b1446258c48 Mon Sep 17 00:00:00 2001 From: amitlan <[email protected]> Date: Mon, 15 Nov 2021 18:22:33 +0900 Subject: [PATCH v15 1/2] Add isolation tests for snapshot behavior in ri_triggers.c They are to check the behavior of RI_FKey_check() and ri_Check_Pk_Match(). A test case whereby RI_FKey_check() queries a partitioned PK table under REPEATABLE READ isolation produces wrong output due to a bug of the partition-descriptor logic and that is noted as such in the comment above the test. A subsequent patch will fix the bug and replace the buggy output by the correct one. --- src/test/isolation/expected/fk-snapshot.out | 124 ++++++++++++++++++++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/fk-snapshot.spec | 61 ++++++++++ 3 files changed, 186 insertions(+) create mode 100644 src/test/isolation/expected/fk-snapshot.out create mode 100644 src/test/isolation/specs/fk-snapshot.spec diff --git a/src/test/isolation/expected/fk-snapshot.out b/src/test/isolation/expected/fk-snapshot.out new file mode 100644 index 0000000000..5faf80d6ce --- /dev/null +++ b/src/test/isolation/expected/fk-snapshot.out @@ -0,0 +1,124 @@ +Parsed test spec with 2 sessions + +starting permutation: s1brr s2brc s2ip2 s1sp s2c s1sp s1ifp2 s1c s1sfp +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s2brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +(1 row) + +step s2c: COMMIT; +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +(1 row) + +step s1ifp2: INSERT INTO fk_parted_pk VALUES (2); +ERROR: insert or update on table "fk_parted_pk_2" violates foreign key constraint "fk_parted_pk_a_fkey" +step s1c: COMMIT; +step s1sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + + +starting permutation: s2ip2 s2brr s1brc s1ifp2 s2sfp s1c s2sfp s2ifn2 s2c s2sfn +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s2brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s1ifp2: INSERT INTO fk_parted_pk VALUES (2); +step s2sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + +step s1c: COMMIT; +step s2sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + +step s2ifn2: INSERT INTO fk_noparted VALUES (2); +step s2c: COMMIT; +step s2sfn: SELECT * FROM fk_noparted; +a +- +1 +2 +(2 rows) + + +starting permutation: s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 s2c s2sfn +step s1brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +(1 row) + +step s2c: COMMIT; +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +2 +(2 rows) + +step s1ifp2: INSERT INTO fk_parted_pk VALUES (2); +step s2brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + +step s1c: COMMIT; +step s1sfp: SELECT * FROM fk_parted_pk; +a +- +1 +2 +(2 rows) + +step s2ifn2: INSERT INTO fk_noparted VALUES (2); +step s2c: COMMIT; +step s2sfn: SELECT * FROM fk_noparted; +a +- +1 +2 +(2 rows) + + +starting permutation: s1brr s1dfp s1ifp1 s1c s1sfn +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1dfp: DELETE FROM fk_parted_pk WHERE a = 1; +step s1ifp1: INSERT INTO fk_parted_pk VALUES (1); +step s1c: COMMIT; +step s1sfn: SELECT * FROM fk_noparted; +a +- +1 +(1 row) + + +starting permutation: s1brc s1dfp s1ifp1 s1c s1sfn +step s1brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s1dfp: DELETE FROM fk_parted_pk WHERE a = 1; +step s1ifp1: INSERT INTO fk_parted_pk VALUES (1); +step s1c: COMMIT; +step s1sfn: SELECT * FROM fk_noparted; +a +- +1 +(1 row) + diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 8e87098150..6907c342aa 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -33,6 +33,7 @@ test: fk-deadlock test: fk-deadlock2 test: fk-partitioned-1 test: fk-partitioned-2 +test: fk-snapshot test: eval-plan-qual test: eval-plan-qual-trigger test: lock-update-delete diff --git a/src/test/isolation/specs/fk-snapshot.spec b/src/test/isolation/specs/fk-snapshot.spec new file mode 100644 index 0000000000..378507fbc3 --- /dev/null +++ b/src/test/isolation/specs/fk-snapshot.spec @@ -0,0 +1,61 @@ +setup +{ + CREATE TABLE pk_noparted ( + a int PRIMARY KEY + ); + + CREATE TABLE fk_parted_pk ( + a int PRIMARY KEY REFERENCES pk_noparted ON DELETE CASCADE + ) PARTITION BY LIST (a); + CREATE TABLE fk_parted_pk_1 PARTITION OF fk_parted_pk FOR VALUES IN (1); + CREATE TABLE fk_parted_pk_2 PARTITION OF fk_parted_pk FOR VALUES IN (2); + + CREATE TABLE fk_noparted ( + a int REFERENCES fk_parted_pk ON DELETE NO ACTION INITIALLY DEFERRED + ); + INSERT INTO pk_noparted VALUES (1); + INSERT INTO fk_parted_pk VALUES (1); + INSERT INTO fk_noparted VALUES (1); +} + +teardown +{ + DROP TABLE pk_noparted, fk_parted_pk, fk_noparted; +} + +session s1 +step s1brr { BEGIN ISOLATION LEVEL REPEATABLE READ; } +step s1brc { BEGIN ISOLATION LEVEL READ COMMITTED; } +step s1ifp2 { INSERT INTO fk_parted_pk VALUES (2); } +step s1ifp1 { INSERT INTO fk_parted_pk VALUES (1); } +step s1dfp { DELETE FROM fk_parted_pk WHERE a = 1; } +step s1c { COMMIT; } +step s1sfp { SELECT * FROM fk_parted_pk; } +step s1sp { SELECT * FROM pk_noparted; } +step s1sfn { SELECT * FROM fk_noparted; } + +session s2 +step s2brr { BEGIN ISOLATION LEVEL REPEATABLE READ; } +step s2brc { BEGIN ISOLATION LEVEL READ COMMITTED; } +step s2ip2 { INSERT INTO pk_noparted VALUES (2); } +step s2ifn2 { INSERT INTO fk_noparted VALUES (2); } +step s2c { COMMIT; } +step s2sfp { SELECT * FROM fk_parted_pk; } +step s2sfn { SELECT * FROM fk_noparted; } + +# inserting into referencing tables in transaction-snapshot mode +# PK table is non-partitioned +permutation s1brr s2brc s2ip2 s1sp s2c s1sp s1ifp2 s1c s1sfp +# PK table is partitioned: buggy, because s2's serialization transaction can +# see the uncommitted row thanks to the latest snapshot taken for +# partition lookup to work correctly also ends up getting used by the PK index +# scan +permutation s2ip2 s2brr s1brc s1ifp2 s2sfp s1c s2sfp s2ifn2 s2c s2sfn + +# inserting into referencing tables in up-to-date snapshot mode +permutation s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 s2c s2sfn + +# deleting a referenced row and then inserting again in the same transaction; works +# the same no matter the snapshot mode +permutation s1brr s1dfp s1ifp1 s1c s1sfn +permutation s1brc s1dfp s1ifp1 s1c s1sfn -- 2.24.1 [application/octet-stream] v15-0002-Avoid-using-SPI-for-some-RI-checks.patch (40.7K, ../../CA+HiwqEGATLvKk8DCT3zm9AtZ02N_M0D5kiFe-+mC6LqFBc8qA@mail.gmail.com/3-v15-0002-Avoid-using-SPI-for-some-RI-checks.patch) download | inline diff: From 437e9c63cafa0263caf204b93cc76f74994903bd Mon Sep 17 00:00:00 2001 From: amitlan <[email protected]> Date: Tue, 12 Jan 2021 14:17:31 +0900 Subject: [PATCH v15 2/2] Avoid using SPI for some RI checks This modifies the subroutines called by RI trigger functions that want to check if a given referenced value exists in the referenced relation to simply scan the foreign key constraint's unique index. That replaces the current way of issuing a `SELECT 1 FROM referenced_relation WHERE ref_key = $1` query through SPI to do the same. This saves a lot of work, especially when inserting into or updating a referencing relation. This rewrite allows to fix a PK row visibility bug caused by a partition descriptor hack which requires ActiveSnapshot to be set to come up with the correct set of partitions for the RI query running under REPEATABLE READ isolation. We now set that snapshot indepedently of the snapshot to be used by the PK index scan, so the two no longer interfere. The buggy output in src/test/isolation/expected/fk-snapshot.out of the relevant test case has been corrected. --- src/backend/executor/execPartition.c | 160 +++++- src/backend/executor/nodeLockRows.c | 160 +++--- src/backend/utils/adt/ri_triggers.c | 573 ++++++++++++-------- src/include/executor/execPartition.h | 6 + src/include/executor/executor.h | 9 + src/test/isolation/expected/fk-snapshot.out | 4 +- src/test/isolation/specs/fk-snapshot.spec | 5 +- 7 files changed, 596 insertions(+), 321 deletions(-) diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 90ed1485d1..bfdc06eaa9 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -175,8 +175,9 @@ static void FormPartitionKeyDatum(PartitionDispatch pd, EState *estate, Datum *values, bool *isnull); -static int get_partition_for_tuple(PartitionDispatch pd, Datum *values, - bool *isnull); +static int get_partition_for_tuple(PartitionKey key, + PartitionDesc partdesc, + Datum *values, bool *isnull); static char *ExecBuildSlotPartitionKeyDescription(Relation rel, Datum *values, bool *isnull, @@ -310,7 +311,9 @@ ExecFindPartition(ModifyTableState *mtstate, * these values, error out. */ if (partdesc->nparts == 0 || - (partidx = get_partition_for_tuple(dispatch, values, isnull)) < 0) + (partidx = get_partition_for_tuple(dispatch->key, + dispatch->partdesc, + values, isnull)) < 0) { char *val_desc; @@ -1240,12 +1243,12 @@ FormPartitionKeyDatum(PartitionDispatch pd, * found or -1 if none found. */ static int -get_partition_for_tuple(PartitionDispatch pd, Datum *values, bool *isnull) +get_partition_for_tuple(PartitionKey key, + PartitionDesc partdesc, + Datum *values, bool *isnull) { int bound_offset; int part_index = -1; - PartitionKey key = pd->key; - PartitionDesc partdesc = pd->partdesc; PartitionBoundInfo boundinfo = partdesc->boundinfo; /* Route as appropriate based on partitioning strategy. */ @@ -1337,6 +1340,151 @@ get_partition_for_tuple(PartitionDispatch pd, Datum *values, bool *isnull) return part_index; } +/* + * find_leaf_part_for_key + * Finds the leaf partition of a partitioned table 'root_rel' that might + * contain the specified key tuple containing a subset of the table's + * columns (including all of the partition key columns) + * + * 'key_natts' specifies the number columns contained in the key, + * 'key_attnums' their attribute numbers as defined in 'root_rel', and + * 'key_vals' and 'key_nulls' specify the key tuple. + * + * Returns NULL if no leaf partition is found for the key. Caller must close + * the relation. + * + * This works because the unique key defined on the root relation is required + * to contain the partition key columns of all of the ancestors that lead up to + * a given leaf partition. + */ +Relation +find_leaf_part_for_key(Relation root_rel, int key_natts, + const AttrNumber *key_attnums, + Datum *key_vals, char *key_nulls, + Oid root_idxoid, int lockmode, + Oid *leaf_idxoid) +{ + Relation rel = root_rel; + Oid constr_idxoid = root_idxoid; + + *leaf_idxoid = InvalidOid; + + /* + * Descend through partitioned parents to find the leaf partition that + * would accept a row with the provided key values, starting with the root + * parent. + */ + while (true) + { + PartitionKey partkey = RelationGetPartitionKey(rel); + PartitionDirectory partdir; + PartitionDesc partdesc; + Datum partkey_vals[PARTITION_MAX_KEYS]; + bool partkey_isnull[PARTITION_MAX_KEYS]; + AttrNumber *root_partattrs = partkey->partattrs; + int i, + j; + int partidx; + Oid partoid; + bool is_leaf; + + /* + * Collect partition key values from the unique key. + * + * Because we only have the root table's copy of pk_attnums, must map + * any non-root table's partition key attribute numbers to the root + * table's. + */ + if (rel != root_rel) + { + /* + * map->attnums will contain root table attribute numbers for each + * attribute of the current partitioned relation. + */ + AttrMap *map = build_attrmap_by_name_if_req(RelationGetDescr(root_rel), + RelationGetDescr(rel)); + + if (map) + { + root_partattrs = palloc(partkey->partnatts * + sizeof(AttrNumber)); + for (i = 0; i < partkey->partnatts; i++) + { + AttrNumber partattno = partkey->partattrs[i]; + + root_partattrs[i] = map->attnums[partattno - 1]; + } + + free_attrmap(map); + } + } + + /* + * Referenced key specification does not allow expressions, so there + * would not be expressions in the partition keys either. + */ + Assert(partkey->partexprs == NIL); + for (i = 0, j = 0; i < partkey->partnatts; i++) + { + int k; + + for (k = 0; k < key_natts; k++) + { + if (root_partattrs[i] == key_attnums[k]) + { + partkey_vals[j] = key_vals[k]; + partkey_isnull[j] = (key_nulls[k] == 'n'); + j++; + break; + } + } + } + /* Had better have found values for all of the partition keys. */ + Assert(j == partkey->partnatts); + + if (root_partattrs != partkey->partattrs) + pfree(root_partattrs); + + /* Get the PartitionDesc using the partition directory machinery. */ + partdir = CreatePartitionDirectory(CurrentMemoryContext, true); + partdesc = PartitionDirectoryLookup(partdir, rel); + + /* Find the partition for the key. */ + partidx = get_partition_for_tuple(partkey, partdesc, + partkey_vals, partkey_isnull); + Assert(partidx < 0 || partidx < partdesc->nparts); + + /* done using the partition directory */ + DestroyPartitionDirectory(partdir); + + /* close any intermediate parents we opened */ + if (rel != root_rel) + table_close(rel, NoLock); + + /* No partition found. */ + if (partidx < 0) + return NULL; + + partoid = partdesc->oids[partidx]; + rel = table_open(partoid, lockmode); + constr_idxoid = index_get_partition(rel, constr_idxoid); + + /* + * Return if the partition is a leaf, else find its partition in the + * next iteration. + */ + is_leaf = partdesc->is_leaf[partidx]; + if (is_leaf) + { + *leaf_idxoid = constr_idxoid; + return rel; + } + } + + Assert(false); + return NULL; +} + /* * ExecBuildSlotPartitionKeyDescription * diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c index 1a9dab25dd..ab54a65e0e 100644 --- a/src/backend/executor/nodeLockRows.c +++ b/src/backend/executor/nodeLockRows.c @@ -79,10 +79,7 @@ lnext: Datum datum; bool isNull; ItemPointerData tid; - TM_FailureData tmfd; LockTupleMode lockmode; - int lockflags = 0; - TM_Result test; TupleTableSlot *markSlot; /* clear any leftover test tuple for this rel */ @@ -179,74 +176,11 @@ lnext: break; } - lockflags = TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS; - if (!IsolationUsesXactSnapshot()) - lockflags |= TUPLE_LOCK_FLAG_FIND_LAST_VERSION; - - test = table_tuple_lock(erm->relation, &tid, estate->es_snapshot, - markSlot, estate->es_output_cid, - lockmode, erm->waitPolicy, - lockflags, - &tmfd); - - switch (test) - { - case TM_WouldBlock: - /* couldn't lock tuple in SKIP LOCKED mode */ - goto lnext; - - case TM_SelfModified: - - /* - * The target tuple was already updated or deleted by the - * current command, or by a later command in the current - * transaction. We *must* ignore the tuple in the former - * case, so as to avoid the "Halloween problem" of repeated - * update attempts. In the latter case it might be sensible - * to fetch the updated tuple instead, but doing so would - * require changing heap_update and heap_delete to not - * complain about updating "invisible" tuples, which seems - * pretty scary (table_tuple_lock will not complain, but few - * callers expect TM_Invisible, and we're not one of them). So - * for now, treat the tuple as deleted and do not process. - */ - goto lnext; - - case TM_Ok: - - /* - * Got the lock successfully, the locked tuple saved in - * markSlot for, if needed, EvalPlanQual testing below. - */ - if (tmfd.traversed) - epq_needed = true; - break; - - case TM_Updated: - if (IsolationUsesXactSnapshot()) - ereport(ERROR, - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), - errmsg("could not serialize access due to concurrent update"))); - elog(ERROR, "unexpected table_tuple_lock status: %u", - test); - break; - - case TM_Deleted: - if (IsolationUsesXactSnapshot()) - ereport(ERROR, - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), - errmsg("could not serialize access due to concurrent update"))); - /* tuple was deleted so don't return it */ - goto lnext; - - case TM_Invisible: - elog(ERROR, "attempted to lock invisible tuple"); - break; - - default: - elog(ERROR, "unrecognized table_tuple_lock status: %u", - test); - } + /* skip tuple if it couldn't be locked */ + if (!ExecLockTableTuple(erm->relation, &tid, markSlot, + estate->es_snapshot, estate->es_output_cid, + lockmode, erm->waitPolicy, &epq_needed)) + goto lnext; /* Remember locked tuple's TID for EPQ testing and WHERE CURRENT OF */ erm->curCtid = tid; @@ -281,6 +215,90 @@ lnext: return slot; } +/* + * ExecLockTableTuple + * Locks tuple with the specified TID in lockmode following given wait + * policy + * + * Returns true if the tuple was successfully locked. Locked tuple is loaded + * into provided slot. + */ +bool +ExecLockTableTuple(Relation relation, ItemPointer tid, TupleTableSlot *slot, + Snapshot snapshot, CommandId cid, + LockTupleMode lockmode, LockWaitPolicy waitPolicy, + bool *epq_needed) +{ + TM_FailureData tmfd; + int lockflags = TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS; + TM_Result test; + + if (!IsolationUsesXactSnapshot()) + lockflags |= TUPLE_LOCK_FLAG_FIND_LAST_VERSION; + + test = table_tuple_lock(relation, tid, snapshot, slot, cid, lockmode, + waitPolicy, lockflags, &tmfd); + + switch (test) + { + case TM_WouldBlock: + /* couldn't lock tuple in SKIP LOCKED mode */ + return false; + + case TM_SelfModified: + /* + * The target tuple was already updated or deleted by the + * current command, or by a later command in the current + * transaction. We *must* ignore the tuple in the former + * case, so as to avoid the "Halloween problem" of repeated + * update attempts. In the latter case it might be sensible + * to fetch the updated tuple instead, but doing so would + * require changing heap_update and heap_delete to not + * complain about updating "invisible" tuples, which seems + * pretty scary (table_tuple_lock will not complain, but few + * callers expect TM_Invisible, and we're not one of them). So + * for now, treat the tuple as deleted and do not process. + */ + return false; + + case TM_Ok: + /* + * Got the lock successfully, the locked tuple saved in + * slot for EvalPlanQual, if asked by the caller. + */ + if (tmfd.traversed && epq_needed) + *epq_needed = true; + break; + + case TM_Updated: + if (IsolationUsesXactSnapshot()) + ereport(ERROR, + (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), + errmsg("could not serialize access due to concurrent update"))); + elog(ERROR, "unexpected table_tuple_lock status: %u", + test); + break; + + case TM_Deleted: + if (IsolationUsesXactSnapshot()) + ereport(ERROR, + (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), + errmsg("could not serialize access due to concurrent update"))); + /* tuple was deleted so don't return it */ + return false; + + case TM_Invisible: + elog(ERROR, "attempted to lock invisible tuple"); + return false; + + default: + elog(ERROR, "unrecognized table_tuple_lock status: %u", test); + return false; + } + + return true; +} + /* ---------------------------------------------------------------- * ExecInitLockRows * diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 01d4c22cfc..11fd4ec500 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -23,22 +23,27 @@ #include "postgres.h" +#include "access/genam.h" #include "access/htup_details.h" +#include "access/skey.h" #include "access/sysattr.h" #include "access/table.h" #include "access/tableam.h" #include "access/xact.h" +#include "catalog/partition.h" #include "catalog/pg_collation.h" #include "catalog/pg_constraint.h" #include "catalog/pg_operator.h" #include "catalog/pg_type.h" #include "commands/trigger.h" +#include "executor/execPartition.h" #include "executor/executor.h" #include "executor/spi.h" #include "lib/ilist.h" #include "miscadmin.h" #include "parser/parse_coerce.h" #include "parser/parse_relation.h" +#include "partitioning/partdesc.h" #include "storage/bufmgr.h" #include "utils/acl.h" #include "utils/builtins.h" @@ -48,6 +53,7 @@ #include "utils/inval.h" #include "utils/lsyscache.h" #include "utils/memutils.h" +#include "utils/partcache.h" #include "utils/rel.h" #include "utils/rls.h" #include "utils/ruleutils.h" @@ -68,19 +74,15 @@ #define RI_KEYS_NONE_NULL 2 /* RI query type codes */ -/* these queries are executed against the PK (referenced) table: */ -#define RI_PLAN_CHECK_LOOKUPPK 1 -#define RI_PLAN_CHECK_LOOKUPPK_FROM_PK 2 -#define RI_PLAN_LAST_ON_PK RI_PLAN_CHECK_LOOKUPPK_FROM_PK /* these queries are executed against the FK (referencing) table: */ -#define RI_PLAN_CASCADE_ONDELETE 3 -#define RI_PLAN_CASCADE_ONUPDATE 4 +#define RI_PLAN_CASCADE_ONDELETE 1 +#define RI_PLAN_CASCADE_ONUPDATE 2 /* For RESTRICT, the same plan can be used for both ON DELETE and ON UPDATE triggers. */ -#define RI_PLAN_RESTRICT 5 -#define RI_PLAN_SETNULL_ONDELETE 6 -#define RI_PLAN_SETNULL_ONUPDATE 7 -#define RI_PLAN_SETDEFAULT_ONDELETE 8 -#define RI_PLAN_SETDEFAULT_ONUPDATE 9 +#define RI_PLAN_RESTRICT 3 +#define RI_PLAN_SETNULL_ONDELETE 4 +#define RI_PLAN_SETNULL_ONUPDATE 5 +#define RI_PLAN_SETDEFAULT_ONDELETE 6 +#define RI_PLAN_SETDEFAULT_ONUPDATE 7 #define MAX_QUOTED_NAME_LEN (NAMEDATALEN*2+3) #define MAX_QUOTED_REL_NAME_LEN (MAX_QUOTED_NAME_LEN*2) @@ -229,8 +231,274 @@ static void ri_ExtractValues(Relation rel, TupleTableSlot *slot, static void ri_ReportViolation(const RI_ConstraintInfo *riinfo, Relation pk_rel, Relation fk_rel, TupleTableSlot *violatorslot, TupleDesc tupdesc, - int queryno, bool partgone) pg_attribute_noreturn(); + bool on_fk, bool partgone) pg_attribute_noreturn(); +static Oid get_fkey_unique_index(Oid conoid); +/* + * Checks whether a tuple containing the unique key as extracted from the + * tuple provided in 'slot' exists in 'pk_rel'. The key is extracted using the + * constraint's index given in 'riinfo', which is also scanned to check the + * existence of the key. + * + * If 'pk_rel' is a partitioned table, the check is performed on its leaf + * partition that would contain the key. + * + * The provided tuple is either the one being inserted into the referencing + * relation ('fk_rel' is non-NULL), or the one being deleted from the + * referenced relation, that is, 'pk_rel' ('fk_rel' is NULL). + */ +static bool +ri_ReferencedKeyExists(Relation pk_rel, Relation fk_rel, + TupleTableSlot *slot, + const RI_ConstraintInfo *riinfo) +{ + Oid constr_id = riinfo->constraint_id; + Oid idxoid; + Relation idxrel; + Relation leaf_pk_rel = NULL; + int num_pk; + int i; + bool found = false; + const Oid *eq_oprs; + Datum pk_vals[INDEX_MAX_KEYS]; + char pk_nulls[INDEX_MAX_KEYS]; + ScanKeyData skey[INDEX_MAX_KEYS]; + Snapshot snap = InvalidSnapshot; + bool pushed_latest_snapshot = false; + IndexScanDesc scan; + TupleTableSlot *outslot; + Oid saved_userid; + int saved_sec_context; + AclResult aclresult; + + /* + * Extract the unique key from the provided slot and choose the equality + * operators to use when scanning the index below. + */ + if (fk_rel) + { + ri_ExtractValues(fk_rel, slot, riinfo, false, pk_vals, pk_nulls); + /* Use PK = FK equality operator. */ + eq_oprs = riinfo->pf_eq_oprs; + + /* + * May need to cast each of the individual values of the foreign key + * to the corresponding PK column's type if the equality operator + * demands it. + */ + for (i = 0; i < riinfo->nkeys; i++) + { + if (pk_nulls[i] != 'n') + { + Oid eq_opr = eq_oprs[i]; + Oid typeid = RIAttType(fk_rel, riinfo->fk_attnums[i]); + RI_CompareHashEntry *entry = ri_HashCompareOp(eq_opr, typeid); + + if (OidIsValid(entry->cast_func_finfo.fn_oid)) + pk_vals[i] = FunctionCall3(&entry->cast_func_finfo, + pk_vals[i], + Int32GetDatum(-1), /* typmod */ + BoolGetDatum(false)); /* implicit coercion */ + } + } + } + else + { + ri_ExtractValues(pk_rel, slot, riinfo, true, pk_vals, pk_nulls); + /* Use PK = PK equality operator. */ + eq_oprs = riinfo->pp_eq_oprs; + } + + /* + * Switch to referenced table's owner to perform the below operations + * as. This matches what ri_PerformCheck() does. + * + * Note that as with queries done by ri_PerformCheck(), the way we select + * the referenced row below effectively bypasses any RLS policies that may + * be present on the referenced table. + */ + GetUserIdAndSecContext(&saved_userid, &saved_sec_context); + SetUserIdAndSecContext(RelationGetForm(pk_rel)->relowner, + saved_sec_context | SECURITY_LOCAL_USERID_CHANGE); + + /* + * Also check that the new user has permissions to look into the schema + * of and SELECT from the referenced table. + */ + aclresult = pg_namespace_aclcheck(RelationGetNamespace(pk_rel), + GetUserId(), ACL_USAGE); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_SCHEMA, + get_namespace_name(RelationGetNamespace(pk_rel))); + aclresult = pg_class_aclcheck(RelationGetRelid(pk_rel), GetUserId(), + ACL_SELECT); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_TABLE, + RelationGetRelationName(pk_rel)); + + /* + * In the case of scanning the PK index for ri_Check_Pk_Match(), we'd like + * to see all rows that could be interesting, even those that would not be + * visible to the transaction snapshot. To do so, force-push the latest + * snapshot. + * + * Also, increment the command counter to make the changes of the current + * command visible in all cases. + */ + CommandCounterIncrement(); + if (fk_rel == NULL) + { + snap = GetLatestSnapshot(); + PushActiveSnapshot(snap); + pushed_latest_snapshot = true; + } + else + { + snap = GetTransactionSnapshot(); + PushActiveSnapshot(snap); + } + + /* + * Open the constraint index to be scanned. + * + * If the target table is partitioned, we must look up the leaf partition + * and its corresponding unique index to search the keys in. + */ + idxoid = get_fkey_unique_index(constr_id); + if (pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + { + Oid leaf_idxoid; + Snapshot mysnap = InvalidSnapshot; + + /* + * XXX the partition descriptor machinery has a hack that assumes that + * the queries originating in this module push the latest snapshot in + * the transaction-snapshot mode. If we haven't push one already, do + * so here. + */ + if (!pushed_latest_snapshot) + { + mysnap = GetLatestSnapshot(); + PushActiveSnapshot(mysnap); + } + + leaf_pk_rel = find_leaf_part_for_key(pk_rel, riinfo->nkeys, + riinfo->pk_attnums, + pk_vals, pk_nulls, + idxoid, RowShareLock, + &leaf_idxoid); + /* + * XXX done fiddling with the partition descriptor machinery so unset + * the active snapshot if we must. + */ + if (mysnap != InvalidSnapshot) + PopActiveSnapshot(); + + /* + * If no suitable leaf partition exists, neither can the key we're + * looking for. + */ + if (leaf_pk_rel == NULL) + { + SetUserIdAndSecContext(saved_userid, saved_sec_context); + PopActiveSnapshot(); + return false; + } + + pk_rel = leaf_pk_rel; + idxoid = leaf_idxoid; + } + idxrel = index_open(idxoid, RowShareLock); + + /* Set up ScanKeys for the index scan. */ + num_pk = IndexRelationGetNumberOfKeyAttributes(idxrel); + for (i = 0; i < num_pk; i++) + { + int pkattno = i + 1; + Oid operator = eq_oprs[i]; + Oid opfamily = idxrel->rd_opfamily[i]; + StrategyNumber strat = get_op_opfamily_strategy(operator, opfamily); + RegProcedure regop = get_opcode(operator); + + /* Initialize the scankey. */ + ScanKeyInit(&skey[i], + pkattno, + strat, + regop, + pk_vals[i]); + + skey[i].sk_collation = idxrel->rd_indcollation[i]; + + /* + * Check for null value. Should not occur, because callers currently + * take care of the cases in which they do occur. + */ + if (pk_nulls[i] == 'n') + skey[i].sk_flags |= SK_ISNULL; + } + + scan = index_beginscan(pk_rel, idxrel, snap, num_pk, 0); + index_rescan(scan, skey, num_pk, NULL, 0); + + /* Look for the tuple, and if found, try to lock it in key share mode. */ + outslot = table_slot_create(pk_rel, NULL); + if (index_getnext_slot(scan, ForwardScanDirection, outslot)) + { + /* + * If we fail to lock the tuple for whatever reason, assume it doesn't + * exist. + */ + found = ExecLockTableTuple(pk_rel, &(outslot->tts_tid), outslot, + snap, + GetCurrentCommandId(false), + LockTupleKeyShare, + LockWaitBlock, NULL); + } + + index_endscan(scan); + ExecDropSingleTupleTableSlot(outslot); + + /* Don't release lock until commit. */ + index_close(idxrel, NoLock); + + /* Close leaf partition relation if any. */ + if (leaf_pk_rel) + table_close(leaf_pk_rel, NoLock); + + /* Restore UID and security context */ + SetUserIdAndSecContext(saved_userid, saved_sec_context); + + PopActiveSnapshot(); + + return found; +} + +/* + * get_fkey_unique_index + * Returns the unique index used by a supposedly foreign key constraint + */ +static Oid +get_fkey_unique_index(Oid conoid) +{ + Oid result = InvalidOid; + HeapTuple tp; + + tp = SearchSysCache1(CONSTROID, ObjectIdGetDatum(conoid)); + if (HeapTupleIsValid(tp)) + { + Form_pg_constraint contup = (Form_pg_constraint) GETSTRUCT(tp); + + if (contup->contype == CONSTRAINT_FOREIGN) + result = contup->conindid; + ReleaseSysCache(tp); + } + + if (!OidIsValid(result)) + elog(ERROR, "unique index not found for foreign key constraint %u", + conoid); + + return result; +} /* * RI_FKey_check - @@ -244,8 +512,6 @@ RI_FKey_check(TriggerData *trigdata) Relation fk_rel; Relation pk_rel; TupleTableSlot *newslot; - RI_QueryKey qkey; - SPIPlanPtr qplan; riinfo = ri_FetchConstraintInfo(trigdata->tg_trigger, trigdata->tg_relation, false); @@ -325,9 +591,9 @@ RI_FKey_check(TriggerData *trigdata) /* * MATCH PARTIAL - all non-null columns must match. (not - * implemented, can be done by modifying the query below - * to only include non-null columns, or by writing a - * special version here) + * implemented, can be done by modifying + * ri_ReferencedKeyExists() to only include non-null + * columns. */ break; #endif @@ -342,74 +608,12 @@ RI_FKey_check(TriggerData *trigdata) break; } - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); - - /* Fetch or prepare a saved plan for the real check */ - ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_CHECK_LOOKUPPK); - - if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL) - { - StringInfoData querybuf; - char pkrelname[MAX_QUOTED_REL_NAME_LEN]; - char attname[MAX_QUOTED_NAME_LEN]; - char paramname[16]; - const char *querysep; - Oid queryoids[RI_MAX_NUMKEYS]; - const char *pk_only; - - /* ---------- - * The query string built is - * SELECT 1 FROM [ONLY] <pktable> x WHERE pkatt1 = $1 [AND ...] - * FOR KEY SHARE OF x - * The type id's for the $ parameters are those of the - * corresponding FK attributes. - * ---------- - */ - initStringInfo(&querybuf); - pk_only = pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ? - "" : "ONLY "; - quoteRelationName(pkrelname, pk_rel); - appendStringInfo(&querybuf, "SELECT 1 FROM %s%s x", - pk_only, pkrelname); - querysep = "WHERE"; - for (int i = 0; i < riinfo->nkeys; i++) - { - Oid pk_type = RIAttType(pk_rel, riinfo->pk_attnums[i]); - Oid fk_type = RIAttType(fk_rel, riinfo->fk_attnums[i]); - - quoteOneName(attname, - RIAttName(pk_rel, riinfo->pk_attnums[i])); - sprintf(paramname, "$%d", i + 1); - ri_GenerateQual(&querybuf, querysep, - attname, pk_type, - riinfo->pf_eq_oprs[i], - paramname, fk_type); - querysep = "AND"; - queryoids[i] = fk_type; - } - appendStringInfoString(&querybuf, " FOR KEY SHARE OF x"); - - /* Prepare and save the plan */ - qplan = ri_PlanCheck(querybuf.data, riinfo->nkeys, queryoids, - &qkey, fk_rel, pk_rel); - } - - /* - * Now check that foreign key exists in PK table - * - * XXX detectNewRows must be true when a partitioned table is on the - * referenced side. The reason is that our snapshot must be fresh in - * order for the hack in find_inheritance_children() to work. - */ - ri_PerformCheck(riinfo, &qkey, qplan, - fk_rel, pk_rel, - NULL, newslot, - pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE, - SPI_OK_SELECT); - - if (SPI_finish() != SPI_OK_FINISH) - elog(ERROR, "SPI_finish failed"); + if (!ri_ReferencedKeyExists(pk_rel, fk_rel, newslot, riinfo)) + ri_ReportViolation(riinfo, + pk_rel, fk_rel, + newslot, + NULL, + true, false); table_close(pk_rel, RowShareLock); @@ -464,81 +668,10 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel, TupleTableSlot *oldslot, const RI_ConstraintInfo *riinfo) { - SPIPlanPtr qplan; - RI_QueryKey qkey; - bool result; - /* Only called for non-null rows */ Assert(ri_NullCheck(RelationGetDescr(pk_rel), oldslot, riinfo, true) == RI_KEYS_NONE_NULL); - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); - - /* - * Fetch or prepare a saved plan for checking PK table with values coming - * from a PK row - */ - ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_CHECK_LOOKUPPK_FROM_PK); - - if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL) - { - StringInfoData querybuf; - char pkrelname[MAX_QUOTED_REL_NAME_LEN]; - char attname[MAX_QUOTED_NAME_LEN]; - char paramname[16]; - const char *querysep; - const char *pk_only; - Oid queryoids[RI_MAX_NUMKEYS]; - - /* ---------- - * The query string built is - * SELECT 1 FROM [ONLY] <pktable> x WHERE pkatt1 = $1 [AND ...] - * FOR KEY SHARE OF x - * The type id's for the $ parameters are those of the - * PK attributes themselves. - * ---------- - */ - initStringInfo(&querybuf); - pk_only = pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ? - "" : "ONLY "; - quoteRelationName(pkrelname, pk_rel); - appendStringInfo(&querybuf, "SELECT 1 FROM %s%s x", - pk_only, pkrelname); - querysep = "WHERE"; - for (int i = 0; i < riinfo->nkeys; i++) - { - Oid pk_type = RIAttType(pk_rel, riinfo->pk_attnums[i]); - - quoteOneName(attname, - RIAttName(pk_rel, riinfo->pk_attnums[i])); - sprintf(paramname, "$%d", i + 1); - ri_GenerateQual(&querybuf, querysep, - attname, pk_type, - riinfo->pp_eq_oprs[i], - paramname, pk_type); - querysep = "AND"; - queryoids[i] = pk_type; - } - appendStringInfoString(&querybuf, " FOR KEY SHARE OF x"); - - /* Prepare and save the plan */ - qplan = ri_PlanCheck(querybuf.data, riinfo->nkeys, queryoids, - &qkey, fk_rel, pk_rel); - } - - /* - * We have a plan now. Run it. - */ - result = ri_PerformCheck(riinfo, &qkey, qplan, - fk_rel, pk_rel, - oldslot, NULL, - true, /* treat like update */ - SPI_OK_SELECT); - - if (SPI_finish() != SPI_OK_FINISH) - elog(ERROR, "SPI_finish failed"); - - return result; + return ri_ReferencedKeyExists(pk_rel, NULL, oldslot, riinfo); } @@ -1608,15 +1741,10 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) errtableconstraint(fk_rel, NameStr(fake_riinfo.conname)))); - /* - * We tell ri_ReportViolation we were doing the RI_PLAN_CHECK_LOOKUPPK - * query, which isn't true, but will cause it to use - * fake_riinfo.fk_attnums as we need. - */ ri_ReportViolation(&fake_riinfo, pk_rel, fk_rel, slot, tupdesc, - RI_PLAN_CHECK_LOOKUPPK, false); + true, false); ExecDropSingleTupleTableSlot(slot); } @@ -1833,7 +1961,7 @@ RI_PartitionRemove_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) fake_riinfo.pk_attnums[i] = i + 1; ri_ReportViolation(&fake_riinfo, pk_rel, fk_rel, - slot, tupdesc, 0, true); + slot, tupdesc, true, true); } if (SPI_finish() != SPI_OK_FINISH) @@ -1970,26 +2098,25 @@ ri_BuildQueryKey(RI_QueryKey *key, const RI_ConstraintInfo *riinfo, { /* * Inherited constraints with a common ancestor can share ri_query_cache - * entries for all query types except RI_PLAN_CHECK_LOOKUPPK_FROM_PK. - * Except in that case, the query processes the other table involved in - * the FK constraint (i.e., not the table on which the trigger has been - * fired), and so it will be the same for all members of the inheritance - * tree. So we may use the root constraint's OID in the hash key, rather - * than the constraint's own OID. This avoids creating duplicate SPI - * plans, saving lots of work and memory when there are many partitions - * with similar FK constraints. + * entries, because each query processes the other table involved in the + * FK constraint (i.e., not the table on which the trigger has been fired), + * and so it will be the same for all members of the inheritance tree. So + * we may use the root constraint's OID in the hash key, rather than the + * constraint's own OID. This avoids creating duplicate SPI plans, saving + * lots of work and memory when there are many partitions with similar FK + * constraints. * * (Note that we must still have a separate RI_ConstraintInfo for each * constraint, because partitions can have different column orders, * resulting in different pk_attnums[] or fk_attnums[] array contents.) * + * (Note also that for a standalone or non-inherited constraint, + * constraint_root_id is same as constraint_id.) + * * We assume struct RI_QueryKey contains no padding bytes, else we'd need * to use memset to clear them. */ - if (constr_queryno != RI_PLAN_CHECK_LOOKUPPK_FROM_PK) - key->constr_id = riinfo->constraint_root_id; - else - key->constr_id = riinfo->constraint_id; + key->constr_id = riinfo->constraint_root_id; key->constr_queryno = constr_queryno; } @@ -2260,19 +2387,11 @@ ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes, RI_QueryKey *qkey, Relation fk_rel, Relation pk_rel) { SPIPlanPtr qplan; - Relation query_rel; + /* There are currently no queries that run on PK table. */ + Relation query_rel = fk_rel; Oid save_userid; int save_sec_context; - /* - * Use the query type code to determine whether the query is run against - * the PK or FK table; we'll do the check as that table's owner - */ - if (qkey->constr_queryno <= RI_PLAN_LAST_ON_PK) - query_rel = pk_rel; - else - query_rel = fk_rel; - /* Switch to proper UID to perform check as */ GetUserIdAndSecContext(&save_userid, &save_sec_context); SetUserIdAndSecContext(RelationGetForm(query_rel)->relowner, @@ -2305,9 +2424,9 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, TupleTableSlot *oldslot, TupleTableSlot *newslot, bool detectNewRows, int expect_OK) { - Relation query_rel, - source_rel; - bool source_is_pk; + /* There are currently no queries that run on PK table. */ + Relation query_rel = fk_rel, + source_rel = pk_rel; Snapshot test_snapshot; Snapshot crosscheck_snapshot; int limit; @@ -2317,46 +2436,17 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, Datum vals[RI_MAX_NUMKEYS * 2]; char nulls[RI_MAX_NUMKEYS * 2]; - /* - * Use the query type code to determine whether the query is run against - * the PK or FK table; we'll do the check as that table's owner - */ - if (qkey->constr_queryno <= RI_PLAN_LAST_ON_PK) - query_rel = pk_rel; - else - query_rel = fk_rel; - - /* - * The values for the query are taken from the table on which the trigger - * is called - it is normally the other one with respect to query_rel. An - * exception is ri_Check_Pk_Match(), which uses the PK table for both (and - * sets queryno to RI_PLAN_CHECK_LOOKUPPK_FROM_PK). We might eventually - * need some less klugy way to determine this. - */ - if (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK) - { - source_rel = fk_rel; - source_is_pk = false; - } - else - { - source_rel = pk_rel; - source_is_pk = true; - } - /* Extract the parameters to be passed into the query */ if (newslot) { - ri_ExtractValues(source_rel, newslot, riinfo, source_is_pk, - vals, nulls); + ri_ExtractValues(source_rel, newslot, riinfo, true, vals, nulls); if (oldslot) - ri_ExtractValues(source_rel, oldslot, riinfo, source_is_pk, + ri_ExtractValues(source_rel, oldslot, riinfo, true, vals + riinfo->nkeys, nulls + riinfo->nkeys); } else { - ri_ExtractValues(source_rel, oldslot, riinfo, source_is_pk, - vals, nulls); + ri_ExtractValues(source_rel, oldslot, riinfo, true, vals, nulls); } /* @@ -2420,14 +2510,12 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, errhint("This is most likely due to a rule having rewritten the query."))); /* XXX wouldn't it be clearer to do this part at the caller? */ - if (qkey->constr_queryno != RI_PLAN_CHECK_LOOKUPPK_FROM_PK && - expect_OK == SPI_OK_SELECT && - (SPI_processed == 0) == (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK)) + if (expect_OK == SPI_OK_SELECT && SPI_processed != 0) ri_ReportViolation(riinfo, pk_rel, fk_rel, newslot ? newslot : oldslot, NULL, - qkey->constr_queryno, false); + false, false); return SPI_processed != 0; } @@ -2458,9 +2546,9 @@ ri_ExtractValues(Relation rel, TupleTableSlot *slot, /* * Produce an error report * - * If the failed constraint was on insert/update to the FK table, - * we want the key names and values extracted from there, and the error - * message to look like 'key blah is not present in PK'. + * If the failed constraint was on insert/update to the FK table (on_fk is + * true), we want the key names and values extracted from there, and the + * error message to look like 'key blah is not present in PK'. * Otherwise, the attr names and values come from the PK table and the * message looks like 'key blah is still referenced from FK'. */ @@ -2468,22 +2556,20 @@ static void ri_ReportViolation(const RI_ConstraintInfo *riinfo, Relation pk_rel, Relation fk_rel, TupleTableSlot *violatorslot, TupleDesc tupdesc, - int queryno, bool partgone) + bool on_fk, bool partgone) { StringInfoData key_names; StringInfoData key_values; - bool onfk; const int16 *attnums; Oid rel_oid; AclResult aclresult; bool has_perm = true; /* - * Determine which relation to complain about. If tupdesc wasn't passed - * by caller, assume the violator tuple came from there. + * If tupdesc wasn't passed by caller, assume the violator tuple came from + * there. */ - onfk = (queryno == RI_PLAN_CHECK_LOOKUPPK); - if (onfk) + if (on_fk) { attnums = riinfo->fk_attnums; rel_oid = fk_rel->rd_id; @@ -2585,7 +2671,7 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo, key_names.data, key_values.data, RelationGetRelationName(fk_rel)), errtableconstraint(fk_rel, NameStr(riinfo->conname)))); - else if (onfk) + else if (on_fk) ereport(ERROR, (errcode(ERRCODE_FOREIGN_KEY_VIOLATION), errmsg("insert or update on table \"%s\" violates foreign key constraint \"%s\"", @@ -2892,7 +2978,10 @@ ri_AttributesEqual(Oid eq_opr, Oid typeid, * ri_HashCompareOp - * * See if we know how to compare two values, and create a new hash entry - * if not. + * if not. The entry contains the FmgrInfo of the equality operator function + * and that of the cast function, if one is needed to convert the right + * operand (whose type OID has been passed) before passing it to the equality + * function. */ static RI_CompareHashEntry * ri_HashCompareOp(Oid eq_opr, Oid typeid) @@ -2948,8 +3037,16 @@ ri_HashCompareOp(Oid eq_opr, Oid typeid) * moment since that will never be generated for implicit coercions. */ op_input_types(eq_opr, &lefttype, &righttype); - Assert(lefttype == righttype); - if (typeid == lefttype) + + /* + * Don't need to cast if the values that will be passed to the + * operator will be of expected operand type(s). The operator can be + * cross-type (such as when called by ri_ReferencedKeyExists()), in + * which case, we only need the cast if the right operand value + * doesn't match the type expected by the operator. + */ + if ((lefttype == righttype && typeid == lefttype) || + (lefttype != righttype && typeid == righttype)) castfunc = InvalidOid; /* simplest case */ else { diff --git a/src/include/executor/execPartition.h b/src/include/executor/execPartition.h index 603d8becc4..e63dcb12f6 100644 --- a/src/include/executor/execPartition.h +++ b/src/include/executor/execPartition.h @@ -124,5 +124,11 @@ extern PartitionPruneState *ExecCreatePartitionPruneState(PlanState *planstate, extern Bitmapset *ExecFindMatchingSubPlans(PartitionPruneState *prunestate); extern Bitmapset *ExecFindInitialMatchingSubPlans(PartitionPruneState *prunestate, int nsubplans); +extern Relation find_leaf_part_for_key(Relation root_rel, + int key_natts, + const AttrNumber *key_attnums, + Datum *key_vals, char *key_nulls, + Oid root_idxoid, int lockmode, + Oid *leaf_idxoid); #endif /* EXECPARTITION_H */ diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index 82925b4b63..2e1052eeb8 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -243,6 +243,15 @@ extern bool ExecShutdownNode(PlanState *node); extern void ExecSetTupleBound(int64 tuples_needed, PlanState *child_node); +/* + * functions in execLockRows.c + */ + +extern bool ExecLockTableTuple(Relation relation, ItemPointer tid, TupleTableSlot *slot, + Snapshot snapshot, CommandId cid, + LockTupleMode lockmode, LockWaitPolicy waitPolicy, + bool *epq_needed); + /* ---------------------------------------------------------------- * ExecProcNode * diff --git a/src/test/isolation/expected/fk-snapshot.out b/src/test/isolation/expected/fk-snapshot.out index 5faf80d6ce..22752cc742 100644 --- a/src/test/isolation/expected/fk-snapshot.out +++ b/src/test/isolation/expected/fk-snapshot.out @@ -47,12 +47,12 @@ a step s2ifn2: INSERT INTO fk_noparted VALUES (2); step s2c: COMMIT; +ERROR: insert or update on table "fk_noparted" violates foreign key constraint "fk_noparted_a_fkey" step s2sfn: SELECT * FROM fk_noparted; a - 1 -2 -(2 rows) +(1 row) starting permutation: s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 s2c s2sfn diff --git a/src/test/isolation/specs/fk-snapshot.spec b/src/test/isolation/specs/fk-snapshot.spec index 378507fbc3..64d27f29c3 100644 --- a/src/test/isolation/specs/fk-snapshot.spec +++ b/src/test/isolation/specs/fk-snapshot.spec @@ -46,10 +46,7 @@ step s2sfn { SELECT * FROM fk_noparted; } # inserting into referencing tables in transaction-snapshot mode # PK table is non-partitioned permutation s1brr s2brc s2ip2 s1sp s2c s1sp s1ifp2 s1c s1sfp -# PK table is partitioned: buggy, because s2's serialization transaction can -# see the uncommitted row thanks to the latest snapshot taken for -# partition lookup to work correctly also ends up getting used by the PK index -# scan +# PK table is partitioned permutation s2ip2 s2brr s1brc s1ifp2 s2sfp s1c s2sfp s2ifn2 s2c s2sfn # inserting into referencing tables in up-to-date snapshot mode -- 2.24.1 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: simplifying foreign key/RI checks 2022-01-18 06:30 Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-03-14 08:33 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-03-14 09:32 ` Re: simplifying foreign key/RI checks Zhihong Yu <[email protected]> 2022-03-22 04:01 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> @ 2022-04-07 01:05 ` Amit Langote <[email protected]> 2022-04-11 07:47 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Amit Langote @ 2022-04-07 01:05 UTC (permalink / raw) To: Zhihong Yu <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Alvaro Herrera <[email protected]>; vignesh C <[email protected]>; Kyotaro Horiguchi <[email protected]>; PostgreSQL Developers <[email protected]> There were rebase conflicts with the recently committed execPartition.c/h changes. While fixing them, I thought maybe find_leaf_part_for_key() doesn't quite match in style with its neighbors in execPartition.h, so changed it to ExecGetLeafPartitionForKey(). -- Amit Langote EDB: http://www.enterprisedb.com Attachments: [application/octet-stream] v15-0001-Add-isolation-tests-for-snapshot-behavior-in-ri_.patch (6.5K, ../../CA+HiwqEhV8s=qTWmPUzsrPJ1wh-VCE97M7QesL8C42mEeZwXjw@mail.gmail.com/2-v15-0001-Add-isolation-tests-for-snapshot-behavior-in-ri_.patch) download | inline diff: From cb0f371ecec0ce6a737478098a81620e7edd495c Mon Sep 17 00:00:00 2001 From: amitlan <[email protected]> Date: Mon, 15 Nov 2021 18:22:33 +0900 Subject: [PATCH v15 1/2] Add isolation tests for snapshot behavior in ri_triggers.c They are to check the behavior of RI_FKey_check() and ri_Check_Pk_Match(). A test case whereby RI_FKey_check() queries a partitioned PK table under REPEATABLE READ isolation produces wrong output due to a bug of the partition-descriptor logic and that is noted as such in the comment above the test. A subsequent patch will fix the bug and replace the buggy output by the correct one. --- src/test/isolation/expected/fk-snapshot.out | 124 ++++++++++++++++++++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/fk-snapshot.spec | 61 ++++++++++ 3 files changed, 186 insertions(+) create mode 100644 src/test/isolation/expected/fk-snapshot.out create mode 100644 src/test/isolation/specs/fk-snapshot.spec diff --git a/src/test/isolation/expected/fk-snapshot.out b/src/test/isolation/expected/fk-snapshot.out new file mode 100644 index 0000000000..5faf80d6ce --- /dev/null +++ b/src/test/isolation/expected/fk-snapshot.out @@ -0,0 +1,124 @@ +Parsed test spec with 2 sessions + +starting permutation: s1brr s2brc s2ip2 s1sp s2c s1sp s1ifp2 s1c s1sfp +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s2brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +(1 row) + +step s2c: COMMIT; +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +(1 row) + +step s1ifp2: INSERT INTO fk_parted_pk VALUES (2); +ERROR: insert or update on table "fk_parted_pk_2" violates foreign key constraint "fk_parted_pk_a_fkey" +step s1c: COMMIT; +step s1sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + + +starting permutation: s2ip2 s2brr s1brc s1ifp2 s2sfp s1c s2sfp s2ifn2 s2c s2sfn +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s2brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s1ifp2: INSERT INTO fk_parted_pk VALUES (2); +step s2sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + +step s1c: COMMIT; +step s2sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + +step s2ifn2: INSERT INTO fk_noparted VALUES (2); +step s2c: COMMIT; +step s2sfn: SELECT * FROM fk_noparted; +a +- +1 +2 +(2 rows) + + +starting permutation: s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 s2c s2sfn +step s1brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +(1 row) + +step s2c: COMMIT; +step s1sp: SELECT * FROM pk_noparted; +a +- +1 +2 +(2 rows) + +step s1ifp2: INSERT INTO fk_parted_pk VALUES (2); +step s2brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2sfp: SELECT * FROM fk_parted_pk; +a +- +1 +(1 row) + +step s1c: COMMIT; +step s1sfp: SELECT * FROM fk_parted_pk; +a +- +1 +2 +(2 rows) + +step s2ifn2: INSERT INTO fk_noparted VALUES (2); +step s2c: COMMIT; +step s2sfn: SELECT * FROM fk_noparted; +a +- +1 +2 +(2 rows) + + +starting permutation: s1brr s1dfp s1ifp1 s1c s1sfn +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1dfp: DELETE FROM fk_parted_pk WHERE a = 1; +step s1ifp1: INSERT INTO fk_parted_pk VALUES (1); +step s1c: COMMIT; +step s1sfn: SELECT * FROM fk_noparted; +a +- +1 +(1 row) + + +starting permutation: s1brc s1dfp s1ifp1 s1c s1sfn +step s1brc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s1dfp: DELETE FROM fk_parted_pk WHERE a = 1; +step s1ifp1: INSERT INTO fk_parted_pk VALUES (1); +step s1c: COMMIT; +step s1sfn: SELECT * FROM fk_noparted; +a +- +1 +(1 row) + diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index a48caae228..0b041559ee 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -33,6 +33,7 @@ test: fk-deadlock test: fk-deadlock2 test: fk-partitioned-1 test: fk-partitioned-2 +test: fk-snapshot test: eval-plan-qual test: eval-plan-qual-trigger test: lock-update-delete diff --git a/src/test/isolation/specs/fk-snapshot.spec b/src/test/isolation/specs/fk-snapshot.spec new file mode 100644 index 0000000000..378507fbc3 --- /dev/null +++ b/src/test/isolation/specs/fk-snapshot.spec @@ -0,0 +1,61 @@ +setup +{ + CREATE TABLE pk_noparted ( + a int PRIMARY KEY + ); + + CREATE TABLE fk_parted_pk ( + a int PRIMARY KEY REFERENCES pk_noparted ON DELETE CASCADE + ) PARTITION BY LIST (a); + CREATE TABLE fk_parted_pk_1 PARTITION OF fk_parted_pk FOR VALUES IN (1); + CREATE TABLE fk_parted_pk_2 PARTITION OF fk_parted_pk FOR VALUES IN (2); + + CREATE TABLE fk_noparted ( + a int REFERENCES fk_parted_pk ON DELETE NO ACTION INITIALLY DEFERRED + ); + INSERT INTO pk_noparted VALUES (1); + INSERT INTO fk_parted_pk VALUES (1); + INSERT INTO fk_noparted VALUES (1); +} + +teardown +{ + DROP TABLE pk_noparted, fk_parted_pk, fk_noparted; +} + +session s1 +step s1brr { BEGIN ISOLATION LEVEL REPEATABLE READ; } +step s1brc { BEGIN ISOLATION LEVEL READ COMMITTED; } +step s1ifp2 { INSERT INTO fk_parted_pk VALUES (2); } +step s1ifp1 { INSERT INTO fk_parted_pk VALUES (1); } +step s1dfp { DELETE FROM fk_parted_pk WHERE a = 1; } +step s1c { COMMIT; } +step s1sfp { SELECT * FROM fk_parted_pk; } +step s1sp { SELECT * FROM pk_noparted; } +step s1sfn { SELECT * FROM fk_noparted; } + +session s2 +step s2brr { BEGIN ISOLATION LEVEL REPEATABLE READ; } +step s2brc { BEGIN ISOLATION LEVEL READ COMMITTED; } +step s2ip2 { INSERT INTO pk_noparted VALUES (2); } +step s2ifn2 { INSERT INTO fk_noparted VALUES (2); } +step s2c { COMMIT; } +step s2sfp { SELECT * FROM fk_parted_pk; } +step s2sfn { SELECT * FROM fk_noparted; } + +# inserting into referencing tables in transaction-snapshot mode +# PK table is non-partitioned +permutation s1brr s2brc s2ip2 s1sp s2c s1sp s1ifp2 s1c s1sfp +# PK table is partitioned: buggy, because s2's serialization transaction can +# see the uncommitted row thanks to the latest snapshot taken for +# partition lookup to work correctly also ends up getting used by the PK index +# scan +permutation s2ip2 s2brr s1brc s1ifp2 s2sfp s1c s2sfp s2ifn2 s2c s2sfn + +# inserting into referencing tables in up-to-date snapshot mode +permutation s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 s2c s2sfn + +# deleting a referenced row and then inserting again in the same transaction; works +# the same no matter the snapshot mode +permutation s1brr s1dfp s1ifp1 s1c s1sfn +permutation s1brc s1dfp s1ifp1 s1c s1sfn -- 2.24.1 [application/octet-stream] v15-0002-Avoid-using-SPI-for-some-RI-checks.patch (40.7K, ../../CA+HiwqEhV8s=qTWmPUzsrPJ1wh-VCE97M7QesL8C42mEeZwXjw@mail.gmail.com/3-v15-0002-Avoid-using-SPI-for-some-RI-checks.patch) download | inline diff: From 9c5659738dbbef3e8dd5edf84952fad42fa5e82c Mon Sep 17 00:00:00 2001 From: amitlan <[email protected]> Date: Tue, 12 Jan 2021 14:17:31 +0900 Subject: [PATCH v15 2/2] Avoid using SPI for some RI checks This modifies the subroutines called by RI trigger functions that want to check if a given referenced value exists in the referenced relation to simply scan the foreign key constraint's unique index. That replaces the current way of issuing a `SELECT 1 FROM referenced_relation WHERE ref_key = $1` query through SPI to do the same. This saves a lot of work, especially when inserting into or updating a referencing relation. This rewrite allows to fix a PK row visibility bug caused by a partition descriptor hack which requires ActiveSnapshot to be set to come up with the correct set of partitions for the RI query running under REPEATABLE READ isolation. We now set that snapshot indepedently of the snapshot to be used by the PK index scan, so the two no longer interfere. The buggy output in src/test/isolation/expected/fk-snapshot.out of the relevant test case has been corrected. --- src/backend/executor/execPartition.c | 160 +++++- src/backend/executor/nodeLockRows.c | 160 +++--- src/backend/utils/adt/ri_triggers.c | 573 ++++++++++++-------- src/include/executor/execPartition.h | 7 +- src/include/executor/executor.h | 9 + src/test/isolation/expected/fk-snapshot.out | 4 +- src/test/isolation/specs/fk-snapshot.spec | 5 +- 7 files changed, 596 insertions(+), 322 deletions(-) diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 615bd80973..d03644ae09 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -176,8 +176,9 @@ static void FormPartitionKeyDatum(PartitionDispatch pd, EState *estate, Datum *values, bool *isnull); -static int get_partition_for_tuple(PartitionDispatch pd, Datum *values, - bool *isnull); +static int get_partition_for_tuple(PartitionKey key, + PartitionDesc partdesc, + Datum *values, bool *isnull); static char *ExecBuildSlotPartitionKeyDescription(Relation rel, Datum *values, bool *isnull, @@ -318,7 +319,9 @@ ExecFindPartition(ModifyTableState *mtstate, * these values, error out. */ if (partdesc->nparts == 0 || - (partidx = get_partition_for_tuple(dispatch, values, isnull)) < 0) + (partidx = get_partition_for_tuple(dispatch->key, + dispatch->partdesc, + values, isnull)) < 0) { char *val_desc; @@ -1341,12 +1344,12 @@ FormPartitionKeyDatum(PartitionDispatch pd, * found or -1 if none found. */ static int -get_partition_for_tuple(PartitionDispatch pd, Datum *values, bool *isnull) +get_partition_for_tuple(PartitionKey key, + PartitionDesc partdesc, + Datum *values, bool *isnull) { int bound_offset; int part_index = -1; - PartitionKey key = pd->key; - PartitionDesc partdesc = pd->partdesc; PartitionBoundInfo boundinfo = partdesc->boundinfo; /* Route as appropriate based on partitioning strategy. */ @@ -1438,6 +1441,151 @@ get_partition_for_tuple(PartitionDispatch pd, Datum *values, bool *isnull) return part_index; } +/* + * ExecGetLeafPartitionForTuple + * Finds the leaf partition of a partitioned table 'root_rel' that might + * contain the specified key tuple containing a subset of the table's + * columns (including all of the partition key columns) + * + * 'key_natts' specifies the number columns contained in the key, + * 'key_attnums' their attribute numbers as defined in 'root_rel', and + * 'key_vals' and 'key_nulls' specify the key tuple. + * + * Returns NULL if no leaf partition is found for the key. Caller must close + * the relation. + * + * This works because the unique key defined on the root relation is required + * to contain the partition key columns of all of the ancestors that lead up to + * a given leaf partition. + */ +Relation +ExecGetLeafPartitionForKey(Relation root_rel, int key_natts, + const AttrNumber *key_attnums, + Datum *key_vals, char *key_nulls, + Oid root_idxoid, int lockmode, + Oid *leaf_idxoid) +{ + Relation rel = root_rel; + Oid constr_idxoid = root_idxoid; + + *leaf_idxoid = InvalidOid; + + /* + * Descend through partitioned parents to find the leaf partition that + * would accept a row with the provided key values, starting with the root + * parent. + */ + while (true) + { + PartitionKey partkey = RelationGetPartitionKey(rel); + PartitionDirectory partdir; + PartitionDesc partdesc; + Datum partkey_vals[PARTITION_MAX_KEYS]; + bool partkey_isnull[PARTITION_MAX_KEYS]; + AttrNumber *root_partattrs = partkey->partattrs; + int i, + j; + int partidx; + Oid partoid; + bool is_leaf; + + /* + * Collect partition key values from the unique key. + * + * Because we only have the root table's copy of pk_attnums, must map + * any non-root table's partition key attribute numbers to the root + * table's. + */ + if (rel != root_rel) + { + /* + * map->attnums will contain root table attribute numbers for each + * attribute of the current partitioned relation. + */ + AttrMap *map = build_attrmap_by_name_if_req(RelationGetDescr(root_rel), + RelationGetDescr(rel)); + + if (map) + { + root_partattrs = palloc(partkey->partnatts * + sizeof(AttrNumber)); + for (i = 0; i < partkey->partnatts; i++) + { + AttrNumber partattno = partkey->partattrs[i]; + + root_partattrs[i] = map->attnums[partattno - 1]; + } + + free_attrmap(map); + } + } + + /* + * Referenced key specification does not allow expressions, so there + * would not be expressions in the partition keys either. + */ + Assert(partkey->partexprs == NIL); + for (i = 0, j = 0; i < partkey->partnatts; i++) + { + int k; + + for (k = 0; k < key_natts; k++) + { + if (root_partattrs[i] == key_attnums[k]) + { + partkey_vals[j] = key_vals[k]; + partkey_isnull[j] = (key_nulls[k] == 'n'); + j++; + break; + } + } + } + /* Had better have found values for all of the partition keys. */ + Assert(j == partkey->partnatts); + + if (root_partattrs != partkey->partattrs) + pfree(root_partattrs); + + /* Get the PartitionDesc using the partition directory machinery. */ + partdir = CreatePartitionDirectory(CurrentMemoryContext, true); + partdesc = PartitionDirectoryLookup(partdir, rel); + + /* Find the partition for the key. */ + partidx = get_partition_for_tuple(partkey, partdesc, + partkey_vals, partkey_isnull); + Assert(partidx < 0 || partidx < partdesc->nparts); + + /* done using the partition directory */ + DestroyPartitionDirectory(partdir); + + /* close any intermediate parents we opened */ + if (rel != root_rel) + table_close(rel, NoLock); + + /* No partition found. */ + if (partidx < 0) + return NULL; + + partoid = partdesc->oids[partidx]; + rel = table_open(partoid, lockmode); + constr_idxoid = index_get_partition(rel, constr_idxoid); + + /* + * Return if the partition is a leaf, else find its partition in the + * next iteration. + */ + is_leaf = partdesc->is_leaf[partidx]; + if (is_leaf) + { + *leaf_idxoid = constr_idxoid; + return rel; + } + } + + Assert(false); + return NULL; +} + /* * ExecBuildSlotPartitionKeyDescription * diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c index 1a9dab25dd..ab54a65e0e 100644 --- a/src/backend/executor/nodeLockRows.c +++ b/src/backend/executor/nodeLockRows.c @@ -79,10 +79,7 @@ lnext: Datum datum; bool isNull; ItemPointerData tid; - TM_FailureData tmfd; LockTupleMode lockmode; - int lockflags = 0; - TM_Result test; TupleTableSlot *markSlot; /* clear any leftover test tuple for this rel */ @@ -179,74 +176,11 @@ lnext: break; } - lockflags = TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS; - if (!IsolationUsesXactSnapshot()) - lockflags |= TUPLE_LOCK_FLAG_FIND_LAST_VERSION; - - test = table_tuple_lock(erm->relation, &tid, estate->es_snapshot, - markSlot, estate->es_output_cid, - lockmode, erm->waitPolicy, - lockflags, - &tmfd); - - switch (test) - { - case TM_WouldBlock: - /* couldn't lock tuple in SKIP LOCKED mode */ - goto lnext; - - case TM_SelfModified: - - /* - * The target tuple was already updated or deleted by the - * current command, or by a later command in the current - * transaction. We *must* ignore the tuple in the former - * case, so as to avoid the "Halloween problem" of repeated - * update attempts. In the latter case it might be sensible - * to fetch the updated tuple instead, but doing so would - * require changing heap_update and heap_delete to not - * complain about updating "invisible" tuples, which seems - * pretty scary (table_tuple_lock will not complain, but few - * callers expect TM_Invisible, and we're not one of them). So - * for now, treat the tuple as deleted and do not process. - */ - goto lnext; - - case TM_Ok: - - /* - * Got the lock successfully, the locked tuple saved in - * markSlot for, if needed, EvalPlanQual testing below. - */ - if (tmfd.traversed) - epq_needed = true; - break; - - case TM_Updated: - if (IsolationUsesXactSnapshot()) - ereport(ERROR, - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), - errmsg("could not serialize access due to concurrent update"))); - elog(ERROR, "unexpected table_tuple_lock status: %u", - test); - break; - - case TM_Deleted: - if (IsolationUsesXactSnapshot()) - ereport(ERROR, - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), - errmsg("could not serialize access due to concurrent update"))); - /* tuple was deleted so don't return it */ - goto lnext; - - case TM_Invisible: - elog(ERROR, "attempted to lock invisible tuple"); - break; - - default: - elog(ERROR, "unrecognized table_tuple_lock status: %u", - test); - } + /* skip tuple if it couldn't be locked */ + if (!ExecLockTableTuple(erm->relation, &tid, markSlot, + estate->es_snapshot, estate->es_output_cid, + lockmode, erm->waitPolicy, &epq_needed)) + goto lnext; /* Remember locked tuple's TID for EPQ testing and WHERE CURRENT OF */ erm->curCtid = tid; @@ -281,6 +215,90 @@ lnext: return slot; } +/* + * ExecLockTableTuple + * Locks tuple with the specified TID in lockmode following given wait + * policy + * + * Returns true if the tuple was successfully locked. Locked tuple is loaded + * into provided slot. + */ +bool +ExecLockTableTuple(Relation relation, ItemPointer tid, TupleTableSlot *slot, + Snapshot snapshot, CommandId cid, + LockTupleMode lockmode, LockWaitPolicy waitPolicy, + bool *epq_needed) +{ + TM_FailureData tmfd; + int lockflags = TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS; + TM_Result test; + + if (!IsolationUsesXactSnapshot()) + lockflags |= TUPLE_LOCK_FLAG_FIND_LAST_VERSION; + + test = table_tuple_lock(relation, tid, snapshot, slot, cid, lockmode, + waitPolicy, lockflags, &tmfd); + + switch (test) + { + case TM_WouldBlock: + /* couldn't lock tuple in SKIP LOCKED mode */ + return false; + + case TM_SelfModified: + /* + * The target tuple was already updated or deleted by the + * current command, or by a later command in the current + * transaction. We *must* ignore the tuple in the former + * case, so as to avoid the "Halloween problem" of repeated + * update attempts. In the latter case it might be sensible + * to fetch the updated tuple instead, but doing so would + * require changing heap_update and heap_delete to not + * complain about updating "invisible" tuples, which seems + * pretty scary (table_tuple_lock will not complain, but few + * callers expect TM_Invisible, and we're not one of them). So + * for now, treat the tuple as deleted and do not process. + */ + return false; + + case TM_Ok: + /* + * Got the lock successfully, the locked tuple saved in + * slot for EvalPlanQual, if asked by the caller. + */ + if (tmfd.traversed && epq_needed) + *epq_needed = true; + break; + + case TM_Updated: + if (IsolationUsesXactSnapshot()) + ereport(ERROR, + (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), + errmsg("could not serialize access due to concurrent update"))); + elog(ERROR, "unexpected table_tuple_lock status: %u", + test); + break; + + case TM_Deleted: + if (IsolationUsesXactSnapshot()) + ereport(ERROR, + (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), + errmsg("could not serialize access due to concurrent update"))); + /* tuple was deleted so don't return it */ + return false; + + case TM_Invisible: + elog(ERROR, "attempted to lock invisible tuple"); + return false; + + default: + elog(ERROR, "unrecognized table_tuple_lock status: %u", test); + return false; + } + + return true; +} + /* ---------------------------------------------------------------- * ExecInitLockRows * diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 01d4c22cfc..3ac7efdfb4 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -23,22 +23,27 @@ #include "postgres.h" +#include "access/genam.h" #include "access/htup_details.h" +#include "access/skey.h" #include "access/sysattr.h" #include "access/table.h" #include "access/tableam.h" #include "access/xact.h" +#include "catalog/partition.h" #include "catalog/pg_collation.h" #include "catalog/pg_constraint.h" #include "catalog/pg_operator.h" #include "catalog/pg_type.h" #include "commands/trigger.h" +#include "executor/execPartition.h" #include "executor/executor.h" #include "executor/spi.h" #include "lib/ilist.h" #include "miscadmin.h" #include "parser/parse_coerce.h" #include "parser/parse_relation.h" +#include "partitioning/partdesc.h" #include "storage/bufmgr.h" #include "utils/acl.h" #include "utils/builtins.h" @@ -48,6 +53,7 @@ #include "utils/inval.h" #include "utils/lsyscache.h" #include "utils/memutils.h" +#include "utils/partcache.h" #include "utils/rel.h" #include "utils/rls.h" #include "utils/ruleutils.h" @@ -68,19 +74,15 @@ #define RI_KEYS_NONE_NULL 2 /* RI query type codes */ -/* these queries are executed against the PK (referenced) table: */ -#define RI_PLAN_CHECK_LOOKUPPK 1 -#define RI_PLAN_CHECK_LOOKUPPK_FROM_PK 2 -#define RI_PLAN_LAST_ON_PK RI_PLAN_CHECK_LOOKUPPK_FROM_PK /* these queries are executed against the FK (referencing) table: */ -#define RI_PLAN_CASCADE_ONDELETE 3 -#define RI_PLAN_CASCADE_ONUPDATE 4 +#define RI_PLAN_CASCADE_ONDELETE 1 +#define RI_PLAN_CASCADE_ONUPDATE 2 /* For RESTRICT, the same plan can be used for both ON DELETE and ON UPDATE triggers. */ -#define RI_PLAN_RESTRICT 5 -#define RI_PLAN_SETNULL_ONDELETE 6 -#define RI_PLAN_SETNULL_ONUPDATE 7 -#define RI_PLAN_SETDEFAULT_ONDELETE 8 -#define RI_PLAN_SETDEFAULT_ONUPDATE 9 +#define RI_PLAN_RESTRICT 3 +#define RI_PLAN_SETNULL_ONDELETE 4 +#define RI_PLAN_SETNULL_ONUPDATE 5 +#define RI_PLAN_SETDEFAULT_ONDELETE 6 +#define RI_PLAN_SETDEFAULT_ONUPDATE 7 #define MAX_QUOTED_NAME_LEN (NAMEDATALEN*2+3) #define MAX_QUOTED_REL_NAME_LEN (MAX_QUOTED_NAME_LEN*2) @@ -229,8 +231,274 @@ static void ri_ExtractValues(Relation rel, TupleTableSlot *slot, static void ri_ReportViolation(const RI_ConstraintInfo *riinfo, Relation pk_rel, Relation fk_rel, TupleTableSlot *violatorslot, TupleDesc tupdesc, - int queryno, bool partgone) pg_attribute_noreturn(); + bool on_fk, bool partgone) pg_attribute_noreturn(); +static Oid get_fkey_unique_index(Oid conoid); +/* + * Checks whether a tuple containing the unique key as extracted from the + * tuple provided in 'slot' exists in 'pk_rel'. The key is extracted using the + * constraint's index given in 'riinfo', which is also scanned to check the + * existence of the key. + * + * If 'pk_rel' is a partitioned table, the check is performed on its leaf + * partition that would contain the key. + * + * The provided tuple is either the one being inserted into the referencing + * relation ('fk_rel' is non-NULL), or the one being deleted from the + * referenced relation, that is, 'pk_rel' ('fk_rel' is NULL). + */ +static bool +ri_ReferencedKeyExists(Relation pk_rel, Relation fk_rel, + TupleTableSlot *slot, + const RI_ConstraintInfo *riinfo) +{ + Oid constr_id = riinfo->constraint_id; + Oid idxoid; + Relation idxrel; + Relation leaf_pk_rel = NULL; + int num_pk; + int i; + bool found = false; + const Oid *eq_oprs; + Datum pk_vals[INDEX_MAX_KEYS]; + char pk_nulls[INDEX_MAX_KEYS]; + ScanKeyData skey[INDEX_MAX_KEYS]; + Snapshot snap = InvalidSnapshot; + bool pushed_latest_snapshot = false; + IndexScanDesc scan; + TupleTableSlot *outslot; + Oid saved_userid; + int saved_sec_context; + AclResult aclresult; + + /* + * Extract the unique key from the provided slot and choose the equality + * operators to use when scanning the index below. + */ + if (fk_rel) + { + ri_ExtractValues(fk_rel, slot, riinfo, false, pk_vals, pk_nulls); + /* Use PK = FK equality operator. */ + eq_oprs = riinfo->pf_eq_oprs; + + /* + * May need to cast each of the individual values of the foreign key + * to the corresponding PK column's type if the equality operator + * demands it. + */ + for (i = 0; i < riinfo->nkeys; i++) + { + if (pk_nulls[i] != 'n') + { + Oid eq_opr = eq_oprs[i]; + Oid typeid = RIAttType(fk_rel, riinfo->fk_attnums[i]); + RI_CompareHashEntry *entry = ri_HashCompareOp(eq_opr, typeid); + + if (OidIsValid(entry->cast_func_finfo.fn_oid)) + pk_vals[i] = FunctionCall3(&entry->cast_func_finfo, + pk_vals[i], + Int32GetDatum(-1), /* typmod */ + BoolGetDatum(false)); /* implicit coercion */ + } + } + } + else + { + ri_ExtractValues(pk_rel, slot, riinfo, true, pk_vals, pk_nulls); + /* Use PK = PK equality operator. */ + eq_oprs = riinfo->pp_eq_oprs; + } + + /* + * Switch to referenced table's owner to perform the below operations + * as. This matches what ri_PerformCheck() does. + * + * Note that as with queries done by ri_PerformCheck(), the way we select + * the referenced row below effectively bypasses any RLS policies that may + * be present on the referenced table. + */ + GetUserIdAndSecContext(&saved_userid, &saved_sec_context); + SetUserIdAndSecContext(RelationGetForm(pk_rel)->relowner, + saved_sec_context | SECURITY_LOCAL_USERID_CHANGE); + + /* + * Also check that the new user has permissions to look into the schema + * of and SELECT from the referenced table. + */ + aclresult = pg_namespace_aclcheck(RelationGetNamespace(pk_rel), + GetUserId(), ACL_USAGE); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_SCHEMA, + get_namespace_name(RelationGetNamespace(pk_rel))); + aclresult = pg_class_aclcheck(RelationGetRelid(pk_rel), GetUserId(), + ACL_SELECT); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_TABLE, + RelationGetRelationName(pk_rel)); + + /* + * In the case of scanning the PK index for ri_Check_Pk_Match(), we'd like + * to see all rows that could be interesting, even those that would not be + * visible to the transaction snapshot. To do so, force-push the latest + * snapshot. + * + * Also, increment the command counter to make the changes of the current + * command visible in all cases. + */ + CommandCounterIncrement(); + if (fk_rel == NULL) + { + snap = GetLatestSnapshot(); + PushActiveSnapshot(snap); + pushed_latest_snapshot = true; + } + else + { + snap = GetTransactionSnapshot(); + PushActiveSnapshot(snap); + } + + /* + * Open the constraint index to be scanned. + * + * If the target table is partitioned, we must look up the leaf partition + * and its corresponding unique index to search the keys in. + */ + idxoid = get_fkey_unique_index(constr_id); + if (pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + { + Oid leaf_idxoid; + Snapshot mysnap = InvalidSnapshot; + + /* + * XXX the partition descriptor machinery has a hack that assumes that + * the queries originating in this module push the latest snapshot in + * the transaction-snapshot mode. If we haven't push one already, do + * so here. + */ + if (!pushed_latest_snapshot) + { + mysnap = GetLatestSnapshot(); + PushActiveSnapshot(mysnap); + } + + leaf_pk_rel = ExecGetLeafPartitionForKey(pk_rel, riinfo->nkeys, + riinfo->pk_attnums, + pk_vals, pk_nulls, + idxoid, RowShareLock, + &leaf_idxoid); + /* + * XXX done fiddling with the partition descriptor machinery so unset + * the active snapshot if we must. + */ + if (mysnap != InvalidSnapshot) + PopActiveSnapshot(); + + /* + * If no suitable leaf partition exists, neither can the key we're + * looking for. + */ + if (leaf_pk_rel == NULL) + { + SetUserIdAndSecContext(saved_userid, saved_sec_context); + PopActiveSnapshot(); + return false; + } + + pk_rel = leaf_pk_rel; + idxoid = leaf_idxoid; + } + idxrel = index_open(idxoid, RowShareLock); + + /* Set up ScanKeys for the index scan. */ + num_pk = IndexRelationGetNumberOfKeyAttributes(idxrel); + for (i = 0; i < num_pk; i++) + { + int pkattno = i + 1; + Oid operator = eq_oprs[i]; + Oid opfamily = idxrel->rd_opfamily[i]; + StrategyNumber strat = get_op_opfamily_strategy(operator, opfamily); + RegProcedure regop = get_opcode(operator); + + /* Initialize the scankey. */ + ScanKeyInit(&skey[i], + pkattno, + strat, + regop, + pk_vals[i]); + + skey[i].sk_collation = idxrel->rd_indcollation[i]; + + /* + * Check for null value. Should not occur, because callers currently + * take care of the cases in which they do occur. + */ + if (pk_nulls[i] == 'n') + skey[i].sk_flags |= SK_ISNULL; + } + + scan = index_beginscan(pk_rel, idxrel, snap, num_pk, 0); + index_rescan(scan, skey, num_pk, NULL, 0); + + /* Look for the tuple, and if found, try to lock it in key share mode. */ + outslot = table_slot_create(pk_rel, NULL); + if (index_getnext_slot(scan, ForwardScanDirection, outslot)) + { + /* + * If we fail to lock the tuple for whatever reason, assume it doesn't + * exist. + */ + found = ExecLockTableTuple(pk_rel, &(outslot->tts_tid), outslot, + snap, + GetCurrentCommandId(false), + LockTupleKeyShare, + LockWaitBlock, NULL); + } + + index_endscan(scan); + ExecDropSingleTupleTableSlot(outslot); + + /* Don't release lock until commit. */ + index_close(idxrel, NoLock); + + /* Close leaf partition relation if any. */ + if (leaf_pk_rel) + table_close(leaf_pk_rel, NoLock); + + /* Restore UID and security context */ + SetUserIdAndSecContext(saved_userid, saved_sec_context); + + PopActiveSnapshot(); + + return found; +} + +/* + * get_fkey_unique_index + * Returns the unique index used by a supposedly foreign key constraint + */ +static Oid +get_fkey_unique_index(Oid conoid) +{ + Oid result = InvalidOid; + HeapTuple tp; + + tp = SearchSysCache1(CONSTROID, ObjectIdGetDatum(conoid)); + if (HeapTupleIsValid(tp)) + { + Form_pg_constraint contup = (Form_pg_constraint) GETSTRUCT(tp); + + if (contup->contype == CONSTRAINT_FOREIGN) + result = contup->conindid; + ReleaseSysCache(tp); + } + + if (!OidIsValid(result)) + elog(ERROR, "unique index not found for foreign key constraint %u", + conoid); + + return result; +} /* * RI_FKey_check - @@ -244,8 +512,6 @@ RI_FKey_check(TriggerData *trigdata) Relation fk_rel; Relation pk_rel; TupleTableSlot *newslot; - RI_QueryKey qkey; - SPIPlanPtr qplan; riinfo = ri_FetchConstraintInfo(trigdata->tg_trigger, trigdata->tg_relation, false); @@ -325,9 +591,9 @@ RI_FKey_check(TriggerData *trigdata) /* * MATCH PARTIAL - all non-null columns must match. (not - * implemented, can be done by modifying the query below - * to only include non-null columns, or by writing a - * special version here) + * implemented, can be done by modifying + * ri_ReferencedKeyExists() to only include non-null + * columns. */ break; #endif @@ -342,74 +608,12 @@ RI_FKey_check(TriggerData *trigdata) break; } - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); - - /* Fetch or prepare a saved plan for the real check */ - ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_CHECK_LOOKUPPK); - - if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL) - { - StringInfoData querybuf; - char pkrelname[MAX_QUOTED_REL_NAME_LEN]; - char attname[MAX_QUOTED_NAME_LEN]; - char paramname[16]; - const char *querysep; - Oid queryoids[RI_MAX_NUMKEYS]; - const char *pk_only; - - /* ---------- - * The query string built is - * SELECT 1 FROM [ONLY] <pktable> x WHERE pkatt1 = $1 [AND ...] - * FOR KEY SHARE OF x - * The type id's for the $ parameters are those of the - * corresponding FK attributes. - * ---------- - */ - initStringInfo(&querybuf); - pk_only = pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ? - "" : "ONLY "; - quoteRelationName(pkrelname, pk_rel); - appendStringInfo(&querybuf, "SELECT 1 FROM %s%s x", - pk_only, pkrelname); - querysep = "WHERE"; - for (int i = 0; i < riinfo->nkeys; i++) - { - Oid pk_type = RIAttType(pk_rel, riinfo->pk_attnums[i]); - Oid fk_type = RIAttType(fk_rel, riinfo->fk_attnums[i]); - - quoteOneName(attname, - RIAttName(pk_rel, riinfo->pk_attnums[i])); - sprintf(paramname, "$%d", i + 1); - ri_GenerateQual(&querybuf, querysep, - attname, pk_type, - riinfo->pf_eq_oprs[i], - paramname, fk_type); - querysep = "AND"; - queryoids[i] = fk_type; - } - appendStringInfoString(&querybuf, " FOR KEY SHARE OF x"); - - /* Prepare and save the plan */ - qplan = ri_PlanCheck(querybuf.data, riinfo->nkeys, queryoids, - &qkey, fk_rel, pk_rel); - } - - /* - * Now check that foreign key exists in PK table - * - * XXX detectNewRows must be true when a partitioned table is on the - * referenced side. The reason is that our snapshot must be fresh in - * order for the hack in find_inheritance_children() to work. - */ - ri_PerformCheck(riinfo, &qkey, qplan, - fk_rel, pk_rel, - NULL, newslot, - pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE, - SPI_OK_SELECT); - - if (SPI_finish() != SPI_OK_FINISH) - elog(ERROR, "SPI_finish failed"); + if (!ri_ReferencedKeyExists(pk_rel, fk_rel, newslot, riinfo)) + ri_ReportViolation(riinfo, + pk_rel, fk_rel, + newslot, + NULL, + true, false); table_close(pk_rel, RowShareLock); @@ -464,81 +668,10 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel, TupleTableSlot *oldslot, const RI_ConstraintInfo *riinfo) { - SPIPlanPtr qplan; - RI_QueryKey qkey; - bool result; - /* Only called for non-null rows */ Assert(ri_NullCheck(RelationGetDescr(pk_rel), oldslot, riinfo, true) == RI_KEYS_NONE_NULL); - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); - - /* - * Fetch or prepare a saved plan for checking PK table with values coming - * from a PK row - */ - ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_CHECK_LOOKUPPK_FROM_PK); - - if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL) - { - StringInfoData querybuf; - char pkrelname[MAX_QUOTED_REL_NAME_LEN]; - char attname[MAX_QUOTED_NAME_LEN]; - char paramname[16]; - const char *querysep; - const char *pk_only; - Oid queryoids[RI_MAX_NUMKEYS]; - - /* ---------- - * The query string built is - * SELECT 1 FROM [ONLY] <pktable> x WHERE pkatt1 = $1 [AND ...] - * FOR KEY SHARE OF x - * The type id's for the $ parameters are those of the - * PK attributes themselves. - * ---------- - */ - initStringInfo(&querybuf); - pk_only = pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ? - "" : "ONLY "; - quoteRelationName(pkrelname, pk_rel); - appendStringInfo(&querybuf, "SELECT 1 FROM %s%s x", - pk_only, pkrelname); - querysep = "WHERE"; - for (int i = 0; i < riinfo->nkeys; i++) - { - Oid pk_type = RIAttType(pk_rel, riinfo->pk_attnums[i]); - - quoteOneName(attname, - RIAttName(pk_rel, riinfo->pk_attnums[i])); - sprintf(paramname, "$%d", i + 1); - ri_GenerateQual(&querybuf, querysep, - attname, pk_type, - riinfo->pp_eq_oprs[i], - paramname, pk_type); - querysep = "AND"; - queryoids[i] = pk_type; - } - appendStringInfoString(&querybuf, " FOR KEY SHARE OF x"); - - /* Prepare and save the plan */ - qplan = ri_PlanCheck(querybuf.data, riinfo->nkeys, queryoids, - &qkey, fk_rel, pk_rel); - } - - /* - * We have a plan now. Run it. - */ - result = ri_PerformCheck(riinfo, &qkey, qplan, - fk_rel, pk_rel, - oldslot, NULL, - true, /* treat like update */ - SPI_OK_SELECT); - - if (SPI_finish() != SPI_OK_FINISH) - elog(ERROR, "SPI_finish failed"); - - return result; + return ri_ReferencedKeyExists(pk_rel, NULL, oldslot, riinfo); } @@ -1608,15 +1741,10 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) errtableconstraint(fk_rel, NameStr(fake_riinfo.conname)))); - /* - * We tell ri_ReportViolation we were doing the RI_PLAN_CHECK_LOOKUPPK - * query, which isn't true, but will cause it to use - * fake_riinfo.fk_attnums as we need. - */ ri_ReportViolation(&fake_riinfo, pk_rel, fk_rel, slot, tupdesc, - RI_PLAN_CHECK_LOOKUPPK, false); + true, false); ExecDropSingleTupleTableSlot(slot); } @@ -1833,7 +1961,7 @@ RI_PartitionRemove_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) fake_riinfo.pk_attnums[i] = i + 1; ri_ReportViolation(&fake_riinfo, pk_rel, fk_rel, - slot, tupdesc, 0, true); + slot, tupdesc, true, true); } if (SPI_finish() != SPI_OK_FINISH) @@ -1970,26 +2098,25 @@ ri_BuildQueryKey(RI_QueryKey *key, const RI_ConstraintInfo *riinfo, { /* * Inherited constraints with a common ancestor can share ri_query_cache - * entries for all query types except RI_PLAN_CHECK_LOOKUPPK_FROM_PK. - * Except in that case, the query processes the other table involved in - * the FK constraint (i.e., not the table on which the trigger has been - * fired), and so it will be the same for all members of the inheritance - * tree. So we may use the root constraint's OID in the hash key, rather - * than the constraint's own OID. This avoids creating duplicate SPI - * plans, saving lots of work and memory when there are many partitions - * with similar FK constraints. + * entries, because each query processes the other table involved in the + * FK constraint (i.e., not the table on which the trigger has been fired), + * and so it will be the same for all members of the inheritance tree. So + * we may use the root constraint's OID in the hash key, rather than the + * constraint's own OID. This avoids creating duplicate SPI plans, saving + * lots of work and memory when there are many partitions with similar FK + * constraints. * * (Note that we must still have a separate RI_ConstraintInfo for each * constraint, because partitions can have different column orders, * resulting in different pk_attnums[] or fk_attnums[] array contents.) * + * (Note also that for a standalone or non-inherited constraint, + * constraint_root_id is same as constraint_id.) + * * We assume struct RI_QueryKey contains no padding bytes, else we'd need * to use memset to clear them. */ - if (constr_queryno != RI_PLAN_CHECK_LOOKUPPK_FROM_PK) - key->constr_id = riinfo->constraint_root_id; - else - key->constr_id = riinfo->constraint_id; + key->constr_id = riinfo->constraint_root_id; key->constr_queryno = constr_queryno; } @@ -2260,19 +2387,11 @@ ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes, RI_QueryKey *qkey, Relation fk_rel, Relation pk_rel) { SPIPlanPtr qplan; - Relation query_rel; + /* There are currently no queries that run on PK table. */ + Relation query_rel = fk_rel; Oid save_userid; int save_sec_context; - /* - * Use the query type code to determine whether the query is run against - * the PK or FK table; we'll do the check as that table's owner - */ - if (qkey->constr_queryno <= RI_PLAN_LAST_ON_PK) - query_rel = pk_rel; - else - query_rel = fk_rel; - /* Switch to proper UID to perform check as */ GetUserIdAndSecContext(&save_userid, &save_sec_context); SetUserIdAndSecContext(RelationGetForm(query_rel)->relowner, @@ -2305,9 +2424,9 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, TupleTableSlot *oldslot, TupleTableSlot *newslot, bool detectNewRows, int expect_OK) { - Relation query_rel, - source_rel; - bool source_is_pk; + /* There are currently no queries that run on PK table. */ + Relation query_rel = fk_rel, + source_rel = pk_rel; Snapshot test_snapshot; Snapshot crosscheck_snapshot; int limit; @@ -2317,46 +2436,17 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, Datum vals[RI_MAX_NUMKEYS * 2]; char nulls[RI_MAX_NUMKEYS * 2]; - /* - * Use the query type code to determine whether the query is run against - * the PK or FK table; we'll do the check as that table's owner - */ - if (qkey->constr_queryno <= RI_PLAN_LAST_ON_PK) - query_rel = pk_rel; - else - query_rel = fk_rel; - - /* - * The values for the query are taken from the table on which the trigger - * is called - it is normally the other one with respect to query_rel. An - * exception is ri_Check_Pk_Match(), which uses the PK table for both (and - * sets queryno to RI_PLAN_CHECK_LOOKUPPK_FROM_PK). We might eventually - * need some less klugy way to determine this. - */ - if (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK) - { - source_rel = fk_rel; - source_is_pk = false; - } - else - { - source_rel = pk_rel; - source_is_pk = true; - } - /* Extract the parameters to be passed into the query */ if (newslot) { - ri_ExtractValues(source_rel, newslot, riinfo, source_is_pk, - vals, nulls); + ri_ExtractValues(source_rel, newslot, riinfo, true, vals, nulls); if (oldslot) - ri_ExtractValues(source_rel, oldslot, riinfo, source_is_pk, + ri_ExtractValues(source_rel, oldslot, riinfo, true, vals + riinfo->nkeys, nulls + riinfo->nkeys); } else { - ri_ExtractValues(source_rel, oldslot, riinfo, source_is_pk, - vals, nulls); + ri_ExtractValues(source_rel, oldslot, riinfo, true, vals, nulls); } /* @@ -2420,14 +2510,12 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, errhint("This is most likely due to a rule having rewritten the query."))); /* XXX wouldn't it be clearer to do this part at the caller? */ - if (qkey->constr_queryno != RI_PLAN_CHECK_LOOKUPPK_FROM_PK && - expect_OK == SPI_OK_SELECT && - (SPI_processed == 0) == (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK)) + if (expect_OK == SPI_OK_SELECT && SPI_processed != 0) ri_ReportViolation(riinfo, pk_rel, fk_rel, newslot ? newslot : oldslot, NULL, - qkey->constr_queryno, false); + false, false); return SPI_processed != 0; } @@ -2458,9 +2546,9 @@ ri_ExtractValues(Relation rel, TupleTableSlot *slot, /* * Produce an error report * - * If the failed constraint was on insert/update to the FK table, - * we want the key names and values extracted from there, and the error - * message to look like 'key blah is not present in PK'. + * If the failed constraint was on insert/update to the FK table (on_fk is + * true), we want the key names and values extracted from there, and the + * error message to look like 'key blah is not present in PK'. * Otherwise, the attr names and values come from the PK table and the * message looks like 'key blah is still referenced from FK'. */ @@ -2468,22 +2556,20 @@ static void ri_ReportViolation(const RI_ConstraintInfo *riinfo, Relation pk_rel, Relation fk_rel, TupleTableSlot *violatorslot, TupleDesc tupdesc, - int queryno, bool partgone) + bool on_fk, bool partgone) { StringInfoData key_names; StringInfoData key_values; - bool onfk; const int16 *attnums; Oid rel_oid; AclResult aclresult; bool has_perm = true; /* - * Determine which relation to complain about. If tupdesc wasn't passed - * by caller, assume the violator tuple came from there. + * If tupdesc wasn't passed by caller, assume the violator tuple came from + * there. */ - onfk = (queryno == RI_PLAN_CHECK_LOOKUPPK); - if (onfk) + if (on_fk) { attnums = riinfo->fk_attnums; rel_oid = fk_rel->rd_id; @@ -2585,7 +2671,7 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo, key_names.data, key_values.data, RelationGetRelationName(fk_rel)), errtableconstraint(fk_rel, NameStr(riinfo->conname)))); - else if (onfk) + else if (on_fk) ereport(ERROR, (errcode(ERRCODE_FOREIGN_KEY_VIOLATION), errmsg("insert or update on table \"%s\" violates foreign key constraint \"%s\"", @@ -2892,7 +2978,10 @@ ri_AttributesEqual(Oid eq_opr, Oid typeid, * ri_HashCompareOp - * * See if we know how to compare two values, and create a new hash entry - * if not. + * if not. The entry contains the FmgrInfo of the equality operator function + * and that of the cast function, if one is needed to convert the right + * operand (whose type OID has been passed) before passing it to the equality + * function. */ static RI_CompareHashEntry * ri_HashCompareOp(Oid eq_opr, Oid typeid) @@ -2948,8 +3037,16 @@ ri_HashCompareOp(Oid eq_opr, Oid typeid) * moment since that will never be generated for implicit coercions. */ op_input_types(eq_opr, &lefttype, &righttype); - Assert(lefttype == righttype); - if (typeid == lefttype) + + /* + * Don't need to cast if the values that will be passed to the + * operator will be of expected operand type(s). The operator can be + * cross-type (such as when called by ri_ReferencedKeyExists()), in + * which case, we only need the cast if the right operand value + * doesn't match the type expected by the operator. + */ + if ((lefttype == righttype && typeid == lefttype) || + (lefttype != righttype && typeid == righttype)) castfunc = InvalidOid; /* simplest case */ else { diff --git a/src/include/executor/execPartition.h b/src/include/executor/execPartition.h index 708435e952..6a69113325 100644 --- a/src/include/executor/execPartition.h +++ b/src/include/executor/execPartition.h @@ -31,7 +31,12 @@ extern ResultRelInfo *ExecFindPartition(ModifyTableState *mtstate, EState *estate); extern void ExecCleanupTupleRouting(ModifyTableState *mtstate, PartitionTupleRouting *proute); - +extern Relation ExecGetLeafPartitionForKey(Relation root_rel, + int key_natts, + const AttrNumber *key_attnums, + Datum *key_vals, char *key_nulls, + Oid root_idxoid, int lockmode, + Oid *leaf_idxoid); /* * PartitionedRelPruningData - Per-partitioned-table data for run-time pruning diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index 873772f188..a69782279b 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -243,6 +243,15 @@ extern bool ExecShutdownNode(PlanState *node); extern void ExecSetTupleBound(int64 tuples_needed, PlanState *child_node); +/* + * functions in execLockRows.c + */ + +extern bool ExecLockTableTuple(Relation relation, ItemPointer tid, TupleTableSlot *slot, + Snapshot snapshot, CommandId cid, + LockTupleMode lockmode, LockWaitPolicy waitPolicy, + bool *epq_needed); + /* ---------------------------------------------------------------- * ExecProcNode * diff --git a/src/test/isolation/expected/fk-snapshot.out b/src/test/isolation/expected/fk-snapshot.out index 5faf80d6ce..22752cc742 100644 --- a/src/test/isolation/expected/fk-snapshot.out +++ b/src/test/isolation/expected/fk-snapshot.out @@ -47,12 +47,12 @@ a step s2ifn2: INSERT INTO fk_noparted VALUES (2); step s2c: COMMIT; +ERROR: insert or update on table "fk_noparted" violates foreign key constraint "fk_noparted_a_fkey" step s2sfn: SELECT * FROM fk_noparted; a - 1 -2 -(2 rows) +(1 row) starting permutation: s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 s2c s2sfn diff --git a/src/test/isolation/specs/fk-snapshot.spec b/src/test/isolation/specs/fk-snapshot.spec index 378507fbc3..64d27f29c3 100644 --- a/src/test/isolation/specs/fk-snapshot.spec +++ b/src/test/isolation/specs/fk-snapshot.spec @@ -46,10 +46,7 @@ step s2sfn { SELECT * FROM fk_noparted; } # inserting into referencing tables in transaction-snapshot mode # PK table is non-partitioned permutation s1brr s2brc s2ip2 s1sp s2c s1sp s1ifp2 s1c s1sfp -# PK table is partitioned: buggy, because s2's serialization transaction can -# see the uncommitted row thanks to the latest snapshot taken for -# partition lookup to work correctly also ends up getting used by the PK index -# scan +# PK table is partitioned permutation s2ip2 s2brr s1brc s1ifp2 s2sfp s1c s2sfp s2ifn2 s2c s2sfn # inserting into referencing tables in up-to-date snapshot mode -- 2.24.1 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: simplifying foreign key/RI checks 2022-01-18 06:30 Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-03-14 08:33 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-03-14 09:32 ` Re: simplifying foreign key/RI checks Zhihong Yu <[email protected]> 2022-03-22 04:01 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-04-07 01:05 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> @ 2022-04-11 07:47 ` Amit Langote <[email protected]> 2022-05-02 11:50 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Amit Langote @ 2022-04-11 07:47 UTC (permalink / raw) To: Zhihong Yu <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Alvaro Herrera <[email protected]>; vignesh C <[email protected]>; Kyotaro Horiguchi <[email protected]>; PostgreSQL Developers <[email protected]> On Thu, Apr 7, 2022 at 10:05 AM Amit Langote <[email protected]> wrote: > There were rebase conflicts with the recently committed > execPartition.c/h changes. While fixing them, I thought maybe > find_leaf_part_for_key() doesn't quite match in style with its > neighbors in execPartition.h, so changed it to > ExecGetLeafPartitionForKey(). This one has been marked Returned with Feedback in the CF app, which makes sense given the discussion on -committers [1]. Agree with the feedback given that it would be better to address *all* RI trigger check/action functions in the project of sidestepping SPI when doing those checks/actions, not only RI_FKey_check_ins / upd() as the current patch does. I guess that will require thinking a little bit harder about how to modularize the new implementation so that the various trigger functions don't end up with their own bespoke check/action implementations. I'll think about that, also consider what Corey proposed in [2], and try to reformulate this for v16. -- Amit Langote EDB: http://www.enterprisedb.com [1] https://www.postgresql.org/message-id/flat/E1ncXX2-000mFt-Pe%40gemulon.postgresql.org [2] https://www.postgresql.org/message-id/flat/CADkLM%3DeZJddpx6RDop-oCrQ%2BJ9R-wfbf6MoLxUUGjbpwTkoUXQ%4... ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: simplifying foreign key/RI checks 2022-01-18 06:30 Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-03-14 08:33 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-03-14 09:32 ` Re: simplifying foreign key/RI checks Zhihong Yu <[email protected]> 2022-03-22 04:01 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-04-07 01:05 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-04-11 07:47 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> @ 2022-05-02 11:50 ` Amit Langote <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Amit Langote @ 2022-05-02 11:50 UTC (permalink / raw) To: Zhihong Yu <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Alvaro Herrera <[email protected]>; vignesh C <[email protected]>; Kyotaro Horiguchi <[email protected]>; PostgreSQL Developers <[email protected]> On Mon, Apr 11, 2022 at 4:47 PM Amit Langote <[email protected]> wrote: > This one has been marked Returned with Feedback in the CF app, which > makes sense given the discussion on -committers [1]. > > Agree with the feedback given that it would be better to address *all* > RI trigger check/action functions in the project of sidestepping SPI > when doing those checks/actions, not only RI_FKey_check_ins / upd() as > the current patch does. I guess that will require thinking a little > bit harder about how to modularize the new implementation so that the > various trigger functions don't end up with their own bespoke > check/action implementations. > > I'll think about that, also consider what Corey proposed in [2], and > try to reformulate this for v16. I've been thinking about this and wondering if the SPI overhead is too big in the other cases (cases where it is the FK table that is to be scanned) that it makes sense to replace the actual planner (invoked via SPI) by a hard-coded mini-planner for the task of figuring out the best way to scan the FK table for a given PK row affected by the main query. Planner's involvement seems necessary in those cases, because the choice of how to scan the FK table is not as clear cut as how to scan the PK table. ISTM, the SPI overhead consists mainly of performing GetCachedPlan() and executor setup/shutdown, which can seem substantial when compared to the core task of scanning the PK/FK table, and does add up over many rows affected by the main query, as seen by the over 2x speedup for the PK table case gained by shaving it off with the proposed patch [1]. In the other cases, the mini-planner will need some cycles of its own, even though maybe not as many as by the use of SPI, so the speedup might be less impressive. Other than coming up with an acceptable implementation for the mini-planner (maybe we have an example in plan_cluster_use_sort() to ape), one more challenge is to figure out a way to implement the CASCADE/SET trigger routines. For those, we might need to introduce restricted forms of ExecUpdate(), ExecDelete() that can be called directly, that is, without a full-fledged plan. Not having to worry about those things does seem like a benefit of just continuing to use the SPI in those cases. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com [1] drop table pk, fk; create table pk (a int primary key); create table fk (a int references pk); insert into pk select generate_series(1, 1000000); insert into fk select i%1000000+1 from generate_series(1, 10000000) i; Time for the last statement: HEAD: 67566.845 ms (01:07.567) Patched: 26759.627 ms (00:26.760) ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 849cac3947..cf4387f443 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 49938c9ed4..282dcb9791 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index b9ba4f9fb3..bcf1497f67 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -795,10 +795,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -829,15 +826,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --7mdtsjmrzitrgzgx Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v9-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v6 09/14] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 10c1c3b616b..a1ec50ab7a8 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2248,7 +2248,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 248ff90904b..04ad14f70b3 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -287,7 +287,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index f1d0d4b78e3..e35bd36e710 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -787,10 +787,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -821,15 +818,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -1988,7 +1980,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2000,7 +1991,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --w4wcjcocxsm37usi Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v6-0010-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v10 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 849cac3947..cf4387f443 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 49938c9ed4..282dcb9791 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index b9ba4f9fb3..bcf1497f67 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -795,10 +795,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -829,15 +826,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --3o7pc6dfau5a5hry Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v10-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v12 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 973734e9ffa..2cb5fb18675 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2335,7 +2335,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 49938c9ed41..282dcb97919 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index 19e99c81092..ff3e8bcdd6f 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -802,10 +802,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -836,15 +833,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -2050,7 +2042,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2062,7 +2053,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --6jpz2j246qmht4bt Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v12-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v7 08/13] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 10c1c3b616b..a1ec50ab7a8 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2248,7 +2248,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 94a5e2da17c..bbdaa591891 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index f1d0d4b78e3..e35bd36e710 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -787,10 +787,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -821,15 +818,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -1988,7 +1980,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2000,7 +1991,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --kqqpqghcwbcc3dt5 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v7-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v6 09/14] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 10c1c3b616b..a1ec50ab7a8 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2248,7 +2248,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 248ff90904b..04ad14f70b3 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -287,7 +287,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index f1d0d4b78e3..e35bd36e710 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -787,10 +787,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -821,15 +818,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -1988,7 +1980,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2000,7 +1991,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --w4wcjcocxsm37usi Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v6-0010-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v6 09/14] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 10c1c3b616b..a1ec50ab7a8 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2248,7 +2248,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 248ff90904b..04ad14f70b3 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -287,7 +287,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index f1d0d4b78e3..e35bd36e710 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -787,10 +787,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -821,15 +818,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -1988,7 +1980,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2000,7 +1991,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --w4wcjcocxsm37usi Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v6-0010-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v8 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 849cac3947..cf4387f443 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 49938c9ed4..282dcb9791 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index b9ba4f9fb3..bcf1497f67 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -795,10 +795,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -829,15 +826,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --xqq4defy3uncu6k6 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v8-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v11 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index ce47a158ae..ddcdbbaf7e 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2308,7 +2308,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 49938c9ed4..282dcb9791 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index 5c2a7b7422..9c7b8bf162 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -806,10 +806,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -840,15 +837,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -2060,7 +2052,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2072,7 +2063,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --owzzsiozz6hgpp7e Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v11-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 849cac3947..cf4387f443 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 49938c9ed4..282dcb9791 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index b9ba4f9fb3..bcf1497f67 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -795,10 +795,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -829,15 +826,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --7mdtsjmrzitrgzgx Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v9-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 849cac3947..cf4387f443 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 49938c9ed4..282dcb9791 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index b9ba4f9fb3..bcf1497f67 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -795,10 +795,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -829,15 +826,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --7mdtsjmrzitrgzgx Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v9-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v11 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index ce47a158ae..ddcdbbaf7e 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2308,7 +2308,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 49938c9ed4..282dcb9791 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index 5c2a7b7422..9c7b8bf162 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -806,10 +806,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -840,15 +837,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -2060,7 +2052,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2072,7 +2063,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --owzzsiozz6hgpp7e Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v11-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v8 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 849cac3947..cf4387f443 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 49938c9ed4..282dcb9791 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index b9ba4f9fb3..bcf1497f67 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -795,10 +795,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -829,15 +826,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --xqq4defy3uncu6k6 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v8-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v10 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 849cac3947..cf4387f443 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 49938c9ed4..282dcb9791 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index b9ba4f9fb3..bcf1497f67 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -795,10 +795,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -829,15 +826,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --3o7pc6dfau5a5hry Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v10-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v7 08/13] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 10c1c3b616b..a1ec50ab7a8 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2248,7 +2248,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 94a5e2da17c..bbdaa591891 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index f1d0d4b78e3..e35bd36e710 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -787,10 +787,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -821,15 +818,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -1988,7 +1980,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2000,7 +1991,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --kqqpqghcwbcc3dt5 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v7-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v8 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 849cac3947..cf4387f443 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 49938c9ed4..282dcb9791 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index b9ba4f9fb3..bcf1497f67 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -795,10 +795,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -829,15 +826,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --xqq4defy3uncu6k6 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v8-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v10 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 849cac3947..cf4387f443 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2250,7 +2250,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 49938c9ed4..282dcb9791 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index b9ba4f9fb3..bcf1497f67 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -795,10 +795,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -829,15 +826,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -2025,7 +2017,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2037,7 +2028,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --3o7pc6dfau5a5hry Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v10-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v7 08/13] Remove table_scan_bitmap_next_tuple parameter tbmres @ 2024-02-12 23:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2024-02-12 23:13 UTC (permalink / raw) With the addition of the proposed streaming read API [1], table_scan_bitmap_next_block() will no longer take a TBMIterateResult as an input. Instead table AMs will be responsible for implementing a callback for the streaming read API which specifies which blocks should be prefetched and read. Thus, it no longer makes sense to use the TBMIterateResult as a means of communication between table_scan_bitmap_next_tuple() and table_scan_bitmap_next_block(). Note that this parameter was unused by heap AM's implementation of table_scan_bitmap_next_tuple(). [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2... --- src/backend/access/heap/heapam_handler.c | 1 - src/backend/executor/nodeBitmapHeapscan.c | 2 +- src/include/access/tableam.h | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 10c1c3b616b..a1ec50ab7a8 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2248,7 +2248,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, static bool heapam_scan_bitmap_next_tuple(TableScanDesc scan, - TBMIterateResult *tbmres, TupleTableSlot *slot) { HeapScanDesc hscan = (HeapScanDesc) scan; diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 94a5e2da17c..bbdaa591891 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -286,7 +286,7 @@ BitmapHeapNext(BitmapHeapScanState *node) /* * Attempt to fetch tuple from AM. */ - if (!table_scan_bitmap_next_tuple(scan, tbmres, slot)) + if (!table_scan_bitmap_next_tuple(scan, slot)) { /* nothing more to look at on this page */ node->tbmres = tbmres = NULL; diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index f1d0d4b78e3..e35bd36e710 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -787,10 +787,7 @@ typedef struct TableAmRoutine * * This will typically read and pin the target block, and do the necessary * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might - * make sense to perform tuple visibility checks at this time). For some - * AMs it will make more sense to do all the work referencing `tbmres` - * contents here, for others it might be better to defer more work to - * scan_bitmap_next_tuple. + * make sense to perform tuple visibility checks at this time). * * If `tbmres->blockno` is -1, this is a lossy scan and all visible tuples * on the page have to be returned, otherwise the tuples at offsets in @@ -821,15 +818,10 @@ typedef struct TableAmRoutine * Fetch the next tuple of a bitmap table scan into `slot` and return true * if a visible tuple was found, false otherwise. * - * For some AMs it will make more sense to do all the work referencing - * `tbmres` contents in scan_bitmap_next_block, for others it might be - * better to defer more work to this callback. - * * Optional callback, but either both scan_bitmap_next_block and * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_tuple) (TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot); /* @@ -1988,7 +1980,6 @@ table_scan_bitmap_next_block(TableScanDesc scan, */ static inline bool table_scan_bitmap_next_tuple(TableScanDesc scan, - struct TBMIterateResult *tbmres, TupleTableSlot *slot) { /* @@ -2000,7 +1991,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, - tbmres, slot); } -- 2.40.1 --kqqpqghcwbcc3dt5 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v7-0009-Make-table_scan_bitmap_next_block-async-friendly.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
end of thread, other threads:[~2024-02-12 23:13 UTC | newest] Thread overview: 26+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-11-28 22:30 [PATCH 3/6] Be less verbose on variable names Alvaro Herrera <[email protected]> 2022-01-18 06:30 Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-03-14 08:33 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-03-14 09:32 ` Re: simplifying foreign key/RI checks Zhihong Yu <[email protected]> 2022-03-22 04:01 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-04-07 01:05 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-04-11 07:47 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2022-05-02 11:50 ` Re: simplifying foreign key/RI checks Amit Langote <[email protected]> 2024-02-12 23:13 [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v6 09/14] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v10 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v12 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v7 08/13] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v6 09/14] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v6 09/14] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v8 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v11 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v9 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v11 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v8 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v10 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v7 08/13] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v8 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v10 08/17] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[email protected]> 2024-02-12 23:13 [PATCH v7 08/13] Remove table_scan_bitmap_next_tuple parameter tbmres Melanie Plageman <[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