public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 1/2] Correct the check for pg_catalog.line in pg_upgrade
15+ messages / 7 participants
[nested] [flat]
* [PATCH 1/2] Correct the check for pg_catalog.line in pg_upgrade
@ 2019-10-14 23:16 Tomas Vondra <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Tomas Vondra @ 2019-10-14 23:16 UTC (permalink / raw)
The pg_upgrade check for pg_catalog.line data type when upgrading from
9.3 had a number of issues with domains and composite types. Firstly, it
detected even composite types unused in objects with storage. So for
example this was enough to trigger pg_upgrade failure:
CREATE TYPE line_composite AS (l pg_catalog.line)
On the other hand, this only happened with composite types directly on
the pg_catalog.line data type, but not with a domain. So this was not
detected
CREATE DOMAIN line_domain AS pg_catalog.line;
CREATE TYPE line_composite_2 AS (l line_domain);
unlike the first example. What's worse, we have not detected this even
when used in a table. So we missed cases like this:
CREATE TABLE t (l line_composite_2);
This fixes these false positives and false negatives by adopting the same
recursive CTE introduced by eaf900e842 for sql_identifier. Backpatch all
the way to 9.4, where the storage for pg_catalog.line data type changed.
Author: Tomas Vondra
Backpatch-to: 9.4-
Discussion: https://postgr.es/m/16045-673e8fa6b5ace196%40postgresql.org
---
src/bin/pg_upgrade/version.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
index 8375a46454..cfe69ea554 100644
--- a/src/bin/pg_upgrade/version.c
+++ b/src/bin/pg_upgrade/version.c
@@ -131,14 +131,41 @@ old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster)
DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
PGconn *conn = connectToServer(cluster, active_db->db_name);
+ /*
+ * We need the recursive CTE because the pg_catalog.line may be wrapped
+ * either in a domain or composite type, or both (9.3 did not allow domains
+ * on composite types, but there may be multi-level composite type).
+ */
res = executeQueryOrDie(conn,
+ "WITH RECURSIVE oids AS ( "
+ /* the pg_catalog.line type itself */
+ " SELECT 'pg_catalog.line'::pg_catalog.regtype AS oid "
+ " UNION ALL "
+ " SELECT * FROM ( "
+ /* domains on the type */
+ " WITH x AS (SELECT oid FROM oids) "
+ " SELECT t.oid FROM pg_catalog.pg_type t, x WHERE typbasetype = x.oid AND typtype = 'd' "
+ " UNION "
+ /* composite types containing the type */
+ " SELECT t.oid FROM pg_catalog.pg_type t, pg_catalog.pg_class c, pg_catalog.pg_attribute a, x "
+ " WHERE t.typtype = 'c' AND "
+ " t.oid = c.reltype AND "
+ " c.oid = a.attrelid AND "
+ " NOT a.attisdropped AND "
+ " a.atttypid = x.oid "
+ " ) foo "
+ ") "
"SELECT n.nspname, c.relname, a.attname "
"FROM pg_catalog.pg_class c, "
" pg_catalog.pg_namespace n, "
" pg_catalog.pg_attribute a "
"WHERE c.oid = a.attrelid AND "
" NOT a.attisdropped AND "
- " a.atttypid = 'pg_catalog.line'::pg_catalog.regtype AND "
+ " a.atttypid IN (SELECT oid FROM oids) AND "
+ " c.relkind IN ("
+ CppAsString2(RELKIND_RELATION) ", "
+ CppAsString2(RELKIND_MATVIEW) ", "
+ CppAsString2(RELKIND_INDEX) ") AND "
" c.relnamespace = n.oid AND "
/* exclude possible orphaned temp tables */
" n.nspname !~ '^pg_temp_' AND "
--
2.21.0
--sspr6rxkpcihpkly
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename="0002-Correct-the-check-for-pg_catalog.unknown-in-pg_upgra.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v5 1/7] Row pattern recognition patch for raw parser.
@ 2023-09-02 06:32 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Tatsuo Ishii @ 2023-09-02 06:32 UTC (permalink / raw)
---
src/backend/parser/gram.y | 216 +++++++++++++++++++++++++++++---
src/include/nodes/parsenodes.h | 56 +++++++++
src/include/parser/kwlist.h | 8 ++
src/include/parser/parse_node.h | 1 +
4 files changed, 267 insertions(+), 14 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7d2032885e..70409cdc9a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -251,6 +251,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
DefElem *defelt;
SortBy *sortby;
WindowDef *windef;
+ RPCommonSyntax *rpcom;
+ RPSubsetItem *rpsubset;
JoinExpr *jexpr;
IndexElem *ielem;
StatsElem *selem;
@@ -453,8 +455,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
TriggerTransitions TriggerReferencing
vacuum_relation_list opt_vacuum_relation_list
drop_option_list pub_obj_list
-
-%type <node> opt_routine_body
+ row_pattern_measure_list row_pattern_definition_list
+ opt_row_pattern_subset_clause
+ row_pattern_subset_list row_pattern_subset_rhs
+ row_pattern
+%type <rpsubset> row_pattern_subset_item
+%type <node> opt_routine_body row_pattern_term
%type <groupclause> group_clause
%type <list> group_by_list
%type <node> group_by_item empty_grouping_set rollup_clause cube_clause
@@ -551,6 +557,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <range> relation_expr_opt_alias
%type <node> tablesample_clause opt_repeatable_clause
%type <target> target_el set_target insert_column_item
+ row_pattern_measure_item row_pattern_definition
%type <str> generic_option_name
%type <node> generic_option_arg
@@ -633,6 +640,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <list> window_clause window_definition_list opt_partition_clause
%type <windef> window_definition over_clause window_specification
opt_frame_clause frame_extent frame_bound
+%type <rpcom> opt_row_pattern_common_syntax opt_row_pattern_skip_to
+%type <boolean> opt_row_pattern_initial_or_seek
+%type <list> opt_row_pattern_measures
%type <ival> opt_window_exclusion_clause
%type <str> opt_existing_window_name
%type <boolean> opt_if_not_exists
@@ -659,7 +669,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
json_object_constructor_null_clause_opt
json_array_constructor_null_clause_opt
-
/*
* Non-keyword token types. These are hard-wired into the "flex" lexer.
* They must be listed first so that their numeric codes do not depend on
@@ -702,7 +711,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
CURRENT_TIME CURRENT_TIMESTAMP CURRENT_USER CURSOR CYCLE
DATA_P DATABASE DAY_P DEALLOCATE DEC DECIMAL_P DECLARE DEFAULT DEFAULTS
- DEFERRABLE DEFERRED DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC
+ DEFERRABLE DEFERRED DEFINE DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC
DETACH DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P
DOUBLE_P DROP
@@ -718,7 +727,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
HANDLER HAVING HEADER_P HOLD HOUR_P
IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE
- INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P
+ INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIAL INITIALLY INLINE_P
INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER
INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION
@@ -731,7 +740,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
LEADING LEAKPROOF LEAST LEFT LEVEL LIKE LIMIT LISTEN LOAD LOCAL
LOCALTIME LOCALTIMESTAMP LOCATION LOCK_P LOCKED LOGGED
- MAPPING MATCH MATCHED MATERIALIZED MAXVALUE MERGE METHOD
+ MAPPING MATCH MATCHED MATERIALIZED MAXVALUE MEASURES MERGE METHOD
MINUTE_P MINVALUE MODE MONTH_P MOVE
NAME_P NAMES NATIONAL NATURAL NCHAR NEW NEXT NFC NFD NFKC NFKD NO NONE
@@ -743,8 +752,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
ORDER ORDINALITY OTHERS OUT_P OUTER_P
OVER OVERLAPS OVERLAY OVERRIDING OWNED OWNER
- PARALLEL PARAMETER PARSER PARTIAL PARTITION PASSING PASSWORD
- PLACING PLANS POLICY
+ PARALLEL PARAMETER PARSER PARTIAL PARTITION PASSING PASSWORD PAST
+ PATTERN_P PERMUTE PLACING PLANS POLICY
POSITION PRECEDING PRECISION PRESERVE PREPARE PREPARED PRIMARY
PRIOR PRIVILEGES PROCEDURAL PROCEDURE PROCEDURES PROGRAM PUBLICATION
@@ -755,12 +764,13 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
RESET RESTART RESTRICT RETURN RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROLLUP
ROUTINE ROUTINES ROW ROWS RULE
- SAVEPOINT SCALAR SCHEMA SCHEMAS SCROLL SEARCH SECOND_P SECURITY SELECT
+ SAVEPOINT SCALAR SCHEMA SCHEMAS SCROLL SEARCH SECOND_P SECURITY SEEK SELECT
SEQUENCE SEQUENCES
+
SERIALIZABLE SERVER SESSION SESSION_USER SET SETS SETOF SHARE SHOW
SIMILAR SIMPLE SKIP SMALLINT SNAPSHOT SOME SQL_P STABLE STANDALONE_P
START STATEMENT STATISTICS STDIN STDOUT STORAGE STORED STRICT_P STRIP_P
- SUBSCRIPTION SUBSTRING SUPPORT SYMMETRIC SYSID SYSTEM_P SYSTEM_USER
+ SUBSCRIPTION SUBSET SUBSTRING SUPPORT SYMMETRIC SYSID SYSTEM_P SYSTEM_USER
TABLE TABLES TABLESAMPLE TABLESPACE TEMP TEMPLATE TEMPORARY TEXT_P THEN
TIES TIME TIMESTAMP TO TRAILING TRANSACTION TRANSFORM
@@ -853,6 +863,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
*/
%nonassoc UNBOUNDED /* ideally would have same precedence as IDENT */
%nonassoc IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
+%nonassoc MEASURES AFTER INITIAL SEEK PATTERN_P
%left Op OPERATOR /* multi-character ops and user-defined operators */
%left '+' '-'
%left '*' '/' '%'
@@ -15857,7 +15868,8 @@ over_clause: OVER window_specification
;
window_specification: '(' opt_existing_window_name opt_partition_clause
- opt_sort_clause opt_frame_clause ')'
+ opt_sort_clause opt_row_pattern_measures opt_frame_clause
+ opt_row_pattern_common_syntax ')'
{
WindowDef *n = makeNode(WindowDef);
@@ -15865,10 +15877,12 @@ window_specification: '(' opt_existing_window_name opt_partition_clause
n->refname = $2;
n->partitionClause = $3;
n->orderClause = $4;
+ n->rowPatternMeasures = $5;
/* copy relevant fields of opt_frame_clause */
- n->frameOptions = $5->frameOptions;
- n->startOffset = $5->startOffset;
- n->endOffset = $5->endOffset;
+ n->frameOptions = $6->frameOptions;
+ n->startOffset = $6->startOffset;
+ n->endOffset = $6->endOffset;
+ n->rpCommonSyntax = $7;
n->location = @1;
$$ = n;
}
@@ -15892,6 +15906,31 @@ opt_partition_clause: PARTITION BY expr_list { $$ = $3; }
| /*EMPTY*/ { $$ = NIL; }
;
+/*
+ * ROW PATTERN_P MEASURES
+ */
+opt_row_pattern_measures: MEASURES row_pattern_measure_list { $$ = $2; }
+ | /*EMPTY*/ { $$ = NIL; }
+ ;
+
+row_pattern_measure_list:
+ row_pattern_measure_item
+ { $$ = list_make1($1); }
+ | row_pattern_measure_list ',' row_pattern_measure_item
+ { $$ = lappend($1, $3); }
+ ;
+
+row_pattern_measure_item:
+ a_expr AS ColLabel
+ {
+ $$ = makeNode(ResTarget);
+ $$->name = $3;
+ $$->indirection = NIL;
+ $$->val = (Node *) $1;
+ $$->location = @1;
+ }
+ ;
+
/*
* For frame clauses, we return a WindowDef, but only some fields are used:
* frameOptions, startOffset, and endOffset.
@@ -16051,6 +16090,139 @@ opt_window_exclusion_clause:
| /*EMPTY*/ { $$ = 0; }
;
+opt_row_pattern_common_syntax:
+opt_row_pattern_skip_to opt_row_pattern_initial_or_seek
+ PATTERN_P '(' row_pattern ')'
+ opt_row_pattern_subset_clause
+ DEFINE row_pattern_definition_list
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ n->rpSkipTo = $1->rpSkipTo;
+ n->rpSkipVariable = $1->rpSkipVariable;
+ n->initial = $2;
+ n->rpPatterns = $5;
+ n->rpSubsetClause = $7;
+ n->rpDefs = $9;
+ $$ = n;
+ }
+ | /*EMPTY*/ { $$ = NULL; }
+ ;
+
+opt_row_pattern_skip_to:
+ AFTER MATCH SKIP TO NEXT ROW
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ n->rpSkipTo = ST_NEXT_ROW;
+ n->rpSkipVariable = NULL;
+ $$ = n;
+ }
+ | AFTER MATCH SKIP PAST LAST_P ROW
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ n->rpSkipTo = ST_PAST_LAST_ROW;
+ n->rpSkipVariable = NULL;
+ $$ = n;
+ }
+/*
+ | AFTER MATCH SKIP TO FIRST_P ColId %prec FIRST_P
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ n->rpSkipTo = ST_FIRST_VARIABLE;
+ n->rpSkipVariable = $6;
+ $$ = n;
+ }
+ | AFTER MATCH SKIP TO LAST_P ColId %prec LAST_P
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ n->rpSkipTo = ST_LAST_VARIABLE;
+ n->rpSkipVariable = $6;
+ $$ = n;
+ }
+ * Shift/reduce
+ | AFTER MATCH SKIP TO ColId
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ n->rpSkipTo = ST_VARIABLE;
+ n->rpSkipVariable = $5;
+ $$ = n;
+ }
+*/
+ | /*EMPTY*/
+ {
+ RPCommonSyntax *n = makeNode(RPCommonSyntax);
+ /* temporary set default to ST_NEXT_ROW */
+ n->rpSkipTo = ST_PAST_LAST_ROW;
+ n->rpSkipVariable = NULL;
+ $$ = n;
+ }
+ ;
+
+opt_row_pattern_initial_or_seek:
+ INITIAL { $$ = true; }
+ | SEEK
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("SEEK is not supported"),
+ errhint("Use INITIAL."),
+ parser_errposition(@1)));
+ }
+ | /*EMPTY*/ { $$ = true; }
+ ;
+
+row_pattern:
+ row_pattern_term { $$ = list_make1($1); }
+ | row_pattern row_pattern_term { $$ = lappend($1, $2); }
+ ;
+
+row_pattern_term:
+ ColId { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "", (Node *)makeString($1), NULL, @1); }
+ | ColId '*' { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "*", (Node *)makeString($1), NULL, @1); }
+ | ColId '+' { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "+", (Node *)makeString($1), NULL, @1); }
+ | ColId '?' { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "?", (Node *)makeString($1), NULL, @1); }
+ ;
+
+opt_row_pattern_subset_clause:
+ SUBSET row_pattern_subset_list { $$ = $2; }
+ | /*EMPTY*/ { $$ = NIL; }
+ ;
+
+row_pattern_subset_list:
+ row_pattern_subset_item { $$ = list_make1($1); }
+ | row_pattern_subset_list ',' row_pattern_subset_item { $$ = lappend($1, $3); }
+ | /*EMPTY*/ { $$ = NIL; }
+ ;
+
+row_pattern_subset_item: ColId '=' '(' row_pattern_subset_rhs ')'
+ {
+ RPSubsetItem *n = makeNode(RPSubsetItem);
+ n->name = $1;
+ n->rhsVariable = $4;
+ $$ = n;
+ }
+ ;
+
+row_pattern_subset_rhs:
+ ColId { $$ = list_make1(makeStringConst($1, @1)); }
+ | row_pattern_subset_rhs ',' ColId { $$ = lappend($1, makeStringConst($3, @1)); }
+ | /*EMPTY*/ { $$ = NIL; }
+ ;
+
+row_pattern_definition_list:
+ row_pattern_definition { $$ = list_make1($1); }
+ | row_pattern_definition_list ',' row_pattern_definition { $$ = lappend($1, $3); }
+ ;
+
+row_pattern_definition:
+ ColId AS a_expr
+ {
+ $$ = makeNode(ResTarget);
+ $$->name = $1;
+ $$->indirection = NIL;
+ $$->val = (Node *) $3;
+ $$->location = @1;
+ }
+ ;
/*
* Supporting nonterminals for expressions.
@@ -17146,6 +17318,7 @@ unreserved_keyword:
| INDEXES
| INHERIT
| INHERITS
+ | INITIAL
| INLINE_P
| INPUT_P
| INSENSITIVE
@@ -17173,6 +17346,7 @@ unreserved_keyword:
| MATCHED
| MATERIALIZED
| MAXVALUE
+ | MEASURES
| MERGE
| METHOD
| MINUTE_P
@@ -17215,6 +17389,9 @@ unreserved_keyword:
| PARTITION
| PASSING
| PASSWORD
+ | PAST
+ | PATTERN_P
+ | PERMUTE
| PLANS
| POLICY
| PRECEDING
@@ -17265,6 +17442,7 @@ unreserved_keyword:
| SEARCH
| SECOND_P
| SECURITY
+ | SEEK
| SEQUENCE
| SEQUENCES
| SERIALIZABLE
@@ -17290,6 +17468,7 @@ unreserved_keyword:
| STRICT_P
| STRIP_P
| SUBSCRIPTION
+ | SUBSET
| SUPPORT
| SYSID
| SYSTEM_P
@@ -17477,6 +17656,7 @@ reserved_keyword:
| CURRENT_USER
| DEFAULT
| DEFERRABLE
+ | DEFINE
| DESC
| DISTINCT
| DO
@@ -17639,6 +17819,7 @@ bare_label_keyword:
| DEFAULTS
| DEFERRABLE
| DEFERRED
+ | DEFINE
| DEFINER
| DELETE_P
| DELIMITER
@@ -17714,6 +17895,7 @@ bare_label_keyword:
| INDEXES
| INHERIT
| INHERITS
+ | INITIAL
| INITIALLY
| INLINE_P
| INNER_P
@@ -17763,6 +17945,7 @@ bare_label_keyword:
| MATCHED
| MATERIALIZED
| MAXVALUE
+ | MEASURES
| MERGE
| METHOD
| MINVALUE
@@ -17816,6 +17999,9 @@ bare_label_keyword:
| PARTITION
| PASSING
| PASSWORD
+ | PAST
+ | PATTERN_P
+ | PERMUTE
| PLACING
| PLANS
| POLICY
@@ -17872,6 +18058,7 @@ bare_label_keyword:
| SCROLL
| SEARCH
| SECURITY
+ | SEEK
| SELECT
| SEQUENCE
| SEQUENCES
@@ -17903,6 +18090,7 @@ bare_label_keyword:
| STRICT_P
| STRIP_P
| SUBSCRIPTION
+ | SUBSET
| SUBSTRING
| SUPPORT
| SYMMETRIC
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index fef4c714b8..4cc1fb417b 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -547,6 +547,44 @@ typedef struct SortBy
int location; /* operator location, or -1 if none/unknown */
} SortBy;
+/*
+ * AFTER MATCH row pattern skip to types in row pattern common syntax
+ */
+typedef enum RPSkipTo
+{
+ ST_NONE, /* AFTER MATCH omitted */
+ ST_NEXT_ROW, /* SKIP TO NEXT ROW */
+ ST_PAST_LAST_ROW, /* SKIP TO PAST LAST ROW */
+ ST_FIRST_VARIABLE, /* SKIP TO FIRST variable name */
+ ST_LAST_VARIABLE, /* SKIP TO LAST variable name */
+ ST_VARIABLE /* SKIP TO variable name */
+} RPSkipTo;
+
+/*
+ * Row Pattern SUBSET clause item
+ */
+typedef struct RPSubsetItem
+{
+ NodeTag type;
+ char *name; /* Row Pattern SUBSET clause variable name */
+ List *rhsVariable; /* Row Pattern SUBSET rhs variables (list of char *string) */
+} RPSubsetItem;
+
+/*
+ * RowPatternCommonSyntax - raw representation of row pattern common syntax
+ *
+ */
+typedef struct RPCommonSyntax
+{
+ NodeTag type;
+ RPSkipTo rpSkipTo; /* Row Pattern AFTER MATCH SKIP type */
+ char *rpSkipVariable; /* Row Pattern Skip To variable name, if any */
+ bool initial; /* true if <row pattern initial or seek> is initial */
+ List *rpPatterns; /* PATTERN variables (list of A_Expr) */
+ List *rpSubsetClause; /* row pattern subset clause (list of RPSubsetItem), if any */
+ List *rpDefs; /* row pattern definitions clause (list of ResTarget) */
+} RPCommonSyntax;
+
/*
* WindowDef - raw representation of WINDOW and OVER clauses
*
@@ -562,6 +600,8 @@ typedef struct WindowDef
char *refname; /* referenced window name, if any */
List *partitionClause; /* PARTITION BY expression list */
List *orderClause; /* ORDER BY (list of SortBy) */
+ List *rowPatternMeasures; /* row pattern measures (list of ResTarget) */
+ RPCommonSyntax *rpCommonSyntax; /* row pattern common syntax */
int frameOptions; /* frame_clause options, see below */
Node *startOffset; /* expression for starting bound, if any */
Node *endOffset; /* expression for ending bound, if any */
@@ -1483,6 +1523,11 @@ typedef struct GroupingSet
* the orderClause might or might not be copied (see copiedOrder); the framing
* options are never copied, per spec.
*
+ * "defineClause" is Row Pattern Recognition DEFINE clause (list of
+ * TargetEntry). TargetEntry.resname represents row pattern definition
+ * variable name. "patternVariable" and "patternRegexp" represents PATTERN
+ * clause.
+ *
* The information relevant for the query jumbling is the partition clause
* type and its bounds.
*/
@@ -1514,6 +1559,17 @@ typedef struct WindowClause
Index winref; /* ID referenced by window functions */
/* did we copy orderClause from refname? */
bool copiedOrder pg_node_attr(query_jumble_ignore);
+ /* Row Pattern AFTER MACH SKIP clause */
+ RPSkipTo rpSkipTo; /* Row Pattern Skip To type */
+ bool initial; /* true if <row pattern initial or seek> is initial */
+ /* Row Pattern DEFINE clause (list of TargetEntry) */
+ List *defineClause;
+ /* Row Pattern DEFINE variable initial names (list of String) */
+ List *defineInitial;
+ /* Row Pattern PATTERN variable name (list of String) */
+ List *patternVariable;
+ /* Row Pattern PATTERN regular expression quantifier ('+' or ''. list of String) */
+ List *patternRegexp;
} WindowClause;
/*
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index 5984dcfa4b..2804333b53 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -128,6 +128,7 @@ PG_KEYWORD("default", DEFAULT, RESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("defaults", DEFAULTS, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("deferrable", DEFERRABLE, RESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("deferred", DEFERRED, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("define", DEFINE, RESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("definer", DEFINER, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("delete", DELETE_P, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("delimiter", DELIMITER, UNRESERVED_KEYWORD, BARE_LABEL)
@@ -212,6 +213,7 @@ PG_KEYWORD("index", INDEX, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("indexes", INDEXES, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("inherit", INHERIT, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("inherits", INHERITS, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("initial", INITIAL, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("initially", INITIALLY, RESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("inline", INLINE_P, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("inner", INNER_P, TYPE_FUNC_NAME_KEYWORD, BARE_LABEL)
@@ -265,6 +267,7 @@ PG_KEYWORD("match", MATCH, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("matched", MATCHED, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("materialized", MATERIALIZED, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("maxvalue", MAXVALUE, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("measures", MEASURES, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("merge", MERGE, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("method", METHOD, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("minute", MINUTE_P, UNRESERVED_KEYWORD, AS_LABEL)
@@ -326,6 +329,9 @@ PG_KEYWORD("partial", PARTIAL, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("partition", PARTITION, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("passing", PASSING, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("password", PASSWORD, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("past", PAST, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("pattern", PATTERN_P, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("permute", PERMUTE, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("placing", PLACING, RESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("plans", PLANS, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("policy", POLICY, UNRESERVED_KEYWORD, BARE_LABEL)
@@ -385,6 +391,7 @@ PG_KEYWORD("scroll", SCROLL, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("search", SEARCH, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("second", SECOND_P, UNRESERVED_KEYWORD, AS_LABEL)
PG_KEYWORD("security", SECURITY, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("seek", SEEK, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("select", SELECT, RESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("sequence", SEQUENCE, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("sequences", SEQUENCES, UNRESERVED_KEYWORD, BARE_LABEL)
@@ -416,6 +423,7 @@ PG_KEYWORD("stored", STORED, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("strict", STRICT_P, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("strip", STRIP_P, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("subscription", SUBSCRIPTION, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("subset", SUBSET, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("substring", SUBSTRING, COL_NAME_KEYWORD, BARE_LABEL)
PG_KEYWORD("support", SUPPORT, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("symmetric", SYMMETRIC, RESERVED_KEYWORD, BARE_LABEL)
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index f589112d5e..6640090910 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -51,6 +51,7 @@ typedef enum ParseExprKind
EXPR_KIND_WINDOW_FRAME_RANGE, /* window frame clause with RANGE */
EXPR_KIND_WINDOW_FRAME_ROWS, /* window frame clause with ROWS */
EXPR_KIND_WINDOW_FRAME_GROUPS, /* window frame clause with GROUPS */
+ EXPR_KIND_RPR_DEFINE, /* DEFINE */
EXPR_KIND_SELECT_TARGET, /* SELECT target list item */
EXPR_KIND_INSERT_TARGET, /* INSERT target list item */
EXPR_KIND_UPDATE_SOURCE, /* UPDATE assignment source item */
--
2.25.1
----Next_Part(Sat_Sep__2_15_52_35_2023_273)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v5-0002-Row-pattern-recognition-patch-parse-analysis.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: locked reads for atomics
@ 2023-11-11 02:55 Nathan Bossart <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Nathan Bossart @ 2023-11-11 02:55 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers
On Fri, Nov 10, 2023 at 06:48:39PM -0800, Andres Freund wrote:
> Yes. We should optimize pg_atomic_exchange_u32() one of these days - it can be
> done *far* faster than a cmpxchg. When I was adding the atomic abstraction
> there was concern with utilizing too many different atomic instructions. I
> didn't really agree back then, but these days I really don't see a reason to
> not use a few more intrinsics.
I might give this a try, if for no other reason than it'd force me to
improve my mental model of this stuff. :)
>> It refers to "unlocked writes," but this isn't
>> pg_atomic_unlocked_write_u32_impl(). The original thread for this comment
>> [0] doesn't offer any hints, either. Does "unlocked" mean something
>> different here, such as "write without any barrier semantics?"
>
> It's just about not using the spinlock. If we were to *not* use a spinlock
> here, we'd break pg_atomic_compare_exchange_u32(), because the
> spinlock-implementation of pg_atomic_compare_exchange_u32() needs to actually
> be able to rely on no concurrent changes to the value to happen.
Thanks for clarifying. I thought it might've been hinting at something
beyond the compare/exchange implications.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: locked reads for atomics
@ 2023-11-27 21:00 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 2 replies; 15+ messages in thread
From: Nathan Bossart @ 2023-11-27 21:00 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers
Here's a v2 of the patch set in which I've attempted to address all
feedback. I've also added a pg_write_membarrier_u* pair of functions that
provide an easy way to write to an atomic variable with full barrier
semantics. In the generic implementation, these are just aliases for an
atomic exchange.
0002 demonstrates how these functions might be used to eliminate the
arch_lck spinlock, which is only ever used for one boolean variable. My
hope is that the membarrier functions make eliminating spinlocks for
non-performance-sensitive code easy to reason about.
(We might be able to use a pg_atomic_flag instead for 0002, but that code
seems intended for a slightly different use-case and has more complicated
barrier semantics.)
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v2-0001-add-membarrier-operations.patch (5.0K, ../../20231127210030.GA140335@nathanxps13/2-v2-0001-add-membarrier-operations.patch)
download | inline diff:
From 53f717e84a3aa912f9d89dce4be7f77d9bffb3c2 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 24 Nov 2023 20:21:12 -0600
Subject: [PATCH v2 1/2] add membarrier operations
---
src/include/port/atomics.h | 59 ++++++++++++++++++++++++++++++
src/include/port/atomics/generic.h | 36 ++++++++++++++++++
2 files changed, 95 insertions(+)
diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index bbff945eba..a889f0236e 100644
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -239,6 +239,26 @@ pg_atomic_read_u32(volatile pg_atomic_uint32 *ptr)
return pg_atomic_read_u32_impl(ptr);
}
+/*
+ * pg_atomic_read_membarrier_u32 - read with barrier semantics.
+ *
+ * This read is guaranteed to return the current value, provided that the value
+ * is only ever updated via operations with barrier semantics, such as
+ * pg_atomic_compare_exchange_u32() and pg_atomic_write_membarrier_u32(). Note
+ * that this is less performant than pg_atomic_read_u32(), but it may be easier
+ * to reason about correctness with this function in less performance-sensitive
+ * code.
+ *
+ * Full barrier semantics.
+ */
+static inline uint32
+pg_atomic_read_membarrier_u32(volatile pg_atomic_uint32 *ptr)
+{
+ AssertPointerAlignment(ptr, 4);
+
+ return pg_atomic_read_membarrier_u32_impl(ptr);
+}
+
/*
* pg_atomic_write_u32 - write to atomic variable.
*
@@ -276,6 +296,27 @@ pg_atomic_unlocked_write_u32(volatile pg_atomic_uint32 *ptr, uint32 val)
pg_atomic_unlocked_write_u32_impl(ptr, val);
}
+/*
+ * pg_atomic_write_membarrier_u32 - write with barrier semantics.
+ *
+ * The write is guaranteed to succeed as a whole, i.e., it's not possible to
+ * observe a partial write for any reader. Note that this correctly interacts
+ * with both pg_atomic_compare_exchange_u32() and
+ * pg_atomic_read_membarrier_u32(). While this may be less performant than
+ * pg_atomic_write_u32() and pg_atomic_unlocked_write_u32(), it may be easier
+ * to reason about correctness with this function in less performance-sensitive
+ * code.
+ *
+ * Full barrier semantics.
+ */
+static inline void
+pg_atomic_write_membarrier_u32(volatile pg_atomic_uint32 *ptr, uint32 val)
+{
+ AssertPointerAlignment(ptr, 4);
+
+ pg_atomic_write_membarrier_u32_impl(ptr, val);
+}
+
/*
* pg_atomic_exchange_u32 - exchange newval with current value
*
@@ -429,6 +470,15 @@ pg_atomic_read_u64(volatile pg_atomic_uint64 *ptr)
return pg_atomic_read_u64_impl(ptr);
}
+static inline uint64
+pg_atomic_read_membarrier_u64(volatile pg_atomic_uint64 *ptr)
+{
+#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
+ AssertPointerAlignment(ptr, 8);
+#endif
+ return pg_atomic_read_membarrier_u64_impl(ptr);
+}
+
static inline void
pg_atomic_write_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
{
@@ -438,6 +488,15 @@ pg_atomic_write_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
pg_atomic_write_u64_impl(ptr, val);
}
+static inline void
+pg_atomic_write_membarrier_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
+{
+#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
+ AssertPointerAlignment(ptr, 8);
+#endif
+ pg_atomic_write_membarrier_u64_impl(ptr, val);
+}
+
static inline uint64
pg_atomic_exchange_u64(volatile pg_atomic_uint64 *ptr, uint64 newval)
{
diff --git a/src/include/port/atomics/generic.h b/src/include/port/atomics/generic.h
index 95d99dd0be..9bcc3b48ce 100644
--- a/src/include/port/atomics/generic.h
+++ b/src/include/port/atomics/generic.h
@@ -243,6 +243,24 @@ pg_atomic_sub_fetch_u32_impl(volatile pg_atomic_uint32 *ptr, int32 sub_)
}
#endif
+#if !defined(PG_HAVE_ATOMIC_READ_MEMBARRIER_U32) && defined(PG_HAVE_ATOMIC_FETCH_ADD_U32)
+#define PG_HAVE_ATOMIC_READ_MEMBARRIER_U32
+static inline uint32
+pg_atomic_read_membarrier_u32_impl(volatile pg_atomic_uint32 *ptr)
+{
+ return pg_atomic_fetch_add_u32_impl(ptr, 0);
+}
+#endif
+
+#if !defined(PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U32) && defined(PG_HAVE_ATOMIC_EXCHANGE_U32)
+#define PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U32
+static inline void
+pg_atomic_write_membarrier_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val)
+{
+ (void) pg_atomic_exchange_u32_impl(ptr, val);
+}
+#endif
+
#if !defined(PG_HAVE_ATOMIC_EXCHANGE_U64) && defined(PG_HAVE_ATOMIC_COMPARE_EXCHANGE_U64)
#define PG_HAVE_ATOMIC_EXCHANGE_U64
static inline uint64
@@ -399,3 +417,21 @@ pg_atomic_sub_fetch_u64_impl(volatile pg_atomic_uint64 *ptr, int64 sub_)
return pg_atomic_fetch_sub_u64_impl(ptr, sub_) - sub_;
}
#endif
+
+#if !defined(PG_HAVE_ATOMIC_READ_MEMBARRIER_U64) && defined(PG_HAVE_ATOMIC_FETCH_ADD_U64)
+#define PG_HAVE_ATOMIC_READ_MEMBARRIER_U64
+static inline uint64
+pg_atomic_read_membarrier_u64_impl(volatile pg_atomic_uint64 *ptr)
+{
+ return pg_atomic_fetch_add_u64_impl(ptr, 0);
+}
+#endif
+
+#if !defined(PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U64) && defined(PG_HAVE_ATOMIC_EXCHANGE_U64)
+#define PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U64
+static inline void
+pg_atomic_write_membarrier_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 val)
+{
+ (void) pg_atomic_exchange_u64_impl(ptr, val);
+}
+#endif
--
2.25.1
[text/x-diff] v2-0002-replace-force_dir_scan-with-atomics.patch (2.2K, ../../20231127210030.GA140335@nathanxps13/3-v2-0002-replace-force_dir_scan-with-atomics.patch)
download | inline diff:
From baafbb3c1cbb1665be19e8d06172840bcb07c820 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Mon, 27 Nov 2023 12:47:06 -0600
Subject: [PATCH v2 2/2] replace force_dir_scan with atomics
---
src/backend/postmaster/pgarch.c | 21 +++++----------------
1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 46af349564..dccefb160a 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -45,7 +45,6 @@
#include "storage/proc.h"
#include "storage/procsignal.h"
#include "storage/shmem.h"
-#include "storage/spin.h"
#include "utils/guc.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
@@ -83,11 +82,9 @@ typedef struct PgArchData
int pgprocno; /* pgprocno of archiver process */
/*
- * Forces a directory scan in pgarch_readyXlog(). Protected by arch_lck.
+ * Forces a directory scan in pgarch_readyXlog().
*/
- bool force_dir_scan;
-
- slock_t arch_lck;
+ pg_atomic_uint32 force_dir_scan;
} PgArchData;
char *XLogArchiveLibrary = "";
@@ -174,7 +171,7 @@ PgArchShmemInit(void)
/* First time through, so initialize */
MemSet(PgArch, 0, PgArchShmemSize());
PgArch->pgprocno = INVALID_PGPROCNO;
- SpinLockInit(&PgArch->arch_lck);
+ pg_atomic_init_u32(&PgArch->force_dir_scan, false);
}
}
@@ -549,18 +546,12 @@ pgarch_readyXlog(char *xlog)
char XLogArchiveStatusDir[MAXPGPATH];
DIR *rldir;
struct dirent *rlde;
- bool force_dir_scan;
/*
* If a directory scan was requested, clear the stored file names and
* proceed.
*/
- SpinLockAcquire(&PgArch->arch_lck);
- force_dir_scan = PgArch->force_dir_scan;
- PgArch->force_dir_scan = false;
- SpinLockRelease(&PgArch->arch_lck);
-
- if (force_dir_scan)
+ if (pg_atomic_exchange_u32(&PgArch->force_dir_scan, false))
arch_files->arch_files_size = 0;
/*
@@ -711,9 +702,7 @@ ready_file_comparator(Datum a, Datum b, void *arg)
void
PgArchForceDirScan(void)
{
- SpinLockAcquire(&PgArch->arch_lck);
- PgArch->force_dir_scan = true;
- SpinLockRelease(&PgArch->arch_lck);
+ pg_atomic_write_membarrier_u32(&PgArch->force_dir_scan, true);
}
/*
--
2.25.1
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: locked reads for atomics
@ 2024-01-17 03:48 Li, Yong <[email protected]>
parent: Nathan Bossart <[email protected]>
1 sibling, 0 replies; 15+ messages in thread
From: Li, Yong @ 2024-01-17 03:48 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers
> On Nov 28, 2023, at 05:00, Nathan Bossart <[email protected]> wrote:
>
> External Email
>
> Here's a v2 of the patch set in which I've attempted to address all
> feedback. I've also added a pg_write_membarrier_u* pair of functions that
> provide an easy way to write to an atomic variable with full barrier
> semantics. In the generic implementation, these are just aliases for an
> atomic exchange.
>
> 0002 demonstrates how these functions might be used to eliminate the
> arch_lck spinlock, which is only ever used for one boolean variable. My
> hope is that the membarrier functions make eliminating spinlocks for
> non-performance-sensitive code easy to reason about.
>
> (We might be able to use a pg_atomic_flag instead for 0002, but that code
> seems intended for a slightly different use-case and has more complicated
> barrier semantics.)
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
Hi Nathan,
The patch looks good to me.
The patch adds two pairs of atomic functions that provide full-barrier semantics to atomic read/write operations. The patch also includes an example of how this new functions can be used to replace spin locks.
The patch applies cleanly to HEAD. “make check-world” also runs cleanly with no error. I am moving it to Ready for Committers.
Regards,
Yong
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: locked reads for atomics
@ 2024-02-22 07:28 Bharath Rupireddy <[email protected]>
parent: Nathan Bossart <[email protected]>
1 sibling, 1 reply; 15+ messages in thread
From: Bharath Rupireddy @ 2024-02-22 07:28 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers
On Tue, Nov 28, 2023 at 2:30 AM Nathan Bossart <[email protected]> wrote:
>
> Here's a v2 of the patch set in which I've attempted to address all
> feedback. I've also added a pg_write_membarrier_u* pair of functions that
There's some immediate use for reads/writes with barrier semantics -
https://www.postgresql.org/message-id/CALj2ACXrePj4E6ocKr-%2Bb%3DrjT-8yeMmHnEeWQP1bc-WXETfTVw%40mail....
Any plan for taking this forward?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: locked reads for atomics
@ 2024-02-22 19:53 Jeff Davis <[email protected]>
parent: Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jeff Davis @ 2024-02-22 19:53 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers
On Thu, 2024-02-22 at 12:58 +0530, Bharath Rupireddy wrote:
> There's some immediate use for reads/writes with barrier semantics -
Is this mainly a convenience for safety/readability? Or is it faster in
some cases than doing an atomic access with separate memory barriers?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: locked reads for atomics
@ 2024-02-23 16:17 Nathan Bossart <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Nathan Bossart @ 2024-02-23 16:17 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
On Thu, Feb 22, 2024 at 11:53:50AM -0800, Jeff Davis wrote:
> On Thu, 2024-02-22 at 12:58 +0530, Bharath Rupireddy wrote:
>> There's some immediate use for reads/writes with barrier semantics -
>
> Is this mainly a convenience for safety/readability? Or is it faster in
> some cases than doing an atomic access with separate memory barriers?
The former. Besides the 0002 patch tracked here, there's at least one
other patch [0] that could probably use these new functions. The idea is
to provide an easy way to remove spinlocks, etc. and use atomics for less
performance-sensitive stuff. The implementations are intended to be
relatively inexpensive and might continue to improve in the future, but the
functions are primarily meant to help reason about correctness.
I don't mind prioritizing these patches, especially since there now seems
to be multiple patches waiting on it. IIRC I was worried about not having
enough support for this change, but I might now have it.
[0] https://commitfest.postgresql.org/47/4330/
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: locked reads for atomics
@ 2024-02-23 18:25 Jeff Davis <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 2 replies; 15+ messages in thread
From: Jeff Davis @ 2024-02-23 18:25 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
On Fri, 2024-02-23 at 10:17 -0600, Nathan Bossart wrote:
> The idea is
> to provide an easy way to remove spinlocks, etc. and use atomics for
> less
> performance-sensitive stuff. The implementations are intended to be
> relatively inexpensive and might continue to improve in the future,
> but the
> functions are primarily meant to help reason about correctness.
To be clear:
x = pg_atomic_[read|write]_membarrier_u64(&v);
is semantically equivalent to:
pg_memory_barrier();
x = pg_atomic_[read|write]_u64(&v);
pg_memory_barrier();
?
If so, that does seem more convenient.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: locked reads for atomics
@ 2024-02-23 19:32 Nathan Bossart <[email protected]>
parent: Jeff Davis <[email protected]>
1 sibling, 1 reply; 15+ messages in thread
From: Nathan Bossart @ 2024-02-23 19:32 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
On Fri, Feb 23, 2024 at 10:25:00AM -0800, Jeff Davis wrote:
> To be clear:
>
> x = pg_atomic_[read|write]_membarrier_u64(&v);
>
> is semantically equivalent to:
>
> pg_memory_barrier();
> x = pg_atomic_[read|write]_u64(&v);
> pg_memory_barrier();
>
> ?
>
> If so, that does seem more convenient.
I think that's about right. The upthread feedback from Andres [0] provides
some additional context.
[0] https://postgr.es/m/20231110231150.fjm77gup2i7xu6hc%40alap3.anarazel.de
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: locked reads for atomics
@ 2024-02-23 20:58 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Nathan Bossart @ 2024-02-23 20:58 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
Here is a v3 of the patch set with the first draft of the commit messages.
There are no code differences between v2 and v3.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v3-0001-Introduce-atomic-read-write-functions-with-full-b.patch (6.1K, ../../20240223205812.GA1804550@nathanxps13/2-v3-0001-Introduce-atomic-read-write-functions-with-full-b.patch)
download | inline diff:
From f1441aca96f157c5557d9a961fe85902b510f293 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 23 Feb 2024 14:23:15 -0600
Subject: [PATCH v3 1/2] Introduce atomic read/write functions with full
barrier semantics.
Writing correct code using atomic variables is often difficult due
to the implied memory barrier semantics (or lack thereof) of the
underlying operations. This commit introduces atomic read/write
functions with full barrier semantics to ease this cognitive load
in areas that are not performance critical. For example, some
spinlocks protect a single value, and these new functions make it
easy to convert the value to an atomic variable (thus eliminating
the need for the spinlock) without modifying the barrier semantics
previously provided by the spinlock. However, since these
functions are less performant than the other atomic reads/writes,
they are not suitable for every use-case.
The base implementations for these new functions are atomic
exchanges (for writes) and atomic fetch/adds with 0 (for reads).
These implementations can be overwritten with better architecture-
specific versions as they are discovered.
This commit leaves converting existing code to use these new
functions as a future exercise.
Reviewed-by: Andres Freund, Yong Li, Jeff Davis
Discussion: https://postgr.es/me/20231110205128.GB1315705%40nathanxps13
---
src/include/port/atomics.h | 59 ++++++++++++++++++++++++++++++
src/include/port/atomics/generic.h | 36 ++++++++++++++++++
2 files changed, 95 insertions(+)
diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index bf151037f7..36a0ac9d85 100644
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -239,6 +239,26 @@ pg_atomic_read_u32(volatile pg_atomic_uint32 *ptr)
return pg_atomic_read_u32_impl(ptr);
}
+/*
+ * pg_atomic_read_membarrier_u32 - read with barrier semantics.
+ *
+ * This read is guaranteed to return the current value, provided that the value
+ * is only ever updated via operations with barrier semantics, such as
+ * pg_atomic_compare_exchange_u32() and pg_atomic_write_membarrier_u32(). Note
+ * that this is less performant than pg_atomic_read_u32(), but it may be easier
+ * to reason about correctness with this function in less performance-sensitive
+ * code.
+ *
+ * Full barrier semantics.
+ */
+static inline uint32
+pg_atomic_read_membarrier_u32(volatile pg_atomic_uint32 *ptr)
+{
+ AssertPointerAlignment(ptr, 4);
+
+ return pg_atomic_read_membarrier_u32_impl(ptr);
+}
+
/*
* pg_atomic_write_u32 - write to atomic variable.
*
@@ -276,6 +296,27 @@ pg_atomic_unlocked_write_u32(volatile pg_atomic_uint32 *ptr, uint32 val)
pg_atomic_unlocked_write_u32_impl(ptr, val);
}
+/*
+ * pg_atomic_write_membarrier_u32 - write with barrier semantics.
+ *
+ * The write is guaranteed to succeed as a whole, i.e., it's not possible to
+ * observe a partial write for any reader. Note that this correctly interacts
+ * with both pg_atomic_compare_exchange_u32() and
+ * pg_atomic_read_membarrier_u32(). While this may be less performant than
+ * pg_atomic_write_u32() and pg_atomic_unlocked_write_u32(), it may be easier
+ * to reason about correctness with this function in less performance-sensitive
+ * code.
+ *
+ * Full barrier semantics.
+ */
+static inline void
+pg_atomic_write_membarrier_u32(volatile pg_atomic_uint32 *ptr, uint32 val)
+{
+ AssertPointerAlignment(ptr, 4);
+
+ pg_atomic_write_membarrier_u32_impl(ptr, val);
+}
+
/*
* pg_atomic_exchange_u32 - exchange newval with current value
*
@@ -429,6 +470,15 @@ pg_atomic_read_u64(volatile pg_atomic_uint64 *ptr)
return pg_atomic_read_u64_impl(ptr);
}
+static inline uint64
+pg_atomic_read_membarrier_u64(volatile pg_atomic_uint64 *ptr)
+{
+#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
+ AssertPointerAlignment(ptr, 8);
+#endif
+ return pg_atomic_read_membarrier_u64_impl(ptr);
+}
+
static inline void
pg_atomic_write_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
{
@@ -438,6 +488,15 @@ pg_atomic_write_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
pg_atomic_write_u64_impl(ptr, val);
}
+static inline void
+pg_atomic_write_membarrier_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
+{
+#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
+ AssertPointerAlignment(ptr, 8);
+#endif
+ pg_atomic_write_membarrier_u64_impl(ptr, val);
+}
+
static inline uint64
pg_atomic_exchange_u64(volatile pg_atomic_uint64 *ptr, uint64 newval)
{
diff --git a/src/include/port/atomics/generic.h b/src/include/port/atomics/generic.h
index c3b5f6d9a4..6113ab62a3 100644
--- a/src/include/port/atomics/generic.h
+++ b/src/include/port/atomics/generic.h
@@ -243,6 +243,24 @@ pg_atomic_sub_fetch_u32_impl(volatile pg_atomic_uint32 *ptr, int32 sub_)
}
#endif
+#if !defined(PG_HAVE_ATOMIC_READ_MEMBARRIER_U32) && defined(PG_HAVE_ATOMIC_FETCH_ADD_U32)
+#define PG_HAVE_ATOMIC_READ_MEMBARRIER_U32
+static inline uint32
+pg_atomic_read_membarrier_u32_impl(volatile pg_atomic_uint32 *ptr)
+{
+ return pg_atomic_fetch_add_u32_impl(ptr, 0);
+}
+#endif
+
+#if !defined(PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U32) && defined(PG_HAVE_ATOMIC_EXCHANGE_U32)
+#define PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U32
+static inline void
+pg_atomic_write_membarrier_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val)
+{
+ (void) pg_atomic_exchange_u32_impl(ptr, val);
+}
+#endif
+
#if !defined(PG_HAVE_ATOMIC_EXCHANGE_U64) && defined(PG_HAVE_ATOMIC_COMPARE_EXCHANGE_U64)
#define PG_HAVE_ATOMIC_EXCHANGE_U64
static inline uint64
@@ -399,3 +417,21 @@ pg_atomic_sub_fetch_u64_impl(volatile pg_atomic_uint64 *ptr, int64 sub_)
return pg_atomic_fetch_sub_u64_impl(ptr, sub_) - sub_;
}
#endif
+
+#if !defined(PG_HAVE_ATOMIC_READ_MEMBARRIER_U64) && defined(PG_HAVE_ATOMIC_FETCH_ADD_U64)
+#define PG_HAVE_ATOMIC_READ_MEMBARRIER_U64
+static inline uint64
+pg_atomic_read_membarrier_u64_impl(volatile pg_atomic_uint64 *ptr)
+{
+ return pg_atomic_fetch_add_u64_impl(ptr, 0);
+}
+#endif
+
+#if !defined(PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U64) && defined(PG_HAVE_ATOMIC_EXCHANGE_U64)
+#define PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U64
+static inline void
+pg_atomic_write_membarrier_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 val)
+{
+ (void) pg_atomic_exchange_u64_impl(ptr, val);
+}
+#endif
--
2.25.1
[text/x-diff] v3-0002-Convert-archiver-s-force_dir_scan-variable-to-an-.patch (2.5K, ../../20240223205812.GA1804550@nathanxps13/3-v3-0002-Convert-archiver-s-force_dir_scan-variable-to-an-.patch)
download | inline diff:
From c45a6aa1453c4b1bde59fb96ef93e2d94ac72a06 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 23 Feb 2024 14:43:50 -0600
Subject: [PATCH v3 2/2] Convert archiver's force_dir_scan variable to an
atomic variable.
Commit XXXXXXXXXX introduced new atomic read/write functions with
full barrier semantics, which are intended to ease converting non-
performance-critical code to use atomic variables. This commit
demonstrates one such conversion.
Reviewed-by: Yong Li
Discussion: https://postgr.es/m/20231110205128.GB1315705%40nathanxps13
---
src/backend/postmaster/pgarch.c | 21 +++++----------------
1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 9c18e4b3ef..a8ec76ab81 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -45,7 +45,6 @@
#include "storage/proc.h"
#include "storage/procsignal.h"
#include "storage/shmem.h"
-#include "storage/spin.h"
#include "utils/guc.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
@@ -83,11 +82,9 @@ typedef struct PgArchData
int pgprocno; /* pgprocno of archiver process */
/*
- * Forces a directory scan in pgarch_readyXlog(). Protected by arch_lck.
+ * Forces a directory scan in pgarch_readyXlog().
*/
- bool force_dir_scan;
-
- slock_t arch_lck;
+ pg_atomic_uint32 force_dir_scan;
} PgArchData;
char *XLogArchiveLibrary = "";
@@ -174,7 +171,7 @@ PgArchShmemInit(void)
/* First time through, so initialize */
MemSet(PgArch, 0, PgArchShmemSize());
PgArch->pgprocno = INVALID_PGPROCNO;
- SpinLockInit(&PgArch->arch_lck);
+ pg_atomic_init_u32(&PgArch->force_dir_scan, false);
}
}
@@ -545,18 +542,12 @@ pgarch_readyXlog(char *xlog)
char XLogArchiveStatusDir[MAXPGPATH];
DIR *rldir;
struct dirent *rlde;
- bool force_dir_scan;
/*
* If a directory scan was requested, clear the stored file names and
* proceed.
*/
- SpinLockAcquire(&PgArch->arch_lck);
- force_dir_scan = PgArch->force_dir_scan;
- PgArch->force_dir_scan = false;
- SpinLockRelease(&PgArch->arch_lck);
-
- if (force_dir_scan)
+ if (pg_atomic_exchange_u32(&PgArch->force_dir_scan, false))
arch_files->arch_files_size = 0;
/*
@@ -707,9 +698,7 @@ ready_file_comparator(Datum a, Datum b, void *arg)
void
PgArchForceDirScan(void)
{
- SpinLockAcquire(&PgArch->arch_lck);
- PgArch->force_dir_scan = true;
- SpinLockRelease(&PgArch->arch_lck);
+ pg_atomic_write_membarrier_u32(&PgArch->force_dir_scan, true);
}
/*
--
2.25.1
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: locked reads for atomics
@ 2024-02-24 01:30 Andres Freund <[email protected]>
parent: Jeff Davis <[email protected]>
1 sibling, 0 replies; 15+ messages in thread
From: Andres Freund @ 2024-02-24 01:30 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers
Hi,
On 2024-02-23 10:25:00 -0800, Jeff Davis wrote:
> On Fri, 2024-02-23 at 10:17 -0600, Nathan Bossart wrote:
> > The idea is
> > to provide an easy way to remove spinlocks, etc. and use atomics for
> > less
> > performance-sensitive stuff. The implementations are intended to be
> > relatively inexpensive and might continue to improve in the future,
> > but the
> > functions are primarily meant to help reason about correctness.
>
> To be clear:
>
> x = pg_atomic_[read|write]_membarrier_u64(&v);
>
> is semantically equivalent to:
>
> pg_memory_barrier();
> x = pg_atomic_[read|write]_u64(&v);
> pg_memory_barrier();
> ?
>
> If so, that does seem more convenient.
Kinda. Semantically I think that's correct, however it doesn't commonly make
sense to have both those memory barriers, so you wouldn't really write code
like that and thus comparing on the basis of convenience doesn't quite seem
right.
Rather than convenience, I think performance and simplicity are better
arguments. If you're going to execute a read and then a memory barrier, it's
going to be faster to just do a single atomic operation. And it's a bit
simpler to analyze on which "side" of the read/write the barrier is needed.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: locked reads for atomics
@ 2024-02-24 01:34 Andres Freund <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2024-02-24 01:34 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Jeff Davis <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers
Hi,
On 2024-02-23 14:58:12 -0600, Nathan Bossart wrote:
> +/*
> + * pg_atomic_write_membarrier_u32 - write with barrier semantics.
> + *
> + * The write is guaranteed to succeed as a whole, i.e., it's not possible to
> + * observe a partial write for any reader. Note that this correctly interacts
> + * with both pg_atomic_compare_exchange_u32() and
> + * pg_atomic_read_membarrier_u32(). While this may be less performant than
> + * pg_atomic_write_u32() and pg_atomic_unlocked_write_u32(), it may be easier
> + * to reason about correctness with this function in less performance-sensitive
> + * code.
> + *
> + * Full barrier semantics.
> + */
The callout to pg_atomic_unlocked_write_u32() is wrong. The reason to use
pg_atomic_unlocked_write_u32() is for variables where we do not ever want to
fall back to spinlocks/semaphores, because the underlying variable isn't
actually shared. In those cases using the other variants is a bug. The only
use of pg_atomic_unlocked_write_u32() is temp-table buffers which share the
data structure with the shared buffers case.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: locked reads for atomics
@ 2024-02-24 15:27 Nathan Bossart <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Nathan Bossart @ 2024-02-24 15:27 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Jeff Davis <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers
On Fri, Feb 23, 2024 at 05:34:49PM -0800, Andres Freund wrote:
> On 2024-02-23 14:58:12 -0600, Nathan Bossart wrote:
>> +/*
>> + * pg_atomic_write_membarrier_u32 - write with barrier semantics.
>> + *
>> + * The write is guaranteed to succeed as a whole, i.e., it's not possible to
>> + * observe a partial write for any reader. Note that this correctly interacts
>> + * with both pg_atomic_compare_exchange_u32() and
>> + * pg_atomic_read_membarrier_u32(). While this may be less performant than
>> + * pg_atomic_write_u32() and pg_atomic_unlocked_write_u32(), it may be easier
>> + * to reason about correctness with this function in less performance-sensitive
>> + * code.
>> + *
>> + * Full barrier semantics.
>> + */
>
> The callout to pg_atomic_unlocked_write_u32() is wrong. The reason to use
> pg_atomic_unlocked_write_u32() is for variables where we do not ever want to
> fall back to spinlocks/semaphores, because the underlying variable isn't
> actually shared. In those cases using the other variants is a bug. The only
> use of pg_atomic_unlocked_write_u32() is temp-table buffers which share the
> data structure with the shared buffers case.
I removed the reference to pg_atomic_unlocked_write_u32().
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v4-0001-Introduce-atomic-read-write-functions-with-full-b.patch (6.1K, ../../20240224152734.GA1862419@nathanxps13/2-v4-0001-Introduce-atomic-read-write-functions-with-full-b.patch)
download | inline diff:
From 647eea91cc07e2d6a4d6634bcb933df245711fa0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 23 Feb 2024 14:23:15 -0600
Subject: [PATCH v4 1/2] Introduce atomic read/write functions with full
barrier semantics.
Writing correct code using atomic variables is often difficult due
to the implied memory barrier semantics (or lack thereof) of the
underlying operations. This commit introduces atomic read/write
functions with full barrier semantics to ease this cognitive load
in areas that are not performance critical. For example, some
spinlocks protect a single value, and these new functions make it
easy to convert the value to an atomic variable (thus eliminating
the need for the spinlock) without modifying the barrier semantics
previously provided by the spinlock. However, since these
functions are less performant than the other atomic reads/writes,
they are not suitable for every use-case.
The base implementations for these new functions are atomic
exchanges (for writes) and atomic fetch/adds with 0 (for reads).
These implementations can be overwritten with better architecture-
specific versions as they are discovered.
This commit leaves converting existing code to use these new
functions as a future exercise.
Reviewed-by: Andres Freund, Yong Li, Jeff Davis
Discussion: https://postgr.es/me/20231110205128.GB1315705%40nathanxps13
---
src/include/port/atomics.h | 58 ++++++++++++++++++++++++++++++
src/include/port/atomics/generic.h | 36 +++++++++++++++++++
2 files changed, 94 insertions(+)
diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index bf151037f7..c3b14440cf 100644
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -239,6 +239,26 @@ pg_atomic_read_u32(volatile pg_atomic_uint32 *ptr)
return pg_atomic_read_u32_impl(ptr);
}
+/*
+ * pg_atomic_read_membarrier_u32 - read with barrier semantics.
+ *
+ * This read is guaranteed to return the current value, provided that the value
+ * is only ever updated via operations with barrier semantics, such as
+ * pg_atomic_compare_exchange_u32() and pg_atomic_write_membarrier_u32().
+ * While this may be less performant than pg_atomic_read_u32(), it may be
+ * easier to reason about correctness with this function in less performance-
+ * sensitive code.
+ *
+ * Full barrier semantics.
+ */
+static inline uint32
+pg_atomic_read_membarrier_u32(volatile pg_atomic_uint32 *ptr)
+{
+ AssertPointerAlignment(ptr, 4);
+
+ return pg_atomic_read_membarrier_u32_impl(ptr);
+}
+
/*
* pg_atomic_write_u32 - write to atomic variable.
*
@@ -276,6 +296,26 @@ pg_atomic_unlocked_write_u32(volatile pg_atomic_uint32 *ptr, uint32 val)
pg_atomic_unlocked_write_u32_impl(ptr, val);
}
+/*
+ * pg_atomic_write_membarrier_u32 - write with barrier semantics.
+ *
+ * The write is guaranteed to succeed as a whole, i.e., it's not possible to
+ * observe a partial write for any reader. Note that this correctly interacts
+ * with both pg_atomic_compare_exchange_u32() and
+ * pg_atomic_read_membarrier_u32(). While this may be less performant than
+ * pg_atomic_write_u32(), it may be easier to reason about correctness with
+ * this function in less performance-sensitive code.
+ *
+ * Full barrier semantics.
+ */
+static inline void
+pg_atomic_write_membarrier_u32(volatile pg_atomic_uint32 *ptr, uint32 val)
+{
+ AssertPointerAlignment(ptr, 4);
+
+ pg_atomic_write_membarrier_u32_impl(ptr, val);
+}
+
/*
* pg_atomic_exchange_u32 - exchange newval with current value
*
@@ -429,6 +469,15 @@ pg_atomic_read_u64(volatile pg_atomic_uint64 *ptr)
return pg_atomic_read_u64_impl(ptr);
}
+static inline uint64
+pg_atomic_read_membarrier_u64(volatile pg_atomic_uint64 *ptr)
+{
+#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
+ AssertPointerAlignment(ptr, 8);
+#endif
+ return pg_atomic_read_membarrier_u64_impl(ptr);
+}
+
static inline void
pg_atomic_write_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
{
@@ -438,6 +487,15 @@ pg_atomic_write_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
pg_atomic_write_u64_impl(ptr, val);
}
+static inline void
+pg_atomic_write_membarrier_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
+{
+#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
+ AssertPointerAlignment(ptr, 8);
+#endif
+ pg_atomic_write_membarrier_u64_impl(ptr, val);
+}
+
static inline uint64
pg_atomic_exchange_u64(volatile pg_atomic_uint64 *ptr, uint64 newval)
{
diff --git a/src/include/port/atomics/generic.h b/src/include/port/atomics/generic.h
index c3b5f6d9a4..6113ab62a3 100644
--- a/src/include/port/atomics/generic.h
+++ b/src/include/port/atomics/generic.h
@@ -243,6 +243,24 @@ pg_atomic_sub_fetch_u32_impl(volatile pg_atomic_uint32 *ptr, int32 sub_)
}
#endif
+#if !defined(PG_HAVE_ATOMIC_READ_MEMBARRIER_U32) && defined(PG_HAVE_ATOMIC_FETCH_ADD_U32)
+#define PG_HAVE_ATOMIC_READ_MEMBARRIER_U32
+static inline uint32
+pg_atomic_read_membarrier_u32_impl(volatile pg_atomic_uint32 *ptr)
+{
+ return pg_atomic_fetch_add_u32_impl(ptr, 0);
+}
+#endif
+
+#if !defined(PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U32) && defined(PG_HAVE_ATOMIC_EXCHANGE_U32)
+#define PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U32
+static inline void
+pg_atomic_write_membarrier_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val)
+{
+ (void) pg_atomic_exchange_u32_impl(ptr, val);
+}
+#endif
+
#if !defined(PG_HAVE_ATOMIC_EXCHANGE_U64) && defined(PG_HAVE_ATOMIC_COMPARE_EXCHANGE_U64)
#define PG_HAVE_ATOMIC_EXCHANGE_U64
static inline uint64
@@ -399,3 +417,21 @@ pg_atomic_sub_fetch_u64_impl(volatile pg_atomic_uint64 *ptr, int64 sub_)
return pg_atomic_fetch_sub_u64_impl(ptr, sub_) - sub_;
}
#endif
+
+#if !defined(PG_HAVE_ATOMIC_READ_MEMBARRIER_U64) && defined(PG_HAVE_ATOMIC_FETCH_ADD_U64)
+#define PG_HAVE_ATOMIC_READ_MEMBARRIER_U64
+static inline uint64
+pg_atomic_read_membarrier_u64_impl(volatile pg_atomic_uint64 *ptr)
+{
+ return pg_atomic_fetch_add_u64_impl(ptr, 0);
+}
+#endif
+
+#if !defined(PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U64) && defined(PG_HAVE_ATOMIC_EXCHANGE_U64)
+#define PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U64
+static inline void
+pg_atomic_write_membarrier_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 val)
+{
+ (void) pg_atomic_exchange_u64_impl(ptr, val);
+}
+#endif
--
2.25.1
[text/x-diff] v4-0002-Convert-archiver-s-force_dir_scan-variable-to-an-.patch (2.5K, ../../20240224152734.GA1862419@nathanxps13/3-v4-0002-Convert-archiver-s-force_dir_scan-variable-to-an-.patch)
download | inline diff:
From 60c529540ce32c98d9d8d088be09e73fde1080a5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 23 Feb 2024 14:43:50 -0600
Subject: [PATCH v4 2/2] Convert archiver's force_dir_scan variable to an
atomic variable.
Commit XXXXXXXXXX introduced new atomic read/write functions with
full barrier semantics, which are intended to ease converting non-
performance-critical code to use atomic variables. This commit
demonstrates one such conversion.
Reviewed-by: Yong Li
Discussion: https://postgr.es/m/20231110205128.GB1315705%40nathanxps13
---
src/backend/postmaster/pgarch.c | 21 +++++----------------
1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 9c18e4b3ef..a8ec76ab81 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -45,7 +45,6 @@
#include "storage/proc.h"
#include "storage/procsignal.h"
#include "storage/shmem.h"
-#include "storage/spin.h"
#include "utils/guc.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
@@ -83,11 +82,9 @@ typedef struct PgArchData
int pgprocno; /* pgprocno of archiver process */
/*
- * Forces a directory scan in pgarch_readyXlog(). Protected by arch_lck.
+ * Forces a directory scan in pgarch_readyXlog().
*/
- bool force_dir_scan;
-
- slock_t arch_lck;
+ pg_atomic_uint32 force_dir_scan;
} PgArchData;
char *XLogArchiveLibrary = "";
@@ -174,7 +171,7 @@ PgArchShmemInit(void)
/* First time through, so initialize */
MemSet(PgArch, 0, PgArchShmemSize());
PgArch->pgprocno = INVALID_PGPROCNO;
- SpinLockInit(&PgArch->arch_lck);
+ pg_atomic_init_u32(&PgArch->force_dir_scan, false);
}
}
@@ -545,18 +542,12 @@ pgarch_readyXlog(char *xlog)
char XLogArchiveStatusDir[MAXPGPATH];
DIR *rldir;
struct dirent *rlde;
- bool force_dir_scan;
/*
* If a directory scan was requested, clear the stored file names and
* proceed.
*/
- SpinLockAcquire(&PgArch->arch_lck);
- force_dir_scan = PgArch->force_dir_scan;
- PgArch->force_dir_scan = false;
- SpinLockRelease(&PgArch->arch_lck);
-
- if (force_dir_scan)
+ if (pg_atomic_exchange_u32(&PgArch->force_dir_scan, false))
arch_files->arch_files_size = 0;
/*
@@ -707,9 +698,7 @@ ready_file_comparator(Datum a, Datum b, void *arg)
void
PgArchForceDirScan(void)
{
- SpinLockAcquire(&PgArch->arch_lck);
- PgArch->force_dir_scan = true;
- SpinLockRelease(&PgArch->arch_lck);
+ pg_atomic_write_membarrier_u32(&PgArch->force_dir_scan, true);
}
/*
--
2.25.1
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: locked reads for atomics
@ 2024-02-29 16:24 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Nathan Bossart @ 2024-02-29 16:24 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Jeff Davis <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers
Committed. Thank you for reviewing!
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2024-02-29 16:24 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-10-14 23:16 [PATCH 1/2] Correct the check for pg_catalog.line in pg_upgrade Tomas Vondra <[email protected]>
2023-09-02 06:32 [PATCH v5 1/7] Row pattern recognition patch for raw parser. Tatsuo Ishii <[email protected]>
2023-11-11 02:55 Re: locked reads for atomics Nathan Bossart <[email protected]>
2023-11-27 21:00 ` Re: locked reads for atomics Nathan Bossart <[email protected]>
2024-01-17 03:48 ` Re: locked reads for atomics Li, Yong <[email protected]>
2024-02-22 07:28 ` Re: locked reads for atomics Bharath Rupireddy <[email protected]>
2024-02-22 19:53 ` Re: locked reads for atomics Jeff Davis <[email protected]>
2024-02-23 16:17 ` Re: locked reads for atomics Nathan Bossart <[email protected]>
2024-02-23 18:25 ` Re: locked reads for atomics Jeff Davis <[email protected]>
2024-02-23 19:32 ` Re: locked reads for atomics Nathan Bossart <[email protected]>
2024-02-23 20:58 ` Re: locked reads for atomics Nathan Bossart <[email protected]>
2024-02-24 01:34 ` Re: locked reads for atomics Andres Freund <[email protected]>
2024-02-24 15:27 ` Re: locked reads for atomics Nathan Bossart <[email protected]>
2024-02-29 16:24 ` Re: locked reads for atomics Nathan Bossart <[email protected]>
2024-02-24 01:30 ` Re: locked reads for atomics Andres Freund <[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