From 7c6b4ce1035491ff5a31a9d15bb8b28f3c0dd5b3 Mon Sep 17 00:00:00 2001 From: Naisila Puka <37271756+naisila@users.noreply.github.com> Date: Fri, 4 Aug 2023 14:51:28 +0300 Subject: [PATCH] PG16 compatibility - outer join checks, subscription password, crash fixes (#7097) PG16 compatibility - Part 4 Check out part 1 https://github.com/citusdata/citus/commit/42d956888d5be65153ccf24cb039027ecd7c0192 part 2 https://github.com/citusdata/citus/commit/0d503dd5ac5547ca71cd0147e53236d8d8a22fce part 3 https://github.com/citusdata/citus/commit/907d72e60d4043924f52200b24d281fe7b79f75f This commit is in the series of PG16 compatibility commits. It adds some outer join checks to the planner, the new password_required option to the subscription, and a crash fix related to PGIOAlignedBlock, see below for more details: - Fix PGIOAlignedBlock Assert crash in PG16 Relevant PG commit: https://github.com/postgres/postgres/commit/faeedbcefd40bfdf314e048c425b6d9208896d90 faeedbcefd40bfdf314e048c425b6d9208896d90 - Pass planner info as argument to make_simple_restrictinfo Pre PG16 passing plannerInfo to make_simple_restrictinfo was only needed for placeholder Vars, which is not the case in this part of the codebase because we are building the expression from shard intervals which don't have placeholder vars. However, PG16 is counting baserels appearing in clause_relids and is deleting the rels mentioned in plannerinfo->outer_join_rels Hence directly accessing plannerinfo. We will crash if we leave it as NULL. For reference https://github.com/postgres/postgres/commit/2489d76c4906f4461a364ca8ad7e0751ead8aa0d#diff-e045c41eda9686451a7993e91518e40056b3739365e39eb1b70ae438dc1f7c76R207 Relevant PG commit: https://github.com/postgres/postgres/commit/2489d76c4906f4461a364ca8ad7e0751ead8aa0d 2489d76c4906f4461a364ca8ad7e0751ead8aa0d - Add outer join checks, root->simple_rel_array - fix rebalancer to include passwork_required option Relevant PG commit: https://github.com/postgres/postgres/commit/c3afe8cf5a1e465bd71e48e4bc717f5bfdc7a7d6 c3afe8cf5a1e465bd71e48e4bc717f5bfdc7a7d6 More PG16 compatibility commits are coming soon ... --- src/backend/columnar/columnar_storage.c | 4 +++ .../planner/insert_select_planner.c | 10 +----- .../relation_restriction_equivalence.c | 32 +++++++++++++++++++ .../replication/multi_logical_replication.c | 16 ++++++++++ src/test/regress/bin/normalize.sed | 9 ++++++ 5 files changed, 62 insertions(+), 9 deletions(-) diff --git a/src/backend/columnar/columnar_storage.c b/src/backend/columnar/columnar_storage.c index 7e7f7990b..21aa7ab9c 100644 --- a/src/backend/columnar/columnar_storage.c +++ b/src/backend/columnar/columnar_storage.c @@ -169,7 +169,11 @@ ColumnarStorageInit(SMgrRelation srel, uint64 storageId) } /* create two pages */ +#if PG_VERSION_NUM >= PG_VERSION_16 + PGIOAlignedBlock block; +#else PGAlignedBlock block; +#endif Page page = block.data; /* write metapage */ diff --git a/src/backend/distributed/planner/insert_select_planner.c b/src/backend/distributed/planner/insert_select_planner.c index f990a06bc..b71d95432 100644 --- a/src/backend/distributed/planner/insert_select_planner.c +++ b/src/backend/distributed/planner/insert_select_planner.c @@ -901,15 +901,7 @@ RouterModifyTaskForShardInterval(Query *originalQuery, continue; } - - /* - * passing NULL for plannerInfo will be problematic if we have placeholder - * vars. However, it won't be the case here because we are building - * the expression from shard intervals which don't have placeholder vars. - * Note that this is only the case with PG14 as the parameter doesn't exist - * prior to that. - */ - shardRestrictionList = make_simple_restrictinfo(NULL, + shardRestrictionList = make_simple_restrictinfo(restriction->plannerInfo, (Expr *) shardOpExpressions); extendedBaseRestrictInfo = lappend(extendedBaseRestrictInfo, shardRestrictionList); diff --git a/src/backend/distributed/planner/relation_restriction_equivalence.c b/src/backend/distributed/planner/relation_restriction_equivalence.c index b57e37735..af2443b19 100644 --- a/src/backend/distributed/planner/relation_restriction_equivalence.c +++ b/src/backend/distributed/planner/relation_restriction_equivalence.c @@ -171,6 +171,8 @@ static bool FindQueryContainingRTEIdentityInternal(Node *node, static int ParentCountPriorToAppendRel(List *appendRelList, AppendRelInfo *appendRelInfo); +static bool IsVarRelOptOuterJoin(PlannerInfo *root, Var *varToBeAdded); + /* * AllDistributionKeysInQueryAreEqual returns true if either @@ -1238,6 +1240,12 @@ AddToAttributeEquivalenceClass(AttributeEquivalenceClass *attributeEquivalenceCl return; } + /* outer join checks in PG16 */ + if (IsVarRelOptOuterJoin(root, varToBeAdded)) + { + return; + } + RangeTblEntry *rangeTableEntry = root->simple_rte_array[varToBeAdded->varno]; if (rangeTableEntry->rtekind == RTE_RELATION) { @@ -1379,6 +1387,30 @@ GetTargetSubquery(PlannerInfo *root, RangeTblEntry *rangeTableEntry, Var *varToB } +/* + * IsVarRelOptOuterJoin returns true if the Var to be added + * is an outer join, false otherwise. + */ +static bool +IsVarRelOptOuterJoin(PlannerInfo *root, Var *varToBeAdded) +{ +#if PG_VERSION_NUM >= PG_VERSION_16 + if (root->simple_rel_array_size <= varToBeAdded->varno) + { + return true; + } + + RelOptInfo *rel = root->simple_rel_array[varToBeAdded->varno]; + if (rel == NULL) + { + /* must be an outer join */ + return true; + } +#endif + return false; +} + + /* * AddUnionAllSetOperationsToAttributeEquivalenceClass recursively iterates on all the * append rels, sets the varno's accordingly and adds the diff --git a/src/backend/distributed/replication/multi_logical_replication.c b/src/backend/distributed/replication/multi_logical_replication.c index 550095875..c8d0325b6 100644 --- a/src/backend/distributed/replication/multi_logical_replication.c +++ b/src/backend/distributed/replication/multi_logical_replication.c @@ -1530,7 +1530,23 @@ CreateSubscriptions(MultiConnection *sourceConnection, appendStringInfo(createSubscriptionCommand, "CREATE SUBSCRIPTION %s CONNECTION %s PUBLICATION %s " "WITH (citus_use_authinfo=true, create_slot=false, " +#if PG_VERSION_NUM >= PG_VERSION_16 + + /* + * password_required specifies whether connections to the publisher + * made as a result of this subscription must use password authentication. + * However, this setting is ignored when the subscription is owned + * by a superuser. + * Given that this command is executed below with superuser + * ExecuteCriticalRemoteCommand(target->superuserConnection, + * createSubscriptionCommand->data); + * We are safe to pass password_required as false because + * it will be ignored anyway + */ + "copy_data=false, enabled=false, slot_name=%s, password_required=false", +#else "copy_data=false, enabled=false, slot_name=%s", +#endif quote_identifier(target->subscriptionName), quote_literal_cstr(conninfo->data), quote_identifier(target->publication->name), diff --git a/src/test/regress/bin/normalize.sed b/src/test/regress/bin/normalize.sed index a374c73f5..7bed04edf 100644 --- a/src/test/regress/bin/normalize.sed +++ b/src/test/regress/bin/normalize.sed @@ -294,3 +294,12 @@ s/\/\*\{"cId":.*\*\///g # Notice message that contains current columnar version that makes it harder to bump versions s/(NOTICE: issuing CREATE EXTENSION IF NOT EXISTS citus_columnar WITH SCHEMA pg_catalog VERSION )"[0-9]+\.[0-9]+-[0-9]+"/\1 "x.y-z"/ + +# pg16 changes +# can be removed when dropping PG14&15 support +#if PG_VERSION_NUM < PG_VERSION_16 +# (This is not preprocessor directive, but a reminder for the developer that will drop PG14&15 support ) + +s/, password_required=false//g + +#endif /* PG_VERSION_NUM < PG_VERSION_16 */