Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1iSrMo-0005ZZ-Sb for pgsql-hackers@arkaria.postgresql.org; Thu, 07 Nov 2019 23:39:07 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1iSrMm-0000lE-N5 for pgsql-hackers@arkaria.postgresql.org; Thu, 07 Nov 2019 23:39:04 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1iSrMl-0000ez-Tp for pgsql-hackers@lists.postgresql.org; Thu, 07 Nov 2019 23:39:04 +0000 Received: from mail-pg1-x52b.google.com ([2607:f8b0:4864:20::52b]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1iSrMd-0000Oy-PC for pgsql-hackers@postgresql.org; Thu, 07 Nov 2019 23:39:02 +0000 Received: by mail-pg1-x52b.google.com with SMTP id z24so2950350pgu.4 for ; Thu, 07 Nov 2019 15:38:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=to:from:subject:message-id:date:user-agent:mime-version :content-language; bh=6PBKkiyGq3Buc7L87gLCyCmgQ80S5IPFFd0ENTp2qGk=; b=kcEE1jMWGm3qXWWgmY/BSBWbtmhtkqPB2yp4OxSbIUikXrCaoscvzh2S02tQdlXx1s FSDp8ad06oMMpWodtJa7F/cS1a5xbhUCPlrsZZzQ3IoIexXNot587ss/ZAyT4LFyhI0F aZBYffXYSE8LL+qhIXFBxg+A3AnrYSPj4LEhJp28jfDpVjMwuasGGj8VayVNEoczha5W kEJoc/F9SF9G4GhyBNNcQdJemXGrkW160WuHktMLNBZjEHUTmeOsqdETfE0vKR1NSJj9 utfGgeops1Vr8lbcIluKa+xirjZSXpighxi+3DdcaROeh2/7+RW48qJFZZzoTAR1jV/L kDEw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:from:subject:message-id:date:user-agent :mime-version:content-language; bh=6PBKkiyGq3Buc7L87gLCyCmgQ80S5IPFFd0ENTp2qGk=; b=Zo+PGOQlWTRBjYBwDufKQnDO0BESvb2xj3/iN7uRxOXT8mqNyRaMm1MayoVcRBcT+a vHdT7jCg4h7otLbCk8ViH80uSLmnUm1XcWllI1KY/6rvQcyKH1HdBAP4wv1V/9mJkNv+ exuC68R1vIx1FXVNyGI/YqbzIgAb1uqV/O5v7SbudWWliJSdfnP8oIyn4ulxu69vDHpu ec6J3El9d70uYvjwC6NpmyKIi5qydrwRVgCjNoD27zCRNLCOHTreqjCxPTQsLjCbOf9z +VCnN4TA4da9WHJxMPs7zQNV7phvTNwEbpwoW2Gr9JQF6rH81tQ/FwKKAl4rujIB7vu5 oy6Q== X-Gm-Message-State: APjAAAV909ghQaVgtQBcZmFTiUUoVvgkygjJx8EcMALFC8ueQ9I1z/71 5DzhKsZOsdyNozZJW/fvoJ/QlOxv X-Google-Smtp-Source: APXvYqxHOyRnUuxSoRtBlg6+TjUeKba4XF4ZVNOBeDbULrJW9b55I6Z3p5zK/e6/Ca9bKkKGpOfVKw== X-Received: by 2002:a63:1b59:: with SMTP id b25mr7960170pgm.267.1573169933761; Thu, 07 Nov 2019 15:38:53 -0800 (PST) Received: from [192.168.0.17] (24-113-193-150.wavecable.com. [24.113.193.150]) by smtp.gmail.com with ESMTPSA id f185sm3524233pfb.183.2019.11.07.15.38.52 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 07 Nov 2019 15:38:52 -0800 (PST) To: PostgreSQL-development From: Mark Dilger Subject: SPI refactoring Message-ID: Date: Thu, 7 Nov 2019 15:38:51 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.1 MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------EE32DA189D040DF51B4A06B1" Content-Language: en-US List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk This is a multi-part message in MIME format. --------------EE32DA189D040DF51B4A06B1 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Hackers, As discussed with Tom in [1] and again with Pavel and Alvaro in [2], here is a partial WIP refactoring of the SPI interface. The goal is to remove as many of the SPI_ERROR_xxx codes as possible from the interface, replacing them with elog(ERROR), without removing the ability of callers to check meaningful return codes and make their own decisions about what to do next. The crucial point here is that many of the error codes in SPI are "can't happen" or "you made a programmatic mistake" type errors that don't require run time remediation, but rather require fixing the C code and recompiling. Those seem better handled as an elog(ERROR) to avoid the need for tests against such error codes sprinkled throughout the system. One upshot of the refactoring is that some of the SPI functions that previously returned an error code can be changed to return void. Tom suggested a nice way to use macros to provide backward compatibility with older third-party software, and I used his suggestion. I need guidance with SPI_ERROR_ARGUMENT because it is used by functions that don't return an integer error code, but rather return a SPIPlanPtr, such as SPI_prepare. Those functions return NULL and set a global variable named SPI_result to the error code. I'd be happy to just convert this to throwing an error, too, but it is a greater API break than anything implemented in the attached patches so far. How do others feel about it? If more places like this can be converted to use elog(ERROR), it may be possible to convert more functions to return void. [1] https://www.postgresql.org/message-id/13404.1558558354%40sss.pgh.pa.us [2] https://www.postgresql.org/message-id/20191106151112.GA12251%40alvherre.pgsql -- Mark Dilger --------------EE32DA189D040DF51B4A06B1 Content-Type: text/x-patch; name="v1-0001-Deprecating-unused-SPI-error-codes.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="v1-0001-Deprecating-unused-SPI-error-codes.patch" From 113d42772be2c2abd71fd142cde9240522f143d7 Mon Sep 17 00:00:00 2001 From: Mark Dilger Date: Thu, 7 Nov 2019 07:51:06 -0800 Subject: [PATCH v1 1/5] Deprecating unused SPI error codes. The SPI_ERROR_NOOUTFUNC and SPI_ERROR_CONNECT codes, defined in spi.h, were no longer used anywhere in the sources except nominally in spi.c where SPI_result_code_string(int code) was translating them to a cstring for use in error messages. But since the system never returns these error codes, it seems unlikely anybody still needs to be able to convert them to a string. Removing these from spi.c, from the docs, and from a code comment in contrib. Leaving the definition in spi.h for backwards compatibility of third-party applications. --- contrib/spi/refint.c | 2 +- doc/src/sgml/spi.sgml | 14 ++------------ src/backend/executor/spi.c | 4 ---- src/include/executor/spi.h | 4 ++-- 4 files changed, 5 insertions(+), 19 deletions(-) diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c index adf0490f85..00253a63e9 100644 --- a/contrib/spi/refint.c +++ b/contrib/spi/refint.c @@ -392,7 +392,7 @@ check_foreign_key(PG_FUNCTION_ARGS) char *oldval = SPI_getvalue(trigtuple, tupdesc, fnumber); char *newval; - /* this shouldn't happen! SPI_ERROR_NOOUTFUNC ? */ + /* this shouldn't happen! */ if (oldval == NULL) /* internal error */ elog(ERROR, "check_foreign_key: SPI_getvalue returned %s", SPI_result_code_string(SPI_result)); diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml index 9e37fc3a14..d1e6582d96 100644 --- a/doc/src/sgml/spi.sgml +++ b/doc/src/sgml/spi.sgml @@ -128,14 +128,6 @@ int SPI_connect_ext(int options) - - SPI_ERROR_CONNECT - - - on error - - - @@ -3231,12 +3223,10 @@ char * SPI_getvalue(HeapTuple row, TupleDesc r Return Value - Column value, or NULL if the column is null, + Column value, or NULL if the column is null, or colnumber is out of range (SPI_result is set to - SPI_ERROR_NOATTRIBUTE), or no output function is - available (SPI_result is set to - SPI_ERROR_NOOUTFUNC). + SPI_ERROR_NOATTRIBUTE). diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index 2c0ae395ba..2bb49e6919 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -1708,8 +1708,6 @@ SPI_result_code_string(int code) switch (code) { - case SPI_ERROR_CONNECT: - return "SPI_ERROR_CONNECT"; case SPI_ERROR_COPY: return "SPI_ERROR_COPY"; case SPI_ERROR_OPUNKNOWN: @@ -1724,8 +1722,6 @@ SPI_result_code_string(int code) return "SPI_ERROR_TRANSACTION"; case SPI_ERROR_NOATTRIBUTE: return "SPI_ERROR_NOATTRIBUTE"; - case SPI_ERROR_NOOUTFUNC: - return "SPI_ERROR_NOOUTFUNC"; case SPI_ERROR_TYPUNKNOWN: return "SPI_ERROR_TYPUNKNOWN"; case SPI_ERROR_REL_DUPLICATE: diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h index ad69a5ce43..a2ea6dda5f 100644 --- a/src/include/executor/spi.h +++ b/src/include/executor/spi.h @@ -36,7 +36,7 @@ typedef struct SPITupleTable /* Plans are opaque structs for standard users of SPI */ typedef struct _SPI_plan *SPIPlanPtr; -#define SPI_ERROR_CONNECT (-1) +#define SPI_ERROR_CONNECT (-1) /* not used anymore */ #define SPI_ERROR_COPY (-2) #define SPI_ERROR_OPUNKNOWN (-3) #define SPI_ERROR_UNCONNECTED (-4) @@ -45,7 +45,7 @@ typedef struct _SPI_plan *SPIPlanPtr; #define SPI_ERROR_PARAM (-7) #define SPI_ERROR_TRANSACTION (-8) #define SPI_ERROR_NOATTRIBUTE (-9) -#define SPI_ERROR_NOOUTFUNC (-10) +#define SPI_ERROR_NOOUTFUNC (-10) /* not used anymore */ #define SPI_ERROR_TYPUNKNOWN (-11) #define SPI_ERROR_REL_DUPLICATE (-12) #define SPI_ERROR_REL_NOT_FOUND (-13) base-commit: effa40281bb1f6c71e81c980f86852ab3be603c3 -- 2.20.1 --------------EE32DA189D040DF51B4A06B1 Content-Type: text/x-patch; name="v1-0002-Changing-SPI_connect-and-SPI_connect_ext-to-retur.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="v1-0002-Changing-SPI_connect-and-SPI_connect_ext-to-retur.pa"; filename*1="tch" From ac1036a40fd15745567ab3f5a4e90930a2f9a7b4 Mon Sep 17 00:00:00 2001 From: Mark Dilger Date: Thu, 7 Nov 2019 08:29:07 -0800 Subject: [PATCH v1 2/5] Changing SPI_connect and SPI_connect_ext to return void. SPI_connect and SPI_connect_ext were always returning SPI_OK_CONNECT if they returned at all. Numerous places in the code were needlessly checking the return value of these two functions. I am changing the functions to return void, and including two backward compatibility macros to avoid breaking third-party code which either does not get updated, or which needs to work across different versions. This patch is by me, but some of these ideas and a sketch of the backward compatibility macros provided by Tom Lane: https://www.postgresql.org/message-id/14823.1558630129%40sss.pgh.pa.us --- contrib/dblink/dblink.c | 4 +--- contrib/spi/refint.c | 8 ++----- contrib/tablefunc/tablefunc.c | 17 ++++--------- src/backend/commands/matview.c | 3 +-- src/backend/executor/spi.c | 11 +++++---- src/backend/utils/adt/ri_triggers.c | 24 +++++++------------ src/backend/utils/adt/ruleutils.c | 6 ++--- src/include/executor/spi.h | 18 ++++++++++++-- src/pl/plpgsql/src/pl_handler.c | 9 +++---- .../modules/test_predtest/test_predtest.c | 3 +-- src/test/regress/regress.c | 3 +-- 11 files changed, 45 insertions(+), 61 deletions(-) diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c index 7e225589a9..5587c57bf9 100644 --- a/contrib/dblink/dblink.c +++ b/contrib/dblink/dblink.c @@ -2379,9 +2379,7 @@ get_tuple_of_interest(Relation rel, int *pkattnums, int pknumatts, char **src_pk /* * Connect to SPI manager */ - if ((ret = SPI_connect()) < 0) - /* internal error */ - elog(ERROR, "SPI connect failure - returned %d", ret); + SPI_connect(); initStringInfo(&buf); diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c index 00253a63e9..a71ef931ec 100644 --- a/contrib/spi/refint.c +++ b/contrib/spi/refint.c @@ -107,9 +107,7 @@ check_primary_key(PG_FUNCTION_ARGS) tupdesc = rel->rd_att; /* Connect to SPI manager */ - if ((ret = SPI_connect()) < 0) - /* internal error */ - elog(ERROR, "check_primary_key: SPI_connect returned %d", ret); + SPI_connect(); /* * We use SPI plan preparation feature, so allocate space to place key @@ -326,9 +324,7 @@ check_foreign_key(PG_FUNCTION_ARGS) tupdesc = rel->rd_att; /* Connect to SPI manager */ - if ((ret = SPI_connect()) < 0) - /* internal error */ - elog(ERROR, "check_foreign_key: SPI_connect returned %d", ret); + SPI_connect(); /* * We use SPI plan preparation feature, so allocate space to place key diff --git a/contrib/tablefunc/tablefunc.c b/contrib/tablefunc/tablefunc.c index 256d52fc62..8866bbf0ed 100644 --- a/contrib/tablefunc/tablefunc.c +++ b/contrib/tablefunc/tablefunc.c @@ -379,9 +379,7 @@ crosstab(PG_FUNCTION_ARGS) per_query_ctx = rsinfo->econtext->ecxt_per_query_memory; /* Connect to SPI manager */ - if ((ret = SPI_connect()) < 0) - /* internal error */ - elog(ERROR, "crosstab: SPI_connect returned %d", ret); + SPI_connect(); /* Retrieve the desired rows */ ret = SPI_execute(sql, true, 0); @@ -726,9 +724,7 @@ load_categories_hash(char *cats_sql, MemoryContext per_query_ctx) HASH_ELEM | HASH_CONTEXT); /* Connect to SPI manager */ - if ((ret = SPI_connect()) < 0) - /* internal error */ - elog(ERROR, "load_categories_hash: SPI_connect returned %d", ret); + SPI_connect(); /* Retrieve the category name rows */ ret = SPI_execute(cats_sql, true, 0); @@ -805,9 +801,7 @@ get_crosstab_tuplestore(char *sql, tupstore = tuplestore_begin_heap(randomAccess, false, work_mem); /* Connect to SPI manager */ - if ((ret = SPI_connect()) < 0) - /* internal error */ - elog(ERROR, "get_crosstab_tuplestore: SPI_connect returned %d", ret); + SPI_connect(); /* Now retrieve the crosstab source rows */ ret = SPI_execute(sql, true, 0); @@ -1157,15 +1151,12 @@ connectby(char *relname, AttInMetadata *attinmeta) { Tuplestorestate *tupstore = NULL; - int ret; MemoryContext oldcontext; int serial = 1; /* Connect to SPI manager */ - if ((ret = SPI_connect()) < 0) - /* internal error */ - elog(ERROR, "connectby: SPI_connect returned %d", ret); + SPI_connect(); /* switch to longer term context to create the tuple store */ oldcontext = MemoryContextSwitchTo(per_query_ctx); diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c index 537d0e8cef..b11c98a093 100644 --- a/src/backend/commands/matview.c +++ b/src/backend/commands/matview.c @@ -605,8 +605,7 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner, relnatts = RelationGetNumberOfAttributes(matviewRel); /* Open SPI context. */ - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); + SPI_connect(); /* Analyze the temp table with the new contents. */ appendStringInfo(&querybuf, "ANALYZE %s", tempname); diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index 2bb49e6919..c3943ebbb9 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -82,16 +82,19 @@ static MemoryContext _SPI_execmem(void); static MemoryContext _SPI_procmem(void); static bool _SPI_checktuples(void); +/* Defend against BACKWARDS_COMPATIBLE_SPI_CALLS wrapper macros */ +#undef SPI_connect +#undef SPI_connect_ext /* =================== interface functions =================== */ -int +void SPI_connect(void) { - return SPI_connect_ext(0); + SPI_connect_ext(0); } -int +void SPI_connect_ext(int options) { int newdepth; @@ -168,8 +171,6 @@ SPI_connect_ext(int options) SPI_processed = 0; SPI_tuptable = NULL; SPI_result = 0; - - return SPI_OK_CONNECT; } int diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index caf27f8bcb..1462f29e25 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -334,8 +334,7 @@ RI_FKey_check(TriggerData *trigdata) break; } - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); + SPI_connect(); /* Fetch or prepare a saved plan for the real check */ ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_CHECK_LOOKUPPK); @@ -459,8 +458,7 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel, /* Only called for non-null rows */ Assert(ri_NullCheck(RelationGetDescr(pk_rel), oldslot, riinfo, true) == RI_KEYS_NONE_NULL); - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); + SPI_connect(); /* * Fetch or prepare a saved plan for checking PK table with values coming @@ -646,8 +644,7 @@ ri_restrict(TriggerData *trigdata, bool is_no_action) return PointerGetDatum(NULL); } - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); + SPI_connect(); /* * Fetch or prepare a saved plan for the restrict lookup (it's the same @@ -756,8 +753,7 @@ RI_FKey_cascade_del(PG_FUNCTION_ARGS) pk_rel = trigdata->tg_relation; oldslot = trigdata->tg_trigslot; - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); + SPI_connect(); /* Fetch or prepare a saved plan for the cascaded delete */ ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_CASCADE_DEL_DODELETE); @@ -865,8 +861,7 @@ RI_FKey_cascade_upd(PG_FUNCTION_ARGS) newslot = trigdata->tg_newslot; oldslot = trigdata->tg_trigslot; - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); + SPI_connect(); /* Fetch or prepare a saved plan for the cascaded update */ ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_CASCADE_UPD_DOUPDATE); @@ -1040,8 +1035,7 @@ ri_set(TriggerData *trigdata, bool is_set_null) pk_rel = trigdata->tg_relation; oldslot = trigdata->tg_trigslot; - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); + SPI_connect(); /* * Fetch or prepare a saved plan for the set null/default operation (it's @@ -1464,8 +1458,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) PGC_USERSET, PGC_S_SESSION, GUC_ACTION_SAVE, true, 0, false); - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); + SPI_connect(); /* * Generate the plan. We don't need to cache it, and there are no @@ -1699,8 +1692,7 @@ RI_PartitionRemove_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) PGC_USERSET, PGC_S_SESSION, GUC_ACTION_SAVE, true, 0, false); - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); + SPI_connect(); /* * Generate the plan. We don't need to cache it, and there are no diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 3e64390d81..acf3f804af 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -534,8 +534,7 @@ pg_get_ruledef_worker(Oid ruleoid, int prettyFlags) /* * Connect to SPI manager */ - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); + SPI_connect(); /* * On the first call prepare the plan to lookup pg_rewrite. We read @@ -727,8 +726,7 @@ pg_get_viewdef_worker(Oid viewoid, int prettyFlags, int wrapColumn) /* * Connect to SPI manager */ - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); + SPI_connect(); /* * On the first call prepare the plan to lookup pg_rewrite. We read diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h index a2ea6dda5f..2d505610ff 100644 --- a/src/include/executor/spi.h +++ b/src/include/executor/spi.h @@ -81,8 +81,8 @@ extern PGDLLIMPORT uint64 SPI_processed; extern PGDLLIMPORT SPITupleTable *SPI_tuptable; extern PGDLLIMPORT int SPI_result; -extern int SPI_connect(void); -extern int SPI_connect_ext(int options); +extern void SPI_connect(void); +extern void SPI_connect_ext(int options); extern int SPI_finish(void); extern int SPI_execute(const char *src, bool read_only, long tcount); extern int SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls, @@ -172,4 +172,18 @@ extern void AtEOXact_SPI(bool isCommit); extern void AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid); extern bool SPI_inside_nonatomic_context(void); +/* + * Backward compatibility for third-party code which expects the older SPI + * function prototypes returning int rather than void. + * + * This switch should only be defined when compiling legacy third-party code + * which expects an integer return value, otherwise, at least under some + * compilers, compilation may draw warnings about the right-hand side value + * being ignored (-Wunused-value). + */ +#ifdef BACKWARDS_COMPATIBLE_SPI_CALLS +#define SPI_connect() (SPI_connect(), SPI_OK_CONNECT) +#define SPI_connect_ext(o) (SPI_connect_ext(o), SPI_OK_CONNECT) +#endif + #endif /* SPI_H */ diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c index 1b592c8a52..3e41f36f61 100644 --- a/src/pl/plpgsql/src/pl_handler.c +++ b/src/pl/plpgsql/src/pl_handler.c @@ -234,8 +234,7 @@ plpgsql_call_handler(PG_FUNCTION_ARGS) /* * Connect to SPI manager */ - if ((rc = SPI_connect_ext(nonatomic ? SPI_OPT_NONATOMIC : 0)) != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed: %s", SPI_result_code_string(rc)); + SPI_connect_ext(nonatomic ? SPI_OPT_NONATOMIC : 0); /* Find or compile the function */ func = plpgsql_compile(fcinfo, false); @@ -303,8 +302,7 @@ plpgsql_inline_handler(PG_FUNCTION_ARGS) /* * Connect to SPI manager */ - if ((rc = SPI_connect_ext(codeblock->atomic ? 0 : SPI_OPT_NONATOMIC)) != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed: %s", SPI_result_code_string(rc)); + SPI_connect_ext(codeblock->atomic ? 0 : SPI_OPT_NONATOMIC); /* Compile the anonymous code block */ func = plpgsql_compile_inline(codeblock->source_text); @@ -468,8 +466,7 @@ plpgsql_validator(PG_FUNCTION_ARGS) /* * Connect to SPI manager (is this needed for compilation?) */ - if ((rc = SPI_connect()) != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed: %s", SPI_result_code_string(rc)); + SPI_connect(); /* * Set up a fake fcinfo with just enough info to satisfy diff --git a/src/test/modules/test_predtest/test_predtest.c b/src/test/modules/test_predtest/test_predtest.c index 10cd4f9ae5..d612e29198 100644 --- a/src/test/modules/test_predtest/test_predtest.c +++ b/src/test/modules/test_predtest/test_predtest.c @@ -54,8 +54,7 @@ test_predtest(PG_FUNCTION_ARGS) int i; /* We use SPI to parse, plan, and execute the test query */ - if (SPI_connect() != SPI_OK_CONNECT) - elog(ERROR, "SPI_connect failed"); + SPI_connect(); /* * First, plan and execute the query, and inspect the results. To the diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index 2f66d76ab3..7d8086cd2c 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -368,8 +368,7 @@ ttdummy(PG_FUNCTION_ARGS) newoff = Int32GetDatum((int32) DatumGetInt64(newoff)); /* Connect to SPI manager */ - if ((ret = SPI_connect()) < 0) - elog(ERROR, "ttdummy (%s): SPI_connect returned %d", relname, ret); + SPI_connect(); /* Fetch tuple values and nulls */ cvals = (Datum *) palloc(natts * sizeof(Datum)); -- 2.20.1 --------------EE32DA189D040DF51B4A06B1 Content-Type: text/x-patch; name="v1-0003-Refactoring-SPI-trigger-related-errors.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="v1-0003-Refactoring-SPI-trigger-related-errors.patch" From e89cb24c2f98cfd706fc053ac60703d374a3e83e Mon Sep 17 00:00:00 2001 From: Mark Dilger Date: Thu, 7 Nov 2019 10:10:17 -0800 Subject: [PATCH v1 3/5] Refactoring SPI trigger related errors. Deprecating SPI_ERROR_REL_DUPLICATE and SPI_ERROR_REL_NOT_FOUND error codes in favor of throwing an error. These errors should not be reachable from within the core code. Specifically, only the function SPI_register_trigger_data is calling SPI_register_relation, which was the only function returning SPI_ERROR_REL_DUPLICATE. The call to SPI_register_relation is for the purpose of registering the NEW and OLD trigger relations, and given how the code is written, it cannot make the mistake of registering them more than once. Hence, SPI_ERROR_REL_DUPLICATE was unreachable by a user, except through poorly written third-party C code that had some other usage pattern. SPI_unregister_relation was the only function returning SPI_ERROR_REL_NOT_FOUND. No calls are made to SPI_unregister_relation from core, so once again, it should be unreachable by a user except through poorly written third-party C code. --- doc/src/sgml/spi.sgml | 27 --------------------------- src/backend/executor/spi.c | 8 ++------ src/include/executor/spi.h | 4 ++-- 3 files changed, 4 insertions(+), 35 deletions(-) diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml index d1e6582d96..abe1475256 100644 --- a/doc/src/sgml/spi.sgml +++ b/doc/src/sgml/spi.sgml @@ -2756,15 +2756,6 @@ int SPI_register_relation(EphemeralNamedRelation enr) - - SPI_ERROR_REL_DUPLICATE - - - if the name specified in the name field of - enr is already registered for this connection - - - @@ -2861,15 +2852,6 @@ int SPI_unregister_relation(const char * name) - - SPI_ERROR_REL_NOT_FOUND - - - if name is not found in the registry for the - current connection - - - @@ -2976,15 +2958,6 @@ int SPI_register_trigger_data(TriggerData *tdata) - - SPI_ERROR_REL_DUPLICATE - - - if the name of any trigger data transient relation is already - registered for this connection - - - diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index c3943ebbb9..7d227a11d2 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -1725,10 +1725,6 @@ SPI_result_code_string(int code) return "SPI_ERROR_NOATTRIBUTE"; case SPI_ERROR_TYPUNKNOWN: return "SPI_ERROR_TYPUNKNOWN"; - case SPI_ERROR_REL_DUPLICATE: - return "SPI_ERROR_REL_DUPLICATE"; - case SPI_ERROR_REL_NOT_FOUND: - return "SPI_ERROR_REL_NOT_FOUND"; case SPI_OK_CONNECT: return "SPI_OK_CONNECT"; case SPI_OK_FINISH: @@ -2875,7 +2871,7 @@ SPI_register_relation(EphemeralNamedRelation enr) match = _SPI_find_ENR_by_name(enr->md.name); if (match) - res = SPI_ERROR_REL_DUPLICATE; + elog(ERROR, "duplicate relation registration"); else { if (_SPI_current->queryEnv == NULL) @@ -2914,7 +2910,7 @@ SPI_unregister_relation(const char *name) res = SPI_OK_REL_UNREGISTER; } else - res = SPI_ERROR_REL_NOT_FOUND; + elog(ERROR, "unknown relation unregistration"); _SPI_end_call(false); diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h index 2d505610ff..7a43b9d21e 100644 --- a/src/include/executor/spi.h +++ b/src/include/executor/spi.h @@ -47,8 +47,8 @@ typedef struct _SPI_plan *SPIPlanPtr; #define SPI_ERROR_NOATTRIBUTE (-9) #define SPI_ERROR_NOOUTFUNC (-10) /* not used anymore */ #define SPI_ERROR_TYPUNKNOWN (-11) -#define SPI_ERROR_REL_DUPLICATE (-12) -#define SPI_ERROR_REL_NOT_FOUND (-13) +#define SPI_ERROR_REL_DUPLICATE (-12) /* not used anymore */ +#define SPI_ERROR_REL_NOT_FOUND (-13) /* not used anymore */ #define SPI_OK_CONNECT 1 #define SPI_OK_FINISH 2 -- 2.20.1 --------------EE32DA189D040DF51B4A06B1 Content-Type: text/x-patch; name="v1-0004-Refactoring-SPI-execute-related-errors.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="v1-0004-Refactoring-SPI-execute-related-errors.patch" From 6a3a28847b7089d7688d8a906b0046f57204bf83 Mon Sep 17 00:00:00 2001 From: Mark Dilger Date: Thu, 7 Nov 2019 12:20:46 -0800 Subject: [PATCH v1 4/5] Refactoring SPI execute related errors. Deprecating SPI_ERROR_PARAM in favor of throwing an error. This error code was only being returned when a caller of SPI_execute_plan, SPI_execute_snapshot, or SPI_execute_with_args passed in a plan which has nargs > 0 but then negligently passed in NULL for the Values and/or argtypes. This would be a C coding error, not a state of affairs that a user could trigger. --- doc/src/sgml/spi.sgml | 9 --------- src/backend/executor/spi.c | 8 +++----- src/include/executor/spi.h | 2 +- 3 files changed, 4 insertions(+), 15 deletions(-) diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml index abe1475256..cca91f6ef6 100644 --- a/doc/src/sgml/spi.sgml +++ b/doc/src/sgml/spi.sgml @@ -1439,15 +1439,6 @@ int SPI_execute_plan(SPIPlanPtr plan, Datum * - - SPI_ERROR_PARAM - - - if values is NULL and - plan was prepared with some parameters - - - diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index 7d227a11d2..579e80a6a7 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -538,7 +538,7 @@ SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls, return SPI_ERROR_ARGUMENT; if (plan->nargs > 0 && Values == NULL) - return SPI_ERROR_PARAM; + elog(ERROR, "SPI_execute_plan passed invalid parameters"); res = _SPI_begin_call(true); if (res < 0) @@ -608,7 +608,7 @@ SPI_execute_snapshot(SPIPlanPtr plan, return SPI_ERROR_ARGUMENT; if (plan->nargs > 0 && Values == NULL) - return SPI_ERROR_PARAM; + elog(ERROR, "SPI_execute_snapshot passed invalid parameters"); res = _SPI_begin_call(true); if (res < 0) @@ -644,7 +644,7 @@ SPI_execute_with_args(const char *src, return SPI_ERROR_ARGUMENT; if (nargs > 0 && (argtypes == NULL || Values == NULL)) - return SPI_ERROR_PARAM; + elog(ERROR, "SPI_execute_with_args passed invalid parameters"); res = _SPI_begin_call(true); if (res < 0) @@ -1717,8 +1717,6 @@ SPI_result_code_string(int code) return "SPI_ERROR_UNCONNECTED"; case SPI_ERROR_ARGUMENT: return "SPI_ERROR_ARGUMENT"; - case SPI_ERROR_PARAM: - return "SPI_ERROR_PARAM"; case SPI_ERROR_TRANSACTION: return "SPI_ERROR_TRANSACTION"; case SPI_ERROR_NOATTRIBUTE: diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h index 7a43b9d21e..575978737f 100644 --- a/src/include/executor/spi.h +++ b/src/include/executor/spi.h @@ -42,7 +42,7 @@ typedef struct _SPI_plan *SPIPlanPtr; #define SPI_ERROR_UNCONNECTED (-4) #define SPI_ERROR_CURSOR (-5) /* not used anymore */ #define SPI_ERROR_ARGUMENT (-6) -#define SPI_ERROR_PARAM (-7) +#define SPI_ERROR_PARAM (-7) /* not used anymore */ #define SPI_ERROR_TRANSACTION (-8) #define SPI_ERROR_NOATTRIBUTE (-9) #define SPI_ERROR_NOOUTFUNC (-10) /* not used anymore */ -- 2.20.1 --------------EE32DA189D040DF51B4A06B1 Content-Type: text/x-patch; name="v1-0005-Refactoring-SPI-related-errors.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="v1-0005-Refactoring-SPI-related-errors.patch" From 15aa7a3ac17a13b35bcf335f6f8230ea12eda598 Mon Sep 17 00:00:00 2001 From: Mark Dilger Date: Thu, 7 Nov 2019 14:38:06 -0800 Subject: [PATCH v1 5/5] Refactoring SPI related errors. Deprecating SPI_ERROR_OPUNKNOWN error code in favor of throwing an exception instead. This code was only returned by SPI_execute and SPI_execute_plan in the event that they failed to handle all relevant cases in a C switch statement. --- doc/src/sgml/spi.sgml | 9 --------- src/backend/executor/spi.c | 9 +++++---- src/include/executor/spi.h | 2 +- 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml index cca91f6ef6..af822cfbe7 100644 --- a/doc/src/sgml/spi.sgml +++ b/doc/src/sgml/spi.sgml @@ -520,15 +520,6 @@ typedef struct SPITupleTable - - SPI_ERROR_OPUNKNOWN - - - if the command type is unknown (shouldn't happen) - - - - SPI_ERROR_UNCONNECTED diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index 579e80a6a7..f2a90f0d0d 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -1711,8 +1711,6 @@ SPI_result_code_string(int code) { case SPI_ERROR_COPY: return "SPI_ERROR_COPY"; - case SPI_ERROR_OPUNKNOWN: - return "SPI_ERROR_OPUNKNOWN"; case SPI_ERROR_UNCONNECTED: return "SPI_ERROR_UNCONNECTED"; case SPI_ERROR_ARGUMENT: @@ -2459,7 +2457,7 @@ _SPI_convert_params(int nargs, Oid *argtypes, static int _SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, uint64 tcount) { - int operation = queryDesc->operation; + CmdType operation = queryDesc->operation; int eflags; int res; @@ -2492,8 +2490,11 @@ _SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, uint64 tcount) else res = SPI_OK_UPDATE; break; + case CMD_UNKNOWN: + case CMD_UTILITY: + case CMD_NOTHING: default: - return SPI_ERROR_OPUNKNOWN; + elog(ERROR, "Unsupported command type %d encountered while executing query", (int)operation); } #ifdef SPI_EXECUTOR_STATS diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h index 575978737f..ac34bb3b5d 100644 --- a/src/include/executor/spi.h +++ b/src/include/executor/spi.h @@ -38,7 +38,7 @@ typedef struct _SPI_plan *SPIPlanPtr; #define SPI_ERROR_CONNECT (-1) /* not used anymore */ #define SPI_ERROR_COPY (-2) -#define SPI_ERROR_OPUNKNOWN (-3) +#define SPI_ERROR_OPUNKNOWN (-3) /* not used anymore */ #define SPI_ERROR_UNCONNECTED (-4) #define SPI_ERROR_CURSOR (-5) /* not used anymore */ #define SPI_ERROR_ARGUMENT (-6) -- 2.20.1 --------------EE32DA189D040DF51B4A06B1--