public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v10] first cut
28+ messages / 7 participants
[nested] [flat]
* [PATCH v10] first cut
@ 2021-01-01 00:42 David Fetter <[email protected]>
0 siblings, 0 replies; 28+ messages in thread
From: David Fetter @ 2021-01-01 00:42 UTC (permalink / raw)
create mode 100644 src/include/executor/nodeTidrangescan.h
create mode 100644 src/backend/executor/nodeTidrangescan.c
create mode 100644 src/test/regress/expected/tidrangescan.out
create mode 100644 src/test/regress/sql/tidrangescan.sql
Attachments:
[text/x-patch] v10-0001-first-cut.patch (59.8K, 2-v10-0001-first-cut.patch)
download | inline diff:
diff --git src/include/access/tableam.h src/include/access/tableam.h
index 387eb34a61..5776f8ba6e 100644
--- src/include/access/tableam.h
+++ src/include/access/tableam.h
@@ -218,6 +218,15 @@ typedef struct TableAmRoutine
bool set_params, bool allow_strat,
bool allow_sync, bool allow_pagemode);
+ /*
+ * Set the range of a scan.
+ *
+ * Optional callback: A table AM can implement this to enable TID range
+ * scans.
+ */
+ void (*scan_setlimits) (TableScanDesc scan,
+ BlockNumber startBlk, BlockNumber numBlks);
+
/*
* Return next tuple from `scan`, store in slot.
*/
@@ -875,6 +884,16 @@ table_rescan(TableScanDesc scan,
scan->rs_rd->rd_tableam->scan_rescan(scan, key, false, false, false, false);
}
+/*
+ * Set the range of a scan.
+ */
+static inline void
+table_scan_setlimits(TableScanDesc scan,
+ BlockNumber startBlk, BlockNumber numBlks)
+{
+ scan->rs_rd->rd_tableam->scan_setlimits(scan, startBlk, numBlks);
+}
+
/*
* Restart a relation scan after changing params.
*
diff --git src/include/catalog/pg_operator.dat src/include/catalog/pg_operator.dat
index 9c6bf6c9d1..bb7193b9e7 100644
--- src/include/catalog/pg_operator.dat
+++ src/include/catalog/pg_operator.dat
@@ -237,15 +237,15 @@
oprname => '<', oprleft => 'tid', oprright => 'tid', oprresult => 'bool',
oprcom => '>(tid,tid)', oprnegate => '>=(tid,tid)', oprcode => 'tidlt',
oprrest => 'scalarltsel', oprjoin => 'scalarltjoinsel' },
-{ oid => '2800', descr => 'greater than',
+{ oid => '2800', oid_symbol => 'TIDGreaterOperator', descr => 'greater than',
oprname => '>', oprleft => 'tid', oprright => 'tid', oprresult => 'bool',
oprcom => '<(tid,tid)', oprnegate => '<=(tid,tid)', oprcode => 'tidgt',
oprrest => 'scalargtsel', oprjoin => 'scalargtjoinsel' },
-{ oid => '2801', descr => 'less than or equal',
+{ oid => '2801', oid_symbol => 'TIDLessEqOperator', descr => 'less than or equal',
oprname => '<=', oprleft => 'tid', oprright => 'tid', oprresult => 'bool',
oprcom => '>=(tid,tid)', oprnegate => '>(tid,tid)', oprcode => 'tidle',
oprrest => 'scalarlesel', oprjoin => 'scalarlejoinsel' },
-{ oid => '2802', descr => 'greater than or equal',
+{ oid => '2802', oid_symbol => 'TIDGreaterEqOperator', descr => 'greater than or equal',
oprname => '>=', oprleft => 'tid', oprright => 'tid', oprresult => 'bool',
oprcom => '<=(tid,tid)', oprnegate => '<(tid,tid)', oprcode => 'tidge',
oprrest => 'scalargesel', oprjoin => 'scalargejoinsel' },
diff --git src/include/executor/nodeTidrangescan.h src/include/executor/nodeTidrangescan.h
new file mode 100644
index 0000000000..f0bbcc6a04
--- /dev/null
+++ src/include/executor/nodeTidrangescan.h
@@ -0,0 +1,24 @@
+/*-------------------------------------------------------------------------
+ *
+ * nodeTidrangescan.h
+ *
+ *
+ *
+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/executor/nodeTidrangescan.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef NODETIDRANGESCAN_H
+#define NODETIDRANGESCAN_H
+
+#include "nodes/execnodes.h"
+
+extern TidRangeScanState *ExecInitTidRangeScan(TidRangeScan *node,
+ EState *estate, int eflags);
+extern void ExecEndTidRangeScan(TidRangeScanState *node);
+extern void ExecReScanTidRangeScan(TidRangeScanState *node);
+
+#endif /* NODETIDRANGESCAN_H */
diff --git src/include/nodes/execnodes.h src/include/nodes/execnodes.h
index 61ba4c3666..ae58ea9eb6 100644
--- src/include/nodes/execnodes.h
+++ src/include/nodes/execnodes.h
@@ -1611,6 +1611,29 @@ typedef struct TidScanState
HeapTupleData tss_htup;
} TidScanState;
+/* ----------------
+ * TidRangeScanState information
+ *
+ * trss_tidexprs list of TidOpExpr structs (see nodeTidrangescan.c)
+ * trss_startBlock first block to scan
+ * trss_endBlock last block to scan (inclusive)
+ * trss_startOffset first offset in first block to scan or InvalidBlockNumber
+ * when the range is not set
+ * trss_endOffset last offset in last block to scan (inclusive)
+ * trss_inScan is a scan currently in progress?
+ * ----------------
+ */
+typedef struct TidRangeScanState
+{
+ ScanState ss; /* its first field is NodeTag */
+ List *trss_tidexprs;
+ BlockNumber trss_startBlock;
+ BlockNumber trss_endBlock;
+ OffsetNumber trss_startOffset;
+ OffsetNumber trss_endOffset;
+ bool trss_inScan;
+} TidRangeScanState;
+
/* ----------------
* SubqueryScanState information
*
diff --git src/include/nodes/nodes.h src/include/nodes/nodes.h
index 3684f87a88..46d8cddfee 100644
--- src/include/nodes/nodes.h
+++ src/include/nodes/nodes.h
@@ -59,6 +59,7 @@ typedef enum NodeTag
T_BitmapIndexScan,
T_BitmapHeapScan,
T_TidScan,
+ T_TidRangeScan,
T_SubqueryScan,
T_FunctionScan,
T_ValuesScan,
@@ -116,6 +117,7 @@ typedef enum NodeTag
T_BitmapIndexScanState,
T_BitmapHeapScanState,
T_TidScanState,
+ T_TidRangeScanState,
T_SubqueryScanState,
T_FunctionScanState,
T_TableFuncScanState,
@@ -229,6 +231,7 @@ typedef enum NodeTag
T_BitmapAndPath,
T_BitmapOrPath,
T_TidPath,
+ T_TidRangePath,
T_SubqueryScanPath,
T_ForeignPath,
T_CustomPath,
diff --git src/include/nodes/pathnodes.h src/include/nodes/pathnodes.h
index b4059895de..79c5f77c82 100644
--- src/include/nodes/pathnodes.h
+++ src/include/nodes/pathnodes.h
@@ -732,6 +732,7 @@ typedef struct RelOptInfo
List *joininfo; /* RestrictInfo structures for join clauses
* involving this rel */
bool has_eclass_joins; /* T means joininfo is incomplete */
+ bool has_scan_setlimits; /* Rel's table AM has scan_setlimits */
/* used by partitionwise joins: */
bool consider_partitionwise_join; /* consider partitionwise join
@@ -1323,6 +1324,18 @@ typedef struct TidPath
List *tidquals; /* qual(s) involving CTID = something */
} TidPath;
+/*
+ * TidRangePath represents a scan by a continguous range of TIDs
+ *
+ * tidrangequals is an implicitly AND'ed list of qual expressions of the form
+ * "CTID relop pseudoconstant", where relop is one of >,>=,<,<=.
+ */
+typedef struct TidRangePath
+{
+ Path path;
+ List *tidrangequals;
+} TidRangePath;
+
/*
* SubqueryScanPath represents a scan of an unflattened subquery-in-FROM
*
diff --git src/include/nodes/plannodes.h src/include/nodes/plannodes.h
index 7e6b10f86b..011fad0ac7 100644
--- src/include/nodes/plannodes.h
+++ src/include/nodes/plannodes.h
@@ -485,6 +485,19 @@ typedef struct TidScan
List *tidquals; /* qual(s) involving CTID = something */
} TidScan;
+/* ----------------
+ * tid range scan node
+ *
+ * tidrangequals is an implicitly AND'ed list of qual expressions of the form
+ * "CTID relop pseudoconstant", where relop is one of >,>=,<,<=.
+ * ----------------
+ */
+typedef struct TidRangeScan
+{
+ Scan scan;
+ List *tidrangequals; /* qual(s) involving CTID op something */
+} TidRangeScan;
+
/* ----------------
* subquery scan node
*
diff --git src/include/optimizer/cost.h src/include/optimizer/cost.h
index 8e621d2f76..be980ea6dc 100644
--- src/include/optimizer/cost.h
+++ src/include/optimizer/cost.h
@@ -83,6 +83,9 @@ extern void cost_bitmap_or_node(BitmapOrPath *path, PlannerInfo *root);
extern void cost_bitmap_tree_node(Path *path, Cost *cost, Selectivity *selec);
extern void cost_tidscan(Path *path, PlannerInfo *root,
RelOptInfo *baserel, List *tidquals, ParamPathInfo *param_info);
+extern void cost_tidrangescan(Path *path, PlannerInfo *root,
+ RelOptInfo *baserel, List *tidquals,
+ ParamPathInfo *param_info);
extern void cost_subqueryscan(SubqueryScanPath *path, PlannerInfo *root,
RelOptInfo *baserel, ParamPathInfo *param_info);
extern void cost_functionscan(Path *path, PlannerInfo *root,
diff --git src/include/optimizer/pathnode.h src/include/optimizer/pathnode.h
index 3bd7072ae8..0105f1fac4 100644
--- src/include/optimizer/pathnode.h
+++ src/include/optimizer/pathnode.h
@@ -63,6 +63,10 @@ extern BitmapOrPath *create_bitmap_or_path(PlannerInfo *root,
List *bitmapquals);
extern TidPath *create_tidscan_path(PlannerInfo *root, RelOptInfo *rel,
List *tidquals, Relids required_outer);
+extern TidRangePath *create_tidrangescan_path(PlannerInfo *root,
+ RelOptInfo *rel,
+ List *tidrangequals,
+ Relids required_outer);
extern AppendPath *create_append_path(PlannerInfo *root, RelOptInfo *rel,
List *subpaths, List *partial_subpaths,
List *pathkeys, Relids required_outer,
diff --git src/backend/access/heap/heapam_handler.c src/backend/access/heap/heapam_handler.c
index 3eea215b85..df9e14234f 100644
--- src/backend/access/heap/heapam_handler.c
+++ src/backend/access/heap/heapam_handler.c
@@ -2539,6 +2539,7 @@ static const TableAmRoutine heapam_methods = {
.scan_begin = heap_beginscan,
.scan_end = heap_endscan,
.scan_rescan = heap_rescan,
+ .scan_setlimits = heap_setscanlimits,
.scan_getnextslot = heap_getnextslot,
.parallelscan_estimate = table_block_parallelscan_estimate,
diff --git src/backend/commands/explain.c src/backend/commands/explain.c
index d797b5f53e..f4930ca8a5 100644
--- src/backend/commands/explain.c
+++ src/backend/commands/explain.c
@@ -1057,6 +1057,7 @@ ExplainPreScanNode(PlanState *planstate, Bitmapset **rels_used)
case T_IndexOnlyScan:
case T_BitmapHeapScan:
case T_TidScan:
+ case T_TidRangeScan:
case T_SubqueryScan:
case T_FunctionScan:
case T_TableFuncScan:
@@ -1223,6 +1224,9 @@ ExplainNode(PlanState *planstate, List *ancestors,
case T_TidScan:
pname = sname = "Tid Scan";
break;
+ case T_TidRangeScan:
+ pname = sname = "Tid Range Scan";
+ break;
case T_SubqueryScan:
pname = sname = "Subquery Scan";
break;
@@ -1417,6 +1421,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
case T_SampleScan:
case T_BitmapHeapScan:
case T_TidScan:
+ case T_TidRangeScan:
case T_SubqueryScan:
case T_FunctionScan:
case T_TableFuncScan:
@@ -1871,6 +1876,23 @@ ExplainNode(PlanState *planstate, List *ancestors,
planstate, es);
}
break;
+ case T_TidRangeScan:
+ {
+ /*
+ * The tidrangequals list has AND semantics, so be sure to
+ * show it as an AND condition.
+ */
+ List *tidquals = ((TidRangeScan *) plan)->tidrangequals;
+
+ if (list_length(tidquals) > 1)
+ tidquals = list_make1(make_andclause(tidquals));
+ show_scan_qual(tidquals, "TID Cond", planstate, ancestors, es);
+ show_scan_qual(plan->qual, "Filter", planstate, ancestors, es);
+ if (plan->qual)
+ show_instrumentation_count("Rows Removed by Filter", 1,
+ planstate, es);
+ }
+ break;
case T_ForeignScan:
show_scan_qual(plan->qual, "Filter", planstate, ancestors, es);
if (plan->qual)
@@ -3558,6 +3580,7 @@ ExplainTargetRel(Plan *plan, Index rti, ExplainState *es)
case T_IndexOnlyScan:
case T_BitmapHeapScan:
case T_TidScan:
+ case T_TidRangeScan:
case T_ForeignScan:
case T_CustomScan:
case T_ModifyTable:
diff --git src/backend/executor/Makefile src/backend/executor/Makefile
index f990c6473a..74ac59faa1 100644
--- src/backend/executor/Makefile
+++ src/backend/executor/Makefile
@@ -67,6 +67,7 @@ OBJS = \
nodeSubplan.o \
nodeSubqueryscan.o \
nodeTableFuncscan.o \
+ nodeTidrangescan.o \
nodeTidscan.o \
nodeUnique.o \
nodeValuesscan.o \
diff --git src/backend/executor/execAmi.c src/backend/executor/execAmi.c
index 0c10f1d35c..5de60a36ac 100644
--- src/backend/executor/execAmi.c
+++ src/backend/executor/execAmi.c
@@ -51,6 +51,7 @@
#include "executor/nodeSubplan.h"
#include "executor/nodeSubqueryscan.h"
#include "executor/nodeTableFuncscan.h"
+#include "executor/nodeTidrangescan.h"
#include "executor/nodeTidscan.h"
#include "executor/nodeUnique.h"
#include "executor/nodeValuesscan.h"
@@ -197,6 +198,10 @@ ExecReScan(PlanState *node)
ExecReScanTidScan((TidScanState *) node);
break;
+ case T_TidRangeScanState:
+ ExecReScanTidRangeScan((TidRangeScanState *) node);
+ break;
+
case T_SubqueryScanState:
ExecReScanSubqueryScan((SubqueryScanState *) node);
break;
diff --git src/backend/executor/execProcnode.c src/backend/executor/execProcnode.c
index 01b7b926bf..a0576ac41a 100644
--- src/backend/executor/execProcnode.c
+++ src/backend/executor/execProcnode.c
@@ -109,6 +109,7 @@
#include "executor/nodeSubplan.h"
#include "executor/nodeSubqueryscan.h"
#include "executor/nodeTableFuncscan.h"
+#include "executor/nodeTidrangescan.h"
#include "executor/nodeTidscan.h"
#include "executor/nodeUnique.h"
#include "executor/nodeValuesscan.h"
@@ -238,6 +239,11 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
estate, eflags);
break;
+ case T_TidRangeScan:
+ result = (PlanState *) ExecInitTidRangeScan((TidRangeScan *) node,
+ estate, eflags);
+ break;
+
case T_SubqueryScan:
result = (PlanState *) ExecInitSubqueryScan((SubqueryScan *) node,
estate, eflags);
@@ -637,6 +643,10 @@ ExecEndNode(PlanState *node)
ExecEndTidScan((TidScanState *) node);
break;
+ case T_TidRangeScanState:
+ ExecEndTidRangeScan((TidRangeScanState *) node);
+ break;
+
case T_SubqueryScanState:
ExecEndSubqueryScan((SubqueryScanState *) node);
break;
diff --git src/backend/executor/nodeTidrangescan.c src/backend/executor/nodeTidrangescan.c
new file mode 100644
index 0000000000..8a72f52074
--- /dev/null
+++ src/backend/executor/nodeTidrangescan.c
@@ -0,0 +1,580 @@
+/*-------------------------------------------------------------------------
+ *
+ * nodeTidrangescan.c
+ * Routines to support tid range scans of relations
+ *
+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ * src/backend/executor/nodeTidrangescan.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "access/relscan.h"
+#include "access/sysattr.h"
+#include "access/tableam.h"
+#include "catalog/pg_operator.h"
+#include "executor/execdebug.h"
+#include "executor/nodeTidrangescan.h"
+#include "nodes/nodeFuncs.h"
+#include "storage/bufmgr.h"
+#include "utils/rel.h"
+
+
+#define IsCTIDVar(node) \
+ ((node) != NULL && \
+ IsA((node), Var) && \
+ ((Var *) (node))->varattno == SelfItemPointerAttributeNumber && \
+ ((Var *) (node))->varlevelsup == 0)
+
+typedef enum
+{
+ TIDEXPR_UPPER_BOUND,
+ TIDEXPR_LOWER_BOUND
+} TidExprType;
+
+/* Upper or lower range bound for scan */
+typedef struct TidOpExpr
+{
+ TidExprType exprtype; /* type of op */
+ ExprState *exprstate; /* ExprState for a TID-yielding subexpr */
+ bool inclusive; /* whether op is inclusive */
+} TidOpExpr;
+
+/*
+ * For the given 'expr', build and return an appropriate TidOpExpr taking into
+ * account the expr's operator and operand order.
+ */
+static TidOpExpr *
+MakeTidOpExpr(OpExpr *expr, TidRangeScanState *tidstate)
+{
+ Node *arg1 = get_leftop((Expr *) expr);
+ Node *arg2 = get_rightop((Expr *) expr);
+ ExprState *exprstate = NULL;
+ bool invert = false;
+ TidOpExpr *tidopexpr;
+
+ if (IsCTIDVar(arg1))
+ exprstate = ExecInitExpr((Expr *) arg2, &tidstate->ss.ps);
+ else if (IsCTIDVar(arg2))
+ {
+ exprstate = ExecInitExpr((Expr *) arg1, &tidstate->ss.ps);
+ invert = true;
+ }
+ else
+ elog(ERROR, "could not identify CTID variable");
+
+ tidopexpr = (TidOpExpr *) palloc0(sizeof(TidOpExpr));
+
+ switch (expr->opno)
+ {
+ case TIDLessEqOperator:
+ tidopexpr->inclusive = true;
+ /* fall through */
+ case TIDLessOperator:
+ tidopexpr->exprtype = invert ? TIDEXPR_LOWER_BOUND : TIDEXPR_UPPER_BOUND;
+ break;
+ case TIDGreaterEqOperator:
+ tidopexpr->inclusive = true;
+ /* fall through */
+ case TIDGreaterOperator:
+ tidopexpr->exprtype = invert ? TIDEXPR_UPPER_BOUND : TIDEXPR_LOWER_BOUND;
+ break;
+ default:
+ elog(ERROR, "could not identify CTID operator");
+ }
+
+ tidopexpr->exprstate = exprstate;
+
+ return tidopexpr;
+}
+
+/*
+ * Extract the qual subexpressions that yield TIDs to search for,
+ * and compile them into ExprStates if they're ordinary expressions.
+ */
+static void
+TidExprListCreate(TidRangeScanState *tidrangestate)
+{
+ TidRangeScan *node = (TidRangeScan *) tidrangestate->ss.ps.plan;
+ List *tidexprs = NIL;
+ ListCell *l;
+
+ foreach(l, node->tidrangequals)
+ {
+ OpExpr *opexpr = lfirst(l);
+ TidOpExpr *tidopexpr;
+
+ if (!IsA(opexpr, OpExpr))
+ elog(ERROR, "could not identify CTID expression");
+
+ tidopexpr = MakeTidOpExpr(opexpr, tidrangestate);
+ tidexprs = lappend(tidexprs, tidopexpr);
+ }
+
+ tidrangestate->trss_tidexprs = tidexprs;
+}
+
+/*
+ * Set 'lowerBound' based on 'tid'. If 'inclusive' is false then the
+ * lowerBound is incremented to the next tid value so that it becomes
+ * inclusive. If there is no valid next tid value then we return false,
+ * otherwise we return true.
+ */
+static bool
+SetTidLowerBound(ItemPointer tid, bool inclusive, ItemPointer lowerBound)
+{
+ OffsetNumber offset;
+
+ *lowerBound = *tid;
+ offset = ItemPointerGetOffsetNumberNoCheck(tid);
+
+ if (!inclusive)
+ {
+ /* Check if the lower bound is actually in the next block. */
+ if (offset >= MaxOffsetNumber)
+ {
+ BlockNumber block = ItemPointerGetBlockNumberNoCheck(lowerBound);
+
+ /*
+ * If the lower bound was already at or above the maximum block
+ * number, then there is no valid value for it be set to.
+ */
+ if (block >= MaxBlockNumber)
+ return false;
+
+ /* Set the lowerBound to the first offset in the next block */
+ ItemPointerSet(lowerBound, block + 1, 1);
+ }
+ else
+ ItemPointerSetOffsetNumber(lowerBound, OffsetNumberNext(offset));
+ }
+ else if (offset == 0)
+ ItemPointerSetOffsetNumber(lowerBound, 1);
+
+ return true;
+}
+
+/*
+ * Set 'upperBound' based on 'tid'. If 'inclusive' is false then the
+ * upperBound is decremented to the previous tid value so that it becomes
+ * inclusive. If there is no valid previous tid value then we return false,
+ * otherwise we return true.
+ */
+static bool
+SetTidUpperBound(ItemPointer tid, bool inclusive, ItemPointer upperBound)
+{
+ OffsetNumber offset;
+
+ *upperBound = *tid;
+ offset = ItemPointerGetOffsetNumberNoCheck(tid);
+
+ /*
+ * Since TID offsets start at 1, an inclusive upper bound with offset 0
+ * can be treated as an exclusive bound. This has the benefit of
+ * eliminating that block from the scan range.
+ */
+ if (inclusive && offset == 0)
+ inclusive = false;
+
+ if (!inclusive)
+ {
+ /* Check if the upper bound is actually in the previous block. */
+ if (offset == 0)
+ {
+ BlockNumber block = ItemPointerGetBlockNumberNoCheck(upperBound);
+
+ /*
+ * If the upper bound was already in block 0, then there is no
+ * valid value for it to be set to.
+ */
+ if (block == 0)
+ return false;
+
+ ItemPointerSet(upperBound, block - 1, MaxOffsetNumber);
+ }
+ else
+ ItemPointerSetOffsetNumber(upperBound, OffsetNumberPrev(offset));
+ }
+
+ return true;
+}
+
+/* ----------------------------------------------------------------
+ * TidRangeEval
+ *
+ * Compute and set node's block and offset range to scan by evaluating
+ * the trss_tidexprs. If we detect an invalid range that cannot yield
+ * any rows, the range is left unset.
+ * ----------------------------------------------------------------
+ */
+static void
+TidRangeEval(TidRangeScanState *node)
+{
+ ExprContext *econtext = node->ss.ps.ps_ExprContext;
+ BlockNumber nblocks;
+ ItemPointerData lowerBound;
+ ItemPointerData upperBound;
+ ListCell *l;
+
+ /*
+ * We silently discard any TIDs that are out of range at the time of scan
+ * start. (Since we hold at least AccessShareLock on the table, it won't
+ * be possible for someone to truncate away the blocks we intend to
+ * visit.)
+ */
+ nblocks = RelationGetNumberOfBlocks(node->ss.ss_currentRelation);
+
+ /* The biggest range on an empty table is empty; just skip it. */
+ if (nblocks == 0)
+ return;
+
+ /* Set the lower and upper bound to scan the whole table. */
+ ItemPointerSet(&lowerBound, 0, 1);
+ ItemPointerSet(&upperBound, nblocks - 1, MaxOffsetNumber);
+
+ foreach(l, node->trss_tidexprs)
+ {
+ TidOpExpr *tidopexpr = (TidOpExpr *) lfirst(l);
+ ItemPointer itemptr;
+ bool isNull;
+
+ /* Evaluate this bound. */
+ itemptr = (ItemPointer)
+ DatumGetPointer(ExecEvalExprSwitchContext(tidopexpr->exprstate,
+ econtext,
+ &isNull));
+
+ /* If the bound is NULL, *nothing* matches the qual. */
+ if (isNull)
+ return;
+
+ if (tidopexpr->exprtype == TIDEXPR_LOWER_BOUND)
+ {
+ ItemPointerData lb;
+
+ /*
+ * If the lower bound is beyond the maximum value for ctid, then
+ * just bail without setting the range. No rows can match.
+ */
+ if (!SetTidLowerBound(itemptr, tidopexpr->inclusive, &lb))
+ return;
+
+ if (ItemPointerCompare(&lb, &lowerBound) > 0)
+ lowerBound = lb;
+ }
+
+ if (tidopexpr->exprtype == TIDEXPR_UPPER_BOUND)
+ {
+ ItemPointerData ub;
+
+ /*
+ * If the upper bound is below the minimum value for ctid, then
+ * just bail without setting the range. No rows can match.
+ */
+ if (!SetTidUpperBound(itemptr, tidopexpr->inclusive, &ub))
+ return;
+
+ if (ItemPointerCompare(&ub, &upperBound) < 0)
+ upperBound = ub;
+ }
+ }
+
+ /* If the resulting range is not empty, set it. */
+ if (ItemPointerCompare(&lowerBound, &upperBound) <= 0)
+ {
+ node->trss_startBlock = ItemPointerGetBlockNumberNoCheck(&lowerBound);
+ node->trss_endBlock = ItemPointerGetBlockNumberNoCheck(&upperBound);
+ node->trss_startOffset = ItemPointerGetOffsetNumberNoCheck(&lowerBound);
+ node->trss_endOffset = ItemPointerGetOffsetNumberNoCheck(&upperBound);
+ }
+}
+
+/* ----------------------------------------------------------------
+ * NextInTidRange
+ *
+ * Fetch the next tuple when scanning a range of TIDs.
+ *
+ * Since the table access method may return tuples that are in the scan
+ * limit, but not within the required TID range, this function will
+ * check for such tuples and skip over them.
+ * ----------------------------------------------------------------
+ */
+static bool
+NextInTidRange(TidRangeScanState *node, TableScanDesc scandesc,
+ TupleTableSlot *slot)
+{
+ for (;;)
+ {
+ BlockNumber block;
+ OffsetNumber offset;
+
+ if (!table_scan_getnextslot(scandesc, ForwardScanDirection, slot))
+ return false;
+
+ /* Check that the tuple is within the required range. */
+ block = ItemPointerGetBlockNumber(&slot->tts_tid);
+ offset = ItemPointerGetOffsetNumber(&slot->tts_tid);
+
+ /* The tuple should never come from outside the scan limits. */
+ Assert(block >= node->trss_startBlock &&
+ block <= node->trss_endBlock);
+
+ /*
+ * If the tuple is in the first block of the range and before the
+ * first requested offset, then we can skip it.
+ */
+ if (block == node->trss_startBlock && offset < node->trss_startOffset)
+ {
+ ExecClearTuple(slot);
+ continue;
+ }
+
+ /*
+ * Similarly, if the tuple is in the last block and after the last
+ * requested offset, we can end the scan.
+ */
+ if (block == node->trss_endBlock && offset > node->trss_endOffset)
+ {
+ ExecClearTuple(slot);
+ return false;
+ }
+
+ return true;
+ }
+}
+
+/* ----------------------------------------------------------------
+ * TidRangeNext
+ *
+ * Retrieve a tuple from the TidRangeScan node's currentRelation
+ * using the tids in the TidRangeScanState information.
+ *
+ * ----------------------------------------------------------------
+ */
+static TupleTableSlot *
+TidRangeNext(TidRangeScanState *node)
+{
+ TableScanDesc scandesc;
+ EState *estate;
+ TupleTableSlot *slot;
+ bool foundTuple;
+
+ /*
+ * extract necessary information from tid scan node
+ */
+ scandesc = node->ss.ss_currentScanDesc;
+ estate = node->ss.ps.state;
+ slot = node->ss.ss_ScanTupleSlot;
+
+ Assert(ScanDirectionIsForward(estate->es_direction));
+
+ if (!node->trss_inScan)
+ {
+ BlockNumber blocks_to_scan;
+
+ /* First time through, compute the list of TID ranges to be visited */
+ if (node->trss_startBlock == InvalidBlockNumber)
+ TidRangeEval(node);
+
+ if (scandesc == NULL)
+ {
+ scandesc = table_beginscan_strat(node->ss.ss_currentRelation,
+ estate->es_snapshot,
+ 0, NULL,
+ false, false);
+ node->ss.ss_currentScanDesc = scandesc;
+ }
+
+ /* Compute the number of blocks to scan and set the scan limits. */
+ if (node->trss_startBlock == InvalidBlockNumber)
+ {
+ /* If the range is empty, set the scan limits to zero blocks. */
+ node->trss_startBlock = 0;
+ blocks_to_scan = 0;
+ }
+ else
+ blocks_to_scan = node->trss_endBlock - node->trss_startBlock + 1;
+
+ table_scan_setlimits(scandesc, node->trss_startBlock, blocks_to_scan);
+ node->trss_inScan = true;
+ }
+
+ /* Fetch the next tuple. */
+ foundTuple = NextInTidRange(node, scandesc, slot);
+
+ /*
+ * If we've exhausted all the tuples in the range, reset the inScan flag.
+ * This will cause the heap to be rescanned for any subsequent fetches,
+ * which is important for some cursor operations: for instance, FETCH LAST
+ * fetches all the tuples in order and then fetches one tuple in reverse.
+ */
+ if (!foundTuple)
+ node->trss_inScan = false;
+
+ return slot;
+}
+
+/*
+ * TidRecheck -- access method routine to recheck a tuple in EvalPlanQual
+ */
+static bool
+TidRangeRecheck(TidRangeScanState *node, TupleTableSlot *slot)
+{
+ /*
+ * XXX shouldn't we check here to make sure tuple is in TID range? In
+ * runtime-key case this is not certain, is it?
+ */
+ return true;
+}
+
+/* ----------------------------------------------------------------
+ * ExecTidRangeScan(node)
+ *
+ * Scans the relation using tids and returns the next qualifying tuple.
+ * We call the ExecScan() routine and pass it the appropriate
+ * access method functions.
+ *
+ * Conditions:
+ * -- the "cursor" maintained by the AMI is positioned at the tuple
+ * returned previously.
+ *
+ * Initial States:
+ * -- the relation indicated is opened for scanning so that the
+ * "cursor" is positioned before the first qualifying tuple.
+ * -- trss_startBlock is InvalidBlockNumber
+ * ----------------------------------------------------------------
+ */
+static TupleTableSlot *
+ExecTidRangeScan(PlanState *pstate)
+{
+ TidRangeScanState *node = castNode(TidRangeScanState, pstate);
+
+ return ExecScan(&node->ss,
+ (ExecScanAccessMtd) TidRangeNext,
+ (ExecScanRecheckMtd) TidRangeRecheck);
+}
+
+/* ----------------------------------------------------------------
+ * ExecReScanTidRangeScan(node)
+ * ----------------------------------------------------------------
+ */
+void
+ExecReScanTidRangeScan(TidRangeScanState *node)
+{
+ TableScanDesc scan = node->ss.ss_currentScanDesc;
+
+ if (scan != NULL)
+ table_rescan(scan, NULL);
+
+ /* mark scan as not in progress, and tid range list as not computed yet */
+ node->trss_inScan = false;
+ node->trss_startBlock = InvalidBlockNumber;
+
+ ExecScanReScan(&node->ss);
+}
+
+/* ----------------------------------------------------------------
+ * ExecEndTidRangeScan
+ *
+ * Releases any storage allocated through C routines.
+ * Returns nothing.
+ * ----------------------------------------------------------------
+ */
+void
+ExecEndTidRangeScan(TidRangeScanState *node)
+{
+ TableScanDesc scan = node->ss.ss_currentScanDesc;
+
+ if (scan != NULL)
+ table_endscan(scan);
+
+ /*
+ * Free the exprcontext
+ */
+ ExecFreeExprContext(&node->ss.ps);
+
+ /*
+ * clear out tuple table slots
+ */
+ if (node->ss.ps.ps_ResultTupleSlot)
+ ExecClearTuple(node->ss.ps.ps_ResultTupleSlot);
+ ExecClearTuple(node->ss.ss_ScanTupleSlot);
+}
+
+/* ----------------------------------------------------------------
+ * ExecInitTidRangeScan
+ *
+ * Initializes the tid range scan's state information, creates
+ * scan keys, and opens the base and tid relations.
+ *
+ * Parameters:
+ * node: TidRangeScan node produced by the planner.
+ * estate: the execution state initialized in InitPlan.
+ * ----------------------------------------------------------------
+ */
+TidRangeScanState *
+ExecInitTidRangeScan(TidRangeScan *node, EState *estate, int eflags)
+{
+ TidRangeScanState *tidrangestate;
+ Relation currentRelation;
+
+ /*
+ * create state structure
+ */
+ tidrangestate = makeNode(TidRangeScanState);
+ tidrangestate->ss.ps.plan = (Plan *) node;
+ tidrangestate->ss.ps.state = estate;
+ tidrangestate->ss.ps.ExecProcNode = ExecTidRangeScan;
+
+ /*
+ * Miscellaneous initialization
+ *
+ * create expression context for node
+ */
+ ExecAssignExprContext(estate, &tidrangestate->ss.ps);
+
+ /*
+ * mark scan as not in progress, and tid range as not computed yet
+ */
+ tidrangestate->trss_inScan = false;
+ tidrangestate->trss_startBlock = InvalidBlockNumber;
+
+ /*
+ * open the scan relation
+ */
+ currentRelation = ExecOpenScanRelation(estate, node->scan.scanrelid, eflags);
+
+ tidrangestate->ss.ss_currentRelation = currentRelation;
+ tidrangestate->ss.ss_currentScanDesc = NULL; /* no table scan here */
+
+ /*
+ * get the scan type from the relation descriptor.
+ */
+ ExecInitScanTupleSlot(estate, &tidrangestate->ss,
+ RelationGetDescr(currentRelation),
+ table_slot_callbacks(currentRelation));
+
+ /*
+ * Initialize result type and projection.
+ */
+ ExecInitResultTypeTL(&tidrangestate->ss.ps);
+ ExecAssignScanProjectionInfo(&tidrangestate->ss);
+
+ /*
+ * initialize child expressions
+ */
+ tidrangestate->ss.ps.qual =
+ ExecInitQual(node->scan.plan.qual, (PlanState *) tidrangestate);
+
+ TidExprListCreate(tidrangestate);
+
+ /*
+ * all done.
+ */
+ return tidrangestate;
+}
diff --git src/backend/nodes/copyfuncs.c src/backend/nodes/copyfuncs.c
index 70f8b718e0..2abc276e1c 100644
--- src/backend/nodes/copyfuncs.c
+++ src/backend/nodes/copyfuncs.c
@@ -585,6 +585,27 @@ _copyTidScan(const TidScan *from)
return newnode;
}
+/*
+ * _copyTidRangeScan
+ */
+static TidRangeScan *
+_copyTidRangeScan(const TidRangeScan *from)
+{
+ TidRangeScan *newnode = makeNode(TidRangeScan);
+
+ /*
+ * copy node superclass fields
+ */
+ CopyScanFields((const Scan *) from, (Scan *) newnode);
+
+ /*
+ * copy remainder of node
+ */
+ COPY_NODE_FIELD(tidrangequals);
+
+ return newnode;
+}
+
/*
* _copySubqueryScan
*/
@@ -4889,6 +4910,9 @@ copyObjectImpl(const void *from)
case T_TidScan:
retval = _copyTidScan(from);
break;
+ case T_TidRangeScan:
+ retval = _copyTidRangeScan(from);
+ break;
case T_SubqueryScan:
retval = _copySubqueryScan(from);
break;
diff --git src/backend/nodes/outfuncs.c src/backend/nodes/outfuncs.c
index d78b16ed1d..93163e3a2f 100644
--- src/backend/nodes/outfuncs.c
+++ src/backend/nodes/outfuncs.c
@@ -608,6 +608,16 @@ _outTidScan(StringInfo str, const TidScan *node)
WRITE_NODE_FIELD(tidquals);
}
+static void
+_outTidRangeScan(StringInfo str, const TidRangeScan *node)
+{
+ WRITE_NODE_TYPE("TIDRANGESCAN");
+
+ _outScanInfo(str, (const Scan *) node);
+
+ WRITE_NODE_FIELD(tidrangequals);
+}
+
static void
_outSubqueryScan(StringInfo str, const SubqueryScan *node)
{
@@ -3770,6 +3780,9 @@ outNode(StringInfo str, const void *obj)
case T_TidScan:
_outTidScan(str, obj);
break;
+ case T_TidRangeScan:
+ _outTidRangeScan(str, obj);
+ break;
case T_SubqueryScan:
_outSubqueryScan(str, obj);
break;
diff --git src/backend/optimizer/README src/backend/optimizer/README
index efb52858c8..4a6c348162 100644
--- src/backend/optimizer/README
+++ src/backend/optimizer/README
@@ -374,6 +374,7 @@ RelOptInfo - a relation or joined relations
IndexPath - index scan
BitmapHeapPath - top of a bitmapped index scan
TidPath - scan by CTID
+ TidRangePath - scan a contiguous range of CTIDs
SubqueryScanPath - scan a subquery-in-FROM
ForeignPath - scan a foreign table, foreign join or foreign upper-relation
CustomPath - for custom scan providers
diff --git src/backend/optimizer/path/costsize.c src/backend/optimizer/path/costsize.c
index 22d6935824..40cd6fe460 100644
--- src/backend/optimizer/path/costsize.c
+++ src/backend/optimizer/path/costsize.c
@@ -1283,6 +1283,101 @@ cost_tidscan(Path *path, PlannerInfo *root,
path->total_cost = startup_cost + run_cost;
}
+/*
+ * cost_tidrangescan
+ * Determines and sets the costs of scanning a relation using a range of
+ * TIDs for 'path'
+ *
+ * 'baserel' is the relation to be scanned
+ * 'tidrangequals' is the list of TID-checkable range quals
+ * 'param_info' is the ParamPathInfo if this is a parameterized path, else NULL
+ */
+void
+cost_tidrangescan(Path *path, PlannerInfo *root,
+ RelOptInfo *baserel, List *tidrangequals,
+ ParamPathInfo *param_info)
+{
+ Selectivity selectivity;
+ double pages;
+ Cost startup_cost = 0;
+ Cost run_cost = 0;
+ QualCost qpqual_cost;
+ Cost cpu_per_tuple;
+ QualCost tid_qual_cost;
+ double ntuples;
+ double nseqpages;
+ double spc_random_page_cost;
+ double spc_seq_page_cost;
+
+ /* Should only be applied to base relations */
+ Assert(baserel->relid > 0);
+ Assert(baserel->rtekind == RTE_RELATION);
+
+ /* Mark the path with the correct row estimate */
+ if (param_info)
+ path->rows = param_info->ppi_rows;
+ else
+ path->rows = baserel->rows;
+
+ /* Count how many tuples and pages we expect to scan */
+ selectivity = clauselist_selectivity(root, tidrangequals, baserel->relid,
+ JOIN_INNER, NULL);
+ pages = ceil(selectivity * baserel->pages);
+
+ if (pages <= 0.0)
+ pages = 1.0;
+
+ /*
+ * The first page in a range requires a random seek, but each subsequent
+ * page is just a normal sequential page read. NOTE: it's desirable for
+ * Tid Range Scans to cost more than the equivalent Sequential Scans,
+ * because Seq Scans have some performance advantages such as scan
+ * synchronization and parallelizability, and we'd prefer one of them to
+ * be picked unless a Tid Range Scan really is better.
+ */
+ ntuples = selectivity * baserel->tuples;
+ nseqpages = pages - 1.0;
+
+ if (!enable_tidscan)
+ startup_cost += disable_cost;
+
+ /*
+ * The TID qual expressions will be computed once, any other baserestrict
+ * quals once per retrieved tuple.
+ */
+ cost_qual_eval(&tid_qual_cost, tidrangequals, root);
+
+ /* fetch estimated page cost for tablespace containing table */
+ get_tablespace_page_costs(baserel->reltablespace,
+ &spc_random_page_cost,
+ &spc_seq_page_cost);
+
+ /* disk costs; 1 random page and the remainder as seq pages */
+ run_cost += spc_random_page_cost + spc_seq_page_cost * nseqpages;
+
+ /* Add scanning CPU costs */
+ get_restriction_qual_cost(root, baserel, param_info, &qpqual_cost);
+
+ /*
+ * XXX currently we assume TID quals are a subset of qpquals at this
+ * point; they will be removed (if possible) when we create the plan, so
+ * we subtract their cost from the total qpqual cost. (If the TID quals
+ * can't be removed, this is a mistake and we're going to underestimate
+ * the CPU cost a bit.)
+ */
+ startup_cost += qpqual_cost.startup + tid_qual_cost.per_tuple;
+ cpu_per_tuple = cpu_tuple_cost + qpqual_cost.per_tuple -
+ tid_qual_cost.per_tuple;
+ run_cost += cpu_per_tuple * ntuples;
+
+ /* tlist eval costs are paid per output row, not per tuple scanned */
+ startup_cost += path->pathtarget->cost.startup;
+ run_cost += path->pathtarget->cost.per_tuple * path->rows;
+
+ path->startup_cost = startup_cost;
+ path->total_cost = startup_cost + run_cost;
+}
+
/*
* cost_subqueryscan
* Determines and returns the cost of scanning a subquery RTE.
diff --git src/backend/optimizer/path/tidpath.c src/backend/optimizer/path/tidpath.c
index 1463a82be8..aa4d6aefad 100644
--- src/backend/optimizer/path/tidpath.c
+++ src/backend/optimizer/path/tidpath.c
@@ -2,9 +2,9 @@
*
* tidpath.c
* Routines to determine which TID conditions are usable for scanning
- * a given relation, and create TidPaths accordingly.
+ * a given relation, and create TidPaths and TidRangePaths accordingly.
*
- * What we are looking for here is WHERE conditions of the form
+ * For TidPaths, we look for WHERE conditions of the form
* "CTID = pseudoconstant", which can be implemented by just fetching
* the tuple directly via heap_fetch(). We can also handle OR'd conditions
* such as (CTID = const1) OR (CTID = const2), as well as ScalarArrayOpExpr
@@ -23,6 +23,9 @@
* a function, but in practice it works better to keep the special node
* representation all the way through to execution.
*
+ * Additionally, TidRangePaths may be created for conditions of the form
+ * "CTID relop pseudoconstant", where relop is one of >,>=,<,<=, and
+ * AND-clauses composed of such conditions.
*
* Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
@@ -63,14 +66,14 @@ IsCTIDVar(Var *var, RelOptInfo *rel)
/*
* Check to see if a RestrictInfo is of the form
- * CTID = pseudoconstant
+ * CTID OP pseudoconstant
* or
- * pseudoconstant = CTID
- * where the CTID Var belongs to relation "rel", and nothing on the
- * other side of the clause does.
+ * pseudoconstant OP CTID
+ * where OP is a binary operation, the CTID Var belongs to relation "rel",
+ * and nothing on the other side of the clause does.
*/
static bool
-IsTidEqualClause(RestrictInfo *rinfo, RelOptInfo *rel)
+IsBinaryTidClause(RestrictInfo *rinfo, RelOptInfo *rel)
{
OpExpr *node;
Node *arg1,
@@ -83,10 +86,9 @@ IsTidEqualClause(RestrictInfo *rinfo, RelOptInfo *rel)
return false;
node = (OpExpr *) rinfo->clause;
- /* Operator must be tideq */
- if (node->opno != TIDEqualOperator)
+ /* OpExpr must have two arguments */
+ if (list_length(node->args) != 2)
return false;
- Assert(list_length(node->args) == 2);
arg1 = linitial(node->args);
arg2 = lsecond(node->args);
@@ -116,6 +118,50 @@ IsTidEqualClause(RestrictInfo *rinfo, RelOptInfo *rel)
return true; /* success */
}
+/*
+ * Check to see if a RestrictInfo is of the form
+ * CTID = pseudoconstant
+ * or
+ * pseudoconstant = CTID
+ * where the CTID Var belongs to relation "rel", and nothing on the
+ * other side of the clause does.
+ */
+static bool
+IsTidEqualClause(RestrictInfo *rinfo, RelOptInfo *rel)
+{
+ if (!IsBinaryTidClause(rinfo, rel))
+ return false;
+
+ if (((OpExpr *) rinfo->clause)->opno == TIDEqualOperator)
+ return true;
+
+ return false;
+}
+
+/*
+ * Check to see if a RestrictInfo is of the form
+ * CTID OP pseudoconstant
+ * or
+ * pseudoconstant OP CTID
+ * where OP is a range operator such as <, <=, >, or >=, the CTID Var belongs
+ * to relation "rel", and nothing on the other side of the clause does.
+ */
+static bool
+IsTidRangeClause(RestrictInfo *rinfo, RelOptInfo *rel)
+{
+ Oid opno;
+
+ if (!IsBinaryTidClause(rinfo, rel))
+ return false;
+ opno = ((OpExpr *) rinfo->clause)->opno;
+
+ if (opno == TIDLessOperator || opno == TIDLessEqOperator ||
+ opno == TIDGreaterOperator || opno == TIDGreaterEqOperator)
+ return true;
+
+ return false;
+}
+
/*
* Check to see if a RestrictInfo is of the form
* CTID = ANY (pseudoconstant_array)
@@ -222,7 +268,7 @@ TidQualFromRestrictInfo(RestrictInfo *rinfo, RelOptInfo *rel)
*
* Returns a List of CTID qual RestrictInfos for the specified rel (with
* implicit OR semantics across the list), or NIL if there are no usable
- * conditions.
+ * equality conditions.
*
* This function is just concerned with handling AND/OR recursion.
*/
@@ -301,6 +347,33 @@ TidQualFromRestrictInfoList(List *rlist, RelOptInfo *rel)
return rlst;
}
+/*
+ * Extract a set of CTID range conditions from implicit-AND List of RestrictInfos
+ *
+ * Returns a List of CTID range qual RestrictInfos for the specified rel
+ * (with implicit AND semantics across the list), or NIL if there are no
+ * usable range conditions.
+ */
+static List *
+TidRangeQualFromRestrictInfoList(List *rlist, RelOptInfo *rel)
+{
+ List *rlst = NIL;
+ ListCell *l;
+
+ if (!rel->has_scan_setlimits)
+ return NIL;
+
+ foreach(l, rlist)
+ {
+ RestrictInfo *rinfo = lfirst_node(RestrictInfo, l);
+
+ if (IsTidRangeClause(rinfo, rel))
+ rlst = lappend(rlst, rinfo);
+ }
+
+ return rlst;
+}
+
/*
* Given a list of join clauses involving our rel, create a parameterized
* TidPath for each one that is a suitable TidEqual clause.
@@ -385,6 +458,7 @@ void
create_tidscan_paths(PlannerInfo *root, RelOptInfo *rel)
{
List *tidquals;
+ List *tidrangequals;
/*
* If any suitable quals exist in the rel's baserestrict list, generate a
@@ -404,6 +478,26 @@ create_tidscan_paths(PlannerInfo *root, RelOptInfo *rel)
required_outer));
}
+ /*
+ * If there are range quals in the baserestrict list, generate a
+ * TidRangePath.
+ */
+ tidrangequals = TidRangeQualFromRestrictInfoList(rel->baserestrictinfo,
+ rel);
+
+ if (tidrangequals)
+ {
+ /*
+ * This path uses no join clauses, but it could still have required
+ * parameterization due to LATERAL refs in its tlist.
+ */
+ Relids required_outer = rel->lateral_relids;
+
+ add_path(rel, (Path *) create_tidrangescan_path(root, rel,
+ tidrangequals,
+ required_outer));
+ }
+
/*
* Try to generate parameterized TidPaths using equality clauses extracted
* from EquivalenceClasses. (This is important since simple "t1.ctid =
diff --git src/backend/optimizer/plan/createplan.c src/backend/optimizer/plan/createplan.c
index f7a8dae3c6..bdfee9cc61 100644
--- src/backend/optimizer/plan/createplan.c
+++ src/backend/optimizer/plan/createplan.c
@@ -129,6 +129,10 @@ static Plan *create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual,
static void bitmap_subplan_mark_shared(Plan *plan);
static TidScan *create_tidscan_plan(PlannerInfo *root, TidPath *best_path,
List *tlist, List *scan_clauses);
+static TidRangeScan *create_tidrangescan_plan(PlannerInfo *root,
+ TidRangePath *best_path,
+ List *tlist,
+ List *scan_clauses);
static SubqueryScan *create_subqueryscan_plan(PlannerInfo *root,
SubqueryScanPath *best_path,
List *tlist, List *scan_clauses);
@@ -193,6 +197,8 @@ static BitmapHeapScan *make_bitmap_heapscan(List *qptlist,
Index scanrelid);
static TidScan *make_tidscan(List *qptlist, List *qpqual, Index scanrelid,
List *tidquals);
+static TidRangeScan *make_tidrangescan(List *qptlist, List *qpqual,
+ Index scanrelid, List *tidrangequals);
static SubqueryScan *make_subqueryscan(List *qptlist,
List *qpqual,
Index scanrelid,
@@ -384,6 +390,7 @@ create_plan_recurse(PlannerInfo *root, Path *best_path, int flags)
case T_IndexOnlyScan:
case T_BitmapHeapScan:
case T_TidScan:
+ case T_TidRangeScan:
case T_SubqueryScan:
case T_FunctionScan:
case T_TableFuncScan:
@@ -679,6 +686,13 @@ create_scan_plan(PlannerInfo *root, Path *best_path, int flags)
scan_clauses);
break;
+ case T_TidRangeScan:
+ plan = (Plan *) create_tidrangescan_plan(root,
+ (TidRangePath *) best_path,
+ tlist,
+ scan_clauses);
+ break;
+
case T_SubqueryScan:
plan = (Plan *) create_subqueryscan_plan(root,
(SubqueryScanPath *) best_path,
@@ -3440,6 +3454,71 @@ create_tidscan_plan(PlannerInfo *root, TidPath *best_path,
return scan_plan;
}
+/*
+ * create_tidrangescan_plan
+ * Returns a tidrangescan plan for the base relation scanned by 'best_path'
+ * with restriction clauses 'scan_clauses' and targetlist 'tlist'.
+ */
+static TidRangeScan *
+create_tidrangescan_plan(PlannerInfo *root, TidRangePath *best_path,
+ List *tlist, List *scan_clauses)
+{
+ TidRangeScan *scan_plan;
+ Index scan_relid = best_path->path.parent->relid;
+ List *tidrangequals = best_path->tidrangequals;
+
+ /* it should be a base rel... */
+ Assert(scan_relid > 0);
+ Assert(best_path->path.parent->rtekind == RTE_RELATION);
+
+ /*
+ * The qpqual list must contain all restrictions not enforced by the
+ * tidrangequals list. tidrangequals has AND semantics, so we can simply
+ * remove any qual that appears in it.
+ */
+ {
+ List *qpqual = NIL;
+ ListCell *l;
+
+ foreach(l, scan_clauses)
+ {
+ RestrictInfo *rinfo = lfirst_node(RestrictInfo, l);
+
+ if (rinfo->pseudoconstant)
+ continue; /* we may drop pseudoconstants here */
+ if (list_member_ptr(tidrangequals, rinfo))
+ continue; /* simple duplicate */
+ qpqual = lappend(qpqual, rinfo);
+ }
+ scan_clauses = qpqual;
+ }
+
+ /* Sort clauses into best execution order */
+ scan_clauses = order_qual_clauses(root, scan_clauses);
+
+ /* Reduce RestrictInfo lists to bare expressions; ignore pseudoconstants */
+ tidrangequals = extract_actual_clauses(tidrangequals, false);
+ scan_clauses = extract_actual_clauses(scan_clauses, false);
+
+ /* Replace any outer-relation variables with nestloop params */
+ if (best_path->path.param_info)
+ {
+ tidrangequals = (List *)
+ replace_nestloop_params(root, (Node *) tidrangequals);
+ scan_clauses = (List *)
+ replace_nestloop_params(root, (Node *) scan_clauses);
+ }
+
+ scan_plan = make_tidrangescan(tlist,
+ scan_clauses,
+ scan_relid,
+ tidrangequals);
+
+ copy_generic_path_info(&scan_plan->scan.plan, &best_path->path);
+
+ return scan_plan;
+}
+
/*
* create_subqueryscan_plan
* Returns a subqueryscan plan for the base relation scanned by 'best_path'
@@ -5373,6 +5452,25 @@ make_tidscan(List *qptlist,
return node;
}
+static TidRangeScan *
+make_tidrangescan(List *qptlist,
+ List *qpqual,
+ Index scanrelid,
+ List *tidrangequals)
+{
+ TidRangeScan *node = makeNode(TidRangeScan);
+ Plan *plan = &node->scan.plan;
+
+ plan->targetlist = qptlist;
+ plan->qual = qpqual;
+ plan->lefttree = NULL;
+ plan->righttree = NULL;
+ node->scan.scanrelid = scanrelid;
+ node->tidrangequals = tidrangequals;
+
+ return node;
+}
+
static SubqueryScan *
make_subqueryscan(List *qptlist,
List *qpqual,
diff --git src/backend/optimizer/plan/setrefs.c src/backend/optimizer/plan/setrefs.c
index 127ea3d856..7ce2d00b2b 100644
--- src/backend/optimizer/plan/setrefs.c
+++ src/backend/optimizer/plan/setrefs.c
@@ -619,6 +619,22 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
rtoffset, 1);
}
break;
+ case T_TidRangeScan:
+ {
+ TidRangeScan *splan = (TidRangeScan *) plan;
+
+ splan->scan.scanrelid += rtoffset;
+ splan->scan.plan.targetlist =
+ fix_scan_list(root, splan->scan.plan.targetlist,
+ rtoffset, NUM_EXEC_TLIST(plan));
+ splan->scan.plan.qual =
+ fix_scan_list(root, splan->scan.plan.qual,
+ rtoffset, NUM_EXEC_QUAL(plan));
+ splan->tidrangequals =
+ fix_scan_list(root, splan->tidrangequals,
+ rtoffset, 1); /* v9_tid XXX Not sure this is right */
+ }
+ break;
case T_SubqueryScan:
/* Needs special treatment, see comments below */
return set_subqueryscan_references(root,
diff --git src/backend/optimizer/plan/subselect.c src/backend/optimizer/plan/subselect.c
index fcce81926b..094d5b50d0 100644
--- src/backend/optimizer/plan/subselect.c
+++ src/backend/optimizer/plan/subselect.c
@@ -2367,6 +2367,12 @@ finalize_plan(PlannerInfo *root, Plan *plan,
context.paramids = bms_add_members(context.paramids, scan_params);
break;
+ case T_TidRangeScan:
+ finalize_primnode((Node *) ((TidRangeScan *) plan)->tidrangequals,
+ &context);
+ context.paramids = bms_add_members(context.paramids, scan_params);
+ break;
+
case T_SubqueryScan:
{
SubqueryScan *sscan = (SubqueryScan *) plan;
diff --git src/backend/optimizer/util/pathnode.c src/backend/optimizer/util/pathnode.c
index 51478957fb..e28d74afe9 100644
--- src/backend/optimizer/util/pathnode.c
+++ src/backend/optimizer/util/pathnode.c
@@ -1203,6 +1203,35 @@ create_tidscan_path(PlannerInfo *root, RelOptInfo *rel, List *tidquals,
return pathnode;
}
+/*
+ * create_tidscan_path
+ * Creates a path corresponding to a scan by a range of TIDs, returning
+ * the pathnode.
+ */
+TidRangePath *
+create_tidrangescan_path(PlannerInfo *root, RelOptInfo *rel,
+ List *tidrangequals, Relids required_outer)
+{
+ TidRangePath *pathnode = makeNode(TidRangePath);
+
+ pathnode->path.pathtype = T_TidRangeScan;
+ pathnode->path.parent = rel;
+ pathnode->path.pathtarget = rel->reltarget;
+ pathnode->path.param_info = get_baserel_parampathinfo(root, rel,
+ required_outer);
+ pathnode->path.parallel_aware = false;
+ pathnode->path.parallel_safe = rel->consider_parallel;
+ pathnode->path.parallel_workers = 0;
+ pathnode->path.pathkeys = NIL; /* always unordered */
+
+ pathnode->tidrangequals = tidrangequals;
+
+ cost_tidrangescan(&pathnode->path, root, rel, tidrangequals,
+ pathnode->path.param_info);
+
+ return pathnode;
+}
+
/*
* create_append_path
* Creates a path corresponding to an Append plan, returning the
diff --git src/backend/optimizer/util/plancat.c src/backend/optimizer/util/plancat.c
index daf1759623..4333f6c4c2 100644
--- src/backend/optimizer/util/plancat.c
+++ src/backend/optimizer/util/plancat.c
@@ -466,6 +466,10 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
/* Collect info about relation's foreign keys, if relevant */
get_relation_foreign_keys(root, rel, relation, inhparent);
+ /* Collect info about functions implemented by the rel's table AM. */
+ rel->has_scan_setlimits = relation->rd_tableam &&
+ relation->rd_tableam->scan_setlimits != NULL;
+
/*
* Collect info about relation's partitioning scheme, if any. Only
* inheritance parents may be partitioned.
diff --git src/backend/optimizer/util/relnode.c src/backend/optimizer/util/relnode.c
index 9c9a738c80..9536c238fb 100644
--- src/backend/optimizer/util/relnode.c
+++ src/backend/optimizer/util/relnode.c
@@ -247,6 +247,7 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
rel->baserestrict_min_security = UINT_MAX;
rel->joininfo = NIL;
rel->has_eclass_joins = false;
+ rel->has_scan_setlimits = false;
rel->consider_partitionwise_join = false; /* might get changed later */
rel->part_scheme = NULL;
rel->nparts = -1;
@@ -659,6 +660,7 @@ build_join_rel(PlannerInfo *root,
joinrel->baserestrict_min_security = UINT_MAX;
joinrel->joininfo = NIL;
joinrel->has_eclass_joins = false;
+ joinrel->has_scan_setlimits = false;
joinrel->consider_partitionwise_join = false; /* might get changed later */
joinrel->top_parent_relids = NULL;
joinrel->part_scheme = NULL;
@@ -836,6 +838,7 @@ build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
joinrel->baserestrictcost.per_tuple = 0;
joinrel->joininfo = NIL;
joinrel->has_eclass_joins = false;
+ joinrel->has_scan_setlimits = false;
joinrel->consider_partitionwise_join = false; /* might get changed later */
joinrel->top_parent_relids = NULL;
joinrel->part_scheme = NULL;
diff --git src/test/regress/expected/tidrangescan.out src/test/regress/expected/tidrangescan.out
new file mode 100644
index 0000000000..fc11894c8e
--- /dev/null
+++ src/test/regress/expected/tidrangescan.out
@@ -0,0 +1,245 @@
+-- tests for tidrangescans
+SET enable_seqscan TO off;
+CREATE TABLE tidrangescan(id integer, data text);
+-- insert enough tuples to fill at least two pages
+INSERT INTO tidrangescan SELECT i,repeat('x', 100) FROM generate_series(1,200) AS s(i);
+-- remove all tuples after the 10th tuple on each page. Trying to ensure
+-- we get the same layout with all CPU architectures and smaller than standard
+-- page sizes.
+DELETE FROM tidrangescan
+WHERE substring(ctid::text from ',(\d+)\)')::integer > 10 OR substring(ctid::text from '\((\d+),')::integer > 2;
+VACUUM tidrangescan;
+-- range scans with upper bound
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid < '(1,0)';
+ QUERY PLAN
+-----------------------------------
+ Tid Range Scan on tidrangescan
+ TID Cond: (ctid < '(1,0)'::tid)
+(2 rows)
+
+SELECT ctid FROM tidrangescan WHERE ctid < '(1,0)';
+ ctid
+--------
+ (0,1)
+ (0,2)
+ (0,3)
+ (0,4)
+ (0,5)
+ (0,6)
+ (0,7)
+ (0,8)
+ (0,9)
+ (0,10)
+(10 rows)
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid <= '(1,5)';
+ QUERY PLAN
+------------------------------------
+ Tid Range Scan on tidrangescan
+ TID Cond: (ctid <= '(1,5)'::tid)
+(2 rows)
+
+SELECT ctid FROM tidrangescan WHERE ctid <= '(1,5)';
+ ctid
+--------
+ (0,1)
+ (0,2)
+ (0,3)
+ (0,4)
+ (0,5)
+ (0,6)
+ (0,7)
+ (0,8)
+ (0,9)
+ (0,10)
+ (1,1)
+ (1,2)
+ (1,3)
+ (1,4)
+ (1,5)
+(15 rows)
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid < '(0,0)';
+ QUERY PLAN
+-----------------------------------
+ Tid Range Scan on tidrangescan
+ TID Cond: (ctid < '(0,0)'::tid)
+(2 rows)
+
+SELECT ctid FROM tidrangescan WHERE ctid < '(0,0)';
+ ctid
+------
+(0 rows)
+
+-- range scans with lower bound
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid > '(2,8)';
+ QUERY PLAN
+-----------------------------------
+ Tid Range Scan on tidrangescan
+ TID Cond: (ctid > '(2,8)'::tid)
+(2 rows)
+
+SELECT ctid FROM tidrangescan WHERE ctid > '(2,8)';
+ ctid
+--------
+ (2,9)
+ (2,10)
+(2 rows)
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE '(2,8)' < ctid;
+ QUERY PLAN
+-----------------------------------
+ Tid Range Scan on tidrangescan
+ TID Cond: ('(2,8)'::tid < ctid)
+(2 rows)
+
+SELECT ctid FROM tidrangescan WHERE '(2,8)' < ctid;
+ ctid
+--------
+ (2,9)
+ (2,10)
+(2 rows)
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid >= '(2,8)';
+ QUERY PLAN
+------------------------------------
+ Tid Range Scan on tidrangescan
+ TID Cond: (ctid >= '(2,8)'::tid)
+(2 rows)
+
+SELECT ctid FROM tidrangescan WHERE ctid >= '(2,8)';
+ ctid
+--------
+ (2,8)
+ (2,9)
+ (2,10)
+(3 rows)
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid >= '(100,0)';
+ QUERY PLAN
+--------------------------------------
+ Tid Range Scan on tidrangescan
+ TID Cond: (ctid >= '(100,0)'::tid)
+(2 rows)
+
+SELECT ctid FROM tidrangescan WHERE ctid >= '(100,0)';
+ ctid
+------
+(0 rows)
+
+-- range scans with both bounds
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid > '(1,4)' AND '(1,7)' >= ctid;
+ QUERY PLAN
+----------------------------------------------------------------
+ Tid Range Scan on tidrangescan
+ TID Cond: ((ctid > '(1,4)'::tid) AND ('(1,7)'::tid >= ctid))
+(2 rows)
+
+SELECT ctid FROM tidrangescan WHERE ctid > '(1,4)' AND '(1,7)' >= ctid;
+ ctid
+-------
+ (1,5)
+ (1,6)
+ (1,7)
+(3 rows)
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE '(1,7)' >= ctid AND ctid > '(1,4)';
+ QUERY PLAN
+----------------------------------------------------------------
+ Tid Range Scan on tidrangescan
+ TID Cond: (('(1,7)'::tid >= ctid) AND (ctid > '(1,4)'::tid))
+(2 rows)
+
+SELECT ctid FROM tidrangescan WHERE '(1,7)' >= ctid AND ctid > '(1,4)';
+ ctid
+-------
+ (1,5)
+ (1,6)
+ (1,7)
+(3 rows)
+
+-- extreme offsets
+SELECT ctid FROM tidrangescan where ctid > '(0,65535)' AND ctid < '(1,0)' LIMIT 1;
+ ctid
+------
+(0 rows)
+
+SELECT ctid FROM tidrangescan where ctid < '(0,0)' LIMIT 1;
+ ctid
+------
+(0 rows)
+
+-- empty table
+CREATE TABLE tidrangescan_empty(id integer, data text);
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan_empty WHERE ctid < '(1, 0)';
+ QUERY PLAN
+--------------------------------------
+ Tid Range Scan on tidrangescan_empty
+ TID Cond: (ctid < '(1,0)'::tid)
+(2 rows)
+
+SELECT ctid FROM tidrangescan_empty WHERE ctid < '(1, 0)';
+ ctid
+------
+(0 rows)
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan_empty WHERE ctid > '(9, 0)';
+ QUERY PLAN
+--------------------------------------
+ Tid Range Scan on tidrangescan_empty
+ TID Cond: (ctid > '(9,0)'::tid)
+(2 rows)
+
+SELECT ctid FROM tidrangescan_empty WHERE ctid > '(9, 0)';
+ ctid
+------
+(0 rows)
+
+-- cursors
+BEGIN;
+DECLARE c SCROLL CURSOR FOR SELECT ctid FROM tidrangescan WHERE ctid < '(1,0)';
+FETCH NEXT c;
+ ctid
+-------
+ (0,1)
+(1 row)
+
+FETCH NEXT c;
+ ctid
+-------
+ (0,2)
+(1 row)
+
+FETCH PRIOR c;
+ ctid
+-------
+ (0,1)
+(1 row)
+
+FETCH FIRST c;
+ ctid
+-------
+ (0,1)
+(1 row)
+
+FETCH LAST c;
+ ctid
+--------
+ (0,10)
+(1 row)
+
+COMMIT;
+DROP TABLE tidrangescan;
+DROP TABLE tidrangescan_empty;
+RESET enable_seqscan;
diff --git src/test/regress/parallel_schedule src/test/regress/parallel_schedule
index e0e1ef71dd..2b9763a869 100644
--- src/test/regress/parallel_schedule
+++ src/test/regress/parallel_schedule
@@ -80,7 +80,7 @@ test: brin gin gist spgist privileges init_privs security_label collate matview
# ----------
# Another group of parallel tests
# ----------
-test: create_table_like alter_generic alter_operator misc async dbsize misc_functions sysviews tsrf tid tidscan collate.icu.utf8 incremental_sort
+test: create_table_like alter_generic alter_operator misc async dbsize misc_functions sysviews tsrf tid tidscan tidrangescan collate.icu.utf8 incremental_sort
# rules cannot run concurrently with any test that creates
# a view or rule in the public schema
diff --git src/test/regress/sql/tidrangescan.sql src/test/regress/sql/tidrangescan.sql
new file mode 100644
index 0000000000..d60439d56c
--- /dev/null
+++ src/test/regress/sql/tidrangescan.sql
@@ -0,0 +1,83 @@
+-- tests for tidrangescans
+
+SET enable_seqscan TO off;
+CREATE TABLE tidrangescan(id integer, data text);
+
+-- insert enough tuples to fill at least two pages
+INSERT INTO tidrangescan SELECT i,repeat('x', 100) FROM generate_series(1,200) AS s(i);
+
+-- remove all tuples after the 10th tuple on each page. Trying to ensure
+-- we get the same layout with all CPU architectures and smaller than standard
+-- page sizes.
+DELETE FROM tidrangescan
+WHERE substring(ctid::text from ',(\d+)\)')::integer > 10 OR substring(ctid::text from '\((\d+),')::integer > 2;
+VACUUM tidrangescan;
+
+-- range scans with upper bound
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid < '(1,0)';
+SELECT ctid FROM tidrangescan WHERE ctid < '(1,0)';
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid <= '(1,5)';
+SELECT ctid FROM tidrangescan WHERE ctid <= '(1,5)';
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid < '(0,0)';
+SELECT ctid FROM tidrangescan WHERE ctid < '(0,0)';
+
+-- range scans with lower bound
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid > '(2,8)';
+SELECT ctid FROM tidrangescan WHERE ctid > '(2,8)';
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE '(2,8)' < ctid;
+SELECT ctid FROM tidrangescan WHERE '(2,8)' < ctid;
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid >= '(2,8)';
+SELECT ctid FROM tidrangescan WHERE ctid >= '(2,8)';
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid >= '(100,0)';
+SELECT ctid FROM tidrangescan WHERE ctid >= '(100,0)';
+
+-- range scans with both bounds
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE ctid > '(1,4)' AND '(1,7)' >= ctid;
+SELECT ctid FROM tidrangescan WHERE ctid > '(1,4)' AND '(1,7)' >= ctid;
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan WHERE '(1,7)' >= ctid AND ctid > '(1,4)';
+SELECT ctid FROM tidrangescan WHERE '(1,7)' >= ctid AND ctid > '(1,4)';
+
+-- extreme offsets
+SELECT ctid FROM tidrangescan where ctid > '(0,65535)' AND ctid < '(1,0)' LIMIT 1;
+SELECT ctid FROM tidrangescan where ctid < '(0,0)' LIMIT 1;
+
+-- empty table
+CREATE TABLE tidrangescan_empty(id integer, data text);
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan_empty WHERE ctid < '(1, 0)';
+SELECT ctid FROM tidrangescan_empty WHERE ctid < '(1, 0)';
+
+EXPLAIN (COSTS OFF)
+SELECT ctid FROM tidrangescan_empty WHERE ctid > '(9, 0)';
+SELECT ctid FROM tidrangescan_empty WHERE ctid > '(9, 0)';
+
+-- cursors
+BEGIN;
+DECLARE c SCROLL CURSOR FOR SELECT ctid FROM tidrangescan WHERE ctid < '(1,0)';
+FETCH NEXT c;
+FETCH NEXT c;
+FETCH PRIOR c;
+FETCH FIRST c;
+FETCH LAST c;
+COMMIT;
+
+DROP TABLE tidrangescan;
+DROP TABLE tidrangescan_empty;
+
+RESET enable_seqscan;
diff --git src/tools/pgindent/typedefs.list src/tools/pgindent/typedefs.list
index 9cd047ba25..f12d60debf 100644
--- src/tools/pgindent/typedefs.list
+++ src/tools/pgindent/typedefs.list
@@ -2526,8 +2526,13 @@ TextPositionState
TheLexeme
TheSubstitute
TidExpr
+TidExprType
TidHashKey
+TidOpExpr
TidPath
+TidRangePath
+TidRangeScan
+TidRangeScanState
TidScan
TidScanState
TimeADT
^ permalink raw reply [nested|flat] 28+ messages in thread
* Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-04-22 01:51 Chao Li <[email protected]>
0 siblings, 2 replies; 28+ messages in thread
From: Chao Li @ 2026-04-22 01:51 UTC (permalink / raw)
To: pgsql-hackers
Hi,
I tested the v19 new feature CREATE SUBSCRIPTION ... SERVER yesterday, and found an issue: once the old server becomes broken, the subscription cannot be recovered by switching it to a good server.
Here is a repro:
```
evantest=# CREATE EXTENSION postgres_fdw;
CREATE EXTENSION
# Create two servers
evantest=# CREATE SERVER old_srv FOREIGN DATA WRAPPER postgres_fdw
evantest-# OPTIONS (host '127.0.0.1', dbname 'postgres', port '5432');
CREATE SERVER
evantest=# CREATE SERVER new_srv FOREIGN DATA WRAPPER postgres_fdw
evantest-# OPTIONS (host '127.0.0.1', dbname 'postgres', port '5432');
CREATE SERVER
evantest=# CREATE USER MAPPING FOR CURRENT_USER SERVER old_srv
evantest-# OPTIONS (user 'dummy', password 'dummy');
CREATE USER MAPPING
evantest=# CREATE USER MAPPING FOR CURRENT_USER SERVER new_srv
evantest-# OPTIONS (user 'dummy', password 'dummy');
CREATE USER MAPPING
# Add old_server to a subscription
evantest=# CREATE SUBSCRIPTION sub_bug SERVER old_srv
evantest-# PUBLICATION pub_does_not_matter
evantest-# WITH (connect = false, slot_name = NONE);
WARNING: subscription was created, but is not connected
HINT: To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
CREATE SUBSCRIPTION
# Break old_srv
evantest=# DROP USER MAPPING FOR CURRENT_USER SERVER old_srv;
DROP USER MAPPING
# Fail to switch to a good server because old_srv is broken
evantest=# ALTER SUBSCRIPTION sub_bug SERVER new_srv;
ERROR: user mapping not found for user "chaol", server "old_srv"
evantest=# ALTER SUBSCRIPTION sub_bug CONNECTION 'host=127.0.0.1 dbname=postgres port=5432 user=dummy password=dummy';
ERROR: user mapping not found for user "chaol", server "old_srv"
```
In AlterSubscription(), this comment made me think this is a bug:
```
/*
* Skip ACL checks on the subscription's foreign server, if any. If
* changing the server (or replacing it with a raw connection), then the
* old one will be removed anyway. If changing something unrelated,
* there's no need to do an additional ACL check here; that will be done
* by the subscription worker anyway.
*/
sub = GetSubscription(subid, false, false);
```
The comment explicitly says to skip ACL checks on the old server because it will be removed anyway.
But after looking into GetSubscription(), I found that when the aclcheck parameter is false, it still calls ForeignServerConnectionString(). I think that is the root cause of the bug.
To fix this, I worked out a solution that stores the server OID in Subscription, and only calls ForeignServerConnectionString()lazily when sub->conninfo is actually needed. I also added a new test case to cover this scenario. Without the fix, the new test fails.
See attached patch for details.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] v1-0001-Allow-altering-subscription-server-connection-wit.patch (12.5K, ../../[email protected]/2-v1-0001-Allow-altering-subscription-server-connection-wit.patch)
download | inline diff:
From fd461cf3f5f1c96e5789dd2d3ac113619e776477 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Wed, 22 Apr 2026 09:14:27 +0800
Subject: [PATCH v1] Allow altering subscription server/connection without
resolving old conninfo
When a subscription uses a foreign server, GetSubscription() resolves the
publisher connection string immediately. That can fail if the subscription
owner no longer has USAGE on the server or no longer has a valid user
mapping.
This becomes a problem for ALTER SUBSCRIPTION commands such as ... SERVER
or ... CONNECTION, because they may need to update the subscription away
from the broken server/conninfo, but currently fail while trying to resolve
the old one first.
Fix this by storing the subscription's server OID in the in-memory
Subscription object and loading conninfo lazily only when it is actually
needed. Add GetSubscriptionConninfo() to fetch the connection string on
demand in the subscription memory context.
This allows ALTER SUBSCRIPTION ... SERVER and ... CONNECTION to succeed
without requiring access to the previous server or user mapping, while
preserving the existing behavior for operations that really need to connect
to the publisher.
Add regression test coverage for both cases.
Author: Chao Li <[email protected]>
Reviewed-by:
Discussion: https://postgr.es/m/
---
src/backend/catalog/pg_subscription.c | 46 +++++++++++++++++-----
src/backend/commands/subscriptioncmds.c | 9 +++--
src/include/catalog/pg_subscription.h | 2 +
src/test/regress/expected/subscription.out | 33 ++++++++++++++++
src/test/regress/regress.c | 2 +
src/test/regress/sql/subscription.sql | 27 +++++++++++++
6 files changed, 106 insertions(+), 13 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 1f1fdc75af6..3f1e6e26066 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -118,32 +118,33 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
sub->retaindeadtuples = subform->subretaindeadtuples;
sub->maxretention = subform->submaxretention;
sub->retentionactive = subform->subretentionactive;
+ sub->serverid = subform->subserver;
+ sub->conninfo = NULL;
/* Get conninfo */
- if (OidIsValid(subform->subserver))
+ if (OidIsValid(sub->serverid))
{
AclResult aclresult;
ForeignServer *server;
- server = GetForeignServer(subform->subserver);
+ server = GetForeignServer(sub->serverid);
/* recheck ACL if requested */
if (aclcheck)
{
aclresult = object_aclcheck(ForeignServerRelationId,
- subform->subserver,
+ sub->serverid,
subform->subowner, ACL_USAGE);
if (aclresult != ACLCHECK_OK)
ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"",
- GetUserNameFromId(subform->subowner, false),
- server->servername)));
+ errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"",
+ GetUserNameFromId(subform->subowner, false),
+ server->servername));
+ sub->conninfo = ForeignServerConnectionString(subform->subowner,
+ server);
}
-
- sub->conninfo = ForeignServerConnectionString(subform->subowner,
- server);
}
else
{
@@ -197,6 +198,31 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
return sub;
}
+/*
+ * Return the subscription's connection string, loading it into the
+ * subscription memory context if necessary.
+ *
+ * GetSubscription must be called earlier to set sub->serverid, because ACL
+ * checks are performed there.
+ */
+char *
+GetSubscriptionConnInfo(Subscription *sub)
+{
+ MemoryContext oldcxt;
+
+ if (sub->conninfo)
+ return sub->conninfo;
+
+ Assert(OidIsValid(sub->serverid));
+
+ oldcxt = MemoryContextSwitchTo(sub->cxt);
+ sub->conninfo = ForeignServerConnectionString(sub->owner,
+ GetForeignServer(sub->serverid));
+ MemoryContextSwitchTo(oldcxt);
+
+ return sub->conninfo;
+}
+
/*
* Return number of subscriptions defined in given database.
* Used by dropdb() to check if database can indeed be dropped.
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index d512e87cfe3..478d19d7a26 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1026,7 +1026,8 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data,
/* Try to connect to the publisher. */
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(sub->conninfo, true, true, must_use_password,
+ wrconn = walrcv_connect(GetSubscriptionConnInfo(sub), true, true,
+ must_use_password,
sub->name, &err);
if (!wrconn)
ereport(ERROR,
@@ -1278,7 +1279,8 @@ AlterSubscription_refresh_seq(Subscription *sub)
/* Try to connect to the publisher. */
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(sub->conninfo, true, true, must_use_password,
+ wrconn = walrcv_connect(GetSubscriptionConnInfo(sub), true, true,
+ must_use_password,
sub->name, &err);
if (!wrconn)
ereport(ERROR,
@@ -2129,7 +2131,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
* available.
*/
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(stmt->conninfo ? stmt->conninfo : sub->conninfo,
+ wrconn = walrcv_connect(stmt->conninfo ? stmt->conninfo :
+ GetSubscriptionConnInfo(sub),
true, true, must_use_password, sub->name,
&err);
if (!wrconn)
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index a6a2ad1e49c..4dfc6dfce17 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -164,6 +164,7 @@ typedef struct Subscription
* and the retention duration has not
* exceeded max_retention_duration, when
* defined */
+ Oid serverid; /* Oid of the foreign server, if any */
char *conninfo; /* Connection string to the publisher */
char *slotname; /* Name of the replication slot */
char *synccommit; /* Synchronous commit setting for worker */
@@ -214,6 +215,7 @@ typedef struct Subscription
extern Subscription *GetSubscription(Oid subid, bool missing_ok,
bool aclcheck);
+extern char *GetSubscriptionConnInfo(Subscription *sub);
extern void DisableSubscription(Oid subid);
extern int CountDBSubscriptions(Oid dbid);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 7e3cabdb93f..c0db7636713 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -174,16 +174,49 @@ WARNING: subscription was created, but is not connected
HINT: To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
RESET SESSION AUTHORIZATION;
+CREATE SERVER test_server2 FOREIGN DATA WRAPPER test_fdw;
+GRANT USAGE ON FOREIGN SERVER test_server2 TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server2 OPTIONS(user 'bar', password 'secret');
+RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
-- fail, must connect but lacks USAGE on server, as well as user mapping
DROP SUBSCRIPTION regress_testsub6;
ERROR: could not connect to publisher when attempting to drop replication slot "dummy": subscription owner "regress_subscription_user3" does not have permission on foreign server "test_server"
HINT: Use ALTER SUBSCRIPTION ... DISABLE to disable the subscription, and then use ALTER SUBSCRIPTION ... SET (slot_name = NONE) to disassociate it from the slot.
+-- ok, should not need to resolve conninfo for the old broken server
+ALTER SUBSCRIPTION regress_testsub6 SERVER test_server2;
+SELECT fs.srvname
+ FROM pg_subscription s
+ JOIN pg_foreign_server fs ON fs.oid = s.subserver
+ WHERE s.subname = 'regress_testsub6';
+ srvname
+--------------
+ test_server2
+(1 row)
+
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server2;
+RESET SESSION AUTHORIZATION;
+REVOKE USAGE ON FOREIGN SERVER test_server2 FROM regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+-- ok, should not need to resolve conninfo for the current broken server
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist2 password=secret';
+RESET SESSION AUTHORIZATION;
+SELECT subserver = 0::oid AS uses_conninfo, subconninfo
+ FROM pg_subscription
+ WHERE subname = 'regress_testsub6';
+ uses_conninfo | subconninfo
+---------------+----------------------------------------------
+ t | dbname=regress_doesnotexist2 password=secret
+(1 row)
+
+SET SESSION AUTHORIZATION regress_subscription_user3;
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
+DROP SERVER test_server2;
DROP SERVER test_server;
-- fail, FDW is dependent
DROP FUNCTION test_fdw_connection(oid, oid, internal);
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 2bcb5559a45..a758b1e2708 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -31,6 +31,7 @@
#include "executor/executor.h"
#include "executor/functions.h"
#include "executor/spi.h"
+#include "foreign/foreign.h"
#include "funcapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
@@ -735,6 +736,7 @@ PG_FUNCTION_INFO_V1(test_fdw_connection);
Datum
test_fdw_connection(PG_FUNCTION_ARGS)
{
+ GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist user=doesnotexist password=secret"));
}
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 6c3d9632e8a..73cb251e192 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -124,6 +124,12 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
+RESET SESSION AUTHORIZATION;
+CREATE SERVER test_server2 FOREIGN DATA WRAPPER test_fdw;
+GRANT USAGE ON FOREIGN SERVER test_server2 TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server2 OPTIONS(user 'bar', password 'secret');
+
RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
@@ -131,12 +137,33 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
-- fail, must connect but lacks USAGE on server, as well as user mapping
DROP SUBSCRIPTION regress_testsub6;
+-- ok, should not need to resolve conninfo for the old broken server
+ALTER SUBSCRIPTION regress_testsub6 SERVER test_server2;
+SELECT fs.srvname
+ FROM pg_subscription s
+ JOIN pg_foreign_server fs ON fs.oid = s.subserver
+ WHERE s.subname = 'regress_testsub6';
+
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server2;
+RESET SESSION AUTHORIZATION;
+REVOKE USAGE ON FOREIGN SERVER test_server2 FROM regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
+-- ok, should not need to resolve conninfo for the current broken server
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist2 password=secret';
+RESET SESSION AUTHORIZATION;
+SELECT subserver = 0::oid AS uses_conninfo, subconninfo
+ FROM pg_subscription
+ WHERE subname = 'regress_testsub6';
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
+DROP SERVER test_server2;
DROP SERVER test_server;
-- fail, FDW is dependent
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 28+ messages in thread
* RE: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-04-22 12:35 Hayato Kuroda (Fujitsu) <[email protected]>
parent: Chao Li <[email protected]>
1 sibling, 2 replies; 28+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2026-04-22 12:35 UTC (permalink / raw)
To: 'Chao Li' <[email protected]>; +Cc: pgsql-hackers
Dear Chao,
> I tested the v19 new feature CREATE SUBSCRIPTION ... SERVER yesterday, and
> found an issue: once the old server becomes broken, the subscription cannot be
> recovered by switching it to a good server.
Thanks for testing. I could reproduce the same issue. In addition to yours, I found
DROP SUBSCRIPTION cannot be done anymore. To switch the connection or drop it,
I had to create the same user mapping must be created again.
```
postgres=# DROP SUBSCRIPTION sub_bug ;
ERROR: user mapping not found for user "postgres", server "old_srv"
postgres=# CREATE USER MAPPING FOR CURRENT_USER SERVER old_srv
OPTIONS (user 'dummy', password 'dummy');
CREATE USER MAPPING
postgres=# DROP SUBSCRIPTION sub_bug ;
DROP SUBSCRIPTION
```
Before deep dive to your fix, I'm unclear why dropping the active USER MAPPING is
allowed. Personally, it should be avoided anyway. Do you know why it's not restricted?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-04-23 03:37 Chao Li <[email protected]>
parent: Hayato Kuroda (Fujitsu) <[email protected]>
1 sibling, 0 replies; 28+ messages in thread
From: Chao Li @ 2026-04-23 03:37 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: pgsql-hackers
> On Apr 22, 2026, at 20:35, Hayato Kuroda (Fujitsu) <[email protected]> wrote:
>
> Dear Chao,
>
>> I tested the v19 new feature CREATE SUBSCRIPTION ... SERVER yesterday, and
>> found an issue: once the old server becomes broken, the subscription cannot be
>> recovered by switching it to a good server.
>
> Thanks for testing. I could reproduce the same issue. In addition to yours, I found
> DROP SUBSCRIPTION cannot be done anymore. To switch the connection or drop it,
> I had to create the same user mapping must be created again.
>
> ```
> postgres=# DROP SUBSCRIPTION sub_bug ;
> ERROR: user mapping not found for user "postgres", server "old_srv"
> postgres=# CREATE USER MAPPING FOR CURRENT_USER SERVER old_srv
> OPTIONS (user 'dummy', password 'dummy');
> CREATE USER MAPPING
> postgres=# DROP SUBSCRIPTION sub_bug ;
> DROP SUBSCRIPTION
> ```
>
> Before deep dive to your fix, I'm unclear why dropping the active USER MAPPING is
> allowed. Personally, it should be avoided anyway. Do you know why it's not restricted?
>
> Best regards,
> Hayato Kuroda
> FUJITSU LIMITED
>
Hi Hayato-san,
There is an existing test case in subscription.sql:
```
-- fail, must connect but lacks USAGE on server, as well as user mapping
DROP SUBSCRIPTION regress_testsub6;
```
So, I guess that’s an intentional behavior. You have to fix the broken server or switch to a good one before dropping the subscription. That’s my understanding from the test cases.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-04-29 04:44 Ajin Cherian <[email protected]>
parent: Chao Li <[email protected]>
1 sibling, 1 reply; 28+ messages in thread
From: Ajin Cherian @ 2026-04-29 04:44 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: pgsql-hackers
On Wed, Apr 22, 2026 at 11:52 AM Chao Li <[email protected]> wrote:
>
> Hi,
>
> The comment explicitly says to skip ACL checks on the old server because it will be removed anyway.
>
> But after looking into GetSubscription(), I found that when the aclcheck parameter is false, it still calls ForeignServerConnectionString(). I think that is the root cause of the bug.
>
> To fix this, I worked out a solution that stores the server OID in Subscription, and only calls ForeignServerConnectionString()lazily when sub->conninfo is actually needed. I also added a new test case to cover this scenario. Without the fix, the new test fails.
> See attached patch for details.
>
Hi Li,
Thanks for the patch.
Some comments:
1.
if (aclresult != ACLCHECK_OK)
ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("subscription owner \"%s\" does not
have permission on foreign server \"%s\"",
- GetUserNameFromId(subform->subowner, false),
- server->servername)));
+ errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("subscription owner \"%s\" does not
have permission on foreign server \"%s\"",
+ GetUserNameFromId(subform->subowner, false),
+ server->servername));
+ sub->conninfo = ForeignServerConnectionString(subform->subowner,
+ server);
}
Add a new line before the call to ForeignServerConnectionString(),
also I think you should put the if condition within curly brackets
because it spans more than one line and might confuse developers while
adding new code.
2. I think you should add a comment in the function header above
GetSubscription() stating that if aclcheck is false, then the conninfo
will be set to null and users need to call GetSubscriptionConnInfo to
get the conninfo.
3.
Datum
test_fdw_connection(PG_FUNCTION_ARGS)
{
+ GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist
user=doesnotexist password=secret"));
}
Add a comment above this change describing why it's required.
regards,
Ajin Cherian
Fujitsu Australia
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-04-30 04:11 Chao Li <[email protected]>
parent: Ajin Cherian <[email protected]>
0 siblings, 2 replies; 28+ messages in thread
From: Chao Li @ 2026-04-30 04:11 UTC (permalink / raw)
To: Ajin Cherian <[email protected]>; +Cc: pgsql-hackers
> On Apr 29, 2026, at 12:44, Ajin Cherian <[email protected]> wrote:
>
> On Wed, Apr 22, 2026 at 11:52 AM Chao Li <[email protected]> wrote:
>>
>> Hi,
>>
>> The comment explicitly says to skip ACL checks on the old server because it will be removed anyway.
>>
>> But after looking into GetSubscription(), I found that when the aclcheck parameter is false, it still calls ForeignServerConnectionString(). I think that is the root cause of the bug.
>>
>> To fix this, I worked out a solution that stores the server OID in Subscription, and only calls ForeignServerConnectionString()lazily when sub->conninfo is actually needed. I also added a new test case to cover this scenario. Without the fix, the new test fails.
>> See attached patch for details.
>>
> Hi Li,
>
> Thanks for the patch.
Thanks for your review.
> Some comments:
>
> 1.
> if (aclresult != ACLCHECK_OK)
> ereport(ERROR,
> - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> - errmsg("subscription owner \"%s\" does not
> have permission on foreign server \"%s\"",
> - GetUserNameFromId(subform->subowner, false),
> - server->servername)));
> + errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + errmsg("subscription owner \"%s\" does not
> have permission on foreign server \"%s\"",
> + GetUserNameFromId(subform->subowner, false),
> + server->servername));
> + sub->conninfo = ForeignServerConnectionString(subform->subowner,
> + server);
> }
>
> Add a new line before the call to ForeignServerConnectionString(),
Okay.
> also I think you should put the if condition within curly brackets
> because it spans more than one line and might confuse developers while
> adding new code.
I don’t think curly brackets are needed as there is only one statement under the if though the statement spans multiple lines. There are plenty of examples in the existing code, for example:
```
if (!wrconn)
ereport(ERROR,
errcode(ERRCODE_CONNECTION_FAILURE),
errmsg("subscription \"%s\" could not connect to the publisher: %s",
sub->name, err));
```
>
> 2. I think you should add a comment in the function header above
> GetSubscription() stating that if aclcheck is false, then the conninfo
> will be set to null and users need to call GetSubscriptionConnInfo to
> get the conninfo.
Updated the header comment.
>
> 3.
> Datum
> test_fdw_connection(PG_FUNCTION_ARGS)
> {
> + GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
> PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist
> user=doesnotexist password=secret"));
> }
>
> Add a comment above this change describing why it's required.
>
Added the comment.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] v2-0001-Allow-altering-subscription-server-connection-wit.patch (13.2K, ../../[email protected]/2-v2-0001-Allow-altering-subscription-server-connection-wit.patch)
download | inline diff:
From cc4b7cb8bdb092f59febc2c67487366e3ef5add5 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Wed, 22 Apr 2026 09:14:27 +0800
Subject: [PATCH v2] Allow altering subscription server/connection without
resolving old conninfo
When a subscription uses a foreign server, GetSubscription() resolves the
publisher connection string immediately. That can fail if the subscription
owner no longer has USAGE on the server or no longer has a valid user
mapping.
This becomes a problem for ALTER SUBSCRIPTION commands such as ... SERVER
or ... CONNECTION, because they may need to update the subscription away
from the broken server/conninfo, but currently fail while trying to resolve
the old one first.
Fix this by storing the subscription's server OID in the in-memory
Subscription object and loading conninfo lazily only when it is actually
needed. Add GetSubscriptionConninfo() to fetch the connection string on
demand in the subscription memory context.
This allows ALTER SUBSCRIPTION ... SERVER and ... CONNECTION to succeed
without requiring access to the previous server or user mapping, while
preserving the existing behavior for operations that really need to connect
to the publisher.
Add regression test coverage for both cases.
Author: Chao Li <[email protected]>
Reviewed-by: Ajin Cherian <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/catalog/pg_subscription.c | 55 ++++++++++++++++++----
src/backend/commands/subscriptioncmds.c | 9 ++--
src/include/catalog/pg_subscription.h | 2 +
src/test/regress/expected/subscription.out | 33 +++++++++++++
src/test/regress/regress.c | 3 ++
src/test/regress/sql/subscription.sql | 27 +++++++++++
6 files changed, 116 insertions(+), 13 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 1f1fdc75af6..5e4a6a07810 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -72,6 +72,14 @@ GetPublicationsStr(List *publications, StringInfo dest, bool quote_literal)
/*
* Fetch the subscription from the syscache.
+ *
+ * If missing_ok is false, throw an error if the subscription is not found.
+ * If true, return NULL in that case.
+ *
+ * If aclcheck is true, check whether the subscription owner has permission on
+ * the subscription's foreign server, and load the connection string from the
+ * foreign server. Later, GetSubscriptionConnInfo() should be called to get
+ * the connection string.
*/
Subscription *
GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
@@ -118,32 +126,34 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
sub->retaindeadtuples = subform->subretaindeadtuples;
sub->maxretention = subform->submaxretention;
sub->retentionactive = subform->subretentionactive;
+ sub->serverid = subform->subserver;
+ sub->conninfo = NULL;
/* Get conninfo */
- if (OidIsValid(subform->subserver))
+ if (OidIsValid(sub->serverid))
{
AclResult aclresult;
ForeignServer *server;
- server = GetForeignServer(subform->subserver);
+ server = GetForeignServer(sub->serverid);
/* recheck ACL if requested */
if (aclcheck)
{
aclresult = object_aclcheck(ForeignServerRelationId,
- subform->subserver,
+ sub->serverid,
subform->subowner, ACL_USAGE);
if (aclresult != ACLCHECK_OK)
ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"",
- GetUserNameFromId(subform->subowner, false),
- server->servername)));
- }
+ errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"",
+ GetUserNameFromId(subform->subowner, false),
+ server->servername));
- sub->conninfo = ForeignServerConnectionString(subform->subowner,
- server);
+ sub->conninfo = ForeignServerConnectionString(subform->subowner,
+ server);
+ }
}
else
{
@@ -197,6 +207,31 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
return sub;
}
+/*
+ * Return the subscription's connection string, loading it into the
+ * subscription memory context if necessary.
+ *
+ * GetSubscription must be called earlier to set sub->serverid, because ACL
+ * checks are performed there.
+ */
+char *
+GetSubscriptionConnInfo(Subscription *sub)
+{
+ MemoryContext oldcxt;
+
+ if (sub->conninfo)
+ return sub->conninfo;
+
+ Assert(OidIsValid(sub->serverid));
+
+ oldcxt = MemoryContextSwitchTo(sub->cxt);
+ sub->conninfo = ForeignServerConnectionString(sub->owner,
+ GetForeignServer(sub->serverid));
+ MemoryContextSwitchTo(oldcxt);
+
+ return sub->conninfo;
+}
+
/*
* Return number of subscriptions defined in given database.
* Used by dropdb() to check if database can indeed be dropped.
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 1e10d9d9a58..2b77f3e5592 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1026,7 +1026,8 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data,
/* Try to connect to the publisher. */
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(sub->conninfo, true, true, must_use_password,
+ wrconn = walrcv_connect(GetSubscriptionConnInfo(sub), true, true,
+ must_use_password,
sub->name, &err);
if (!wrconn)
ereport(ERROR,
@@ -1278,7 +1279,8 @@ AlterSubscription_refresh_seq(Subscription *sub)
/* Try to connect to the publisher. */
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(sub->conninfo, true, true, must_use_password,
+ wrconn = walrcv_connect(GetSubscriptionConnInfo(sub), true, true,
+ must_use_password,
sub->name, &err);
if (!wrconn)
ereport(ERROR,
@@ -2129,7 +2131,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
* available.
*/
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(stmt->conninfo ? stmt->conninfo : sub->conninfo,
+ wrconn = walrcv_connect(stmt->conninfo ? stmt->conninfo :
+ GetSubscriptionConnInfo(sub),
true, true, must_use_password, sub->name,
&err);
if (!wrconn)
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index a6a2ad1e49c..4dfc6dfce17 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -164,6 +164,7 @@ typedef struct Subscription
* and the retention duration has not
* exceeded max_retention_duration, when
* defined */
+ Oid serverid; /* Oid of the foreign server, if any */
char *conninfo; /* Connection string to the publisher */
char *slotname; /* Name of the replication slot */
char *synccommit; /* Synchronous commit setting for worker */
@@ -214,6 +215,7 @@ typedef struct Subscription
extern Subscription *GetSubscription(Oid subid, bool missing_ok,
bool aclcheck);
+extern char *GetSubscriptionConnInfo(Subscription *sub);
extern void DisableSubscription(Oid subid);
extern int CountDBSubscriptions(Oid dbid);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 7e3cabdb93f..c0db7636713 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -174,16 +174,49 @@ WARNING: subscription was created, but is not connected
HINT: To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
RESET SESSION AUTHORIZATION;
+CREATE SERVER test_server2 FOREIGN DATA WRAPPER test_fdw;
+GRANT USAGE ON FOREIGN SERVER test_server2 TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server2 OPTIONS(user 'bar', password 'secret');
+RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
-- fail, must connect but lacks USAGE on server, as well as user mapping
DROP SUBSCRIPTION regress_testsub6;
ERROR: could not connect to publisher when attempting to drop replication slot "dummy": subscription owner "regress_subscription_user3" does not have permission on foreign server "test_server"
HINT: Use ALTER SUBSCRIPTION ... DISABLE to disable the subscription, and then use ALTER SUBSCRIPTION ... SET (slot_name = NONE) to disassociate it from the slot.
+-- ok, should not need to resolve conninfo for the old broken server
+ALTER SUBSCRIPTION regress_testsub6 SERVER test_server2;
+SELECT fs.srvname
+ FROM pg_subscription s
+ JOIN pg_foreign_server fs ON fs.oid = s.subserver
+ WHERE s.subname = 'regress_testsub6';
+ srvname
+--------------
+ test_server2
+(1 row)
+
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server2;
+RESET SESSION AUTHORIZATION;
+REVOKE USAGE ON FOREIGN SERVER test_server2 FROM regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+-- ok, should not need to resolve conninfo for the current broken server
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist2 password=secret';
+RESET SESSION AUTHORIZATION;
+SELECT subserver = 0::oid AS uses_conninfo, subconninfo
+ FROM pg_subscription
+ WHERE subname = 'regress_testsub6';
+ uses_conninfo | subconninfo
+---------------+----------------------------------------------
+ t | dbname=regress_doesnotexist2 password=secret
+(1 row)
+
+SET SESSION AUTHORIZATION regress_subscription_user3;
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
+DROP SERVER test_server2;
DROP SERVER test_server;
-- fail, FDW is dependent
DROP FUNCTION test_fdw_connection(oid, oid, internal);
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 2bcb5559a45..598635b6558 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -31,6 +31,7 @@
#include "executor/executor.h"
#include "executor/functions.h"
#include "executor/spi.h"
+#include "foreign/foreign.h"
#include "funcapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
@@ -735,6 +736,8 @@ PG_FUNCTION_INFO_V1(test_fdw_connection);
Datum
test_fdw_connection(PG_FUNCTION_ARGS)
{
+ /* Ensure the test fails if no valid user mapping exists. */
+ GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist user=doesnotexist password=secret"));
}
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 6c3d9632e8a..73cb251e192 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -124,6 +124,12 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
+RESET SESSION AUTHORIZATION;
+CREATE SERVER test_server2 FOREIGN DATA WRAPPER test_fdw;
+GRANT USAGE ON FOREIGN SERVER test_server2 TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server2 OPTIONS(user 'bar', password 'secret');
+
RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
@@ -131,12 +137,33 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
-- fail, must connect but lacks USAGE on server, as well as user mapping
DROP SUBSCRIPTION regress_testsub6;
+-- ok, should not need to resolve conninfo for the old broken server
+ALTER SUBSCRIPTION regress_testsub6 SERVER test_server2;
+SELECT fs.srvname
+ FROM pg_subscription s
+ JOIN pg_foreign_server fs ON fs.oid = s.subserver
+ WHERE s.subname = 'regress_testsub6';
+
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server2;
+RESET SESSION AUTHORIZATION;
+REVOKE USAGE ON FOREIGN SERVER test_server2 FROM regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
+-- ok, should not need to resolve conninfo for the current broken server
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist2 password=secret';
+RESET SESSION AUTHORIZATION;
+SELECT subserver = 0::oid AS uses_conninfo, subconninfo
+ FROM pg_subscription
+ WHERE subname = 'regress_testsub6';
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
+DROP SERVER test_server2;
DROP SERVER test_server;
-- fail, FDW is dependent
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-05-01 03:58 Ajin Cherian <[email protected]>
parent: Chao Li <[email protected]>
1 sibling, 1 reply; 28+ messages in thread
From: Ajin Cherian @ 2026-05-01 03:58 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: pgsql-hackers
On Thu, Apr 30, 2026 at 2:12 PM Chao Li <[email protected]> wrote:
>
>
>
> >
> > 2. I think you should add a comment in the function header above
> > GetSubscription() stating that if aclcheck is false, then the conninfo
> > will be set to null and users need to call GetSubscriptionConnInfo to
> > get the conninfo.
>
> Updated the header comment.
/*
* Fetch the subscription from the syscache.
+ *
+ * If missing_ok is false, throw an error if the subscription is not found.
+ * If true, return NULL in that case.
+ *
+ * If aclcheck is true, check whether the subscription owner has permission on
+ * the subscription's foreign server, and load the connection string from the
+ * foreign server. Later, GetSubscriptionConnInfo() should be called to get
+ * the connection string.
*/
Subscription *
GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
I don't think this comment is right. If aclcheck is true, users need
not call GetSubscriptionConnInfo() to get the connection string, as it
is populated in this function itself. It is only if aclecheck is
false, do callers need to do that.
Isn't that the case? There are multiple places in the apply worker
where GetSubscription() is called with aclcheck is true and
GetSubscriptionConnInfo() is not called there.
regards,
Ajin Cherian
Fujitsu Australia
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-05-05 20:53 Zsolt Parragi <[email protected]>
parent: Chao Li <[email protected]>
1 sibling, 1 reply; 28+ messages in thread
From: Zsolt Parragi @ 2026-05-05 20:53 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: Ajin Cherian <[email protected]>; pgsql-hackers
Hello
- server = GetForeignServer(subform->subserver);
+ server = GetForeignServer(sub->serverid);
Couldn't we also move this inside the if?
+/*
+ * Return the subscription's connection string, loading it into the
+ * subscription memory context if necessary.
+ *
+ * GetSubscription must be called earlier to set sub->serverid, because ACL
+ * checks are performed there.
+ */
+char *
+GetSubscriptionConnInfo(Subscription *sub)
This is related to Ajin's comment earlier, the part about ACL check
seems incorrect to me.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-05-06 07:47 Chao Li <[email protected]>
parent: Ajin Cherian <[email protected]>
0 siblings, 0 replies; 28+ messages in thread
From: Chao Li @ 2026-05-06 07:47 UTC (permalink / raw)
To: Ajin Cherian <[email protected]>; +Cc: pgsql-hackers
> On May 1, 2026, at 11:58, Ajin Cherian <[email protected]> wrote:
>
> On Thu, Apr 30, 2026 at 2:12 PM Chao Li <[email protected]> wrote:
>>
>>
>>
>>>
>>> 2. I think you should add a comment in the function header above
>>> GetSubscription() stating that if aclcheck is false, then the conninfo
>>> will be set to null and users need to call GetSubscriptionConnInfo to
>>> get the conninfo.
>>
>> Updated the header comment.
>
> /*
> * Fetch the subscription from the syscache.
> + *
> + * If missing_ok is false, throw an error if the subscription is not found.
> + * If true, return NULL in that case.
> + *
> + * If aclcheck is true, check whether the subscription owner has permission on
> + * the subscription's foreign server, and load the connection string from the
> + * foreign server. Later, GetSubscriptionConnInfo() should be called to get
> + * the connection string.
> */
> Subscription *
> GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
>
> I don't think this comment is right. If aclcheck is true, users need
> not call GetSubscriptionConnInfo() to get the connection string, as it
> is populated in this function itself. It is only if aclecheck is
> false, do callers need to do that.
> Isn't that the case? There are multiple places in the apply worker
> where GetSubscription() is called with aclcheck is true and
> GetSubscriptionConnInfo() is not called there.
>
> regards,
> Ajin Cherian
> Fujitsu Australia
Hi Ajin,
Thank you for the comment. After rereading that part, I agree the wording is not clear.
What I meant is that GetSubscriptionConnInfo() is a safe accessor, if sub->conninfo has already been resolved, it just returns it; otherwise, it resolves it on demand. So the intended usage is that callers can consistently use GetSubscriptionConnInfo() when they need the connection string.
I also missed doing a broader search for direct uses of sub->conninfo and replacing them with GetSubscriptionConnInfo() where appropriate. Sorry about that.
I will address both issues in v3.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-05-06 07:57 Chao Li <[email protected]>
parent: Zsolt Parragi <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Chao Li @ 2026-05-06 07:57 UTC (permalink / raw)
To: Zsolt Parragi <[email protected]>; +Cc: Ajin Cherian <[email protected]>; pgsql-hackers
> On May 6, 2026, at 04:53, Zsolt Parragi <[email protected]> wrote:
>
> Hello
>
> - server = GetForeignServer(subform->subserver);
> + server = GetForeignServer(sub->serverid);
>
> Couldn't we also move this inside the if?
Ah, true. Both aclresult and server can be moved into the if.
>
> +/*
> + * Return the subscription's connection string, loading it into the
> + * subscription memory context if necessary.
> + *
> + * GetSubscription must be called earlier to set sub->serverid, because ACL
> + * checks are performed there.
> + */
> +char *
> +GetSubscriptionConnInfo(Subscription *sub)
>
> This is related to Ajin's comment earlier, the part about ACL check
> seems incorrect to me.
Yes, see my reply to Ajin in the previous email.
PFA v3 - addressed Ajin and Zsolt’s comments.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] v3-0001-Allow-altering-subscription-server-connection-wit.patch (16.4K, ../../[email protected]/2-v3-0001-Allow-altering-subscription-server-connection-wit.patch)
download | inline diff:
From 065d4574940bf931c070f264c00267ecb58db734 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Wed, 22 Apr 2026 09:14:27 +0800
Subject: [PATCH v3] Allow altering subscription server/connection without
resolving old conninfo
When a subscription uses a foreign server, GetSubscription() resolves the
publisher connection string immediately. That can fail if the subscription
owner no longer has USAGE on the server or no longer has a valid user
mapping.
This becomes a problem for ALTER SUBSCRIPTION commands such as ... SERVER
or ... CONNECTION, because they may need to update the subscription away
from the broken server/conninfo, but currently fail while trying to resolve
the old one first.
Fix this by storing the subscription's server OID in the in-memory
Subscription object and loading conninfo lazily only when it is actually
needed. Add GetSubscriptionConninfo() to fetch the connection string on
demand in the subscription memory context.
This allows ALTER SUBSCRIPTION ... SERVER and ... CONNECTION to succeed
without requiring access to the previous server or user mapping, while
preserving the existing behavior for operations that really need to connect
to the publisher.
Add regression test coverage for both cases.
Author: Chao Li <[email protected]>
Reviewed-by: Ajin Cherian <[email protected]>
Reviewed-by: Zsolt Parragi <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/catalog/pg_subscription.c | 67 ++++++++++++++-----
src/backend/commands/subscriptioncmds.c | 9 ++-
.../replication/logical/sequencesync.c | 2 +-
src/backend/replication/logical/tablesync.c | 2 +-
src/backend/replication/logical/worker.c | 9 +--
src/include/catalog/pg_subscription.h | 2 +
src/test/regress/expected/subscription.out | 33 +++++++++
src/test/regress/regress.c | 3 +
src/test/regress/sql/subscription.sql | 27 ++++++++
9 files changed, 130 insertions(+), 24 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 1f1fdc75af6..0f2594a702f 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -72,6 +72,18 @@ GetPublicationsStr(List *publications, StringInfo dest, bool quote_literal)
/*
* Fetch the subscription from the syscache.
+ *
+ * If missing_ok is false, throw an error if the subscription is not found.
+ * If true, return NULL in that case.
+ *
+ * If the subscription uses a foreign server and aclcheck is true, check
+ * whether the subscription owner has permission on that server and eagerly
+ * load the connection string into sub->conninfo.
+ *
+ * If the subscription uses a foreign server and aclcheck is false,
+ * sub->conninfo is left NULL until it is needed. Call
+ * GetSubscriptionConnInfo() to fetch it on demand. That accessor is also safe
+ * to call when sub->conninfo has already been populated.
*/
Subscription *
GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
@@ -118,32 +130,32 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
sub->retaindeadtuples = subform->subretaindeadtuples;
sub->maxretention = subform->submaxretention;
sub->retentionactive = subform->subretentionactive;
+ sub->serverid = subform->subserver;
+ sub->conninfo = NULL;
/* Get conninfo */
- if (OidIsValid(subform->subserver))
+ if (OidIsValid(sub->serverid))
{
- AclResult aclresult;
- ForeignServer *server;
-
- server = GetForeignServer(subform->subserver);
-
/* recheck ACL if requested */
if (aclcheck)
{
+ AclResult aclresult;
+ ForeignServer *server;
+
+ server = GetForeignServer(sub->serverid);
aclresult = object_aclcheck(ForeignServerRelationId,
- subform->subserver,
+ sub->serverid,
subform->subowner, ACL_USAGE);
-
if (aclresult != ACLCHECK_OK)
ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"",
- GetUserNameFromId(subform->subowner, false),
- server->servername)));
- }
+ errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"",
+ GetUserNameFromId(subform->subowner, false),
+ server->servername));
- sub->conninfo = ForeignServerConnectionString(subform->subowner,
- server);
+ sub->conninfo = ForeignServerConnectionString(subform->subowner,
+ server);
+ }
}
else
{
@@ -197,6 +209,31 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
return sub;
}
+/*
+ * Return the subscription's connection string, loading it into the
+ * subscription memory context if necessary.
+ *
+ * GetSubscription must be called earlier to set sub->serverid, because ACL
+ * checks are performed there.
+ */
+char *
+GetSubscriptionConnInfo(Subscription *sub)
+{
+ MemoryContext oldcxt;
+
+ if (sub->conninfo)
+ return sub->conninfo;
+
+ Assert(OidIsValid(sub->serverid));
+
+ oldcxt = MemoryContextSwitchTo(sub->cxt);
+ sub->conninfo = ForeignServerConnectionString(sub->owner,
+ GetForeignServer(sub->serverid));
+ MemoryContextSwitchTo(oldcxt);
+
+ return sub->conninfo;
+}
+
/*
* Return number of subscriptions defined in given database.
* Used by dropdb() to check if database can indeed be dropped.
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 1e10d9d9a58..2b77f3e5592 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1026,7 +1026,8 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data,
/* Try to connect to the publisher. */
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(sub->conninfo, true, true, must_use_password,
+ wrconn = walrcv_connect(GetSubscriptionConnInfo(sub), true, true,
+ must_use_password,
sub->name, &err);
if (!wrconn)
ereport(ERROR,
@@ -1278,7 +1279,8 @@ AlterSubscription_refresh_seq(Subscription *sub)
/* Try to connect to the publisher. */
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(sub->conninfo, true, true, must_use_password,
+ wrconn = walrcv_connect(GetSubscriptionConnInfo(sub), true, true,
+ must_use_password,
sub->name, &err);
if (!wrconn)
ereport(ERROR,
@@ -2129,7 +2131,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
* available.
*/
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(stmt->conninfo ? stmt->conninfo : sub->conninfo,
+ wrconn = walrcv_connect(stmt->conninfo ? stmt->conninfo :
+ GetSubscriptionConnInfo(sub),
true, true, must_use_password, sub->name,
&err);
if (!wrconn)
diff --git a/src/backend/replication/logical/sequencesync.c b/src/backend/replication/logical/sequencesync.c
index e2ff8d77b16..2ba521c5a43 100644
--- a/src/backend/replication/logical/sequencesync.c
+++ b/src/backend/replication/logical/sequencesync.c
@@ -711,7 +711,7 @@ LogicalRepSyncSequences(void)
* Establish the connection to the publisher for sequence synchronization.
*/
LogRepWorkerWalRcvConn =
- walrcv_connect(MySubscription->conninfo, true, true,
+ walrcv_connect(GetSubscriptionConnInfo(MySubscription), true, true,
must_use_password,
app_name.data, &err);
if (LogRepWorkerWalRcvConn == NULL)
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index eb718114297..d859b0d7311 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1304,7 +1304,7 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
* so that synchronous replication can distinguish them.
*/
LogRepWorkerWalRcvConn =
- walrcv_connect(MySubscription->conninfo, true, true,
+ walrcv_connect(GetSubscriptionConnInfo(MySubscription), true, true,
must_use_password,
slotname, &err);
if (LogRepWorkerWalRcvConn == NULL)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index dd6fc38a41e..e2855b0d077 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -5105,7 +5105,8 @@ maybe_reread_subscription(void)
* 'parallel' to any other value or the server decides not to stream the
* in-progress transaction.
*/
- if (strcmp(newsub->conninfo, MySubscription->conninfo) != 0 ||
+ if (strcmp(GetSubscriptionConnInfo(newsub),
+ GetSubscriptionConnInfo(MySubscription)) != 0 ||
strcmp(newsub->name, MySubscription->name) != 0 ||
strcmp(newsub->slotname, MySubscription->slotname) != 0 ||
newsub->binary != MySubscription->binary ||
@@ -5703,8 +5704,8 @@ run_apply_worker(void)
must_use_password = MySubscription->passwordrequired &&
!MySubscription->ownersuperuser;
- LogRepWorkerWalRcvConn = walrcv_connect(MySubscription->conninfo, true,
- true, must_use_password,
+ LogRepWorkerWalRcvConn = walrcv_connect(GetSubscriptionConnInfo(MySubscription),
+ true, true, must_use_password,
MySubscription->name, &err);
if (LogRepWorkerWalRcvConn == NULL)
@@ -5972,7 +5973,7 @@ SetupApplyOrSyncWorker(int worker_slot)
/* Connect to the origin and start the replication. */
elog(DEBUG1, "connecting to publisher using connection string \"%s\"",
- MySubscription->conninfo);
+ GetSubscriptionConnInfo(MySubscription));
/*
* Setup callback for syscache so that we know when something changes in
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index a6a2ad1e49c..4dfc6dfce17 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -164,6 +164,7 @@ typedef struct Subscription
* and the retention duration has not
* exceeded max_retention_duration, when
* defined */
+ Oid serverid; /* Oid of the foreign server, if any */
char *conninfo; /* Connection string to the publisher */
char *slotname; /* Name of the replication slot */
char *synccommit; /* Synchronous commit setting for worker */
@@ -214,6 +215,7 @@ typedef struct Subscription
extern Subscription *GetSubscription(Oid subid, bool missing_ok,
bool aclcheck);
+extern char *GetSubscriptionConnInfo(Subscription *sub);
extern void DisableSubscription(Oid subid);
extern int CountDBSubscriptions(Oid dbid);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 7e3cabdb93f..c0db7636713 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -174,16 +174,49 @@ WARNING: subscription was created, but is not connected
HINT: To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
RESET SESSION AUTHORIZATION;
+CREATE SERVER test_server2 FOREIGN DATA WRAPPER test_fdw;
+GRANT USAGE ON FOREIGN SERVER test_server2 TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server2 OPTIONS(user 'bar', password 'secret');
+RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
-- fail, must connect but lacks USAGE on server, as well as user mapping
DROP SUBSCRIPTION regress_testsub6;
ERROR: could not connect to publisher when attempting to drop replication slot "dummy": subscription owner "regress_subscription_user3" does not have permission on foreign server "test_server"
HINT: Use ALTER SUBSCRIPTION ... DISABLE to disable the subscription, and then use ALTER SUBSCRIPTION ... SET (slot_name = NONE) to disassociate it from the slot.
+-- ok, should not need to resolve conninfo for the old broken server
+ALTER SUBSCRIPTION regress_testsub6 SERVER test_server2;
+SELECT fs.srvname
+ FROM pg_subscription s
+ JOIN pg_foreign_server fs ON fs.oid = s.subserver
+ WHERE s.subname = 'regress_testsub6';
+ srvname
+--------------
+ test_server2
+(1 row)
+
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server2;
+RESET SESSION AUTHORIZATION;
+REVOKE USAGE ON FOREIGN SERVER test_server2 FROM regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+-- ok, should not need to resolve conninfo for the current broken server
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist2 password=secret';
+RESET SESSION AUTHORIZATION;
+SELECT subserver = 0::oid AS uses_conninfo, subconninfo
+ FROM pg_subscription
+ WHERE subname = 'regress_testsub6';
+ uses_conninfo | subconninfo
+---------------+----------------------------------------------
+ t | dbname=regress_doesnotexist2 password=secret
+(1 row)
+
+SET SESSION AUTHORIZATION regress_subscription_user3;
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
+DROP SERVER test_server2;
DROP SERVER test_server;
-- fail, FDW is dependent
DROP FUNCTION test_fdw_connection(oid, oid, internal);
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 2bcb5559a45..598635b6558 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -31,6 +31,7 @@
#include "executor/executor.h"
#include "executor/functions.h"
#include "executor/spi.h"
+#include "foreign/foreign.h"
#include "funcapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
@@ -735,6 +736,8 @@ PG_FUNCTION_INFO_V1(test_fdw_connection);
Datum
test_fdw_connection(PG_FUNCTION_ARGS)
{
+ /* Ensure the test fails if no valid user mapping exists. */
+ GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist user=doesnotexist password=secret"));
}
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 6c3d9632e8a..73cb251e192 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -124,6 +124,12 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
+RESET SESSION AUTHORIZATION;
+CREATE SERVER test_server2 FOREIGN DATA WRAPPER test_fdw;
+GRANT USAGE ON FOREIGN SERVER test_server2 TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server2 OPTIONS(user 'bar', password 'secret');
+
RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
@@ -131,12 +137,33 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
-- fail, must connect but lacks USAGE on server, as well as user mapping
DROP SUBSCRIPTION regress_testsub6;
+-- ok, should not need to resolve conninfo for the old broken server
+ALTER SUBSCRIPTION regress_testsub6 SERVER test_server2;
+SELECT fs.srvname
+ FROM pg_subscription s
+ JOIN pg_foreign_server fs ON fs.oid = s.subserver
+ WHERE s.subname = 'regress_testsub6';
+
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server2;
+RESET SESSION AUTHORIZATION;
+REVOKE USAGE ON FOREIGN SERVER test_server2 FROM regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
+-- ok, should not need to resolve conninfo for the current broken server
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist2 password=secret';
+RESET SESSION AUTHORIZATION;
+SELECT subserver = 0::oid AS uses_conninfo, subconninfo
+ FROM pg_subscription
+ WHERE subname = 'regress_testsub6';
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
+DROP SERVER test_server2;
DROP SERVER test_server;
-- fail, FDW is dependent
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-05-09 01:01 Jeff Davis <[email protected]>
parent: Chao Li <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Jeff Davis @ 2026-05-09 01:01 UTC (permalink / raw)
To: Chao Li <[email protected]>; Zsolt Parragi <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: Ajin Cherian <[email protected]>; pgsql-hackers
On Wed, 2026-05-06 at 15:57 +0800, Chao Li wrote:
> PFA v3 - addressed Ajin and Zsolt’s comments.
Thank you for the report!
The proposed patch seems unnecessarily complex, though. It seems too
easy to add GetSubscriptionConninfo() in the wrong place and end up
with another problem that's not easily detected.
Can't we just do something like the attached? It's easy to explain at
the call site that, when changing to a different server or using
CONNECTION instead, that we don't need the old conninfo at all.
I included your test case in my patch, and it passes.
Also, Hayato Kuroda's report was an issue also because the error could
be thrown even if slotname was NULL. Patch attached for that, as well.
Thank you, also!
Regards,
Jeff Davis
Attachments:
[text/x-patch] v4-0001-Avoid-errors-during-ALTER-SUBSCRIPTION.patch (12.4K, ../../[email protected]/2-v4-0001-Avoid-errors-during-ALTER-SUBSCRIPTION.patch)
download | inline diff:
From 3430b5c0e29351ebc19c6763411ca1b29b401fec Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Fri, 8 May 2026 16:23:00 -0700
Subject: [PATCH v4 1/2] Avoid errors during ALTER SUBSCRIPTION.
Previously, when retrieving the old Subscription object, constructing
the conninfo could encounter an error during
ForeignServerConnectionString(). ACL errors were handled properly, but
other errors could interfere with a user fixing the problem with ALTER
SUBSCRIPTION.
Add parameter to GetSubscrpition() that allows the caller to bypass
generating the conninfo, which is unneeded in these cases because it's
about to be replaced anyway.
Test case from Chao Li.
Reported-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/catalog/pg_subscription.c | 64 ++++++++++++----------
src/backend/commands/subscriptioncmds.c | 22 ++++++--
src/backend/replication/logical/worker.c | 4 +-
src/include/catalog/pg_subscription.h | 3 +-
src/test/regress/expected/subscription.out | 16 ++++++
src/test/regress/regress.c | 3 +
src/test/regress/sql/subscription.sql | 19 +++++++
7 files changed, 92 insertions(+), 39 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 1f1fdc75af6..79739f0354b 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -74,7 +74,8 @@ GetPublicationsStr(List *publications, StringInfo dest, bool quote_literal)
* Fetch the subscription from the syscache.
*/
Subscription *
-GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
+GetSubscription(Oid subid, bool missing_ok, bool conninfo_needed,
+ bool conninfo_aclcheck)
{
HeapTuple tup;
Subscription *sub;
@@ -100,7 +101,7 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
subform = (Form_pg_subscription) GETSTRUCT(tup);
- sub = palloc_object(Subscription);
+ sub = palloc0_object(Subscription);
sub->cxt = cxt;
sub->oid = subid;
sub->dbid = subform->subdbid;
@@ -120,37 +121,40 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
sub->retentionactive = subform->subretentionactive;
/* Get conninfo */
- if (OidIsValid(subform->subserver))
+ if (conninfo_needed)
{
- AclResult aclresult;
- ForeignServer *server;
-
- server = GetForeignServer(subform->subserver);
-
- /* recheck ACL if requested */
- if (aclcheck)
+ if (OidIsValid(subform->subserver))
{
- aclresult = object_aclcheck(ForeignServerRelationId,
- subform->subserver,
- subform->subowner, ACL_USAGE);
-
- if (aclresult != ACLCHECK_OK)
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"",
- GetUserNameFromId(subform->subowner, false),
- server->servername)));
+ AclResult aclresult;
+ ForeignServer *server;
+
+ server = GetForeignServer(subform->subserver);
+
+ /* recheck ACL if requested */
+ if (conninfo_aclcheck)
+ {
+ aclresult = object_aclcheck(ForeignServerRelationId,
+ subform->subserver,
+ subform->subowner, ACL_USAGE);
+
+ if (aclresult != ACLCHECK_OK)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"",
+ GetUserNameFromId(subform->subowner, false),
+ server->servername)));
+ }
+
+ sub->conninfo = ForeignServerConnectionString(subform->subowner,
+ server);
+ }
+ else
+ {
+ datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID,
+ tup,
+ Anum_pg_subscription_subconninfo);
+ sub->conninfo = TextDatumGetCString(datum);
}
-
- sub->conninfo = ForeignServerConnectionString(subform->subowner,
- server);
- }
- else
- {
- datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID,
- tup,
- Anum_pg_subscription_subconninfo);
- sub->conninfo = TextDatumGetCString(datum);
}
/* Get slotname */
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 1e10d9d9a58..21b27991d3c 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1424,6 +1424,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
bool update_failover = false;
bool update_two_phase = false;
bool check_pub_rdt = false;
+ bool conninfo_needed = true;
bool retain_dead_tuples;
int max_retention;
bool retention_active;
@@ -1454,13 +1455,22 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
stmt->subname);
/*
- * Skip ACL checks on the subscription's foreign server, if any. If
- * changing the server (or replacing it with a raw connection), then the
- * old one will be removed anyway. If changing something unrelated,
- * there's no need to do an additional ACL check here; that will be done
- * by the subscription worker anyway.
+ * If we're replacing the connection information, we don't need the old
+ * conninfo at all. Trying to construct it could encounter errors, which
+ * would prevent the user from fixing the problem.
*/
- sub = GetSubscription(subid, false, false);
+ if (stmt->kind == ALTER_SUBSCRIPTION_SERVER ||
+ stmt->kind == ALTER_SUBSCRIPTION_CONNECTION)
+ conninfo_needed = false;
+
+ /*
+ * Skip ACL checks when constructing the existing connection information;
+ * the ACL check will be performed on the new connection information, if
+ * any. If changing something unrelated to conninfo, there's no need to do
+ * an additional ACL check on the foreign server here; that will be done
+ * by the subscription worker.
+ */
+ sub = GetSubscription(subid, false, conninfo_needed, false);
retain_dead_tuples = sub->retaindeadtuples;
origin = sub->origin;
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index dd6fc38a41e..909bd47f0a9 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -5059,7 +5059,7 @@ maybe_reread_subscription(void)
started_tx = true;
}
- newsub = GetSubscription(MyLogicalRepWorker->subid, true, true);
+ newsub = GetSubscription(MyLogicalRepWorker->subid, true, true, true);
if (newsub)
{
@@ -5808,7 +5808,7 @@ InitializeLogRepWorker(void)
LockSharedObject(SubscriptionRelationId, MyLogicalRepWorker->subid, 0,
AccessShareLock);
- MySubscription = GetSubscription(MyLogicalRepWorker->subid, true, true);
+ MySubscription = GetSubscription(MyLogicalRepWorker->subid, true, true, true);
if (MySubscription)
{
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index a6a2ad1e49c..48944201889 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -213,7 +213,8 @@ typedef struct Subscription
#endif /* EXPOSE_TO_CLIENT_CODE */
extern Subscription *GetSubscription(Oid subid, bool missing_ok,
- bool aclcheck);
+ bool conninfo_needed,
+ bool conninfo_aclcheck);
extern void DisableSubscription(Oid subid);
extern int CountDBSubscriptions(Oid dbid);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 7e3cabdb93f..128f44eaacd 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -174,16 +174,32 @@ WARNING: subscription was created, but is not connected
HINT: To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
RESET SESSION AUTHORIZATION;
+CREATE SERVER test_server2 FOREIGN DATA WRAPPER test_fdw;
+GRANT USAGE ON FOREIGN SERVER test_server2 TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server2 OPTIONS(user 'bar', password 'secret');
+RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
-- fail, must connect but lacks USAGE on server, as well as user mapping
DROP SUBSCRIPTION regress_testsub6;
ERROR: could not connect to publisher when attempting to drop replication slot "dummy": subscription owner "regress_subscription_user3" does not have permission on foreign server "test_server"
HINT: Use ALTER SUBSCRIPTION ... DISABLE to disable the subscription, and then use ALTER SUBSCRIPTION ... SET (slot_name = NONE) to disassociate it from the slot.
+-- ok, should not need to resolve conninfo for the old broken server
+ALTER SUBSCRIPTION regress_testsub6 SERVER test_server2;
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server2;
+RESET SESSION AUTHORIZATION;
+REVOKE USAGE ON FOREIGN SERVER test_server2 FROM regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+-- ok, should not need to resolve conninfo for the current broken server
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist2 password=secret';
+RESET SESSION AUTHORIZATION;
+SET SESSION AUTHORIZATION regress_subscription_user3;
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
+DROP SERVER test_server2;
DROP SERVER test_server;
-- fail, FDW is dependent
DROP FUNCTION test_fdw_connection(oid, oid, internal);
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 2bcb5559a45..598635b6558 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -31,6 +31,7 @@
#include "executor/executor.h"
#include "executor/functions.h"
#include "executor/spi.h"
+#include "foreign/foreign.h"
#include "funcapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
@@ -735,6 +736,8 @@ PG_FUNCTION_INFO_V1(test_fdw_connection);
Datum
test_fdw_connection(PG_FUNCTION_ARGS)
{
+ /* Ensure the test fails if no valid user mapping exists. */
+ GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist user=doesnotexist password=secret"));
}
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 6c3d9632e8a..c90f8a03ed8 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -124,6 +124,12 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
+RESET SESSION AUTHORIZATION;
+CREATE SERVER test_server2 FOREIGN DATA WRAPPER test_fdw;
+GRANT USAGE ON FOREIGN SERVER test_server2 TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server2 OPTIONS(user 'bar', password 'secret');
+
RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
@@ -131,12 +137,25 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
-- fail, must connect but lacks USAGE on server, as well as user mapping
DROP SUBSCRIPTION regress_testsub6;
+-- ok, should not need to resolve conninfo for the old broken server
+ALTER SUBSCRIPTION regress_testsub6 SERVER test_server2;
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server2;
+RESET SESSION AUTHORIZATION;
+REVOKE USAGE ON FOREIGN SERVER test_server2 FROM regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
+-- ok, should not need to resolve conninfo for the current broken server
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist2 password=secret';
+RESET SESSION AUTHORIZATION;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
+DROP SERVER test_server2;
DROP SERVER test_server;
-- fail, FDW is dependent
--
2.43.0
[text/x-patch] v4-0002-Avoid-errors-during-DROP-SUBSCRIPTION.patch (4.1K, ../../[email protected]/3-v4-0002-Avoid-errors-during-DROP-SUBSCRIPTION.patch)
download | inline diff:
From 39b2a13ee7904256fc100a0f92104c056386712e Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Fri, 8 May 2026 17:16:50 -0700
Subject: [PATCH v4 2/2] Avoid errors during DROP SUBSCRIPTION.
Previously, an error during ForeignServerConnectionString() could
prevent a subscription from being dropped, even if slot_name was
NULL. ACL check errors were handled, but not other errors, such as a
dropped user mapping.
Change to only construct conninfo if slotname is defined. Errors
(aside from ACL check failures) will not benefit from the more helpful
ReportSlotConnectionError(), but at least they will not interfere with
a user fixing the problem.
Reported-by: Hayato Kuroda (Fujitsu) <[email protected]>
Discussion: https://postgr.es/m/OS9PR01MB12149B54DEA148108C6FA5667F52D2@OS9PR01MB12149.jpnprd01.prod.outlook.com
---
src/backend/commands/subscriptioncmds.c | 71 +++++++++++++------------
1 file changed, 37 insertions(+), 34 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 21b27991d3c..498a102200e 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2193,7 +2193,7 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
Datum datum;
bool isnull;
char *subname;
- char *conninfo;
+ char *conninfo = NULL;
char *slotname;
List *subworkers;
ListCell *lc;
@@ -2256,39 +2256,6 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
Anum_pg_subscription_subname);
subname = pstrdup(NameStr(*DatumGetName(datum)));
- /* Get conninfo */
- if (OidIsValid(form->subserver))
- {
- AclResult aclresult;
- ForeignServer *server;
-
- server = GetForeignServer(form->subserver);
- aclresult = object_aclcheck(ForeignServerRelationId, form->subserver,
- form->subowner, ACL_USAGE);
- if (aclresult != ACLCHECK_OK)
- {
- /*
- * Unable to generate connection string because permissions on the
- * foreign server have been removed. Follow the same logic as an
- * unusable subconninfo (which will result in an ERROR later
- * unless slot_name = NONE).
- */
- err = psprintf(_("subscription owner \"%s\" does not have permission on foreign server \"%s\""),
- GetUserNameFromId(form->subowner, false),
- server->servername);
- conninfo = NULL;
- }
- else
- conninfo = ForeignServerConnectionString(form->subowner,
- server);
- }
- else
- {
- datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
- Anum_pg_subscription_subconninfo);
- conninfo = TextDatumGetCString(datum);
- }
-
/* Get slotname */
datum = SysCacheGetAttr(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_subslotname, &isnull);
@@ -2297,6 +2264,42 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
else
slotname = NULL;
+ /* conninfo only needed if slotname is valid */
+ if (slotname)
+ {
+ if (OidIsValid(form->subserver))
+ {
+ AclResult aclresult;
+ ForeignServer *server;
+
+ server = GetForeignServer(form->subserver);
+ aclresult = object_aclcheck(ForeignServerRelationId, form->subserver,
+ form->subowner, ACL_USAGE);
+ if (aclresult != ACLCHECK_OK)
+ {
+ /*
+ * Unable to generate connection string because permissions on
+ * the foreign server have been removed. Follow the same logic
+ * as an unusable subconninfo and ReportSlotConnectionError()
+ * for a more helpful error.
+ */
+ err = psprintf(_("subscription owner \"%s\" does not have permission on foreign server \"%s\""),
+ GetUserNameFromId(form->subowner, false),
+ server->servername);
+ conninfo = NULL;
+ }
+ else
+ conninfo = ForeignServerConnectionString(form->subowner,
+ server);
+ }
+ else
+ {
+ datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
+ Anum_pg_subscription_subconninfo);
+ conninfo = TextDatumGetCString(datum);
+ }
+ }
+
/*
* Since dropping a replication slot is not transactional, the replication
* slot stays dropped even if the transaction rolls back. So we cannot
--
2.43.0
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-05-09 03:08 Chao Li <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Chao Li @ 2026-05-09 03:08 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
> On May 9, 2026, at 09:01, Jeff Davis <[email protected]> wrote:
>
> On Wed, 2026-05-06 at 15:57 +0800, Chao Li wrote:
>> PFA v3 - addressed Ajin and Zsolt’s comments.
>
> Thank you for the report!
>
> The proposed patch seems unnecessarily complex, though. It seems too
> easy to add GetSubscriptionConninfo() in the wrong place and end up
> with another problem that's not easily detected.
>
> Can't we just do something like the attached? It's easy to explain at
> the call site that, when changing to a different server or using
> CONNECTION instead, that we don't need the old conninfo at all.
>
> I included your test case in my patch, and it passes.
>
> Also, Hayato Kuroda's report was an issue also because the error could
> be thrown even if slotname was NULL. Patch attached for that, as well.
> Thank you, also!
>
> Regards,
> Jeff Davis
>
> <v4-0001-Avoid-errors-during-ALTER-SUBSCRIPTION.patch><v4-0002-Avoid-errors-during-DROP-SUBSCRIPTION.patch>
Ah, I see. You added a new conninfo_needed parameter to GetSubscription(), which separates the decision of building conninfo from the ACL check. Cool, I believe this is a better approach.
So 0001 looks good to me. nitpick is that conninfo_aclcheck is now only meaningful when conninfo_needed is true. I wonder if we should mention that briefly in the function header comment, or add an assertion such as: Assert(conninfo_needed || !conninfo_aclcheck); to avoid possible misuse of conninfo_aclcheck in the future.
For 0002, I have a doubt. Now conninfo is built only when slotname is not NULL. But after reading through DropSubscription(), I am not sure conninfo is strictly tied to slotname.
For example, this fast path returns only when both slotname is NULL and rstates is NIL:
```
/*
* If there is no slot associated with the subscription, we can finish
* here.
*/
if (!slotname && rstates == NIL)
{
table_close(rel, NoLock);
return;
}
```
That seems to imply that even when slotname is NULL, rstates might still be not NIL.
Later, if conninfo is not NULL, the code connects to the publisher and does some cleanup work for tablesync slots:
```
if (conninfo)
wrconn = walrcv_connect(conninfo, true, true, must_use_password,
subname, &err);
...
/*
* Drop the tablesync slots associated with removed tables.
*
* For SYNCDONE/READY states, the tablesync slot is known to have
* already been dropped by the tablesync worker.
*
* For other states, there is no certainty, maybe the slot does
* not exist yet. Also, if we fail after removing some of the
* slots, next time, it will again try to drop already dropped
* slots and fail. For these reasons, we allow missing_ok = true
* for the drop.
*/
if (rstate->state != SUBREL_STATE_SYNCDONE)
{
char syncslotname[NAMEDATALEN] = {0};
ReplicationSlotNameForTablesync(subid, relid, syncslotname,
sizeof(syncslotname));
ReplicationSlotDropAtPubNode(wrconn, syncslotname, true);
}
```
So with 0002, if slotname is NULL but rstates is not NIL, it looks possible that we no longer build conninfo and therefore skip the cleanup on the publisher side.
Best reagards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-05-14 21:45 Jeff Davis <[email protected]>
parent: Chao Li <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Jeff Davis @ 2026-05-14 21:45 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
Hi,
I added a fix to the series: v5-0001 fixes check_pub_rdt for the
foreign server case.
On Sat, 2026-05-09 at 11:08 +0800, Chao Li wrote:
> Ah, I see. You added a new conninfo_needed parameter to
> GetSubscription(), which separates the decision of building conninfo
> from the ACL check. Cool, I believe this is a better approach.
>
> So 0001 looks good to me. nitpick is that conninfo_aclcheck is now
> only meaningful when conninfo_needed is true. I wonder if we should
> mention that briefly in the function header comment, or add an
> assertion such as: Assert(conninfo_needed || !conninfo_aclcheck); to
> avoid possible misuse of conninfo_aclcheck in the future.
I refactored the fix in v5-0002 to do this in a more organized way: now
all option parsing happens first, so I can more precisely decide which
paths need conninfo and which ones don't.
> So with 0002, if slotname is NULL but rstates is not NIL, it looks
> possible that we no longer build conninfo and therefore skip the
> cleanup on the publisher side.
I separated this into two patches:
v5-0003 just moves the connection string building after the early exit,
so that if slotname is NONE and rstates is NIL, then it won't try to
build the connection string at all (and therefore won't get an error
while doing so).
v5-0004 fixes the remaining issue when slotname is NONE and rstates is
*not* NIL. It uses a subtransaction to catch the error, so that the
DROP TRANSACTION will still succeed even though it can't connect to the
publisher to drop the tablesync slots. This feels a bit over-
engineered, but it does maintain the expected behavior in this case. It
also routes errors inside of ForeignServerConnectionString() through
ReportSlotConnectionError(), which adds a helpful hint.
Regards,
Jeff Davis
Attachments:
[text/x-patch] v5-0001-Check-retain_dead_tuples-for-ALTER-SUBSCRIPTION-..patch (3.5K, ../../[email protected]/2-v5-0001-Check-retain_dead_tuples-for-ALTER-SUBSCRIPTION-..patch)
download | inline diff:
From f0edc45856a4c6d2ddb9f83d53bd1c50978801d9 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Tue, 12 May 2026 14:57:31 -0700
Subject: [PATCH v5 1/4] Check retain_dead_tuples for ALTER SUBSCRIPTION ...
SERVER.
Previously, the subscription setting retain_dead_tuples didn't cause
ALTER SUBSCRIPTION ... SERVER to check the publisher. And if the
publisher was checked for some other reason, then it would use the old
conninfo.
Fix ALTER SUBSCRIPTION ... SERVER to always check the publisher when
retain_dead_tuples is set, and to use the new connection info, like
ALTER SUBSCRIPTION ... CONNECTION.
---
src/backend/commands/subscriptioncmds.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 7818f667edf..523959ba0ce 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1427,6 +1427,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
bool retain_dead_tuples;
int max_retention;
bool retention_active;
+ char *new_conninfo = NULL;
char *origin;
Subscription *sub;
Form_pg_subscription form;
@@ -1810,7 +1811,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
ForeignServer *new_server;
ObjectAddress referenced;
AclResult aclresult;
- char *conninfo;
/*
* Remove what was there before, either another foreign server
@@ -1846,13 +1846,13 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
/* make sure a user mapping exists */
GetUserMapping(form->subowner, new_server->serverid);
- conninfo = ForeignServerConnectionString(form->subowner,
- new_server);
+ new_conninfo = ForeignServerConnectionString(form->subowner,
+ new_server);
/* Load the library providing us libpq calls. */
load_file("libpqwalreceiver", false);
/* Check the connection info string. */
- walrcv_check_conninfo(conninfo,
+ walrcv_check_conninfo(new_conninfo,
sub->passwordrequired && !sub->ownersuperuser);
values[Anum_pg_subscription_subserver - 1] = ObjectIdGetDatum(new_server->serverid);
@@ -1863,6 +1863,13 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
update_tuple = true;
}
+
+ /*
+ * Since the remote server configuration might have changed,
+ * perform a check to ensure it permits enabling
+ * retain_dead_tuples.
+ */
+ check_pub_rdt = sub->retaindeadtuples;
break;
case ALTER_SUBSCRIPTION_CONNECTION:
@@ -1877,10 +1884,12 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
replaces[Anum_pg_subscription_subserver - 1] = true;
}
+ new_conninfo = stmt->conninfo;
+
/* Load the library providing us libpq calls. */
load_file("libpqwalreceiver", false);
/* Check the connection info string. */
- walrcv_check_conninfo(stmt->conninfo,
+ walrcv_check_conninfo(new_conninfo,
sub->passwordrequired && !sub->ownersuperuser);
values[Anum_pg_subscription_subconninfo - 1] =
@@ -2129,7 +2138,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
* available.
*/
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(stmt->conninfo ? stmt->conninfo : sub->conninfo,
+ wrconn = walrcv_connect(new_conninfo ? new_conninfo : sub->conninfo,
true, true, must_use_password, sub->name,
&err);
if (!wrconn)
--
2.43.0
[text/x-patch] v5-0002-Avoid-errors-during-ALTER-SUBSCRIPTION.patch (18.4K, ../../[email protected]/3-v5-0002-Avoid-errors-during-ALTER-SUBSCRIPTION.patch)
download | inline diff:
From 6ab85d7b22204dd40562157096e6989f04932ae2 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Tue, 12 May 2026 14:10:07 -0700
Subject: [PATCH v5 2/4] Avoid errors during ALTER SUBSCRIPTION.
Previously, when retrieving the old Subscription object, constructing
the conninfo could encounter an error during
ForeignServerConnectionString(). ACL errors were handled properly, but
other errors could interfere with a user fixing the problem with ALTER
SUBSCRIPTION.
Reported-by: Chao Li <[email protected]>
Reviewed-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/catalog/pg_subscription.c | 72 ++++++++------
src/backend/commands/subscriptioncmds.c | 110 +++++++++++++++------
src/backend/replication/logical/worker.c | 4 +-
src/include/catalog/pg_subscription.h | 3 +-
src/test/regress/expected/subscription.out | 28 +++++-
src/test/regress/regress.c | 3 +
src/test/regress/sql/subscription.sql | 34 ++++++-
7 files changed, 181 insertions(+), 73 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 1f1fdc75af6..82fc91e0810 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -72,9 +72,15 @@ GetPublicationsStr(List *publications, StringInfo dest, bool quote_literal)
/*
* Fetch the subscription from the syscache.
+ *
+ * If conninfo_needed is true, conninfo will be constructed, possibly
+ * encountering errors in ForeignServerConnectionString(). Callers not
+ * expecting such errors should pass false, in which case conninfo will be
+ * NULL.
*/
Subscription *
-GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
+GetSubscription(Oid subid, bool missing_ok, bool conninfo_needed,
+ bool conninfo_aclcheck)
{
HeapTuple tup;
Subscription *sub;
@@ -84,6 +90,8 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
MemoryContext cxt;
MemoryContext oldcxt;
+ Assert(conninfo_needed || !conninfo_aclcheck);
+
tup = SearchSysCache1(SUBSCRIPTIONOID, ObjectIdGetDatum(subid));
if (!HeapTupleIsValid(tup))
@@ -100,7 +108,7 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
subform = (Form_pg_subscription) GETSTRUCT(tup);
- sub = palloc_object(Subscription);
+ sub = palloc0_object(Subscription);
sub->cxt = cxt;
sub->oid = subid;
sub->dbid = subform->subdbid;
@@ -119,38 +127,40 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
sub->maxretention = subform->submaxretention;
sub->retentionactive = subform->subretentionactive;
- /* Get conninfo */
- if (OidIsValid(subform->subserver))
+ if (conninfo_needed)
{
- AclResult aclresult;
- ForeignServer *server;
-
- server = GetForeignServer(subform->subserver);
-
- /* recheck ACL if requested */
- if (aclcheck)
+ if (OidIsValid(subform->subserver))
{
- aclresult = object_aclcheck(ForeignServerRelationId,
- subform->subserver,
- subform->subowner, ACL_USAGE);
-
- if (aclresult != ACLCHECK_OK)
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"",
- GetUserNameFromId(subform->subowner, false),
- server->servername)));
+ AclResult aclresult;
+ ForeignServer *server;
+
+ server = GetForeignServer(subform->subserver);
+
+ if (conninfo_aclcheck)
+ {
+ /* recheck ACL if requested */
+ aclresult = object_aclcheck(ForeignServerRelationId,
+ subform->subserver,
+ subform->subowner, ACL_USAGE);
+
+ if (aclresult != ACLCHECK_OK)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"",
+ GetUserNameFromId(subform->subowner, false),
+ server->servername)));
+ }
+
+ sub->conninfo = ForeignServerConnectionString(subform->subowner,
+ server);
+ }
+ else
+ {
+ datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID,
+ tup,
+ Anum_pg_subscription_subconninfo);
+ sub->conninfo = TextDatumGetCString(datum);
}
-
- sub->conninfo = ForeignServerConnectionString(subform->subowner,
- server);
- }
- else
- {
- datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID,
- tup,
- Anum_pg_subscription_subconninfo);
- sub->conninfo = TextDatumGetCString(datum);
}
/* Get slotname */
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 523959ba0ce..01e992f656e 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1420,6 +1420,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
Datum values[Natts_pg_subscription];
HeapTuple tup;
Oid subid;
+ bool conninfo_needed = true;
bool update_tuple = false;
bool update_failover = false;
bool update_two_phase = false;
@@ -1454,14 +1455,88 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION,
stmt->subname);
+ /* parse and check options */
+ switch (stmt->kind)
+ {
+ case ALTER_SUBSCRIPTION_OPTIONS:
+ supported_opts = (SUBOPT_SLOT_NAME |
+ SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
+ SUBOPT_STREAMING | SUBOPT_TWOPHASE_COMMIT |
+ SUBOPT_DISABLE_ON_ERR |
+ SUBOPT_PASSWORD_REQUIRED |
+ SUBOPT_RUN_AS_OWNER | SUBOPT_FAILOVER |
+ SUBOPT_RETAIN_DEAD_TUPLES |
+ SUBOPT_MAX_RETENTION_DURATION |
+ SUBOPT_WAL_RECEIVER_TIMEOUT |
+ SUBOPT_ORIGIN);
+ break;
+
+ case ALTER_SUBSCRIPTION_ENABLED:
+ supported_opts = SUBOPT_ENABLED;
+ break;
+
+ case ALTER_SUBSCRIPTION_SET_PUBLICATION:
+ supported_opts = SUBOPT_COPY_DATA | SUBOPT_REFRESH;
+ break;
+
+ case ALTER_SUBSCRIPTION_ADD_PUBLICATION:
+ case ALTER_SUBSCRIPTION_DROP_PUBLICATION:
+ supported_opts = SUBOPT_REFRESH | SUBOPT_COPY_DATA;
+ break;
+
+ case ALTER_SUBSCRIPTION_REFRESH_PUBLICATION:
+ supported_opts = SUBOPT_COPY_DATA;
+ break;
+
+ case ALTER_SUBSCRIPTION_SKIP:
+ supported_opts = SUBOPT_LSN;
+ break;
+
+ default:
+ supported_opts = 0;
+ break;
+ }
+
+ if (supported_opts > 0)
+ parse_subscription_options(pstate, stmt->options, supported_opts, &opts);
+
+ /*
+ * Ensure that ALTER SUBSCRIPTION commands that could be used to fix a
+ * broken connection or prepare to drop a broken subscription don't
+ * attempt to construct the conninfo. Otherwise, we might encounter the
+ * error the user is trying to fix.
+ *
+ * Specifically, ALTER SUBSCRIPTION ... SERVER, ALTER SUBSCRIPTION ...
+ * CONNECTION, or ALTER SUBSCRIPTION ... SET (slot_name = NONE).
+ *
+ * NB: if the user specifies multiple SET options, then we may still need
+ * to construct conninfo even if slot_name is set to NONE.
+ */
+ if (stmt->kind == ALTER_SUBSCRIPTION_SERVER ||
+ stmt->kind == ALTER_SUBSCRIPTION_CONNECTION)
+ {
+ conninfo_needed = false;
+ }
+ else if (stmt->kind == ALTER_SUBSCRIPTION_OPTIONS)
+ {
+ if (IsSet(opts.specified_opts, SUBOPT_SLOT_NAME) && !opts.slot_name)
+ conninfo_needed = false;
+
+ /* for these options, we still need conninfo */
+ if (IsSet(opts.specified_opts, SUBOPT_FAILOVER) ||
+ IsSet(opts.specified_opts, SUBOPT_TWOPHASE_COMMIT) ||
+ IsSet(opts.specified_opts, SUBOPT_RETAIN_DEAD_TUPLES))
+ conninfo_needed = true;
+ }
+
/*
* Skip ACL checks on the subscription's foreign server, if any. If
* changing the server (or replacing it with a raw connection), then the
* old one will be removed anyway. If changing something unrelated,
* there's no need to do an additional ACL check here; that will be done
- * by the subscription worker anyway.
+ * by the subscription worker.
*/
- sub = GetSubscription(subid, false, false);
+ sub = GetSubscription(subid, false, conninfo_needed, false);
retain_dead_tuples = sub->retaindeadtuples;
origin = sub->origin;
@@ -1492,20 +1567,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
{
case ALTER_SUBSCRIPTION_OPTIONS:
{
- supported_opts = (SUBOPT_SLOT_NAME |
- SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
- SUBOPT_STREAMING | SUBOPT_TWOPHASE_COMMIT |
- SUBOPT_DISABLE_ON_ERR |
- SUBOPT_PASSWORD_REQUIRED |
- SUBOPT_RUN_AS_OWNER | SUBOPT_FAILOVER |
- SUBOPT_RETAIN_DEAD_TUPLES |
- SUBOPT_MAX_RETENTION_DURATION |
- SUBOPT_WAL_RECEIVER_TIMEOUT |
- SUBOPT_ORIGIN);
-
- parse_subscription_options(pstate, stmt->options,
- supported_opts, &opts);
-
if (IsSet(opts.specified_opts, SUBOPT_SLOT_NAME))
{
/*
@@ -1769,8 +1830,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
case ALTER_SUBSCRIPTION_ENABLED:
{
- parse_subscription_options(pstate, stmt->options,
- SUBOPT_ENABLED, &opts);
Assert(IsSet(opts.specified_opts, SUBOPT_ENABLED));
if (!sub->slotname && opts.enabled)
@@ -1907,10 +1966,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
case ALTER_SUBSCRIPTION_SET_PUBLICATION:
{
- supported_opts = SUBOPT_COPY_DATA | SUBOPT_REFRESH;
- parse_subscription_options(pstate, stmt->options,
- supported_opts, &opts);
-
values[Anum_pg_subscription_subpublications - 1] =
publicationListToArray(stmt->publication);
replaces[Anum_pg_subscription_subpublications - 1] = true;
@@ -1954,10 +2009,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
List *publist;
bool isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
- supported_opts = SUBOPT_REFRESH | SUBOPT_COPY_DATA;
- parse_subscription_options(pstate, stmt->options,
- supported_opts, &opts);
-
publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
values[Anum_pg_subscription_subpublications - 1] =
publicationListToArray(publist);
@@ -2015,9 +2066,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
errmsg("%s is not allowed for disabled subscriptions",
"ALTER SUBSCRIPTION ... REFRESH PUBLICATION")));
- parse_subscription_options(pstate, stmt->options,
- SUBOPT_COPY_DATA, &opts);
-
/*
* The subscription option "two_phase" requires that
* replication has passed the initial table synchronization
@@ -2063,8 +2111,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
case ALTER_SUBSCRIPTION_SKIP:
{
- parse_subscription_options(pstate, stmt->options, SUBOPT_LSN, &opts);
-
/* ALTER SUBSCRIPTION ... SKIP supports only LSN option */
Assert(IsSet(opts.specified_opts, SUBOPT_LSN));
@@ -2130,6 +2176,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
char *err;
WalReceiverConn *wrconn;
+ Assert(conninfo_needed);
+
/* Load the library providing us libpq calls. */
load_file("libpqwalreceiver", false);
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index dd6fc38a41e..909bd47f0a9 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -5059,7 +5059,7 @@ maybe_reread_subscription(void)
started_tx = true;
}
- newsub = GetSubscription(MyLogicalRepWorker->subid, true, true);
+ newsub = GetSubscription(MyLogicalRepWorker->subid, true, true, true);
if (newsub)
{
@@ -5808,7 +5808,7 @@ InitializeLogRepWorker(void)
LockSharedObject(SubscriptionRelationId, MyLogicalRepWorker->subid, 0,
AccessShareLock);
- MySubscription = GetSubscription(MyLogicalRepWorker->subid, true, true);
+ MySubscription = GetSubscription(MyLogicalRepWorker->subid, true, true, true);
if (MySubscription)
{
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index a6a2ad1e49c..48944201889 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -213,7 +213,8 @@ typedef struct Subscription
#endif /* EXPOSE_TO_CLIENT_CODE */
extern Subscription *GetSubscription(Oid subid, bool missing_ok,
- bool aclcheck);
+ bool conninfo_needed,
+ bool conninfo_aclcheck);
extern void DisableSubscription(Oid subid);
extern int CountDBSubscriptions(Oid dbid);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 7e3cabdb93f..3e44232eb23 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -169,19 +169,39 @@ DETAIL: Foreign data wrapper must be defined with CONNECTION specified.
RESET SESSION AUTHORIZATION;
ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
SET SESSION AUTHORIZATION regress_subscription_user3;
-CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
+CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
+ PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
WARNING: subscription was created, but is not connected
HINT: To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
--- fail, must connect but lacks USAGE on server, as well as user mapping
+-- ok, lacks USAGE on test_server, but replacing connection anyway
+BEGIN;
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
+ABORT;
+-- fails, cannot drop slot
DROP SUBSCRIPTION regress_testsub6;
ERROR: could not connect to publisher when attempting to drop replication slot "dummy": subscription owner "regress_subscription_user3" does not have permission on foreign server "test_server"
HINT: Use ALTER SUBSCRIPTION ... DISABLE to disable the subscription, and then use ALTER SUBSCRIPTION ... SET (slot_name = NONE) to disassociate it from the slot.
+RESET SESSION AUTHORIZATION;
+GRANT USAGE ON FOREIGN SERVER test_server TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
-DROP SUBSCRIPTION regress_testsub6;
+DROP SUBSCRIPTION regress_testsub6; --ok
+CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
+ PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
+WARNING: subscription was created, but is not connected
+HINT: To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
+-- ok, test_server lacks user mapping, but replacing connection anyway
+BEGIN;
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
+ABORT;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
+ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
+DROP SUBSCRIPTION regress_testsub6; --ok
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
DROP SERVER test_server;
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 2bcb5559a45..598635b6558 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -31,6 +31,7 @@
#include "executor/executor.h"
#include "executor/functions.h"
#include "executor/spi.h"
+#include "foreign/foreign.h"
#include "funcapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
@@ -735,6 +736,8 @@ PG_FUNCTION_INFO_V1(test_fdw_connection);
Datum
test_fdw_connection(PG_FUNCTION_ARGS)
{
+ /* Ensure the test fails if no valid user mapping exists. */
+ GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist user=doesnotexist password=secret"));
}
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 6c3d9632e8a..f610d6ade18 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -121,18 +121,44 @@ RESET SESSION AUTHORIZATION;
ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
SET SESSION AUTHORIZATION regress_subscription_user3;
-CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
+CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
+ PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
--- fail, must connect but lacks USAGE on server, as well as user mapping
+-- ok, lacks USAGE on test_server, but replacing connection anyway
+BEGIN;
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
+ABORT;
+
+-- fails, cannot drop slot
DROP SUBSCRIPTION regress_testsub6;
+RESET SESSION AUTHORIZATION;
+GRANT USAGE ON FOREIGN SERVER test_server TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
-DROP SUBSCRIPTION regress_testsub6;
+DROP SUBSCRIPTION regress_testsub6; --ok
+
+CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
+ PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
+
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
+
+-- ok, test_server lacks user mapping, but replacing connection anyway
+BEGIN;
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
+ABORT;
+
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
+
+ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
+DROP SUBSCRIPTION regress_testsub6; --ok
+
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
--
2.43.0
[text/x-patch] v5-0003-Avoid-errors-during-DROP-SUBSCRIPTION-when-slot_n.patch (7.2K, ../../[email protected]/4-v5-0003-Avoid-errors-during-DROP-SUBSCRIPTION-when-slot_n.patch)
download | inline diff:
From 42bd12a6338d09d6832e4af1e69f85c2790dad21 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Tue, 12 May 2026 14:29:29 -0700
Subject: [PATCH v5 3/4] Avoid errors during DROP SUBSCRIPTION when slot_name
is NONE.
Previously, if the subscription used a server,
ForeignServerConnectionString() could raise an error (e.g. missing
user mapping) during DROP SUBSCRIPTION even if the conninfo wasn't
needed at all.
Construct conninfo after the early return, so that if slot_name is
NONE and rstates is NIL, the DROP SUBSCRIPTION will succeed even if
ForeignServerConnectionString() raises an error (e.g. missing user
mapping).
If slot_name is NONE and rstates is not NIL, DROP SUBSCRIPTION may
still encounter an error from ForeignServerConnectionString().
Reported-by: Hayato Kuroda (Fujitsu) <[email protected]>
Reviewed-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/OS9PR01MB12149B54DEA148108C6FA5667F52D2@OS9PR01MB12149.jpnprd01.prod.outlook.com
---
src/backend/commands/subscriptioncmds.c | 86 +++++++++++++---------
src/test/regress/expected/subscription.out | 5 +-
src/test/regress/sql/subscription.sql | 5 +-
3 files changed, 57 insertions(+), 39 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 01e992f656e..8a349f7e0be 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -54,6 +54,7 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/pg_lsn.h"
+#include "utils/resowner.h"
#include "utils/syscache.h"
/*
@@ -2226,6 +2227,43 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
return myself;
}
+/*
+ * Construct conninfo from a subscription's server. Like libpqrcv_connect(),
+ * if an error occurs, set *err to the error message and return NULL.
+ *
+ * However, failures in ForeignServerConnectionString() may ereport(ERROR),
+ * and (also like libpqrcv_connect) it's not worth adding the machinery to
+ * pass all of those back to the caller just to cover this one case.
+ */
+static char *
+construct_subserver_conninfo(Oid subserver, Oid subowner, char **err)
+{
+ AclResult aclresult;
+ ForeignServer *server;
+
+ *err = NULL;
+
+ server = GetForeignServer(subserver);
+
+ aclresult = object_aclcheck(ForeignServerRelationId, subserver,
+ subowner, ACL_USAGE);
+ if (aclresult != ACLCHECK_OK)
+ {
+ /*
+ * Unable to generate connection string because permissions on the
+ * foreign server have been removed. Follow the same logic as an
+ * unusable subconninfo (which will result in an ERROR later
+ * unless slot_name = NONE).
+ */
+ *err = psprintf(_("subscription owner \"%s\" does not have permission on foreign server \"%s\""),
+ GetUserNameFromId(subowner, false),
+ server->servername);
+ return NULL;
+ }
+
+ return ForeignServerConnectionString(subowner, server);
+}
+
/*
* Drop a subscription
*/
@@ -2237,10 +2275,12 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
HeapTuple tup;
Oid subid;
Oid subowner;
+ Oid subserver;
+ char *subconninfo = NULL;
Datum datum;
bool isnull;
char *subname;
- char *conninfo;
+ char *conninfo = NULL;
char *slotname;
List *subworkers;
ListCell *lc;
@@ -2279,9 +2319,15 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
return;
}
+ datum = SysCacheGetAttr(SUBSCRIPTIONOID, tup,
+ Anum_pg_subscription_subconninfo, &isnull);
+ if (!isnull)
+ subconninfo = TextDatumGetCString(datum);
+
form = (Form_pg_subscription) GETSTRUCT(tup);
subid = form->oid;
subowner = form->subowner;
+ subserver = form->subserver;
must_use_password = !superuser_arg(subowner) && form->subpasswordrequired;
/* must be owner */
@@ -2303,39 +2349,6 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
Anum_pg_subscription_subname);
subname = pstrdup(NameStr(*DatumGetName(datum)));
- /* Get conninfo */
- if (OidIsValid(form->subserver))
- {
- AclResult aclresult;
- ForeignServer *server;
-
- server = GetForeignServer(form->subserver);
- aclresult = object_aclcheck(ForeignServerRelationId, form->subserver,
- form->subowner, ACL_USAGE);
- if (aclresult != ACLCHECK_OK)
- {
- /*
- * Unable to generate connection string because permissions on the
- * foreign server have been removed. Follow the same logic as an
- * unusable subconninfo (which will result in an ERROR later
- * unless slot_name = NONE).
- */
- err = psprintf(_("subscription owner \"%s\" does not have permission on foreign server \"%s\""),
- GetUserNameFromId(form->subowner, false),
- server->servername);
- conninfo = NULL;
- }
- else
- conninfo = ForeignServerConnectionString(form->subowner,
- server);
- }
- else
- {
- datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
- Anum_pg_subscription_subconninfo);
- conninfo = TextDatumGetCString(datum);
- }
-
/* Get slotname */
datum = SysCacheGetAttr(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_subslotname, &isnull);
@@ -2472,6 +2485,11 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
*/
load_file("libpqwalreceiver", false);
+ if (OidIsValid(subserver))
+ conninfo = construct_subserver_conninfo(subserver, subowner, &err);
+ else
+ conninfo = subconninfo;
+
if (conninfo)
wrconn = walrcv_connect(conninfo, true, true, must_use_password,
subname, &err);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 3e44232eb23..41bc265edfb 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -198,10 +198,11 @@ DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
BEGIN;
ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
ABORT;
-CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
+-- fails, cannot drop slot
+DROP SUBSCRIPTION regress_testsub6;
+ERROR: user mapping not found for user "regress_subscription_user3", server "test_server"
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6; --ok
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
DROP SERVER test_server;
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index f610d6ade18..576ee17dfd4 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -153,13 +153,12 @@ BEGIN;
ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
ABORT;
-CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
+-- fails, cannot drop slot
+DROP SUBSCRIPTION regress_testsub6;
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6; --ok
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
-
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
--
2.43.0
[text/x-patch] v5-0004-DROP-SUBSCRIPTION-improve-error-message.patch (4.9K, ../../[email protected]/5-v5-0004-DROP-SUBSCRIPTION-improve-error-message.patch)
download | inline diff:
From 56d29ab1c09839fd24dd1c947bf22069534554a0 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Thu, 14 May 2026 10:59:53 -0700
Subject: [PATCH v5 4/4] DROP SUBSCRIPTION: improve error message.
Previously, when constructing the conninfo for a subscription using a
server, errors from ForeignServerConnectionString() were raised
immediately, losing the helpful HINT as well as warnings about
tablesync slots. ACL errors were handled by explicitly formatting an
error message and passing it to ReportSlotConnectionError().
Use a subtransaction to capture ACL errors and
ForeignServerConnectionString() errors and report with
ReportSlotConnectionError() consistently.
Reported-by: Hayato Kuroda (Fujitsu) <[email protected]>
Reviewed-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/OS9PR01MB12149B54DEA148108C6FA5667F52D2@OS9PR01MB12149.jpnprd01.prod.outlook.com
---
src/backend/commands/subscriptioncmds.c | 65 +++++++++++++++-------
src/test/regress/expected/subscription.out | 3 +-
2 files changed, 47 insertions(+), 21 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 8a349f7e0be..7b05b54f9a4 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2231,37 +2231,62 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
* Construct conninfo from a subscription's server. Like libpqrcv_connect(),
* if an error occurs, set *err to the error message and return NULL.
*
- * However, failures in ForeignServerConnectionString() may ereport(ERROR),
- * and (also like libpqrcv_connect) it's not worth adding the machinery to
- * pass all of those back to the caller just to cover this one case.
+ * Uses a subtransaction so that we can catch arbitrary errors from
+ * ForeignServerConnectionString().
*/
static char *
construct_subserver_conninfo(Oid subserver, Oid subowner, char **err)
{
- AclResult aclresult;
- ForeignServer *server;
+ MemoryContext oldcxt = CurrentMemoryContext;
+ ResourceOwner oldresowner = CurrentResourceOwner;
+ ErrorData *edata = NULL;
+ char *conninfo = NULL;
*err = NULL;
- server = GetForeignServer(subserver);
+ BeginInternalSubTransaction(NULL);
+ MemoryContextSwitchTo(oldcxt);
- aclresult = object_aclcheck(ForeignServerRelationId, subserver,
- subowner, ACL_USAGE);
- if (aclresult != ACLCHECK_OK)
+ PG_TRY();
{
- /*
- * Unable to generate connection string because permissions on the
- * foreign server have been removed. Follow the same logic as an
- * unusable subconninfo (which will result in an ERROR later
- * unless slot_name = NONE).
- */
- *err = psprintf(_("subscription owner \"%s\" does not have permission on foreign server \"%s\""),
- GetUserNameFromId(subowner, false),
- server->servername);
- return NULL;
+ AclResult aclresult;
+ ForeignServer *server;
+
+ server = GetForeignServer(subserver);
+ aclresult = object_aclcheck(ForeignServerRelationId, subserver,
+ subowner, ACL_USAGE);
+ if (aclresult != ACLCHECK_OK)
+ ereport(ERROR,
+ errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"",
+ GetUserNameFromId(subowner, false),
+ server->servername));
+
+ conninfo = ForeignServerConnectionString(subowner, server);
+ ReleaseCurrentSubTransaction();
+ MemoryContextSwitchTo(oldcxt);
+ CurrentResourceOwner = oldresowner;
+ }
+ PG_CATCH();
+ {
+ MemoryContextSwitchTo(oldcxt);
+ edata = CopyErrorData();
+ FlushErrorState();
+ RollbackAndReleaseCurrentSubTransaction();
+ MemoryContextSwitchTo(oldcxt);
+ CurrentResourceOwner = oldresowner;
+
+ conninfo = NULL;
+ }
+ PG_END_TRY();
+
+ if (!conninfo)
+ {
+ *err = pstrdup(edata->message);
+ FreeErrorData(edata);
}
- return ForeignServerConnectionString(subowner, server);
+ return conninfo;
}
/*
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 41bc265edfb..1af5aec878f 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -200,7 +200,8 @@ ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist pass
ABORT;
-- fails, cannot drop slot
DROP SUBSCRIPTION regress_testsub6;
-ERROR: user mapping not found for user "regress_subscription_user3", server "test_server"
+ERROR: could not connect to publisher when attempting to drop replication slot "dummy": user mapping not found for user "regress_subscription_user3", server "test_server"
+HINT: Use ALTER SUBSCRIPTION ... DISABLE to disable the subscription, and then use ALTER SUBSCRIPTION ... SET (slot_name = NONE) to disassociate it from the slot.
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6; --ok
SET SESSION AUTHORIZATION regress_subscription_user;
--
2.43.0
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-05-15 07:18 Chao Li <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Chao Li @ 2026-05-15 07:18 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
> On May 15, 2026, at 05:45, Jeff Davis <[email protected]> wrote:
>
> Hi,
>
> I added a fix to the series: v5-0001 fixes check_pub_rdt for the
> foreign server case.
>
> On Sat, 2026-05-09 at 11:08 +0800, Chao Li wrote:
>> Ah, I see. You added a new conninfo_needed parameter to
>> GetSubscription(), which separates the decision of building conninfo
>> from the ACL check. Cool, I believe this is a better approach.
>>
>> So 0001 looks good to me. nitpick is that conninfo_aclcheck is now
>> only meaningful when conninfo_needed is true. I wonder if we should
>> mention that briefly in the function header comment, or add an
>> assertion such as: Assert(conninfo_needed || !conninfo_aclcheck); to
>> avoid possible misuse of conninfo_aclcheck in the future.
>
> I refactored the fix in v5-0002 to do this in a more organized way: now
> all option parsing happens first, so I can more precisely decide which
> paths need conninfo and which ones don't.
>
>> So with 0002, if slotname is NULL but rstates is not NIL, it looks
>> possible that we no longer build conninfo and therefore skip the
>> cleanup on the publisher side.
>
> I separated this into two patches:
>
> v5-0003 just moves the connection string building after the early exit,
> so that if slotname is NONE and rstates is NIL, then it won't try to
> build the connection string at all (and therefore won't get an error
> while doing so).
>
> v5-0004 fixes the remaining issue when slotname is NONE and rstates is
> *not* NIL. It uses a subtransaction to catch the error, so that the
> DROP TRANSACTION will still succeed even though it can't connect to the
> publisher to drop the tablesync slots. This feels a bit over-
> engineered, but it does maintain the expected behavior in this case. It
> also routes errors inside of ForeignServerConnectionString() through
> ReportSlotConnectionError(), which adds a helpful hint.
>
> Regards,
> Jeff Davis
>
>
>
>
> <v5-0001-Check-retain_dead_tuples-for-ALTER-SUBSCRIPTION-..patch><v5-0002-Avoid-errors-during-ALTER-SUBSCRIPTION.patch><v5-0003-Avoid-errors-during-DROP-SUBSCRIPTION-when-slot_n.patch><v5-0004-DROP-SUBSCRIPTION-improve-error-message.patch>
I have just one comment on v5:
In 0002, for both ALTER_SUBSCRIPTION_SERVER and ALTER_SUBSCRIPTION_CONNECTION, conninfo_needed is false:
```
if (stmt->kind == ALTER_SUBSCRIPTION_SERVER ||
stmt->kind == ALTER_SUBSCRIPTION_CONNECTION)
{
conninfo_needed = false;
}
```
0001 adds "check_pub_rdt = sub->retaindeadtuples;" to the both paths:
0002 adds this Assert:
```
if (update_failover || update_two_phase || check_pub_rdt)
{
bool must_use_password;
char *err;
WalReceiverConn *wrconn;
Assert(conninfo_needed);
```
So, for those two paths, if check_pub_rdt is true, then the Assert will be fired, is that intentional?
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-05-18 05:28 Chao Li <[email protected]>
parent: Chao Li <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Chao Li @ 2026-05-18 05:28 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
> On May 16, 2026, at 07:18, Jeff Davis <[email protected]> wrote:
>
> On Fri, 2026-05-15 at 15:18 +0800, Chao Li wrote:
>> 0002 adds this Assert:
>> ```
>> if (update_failover || update_two_phase || check_pub_rdt)
>> {
>> bool must_use_password;
>> char *err;
>> WalReceiverConn *wrconn;
>>
>> Assert(conninfo_needed);
>> ```
>>
>> So, for those two paths, if check_pub_rdt is true, then the Assert
>> will be fired, is that intentional?
>
> I've fixed it to be Assert(new_conninfo || orig_conninfo_needed).
>
> Also, the code above was missing the case of SUBOPT_ORIGIN which could
> set check_pub_rdt. I changed it to be more conservative and set
> orig_conninfo_needed=false when one of:
>
> ALTER SUBSCRIPTION ... SERVER
> ALTER SUBSCRIPTION ... CONNECTION
> ALTER SUBSCRIPTION ... SET (slot_name=NONE)
>
> and not try to be precise about which other settings might need
> check_pub_rdt or not.
Yep, this part looks good now.
>
> What do you think of v6-0003? Is it over-engineered? Should the
> subtransaction happen at a lower level? Is there an alternative to
> using a subtransaction?
>
For the reason you described in the commit message, catching the error and later reporting it through ReportSlotConnectionError(), I don't think this is over-engineered. I also think keeping the subtransaction inside construct_subserver_conninfo() is reasonable, because this error-capturing behavior seems specific to the DROP SUBSCRIPTION path.
As for whether the HINT itself really helps, I would reserve my opinion. As the test output shows:
```
DROP SUBSCRIPTION regress_testsub6;
ERROR: could not connect to publisher when attempting to drop replication slot "dummy": user mapping not found for user "regress_subscription_user3", server "test_server"
HINT: Use ALTER SUBSCRIPTION ... DISABLE to disable the subscription, and then use ALTER SUBSCRIPTION ... SET (slot_name = NONE) to disassociate it from the slot.
```
The error message already says that the problem is “user mapping not found”, so fixing the user mapping could also be a solution. So, the HINT is still useful, but it might not be the most direct fix in some case.
I got another small comment. Now construct_subserver_conninfo() has some duplicate code block of getting foreign server, aclcheck and ForeignServerConnectionString as what GetSubscription() has, maybe we can wrap that piece of code into a helper function.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-05-27 00:44 Amit Kapila <[email protected]>
parent: Chao Li <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Amit Kapila @ 2026-05-27 00:44 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: Jeff Davis <[email protected]>; Zsolt Parragi <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
On Sun, May 17, 2026 at 10:29 PM Chao Li <[email protected]> wrote:
>
> > On May 16, 2026, at 07:18, Jeff Davis <[email protected]> wrote:
> >
> > On Fri, 2026-05-15 at 15:18 +0800, Chao Li wrote:
> >> 0002 adds this Assert:
> >> ```
> >> if (update_failover || update_two_phase || check_pub_rdt)
> >> {
> >> bool must_use_password;
> >> char *err;
> >> WalReceiverConn *wrconn;
> >>
> >> Assert(conninfo_needed);
> >> ```
> >>
> >> So, for those two paths, if check_pub_rdt is true, then the Assert
> >> will be fired, is that intentional?
> >
> > I've fixed it to be Assert(new_conninfo || orig_conninfo_needed).
> >
> > Also, the code above was missing the case of SUBOPT_ORIGIN which could
> > set check_pub_rdt. I changed it to be more conservative and set
> > orig_conninfo_needed=false when one of:
> >
> > ALTER SUBSCRIPTION ... SERVER
> > ALTER SUBSCRIPTION ... CONNECTION
> > ALTER SUBSCRIPTION ... SET (slot_name=NONE)
> >
> > and not try to be precise about which other settings might need
> > check_pub_rdt or not.
>
> Yep, this part looks good now.
>
> >
> > What do you think of v6-0003? Is it over-engineered? Should the
> > subtransaction happen at a lower level? Is there an alternative to
> > using a subtransaction?
> >
>
> For the reason you described in the commit message, catching the error and later reporting it through ReportSlotConnectionError(), I don't think this is over-engineered. I also think keeping the subtransaction inside construct_subserver_conninfo() is reasonable, because this error-capturing behavior seems specific to the DROP SUBSCRIPTION path.
>
> As for whether the HINT itself really helps, I would reserve my opinion. As the test output shows:
> ```
> DROP SUBSCRIPTION regress_testsub6;
> ERROR: could not connect to publisher when attempting to drop replication slot "dummy": user mapping not found for user "regress_subscription_user3", server "test_server"
> HINT: Use ALTER SUBSCRIPTION ... DISABLE to disable the subscription, and then use ALTER SUBSCRIPTION ... SET (slot_name = NONE) to disassociate it from the slot.
> ```
>
> The error message already says that the problem is “user mapping not found”, so fixing the user mapping could also be a solution. So, the HINT is still useful, but it might not be the most direct fix in some case.
>
Right, the hint doesn't sound to be a right solution for the problem reported.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-06-17 20:26 Jeff Davis <[email protected]>
parent: Hayato Kuroda (Fujitsu) <[email protected]>
1 sibling, 0 replies; 28+ messages in thread
From: Jeff Davis @ 2026-06-17 20:26 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <[email protected]>; 'Chao Li' <[email protected]>; +Cc: pgsql-hackers
On Wed, 2026-04-22 at 12:35 +0000, Hayato Kuroda (Fujitsu) wrote:
> Before deep dive to your fix, I'm unclear why dropping the active
> USER MAPPING is
> allowed. Personally, it should be avoided anyway. Do you know why
> it's not restricted?
We don't record dependencies on user mappings because technically it's
not dependent on a specific user mapping. That is, GetUserMapping() can
fall back to the public user mapping.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-06-17 23:10 Jeff Davis <[email protected]>
parent: Amit Kapila <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Jeff Davis @ 2026-06-17 23:10 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; Chao Li <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
On Tue, 2026-05-26 at 17:44 -0700, Amit Kapila wrote:
> > Yep, this part looks good now.
Thank you. I committed 0001 with one additional fix: make sure that
ALTER SUBSCRIPTION DISABLE doesn't try to construct the old conninfo.
I plan to commit 0002 soon.
> >
> > For the reason you described in the commit message, catching the
> > error and later reporting it through ReportSlotConnectionError(), I
> > don't think this is over-engineered. I also think keeping the
> > subtransaction inside construct_subserver_conninfo() is reasonable,
> > because this error-capturing behavior seems specific to the DROP
> > SUBSCRIPTION path.
OK.
> > As for whether the HINT itself really helps, I would reserve my
> > opinion. As the test output shows:
> > ```
> > DROP SUBSCRIPTION regress_testsub6;
> > ERROR: could not connect to publisher when attempting to drop
> > replication slot "dummy": user mapping not found for user
> > "regress_subscription_user3", server "test_server"
> > HINT: Use ALTER SUBSCRIPTION ... DISABLE to disable the
> > subscription, and then use ALTER SUBSCRIPTION ... SET (slot_name =
> > NONE) to disassociate it from the slot.
> > ```
> >
> > The error message already says that the problem is “user mapping
> > not found”, so fixing the user mapping could also be a solution.
> > So, the HINT is still useful, but it might not be the most direct
> > fix in some case.
> >
>
> Right, the hint doesn't sound to be a right solution for the problem
> reported.
The user might no longer have permissions on the server to create a new
user mapping, so if they want to drop the subscription, then they need
to set slot_name=NONE.
In other words, we can't delete the existing hint language; we can only
add new language about adding the user mapping back. I don't see a
great way to add that language without making it too long and
confusing, but I am open to suggestion.
v7-0003 is more than just an error message change, so I updated the
commit message. It handles the case where the drop path has
rstates!=NIL but slot_name=NONE: without 0003, the drop will fail while
trying to construct the conninfo (without the HINT); with 0003, the
drop will silently succeed without removing the tablesync slots (as it
does with a connection failure when subconninfo is set).
If the use of a subtransaction is fine there, then I think we should
proceed with 0003 and whatever hint message is agreeable.
Regards,
Jeff Davis
Attachments:
[text/x-patch] v7-0002-Avoid-errors-during-DROP-SUBSCRIPTION-when-slot_n.patch (7.2K, ../../[email protected]/2-v7-0002-Avoid-errors-during-DROP-SUBSCRIPTION-when-slot_n.patch)
download | inline diff:
From 0b8958d75d65d32c4c41e784ec17ea344957e353 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Tue, 12 May 2026 14:29:29 -0700
Subject: [PATCH v7 2/3] Avoid errors during DROP SUBSCRIPTION when slot_name
is NONE.
Previously, if the subscription used a server,
ForeignServerConnectionString() could raise an error (e.g. missing
user mapping) during DROP SUBSCRIPTION even if the conninfo wasn't
needed at all.
Construct conninfo after the early return, so that if slot_name is
NONE and rstates is NIL, the DROP SUBSCRIPTION will succeed even if
ForeignServerConnectionString() raises an error (e.g. missing user
mapping).
If slot_name is NONE and rstates is not NIL, DROP SUBSCRIPTION may
still encounter an error from ForeignServerConnectionString().
Reported-by: Hayato Kuroda (Fujitsu) <[email protected]>
Reviewed-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/OS9PR01MB12149B54DEA148108C6FA5667F52D2@OS9PR01MB12149.jpnprd01.prod.outlook.com
---
src/backend/commands/subscriptioncmds.c | 85 +++++++++++++---------
src/test/regress/expected/subscription.out | 5 +-
src/test/regress/sql/subscription.sql | 5 +-
3 files changed, 56 insertions(+), 39 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index c9faf68cbc5..ee06a726f42 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2260,6 +2260,43 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
return myself;
}
+/*
+ * Construct conninfo from a subscription's server. Like libpqrcv_connect(),
+ * if an error occurs, set *err to the error message and return NULL.
+ *
+ * However, failures in ForeignServerConnectionString() may ereport(ERROR),
+ * and (also like libpqrcv_connect) it's not worth adding the machinery to
+ * pass all of those back to the caller just to cover this one case.
+ */
+static char *
+construct_subserver_conninfo(Oid subserver, Oid subowner, char **err)
+{
+ AclResult aclresult;
+ ForeignServer *server;
+
+ *err = NULL;
+
+ server = GetForeignServer(subserver);
+
+ aclresult = object_aclcheck(ForeignServerRelationId, subserver,
+ subowner, ACL_USAGE);
+ if (aclresult != ACLCHECK_OK)
+ {
+ /*
+ * Unable to generate connection string because permissions on the
+ * foreign server have been removed. Follow the same logic as an
+ * unusable subconninfo (which will result in an ERROR later unless
+ * slot_name = NONE).
+ */
+ *err = psprintf(_("subscription owner \"%s\" does not have permission on foreign server \"%s\""),
+ GetUserNameFromId(subowner, false),
+ server->servername);
+ return NULL;
+ }
+
+ return ForeignServerConnectionString(subowner, server);
+}
+
/*
* Drop a subscription
*/
@@ -2271,10 +2308,12 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
HeapTuple tup;
Oid subid;
Oid subowner;
+ Oid subserver;
+ char *subconninfo = NULL;
Datum datum;
bool isnull;
char *subname;
- char *conninfo;
+ char *conninfo = NULL;
char *slotname;
List *subworkers;
ListCell *lc;
@@ -2313,9 +2352,15 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
return;
}
+ datum = SysCacheGetAttr(SUBSCRIPTIONOID, tup,
+ Anum_pg_subscription_subconninfo, &isnull);
+ if (!isnull)
+ subconninfo = TextDatumGetCString(datum);
+
form = (Form_pg_subscription) GETSTRUCT(tup);
subid = form->oid;
subowner = form->subowner;
+ subserver = form->subserver;
must_use_password = !superuser_arg(subowner) && form->subpasswordrequired;
/* must be owner */
@@ -2337,39 +2382,6 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
Anum_pg_subscription_subname);
subname = pstrdup(NameStr(*DatumGetName(datum)));
- /* Get conninfo */
- if (OidIsValid(form->subserver))
- {
- AclResult aclresult;
- ForeignServer *server;
-
- server = GetForeignServer(form->subserver);
- aclresult = object_aclcheck(ForeignServerRelationId, form->subserver,
- form->subowner, ACL_USAGE);
- if (aclresult != ACLCHECK_OK)
- {
- /*
- * Unable to generate connection string because permissions on the
- * foreign server have been removed. Follow the same logic as an
- * unusable subconninfo (which will result in an ERROR later
- * unless slot_name = NONE).
- */
- err = psprintf(_("subscription owner \"%s\" does not have permission on foreign server \"%s\""),
- GetUserNameFromId(form->subowner, false),
- server->servername);
- conninfo = NULL;
- }
- else
- conninfo = ForeignServerConnectionString(form->subowner,
- server);
- }
- else
- {
- datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
- Anum_pg_subscription_subconninfo);
- conninfo = TextDatumGetCString(datum);
- }
-
/* Get slotname */
datum = SysCacheGetAttr(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_subslotname, &isnull);
@@ -2506,6 +2518,11 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
*/
load_file("libpqwalreceiver", false);
+ if (OidIsValid(subserver))
+ conninfo = construct_subserver_conninfo(subserver, subowner, &err);
+ else
+ conninfo = subconninfo;
+
if (conninfo)
wrconn = walrcv_connect(conninfo, true, true, must_use_password,
subname, &err);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 61236672ce4..8dbfac66326 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -213,11 +213,12 @@ DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
BEGIN;
ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
ABORT;
-CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
+-- fails, cannot drop slot
+DROP SUBSCRIPTION regress_testsub6;
+ERROR: user mapping not found for user "regress_subscription_user3", server "test_server"
ALTER SUBSCRIPTION regress_testsub6 DISABLE;
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6; --ok
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
DROP SERVER test_server;
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 665b510f180..05533d66675 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -164,14 +164,13 @@ BEGIN;
ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
ABORT;
-CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
+-- fails, cannot drop slot
+DROP SUBSCRIPTION regress_testsub6;
ALTER SUBSCRIPTION regress_testsub6 DISABLE;
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6; --ok
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
-
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
--
2.43.0
[text/x-patch] v7-0003-DROP-SUBSCRIPTION-handle-errors-from-fdwconnectio.patch (5.2K, ../../[email protected]/3-v7-0003-DROP-SUBSCRIPTION-handle-errors-from-fdwconnectio.patch)
download | inline diff:
From fc35789170a27db04af0a650058bbe81a546a58f Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Thu, 14 May 2026 10:59:53 -0700
Subject: [PATCH v7 3/3] DROP SUBSCRIPTION: handle errors from fdwconnection.
Previously, when constructing the conninfo for a subscription using a
server, errors from ForeignServerConnectionString() were raised
immediately. That could cause the drop to fail if there were leftover
tablesync slots, even if slot_name was set to NONE. It also lost the
helpful HINT when slot_name was defined. ACL errors were handled, but
arbitrary errors from fdwconnection were not.
Fix by using a subtransaction to capture errors consistently and
either report with ReportSlotConnectionError() if slot_name is defined
or succeed silently if slot_name=NONE.
Reported-by: Hayato Kuroda (Fujitsu) <[email protected]>
Reviewed-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/OS9PR01MB12149B54DEA148108C6FA5667F52D2@OS9PR01MB12149.jpnprd01.prod.outlook.com
---
src/backend/commands/subscriptioncmds.c | 67 +++++++++++++++-------
src/test/regress/expected/subscription.out | 3 +-
2 files changed, 49 insertions(+), 21 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index ee06a726f42..137bfa1a9b0 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -54,6 +54,7 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/pg_lsn.h"
+#include "utils/resowner.h"
#include "utils/syscache.h"
/*
@@ -2264,37 +2265,63 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
* Construct conninfo from a subscription's server. Like libpqrcv_connect(),
* if an error occurs, set *err to the error message and return NULL.
*
- * However, failures in ForeignServerConnectionString() may ereport(ERROR),
- * and (also like libpqrcv_connect) it's not worth adding the machinery to
- * pass all of those back to the caller just to cover this one case.
+ * Uses a subtransaction so that we can catch arbitrary errors from
+ * ForeignServerConnectionString().
*/
static char *
construct_subserver_conninfo(Oid subserver, Oid subowner, char **err)
{
- AclResult aclresult;
- ForeignServer *server;
+ MemoryContext oldcxt = CurrentMemoryContext;
+ ResourceOwner oldresowner = CurrentResourceOwner;
+ ErrorData *edata = NULL;
+ char *conninfo = NULL;
*err = NULL;
- server = GetForeignServer(subserver);
+ BeginInternalSubTransaction(NULL);
+ MemoryContextSwitchTo(oldcxt);
- aclresult = object_aclcheck(ForeignServerRelationId, subserver,
- subowner, ACL_USAGE);
- if (aclresult != ACLCHECK_OK)
+ PG_TRY();
{
- /*
- * Unable to generate connection string because permissions on the
- * foreign server have been removed. Follow the same logic as an
- * unusable subconninfo (which will result in an ERROR later unless
- * slot_name = NONE).
- */
- *err = psprintf(_("subscription owner \"%s\" does not have permission on foreign server \"%s\""),
- GetUserNameFromId(subowner, false),
- server->servername);
- return NULL;
+ AclResult aclresult;
+ ForeignServer *server;
+
+ server = GetForeignServer(subserver);
+ aclresult = object_aclcheck(ForeignServerRelationId, subserver,
+ subowner, ACL_USAGE);
+ if (aclresult != ACLCHECK_OK)
+ ereport(ERROR,
+ errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"",
+ GetUserNameFromId(subowner, false),
+ server->servername));
+
+ conninfo = ForeignServerConnectionString(subowner, server);
+ Assert(conninfo != NULL);
+ ReleaseCurrentSubTransaction();
+ MemoryContextSwitchTo(oldcxt);
+ CurrentResourceOwner = oldresowner;
+ }
+ PG_CATCH();
+ {
+ MemoryContextSwitchTo(oldcxt);
+ edata = CopyErrorData();
+ FlushErrorState();
+ RollbackAndReleaseCurrentSubTransaction();
+ MemoryContextSwitchTo(oldcxt);
+ CurrentResourceOwner = oldresowner;
+
+ conninfo = NULL;
+ }
+ PG_END_TRY();
+
+ if (!conninfo)
+ {
+ *err = pstrdup(edata->message);
+ FreeErrorData(edata);
}
- return ForeignServerConnectionString(subowner, server);
+ return conninfo;
}
/*
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 8dbfac66326..1a4c4d6392e 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -215,7 +215,8 @@ ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist pass
ABORT;
-- fails, cannot drop slot
DROP SUBSCRIPTION regress_testsub6;
-ERROR: user mapping not found for user "regress_subscription_user3", server "test_server"
+ERROR: could not connect to publisher when attempting to drop replication slot "dummy": user mapping not found for user "regress_subscription_user3", server "test_server"
+HINT: Use ALTER SUBSCRIPTION ... DISABLE to disable the subscription, and then use ALTER SUBSCRIPTION ... SET (slot_name = NONE) to disassociate it from the slot.
ALTER SUBSCRIPTION regress_testsub6 DISABLE;
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6; --ok
--
2.43.0
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-06-19 18:40 Jeff Davis <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Jeff Davis @ 2026-06-19 18:40 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; Chao Li <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
On Wed, 2026-06-17 at 16:10 -0700, Jeff Davis wrote:
> v7-0003 is more than just an error message change, so I updated the
> commit message. It handles the case where the drop path has
> rstates!=NIL but slot_name=NONE: without 0003, the drop will fail
> while
> trying to construct the conninfo (without the HINT); with 0003, the
> drop will silently succeed without removing the tablesync slots (as
> it
> does with a connection failure when subconninfo is set).
>
> If the use of a subtransaction is fine there, then I think we should
> proceed with 0003 and whatever hint message is agreeable.
0002 committed.
For 0003, I don't think we can absorb every kind of error that might
happen inside fdwconnection. libpqrcv_connect() doesn't attempt to do
so, so neither should construct_subserver_conninfo().
What 0003 is really trying to avoid is fairly normal kinds of errors
that can happen on a non-broken FDW that get in the way of dropping a
subscription. That includes a missing user mapping or a failed ACL
check. But aside from that, it's hard to think of other examples we'd
clearly want to absorb.
Options:
* check for those two error codes explicitly
* try to be more systematic about what kinds of errors should be
absorbed or not
* add an escape hatch for users to turn off tablesync slots so that
DROP will always succeed
* consider it an unimportant edge case and leave it the way it is
(with 0001 & 0002 already done), and close the open item
Thoughts?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-06-22 21:10 Jeff Davis <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 2 replies; 28+ messages in thread
From: Jeff Davis @ 2026-06-22 21:10 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; Chao Li <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
On Fri, 2026-06-19 at 11:40 -0700, Jeff Davis wrote:
> * add an escape hatch for users to turn off tablesync slots so that
> DROP will always succeed
It looks like SLOT_NAME=NONE is already supposed to be this escape
hatch, even for tablesync slots. From the docs:
"To proceed in this situation, first disable the subscription by
executing ALTER SUBSCRIPTION ... DISABLE, and then disassociate it from
the replication slot by executing ALTER SUBSCRIPTION ... SET (slot_name
= NONE). After that, DROP SUBSCRIPTION will no longer attempt any
actions on a remote host."
https://www.postgresql.org/docs/devel/sql-dropsubscription.html
But DropSubscription() only does the early-return if there are no
tablesync slots. If there are tablesync slots, it still tries to
contact the publisher, even if SLOT_NAME=NONE.
> * consider it an unimportant edge case and leave it the way it is
> (with 0001 & 0002 already done), and close the open item
I plan to close this open item, and treat the above as a pre-existing
bug, which may require a backport.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 28+ messages in thread
* RE: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-06-24 02:18 Hayato Kuroda (Fujitsu) <[email protected]>
parent: Jeff Davis <[email protected]>
1 sibling, 1 reply; 28+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2026-06-24 02:18 UTC (permalink / raw)
To: 'Jeff Davis' <[email protected]>; Amit Kapila <[email protected]>; Chao Li <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
Dear Jeff,
I went through the 0003 patch. Apart from HINT message, I found that error code
ERRCODE_CONNECTION_FAILURE is always used. Should we report the original errcode
of the root cause? Or it's OK because the observed outcome is the cannot-connect
error?
Another comment:
```
+ conninfo = ForeignServerConnectionString(subowner, server);
+ Assert(conninfo != NULL);
```
Do we have to consider the case that ForeignServerConnectionString() returns NULL
without any ERRORs? For the production build, the Assert() would be skipped and
the segmentation fault would happen in the `if (!conninfo)` part.
I checked the doc [1] but there are no specifications for it.
[1]: https://www.postgresql.org/docs/devel/sql-createforeigndatawrapper.html#:~:text=ereport(ERROR)%20fun...
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-06-29 04:27 Amit Kapila <[email protected]>
parent: Jeff Davis <[email protected]>
1 sibling, 1 reply; 28+ messages in thread
From: Amit Kapila @ 2026-06-29 04:27 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Chao Li <[email protected]>; Zsolt Parragi <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
On Tue, Jun 23, 2026 at 2:40 AM Jeff Davis <[email protected]> wrote:
>
> On Fri, 2026-06-19 at 11:40 -0700, Jeff Davis wrote:
> > * add an escape hatch for users to turn off tablesync slots so that
> > DROP will always succeed
>
> It looks like SLOT_NAME=NONE is already supposed to be this escape
> hatch, even for tablesync slots. From the docs:
>
> "To proceed in this situation, first disable the subscription by
> executing ALTER SUBSCRIPTION ... DISABLE, and then disassociate it from
> the replication slot by executing ALTER SUBSCRIPTION ... SET (slot_name
> = NONE). After that, DROP SUBSCRIPTION will no longer attempt any
> actions on a remote host."
>
> https://www.postgresql.org/docs/devel/sql-dropsubscription.html
>
> But DropSubscription() only does the early-return if there are no
> tablesync slots. If there are tablesync slots, it still tries to
> contact the publisher, even if SLOT_NAME=NONE.
>
AFAICS, the corresponding DropSubscription() code is as follows:
if (!slotname && rstates == NIL)
{
table_close(rel, NoLock);
return;
}
....
if (OidIsValid(subserver))
conninfo = construct_subserver_conninfo(subserver, subowner, &err);
else
conninfo = subconninfo;
if (conninfo)
wrconn = walrcv_connect(conninfo, true, true, must_use_password,
subname, &err);
if (wrconn == NULL)
{
if (!slotname)
{
/* be tidy */
list_free(rstates);
table_close(rel, NoLock);
return;
}
else
{
ReportSlotConnectionError(rstates, subid, slotname, err);
}
}
So, if the connection is not successful then we simply return without
checking tablesync slots (when slotname is NONE) whereas with FDW
server connection, the ERROR will be raised from
construct_subserver_conninfo->ForeignServerConnectionString(). Isn't
that different from current situation?
> > * consider it an unimportant edge case and leave it the way it is
> > (with 0001 & 0002 already done), and close the open item
>
> I plan to close this open item, and treat the above as a pre-existing
> bug, which may require a backport.
>
As per my understanding it is better to go with your fix idea even
though we don't have better ideas for HINT message. I see your point
of making HINT much longer as we may need to add something like (...
or re-create the user mapping/ACL and retry..), so we can keep the
current one as it is and if we see any real user complaint then we can
consider improving it in the future.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-06-29 04:37 Amit Kapila <[email protected]>
parent: Hayato Kuroda (Fujitsu) <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Amit Kapila @ 2026-06-29 04:37 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: Jeff Davis <[email protected]>; Chao Li <[email protected]>; Zsolt Parragi <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
On Wed, Jun 24, 2026 at 7:48 AM Hayato Kuroda (Fujitsu)
<[email protected]> wrote:
>
> I went through the 0003 patch. Apart from HINT message, I found that error code
> ERRCODE_CONNECTION_FAILURE is always used. Should we report the original errcode
> of the root cause?
>
That could be marginally better but not sure if it is worth the
complexity. I have a few more points regarding 0003:
* Regarding the point: "we can't absorb every kind of error." In the
PG_CATCH, re-throw query-cancel / interrupt-class conditions (if those
are possible) instead of swallowing them, otherwise a SIGINT during
ForeignServerConnectionString() becomes a silent successful DROP (for
slot_name=NONE) or a mislabeled connection error.
*
+ if (aclresult != ACLCHECK_OK)
+ ereport(ERROR,
+ errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("subscription owner \"%s\" does not have permission on
foreign server \"%s\"",
+ GetUserNameFromId(subowner, false),
+ server->servername));
I am not sure if this is a good idea as the outer catch will anyway
silence this.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-07-06 23:41 Jeff Davis <[email protected]>
parent: Amit Kapila <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Jeff Davis @ 2026-07-06 23:41 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: Chao Li <[email protected]>; Zsolt Parragi <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
On Mon, 2026-06-29 at 10:07 +0530, Amit Kapila wrote:
> On Wed, Jun 24, 2026 at 7:48 AM Hayato Kuroda (Fujitsu)
> <[email protected]> wrote:
> >
> > I went through the 0003 patch. Apart from HINT message, I found
> > that error code
> > ERRCODE_CONNECTION_FAILURE is always used. Should we report the
> > original errcode
> > of the root cause?
> >
>
> That could be marginally better but not sure if it is worth the
> complexity. I have a few more points regarding 0003:
> * Regarding the point: "we can't absorb every kind of error." In the
> PG_CATCH, re-throw query-cancel / interrupt-class conditions (if
> those
> are possible) instead of swallowing them, otherwise a SIGINT during
> ForeignServerConnectionString() becomes a silent successful DROP (for
> slot_name=NONE) or a mislabeled connection error.
The DROP SUBSCRIPTION documentation says that:
"To proceed in this situation, first disable the subscription by
executing ALTER SUBSCRIPTION ... DISABLE, and then disassociate it from
the replication slot by executing ALTER SUBSCRIPTION ... SET (slot_name
= NONE). After that, DROP SUBSCRIPTION will no longer attempt any
actions on a remote host."
But that stopped being true after commit ce0fdbfe97. There should be
some kind of escape such that we can force a DROP SUBSCRIPTION to
succeed without trying to connect to the remote host. And if we have
that escape, then we don't need to overcomplicate this by using a
subtransaction and trying to guess which errors are recoverable or not.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-07-06 23:46 Jeff Davis <[email protected]>
parent: Amit Kapila <[email protected]>
0 siblings, 0 replies; 28+ messages in thread
From: Jeff Davis @ 2026-07-06 23:46 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; +Cc: Chao Li <[email protected]>; Zsolt Parragi <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
On Mon, 2026-06-29 at 09:57 +0530, Amit Kapila wrote:
> wrconn = walrcv_connect(conninfo, true, true, must_use_password,
> subname, &err);
...
> So, if the connection is not successful then we simply return without
> checking tablesync slots (when slotname is NONE) whereas with FDW
> server connection, the ERROR will be raised from
> construct_subserver_conninfo->ForeignServerConnectionString(). Isn't
> that different from current situation?
It's easier to cause an error in ForeignServerConnectionString(); but
walrcv_connect() can throw errors, too. Also, a network connection is a
side-effect that you may not want to have for whatever reason.
I think it's better to have an escape that prevents any connection to
the publisher, as the documentation says, to deal with these kinds of
edge cases.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-07-08 04:59 Amit Kapila <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Amit Kapila @ 2026-07-08 04:59 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Hayato Kuroda (Fujitsu) <[email protected]>; Chao Li <[email protected]>; Zsolt Parragi <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
On Tue, Jul 7, 2026 at 5:11 AM Jeff Davis <[email protected]> wrote:
>
> On Mon, 2026-06-29 at 10:07 +0530, Amit Kapila wrote:
> > On Wed, Jun 24, 2026 at 7:48 AM Hayato Kuroda (Fujitsu)
> > <[email protected]> wrote:
> > >
> > > I went through the 0003 patch. Apart from HINT message, I found
> > > that error code
> > > ERRCODE_CONNECTION_FAILURE is always used. Should we report the
> > > original errcode
> > > of the root cause?
> > >
> >
> > That could be marginally better but not sure if it is worth the
> > complexity. I have a few more points regarding 0003:
> > * Regarding the point: "we can't absorb every kind of error." In the
> > PG_CATCH, re-throw query-cancel / interrupt-class conditions (if
> > those
> > are possible) instead of swallowing them, otherwise a SIGINT during
> > ForeignServerConnectionString() becomes a silent successful DROP (for
> > slot_name=NONE) or a mislabeled connection error.
>
> The DROP SUBSCRIPTION documentation says that:
>
> "To proceed in this situation, first disable the subscription by
> executing ALTER SUBSCRIPTION ... DISABLE, and then disassociate it from
> the replication slot by executing ALTER SUBSCRIPTION ... SET (slot_name
> = NONE). After that, DROP SUBSCRIPTION will no longer attempt any
> actions on a remote host."
>
> But that stopped being true after commit ce0fdbfe97. There should be
> some kind of escape such that we can force a DROP SUBSCRIPTION to
> succeed without trying to connect to the remote host. And if we have
> that escape, then we don't need to overcomplicate this by using a
> subtransaction and trying to guess which errors are recoverable or not.
>
Okay, I see your point and we can make it return early when slot_name
is NONE. However, do we need to consider a case where users want to
manually manage the subscription's main slot, so it uses
slot_name=NONE? I mean users want to manually maintain the lifecycle
of the slot and due to that even during the CREATE SUBSCRIPTION, it
uses the specified slot_name instead of the default. But still
tablesync slots need to be removed in such a case as those are
internally created. Actually, this best-case try to connect to
publisher works for such cases. I think this is mostly a theoretical
scenario because ideally these tablesync slots should be removed at
the end of tablesync especially when users are doing drop subscription
as part of planned operation, so we can as well rely on the document
as we are doing now.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-07-08 19:20 Jeff Davis <[email protected]>
parent: Amit Kapila <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Jeff Davis @ 2026-07-08 19:20 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; +Cc: Hayato Kuroda (Fujitsu) <[email protected]>; Chao Li <[email protected]>; Zsolt Parragi <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
On Wed, 2026-07-08 at 10:29 +0530, Amit Kapila wrote:
> Okay, I see your point and we can make it return early when slot_name
> is NONE. However, do we need to consider a case where users want to
> manually manage the subscription's main slot, so it uses
> slot_name=NONE? I mean users want to manually maintain the lifecycle
> of the slot and due to that even during the CREATE SUBSCRIPTION, it
> uses the specified slot_name instead of the default. But still
> tablesync slots need to be removed in such a case as those are
> internally created.
The problem is that we are overloading slot_name=NONE, and as you point
out, it can mean multiple things.
Can we just have DROP SUBSCRIPTION ... FORCE or DROP SUBSCRIPTION ...
NOCONNECT? That allows the user to clearly express their intent at the
time of the drop, rather than guessing from the state of the
subscription.
If it's too late to do that for 19, we should probably just make the
code match the documentation and don't try to connect if
slot_name=NONE. In most ordinary cases that would not cause a problem,
you'd have to have slot_name=NONE and also drop the subscription before
the table syncs finish.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
@ 2026-07-09 07:17 Amit Kapila <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 0 replies; 28+ messages in thread
From: Amit Kapila @ 2026-07-09 07:17 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Hayato Kuroda (Fujitsu) <[email protected]>; Chao Li <[email protected]>; Zsolt Parragi <[email protected]>; Ajin Cherian <[email protected]>; pgsql-hackers
On Thu, Jul 9, 2026 at 12:50 AM Jeff Davis <[email protected]> wrote:
>
> On Wed, 2026-07-08 at 10:29 +0530, Amit Kapila wrote:
> > Okay, I see your point and we can make it return early when slot_name
> > is NONE. However, do we need to consider a case where users want to
> > manually manage the subscription's main slot, so it uses
> > slot_name=NONE? I mean users want to manually maintain the lifecycle
> > of the slot and due to that even during the CREATE SUBSCRIPTION, it
> > uses the specified slot_name instead of the default. But still
> > tablesync slots need to be removed in such a case as those are
> > internally created.
>
> The problem is that we are overloading slot_name=NONE, and as you point
> out, it can mean multiple things.
>
> Can we just have DROP SUBSCRIPTION ... FORCE or DROP SUBSCRIPTION ...
> NOCONNECT? That allows the user to clearly express their intent at the
> time of the drop, rather than guessing from the state of the
> subscription.
>
That sounds like an appropriate solution or we can use (connect =
false) option as well a way to indicate NOCONNECT.
> If it's too late to do that for 19,
>
I feel it is late for 19.
we should probably just make the
> code match the documentation and don't try to connect if
> slot_name=NONE. In most ordinary cases that would not cause a problem,
> you'd have to have slot_name=NONE and also drop the subscription before
> the table syncs finish.
>
Yeah, we can do this but if we want to change this behaviour in the
next version, isn't it better to tweak the doc wording as:"After that,
DROP SUBSCRIPTION will not attempt to drop the subscription's own
replication slot. It may still connect to the publisher to drop
internally-created table synchronization slots if some table
synchronization was left unfinished; if the publisher is unreachable,
those slots (and the main slot, if it still exists) must be dropped
manually.". I think in most cases where we need this is where the
publisher is not available or the network is broken and in those
cases, the current code still works and won't leave any tablesync
slots.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
end of thread, other threads:[~2026-07-09 07:17 UTC | newest]
Thread overview: 28+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-01 00:42 [PATCH v10] first cut David Fetter <[email protected]>
2026-04-22 01:51 Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Chao Li <[email protected]>
2026-04-22 12:35 ` RE: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Hayato Kuroda (Fujitsu) <[email protected]>
2026-04-23 03:37 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Chao Li <[email protected]>
2026-06-17 20:26 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Jeff Davis <[email protected]>
2026-04-29 04:44 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Ajin Cherian <[email protected]>
2026-04-30 04:11 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Chao Li <[email protected]>
2026-05-01 03:58 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Ajin Cherian <[email protected]>
2026-05-06 07:47 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Chao Li <[email protected]>
2026-05-05 20:53 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Zsolt Parragi <[email protected]>
2026-05-06 07:57 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Chao Li <[email protected]>
2026-05-09 01:01 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Jeff Davis <[email protected]>
2026-05-09 03:08 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Chao Li <[email protected]>
2026-05-14 21:45 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Jeff Davis <[email protected]>
2026-05-15 07:18 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Chao Li <[email protected]>
2026-05-18 05:28 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Chao Li <[email protected]>
2026-05-27 00:44 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Amit Kapila <[email protected]>
2026-06-17 23:10 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Jeff Davis <[email protected]>
2026-06-19 18:40 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Jeff Davis <[email protected]>
2026-06-22 21:10 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Jeff Davis <[email protected]>
2026-06-24 02:18 ` RE: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Hayato Kuroda (Fujitsu) <[email protected]>
2026-06-29 04:37 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Amit Kapila <[email protected]>
2026-07-06 23:41 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Jeff Davis <[email protected]>
2026-07-08 04:59 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Amit Kapila <[email protected]>
2026-07-08 19:20 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Jeff Davis <[email protected]>
2026-07-09 07:17 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Amit Kapila <[email protected]>
2026-06-29 04:27 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Amit Kapila <[email protected]>
2026-07-06 23:46 ` Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Jeff Davis <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox