While the fix looks good to me, I have a few review comments on the regression test part:
+-- bug #19616 +-- pgoutput protocol compatibility could be broken for an aborted xact +-- discarded at spill eviction while a subxact remained in memory. +-- Stream Abort ('A'), valid only since protocol version 2, could be seen +-- with protocol version 1. ... +SELECT chr(get_byte(data, 0)) AS msgtype, count(*) +FROM pg_logical_slot_peek_binary_changes('regression_slot_pgoutput', NULL, NULL, + 'proto_version', '1', 'publication_names', 'spill_pub') +GROUP BY 1 ORDER BY 1;
I'm not sure we should test the case against proto_version=1 for the reason I mentioned above. I think we can use proto_version=4 and streaming=0 instead. Also, the comment needs to be adjusted.
stream.sql would be more suitable to put this test.
Dear Sawada-san,
You are right, and these remarks have been addressed in v5 of the patch.
The spurious Stream Abort is not really about the protocol version. So the real invariant is that a client which did not enable streaming must not receive streaming messages. proto_version=1 only tested a special case. proto_version=4 with streaming=0 exercises the invariant directly. I confirmed the reformulated test still catches the bug. I moved the test to stream.sql as you suggested, keeping the structure of the already-reviewed comment.
