public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v5 1/2] Add a new OUTDATED filtering facility for REINDEX command. 4+ messages / 3 participants [nested] [flat]
* [PATCH v5 1/2] Add a new OUTDATED filtering facility for REINDEX command. @ 2020-12-03 07:54 Julien Rouhaud <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Julien Rouhaud @ 2020-12-03 07:54 UTC (permalink / raw) OUTDATED is added a new unreserved keyword. When used, REINDEX will only process indexes that have an outdated dependency. For now, only dependency on collations are supported but we'll likely support other kind of dependency in the future. Also add a new pg_index_has_outdated_dependency(regclass) SQL function, so client code can filter such indexes if needed. This function will also be used in a following commit to teach reindexdb to use this new OUTDATED option and order the tables by the amount of work that will actually be done. Catversion (should be) bumped. Author: Julien Rouhaud <[email protected]> Reviewed-by: Discussion: https://postgr.es/m/20201203093143.GA64934%40nol --- doc/src/sgml/func.sgml | 27 ++++-- doc/src/sgml/ref/reindex.sgml | 12 +++ src/backend/catalog/index.c | 107 ++++++++++++++++++++- src/backend/commands/indexcmds.c | 12 ++- src/backend/parser/gram.y | 4 +- src/backend/utils/cache/relcache.c | 40 ++++++++ src/bin/psql/tab-complete.c | 2 +- src/include/catalog/index.h | 3 + src/include/catalog/pg_proc.dat | 4 + src/include/parser/kwlist.h | 1 + src/include/utils/relcache.h | 1 + src/test/regress/expected/create_index.out | 10 ++ src/test/regress/sql/create_index.sql | 10 ++ 13 files changed, 221 insertions(+), 12 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index bf99f82149..2cf6e66234 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -26381,12 +26381,13 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size <para> <xref linkend="functions-admin-index-table"/> shows the functions - available for index maintenance tasks. (Note that these maintenance - tasks are normally done automatically by autovacuum; use of these - functions is only required in special cases.) - These functions cannot be executed during recovery. - Use of these functions is restricted to superusers and the owner - of the given index. + available for index maintenance tasks. (Note that the maintenance + tasks performing actions on indexes are normally done automatically by + autovacuum; use of these functions is only required in special cases.) + The functions performing actions on indexes cannot be executed during + recovery. + Use of the functions performing actions on indexes is restricted to + superusers and the owner of the given index. </para> <table id="functions-admin-index-table"> @@ -26471,6 +26472,20 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size option. </para></entry> </row> + + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>pg_index_has_outdated_dependency</primary> + </indexterm> + <function>pg_index_has_outdated_dependency</function> ( <parameter>index</parameter> <type>regclass</type> ) + <returnvalue>boolean</returnvalue> + </para> + <para> + Check if the specified index has any outdated dependency. For now only + dependency on collations are supported. + </para></entry> + </row> </tbody> </tgroup> </table> diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index b22d39eba9..2d94d49cde 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -26,6 +26,7 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN <phrase>where <replaceable class="parameter">option</replaceable> can be one of:</phrase> CONCURRENTLY [ <replaceable class="parameter">boolean</replaceable> ] + OUTDATED [ <replaceable class="parameter">boolean</replaceable> ] TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> VERBOSE [ <replaceable class="parameter">boolean</replaceable> ] </synopsis> @@ -188,6 +189,17 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN </listitem> </varlistentry> + <varlistentry> + <term><literal>OUTDATED</literal></term> + <listitem> + <para> + This option can be used to filter the list of indexes to rebuild and only + process indexes that have outdated dependencies. Fow now, the only + handle dependency is for the collation provider version. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><literal>TABLESPACE</literal></term> <listitem> diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 4ef61b5efd..571feac5db 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -100,6 +100,12 @@ typedef struct Oid pendingReindexedIndexes[FLEXIBLE_ARRAY_MEMBER]; } SerializedReindexState; +typedef struct +{ + Oid relid; /* targetr index oid */ + bool outdated; /* depends on at least on deprected collation? */ +} IndexHasOutdatedColl; + /* non-export function prototypes */ static bool relationHasPrimaryKey(Relation rel); static TupleDesc ConstructTupleDescriptor(Relation heapRelation, @@ -1351,6 +1357,105 @@ index_check_collation_versions(Oid relid) list_free(context.warned_colls); } +/* + * Detect if an index depends on at least one outdated collation. + * This is a callback for visitDependenciesOf(). + */ +static bool +do_check_index_has_outdated_collation(const ObjectAddress *otherObject, + const char *version, + char **new_version, + void *data) +{ + IndexHasOutdatedColl *context = data; + char *current_version; + + /* We only care about dependencies on collations. */ + if (otherObject->classId != CollationRelationId) + return false; + + /* Fast exit if we already found a outdated collation version. */ + if (context->outdated) + return false; + + /* Ask the provider for the current version. Give up if unsupported. */ + current_version = get_collation_version_for_oid(otherObject->objectId, + false); + if (!current_version) + return false; + + if (!version || strcmp(version, current_version) != 0) + context->outdated = true; + + return false; +} + +Datum +pg_index_has_outdated_dependency(PG_FUNCTION_ARGS) +{ + Oid indexOid = PG_GETARG_OID(0); + Relation rel; + bool isIndex; + bool res; + + rel = try_relation_open(indexOid, AccessShareLock); + + if (rel == NULL) + PG_RETURN_NULL(); + + isIndex = rel->rd_rel->relkind == RELKIND_INDEX; + + if (!isIndex) + { + relation_close(rel, AccessShareLock); + PG_RETURN_NULL(); + } + + res = index_has_outdated_dependency(indexOid); + + relation_close(rel, AccessShareLock); + + PG_RETURN_BOOL(res); +} + +/* + * Check whether the given index has a dependency with an outdated + * collation version. + * Caller must hold a suitable lock and make sure that the given Oid belongs to + * an index. + */ +bool +index_has_outdated_collation(Oid indexOid) +{ + ObjectAddress object; + IndexHasOutdatedColl context; + + object.classId = RelationRelationId; + object.objectId = indexOid; + object.objectSubId = 0; + + context.relid = indexOid; + context.outdated = false; + + visitDependenciesOf(&object, &do_check_index_has_outdated_collation, + &context); + + return context.outdated; +} + +/* + * Check whether the given index has a dependency with an outdated + * refobjversion. + * Caller must hold a suitable lock and make sure that the given Oid belongs to + * an index. + * For now, only dependency on collations are supported. + */ +bool +index_has_outdated_dependency(Oid indexOid) +{ + return index_has_outdated_collation(indexOid); +} + /* * Update the version for collations. A callback for visitDependenciesOf(). */ @@ -3991,7 +4096,7 @@ reindex_relation(Oid relid, int flags, ReindexParams *params) * relcache to get this with a sequential scan if ignoring system * indexes.) */ - indexIds = RelationGetIndexList(rel); + indexIds = RelationGetIndexListFiltered(rel, params->options); if (flags & REINDEX_REL_SUPPRESS_INDEX_USE) { diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 8bc652ecd3..0e1bcad101 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2484,6 +2484,7 @@ ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel) bool concurrently = false; bool verbose = false; char *tablespacename = NULL; + bool outdated_filter = false; /* Parse option list */ foreach(lc, stmt->params) @@ -2496,6 +2497,8 @@ ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel) concurrently = defGetBoolean(opt); else if (strcmp(opt->defname, "tablespace") == 0) tablespacename = defGetString(opt); + else if (strcmp(opt->defname, "outdated") == 0) + outdated_filter = defGetBoolean(opt); else ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -2510,7 +2513,8 @@ ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel) params.options = (verbose ? REINDEXOPT_VERBOSE : 0) | - (concurrently ? REINDEXOPT_CONCURRENTLY : 0); + (concurrently ? REINDEXOPT_CONCURRENTLY : 0) | + (outdated_filter ? REINDEXOPT_OUTDATED : 0); /* * Assign the tablespace OID to move indexes to, with InvalidOid to do @@ -3307,7 +3311,8 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) RelationGetRelationName(heapRelation)))); /* Add all the valid indexes of relation to list */ - foreach(lc, RelationGetIndexList(heapRelation)) + foreach(lc, RelationGetIndexListFiltered(heapRelation, + params->options)) { Oid cellOid = lfirst_oid(lc); Relation indexRelation = index_open(cellOid, @@ -3359,7 +3364,8 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) MemoryContextSwitchTo(oldcontext); - foreach(lc2, RelationGetIndexList(toastRelation)) + foreach(lc2, RelationGetIndexListFiltered(toastRelation, + params->options)) { Oid cellOid = lfirst_oid(lc2); Relation indexRelation = index_open(cellOid, diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 652be0b96d..a67cfa867c 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -674,7 +674,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); NULLS_P NUMERIC OBJECT_P OF OFF OFFSET OIDS OLD ON ONLY OPERATOR OPTION OPTIONS OR - ORDER ORDINALITY OTHERS OUT_P OUTER_P + ORDER ORDINALITY OTHERS OUT_P OUTDATED OUTER_P OVER OVERLAPS OVERLAY OVERRIDING OWNED OWNER PARALLEL PARSER PARTIAL PARTITION PASSING PASSWORD PLACING PLANS POLICY @@ -15430,6 +15430,7 @@ unreserved_keyword: | OPTIONS | ORDINALITY | OTHERS + | OUTDATED | OVER | OVERRIDING | OWNED @@ -16002,6 +16003,7 @@ bare_label_keyword: | ORDINALITY | OTHERS | OUT_P + | OUTDATED | OUTER_P | OVERLAY | OVERRIDING diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 7ef510cd01..06545ee550 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -4620,6 +4620,46 @@ RelationGetIndexList(Relation relation) return result; } +/* + * RelationGetIndexListFiltered -- get a filtered list of indexes on this + * relation. + * + * Calls RelationGetIndexList and only keep indexes that have an outdated + * dependency. For now, only collation version dependency is supported. + */ +List * +RelationGetIndexListFiltered(Relation relation, bits32 options) +{ + List *result, + *full_list; + ListCell *lc; + + full_list = RelationGetIndexList(relation); + + /* Fast exit if no filtering was asked, or if the list if empty. */ + if (((options & REINDEXOPT_OUTDATED) == 0) || full_list == NIL) + return full_list; + + result = NIL; + foreach(lc, full_list) + { + Oid indexOid = lfirst_oid(lc); + + /* + * Check for outdated collation version dependency. + */ + if (index_has_outdated_collation(indexOid)) + { + result = lappend_oid(result, indexOid); + continue; + } + + /* Didn't find any outdated dependency, index will be ignored. */ + } + + return result; +} + /* * RelationGetStatExtList * get a list of OIDs of statistics objects on this relation diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 9f0208ac49..fac3e0476a 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -3674,7 +3674,7 @@ psql_completion(const char *text, int start, int end) * one word, so the above test is correct. */ if (ends_with(prev_wd, '(') || ends_with(prev_wd, ',')) - COMPLETE_WITH("CONCURRENTLY", "TABLESPACE", "VERBOSE"); + COMPLETE_WITH("CONCURRENTLY", "OUTDATED'", "TABLESPACE", "VERBOSE"); else if (TailMatches("TABLESPACE")) COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces); } diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h index e22d506436..298c0c633c 100644 --- a/src/include/catalog/index.h +++ b/src/include/catalog/index.h @@ -42,6 +42,7 @@ typedef struct ReindexParams #define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */ #define REINDEXOPT_MISSING_OK 0x04 /* skip missing relations */ #define REINDEXOPT_CONCURRENTLY 0x08 /* concurrent mode */ +#define REINDEXOPT_OUTDATED 0x10/* outdated collation only */ /* state info for validate_index bulkdelete callback */ typedef struct ValidateIndexState @@ -137,6 +138,8 @@ extern void FormIndexDatum(IndexInfo *indexInfo, bool *isnull); extern void index_check_collation_versions(Oid relid); +extern bool index_has_outdated_collation(Oid indexOid); +extern bool index_has_outdated_dependency(Oid indexOid); extern void index_update_collation_versions(Oid relid, Oid coll); extern void index_build(Relation heapRelation, diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 3d3974f467..4874d33996 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -949,6 +949,10 @@ proname => 'pg_indexam_has_property', provolatile => 's', prorettype => 'bool', proargtypes => 'oid text', prosrc => 'pg_indexam_has_property' }, +{ oid => '8102', descr => 'test property of an index', + proname => 'pg_index_has_outdated_dependency', provolatile => 's', + prorettype => 'bool', proargtypes => 'regclass', + prosrc => 'pg_index_has_outdated_dependency' }, { oid => '637', descr => 'test property of an index', proname => 'pg_index_has_property', provolatile => 's', prorettype => 'bool', proargtypes => 'regclass text', prosrc => 'pg_index_has_property' }, diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h index 28083aaac9..e6c725d9a6 100644 --- a/src/include/parser/kwlist.h +++ b/src/include/parser/kwlist.h @@ -295,6 +295,7 @@ PG_KEYWORD("order", ORDER, RESERVED_KEYWORD, AS_LABEL) PG_KEYWORD("ordinality", ORDINALITY, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("others", OTHERS, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("out", OUT_P, COL_NAME_KEYWORD, BARE_LABEL) +PG_KEYWORD("outdated", OUTDATED, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("outer", OUTER_P, TYPE_FUNC_NAME_KEYWORD, BARE_LABEL) PG_KEYWORD("over", OVER, UNRESERVED_KEYWORD, AS_LABEL) PG_KEYWORD("overlaps", OVERLAPS, TYPE_FUNC_NAME_KEYWORD, AS_LABEL) diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h index 2fcdf79323..a7a2272abd 100644 --- a/src/include/utils/relcache.h +++ b/src/include/utils/relcache.h @@ -45,6 +45,7 @@ extern void RelationClose(Relation relation); */ extern List *RelationGetFKeyList(Relation relation); extern List *RelationGetIndexList(Relation relation); +extern List *RelationGetIndexListFiltered(Relation relation, bits32 options); extern List *RelationGetStatExtList(Relation relation); extern Oid RelationGetPrimaryKeyIndex(Relation relation); extern Oid RelationGetReplicaIndex(Relation relation); diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out index 830fdddf24..5f7c49c650 100644 --- a/src/test/regress/expected/create_index.out +++ b/src/test/regress/expected/create_index.out @@ -2018,6 +2018,16 @@ INFO: index "reindex_verbose_pkey" was reindexed \set VERBOSITY default DROP TABLE reindex_verbose; -- +-- REINDEX (OUTDATED) +-- +CREATE TABLE reindex_coll(id integer primary key); +\set VERBOSITY terse \\ -- suppress machine-dependent details +-- no suitable index should be found +REINDEX (OUTDATED) TABLE reindex_coll; +NOTICE: table "reindex_coll" has no indexes to reindex +\set VERBOSITY default +DROP TABLE reindex_coll; +-- -- REINDEX CONCURRENTLY -- CREATE TABLE concur_reindex_tab (c1 int); diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql index 8bc76f7c6f..808cacee5d 100644 --- a/src/test/regress/sql/create_index.sql +++ b/src/test/regress/sql/create_index.sql @@ -790,6 +790,16 @@ REINDEX (VERBOSE) TABLE reindex_verbose; \set VERBOSITY default DROP TABLE reindex_verbose; +-- +-- REINDEX (OUTDATED) +-- +CREATE TABLE reindex_coll(id integer primary key); +\set VERBOSITY terse \\ -- suppress machine-dependent details +-- no suitable index should be found +REINDEX (OUTDATED) TABLE reindex_coll; +\set VERBOSITY default +DROP TABLE reindex_coll; + -- -- REINDEX CONCURRENTLY -- -- 2.30.1 --oao5se3im4osqowp Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v5-0002-Add-a-outdated-option-to-reindexdb.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: support for MERGE @ 2022-03-12 15:36 Alvaro Herrera <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Alvaro Herrera @ 2022-03-12 15:36 UTC (permalink / raw) To: Zhihong Yu <[email protected]>; Simon Riggs <[email protected]>; +Cc: Pg Hackers <[email protected]>; Tomas Vondra <[email protected]>; Daniel Westermann <[email protected]>; Amit Langote <[email protected]>; Justin Pryzby <[email protected]>; Japin Li <[email protected]>; Erik Rijkers <[email protected]>; Jaime Casanova <[email protected]>; Andres Freund <[email protected]> FYI I intend to get the ModifyTable split patch (0001+0003) pushed hopefully on Tuesday or Wednesday next week, unless something really ugly is found on it. As for MERGE proper, I'm aiming at getting that one pushed on the week starting March 21st, though of course I'll spend some more time on it and will probably post further versions of it before that. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ Thou shalt check the array bounds of all strings (indeed, all arrays), for surely where thou typest "foo" someone someday shall type "supercalifragilisticexpialidocious" (5th Commandment for C programmers) ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: support for MERGE @ 2022-03-14 14:36 Amit Langote <[email protected]> parent: Alvaro Herrera <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Amit Langote @ 2022-03-14 14:36 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Zhihong Yu <[email protected]>; Simon Riggs <[email protected]>; Pg Hackers <[email protected]>; Tomas Vondra <[email protected]>; Daniel Westermann <[email protected]>; Justin Pryzby <[email protected]>; Japin Li <[email protected]>; Erik Rijkers <[email protected]>; Jaime Casanova <[email protected]>; Andres Freund <[email protected]> On Sun, Mar 13, 2022 at 12:36 AM Alvaro Herrera <[email protected]> wrote: > FYI I intend to get the ModifyTable split patch (0001+0003) pushed > hopefully on Tuesday or Wednesday next week, unless something really > ugly is found on it. I looked at 0001 and found a few things that could perhaps be improved. + /* Slot containing tuple obtained from subplan, for RETURNING */ + TupleTableSlot *planSlot; I think planSlot is also used by an FDW to look up any FDW-specific junk attributes that the FDW's scan method would have injected into the slot, so maybe no need to specifically mention only RETURNING here as what cares about it. + /* + * The tuple produced by EvalPlanQual to retry from, if a cross-partition + * UPDATE requires it + */ + TupleTableSlot *retrySlot; Maybe a bit long name, though how about calling this updateRetrySlot to make it apparent that what is to be retried with the slot is the whole UPDATE operation, not some sub-operation (like say the cross-partition portion)? + /* + * The tuple projected by the INSERT's RETURNING clause, when doing a + * cross-partition UPDATE + */ + TupleTableSlot *projectedFromInsert; I'd have gone with cpUpdateReturningSlot here, because that is what it looks to me to be. The fact that it is ExecInsert() that computes the RETURNING targetlist projection seems like an implementation detail. I know you said you don't like having "Slot" in the name, but then we also have retrySlot. :) +/* + * Context struct containing output data specific to UPDATE operations. + */ +typedef struct UpdateContext +{ + bool updateIndexes; /* index update required? */ + bool crossPartUpdate; /* was it a cross-partition update? */ +} UpdateContext; I wonder if it isn't more readable to just have these two be the output arguments of ExecUpdateAct()? +redo_act: + /* XXX reinit ->crossPartUpdate here? */ Why not just make ExecUpdateAct() always set the flag, not only when a cross-partition update does occur? Will look some more in the morning tomorrow. -- Amit Langote EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: support for MERGE @ 2022-03-15 09:40 Amit Langote <[email protected]> parent: Amit Langote <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Amit Langote @ 2022-03-15 09:40 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Zhihong Yu <[email protected]>; Simon Riggs <[email protected]>; Pg Hackers <[email protected]>; Tomas Vondra <[email protected]>; Daniel Westermann <[email protected]>; Justin Pryzby <[email protected]>; Japin Li <[email protected]>; Erik Rijkers <[email protected]>; Jaime Casanova <[email protected]>; Andres Freund <[email protected]> On Mon, Mar 14, 2022 at 11:36 PM Amit Langote <[email protected]> wrote: > On Sun, Mar 13, 2022 at 12:36 AM Alvaro Herrera <[email protected]> wrote: > > FYI I intend to get the ModifyTable split patch (0001+0003) pushed > > hopefully on Tuesday or Wednesday next week, unless something really > > ugly is found on it. > > I looked at 0001 and found a few things that could perhaps be improved. > > + /* Slot containing tuple obtained from subplan, for RETURNING */ > + TupleTableSlot *planSlot; > > I think planSlot is also used by an FDW to look up any FDW-specific > junk attributes that the FDW's scan method would have injected into > the slot, so maybe no need to specifically mention only RETURNING here > as what cares about it. > > + /* > + * The tuple produced by EvalPlanQual to retry from, if a cross-partition > + * UPDATE requires it > + */ > + TupleTableSlot *retrySlot; > > Maybe a bit long name, though how about calling this updateRetrySlot > to make it apparent that what is to be retried with the slot is the > whole UPDATE operation, not some sub-operation (like say the > cross-partition portion)? I think I meant to suggest cpUpdateRetrySlot, as in, the slot populated by ExecCrossPartitionUpdate() with a tuple to retry the UPDATE operation with, at least the portion of it that ExecUpdateAct() is charge of. > + /* > + * The tuple projected by the INSERT's RETURNING clause, when doing a > + * cross-partition UPDATE > + */ > + TupleTableSlot *projectedFromInsert; > > I'd have gone with cpUpdateReturningSlot here, because that is what it > looks to me to be. The fact that it is ExecInsert() that computes the > RETURNING targetlist projection seems like an implementation detail. > > I know you said you don't like having "Slot" in the name, but then we > also have retrySlot. :) > > +/* > + * Context struct containing output data specific to UPDATE operations. > + */ > +typedef struct UpdateContext > +{ > + bool updateIndexes; /* index update required? */ > + bool crossPartUpdate; /* was it a cross-partition update? */ > +} UpdateContext; > > I wonder if it isn't more readable to just have these two be the > output arguments of ExecUpdateAct()? > > +redo_act: > + /* XXX reinit ->crossPartUpdate here? */ > > Why not just make ExecUpdateAct() always set the flag, not only when a > cross-partition update does occur? > > Will look some more in the morning tomorrow. Here are some more improvement suggestions: +/* + * Context struct for a ModifyTable operation, containing basic execution state + * and some output data. + */ Does it make sense to expand "some output data", maybe as: ...and some output variables populated by the ExecUpdateAct() and ExecDeleteAct() to report the result of their actions to ExecUpdate() and ExecDelete() respectively. + /* output */ + TM_FailureData tmfd; /* stock failure data */ Maybe we should be more specific here, say: Information about the changes that were found to be made concurrently to a tuple being updated or deleted + LockTupleMode lockmode; /* tuple lock mode */ Also here, say: Lock mode to acquire on the latest tuple before performing EvalPlanQual() on it +/* + * ExecDeleteAct -- subroutine for ExecDelete + * + * Actually delete the tuple, when operating on a plain or partitioned table. +/* + * ExecUpdateAct -- subroutine for ExecUpdate + * + * Actually update the tuple, when operating on a plain or partitioned table. Hmm, ExecDelete() and ExecUpdate() never see a partitioned table, because both DELETE and UPDATE are always performed directly on leaf partitions. The (root) partitioned table only gets involved if the UPDATE of a leaf partition tuple causes it to move to another partition, that too only for applying tuple routing algorithm to find the destination leaf partition. So the following suffices, for ExecDeleteAct(): Actually delete the tuple from a plain table. and for ExecUpdateAct(), maybe it makes sense to mention the possibility of a cross-partition partition. Actually update the tuple of a plain table. If the table happens to be a leaf partition and the update causes its partition constraint to be violated, ExecCrossPartitionUpdate() is invoked to move the updated tuple into the appropriate partition. + * Closing steps of tuple deletion; this invokes AFTER FOR EACH ROW triggers, + * including the UPDATE triggers if there was a cross-partition tuple move. How about: ...including the UPDATE triggers if the ExecDelete() is being done as part of a cross-partition tuple move. + /* and project to create a current version of the new tuple */ How about: and project the new tuple to retry the UPDATE with -- Amit Langote EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2022-03-15 09:40 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-12-03 07:54 [PATCH v5 1/2] Add a new OUTDATED filtering facility for REINDEX command. Julien Rouhaud <[email protected]> 2022-03-12 15:36 Re: support for MERGE Alvaro Herrera <[email protected]> 2022-03-14 14:36 ` Re: support for MERGE Amit Langote <[email protected]> 2022-03-15 09:40 ` Re: support for MERGE Amit Langote <[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