Date: Mon, 27 Jul 2020 13:25:29 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: [BUG] Error in BRIN summarization
Message-ID: <20200727172529.GA12531@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <4e07c304-76c8-2645-d4a4-e5706072b7d7@postgrespro.ru>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2020-Jul-27, Anastasia Lubennikova wrote:

> Here is the updated version of the fix.
> The problem can be reproduced on all supported versions, so I suggest to
> backpatch it.
> Code slightly changed in v12, so here are two patches: one for versions 9.5
> to 11 and another for versions from 12 to master.

Hi Anastasia, thanks for this report and fix.  I was considering this
last week and noticed that the patch changes the ABI of
heap_get_root_tuples, which may be problematic in back branches.  I
suggest that for unreleased branches (12 and prior) we need to create a
new function with the new signature, and keep heap_get_root_tuples
unchanged.  In 13 and master we don't need that trick, so we can keep
the code as you have it in this version of the patch.

OffsetNumber
heap_get_root_tuples_new(Page page, OffsetNumber *root_offsets)
{ .. full implementation ... }

/* ABI compatibility only */
void
heap_get_root_tuples(Page page, OffsetNumber *root_offsets)
{
	(void) heap_get_root_tuples_new(page, root_offsets);
}


(I was also considering whether it needs to be a loop to reobtain root
tuples, in case a concurrent transaction can create a new item while
we're checking that item; but I don't think that can really happen for
one individual tuple.)

Thanks

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services