From fbdd6559ceea4bf860a2c4bab58d55827ba23ecb Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Mon, 28 Jan 2019 22:09:33 +0100 Subject: [PATCH] Fix a crash in logical replication The bug was that determining which columns are part of the replica identity index using RelationGetIndexAttrBitmap() would run eval_const_expressions() on index expressions and predicates across all indexes of the table, which in turn might require a snapshot, but there wasn't one set, so it crashes. There were actually two separate bugs, one on the publisher and one on the subscriber. To trigger the bug, a table that is part of a publication or subscription needs to have an index with a predicate or expression that lends itself to constant expressions simplification. The fix is to avoid the constant expressions simplification in RelationGetIndexAttrBitmap(), so that it becomes safe to call in these contexts. The constant expressions simplification comes from the calls to RelationGetIndexExpressions()/RelationGetIndexPredicate() via BuildIndexInfo(). But RelationGetIndexAttrBitmap() calling BuildIndexInfo() is overkill. The latter just takes pg_index catalog information, packs it into the IndexInfo structure, which former than just unpacks again and throws away. We can just do this directly with less overhead and skip the troublesome calls to eval_const_expressions(). This also removes the awkward cross-dependency between relcache.c and index.c. Bug: #15114 --- src/backend/utils/cache/relcache.c | 50 +++++++++++++++------- src/test/subscription/t/100_bugs.pl | 65 +++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 14 deletions(-) create mode 100644 src/test/subscription/t/100_bugs.pl diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index bcf4f104cf..12624a130c 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -41,7 +41,6 @@ #include "access/xact.h" #include "access/xlog.h" #include "catalog/catalog.h" -#include "catalog/index.h" #include "catalog/indexing.h" #include "catalog/namespace.h" #include "catalog/partition.h" @@ -4717,7 +4716,10 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind) { Oid indexOid = lfirst_oid(l); Relation indexDesc; - IndexInfo *indexInfo; + Datum datum; + bool isnull; + Node *indexExpressions; + Node *indexPredicate; int i; bool isKey; /* candidate key */ bool isPK; /* primary key */ @@ -4725,13 +4727,33 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind) indexDesc = index_open(indexOid, AccessShareLock); - /* Extract index key information from the index's pg_index row */ - indexInfo = BuildIndexInfo(indexDesc); + /* + * Extract index expressions and index predicate. Note: Don't use + * RelationGetIndexExpressions()/RelationGetIndexPredicate(), because + * those might run constants evaluation, which needs a snapshot, which + * we might not have here. (Also, it's probably more sound to collect + * the bitmaps before any transformations that might eliminate + * columns, but the practical impact of this is limited.) + */ + + datum = heap_getattr(indexDesc->rd_indextuple, Anum_pg_index_indexprs, + GetPgIndexDescriptor(), &isnull); + if (!isnull) + indexExpressions = stringToNode(TextDatumGetCString(datum)); + else + indexExpressions = NULL; + + datum = heap_getattr(indexDesc->rd_indextuple, Anum_pg_index_indpred, + GetPgIndexDescriptor(), &isnull); + if (!isnull) + indexPredicate = stringToNode(TextDatumGetCString(datum)); + else + indexPredicate = NULL; /* Can this index be referenced by a foreign key? */ - isKey = indexInfo->ii_Unique && - indexInfo->ii_Expressions == NIL && - indexInfo->ii_Predicate == NIL; + isKey = indexDesc->rd_index->indisunique && + indexExpressions == NULL && + indexPredicate == NULL; /* Is this a primary key? */ isPK = (indexOid == relpkindex); @@ -4740,9 +4762,9 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind) isIDKey = (indexOid == relreplindex); /* Collect simple attribute references */ - for (i = 0; i < indexInfo->ii_NumIndexAttrs; i++) + for (i = 0; i < indexDesc->rd_index->indnatts; i++) { - int attrnum = indexInfo->ii_IndexAttrNumbers[i]; + int attrnum = indexDesc->rd_index->indkey.values[i]; /* * Since we have covering indexes with non-key columns, we must @@ -4757,25 +4779,25 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind) indexattrs = bms_add_member(indexattrs, attrnum - FirstLowInvalidHeapAttributeNumber); - if (isKey && i < indexInfo->ii_NumIndexKeyAttrs) + if (isKey && i < indexDesc->rd_index->indnkeyatts) uindexattrs = bms_add_member(uindexattrs, attrnum - FirstLowInvalidHeapAttributeNumber); - if (isPK && i < indexInfo->ii_NumIndexKeyAttrs) + if (isPK && i < indexDesc->rd_index->indnkeyatts) pkindexattrs = bms_add_member(pkindexattrs, attrnum - FirstLowInvalidHeapAttributeNumber); - if (isIDKey && i < indexInfo->ii_NumIndexKeyAttrs) + if (isIDKey && i < indexDesc->rd_index->indnkeyatts) idindexattrs = bms_add_member(idindexattrs, attrnum - FirstLowInvalidHeapAttributeNumber); } } /* Collect all attributes used in expressions, too */ - pull_varattnos((Node *) indexInfo->ii_Expressions, 1, &indexattrs); + pull_varattnos(indexExpressions, 1, &indexattrs); /* Collect all attributes in the index predicate, too */ - pull_varattnos((Node *) indexInfo->ii_Predicate, 1, &indexattrs); + pull_varattnos(indexPredicate, 1, &indexattrs); index_close(indexDesc, AccessShareLock); } diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl new file mode 100644 index 0000000000..cbdb30a8bb --- /dev/null +++ b/src/test/subscription/t/100_bugs.pl @@ -0,0 +1,65 @@ +# Tests for various bugs found over time +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More tests => 1; + +# Bug #15114 + +# The bug was that determining which columns are part of the replica +# identity index using RelationGetIndexAttrBitmap() would run +# eval_const_expressions() on index expressions and predicates across +# all indexes of the table, which in turn might require a snapshot, +# but there wasn't one set, so it crashes. There were actually two +# separate bugs, one on the publisher and one on the subscriber. The +# fix was to avoid the constant expressions simplification in +# RelationGetIndexAttrBitmap(), so it's safe to call in more contexts. + +my $node_publisher = get_new_node('publisher'); +$node_publisher->init(allows_streaming => 'logical'); +$node_publisher->start; + +my $node_subscriber = get_new_node('subscriber'); +$node_subscriber->init(allows_streaming => 'logical'); +$node_subscriber->start; + +my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres'; + +$node_publisher->safe_psql('postgres', + "CREATE TABLE tab1 (a int PRIMARY KEY, b int)"); + +$node_publisher->safe_psql('postgres', + "CREATE FUNCTION double(x int) RETURNS int IMMUTABLE LANGUAGE SQL AS 'select x * 2'"); + +# an index with a predicate that lends itself to constant expressions +# evaluation +$node_publisher->safe_psql('postgres', + "CREATE INDEX ON tab1 (b) WHERE a > double(1)"); + +# and the same setup on the subscriber +$node_subscriber->safe_psql('postgres', + "CREATE TABLE tab1 (a int PRIMARY KEY, b int)"); + +$node_subscriber->safe_psql('postgres', + "CREATE FUNCTION double(x int) RETURNS int IMMUTABLE LANGUAGE SQL AS 'select x * 2'"); + +$node_subscriber->safe_psql('postgres', + "CREATE INDEX ON tab1 (b) WHERE a > double(1)"); + +$node_publisher->safe_psql('postgres', + "CREATE PUBLICATION pub1 FOR ALL TABLES"); + +$node_subscriber->safe_psql('postgres', + "CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr application_name=sub1' PUBLICATION pub1"); + +$node_publisher->wait_for_catchup('sub1'); + +# This would crash, first on the publisher, and then (if the publisher +# is fixed) on the subscriber. +$node_publisher->safe_psql('postgres', + "INSERT INTO tab1 VALUES (1, 2)"); + +$node_publisher->wait_for_catchup('sub1'); + +pass('index predicates do not cause crash'); -- 2.20.1