Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ljnrf-0003do-La for pgsql-hackers@arkaria.postgresql.org; Thu, 20 May 2021 18:57:47 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1ljnre-0001fx-Gf for pgsql-hackers@arkaria.postgresql.org; Thu, 20 May 2021 18:57:46 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ljnre-0001fp-6m for pgsql-hackers@lists.postgresql.org; Thu, 20 May 2021 18:57:46 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ljnrb-0000ik-Bt for pgsql-hackers@lists.postgresql.org; Thu, 20 May 2021 18:57:45 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 14KIvenQ3790836; Thu, 20 May 2021 14:57:40 -0400 From: Tom Lane To: Michael Paquier cc: Amit Langote , Amit Kapila , PostgreSQL Hackers Subject: Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS In-reply-to: <3566941.1621471774@sss.pgh.pa.us> References: <3382681.1621381328@sss.pgh.pa.us> <3392911.1621395985@sss.pgh.pa.us> <3508426.1621455835@sss.pgh.pa.us> <3566941.1621471774@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Wed, 19 May 2021 20:49:34 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <3790815.1621537035.0@sss.pgh.pa.us> Date: Thu, 20 May 2021 14:57:40 -0400 Message-ID: <3790835.1621537060@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <3790815.1621537035.1@sss.pgh.pa.us> I wrote: > Michael Paquier writes: >> On Wed, May 19, 2021 at 04:23:55PM -0400, Tom Lane wrote: >>> * Replace the edata->resultRelInfo field with two fields, one for >>> the original parent and one for the actual/current target. Perhaps >>> this is worth doing, not sure. >> This one sounds more natural to me, though. > OK, I'll give that a try tomorrow. Here's a version that does it like that. I'm not entirely convinced whether this is better or not. regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="postpone-tuple-routing-cleanup-2.patch"; charset="us-ascii" Content-ID: <3790815.1621537035.2@sss.pgh.pa.us> Content-Description: postpone-tuple-routing-cleanup-2.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/replication/logical/worker.c b/src/backend/replic= ation/logical/worker.c index 1432554d5a..d19269ebce 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -178,6 +178,18 @@ static HTAB *xidhash =3D NULL; /* BufFile handle of the current streaming file */ static BufFile *stream_fd =3D NULL; = +typedef struct ApplyExecutionData +{ + EState *estate; /* executor state, used to track resources */ + ResultRelInfo *origRelInfo; /* the originally-named target relation */ + ResultRelInfo *activeRelInfo; /* the actual target relation */ + /* activeRelInfo can equal origRelInfo, or be for a partition of it */ + + /* These fields are used when the target relation is partitioned: */ + ModifyTableState *mtstate; /* dummy ModifyTable state */ + PartitionTupleRouting *proute; /* partition routing info */ +} ApplyExecutionData; + typedef struct SubXactInfo { TransactionId xid; /* XID of the subxact */ @@ -226,21 +238,20 @@ static void apply_dispatch(StringInfo s); = static void apply_handle_commit_internal(StringInfo s, LogicalRepCommitData *commit_data); -static void apply_handle_insert_internal(ResultRelInfo *relinfo, - EState *estate, TupleTableSlot *remoteslot); -static void apply_handle_update_internal(ResultRelInfo *relinfo, - EState *estate, TupleTableSlot *remoteslot, +static void apply_handle_insert_internal(ApplyExecutionData *edata, + TupleTableSlot *remoteslot); +static void apply_handle_update_internal(ApplyExecutionData *edata, + TupleTableSlot *remoteslot, LogicalRepTupleData *newtup, LogicalRepRelMapEntry *relmapentry); -static void apply_handle_delete_internal(ResultRelInfo *relinfo, EState *= estate, +static void apply_handle_delete_internal(ApplyExecutionData *edata, TupleTableSlot *remoteslot, LogicalRepRelation *remoterel); static bool FindReplTupleInLocalRel(EState *estate, Relation localrel, LogicalRepRelation *remoterel, TupleTableSlot *remoteslot, TupleTableSlot **localslot); -static void apply_handle_tuple_routing(ResultRelInfo *relinfo, - EState *estate, +static void apply_handle_tuple_routing(ApplyExecutionData *edata, TupleTableSlot *remoteslot, LogicalRepTupleData *newtup, LogicalRepRelMapEntry *relmapentry, @@ -336,20 +347,21 @@ handle_streamed_transaction(LogicalRepMsgType action= , StringInfo s) = /* * Executor state preparation for evaluation of constraint expressions, - * indexes and triggers. + * indexes and triggers for the specified relation. * - * resultRelInfo is a ResultRelInfo for the relation to be passed to the - * executor routines. The caller must open and close any indexes to be - * updated independently of the relation registered here. + * Note that the caller must open and close any indexes to be updated. */ -static EState * -create_estate_for_relation(LogicalRepRelMapEntry *rel, - ResultRelInfo **resultRelInfo) +static ApplyExecutionData * +create_edata_for_relation(LogicalRepRelMapEntry *rel) { + ApplyExecutionData *edata; EState *estate; RangeTblEntry *rte; + ResultRelInfo *resultRelInfo; = - estate =3D CreateExecutorState(); + edata =3D (ApplyExecutionData *) palloc0(sizeof(ApplyExecutionData)); + + edata->estate =3D estate =3D CreateExecutorState(); = rte =3D makeNode(RangeTblEntry); rte->rtekind =3D RTE_RELATION; @@ -358,13 +370,16 @@ create_estate_for_relation(LogicalRepRelMapEntry *re= l, rte->rellockmode =3D AccessShareLock; ExecInitRangeTable(estate, list_make1(rte)); = - *resultRelInfo =3D makeNode(ResultRelInfo); + edata->origRelInfo =3D resultRelInfo =3D makeNode(ResultRelInfo); + + /* Initially, set activeRelInfo to be the named rel */ + edata->activeRelInfo =3D resultRelInfo; = /* * Use Relation opened by logicalrep_rel_open() instead of opening it * again. */ - InitResultRelInfo(*resultRelInfo, rel->localrel, 1, NULL, 0); + InitResultRelInfo(resultRelInfo, rel->localrel, 1, NULL, 0); = /* * We put the ResultRelInfo in the es_opened_result_relations list, even @@ -377,29 +392,38 @@ create_estate_for_relation(LogicalRepRelMapEntry *re= l, * an apply operation being responsible for that. */ estate->es_opened_result_relations =3D - lappend(estate->es_opened_result_relations, *resultRelInfo); + lappend(estate->es_opened_result_relations, resultRelInfo); = estate->es_output_cid =3D GetCurrentCommandId(true); = /* Prepare to catch AFTER triggers. */ AfterTriggerBeginQuery(); = - return estate; + /* other fields of edata remain NULL for now */ + + return edata; } = /* * Finish any operations related to the executor state created by - * create_estate_for_relation(). + * create_edata_for_relation(). */ static void -finish_estate(EState *estate) +finish_edata(ApplyExecutionData *edata) { + EState *estate =3D edata->estate; + /* Handle any queued AFTER triggers. */ AfterTriggerEndQuery(estate); = + /* Shut down tuple routing, if any was done. */ + if (edata->proute) + ExecCleanupTupleRouting(edata->mtstate, edata->proute); + /* Cleanup. */ ExecResetTupleTable(estate->es_tupleTable, false); FreeExecutorState(estate); + pfree(edata); } = /* @@ -1181,10 +1205,10 @@ GetRelationIdentityOrPK(Relation rel) static void apply_handle_insert(StringInfo s) { - ResultRelInfo *resultRelInfo; LogicalRepRelMapEntry *rel; LogicalRepTupleData newtup; LogicalRepRelId relid; + ApplyExecutionData *edata; EState *estate; TupleTableSlot *remoteslot; MemoryContext oldctx; @@ -1207,7 +1231,8 @@ apply_handle_insert(StringInfo s) } = /* Initialize the executor state. */ - estate =3D create_estate_for_relation(rel, &resultRelInfo); + edata =3D create_edata_for_relation(rel); + estate =3D edata->estate; remoteslot =3D ExecInitExtraTupleSlot(estate, RelationGetDescr(rel->localrel), &TTSOpsVirtual); @@ -1223,15 +1248,14 @@ apply_handle_insert(StringInfo s) = /* For a partitioned table, insert the tuple into a partition. */ if (rel->localrel->rd_rel->relkind =3D=3D RELKIND_PARTITIONED_TABLE) - apply_handle_tuple_routing(resultRelInfo, estate, + apply_handle_tuple_routing(edata, remoteslot, NULL, rel, CMD_INSERT); else - apply_handle_insert_internal(resultRelInfo, estate, - remoteslot); + apply_handle_insert_internal(edata, remoteslot); = PopActiveSnapshot(); = - finish_estate(estate); + finish_edata(edata); = logicalrep_rel_close(rel, NoLock); = @@ -1240,9 +1264,13 @@ apply_handle_insert(StringInfo s) = /* Workhorse for apply_handle_insert() */ static void -apply_handle_insert_internal(ResultRelInfo *relinfo, - EState *estate, TupleTableSlot *remoteslot) +apply_handle_insert_internal(ApplyExecutionData *edata, + TupleTableSlot *remoteslot) { + EState *estate =3D edata->estate; + ResultRelInfo *relinfo =3D edata->activeRelInfo; + + /* We must open indexes here. */ ExecOpenIndices(relinfo, false); = /* Do the insert. */ @@ -1293,9 +1321,9 @@ check_relation_updatable(LogicalRepRelMapEntry *rel) static void apply_handle_update(StringInfo s) { - ResultRelInfo *resultRelInfo; LogicalRepRelMapEntry *rel; LogicalRepRelId relid; + ApplyExecutionData *edata; EState *estate; LogicalRepTupleData oldtup; LogicalRepTupleData newtup; @@ -1326,7 +1354,8 @@ apply_handle_update(StringInfo s) check_relation_updatable(rel); = /* Initialize the executor state. */ - estate =3D create_estate_for_relation(rel, &resultRelInfo); + edata =3D create_edata_for_relation(rel); + estate =3D edata->estate; remoteslot =3D ExecInitExtraTupleSlot(estate, RelationGetDescr(rel->localrel), &TTSOpsVirtual); @@ -1368,15 +1397,15 @@ apply_handle_update(StringInfo s) = /* For a partitioned table, apply update to correct partition. */ if (rel->localrel->rd_rel->relkind =3D=3D RELKIND_PARTITIONED_TABLE) - apply_handle_tuple_routing(resultRelInfo, estate, + apply_handle_tuple_routing(edata, remoteslot, &newtup, rel, CMD_UPDATE); else - apply_handle_update_internal(resultRelInfo, estate, + apply_handle_update_internal(edata, remoteslot, &newtup, rel); = PopActiveSnapshot(); = - finish_estate(estate); + finish_edata(edata); = logicalrep_rel_close(rel, NoLock); = @@ -1385,11 +1414,13 @@ apply_handle_update(StringInfo s) = /* Workhorse for apply_handle_update() */ static void -apply_handle_update_internal(ResultRelInfo *relinfo, - EState *estate, TupleTableSlot *remoteslot, +apply_handle_update_internal(ApplyExecutionData *edata, + TupleTableSlot *remoteslot, LogicalRepTupleData *newtup, LogicalRepRelMapEntry *relmapentry) { + EState *estate =3D edata->estate; + ResultRelInfo *relinfo =3D edata->activeRelInfo; Relation localrel =3D relinfo->ri_RelationDesc; EPQState epqstate; TupleTableSlot *localslot; @@ -1448,10 +1479,10 @@ apply_handle_update_internal(ResultRelInfo *relinf= o, static void apply_handle_delete(StringInfo s) { - ResultRelInfo *resultRelInfo; LogicalRepRelMapEntry *rel; LogicalRepTupleData oldtup; LogicalRepRelId relid; + ApplyExecutionData *edata; EState *estate; TupleTableSlot *remoteslot; MemoryContext oldctx; @@ -1477,7 +1508,8 @@ apply_handle_delete(StringInfo s) check_relation_updatable(rel); = /* Initialize the executor state. */ - estate =3D create_estate_for_relation(rel, &resultRelInfo); + edata =3D create_edata_for_relation(rel); + estate =3D edata->estate; remoteslot =3D ExecInitExtraTupleSlot(estate, RelationGetDescr(rel->localrel), &TTSOpsVirtual); @@ -1491,15 +1523,14 @@ apply_handle_delete(StringInfo s) = /* For a partitioned table, apply delete to correct partition. */ if (rel->localrel->rd_rel->relkind =3D=3D RELKIND_PARTITIONED_TABLE) - apply_handle_tuple_routing(resultRelInfo, estate, + apply_handle_tuple_routing(edata, remoteslot, NULL, rel, CMD_DELETE); else - apply_handle_delete_internal(resultRelInfo, estate, - remoteslot, &rel->remoterel); + apply_handle_delete_internal(edata, remoteslot, &rel->remoterel); = PopActiveSnapshot(); = - finish_estate(estate); + finish_edata(edata); = logicalrep_rel_close(rel, NoLock); = @@ -1508,10 +1539,12 @@ apply_handle_delete(StringInfo s) = /* Workhorse for apply_handle_delete() */ static void -apply_handle_delete_internal(ResultRelInfo *relinfo, EState *estate, +apply_handle_delete_internal(ApplyExecutionData *edata, TupleTableSlot *remoteslot, LogicalRepRelation *remoterel) { + EState *estate =3D edata->estate; + ResultRelInfo *relinfo =3D edata->activeRelInfo; Relation localrel =3D relinfo->ri_RelationDesc; EPQState epqstate; TupleTableSlot *localslot; @@ -1582,16 +1615,17 @@ FindReplTupleInLocalRel(EState *estate, Relation l= ocalrel, * This handles insert, update, delete on a partitioned table. */ static void -apply_handle_tuple_routing(ResultRelInfo *relinfo, - EState *estate, +apply_handle_tuple_routing(ApplyExecutionData *edata, TupleTableSlot *remoteslot, LogicalRepTupleData *newtup, LogicalRepRelMapEntry *relmapentry, CmdType operation) { + EState *estate =3D edata->estate; + ResultRelInfo *relinfo =3D edata->origRelInfo; Relation parentrel =3D relinfo->ri_RelationDesc; - ModifyTableState *mtstate =3D NULL; - PartitionTupleRouting *proute =3D NULL; + ModifyTableState *mtstate; + PartitionTupleRouting *proute; ResultRelInfo *partrelinfo; Relation partrel; TupleTableSlot *remoteslot_part; @@ -1599,12 +1633,14 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo, MemoryContext oldctx; = /* ModifyTableState is needed for ExecFindPartition(). */ - mtstate =3D makeNode(ModifyTableState); + edata->mtstate =3D mtstate =3D makeNode(ModifyTableState); mtstate->ps.plan =3D NULL; mtstate->ps.state =3D estate; mtstate->operation =3D operation; mtstate->resultRelInfo =3D relinfo; - proute =3D ExecSetupPartitionTupleRouting(estate, parentrel); + + /* ... as is PartitionTupleRouting. */ + edata->proute =3D proute =3D ExecSetupPartitionTupleRouting(estate, pare= ntrel); = /* * Find the partition to which the "search tuple" belongs. @@ -1616,6 +1652,9 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo, Assert(partrelinfo !=3D NULL); partrel =3D partrelinfo->ri_RelationDesc; = + /* Make that partition the active target rel. */ + edata->activeRelInfo =3D partrelinfo; + /* * To perform any of the operations below, the tuple must match the * partition's rowtype. Convert if needed or just copy, using a dedicate= d @@ -1638,12 +1677,12 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo, switch (operation) { case CMD_INSERT: - apply_handle_insert_internal(partrelinfo, estate, + apply_handle_insert_internal(edata, remoteslot_part); break; = case CMD_DELETE: - apply_handle_delete_internal(partrelinfo, estate, + apply_handle_delete_internal(edata, remoteslot_part, &relmapentry->remoterel); break; @@ -1757,11 +1796,12 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo, Assert(partrelinfo_new !=3D partrelinfo); = /* DELETE old tuple found in the old partition. */ - apply_handle_delete_internal(partrelinfo, estate, + apply_handle_delete_internal(edata, localslot, &relmapentry->remoterel); = /* INSERT new tuple into the new partition. */ + edata->activeRelInfo =3D partrelinfo_new; = /* * Convert the replacement tuple to match the destination @@ -1787,7 +1827,7 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo, slot_getallattrs(remoteslot); } MemoryContextSwitchTo(oldctx); - apply_handle_insert_internal(partrelinfo_new, estate, + apply_handle_insert_internal(edata, remoteslot_part); } } @@ -1797,8 +1837,6 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo, elog(ERROR, "unrecognized CmdType: %d", (int) operation); break; } - - ExecCleanupTupleRouting(mtstate, proute); } = /* ------- =_aaaaaaaaaa0--