public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2 4/7] Row pattern recognition patch (executor).
19+ messages / 4 participants
[nested] [flat]
* [PATCH v2 4/7] Row pattern recognition patch (executor).
@ 2023-06-26 08:05 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Tatsuo Ishii @ 2023-06-26 08:05 UTC (permalink / raw)
---
src/backend/executor/nodeWindowAgg.c | 225 +++++++++++++++++++-
src/backend/utils/adt/windowfuncs.c | 302 ++++++++++++++++++++++++++-
src/include/catalog/pg_proc.dat | 9 +
src/include/nodes/execnodes.h | 13 ++
src/include/windowapi.h | 9 +
5 files changed, 548 insertions(+), 10 deletions(-)
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 310ac23e3a..bef2bc62b2 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -48,6 +48,7 @@
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/datum.h"
+#include "utils/fmgroids.h"
#include "utils/expandeddatum.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -159,6 +160,14 @@ typedef struct WindowStatePerAggData
bool restart; /* need to restart this agg in this cycle? */
} WindowStatePerAggData;
+/*
+ * Map between Var attno in a target list and the parsed attno.
+ */
+typedef struct AttnoMap {
+ List *attno; /* att number in target list (list of AttNumber) */
+ List *attnosyn; /* parsed att number (list of AttNumber) */
+} AttnoMap;
+
static void initialize_windowaggregate(WindowAggState *winstate,
WindowStatePerFunc perfuncstate,
WindowStatePerAgg peraggstate);
@@ -195,9 +204,9 @@ static Datum GetAggInitVal(Datum textInitVal, Oid transtype);
static bool are_peers(WindowAggState *winstate, TupleTableSlot *slot1,
TupleTableSlot *slot2);
-static bool window_gettupleslot(WindowObject winobj, int64 pos,
- TupleTableSlot *slot);
+static void attno_map(Node *node, AttnoMap *map);
+static bool attno_map_walker(Node *node, void *context);
/*
* initialize_windowaggregate
@@ -2388,6 +2397,12 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
TupleDesc scanDesc;
ListCell *l;
+ TargetEntry *te;
+ Expr *expr;
+ Var *var;
+ int nargs;
+ AttnoMap attnomap;
+
/* check for unsupported flags */
Assert(!(eflags & (EXEC_FLAG_BACKWARD | EXEC_FLAG_MARK)));
@@ -2483,6 +2498,16 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
winstate->temp_slot_2 = ExecInitExtraTupleSlot(estate, scanDesc,
&TTSOpsMinimalTuple);
+ winstate->prev_slot = ExecInitExtraTupleSlot(estate, scanDesc,
+ &TTSOpsMinimalTuple);
+
+ winstate->next_slot = ExecInitExtraTupleSlot(estate, scanDesc,
+ &TTSOpsMinimalTuple);
+
+ winstate->null_slot = ExecInitExtraTupleSlot(estate, scanDesc,
+ &TTSOpsMinimalTuple);
+ winstate->null_slot = ExecStoreAllNullTuple(winstate->null_slot);
+
/*
* create frame head and tail slots only if needed (must create slots in
* exactly the same cases that update_frameheadpos and update_frametailpos
@@ -2667,6 +2692,71 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
winstate->inRangeAsc = node->inRangeAsc;
winstate->inRangeNullsFirst = node->inRangeNullsFirst;
+ /* Set up SKIP TO type */
+ winstate->rpSkipTo = node->rpSkipTo;
+ /* Set up row pattern recognition PATTERN clause */
+ winstate->patternVariableList = node->patternVariable;
+ winstate->patternRegexpList = node->patternRegexp;
+
+ /* Set up row pattern recognition DEFINE clause */
+
+ /*
+ * Collect mapping between varattno and varattnosyn in the targetlist.
+ * XXX: For now we only check RPR's argument. Eventually we have to
+ * recurse the targetlist to find out all mappings in Var nodes.
+ */
+ attnomap.attno = NIL;
+ attnomap.attnosyn = NIL;
+
+ foreach (l, node->plan.targetlist)
+ {
+ te = lfirst(l);
+ if (IsA(te->expr, WindowFunc))
+ {
+ WindowFunc *func = (WindowFunc *)te->expr;
+ if (func->winfnoid != F_RPR)
+ continue;
+
+ /* sanity check */
+ nargs = list_length(func->args);
+ if (list_length(func->args) != 1)
+ elog(ERROR, "RPR must have 1 argument but function %d has %d args", func->winfnoid, nargs);
+
+ expr = (Expr *) lfirst(list_head(func->args));
+ if (!IsA(expr, Var))
+ elog(ERROR, "RPR's arg is not Var");
+
+ var = (Var *)expr;
+ elog(DEBUG1, "resname: %s varattno: %d varattnosyn: %d",
+ te->resname, var->varattno, var->varattnosyn);
+ attnomap.attno = lappend_int(attnomap.attno, var->varattno);
+ attnomap.attnosyn = lappend_int(attnomap.attnosyn, var->varattnosyn);
+ }
+ }
+
+ winstate->defineVariableList = NIL;
+ winstate->defineClauseList = NIL;
+ if (node->defineClause != NIL)
+ {
+ foreach(l, node->defineClause)
+ {
+ char *name;
+ ExprState *exps;
+
+ te = lfirst(l);
+ name = te->resname;
+ expr = te->expr;
+
+ elog(DEBUG1, "defineVariable name: %s", name);
+ winstate->defineVariableList = lappend(winstate->defineVariableList,
+ makeString(pstrdup(name)));
+ /* tweak expr so that it referes to outer slot */
+ attno_map((Node *)expr, &attnomap);
+ exps = ExecInitExpr(expr, (PlanState *) winstate);
+ winstate->defineClauseList = lappend(winstate->defineClauseList, exps);
+ }
+ }
+
winstate->all_first = true;
winstate->partition_spooled = false;
winstate->more_partitions = false;
@@ -2674,6 +2764,76 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
return winstate;
}
+/*
+ * Rewrite Var node's varattno to the varattno which is used in the target
+ * list using AttnoMap. We also rewrite varno so that it sees outer tuple
+ * (PREV) or inner tuple (NEXT).
+ */
+static void
+attno_map(Node *node, AttnoMap *map)
+{
+ (void) expression_tree_walker(node, attno_map_walker, (void *) map);
+}
+
+static bool
+attno_map_walker(Node *node, void *context)
+{
+ FuncExpr *func;
+ int nargs;
+ Expr *expr;
+ Var *var;
+ AttnoMap *attnomap;
+ ListCell *lc1, *lc2;
+
+ if (node == NULL)
+ return false;
+
+ attnomap = (AttnoMap *) context;
+
+ if (IsA(node, FuncExpr))
+ {
+ func = (FuncExpr *)node;
+
+ if (func->funcid == F_PREV || func->funcid == F_NEXT)
+ {
+ /* sanity check */
+ nargs = list_length(func->args);
+ if (list_length(func->args) != 1)
+ elog(ERROR, "PREV/NEXT must have 1 argument but function %d has %d args", func->funcid, nargs);
+
+ expr = (Expr *) lfirst(list_head(func->args));
+ if (!IsA(expr, Var))
+ elog(ERROR, "PREV/NEXT's arg is not Var"); /* XXX: is it possible that arg type is Const? */
+ var = (Var *)expr;
+
+ if (func->funcid == F_PREV)
+ var->varno = OUTER_VAR;
+ else
+ var->varno = INNER_VAR;
+ }
+ return expression_tree_walker(node, attno_map_walker, (void *) context);
+ }
+ else if (IsA(node, Var))
+ {
+ var = (Var *)node;
+
+ elog(DEBUG1, "original varno: %d varattno: %d", var->varno, var->varattno);
+
+ forboth(lc1, attnomap->attno, lc2, attnomap->attnosyn)
+ {
+ int attno = lfirst_int(lc1);
+ int attnosyn = lfirst_int(lc2);
+
+ if (var->varattno == attnosyn)
+ {
+ elog(DEBUG1, "loc: %d rewrite varattno from: %d to %d", var->location, attnosyn, attno);
+ var->varattno = attno;
+ }
+ }
+ }
+ return expression_tree_walker(node, attno_map_walker, (void *) context);
+}
+
/* -----------------
* ExecEndWindowAgg
* -----------------
@@ -2691,6 +2851,8 @@ ExecEndWindowAgg(WindowAggState *node)
ExecClearTuple(node->agg_row_slot);
ExecClearTuple(node->temp_slot_1);
ExecClearTuple(node->temp_slot_2);
+ ExecClearTuple(node->prev_slot);
+ ExecClearTuple(node->next_slot);
if (node->framehead_slot)
ExecClearTuple(node->framehead_slot);
if (node->frametail_slot)
@@ -2740,6 +2902,8 @@ ExecReScanWindowAgg(WindowAggState *node)
ExecClearTuple(node->agg_row_slot);
ExecClearTuple(node->temp_slot_1);
ExecClearTuple(node->temp_slot_2);
+ ExecClearTuple(node->prev_slot);
+ ExecClearTuple(node->next_slot);
if (node->framehead_slot)
ExecClearTuple(node->framehead_slot);
if (node->frametail_slot)
@@ -3080,7 +3244,7 @@ are_peers(WindowAggState *winstate, TupleTableSlot *slot1,
*
* Returns true if successful, false if no such row
*/
-static bool
+bool
window_gettupleslot(WindowObject winobj, int64 pos, TupleTableSlot *slot)
{
WindowAggState *winstate = winobj->winstate;
@@ -3420,14 +3584,53 @@ WinGetFuncArgInFrame(WindowObject winobj, int argno,
WindowAggState *winstate;
ExprContext *econtext;
TupleTableSlot *slot;
- int64 abs_pos;
- int64 mark_pos;
Assert(WindowObjectIsValid(winobj));
winstate = winobj->winstate;
econtext = winstate->ss.ps.ps_ExprContext;
slot = winstate->temp_slot_1;
+ if (WinGetSlotInFrame(winobj, slot,
+ relpos, seektype, set_mark,
+ isnull, isout) == 0)
+ {
+ econtext->ecxt_outertuple = slot;
+ return ExecEvalExpr((ExprState *) list_nth(winobj->argstates, argno),
+ econtext, isnull);
+ }
+
+ if (isout)
+ *isout = true;
+ *isnull = true;
+ return (Datum) 0;
+}
+
+/*
+ * WinGetSlotInFrame
+ * slot: TupleTableSlot to store the result
+ * relpos: signed rowcount offset from the seek position
+ * seektype: WINDOW_SEEK_HEAD or WINDOW_SEEK_TAIL
+ * set_mark: If the row is found/in frame and set_mark is true, the mark is
+ * moved to the row as a side-effect.
+ * isnull: output argument, receives isnull status of result
+ * isout: output argument, set to indicate whether target row position
+ * is out of frame (can pass NULL if caller doesn't care about this)
+ *
+ * Returns 0 if we successfullt got the slot. false if out of frame.
+ * (also isout is set)
+ */
+int
+WinGetSlotInFrame(WindowObject winobj, TupleTableSlot *slot,
+ int relpos, int seektype, bool set_mark,
+ bool *isnull, bool *isout)
+{
+ WindowAggState *winstate;
+ int64 abs_pos;
+ int64 mark_pos;
+
+ Assert(WindowObjectIsValid(winobj));
+ winstate = winobj->winstate;
+
switch (seektype)
{
case WINDOW_SEEK_CURRENT:
@@ -3583,15 +3786,13 @@ WinGetFuncArgInFrame(WindowObject winobj, int argno,
*isout = false;
if (set_mark)
WinSetMarkPosition(winobj, mark_pos);
- econtext->ecxt_outertuple = slot;
- return ExecEvalExpr((ExprState *) list_nth(winobj->argstates, argno),
- econtext, isnull);
+ return 0;
out_of_frame:
if (isout)
*isout = true;
*isnull = true;
- return (Datum) 0;
+ return -1;
}
/*
@@ -3622,3 +3823,9 @@ WinGetFuncArgCurrent(WindowObject winobj, int argno, bool *isnull)
return ExecEvalExpr((ExprState *) list_nth(winobj->argstates, argno),
econtext, isnull);
}
+
+WindowAggState *
+WinGetAggState(WindowObject winobj)
+{
+ return winobj->winstate;
+}
diff --git a/src/backend/utils/adt/windowfuncs.c b/src/backend/utils/adt/windowfuncs.c
index b87a624fb2..74ef11ce55 100644
--- a/src/backend/utils/adt/windowfuncs.c
+++ b/src/backend/utils/adt/windowfuncs.c
@@ -13,6 +13,9 @@
*/
#include "postgres.h"
+#include "catalog/pg_collation_d.h"
+#include "executor/executor.h"
+#include "nodes/execnodes.h"
#include "nodes/supportnodes.h"
#include "utils/builtins.h"
#include "windowapi.h"
@@ -36,10 +39,21 @@ typedef struct
int64 remainder; /* (total rows) % (bucket num) */
} ntile_context;
+/*
+ * rpr process information.
+ * Used for AFTER MATCH SKIP PAST LAST ROW
+ */
+typedef struct SkipContext
+{
+ int64 pos; /* last row absolute position */
+} SkipContext;
+
static bool rank_up(WindowObject winobj);
static Datum leadlag_common(FunctionCallInfo fcinfo,
bool forward, bool withoffset, bool withdefault);
-
+static bool get_slots(WindowObject winobj, WindowAggState *winstate, int current_pos);
+static int evaluate_pattern(WindowObject winobj, WindowAggState *winstate,
+ int relpos, char *vname, char *quantifier, bool *result);
/*
* utility routine for *_rank functions.
@@ -713,3 +727,289 @@ window_nth_value(PG_FUNCTION_ARGS)
PG_RETURN_DATUM(result);
}
+
+/*
+ * rpr
+ * allow to use "Row pattern recognition: WINDOW clause" (SQL:2016 R020) in
+ * the target list.
+ * Usage: SELECT rpr(colname) OVER (..)
+ * where colname is defined in PATTERN clause.
+ */
+Datum
+window_rpr(PG_FUNCTION_ARGS)
+{
+#define MAX_PATTERNS 16 /* max variables in PATTERN clause */
+
+ WindowObject winobj = PG_WINDOW_OBJECT();
+ WindowAggState *winstate = WinGetAggState(winobj);
+ Datum result;
+ bool expression_result;
+ bool isnull;
+ int relpos;
+ int64 curr_pos, markpos;
+ ListCell *lc, *lc1;
+ SkipContext *context = NULL;
+
+ curr_pos = WinGetCurrentPosition(winobj);
+ elog(DEBUG1, "rpr is called. row: " INT64_FORMAT, curr_pos);
+
+ if (winstate->rpSkipTo == ST_PAST_LAST_ROW)
+ {
+ context = (SkipContext *) WinGetPartitionLocalMemory(winobj, sizeof(SkipContext));
+ if (curr_pos < context->pos)
+ {
+ elog(DEBUG1, "skip this row: curr_pos: " INT64_FORMAT "context->pos: " INT64_FORMAT,
+ curr_pos, context->pos);
+ PG_RETURN_NULL();
+ }
+ }
+
+ /*
+ * Evaluate PATTERN until one of expressions is not true or out of frame.
+ */
+ relpos = 0;
+
+ forboth(lc, winstate->patternVariableList, lc1, winstate->patternRegexpList)
+ {
+ char *vname = strVal(lfirst(lc));
+ char *quantifier = strVal(lfirst(lc1));
+
+ elog(DEBUG1, "relpos: %d pattern vname: %s quantifier: %s", relpos, vname, quantifier);
+
+ /* evaluate row pattern against current row */
+ relpos = evaluate_pattern(winobj, winstate, relpos, vname, quantifier, &expression_result);
+
+ /*
+ * If the expression did not match, we are done.
+ */
+ if (!expression_result)
+ break;
+
+ /* out of frame? */
+ if (relpos < 0)
+ break;
+
+ /* count up relative row position */
+ relpos++;
+ }
+
+ elog(DEBUG1, "relpos: %d", relpos);
+
+ /*
+ * If current row satified the pattern, return argument expression.
+ */
+ if (expression_result)
+ {
+ result = WinGetFuncArgInFrame(winobj, 0,
+ 0, WINDOW_SEEK_HEAD, false,
+ &isnull, NULL);
+ }
+
+ /*
+ * At this point we can set mark down to current pos -2.
+ */
+ markpos = curr_pos -2;
+ elog(DEBUG1, "markpos: " INT64_FORMAT, markpos);
+ if (markpos >= 0)
+ WinSetMarkPosition(winobj, markpos);
+
+ if (expression_result)
+ {
+ if (winstate->rpSkipTo == ST_PAST_LAST_ROW)
+ {
+ context->pos += relpos;
+ elog(DEBUG1, "context->pos: " INT64_FORMAT, context->pos);
+ }
+ PG_RETURN_DATUM(result);
+ }
+
+ PG_RETURN_NULL();
+}
+
+/*
+ * Evaluate expression associated with PATTERN variable vname.
+ * relpos is relative row position in a frame (starting from 0).
+ * "quantifier" is the quatifier part of the PATTERN regular expression.
+ * Currently only '+' is allowed.
+ * result is out paramater representing the expression evaluation result
+ * is true of false.
+ * Return values are:
+ * >=0: the last match relative row position
+ * -1: current row is out of frame
+ */
+static
+int evaluate_pattern(WindowObject winobj, WindowAggState *winstate,
+ int relpos, char *vname, char *quantifier, bool *result)
+{
+ ExprContext *econtext = winstate->ss.ps.ps_ExprContext;
+ ListCell *lc1, *lc2;
+ ExprState *pat;
+ Datum eval_result;
+ int sts;
+ bool out_of_frame = false;
+ bool isnull;
+ StringInfo encoded_str = makeStringInfo();
+ char pattern_str[128];
+
+ forboth (lc1, winstate->defineVariableList, lc2, winstate->defineClauseList)
+ {
+ char *name = strVal(lfirst(lc1));
+ bool second_try_match = false;
+
+ if (strcmp(vname, name))
+ continue;
+
+ /* set expression to evaluate */
+ pat = lfirst(lc2);
+
+ for (;;)
+ {
+ if (!get_slots(winobj, winstate, relpos))
+ {
+ out_of_frame = true;
+ break; /* current row is out of frame */
+ }
+
+ /* evaluate the expression */
+ eval_result = ExecEvalExpr(pat, econtext, &isnull);
+ if (isnull)
+ {
+ /* expression is NULL */
+ elog(DEBUG1, "expression for %s is NULL at row: %d", vname, relpos);
+ break;
+ }
+ else
+ {
+ if (!DatumGetBool(eval_result))
+ {
+ /* expression is false */
+ elog(DEBUG1, "expression for %s is false at row: %d", vname, relpos);
+ break;
+ }
+ else
+ {
+ /* expression is true */
+ elog(DEBUG1, "expression for %s is true at row: %d", vname, relpos);
+ appendStringInfoChar(encoded_str, vname[0]);
+
+ /* If quantifier is "+", we need to look for more matching row */
+ if (quantifier && !strcmp(quantifier, "+"))
+ {
+ /* remember that we want to try another row */
+ second_try_match = true;
+ relpos++;
+ }
+ else
+ break;
+ }
+ }
+ }
+ if (second_try_match)
+ relpos--;
+
+ if (out_of_frame)
+ {
+ *result = false;
+ return -1;
+ }
+
+ /* build regular expression */
+ snprintf(pattern_str, sizeof(pattern_str), "%c%s", vname[0], quantifier);
+
+ /*
+ * Do regular expression matching against sequence of rows satisfying
+ * the expression using regexp_instr().
+ */
+ sts = DatumGetInt32(DirectFunctionCall2Coll(regexp_instr, DEFAULT_COLLATION_OID,
+ PointerGetDatum(cstring_to_text(encoded_str->data)),
+ PointerGetDatum(cstring_to_text(pattern_str))));
+ elog(DEBUG1, "regexp_instr returned: %d. str: %s regexp: %s",
+ sts, encoded_str->data, pattern_str);
+ *result = (sts > 0)? true : false;
+ }
+ return relpos;
+}
+
+/*
+ * Get current, previous and next tuple.
+ * Returns true if still within frame.
+ */
+static bool
+get_slots(WindowObject winobj, WindowAggState *winstate, int current_pos)
+{
+ TupleTableSlot *slot;
+ bool isnull, isout;
+ int sts;
+ ExprContext *econtext;
+
+ econtext = winstate->ss.ps.ps_ExprContext;
+
+ /* for current row */
+ slot = winstate->temp_slot_1;
+ sts = WinGetSlotInFrame(winobj, slot,
+ current_pos, WINDOW_SEEK_HEAD, false,
+ &isnull, &isout);
+ if (sts < 0)
+ {
+ elog(DEBUG1, "current row is out of frame");
+ econtext->ecxt_scantuple = winstate->null_slot;
+ return false;
+ }
+ else
+ econtext->ecxt_scantuple = slot;
+
+ /* for PREV */
+ if (current_pos > 0)
+ {
+ slot = winstate->prev_slot;
+ sts = WinGetSlotInFrame(winobj, slot,
+ current_pos - 1, WINDOW_SEEK_HEAD, false,
+ &isnull, &isout);
+ if (sts < 0)
+ {
+ elog(DEBUG1, "previous row out of frame at: %d", current_pos);
+ econtext->ecxt_outertuple = winstate->null_slot;
+ }
+ econtext->ecxt_outertuple = slot;
+ }
+ else
+ econtext->ecxt_outertuple = winstate->null_slot;
+
+ /* for NEXT */
+ slot = winstate->next_slot;
+ sts = WinGetSlotInFrame(winobj, slot,
+ current_pos + 1, WINDOW_SEEK_HEAD, false,
+ &isnull, &isout);
+ if (sts < 0)
+ {
+ elog(DEBUG1, "next row out of frame at: %d", current_pos);
+ econtext->ecxt_innertuple = winstate->null_slot;
+ }
+ else
+ econtext->ecxt_innertuple = slot;
+
+ return true;
+}
+
+/*
+ * prev
+ * Dummy function to invoke RPR's navigation operator "PREV".
+ * This is *not* a window function.
+ */
+Datum
+window_prev(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_DATUM(PG_GETARG_DATUM(0));
+}
+
+/*
+ * next
+ * Dummy function to invoke RPR's navigation operation "NEXT".
+ * This is *not* a window function.
+ */
+Datum
+window_next(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_DATUM(PG_GETARG_DATUM(0));
+}
+
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 6996073989..e3a9e0ffeb 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10397,6 +10397,15 @@
{ oid => '3114', descr => 'fetch the Nth row value',
proname => 'nth_value', prokind => 'w', prorettype => 'anyelement',
proargtypes => 'anyelement int4', prosrc => 'window_nth_value' },
+{ oid => '6122', descr => 'row pattern recognition in window',
+ proname => 'rpr', prokind => 'w', prorettype => 'anyelement',
+ proargtypes => 'anyelement', prosrc => 'window_rpr' },
+{ oid => '6123', descr => 'previous value',
+ proname => 'prev', provolatile => 's', prorettype => 'anyelement',
+ proargtypes => 'anyelement', prosrc => 'window_prev' },
+{ oid => '6124', descr => 'next value',
+ proname => 'next', provolatile => 's', prorettype => 'anyelement',
+ proargtypes => 'anyelement', prosrc => 'window_next' },
# functions for range types
{ oid => '3832', descr => 'I/O',
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index cb714f4a19..1643eaa6f1 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2519,6 +2519,14 @@ typedef struct WindowAggState
int64 groupheadpos; /* current row's peer group head position */
int64 grouptailpos; /* " " " " tail position (group end+1) */
+ /* these fields are used in Row pattern recognition: */
+ RPSkipTo rpSkipTo; /* Row Pattern Skip To type */
+ List *patternVariableList; /* list of row pattern variables names (list of String) */
+ List *patternRegexpList; /* list of row pattern regular expressions ('+' or ''. list of String) */
+ List *defineVariableList; /* list of row pattern definition variables (list of String) */
+ List *defineClauseList; /* expression for row pattern definition
+ * search conditions ExprState list */
+
MemoryContext partcontext; /* context for partition-lifespan data */
MemoryContext aggcontext; /* shared context for aggregate working data */
MemoryContext curaggcontext; /* current aggregate's working data */
@@ -2555,6 +2563,11 @@ typedef struct WindowAggState
TupleTableSlot *agg_row_slot;
TupleTableSlot *temp_slot_1;
TupleTableSlot *temp_slot_2;
+
+ /* temporary slots for RPR */
+ TupleTableSlot *prev_slot; /* PREV row navigation operator */
+ TupleTableSlot *next_slot; /* NEXT row navigation operator */
+ TupleTableSlot *null_slot; /* all NULL slot */
} WindowAggState;
/* ----------------
diff --git a/src/include/windowapi.h b/src/include/windowapi.h
index b8c2c565d1..a0facf38fe 100644
--- a/src/include/windowapi.h
+++ b/src/include/windowapi.h
@@ -58,7 +58,16 @@ extern Datum WinGetFuncArgInFrame(WindowObject winobj, int argno,
int relpos, int seektype, bool set_mark,
bool *isnull, bool *isout);
+extern int WinGetSlotInFrame(WindowObject winobj, TupleTableSlot *slot,
+ int relpos, int seektype, bool set_mark,
+ bool *isnull, bool *isout);
+
extern Datum WinGetFuncArgCurrent(WindowObject winobj, int argno,
bool *isnull);
+extern WindowAggState *WinGetAggState(WindowObject winobj);
+
+extern bool window_gettupleslot(WindowObject winobj, int64 pos, TupleTableSlot *slot);
+
+
#endif /* WINDOWAPI_H */
--
2.25.1
----Next_Part(Mon_Jun_26_17_45_07_2023_724)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v2-0005-Row-pattern-recognition-patch-docs.patch"
^ permalink raw reply [nested|flat] 19+ messages in thread
* pg_sequence_last_value() for unlogged sequences on standbys
@ 2024-05-01 00:57 Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Bossart @ 2024-05-01 00:57 UTC (permalink / raw)
To: pgsql-hackers
If you create an unlogged sequence on a primary, pg_sequence_last_value()
for that sequence on a standby will error like so:
postgres=# select pg_sequence_last_value('test'::regclass);
ERROR: could not open file "base/5/16388": No such file or directory
This function is used by the pg_sequences system view, which fails with the
same error on standbys. The two options I see are:
* Return a better ERROR and adjust pg_sequences to avoid calling this
function for unlogged sequences on standbys.
* Return NULL from pg_sequence_last_value() if called for an unlogged
sequence on a standby.
As pointed out a few years ago [0], this function is undocumented, so
there's no stated contract to uphold. I lean towards just returning NULL
because that's what we'll have to put in the relevant pg_sequences field
anyway, but I can see an argument for fixing the ERROR to align with what
you see when you try to access unlogged relations on a standby (i.e.,
"cannot access temporary or unlogged relations during recovery").
Thoughts?
[0] https://postgr.es/m/CAAaqYe8JL8Et2DoO0RRjGaMvy7-C6eDH-2wHXK-gp3dOssvBkQ%40mail.gmail.com
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
@ 2024-05-01 01:06 ` Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Tom Lane @ 2024-05-01 01:06 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers
Nathan Bossart <[email protected]> writes:
> If you create an unlogged sequence on a primary, pg_sequence_last_value()
> for that sequence on a standby will error like so:
> postgres=# select pg_sequence_last_value('test'::regclass);
> ERROR: could not open file "base/5/16388": No such file or directory
> As pointed out a few years ago [0], this function is undocumented, so
> there's no stated contract to uphold. I lean towards just returning NULL
> because that's what we'll have to put in the relevant pg_sequences field
> anyway, but I can see an argument for fixing the ERROR to align with what
> you see when you try to access unlogged relations on a standby (i.e.,
> "cannot access temporary or unlogged relations during recovery").
Yeah, I agree with putting that logic into the function. Putting
such conditions into the SQL of a system view is risky because it
is really, really painful to adjust the SQL in a released version.
You could back-patch a fix for this if done at the C level, but
I doubt we'd go to the trouble if it's done in the view.
regards, tom lane
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
@ 2024-05-01 01:13 ` Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Bossart @ 2024-05-01 01:13 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: pgsql-hackers
On Tue, Apr 30, 2024 at 09:06:04PM -0400, Tom Lane wrote:
> Nathan Bossart <[email protected]> writes:
>> If you create an unlogged sequence on a primary, pg_sequence_last_value()
>> for that sequence on a standby will error like so:
>> postgres=# select pg_sequence_last_value('test'::regclass);
>> ERROR: could not open file "base/5/16388": No such file or directory
>
>> As pointed out a few years ago [0], this function is undocumented, so
>> there's no stated contract to uphold. I lean towards just returning NULL
>> because that's what we'll have to put in the relevant pg_sequences field
>> anyway, but I can see an argument for fixing the ERROR to align with what
>> you see when you try to access unlogged relations on a standby (i.e.,
>> "cannot access temporary or unlogged relations during recovery").
>
> Yeah, I agree with putting that logic into the function. Putting
> such conditions into the SQL of a system view is risky because it
> is really, really painful to adjust the SQL in a released version.
> You could back-patch a fix for this if done at the C level, but
> I doubt we'd go to the trouble if it's done in the view.
Good point. I'll work on a patch along these lines, then.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
@ 2024-05-01 02:05 ` Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Bossart @ 2024-05-01 02:05 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: pgsql-hackers
On Tue, Apr 30, 2024 at 08:13:17PM -0500, Nathan Bossart wrote:
> Good point. I'll work on a patch along these lines, then.
This ended up being easier than I expected. While unlogged sequences are
only supported on v15 and above, temporary sequences have been around since
v7.2, so this will probably need to be back-patched to all supported
versions. The added test case won't work for v12-v14 since it uses an
unlogged sequence. I'm not sure it's worth constructing a test case for
temporary sequences.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v1-0001-Fix-pg_sequence_last_value-for-non-permanent-sequ.patch (2.5K, ../../20240501020531.GA721953@nathanxps13/2-v1-0001-Fix-pg_sequence_last_value-for-non-permanent-sequ.patch)
download | inline diff:
From 71008f13da88f41a205e0643129162df9d2ebc81 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 30 Apr 2024 20:54:51 -0500
Subject: [PATCH v1 1/1] Fix pg_sequence_last_value() for non-permanent
sequences on standbys.
---
src/backend/commands/sequence.c | 18 +++++++++++++-----
src/test/recovery/t/001_stream_rep.pl | 8 ++++++++
2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 46103561c3..659d2ad4fc 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1780,7 +1780,7 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Buffer buf;
HeapTupleData seqtuple;
Form_pg_sequence_data seq;
- bool is_called;
+ bool is_called = false;
int64 result;
/* open and lock sequence */
@@ -1792,12 +1792,20 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * For the benefit of the pg_sequences system view, we return NULL for
+ * temporary and unlogged sequences on standbys instead of throwing an
+ * error.
+ */
+ if (RelationIsPermanent(seqrel) || !RecoveryInProgress())
+ {
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- is_called = seq->is_called;
- result = seq->last_value;
+ is_called = seq->is_called;
+ result = seq->last_value;
- UnlockReleaseBuffer(buf);
+ UnlockReleaseBuffer(buf);
+ }
sequence_close(seqrel, NoLock);
if (is_called)
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 5311ade509..4c698b5ce1 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -95,6 +95,14 @@ $result = $node_standby_2->safe_psql('postgres', "SELECT * FROM seq1");
print "standby 2: $result\n";
is($result, qq(33|0|t), 'check streamed sequence content on standby 2');
+# Check pg_sequence_last_value() returns NULL for unlogged sequence on standby
+$node_primary->safe_psql('postgres',
+ "CREATE UNLOGGED SEQUENCE ulseq; SELECT nextval('ulseq')");
+$node_primary->wait_for_replay_catchup($node_standby_1);
+is($node_standby_1->safe_psql('postgres',
+ "SELECT pg_sequence_last_value('ulseq'::regclass) IS NULL"),
+ 't', 'pg_sequence_last_value() on unlogged sequence on standby 1');
+
# Check that only READ-only queries can run on standbys
is($node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
3, 'read-only queries on standby 1');
--
2.25.1
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
@ 2024-05-01 03:39 ` Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Michael Paquier @ 2024-05-01 03:39 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
On Tue, Apr 30, 2024 at 09:05:31PM -0500, Nathan Bossart wrote:
> This ended up being easier than I expected. While unlogged sequences are
> only supported on v15 and above, temporary sequences have been around since
> v7.2, so this will probably need to be back-patched to all supported
> versions.
Unlogged and temporary relations cannot be accessed during recovery,
so I'm OK with your change to force a NULL for both relpersistences.
However, it seems to me that you should also drop the
pg_is_other_temp_schema() in system_views.sql for the definition of
pg_sequences. Doing that on HEAD now would be OK, but there's nothing
urgent to it so it may be better done once v18 opens up. Note that
pg_is_other_temp_schema() is only used for this sequence view, which
is a nice cleanup.
By the way, shouldn't we also change the function to return NULL for a
failed permission check? It would be possible to remove the
has_sequence_privilege() as well, thanks to that, and a duplication
between the code and the function view. I've been looking around a
bit, noticing one use of this function in check_pgactivity (nagios
agent), and its query also has a has_sequence_privilege() so returning
NULL would simplify its definition in the long-run. I'd suspect other
monitoring queries to do something similar to bypass permission
errors.
> The added test case won't work for v12-v14 since it uses an
> unlogged sequence.
That would require a BackgroundPsql to maintain the connection to the
primary, so not having a test is OK by me.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
@ 2024-05-03 20:49 ` Nathan Bossart <[email protected]>
2024-05-03 21:22 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-04 09:47 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
0 siblings, 2 replies; 19+ messages in thread
From: Nathan Bossart @ 2024-05-03 20:49 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
On Wed, May 01, 2024 at 12:39:53PM +0900, Michael Paquier wrote:
> However, it seems to me that you should also drop the
> pg_is_other_temp_schema() in system_views.sql for the definition of
> pg_sequences. Doing that on HEAD now would be OK, but there's nothing
> urgent to it so it may be better done once v18 opens up. Note that
> pg_is_other_temp_schema() is only used for this sequence view, which
> is a nice cleanup.
IIUC this would cause other sessions' temporary sequences to appear in the
view. Is that desirable?
> By the way, shouldn't we also change the function to return NULL for a
> failed permission check? It would be possible to remove the
> has_sequence_privilege() as well, thanks to that, and a duplication
> between the code and the function view. I've been looking around a
> bit, noticing one use of this function in check_pgactivity (nagios
> agent), and its query also has a has_sequence_privilege() so returning
> NULL would simplify its definition in the long-run. I'd suspect other
> monitoring queries to do something similar to bypass permission
> errors.
I'm okay with that, but it would be v18 material that I'd track separately
from the back-patchable fix proposed in this thread.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
@ 2024-05-03 21:22 ` Tom Lane <[email protected]>
2024-05-04 09:45 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
1 sibling, 1 reply; 19+ messages in thread
From: Tom Lane @ 2024-05-03 21:22 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers
Nathan Bossart <[email protected]> writes:
> On Wed, May 01, 2024 at 12:39:53PM +0900, Michael Paquier wrote:
>> However, it seems to me that you should also drop the
>> pg_is_other_temp_schema() in system_views.sql for the definition of
>> pg_sequences. Doing that on HEAD now would be OK, but there's nothing
>> urgent to it so it may be better done once v18 opens up. Note that
>> pg_is_other_temp_schema() is only used for this sequence view, which
>> is a nice cleanup.
> IIUC this would cause other sessions' temporary sequences to appear in the
> view. Is that desirable?
I assume Michael meant to move the test into the C code, not drop
it entirely --- I agree we don't want that.
Moving it has some attraction, but pg_is_other_temp_schema() is also
used in a lot of information_schema views, so we couldn't get rid of
it without a lot of further hacking. Not sure we want to relocate
that filter responsibility in just one view.
regards, tom lane
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-03 21:22 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
@ 2024-05-04 09:45 ` Michael Paquier <[email protected]>
2024-05-07 17:10 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Michael Paquier @ 2024-05-04 09:45 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; pgsql-hackers
On Fri, May 03, 2024 at 05:22:06PM -0400, Tom Lane wrote:
> Nathan Bossart <[email protected]> writes:
>> IIUC this would cause other sessions' temporary sequences to appear in the
>> view. Is that desirable?
>
> I assume Michael meant to move the test into the C code, not drop
> it entirely --- I agree we don't want that.
Yup. I meant to remove it from the script and keep only something in
the C code to avoid the duplication, but you're right that the temp
sequences would create more noise than now.
> Moving it has some attraction, but pg_is_other_temp_schema() is also
> used in a lot of information_schema views, so we couldn't get rid of
> it without a lot of further hacking. Not sure we want to relocate
> that filter responsibility in just one view.
Okay.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-03 21:22 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-04 09:45 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
@ 2024-05-07 17:10 ` Nathan Bossart <[email protected]>
2024-05-07 17:44 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Bossart @ 2024-05-07 17:10 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
On Sat, May 04, 2024 at 06:45:32PM +0900, Michael Paquier wrote:
> On Fri, May 03, 2024 at 05:22:06PM -0400, Tom Lane wrote:
>> Nathan Bossart <[email protected]> writes:
>>> IIUC this would cause other sessions' temporary sequences to appear in the
>>> view. Is that desirable?
>>
>> I assume Michael meant to move the test into the C code, not drop
>> it entirely --- I agree we don't want that.
>
> Yup. I meant to remove it from the script and keep only something in
> the C code to avoid the duplication, but you're right that the temp
> sequences would create more noise than now.
>
>> Moving it has some attraction, but pg_is_other_temp_schema() is also
>> used in a lot of information_schema views, so we couldn't get rid of
>> it without a lot of further hacking. Not sure we want to relocate
>> that filter responsibility in just one view.
>
> Okay.
Okay, so are we okay to back-patch something like v1? Or should we also
return NULL for other sessions' temporary schemas on primaries? That would
change the condition to something like
char relpersist = seqrel->rd_rel->relpersistence;
if (relpersist == RELPERSISTENCE_PERMANENT ||
(relpersist == RELPERSISTENCE_UNLOGGED && !RecoveryInProgress()) ||
!RELATION_IS_OTHER_TEMP(seqrel))
{
...
}
I personally think that would be fine to back-patch since pg_sequences
already filters it out anyway.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-03 21:22 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-04 09:45 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-07 17:10 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
@ 2024-05-07 17:44 ` Tom Lane <[email protected]>
2024-05-07 18:40 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Tom Lane @ 2024-05-07 17:44 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers
Nathan Bossart <[email protected]> writes:
> Okay, so are we okay to back-patch something like v1? Or should we also
> return NULL for other sessions' temporary schemas on primaries? That would
> change the condition to something like
> char relpersist = seqrel->rd_rel->relpersistence;
> if (relpersist == RELPERSISTENCE_PERMANENT ||
> (relpersist == RELPERSISTENCE_UNLOGGED && !RecoveryInProgress()) ||
> !RELATION_IS_OTHER_TEMP(seqrel))
> {
> ...
> }
Should be AND'ing not OR'ing the !TEMP condition, no? Also I liked
your other formulation of the persistence check better.
> I personally think that would be fine to back-patch since pg_sequences
> already filters it out anyway.
+1 to include that, as it offers a defense if someone invokes this
function directly. In HEAD we could then rip out the test in the
view.
BTW, I think you also need something like
- int64 result;
+ int64 result = 0;
Your compiler may not complain about result being possibly
uninitialized, but IME others will.
regards, tom lane
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-03 21:22 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-04 09:45 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-07 17:10 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 17:44 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
@ 2024-05-07 18:40 ` Nathan Bossart <[email protected]>
2024-05-07 19:02 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Bossart @ 2024-05-07 18:40 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers
On Tue, May 07, 2024 at 01:44:16PM -0400, Tom Lane wrote:
> Nathan Bossart <[email protected]> writes:
>> char relpersist = seqrel->rd_rel->relpersistence;
>
>> if (relpersist == RELPERSISTENCE_PERMANENT ||
>> (relpersist == RELPERSISTENCE_UNLOGGED && !RecoveryInProgress()) ||
>> !RELATION_IS_OTHER_TEMP(seqrel))
>> {
>> ...
>> }
>
> Should be AND'ing not OR'ing the !TEMP condition, no? Also I liked
> your other formulation of the persistence check better.
Yes, that's a silly mistake on my part. I changed it to
if ((RelationIsPermanent(seqrel) || !RecoveryInProgress()) &&
!RELATION_IS_OTHER_TEMP(seqrel))
{
...
}
in the attached v2.
>> I personally think that would be fine to back-patch since pg_sequences
>> already filters it out anyway.
>
> +1 to include that, as it offers a defense if someone invokes this
> function directly. In HEAD we could then rip out the test in the
> view.
I apologize for belaboring this point, but I don't see how we would be
comfortable removing that check unless we are okay with other sessions'
temporary sequences appearing in the view, albeit with a NULL last_value.
This check lives in the WHERE clause today, so if we remove it, we'd no
longer exclude those sequences. Michael and you seem united on this, so I
have a sinking feeling that I'm missing something terribly obvious.
> BTW, I think you also need something like
>
> - int64 result;
> + int64 result = 0;
>
> Your compiler may not complain about result being possibly
> uninitialized, but IME others will.
Good call.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v2-0001-Fix-pg_sequence_last_value-for-non-permanent-sequ.patch (2.7K, ../../20240507184051.GA2600328@nathanxps13/2-v2-0001-Fix-pg_sequence_last_value-for-non-permanent-sequ.patch)
download | inline diff:
From 974f56896add92983b664c11fd25010ef29ac42c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 30 Apr 2024 20:54:51 -0500
Subject: [PATCH v2 1/1] Fix pg_sequence_last_value() for non-permanent
sequences on standbys.
---
src/backend/commands/sequence.c | 22 ++++++++++++++++------
src/test/recovery/t/001_stream_rep.pl | 8 ++++++++
2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 46103561c3..9d7468d7bb 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1780,8 +1780,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Buffer buf;
HeapTupleData seqtuple;
Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1792,12 +1792,22 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * For the benefit of the pg_sequences system view, we return NULL for
+ * temporary and unlogged sequences on standbys instead of throwing an
+ * error. We also always return NULL for other sessions' temporary
+ * sequences.
+ */
+ if ((RelationIsPermanent(seqrel) || !RecoveryInProgress()) &&
+ !RELATION_IS_OTHER_TEMP(seqrel))
+ {
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- is_called = seq->is_called;
- result = seq->last_value;
+ is_called = seq->is_called;
+ result = seq->last_value;
- UnlockReleaseBuffer(buf);
+ UnlockReleaseBuffer(buf);
+ }
sequence_close(seqrel, NoLock);
if (is_called)
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 5311ade509..4c698b5ce1 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -95,6 +95,14 @@ $result = $node_standby_2->safe_psql('postgres', "SELECT * FROM seq1");
print "standby 2: $result\n";
is($result, qq(33|0|t), 'check streamed sequence content on standby 2');
+# Check pg_sequence_last_value() returns NULL for unlogged sequence on standby
+$node_primary->safe_psql('postgres',
+ "CREATE UNLOGGED SEQUENCE ulseq; SELECT nextval('ulseq')");
+$node_primary->wait_for_replay_catchup($node_standby_1);
+is($node_standby_1->safe_psql('postgres',
+ "SELECT pg_sequence_last_value('ulseq'::regclass) IS NULL"),
+ 't', 'pg_sequence_last_value() on unlogged sequence on standby 1');
+
# Check that only READ-only queries can run on standbys
is($node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
3, 'read-only queries on standby 1');
--
2.25.1
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-03 21:22 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-04 09:45 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-07 17:10 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 17:44 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-07 18:40 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
@ 2024-05-07 19:02 ` Tom Lane <[email protected]>
2024-05-07 19:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Tom Lane @ 2024-05-07 19:02 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers
Nathan Bossart <[email protected]> writes:
> On Tue, May 07, 2024 at 01:44:16PM -0400, Tom Lane wrote:
>> +1 to include that, as it offers a defense if someone invokes this
>> function directly. In HEAD we could then rip out the test in the
>> view.
> I apologize for belaboring this point, but I don't see how we would be
> comfortable removing that check unless we are okay with other sessions'
> temporary sequences appearing in the view, albeit with a NULL last_value.
Oh! You're right, I'm wrong. I was looking at the CASE filter, which
we could get rid of -- but the "WHERE NOT pg_is_other_temp_schema(N.oid)"
part has to stay.
regards, tom lane
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-03 21:22 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-04 09:45 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-07 17:10 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 17:44 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-07 18:40 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 19:02 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
@ 2024-05-07 19:39 ` Nathan Bossart <[email protected]>
2024-05-08 02:01 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Bossart @ 2024-05-07 19:39 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers
On Tue, May 07, 2024 at 03:02:01PM -0400, Tom Lane wrote:
> Nathan Bossart <[email protected]> writes:
>> On Tue, May 07, 2024 at 01:44:16PM -0400, Tom Lane wrote:
>>> +1 to include that, as it offers a defense if someone invokes this
>>> function directly. In HEAD we could then rip out the test in the
>>> view.
>
>> I apologize for belaboring this point, but I don't see how we would be
>> comfortable removing that check unless we are okay with other sessions'
>> temporary sequences appearing in the view, albeit with a NULL last_value.
>
> Oh! You're right, I'm wrong. I was looking at the CASE filter, which
> we could get rid of -- but the "WHERE NOT pg_is_other_temp_schema(N.oid)"
> part has to stay.
Okay, phew. We can still do something like v3-0002 for v18. I'll give
Michael a chance to comment on 0001 before committing/back-patching that
one.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v3-0001-Fix-pg_sequence_last_value-for-non-permanent-sequ.patch (2.7K, ../../20240507193942.GB2600328@nathanxps13/2-v3-0001-Fix-pg_sequence_last_value-for-non-permanent-sequ.patch)
download | inline diff:
From 2a37834699587eef18b50bf8d58723790bbcdde7 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 30 Apr 2024 20:54:51 -0500
Subject: [PATCH v3 1/2] Fix pg_sequence_last_value() for non-permanent
sequences on standbys.
---
src/backend/commands/sequence.c | 22 ++++++++++++++++------
src/test/recovery/t/001_stream_rep.pl | 8 ++++++++
2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 46103561c3..9d7468d7bb 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1780,8 +1780,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Buffer buf;
HeapTupleData seqtuple;
Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1792,12 +1792,22 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * For the benefit of the pg_sequences system view, we return NULL for
+ * temporary and unlogged sequences on standbys instead of throwing an
+ * error. We also always return NULL for other sessions' temporary
+ * sequences.
+ */
+ if ((RelationIsPermanent(seqrel) || !RecoveryInProgress()) &&
+ !RELATION_IS_OTHER_TEMP(seqrel))
+ {
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- is_called = seq->is_called;
- result = seq->last_value;
+ is_called = seq->is_called;
+ result = seq->last_value;
- UnlockReleaseBuffer(buf);
+ UnlockReleaseBuffer(buf);
+ }
sequence_close(seqrel, NoLock);
if (is_called)
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 5311ade509..4c698b5ce1 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -95,6 +95,14 @@ $result = $node_standby_2->safe_psql('postgres', "SELECT * FROM seq1");
print "standby 2: $result\n";
is($result, qq(33|0|t), 'check streamed sequence content on standby 2');
+# Check pg_sequence_last_value() returns NULL for unlogged sequence on standby
+$node_primary->safe_psql('postgres',
+ "CREATE UNLOGGED SEQUENCE ulseq; SELECT nextval('ulseq')");
+$node_primary->wait_for_replay_catchup($node_standby_1);
+is($node_standby_1->safe_psql('postgres',
+ "SELECT pg_sequence_last_value('ulseq'::regclass) IS NULL"),
+ 't', 'pg_sequence_last_value() on unlogged sequence on standby 1');
+
# Check that only READ-only queries can run on standbys
is($node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
3, 'read-only queries on standby 1');
--
2.25.1
[text/x-diff] v3-0002-Simplify-pg_sequences-a-bit.patch (3.2K, ../../20240507193942.GB2600328@nathanxps13/3-v3-0002-Simplify-pg_sequences-a-bit.patch)
download | inline diff:
From b96d1f21f6144640561360c84b361f569a2edc48 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 7 May 2024 14:35:34 -0500
Subject: [PATCH v3 2/2] Simplify pg_sequences a bit.
XXX: NEEDS CATVERSION BUMP
---
src/backend/catalog/system_views.sql | 6 +-----
src/backend/commands/sequence.c | 15 +++++----------
src/test/regress/expected/rules.out | 5 +----
3 files changed, 7 insertions(+), 19 deletions(-)
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 53047cab5f..b32e5c3170 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -176,11 +176,7 @@ CREATE VIEW pg_sequences AS
S.seqincrement AS increment_by,
S.seqcycle AS cycle,
S.seqcache AS cache_size,
- CASE
- WHEN has_sequence_privilege(C.oid, 'SELECT,USAGE'::text)
- THEN pg_sequence_last_value(C.oid)
- ELSE NULL
- END AS last_value
+ pg_sequence_last_value(C.oid) AS last_value
FROM pg_sequence S JOIN pg_class C ON (C.oid = S.seqrelid)
LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
WHERE NOT pg_is_other_temp_schema(N.oid)
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 9d7468d7bb..f129375915 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1786,19 +1786,14 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
- if (pg_class_aclcheck(relid, GetUserId(), ACL_SELECT | ACL_USAGE) != ACLCHECK_OK)
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied for sequence %s",
- RelationGetRelationName(seqrel))));
-
/*
* For the benefit of the pg_sequences system view, we return NULL for
- * temporary and unlogged sequences on standbys instead of throwing an
- * error. We also always return NULL for other sessions' temporary
- * sequences.
+ * temporary and unlogged sequences on standbys as well as for sequences
+ * for which we lack USAGE or SELECT privileges. We also always return
+ * NULL for other sessions' temporary sequences.
*/
- if ((RelationIsPermanent(seqrel) || !RecoveryInProgress()) &&
+ if (pg_class_aclcheck(relid, GetUserId(), ACL_SELECT | ACL_USAGE) == ACLCHECK_OK &&
+ (RelationIsPermanent(seqrel) || !RecoveryInProgress()) &&
!RELATION_IS_OTHER_TEMP(seqrel))
{
seq = read_seq_tuple(seqrel, &buf, &seqtuple);
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index ef658ad740..04b3790bdd 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1699,10 +1699,7 @@ pg_sequences| SELECT n.nspname AS schemaname,
s.seqincrement AS increment_by,
s.seqcycle AS cycle,
s.seqcache AS cache_size,
- CASE
- WHEN has_sequence_privilege(c.oid, 'SELECT,USAGE'::text) THEN pg_sequence_last_value((c.oid)::regclass)
- ELSE NULL::bigint
- END AS last_value
+ pg_sequence_last_value((c.oid)::regclass) AS last_value
FROM ((pg_sequence s
JOIN pg_class c ON ((c.oid = s.seqrelid)))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
--
2.25.1
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-03 21:22 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-04 09:45 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-07 17:10 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 17:44 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-07 18:40 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 19:02 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-07 19:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
@ 2024-05-08 02:01 ` Michael Paquier <[email protected]>
2024-05-10 21:00 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Michael Paquier @ 2024-05-08 02:01 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
On Tue, May 07, 2024 at 02:39:42PM -0500, Nathan Bossart wrote:
> Okay, phew. We can still do something like v3-0002 for v18. I'll give
> Michael a chance to comment on 0001 before committing/back-patching that
> one.
What you are doing in 0001, and 0002 for v18 sounds fine to me.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-03 21:22 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-04 09:45 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-07 17:10 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 17:44 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-07 18:40 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 19:02 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-07 19:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-08 02:01 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
@ 2024-05-10 21:00 ` Nathan Bossart <[email protected]>
2024-05-13 21:01 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Bossart @ 2024-05-10 21:00 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
On Wed, May 08, 2024 at 11:01:01AM +0900, Michael Paquier wrote:
> On Tue, May 07, 2024 at 02:39:42PM -0500, Nathan Bossart wrote:
>> Okay, phew. We can still do something like v3-0002 for v18. I'll give
>> Michael a chance to comment on 0001 before committing/back-patching that
>> one.
>
> What you are doing in 0001, and 0002 for v18 sounds fine to me.
Great. Rather than commit this on a Friday afternoon, I'll just post what
I have staged for commit early next week.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
From 19d9a1dd88385664e6991121e4751aba85a45639 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 May 2024 15:55:24 -0500
Subject: [PATCH v4 1/1] Fix pg_sequence_last_value() for unlogged sequences on
standbys.
Presently, when this function is called for an unlogged sequence on
a standby server, it will error out with a message like
ERROR: could not open file "base/5/16388": No such file or directory
Since the pg_sequences system view uses pg_sequence_last_value(),
it can error similarly. To fix, modify the function to return NULL
for unlogged sequences on standby servers. Since this bug is
present on all versions since v15, this approach is preferable to
making the ERROR nicer because we need to repair the pg_sequences
view without modifying its definition on released versions. For
consistency, this commit also modifies the function to return NULL
for other sessions' temporary sequences. The pg_sequences view
already appropriately filters out such sequences, so there's no bug
there, but we might as well offer some defense in case someone
invokes this function directly.
Unlogged sequences were first introduced in v15, but temporary
sequences are much older, so while the fix for unlogged sequences
is only back-patched to v15, the temporary sequence portion is
back-patched to all supported versions.
We could also remove the privilege check in the pg_sequences view
definition in v18 if we modify this function to return NULL for
sequences for which the current user lacks privileges, but that is
left as a future exercise for when v18 development begins.
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20240501005730.GA594666%40nathanxps13
Backpatch-through: 12
---
doc/src/sgml/system-views.sgml | 34 +++++++++++++++++++++++----
src/backend/commands/sequence.c | 31 +++++++++++++++++-------
src/test/recovery/t/001_stream_rep.pl | 8 +++++++
3 files changed, 60 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index a54f4a4743..9842ee276e 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -3091,15 +3091,41 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<para>
The last sequence value written to disk. If caching is used,
this value can be greater than the last value handed out from the
- sequence. Null if the sequence has not been read from yet. Also, if
- the current user does not have <literal>USAGE</literal>
- or <literal>SELECT</literal> privilege on the sequence, the value is
- null.
+ sequence.
</para></entry>
</row>
</tbody>
</tgroup>
</table>
+
+ <para>
+ The <structfield>last_value</structfield> column will read as null if any of
+ the following are true:
+ <itemizedlist>
+ <listitem>
+ <para>
+ The sequence has not been read from yet.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The current user does not have <literal>USAGE</literal> or
+ <literal>SELECT</literal> privilege on the sequence.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is a temporary sequence created by another session.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is unlogged and the server is a standby.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
</sect1>
<sect1 id="view-pg-settings">
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 46103561c3..28f8522264 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1777,11 +1777,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
SeqTable elm;
Relation seqrel;
- Buffer buf;
- HeapTupleData seqtuple;
- Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1792,12 +1789,28 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * We return NULL for other sessions' temporary sequences. The
+ * pg_sequences system view already filters those out, but this offers a
+ * defense against ERRORs in case someone invokes this function directly.
+ *
+ * Also, for the benefit of the pg_sequences view, we return NULL for
+ * unlogged sequences on standbys instead of throwing an error.
+ */
+ if (!RELATION_IS_OTHER_TEMP(seqrel) &&
+ (RelationIsPermanent(seqrel) || !RecoveryInProgress()))
+ {
+ Buffer buf;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
+
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- is_called = seq->is_called;
- result = seq->last_value;
+ is_called = seq->is_called;
+ result = seq->last_value;
- UnlockReleaseBuffer(buf);
+ UnlockReleaseBuffer(buf);
+ }
sequence_close(seqrel, NoLock);
if (is_called)
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 5311ade509..4c698b5ce1 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -95,6 +95,14 @@ $result = $node_standby_2->safe_psql('postgres', "SELECT * FROM seq1");
print "standby 2: $result\n";
is($result, qq(33|0|t), 'check streamed sequence content on standby 2');
+# Check pg_sequence_last_value() returns NULL for unlogged sequence on standby
+$node_primary->safe_psql('postgres',
+ "CREATE UNLOGGED SEQUENCE ulseq; SELECT nextval('ulseq')");
+$node_primary->wait_for_replay_catchup($node_standby_1);
+is($node_standby_1->safe_psql('postgres',
+ "SELECT pg_sequence_last_value('ulseq'::regclass) IS NULL"),
+ 't', 'pg_sequence_last_value() on unlogged sequence on standby 1');
+
# Check that only READ-only queries can run on standbys
is($node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
3, 'read-only queries on standby 1');
--
2.25.1
From 6f99d2cfcf3572d2815055ff2e3e75a314d9c7e3 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 May 2024 15:55:24 -0500
Subject: [PATCH v4 1/1] Fix pg_sequence_last_value() for unlogged sequences on
standbys.
Presently, when this function is called for an unlogged sequence on
a standby server, it will error out with a message like
ERROR: could not open file "base/5/16388": No such file or directory
Since the pg_sequences system view uses pg_sequence_last_value(),
it can error similarly. To fix, modify the function to return NULL
for unlogged sequences on standby servers. Since this bug is
present on all versions since v15, this approach is preferable to
making the ERROR nicer because we need to repair the pg_sequences
view without modifying its definition on released versions. For
consistency, this commit also modifies the function to return NULL
for other sessions' temporary sequences. The pg_sequences view
already appropriately filters out such sequences, so there's no bug
there, but we might as well offer some defense in case someone
invokes this function directly.
Unlogged sequences were first introduced in v15, but temporary
sequences are much older, so while the fix for unlogged sequences
is only back-patched to v15, the temporary sequence portion is
back-patched to all supported versions.
We could also remove the privilege check in the pg_sequences view
definition in v18 if we modify this function to return NULL for
sequences for which the current user lacks privileges, but that is
left as a future exercise for when v18 development begins.
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20240501005730.GA594666%40nathanxps13
Backpatch-through: 12
---
doc/src/sgml/system-views.sgml | 34 +++++++++++++++++++++++----
src/backend/commands/sequence.c | 31 +++++++++++++++++-------
src/test/recovery/t/001_stream_rep.pl | 8 +++++++
3 files changed, 60 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 39815d5faf..82a56f6af4 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -3009,15 +3009,41 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<para>
The last sequence value written to disk. If caching is used,
this value can be greater than the last value handed out from the
- sequence. Null if the sequence has not been read from yet. Also, if
- the current user does not have <literal>USAGE</literal>
- or <literal>SELECT</literal> privilege on the sequence, the value is
- null.
+ sequence.
</para></entry>
</row>
</tbody>
</tgroup>
</table>
+
+ <para>
+ The <structfield>last_value</structfield> column will read as null if any of
+ the following are true:
+ <itemizedlist>
+ <listitem>
+ <para>
+ The sequence has not been read from yet.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The current user does not have <literal>USAGE</literal> or
+ <literal>SELECT</literal> privilege on the sequence.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is a temporary sequence created by another session.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is unlogged and the server is a standby.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
</sect1>
<sect1 id="view-pg-settings">
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index c7e262c0fc..3fa4e78857 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1795,11 +1795,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
SeqTable elm;
Relation seqrel;
- Buffer buf;
- HeapTupleData seqtuple;
- Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1810,12 +1807,28 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * We return NULL for other sessions' temporary sequences. The
+ * pg_sequences system view already filters those out, but this offers a
+ * defense against ERRORs in case someone invokes this function directly.
+ *
+ * Also, for the benefit of the pg_sequences view, we return NULL for
+ * unlogged sequences on standbys instead of throwing an error.
+ */
+ if (!RELATION_IS_OTHER_TEMP(seqrel) &&
+ (RelationIsPermanent(seqrel) || !RecoveryInProgress()))
+ {
+ Buffer buf;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
+
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- is_called = seq->is_called;
- result = seq->last_value;
+ is_called = seq->is_called;
+ result = seq->last_value;
- UnlockReleaseBuffer(buf);
+ UnlockReleaseBuffer(buf);
+ }
relation_close(seqrel, NoLock);
if (is_called)
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 0c72ba0944..710bdd54da 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -76,6 +76,14 @@ $result = $node_standby_2->safe_psql('postgres', "SELECT * FROM seq1");
print "standby 2: $result\n";
is($result, qq(33|0|t), 'check streamed sequence content on standby 2');
+# Check pg_sequence_last_value() returns NULL for unlogged sequence on standby
+$node_primary->safe_psql('postgres',
+ "CREATE UNLOGGED SEQUENCE ulseq; SELECT nextval('ulseq')");
+$node_primary->wait_for_replay_catchup($node_standby_1);
+is($node_standby_1->safe_psql('postgres',
+ "SELECT pg_sequence_last_value('ulseq'::regclass) IS NULL"),
+ 't', 'pg_sequence_last_value() on unlogged sequence on standby 1');
+
# Check that only READ-only queries can run on standbys
is($node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
3, 'read-only queries on standby 1');
--
2.25.1
From 23e3ced85cfdcff5760e3ea32b962c009b50aede Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 May 2024 15:55:24 -0500
Subject: [PATCH v4 1/1] Fix pg_sequence_last_value() for unlogged sequences on
standbys.
Presently, when this function is called for an unlogged sequence on
a standby server, it will error out with a message like
ERROR: could not open file "base/5/16388": No such file or directory
Since the pg_sequences system view uses pg_sequence_last_value(),
it can error similarly. To fix, modify the function to return NULL
for unlogged sequences on standby servers. Since this bug is
present on all versions since v15, this approach is preferable to
making the ERROR nicer because we need to repair the pg_sequences
view without modifying its definition on released versions. For
consistency, this commit also modifies the function to return NULL
for other sessions' temporary sequences. The pg_sequences view
already appropriately filters out such sequences, so there's no bug
there, but we might as well offer some defense in case someone
invokes this function directly.
Unlogged sequences were first introduced in v15, but temporary
sequences are much older, so while the fix for unlogged sequences
is only back-patched to v15, the temporary sequence portion is
back-patched to all supported versions.
We could also remove the privilege check in the pg_sequences view
definition in v18 if we modify this function to return NULL for
sequences for which the current user lacks privileges, but that is
left as a future exercise for when v18 development begins.
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20240501005730.GA594666%40nathanxps13
Backpatch-through: 12
---
doc/src/sgml/system-views.sgml | 34 +++++++++++++++++++++++----
src/backend/commands/sequence.c | 31 +++++++++++++++++-------
src/test/recovery/t/001_stream_rep.pl | 9 +++++++
3 files changed, 61 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 5f8b99bf69..e7284e2df5 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -2927,15 +2927,41 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<para>
The last sequence value written to disk. If caching is used,
this value can be greater than the last value handed out from the
- sequence. Null if the sequence has not been read from yet. Also, if
- the current user does not have <literal>USAGE</literal>
- or <literal>SELECT</literal> privilege on the sequence, the value is
- null.
+ sequence.
</para></entry>
</row>
</tbody>
</tgroup>
</table>
+
+ <para>
+ The <structfield>last_value</structfield> column will read as null if any of
+ the following are true:
+ <itemizedlist>
+ <listitem>
+ <para>
+ The sequence has not been read from yet.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The current user does not have <literal>USAGE</literal> or
+ <literal>SELECT</literal> privilege on the sequence.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is a temporary sequence created by another session.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is unlogged and the server is a standby.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
</sect1>
<sect1 id="view-pg-settings">
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index acaf660c68..1a73a63d61 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1810,11 +1810,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
SeqTable elm;
Relation seqrel;
- Buffer buf;
- HeapTupleData seqtuple;
- Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1825,12 +1822,28 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * We return NULL for other sessions' temporary sequences. The
+ * pg_sequences system view already filters those out, but this offers a
+ * defense against ERRORs in case someone invokes this function directly.
+ *
+ * Also, for the benefit of the pg_sequences view, we return NULL for
+ * unlogged sequences on standbys instead of throwing an error.
+ */
+ if (!RELATION_IS_OTHER_TEMP(seqrel) &&
+ (RelationIsPermanent(seqrel) || !RecoveryInProgress()))
+ {
+ Buffer buf;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
+
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- is_called = seq->is_called;
- result = seq->last_value;
+ is_called = seq->is_called;
+ result = seq->last_value;
- UnlockReleaseBuffer(buf);
+ UnlockReleaseBuffer(buf);
+ }
relation_close(seqrel, NoLock);
if (is_called)
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 86864098f9..54c3d3bdf5 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -78,6 +78,15 @@ $result = $node_standby_2->safe_psql('postgres', "SELECT * FROM seq1");
print "standby 2: $result\n";
is($result, qq(33|0|t), 'check streamed sequence content on standby 2');
+# Check pg_sequence_last_value() returns NULL for unlogged sequence on standby
+$node_primary->safe_psql('postgres',
+ "CREATE UNLOGGED SEQUENCE ulseq; SELECT nextval('ulseq')");
+$primary_lsn = $node_primary->lsn('write');
+$node_primary->wait_for_catchup($node_standby_1, 'replay', $primary_lsn);
+is($node_standby_1->safe_psql('postgres',
+ "SELECT pg_sequence_last_value('ulseq'::regclass) IS NULL"),
+ 't', 'pg_sequence_last_value() on unlogged sequence on standby 1');
+
# Check that only READ-only queries can run on standbys
is($node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
3, 'read-only queries on standby 1');
--
2.25.1
From 9d454ea9fd11e42a2408cf35dd40957529c06308 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 May 2024 15:55:24 -0500
Subject: [PATCH v4 1/1] Fix pg_sequence_last_value() for unlogged sequences on
standbys.
Presently, when this function is called for an unlogged sequence on
a standby server, it will error out with a message like
ERROR: could not open file "base/5/16388": No such file or directory
Since the pg_sequences system view uses pg_sequence_last_value(),
it can error similarly. To fix, modify the function to return NULL
for unlogged sequences on standby servers. Since this bug is
present on all versions since v15, this approach is preferable to
making the ERROR nicer because we need to repair the pg_sequences
view without modifying its definition on released versions. For
consistency, this commit also modifies the function to return NULL
for other sessions' temporary sequences. The pg_sequences view
already appropriately filters out such sequences, so there's no bug
there, but we might as well offer some defense in case someone
invokes this function directly.
Unlogged sequences were first introduced in v15, but temporary
sequences are much older, so while the fix for unlogged sequences
is only back-patched to v15, the temporary sequence portion is
back-patched to all supported versions.
We could also remove the privilege check in the pg_sequences view
definition in v18 if we modify this function to return NULL for
sequences for which the current user lacks privileges, but that is
left as a future exercise for when v18 development begins.
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20240501005730.GA594666%40nathanxps13
Backpatch-through: 12
---
doc/src/sgml/catalogs.sgml | 29 +++++++++++++++++++++++++----
src/backend/commands/sequence.c | 27 ++++++++++++++++++---------
2 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index dba6479cf5..b444ca776c 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -12108,15 +12108,36 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<para>
The last sequence value written to disk. If caching is used,
this value can be greater than the last value handed out from the
- sequence. Null if the sequence has not been read from yet. Also, if
- the current user does not have <literal>USAGE</literal>
- or <literal>SELECT</literal> privilege on the sequence, the value is
- null.
+ sequence.
</para></entry>
</row>
</tbody>
</tgroup>
</table>
+
+ <para>
+ The <structfield>last_value</structfield> column will read as null if any of
+ the following are true:
+ <itemizedlist>
+ <listitem>
+ <para>
+ The sequence has not been read from yet.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The current user does not have <literal>USAGE</literal> or
+ <literal>SELECT</literal> privilege on the sequence.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is a temporary sequence created by another session.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
</sect1>
<sect1 id="view-pg-settings">
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 98649986e1..ad34aaff6d 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1856,11 +1856,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
SeqTable elm;
Relation seqrel;
- Buffer buf;
- HeapTupleData seqtuple;
- Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1871,12 +1868,24 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * We return NULL for other sessions' temporary sequences. The
+ * pg_sequences system view already filters those out, but this offers a
+ * defense against ERRORs in case someone invokes this function directly.
+ */
+ if (!RELATION_IS_OTHER_TEMP(seqrel))
+ {
+ Buffer buf;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
- is_called = seq->is_called;
- result = seq->last_value;
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- UnlockReleaseBuffer(buf);
+ is_called = seq->is_called;
+ result = seq->last_value;
+
+ UnlockReleaseBuffer(buf);
+ }
relation_close(seqrel, NoLock);
if (is_called)
--
2.25.1
From 8e614d1d1439d9b75e3c5218ccfb9e4123fb6d14 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 May 2024 15:55:24 -0500
Subject: [PATCH v4 1/1] Fix pg_sequence_last_value() for unlogged sequences on
standbys.
Presently, when this function is called for an unlogged sequence on
a standby server, it will error out with a message like
ERROR: could not open file "base/5/16388": No such file or directory
Since the pg_sequences system view uses pg_sequence_last_value(),
it can error similarly. To fix, modify the function to return NULL
for unlogged sequences on standby servers. Since this bug is
present on all versions since v15, this approach is preferable to
making the ERROR nicer because we need to repair the pg_sequences
view without modifying its definition on released versions. For
consistency, this commit also modifies the function to return NULL
for other sessions' temporary sequences. The pg_sequences view
already appropriately filters out such sequences, so there's no bug
there, but we might as well offer some defense in case someone
invokes this function directly.
Unlogged sequences were first introduced in v15, but temporary
sequences are much older, so while the fix for unlogged sequences
is only back-patched to v15, the temporary sequence portion is
back-patched to all supported versions.
We could also remove the privilege check in the pg_sequences view
definition in v18 if we modify this function to return NULL for
sequences for which the current user lacks privileges, but that is
left as a future exercise for when v18 development begins.
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20240501005730.GA594666%40nathanxps13
Backpatch-through: 12
---
doc/src/sgml/catalogs.sgml | 29 +++++++++++++++++++++++++----
src/backend/commands/sequence.c | 27 ++++++++++++++++++---------
2 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 0569093486..23dd3df40a 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -11794,15 +11794,36 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<para>
The last sequence value written to disk. If caching is used,
this value can be greater than the last value handed out from the
- sequence. Null if the sequence has not been read from yet. Also, if
- the current user does not have <literal>USAGE</literal>
- or <literal>SELECT</literal> privilege on the sequence, the value is
- null.
+ sequence.
</para></entry>
</row>
</tbody>
</tgroup>
</table>
+
+ <para>
+ The <structfield>last_value</structfield> column will read as null if any of
+ the following are true:
+ <itemizedlist>
+ <listitem>
+ <para>
+ The sequence has not been read from yet.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The current user does not have <literal>USAGE</literal> or
+ <literal>SELECT</literal> privilege on the sequence.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is a temporary sequence created by another session.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
</sect1>
<sect1 id="view-pg-settings">
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index ca0bdb5771..5106ef685b 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1857,11 +1857,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
SeqTable elm;
Relation seqrel;
- Buffer buf;
- HeapTupleData seqtuple;
- Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1872,12 +1869,24 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * We return NULL for other sessions' temporary sequences. The
+ * pg_sequences system view already filters those out, but this offers a
+ * defense against ERRORs in case someone invokes this function directly.
+ */
+ if (!RELATION_IS_OTHER_TEMP(seqrel))
+ {
+ Buffer buf;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
- is_called = seq->is_called;
- result = seq->last_value;
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- UnlockReleaseBuffer(buf);
+ is_called = seq->is_called;
+ result = seq->last_value;
+
+ UnlockReleaseBuffer(buf);
+ }
relation_close(seqrel, NoLock);
if (is_called)
--
2.25.1
From 8078c01d39e2f66c9d1f6161799e19d4e5ceb8a5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 May 2024 15:55:24 -0500
Subject: [PATCH v4 1/1] Fix pg_sequence_last_value() for unlogged sequences on
standbys.
Presently, when this function is called for an unlogged sequence on
a standby server, it will error out with a message like
ERROR: could not open file "base/5/16388": No such file or directory
Since the pg_sequences system view uses pg_sequence_last_value(),
it can error similarly. To fix, modify the function to return NULL
for unlogged sequences on standby servers. Since this bug is
present on all versions since v15, this approach is preferable to
making the ERROR nicer because we need to repair the pg_sequences
view without modifying its definition on released versions. For
consistency, this commit also modifies the function to return NULL
for other sessions' temporary sequences. The pg_sequences view
already appropriately filters out such sequences, so there's no bug
there, but we might as well offer some defense in case someone
invokes this function directly.
Unlogged sequences were first introduced in v15, but temporary
sequences are much older, so while the fix for unlogged sequences
is only back-patched to v15, the temporary sequence portion is
back-patched to all supported versions.
We could also remove the privilege check in the pg_sequences view
definition in v18 if we modify this function to return NULL for
sequences for which the current user lacks privileges, but that is
left as a future exercise for when v18 development begins.
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20240501005730.GA594666%40nathanxps13
Backpatch-through: 12
---
doc/src/sgml/catalogs.sgml | 29 +++++++++++++++++++++++++----
src/backend/commands/sequence.c | 27 ++++++++++++++++++---------
2 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 118e325464..2337ce8fd2 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10381,14 +10381,35 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<entry></entry>
<entry>The last sequence value written to disk. If caching is used,
this value can be greater than the last value handed out from the
- sequence. Null if the sequence has not been read from yet. Also, if
- the current user does not have <literal>USAGE</literal>
- or <literal>SELECT</literal> privilege on the sequence, the value is
- null.</entry>
+ sequence.</entry>
</row>
</tbody>
</tgroup>
</table>
+
+ <para>
+ The <structfield>last_value</structfield> column will read as null if any of
+ the following are true:
+ <itemizedlist>
+ <listitem>
+ <para>
+ The sequence has not been read from yet.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The current user does not have <literal>USAGE</literal> or
+ <literal>SELECT</literal> privilege on the sequence.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is a temporary sequence created by another session.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
</sect1>
<sect1 id="view-pg-settings">
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 0577184f82..daaf8ee3d4 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1857,11 +1857,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
SeqTable elm;
Relation seqrel;
- Buffer buf;
- HeapTupleData seqtuple;
- Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1872,12 +1869,24 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * We return NULL for other sessions' temporary sequences. The
+ * pg_sequences system view already filters those out, but this offers a
+ * defense against ERRORs in case someone invokes this function directly.
+ */
+ if (!RELATION_IS_OTHER_TEMP(seqrel))
+ {
+ Buffer buf;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
- is_called = seq->is_called;
- result = seq->last_value;
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- UnlockReleaseBuffer(buf);
+ is_called = seq->is_called;
+ result = seq->last_value;
+
+ UnlockReleaseBuffer(buf);
+ }
relation_close(seqrel, NoLock);
if (is_called)
--
2.25.1
Attachments:
[text/plain] v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.master (6.0K, ../../20240510210055.GA428795@nathanxps13/2-v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.master)
download | inline diff:
From 19d9a1dd88385664e6991121e4751aba85a45639 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 May 2024 15:55:24 -0500
Subject: [PATCH v4 1/1] Fix pg_sequence_last_value() for unlogged sequences on
standbys.
Presently, when this function is called for an unlogged sequence on
a standby server, it will error out with a message like
ERROR: could not open file "base/5/16388": No such file or directory
Since the pg_sequences system view uses pg_sequence_last_value(),
it can error similarly. To fix, modify the function to return NULL
for unlogged sequences on standby servers. Since this bug is
present on all versions since v15, this approach is preferable to
making the ERROR nicer because we need to repair the pg_sequences
view without modifying its definition on released versions. For
consistency, this commit also modifies the function to return NULL
for other sessions' temporary sequences. The pg_sequences view
already appropriately filters out such sequences, so there's no bug
there, but we might as well offer some defense in case someone
invokes this function directly.
Unlogged sequences were first introduced in v15, but temporary
sequences are much older, so while the fix for unlogged sequences
is only back-patched to v15, the temporary sequence portion is
back-patched to all supported versions.
We could also remove the privilege check in the pg_sequences view
definition in v18 if we modify this function to return NULL for
sequences for which the current user lacks privileges, but that is
left as a future exercise for when v18 development begins.
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20240501005730.GA594666%40nathanxps13
Backpatch-through: 12
---
doc/src/sgml/system-views.sgml | 34 +++++++++++++++++++++++----
src/backend/commands/sequence.c | 31 +++++++++++++++++-------
src/test/recovery/t/001_stream_rep.pl | 8 +++++++
3 files changed, 60 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index a54f4a4743..9842ee276e 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -3091,15 +3091,41 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<para>
The last sequence value written to disk. If caching is used,
this value can be greater than the last value handed out from the
- sequence. Null if the sequence has not been read from yet. Also, if
- the current user does not have <literal>USAGE</literal>
- or <literal>SELECT</literal> privilege on the sequence, the value is
- null.
+ sequence.
</para></entry>
</row>
</tbody>
</tgroup>
</table>
+
+ <para>
+ The <structfield>last_value</structfield> column will read as null if any of
+ the following are true:
+ <itemizedlist>
+ <listitem>
+ <para>
+ The sequence has not been read from yet.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The current user does not have <literal>USAGE</literal> or
+ <literal>SELECT</literal> privilege on the sequence.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is a temporary sequence created by another session.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is unlogged and the server is a standby.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
</sect1>
<sect1 id="view-pg-settings">
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 46103561c3..28f8522264 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1777,11 +1777,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
SeqTable elm;
Relation seqrel;
- Buffer buf;
- HeapTupleData seqtuple;
- Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1792,12 +1789,28 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * We return NULL for other sessions' temporary sequences. The
+ * pg_sequences system view already filters those out, but this offers a
+ * defense against ERRORs in case someone invokes this function directly.
+ *
+ * Also, for the benefit of the pg_sequences view, we return NULL for
+ * unlogged sequences on standbys instead of throwing an error.
+ */
+ if (!RELATION_IS_OTHER_TEMP(seqrel) &&
+ (RelationIsPermanent(seqrel) || !RecoveryInProgress()))
+ {
+ Buffer buf;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
+
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- is_called = seq->is_called;
- result = seq->last_value;
+ is_called = seq->is_called;
+ result = seq->last_value;
- UnlockReleaseBuffer(buf);
+ UnlockReleaseBuffer(buf);
+ }
sequence_close(seqrel, NoLock);
if (is_called)
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 5311ade509..4c698b5ce1 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -95,6 +95,14 @@ $result = $node_standby_2->safe_psql('postgres', "SELECT * FROM seq1");
print "standby 2: $result\n";
is($result, qq(33|0|t), 'check streamed sequence content on standby 2');
+# Check pg_sequence_last_value() returns NULL for unlogged sequence on standby
+$node_primary->safe_psql('postgres',
+ "CREATE UNLOGGED SEQUENCE ulseq; SELECT nextval('ulseq')");
+$node_primary->wait_for_replay_catchup($node_standby_1);
+is($node_standby_1->safe_psql('postgres',
+ "SELECT pg_sequence_last_value('ulseq'::regclass) IS NULL"),
+ 't', 'pg_sequence_last_value() on unlogged sequence on standby 1');
+
# Check that only READ-only queries can run on standbys
is($node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
3, 'read-only queries on standby 1');
--
2.25.1
[text/plain] v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v16 (6.0K, ../../20240510210055.GA428795@nathanxps13/3-v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v16)
download | inline diff:
From 6f99d2cfcf3572d2815055ff2e3e75a314d9c7e3 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 May 2024 15:55:24 -0500
Subject: [PATCH v4 1/1] Fix pg_sequence_last_value() for unlogged sequences on
standbys.
Presently, when this function is called for an unlogged sequence on
a standby server, it will error out with a message like
ERROR: could not open file "base/5/16388": No such file or directory
Since the pg_sequences system view uses pg_sequence_last_value(),
it can error similarly. To fix, modify the function to return NULL
for unlogged sequences on standby servers. Since this bug is
present on all versions since v15, this approach is preferable to
making the ERROR nicer because we need to repair the pg_sequences
view without modifying its definition on released versions. For
consistency, this commit also modifies the function to return NULL
for other sessions' temporary sequences. The pg_sequences view
already appropriately filters out such sequences, so there's no bug
there, but we might as well offer some defense in case someone
invokes this function directly.
Unlogged sequences were first introduced in v15, but temporary
sequences are much older, so while the fix for unlogged sequences
is only back-patched to v15, the temporary sequence portion is
back-patched to all supported versions.
We could also remove the privilege check in the pg_sequences view
definition in v18 if we modify this function to return NULL for
sequences for which the current user lacks privileges, but that is
left as a future exercise for when v18 development begins.
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20240501005730.GA594666%40nathanxps13
Backpatch-through: 12
---
doc/src/sgml/system-views.sgml | 34 +++++++++++++++++++++++----
src/backend/commands/sequence.c | 31 +++++++++++++++++-------
src/test/recovery/t/001_stream_rep.pl | 8 +++++++
3 files changed, 60 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 39815d5faf..82a56f6af4 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -3009,15 +3009,41 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<para>
The last sequence value written to disk. If caching is used,
this value can be greater than the last value handed out from the
- sequence. Null if the sequence has not been read from yet. Also, if
- the current user does not have <literal>USAGE</literal>
- or <literal>SELECT</literal> privilege on the sequence, the value is
- null.
+ sequence.
</para></entry>
</row>
</tbody>
</tgroup>
</table>
+
+ <para>
+ The <structfield>last_value</structfield> column will read as null if any of
+ the following are true:
+ <itemizedlist>
+ <listitem>
+ <para>
+ The sequence has not been read from yet.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The current user does not have <literal>USAGE</literal> or
+ <literal>SELECT</literal> privilege on the sequence.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is a temporary sequence created by another session.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is unlogged and the server is a standby.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
</sect1>
<sect1 id="view-pg-settings">
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index c7e262c0fc..3fa4e78857 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1795,11 +1795,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
SeqTable elm;
Relation seqrel;
- Buffer buf;
- HeapTupleData seqtuple;
- Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1810,12 +1807,28 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * We return NULL for other sessions' temporary sequences. The
+ * pg_sequences system view already filters those out, but this offers a
+ * defense against ERRORs in case someone invokes this function directly.
+ *
+ * Also, for the benefit of the pg_sequences view, we return NULL for
+ * unlogged sequences on standbys instead of throwing an error.
+ */
+ if (!RELATION_IS_OTHER_TEMP(seqrel) &&
+ (RelationIsPermanent(seqrel) || !RecoveryInProgress()))
+ {
+ Buffer buf;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
+
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- is_called = seq->is_called;
- result = seq->last_value;
+ is_called = seq->is_called;
+ result = seq->last_value;
- UnlockReleaseBuffer(buf);
+ UnlockReleaseBuffer(buf);
+ }
relation_close(seqrel, NoLock);
if (is_called)
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 0c72ba0944..710bdd54da 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -76,6 +76,14 @@ $result = $node_standby_2->safe_psql('postgres', "SELECT * FROM seq1");
print "standby 2: $result\n";
is($result, qq(33|0|t), 'check streamed sequence content on standby 2');
+# Check pg_sequence_last_value() returns NULL for unlogged sequence on standby
+$node_primary->safe_psql('postgres',
+ "CREATE UNLOGGED SEQUENCE ulseq; SELECT nextval('ulseq')");
+$node_primary->wait_for_replay_catchup($node_standby_1);
+is($node_standby_1->safe_psql('postgres',
+ "SELECT pg_sequence_last_value('ulseq'::regclass) IS NULL"),
+ 't', 'pg_sequence_last_value() on unlogged sequence on standby 1');
+
# Check that only READ-only queries can run on standbys
is($node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
3, 'read-only queries on standby 1');
--
2.25.1
[text/plain] v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v15 (6.0K, ../../20240510210055.GA428795@nathanxps13/4-v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v15)
download | inline diff:
From 23e3ced85cfdcff5760e3ea32b962c009b50aede Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 May 2024 15:55:24 -0500
Subject: [PATCH v4 1/1] Fix pg_sequence_last_value() for unlogged sequences on
standbys.
Presently, when this function is called for an unlogged sequence on
a standby server, it will error out with a message like
ERROR: could not open file "base/5/16388": No such file or directory
Since the pg_sequences system view uses pg_sequence_last_value(),
it can error similarly. To fix, modify the function to return NULL
for unlogged sequences on standby servers. Since this bug is
present on all versions since v15, this approach is preferable to
making the ERROR nicer because we need to repair the pg_sequences
view without modifying its definition on released versions. For
consistency, this commit also modifies the function to return NULL
for other sessions' temporary sequences. The pg_sequences view
already appropriately filters out such sequences, so there's no bug
there, but we might as well offer some defense in case someone
invokes this function directly.
Unlogged sequences were first introduced in v15, but temporary
sequences are much older, so while the fix for unlogged sequences
is only back-patched to v15, the temporary sequence portion is
back-patched to all supported versions.
We could also remove the privilege check in the pg_sequences view
definition in v18 if we modify this function to return NULL for
sequences for which the current user lacks privileges, but that is
left as a future exercise for when v18 development begins.
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20240501005730.GA594666%40nathanxps13
Backpatch-through: 12
---
doc/src/sgml/system-views.sgml | 34 +++++++++++++++++++++++----
src/backend/commands/sequence.c | 31 +++++++++++++++++-------
src/test/recovery/t/001_stream_rep.pl | 9 +++++++
3 files changed, 61 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 5f8b99bf69..e7284e2df5 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -2927,15 +2927,41 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<para>
The last sequence value written to disk. If caching is used,
this value can be greater than the last value handed out from the
- sequence. Null if the sequence has not been read from yet. Also, if
- the current user does not have <literal>USAGE</literal>
- or <literal>SELECT</literal> privilege on the sequence, the value is
- null.
+ sequence.
</para></entry>
</row>
</tbody>
</tgroup>
</table>
+
+ <para>
+ The <structfield>last_value</structfield> column will read as null if any of
+ the following are true:
+ <itemizedlist>
+ <listitem>
+ <para>
+ The sequence has not been read from yet.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The current user does not have <literal>USAGE</literal> or
+ <literal>SELECT</literal> privilege on the sequence.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is a temporary sequence created by another session.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is unlogged and the server is a standby.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
</sect1>
<sect1 id="view-pg-settings">
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index acaf660c68..1a73a63d61 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1810,11 +1810,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
SeqTable elm;
Relation seqrel;
- Buffer buf;
- HeapTupleData seqtuple;
- Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1825,12 +1822,28 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * We return NULL for other sessions' temporary sequences. The
+ * pg_sequences system view already filters those out, but this offers a
+ * defense against ERRORs in case someone invokes this function directly.
+ *
+ * Also, for the benefit of the pg_sequences view, we return NULL for
+ * unlogged sequences on standbys instead of throwing an error.
+ */
+ if (!RELATION_IS_OTHER_TEMP(seqrel) &&
+ (RelationIsPermanent(seqrel) || !RecoveryInProgress()))
+ {
+ Buffer buf;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
+
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- is_called = seq->is_called;
- result = seq->last_value;
+ is_called = seq->is_called;
+ result = seq->last_value;
- UnlockReleaseBuffer(buf);
+ UnlockReleaseBuffer(buf);
+ }
relation_close(seqrel, NoLock);
if (is_called)
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 86864098f9..54c3d3bdf5 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -78,6 +78,15 @@ $result = $node_standby_2->safe_psql('postgres', "SELECT * FROM seq1");
print "standby 2: $result\n";
is($result, qq(33|0|t), 'check streamed sequence content on standby 2');
+# Check pg_sequence_last_value() returns NULL for unlogged sequence on standby
+$node_primary->safe_psql('postgres',
+ "CREATE UNLOGGED SEQUENCE ulseq; SELECT nextval('ulseq')");
+$primary_lsn = $node_primary->lsn('write');
+$node_primary->wait_for_catchup($node_standby_1, 'replay', $primary_lsn);
+is($node_standby_1->safe_psql('postgres',
+ "SELECT pg_sequence_last_value('ulseq'::regclass) IS NULL"),
+ 't', 'pg_sequence_last_value() on unlogged sequence on standby 1');
+
# Check that only READ-only queries can run on standbys
is($node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
3, 'read-only queries on standby 1');
--
2.25.1
[text/plain] v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v14 (4.6K, ../../20240510210055.GA428795@nathanxps13/5-v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v14)
download | inline diff:
From 9d454ea9fd11e42a2408cf35dd40957529c06308 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 May 2024 15:55:24 -0500
Subject: [PATCH v4 1/1] Fix pg_sequence_last_value() for unlogged sequences on
standbys.
Presently, when this function is called for an unlogged sequence on
a standby server, it will error out with a message like
ERROR: could not open file "base/5/16388": No such file or directory
Since the pg_sequences system view uses pg_sequence_last_value(),
it can error similarly. To fix, modify the function to return NULL
for unlogged sequences on standby servers. Since this bug is
present on all versions since v15, this approach is preferable to
making the ERROR nicer because we need to repair the pg_sequences
view without modifying its definition on released versions. For
consistency, this commit also modifies the function to return NULL
for other sessions' temporary sequences. The pg_sequences view
already appropriately filters out such sequences, so there's no bug
there, but we might as well offer some defense in case someone
invokes this function directly.
Unlogged sequences were first introduced in v15, but temporary
sequences are much older, so while the fix for unlogged sequences
is only back-patched to v15, the temporary sequence portion is
back-patched to all supported versions.
We could also remove the privilege check in the pg_sequences view
definition in v18 if we modify this function to return NULL for
sequences for which the current user lacks privileges, but that is
left as a future exercise for when v18 development begins.
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20240501005730.GA594666%40nathanxps13
Backpatch-through: 12
---
doc/src/sgml/catalogs.sgml | 29 +++++++++++++++++++++++++----
src/backend/commands/sequence.c | 27 ++++++++++++++++++---------
2 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index dba6479cf5..b444ca776c 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -12108,15 +12108,36 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<para>
The last sequence value written to disk. If caching is used,
this value can be greater than the last value handed out from the
- sequence. Null if the sequence has not been read from yet. Also, if
- the current user does not have <literal>USAGE</literal>
- or <literal>SELECT</literal> privilege on the sequence, the value is
- null.
+ sequence.
</para></entry>
</row>
</tbody>
</tgroup>
</table>
+
+ <para>
+ The <structfield>last_value</structfield> column will read as null if any of
+ the following are true:
+ <itemizedlist>
+ <listitem>
+ <para>
+ The sequence has not been read from yet.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The current user does not have <literal>USAGE</literal> or
+ <literal>SELECT</literal> privilege on the sequence.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is a temporary sequence created by another session.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
</sect1>
<sect1 id="view-pg-settings">
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 98649986e1..ad34aaff6d 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1856,11 +1856,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
SeqTable elm;
Relation seqrel;
- Buffer buf;
- HeapTupleData seqtuple;
- Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1871,12 +1868,24 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * We return NULL for other sessions' temporary sequences. The
+ * pg_sequences system view already filters those out, but this offers a
+ * defense against ERRORs in case someone invokes this function directly.
+ */
+ if (!RELATION_IS_OTHER_TEMP(seqrel))
+ {
+ Buffer buf;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
- is_called = seq->is_called;
- result = seq->last_value;
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- UnlockReleaseBuffer(buf);
+ is_called = seq->is_called;
+ result = seq->last_value;
+
+ UnlockReleaseBuffer(buf);
+ }
relation_close(seqrel, NoLock);
if (is_called)
--
2.25.1
[text/plain] v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v13 (4.6K, ../../20240510210055.GA428795@nathanxps13/6-v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v13)
download | inline diff:
From 8e614d1d1439d9b75e3c5218ccfb9e4123fb6d14 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 May 2024 15:55:24 -0500
Subject: [PATCH v4 1/1] Fix pg_sequence_last_value() for unlogged sequences on
standbys.
Presently, when this function is called for an unlogged sequence on
a standby server, it will error out with a message like
ERROR: could not open file "base/5/16388": No such file or directory
Since the pg_sequences system view uses pg_sequence_last_value(),
it can error similarly. To fix, modify the function to return NULL
for unlogged sequences on standby servers. Since this bug is
present on all versions since v15, this approach is preferable to
making the ERROR nicer because we need to repair the pg_sequences
view without modifying its definition on released versions. For
consistency, this commit also modifies the function to return NULL
for other sessions' temporary sequences. The pg_sequences view
already appropriately filters out such sequences, so there's no bug
there, but we might as well offer some defense in case someone
invokes this function directly.
Unlogged sequences were first introduced in v15, but temporary
sequences are much older, so while the fix for unlogged sequences
is only back-patched to v15, the temporary sequence portion is
back-patched to all supported versions.
We could also remove the privilege check in the pg_sequences view
definition in v18 if we modify this function to return NULL for
sequences for which the current user lacks privileges, but that is
left as a future exercise for when v18 development begins.
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20240501005730.GA594666%40nathanxps13
Backpatch-through: 12
---
doc/src/sgml/catalogs.sgml | 29 +++++++++++++++++++++++++----
src/backend/commands/sequence.c | 27 ++++++++++++++++++---------
2 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 0569093486..23dd3df40a 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -11794,15 +11794,36 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<para>
The last sequence value written to disk. If caching is used,
this value can be greater than the last value handed out from the
- sequence. Null if the sequence has not been read from yet. Also, if
- the current user does not have <literal>USAGE</literal>
- or <literal>SELECT</literal> privilege on the sequence, the value is
- null.
+ sequence.
</para></entry>
</row>
</tbody>
</tgroup>
</table>
+
+ <para>
+ The <structfield>last_value</structfield> column will read as null if any of
+ the following are true:
+ <itemizedlist>
+ <listitem>
+ <para>
+ The sequence has not been read from yet.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The current user does not have <literal>USAGE</literal> or
+ <literal>SELECT</literal> privilege on the sequence.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is a temporary sequence created by another session.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
</sect1>
<sect1 id="view-pg-settings">
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index ca0bdb5771..5106ef685b 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1857,11 +1857,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
SeqTable elm;
Relation seqrel;
- Buffer buf;
- HeapTupleData seqtuple;
- Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1872,12 +1869,24 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * We return NULL for other sessions' temporary sequences. The
+ * pg_sequences system view already filters those out, but this offers a
+ * defense against ERRORs in case someone invokes this function directly.
+ */
+ if (!RELATION_IS_OTHER_TEMP(seqrel))
+ {
+ Buffer buf;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
- is_called = seq->is_called;
- result = seq->last_value;
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- UnlockReleaseBuffer(buf);
+ is_called = seq->is_called;
+ result = seq->last_value;
+
+ UnlockReleaseBuffer(buf);
+ }
relation_close(seqrel, NoLock);
if (is_called)
--
2.25.1
[text/plain] v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v12 (4.6K, ../../20240510210055.GA428795@nathanxps13/7-v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v12)
download | inline diff:
From 8078c01d39e2f66c9d1f6161799e19d4e5ceb8a5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 May 2024 15:55:24 -0500
Subject: [PATCH v4 1/1] Fix pg_sequence_last_value() for unlogged sequences on
standbys.
Presently, when this function is called for an unlogged sequence on
a standby server, it will error out with a message like
ERROR: could not open file "base/5/16388": No such file or directory
Since the pg_sequences system view uses pg_sequence_last_value(),
it can error similarly. To fix, modify the function to return NULL
for unlogged sequences on standby servers. Since this bug is
present on all versions since v15, this approach is preferable to
making the ERROR nicer because we need to repair the pg_sequences
view without modifying its definition on released versions. For
consistency, this commit also modifies the function to return NULL
for other sessions' temporary sequences. The pg_sequences view
already appropriately filters out such sequences, so there's no bug
there, but we might as well offer some defense in case someone
invokes this function directly.
Unlogged sequences were first introduced in v15, but temporary
sequences are much older, so while the fix for unlogged sequences
is only back-patched to v15, the temporary sequence portion is
back-patched to all supported versions.
We could also remove the privilege check in the pg_sequences view
definition in v18 if we modify this function to return NULL for
sequences for which the current user lacks privileges, but that is
left as a future exercise for when v18 development begins.
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20240501005730.GA594666%40nathanxps13
Backpatch-through: 12
---
doc/src/sgml/catalogs.sgml | 29 +++++++++++++++++++++++++----
src/backend/commands/sequence.c | 27 ++++++++++++++++++---------
2 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 118e325464..2337ce8fd2 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10381,14 +10381,35 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<entry></entry>
<entry>The last sequence value written to disk. If caching is used,
this value can be greater than the last value handed out from the
- sequence. Null if the sequence has not been read from yet. Also, if
- the current user does not have <literal>USAGE</literal>
- or <literal>SELECT</literal> privilege on the sequence, the value is
- null.</entry>
+ sequence.</entry>
</row>
</tbody>
</tgroup>
</table>
+
+ <para>
+ The <structfield>last_value</structfield> column will read as null if any of
+ the following are true:
+ <itemizedlist>
+ <listitem>
+ <para>
+ The sequence has not been read from yet.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The current user does not have <literal>USAGE</literal> or
+ <literal>SELECT</literal> privilege on the sequence.
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ The sequence is a temporary sequence created by another session.
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+
</sect1>
<sect1 id="view-pg-settings">
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 0577184f82..daaf8ee3d4 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1857,11 +1857,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
SeqTable elm;
Relation seqrel;
- Buffer buf;
- HeapTupleData seqtuple;
- Form_pg_sequence_data seq;
- bool is_called;
- int64 result;
+ bool is_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1872,12 +1869,24 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+ * We return NULL for other sessions' temporary sequences. The
+ * pg_sequences system view already filters those out, but this offers a
+ * defense against ERRORs in case someone invokes this function directly.
+ */
+ if (!RELATION_IS_OTHER_TEMP(seqrel))
+ {
+ Buffer buf;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
- is_called = seq->is_called;
- result = seq->last_value;
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
- UnlockReleaseBuffer(buf);
+ is_called = seq->is_called;
+ result = seq->last_value;
+
+ UnlockReleaseBuffer(buf);
+ }
relation_close(seqrel, NoLock);
if (is_called)
--
2.25.1
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-03 21:22 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-04 09:45 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-07 17:10 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 17:44 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-07 18:40 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 19:02 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-07 19:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-08 02:01 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-10 21:00 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
@ 2024-05-13 21:01 ` Nathan Bossart <[email protected]>
2024-05-17 01:33 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Bossart @ 2024-05-13 21:01 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
Committed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-03 21:22 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-04 09:45 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-07 17:10 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 17:44 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-07 18:40 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 19:02 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-07 19:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-08 02:01 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-10 21:00 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-13 21:01 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
@ 2024-05-17 01:33 ` Nathan Bossart <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Nathan Bossart @ 2024-05-17 01:33 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
Here is a rebased version of 0002, which I intend to commit once v18
development begins.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v5-0001-Simplify-pg_sequences-a-bit.patch (3.2K, ../../20240517013335.GA1743971@nathanxps13/2-v5-0001-Simplify-pg_sequences-a-bit.patch)
download | inline diff:
From e9cba5e4303c7fa5ad2d7d5deb23fe0b1c740b09 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 7 May 2024 14:35:34 -0500
Subject: [PATCH v5 1/1] Simplify pg_sequences a bit.
XXX: NEEDS CATVERSION BUMP
---
src/backend/catalog/system_views.sql | 6 +-----
src/backend/commands/sequence.c | 12 ++++--------
src/test/regress/expected/rules.out | 5 +----
3 files changed, 6 insertions(+), 17 deletions(-)
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 53047cab5f..b32e5c3170 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -176,11 +176,7 @@ CREATE VIEW pg_sequences AS
S.seqincrement AS increment_by,
S.seqcycle AS cycle,
S.seqcache AS cache_size,
- CASE
- WHEN has_sequence_privilege(C.oid, 'SELECT,USAGE'::text)
- THEN pg_sequence_last_value(C.oid)
- ELSE NULL
- END AS last_value
+ pg_sequence_last_value(C.oid) AS last_value
FROM pg_sequence S JOIN pg_class C ON (C.oid = S.seqrelid)
LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
WHERE NOT pg_is_other_temp_schema(N.oid)
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 28f8522264..cd0e746577 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1783,21 +1783,17 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
- if (pg_class_aclcheck(relid, GetUserId(), ACL_SELECT | ACL_USAGE) != ACLCHECK_OK)
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied for sequence %s",
- RelationGetRelationName(seqrel))));
-
/*
* We return NULL for other sessions' temporary sequences. The
* pg_sequences system view already filters those out, but this offers a
* defense against ERRORs in case someone invokes this function directly.
*
* Also, for the benefit of the pg_sequences view, we return NULL for
- * unlogged sequences on standbys instead of throwing an error.
+ * unlogged sequences on standbys and for sequences for which we lack
+ * USAGE or SELECT privileges instead of throwing an error.
*/
- if (!RELATION_IS_OTHER_TEMP(seqrel) &&
+ if (pg_class_aclcheck(relid, GetUserId(), ACL_SELECT | ACL_USAGE) == ACLCHECK_OK &&
+ !RELATION_IS_OTHER_TEMP(seqrel) &&
(RelationIsPermanent(seqrel) || !RecoveryInProgress()))
{
Buffer buf;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index ef658ad740..04b3790bdd 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1699,10 +1699,7 @@ pg_sequences| SELECT n.nspname AS schemaname,
s.seqincrement AS increment_by,
s.seqcycle AS cycle,
s.seqcache AS cache_size,
- CASE
- WHEN has_sequence_privilege(c.oid, 'SELECT,USAGE'::text) THEN pg_sequence_last_value((c.oid)::regclass)
- ELSE NULL::bigint
- END AS last_value
+ pg_sequence_last_value((c.oid)::regclass) AS last_value
FROM ((pg_sequence s
JOIN pg_class c ON ((c.oid = s.seqrelid)))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
--
2.25.1
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: pg_sequence_last_value() for unlogged sequences on standbys
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
@ 2024-05-04 09:47 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 19+ messages in thread
From: Michael Paquier @ 2024-05-04 09:47 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
On Fri, May 03, 2024 at 03:49:08PM -0500, Nathan Bossart wrote:
> On Wed, May 01, 2024 at 12:39:53PM +0900, Michael Paquier wrote:
>> By the way, shouldn't we also change the function to return NULL for a
>> failed permission check? It would be possible to remove the
>> has_sequence_privilege() as well, thanks to that, and a duplication
>> between the code and the function view. I've been looking around a
>> bit, noticing one use of this function in check_pgactivity (nagios
>> agent), and its query also has a has_sequence_privilege() so returning
>> NULL would simplify its definition in the long-run. I'd suspect other
>> monitoring queries to do something similar to bypass permission
>> errors.
>
> I'm okay with that, but it would be v18 material that I'd track separately
> from the back-patchable fix proposed in this thread.
Of course. I mean only the permission check simplification for HEAD.
My apologies if my words were unclear.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 19+ messages in thread
end of thread, other threads:[~2024-05-17 01:33 UTC | newest]
Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-06-26 08:05 [PATCH v2 4/7] Row pattern recognition patch (executor). Tatsuo Ishii <[email protected]>
2024-05-01 00:57 pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 01:06 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-01 01:13 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 02:05 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-01 03:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-03 20:49 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-03 21:22 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-04 09:45 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-07 17:10 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 17:44 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-07 18:40 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-07 19:02 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Tom Lane <[email protected]>
2024-05-07 19:39 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-08 02:01 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[email protected]>
2024-05-10 21:00 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-13 21:01 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-17 01:33 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Nathan Bossart <[email protected]>
2024-05-04 09:47 ` Re: pg_sequence_last_value() for unlogged sequences on standbys Michael Paquier <[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