Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes?
On Sat, Sep 24, 2022 at 8:24 AM Nathan Bossart wrote:
> +#ifdef WIN32
> +/*
> + * XXX: It looks like on Windows, we need an explicit lseek() call
> here
> + * despite using pwrite() implementation from win32pwrite.c.
> Otherwise
> + * an error occurs.
> + */
>
> I think this comment is too vague. Can we describe the error in more
> detail? Or better yet, can we fix it as a prerequisite to this patch set?
Although WriteFile() with a synchronous file handle and an explicit
offset doesn't use the current file position, it appears that it still
changes it. :-(
You'd think from the documentation[1] that that isn't the case, because it says:
"Considerations for working with synchronous file handles:
* If lpOverlapped is NULL, the write operation starts at the current
file position and WriteFile does not return until the operation is
complete, and the system updates the file pointer before WriteFile
returns.
* If lpOverlapped is not NULL, the write operation starts at the
offset that is specified in the OVERLAPPED structure and WriteFile
does not return until the write operation is complete. The system
updates the OVERLAPPED Internal and InternalHigh fields before
WriteFile returns."
So it's explicitly saying the file pointer is updated in the first
bullet point and not the second, but src/port/win32pwrite.c is doing
the second thing. Yet the following assertion added to Bharath's code
fails[2]:
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -221,6 +221,10 @@ dir_open_for_write(WalWriteMethod *wwmethod,
const char *pathname,
if (pad_to_size && wwmethod->compression_algorithm ==
PG_COMPRESSION_NONE)
{
ssize_t rc;
+ off_t before_offset;
+ off_t after_offset;
+
+ before_offset = lseek(fd, 0, SEEK_CUR);
rc = pg_pwritev_zeros(fd, pad_to_size);
@@ -231,6 +235,9 @@ dir_open_for_write(WalWriteMethod *wwmethod, const
char *pathname,
return NULL;
}
+ after_offset = lseek(fd, 0, SEEK_CUR);
+ Assert(before_offset == after_offset);
+
[1]
https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-writefile#synchronization-and-file-position
[2]
https://api.cirrus-ci.com/v1/artifact/task/6201051266154496/testrun/build/testrun/pg_basebackup/010_pg_basebackup/log/regress_log_010_pg_basebackup
Re: [PoC] Federated Authn/z with OAUTHBEARER
On Fri, Mar 25, 2022 at 5:00 PM Jacob Champion wrote: > v4 rebases over the latest version of the pluggable auth patchset > (included as 0001-4). Note that there's a recent conflict as > of d4781d887; use an older commit as the base (or wait for the other > thread to be updated). Here's a newly rebased v5. (They're all zipped now, which I probably should have done a while back, sorry.) - As before, 0001-4 are the pluggable auth set; they've now diverged from the official version over on the other thread [1]. - I'm not sure that 0005 is still completely coherent after the rebase, given the recent changes to jsonapi.c. But for now, the tests are green, and that should be enough to keep the conversation going. - 0008 will hopefully be obsoleted when the SYSTEM_USER proposal [2] lands. Thanks, --Jacob [1] https://www.postgresql.org/message-id/CAJxrbyxgFzfqby%2BVRCkeAhJnwVZE50%2BZLPx0JT2TDg9LbZtkCg%40mail.gmail.com [2] https://www.postgresql.org/message-id/flat/7e692b8c-0b11-45db-1cad-3afc5b574...@amazon.com v5-0004-Add-support-for-map-and-custom-auth-options.patch.gz Description: application/gzip v5-0001-Add-support-for-custom-authentication-methods.patch.gz Description: application/gzip v5-0002-Add-sample-extension-to-test-custom-auth-provider.patch.gz Description: application/gzip v5-0005-common-jsonapi-support-FRONTEND-clients.patch.gz Description: application/gzip v5-0003-Add-tests-for-test_auth_provider-extension.patch.gz Description: application/gzip v5-0006-libpq-add-OAUTHBEARER-SASL-mechanism.patch.gz Description: application/gzip v5-0009-Add-pytest-suite-for-OAuth.patch.gz Description: application/gzip v5-0010-contrib-oauth-switch-to-pluggable-auth-API.patch.gz Description: application/gzip v5-0008-Add-a-very-simple-authn_id-extension.patch.gz Description: application/gzip v5-0007-backend-add-OAUTHBEARER-SASL-mechanism.patch.gz Description: application/gzip
Re: Fix typos in code comments
On Mon, Sep 19, 2022 at 06:10:00AM -0500, Justin Pryzby wrote: > On Mon, Sep 19, 2022 at 11:05:24AM +0800, Zhang Mingli wrote: > > Good catch. There is a similar typo in doc, runtime.sgml. > > ```using TLS protocols enabled by by setting the parameter``` > > That one should be backpatched to v15. This is still needed -- "by by" -- Justin
Re: First draft of the PG 15 release notes
On 9/23/22 1:33 PM, Tom Lane wrote: "Jonathan S. Katz" writes: On 9/23/22 11:25 AM, Tom Lane wrote: I'm planning to do a final(?) pass over the v15 notes today, but I thought it'd be appropriate to push this separately. RE "final pass", there's still an errant "BACKPATCHED"[1] that still needs addressing. I didn't have a chance to verify if it was indeed backpatched. Yeah, that one indeed needs removed (and I've done so). I see a few other places where Bruce left notes about things that need more clarification. I'm just finishing a pass of "update for subsequent commits", and then I'll start on copy-editing. ACK. I will available to review during the weekend (Sunday). Jonathan OpenPGP_signature Description: OpenPGP digital signature
Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)
Hi, On 2022-09-01 08:40:54 +0200, Drouvot, Bertrand wrote: > Thanks for the suggestion, I'm coming up with this proposal in v4 attached: I pushed the bugfix / related test portion to 15, master. Thanks! I left the replication stuff out - it seemed somewhat independent. Probably will just push that to master, unless somebody thinks it should be in both? Greetings, Andres Freund
Re: libpq error message refactoring
On 23.09.22 04:45, Andres Freund wrote:
On 2022-09-22 19:27:27 -0700, Andres Freund wrote:
I just noticed it when trying to understand the linker failure - which I
still don't...
Heh, figured it out. It's inside #ifdef ENABLE_NLS. So it fails on all
platforms without NLS enabled.
Hah!
Here is an updated patch to get the CI clean. I'll look into the other
discussed issues later.
From de4cb33dcc98c7c71f21349eba650105a3385cbd Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Fri, 23 Sep 2022 16:26:16 -0400
Subject: [PATCH v2] WIP: libpq_append_error
Discussion:
https://www.postgresql.org/message-id/flat/7c0232ef-7b44-68db-599d-b327d0640...@enterprisedb.com
---
src/interfaces/libpq/fe-connect.c | 271 +++--
src/interfaces/libpq/fe-exec.c | 110
src/interfaces/libpq/fe-misc.c | 15 ++
src/interfaces/libpq/libpq-int.h | 2 +
src/interfaces/libpq/nls.mk| 4 +-
src/interfaces/libpq/pqexpbuffer.c | 27 ++-
src/interfaces/libpq/pqexpbuffer.h | 2 +
7 files changed, 186 insertions(+), 245 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 746e9b4f1efc..b99c4549d08e 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -896,8 +896,7 @@ fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
*connmember = strdup(tmp);
if (*connmember == NULL)
{
-
appendPQExpBufferStr(>errorMessage,
-
libpq_gettext("out of memory\n"));
+ libpq_append_error(conn, "out of
memory");
return false;
}
}
@@ -1079,9 +1078,8 @@ connectOptions2(PGconn *conn)
if (more || i != conn->nconnhost)
{
conn->status = CONNECTION_BAD;
- appendPQExpBuffer(>errorMessage,
- libpq_gettext("could
not match %d host names to %d hostaddr values\n"),
-
count_comma_separated_elems(conn->pghost), conn->nconnhost);
+ libpq_append_error(conn, "could not match %d host names
to %d hostaddr values",
+
count_comma_separated_elems(conn->pghost), conn->nconnhost);
return false;
}
}
@@ -1160,9 +1158,8 @@ connectOptions2(PGconn *conn)
else if (more || i != conn->nconnhost)
{
conn->status = CONNECTION_BAD;
- appendPQExpBuffer(>errorMessage,
- libpq_gettext("could
not match %d port numbers to %d hosts\n"),
-
count_comma_separated_elems(conn->pgport), conn->nconnhost);
+ libpq_append_error(conn, "could not match %d port
numbers to %d hosts",
+
count_comma_separated_elems(conn->pgport), conn->nconnhost);
return false;
}
}
@@ -1250,9 +1247,8 @@ connectOptions2(PGconn *conn)
&& strcmp(conn->channel_binding, "require") != 0)
{
conn->status = CONNECTION_BAD;
- appendPQExpBuffer(>errorMessage,
-
libpq_gettext("invalid %s value: \"%s\"\n"),
- "channel_binding",
conn->channel_binding);
+ libpq_append_error(conn, "invalid %s value: \"%s\"",
+ "channel_binding",
conn->channel_binding);
return false;
}
}
@@ -1276,9 +1272,8 @@ connectOptions2(PGconn *conn)
&& strcmp(conn->sslmode, "verify-full") != 0)
{
conn->status = CONNECTION_BAD;
- appendPQExpBuffer(>errorMessage,
-
libpq_gettext("invalid %s value: \"%s\"\n"),
- "sslmode",
conn->sslmode);
+ libpq_append_error(conn, "invalid %s value: \"%s\"",
+ "sslmode",
conn->sslmode);
return false;
}
@@ -1297,9 +1292,8 @@ connectOptions2(PGconn *conn)
case 'r': /* "require" */
case 'v':
Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes?
+PGAlignedXLogBlock zbuffer; + +memset(zbuffer.data, 0, XLOG_BLCKSZ); This seems excessive for only writing a single byte. +#ifdef WIN32 +/* + * XXX: It looks like on Windows, we need an explicit lseek() call here + * despite using pwrite() implementation from win32pwrite.c. Otherwise + * an error occurs. + */ I think this comment is too vague. Can we describe the error in more detail? Or better yet, can we fix it as a prerequisite to this patch set? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Summary function for pg_buffercache
Hi Andres, Adjusted the patch so that it will work with meson now. Also addressed your other reviews as well. I hope explanations in comments/docs are better now. Best, Melih v11-0001-Added-pg_buffercache_summary-function.patch Description: Binary data
Re: Pluggable toaster
Hi hackers! Cfbot is not happy with previous patchset, so I'm attaching new one, rebased onto current master (15b4). Also providing patch with documentation package, and the second one contains large README.toastapi file providing additional in-depth docs for developers. Comments would be greatly appreciated. Also, after checking patch sources I have a strong opinion that it needs some refactoring - move all files related to TOAST implementation into new folder /backend/access/toast where Generic (default) Toaster resides. Patchset consists of: v15-0001-toaster-interface.patch - Pluggable TOAST API interface along with reference TOAST mechanics; v15-0002-toaster-default.patch - Default TOAST re-implemented using Toaster API; v15-0003-toaster-docs.patch - Pluggable TOAST API documentation package On Tue, Sep 13, 2022 at 7:50 PM Jacob Champion wrote: > On Mon, Sep 12, 2022 at 11:45 PM Nikita Malakhov > wrote: > > It would be more clear for complex data types like JSONB, where > developers could > > need some additional functionality to work with internal representation > of data type, > > and its full potential is revealed in our JSONB toaster extension. The > JSONB toaster > > is still in development but we plan to make it available soon. > > Okay. It'll be good to have that, because as it is now it's hard to > see the whole picture. > > > On installing dummy_toaster contrib: I've just checked it by making a > patch from commit > > and applying onto my clone of master and 2 patches provided in previous > email without > > any errors and sll checks passed - applying with git am, configure with > debug, cassert, > > depend and enable-tap-tests flags and run checks. > > Please advice what would cause such a behavior? > > I don't think the default pg_upgrade tests will upgrade contrib > objects (there are instructions in src/bin/pg_upgrade/TESTING that > cover manual dumps, if you prefer that method). My manual steps were > roughly > > =# CREATE EXTENSION dummy_toaster; > =# CREATE TABLE test (t TEXT > STORAGE external > TOASTER dummy_toaster_handler); > =# \q > $ initdb -D newdb > $ pg_ctl -D olddb stop > $ pg_upgrade -b /bin -B /bin -d > ./olddb -D ./newdb > > (where /bin is on the PATH, so we're using the right > binaries). > > Thanks, > --Jacob > -- Regards, Nikita Malakhov Postgres Professional https://postgrespro.ru/ v15-0002-toaster-default.patch.gz Description: GNU Zip compressed data v15-0003-toaster-docs.patch.gz Description: GNU Zip compressed data v15-0001-toaster-interface.patch.gz Description: GNU Zip compressed data
Re: O(n) tasks cause lengthy startups and checkpoints
On Fri, Sep 02, 2022 at 03:07:44PM -0700, Nathan Bossart wrote:
> And another.
v11 adds support for building with meson.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 56c9ff2bf1a6524518b62193c0da02372f9674a1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 5 Jan 2022 19:24:22 +
Subject: [PATCH v11 1/6] Introduce custodian.
The custodian process is a new auxiliary process that is intended
to help offload tasks could otherwise delay startup and
checkpointing. This commit simply adds the new process; it does
not yet do anything useful.
---
src/backend/postmaster/Makefile | 1 +
src/backend/postmaster/auxprocess.c | 8 +
src/backend/postmaster/custodian.c | 383
src/backend/postmaster/meson.build | 1 +
src/backend/postmaster/postmaster.c | 44 ++-
src/backend/storage/ipc/ipci.c | 3 +
src/backend/storage/lmgr/proc.c | 1 +
src/backend/utils/activity/wait_event.c | 3 +
src/backend/utils/init/miscinit.c | 3 +
src/include/miscadmin.h | 3 +
src/include/postmaster/custodian.h | 32 ++
src/include/storage/proc.h | 11 +-
src/include/utils/wait_event.h | 1 +
13 files changed, 489 insertions(+), 5 deletions(-)
create mode 100644 src/backend/postmaster/custodian.c
create mode 100644 src/include/postmaster/custodian.h
diff --git a/src/backend/postmaster/Makefile b/src/backend/postmaster/Makefile
index 3a794e54d6..e1e1d1123f 100644
--- a/src/backend/postmaster/Makefile
+++ b/src/backend/postmaster/Makefile
@@ -18,6 +18,7 @@ OBJS = \
bgworker.o \
bgwriter.o \
checkpointer.o \
+ custodian.o \
fork_process.o \
interrupt.o \
pgarch.o \
diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index 7765d1c83d..c275271c95 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -20,6 +20,7 @@
#include "pgstat.h"
#include "postmaster/auxprocess.h"
#include "postmaster/bgwriter.h"
+#include "postmaster/custodian.h"
#include "postmaster/startup.h"
#include "postmaster/walwriter.h"
#include "replication/walreceiver.h"
@@ -74,6 +75,9 @@ AuxiliaryProcessMain(AuxProcType auxtype)
case CheckpointerProcess:
MyBackendType = B_CHECKPOINTER;
break;
+ case CustodianProcess:
+ MyBackendType = B_CUSTODIAN;
+ break;
case WalWriterProcess:
MyBackendType = B_WAL_WRITER;
break;
@@ -153,6 +157,10 @@ AuxiliaryProcessMain(AuxProcType auxtype)
CheckpointerMain();
proc_exit(1);
+ case CustodianProcess:
+ CustodianMain();
+ proc_exit(1);
+
case WalWriterProcess:
WalWriterMain();
proc_exit(1);
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
new file mode 100644
index 00..e90f5d0d1f
--- /dev/null
+++ b/src/backend/postmaster/custodian.c
@@ -0,0 +1,383 @@
+/*-
+ *
+ * custodian.c
+ *
+ * The custodian process handles a variety of non-critical tasks that might
+ * otherwise delay startup, checkpointing, etc. Offloaded tasks should not
+ * be synchronous (e.g., checkpointing shouldn't wait for the custodian to
+ * complete a task before proceeding). However, tasks can be synchronously
+ * executed when necessary (e.g., single-user mode). The custodian is not
+ * an essential process and can shutdown quickly when requested. The
+ * custodian only wakes up to perform its tasks when its latch is set.
+ *
+ *
+ * Copyright (c) 2022, PostgreSQL Global Development Group
+ *
+ *
+ * IDENTIFICATION
+ * src/backend/postmaster/custodian.c
+ *
+ *-
+ */
+#include "postgres.h"
+
+#include "libpq/pqsignal.h"
+#include "pgstat.h"
+#include "postmaster/custodian.h"
+#include "postmaster/interrupt.h"
+#include "storage/bufmgr.h"
+#include "storage/condition_variable.h"
+#include "storage/fd.h"
+#include "storage/proc.h"
+#include "storage/procsignal.h"
+#include "storage/smgr.h"
+#include "utils/memutils.h"
+
+static void DoCustodianTasks(bool retry);
+static CustodianTask CustodianGetNextTask(void);
+static void CustodianEnqueueTask(CustodianTask task);
+static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+
+typedef struct
+{
+ slock_t cust_lck;
+
+ CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
+ int task_queue_head;
+} CustodianShmemStruct;
+
+static CustodianShmemStruct *CustodianShmem;
+
+typedef void (*CustodianTaskFunction) (void);
+typedef void (*CustodianTaskHandleArg) (Datum arg);
+
+struct cust_task_funcs_entry
+{
+ CustodianTask task;
+ CustodianTaskFunction task_func; /* performs task */
+ CustodianTaskHandleArg handle_arg_func; /* handles additional info in request */
+};
+
+/*
+ * Add new tasks here.
+ *
+ * task_func is the logic that will be executed via DoCustodianTasks() when the
+ * matching
Re: First draft of the PG 15 release notes
"Jonathan S. Katz" writes: > On 9/23/22 11:25 AM, Tom Lane wrote: >> I'm planning to do a final(?) pass over the v15 notes today, >> but I thought it'd be appropriate to push this separately. > RE "final pass", there's still an errant "BACKPATCHED"[1] that still > needs addressing. I didn't have a chance to verify if it was indeed > backpatched. Yeah, that one indeed needs removed (and I've done so). I see a few other places where Bruce left notes about things that need more clarification. I'm just finishing a pass of "update for subsequent commits", and then I'll start on copy-editing. regards, tom lane
Re: First draft of the PG 15 release notes
On 9/23/22 11:25 AM, Tom Lane wrote: "Jonathan S. Katz" writes: Adjusted to be similar to your suggestion. Updated patch attached. I pushed this with a bit more copy-editing. I'm planning to do a final(?) pass over the v15 notes today, but I thought it'd be appropriate to push this separately. Thanks! RE "final pass", there's still an errant "BACKPATCHED"[1] that still needs addressing. I didn't have a chance to verify if it was indeed backpatched. Jonathan [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=doc/src/sgml/release-15.sgml;hb=refs/heads/REL_15_STABLE#l460 OpenPGP_signature Description: OpenPGP digital signature
Re: archive modules
On Fri, Sep 23, 2022 at 05:58:42AM -0400, Peter Eisentraut wrote: > On 15.09.22 00:27, Nathan Bossart wrote: >> Both archive_command and archive_library are PGC_SIGHUP, so IIUC that >> wouldn't be sufficient. I attached a quick sketch that seems to provide >> the desired behavior. It's nowhere near committable yet, but it >> demonstrates what I'm thinking. > > What is the effect of issuing a warning like in this patch? Would it just > not archive anything until the configuration is fixed? I'm not sure what > behavior you are going for; it's a bit hard to imagine from just reading the > patch. Yes, it will halt archiving and emit a WARNING, just like what happens on released versions when you leave archive_command empty. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: First draft of the PG 15 release notes
"Jonathan S. Katz" writes: > Adjusted to be similar to your suggestion. Updated patch attached. I pushed this with a bit more copy-editing. I'm planning to do a final(?) pass over the v15 notes today, but I thought it'd be appropriate to push this separately. regards, tom lane
Re: Temporary file access API
Hi, John Morris wrote: > I’m a latecomer to the discussion, but as a word of introduction, I’m working > with Stephen, and I have started looking over the temp file proposal with the > idea of helping it move along. > > I’ll start by summarizing the temp file proposal and its goals. > > From a high level, the proposed code: > > * Creates an fread/fwrite replacement (BufFileStream) for buffering data to a > single file. > > * Updates BufFile, which reads/writes a set of files, to use BufFileStream > internally. > > * Does not impact the normal (page cached) database I/O. > > * Updates all the other places where fread/fwrite and read/write are used. Not really all, just those where the change seemed reasonable (i.e. where it does not make the code more complex) > * Creates and removes transient files. The "stream API" is rather an additional layer on top of files that user needs to create / remove at lower level. > I see the following goals: > > * Unify all the “other” file accesses into a single, consistent API. > > * Integrate with VFDs. > > * Integrate transient files with transactions and tablespaces. If you mean automatic closing/deletion of files on transaction end, this is also the lower level thing that I didn't try to change. > * Create a consolidated framework where features like encryption and > compression can be more easily added. > > * Maintain good streaming performance. > > Is this a fair description of the proposal? Basically that's it. > For myself, I’d like to map out how features like compression and encryption > would fit into the framework, more as a sanity check than anything else, and > I’d like to look closer at some of the implementation details. But at the > moment, I want to make sure I have the > proper high-level view of the temp file proposal. I think the high level design (i.e. how the API should be used) still needs discussion. In particular, I don't know whether it should aim at the encryption adoption or not. If it does, then it makes sense to base it on buffile.c, because encryption essentially takes place in memory. But if buffering itself (w/o encryption) is not really useful at other places (see Robert's comments in [1]), then we can design something simpler, w/o touching buffile.c (which, in turn, won't be usable for encryption, compression or so). So I think that code simplification and easy adoption of in-memory data changes (such as encryption or compression) are two rather distinct goals. admit that I'm running out of ideas how to develop a framework that'd be useful for both. [1] https://www.postgresql.org/message-id/CA%2BTgmoZWP8UtkNVLd75Qqoh9VGOVy_0xBK%2BSHZAdNvnfaikKsQ%40mail.gmail.com > From: Robert Haas > Date: Wednesday, September 21, 2022 at 11:54 AM > To: Antonin Houska > Cc: PostgreSQL Hackers , Peter Eisentraut > , Stephen Frost > Subject: Re: Temporary file access API > > On Mon, Aug 8, 2022 at 2:26 PM Antonin Houska wrote: > > > I don't think that (3) is a separate advantage from (1) and (2), so I > > > don't have anything separate to say about it. > > > > I thought that the uncontrollable buffer size is one of the things you > > complaint about in > > > > https://www.postgresql.org/message-id/CA+TgmoYGjN_f=fcerx49bzjhng+gocty+a+xhnrwcvvdy8u...@mail.gmail.com > > Well, I think that email is mostly complaining about there being no > buffering at all in a situation where it would be advantageous to do > some buffering. But that particular code I believe is gone now because > of the shared-memory stats collector, and when looking through the > patch set, I didn't see that any of the cases that it touched were > similar to that one. > > -- > Robert Haas > EDB: http://www.enterprisedb.com > -- Antonin Houska Web: https://www.cybertec-postgresql.com
Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?)
On Fri, Sep 23, 2022 at 06:02:24AM +0530, Bharath Rupireddy wrote: > PSA v12 patch with the above review comments addressed. I've read this one, and nothing is standing out at quick glance, so that looks rather reasonable to me. I should be able to spend more time on that at the beginning of next week, and maybe apply it. -- Michael signature.asc Description: PGP signature
Re: archive modules
On 15.09.22 00:27, Nathan Bossart wrote: Both archive_command and archive_library are PGC_SIGHUP, so IIUC that wouldn't be sufficient. I attached a quick sketch that seems to provide the desired behavior. It's nowhere near committable yet, but it demonstrates what I'm thinking. What is the effect of issuing a warning like in this patch? Would it just not archive anything until the configuration is fixed? I'm not sure what behavior you are going for; it's a bit hard to imagine from just reading the patch.
Re: RFC: Logging plan of the running query
Sorry, I wrote confusingly at that time. No, I suggested adding comment about the explanation of HandleLogQueryPlanInterrupt() only in the explain.h and not removing from the explain.c. I seemed to be necessary separating declaration function for 'explaining feature' of executed query from our logging plan of the running query interrupts function. But now, I doubt it. I'm not sure I understand your comment correctly, do you mean HandleLogQueryPlanInterrupt() should not be placed in explain.c? Thank you for having reminded about this function and I looked at ProcessLogMemoryContextInterrupt() declaration. I'm noticed comments in the memutils.h are missed tooю Description of this function is written only in mcxt.c. However, given that ProcesLogMemoryContextInterrupt(), which similarly handles interrupts for pg_log_backend_memory_contexts(), is located in mcxt.c, I also think current location might be acceptable. So I think you are right and the comment about the explanation of HandleLogQueryPlanInterrupt() written in explain.h is redundant. I feel this comment is unnecessary since the explanation of HandleLogQueryPlanInterrupt() is written in explain.c and no functions in explain.h have comments in it. Regards, -- Alena Rybakina Postgres Professional
Re: LogwrtResult contended spinlock
On 2022-Jul-28, Alvaro Herrera wrote: > v10 is just a trivial rebase. No changes. Moved to next commitfest. I realized that because of commit e369f3708636 this change is no longer as critical as it used to be, so I'm withdrawing this patch from the commitfest. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "No necesitamos banderas No reconocemos fronteras" (Jorge González)
Re: binary version of pg_current_wal_insert_lsn and pg_walfile_name functions
On Fri, Sep 23, 2022 at 12:24 PM Ashutosh Sharma wrote: > > PFA v2 patch. > > Changes in the v2 patch: > > - Reuse the existing get_controlfile function in > src/common/controldata_utils.c instead of adding a new one. > > - Set env variable PGDATA with the data directory specified by the user. > Forgot to attach the patch with above changes. Here it is. -- With Regards, Ashutosh Sharma. v2-0001-Enhance-pg_waldump-to-show-latest-LSN-and-the-corrre.patch Description: Binary data
Re: binary version of pg_current_wal_insert_lsn and pg_walfile_name functions
PFA v2 patch. Changes in the v2 patch: - Reuse the existing get_controlfile function in src/common/controldata_utils.c instead of adding a new one. - Set env variable PGDATA with the data directory specified by the user. -- With Regards, Ashutosh Sharma.
Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes?
On Wed, Sep 21, 2022 at 4:30 AM Nathan Bossart wrote:
>
> 0001 looks reasonable to me.
Thanks for reviewing.
> +errno = 0;
> +rc = pg_pwritev_zeros(fd, pad_to_size);
>
> Do we need to reset errno? pg_pwritev_zeros() claims to set errno
> appropriately.
Right, pg_pwritev_zeros(), (rather pg_pwritev_with_retry() ensures
that pwritev() or pwrite()) sets the correct errno, please see
Thomas's comments [1] as well. Removed it.
> +/*
> + * PWRITEV_BLCKSZ is same as XLOG_BLCKSZ for now, however it may change if
> + * writing more bytes per pg_pwritev_with_retry() call is proven to be more
> + * performant.
> + */
> +#define PWRITEV_BLCKSZ XLOG_BLCKSZ
>
> This seems like something we should sort out now instead of leaving as
> future work. Given your recent note, I think we should just use
> XLOG_BLCKSZ and PGAlignedXLogBlock and add a comment about the performance
> findings with different buffer sizes.
Agreed. Removed the new structure and added a comment.
Another change that I had to do was to allow lseek() even after
pwrite() (via pg_pwritev_zeros()) on Windows in walmethods.c. Without
this, the regression tests start to fail on Windows. And I figured out
that it's not an issue with this patch, it looks like an issue with
pwrite() implementation in win32pwrite.c, see the failure here [2], I
plan to start a separate thread to discuss this.
Please review the attached v4 patch set further.
[1]
https://www.postgresql.org/message-id/CA%2BhUKGJKwUrpP0igOFAv5khj3dbHvfWqLzFeo7vtNzDYXv_YZw%40mail.gmail.com
[2]
https://github.com/BRupireddy/postgres/tree/use_pwrite_without_lseek_on_windiws
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From c60026519becfc513dc06ddf889caa3b4b0fade3 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Fri, 23 Sep 2022 00:47:56 +
Subject: [PATCH v4] Move pg_pwritev_with_retry() to file_utils.c
Move pg_pwritev_with_retry() from file/fd.c to common/file_utils.c
so that non-backend code or FRONTEND cases can also use it.
---
src/backend/storage/file/fd.c | 65 -
src/common/file_utils.c | 65 +
src/include/common/file_utils.h | 7
src/include/storage/fd.h| 6 ---
4 files changed, 72 insertions(+), 71 deletions(-)
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 073dab2be5..75ba178dfb 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -93,7 +93,6 @@
#include "common/pg_prng.h"
#include "miscadmin.h"
#include "pgstat.h"
-#include "port/pg_iovec.h"
#include "portability/mem.h"
#include "postmaster/startup.h"
#include "storage/fd.h"
@@ -3738,67 +3737,3 @@ data_sync_elevel(int elevel)
{
return data_sync_retry ? elevel : PANIC;
}
-
-/*
- * A convenience wrapper for pwritev() that retries on partial write. If an
- * error is returned, it is unspecified how much has been written.
- */
-ssize_t
-pg_pwritev_with_retry(int fd, const struct iovec *iov, int iovcnt, off_t offset)
-{
- struct iovec iov_copy[PG_IOV_MAX];
- ssize_t sum = 0;
- ssize_t part;
-
- /* We'd better have space to make a copy, in case we need to retry. */
- if (iovcnt > PG_IOV_MAX)
- {
- errno = EINVAL;
- return -1;
- }
-
- for (;;)
- {
- /* Write as much as we can. */
- part = pwritev(fd, iov, iovcnt, offset);
- if (part < 0)
- return -1;
-
-#ifdef SIMULATE_SHORT_WRITE
- part = Min(part, 4096);
-#endif
-
- /* Count our progress. */
- sum += part;
- offset += part;
-
- /* Step over iovecs that are done. */
- while (iovcnt > 0 && iov->iov_len <= part)
- {
- part -= iov->iov_len;
- ++iov;
- --iovcnt;
- }
-
- /* Are they all done? */
- if (iovcnt == 0)
- {
- /* We don't expect the kernel to write more than requested. */
- Assert(part == 0);
- break;
- }
-
- /*
- * Move whatever's left to the front of our mutable copy and adjust
- * the leading iovec.
- */
- Assert(iovcnt > 0);
- memmove(iov_copy, iov, sizeof(*iov) * iovcnt);
- Assert(iov->iov_len > part);
- iov_copy[0].iov_base = (char *) iov_copy[0].iov_base + part;
- iov_copy[0].iov_len -= part;
- iov = iov_copy;
- }
-
- return sum;
-}
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index df4d6d240c..4af216e56c 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -28,6 +28,7 @@
#ifdef FRONTEND
#include "common/logging.h"
#endif
+#include "port/pg_iovec.h"
#ifdef FRONTEND
@@ -460,3 +461,67 @@ get_dirent_type(const char *path,
return result;
}
+
+/*
+ * A convenience wrapper for pwritev() that retries on partial write. If an
+ * error is returned, it is unspecified how much has been written.
+ */
+ssize_t
+pg_pwritev_with_retry(int fd, const struct iovec *iov, int iovcnt, off_t offset)
+{
+ struct iovec iov_copy[PG_IOV_MAX];
+ ssize_t sum = 0;
+ ssize_t part;
+
+ /* We'd better have space to make a copy, in case we need to
