Thread: Improve checking for pg_index.xmin
Hi!
Our customer faced with issue, when index is invisible after creation.
The reproducible case is following.
$ psql db2
# begin;
# select txid_current();
$ psql db1
# select i as id, 0 as v into t from generate_series(1, 100000) i;
# create unique index idx on t (id);
# update t set v = v + 1 where id = 10000;
# update t set v = v + 1 where id = 10000;
# update t set v = v + 1 where id = 10000;
# update t set v = v + 1 where id = 10000;
# update t set v = v + 1 where id = 10000;
# drop index idx;
# create unique index idx on t (id);
# explain analyze select v from t where id = 10000;
There is no issue if there is no parallel session in database db2.
The fact that index visibility depends on open transaction in
different database is ridiculous for users.
This happens so, because we're checking that there is no broken HOT
chains after index creation by comparison pg_index.xmin and
TransactionXmin. So, we check that pg_index.xmin is in the past for
current transaction in lossy way by comparison just xmins. Attached
patch changes this check to XidInMVCCSnapshot().
With patch the issue is gone. My doubt about this patch is that it
changes check with TransactionXmin to check with GetActiveSnapshot(),
which might be more recent. However, query shouldn't be executer with
older snapshot than one it was planned with.
Any thoughts?
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachment
On 01/11/2019 01:50, Alexander Korotkov wrote: > Hi! > > Our customer faced with issue, when index is invisible after creation. > The reproducible case is following. > > $ psql db2 > # begin; > # select txid_current(); > $ psql db1 > # select i as id, 0 as v into t from generate_series(1, 100000) i; > # create unique index idx on t (id); > # update t set v = v + 1 where id = 10000; > # update t set v = v + 1 where id = 10000; > # update t set v = v + 1 where id = 10000; > # update t set v = v + 1 where id = 10000; > # update t set v = v + 1 where id = 10000; > # drop index idx; > # create unique index idx on t (id); > # explain analyze select v from t where id = 10000; > > There is no issue if there is no parallel session in database db2. > The fact that index visibility depends on open transaction in > different database is ridiculous for users. > > This happens so, because we're checking that there is no broken HOT > chains after index creation by comparison pg_index.xmin and > TransactionXmin. So, we check that pg_index.xmin is in the past for > current transaction in lossy way by comparison just xmins. Attached > patch changes this check to XidInMVCCSnapshot(). > > With patch the issue is gone. My doubt about this patch is that it > changes check with TransactionXmin to check with GetActiveSnapshot(), > which might be more recent. However, query shouldn't be executer with > older snapshot than one it was planned with. Hmm. Maybe you could construct a case like that with a creative mix of stable and volatile functions? Using GetOldestSnapshot() would be safer. - Heikki
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 01/11/2019 01:50, Alexander Korotkov wrote:
>> This happens so, because we're checking that there is no broken HOT
>> chains after index creation by comparison pg_index.xmin and
>> TransactionXmin. So, we check that pg_index.xmin is in the past for
>> current transaction in lossy way by comparison just xmins. Attached
>> patch changes this check to XidInMVCCSnapshot().
>> With patch the issue is gone. My doubt about this patch is that it
>> changes check with TransactionXmin to check with GetActiveSnapshot(),
>> which might be more recent. However, query shouldn't be executer with
>> older snapshot than one it was planned with.
> Hmm. Maybe you could construct a case like that with a creative mix of
> stable and volatile functions? Using GetOldestSnapshot() would be safer.
I really wonder if this is safe at all.
(1) Can we assume that the query will be executed with same-or-newer
snapshot as what was used by the planner? There's no such constraint
in the plancache, I'm pretty sure.
(2) Is committed-ness of the index-creating transaction actually
sufficient to ensure that none of the broken HOT chains it saw are
a problem for the onlooker transaction? This is, at best, really
un-obvious. Some of those HOT chains could involve xacts that were
still not committed when the index build finished, I believe.
(3) What if the index was made with CREATE INDEX CONCURRENTLY ---
which xid is actually on the pg_index row, and how does that factor
into (1) and (2)?
On the whole I don't find the risk/reward tradeoff of this looking
promising. Even if it works reliably, I think the situations where
it'll help a lot are a bit artificial.
regards, tom lane
On Wed, Jan 8, 2020 at 4:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Heikki Linnakangas <hlinnaka@iki.fi> writes: > > On 01/11/2019 01:50, Alexander Korotkov wrote: > >> This happens so, because we're checking that there is no broken HOT > >> chains after index creation by comparison pg_index.xmin and > >> TransactionXmin. So, we check that pg_index.xmin is in the past for > >> current transaction in lossy way by comparison just xmins. Attached > >> patch changes this check to XidInMVCCSnapshot(). > >> With patch the issue is gone. My doubt about this patch is that it > >> changes check with TransactionXmin to check with GetActiveSnapshot(), > >> which might be more recent. However, query shouldn't be executer with > >> older snapshot than one it was planned with. > > > Hmm. Maybe you could construct a case like that with a creative mix of > > stable and volatile functions? Using GetOldestSnapshot() would be safer. > > I really wonder if this is safe at all. > > (1) Can we assume that the query will be executed with same-or-newer > snapshot as what was used by the planner? There's no such constraint > in the plancache, I'm pretty sure. > > (2) Is committed-ness of the index-creating transaction actually > sufficient to ensure that none of the broken HOT chains it saw are > a problem for the onlooker transaction? This is, at best, really > un-obvious. Some of those HOT chains could involve xacts that were > still not committed when the index build finished, I believe. > > (3) What if the index was made with CREATE INDEX CONCURRENTLY --- > which xid is actually on the pg_index row, and how does that factor > into (1) and (2)? Thank you for pointing. I'll investigate these issues in detail. > On the whole I don't find the risk/reward tradeoff of this looking > promising. Even if it works reliably, I think the situations where > it'll help a lot are a bit artificial. I can't agree that these situations are artificial. For me, it seems natural that user expects index to be visible once it's created. Also, we always teach users that long-running transactions are evil, but nevertheless they are frequent in real life. So, it doesn't seem unlikely that one expects index to become visible, while long transaction is running in parallel. This particular case was reported by our customer. After investigation I was surprised how rare such cases are reported... ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
On Sun, Jan 12, 2020 at 3:33 AM Alexander Korotkov <a.korotkov@postgrespro.ru> wrote: > > On Wed, Jan 8, 2020 at 4:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Heikki Linnakangas <hlinnaka@iki.fi> writes: > > > On 01/11/2019 01:50, Alexander Korotkov wrote: > > >> This happens so, because we're checking that there is no broken HOT > > >> chains after index creation by comparison pg_index.xmin and > > >> TransactionXmin. So, we check that pg_index.xmin is in the past for > > >> current transaction in lossy way by comparison just xmins. Attached > > >> patch changes this check to XidInMVCCSnapshot(). > > >> With patch the issue is gone. My doubt about this patch is that it > > >> changes check with TransactionXmin to check with GetActiveSnapshot(), > > >> which might be more recent. However, query shouldn't be executer with > > >> older snapshot than one it was planned with. > > > > > Hmm. Maybe you could construct a case like that with a creative mix of > > > stable and volatile functions? Using GetOldestSnapshot() would be safer. > > > > I really wonder if this is safe at all. > > > > (1) Can we assume that the query will be executed with same-or-newer > > snapshot as what was used by the planner? There's no such constraint > > in the plancache, I'm pretty sure. > > > > (2) Is committed-ness of the index-creating transaction actually > > sufficient to ensure that none of the broken HOT chains it saw are > > a problem for the onlooker transaction? This is, at best, really > > un-obvious. Some of those HOT chains could involve xacts that were > > still not committed when the index build finished, I believe. > > > > (3) What if the index was made with CREATE INDEX CONCURRENTLY --- > > which xid is actually on the pg_index row, and how does that factor > > into (1) and (2)? > > Thank you for pointing. I'll investigate these issues in detail. > Are you planning to work on this patch [1] for current CF? If not, then I think it is better to either move this to the next CF or mark it as RWF. [1] - https://commitfest.postgresql.org/27/2337/ -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Mar 24, 2020 at 3:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Sun, Jan 12, 2020 at 3:33 AM Alexander Korotkov > <a.korotkov@postgrespro.ru> wrote: > > > > On Wed, Jan 8, 2020 at 4:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > Heikki Linnakangas <hlinnaka@iki.fi> writes: > > > > On 01/11/2019 01:50, Alexander Korotkov wrote: > > > >> This happens so, because we're checking that there is no broken HOT > > > >> chains after index creation by comparison pg_index.xmin and > > > >> TransactionXmin. So, we check that pg_index.xmin is in the past for > > > >> current transaction in lossy way by comparison just xmins. Attached > > > >> patch changes this check to XidInMVCCSnapshot(). > > > >> With patch the issue is gone. My doubt about this patch is that it > > > >> changes check with TransactionXmin to check with GetActiveSnapshot(), > > > >> which might be more recent. However, query shouldn't be executer with > > > >> older snapshot than one it was planned with. > > > > > > > Hmm. Maybe you could construct a case like that with a creative mix of > > > > stable and volatile functions? Using GetOldestSnapshot() would be safer. > > > > > > I really wonder if this is safe at all. > > > > > > (1) Can we assume that the query will be executed with same-or-newer > > > snapshot as what was used by the planner? There's no such constraint > > > in the plancache, I'm pretty sure. > > > > > > (2) Is committed-ness of the index-creating transaction actually > > > sufficient to ensure that none of the broken HOT chains it saw are > > > a problem for the onlooker transaction? This is, at best, really > > > un-obvious. Some of those HOT chains could involve xacts that were > > > still not committed when the index build finished, I believe. > > > > > > (3) What if the index was made with CREATE INDEX CONCURRENTLY --- > > > which xid is actually on the pg_index row, and how does that factor > > > into (1) and (2)? > > > > Thank you for pointing. I'll investigate these issues in detail. > > > > Are you planning to work on this patch [1] for current CF? If not, > then I think it is better to either move this to the next CF or mark > it as RWF. I didn't manage to investigate this subject and provide new patch version. I'm marking this RWF. ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company