MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_258e7eae2d63fcabf7b3fdd5a72b9570"
Content-Transfer-Encoding: 7bit
Date: Wed, 19 May 2021 19:18:27 +0300
From: Yura Sokolov <y.sokolov@postgrespro.ru>
To: pgsql-hackers@lists.postgresql.org
Subject: Add PortalDrop in exec_execute_message
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <4aa370cb91ecf2f9885d98b80ad1109c@postgrespro.ru>
Archived-At: 
 <https://www.postgresql.org/message-id/4aa370cb91ecf2f9885d98b80ad1109c%40postgrespro.ru>
Precedence: bulk

--=_258e7eae2d63fcabf7b3fdd5a72b9570
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=US-ASCII;
 format=flowed

Hi, hackers.

I've been playing with "autoprepared" patch, and have got isolation
"freeze-the-dead" test stuck on first VACUUM FREEZE statement.
After some research I found issue is reproduced with unmodified master
branch if extended protocol used. I've prepared ruby script for
demonstration (cause ruby-pg has simple interface to PQsendQueryParams).

Further investigation showed it happens due to portal is not dropped
inside of exec_execute_message, and it is kept in third session till
COMMIT is called. Therefore heap page remains pinned, and VACUUM FREEZE
became locked inside LockBufferForCleanup.

It seems that it is usually invisible to common users since either:
- command is called as standalone and then transaction is closed
   immediately,
- next PQsendQueryParams will initiate another unnamed portal using
   CreatePortal("", true, true) and this action will drop previous
   one.

But "freeze-the-dead" remains locked since third session could not
send COMMIT until VACUUM FULL finished.

I propose to add PortalDrop at the 'if (completed)' branch of
exec_execute_message.

--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2209,6 +2209,8 @@ exec_execute_message(const char *portal_name, long 
max_rows)

         if (completed)
         {
+               PortalDrop(portal, false);
+
                 if (is_xact_command)
                 {

With this change 'make check-world' runs without flaws (at least
on empty configure with enable-cassert and enable-tap-tests).

There is small chance applications exist which abuses seekable
portals with 'execute' protocol message so not every completed
portal can be safely dropped. But I believe there is some sane
conditions that cover common case. For example, isn't empty name
check is enough? Can client reset or seek portal with empty
name?

regards,
Sokolov Yura aka funny_falcon
--=_258e7eae2d63fcabf7b3fdd5a72b9570
Content-Transfer-Encoding: base64
Content-Type: text/x-ruby;
 name=freeze-the-dead.rb
Content-Disposition: attachment;
 filename=freeze-the-dead.rb;
 size=1120

cmVxdWlyZSAncGcnCgpjMSA9IFBHLmNvbm5lY3QoaG9zdDogImxvY2FsaG9zdCIsIGRibmFtZTog
InBvc3RncmVzIikKYzIgPSBQRy5jb25uZWN0KGhvc3Q6ICJsb2NhbGhvc3QiLCBkYm5hbWU6ICJw
b3N0Z3JlcyIpCmMzID0gUEcuY29ubmVjdChob3N0OiAibG9jYWxob3N0IiwgZGJuYW1lOiAicG9z
dGdyZXMiKQoKY2xhc3MgUEc6OkNvbm5lY3Rpb24KICBkZWYgc2ltcGxlKHNxbCkKICAgIHB1dHMg
c3FsCiAgICBleGVjKHNxbCkKICBlbmQKICBkZWYgZXh0ZW5kZWQoc3FsKQogICAgcHV0cyAiI3tz
cWx9IgogICAgZXhlY19wYXJhbXMoc3FsLCBbXSkKICBlbmQKZW5kCgpjMS5zaW1wbGUgIkRST1Ag
VEFCTEUgSUYgRVhJU1RTIHRhYl9mcmVlemUiIApjMS5zaW1wbGUgIgpDUkVBVEUgVEFCTEUgdGFi
X2ZyZWV6ZSAoCiAgaWQgaW50IFBSSU1BUlkgS0VZLAogIG5hbWUgY2hhcigzKSwKICB4IGludCk7
CklOU0VSVCBJTlRPIHRhYl9mcmVlemUgVkFMVUVTICgxLCAnMTExJywgMCk7CklOU0VSVCBJTlRP
IHRhYl9mcmVlemUgVkFMVUVTICgzLCAnMzMzJywgMCk7CiIKCmMxLnNpbXBsZSAiQkVHSU4iCmMy
LnNpbXBsZSAiQkVHSU4iCmMzLnNpbXBsZSAiQkVHSU4iCmMxLnNpbXBsZSAiVVBEQVRFIHRhYl9m
cmVlemUgU0VUIHggPSB4ICsgMSBXSEVSRSBpZCA9IDMiCmMyLmV4dGVuZGVkICJTRUxFQ1QgaWQg
RlJPTSB0YWJfZnJlZXplIFdIRVJFIGlkID0gMyBGT1IgS0VZIFNIQVJFIgpjMy5leHRlbmRlZCAi
U0VMRUNUIGlkIEZST00gdGFiX2ZyZWV6ZSBXSEVSRSBpZCA9IDMgRk9SIEtFWSBTSEFSRSIKYzEu
c2ltcGxlICJDT01NSVQiCmMyLnNpbXBsZSAiQ09NTUlUIgpjMi5zaW1wbGUgIlZBQ1VVTSBGUkVF
WkUgdGFiX2ZyZWV6ZSIKYzEuc2ltcGxlICIKQkVHSU47ClNFVCBMT0NBTCBlbmFibGVfc2Vxc2Nh
biA9IGZhbHNlOwpTRVQgTE9DQUwgZW5hYmxlX2JpdG1hcHNjYW4gPSBmYWxzZTsKU0VMRUNUICog
RlJPTSB0YWJfZnJlZXplIFdIRVJFIGlkID0gMzsKQ09NTUlUOwoiCmMzLnNpbXBsZSAiQ09NTUlU
IgpjMi5zaW1wbGUgIlZBQ1VVTSBGUkVFWkUgdGFiX2ZyZWV6ZSIKYzEuc2ltcGxlICJTRUxFQ1Qg
KiBGUk9NIHRhYl9mcmVlemUgT1JERVIgQlkgbmFtZSwgaWQiCg==
--=_258e7eae2d63fcabf7b3fdd5a72b9570--