public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 1/3] Add bulk insert for foreign tables 12+ messages / 4 participants [nested] [flat]
* [PATCH 1/3] Add bulk insert for foreign tables @ 2021-01-18 14:18 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Tomas Vondra @ 2021-01-18 14:18 UTC (permalink / raw) --- contrib/postgres_fdw/deparse.c | 43 ++- .../postgres_fdw/expected/postgres_fdw.out | 116 ++++++- contrib/postgres_fdw/option.c | 14 + contrib/postgres_fdw/postgres_fdw.c | 302 ++++++++++++++---- contrib/postgres_fdw/postgres_fdw.h | 5 +- contrib/postgres_fdw/sql/postgres_fdw.sql | 91 ++++++ doc/src/sgml/fdwhandler.sgml | 89 +++++- doc/src/sgml/postgres-fdw.sgml | 13 + src/backend/executor/execPartition.c | 12 + src/backend/executor/nodeModifyTable.c | 157 +++++++++ src/backend/nodes/list.c | 15 + src/include/foreign/fdwapi.h | 10 + src/include/nodes/execnodes.h | 6 + src/include/nodes/pg_list.h | 15 + 14 files changed, 822 insertions(+), 66 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 3cf7b4eb1e..2d38ab25cb 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -1711,7 +1711,7 @@ deparseInsertSql(StringInfo buf, RangeTblEntry *rte, Index rtindex, Relation rel, List *targetAttrs, bool doNothing, List *withCheckOptionList, List *returningList, - List **retrieved_attrs) + List **retrieved_attrs, int *values_end_len) { AttrNumber pindex; bool first; @@ -1754,6 +1754,7 @@ deparseInsertSql(StringInfo buf, RangeTblEntry *rte, } else appendStringInfoString(buf, " DEFAULT VALUES"); + *values_end_len = buf->len; if (doNothing) appendStringInfoString(buf, " ON CONFLICT DO NOTHING"); @@ -1763,6 +1764,46 @@ deparseInsertSql(StringInfo buf, RangeTblEntry *rte, withCheckOptionList, returningList, retrieved_attrs); } +/* + * rebuild remote INSERT statement + * + */ +void +rebuildInsertSql(StringInfo buf, char *orig_query, + int values_end_len, int num_cols, + int num_rows) +{ + int i, j; + int pindex; + bool first; + + /* Copy up to the end of the first record from the original query */ + appendBinaryStringInfo(buf, orig_query, values_end_len); + + /* Add records to VALUES clause */ + pindex = num_cols + 1; + for (i = 0; i < num_rows; i++) + { + appendStringInfoString(buf, ", ("); + + first = true; + for (j = 0; j < num_cols; j++) + { + if (!first) + appendStringInfoString(buf, ", "); + first = false; + + appendStringInfo(buf, "$%d", pindex); + pindex++; + } + + appendStringInfoChar(buf, ')'); + } + + /* Copy stuff after VALUES clause from the original query */ + appendStringInfoString(buf, orig_query + values_end_len); +} + /* * deparse remote UPDATE statement * diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 1cad311436..8c0fdb5a9a 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -8923,7 +8923,7 @@ DO $d$ END; $d$; ERROR: invalid option "password" -HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, requirepeer, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size +HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, requirepeer, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size, batch_size CONTEXT: SQL statement "ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')" PL/pgSQL function inline_code_block line 3 at EXECUTE -- If we add a password for our user mapping instead, we should get a different @@ -9112,3 +9112,117 @@ SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; loopback2 | t (1 row) +-- =================================================================== +-- batch insert +-- =================================================================== +BEGIN; +CREATE SERVER batch10 FOREIGN DATA WRAPPER postgres_fdw OPTIONS( batch_size '10' ); +SELECT count(*) +FROM pg_foreign_server +WHERE srvname = 'batch10' +AND srvoptions @> array['batch_size=10']; + count +------- + 1 +(1 row) + +ALTER SERVER batch10 OPTIONS( SET batch_size '20' ); +SELECT count(*) +FROM pg_foreign_server +WHERE srvname = 'batch10' +AND srvoptions @> array['batch_size=10']; + count +------- + 0 +(1 row) + +SELECT count(*) +FROM pg_foreign_server +WHERE srvname = 'batch10' +AND srvoptions @> array['batch_size=20']; + count +------- + 1 +(1 row) + +CREATE FOREIGN TABLE table30 ( x int ) SERVER batch10 OPTIONS ( batch_size '30' ); +SELECT COUNT(*) +FROM pg_foreign_table +WHERE ftrelid = 'table30'::regclass +AND ftoptions @> array['batch_size=30']; + count +------- + 1 +(1 row) + +ALTER FOREIGN TABLE table30 OPTIONS ( SET batch_size '40'); +SELECT COUNT(*) +FROM pg_foreign_table +WHERE ftrelid = 'table30'::regclass +AND ftoptions @> array['batch_size=30']; + count +------- + 0 +(1 row) + +SELECT COUNT(*) +FROM pg_foreign_table +WHERE ftrelid = 'table30'::regclass +AND ftoptions @> array['batch_size=40']; + count +------- + 1 +(1 row) + +ROLLBACK; +CREATE TABLE batch_table ( x int ); +CREATE FOREIGN TABLE ftable ( x int ) SERVER loopback OPTIONS ( table_name 'batch_table', batch_size '10' ); +INSERT INTO ftable SELECT * FROM generate_series(1, 10) i; +INSERT INTO ftable SELECT * FROM generate_series(11, 31) i; +INSERT INTO ftable VALUES (32); +INSERT INTO ftable VALUES (33), (34); +SELECT COUNT(*) FROM ftable; + count +------- + 34 +(1 row) + +TRUNCATE batch_table; +DROP FOREIGN TABLE ftable; +-- Disable batch insert +CREATE FOREIGN TABLE ftable ( x int ) SERVER loopback OPTIONS ( table_name 'batch_table', batch_size '1' ); +INSERT INTO ftable VALUES (1), (2); +SELECT COUNT(*) FROM ftable; + count +------- + 2 +(1 row) + +DROP FOREIGN TABLE ftable; +DROP TABLE batch_table; +-- Use partitioning +CREATE TABLE batch_table ( x int ) PARTITION BY HASH (x); +CREATE TABLE batch_table_p0 (LIKE batch_table); +CREATE FOREIGN TABLE batch_table_p0f + PARTITION OF batch_table + FOR VALUES WITH (MODULUS 3, REMAINDER 0) + SERVER loopback + OPTIONS (table_name 'batch_table_p0', batch_size '10'); +CREATE TABLE batch_table_p1 (LIKE batch_table); +CREATE FOREIGN TABLE batch_table_p1f + PARTITION OF batch_table + FOR VALUES WITH (MODULUS 3, REMAINDER 1) + SERVER loopback + OPTIONS (table_name 'batch_table_p1', batch_size '1'); +CREATE TABLE batch_table_p2 + PARTITION OF batch_table + FOR VALUES WITH (MODULUS 3, REMAINDER 2); +INSERT INTO batch_table SELECT * FROM generate_series(1, 66) i; +SELECT COUNT(*) FROM batch_table; + count +------- + 66 +(1 row) + +-- Clean up +DROP TABLE batch_table CASCADE; diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 1fec3c3eea..64698c4da3 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -142,6 +142,17 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) errmsg("%s requires a non-negative integer value", def->defname))); } + else if (strcmp(def->defname, "batch_size") == 0) + { + int batch_size; + + batch_size = strtol(defGetString(def), NULL, 10); + if (batch_size <= 0) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("%s requires a non-negative integer value", + def->defname))); + } else if (strcmp(def->defname, "password_required") == 0) { bool pw_required = defGetBoolean(def); @@ -203,6 +214,9 @@ InitPgFdwOptions(void) /* fetch_size is available on both server and table */ {"fetch_size", ForeignServerRelationId, false}, {"fetch_size", ForeignTableRelationId, false}, + /* batch_size is available on both server and table */ + {"batch_size", ForeignServerRelationId, false}, + {"batch_size", ForeignTableRelationId, false}, {"password_required", UserMappingRelationId, false}, /* diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 2f2d4d171c..b317942596 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -87,8 +87,10 @@ enum FdwScanPrivateIndex * 1) INSERT/UPDATE/DELETE statement text to be sent to the remote server * 2) Integer list of target attribute numbers for INSERT/UPDATE * (NIL for a DELETE) - * 3) Boolean flag showing if the remote query has a RETURNING clause - * 4) Integer list of attribute numbers retrieved by RETURNING, if any + * 3) Length till the end of VALUES clause for INSERT + * (-1 for a DELETE/UPDATE) + * 4) Boolean flag showing if the remote query has a RETURNING clause + * 5) Integer list of attribute numbers retrieved by RETURNING, if any */ enum FdwModifyPrivateIndex { @@ -96,6 +98,8 @@ enum FdwModifyPrivateIndex FdwModifyPrivateUpdateSql, /* Integer list of target attribute numbers for INSERT/UPDATE */ FdwModifyPrivateTargetAttnums, + /* Length till the end of VALUES clause (as an integer Value node) */ + FdwModifyPrivateLen, /* has-returning flag (as an integer Value node) */ FdwModifyPrivateHasReturning, /* Integer list of attribute numbers retrieved by RETURNING */ @@ -176,7 +180,10 @@ typedef struct PgFdwModifyState /* extracted fdw_private data */ char *query; /* text of INSERT/UPDATE/DELETE command */ + char *orig_query; /* original text of INSERT command */ List *target_attrs; /* list of target attribute numbers */ + int values_end; /* length up to the end of VALUES */ + int batch_size; /* value of FDW option "batch_size" */ bool has_returning; /* is there a RETURNING clause? */ List *retrieved_attrs; /* attr numbers retrieved by RETURNING */ @@ -185,6 +192,9 @@ typedef struct PgFdwModifyState int p_nums; /* number of parameters to transmit */ FmgrInfo *p_flinfo; /* output conversion functions for them */ + /* batch operation stuff */ + int num_slots; /* number of slots to insert */ + /* working memory context */ MemoryContext temp_cxt; /* context for per-tuple temporary data */ @@ -343,6 +353,12 @@ static TupleTableSlot *postgresExecForeignInsert(EState *estate, ResultRelInfo *resultRelInfo, TupleTableSlot *slot, TupleTableSlot *planSlot); +static TupleTableSlot **postgresExecForeignBatchInsert(EState *estate, + ResultRelInfo *resultRelInfo, + TupleTableSlot **slots, + TupleTableSlot **planSlots, + int *numSlots); +static int postgresGetForeignModifyBatchSize(ResultRelInfo *resultRelInfo); static TupleTableSlot *postgresExecForeignUpdate(EState *estate, ResultRelInfo *resultRelInfo, TupleTableSlot *slot, @@ -429,20 +445,24 @@ static PgFdwModifyState *create_foreign_modify(EState *estate, Plan *subplan, char *query, List *target_attrs, + int len, bool has_returning, List *retrieved_attrs); -static TupleTableSlot *execute_foreign_modify(EState *estate, +static TupleTableSlot **execute_foreign_modify(EState *estate, ResultRelInfo *resultRelInfo, CmdType operation, - TupleTableSlot *slot, - TupleTableSlot *planSlot); + TupleTableSlot **slots, + TupleTableSlot **planSlots, + int *numSlots); static void prepare_foreign_modify(PgFdwModifyState *fmstate); static const char **convert_prep_stmt_params(PgFdwModifyState *fmstate, ItemPointer tupleid, - TupleTableSlot *slot); + TupleTableSlot **slots, + int numSlots); static void store_returning_result(PgFdwModifyState *fmstate, TupleTableSlot *slot, PGresult *res); static void finish_foreign_modify(PgFdwModifyState *fmstate); +static void deallocate_query(PgFdwModifyState *fmstate); static List *build_remote_returning(Index rtindex, Relation rel, List *returningList); static void rebuild_fdw_scan_tlist(ForeignScan *fscan, List *tlist); @@ -530,6 +550,8 @@ postgres_fdw_handler(PG_FUNCTION_ARGS) routine->PlanForeignModify = postgresPlanForeignModify; routine->BeginForeignModify = postgresBeginForeignModify; routine->ExecForeignInsert = postgresExecForeignInsert; + routine->ExecForeignBatchInsert = postgresExecForeignBatchInsert; + routine->GetForeignModifyBatchSize = postgresGetForeignModifyBatchSize; routine->ExecForeignUpdate = postgresExecForeignUpdate; routine->ExecForeignDelete = postgresExecForeignDelete; routine->EndForeignModify = postgresEndForeignModify; @@ -1665,6 +1687,7 @@ postgresPlanForeignModify(PlannerInfo *root, List *returningList = NIL; List *retrieved_attrs = NIL; bool doNothing = false; + int values_end_len = -1; initStringInfo(&sql); @@ -1752,7 +1775,7 @@ postgresPlanForeignModify(PlannerInfo *root, deparseInsertSql(&sql, rte, resultRelation, rel, targetAttrs, doNothing, withCheckOptionList, returningList, - &retrieved_attrs); + &retrieved_attrs, &values_end_len); break; case CMD_UPDATE: deparseUpdateSql(&sql, rte, resultRelation, rel, @@ -1776,8 +1799,9 @@ postgresPlanForeignModify(PlannerInfo *root, * Build the fdw_private list that will be available to the executor. * Items in the list must match enum FdwModifyPrivateIndex, above. */ - return list_make4(makeString(sql.data), + return list_make5(makeString(sql.data), targetAttrs, + makeInteger(values_end_len), makeInteger((retrieved_attrs != NIL)), retrieved_attrs); } @@ -1797,6 +1821,7 @@ postgresBeginForeignModify(ModifyTableState *mtstate, char *query; List *target_attrs; bool has_returning; + int values_end_len; List *retrieved_attrs; RangeTblEntry *rte; @@ -1812,6 +1837,8 @@ postgresBeginForeignModify(ModifyTableState *mtstate, FdwModifyPrivateUpdateSql)); target_attrs = (List *) list_nth(fdw_private, FdwModifyPrivateTargetAttnums); + values_end_len = intVal(list_nth(fdw_private, + FdwModifyPrivateLen)); has_returning = intVal(list_nth(fdw_private, FdwModifyPrivateHasReturning)); retrieved_attrs = (List *) list_nth(fdw_private, @@ -1829,6 +1856,7 @@ postgresBeginForeignModify(ModifyTableState *mtstate, mtstate->mt_plans[subplan_index]->plan, query, target_attrs, + values_end_len, has_returning, retrieved_attrs); @@ -1846,7 +1874,8 @@ postgresExecForeignInsert(EState *estate, TupleTableSlot *planSlot) { PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState; - TupleTableSlot *rslot; + TupleTableSlot **rslot; + int numSlots = 1; /* * If the fmstate has aux_fmstate set, use the aux_fmstate (see @@ -1855,7 +1884,36 @@ postgresExecForeignInsert(EState *estate, if (fmstate->aux_fmstate) resultRelInfo->ri_FdwState = fmstate->aux_fmstate; rslot = execute_foreign_modify(estate, resultRelInfo, CMD_INSERT, - slot, planSlot); + &slot, &planSlot, &numSlots); + /* Revert that change */ + if (fmstate->aux_fmstate) + resultRelInfo->ri_FdwState = fmstate; + + return rslot ? *rslot : NULL; +} + +/* + * postgresExecForeignBatchInsert + * Insert multiple rows into a foreign table + */ +static TupleTableSlot ** +postgresExecForeignBatchInsert(EState *estate, + ResultRelInfo *resultRelInfo, + TupleTableSlot **slots, + TupleTableSlot **planSlots, + int *numSlots) +{ + PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState; + TupleTableSlot **rslot; + + /* + * If the fmstate has aux_fmstate set, use the aux_fmstate (see + * postgresBeginForeignInsert()) + */ + if (fmstate->aux_fmstate) + resultRelInfo->ri_FdwState = fmstate->aux_fmstate; + rslot = execute_foreign_modify(estate, resultRelInfo, CMD_INSERT, + slots, planSlots, numSlots); /* Revert that change */ if (fmstate->aux_fmstate) resultRelInfo->ri_FdwState = fmstate; @@ -1863,6 +1921,27 @@ postgresExecForeignInsert(EState *estate, return rslot; } +/* + * postgresGetForeignModifyBatchSize + * Report the maximum number of tuples that can be inserted in bulk + */ +static int +postgresGetForeignModifyBatchSize(ResultRelInfo *resultRelInfo) +{ + /* In EXPLAIN without ANALYZE, ri_fdwstate is NULL */ + if (resultRelInfo->ri_FdwState == NULL) + return 0; + + /* Disable batching when we have to use RETURNING. */ + if (resultRelInfo->ri_projectReturning != NULL || + (resultRelInfo->ri_TrigDesc && + resultRelInfo->ri_TrigDesc->trig_insert_after_row)) + return 1; + + /* Otherwise use the batch size specified for server/table. */ + return ((PgFdwModifyState *) resultRelInfo->ri_FdwState)->batch_size; +} + /* * postgresExecForeignUpdate * Update one row in a foreign table @@ -1873,8 +1952,13 @@ postgresExecForeignUpdate(EState *estate, TupleTableSlot *slot, TupleTableSlot *planSlot) { - return execute_foreign_modify(estate, resultRelInfo, CMD_UPDATE, - slot, planSlot); + TupleTableSlot **rslot; + int numSlots = 1; + + rslot = execute_foreign_modify(estate, resultRelInfo, CMD_UPDATE, + &slot, &planSlot, &numSlots); + + return rslot ? rslot[0] : NULL; } /* @@ -1887,8 +1971,13 @@ postgresExecForeignDelete(EState *estate, TupleTableSlot *slot, TupleTableSlot *planSlot) { - return execute_foreign_modify(estate, resultRelInfo, CMD_DELETE, - slot, planSlot); + TupleTableSlot **rslot; + int numSlots = 1; + + rslot = execute_foreign_modify(estate, resultRelInfo, CMD_DELETE, + &slot, &planSlot, &numSlots); + + return rslot ? rslot[0] : NULL; } /* @@ -1925,6 +2014,7 @@ postgresBeginForeignInsert(ModifyTableState *mtstate, RangeTblEntry *rte; TupleDesc tupdesc = RelationGetDescr(rel); int attnum; + int values_end_len; StringInfoData sql; List *targetAttrs = NIL; List *retrieved_attrs = NIL; @@ -2001,7 +2091,7 @@ postgresBeginForeignInsert(ModifyTableState *mtstate, deparseInsertSql(&sql, rte, resultRelation, rel, targetAttrs, doNothing, resultRelInfo->ri_WithCheckOptions, resultRelInfo->ri_returningList, - &retrieved_attrs); + &retrieved_attrs, &values_end_len); /* Construct an execution state. */ fmstate = create_foreign_modify(mtstate->ps.state, @@ -2011,6 +2101,7 @@ postgresBeginForeignInsert(ModifyTableState *mtstate, NULL, sql.data, targetAttrs, + values_end_len, retrieved_attrs != NIL, retrieved_attrs); @@ -2636,6 +2727,9 @@ postgresExplainForeignModify(ModifyTableState *mtstate, FdwModifyPrivateUpdateSql)); ExplainPropertyText("Remote SQL", sql, es); + + if (rinfo->ri_BatchSize > 0) + ExplainPropertyInteger("Batch Size", NULL, rinfo->ri_BatchSize, es); } } @@ -3530,6 +3624,7 @@ create_foreign_modify(EState *estate, Plan *subplan, char *query, List *target_attrs, + int values_end, bool has_returning, List *retrieved_attrs) { @@ -3538,6 +3633,7 @@ create_foreign_modify(EState *estate, TupleDesc tupdesc = RelationGetDescr(rel); Oid userid; ForeignTable *table; + ForeignServer *server; UserMapping *user; AttrNumber n_params; Oid typefnoid; @@ -3564,7 +3660,10 @@ create_foreign_modify(EState *estate, /* Set up remote query information. */ fmstate->query = query; + if (operation == CMD_INSERT) + fmstate->orig_query = pstrdup(fmstate->query); fmstate->target_attrs = target_attrs; + fmstate->values_end = values_end; fmstate->has_returning = has_returning; fmstate->retrieved_attrs = retrieved_attrs; @@ -3616,6 +3715,44 @@ create_foreign_modify(EState *estate, Assert(fmstate->p_nums <= n_params); + /* Set batch_size from foreign server/table options. */ + if (operation == CMD_INSERT) + { + /* Check the foreign table option. */ + foreach(lc, table->options) + { + DefElem *def = (DefElem *) lfirst(lc); + + if (strcmp(def->defname, "batch_size") == 0) + { + fmstate->batch_size = strtol(defGetString(def), NULL, 10); + break; + } + } + + /* Check the foreign server option if the table option is not set. */ + if (fmstate->batch_size == 0) + { + server = GetForeignServer(table->serverid); + foreach(lc, server->options) + { + DefElem *def = (DefElem *) lfirst(lc); + + if (strcmp(def->defname, "batch_size") == 0) + { + fmstate->batch_size = strtol(defGetString(def), NULL, 10); + break; + } + } + } + + /* If neither the table nor server option is set, set the default. */ + if (fmstate->batch_size == 0) + fmstate->batch_size = 100; + } + + fmstate->num_slots = 1; + /* Initialize auxiliary state */ fmstate->aux_fmstate = NULL; @@ -3626,26 +3763,50 @@ create_foreign_modify(EState *estate, * execute_foreign_modify * Perform foreign-table modification as required, and fetch RETURNING * result if any. (This is the shared guts of postgresExecForeignInsert, - * postgresExecForeignUpdate, and postgresExecForeignDelete.) + * postgresExecForeignBatchInsert, postgresExecForeignUpdate, and + * postgresExecForeignDelete.) */ -static TupleTableSlot * +static TupleTableSlot ** execute_foreign_modify(EState *estate, ResultRelInfo *resultRelInfo, CmdType operation, - TupleTableSlot *slot, - TupleTableSlot *planSlot) + TupleTableSlot **slots, + TupleTableSlot **planSlots, + int *numSlots) { PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState; ItemPointer ctid = NULL; const char **p_values; PGresult *res; int n_rows; + StringInfoData sql; /* The operation should be INSERT, UPDATE, or DELETE */ Assert(operation == CMD_INSERT || operation == CMD_UPDATE || operation == CMD_DELETE); + /* + * If the existing query was deparsed and prepared for a different number + * of rows, rebuild it for the proper number. + */ + if (operation == CMD_INSERT && fmstate->num_slots != *numSlots) + { + /* Destroy the prepared statement created previously */ + if (fmstate->p_name) + deallocate_query(fmstate); + + /* + * Build INSERT string with numSlots records in its VALUES clause. + */ + initStringInfo(&sql); + rebuildInsertSql(&sql, fmstate->orig_query, fmstate->values_end, + fmstate->p_nums, *numSlots - 1); + pfree(fmstate->query); + fmstate->query = sql.data; + fmstate->num_slots = *numSlots; + } + /* Set up the prepared statement on the remote server, if we didn't yet */ if (!fmstate->p_name) prepare_foreign_modify(fmstate); @@ -3658,7 +3819,7 @@ execute_foreign_modify(EState *estate, Datum datum; bool isNull; - datum = ExecGetJunkAttribute(planSlot, + datum = ExecGetJunkAttribute(planSlots[0], fmstate->ctidAttno, &isNull); /* shouldn't ever get a null result... */ @@ -3668,14 +3829,14 @@ execute_foreign_modify(EState *estate, } /* Convert parameters needed by prepared statement to text form */ - p_values = convert_prep_stmt_params(fmstate, ctid, slot); + p_values = convert_prep_stmt_params(fmstate, ctid, slots, *numSlots); /* * Execute the prepared statement. */ if (!PQsendQueryPrepared(fmstate->conn, fmstate->p_name, - fmstate->p_nums, + fmstate->p_nums * (*numSlots), p_values, NULL, NULL, @@ -3696,9 +3857,10 @@ execute_foreign_modify(EState *estate, /* Check number of rows affected, and fetch RETURNING tuple if any */ if (fmstate->has_returning) { + Assert(*numSlots == 1); n_rows = PQntuples(res); if (n_rows > 0) - store_returning_result(fmstate, slot, res); + store_returning_result(fmstate, slots[0], res); } else n_rows = atoi(PQcmdTuples(res)); @@ -3708,10 +3870,12 @@ execute_foreign_modify(EState *estate, MemoryContextReset(fmstate->temp_cxt); + *numSlots = n_rows; + /* * Return NULL if nothing was inserted/updated/deleted on the remote end */ - return (n_rows > 0) ? slot : NULL; + return (n_rows > 0) ? slots : NULL; } /* @@ -3771,52 +3935,64 @@ prepare_foreign_modify(PgFdwModifyState *fmstate) static const char ** convert_prep_stmt_params(PgFdwModifyState *fmstate, ItemPointer tupleid, - TupleTableSlot *slot) + TupleTableSlot **slots, + int numSlots) { const char **p_values; + int i; + int j; int pindex = 0; MemoryContext oldcontext; oldcontext = MemoryContextSwitchTo(fmstate->temp_cxt); - p_values = (const char **) palloc(sizeof(char *) * fmstate->p_nums); + p_values = (const char **) palloc(sizeof(char *) * fmstate->p_nums * numSlots); + + /* ctid is provided only for UPDATE/DELETE, which don't allow batching */ + Assert(!(tupleid != NULL && numSlots > 1)); /* 1st parameter should be ctid, if it's in use */ if (tupleid != NULL) { + Assert(numSlots == 1); /* don't need set_transmission_modes for TID output */ p_values[pindex] = OutputFunctionCall(&fmstate->p_flinfo[pindex], PointerGetDatum(tupleid)); pindex++; } - /* get following parameters from slot */ - if (slot != NULL && fmstate->target_attrs != NIL) + /* get following parameters from slots */ + if (slots != NULL && fmstate->target_attrs != NIL) { int nestlevel; ListCell *lc; nestlevel = set_transmission_modes(); - foreach(lc, fmstate->target_attrs) + for (i = 0; i < numSlots; i++) { - int attnum = lfirst_int(lc); - Datum value; - bool isnull; + j = (tupleid != NULL) ? 1 : 0; + foreach(lc, fmstate->target_attrs) + { + int attnum = lfirst_int(lc); + Datum value; + bool isnull; - value = slot_getattr(slot, attnum, &isnull); - if (isnull) - p_values[pindex] = NULL; - else - p_values[pindex] = OutputFunctionCall(&fmstate->p_flinfo[pindex], - value); - pindex++; + value = slot_getattr(slots[i], attnum, &isnull); + if (isnull) + p_values[pindex] = NULL; + else + p_values[pindex] = OutputFunctionCall(&fmstate->p_flinfo[j], + value); + pindex++; + j++; + } } reset_transmission_modes(nestlevel); } - Assert(pindex == fmstate->p_nums); + Assert(pindex == fmstate->p_nums * numSlots); MemoryContextSwitchTo(oldcontext); @@ -3870,29 +4046,41 @@ finish_foreign_modify(PgFdwModifyState *fmstate) Assert(fmstate != NULL); /* If we created a prepared statement, destroy it */ - if (fmstate->p_name) - { - char sql[64]; - PGresult *res; - - snprintf(sql, sizeof(sql), "DEALLOCATE %s", fmstate->p_name); - - /* - * We don't use a PG_TRY block here, so be careful not to throw error - * without releasing the PGresult. - */ - res = pgfdw_exec_query(fmstate->conn, sql); - if (PQresultStatus(res) != PGRES_COMMAND_OK) - pgfdw_report_error(ERROR, res, fmstate->conn, true, sql); - PQclear(res); - fmstate->p_name = NULL; - } + deallocate_query(fmstate); /* Release remote connection */ ReleaseConnection(fmstate->conn); fmstate->conn = NULL; } +/* + * deallocate_query + * Deallocate a prepared statement for a foreign insert/update/delete + * operation + */ +static void +deallocate_query(PgFdwModifyState *fmstate) +{ + char sql[64]; + PGresult *res; + + /* do nothing if the query is not allocated */ + if (!fmstate->p_name) + return; + + snprintf(sql, sizeof(sql), "DEALLOCATE %s", fmstate->p_name); + + /* + * We don't use a PG_TRY block here, so be careful not to throw error + * without releasing the PGresult. + */ + res = pgfdw_exec_query(fmstate->conn, sql); + if (PQresultStatus(res) != PGRES_COMMAND_OK) + pgfdw_report_error(ERROR, res, fmstate->conn, true, sql); + PQclear(res); + fmstate->p_name = NULL; +} + /* * build_remote_returning * Build a RETURNING targetlist of a remote query for performing an diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index 19ea27a1bc..1f67b4d9fd 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -161,7 +161,10 @@ extern void deparseInsertSql(StringInfo buf, RangeTblEntry *rte, Index rtindex, Relation rel, List *targetAttrs, bool doNothing, List *withCheckOptionList, List *returningList, - List **retrieved_attrs); + List **retrieved_attrs, int *values_end_len); +extern void rebuildInsertSql(StringInfo buf, char *orig_query, + int values_end_len, int num_cols, + int num_rows); 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 ebf6eb10a6..f152e9f8ca 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -2738,3 +2738,94 @@ COMMIT; -- should not be output because they should be closed at the end of -- the above transaction. SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + +-- =================================================================== +-- batch insert +-- =================================================================== + +BEGIN; + +CREATE SERVER batch10 FOREIGN DATA WRAPPER postgres_fdw OPTIONS( batch_size '10' ); + +SELECT count(*) +FROM pg_foreign_server +WHERE srvname = 'batch10' +AND srvoptions @> array['batch_size=10']; + +ALTER SERVER batch10 OPTIONS( SET batch_size '20' ); + +SELECT count(*) +FROM pg_foreign_server +WHERE srvname = 'batch10' +AND srvoptions @> array['batch_size=10']; + +SELECT count(*) +FROM pg_foreign_server +WHERE srvname = 'batch10' +AND srvoptions @> array['batch_size=20']; + +CREATE FOREIGN TABLE table30 ( x int ) SERVER batch10 OPTIONS ( batch_size '30' ); + +SELECT COUNT(*) +FROM pg_foreign_table +WHERE ftrelid = 'table30'::regclass +AND ftoptions @> array['batch_size=30']; + +ALTER FOREIGN TABLE table30 OPTIONS ( SET batch_size '40'); + +SELECT COUNT(*) +FROM pg_foreign_table +WHERE ftrelid = 'table30'::regclass +AND ftoptions @> array['batch_size=30']; + +SELECT COUNT(*) +FROM pg_foreign_table +WHERE ftrelid = 'table30'::regclass +AND ftoptions @> array['batch_size=40']; + +ROLLBACK; + +CREATE TABLE batch_table ( x int ); + +CREATE FOREIGN TABLE ftable ( x int ) SERVER loopback OPTIONS ( table_name 'batch_table', batch_size '10' ); +INSERT INTO ftable SELECT * FROM generate_series(1, 10) i; +INSERT INTO ftable SELECT * FROM generate_series(11, 31) i; +INSERT INTO ftable VALUES (32); +INSERT INTO ftable VALUES (33), (34); +SELECT COUNT(*) FROM ftable; +TRUNCATE batch_table; +DROP FOREIGN TABLE ftable; + +-- Disable batch insert +CREATE FOREIGN TABLE ftable ( x int ) SERVER loopback OPTIONS ( table_name 'batch_table', batch_size '1' ); +INSERT INTO ftable VALUES (1), (2); +SELECT COUNT(*) FROM ftable; +DROP FOREIGN TABLE ftable; +DROP TABLE batch_table; + +-- Use partitioning +CREATE TABLE batch_table ( x int ) PARTITION BY HASH (x); + +CREATE TABLE batch_table_p0 (LIKE batch_table); +CREATE FOREIGN TABLE batch_table_p0f + PARTITION OF batch_table + FOR VALUES WITH (MODULUS 3, REMAINDER 0) + SERVER loopback + OPTIONS (table_name 'batch_table_p0', batch_size '10'); + +CREATE TABLE batch_table_p1 (LIKE batch_table); +CREATE FOREIGN TABLE batch_table_p1f + PARTITION OF batch_table + FOR VALUES WITH (MODULUS 3, REMAINDER 1) + SERVER loopback + OPTIONS (table_name 'batch_table_p1', batch_size '1'); + +CREATE TABLE batch_table_p2 + PARTITION OF batch_table + FOR VALUES WITH (MODULUS 3, REMAINDER 2); + +INSERT INTO batch_table SELECT * FROM generate_series(1, 66) i; +SELECT COUNT(*) FROM batch_table; + +-- Clean up +DROP TABLE batch_table CASCADE; diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index 9c9293414c..854913ae5f 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -523,8 +523,9 @@ BeginForeignModify(ModifyTableState *mtstate, Begin executing a foreign table modification operation. This routine is called during executor startup. It should perform any initialization needed prior to the actual table modifications. Subsequently, - <function>ExecForeignInsert</function>, <function>ExecForeignUpdate</function> or - <function>ExecForeignDelete</function> will be called for each tuple to be + <function>ExecForeignInsert/ExecForeignBatchInsert</function>, + <function>ExecForeignUpdate</function> or + <function>ExecForeignDelete</function> will be called for tuple(s) to be inserted, updated, or deleted. </para> @@ -614,6 +615,81 @@ ExecForeignInsert(EState *estate, <para> <programlisting> +TupleTableSlot ** +ExecForeignBatchInsert(EState *estate, + ResultRelInfo *rinfo, + TupleTableSlot **slots, + TupleTableSlot *planSlots, + int *numSlots); +</programlisting> + + Insert multiple tuples in bulk into the foreign table. + The parameters are the same for <function>ExecForeignInsert</function> + except <literal>slots</literal> and <literal>planSlots</literal> contain + multiple tuples and <literal>*numSlots></literal> specifies the number of + tuples in those arrays. + </para> + + <para> + The return value is an array of slots containing the data that was + actually inserted (this might differ from the data supplied, for + example as a result of trigger actions.) + The passed-in <literal>slots</literal> can be re-used for this purpose. + The number of successfully inserted tuples is returned in + <literal>*numSlots</literal>. + </para> + + <para> + The data in the returned slot is used only if the <command>INSERT</command> + statement involves a view + <literal>WITH CHECK OPTION</literal>; or if the foreign table has + an <literal>AFTER ROW</literal> trigger. Triggers require all columns, + but the FDW could choose to optimize away returning some or all columns + depending on the contents of the + <literal>WITH CHECK OPTION</literal> constraints. + </para> + + <para> + If the <function>ExecForeignBatchInsert</function> or + <function>GetForeignModifyBatchSize</function> pointer is set to + <literal>NULL</literal>, attempts to insert into the foreign table will + use <function>ExecForeignInsert</function>. + This function is not used if the <command>INSERT</command> has the + <literal>RETURNING></literal> clause. + </para> + + <para> + Note that this function is also called when inserting routed tuples into + a foreign-table partition. See the callback functions + described below that allow the FDW to support that. + </para> + + <para> +<programlisting> +int +GetForeignModifyBatchSize(ResultRelInfo *rinfo); +</programlisting> + + Report the maximum number of tuples that a single + <function>ExecForeignBatchInsert</function> call can handle for + the specified foreign table. That is, The executor passes at most + the number of tuples that this function returns to + <function>ExecForeignBatchInsert</function>. + <literal>rinfo</literal> is the <structname>ResultRelInfo</structname> struct describing + the target foreign table. + The FDW is expected to provide a foreign server and/or foreign + table option for the user to set this value, or some hard-coded value. + </para> + + <para> + If the <function>ExecForeignBatchInsert</function> or + <function>GetForeignModifyBatchSize</function> pointer is set to + <literal>NULL</literal>, attempts to insert into the foreign table will + use <function>ExecForeignInsert</function>. + </para> + + <para> +<programlisting> TupleTableSlot * ExecForeignUpdate(EState *estate, ResultRelInfo *rinfo, @@ -741,8 +817,9 @@ BeginForeignInsert(ModifyTableState *mtstate, in both cases when it is the partition chosen for tuple routing and the target specified in a <command>COPY FROM</command> command. It should perform any initialization needed prior to the actual insertion. - Subsequently, <function>ExecForeignInsert</function> will be called for - each tuple to be inserted into the foreign table. + Subsequently, <function>ExecForeignInsert</function> or + <function>ExecForeignBatchInsert</function> will be called for + tuple(s) to be inserted into the foreign table. </para> <para> @@ -773,8 +850,8 @@ BeginForeignInsert(ModifyTableState *mtstate, <para> Note that if the FDW does not support routable foreign-table partitions and/or executing <command>COPY FROM</command> on foreign tables, this - function or <function>ExecForeignInsert</function> subsequently called - must throw error as needed. + function or <function>ExecForeignInsert/ExecForeignBatchInsert</function> + subsequently called must throw error as needed. </para> <para> diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 6a91926da8..33fac42512 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -354,6 +354,19 @@ OPTIONS (ADD password_required 'false'); </listitem> </varlistentry> + <varlistentry> + <term><literal>batch_size</literal></term> + <listitem> + <para> + This option specifies the number of rows <filename>postgres_fdw</filename> + should insert in each insert operation. It can be specified for a + foreign table or a foreign server. The option specified on a table + overrides an option specified for the server. + The default is <literal>100</literal>. + </para> + </listitem> + </varlistentry> + </variablelist> </sect3> diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 941731a0a9..9349e2c859 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -993,6 +993,18 @@ ExecInitRoutingInfo(ModifyTableState *mtstate, partRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL) partRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate, partRelInfo); + /* + * Determine if the FDW supports batch insert and determine the batch + * size (a FDW may support batching, but it may be disabled for the + * server/table). + */ + if (partRelInfo->ri_FdwRoutine != NULL && + partRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize && + partRelInfo->ri_FdwRoutine->ExecForeignBatchInsert) + partRelInfo->ri_BatchSize = + partRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize(partRelInfo); + Assert(partRelInfo->ri_BatchSize >= 0); + partRelInfo->ri_CopyMultiInsertBuffer = NULL; /* diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 921e695419..1412f97f20 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -58,6 +58,13 @@ #include "utils/rel.h" +static void ExecBatchInsert(ModifyTableState *mtstate, + ResultRelInfo *resultRelInfo, + TupleTableSlot **slots, + TupleTableSlot **planSlots, + int numSlots, + EState *estate, + bool canSetTag); static bool ExecOnConflictUpdate(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo, ItemPointer conflictTid, @@ -389,6 +396,7 @@ ExecInsert(ModifyTableState *mtstate, ModifyTable *node = (ModifyTable *) mtstate->ps.plan; OnConflictAction onconflict = node->onConflictAction; PartitionTupleRouting *proute = mtstate->mt_partition_tuple_routing; + MemoryContext oldContext; /* * If the input result relation is a partitioned table, find the leaf @@ -441,6 +449,55 @@ ExecInsert(ModifyTableState *mtstate, ExecComputeStoredGenerated(resultRelInfo, estate, slot, CMD_INSERT); + /* + * If the FDW supports batching, and batching is requested, accumulate + * rows and insert them in batches. Otherwise use the per-row inserts. + */ + if (resultRelInfo->ri_BatchSize > 1) + { + /* + * If a certain number of tuples have already been accumulated, + * or a tuple has come for a different relation than that for + * the accumulated tuples, perform the batch insert + */ + if (resultRelInfo->ri_NumSlots == resultRelInfo->ri_BatchSize) + { + ExecBatchInsert(mtstate, resultRelInfo, + resultRelInfo->ri_Slots, + resultRelInfo->ri_PlanSlots, + resultRelInfo->ri_NumSlots, + estate, canSetTag); + resultRelInfo->ri_NumSlots = 0; + } + + oldContext = MemoryContextSwitchTo(estate->es_query_cxt); + + if (resultRelInfo->ri_Slots == NULL) + { + resultRelInfo->ri_Slots = palloc(sizeof(TupleTableSlot *) * + resultRelInfo->ri_BatchSize); + resultRelInfo->ri_PlanSlots = palloc(sizeof(TupleTableSlot *) * + resultRelInfo->ri_BatchSize); + } + + resultRelInfo->ri_Slots[resultRelInfo->ri_NumSlots] = + MakeSingleTupleTableSlot(slot->tts_tupleDescriptor, + slot->tts_ops); + ExecCopySlot(resultRelInfo->ri_Slots[resultRelInfo->ri_NumSlots], + slot); + resultRelInfo->ri_PlanSlots[resultRelInfo->ri_NumSlots] = + MakeSingleTupleTableSlot(planSlot->tts_tupleDescriptor, + planSlot->tts_ops); + ExecCopySlot(resultRelInfo->ri_PlanSlots[resultRelInfo->ri_NumSlots], + planSlot); + + resultRelInfo->ri_NumSlots++; + + MemoryContextSwitchTo(oldContext); + + return NULL; + } + /* * insert into foreign table: let the FDW do it */ @@ -698,6 +755,70 @@ ExecInsert(ModifyTableState *mtstate, return result; } +/* ---------------------------------------------------------------- + * ExecBatchInsert + * + * Insert multiple tuples in an efficient way. + * Currently, this handles inserting into a foreign table without + * RETURNING clause. + * ---------------------------------------------------------------- + */ +static void +ExecBatchInsert(ModifyTableState *mtstate, + ResultRelInfo *resultRelInfo, + TupleTableSlot **slots, + TupleTableSlot **planSlots, + int numSlots, + EState *estate, + bool canSetTag) +{ + int i; + int numInserted = numSlots; + TupleTableSlot *slot = NULL; + TupleTableSlot **rslots; + + /* + * insert into foreign table: let the FDW do it + */ + rslots = resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert(estate, + resultRelInfo, + slots, + planSlots, + &numInserted); + + for (i = 0; i < numInserted; i++) + { + slot = rslots[i]; + + /* + * AFTER ROW Triggers or RETURNING expressions might reference the + * tableoid column, so (re-)initialize tts_tableOid before evaluating + * them. + */ + slot->tts_tableOid = RelationGetRelid(resultRelInfo->ri_RelationDesc); + + /* AFTER ROW INSERT Triggers */ + ExecARInsertTriggers(estate, resultRelInfo, slot, NIL, + mtstate->mt_transition_capture); + + /* + * Check any WITH CHECK OPTION constraints from parent views. See the + * comment in ExecInsert. + */ + if (resultRelInfo->ri_WithCheckOptions != NIL) + ExecWithCheckOptions(WCO_VIEW_CHECK, resultRelInfo, slot, estate); + } + + if (canSetTag && numInserted > 0) + estate->es_processed += numInserted; + + for (i = 0; i < numSlots; i++) + { + ExecDropSingleTupleTableSlot(slots[i]); + ExecDropSingleTupleTableSlot(planSlots[i]); + } +} + /* ---------------------------------------------------------------- * ExecDelete * @@ -1937,6 +2058,9 @@ ExecModifyTable(PlanState *pstate) ItemPointerData tuple_ctid; HeapTupleData oldtupdata; HeapTuple oldtuple; + PartitionTupleRouting *proute = node->mt_partition_tuple_routing; + List *relinfos = NIL; + ListCell *lc; CHECK_FOR_INTERRUPTS(); @@ -2152,6 +2276,25 @@ ExecModifyTable(PlanState *pstate) return slot; } + /* + * Insert remaining tuples for batch insert. + */ + if (proute) + relinfos = estate->es_tuple_routing_result_relations; + else + relinfos = estate->es_opened_result_relations; + + foreach(lc, relinfos) + { + resultRelInfo = lfirst(lc); + if (resultRelInfo->ri_NumSlots > 0) + ExecBatchInsert(node, resultRelInfo, + resultRelInfo->ri_Slots, + resultRelInfo->ri_PlanSlots, + resultRelInfo->ri_NumSlots, + estate, node->canSetTag); + } + /* * We're done, but fire AFTER STATEMENT triggers before exiting. */ @@ -2650,6 +2793,20 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) } } + /* + * Determine if the FDW supports batch insert and determine the batch + * size (a FDW may support batching, but it may be disabled for the + * server/table). + */ + if (!resultRelInfo->ri_usesFdwDirectModify && + operation == CMD_INSERT && + resultRelInfo->ri_FdwRoutine != NULL && + resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize && + resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert) + resultRelInfo->ri_BatchSize = + resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize(resultRelInfo); + Assert(resultRelInfo->ri_BatchSize >= 0); + /* * Lastly, if this is not the primary (canSetTag) ModifyTable node, add it * to estate->es_auxmodifytables so that it will be run to completion by diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c index c4eba6b053..dbf6b30233 100644 --- a/src/backend/nodes/list.c +++ b/src/backend/nodes/list.c @@ -277,6 +277,21 @@ list_make4_impl(NodeTag t, ListCell datum1, ListCell datum2, return list; } +List * +list_make5_impl(NodeTag t, ListCell datum1, ListCell datum2, + ListCell datum3, ListCell datum4, ListCell datum5) +{ + List *list = new_list(t, 5); + + list->elements[0] = datum1; + list->elements[1] = datum2; + list->elements[2] = datum3; + list->elements[3] = datum4; + list->elements[4] = datum5; + check_list_invariants(list); + return list; +} + /* * Make room for a new head cell in the given (non-NIL) list. * diff --git a/src/include/foreign/fdwapi.h b/src/include/foreign/fdwapi.h index 2953499fb1..248f78da45 100644 --- a/src/include/foreign/fdwapi.h +++ b/src/include/foreign/fdwapi.h @@ -85,6 +85,14 @@ typedef TupleTableSlot *(*ExecForeignInsert_function) (EState *estate, TupleTableSlot *slot, TupleTableSlot *planSlot); +typedef TupleTableSlot **(*ExecForeignBatchInsert_function) (EState *estate, + ResultRelInfo *rinfo, + TupleTableSlot **slots, + TupleTableSlot **planSlots, + int *numSlots); + +typedef int (*GetForeignModifyBatchSize_function) (ResultRelInfo *rinfo); + typedef TupleTableSlot *(*ExecForeignUpdate_function) (EState *estate, ResultRelInfo *rinfo, TupleTableSlot *slot, @@ -209,6 +217,8 @@ typedef struct FdwRoutine PlanForeignModify_function PlanForeignModify; BeginForeignModify_function BeginForeignModify; ExecForeignInsert_function ExecForeignInsert; + ExecForeignBatchInsert_function ExecForeignBatchInsert; + GetForeignModifyBatchSize_function GetForeignModifyBatchSize; ExecForeignUpdate_function ExecForeignUpdate; ExecForeignDelete_function ExecForeignDelete; EndForeignModify_function EndForeignModify; diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 48c3f570fa..d65099c94a 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -446,6 +446,12 @@ typedef struct ResultRelInfo /* true when modifying foreign table directly */ bool ri_usesFdwDirectModify; + /* batch insert stuff */ + int ri_NumSlots; /* number of slots in the array */ + int ri_BatchSize; /* max slots inserted in a single batch */ + TupleTableSlot **ri_Slots; /* input tuples for batch insert */ + TupleTableSlot **ri_PlanSlots; + /* list of WithCheckOption's to be checked */ List *ri_WithCheckOptions; diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h index 710dcd37ef..404e03f132 100644 --- a/src/include/nodes/pg_list.h +++ b/src/include/nodes/pg_list.h @@ -213,6 +213,10 @@ list_length(const List *l) #define list_make4(x1,x2,x3,x4) \ list_make4_impl(T_List, list_make_ptr_cell(x1), list_make_ptr_cell(x2), \ list_make_ptr_cell(x3), list_make_ptr_cell(x4)) +#define list_make5(x1,x2,x3,x4,x5) \ + list_make5_impl(T_List, list_make_ptr_cell(x1), list_make_ptr_cell(x2), \ + list_make_ptr_cell(x3), list_make_ptr_cell(x4), \ + list_make_ptr_cell(x5)) #define list_make1_int(x1) \ list_make1_impl(T_IntList, list_make_int_cell(x1)) @@ -224,6 +228,10 @@ list_length(const List *l) #define list_make4_int(x1,x2,x3,x4) \ list_make4_impl(T_IntList, list_make_int_cell(x1), list_make_int_cell(x2), \ list_make_int_cell(x3), list_make_int_cell(x4)) +#define list_make5_int(x1,x2,x3,x4,x5) \ + list_make5_impl(T_IntList, list_make_int_cell(x1), list_make_int_cell(x2), \ + list_make_int_cell(x3), list_make_int_cell(x4), \ + list_make_int_cell(x5)) #define list_make1_oid(x1) \ list_make1_impl(T_OidList, list_make_oid_cell(x1)) @@ -235,6 +243,10 @@ list_length(const List *l) #define list_make4_oid(x1,x2,x3,x4) \ list_make4_impl(T_OidList, list_make_oid_cell(x1), list_make_oid_cell(x2), \ list_make_oid_cell(x3), list_make_oid_cell(x4)) +#define list_make5_oid(x1,x2,x3,x4,x5) \ + list_make5_impl(T_OidList, list_make_oid_cell(x1), list_make_oid_cell(x2), \ + list_make_oid_cell(x3), list_make_oid_cell(x4), \ + list_make_oid_cell(x5)) /* * Locate the n'th cell (counting from 0) of the list. @@ -520,6 +532,9 @@ extern List *list_make3_impl(NodeTag t, ListCell datum1, ListCell datum2, ListCell datum3); extern List *list_make4_impl(NodeTag t, ListCell datum1, ListCell datum2, ListCell datum3, ListCell datum4); +extern List *list_make5_impl(NodeTag t, ListCell datum1, ListCell datum2, + ListCell datum3, ListCell datum4, + ListCell datum5); extern pg_nodiscard List *lappend(List *list, void *datum); extern pg_nodiscard List *lappend_int(List *list, int datum); -- 2.26.2 --------------FBA1D5F96F051BF27C34C171 Content-Type: text/x-patch; charset=UTF-8; name="0002-tweaks-v11.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0002-tweaks-v11.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add column name to error description @ 2024-03-31 18:57 Erik Wienhold <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Erik Wienhold @ 2024-03-31 18:57 UTC (permalink / raw) To: Marcos Pegoraro <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On 2024-03-31 15:22 +0200, Marcos Pegoraro wrote: > This is my first patch, so sorry if I miss something. Please make sure that tests are passing by running make check: https://www.postgresql.org/docs/current/regress-run.html#REGRESS-RUN-TEMP-INST The patch breaks src/test/regress/sql/plpgsql.sql at: -- this does not work currently (no implicit casting) create or replace function compos() returns compostype as $$ begin return (1, 'hello'); end; $$ language plpgsql; select compos(); server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. connection to server was lost > If I have a function which returns lots of columns and any of these columns > returns a wrong type it's not easy to see which one is that column because > it points me only to its position, not its name. So, this patch only adds > that column name, just that. +1 for this improvement. > create function my_f(a integer, b integer) returns table(first_col integer, > lots_of_cols_later numeric) language plpgsql as $function$ > begin > return query select a, b; > end;$function$; > > select * from my_f(1,1); > --ERROR: structure of query does not match function result type > --Returned type integer does not match expected type numeric in column 2. > > For a function which has just 2 columns is easy but if it returns a hundred > of columns, which one is that 66th column ? > > My patch just adds column name to that description message. > --ERROR: structure of query does not match function result type > --Returned type integer does not match expected type numeric in column 2- > lots_of_cols_later. > diff --git a/src/backend/access/common/attmap.c b/src/backend/access/common/attmap.c > index b0fe27ef57..85f7c0cb8c 100644 > --- a/src/backend/access/common/attmap.c > +++ b/src/backend/access/common/attmap.c > @@ -118,12 +118,13 @@ build_attrmap_by_position(TupleDesc indesc, > ereport(ERROR, > (errcode(ERRCODE_DATATYPE_MISMATCH), > errmsg_internal("%s", _(msg)), > - errdetail("Returned type %s does not match expected type %s in column %d.", > + errdetail("Returned type %s does not match expected type %s in column %d-%s.", The format "%d-%s" is not ideal. I suggesst "%d (%s)". > format_type_with_typemod(att->atttypid, > att->atttypmod), > format_type_with_typemod(atttypid, > atttypmod), > - noutcols))); > + noutcols, > + att->attname))); Must be NameStr(att->attname) for use with printf's %s. GCC even prints a warning: attmap.c:121:60: warning: format ‘%s’ expects argument of type ‘char *’, but argument 5 has type ‘NameData’ {aka ‘struct nameData’} [-Wformat=] -- Erik ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add column name to error description @ 2024-03-31 19:15 Tom Lane <[email protected]> parent: Erik Wienhold <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Tom Lane @ 2024-03-31 19:15 UTC (permalink / raw) To: Erik Wienhold <[email protected]>; +Cc: Marcos Pegoraro <[email protected]>; PostgreSQL Hackers <[email protected]> Erik Wienhold <[email protected]> writes: > On 2024-03-31 15:22 +0200, Marcos Pegoraro wrote: >> This is my first patch, so sorry if I miss something. > Please make sure that tests are passing by running make check: check-world, in fact. > The format "%d-%s" is not ideal. I suggesst "%d (%s)". I didn't like that either, for two reasons: if we have a column name it ought to be the prominent label, but we might not have one if the TupleDesc came from some anonymous source (possibly that case explains the test crash? Although I think the attname would be an empty string rather than missing entirely in such cases). I think it'd be worth providing two distinct message strings: "Returned type %s does not match expected type %s in column \"%s\" (position %d)." "Returned type %s does not match expected type %s in column position %d." I'd suggest dropping the column number entirely in the first case, were it not that the attnames might well not be unique if we're dealing with an anonymous record type such as a SELECT result. regards, tom lane ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add column name to error description @ 2024-09-13 08:02 jian he <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: jian he @ 2024-09-13 08:02 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Erik Wienhold <[email protected]>; Marcos Pegoraro <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Apr 1, 2024 at 3:15 AM Tom Lane <[email protected]> wrote: > > > > The format "%d-%s" is not ideal. I suggesst "%d (%s)". > > I didn't like that either, for two reasons: if we have a column name > it ought to be the prominent label, but we might not have one if the > TupleDesc came from some anonymous source (possibly that case explains > the test crash? Although I think the attname would be an empty string > rather than missing entirely in such cases). I think it'd be worth > providing two distinct message strings: > > "Returned type %s does not match expected type %s in column \"%s\" (position %d)." > "Returned type %s does not match expected type %s in column position %d." > > I'd suggest dropping the column number entirely in the first case, > were it not that the attnames might well not be unique if we're > dealing with an anonymous record type such as a SELECT result. > please check the attached POC, hope the output is what you expected. now we can output these two message. > "Returned type %s does not match expected type %s in column \"%s\" (position %d)." > "Returned type %s does not match expected type %s in column position %d." create type compostype as (x int, y varchar); create or replace function compos() returns compostype as $$ begin return (1, 'hello'); end; $$ language plpgsql; select compos(); HEAD error message is ERROR: returned record type does not match expected record type DETAIL: Returned type unknown does not match expected type character varying in column 2. CONTEXT: PL/pgSQL function compos() while casting return value to function's return type if we print out NameStr(att->attname) then error becomes: +DETAIL: Returned type unknown does not match expected type character varying in column "f2" (position 2). In this case, printing out {column \"%s\"} is not helpful at all. ---------------case1 create function my_f(a integer, b integer) returns table(first_col integer, lots_of_cols_later numeric) language plpgsql as $function$ begin return query select a, b; end; $function$; -----------------case2 create or replace function returnsrecord(int) returns record language plpgsql as $$ begin return row($1,$1+1); end $$; select * from my_f(1,1); select * from returnsrecord(42) as r(x int, y bigint); In the first case, we want to print out the column \"%s\", but in the second case, we don't. in plpgsql_exec_function first case, return first tuple values then check tuple attributes in the second case, check the tuple attribute error out immediately. build_attrmap_by_position both indesc->tdtypeid = 2249, outdesc->tdtypeid = 2249 so build_attrmap_by_position itself cannot distinguish these two cases. To solve this, we can add a boolean argument to convert_tuples_by_position. Also this error ERROR: structure of query does not match function result type occurred quite often on the internet, see [1] but there are no tests for it. so we can add a test in src/test/regress/sql/plpgsql.sql [1] https://stackoverflow.com/search?q=structure+of+query+does+not+match+function+result+type Attachments: [text/x-patch] v2-0001-improve-error-message-in-build_attrmap_by_positio.patch (14.7K, ../../CACJufxHiXzSOB7FqFFQGqJM0b3MNP08AB84EzTH5ZZGwGuDGgA@mail.gmail.com/2-v2-0001-improve-error-message-in-build_attrmap_by_positio.patch) download | inline diff: From b37ad3adc4534dd5dfb8bc78f58a6b81a6c078cf Mon Sep 17 00:00:00 2001 From: jian he <[email protected]> Date: Fri, 13 Sep 2024 15:54:43 +0800 Subject: [PATCH v2 1/1] improve error message in build_attrmap_by_position make build_attrmap_by_position error message more helpful. error message print out the column name conditionally. minor refactor error message, change from "column %d" to "column position %d". also add tests for "ERROR: structure of query does not match function result type" discussion: https://postgr.es/m/CAB-JLwanky28gjAMdnMh1CjyO1b2zLdr6UOA1-oY9G7PVL9KKQ@mail.gmail.com --- src/backend/access/common/attmap.c | 30 ++++++++++++++----- src/backend/access/common/tupconvert.c | 5 ++-- src/backend/executor/tstoreReceiver.c | 3 +- src/include/access/attmap.h | 3 +- src/include/access/tupconvert.h | 3 +- .../plpgsql/src/expected/plpgsql_record.out | 12 ++++---- src/pl/plpgsql/src/pl_exec.c | 18 +++++++---- src/test/regress/expected/plpgsql.out | 17 ++++++++++- src/test/regress/sql/plpgsql.sql | 12 ++++++++ 9 files changed, 78 insertions(+), 25 deletions(-) diff --git a/src/backend/access/common/attmap.c b/src/backend/access/common/attmap.c index b0fe27ef57..c55e587efd 100644 --- a/src/backend/access/common/attmap.c +++ b/src/backend/access/common/attmap.c @@ -74,7 +74,8 @@ free_attrmap(AttrMap *map) AttrMap * build_attrmap_by_position(TupleDesc indesc, TupleDesc outdesc, - const char *msg) + const char *msg, + bool extra) { AttrMap *attrMap; int nincols; @@ -115,15 +116,30 @@ build_attrmap_by_position(TupleDesc indesc, /* Found matching column, now check type */ if (atttypid != att->atttypid || (atttypmod != att->atttypmod && atttypmod >= 0)) + { + char *returned_type; + char *expected_type; + + returned_type = format_type_with_typemod(att->atttypid, + att->atttypmod); + expected_type = format_type_with_typemod(atttypid, + atttypmod); + ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg_internal("%s", _(msg)), - errdetail("Returned type %s does not match expected type %s in column %d.", - format_type_with_typemod(att->atttypid, - att->atttypmod), - format_type_with_typemod(atttypid, - atttypmod), - noutcols))); + extra ? + errdetail("Returned type %s does not match expected type %s in column \"%s\" (position %d).", + returned_type, + expected_type, + pstrdup(NameStr(att->attname)), + noutcols) + : + errdetail("Returned type %s does not match expected type %s in column position %d.", + returned_type, + expected_type, + noutcols))); + } attrMap->attnums[i] = (AttrNumber) (j + 1); j++; break; diff --git a/src/backend/access/common/tupconvert.c b/src/backend/access/common/tupconvert.c index 4d596c4bef..e975cba879 100644 --- a/src/backend/access/common/tupconvert.c +++ b/src/backend/access/common/tupconvert.c @@ -58,14 +58,15 @@ TupleConversionMap * convert_tuples_by_position(TupleDesc indesc, TupleDesc outdesc, - const char *msg) + const char *msg, + bool extra) { TupleConversionMap *map; int n; AttrMap *attrMap; /* Verify compatibility and prepare attribute-number map */ - attrMap = build_attrmap_by_position(indesc, outdesc, msg); + attrMap = build_attrmap_by_position(indesc, outdesc, msg, extra); if (attrMap == NULL) { diff --git a/src/backend/executor/tstoreReceiver.c b/src/backend/executor/tstoreReceiver.c index de4646b5c2..e97ac62444 100644 --- a/src/backend/executor/tstoreReceiver.c +++ b/src/backend/executor/tstoreReceiver.c @@ -81,7 +81,8 @@ tstoreStartupReceiver(DestReceiver *self, int operation, TupleDesc typeinfo) if (myState->target_tupdesc) myState->tupmap = convert_tuples_by_position(typeinfo, myState->target_tupdesc, - myState->map_failure_msg); + myState->map_failure_msg, + true); else myState->tupmap = NULL; diff --git a/src/include/access/attmap.h b/src/include/access/attmap.h index 123ec45c13..3afdde5eeb 100644 --- a/src/include/access/attmap.h +++ b/src/include/access/attmap.h @@ -49,6 +49,7 @@ extern AttrMap *build_attrmap_by_name_if_req(TupleDesc indesc, bool missing_ok); extern AttrMap *build_attrmap_by_position(TupleDesc indesc, TupleDesc outdesc, - const char *msg); + const char *msg, + bool extra); #endif /* ATTMAP_H */ diff --git a/src/include/access/tupconvert.h b/src/include/access/tupconvert.h index 62a6d12761..2d8182782b 100644 --- a/src/include/access/tupconvert.h +++ b/src/include/access/tupconvert.h @@ -35,7 +35,8 @@ typedef struct TupleConversionMap extern TupleConversionMap *convert_tuples_by_position(TupleDesc indesc, TupleDesc outdesc, - const char *msg); + const char *msg, + bool extra); extern TupleConversionMap *convert_tuples_by_name(TupleDesc indesc, TupleDesc outdesc); diff --git a/src/pl/plpgsql/src/expected/plpgsql_record.out b/src/pl/plpgsql/src/expected/plpgsql_record.out index 6974c8f4a4..319fd26972 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_record.out +++ b/src/pl/plpgsql/src/expected/plpgsql_record.out @@ -28,7 +28,7 @@ create or replace function retc(int) returns two_int8s language plpgsql as $$ begin return row($1,1); end $$; select retc(42); ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 1. +DETAIL: Returned type integer does not match expected type bigint in column position 1. CONTEXT: PL/pgSQL function retc(integer) while casting return value to function's return type -- nor extra columns create or replace function retc(int) returns two_int8s language plpgsql as @@ -50,7 +50,7 @@ create or replace function retc(int) returns two_int8s language plpgsql as $$ declare r record; begin r := row($1,1); return r; end $$; select retc(42); ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 1. +DETAIL: Returned type integer does not match expected type bigint in column position 1. CONTEXT: PL/pgSQL function retc(integer) while casting return value to function's return type create or replace function retc(int) returns two_int8s language plpgsql as $$ declare r record; begin r := row($1::int8, 1::int8, 42); return r; end $$; @@ -386,7 +386,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column position 2. CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- same with an intermediate record variable create or replace function returnsrecord(int) returns record language plpgsql as @@ -409,7 +409,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column position 2. CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- should work the same with a missing column in the actual result value create table has_hole(f1 int, f2 int, f3 int); @@ -434,7 +434,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column position 2. CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- same with an intermediate record variable create or replace function returnsrecord(int) returns record language plpgsql as @@ -457,7 +457,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column position 2. CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- check access to a field of an argument declared "record" create function getf1(x record) returns int language plpgsql as diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index ea9740e3f8..d7fda18f16 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -831,7 +831,8 @@ coerce_function_result_tuple(PLpgSQL_execstate *estate, TupleDesc tupdesc) /* check rowtype compatibility */ tupmap = convert_tuples_by_position(retdesc, tupdesc, - gettext_noop("returned record type does not match expected record type")); + gettext_noop("returned record type does not match expected record type"), + false); /* it might need conversion */ if (tupmap) @@ -895,7 +896,8 @@ coerce_function_result_tuple(PLpgSQL_execstate *estate, TupleDesc tupdesc) /* check rowtype compatibility */ tupmap = convert_tuples_by_position(retdesc, tupdesc, - gettext_noop("returned record type does not match expected record type")); + gettext_noop("returned record type does not match expected record type"), + false); /* it might need conversion */ if (tupmap) @@ -1091,7 +1093,8 @@ plpgsql_exec_trigger(PLpgSQL_function *func, /* check rowtype compatibility */ tupmap = convert_tuples_by_position(retdesc, RelationGetDescr(trigdata->tg_relation), - gettext_noop("returned row structure does not match the structure of the triggering table")); + gettext_noop("returned row structure does not match the structure of the triggering table"), + false); /* it might need conversion */ if (tupmap) rettup = execute_attr_map_tuple(rettup, tupmap); @@ -1119,7 +1122,8 @@ plpgsql_exec_trigger(PLpgSQL_function *func, /* check rowtype compatibility */ tupmap = convert_tuples_by_position(retdesc, RelationGetDescr(trigdata->tg_relation), - gettext_noop("returned row structure does not match the structure of the triggering table")); + gettext_noop("returned row structure does not match the structure of the triggering table"), + false); /* it might need conversion */ if (tupmap) rettup = execute_attr_map_tuple(rettup, tupmap); @@ -3415,7 +3419,8 @@ exec_stmt_return_next(PLpgSQL_execstate *estate, rec_tupdesc = expanded_record_get_tupdesc(rec->erh); tupmap = convert_tuples_by_position(rec_tupdesc, tupdesc, - gettext_noop("wrong record type supplied in RETURN NEXT")); + gettext_noop("wrong record type supplied in RETURN NEXT"), + false); tuple = expanded_record_get_tuple(rec->erh); if (tupmap) tuple = execute_attr_map_tuple(tuple, tupmap); @@ -3479,7 +3484,8 @@ exec_stmt_return_next(PLpgSQL_execstate *estate, retvaldesc = deconstruct_composite_datum(retval, &tmptup); tuple = &tmptup; tupmap = convert_tuples_by_position(retvaldesc, tupdesc, - gettext_noop("returned record type does not match expected record type")); + gettext_noop("returned record type does not match expected record type"), + false); if (tupmap) tuple = execute_attr_map_tuple(tuple, tupmap); tuplestore_puttuple(estate->tuple_store, tuple); diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out index 0a6945581b..9bee00ded5 100644 --- a/src/test/regress/expected/plpgsql.out +++ b/src/test/regress/expected/plpgsql.out @@ -3777,7 +3777,7 @@ end; $$ language plpgsql; select compos(); ERROR: returned record type does not match expected record type -DETAIL: Returned type unknown does not match expected type character varying in column 2. +DETAIL: Returned type unknown does not match expected type character varying in column position 2. CONTEXT: PL/pgSQL function compos() while casting return value to function's return type -- ... but this does create or replace function compos() returns compostype as $$ @@ -5852,3 +5852,18 @@ END; $$ LANGUAGE plpgsql; ERROR: "x" is not a scalar variable LINE 3: GET DIAGNOSTICS x = ROW_COUNT; ^ +-- +-- Check returned type with the declared type +-- +create function my_f(a integer, b integer) +returns table(first_col integer, lots_of_cols_later numeric) language plpgsql as +$function$ +begin + return query select a, b; +end; +$function$; +select * from my_f(1,1); +ERROR: structure of query does not match function result type +DETAIL: Returned type integer does not match expected type numeric in column "b" (position 2). +CONTEXT: SQL statement "select a, b" +PL/pgSQL function my_f(integer,integer) line 3 at RETURN QUERY diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql index 18c91572ae..d63afc8b97 100644 --- a/src/test/regress/sql/plpgsql.sql +++ b/src/test/regress/sql/plpgsql.sql @@ -4774,3 +4774,15 @@ BEGIN GET DIAGNOSTICS x = ROW_COUNT; RETURN; END; $$ LANGUAGE plpgsql; + +-- +-- Check returned type with the declared type +-- +create function my_f(a integer, b integer) +returns table(first_col integer, lots_of_cols_later numeric) language plpgsql as +$function$ +begin + return query select a, b; +end; +$function$; +select * from my_f(1,1); \ No newline at end of file base-commit: 2b67bdca529c6aed4303eb6963d09d1b540137b8 -- 2.34.1 ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add column name to error description @ 2025-03-06 20:56 Tom Lane <[email protected]> parent: jian he <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Tom Lane @ 2025-03-06 20:56 UTC (permalink / raw) To: jian he <[email protected]>; +Cc: Erik Wienhold <[email protected]>; Marcos Pegoraro <[email protected]>; PostgreSQL Hackers <[email protected]> [ sorry about having let this thread fall off my radar ] jian he <[email protected]> writes: > if we print out NameStr(att->attname) then error becomes: > +DETAIL: Returned type unknown does not match expected type character > varying in column "f2" (position 2). > In this case, printing out {column \"%s\"} is not helpful at all. Actually, the problem is that we should be printing the expected column name not the input column name. At least in the test cases we have, that gives a user-supplied name in every case. Even if there are some cases where you just get "f2", that's not horrible. So I don't think this is worth the amount of code churn involved in the v2 patch --- let's just print it unconditionally, as attached. I do still think that including the column number is potentially helpful, though, so I didn't remove that. regards, tom lane Attachments: [text/x-diff] v3-0001-improve-error-message-in-build_attrmap_by_positio.patch (8.8K, ../../[email protected]/2-v3-0001-improve-error-message-in-build_attrmap_by_positio.patch) download | inline diff: diff --git a/src/backend/access/common/attmap.c b/src/backend/access/common/attmap.c index 4b6cfe05c02..365243aeaad 100644 --- a/src/backend/access/common/attmap.c +++ b/src/backend/access/common/attmap.c @@ -96,33 +96,35 @@ build_attrmap_by_position(TupleDesc indesc, same = true; for (i = 0; i < n; i++) { - Form_pg_attribute att = TupleDescAttr(outdesc, i); + Form_pg_attribute outatt = TupleDescAttr(outdesc, i); Oid atttypid; int32 atttypmod; - if (att->attisdropped) + if (outatt->attisdropped) continue; /* attrMap->attnums[i] is already 0 */ noutcols++; - atttypid = att->atttypid; - atttypmod = att->atttypmod; + atttypid = outatt->atttypid; + atttypmod = outatt->atttypmod; for (; j < indesc->natts; j++) { - att = TupleDescAttr(indesc, j); - if (att->attisdropped) + Form_pg_attribute inatt = TupleDescAttr(indesc, j); + + if (inatt->attisdropped) continue; nincols++; /* Found matching column, now check type */ - if (atttypid != att->atttypid || - (atttypmod != att->atttypmod && atttypmod >= 0)) + if (atttypid != inatt->atttypid || + (atttypmod != inatt->atttypmod && atttypmod >= 0)) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg_internal("%s", _(msg)), - errdetail("Returned type %s does not match expected type %s in column %d.", - format_type_with_typemod(att->atttypid, - att->atttypmod), + errdetail("Returned type %s does not match expected type %s in column \"%s\" (position %d).", + format_type_with_typemod(inatt->atttypid, + inatt->atttypmod), format_type_with_typemod(atttypid, atttypmod), + NameStr(outatt->attname), noutcols))); attrMap->attnums[i] = (AttrNumber) (j + 1); j++; diff --git a/src/pl/plpgsql/src/expected/plpgsql_record.out b/src/pl/plpgsql/src/expected/plpgsql_record.out index 6974c8f4a44..e5de7143606 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_record.out +++ b/src/pl/plpgsql/src/expected/plpgsql_record.out @@ -28,7 +28,7 @@ create or replace function retc(int) returns two_int8s language plpgsql as $$ begin return row($1,1); end $$; select retc(42); ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 1. +DETAIL: Returned type integer does not match expected type bigint in column "q1" (position 1). CONTEXT: PL/pgSQL function retc(integer) while casting return value to function's return type -- nor extra columns create or replace function retc(int) returns two_int8s language plpgsql as @@ -50,7 +50,7 @@ create or replace function retc(int) returns two_int8s language plpgsql as $$ declare r record; begin r := row($1,1); return r; end $$; select retc(42); ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 1. +DETAIL: Returned type integer does not match expected type bigint in column "q1" (position 1). CONTEXT: PL/pgSQL function retc(integer) while casting return value to function's return type create or replace function retc(int) returns two_int8s language plpgsql as $$ declare r record; begin r := row($1::int8, 1::int8, 42); return r; end $$; @@ -386,7 +386,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column "y" (position 2). CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- same with an intermediate record variable create or replace function returnsrecord(int) returns record language plpgsql as @@ -409,7 +409,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column "y" (position 2). CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- should work the same with a missing column in the actual result value create table has_hole(f1 int, f2 int, f3 int); @@ -434,7 +434,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column "y" (position 2). CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- same with an intermediate record variable create or replace function returnsrecord(int) returns record language plpgsql as @@ -457,7 +457,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column "y" (position 2). CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- check access to a field of an argument declared "record" create function getf1(x record) returns int language plpgsql as @@ -545,6 +545,7 @@ begin return next h; return next row(5,6); return next row(7,8)::has_hole; + return query select 9, 10; end$$; select returnssetofholes(); returnssetofholes @@ -554,7 +555,8 @@ select returnssetofholes(); (3,4) (5,6) (7,8) -(5 rows) + (9,10) +(6 rows) create or replace function returnssetofholes() returns setof has_hole language plpgsql as $$ @@ -575,6 +577,16 @@ select returnssetofholes(); ERROR: returned record type does not match expected record type DETAIL: Number of returned columns (3) does not match expected column count (2). CONTEXT: PL/pgSQL function returnssetofholes() line 3 at RETURN NEXT +create or replace function returnssetofholes() returns setof has_hole language plpgsql as +$$ +begin + return query select 1, 2.0; -- fails +end$$; +select returnssetofholes(); +ERROR: structure of query does not match function result type +DETAIL: Returned type numeric does not match expected type integer in column "f3" (position 2). +CONTEXT: SQL statement "select 1, 2.0" +PL/pgSQL function returnssetofholes() line 3 at RETURN QUERY -- check behavior with changes of a named rowtype create table mutable(f1 int, f2 text); create function sillyaddone(int) returns int language plpgsql as diff --git a/src/pl/plpgsql/src/sql/plpgsql_record.sql b/src/pl/plpgsql/src/sql/plpgsql_record.sql index 96dcc414e92..4fbed38b8bb 100644 --- a/src/pl/plpgsql/src/sql/plpgsql_record.sql +++ b/src/pl/plpgsql/src/sql/plpgsql_record.sql @@ -338,6 +338,7 @@ begin return next h; return next row(5,6); return next row(7,8)::has_hole; + return query select 9, 10; end$$; select returnssetofholes(); @@ -356,6 +357,13 @@ begin end$$; select returnssetofholes(); +create or replace function returnssetofholes() returns setof has_hole language plpgsql as +$$ +begin + return query select 1, 2.0; -- fails +end$$; +select returnssetofholes(); + -- check behavior with changes of a named rowtype create table mutable(f1 int, f2 text); diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out index c5f73fef297..d8ce39dba3c 100644 --- a/src/test/regress/expected/plpgsql.out +++ b/src/test/regress/expected/plpgsql.out @@ -3779,7 +3779,7 @@ end; $$ language plpgsql; select compos(); ERROR: returned record type does not match expected record type -DETAIL: Returned type unknown does not match expected type character varying in column 2. +DETAIL: Returned type unknown does not match expected type character varying in column "y" (position 2). CONTEXT: PL/pgSQL function compos() while casting return value to function's return type -- ... but this does create or replace function compos() returns compostype as $$ ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add column name to error description @ 2025-03-07 01:42 Erik Wienhold <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Erik Wienhold @ 2025-03-07 01:42 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: jian he <[email protected]>; Marcos Pegoraro <[email protected]>; PostgreSQL Hackers <[email protected]> On 2025-03-06 21:56 +0100, Tom Lane wrote: > jian he <[email protected]> writes: > > if we print out NameStr(att->attname) then error becomes: > > +DETAIL: Returned type unknown does not match expected type character > > varying in column "f2" (position 2). > > > In this case, printing out {column \"%s\"} is not helpful at all. > > Actually, the problem is that we should be printing the expected > column name not the input column name. At least in the test cases > we have, that gives a user-supplied name in every case. Even if > there are some cases where you just get "f2", that's not horrible. > So I don't think this is worth the amount of code churn involved in > the v2 patch --- let's just print it unconditionally, as attached. > I do still think that including the column number is potentially > helpful, though, so I didn't remove that. +1 But I don't see the point in keeping variables atttypid and atttypmod around when those values are now available via outatt. Removing these two variables makes the code easier to read IMO. Done so in the attached v4. -- Erik Wienhold diff --git a/src/backend/access/common/attmap.c b/src/backend/access/common/attmap.c index 4b6cfe05c02..4901ebecef7 100644 --- a/src/backend/access/common/attmap.c +++ b/src/backend/access/common/attmap.c @@ -96,33 +96,31 @@ build_attrmap_by_position(TupleDesc indesc, same = true; for (i = 0; i < n; i++) { - Form_pg_attribute att = TupleDescAttr(outdesc, i); - Oid atttypid; - int32 atttypmod; + Form_pg_attribute outatt = TupleDescAttr(outdesc, i); - if (att->attisdropped) + if (outatt->attisdropped) continue; /* attrMap->attnums[i] is already 0 */ noutcols++; - atttypid = att->atttypid; - atttypmod = att->atttypmod; for (; j < indesc->natts; j++) { - att = TupleDescAttr(indesc, j); - if (att->attisdropped) + Form_pg_attribute inatt = TupleDescAttr(indesc, j); + + if (inatt->attisdropped) continue; nincols++; /* Found matching column, now check type */ - if (atttypid != att->atttypid || - (atttypmod != att->atttypmod && atttypmod >= 0)) + if (outatt->atttypid != inatt->atttypid || + (outatt->atttypmod != inatt->atttypmod && outatt->atttypmod >= 0)) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg_internal("%s", _(msg)), - errdetail("Returned type %s does not match expected type %s in column %d.", - format_type_with_typemod(att->atttypid, - att->atttypmod), - format_type_with_typemod(atttypid, - atttypmod), + errdetail("Returned type %s does not match expected type %s in column \"%s\" (position %d).", + format_type_with_typemod(inatt->atttypid, + inatt->atttypmod), + format_type_with_typemod(outatt->atttypid, + outatt->atttypmod), + NameStr(outatt->attname), noutcols))); attrMap->attnums[i] = (AttrNumber) (j + 1); j++; diff --git a/src/pl/plpgsql/src/expected/plpgsql_record.out b/src/pl/plpgsql/src/expected/plpgsql_record.out index 6974c8f4a44..e5de7143606 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_record.out +++ b/src/pl/plpgsql/src/expected/plpgsql_record.out @@ -28,7 +28,7 @@ create or replace function retc(int) returns two_int8s language plpgsql as $$ begin return row($1,1); end $$; select retc(42); ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 1. +DETAIL: Returned type integer does not match expected type bigint in column "q1" (position 1). CONTEXT: PL/pgSQL function retc(integer) while casting return value to function's return type -- nor extra columns create or replace function retc(int) returns two_int8s language plpgsql as @@ -50,7 +50,7 @@ create or replace function retc(int) returns two_int8s language plpgsql as $$ declare r record; begin r := row($1,1); return r; end $$; select retc(42); ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 1. +DETAIL: Returned type integer does not match expected type bigint in column "q1" (position 1). CONTEXT: PL/pgSQL function retc(integer) while casting return value to function's return type create or replace function retc(int) returns two_int8s language plpgsql as $$ declare r record; begin r := row($1::int8, 1::int8, 42); return r; end $$; @@ -386,7 +386,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column "y" (position 2). CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- same with an intermediate record variable create or replace function returnsrecord(int) returns record language plpgsql as @@ -409,7 +409,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column "y" (position 2). CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- should work the same with a missing column in the actual result value create table has_hole(f1 int, f2 int, f3 int); @@ -434,7 +434,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column "y" (position 2). CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- same with an intermediate record variable create or replace function returnsrecord(int) returns record language plpgsql as @@ -457,7 +457,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column "y" (position 2). CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- check access to a field of an argument declared "record" create function getf1(x record) returns int language plpgsql as @@ -545,6 +545,7 @@ begin return next h; return next row(5,6); return next row(7,8)::has_hole; + return query select 9, 10; end$$; select returnssetofholes(); returnssetofholes @@ -554,7 +555,8 @@ select returnssetofholes(); (3,4) (5,6) (7,8) -(5 rows) + (9,10) +(6 rows) create or replace function returnssetofholes() returns setof has_hole language plpgsql as $$ @@ -575,6 +577,16 @@ select returnssetofholes(); ERROR: returned record type does not match expected record type DETAIL: Number of returned columns (3) does not match expected column count (2). CONTEXT: PL/pgSQL function returnssetofholes() line 3 at RETURN NEXT +create or replace function returnssetofholes() returns setof has_hole language plpgsql as +$$ +begin + return query select 1, 2.0; -- fails +end$$; +select returnssetofholes(); +ERROR: structure of query does not match function result type +DETAIL: Returned type numeric does not match expected type integer in column "f3" (position 2). +CONTEXT: SQL statement "select 1, 2.0" +PL/pgSQL function returnssetofholes() line 3 at RETURN QUERY -- check behavior with changes of a named rowtype create table mutable(f1 int, f2 text); create function sillyaddone(int) returns int language plpgsql as diff --git a/src/pl/plpgsql/src/sql/plpgsql_record.sql b/src/pl/plpgsql/src/sql/plpgsql_record.sql index 96dcc414e92..4fbed38b8bb 100644 --- a/src/pl/plpgsql/src/sql/plpgsql_record.sql +++ b/src/pl/plpgsql/src/sql/plpgsql_record.sql @@ -338,6 +338,7 @@ begin return next h; return next row(5,6); return next row(7,8)::has_hole; + return query select 9, 10; end$$; select returnssetofholes(); @@ -356,6 +357,13 @@ begin end$$; select returnssetofholes(); +create or replace function returnssetofholes() returns setof has_hole language plpgsql as +$$ +begin + return query select 1, 2.0; -- fails +end$$; +select returnssetofholes(); + -- check behavior with changes of a named rowtype create table mutable(f1 int, f2 text); diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out index c5f73fef297..d8ce39dba3c 100644 --- a/src/test/regress/expected/plpgsql.out +++ b/src/test/regress/expected/plpgsql.out @@ -3779,7 +3779,7 @@ end; $$ language plpgsql; select compos(); ERROR: returned record type does not match expected record type -DETAIL: Returned type unknown does not match expected type character varying in column 2. +DETAIL: Returned type unknown does not match expected type character varying in column "y" (position 2). CONTEXT: PL/pgSQL function compos() while casting return value to function's return type -- ... but this does create or replace function compos() returns compostype as $$ Attachments: [text/plain] v4-0001-improve-error-message-in-build_attrmap_by_positio.patch (8.9K, ../../[email protected]/2-v4-0001-improve-error-message-in-build_attrmap_by_positio.patch) download | inline diff: diff --git a/src/backend/access/common/attmap.c b/src/backend/access/common/attmap.c index 4b6cfe05c02..4901ebecef7 100644 --- a/src/backend/access/common/attmap.c +++ b/src/backend/access/common/attmap.c @@ -96,33 +96,31 @@ build_attrmap_by_position(TupleDesc indesc, same = true; for (i = 0; i < n; i++) { - Form_pg_attribute att = TupleDescAttr(outdesc, i); - Oid atttypid; - int32 atttypmod; + Form_pg_attribute outatt = TupleDescAttr(outdesc, i); - if (att->attisdropped) + if (outatt->attisdropped) continue; /* attrMap->attnums[i] is already 0 */ noutcols++; - atttypid = att->atttypid; - atttypmod = att->atttypmod; for (; j < indesc->natts; j++) { - att = TupleDescAttr(indesc, j); - if (att->attisdropped) + Form_pg_attribute inatt = TupleDescAttr(indesc, j); + + if (inatt->attisdropped) continue; nincols++; /* Found matching column, now check type */ - if (atttypid != att->atttypid || - (atttypmod != att->atttypmod && atttypmod >= 0)) + if (outatt->atttypid != inatt->atttypid || + (outatt->atttypmod != inatt->atttypmod && outatt->atttypmod >= 0)) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg_internal("%s", _(msg)), - errdetail("Returned type %s does not match expected type %s in column %d.", - format_type_with_typemod(att->atttypid, - att->atttypmod), - format_type_with_typemod(atttypid, - atttypmod), + errdetail("Returned type %s does not match expected type %s in column \"%s\" (position %d).", + format_type_with_typemod(inatt->atttypid, + inatt->atttypmod), + format_type_with_typemod(outatt->atttypid, + outatt->atttypmod), + NameStr(outatt->attname), noutcols))); attrMap->attnums[i] = (AttrNumber) (j + 1); j++; diff --git a/src/pl/plpgsql/src/expected/plpgsql_record.out b/src/pl/plpgsql/src/expected/plpgsql_record.out index 6974c8f4a44..e5de7143606 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_record.out +++ b/src/pl/plpgsql/src/expected/plpgsql_record.out @@ -28,7 +28,7 @@ create or replace function retc(int) returns two_int8s language plpgsql as $$ begin return row($1,1); end $$; select retc(42); ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 1. +DETAIL: Returned type integer does not match expected type bigint in column "q1" (position 1). CONTEXT: PL/pgSQL function retc(integer) while casting return value to function's return type -- nor extra columns create or replace function retc(int) returns two_int8s language plpgsql as @@ -50,7 +50,7 @@ create or replace function retc(int) returns two_int8s language plpgsql as $$ declare r record; begin r := row($1,1); return r; end $$; select retc(42); ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 1. +DETAIL: Returned type integer does not match expected type bigint in column "q1" (position 1). CONTEXT: PL/pgSQL function retc(integer) while casting return value to function's return type create or replace function retc(int) returns two_int8s language plpgsql as $$ declare r record; begin r := row($1::int8, 1::int8, 42); return r; end $$; @@ -386,7 +386,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column "y" (position 2). CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- same with an intermediate record variable create or replace function returnsrecord(int) returns record language plpgsql as @@ -409,7 +409,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column "y" (position 2). CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- should work the same with a missing column in the actual result value create table has_hole(f1 int, f2 int, f3 int); @@ -434,7 +434,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column "y" (position 2). CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- same with an intermediate record variable create or replace function returnsrecord(int) returns record language plpgsql as @@ -457,7 +457,7 @@ DETAIL: Number of returned columns (2) does not match expected column count (3) CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type select * from returnsrecord(42) as r(x int, y bigint); -- fail ERROR: returned record type does not match expected record type -DETAIL: Returned type integer does not match expected type bigint in column 2. +DETAIL: Returned type integer does not match expected type bigint in column "y" (position 2). CONTEXT: PL/pgSQL function returnsrecord(integer) while casting return value to function's return type -- check access to a field of an argument declared "record" create function getf1(x record) returns int language plpgsql as @@ -545,6 +545,7 @@ begin return next h; return next row(5,6); return next row(7,8)::has_hole; + return query select 9, 10; end$$; select returnssetofholes(); returnssetofholes @@ -554,7 +555,8 @@ select returnssetofholes(); (3,4) (5,6) (7,8) -(5 rows) + (9,10) +(6 rows) create or replace function returnssetofholes() returns setof has_hole language plpgsql as $$ @@ -575,6 +577,16 @@ select returnssetofholes(); ERROR: returned record type does not match expected record type DETAIL: Number of returned columns (3) does not match expected column count (2). CONTEXT: PL/pgSQL function returnssetofholes() line 3 at RETURN NEXT +create or replace function returnssetofholes() returns setof has_hole language plpgsql as +$$ +begin + return query select 1, 2.0; -- fails +end$$; +select returnssetofholes(); +ERROR: structure of query does not match function result type +DETAIL: Returned type numeric does not match expected type integer in column "f3" (position 2). +CONTEXT: SQL statement "select 1, 2.0" +PL/pgSQL function returnssetofholes() line 3 at RETURN QUERY -- check behavior with changes of a named rowtype create table mutable(f1 int, f2 text); create function sillyaddone(int) returns int language plpgsql as diff --git a/src/pl/plpgsql/src/sql/plpgsql_record.sql b/src/pl/plpgsql/src/sql/plpgsql_record.sql index 96dcc414e92..4fbed38b8bb 100644 --- a/src/pl/plpgsql/src/sql/plpgsql_record.sql +++ b/src/pl/plpgsql/src/sql/plpgsql_record.sql @@ -338,6 +338,7 @@ begin return next h; return next row(5,6); return next row(7,8)::has_hole; + return query select 9, 10; end$$; select returnssetofholes(); @@ -356,6 +357,13 @@ begin end$$; select returnssetofholes(); +create or replace function returnssetofholes() returns setof has_hole language plpgsql as +$$ +begin + return query select 1, 2.0; -- fails +end$$; +select returnssetofholes(); + -- check behavior with changes of a named rowtype create table mutable(f1 int, f2 text); diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out index c5f73fef297..d8ce39dba3c 100644 --- a/src/test/regress/expected/plpgsql.out +++ b/src/test/regress/expected/plpgsql.out @@ -3779,7 +3779,7 @@ end; $$ language plpgsql; select compos(); ERROR: returned record type does not match expected record type -DETAIL: Returned type unknown does not match expected type character varying in column 2. +DETAIL: Returned type unknown does not match expected type character varying in column "y" (position 2). CONTEXT: PL/pgSQL function compos() while casting return value to function's return type -- ... but this does create or replace function compos() returns compostype as $$ ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add column name to error description @ 2025-03-07 03:05 Tom Lane <[email protected]> parent: Erik Wienhold <[email protected]> 0 siblings, 2 replies; 12+ messages in thread From: Tom Lane @ 2025-03-07 03:05 UTC (permalink / raw) To: Erik Wienhold <[email protected]>; +Cc: jian he <[email protected]>; Marcos Pegoraro <[email protected]>; PostgreSQL Hackers <[email protected]> Erik Wienhold <[email protected]> writes: > But I don't see the point in keeping variables atttypid and atttypmod > around when those values are now available via outatt. Removing these > two variables makes the code easier to read IMO. Done so in the > attached v4. I think the idea of the original coding was to keep those values in registers in the inner loop rather than re-fetching them each time. But that's probably an unmeasurably microscopic optimization, if real at all (modern compilers might figure it out for themselves). Do others agree Erik's version improves readability? regards, tom lane ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add column name to error description @ 2025-03-07 03:36 Erik Wienhold <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 12+ messages in thread From: Erik Wienhold @ 2025-03-07 03:36 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: jian he <[email protected]>; Marcos Pegoraro <[email protected]>; PostgreSQL Hackers <[email protected]> On 2025-03-07 04:05 +0100, Tom Lane wrote: > Erik Wienhold <[email protected]> writes: > > But I don't see the point in keeping variables atttypid and atttypmod > > around when those values are now available via outatt. Removing these > > two variables makes the code easier to read IMO. Done so in the > > attached v4. > > I think the idea of the original coding was to keep those values in > registers in the inner loop rather than re-fetching them each time. Could be. But the main reason was to hold the output column type as the inner loop repurposed att for the input column. With the separate outatt and inatt this is no longer necessary. -- Erik Wienhold ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add column name to error description @ 2025-03-07 03:57 Tom Lane <[email protected]> parent: Erik Wienhold <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Tom Lane @ 2025-03-07 03:57 UTC (permalink / raw) To: Erik Wienhold <[email protected]>; +Cc: jian he <[email protected]>; Marcos Pegoraro <[email protected]>; PostgreSQL Hackers <[email protected]> Erik Wienhold <[email protected]> writes: > On 2025-03-07 04:05 +0100, Tom Lane wrote: >> I think the idea of the original coding was to keep those values in >> registers in the inner loop rather than re-fetching them each time. > Could be. But the main reason was to hold the output column type as the > inner loop repurposed att for the input column. [ shrug... ] "git blame" traces this code to my commit 12b1b5d83 of 2004-12-11. I admit that my memory might be faulty 20 years later, but that's my impression of what I was thinking. regards, tom lane ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add column name to error description @ 2025-03-07 14:54 jian he <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 2 replies; 12+ messages in thread From: jian he @ 2025-03-07 14:54 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Erik Wienhold <[email protected]>; Marcos Pegoraro <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Mar 7, 2025 at 11:05 AM Tom Lane <[email protected]> wrote: > > Erik Wienhold <[email protected]> writes: > > But I don't see the point in keeping variables atttypid and atttypmod > > around when those values are now available via outatt. Removing these > > two variables makes the code easier to read IMO. Done so in the > > attached v4. > > I think the idea of the original coding was to keep those values in > registers in the inner loop rather than re-fetching them each time. > But that's probably an unmeasurably microscopic optimization, if > real at all (modern compilers might figure it out for themselves). > Do others agree Erik's version improves readability? > i think so. While looking at it, in build_attrmap_by_position I guess errmsg may be better than errmsg_internal since there are around 10 related error messages in src/pl/plpgsql/src/expected/plpgsql_record.out, so it's user visible. but I am confused by the below errmsg_internal comments about translation message infinite error recursion. /* * errmsg_internal --- add a primary error message text to the current error * * This is exactly like errmsg() except that strings passed to errmsg_internal * are not translated, and are customarily left out of the * internationalization message dictionary. This should be used for "can't * happen" cases that are probably not worth spending translation effort on. * We also use this for certain cases where we *must* not try to translate * the message because the translation would fail and result in infinite * error recursion. */ ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add column name to error description @ 2025-03-07 17:19 Tom Lane <[email protected]> parent: jian he <[email protected]> 1 sibling, 0 replies; 12+ messages in thread From: Tom Lane @ 2025-03-07 17:19 UTC (permalink / raw) To: jian he <[email protected]>; +Cc: Erik Wienhold <[email protected]>; Marcos Pegoraro <[email protected]>; PostgreSQL Hackers <[email protected]> jian he <[email protected]> writes: > While looking at it, in build_attrmap_by_position > I guess errmsg may be better than errmsg_internal > since there are around 10 related error messages in > src/pl/plpgsql/src/expected/plpgsql_record.out, > so it's user visible. No, you misunderstand how that works: errmsg_internal("%s", _(msg)), The useful translation happens in the invocation of "_(msg)", that is gettext(msg). Using errmsg_internal instead of errmsg just indicates that there's no point in trying to translate the string "%s". We do it like this rather than simply writing errmsg(msg), because of the risk of sprintf doing something unexpected with '%' characters in the translated message. regards, tom lane ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add column name to error description @ 2025-03-07 18:27 Tom Lane <[email protected]> parent: jian he <[email protected]> 1 sibling, 0 replies; 12+ messages in thread From: Tom Lane @ 2025-03-07 18:27 UTC (permalink / raw) To: jian he <[email protected]>; +Cc: Erik Wienhold <[email protected]>; Marcos Pegoraro <[email protected]>; PostgreSQL Hackers <[email protected]> jian he <[email protected]> writes: > On Fri, Mar 7, 2025 at 11:05 AM Tom Lane <[email protected]> wrote: >> Do others agree Erik's version improves readability? > i think so. Pushed v4, then. regards, tom lane ^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2025-03-07 18:27 UTC | newest] Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-01-18 14:18 [PATCH 1/3] Add bulk insert for foreign tables Tomas Vondra <[email protected]> 2024-03-31 18:57 Re: Add column name to error description Erik Wienhold <[email protected]> 2024-03-31 19:15 ` Re: Add column name to error description Tom Lane <[email protected]> 2024-09-13 08:02 ` Re: Add column name to error description jian he <[email protected]> 2025-03-06 20:56 ` Re: Add column name to error description Tom Lane <[email protected]> 2025-03-07 01:42 ` Re: Add column name to error description Erik Wienhold <[email protected]> 2025-03-07 03:05 ` Re: Add column name to error description Tom Lane <[email protected]> 2025-03-07 03:36 ` Re: Add column name to error description Erik Wienhold <[email protected]> 2025-03-07 03:57 ` Re: Add column name to error description Tom Lane <[email protected]> 2025-03-07 14:54 ` Re: Add column name to error description jian he <[email protected]> 2025-03-07 17:19 ` Re: Add column name to error description Tom Lane <[email protected]> 2025-03-07 18:27 ` Re: Add column name to error description 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