[pgpool-hackers: 4427] Guard against ill mannered frontend

[Tatsuo Ishii]

Recently a user complained that pgpool hangs up if JDBC driver is
used with certain option (autosave=always).

[pgpool-general: 8990] autosave=always jdbc option & it only sends query SAVEPOINT PGJDBC_AUTOSAVE and hangs
https://www.pgpool.net/pipermail/pgpool-general/2023-December/009051.html

I found a reliable reproducer of the problem:
[pgpool-general: 9007]
https://www.pgpool.net/pipermail/pgpool-general/2024-January/009068.html

> I have taken a look at the log. I tried to reproduce the problem using
> pgproto (a protocol level speaking test tool coming with Pgpool-II)
> and succeeded. The essential condition seems:
> 
> 1) set backend_weight for the standby node 0 so that everything goes
>   to primary PostgreSQL.
> 
> 2) after sequence of extended queries, send simple query like
>   "SAVEPOINT PGJDBC_AUTOSAVE" (I think this is generated by the JDBC
>   driver).
> 
> Here is the minimal data set for pgproto to reproduce the problem:
> 
> 'P'	""	"BEGIN"	0
> 'B'	""	""	0	0	0
> 'E'	""	0
> 'Q'	"SAVEPOINT PGJDBC_AUTOSAVE"
> 'Y'
> 'P'	""	"SELECT 1"	0
> 'X'
> 
> This represents following command sequence:
> 
> Parse "BEGIN"
> Bind to an unamed portal
> Execute the portal
> Send simple query "SAVEPOINT PGJDBC_AUTOSAVE"
> Wait for response from server
> Parse "SELECT 1"
> Terminate the session
> 
> I think the key to reproduce the problem is, sending a simple query
> before sending "Sync" message (which indicates the end of an extended
> protocol messages). I am not sure this is violation against the
> PostgreSQL protocol, but it seems PostgreSQL accepts the sequence.
> 
> Response from pgproto against pgpool:
> 
> FE=> Parse(stmt="", query="BEGIN")
> FE=> Bind(stmt="", portal="")
> FE=> Execute(portal="")
> FE=> Query (query="SAVEPOINT PGJDBC_AUTOSAVE")
> --> hung here

I opened a discussion on pgsql-hackers to know whether the sequence
(to send a simple query protocol message without ending an extended
query protocol message) is a protocol violation. The answer from Tom
is,

> I think it's poor practice, at best.  You should end the
> extended-protocol query cycle before invoking simple query.

So it's not a protocol violation but not a good practice at least. On
the other hand, for pgpool "hanging up" is the worst scenario because
after locking up client cannot do anything. The only solution is
killing pgpool process or restarting pgpool, which users want to
avoid.

So my proposal is, guarding against such a ill-mannerd message
sequence and throwing an error to terminate the session if such
messages are recieved.

Here are details of the proposal:

1) add a switch to turn on the "safe guard". This implies that the
   feature is only implemented on master branch. Others may argue that
   this should be back-patched.

2) if such a violation is detected, raise an error and terminate the
   session. Continuing the session will be better but it's hard to
   implement and I think it's not worth the trouble.

Attached is the PoC patch for #2.

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
-------------- next part --------------
A non-text attachment was scrubbed...
Name: guard_ill_mannered_message.patch
Type: text/x-patch
Size: 856 bytes
Desc: not available
URL: <http://www.pgpool.net/pipermail/pgpool-hackers/attachments/20240210/b4070ba6/attachment.bin>