On Wed, Mar 15, 2017 at 6:43 AM, Pavel Stehule <pavel.steh...@gmail.com> wrote:
> I don't understand - CHECK_FOR_INTERRUPTS called from executor implicitly.

True.  So there shouldn't be any problem here.  I'm confused as can be
about what you want changed.

Some review of the patch itself:

+    pq_redirect_to_shm_mq(session->seg, session->command_qh);
+    pq_beginmessage(&msg, 'X');
+    pq_endmessage(&msg);
+    pq_stop_redirect_to_shm_mq();

shm_redirect_to_shm_mq() wasn't really designed to be used this way;
it's designed for use by the worker, not the process that launched it.
If an error occurs while output is redirected, bad things will happen.
I think it would be better to find a way of sending that message to
the queue without doing this.

Also, I suspect this is deadlock-prone.  If we get stuck trying to
send a message to the background session while the queue is full, and
at the same time the session is stuck trying to send us a long error
message, we will have an undetected deadlock.  That's why
pg_background() puts the string being passed to the worker into the
DSM segment in its entirety, rather than sending it through a shm_mq.

+                    elog(ERROR, "no T before D");

That's not much of an error message, even for an elog.

+                    elog(ERROR, "already received a T message");

Nor that.

None of these functions have function header comments.  Or much in the
way of internal comments.  For example:

+        case '1':
+            break;
+        case 'E':
+            rethrow_errornotice(&msg);
+            break;

That's really not clear without more commentary.

Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to