restoring user id and SecContext before logging error in ri_PlanCheck
Hi,
I was looking at the code in ri_PlanCheck
of src/backend/utils/adt/ri_triggers.c starting at line 2289.
When qplan is NULL, we log an error. This would skip
calling SetUserIdAndSecContext().
I think the intention of the code should be restoring user id
and SecContext regardless of the outcome from SPI_prepare().
If my understanding is correct, please take a look at the patch.
Thanks
Looking down in ri_PerformCheck(), I see there may be case where error from
SPI_execute_snapshot() would skip restoring UID.
Please look at patch v2 which tried to handle such case.
Thanks
Attachments:
v2-restore-uid-trigger.patchapplication/octet-stream; name=v2-restore-uid-trigger.patchDownload
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 1d503e7e01..57b4a40540 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -2289,12 +2289,12 @@ ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes,
/* Create the plan */
qplan = SPI_prepare(querystr, nargs, argtypes);
- if (qplan == NULL)
- elog(ERROR, "SPI_prepare returned %s for %s", SPI_result_code_string(SPI_result), querystr);
-
/* Restore UID and security context */
SetUserIdAndSecContext(save_userid, save_sec_context);
+ if (qplan == NULL)
+ elog(ERROR, "SPI_prepare returned %s for %s", SPI_result_code_string(SPI_result), querystr);
+
/* Save the plan */
SPI_keepplan(qplan);
ri_HashPreparedPlan(qkey, qplan);
@@ -2405,13 +2405,19 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
SECURITY_NOFORCE_RLS);
/* Finally we can run the query. */
- spi_result = SPI_execute_snapshot(qplan,
- vals, nulls,
- test_snapshot, crosscheck_snapshot,
- false, false, limit);
-
- /* Restore UID and security context */
- SetUserIdAndSecContext(save_userid, save_sec_context);
+ PG_TRY();
+ {
+ spi_result = SPI_execute_snapshot(qplan,
+ vals, nulls,
+ test_snapshot, crosscheck_snapshot,
+ false, false, limit);
+ }
+ PG_FINALLY();
+ {
+ /* Restore UID and security context */
+ SetUserIdAndSecContext(save_userid, save_sec_context);
+ }
+ PG_END_TRY();
/* Check result */
if (spi_result < 0)
On Wed, Nov 02, 2022 at 08:00:58AM -0700, Zhihong Yu wrote:
Looking down in ri_PerformCheck(), I see there may be case where error from
SPI_execute_snapshot() would skip restoring UID.
@@ -2405,13 +2405,19 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
SECURITY_NOFORCE_RLS);/* Finally we can run the query. */ - spi_result = SPI_execute_snapshot(qplan, - vals, nulls, - test_snapshot, crosscheck_snapshot, - false, false, limit); - - /* Restore UID and security context */ - SetUserIdAndSecContext(save_userid, save_sec_context); + PG_TRY(); + { + spi_result = SPI_execute_snapshot(qplan, + vals, nulls, + test_snapshot, crosscheck_snapshot, + false, false, limit); + } + PG_FINALLY(); + { + /* Restore UID and security context */ + SetUserIdAndSecContext(save_userid, save_sec_context); + } + PG_END_TRY();
After an error, AbortSubTransaction() or AbortTransaction() will restore
userid and sec_context. That makes such changes unnecessary.