public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2] pg_wal on COW fs
15+ messages / 7 participants
[nested] [flat]
* [PATCH v2] pg_wal on COW fs
@ 2019-02-27 22:41 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Alvaro Herrera @ 2019-02-27 22:41 UTC (permalink / raw)
---
doc/src/sgml/config.sgml | 20 ++++
src/backend/access/transam/xlog.c | 101 ++++++++++++------
src/backend/utils/misc/guc.c | 13 +++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/access/xlog.h | 1 +
5 files changed, 102 insertions(+), 34 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8bd57f376b2..60a873273aa 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2959,6 +2959,26 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry id="guc-wal-cow-filesystem" xreflabel="wal_cow_filesystem">
+ <term><varname>wal_cow_filesystem</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>wal_cow_filesystem</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ This parameter should only be set to <literal>on</literal> when the WAL
+ resides on a <firstterm>Copy-On-Write</firstterm> (<acronym>COW</acronym>)
+ filesystem.
+ Enabling this option adjusts some behavior to take advantage of the
+ filesystem characteristics (for example, recycling WAL files and
+ zero-filling new WAL files are disabled).
+ This setting is only appropriate for filesystems which
+ allocate new disk blocks on every write.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</sect2>
<sect2 id="runtime-config-wal-archiving">
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index ecd12fc53ae..1acce1c70d6 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -94,6 +94,7 @@ bool wal_log_hints = false;
bool wal_compression = false;
char *wal_consistency_checking_string = NULL;
bool *wal_consistency_checking = NULL;
+bool wal_cow_filesystem = false;
bool log_checkpoints = false;
int sync_method = DEFAULT_SYNC_METHOD;
int wal_level = WAL_LEVEL_MINIMAL;
@@ -3216,6 +3217,7 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock)
XLogSegNo max_segno;
int fd;
int nbytes;
+ bool fail;
XLogFilePath(path, ThisTimeLineID, logsegno, wal_segment_size);
@@ -3255,41 +3257,65 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock)
(errcode_for_file_access(),
errmsg("could not create file \"%s\": %m", tmppath)));
- /*
- * Zero-fill the file. We have to do this the hard way to ensure that all
- * the file space has really been allocated --- on platforms that allow
- * "holes" in files, just seeking to the end doesn't allocate intermediate
- * space. This way, we know that we have all the space and (after the
- * fsync below) that all the indirect blocks are down on disk. Therefore,
- * fdatasync(2) or O_DSYNC will be sufficient to sync future writes to the
- * log file.
- */
memset(zbuffer.data, 0, XLOG_BLCKSZ);
- for (nbytes = 0; nbytes < wal_segment_size; nbytes += XLOG_BLCKSZ)
+
+ /*
+ * Ensure the filesystem has physically allocated disk space for the data.
+ */
+ if (!wal_cow_filesystem)
{
+ /*
+ * In non-CoW filesystems, zero-fill the file. We have to do this the
+ * hard way to ensure that all the file space has really been
+ * allocated --- on platforms that allow "holes" in files, just seeking
+ * to the end doesn't allocate intermediate space. This way, we know
+ * that we have all the space and (after the fsync below) that all the
+ * indirect blocks are down on disk. Therefore, fdatasync(2) or
+ * O_DSYNC will be sufficient to sync future writes to the log file.
+ */
+ fail = false; /* keep compiler quiet */
+ for (nbytes = 0; nbytes < wal_segment_size; nbytes += XLOG_BLCKSZ)
+ {
+ errno = 0;
+ pgstat_report_wait_start(WAIT_EVENT_WAL_INIT_WRITE);
+ fail = (int) write(fd, zbuffer.data, XLOG_BLCKSZ) != (int) XLOG_BLCKSZ;
+ pgstat_report_wait_end();
+ if (fail)
+ break;
+ }
+ }
+ else
+ {
+ /*
+ * In CoW filesystems, seeking to the end and writing a solitary byte
+ * is enough.
+ */
errno = 0;
pgstat_report_wait_start(WAIT_EVENT_WAL_INIT_WRITE);
- if ((int) write(fd, zbuffer.data, XLOG_BLCKSZ) != (int) XLOG_BLCKSZ)
- {
- int save_errno = errno;
- /*
- * If we fail to make the file, delete it to release disk space
- */
- unlink(tmppath);
+ fail = lseek(fd, (off_t) (wal_segment_size - 1), SEEK_SET) < (off_t) 0 ||
+ (int) write(fd, zbuffer.data, 1) != (int) 1;
- close(fd);
-
- /* if write didn't set errno, assume problem is no disk space */
- errno = save_errno ? save_errno : ENOSPC;
-
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not write to file \"%s\": %m", tmppath)));
- }
pgstat_report_wait_end();
}
+ if (fail)
+ {
+ int save_errno = errno;
+
+ /* If we fail to make the file, delete it to release disk space */
+ unlink(tmppath);
+
+ close(fd);
+
+ /* if write didn't set errno, assume problem is no disk space */
+ errno = save_errno ? save_errno : ENOSPC;
+
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not write to file \"%s\": %m", tmppath)));
+ }
+
pgstat_report_wait_start(WAIT_EVENT_WAL_INIT_SYNC);
if (pg_fsync(fd) != 0)
{
@@ -4053,14 +4079,17 @@ RemoveXlogFile(const char *segname, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
XLogSegNo endlogSegNo;
XLogSegNo recycleSegNo;
- /*
- * Initialize info about where to try to recycle to.
- */
- XLByteToSeg(endptr, endlogSegNo, wal_segment_size);
- if (RedoRecPtr == InvalidXLogRecPtr)
- recycleSegNo = endlogSegNo + 10;
+ /* Initialize info about where to try to recycle to, if needed. */
+ if (!wal_cow_filesystem)
+ {
+ XLByteToSeg(endptr, endlogSegNo, wal_segment_size);
+ if (RedoRecPtr == InvalidXLogRecPtr)
+ recycleSegNo = endlogSegNo + 10;
+ else
+ recycleSegNo = XLOGfileslop(RedoRecPtr);
+ }
else
- recycleSegNo = XLOGfileslop(RedoRecPtr);
+ recycleSegNo = (XLogSegNo) 0; /* keep compiler quiet */
snprintf(path, MAXPGPATH, XLOGDIR "/%s", segname);
@@ -4068,8 +4097,12 @@ RemoveXlogFile(const char *segname, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
* Before deleting the file, see if it can be recycled as a future log
* segment. Only recycle normal files, pg_standby for example can create
* symbolic links pointing to a separate archive directory.
+ *
+ * Skip recycling on COW filesystems, though. It's better to create
+ * new files each time.
*/
- if (endlogSegNo <= recycleSegNo &&
+ if (!wal_cow_filesystem &&
+ endlogSegNo <= recycleSegNo &&
lstat(path, &statbuf) == 0 && S_ISREG(statbuf.st_mode) &&
InstallXLogFileSegment(&endlogSegNo, path,
true, recycleSegNo, true))
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 156d147c85b..cbcf475885e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1177,6 +1177,19 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
+ {
+ {"wal_cow_filesystem", PGC_SUSET, WAL_SETTINGS,
+ gettext_noop("WAL is stored on Copy-On-Write file system."),
+ gettext_noop("This option adjusts behavior to take advantage of "
+ "filesystem characteristics specific to CoW filesystems, "
+ "improving performance. "
+ "It should be enabled on ZFS and other similar filesystems.")
+ },
+ &wal_cow_filesystem,
+ false,
+ NULL, NULL, NULL
+ },
+
{
{"log_checkpoints", PGC_SIGHUP, LOGGING_WHAT,
gettext_noop("Logs each checkpoint."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 194f3120964..c9254c04bab 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -206,6 +206,7 @@
#wal_compression = off # enable compression of full-page writes
#wal_log_hints = off # also do full page writes of non-critical updates
# (change requires restart)
+#wal_cow_filesystem = off # is pg_wal on a Copy-on-Write filesystem?
#wal_buffers = -1 # min 32kB, -1 sets based on shared_buffers
# (change requires restart)
#wal_writer_delay = 200ms # 1-10000 milliseconds
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index f90a6a91391..fa9d69affe4 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -116,6 +116,7 @@ extern bool EnableHotStandby;
extern bool fullPageWrites;
extern bool wal_log_hints;
extern bool wal_compression;
+extern bool wal_cow_filesystem;
extern bool *wal_consistency_checking;
extern char *wal_consistency_checking_string;
extern bool log_checkpoints;
--
2.17.1
--BOKacYhQ+x31HxR3--
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v7 2/7] Row pattern recognition patch (parse/analysis).
@ 2023-09-22 04:53 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Tatsuo Ishii @ 2023-09-22 04:53 UTC (permalink / raw)
---
src/backend/parser/parse_agg.c | 7 +
src/backend/parser/parse_clause.c | 293 +++++++++++++++++++++++++++++-
src/backend/parser/parse_expr.c | 4 +
src/backend/parser/parse_func.c | 3 +
4 files changed, 306 insertions(+), 1 deletion(-)
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 85cd47b7ae..aa7a1cee80 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -564,6 +564,10 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
+
/*
* There is intentionally no default: case here, so that the
* compiler will warn if we add a new ParseExprKind without
@@ -953,6 +957,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_CYCLE_MARK:
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 334b9b42bd..293d4b1680 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -100,7 +100,10 @@ static WindowClause *findWindowClause(List *wclist, const char *name);
static Node *transformFrameOffset(ParseState *pstate, int frameOptions,
Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc,
Node *clause);
-
+static void transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist);
+static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist);
+static void transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef);
+static List *transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef);
/*
* transformFromClause -
@@ -2950,6 +2953,10 @@ transformWindowDefinitions(ParseState *pstate,
rangeopfamily, rangeopcintype,
&wc->endInRangeFunc,
windef->endOffset);
+
+ /* Process Row Pattern Recognition related clauses */
+ transformRPR(pstate, wc, windef, targetlist);
+
wc->runCondition = NIL;
wc->winref = winref;
@@ -3815,3 +3822,287 @@ transformFrameOffset(ParseState *pstate, int frameOptions,
return node;
}
+
+/*
+ * transformRPR
+ * Process Row Pattern Recognition related clauses
+ */
+static void
+transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist)
+{
+ /*
+ * Window definition exists?
+ */
+ if (windef == NULL)
+ return;
+
+ /*
+ * Row Pattern Common Syntax clause exists?
+ */
+ if (windef->rpCommonSyntax == NULL)
+ return;
+
+ /* Check Frame option. Frame must start at current row */
+ if ((wc->frameOptions & FRAMEOPTION_START_CURRENT_ROW) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("FRAME must start at current row when row patttern recognition is used")));
+
+ /* Transform AFTER MACH SKIP TO clause */
+ wc->rpSkipTo = windef->rpCommonSyntax->rpSkipTo;
+
+ /* Transform SEEK or INITIAL clause */
+ wc->initial = windef->rpCommonSyntax->initial;
+
+ /* Transform DEFINE clause into list of TargetEntry's */
+ wc->defineClause = transformDefineClause(pstate, wc, windef, targetlist);
+
+ /* Check PATTERN clause and copy to patternClause */
+ transformPatternClause(pstate, wc, windef);
+
+ /* Transform MEASURE clause */
+ transformMeasureClause(pstate, wc, windef);
+}
+
+/*
+ * transformDefineClause Process DEFINE clause and transform ResTarget into
+ * list of TargetEntry.
+ *
+ * XXX we only support column reference in row pattern definition search
+ * condition, e.g. "price". <row pattern definition variable name>.<column
+ * reference> is not supported, e.g. "A.price".
+ */
+static List *
+transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist)
+{
+ /* DEFINE variable name initials */
+ static char *defineVariableInitials = "abcdefghijklmnopqrstuvwxyz";
+
+ ListCell *lc, *l;
+ ResTarget *restarget, *r;
+ List *restargets;
+ char *name;
+ int initialLen;
+ int i;
+
+ /*
+ * If Row Definition Common Syntax exists, DEFINE clause must exist.
+ * (the raw parser should have already checked it.)
+ */
+ Assert(windef->rpCommonSyntax->rpDefs != NULL);
+
+ /*
+ * Check and add "A AS A IS TRUE" if pattern variable is missing in DEFINE
+ * per the SQL standard.
+ */
+ restargets = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpPatterns)
+ {
+ A_Expr *a;
+ bool found = false;
+
+ if (!IsA(lfirst(lc), A_Expr))
+ ereport(ERROR,
+ errmsg("node type is not A_Expr"));
+
+ a = (A_Expr *)lfirst(lc);
+ name = strVal(a->lexpr);
+
+ foreach(l, windef->rpCommonSyntax->rpDefs)
+ {
+ restarget = (ResTarget *)lfirst(l);
+
+ if (!strcmp(restarget->name, name))
+ {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ {
+ /*
+ * "name" is missing. So create "name AS name IS TRUE" ResTarget
+ * node and add it to the temporary list.
+ */
+ A_Const *n;
+
+ restarget = makeNode(ResTarget);
+ n = makeNode(A_Const);
+ n->val.boolval.type = T_Boolean;
+ n->val.boolval.boolval = true;
+ n->location = -1;
+ restarget->name = pstrdup(name);
+ restarget->indirection = NIL;
+ restarget->val = (Node *)n;
+ restarget->location = -1;
+ restargets = lappend((List *)restargets, restarget);
+ }
+ }
+
+ if (list_length(restargets) >= 1)
+ {
+ /* add missing DEFINEs */
+ windef->rpCommonSyntax->rpDefs = list_concat(windef->rpCommonSyntax->rpDefs,
+ restargets);
+ list_free(restargets);
+ }
+
+ /*
+ * Check for duplicate row pattern definition variables. The standard
+ * requires that no two row pattern definition variable names shall be
+ * equivalent.
+ */
+ restargets = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpDefs)
+ {
+ restarget = (ResTarget *)lfirst(lc);
+ name = restarget->name;
+
+ /*
+ * Add DEFINE expression (Restarget->val) to the targetlist as a
+ * TargetEntry if it does not exist yet. Planner will add the column
+ * ref var node to the outer plan's target list later on. This makes
+ * DEFINE expression could access the outer tuple while evaluating
+ * PATTERN.
+ *
+ * XXX: adding whole expressions of DEFINE to the plan.targetlist is
+ * not so good, because it's not necessary to evalute the expression
+ * in the target list while running the plan. We should extract the
+ * var nodes only then add them to the plan.targetlist.
+ */
+ findTargetlistEntrySQL99(pstate, (Node *)restarget->val, targetlist, EXPR_KIND_RPR_DEFINE);
+
+ /*
+ * Make sure that the row pattern definition search condition is a
+ * boolean expression.
+ */
+ transformWhereClause(pstate, restarget->val,
+ EXPR_KIND_RPR_DEFINE, "DEFINE");
+
+ foreach(l, restargets)
+ {
+ char *n;
+
+ r = (ResTarget *) lfirst(l);
+ n = r->name;
+
+ if (!strcmp(n, name))
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("row pattern definition variable name \"%s\" appears more than once in DEFINE clause",
+ name),
+ parser_errposition(pstate, exprLocation((Node *)r))));
+ }
+ restargets = lappend(restargets, restarget);
+ }
+ list_free(restargets);
+
+ /*
+ * Create list of row pattern DEFINE variable name's initial.
+ * We assign [a-z] to them (up to 26 variable names are allowed).
+ */
+ restargets = NIL;
+ i = 0;
+ initialLen = strlen(defineVariableInitials);
+
+ foreach(lc, windef->rpCommonSyntax->rpDefs)
+ {
+ char initial[2];
+
+ restarget = (ResTarget *)lfirst(lc);
+ name = restarget->name;
+
+ if (i >= initialLen)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("number of row pattern definition variable names exceeds %d", initialLen),
+ parser_errposition(pstate, exprLocation((Node *)restarget))));
+ }
+ initial[0] = defineVariableInitials[i++];
+ initial[1] = '\0';
+ wc->defineInitial = lappend(wc->defineInitial, makeString(pstrdup(initial)));
+ }
+
+ return transformTargetList(pstate, windef->rpCommonSyntax->rpDefs,
+ EXPR_KIND_RPR_DEFINE);
+}
+
+/*
+ * transformPatternClause
+ * Process PATTERN clause and return PATTERN clause in the raw parse tree
+ */
+static void
+transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef)
+{
+ ListCell *lc, *l;
+
+ /*
+ * Row Pattern Common Syntax clause exists?
+ */
+ if (windef->rpCommonSyntax == NULL)
+ return;
+
+ /*
+ * Primary row pattern variable names in PATTERN clause must appear in
+ * DEFINE clause as row pattern definition variable names.
+ */
+ wc->patternVariable = NIL;
+ wc->patternRegexp = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpPatterns)
+ {
+ A_Expr *a;
+ char *name;
+ char *regexp;
+ bool found = false;
+
+ if (!IsA(lfirst(lc), A_Expr))
+ ereport(ERROR,
+ errmsg("node type is not A_Expr"));
+
+ a = (A_Expr *)lfirst(lc);
+ name = strVal(a->lexpr);
+
+ foreach(l, windef->rpCommonSyntax->rpDefs)
+ {
+ ResTarget *restarget = (ResTarget *)lfirst(l);
+
+ if (!strcmp(restarget->name, name))
+ {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("primary row pattern variable name \"%s\" does not appear in DEFINE clause",
+ name),
+ parser_errposition(pstate, exprLocation((Node *)a))));
+ }
+ wc->patternVariable = lappend(wc->patternVariable, makeString(pstrdup(name)));
+ regexp = strVal(lfirst(list_head(a->name)));
+ wc->patternRegexp = lappend(wc->patternRegexp, makeString(pstrdup(regexp)));
+ }
+}
+
+/*
+ * transformMeasureClause
+ * Process MEASURE clause
+ * XXX MEASURE clause is not supported yet
+ */
+static List *
+transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef)
+{
+ if (windef->rowPatternMeasures == NIL)
+ return NIL;
+
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s","MEASURE clause is not supported yet"),
+ parser_errposition(pstate, exprLocation((Node *)windef->rowPatternMeasures))));
+ return NIL;
+}
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 64c582c344..18b58ac263 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -557,6 +557,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
case EXPR_KIND_COPY_WHERE:
case EXPR_KIND_GENERATED_COLUMN:
case EXPR_KIND_CYCLE_MARK:
+ case EXPR_KIND_RPR_DEFINE:
/* okay */
break;
@@ -1770,6 +1771,7 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
case EXPR_KIND_VALUES:
case EXPR_KIND_VALUES_SINGLE:
case EXPR_KIND_CYCLE_MARK:
+ case EXPR_KIND_RPR_DEFINE:
/* okay */
break;
case EXPR_KIND_CHECK_CONSTRAINT:
@@ -3149,6 +3151,8 @@ ParseExprKindName(ParseExprKind exprKind)
return "GENERATED AS";
case EXPR_KIND_CYCLE_MARK:
return "CYCLE";
+ case EXPR_KIND_RPR_DEFINE:
+ return "DEFINE";
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index b3f0b6a137..2ff3699538 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2656,6 +2656,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
case EXPR_KIND_CYCLE_MARK:
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
--
2.25.1
----Next_Part(Fri_Sep_22_14_16_40_2023_530)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v7-0003-Row-pattern-recognition-patch-planner.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: race condition in pg_class
@ 2023-10-25 18:06 Tom Lane <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2023-10-25 18:06 UTC (permalink / raw)
To: Smolkin Grigory <[email protected]>; +Cc: pgsql-hackers
Smolkin Grigory <[email protected]> writes:
> We are running PG13.10 and recently we have encountered what appears to be
> a bug due to some race condition between ALTER TABLE ... ADD CONSTRAINT and
> some other catalog-writer, possibly ANALYZE.
> The problem is that after successfully creating index on relation (which
> previosly didnt have any indexes), its pg_class.relhasindex remains set to
> "false", which is illegal, I think.
> Index was built using the following statement:
> ALTER TABLE "example" ADD constraint "example_pkey" PRIMARY KEY (id);
ALTER TABLE ADD CONSTRAINT would certainly have taken
AccessExclusiveLock on the "example" table, which should be sufficient
to prevent anything else from touching its pg_class row. The only
mechanism I can think of that might bypass that is a manual UPDATE on
pg_class, which would just manipulate the row as a row without concern
for associated relation-level locks. Any chance that somebody was
doing something like that?
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: race condition in pg_class
@ 2023-10-26 09:52 Smolkin Grigory <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Smolkin Grigory @ 2023-10-26 09:52 UTC (permalink / raw)
To: pgsql-hackers
> ALTER TABLE ADD CONSTRAINT would certainly have taken
> AccessExclusiveLock on the "example" table, which should be sufficient
> to prevent anything else from touching its pg_class row. The only
> mechanism I can think of that might bypass that is a manual UPDATE on
> pg_class, which would just manipulate the row as a row without concern
> for associated relation-level locks. Any chance that somebody was
> doing something like that?
No chance. Our infrastructure dont do that, and users dont just have the
privileges to mess with pg_catalog.
ср, 25 окт. 2023 г. в 21:06, Tom Lane <[email protected]>:
> Smolkin Grigory <[email protected]> writes:
> > We are running PG13.10 and recently we have encountered what appears to
> be
> > a bug due to some race condition between ALTER TABLE ... ADD CONSTRAINT
> and
> > some other catalog-writer, possibly ANALYZE.
> > The problem is that after successfully creating index on relation (which
> > previosly didnt have any indexes), its pg_class.relhasindex remains set
> to
> > "false", which is illegal, I think.
> > Index was built using the following statement:
> > ALTER TABLE "example" ADD constraint "example_pkey" PRIMARY KEY (id);
>
> ALTER TABLE ADD CONSTRAINT would certainly have taken
> AccessExclusiveLock on the "example" table, which should be sufficient
> to prevent anything else from touching its pg_class row. The only
> mechanism I can think of that might bypass that is a manual UPDATE on
> pg_class, which would just manipulate the row as a row without concern
> for associated relation-level locks. Any chance that somebody was
> doing something like that?
>
> regards, tom lane
>
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
@ 2025-03-07 16:38 Andres Freund <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2025-03-07 16:38 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
Hi,
On 2025-03-06 15:39:44 -0800, Jacob Champion wrote:
> I've reattached the wait event patches, to get the cfbot back to where it was.
FWIW, I continue to think that this is a misuse of wait events. We shouldn't
use them as a poor man's general purpose tracing framework.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
@ 2025-03-07 17:03 Jacob Champion <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jacob Champion @ 2025-03-07 17:03 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Fri, Mar 7, 2025 at 8:38 AM Andres Freund <[email protected]> wrote:
> FWIW, I continue to think that this is a misuse of wait events. We shouldn't
> use them as a poor man's general purpose tracing framework.
Well, okay. That's frustrating.
If I return to the original design, but replace all of the high-level
wait events with calls to pgstat_report_activity(), does that work?
--Jacob
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
@ 2025-03-07 17:25 Andres Freund <[email protected]>
parent: Jacob Champion <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2025-03-07 17:25 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
Hi,
On 2025-03-07 09:03:18 -0800, Jacob Champion wrote:
> On Fri, Mar 7, 2025 at 8:38 AM Andres Freund <[email protected]> wrote:
> > FWIW, I continue to think that this is a misuse of wait events. We shouldn't
> > use them as a poor man's general purpose tracing framework.
>
> Well, okay. That's frustrating.
I should have clarified - there are a few that I think are ok, basically the
places where we wrap syscalls, e.g. around the sendto, select and recvfrom in
PerformRadiusTransaction().
OTOH that code is effectively completely broken. Doing a blocking select() is
just a no-go, the code isn't interruptible, breaking authentication
timeout. And using select() means that we theoretically could crash due to an
fd that's above FD_SETSIZE.
Most of the other places I'm not on board with, that's wrapping large amounts
of code in a wait event, which pretty much means we're not waiting.
I think some of the wrapped calls into library code might actually call back
into our code (to receive/send data), and our code then will use wait events
around lower level operations done as part of that.
Which pretty much explains my main issue with this - either the code can't
wait in those function calls, in which case it's wrong to use wait events, or
the code is flat out broken.
It's also IMO quite wrong to do something that can throw an error inside a
wait event, because that means that the wait event will still be reported
during error recovery. Probably not the only place doing so, but it's still
wrong.
> If I return to the original design, but replace all of the high-level
> wait events with calls to pgstat_report_activity(), does that work?
It'd be less wrong.
But I really doubt that it's a good idea to encode all kinds of function calls
happening during authentication into something SQL visible. Why stop with
these functions and not just do that for *all* functions in postgres? I mean
it'd not work and slow everything down, but how do you define that line?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
@ 2025-03-07 18:28 Jacob Champion <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jacob Champion @ 2025-03-07 18:28 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Fri, Mar 7, 2025 at 9:25 AM Andres Freund <[email protected]> wrote:
> I should have clarified - there are a few that I think are ok, basically the
> places where we wrap syscalls, e.g. around the sendto, select and recvfrom in
> PerformRadiusTransaction().
Okay.
> OTOH that code is effectively completely broken. Doing a blocking select() is
> just a no-go, the code isn't interruptible, breaking authentication
> timeout. And using select() means that we theoretically could crash due to an
> fd that's above FD_SETSIZE.
I think we're in agreement here; I'm just trying to improve things
incrementally. If someone actually hits the broken case, I think it'd
be helpful for them to see it.
> I think some of the wrapped calls into library code might actually call back
> into our code (to receive/send data), and our code then will use wait events
> around lower level operations done as part of that.
That would be a problem, agreed, but I didn't think I'd wrapped any
callback APIs. (Admittedly I have little experience with the SSPI
stuff.) But looking at the wrapped calls in the patch... which are you
suspicious of?
> It's also IMO quite wrong to do something that can throw an error inside a
> wait event, because that means that the wait event will still be reported
> during error recovery.
Hm, okay. I can change that for the LookupAccountSid case.
> Probably not the only place doing so, but it's still
> wrong.
It's definitely not the only place. :D
> Why stop with
> these functions and not just do that for *all* functions in postgres? I mean
> it'd not work and slow everything down,
(That seems like a good reason not to do it for all functions in
Postgres, no? I hope the slope is not all that slippery in practice.)
> but how do you define that line?
Cost/benefit. In this case, authentication hanging in an unknown place
in PAM and LDAP has caused tangible support problems. I suspect I'd
have gotten complaints if I only focused on those two places, though,
so I expanded it to the other blocking calls I could see.
Thanks,
--Jacob
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
@ 2025-03-12 22:16 Jacob Champion <[email protected]>
parent: Jacob Champion <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jacob Champion @ 2025-03-12 22:16 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Fri, Mar 7, 2025 at 10:28 AM Jacob Champion
<[email protected]> wrote:
> > I think some of the wrapped calls into library code might actually call back
> > into our code (to receive/send data), and our code then will use wait events
> > around lower level operations done as part of that.
>
> That would be a problem, agreed, but I didn't think I'd wrapped any
> callback APIs. (Admittedly I have little experience with the SSPI
> stuff.) But looking at the wrapped calls in the patch... which are you
> suspicious of?
I missed PAM_CONV, sorry. I'm worried about the sendAuthRequest()
being done there; it doesn't seem safe to potentially ereport(ERROR)
and longjmp through a PAM call stack? But I'll switch those over to
something safe or else drop that part of the patch.
Thanks,
--Jacob
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
@ 2025-03-13 16:23 Jacob Champion <[email protected]>
parent: Jacob Champion <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jacob Champion @ 2025-03-13 16:23 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Wed, Mar 12, 2025 at 3:16 PM Jacob Champion
<[email protected]> wrote:
> I missed PAM_CONV, sorry. I'm worried about the sendAuthRequest()
> being done there; it doesn't seem safe to potentially ereport(ERROR)
> and longjmp through a PAM call stack? But I'll switch those over to
> something safe or else drop that part of the patch.
PAM aside... Michael, what's your level of enthusiasm for the rest of
this patch? I was confidently, embarrassingly wrong about how
CheckPAMAuth worked, and it makes me think I need to put this down and
take a completely new crack at it in 19.
Thanks,
--Jacob
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
@ 2025-03-13 16:56 Andres Freund <[email protected]>
parent: Jacob Champion <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2025-03-13 16:56 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
Hi,
On 2025-03-13 09:23:10 -0700, Jacob Champion wrote:
> On Wed, Mar 12, 2025 at 3:16 PM Jacob Champion
> <[email protected]> wrote:
> > I missed PAM_CONV, sorry. I'm worried about the sendAuthRequest()
> > being done there; it doesn't seem safe to potentially ereport(ERROR)
> > and longjmp through a PAM call stack?
That indeed doesn't seem safe.
I am wondering if PAM is so fundamentally incompatible with handling
interrupts / a non-blocking interface that we have little choice but to
eventually remove it...
> PAM aside... Michael, what's your level of enthusiasm for the rest of this
> patch? I was confidently, embarrassingly wrong about how CheckPAMAuth
> worked, and it makes me think I need to put this down and take a completely
> new crack at it in 19.
FWIW, I continue to think that it's better to invest in making more auth
methods non-blocking, rather than adding wait events for code that could maybe
sometimes wait on different things internally.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
@ 2025-03-13 17:29 Jacob Champion <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jacob Champion @ 2025-03-13 17:29 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Thu, Mar 13, 2025 at 9:56 AM Andres Freund <[email protected]> wrote:
> I am wondering if PAM is so fundamentally incompatible with handling
> interrupts / a non-blocking interface that we have little choice but to
> eventually remove it...
Given the choice between a usually-working PAM module with known
architectural flaws, and not having PAM at all, I think many users
would rather continue using what's working for them.
> FWIW, I continue to think that it's better to invest in making more auth
> methods non-blocking, rather than adding wait events for code that could maybe
> sometimes wait on different things internally.
I think we disagree on the either/or nature of that. If I can get
proof that a certain thing is causing bugs in the wild, then I have
ammunition to fix that thing. Right now there is no visibility, and my
interest in rewriting old authentication methods without bug reports
to motivate that work is pretty low. I'm not willing to sign up for
that at the moment.
(But I do really appreciate the review. I'm just feeling crispy about
the overall result...)
Thanks,
--Jacob
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
@ 2025-03-13 17:56 Andres Freund <[email protected]>
parent: Jacob Champion <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2025-03-13 17:56 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On 2025-03-13 10:29:49 -0700, Jacob Champion wrote:
> On Thu, Mar 13, 2025 at 9:56 AM Andres Freund <[email protected]> wrote:
> > I am wondering if PAM is so fundamentally incompatible with handling
> > interrupts / a non-blocking interface that we have little choice but to
> > eventually remove it...
>
> Given the choice between a usually-working PAM module with known
> architectural flaws, and not having PAM at all, I think many users
> would rather continue using what's working for them.
authentication_timeout currently doesn't reliably work while in some auth
methods, nor does pg_terminate_backend() etc. That's IMO is rather bad from a
DOSability perspective.
The fact that some auth methods are broken like that has had a sizable
negative impact on postgres for a long time. Not just when those methods are
used, but also architecturally.
It's e.g. one of the main reasons we need the ugly escalating logic in
postmaster shutdowns to send SIGQUITs and then SIGKILL after a while, because
we don't have a reliable way of terminating backends normally. This used to
be way worse because historically postgres considered it sane (why, I have no
idea) to ereport() in timeout functions, which then occasionally lead to
backends stuck in malloc locks etc.
> > FWIW, I continue to think that it's better to invest in making more auth
> > methods non-blocking, rather than adding wait events for code that could maybe
> > sometimes wait on different things internally.
>
> I think we disagree on the either/or nature of that. If I can get
> proof that a certain thing is causing bugs in the wild, then I have
> ammunition to fix that thing.
FWIW, I've have repeatedly seen production issues due to authentication
timeout not working for some auth methods.
It's not hard to see why - e.g. a non-resonsive radius server just leaves the
backend hanging in select(). Even though it would get interrupted by signals,
we'll just retry without even checking interrupts / timeouts :(.
> Right now there is no visibility, and my interest in rewriting old
> authentication methods without bug reports to motivate that work is pretty
> low. I'm not willing to sign up for that at the moment.
Fair enough.
> (But I do really appreciate the review. I'm just feeling crispy about
> the overall result...)
Also fair enough :)
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
@ 2025-03-17 17:22 Jacob Champion <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jacob Champion @ 2025-03-17 17:22 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Thu, Mar 13, 2025 at 10:56 AM Andres Freund <[email protected]> wrote:
> > Given the choice between a usually-working PAM module with known
> > architectural flaws, and not having PAM at all, I think many users
> > would rather continue using what's working for them.
>
> authentication_timeout currently doesn't reliably work while in some auth
> methods, nor does pg_terminate_backend() etc. That's IMO is rather bad from a
> DOSability perspective.
>
> The fact that some auth methods are broken like that has had a sizable
> negative impact on postgres for a long time. Not just when those methods are
> used, but also architecturally.
Right -- I just don't think end users are going to factor that into
their choice of authentication method. If IT tells you "use this PAM
module", then... that's it.
If we remove PAM, maybe they change authentication methods... or maybe
they just don't ever upgrade Postgres again. My money's on the latter.
--
I looked into switching over to pgstat_report_activity(), but that
wasn't designed to be called in the middle of backend initialization.
It would take more work to make those calls safe/sane when `st_state
== STATE_STARTING`. I plan to mark this patchset as Withdrawn for now.
Thanks all!
--Jacob
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
@ 2025-03-21 05:53 Michael Paquier <[email protected]>
parent: Jacob Champion <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Michael Paquier @ 2025-03-21 05:53 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Mon, Mar 17, 2025 at 10:22:47AM -0700, Jacob Champion wrote:
> I looked into switching over to pgstat_report_activity(), but that
> wasn't designed to be called in the middle of backend initialization.
> It would take more work to make those calls safe/sane when `st_state
> == STATE_STARTING`. I plan to mark this patchset as Withdrawn for now.
Okay, fine by me. I had the impression that it would have been
possible to salvage some of the wait event states, but at least the
starting state showing up in pg_stat_activity will be able to provide
some information, so it's better than none. Unfortunately, I don't
have any room until the feature freeze for that.
Outside the stat report activity calls, I've been wondering if we
should add some dynamic tracking of which hba/ident entry a backend
PID is working with. For example, if we knew the file and the entry
line number, we would know on which auth method this backend is
bumping into. That maybe of course limited if someone modifies and
reloads the HBA file while a backend is stuck. Now, these files are
mostly static, and we have system views that provide the contents of
the ident and HBA files as SQL, so with a JOIN..
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2025-03-21 05:53 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-02-27 22:41 [PATCH v2] pg_wal on COW fs Alvaro Herrera <[email protected]>
2023-09-22 04:53 [PATCH v7 2/7] Row pattern recognition patch (parse/analysis). Tatsuo Ishii <[email protected]>
2023-10-25 18:06 Re: race condition in pg_class Tom Lane <[email protected]>
2023-10-26 09:52 ` Re: race condition in pg_class Smolkin Grigory <[email protected]>
2025-03-07 16:38 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 17:03 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 17:25 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 18:28 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-12 22:16 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 16:23 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 16:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-13 17:29 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 17:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-17 17:22 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-21 05:53 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible 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