[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