On Monday, March 13, 2023 2:23 PM Önder Kalacı <onderkal...@gmail.com> wrote: Hi,
> > > > > > > > > Reading [1], I think I can follow what you suggest. So, basically, > > > if the leftmost column is not filtered, we have the following: > > > > > >> but the entire index would have to be scanned, so in most cases the > > >> planner > > would prefer a sequential table scan over using the index. > > > > > > > > > So, in our case, we could follow a similar approach. If the leftmost > > > column of > > the index > > > is not sent over the wire from the pub, we can prefer the sequential scan. > > > > > > Is my understanding of your suggestion accurate? > > > > > > > Yes. I request an opinion from Shi-San who has reported the problem. > > > > I also agree with this. > And I think we can mention this in the comments if we do so. > > Already commented on FindUsableIndexForReplicaIdentityFull() on v44. Thanks for updating the patch. I noticed one problem: +static bool +RemoteRelContainsLeftMostColumnOnIdx(IndexInfo *indexInfo, + LogicalRepRelation *remoterel) +{ + AttrNumber keycol; + + if (indexInfo->ii_NumIndexAttrs < 1) + return false; + + keycol = indexInfo->ii_IndexAttrNumbers[0]; + if (!AttributeNumberIsValid(keycol)) + return false; + + return bms_is_member(keycol-1, remoterel->attkeys); +} In this function, it used the local column number(keycol) to match the remote column number(attkeys), I think it will cause problem if the column order between pub/sub doesn't match. Like: ------- - pub CREATE TABLE test_replica_id_full (x int, y int); ALTER TABLE test_replica_id_full REPLICA IDENTITY FULL; CREATE PUBLICATION tap_pub_rep_full FOR TABLE test_replica_id_full; - sub CREATE TABLE test_replica_id_full (z int, y int, x int); CREATE unique INDEX idx ON test_replica_id_full(z); CREATE SUBSCRIPTION tap_sub_rep_full_0 CONNECTION 'dbname=postgres port=5432' PUBLICATION tap_pub_rep_full; ------- I think we need to use the attrmap->attnums to convert the column number before comparing. Just for reference, attach a diff(0001) which I noted down when trying to fix the problem. Besides, I also look at the "WIP: Optimize for non-pkey / non-RI unique indexes" patch, I think it also had a similar problem about the column matching. And another thing I think we can improved in this WIP patch is that we can cache the result of IsIdxSafeToSkipDuplicates() instead of doing it for each UPDATE, because the cost of this function becomes bigger after applying this patch. And for reference, I tried to improve the WIP for the same, and here is a slight modified version of this WIP(0002). Feel free to modify or merge it if needed. Thanks for Shi-san for helping to finish these fixes. Best Regards, Hou zj
From 3e7f94ff0492a98d2d7970146d3a9a1a43cecd92 Mon Sep 17 00:00:00 2001 From: Hou Zhijie <houzj.f...@cn.fujitsu.com> Date: Mon, 13 Mar 2023 17:36:55 +0800 Subject: [PATCH 2/2] WIP: Optimize for non-pkey / non-RI unique indexes --- src/backend/executor/execReplication.c | 7 +- src/backend/replication/logical/relation.c | 150 +++++++++++++++++---- src/backend/replication/logical/worker.c | 40 ++++-- src/include/executor/executor.h | 2 + src/include/replication/logicalrelation.h | 4 +- 5 files changed, 158 insertions(+), 45 deletions(-) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index cd17be0681..ba19ef2bf8 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -135,6 +135,7 @@ build_replindex_scan_key(ScanKey skey, Relation rel, Relation idxrel, */ bool RelationFindReplTupleByIndex(Relation rel, Oid idxoid, + bool isIdxSafeToSkipDuplicates, LockTupleMode lockmode, TupleTableSlot *searchslot, TupleTableSlot *outslot) @@ -147,13 +148,10 @@ RelationFindReplTupleByIndex(Relation rel, Oid idxoid, Relation idxrel; bool found; TypeCacheEntry **eq = NULL; - bool isIdxSafeToSkipDuplicates; /* Open the index. */ idxrel = index_open(idxoid, RowExclusiveLock); - isIdxSafeToSkipDuplicates = IsIdxSafeToSkipDuplicates(rel, idxoid); - InitDirtySnapshot(snap); /* Build scan key. */ @@ -171,8 +169,7 @@ retry: while (index_getnext_slot(scan, ForwardScanDirection, outslot)) { /* - * Avoid expensive equality check if the index is primary key or - * replica identity index. + * Avoid expensive equality check if the index is unique. */ if (!isIdxSafeToSkipDuplicates) { diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c index ddabab0e01..3f16b25b89 100644 --- a/src/backend/replication/logical/relation.c +++ b/src/backend/replication/logical/relation.c @@ -27,6 +27,7 @@ #include "replication/logicalrelation.h" #include "replication/worker_internal.h" #include "utils/inval.h" +#include "utils/syscache.h" static MemoryContext LogicalRepRelMapContext = NULL; @@ -52,9 +53,9 @@ typedef struct LogicalRepPartMapEntry LogicalRepRelMapEntry relmapentry; } LogicalRepPartMapEntry; -static Oid FindLogicalRepLocalIndex(Relation localrel, - LogicalRepRelation *remoterel, - AttrMap *attrmap); +static void FindLogicalRepLocalIndex(LogicalRepRelMapEntry *entry); +static bool IsIdxSafeToSkipDuplicates(Relation localrel, Relation idxrel, + AttrMap *attrmap); /* * Relcache invalidation callback for our relation map cache. @@ -451,9 +452,7 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode) * of the relation cache entry (such as ANALYZE or CREATE/DROP index * on the relation). */ - entry->localindexoid = FindLogicalRepLocalIndex(entry->localrel, - remoterel, - entry->attrmap); + FindLogicalRepLocalIndex(entry); entry->localrelvalid = true; } @@ -725,7 +724,7 @@ logicalrep_partition_open(LogicalRepRelMapEntry *root, * We also prefer to run this code on the oldctx so that we do not leak * anything in the LogicalRepPartMapContext (hence CacheMemoryContext). */ - entry->localindexoid = FindLogicalRepLocalIndex(partrel, remoterel, entry->attrmap); + FindLogicalRepLocalIndex(entry); entry->localrelvalid = true; @@ -775,6 +774,84 @@ RemoteRelContainsLeftMostColumnOnIdx(IndexInfo *indexInfo, return attrmap->attnums[AttrNumberGetAttrOffset(keycol)] >= 0; } +/* + * WIP + */ +static bool +IndexContainsAllRemoteColumns(IndexInfo *indexInfo, AttrMap *attrmap) +{ + Bitmapset *indexattrs = NULL; /* indexed columns */ + int remoteColumnCount = 0; + + for (int i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++) + { + int keycol = indexInfo->ii_IndexAttrNumbers[i]; + + if (!AttributeNumberIsValid(keycol)) + return false; + + if (attrmap->attnums[AttrNumberGetAttrOffset(keycol)] < 0) + return false; + + indexattrs = bms_add_member(indexattrs, AttrNumberGetAttrOffset(keycol)); + } + + for (int i = 0; i < attrmap->maplen; i++) + { + if (attrmap->attnums[i] >= 0) + remoteColumnCount++; + } + + return remoteColumnCount == bms_num_members(indexattrs); + +} + +bool +IndexHasPkeyCharacterstics(IndexInfo *indexInfo, Relation localrel) +{ + + if (!indexInfo->ii_Unique) + return false; + + if (indexInfo->ii_NullsNotDistinct) + return false; + + /* + * Check that all of the attributes in a primary key are marked as not + * null. (We don't really expect to see that; it'd mean the parser messed + * up. But it seems wise to check anyway.) + */ + for (int i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++) + { + AttrNumber attnum = indexInfo->ii_IndexAttrNumbers[i]; + HeapTuple atttuple; + Form_pg_attribute attform; + + if (attnum == 0) + return false; + + /* System attributes are never null, so no need to check */ + if (attnum < 0) + return false; + + atttuple = SearchSysCache2(ATTNUM, + ObjectIdGetDatum(RelationGetRelid(localrel)), + Int16GetDatum(attnum)); + if (!HeapTupleIsValid(atttuple)) + return false; + + attform = (Form_pg_attribute) GETSTRUCT(atttuple); + + if (!attform->attnotnull) + return false; + + ReleaseSysCache(atttuple); + } + + return true; + +} + /* * Returns the oid of an index that can be used by the apply worker to scan * the relation. The index must be btree, non-partial, and have at least @@ -803,11 +880,13 @@ RemoteRelContainsLeftMostColumnOnIdx(IndexInfo *indexInfo, * * If no suitable index is found, returns InvalidOid. */ -static Oid -FindUsableIndexForReplicaIdentityFull(Relation localrel, AttrMap *attrmap) +static void +FindUsableIndexForReplicaIdentityFull(LogicalRepRelMapEntry *entry) { - List *idxlist = RelationGetIndexList(localrel); ListCell *lc; + Relation localrel = entry->localrel; + AttrMap *attrmap = entry->attrmap; + List *idxlist = RelationGetIndexList(localrel); foreach(lc, idxlist) { @@ -822,14 +901,19 @@ FindUsableIndexForReplicaIdentityFull(Relation localrel, AttrMap *attrmap) isUsableIdx = IsIndexUsableForReplicaIdentityFull(idxInfo); containsLeftMostCol = RemoteRelContainsLeftMostColumnOnIdx(idxInfo, attrmap); - index_close(idxRel, AccessShareLock); /* Return the first eligible index found */ if (isUsableIdx && containsLeftMostCol) - return idxoid; + { + entry->localindexoid = idxoid; + entry->isIdxSafeToSkipDuplicates = IsIdxSafeToSkipDuplicates(localrel, idxRel, attrmap); + index_close(idxRel, AccessShareLock); + return; + } + index_close(idxRel, AccessShareLock); } - return InvalidOid; + return; } /* @@ -865,42 +949,56 @@ GetRelationIdentityOrPK(Relation rel) } /* - * Given a relation and OID of an index, returns true if the index is relation's - * replica identity index or relation's primary key's index. + * Given index relation, returns true if the index is unique. * * Returns false otherwise. */ -bool -IsIdxSafeToSkipDuplicates(Relation rel, Oid idxoid) +static bool +IsIdxSafeToSkipDuplicates(Relation localrel, Relation idxrel, + AttrMap *attrmap) { - Assert(OidIsValid(idxoid)); + IndexInfo *indexInfo; + + if (GetRelationIdentityOrPK(localrel) == RelationGetRelid(idxrel)) + return true; - return GetRelationIdentityOrPK(rel) == idxoid; + indexInfo = BuildIndexInfo(idxrel); + return IndexContainsAllRemoteColumns(indexInfo, attrmap) && + IndexHasPkeyCharacterstics(indexInfo, localrel); } /* * Returns the index oid if we can use an index for subscriber. Otherwise, * returns InvalidOid. */ -static Oid -FindLogicalRepLocalIndex(Relation localrel, LogicalRepRelation *remoterel, - AttrMap *attrmap) +static void +FindLogicalRepLocalIndex(LogicalRepRelMapEntry *entry) { Oid idxoid; + Relation localrel = entry->localrel; + LogicalRepRelation *remoterel = &entry->remoterel; + + entry->localindexoid = InvalidOid; + entry->isIdxSafeToSkipDuplicates = false; /* * We never need index oid for partitioned tables, always rely on leaf * partition's index. */ if (localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - return InvalidOid; + return; + /* * Simple case, we already have a primary key or a replica identity index. */ idxoid = GetRelationIdentityOrPK(localrel); if (OidIsValid(idxoid)) - return idxoid; + { + entry->localindexoid = idxoid; + entry->isIdxSafeToSkipDuplicates = true; + return; + } if (remoterel->replident == REPLICA_IDENTITY_FULL) { @@ -919,8 +1017,8 @@ FindLogicalRepLocalIndex(Relation localrel, LogicalRepRelation *remoterel, * long run or use the full-fledged planner which could cause * overhead. */ - return FindUsableIndexForReplicaIdentityFull(localrel, attrmap); + FindUsableIndexForReplicaIdentityFull(entry); } - return InvalidOid; + return; } diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index 10f9711972..234a688efc 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -401,16 +401,19 @@ static void apply_handle_update_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo, TupleTableSlot *remoteslot, LogicalRepTupleData *newtup, - Oid localindexoid); + Oid localindexoid, + bool isIdxSafeToSkipDuplicates); static void apply_handle_delete_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo, TupleTableSlot *remoteslot, - Oid localindexoid); + Oid localindexoid, + bool isIdxSafeToSkipDuplicates); static bool FindReplTupleInLocalRel(EState *estate, Relation localrel, LogicalRepRelation *remoterel, Oid localidxoid, TupleTableSlot *remoteslot, - TupleTableSlot **localslot); + TupleTableSlot **localslot, + bool isIdxSafeToSkipDuplicates); static void apply_handle_tuple_routing(ApplyExecutionData *edata, TupleTableSlot *remoteslot, LogicalRepTupleData *newtup, @@ -2612,7 +2615,8 @@ apply_handle_update(StringInfo s) remoteslot, &newtup, CMD_UPDATE); else apply_handle_update_internal(edata, edata->targetRelInfo, - remoteslot, &newtup, rel->localindexoid); + remoteslot, &newtup, rel->localindexoid, + rel->isIdxSafeToSkipDuplicates); finish_edata(edata); @@ -2634,7 +2638,8 @@ apply_handle_update_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo, TupleTableSlot *remoteslot, LogicalRepTupleData *newtup, - Oid localindexoid) + Oid localindexoid, + bool isIdxSafeToSkipDuplicates) { EState *estate = edata->estate; LogicalRepRelMapEntry *relmapentry = edata->targetRel; @@ -2650,7 +2655,8 @@ apply_handle_update_internal(ApplyExecutionData *edata, found = FindReplTupleInLocalRel(estate, localrel, &relmapentry->remoterel, localindexoid, - remoteslot, &localslot); + remoteslot, &localslot, + isIdxSafeToSkipDuplicates); ExecClearTuple(remoteslot); /* @@ -2754,7 +2760,8 @@ apply_handle_delete(StringInfo s) remoteslot, NULL, CMD_DELETE); else apply_handle_delete_internal(edata, edata->targetRelInfo, - remoteslot, rel->localindexoid); + remoteslot, rel->localindexoid, + rel->isIdxSafeToSkipDuplicates); finish_edata(edata); @@ -2775,7 +2782,8 @@ static void apply_handle_delete_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo, TupleTableSlot *remoteslot, - Oid localindexoid) + Oid localindexoid, + bool isIdxSafeToSkipDuplicates) { EState *estate = edata->estate; Relation localrel = relinfo->ri_RelationDesc; @@ -2788,7 +2796,8 @@ apply_handle_delete_internal(ApplyExecutionData *edata, ExecOpenIndices(relinfo, false); found = FindReplTupleInLocalRel(estate, localrel, remoterel, localindexoid, - remoteslot, &localslot); + remoteslot, &localslot, + isIdxSafeToSkipDuplicates); /* If found delete it. */ if (found) @@ -2830,7 +2839,8 @@ FindReplTupleInLocalRel(EState *estate, Relation localrel, LogicalRepRelation *remoterel, Oid localidxoid, TupleTableSlot *remoteslot, - TupleTableSlot **localslot) + TupleTableSlot **localslot, + bool isIdxSafeToSkipDuplicates) { bool found; @@ -2847,6 +2857,7 @@ FindReplTupleInLocalRel(EState *estate, Relation localrel, if (OidIsValid(localidxoid)) found = RelationFindReplTupleByIndex(localrel, localidxoid, + isIdxSafeToSkipDuplicates, LockTupleExclusive, remoteslot, *localslot); else @@ -2948,7 +2959,8 @@ apply_handle_tuple_routing(ApplyExecutionData *edata, case CMD_DELETE: apply_handle_delete_internal(edata, partrelinfo, remoteslot_part, - part_entry->localindexoid); + part_entry->localindexoid, + part_entry->isIdxSafeToSkipDuplicates); break; case CMD_UPDATE: @@ -2969,7 +2981,8 @@ apply_handle_tuple_routing(ApplyExecutionData *edata, found = FindReplTupleInLocalRel(estate, partrel, &part_entry->remoterel, part_entry->localindexoid, - remoteslot_part, &localslot); + remoteslot_part, &localslot, + part_entry->isIdxSafeToSkipDuplicates); if (!found) { /* @@ -3066,7 +3079,8 @@ apply_handle_tuple_routing(ApplyExecutionData *edata, /* DELETE old tuple found in the old partition. */ apply_handle_delete_internal(edata, partrelinfo, localslot, - part_entry->localindexoid); + part_entry->localindexoid, + part_entry->isIdxSafeToSkipDuplicates); /* INSERT new tuple into the new partition. */ diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index 946abc0051..78b7ba96b5 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -634,7 +634,9 @@ extern void check_exclusion_constraint(Relation heap, Relation index, /* * prototypes from functions in execReplication.c */ +#include "replication/logicalproto.h" extern bool RelationFindReplTupleByIndex(Relation rel, Oid idxoid, + bool isIdxSafeToSkipDuplicates, LockTupleMode lockmode, TupleTableSlot *searchslot, TupleTableSlot *outslot); diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h index e9fc368cdb..c7acafa522 100644 --- a/src/include/replication/logicalrelation.h +++ b/src/include/replication/logicalrelation.h @@ -33,6 +33,7 @@ typedef struct LogicalRepRelMapEntry AttrMap *attrmap; /* map of local attributes to remote ones */ bool updatable; /* Can apply updates/deletes? */ Oid localindexoid; /* which index to use, or InvalidOid if none */ + bool isIdxSafeToSkipDuplicates; /* Sync state. */ char state; @@ -49,7 +50,8 @@ extern LogicalRepRelMapEntry *logicalrep_partition_open(LogicalRepRelMapEntry *r extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel, LOCKMODE lockmode); extern bool IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo); +extern bool AllIndexColumnsProvided(IndexInfo *indexInfo, LogicalRepRelation *remoterel); extern Oid GetRelationIdentityOrPK(Relation rel); -extern bool IsIdxSafeToSkipDuplicates(Relation rel, Oid idxoid); +extern bool IndexHasPkeyCharacterstics(IndexInfo *indexInfo, Relation localrel); #endif /* LOGICALRELATION_H */ -- 2.30.0.windows.2
From 35f72956482c84fa592c49f629933694e10fad1e Mon Sep 17 00:00:00 2001 From: Hou Zhijie <houzj.f...@cn.fujitsu.com> Date: Mon, 13 Mar 2023 17:36:32 +0800 Subject: [PATCH 1/2] Fix column order problem --- src/backend/replication/logical/relation.c | 23 ++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c index 7364bc3f07..ddabab0e01 100644 --- a/src/backend/replication/logical/relation.c +++ b/src/backend/replication/logical/relation.c @@ -53,7 +53,8 @@ typedef struct LogicalRepPartMapEntry } LogicalRepPartMapEntry; static Oid FindLogicalRepLocalIndex(Relation localrel, - LogicalRepRelation *remoterel); + LogicalRepRelation *remoterel, + AttrMap *attrmap); /* * Relcache invalidation callback for our relation map cache. @@ -450,7 +451,9 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode) * of the relation cache entry (such as ANALYZE or CREATE/DROP index * on the relation). */ - entry->localindexoid = FindLogicalRepLocalIndex(entry->localrel, remoterel); + entry->localindexoid = FindLogicalRepLocalIndex(entry->localrel, + remoterel, + entry->attrmap); entry->localrelvalid = true; } @@ -722,7 +725,7 @@ logicalrep_partition_open(LogicalRepRelMapEntry *root, * We also prefer to run this code on the oldctx so that we do not leak * anything in the LogicalRepPartMapContext (hence CacheMemoryContext). */ - entry->localindexoid = FindLogicalRepLocalIndex(partrel, remoterel); + entry->localindexoid = FindLogicalRepLocalIndex(partrel, remoterel, entry->attrmap); entry->localrelvalid = true; @@ -758,7 +761,7 @@ IsIndexOnlyOnExpression(IndexInfo *indexInfo) */ static bool RemoteRelContainsLeftMostColumnOnIdx(IndexInfo *indexInfo, - LogicalRepRelation *remoterel) + AttrMap *attrmap) { AttrNumber keycol; @@ -769,7 +772,7 @@ RemoteRelContainsLeftMostColumnOnIdx(IndexInfo *indexInfo, if (!AttributeNumberIsValid(keycol)) return false; - return bms_is_member(keycol-1, remoterel->attkeys); + return attrmap->attnums[AttrNumberGetAttrOffset(keycol)] >= 0; } /* @@ -801,8 +804,7 @@ RemoteRelContainsLeftMostColumnOnIdx(IndexInfo *indexInfo, * If no suitable index is found, returns InvalidOid. */ static Oid -FindUsableIndexForReplicaIdentityFull(Relation localrel, - LogicalRepRelation *remoterel) +FindUsableIndexForReplicaIdentityFull(Relation localrel, AttrMap *attrmap) { List *idxlist = RelationGetIndexList(localrel); ListCell *lc; @@ -819,7 +821,7 @@ FindUsableIndexForReplicaIdentityFull(Relation localrel, idxInfo = BuildIndexInfo(idxRel); isUsableIdx = IsIndexUsableForReplicaIdentityFull(idxInfo); containsLeftMostCol = - RemoteRelContainsLeftMostColumnOnIdx(idxInfo, remoterel); + RemoteRelContainsLeftMostColumnOnIdx(idxInfo, attrmap); index_close(idxRel, AccessShareLock); /* Return the first eligible index found */ @@ -881,7 +883,8 @@ IsIdxSafeToSkipDuplicates(Relation rel, Oid idxoid) * returns InvalidOid. */ static Oid -FindLogicalRepLocalIndex(Relation localrel, LogicalRepRelation *remoterel) +FindLogicalRepLocalIndex(Relation localrel, LogicalRepRelation *remoterel, + AttrMap *attrmap) { Oid idxoid; @@ -916,7 +919,7 @@ FindLogicalRepLocalIndex(Relation localrel, LogicalRepRelation *remoterel) * long run or use the full-fledged planner which could cause * overhead. */ - return FindUsableIndexForReplicaIdentityFull(localrel, remoterel); + return FindUsableIndexForReplicaIdentityFull(localrel, attrmap); } return InvalidOid; -- 2.30.0.windows.2