public inbox for [email protected]
help / color / mirror / Atom feedFrom: Mark Dilger <[email protected]>
Subject: [PATCH v1 2/5] Changing SPI_connect and SPI_connect_ext to return void.
Date: Thu, 7 Nov 2019 08:29:07 -0800
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"
view thread (15+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: [PATCH v1 2/5] Changing SPI_connect and SPI_connect_ext to return void.
In-Reply-To: <no-message-id-1883339@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox