[pgpool-hackers: 1664] Re: kind does not match error in pgpool

Muhammad Usama m.usama at gmail.com
Sun Jun 26 20:25:08 JST 2016


On Wed, Jun 22, 2016 at 7:06 PM, Tatsuo Ishii <ishii at postgresql.org> wrote:

> > On Wed, Jun 22, 2016 at 7:47 AM, Tatsuo Ishii <ishii at postgresql.org>
> wrote:
> >
> >> > On Tue, Jun 21, 2016 at 6:11 AM, Tatsuo Ishii <ishii at postgresql.org>
> >> wrote:
> >> >
> >> >> > Hi Ishii-San
> >> >> >
> >> >> > Can you please have a look at the attached patch, It try to solve
> this
> >> >> > "Kind does not match .." problem by ignoring the notice messages
> while
> >> >> > reading the backend response in read_kind_from_backend() function
> >> >>
> >> >> Doesn't this patch simply ignore important messages like this?
> >> >>
> >> >
> >> > Basically patch only ignores the notice log messages and these
> messages
> >> are
> >> > important, especially ones with severity level of WARNING and NOTICE,
> to
> >> > inform the user about some critical issue.
> >> > I may be wrong, but I don't think these log message are important in
> >> terms
> >> > of PG protocol flow. i.e. notice (kind = 'N') message only contains
> the
> >> log
> >> >  and is delivered to frontend (pgpool-II in our case) depending on
> >> > *client_min_messages* settings in postgresql.conf.
> >> > So I think it should be safe to ignore these.
> >>
> >> Is it possible to send the NOTICE message (kind = N) to frontend,
> >> rather than discard it? By the time when read_kind_from_backend() gets
> >> called, all the messages are sent to frontend and it should be in the
> >> message boundary (i.e. not in the middle of the message packet), I
> >> guess it should be safe.
> >>
> >
> > This is a good idea, But since notice messages (packet kind = N) can
> > contain a log for severity level ranging from DEBUG to WARNING
> > so instead of forwarding all notice type messages to frontend we should
> > only froward the messages with severity >= WARNING or may be consider
> > pgpool's *client_min_messages* config parameters and if the message
> > received from backend has severity >= client_min_messages only then
> forward
> > it to the frontend
>
> ? PostgreSQL has already decided which message should be sent to
> client (in this case pgpool) based on client_min_messages setting in
> postgresql.conf. So pgpool can unconditionally forward the kind = N
> packet to client.
>
Pgpool's *client_min_messages* is another story. I think it should
> only affect to messages generated in pgpool.
>

Totally agreed on both the points. I think you have a valid point and
unconditionally forwarding all the kind = N messages to frontend is the
best choice. The attached version 2 of the patch does the same as suggested.

Best Regards
Muhammad Usama


> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20160626/e710d029/attachment-0001.html>
-------------- next part --------------
diff --git a/src/protocol/pool_process_query.c b/src/protocol/pool_process_query.c
index 2625628..b399d02 100644
--- a/src/protocol/pool_process_query.c
+++ b/src/protocol/pool_process_query.c
@@ -84,7 +84,7 @@ static bool has_lock_target(POOL_CONNECTION *frontend, POOL_CONNECTION_POOL *bac
 static POOL_STATUS insert_oid_into_insert_lock(POOL_CONNECTION *frontend, POOL_CONNECTION_POOL *backend, char* table);
 static POOL_STATUS read_packets_and_process(POOL_CONNECTION *frontend, POOL_CONNECTION_POOL *backend, int reset_request, int *state, short *num_fields, bool *cont);
 static bool is_all_slaves_command_complete(unsigned char *kind_list, int num_backends, int master);
-
+static bool pool_process_notice_message_from_one_backend(POOL_CONNECTION *frontend, POOL_CONNECTION_POOL *backend, int backend_idx, char kind);
 /* timeout sec for pool_check_fd */
 static int timeoutsec = -1;
 
@@ -3334,32 +3334,40 @@ void read_kind_from_backend(POOL_CONNECTION *frontend, POOL_CONNECTION_POOL *bac
 						 errdetail("backend:%d kind:'%c'",i, kind)));
 
 				/*
-				 * Read and discard parameter status
+				 * Read and discard parameter status and notice messages
 				 */
-				if (kind != 'S')
+				if (kind == 'N')
 				{
-					break;
+
+					ereport(DEBUG1,
+						(errmsg("received log message from backend %d while reading packet kind",i)));
+					pool_process_notice_message_from_one_backend(frontend, backend, i, kind);
 				}
 
-				pool_read(CONNECTION(backend, i), &len, sizeof(len));
-				len = htonl(len) - 4;
-				p = pool_read2(CONNECTION(backend, i), len);
-				if (p)
+				else if (kind == 'S')
 				{
-					value = p + strlen(p) + 1;
-					ereport(DEBUG1,
-						(errmsg("reading backend data packet kind"),
-							 errdetail("parameter name: %s value: \"%s\"", p, value)));
+					pool_read(CONNECTION(backend, i), &len, sizeof(len));
+					len = htonl(len) - 4;
+					p = pool_read2(CONNECTION(backend, i), len);
+					if (p)
+					{
+						value = p + strlen(p) + 1;
+						ereport(DEBUG1,
+							(errmsg("reading backend data packet kind"),
+								 errdetail("parameter name: %s value: \"%s\"", p, value)));
 
-					if (IS_MASTER_NODE_ID(i))
-						pool_add_param(&CONNECTION(backend, i)->params, p, value);
+						if (IS_MASTER_NODE_ID(i))
+							pool_add_param(&CONNECTION(backend, i)->params, p, value);
+					}
+					else
+					{
+						ereport(WARNING,
+							(errmsg("failed to read parameter status packet from backend %d", i),
+								 errdetail("read from backend failed")));
+					}
 				}
-				else
-					ereport(WARNING,
-						(errmsg("failed to read parameter status packet from backend %d", i),
-							 errdetail("read from backend failed")));
 
-			} while (kind == 'S');
+			} while (kind == 'S' || kind == 'N' );
 
 #ifdef DEALLOCATE_ERROR_TEST
 			if (i == 1 && kind == 'C' &&
@@ -4258,6 +4266,85 @@ static int detect_error(POOL_CONNECTION *backend, char *error_code, int major, c
 	return is_error;
 }
 
+/* The function forwards the NOTICE mesaage received from on backend
+ * to the frontend and also puts the human readable message to the
+ * pgpool log
+ */
+
+static bool pool_process_notice_message_from_one_backend(POOL_CONNECTION *frontend, POOL_CONNECTION_POOL *backend, int backend_idx, char kind)
+{
+	int major = MAJOR(backend);
+	POOL_CONNECTION *backend_conn = CONNECTION(backend, backend_idx);
+
+	if (kind != 'N')
+		return false;
+
+	/* read actual message */
+	if (major == PROTO_MAJOR_V3)
+	{
+		char *e;
+		int len, datalen;
+		char *buff;
+		char *errorSev = NULL;
+		char *errorMsg = NULL;
+
+		pool_read(backend_conn, &datalen, sizeof(datalen));
+		len = ntohl(datalen) - 4;
+
+		if (len <= 0 )
+			return false;
+
+		buff = palloc(len);
+
+		pool_read(backend_conn, buff, len);
+
+		e = buff;
+
+		while (*e)
+		{
+			char tok = *e;
+			e++;
+			if(*e == 0)
+				break;
+			if (tok == 'M')
+				errorMsg = e;
+			else if(tok == 'S')
+				errorSev = e;
+			else
+				e += strlen(e) + 1;
+
+			if(errorSev && errorMsg) /* we have all what we need */
+				break;
+		}
+
+		/* produce a pgpool log entry */
+		ereport(LOG,
+			(errmsg("backend [%d]: %s: %s",backend_idx,errorSev,errorMsg)));
+		/* forward it to the frontend */
+		pool_write(frontend, &kind, 1);
+		pool_write(frontend, &datalen, sizeof(datalen));
+		pool_write_and_flush(frontend, buff, len);
+
+		pfree(buff);
+	}
+	else /* Old Protocol */
+	{
+		int len = 0;
+		char *str = pool_read_string(backend_conn, &len, 0);
+
+		if (str == NULL || len <= 0)
+			return false;
+
+		/* produce a pgpool log entry */
+		ereport(LOG,
+				(errmsg("backend [%d]: NOTICE: %s",backend_idx,str)));
+		/* forward it to the frontend */
+		pool_write(frontend, &kind, 1);
+		pool_write_and_flush(frontend, str, len);
+	}
+	return true;
+}
+
 /*
  * pool_extract_error_message: Extract human readable message from
  * ERROR/NOTICE reponse packet and return it. If "read_kind" is true,


More information about the pgpool-hackers mailing list