Subject: Re: Snapshot leak warning with lo_export in subtransaction
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Pg Bugs <pgsql-bugs@postgresql.org>,
 Andrew B <pgsql.20.drkshadow@spamgourmet.com>
References: <202109271233.5zn5bqxn657w@alvherre.pgsql>
From: Heikki Linnakangas <hlinnaka@iki.fi>
Message-ID: <f698e989-df50-9e4e-d7c3-3c05ebe56ddd@iki.fi>
Date: Mon, 18 Oct 2021 12:59:56 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <202109271233.5zn5bqxn657w@alvherre.pgsql>
Content-Type: multipart/mixed;
 boundary="------------4B67429588ABA6FBAFB00132"
Content-Language: en-US
Archived-At: 
 <https://www.postgresql.org/message-id/f698e989-df50-9e4e-d7c3-3c05ebe56ddd%40iki.fi>
Precedence: bulk

This is a multi-part message in MIME format.
--------------4B67429588ABA6FBAFB00132
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

On 27/09/2021 15:33, Alvaro Herrera wrote:
> On 2021-Sep-27, Heikki Linnakangas wrote:
> 
>> This should be backpatched to all supported versions. This adds an argument
>> to 'inv_open' function, but I don't think there are extensions that use the
>> inv_*() functions directly. inv_api.c relies on close_lo_relation() being
>> called at the end of transaction, so I think an extension would find it hard
>> to use those functions correctly, anyway.
> 
> Hmm, but you could also use a new value in its existing 'flags' argument
> instead of changing ABI.

I tried that, but didn't like the result. It conflated the user-visible 
INV_READ/WRITE flags with the new internal-only flag.

Thinking about this some more, I came up with the attached. It moves the 
responsibility of registering the snapshot from inv_api.c to the caller. 
With that change, there's no need for a new option to inv_open(). The 
division of labor between be-fsstubs.c and inv_api.c has always been a 
bit blurry, I think that this makes it slightly more clear.

With this change, if there is an existing extension out there that calls 
inv_open(), the returned desc will be short-lived. As I said above, I 
think that's a reasonable assumption because dealing with a long-lived 
LO desc correctly would require the caller to jump through some hoops 
even before this change.

- Heikki

--------------4B67429588ABA6FBAFB00132
Content-Type: text/x-patch; charset=UTF-8;
 name="v2-0001-Fix-snapshot-reference-leak-if-lo_export-fails.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v2-0001-Fix-snapshot-reference-leak-if-lo_export-fails.patch"