Merge pull request #59 from pgsql-io/py-3.11

mfenniak · web-flow · commit aebb975d6081 · 2024-05-29T12:05:33.000-06:00
Python 3.11 support
diff --git a/CHANGELOG b/CHANGELOG
@@ -11,6 +11,8 @@ done so far:
    - Add support for bulk_insert FDWs on PG14+ (https://github.com/pgsql-io/multicorn2/pull/45)
    - PG16: Fix compatibility issues w/ log_to_postgres and join query planning in PostgreSQL 16 (https://github.com/pgsql-io/multicorn2/pull/51)
    - Fix crashes in EXPLAIN with complex quals (https://github.com/pgsql-io/multicorn2/pull/54)
+   - Support Python 3.11 (https://github.com/pgsql-io/multicorn2/pull/59)
+   - Behavior change: When log_to_postgres with level ERROR or FATAL is invoked, a specialized Python exception will be thrown and the stack unwound, allowing `catch` and `finally` blocks, and other things like context handler exits, to be invoked in the FDW.  (https://github.com/pgsql-io/multicorn2/pull/59)
 
 to do:
    - confirm support for Python 3.11 & 3.12
diff --git a/README.md b/README.md
@@ -2,7 +2,7 @@
 Multicorn2
 ==========
 
-Multicorn Python3 Foreign Data Wrapper (FDW) for Postgresql.  Tested on Linux w/ Python 3.9-3.12 & Postgres 12-17.
+Multicorn Python3 Foreign Data Wrapper (FDW) for Postgresql.  Tested on Linux w/ Python 3.9-3.11 & Postgres 12-17.
 
 The Multicorn Foreign Data Wrapper allows you to fetch foreign data in Python in your PostgreSQL server.
 
@@ -151,7 +151,7 @@ nix build .#testSuites.test_pg12_py39
 ```
 
 **Known issues:**
-- The tests cover only the supported range of Python & PostgreSQL combinations; in particular, Python releases 3.11 and later are disabled due to failures that have not been addressed.
+- The tests cover only the supported range of Python & PostgreSQL combinations; in particular, Python releases 3.12 and later are disabled due to failures that have not been addressed.
 
 ### Adding new Python or PostgreSQL versions to the test suite
 
diff --git a/flake.nix b/flake.nix
@@ -24,7 +24,7 @@
       testPythonVersions = with pkgs; [
         python39
         python310
-        # python311 # tests are currently broken
+        python311
         # python312 # tests are currently broken
         # python313 # tests are currently broken
       ];
@@ -127,6 +127,7 @@
           ./Makefile
           ./test-3.9
           ./test-3.10
+          ./test-3.11
           ./test-common
         ];
         unpackPhase = ''
diff --git a/python/multicorn/utils.py b/python/multicorn/utils.py
@@ -20,9 +20,25 @@ def _log_to_postgres(message, level=0, hint=None, detail=None):
 }
 
 
+class MulticornException(Exception):
+    def __init__(self, message, code, hint, detail):
+        self._is_multicorn_exception = True
+        self.message = message
+        self.code = code
+        self.hint = hint
+        self.detail = detail
+
+
 def log_to_postgres(message, level=INFO, hint=None, detail=None):
     code = REPORT_CODES.get(level, None)
     if code is None:
         raise KeyError("Not a valid log level")
-    _log_to_postgres(message, code, hint=hint, detail=detail)
-
+    if level in (ERROR, CRITICAL):
+        # if we sent an ERROR or FATAL(=CRITICAL) message to _log_to_postgres, we would trigger the PostgreSQL C-level
+        # exception handling, which would prevent us from cleanly exiting whatever Python context we're currently in.
+        # To avoid this, these log levels are replaced with exceptions which are bubbled back to Multicorn's entry
+        # points, and those exceptions are translated into appropriate logging after we exit the method at the top of
+        # the multicorn stack.
+        raise MulticornException(message, code, hint, detail)
+    else:
+        _log_to_postgres(message, code, hint=hint, detail=detail)
diff --git a/src/errors.c b/src/errors.c
@@ -17,6 +17,7 @@ void reportException(PyObject *pErrType,
 				PyObject *pErrValue,
 				PyObject *pErrTraceback);
 
+void reportMulticornException(PyObject *pErrValue);
 
 PGDLLEXPORT void
 errorCheck()
@@ -28,7 +29,22 @@ errorCheck()
 	PyErr_Fetch(&pErrType, &pErrValue, &pErrTraceback);
 	if (pErrType)
 	{
-		reportException(pErrType, pErrValue, pErrTraceback);
+		// if the error value has a property _is_multicorn_exception and a boolean value True, then we don't report the
+		// error as a generic exception with a stack trace -- instead we just take the message, code(severity), hint,
+		// and detail, and log it to Postgres.  These exceptions are generated in utils.py to intercept ERROR/FATAL log
+		// messages.  So, first detect whether that's the case, and call a new reporting function...
+		PyObject *is_multicorn_exception = PyObject_GetAttrString(pErrValue, "_is_multicorn_exception");
+		if (is_multicorn_exception != NULL && PyObject_IsTrue(is_multicorn_exception))
+		{
+			Py_DECREF(is_multicorn_exception);
+			Py_DECREF(pErrType);
+			Py_DECREF(pErrTraceback);
+			reportMulticornException(pErrValue);
+		}
+		else
+		{
+			reportException(pErrType, pErrValue, pErrTraceback);
+		}
 	}
 }
 
@@ -92,3 +108,64 @@ reportException(PyObject *pErrType, PyObject *pErrValue, PyObject *pErrTraceback
 		errfinish(0);
 #endif
 }
+
+void reportMulticornException(PyObject* pErrValue)
+{
+	int severity;
+	PyObject *message = PyObject_GetAttrString(pErrValue, "message");
+	PyObject *hint = PyObject_GetAttrString(pErrValue, "hint");
+	PyObject *detail = PyObject_GetAttrString(pErrValue, "detail");
+	PyObject *code = PyObject_GetAttrString(pErrValue, "code");
+	int level = PyLong_AsLong(code);
+
+	// Matches up with REPORT_CODES in utils.py
+	switch (level)
+	{
+		case 3:
+			severity = ERROR;
+			break;
+		default:
+		case 4:
+			severity = FATAL;
+			break;
+	}
+
+	PG_TRY();
+	{
+
+	#if PG_VERSION_NUM >= 130000
+		if (errstart(severity, TEXTDOMAIN))
+	#else
+		if (errstart(severity, __FILE__, __LINE__, PG_FUNCNAME_MACRO, TEXTDOMAIN))
+	#endif
+		{
+			errmsg("%s", PyString_AsString(message));
+			if (hint != NULL && hint != Py_None)
+			{
+				char* hintstr = PyString_AsString(hint);
+				errhint("%s", hintstr);
+			}
+			if (detail != NULL && detail != Py_None)
+			{
+				char* detailstr = PyString_AsString(detail);
+				errdetail("%s", detailstr);
+			}
+	#if PG_VERSION_NUM >= 130000
+			errfinish(__FILE__, __LINE__, PG_FUNCNAME_MACRO);
+	#else
+			errfinish(0);
+	#endif
+		}
+
+	}
+	PG_CATCH();
+	{
+		Py_DECREF(message);
+		Py_DECREF(hint);
+		Py_DECREF(detail);
+		Py_DECREF(code);
+		Py_DECREF(pErrValue);
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
+}
diff --git a/src/python.c b/src/python.c
@@ -667,7 +667,6 @@ getCacheEntry(Oid foreigntableid)
 		MemoryContextDelete(tempContext);
 	}
 	RelationClose(rel);
-	Py_INCREF(entry->value);
 
 	/*
 	 * Start a new transaction or subtransaction if needed.
@@ -685,7 +684,9 @@ getCacheEntry(Oid foreigntableid)
 PyObject *
 getInstance(Oid foreigntableid)
 {
-	return getCacheEntry(foreigntableid)->value;
+	PyObject* retval = getCacheEntry(foreigntableid)->value;
+	Py_INCREF(retval);
+	return retval;
 }
 
 
diff --git a/test-3.11 b/test-3.11
@@ -0,0 +1 @@
+test-3.9
diff --git a/test-3.9/expected/write_test_3.out b/test-3.9/expected/write_test_3.out
@@ -0,0 +1,190 @@
+CREATE EXTENSION multicorn;
+CREATE server multicorn_srv foreign data wrapper multicorn options (
+    wrapper 'multicorn.testfdw.TestForeignDataWrapper'
+);
+CREATE user mapping FOR current_user server multicorn_srv options (usermapping 'test');
+CREATE foreign table testmulticorn (
+    test1 character varying,
+    test2 character varying
+) server multicorn_srv options (
+    option1 'option1',
+    test_type 'nowrite',
+    tx_hook 'true'
+);
+insert into testmulticorn(test1, test2) VALUES ('test', 'test2');
+NOTICE:  [('option1', 'option1'), ('test_type', 'nowrite'), ('tx_hook', 'true'), ('usermapping', 'test')]
+NOTICE:  [('test1', 'character varying'), ('test2', 'character varying')]
+NOTICE:  BEGIN
+ERROR:  Error in python: NotImplementedError
+DETAIL:  This FDW does not support the writable API
+NOTICE:  ROLLBACK
+update testmulticorn set test1 = 'test';
+NOTICE:  BEGIN
+NOTICE:  []
+NOTICE:  ['test1', 'test2']
+NOTICE:  ROLLBACK
+ERROR:  Error in python: NotImplementedError
+DETAIL:  This FDW does not support the writable API
+delete from testmulticorn where test2 = 'test2 2 0';
+NOTICE:  BEGIN
+NOTICE:  [test2 = test2 2 0]
+NOTICE:  ['test1', 'test2']
+NOTICE:  ROLLBACK
+ERROR:  Error in python: NotImplementedError
+DETAIL:  This FDW does not support the writable API
+CREATE foreign table testmulticorn_write (
+    test1 character varying,
+    test2 character varying
+) server multicorn_srv options (
+    option1 'option1',
+    row_id_column 'test1',
+	test_type 'returning',
+    tx_hook 'true'
+);
+insert into testmulticorn_write(test1, test2) VALUES ('test', 'test2');
+NOTICE:  [('option1', 'option1'), ('row_id_column', 'test1'), ('test_type', 'returning'), ('tx_hook', 'true'), ('usermapping', 'test')]
+NOTICE:  [('test1', 'character varying'), ('test2', 'character varying')]
+NOTICE:  BEGIN
+NOTICE:  INSERTING: [('test1', 'test'), ('test2', 'test2')]
+NOTICE:  PRECOMMIT
+NOTICE:  COMMIT
+update testmulticorn_write set test1 = 'test' where test1 ilike 'test1 3%';
+NOTICE:  BEGIN
+NOTICE:  [test1 ~~* test1 3%]
+NOTICE:  ['test1', 'test2']
+NOTICE:  UPDATING: test1 3 1 with [('test1', 'test'), ('test2', 'test2 1 1')]
+NOTICE:  UPDATING: test1 3 4 with [('test1', 'test'), ('test2', 'test2 1 4')]
+NOTICE:  UPDATING: test1 3 7 with [('test1', 'test'), ('test2', 'test2 1 7')]
+NOTICE:  UPDATING: test1 3 10 with [('test1', 'test'), ('test2', 'test2 1 10')]
+NOTICE:  UPDATING: test1 3 13 with [('test1', 'test'), ('test2', 'test2 1 13')]
+NOTICE:  UPDATING: test1 3 16 with [('test1', 'test'), ('test2', 'test2 1 16')]
+NOTICE:  UPDATING: test1 3 19 with [('test1', 'test'), ('test2', 'test2 1 19')]
+NOTICE:  PRECOMMIT
+NOTICE:  COMMIT
+delete from testmulticorn_write where test2 = 'test2 2 0';
+NOTICE:  BEGIN
+NOTICE:  [test2 = test2 2 0]
+NOTICE:  ['test1', 'test2']
+NOTICE:  DELETING: test1 1 0
+NOTICE:  PRECOMMIT
+NOTICE:  COMMIT
+-- Test returning
+insert into testmulticorn_write(test1, test2) VALUES ('test', 'test2') RETURNING test1;
+NOTICE:  BEGIN
+NOTICE:  INSERTING: [('test1', 'test'), ('test2', 'test2')]
+NOTICE:  PRECOMMIT
+NOTICE:  COMMIT
+     test1      
+----------------
+ INSERTED: test
+(1 row)
+
+update testmulticorn_write set test1 = 'test' where test1 ilike 'test1 3%' RETURNING test1;
+NOTICE:  BEGIN
+NOTICE:  [test1 ~~* test1 3%]
+NOTICE:  ['test1', 'test2']
+NOTICE:  UPDATING: test1 3 1 with [('test1', 'test'), ('test2', 'test2 1 1')]
+NOTICE:  UPDATING: test1 3 4 with [('test1', 'test'), ('test2', 'test2 1 4')]
+NOTICE:  UPDATING: test1 3 7 with [('test1', 'test'), ('test2', 'test2 1 7')]
+NOTICE:  UPDATING: test1 3 10 with [('test1', 'test'), ('test2', 'test2 1 10')]
+NOTICE:  UPDATING: test1 3 13 with [('test1', 'test'), ('test2', 'test2 1 13')]
+NOTICE:  UPDATING: test1 3 16 with [('test1', 'test'), ('test2', 'test2 1 16')]
+NOTICE:  UPDATING: test1 3 19 with [('test1', 'test'), ('test2', 'test2 1 19')]
+NOTICE:  PRECOMMIT
+NOTICE:  COMMIT
+     test1     
+---------------
+ UPDATED: test
+ UPDATED: test
+ UPDATED: test
+ UPDATED: test
+ UPDATED: test
+ UPDATED: test
+ UPDATED: test
+(7 rows)
+
+delete from testmulticorn_write where test1 = 'test1 1 0' returning test2, test1;
+NOTICE:  BEGIN
+NOTICE:  [test1 = test1 1 0]
+NOTICE:  ['test1', 'test2']
+NOTICE:  DELETING: test1 1 0
+NOTICE:  PRECOMMIT
+NOTICE:  COMMIT
+   test2   |   test1   
+-----------+-----------
+ test2 2 0 | test1 1 0
+(1 row)
+
+DROP foreign table testmulticorn_write;
+-- Now test with another column
+CREATE foreign table testmulticorn_write(
+    test1 character varying,
+    test2 character varying
+) server multicorn_srv options (
+    option1 'option1',
+    row_id_column 'test2'
+);
+insert into testmulticorn_write(test1, test2) VALUES ('test', 'test2');
+NOTICE:  [('option1', 'option1'), ('row_id_column', 'test2'), ('usermapping', 'test')]
+NOTICE:  [('test1', 'character varying'), ('test2', 'character varying')]
+NOTICE:  INSERTING: [('test1', 'test'), ('test2', 'test2')]
+update testmulticorn_write set test1 = 'test' where test1 ilike 'test1 3%';
+NOTICE:  [test1 ~~* test1 3%]
+NOTICE:  ['test1', 'test2']
+NOTICE:  UPDATING: test2 1 1 with [('test1', 'test'), ('test2', 'test2 1 1')]
+NOTICE:  UPDATING: test2 1 4 with [('test1', 'test'), ('test2', 'test2 1 4')]
+NOTICE:  UPDATING: test2 1 7 with [('test1', 'test'), ('test2', 'test2 1 7')]
+NOTICE:  UPDATING: test2 1 10 with [('test1', 'test'), ('test2', 'test2 1 10')]
+NOTICE:  UPDATING: test2 1 13 with [('test1', 'test'), ('test2', 'test2 1 13')]
+NOTICE:  UPDATING: test2 1 16 with [('test1', 'test'), ('test2', 'test2 1 16')]
+NOTICE:  UPDATING: test2 1 19 with [('test1', 'test'), ('test2', 'test2 1 19')]
+delete from testmulticorn_write where test2 = 'test2 2 0';
+NOTICE:  [test2 = test2 2 0]
+NOTICE:  ['test2']
+NOTICE:  DELETING: test2 2 0
+update testmulticorn_write set test2 = 'test' where test2 = 'test2 1 1';
+NOTICE:  [test2 = test2 1 1]
+NOTICE:  ['test1', 'test2']
+NOTICE:  UPDATING: test2 1 1 with [('test1', 'test1 3 1'), ('test2', 'test')]
+DROP foreign table testmulticorn_write;
+-- Now test with other types
+CREATE foreign table testmulticorn_write(
+    test1 date,
+    test2 timestamp
+) server multicorn_srv options (
+    option1 'option1',
+    row_id_column 'test2',
+	test_type 'date'
+);
+insert into testmulticorn_write(test1, test2) VALUES ('2012-01-01', '2012-01-01 00:00:00');
+NOTICE:  [('option1', 'option1'), ('row_id_column', 'test2'), ('test_type', 'date'), ('usermapping', 'test')]
+NOTICE:  [('test1', 'date'), ('test2', 'timestamp without time zone')]
+NOTICE:  INSERTING: [('test1', datetime.date(2012, 1, 1)), ('test2', datetime.datetime(2012, 1, 1, 0, 0))]
+delete from testmulticorn_write where test2 > '2011-12-03';
+NOTICE:  [test2 > 2011-12-03 00:00:00]
+NOTICE:  ['test2']
+NOTICE:  DELETING: 2011-12-03 14:30:25
+update testmulticorn_write set test1 = date_trunc('day', test1) where test2 = '2011-09-03 14:30:25';
+NOTICE:  [test2 = 2011-09-03 14:30:25]
+NOTICE:  ['test1', 'test2']
+NOTICE:  UPDATING: 2011-09-03 14:30:25 with [('test1', datetime.date(2011, 9, 2)), ('test2', datetime.datetime(2011, 9, 3, 14, 30, 25))]
+DROP foreign table testmulticorn_write;
+-- Test with unknown column
+CREATE foreign table testmulticorn_write(
+    test1 date,
+    test2 timestamp
+) server multicorn_srv options (
+    option1 'option1',
+    row_id_column 'teststuff',
+	test_type 'date'
+);
+delete from testmulticorn_write;
+NOTICE:  [('option1', 'option1'), ('row_id_column', 'teststuff'), ('test_type', 'date'), ('usermapping', 'test')]
+NOTICE:  [('test1', 'date'), ('test2', 'timestamp without time zone')]
+ERROR:  The rowid attribute does not exist
+DROP USER MAPPING FOR current_user SERVER multicorn_srv;
+DROP EXTENSION multicorn cascade;
+NOTICE:  drop cascades to 3 other objects
+DETAIL:  drop cascades to server multicorn_srv
+drop cascades to foreign table testmulticorn
+drop cascades to foreign table testmulticorn_write

Original file line number	Diff line number	Diff line change
`@@ -667,7 +667,6 @@ getCacheEntry(Oid foreigntableid)`
`667`	`667`	`MemoryContextDelete(tempContext);`
`668`	`668`	`}`
`669`	`669`	`RelationClose(rel);`
`670`		`- Py_INCREF(entry->value);`
`671`	`670`
`672`	`671`	`/*`
`673`	`672`	`* Start a new transaction or subtransaction if needed.`
`@@ -685,7 +684,9 @@ getCacheEntry(Oid foreigntableid)`
`685`	`684`	`PyObject *`
`686`	`685`	`getInstance(Oid foreigntableid)`
`687`	`686`	`{`
`688`		`- return getCacheEntry(foreigntableid)->value;`
	`687`	`+ PyObject* retval = getCacheEntry(foreigntableid)->value;`
	`688`	`+ Py_INCREF(retval);`
	`689`	`+ return retval;`
`689`	`690`	`}`
`690`	`691`
`691`	`692`