public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 2/2] Fast COPY FROM into the foreign or sharded table. 4+ messages / 3 participants [nested] [flat]
* [PATCH 2/2] Fast COPY FROM into the foreign or sharded table. @ 2020-12-14 08:37 Andrey Lepikhov <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Andrey Lepikhov @ 2020-12-14 08:37 UTC (permalink / raw) This feature enables bulk COPY into foreign table in the case of multi inserts is possible and foreign table has non-zero number of columns. FDWAPI was extended by next routines: * BeginForeignCopy * EndForeignCopy * ExecForeignCopy BeginForeignCopy and EndForeignCopy initialize and free the CopyState of bulk COPY. The ExecForeignCopy routine send 'COPY ... FROM STDIN' command to the foreign server, in iterative manner send tuples by CopyTo() machinery, send EOF to this connection. Code that constructed list of columns for a given foreign relation in the deparseAnalyzeSql() routine is separated to the deparseRelColumnList(). It is reused in the deparseCopyFromSql(). Added TAP-tests on the specific corner cases of COPY FROM STDIN operation. By the analogy of CopyFrom() the CopyState structure was extended with data_dest_cb callback. It is used for send text representation of a tuple to a custom destination. The PgFdwModifyState structure is extended with the cstate field. It is needed for avoid repeated initialization of CopyState. ALso for this reason CopyTo() routine was split into the set of routines CopyToStart()/ CopyTo()/CopyToFinish(). Enum CopyInsertMethod removed. This logic implements by ri_usesMultiInsert field of the ResultRelInfo sructure. Discussion: https://www.postgresql.org/message-id/flat/3d0909dc-3691-a576-208a-90986e55489f%40postgrespro.ru Authors: Andrey Lepikhov, Ashutosh Bapat, Amit Langote --- contrib/postgres_fdw/deparse.c | 60 ++++++-- .../postgres_fdw/expected/postgres_fdw.out | 46 +++++- contrib/postgres_fdw/postgres_fdw.c | 137 ++++++++++++++++++ contrib/postgres_fdw/postgres_fdw.h | 1 + contrib/postgres_fdw/sql/postgres_fdw.sql | 45 ++++++ doc/src/sgml/fdwhandler.sgml | 73 ++++++++++ src/backend/commands/copy.c | 4 +- src/backend/commands/copyfrom.c | 133 +++++++++-------- src/backend/commands/copyto.c | 84 ++++++++--- src/backend/executor/execMain.c | 8 +- src/backend/executor/execPartition.c | 27 +++- src/include/commands/copy.h | 8 +- src/include/foreign/fdwapi.h | 15 ++ 13 files changed, 540 insertions(+), 101 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index ca2f9f3215..b2a71faabc 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -184,6 +184,8 @@ static void appendAggOrderBy(List *orderList, List *targetList, static void appendFunctionName(Oid funcid, deparse_expr_cxt *context); static Node *deparseSortGroupClause(Index ref, List *tlist, bool force_colno, deparse_expr_cxt *context); +static List *deparseRelColumnList(StringInfo buf, Relation rel, + bool enclose_in_parens); /* * Helper functions @@ -1763,6 +1765,20 @@ deparseInsertSql(StringInfo buf, RangeTblEntry *rte, withCheckOptionList, returningList, retrieved_attrs); } +/* + * Deparse COPY FROM into given buf. + * We need to use list of parameters at each query. + */ +void +deparseCopyFromSql(StringInfo buf, Relation rel) +{ + appendStringInfoString(buf, "COPY "); + deparseRelation(buf, rel); + (void) deparseRelColumnList(buf, rel, true); + + appendStringInfoString(buf, " FROM STDIN "); +} + /* * deparse remote UPDATE statement * @@ -2066,6 +2082,30 @@ deparseAnalyzeSizeSql(StringInfo buf, Relation rel) */ void deparseAnalyzeSql(StringInfo buf, Relation rel, List **retrieved_attrs) +{ + appendStringInfoString(buf, "SELECT "); + *retrieved_attrs = deparseRelColumnList(buf, rel, false); + + /* Don't generate bad syntax for zero-column relation. */ + if (list_length(*retrieved_attrs) == 0) + appendStringInfoString(buf, "NULL"); + + /* + * Construct FROM clause + */ + appendStringInfoString(buf, " FROM "); + deparseRelation(buf, rel); +} + +/* + * Construct the list of columns of given foreign relation in the order they + * appear in the tuple descriptor of the relation. Ignore any dropped columns. + * Use column names on the foreign server instead of local names. + * + * Optionally enclose the list in parantheses. + */ +static List * +deparseRelColumnList(StringInfo buf, Relation rel, bool enclose_in_parens) { Oid relid = RelationGetRelid(rel); TupleDesc tupdesc = RelationGetDescr(rel); @@ -2074,10 +2114,8 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List **retrieved_attrs) List *options; ListCell *lc; bool first = true; + List *retrieved_attrs = NIL; - *retrieved_attrs = NIL; - - appendStringInfoString(buf, "SELECT "); for (i = 0; i < tupdesc->natts; i++) { /* Ignore dropped columns. */ @@ -2086,6 +2124,9 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List **retrieved_attrs) if (!first) appendStringInfoString(buf, ", "); + else if (enclose_in_parens) + appendStringInfoChar(buf, '('); + first = false; /* Use attribute name or column_name option. */ @@ -2105,18 +2146,13 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List **retrieved_attrs) appendStringInfoString(buf, quote_identifier(colname)); - *retrieved_attrs = lappend_int(*retrieved_attrs, i + 1); + retrieved_attrs = lappend_int(retrieved_attrs, i + 1); } - /* Don't generate bad syntax for zero-column relation. */ - if (first) - appendStringInfoString(buf, "NULL"); + if (enclose_in_parens && list_length(retrieved_attrs) > 0) + appendStringInfoChar(buf, ')'); - /* - * Construct FROM clause - */ - appendStringInfoString(buf, " FROM "); - deparseRelation(buf, rel); + return retrieved_attrs; } /* diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 2d88d06358..0e2c15c648 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -8076,8 +8076,9 @@ copy rem2 from stdin; copy rem2 from stdin; -- ERROR ERROR: new row for relation "loc2" violates check constraint "loc2_f1positive" DETAIL: Failing row contains (-1, xyzzy). -CONTEXT: remote SQL command: INSERT INTO public.loc2(f1, f2) VALUES ($1, $2) -COPY rem2, line 1: "-1 xyzzy" +CONTEXT: COPY loc2, line 1: "-1 xyzzy" +remote SQL command: COPY public.loc2(f1, f2) FROM STDIN +COPY rem2, line 2: "" select * from rem2; f1 | f2 ----+----- @@ -8088,6 +8089,19 @@ select * from rem2; alter foreign table rem2 drop constraint rem2_f1positive; alter table loc2 drop constraint loc2_f1positive; delete from rem2; +create table foo (a int) partition by list (a); +create table foo1 (like foo); +create foreign table ffoo1 partition of foo for values in (1) + server loopback options (table_name 'foo1'); +create table foo2 (like foo); +create foreign table ffoo2 partition of foo for values in (2) + server loopback options (table_name 'foo2'); +create function print_new_row() returns trigger language plpgsql as $$ + begin raise notice '%', new; return new; end; $$; +create trigger ffoo1_br_trig before insert on ffoo1 + for each row execute function print_new_row(); +copy foo from stdin; +NOTICE: (1) -- Test local triggers create trigger trig_stmt_before before insert on rem2 for each statement execute procedure trigger_func(); @@ -8196,6 +8210,34 @@ drop trigger rem2_trig_row_before on rem2; drop trigger rem2_trig_row_after on rem2; drop trigger loc2_trig_row_before_insert on loc2; delete from rem2; +alter table loc2 drop column f1; +alter table loc2 drop column f2; +copy rem2 from stdin; +ERROR: column "f1" of relation "loc2" does not exist +CONTEXT: remote SQL command: COPY public.loc2(f1, f2) FROM STDIN +COPY rem2, line 3: "" +alter table loc2 add column f1 int; +alter table loc2 add column f2 int; +select * from rem2; + f1 | f2 +----+---- +(0 rows) + +-- dropped columns locally and on the foreign server +alter table rem2 drop column f1; +alter table rem2 drop column f2; +copy rem2 from stdin; +select * from rem2; +-- +(2 rows) + +alter table loc2 drop column f1; +alter table loc2 drop column f2; +copy rem2 from stdin; +select * from rem2; +-- +(4 rows) + -- test COPY FROM with foreign table created in the same transaction create table loc3 (f1 int, f2 text); begin; diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index b6c72e1d1e..dd185bdc3b 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -18,6 +18,7 @@ #include "access/sysattr.h" #include "access/table.h" #include "catalog/pg_class.h" +#include "commands/copy.h" #include "commands/defrem.h" #include "commands/explain.h" #include "commands/vacuum.h" @@ -191,6 +192,7 @@ typedef struct PgFdwModifyState /* for update row movement if subplan result rel */ struct PgFdwModifyState *aux_fmstate; /* foreign-insert state, if * created */ + CopyToState cstate; /* foreign COPY state, if used */ } PgFdwModifyState; /* @@ -357,6 +359,13 @@ static void postgresBeginForeignInsert(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo); static void postgresEndForeignInsert(EState *estate, ResultRelInfo *resultRelInfo); +static void postgresBeginForeignCopy(ModifyTableState *mtstate, + ResultRelInfo *resultRelInfo); +static void postgresEndForeignCopy(EState *estate, + ResultRelInfo *resultRelInfo); +static void postgresExecForeignCopy(ResultRelInfo *resultRelInfo, + TupleTableSlot **slots, + int nslots); static int postgresIsForeignRelUpdatable(Relation rel); static bool postgresPlanDirectModify(PlannerInfo *root, ModifyTable *plan, @@ -535,6 +544,9 @@ postgres_fdw_handler(PG_FUNCTION_ARGS) routine->EndForeignModify = postgresEndForeignModify; routine->BeginForeignInsert = postgresBeginForeignInsert; routine->EndForeignInsert = postgresEndForeignInsert; + routine->BeginForeignCopy = postgresBeginForeignCopy; + routine->EndForeignCopy = postgresEndForeignCopy; + routine->ExecForeignCopy = postgresExecForeignCopy; routine->IsForeignRelUpdatable = postgresIsForeignRelUpdatable; routine->PlanDirectModify = postgresPlanDirectModify; routine->BeginDirectModify = postgresBeginDirectModify; @@ -2052,6 +2064,131 @@ postgresEndForeignInsert(EState *estate, finish_foreign_modify(fmstate); } +static PgFdwModifyState *copy_fmstate = NULL; + +static void +pgfdw_copy_dest_cb(void *buf, int len) +{ + PGconn *conn = copy_fmstate->conn; + + if (PQputCopyData(conn, (char *) buf, len) <= 0) + pgfdw_report_error(ERROR, NULL, conn, false, copy_fmstate->query); +} + +/* + * + * postgresBeginForeignCopy + * Begin an COPY operation on a foreign table + */ +static void +postgresBeginForeignCopy(ModifyTableState *mtstate, + ResultRelInfo *resultRelInfo) +{ + PgFdwModifyState *fmstate; + StringInfoData sql; + RangeTblEntry *rte; + Relation rel = resultRelInfo->ri_RelationDesc; + + rte = exec_rt_fetch(resultRelInfo->ri_RangeTableIndex, mtstate->ps.state); + initStringInfo(&sql); + deparseCopyFromSql(&sql, rel); + + fmstate = create_foreign_modify(mtstate->ps.state, + rte, + resultRelInfo, + CMD_INSERT, + NULL, + sql.data, + NIL, + false, + NIL); + + fmstate->cstate = BeginCopyTo(NULL, NULL, RelationGetDescr(rel), NULL, + InvalidOid, NULL, false, pgfdw_copy_dest_cb, + NIL, NIL); + CopyToStart(fmstate->cstate); + resultRelInfo->ri_FdwState = fmstate; +} + +/* + * postgresEndForeignCopy + * Finish an COPY operation on a foreign table + */ +static void +postgresEndForeignCopy(EState *estate, ResultRelInfo *resultRelInfo) +{ + PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState; + + /* Check correct use of CopyIn FDW API. */ + Assert(fmstate->cstate != NULL); + CopyToFinish(fmstate->cstate); + pfree(fmstate->cstate); + fmstate->cstate = NULL; + finish_foreign_modify(fmstate); +} + +/* + * + * postgresExecForeignCopy + * Send a number of tuples to the foreign relation. + */ +static void +postgresExecForeignCopy(ResultRelInfo *resultRelInfo, + TupleTableSlot **slots, int nslots) +{ + PgFdwModifyState *fmstate = resultRelInfo->ri_FdwState; + PGresult *res; + PGconn *conn = fmstate->conn; + bool OK = false; + int i; + + /* Check correct use of CopyIn FDW API. */ + Assert(fmstate->cstate != NULL); + Assert(copy_fmstate == NULL); + + res = PQexec(conn, fmstate->query); + if (PQresultStatus(res) != PGRES_COPY_IN) + pgfdw_report_error(ERROR, res, conn, true, fmstate->query); + PQclear(res); + + PG_TRY(); + { + copy_fmstate = fmstate; + for (i = 0; i < nslots; i++) + CopyOneRowTo(fmstate->cstate, slots[i]); + + OK = true; + } + PG_FINALLY(); + { + copy_fmstate = NULL; /* Detect problems */ + + /* Finish COPY IN protocol. It is needed to do after successful copy or + * after an error. + */ + if (PQputCopyEnd(conn, OK ? NULL : _("canceled by server")) <= 0 || + PQflush(conn)) + pgfdw_report_error(ERROR, NULL, fmstate->conn, false, fmstate->query); + + /* After successfully sending an EOF signal, check command OK. */ + res = PQgetResult(conn); + if ((!OK && PQresultStatus(res) != PGRES_FATAL_ERROR) || + (OK && PQresultStatus(res) != PGRES_COMMAND_OK)) + pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query); + + PQclear(res); + /* Do this to ensure we've pumped libpq back to idle state */ + if (PQgetResult(conn) != NULL) + ereport(ERROR, + (errmsg("unexpected extra results during COPY of table: %s", + PQerrorMessage(conn)))); + + if (!OK) + PG_RE_THROW(); + } + PG_END_TRY(); +} + /* * postgresIsForeignRelUpdatable * Determine whether a foreign table supports INSERT, UPDATE and/or diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index eef410db39..8fc5ff018f 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -162,6 +162,7 @@ extern void deparseInsertSql(StringInfo buf, RangeTblEntry *rte, List *targetAttrs, bool doNothing, List *withCheckOptionList, List *returningList, List **retrieved_attrs); +extern void deparseCopyFromSql(StringInfo buf, Relation rel); extern void deparseUpdateSql(StringInfo buf, RangeTblEntry *rte, Index rtindex, Relation rel, List *targetAttrs, diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 7581c5417b..22dcd12f02 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -2212,6 +2212,23 @@ alter table loc2 drop constraint loc2_f1positive; delete from rem2; +create table foo (a int) partition by list (a); +create table foo1 (like foo); +create foreign table ffoo1 partition of foo for values in (1) + server loopback options (table_name 'foo1'); +create table foo2 (like foo); +create foreign table ffoo2 partition of foo for values in (2) + server loopback options (table_name 'foo2'); +create function print_new_row() returns trigger language plpgsql as $$ + begin raise notice '%', new; return new; end; $$; +create trigger ffoo1_br_trig before insert on ffoo1 + for each row execute function print_new_row(); + +copy foo from stdin; +1 +2 +\. + -- Test local triggers create trigger trig_stmt_before before insert on rem2 for each statement execute procedure trigger_func(); @@ -2312,6 +2329,34 @@ drop trigger loc2_trig_row_before_insert on loc2; delete from rem2; +alter table loc2 drop column f1; +alter table loc2 drop column f2; +copy rem2 from stdin; +1 foo +2 bar +\. + +alter table loc2 add column f1 int; +alter table loc2 add column f2 int; +select * from rem2; + +-- dropped columns locally and on the foreign server +alter table rem2 drop column f1; +alter table rem2 drop column f2; +copy rem2 from stdin; + + +\. +select * from rem2; + +alter table loc2 drop column f1; +alter table loc2 drop column f2; +copy rem2 from stdin; + + +\. +select * from rem2; + -- test COPY FROM with foreign table created in the same transaction create table loc3 (f1 int, f2 text); begin; diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index 9c9293414c..a9a7402440 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -796,6 +796,79 @@ EndForeignInsert(EState *estate, <para> <programlisting> +void +BeginForeignCopy(ModifyTableState *mtstate, + ResultRelInfo *rinfo); +</programlisting> + + Begin executing an copy operation on a foreign table. This routine is + called right before the first call of <function>ExecForeignCopy</function> + routine for the foreign table. It should perform any initialization needed + prior to the actual COPY FROM operation. + Subsequently, <function>ExecForeignCopy</function> will be called for + a bulk of tuples to be copied into the foreign table. + </para> + + <para> + <literal>mtstate</literal> is the overall state of the + <structname>ModifyTable</structname> plan node being executed; global data about + the plan and execution state is available via this structure. + <literal>rinfo</literal> is the <structname>ResultRelInfo</structname> struct describing + the target foreign table. (The <structfield>ri_FdwState</structfield> field of + <structname>ResultRelInfo</structname> is available for the FDW to store any + private state it needs for this operation.) + </para> + + <para> + When this is called by a <command>COPY FROM</command> command, the + plan-related global data in <literal>mtstate</literal> is not provided. + </para> + + <para> + If the <function>BeginForeignCopy</function> pointer is set to + <literal>NULL</literal>, no action is taken for the initialization. + </para> + + <para> +<programlisting> +void +EndForeignCopy(EState *estate, + ResultRelInfo *rinfo); +</programlisting> + + End the copy operation and release resources. It is normally not important + to release palloc'd memory, but for example open files and connections + to remote servers should be cleaned up. + </para> + + <para> + If the <function>EndForeignCopy</function> pointer is set to + <literal>NULL</literal>, no action is taken for the termination. + </para> + + <para> +<programlisting> +void +ExecForeignCopy(ResultRelInfo *rinfo, + TupleTableSlot **slots, + int nslots); +</programlisting> + + Copy a bulk of tuples into the foreign table. + <literal>rinfo</literal> is the <structname>ResultRelInfo</structname> struct describing + the target foreign table. + <literal>slots</literal> contains the tuples to be inserted; it will match the + row-type definition of the foreign table. + <literal>nslots</literal> is a number of tuples in the <literal>slots</literal> + </para> + + <para> + If the <function>ExecForeignCopy</function> pointer is set to + <literal>NULL</literal>, the <function>ExecForeignInsert</function> routine will be used to run COPY on the foreign table. + </para> + + <para> +<programlisting> int IsForeignRelUpdatable(Relation rel); </programlisting> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index b6143b8bf2..32cff00762 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -303,8 +303,8 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, { CopyToState cstate; - cstate = BeginCopyTo(pstate, rel, query, relid, - stmt->filename, stmt->is_program, + cstate = BeginCopyTo(pstate, rel, NULL, query, relid, + stmt->filename, stmt->is_program, NULL, stmt->attlist, stmt->options); *processed = DoCopyTo(cstate); /* copy from database to file */ EndCopyTo(cstate); diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index 6d4f6cb80d..73fc838625 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -307,61 +307,63 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo, ResultRelInfo *resultRelInfo = buffer->resultRelInfo; TupleTableSlot **slots = buffer->slots; - /* - * Print error context information correctly, if one of the operations - * below fail. - */ - cstate->line_buf_valid = false; - save_cur_lineno = cstate->cur_lineno; - - /* - * table_multi_insert may leak memory, so switch to short-lived memory - * context before calling it. - */ - oldcontext = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate)); - table_multi_insert(resultRelInfo->ri_RelationDesc, - slots, - nused, - mycid, - ti_options, - buffer->bistate); - MemoryContextSwitchTo(oldcontext); - - for (i = 0; i < nused; i++) + if (resultRelInfo->ri_RelationDesc->rd_rel->relkind == RELKIND_FOREIGN_TABLE) + { + /* Flush into foreign table or partition */ + resultRelInfo->ri_FdwRoutine->ExecForeignCopy(resultRelInfo, + slots, + nused); + } + else { /* - * If there are any indexes, update them for all the inserted tuples, - * and run AFTER ROW INSERT triggers. + * table_multi_insert may leak memory, so switch to short-lived memory + * context before calling it. */ - if (resultRelInfo->ri_NumIndices > 0) - { - List *recheckIndexes; - - cstate->cur_lineno = buffer->linenos[i]; - recheckIndexes = - ExecInsertIndexTuples(resultRelInfo, - buffer->slots[i], estate, false, NULL, - NIL); - ExecARInsertTriggers(estate, resultRelInfo, - slots[i], recheckIndexes, - cstate->transition_capture); - list_free(recheckIndexes); - } + oldcontext = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate)); + table_multi_insert(resultRelInfo->ri_RelationDesc, + slots, + nused, + mycid, + ti_options, + buffer->bistate); + MemoryContextSwitchTo(oldcontext); - /* - * There's no indexes, but see if we need to run AFTER ROW INSERT - * triggers anyway. - */ - else if (resultRelInfo->ri_TrigDesc != NULL && - (resultRelInfo->ri_TrigDesc->trig_insert_after_row || - resultRelInfo->ri_TrigDesc->trig_insert_new_table)) + for (i = 0; i < nused; i++) { - cstate->cur_lineno = buffer->linenos[i]; - ExecARInsertTriggers(estate, resultRelInfo, - slots[i], NIL, cstate->transition_capture); - } + /* + * If there are any indexes, update them for all the inserted tuples, + * and run AFTER ROW INSERT triggers. + */ + if (resultRelInfo->ri_NumIndices > 0) + { + List *recheckIndexes; + + cstate->cur_lineno = buffer->linenos[i]; + recheckIndexes = + ExecInsertIndexTuples(resultRelInfo, buffer->slots[i], + estate, false, NULL, NIL); + ExecARInsertTriggers(estate, resultRelInfo, + slots[i], recheckIndexes, + cstate->transition_capture); + list_free(recheckIndexes); + } + + /* + * There's no indexes, but see if we need to run AFTER ROW INSERT + * triggers anyway. + */ + else if (resultRelInfo->ri_TrigDesc != NULL && + (resultRelInfo->ri_TrigDesc->trig_insert_after_row || + resultRelInfo->ri_TrigDesc->trig_insert_new_table)) + { + cstate->cur_lineno = buffer->linenos[i]; + ExecARInsertTriggers(estate, resultRelInfo, + slots[i], NIL, cstate->transition_capture); + } - ExecClearTuple(slots[i]); + ExecClearTuple(slots[i]); + } } /* Mark that all slots are free */ @@ -666,8 +668,11 @@ CopyFrom(CopyFromState cstate) * checked by calling ExecSetRelationUsesMultiInsert(). It does not matter * whether partitions have any volatile default expressions as we use the * defaults from the target of the COPY command. + * Also, the COPY command requires a non-zero input list of attributes. + * Therefore, the length of the attribute list is checked here. */ if (!cstate->volatile_defexprs && + list_length(cstate->attnumlist) > 0 && !contain_volatile_functions(cstate->whereClause)) target_resultRelInfo->ri_usesMultiInsert = ExecSetRelationUsesMultiInsert(target_resultRelInfo, NULL); @@ -691,10 +696,18 @@ CopyFrom(CopyFromState cstate) * Init copying process into foreign table. Initialization of copying into * foreign partitions will be done later. */ - if (resultRelInfo->ri_FdwRoutine != NULL && - resultRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL) - resultRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate, - resultRelInfo); + if (target_resultRelInfo->ri_FdwRoutine != NULL) + { + if (target_resultRelInfo->ri_usesMultiInsert) + { + Assert(target_resultRelInfo->ri_FdwRoutine->BeginForeignCopy != NULL); + target_resultRelInfo->ri_FdwRoutine->BeginForeignCopy(mtstate, + resultRelInfo); + } + else if (target_resultRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL) + target_resultRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate, + resultRelInfo); + } /* Prepare to catch AFTER triggers. */ AfterTriggerBeginQuery(); @@ -1072,10 +1085,16 @@ CopyFrom(CopyFromState cstate) ExecResetTupleTable(estate->es_tupleTable, false); /* Allow the FDW to shut down */ - if (target_resultRelInfo->ri_FdwRoutine != NULL && - target_resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL) - target_resultRelInfo->ri_FdwRoutine->EndForeignInsert(estate, - target_resultRelInfo); + if (target_resultRelInfo->ri_FdwRoutine != NULL) + { + if (target_resultRelInfo->ri_usesMultiInsert && + target_resultRelInfo->ri_FdwRoutine->EndForeignCopy != NULL) + target_resultRelInfo->ri_FdwRoutine->EndForeignCopy(estate, + target_resultRelInfo); + else if (target_resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL) + target_resultRelInfo->ri_FdwRoutine->EndForeignInsert(estate, + target_resultRelInfo); + } /* Tear down the multi-insert buffer data */ CopyMultiInsertInfoCleanup(&multiInsertInfo); diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c index c7e5f04446..b1d50b01cc 100644 --- a/src/backend/commands/copyto.c +++ b/src/backend/commands/copyto.c @@ -50,6 +50,7 @@ typedef enum CopyDest COPY_FILE, /* to file (or a piped program) */ COPY_OLD_FE, /* to frontend (2.0 protocol) */ COPY_NEW_FE, /* to frontend (3.0 protocol) */ + COPY_CALLBACK /* to callback function */ } CopyDest; /* @@ -80,11 +81,14 @@ typedef struct CopyToStateData /* parameters from the COPY command */ Relation rel; /* relation to copy to */ + TupleDesc tupDesc; /* COPY TO will be used for manual tuple copying + * into the destination */ QueryDesc *queryDesc; /* executable query to copy from */ List *attnumlist; /* integer list of attnums to copy */ char *filename; /* filename, or NULL for STDOUT */ bool is_program; /* is 'filename' a program to popen? */ + copy_data_dest_cb data_dest_cb; /* function for writing data */ CopyFormatOptions opts; Node *whereClause; /* WHERE condition (or NULL) */ @@ -114,7 +118,6 @@ static const char BinarySignature[11] = "PGCOPY\n\377\r\n\0"; static void EndCopy(CopyToState cstate); static void ClosePipeToProgram(CopyToState cstate); static uint64 CopyTo(CopyToState cstate); -static void CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot); static void CopyAttributeOutText(CopyToState cstate, char *string); static void CopyAttributeOutCSV(CopyToState cstate, char *string, bool use_quote, bool single_attr); @@ -286,6 +289,14 @@ CopySendEndOfRow(CopyToState cstate) /* Dump the accumulated row as one CopyData message */ (void) pq_putmessage('d', fe_msgbuf->data, fe_msgbuf->len); break; + case COPY_CALLBACK: + Assert(!cstate->binary); +#ifndef WIN32 + CopySendChar(cstate, '\n'); +#else + CopySendString(cstate, "\r\n"); +#endif + cstate->data_dest_cb(fe_msgbuf->data, fe_msgbuf->len); } resetStringInfo(fe_msgbuf); @@ -373,19 +384,24 @@ EndCopy(CopyToState cstate) CopyToState BeginCopyTo(ParseState *pstate, Relation rel, + TupleDesc srcTupDesc, RawStmt *raw_query, Oid queryRelId, const char *filename, bool is_program, + copy_data_dest_cb data_dest_cb, List *attnamelist, List *options) { CopyToState cstate; - bool pipe = (filename == NULL); + bool pipe = (filename == NULL) && (data_dest_cb == NULL); TupleDesc tupDesc; int num_phys_attrs; MemoryContext oldcontext; + /* Impossible to mix CopyTo modes */ + Assert(rel == NULL || srcTupDesc == NULL); + if (rel != NULL && rel->rd_rel->relkind != RELKIND_RELATION) { if (rel->rd_rel->relkind == RELKIND_VIEW) @@ -450,6 +466,11 @@ BeginCopyTo(ParseState *pstate, tupDesc = RelationGetDescr(cstate->rel); } + else if (srcTupDesc) + { + Assert(!raw_query && !is_from); + tupDesc = cstate->tupDesc = srcTupDesc; + } else { List *rewritten; @@ -695,6 +716,11 @@ BeginCopyTo(ParseState *pstate, if (whereToSendOutput != DestRemote) cstate->copy_file = stdout; } + else if (data_dest_cb) + { + cstate->copy_dest = COPY_CALLBACK; + cstate->data_dest_cb = data_dest_cb; + } else { cstate->filename = pstrdup(filename); @@ -772,7 +798,7 @@ BeginCopyTo(ParseState *pstate, uint64 DoCopyTo(CopyToState cstate) { - bool pipe = (cstate->filename == NULL); + bool pipe = (cstate->filename == NULL) && (cstate->data_dest_cb == NULL); bool fe_copy = (pipe && whereToSendOutput == DestRemote); uint64 processed; @@ -781,7 +807,9 @@ DoCopyTo(CopyToState cstate) if (fe_copy) SendCopyBegin(cstate); + CopyToStart(cstate); processed = CopyTo(cstate); + CopyToFinish(cstate); if (fe_copy) SendCopyEnd(cstate); @@ -821,18 +849,22 @@ EndCopyTo(CopyToState cstate) } /* - * Copy from relation or query TO file. + * Start COPY TO operation. + * Separated to the routine to prevent duplicate operations in the case of + * manual mode, where tuples are copied to the destination one by one, by call of + * the CopyOneRowTo() routine. */ -static uint64 -CopyTo(CopyToState cstate) +void +CopyToStart(CopyToState cstate) { TupleDesc tupDesc; int num_phys_attrs; ListCell *cur; - uint64 processed; if (cstate->rel) tupDesc = RelationGetDescr(cstate->rel); + else if (cstate->tupDesc) + tupDesc = cstate->tupDesc; else tupDesc = cstate->queryDesc->tupDesc; num_phys_attrs = tupDesc->natts; @@ -919,6 +951,32 @@ CopyTo(CopyToState cstate) CopySendEndOfRow(cstate); } } +} + +/* + * Finish COPY TO operation. + */ +void +CopyToFinish(CopyToState cstate) +{ + if (cstate->opts.binary) + { + /* Generate trailer for a binary copy */ + CopySendInt16(cstate, -1); + /* Need to flush out the trailer */ + CopySendEndOfRow(cstate); + } + + MemoryContextDelete(cstate->rowcontext); +} + +/* + * Copy from relation or query TO file. + */ +static uint64 +CopyTo(CopyToState cstate) +{ + uint64 processed; if (cstate->rel) { @@ -951,23 +1009,13 @@ CopyTo(CopyToState cstate) processed = ((DR_copy *) cstate->queryDesc->dest)->processed; } - if (cstate->opts.binary) - { - /* Generate trailer for a binary copy */ - CopySendInt16(cstate, -1); - /* Need to flush out the trailer */ - CopySendEndOfRow(cstate); - } - - MemoryContextDelete(cstate->rowcontext); - return processed; } /* * Emit one row during CopyTo(). */ -static void +void CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot) { bool need_delim = false; diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 9809c03a8e..a21d4d2fc1 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1294,8 +1294,12 @@ ExecSetRelationUsesMultiInsert(const ResultRelInfo *rri, rri->ri_TrigDesc->trig_insert_new_table) return false; - /* Foreign tables don't support multi-inserts. */ - if (rri->ri_FdwRoutine != NULL) + if (rri->ri_FdwRoutine != NULL && + rri->ri_FdwRoutine->ExecForeignCopy == NULL) + /* + * Foreign tables don't support multi-inserts, unless their FDW + * provides the necessary COPY interface. + */ return false; /* OK, caller can use multi-insert on this relation. */ diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 5a201dfbfa..56ec9bbf41 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -997,9 +997,16 @@ ExecInitRoutingInfo(ModifyTableState *mtstate, * If the partition is a foreign table, let the FDW init itself for * routing tuples to the partition. */ - if (partRelInfo->ri_FdwRoutine != NULL && - partRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL) - partRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate, partRelInfo); + if (partRelInfo->ri_FdwRoutine != NULL) + { + if (partRelInfo->ri_usesMultiInsert) + { + Assert(partRelInfo->ri_FdwRoutine->BeginForeignCopy != NULL); + partRelInfo->ri_FdwRoutine->BeginForeignCopy(mtstate, partRelInfo); + } + else if (partRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL) + partRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate, partRelInfo); + } partRelInfo->ri_CopyMultiInsertBuffer = NULL; @@ -1200,10 +1207,16 @@ ExecCleanupTupleRouting(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo = proute->partitions[i]; /* Allow any FDWs to shut down */ - if (resultRelInfo->ri_FdwRoutine != NULL && - resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL) - resultRelInfo->ri_FdwRoutine->EndForeignInsert(mtstate->ps.state, - resultRelInfo); + if (resultRelInfo->ri_FdwRoutine != NULL) + { + if (resultRelInfo->ri_usesMultiInsert && + resultRelInfo->ri_FdwRoutine->EndForeignCopy != NULL) + resultRelInfo->ri_FdwRoutine->EndForeignCopy(mtstate->ps.state, + resultRelInfo); + else if (resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL) + resultRelInfo->ri_FdwRoutine->EndForeignInsert(mtstate->ps.state, + resultRelInfo); + } /* * Check if this result rel is one belonging to the node's subplans, diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h index 127a3c61e2..01bb3e8ad4 100644 --- a/src/include/commands/copy.h +++ b/src/include/commands/copy.h @@ -55,6 +55,7 @@ typedef struct CopyFromStateData *CopyFromState; typedef struct CopyToStateData *CopyToState; typedef int (*copy_data_source_cb) (void *outbuf, int minread, int maxread); +typedef void (*copy_data_dest_cb) (void *outbuf, int len); extern void DoCopy(ParseState *state, const CopyStmt *stmt, int stmt_location, int stmt_len, @@ -78,12 +79,17 @@ extern DestReceiver *CreateCopyDestReceiver(void); /* * internal prototypes */ -extern CopyToState BeginCopyTo(ParseState *pstate, Relation rel, RawStmt *query, +extern CopyToState BeginCopyTo(ParseState *pstate, Relation rel, + TupleDesc tupDesc, RawStmt *query, Oid queryRelId, const char *filename, bool is_program, + copy_data_dest_cb data_dest_cb, List *attnamelist, List *options); extern void EndCopyTo(CopyToState cstate); extern uint64 DoCopyTo(CopyToState cstate); extern List *CopyGetAttnums(TupleDesc tupDesc, Relation rel, List *attnamelist); +extern void CopyToStart(CopyToState cstate); +extern void CopyToFinish(CopyToState cstate); +extern void CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot); #endif /* COPY_H */ diff --git a/src/include/foreign/fdwapi.h b/src/include/foreign/fdwapi.h index 95556dfb15..52b213f5aa 100644 --- a/src/include/foreign/fdwapi.h +++ b/src/include/foreign/fdwapi.h @@ -104,6 +104,16 @@ typedef void (*BeginForeignInsert_function) (ModifyTableState *mtstate, typedef void (*EndForeignInsert_function) (EState *estate, ResultRelInfo *rinfo); +typedef void (*BeginForeignCopy_function) (ModifyTableState *mtstate, + ResultRelInfo *rinfo); + +typedef void (*ExecForeignCopy_function) (ResultRelInfo *rinfo, + TupleTableSlot **slots, + int nslots); + +typedef void (*EndForeignCopy_function) (EState *estate, + ResultRelInfo *rinfo); + typedef int (*IsForeignRelUpdatable_function) (Relation rel); typedef bool (*PlanDirectModify_function) (PlannerInfo *root, @@ -220,6 +230,11 @@ typedef struct FdwRoutine IterateDirectModify_function IterateDirectModify; EndDirectModify_function EndDirectModify; + /* Support functions for COPY into foreign tables */ + BeginForeignCopy_function BeginForeignCopy; + ExecForeignCopy_function ExecForeignCopy; + EndForeignCopy_function EndForeignCopy; + /* Functions for SELECT FOR UPDATE/SHARE row locking */ GetForeignRowMarkType_function GetForeignRowMarkType; RefetchForeignRow_function RefetchForeignRow; -- 2.25.1 --------------864DF32B410D06E611DA27E2-- ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Speeding up ruleutils' name de-duplication code, redux @ 2024-09-10 15:06 Tom Lane <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Tom Lane @ 2024-09-10 15:06 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: [email protected] David Rowley <[email protected]> writes: > On Tue, 30 Jul 2024 at 10:14, Tom Lane <[email protected]> wrote: >> On my development machine, it takes over 14 minutes to pg_upgrade >> this, and it turns out that that time is largely spent in column >> name de-duplication while deparsing the CHECK constraints. The >> attached patch reduces that to about 3m45s. > I looked at the patch and tried it out. Thanks for looking! > This gives me what I'd expect to see. I wanted to ensure the point > where you're switching to the hashing method was about the right > place. It seems to be, at least for my test. Yeah, I was just going by gut feel there. It's good to have some numbers showing it's not a totally silly choice. > Perhaps you don't think it's worth the additional complexity, but I > see that in both locations you're calling build_colinfo_names_hash(), > it's done just after a call to expand_colnames_array_to(). I wondered > if it was worthwhile unifying both of those functions maybe with a new > name so that you don't need to loop over the always NULL element of > the colnames[] array when building the hash table. This is likely > quite a small overhead compared to the quadratic search you've > removed, so it might not move the needle any. I just wanted to point > it out as I've little else I can find to comment on. Hmm, but there are quite a few expand_colnames_array_to calls that are not associated with build_colinfo_names_hash. On the whole it feels like those are separate concerns that are better kept separate. We could accomplish what you suggest by re-ordering the calls so that we build the hash table before enlarging the array. 0001 attached is the same as before (modulo line number changes from being rebased up to HEAD) and then 0002 implements this idea on top. On the whole though I find 0002 fairly ugly and would prefer to stick to 0001. I really doubt that scanning any newly-created column positions is going to take long enough to justify intertwining things like this. regards, tom lane Attachments: [text/x-diff] v2-0001-speed-up-column-name-deduplication.patch (9.9K, ../../[email protected]/2-v2-0001-speed-up-column-name-deduplication.patch) download | inline diff: diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index ee1b7f3dc9..badbf111ee 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -224,6 +224,10 @@ typedef struct * of aliases to columns of the right input. Thus, positions in the printable * column alias list are not necessarily one-for-one with varattnos of the * JOIN, so we need a separate new_colnames[] array for printing purposes. + * + * Finally, when dealing with wide tables we risk O(N^2) costs in assigning + * non-duplicate column names. We ameliorate that by using a hash table that + * holds all the strings appearing in colnames, new_colnames, and parentUsing. */ typedef struct { @@ -291,6 +295,15 @@ typedef struct int *leftattnos; /* left-child varattnos of join cols, or 0 */ int *rightattnos; /* right-child varattnos of join cols, or 0 */ List *usingNames; /* names assigned to merged columns */ + + /* + * Hash table holding copies of all the strings appearing in this struct's + * colnames, new_colnames, and parentUsing. We use a hash table only for + * sufficiently wide relations, and only during the colname-assignment + * functions set_relation_column_names and set_join_column_names; + * otherwise, names_hash is NULL. + */ + HTAB *names_hash; /* entries are just strings */ } deparse_columns; /* This macro is analogous to rt_fetch(), but for deparse_columns structs */ @@ -376,6 +389,9 @@ static bool colname_is_unique(const char *colname, deparse_namespace *dpns, static char *make_colname_unique(char *colname, deparse_namespace *dpns, deparse_columns *colinfo); static void expand_colnames_array_to(deparse_columns *colinfo, int n); +static void build_colinfo_names_hash(deparse_columns *colinfo); +static void add_to_names_hash(deparse_columns *colinfo, const char *name); +static void destroy_colinfo_names_hash(deparse_columns *colinfo); static void identify_join_columns(JoinExpr *j, RangeTblEntry *jrte, deparse_columns *colinfo); static char *get_rtable_name(int rtindex, deparse_context *context); @@ -4133,6 +4149,10 @@ has_dangerous_join_using(deparse_namespace *dpns, Node *jtnode) * * parentUsing is a list of all USING aliases assigned in parent joins of * the current jointree node. (The passed-in list must not be modified.) + * + * Note that we do not use per-deparse_columns hash tables in this function. + * The number of names that need to be assigned should be small enough that + * we don't need to trouble with that. */ static void set_using_names(deparse_namespace *dpns, Node *jtnode, List *parentUsing) @@ -4408,6 +4428,9 @@ set_relation_column_names(deparse_namespace *dpns, RangeTblEntry *rte, colinfo->new_colnames = (char **) palloc(ncolumns * sizeof(char *)); colinfo->is_new_col = (bool *) palloc(ncolumns * sizeof(bool)); + /* If the RTE is wide enough, use a hash table to avoid O(N^2) costs */ + build_colinfo_names_hash(colinfo); + /* * Scan the columns, select a unique alias for each one, and store it in * colinfo->colnames and colinfo->new_colnames. The former array has NULL @@ -4443,6 +4466,7 @@ set_relation_column_names(deparse_namespace *dpns, RangeTblEntry *rte, colname = make_colname_unique(colname, dpns, colinfo); colinfo->colnames[i] = colname; + add_to_names_hash(colinfo, colname); } /* Put names of non-dropped columns in new_colnames[] too */ @@ -4456,6 +4480,9 @@ set_relation_column_names(deparse_namespace *dpns, RangeTblEntry *rte, changed_any = true; } + /* We're now done needing the colinfo's names_hash */ + destroy_colinfo_names_hash(colinfo); + /* * Set correct length for new_colnames[] array. (Note: if columns have * been added, colinfo->num_cols includes them, which is not really quite @@ -4526,6 +4553,9 @@ set_join_column_names(deparse_namespace *dpns, RangeTblEntry *rte, expand_colnames_array_to(colinfo, noldcolumns); Assert(colinfo->num_cols == noldcolumns); + /* If the RTE is wide enough, use a hash table to avoid O(N^2) costs */ + build_colinfo_names_hash(colinfo); + /* * Scan the join output columns, select an alias for each one, and store * it in colinfo->colnames. If there are USING columns, set_using_names() @@ -4563,6 +4593,7 @@ set_join_column_names(deparse_namespace *dpns, RangeTblEntry *rte, if (rte->alias == NULL) { colinfo->colnames[i] = real_colname; + add_to_names_hash(colinfo, real_colname); continue; } @@ -4579,6 +4610,7 @@ set_join_column_names(deparse_namespace *dpns, RangeTblEntry *rte, colname = make_colname_unique(colname, dpns, colinfo); colinfo->colnames[i] = colname; + add_to_names_hash(colinfo, colname); } /* Remember if any assigned aliases differ from "real" name */ @@ -4677,6 +4709,7 @@ set_join_column_names(deparse_namespace *dpns, RangeTblEntry *rte, } else colinfo->new_colnames[j] = child_colname; + add_to_names_hash(colinfo, colinfo->new_colnames[j]); } colinfo->is_new_col[j] = leftcolinfo->is_new_col[jc]; @@ -4726,6 +4759,7 @@ set_join_column_names(deparse_namespace *dpns, RangeTblEntry *rte, } else colinfo->new_colnames[j] = child_colname; + add_to_names_hash(colinfo, colinfo->new_colnames[j]); } colinfo->is_new_col[j] = rightcolinfo->is_new_col[jc]; @@ -4740,6 +4774,9 @@ set_join_column_names(deparse_namespace *dpns, RangeTblEntry *rte, Assert(j == nnewcolumns); #endif + /* We're now done needing the colinfo's names_hash */ + destroy_colinfo_names_hash(colinfo); + /* * For a named join, print column aliases if we changed any from the child * names. Unnamed joins cannot print aliases. @@ -4762,38 +4799,59 @@ colname_is_unique(const char *colname, deparse_namespace *dpns, int i; ListCell *lc; - /* Check against already-assigned column aliases within RTE */ - for (i = 0; i < colinfo->num_cols; i++) - { - char *oldname = colinfo->colnames[i]; - - if (oldname && strcmp(oldname, colname) == 0) - return false; - } - /* - * If we're building a new_colnames array, check that too (this will be - * partially but not completely redundant with the previous checks) + * If we have a hash table, consult that instead of linearly scanning the + * colinfo's strings. */ - for (i = 0; i < colinfo->num_new_cols; i++) + if (colinfo->names_hash) { - char *oldname = colinfo->new_colnames[i]; - - if (oldname && strcmp(oldname, colname) == 0) + if (hash_search(colinfo->names_hash, + colname, + HASH_FIND, + NULL) != NULL) return false; } - - /* Also check against USING-column names that must be globally unique */ - foreach(lc, dpns->using_names) + else { - char *oldname = (char *) lfirst(lc); + /* Check against already-assigned column aliases within RTE */ + for (i = 0; i < colinfo->num_cols; i++) + { + char *oldname = colinfo->colnames[i]; - if (strcmp(oldname, colname) == 0) - return false; + if (oldname && strcmp(oldname, colname) == 0) + return false; + } + + /* + * If we're building a new_colnames array, check that too (this will + * be partially but not completely redundant with the previous checks) + */ + for (i = 0; i < colinfo->num_new_cols; i++) + { + char *oldname = colinfo->new_colnames[i]; + + if (oldname && strcmp(oldname, colname) == 0) + return false; + } + + /* + * Also check against names already assigned for parent-join USING + * cols + */ + foreach(lc, colinfo->parentUsing) + { + char *oldname = (char *) lfirst(lc); + + if (strcmp(oldname, colname) == 0) + return false; + } } - /* Also check against names already assigned for parent-join USING cols */ - foreach(lc, colinfo->parentUsing) + /* + * Also check against USING-column names that must be globally unique. + * These are not hashed, but there should be few of them. + */ + foreach(lc, dpns->using_names) { char *oldname = (char *) lfirst(lc); @@ -4861,6 +4919,90 @@ expand_colnames_array_to(deparse_columns *colinfo, int n) } } +/* + * build_colinfo_names_hash: optionally construct a hash table for colinfo + */ +static void +build_colinfo_names_hash(deparse_columns *colinfo) +{ + HASHCTL hash_ctl; + int i; + ListCell *lc; + + /* + * Use a hash table only for RTEs with at least 32 columns. (The cutoff + * is somewhat arbitrary, but let's choose it so that this code does get + * exercised in the regression tests.) + */ + if (colinfo->num_cols < 32) + return; + + /* + * Set up the hash table. The entries are just strings with no other + * payload. + */ + hash_ctl.keysize = NAMEDATALEN; + hash_ctl.entrysize = NAMEDATALEN; + hash_ctl.hcxt = CurrentMemoryContext; + colinfo->names_hash = hash_create("deparse_columns names", + colinfo->num_cols + colinfo->num_new_cols, + &hash_ctl, + HASH_ELEM | HASH_STRINGS | HASH_CONTEXT); + + /* + * Preload the hash table with any names already present (these would have + * come from set_using_names). + */ + for (i = 0; i < colinfo->num_cols; i++) + { + char *oldname = colinfo->colnames[i]; + + if (oldname) + add_to_names_hash(colinfo, oldname); + } + + for (i = 0; i < colinfo->num_new_cols; i++) + { + char *oldname = colinfo->new_colnames[i]; + + if (oldname) + add_to_names_hash(colinfo, oldname); + } + + foreach(lc, colinfo->parentUsing) + { + char *oldname = (char *) lfirst(lc); + + add_to_names_hash(colinfo, oldname); + } +} + +/* + * add_to_names_hash: add a string to the names_hash, if we're using one + */ +static void +add_to_names_hash(deparse_columns *colinfo, const char *name) +{ + if (colinfo->names_hash) + (void) hash_search(colinfo->names_hash, + name, + HASH_ENTER, + NULL); +} + +/* + * destroy_colinfo_names_hash: destroy hash table when done with it + */ +static void +destroy_colinfo_names_hash(deparse_columns *colinfo) +{ + if (colinfo->names_hash) + { + hash_destroy(colinfo->names_hash); + colinfo->names_hash = NULL; + } +} + /* * identify_join_columns: figure out where columns of a join come from * [text/x-diff] v2-0002-avoid-useless-scanning.patch (3.1K, ../../[email protected]/3-v2-0002-avoid-useless-scanning.patch) download | inline diff: diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index badbf111ee..63b09805e0 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -4407,6 +4407,10 @@ set_relation_column_names(deparse_namespace *dpns, RangeTblEntry *rte, } } + /* If the RTE is wide enough, use a hash table to avoid O(N^2) costs */ + if (ncolumns >= 32) + build_colinfo_names_hash(colinfo); + /* * Ensure colinfo->colnames has a slot for each column. (It could be long * enough already, if we pushed down a name for the last column.) Note: @@ -4428,9 +4432,6 @@ set_relation_column_names(deparse_namespace *dpns, RangeTblEntry *rte, colinfo->new_colnames = (char **) palloc(ncolumns * sizeof(char *)); colinfo->is_new_col = (bool *) palloc(ncolumns * sizeof(bool)); - /* If the RTE is wide enough, use a hash table to avoid O(N^2) costs */ - build_colinfo_names_hash(colinfo); - /* * Scan the columns, select a unique alias for each one, and store it in * colinfo->colnames and colinfo->new_colnames. The former array has NULL @@ -4542,6 +4543,11 @@ set_join_column_names(deparse_namespace *dpns, RangeTblEntry *rte, leftcolinfo = deparse_columns_fetch(colinfo->leftrti, dpns); rightcolinfo = deparse_columns_fetch(colinfo->rightrti, dpns); + /* If the RTE is wide enough, use a hash table to avoid O(N^2) costs */ + noldcolumns = list_length(rte->eref->colnames); + if (noldcolumns >= 32) + build_colinfo_names_hash(colinfo); + /* * Ensure colinfo->colnames has a slot for each column. (It could be long * enough already, if we pushed down a name for the last column.) Note: @@ -4549,13 +4555,9 @@ set_join_column_names(deparse_namespace *dpns, RangeTblEntry *rte, * were when the query was parsed, but we'll deal with that below. We * only need entries in colnames for pre-existing columns. */ - noldcolumns = list_length(rte->eref->colnames); expand_colnames_array_to(colinfo, noldcolumns); Assert(colinfo->num_cols == noldcolumns); - /* If the RTE is wide enough, use a hash table to avoid O(N^2) costs */ - build_colinfo_names_hash(colinfo); - /* * Scan the join output columns, select an alias for each one, and store * it in colinfo->colnames. If there are USING columns, set_using_names() @@ -4920,7 +4922,10 @@ expand_colnames_array_to(deparse_columns *colinfo, int n) } /* - * build_colinfo_names_hash: optionally construct a hash table for colinfo + * build_colinfo_names_hash: construct a hash table for colinfo + * + * We use this only for sufficiently wide RTEs: currently, those with at + * least 32 columns. That check is made at the callers though. */ static void build_colinfo_names_hash(deparse_columns *colinfo) @@ -4929,14 +4934,6 @@ build_colinfo_names_hash(deparse_columns *colinfo) int i; ListCell *lc; - /* - * Use a hash table only for RTEs with at least 32 columns. (The cutoff - * is somewhat arbitrary, but let's choose it so that this code does get - * exercised in the regression tests.) - */ - if (colinfo->num_cols < 32) - return; - /* * Set up the hash table. The entries are just strings with no other * payload. ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Speeding up ruleutils' name de-duplication code, redux @ 2024-09-10 20:33 David Rowley <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: David Rowley @ 2024-09-10 20:33 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: [email protected] On Wed, 11 Sept 2024 at 03:06, Tom Lane <[email protected]> wrote: > We could accomplish what you suggest by re-ordering the calls so that > we build the hash table before enlarging the array. 0001 attached > is the same as before (modulo line number changes from being rebased > up to HEAD) and then 0002 implements this idea on top. On the whole > though I find 0002 fairly ugly and would prefer to stick to 0001. > I really doubt that scanning any newly-created column positions is > going to take long enough to justify intertwining things like this. I'm fine with that. I did test the performance with and without v2-0002 and the performance is just a little too noisy to tell. Both runs I did with v2-0002, it was slower, so I agree it's not worth making the code uglier for. I've no more comments. Looks good. David ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Speeding up ruleutils' name de-duplication code, redux @ 2024-09-10 20:36 Tom Lane <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Tom Lane @ 2024-09-10 20:36 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: [email protected] David Rowley <[email protected]> writes: > On Wed, 11 Sept 2024 at 03:06, Tom Lane <[email protected]> wrote: >> We could accomplish what you suggest by re-ordering the calls so that >> we build the hash table before enlarging the array. 0001 attached >> is the same as before (modulo line number changes from being rebased >> up to HEAD) and then 0002 implements this idea on top. On the whole >> though I find 0002 fairly ugly and would prefer to stick to 0001. >> I really doubt that scanning any newly-created column positions is >> going to take long enough to justify intertwining things like this. > I'm fine with that. I did test the performance with and without > v2-0002 and the performance is just a little too noisy to tell. Both > runs I did with v2-0002, it was slower, so I agree it's not worth > making the code uglier for. > I've no more comments. Looks good. Thanks for the review! I'll go push just 0001. regards, tom lane ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-09-10 20:36 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-12-14 08:37 [PATCH 2/2] Fast COPY FROM into the foreign or sharded table. Andrey Lepikhov <[email protected]> 2024-09-10 15:06 Re: Speeding up ruleutils' name de-duplication code, redux Tom Lane <[email protected]> 2024-09-10 20:33 ` Re: Speeding up ruleutils' name de-duplication code, redux David Rowley <[email protected]> 2024-09-10 20:36 ` Re: Speeding up ruleutils' name de-duplication code, redux Tom Lane <[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