RE: Support tab completion for upper character inputs in psql
On Saturday, January 29, 2022 7:17 AM, Tom Lane wrote: > Sigh ... per the cfbot, this was already blindsided by 95787e849. > As I said, I don't want to sit on this for very long. Thanks for your V16 patch, I tested it. The results LGTM. Regards, Tang
RE: Support tab completion for upper character inputs in psql
On Saturday, January 29, 2022 1:03 AM, Tom Lane wrote: > "tanghy.f...@fujitsu.com" writes: > > I did some tests on it and here are something cases I feel we need to > > confirm > > whether they are suitable. > > > 1) postgres=# create table atest(id int, "iD" int, "ID" int); > > 2) CREATE TABLE > > 3) postgres=# alter table atest rename i[TAB] > > 4) id"iD" > > 5) postgres=# alter table atest rename I[TAB] > > 6) id"iD" > > > The tab completion for 5) ignored "ID", is that correct? > > Perhaps I misunderstood your original complaint, but what I thought > you were unhappy about was that unquoted ID is a legal spelling of > "id" and so I ought to be willing to complete that. These > examples with case variants of the same word are of some interest, > but people aren't really going to create tables with these sorts of > names, so we shouldn't let them drive the design IMO. > > Anyway, the existing behavior for these examples is > > alter table atest rename i --- completes immediately to id > alter table atest rename I --- offers nothing > > It's certainly arguable that the first case is right as-is and we > shouldn't change it. I think that could be handled by tweaking my > patch so that it wouldn't offer completions that start with a quote > unless the input word does. That would also cause I to complete > immediately to id, which is arguably fine. > > > I think what we are trying to do is to ease the burden of typing double > > quote > for user. > > I'm not thinking about it that way at all. To me, the goal is to make > tab completion do something sensible when presented with legal variant > spellings of a word. The two cases where it currently fails to do > that are (1) unquoted input that needs to be downcased, and (2) input > that is quoted when it doesn't strictly need to be. > > To the extent that we can supply a required quote that the user > failed to type, that's fine, but it's not a primary goal of the patch. > Examples like these make me question whether it's even something we > want; it's resulting in extraneous matches that people might find more > annoying than helpful. Now I *think* that these aren't realistic > cases and that in real cases adding quotes will be helpful more often > than not, but it's debatable. > > > One the other hand, I'm not so comfortable with the output of "iD" in line > 13. > > If user doesn't type double quote, why we add double quote to the output? > > That's certainly a valid argument. > > > Could we make the output of 13) like below? > > 12) postgres=# alter table atest rename i[TAB] > > ??) id iD > > That doesn't seem sensible at all. Thanks for your kindly explanation. I'm fine with the current tap completion style with your V16 patch. Regards, Tang
Re: CREATEROLE and role ownership hierarchies
> On Jan 25, 2022, at 12:44 PM, Stephen Frost wrote: > > I agree that CREATEROLE is overpowered and that the goal of this should > be to provide a way for roles to be created and dropped that doesn't > give the user who has that power everything that CREATEROLE currently > does. I'm attaching a patch that attempts to fix CREATEROLE without any connection to role ownership. > The point I was making is that the concept of role ownership > isn't intrinsically linked to that and is, therefore, as you say, gravy. I agree, they aren't intrinsically linked, though the solution to one might interact in some ways with the solution to the other. > That isn't to say that I'm entirely against the role ownership idea but > I'd want it to be focused on the goal of providing ways of creating and > dropping users and otherwise performing that kind of administration and > that doesn't require the specific change to make owners be members of > all roles they own and automatically have all privileges of those roles > all the time. The attached WIP patch attempts to solve most of the CREATEROLE problems but not the problem of which role who can drop which other role. That will likely require an ownership concept. The main idea here is that having CREATEROLE doesn't give you ADMIN on roles, nor on role attributes. For role attributes, the syntax has been extended. An excerpt from the patch's regression test illustrates some of that concept: -- ok, superuser can create a role that can create login replication users, but -- cannot itself login, nor perform replication CREATE ROLE regress_role_repladmin CREATEROLE WITHOUT ADMIN OPTION -- can create roles, but cannot give it away NOCREATEDB WITHOUT ADMIN OPTION -- cannot create db, nor give it away NOLOGIN WITH ADMIN OPTION -- cannot log in, but can give it away NOREPLICATION WITH ADMIN OPTION -- cannot replicate, but can give it away NOBYPASSRLS WITHOUT ADMIN OPTION; -- cannot bypassrls, nor give it away -- ok, superuser can create a role with CREATEROLE but restrict give-aways CREATE ROLE regress_role_minoradmin NOSUPERUSER -- WITHOUT ADMIN OPTION is implied CREATEROLE WITHOUT ADMIN OPTION NOCREATEDB WITHOUT ADMIN OPTION NOLOGIN WITHOUT ADMIN OPTION NOREPLICATION -- WITHOUT ADMIN OPTION is implied NOBYPASSRLS -- WITHOUT ADMIN OPTION is implied NOINHERIT WITHOUT ADMIN OPTION CONNECTION LIMIT NONE WITHOUT ADMIN OPTION VALID ALWAYS WITHOUT ADMIN OPTION PASSWORD NULL WITHOUT ADMIN OPTION; -- fail, having CREATEROLE is not enough to create roles in privileged roles SET SESSION AUTHORIZATION regress_role_minoradmin; CREATE ROLE regress_nosuch_read_all_data IN ROLE pg_read_all_data; ERROR: must have admin option on role "pg_read_all_data" -- fail, cannot change attributes without ADMIN for them SET SESSION AUTHORIZATION regress_role_minoradmin; ALTER ROLE regress_role_login LOGIN; ERROR: must have admin on login to change login attribute ALTER ROLE regress_role_login NOLOGIN; ERROR: must have admin on login to change login attribute Whether "WITH ADMIN OPTION" or "WITHOUT ADMIN OPTION" is implied hinges on whether the role is given CREATEROLE. That hackery is necessary to preserve backwards compatibility. If we don't care about compatibility, I could change the patch to make "WITHOUT ADMIN OPTION" implied for all attributes when not specified. I'd appreciate feedback on the direction this patch is going. v8-0001-Adding-admin-options-for-role-attributes.patch.WIP Description: Binary data — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: archive modules
On Sat, Jan 29, 2022 at 04:31:48PM -0800, Nathan Bossart wrote:
> On Sat, Jan 29, 2022 at 12:50:18PM -0800, Nathan Bossart wrote:
>> Here is a new revision. I've moved basic_archive to contrib, hardened it
>> as suggested, and added shutdown support for archive modules.
>
> cfbot was unhappy with v14, so here's another attempt. One other change I
> am pondering is surrounding pgarch_MainLoop() with PG_TRY/PG_FINALLY so
> that we can also call the shutdown callback in the event of an ERROR. This
> might be necessary for an archive module that uses background workers.
Ugh. Apologies for the noise. cfbot still isn't happy, so here's yet
another attempt. This new patch set also ensures the shutdown callback is
called when the archiver process exits.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From aab6a7cbb2ae7d0d181062f972d2e559bbd4cef6 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 19 Nov 2021 01:04:41 +
Subject: [PATCH v16 1/3] Introduce archive modules infrastructure.
---
src/backend/access/transam/xlog.c | 2 +-
src/backend/postmaster/pgarch.c | 111 --
src/backend/postmaster/shell_archive.c| 24 +++-
src/backend/utils/init/miscinit.c | 1 +
src/backend/utils/misc/guc.c | 12 +-
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/access/xlog.h | 1 -
src/include/postmaster/pgarch.h | 52 +++-
8 files changed, 189 insertions(+), 15 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index dfe2a0bcce..958220c495 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8831,7 +8831,7 @@ ShutdownXLOG(int code, Datum arg)
* process one more time at the end of shutdown). The checkpoint
* record will go to the next XLOG file and won't be archived (yet).
*/
- if (XLogArchivingActive() && XLogArchiveCommandSet())
+ if (XLogArchivingActive())
RequestXLogSwitch(false);
CreateCheckPoint(CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_IMMEDIATE);
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 6e3fcedc97..865f1930df 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -89,6 +89,8 @@ typedef struct PgArchData
slock_t arch_lck;
} PgArchData;
+char *XLogArchiveLibrary = "";
+
/* --
* Local data
@@ -96,6 +98,8 @@ typedef struct PgArchData
*/
static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
+static ArchiveModuleCallbacks ArchiveContext;
+
/*
* Stuff for tracking multiple files to archive from each scan of
@@ -140,6 +144,8 @@ static void pgarch_archiveDone(char *xlog);
static void pgarch_die(int code, Datum arg);
static void HandlePgArchInterrupts(void);
static int ready_file_comparator(Datum a, Datum b, void *arg);
+static void LoadArchiveLibrary(void);
+static void call_archive_module_shutdown_callback(int code, Datum arg);
/* Report shared memory space needed by PgArchShmemInit */
Size
@@ -244,7 +250,16 @@ PgArchiverMain(void)
arch_files->arch_heap = binaryheap_allocate(NUM_FILES_PER_DIRECTORY_SCAN,
ready_file_comparator, NULL);
- pgarch_MainLoop();
+ /* Load the archive_library. */
+ LoadArchiveLibrary();
+
+ PG_ENSURE_ERROR_CLEANUP(call_archive_module_shutdown_callback, 0);
+ {
+ pgarch_MainLoop();
+ }
+ PG_END_ENSURE_ERROR_CLEANUP(call_archive_module_shutdown_callback, 0);
+
+ call_archive_module_shutdown_callback(0, 0);
proc_exit(0);
}
@@ -407,11 +422,12 @@ pgarch_ArchiverCopyLoop(void)
*/
HandlePgArchInterrupts();
- /* can't do anything if no command ... */
- if (!XLogArchiveCommandSet())
+ /* can't do anything if not configured ... */
+ if (ArchiveContext.check_configured_cb != NULL &&
+!ArchiveContext.check_configured_cb())
{
ereport(WARNING,
- (errmsg("archive_mode enabled, yet archive_command is not set")));
+ (errmsg("archive_mode enabled, yet archiving is not configured")));
return;
}
@@ -492,7 +508,7 @@ pgarch_ArchiverCopyLoop(void)
/*
* pgarch_archiveXlog
*
- * Invokes system(3) to copy one archive file to wherever it should go
+ * Invokes archive_file_cb to copy one archive file to wherever it should go
*
* Returns true if successful
*/
@@ -509,7 +525,7 @@ pgarch_archiveXlog(char *xlog)
snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
set_ps_display(activitymsg);
- ret = shell_archive_file(xlog, pathname);
+ ret = ArchiveContext.archive_file_cb(xlog, pathname);
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
@@ -759,13 +775,90 @@ HandlePgArchInterrupts(void)
if (ProcSignalBarrierPending)
ProcessProcSignalBarrier();
+ /* Perform logging of memory contexts of this process */
+ if (LogMemoryContextPending)
+ ProcessLogMemoryContextInterrupt();
+
if
[PATCH] nodeindexscan with reorder memory leak
Hey! I was investigating a leak reported in the PostGIS issues tracker [1] which led me to the Postgres side where the problem really is. The leak is reproducible with query from original ticket [1]: WITH latitudes AS ( SELECT generate_series AS latitude FROM generate_series(-90, 90, 0.1) ), longitudes AS ( SELECT generate_series AS longitude FROM generate_series(-180, 180, 0.1) ), points AS ( SELECT ST_SetSRID(ST_Point(longitude, latitude), 4326)::geography AS geog FROM latitudes CROSS JOIN longitudes ) SELECT geog, ( SELECT name FROM ne_110m_admin_0_countries AS ne ORDER BY p.geog <-> ne.geog LIMIT 1 ) FROM points AS p ; The leak is only noticeable when index scan with reorder happens as part of subquery plan which is explained by the fact that heap tuples cloned in reorderqueue_push are not freed during flush of reorder queue in ExecReScanIndex. [1] https://trac.osgeo.org/postgis/ticket/4720 0001-nodeindexscan_with_reorder_memory_leak.patch Description: Binary data
Re: archive modules
On Sat, Jan 29, 2022 at 12:50:18PM -0800, Nathan Bossart wrote:
> Here is a new revision. I've moved basic_archive to contrib, hardened it
> as suggested, and added shutdown support for archive modules.
cfbot was unhappy with v14, so here's another attempt. One other change I
am pondering is surrounding pgarch_MainLoop() with PG_TRY/PG_FINALLY so
that we can also call the shutdown callback in the event of an ERROR. This
might be necessary for an archive module that uses background workers.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From f62fea53b93ba7181dfe084b4100eba59eb82aaa Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 19 Nov 2021 01:04:41 +
Subject: [PATCH v15 1/3] Introduce archive modules infrastructure.
---
src/backend/access/transam/xlog.c | 2 +-
src/backend/postmaster/pgarch.c | 93 +--
src/backend/postmaster/shell_archive.c| 24 -
src/backend/utils/init/miscinit.c | 1 +
src/backend/utils/misc/guc.c | 12 ++-
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/access/xlog.h | 1 -
src/include/postmaster/pgarch.h | 52 ++-
8 files changed, 172 insertions(+), 14 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index dfe2a0bcce..958220c495 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8831,7 +8831,7 @@ ShutdownXLOG(int code, Datum arg)
* process one more time at the end of shutdown). The checkpoint
* record will go to the next XLOG file and won't be archived (yet).
*/
- if (XLogArchivingActive() && XLogArchiveCommandSet())
+ if (XLogArchivingActive())
RequestXLogSwitch(false);
CreateCheckPoint(CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_IMMEDIATE);
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 6e3fcedc97..d4a7ca97ca 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -89,6 +89,8 @@ typedef struct PgArchData
slock_t arch_lck;
} PgArchData;
+char *XLogArchiveLibrary = "";
+
/* --
* Local data
@@ -96,6 +98,8 @@ typedef struct PgArchData
*/
static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
+static ArchiveModuleCallbacks ArchiveContext;
+
/*
* Stuff for tracking multiple files to archive from each scan of
@@ -140,6 +144,7 @@ static void pgarch_archiveDone(char *xlog);
static void pgarch_die(int code, Datum arg);
static void HandlePgArchInterrupts(void);
static int ready_file_comparator(Datum a, Datum b, void *arg);
+static void LoadArchiveLibrary(void);
/* Report shared memory space needed by PgArchShmemInit */
Size
@@ -236,6 +241,11 @@ PgArchiverMain(void)
*/
PgArch->pgprocno = MyProc->pgprocno;
+ /*
+ * Load the archive_library.
+ */
+ LoadArchiveLibrary();
+
/* Create workspace for pgarch_readyXlog() */
arch_files = palloc(sizeof(struct arch_files_state));
arch_files->arch_files_size = 0;
@@ -407,11 +417,12 @@ pgarch_ArchiverCopyLoop(void)
*/
HandlePgArchInterrupts();
- /* can't do anything if no command ... */
- if (!XLogArchiveCommandSet())
+ /* can't do anything if not configured ... */
+ if (ArchiveContext.check_configured_cb != NULL &&
+!ArchiveContext.check_configured_cb())
{
ereport(WARNING,
- (errmsg("archive_mode enabled, yet archive_command is not set")));
+ (errmsg("archive_mode enabled, yet archiving is not configured")));
return;
}
@@ -492,7 +503,7 @@ pgarch_ArchiverCopyLoop(void)
/*
* pgarch_archiveXlog
*
- * Invokes system(3) to copy one archive file to wherever it should go
+ * Invokes archive_file_cb to copy one archive file to wherever it should go
*
* Returns true if successful
*/
@@ -509,7 +520,7 @@ pgarch_archiveXlog(char *xlog)
snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
set_ps_display(activitymsg);
- ret = shell_archive_file(xlog, pathname);
+ ret = ArchiveContext.archive_file_cb(xlog, pathname);
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
@@ -759,13 +770,79 @@ HandlePgArchInterrupts(void)
if (ProcSignalBarrierPending)
ProcessProcSignalBarrier();
+ /* Perform logging of memory contexts of this process */
+ if (LogMemoryContextPending)
+ ProcessLogMemoryContextInterrupt();
+
if (ConfigReloadPending)
{
+ char *archiveLib = pstrdup(XLogArchiveLibrary);
+ bool archiveLibChanged;
+
ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
+
+ archiveLibChanged = strcmp(XLogArchiveLibrary, archiveLib) != 0;
+ pfree(archiveLib);
+
+ if (archiveLibChanged)
+ {
+ /*
+ * Call the currently loaded archive module's shutdown callback, if
+ * one is defined.
+ */
+ if (ArchiveContext.shutdown_cb != NULL)
+ArchiveContext.shutdown_cb();
+
+ /*
+ * Ideally, we would simply unload
Re: GUC flags
On Sat, Jan 29, 2022 at 03:38:53PM +0900, Michael Paquier wrote:
> +-- Three exceptions as of transaction_*
> +SELECT name FROM pg_settings_flags
> + WHERE NOT no_show_all AND no_reset_all
> + ORDER BY 1;
> + name
> +
> + transaction_deferrable
> + transaction_isolation
> + transaction_read_only
> +(3 rows)
I think "as of" is not the right phrase here.
Maybe say: Exceptions are transaction_*
> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -23596,6 +23596,45 @@ SELECT
> pg_type_is_visible('myschema.widget'::regtype);
> +
> +Returns an array of the flags associated with the given GUC, or
> +NULL if the does not exist. The result is
I guess it should say "if the GUC does not exist".
> +an empty array if the GUC exists but there are no flags to show,
> +as supported by the list below.
I'd say "...but none of the GUC's flags are exposed by this function."
> +The following flags are exposed (the most meaningful ones are
> +included):
"The most meaningful ones are included" doesn't seem to add anything.
Maybe it'd be useful to say "(Only the most useful flags are exposed)"
> + EXPLAIN, parameters included in
> + EXPLAIN commands.
> +
> +
I think the description is wrong, or just copied from the others.
EXPLAIN is for GUCs which are shown in EXPLAIN(SETTINGS).
|EXPLAIN, parameters included in EXPLAIN commands.
|NO_SHOW_ALL, parameters excluded from SHOW ALL commands.
|NO_RESET_ALL, parameters excluded from RESET ALL commands.
|NOT_IN_SAMPLE, parameters not included in postgresql.conf by default.
|RUNTIME_COMPUTED, runtime-computed parameters.
Instead of a comma, these should use a colon, or something else?
--
Justin
Re: pg_basebackup WAL streamer shutdown is bogus - leading to slow tests
Hi,
On 2022-01-29 12:44:22 -0800, Andres Freund wrote:
> On 2022-01-17 10:06:56 -0800, Andres Freund wrote:
> > Yes, that's what I was suggesting. I wasn't thinking of using a static var,
> > but putting it in StreamCtl. Note that what pgwin32_waitforsinglesocket()
> > is doing doesn't protect against the problem referenced above, because it
> > still is reset by WSAEventSelect.
>
> Do we are about breaking StreamCtl ABI? I don't think so?
Here's a version of the patch only creating the event once. Needs a small bit
of comment polishing, but otherwise I think it's sane?
Greetings,
Andres Freund
>From 2c1d67a0b8dff8b6be9683d422d251c937db9121 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Sun, 16 Jan 2022 01:58:24 -0800
Subject: [PATCH v3] Avoid slow shutdown of pg_basebackup, windows edition.
See also 7834d20b57a.
Discussion: https://postgr.es/m/20220129204422.ljyxclfy5ubws...@alap3.anarazel.de
---
src/bin/pg_basebackup/pg_basebackup.c | 24 ++-
src/bin/pg_basebackup/pg_receivewal.c | 4 ++
src/bin/pg_basebackup/receivelog.c| 96 ---
src/bin/pg_basebackup/receivelog.h| 6 ++
4 files changed, 118 insertions(+), 12 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index c40925c1f04..a539fbe6e0e 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -169,6 +169,8 @@ static const char *progress_filename;
/* Pipe to communicate with background wal receiver process */
#ifndef WIN32
static int bgpipe[2] = {-1, -1};
+#else
+HANDLE *bgevent = NULL;
#endif
/* Handle to child process */
@@ -506,7 +508,14 @@ reached_end_position(XLogRecPtr segendpos, uint32 timeline,
/*
* At this point we have an end pointer, so compare it to the current
* position to figure out if it's time to stop.
+ *
+ * On windows we need to reset the event used to wake up the streaming
+ * thread, otherwise CopyStreamPoll() will start to immediately return.
*/
+#ifdef WIN32
+ ResetEvent(bgevent);
+#endif
+
if (segendpos >= xlogendptr)
return true;
@@ -541,7 +550,7 @@ LogStreamerMain(logstreamer_param *param)
#ifndef WIN32
stream.stop_socket = bgpipe[0];
#else
- stream.stop_socket = PGINVALID_SOCKET;
+ stream.stop_event = bgevent;
#endif
stream.standby_message_timeout = standby_message_timeout;
stream.synchronous = false;
@@ -627,6 +636,14 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
pg_log_error("could not create pipe for background process: %m");
exit(1);
}
+#else
+ bgevent = CreateEvent(NULL, TRUE, FALSE, NULL);
+ if (bgevent == NULL)
+ {
+ pg_log_error("could not create event for background thread: %lu",
+ GetLastError());
+ exit(1);
+ }
#endif
/* Get a second connection */
@@ -2216,7 +2233,9 @@ BaseBackup(void)
/*
* On Windows, since we are in the same process, we can just store the
* value directly in the variable, and then set the flag that says
- * it's there.
+ * it's there. To interrupt the thread while it's waiting for network
+ * IO, we set an event (which the thread waits on in addition to the
+ * socket).
*/
if (sscanf(xlogend, "%X/%X", , ) != 2)
{
@@ -2226,6 +2245,7 @@ BaseBackup(void)
}
xlogendptr = ((uint64) hi) << 32 | lo;
InterlockedIncrement(_xlogendptr);
+ SetEvent(bgevent);
/* First wait for the thread to exit */
if (WaitForSingleObjectEx((HANDLE) bgchild_handle, INFINITE, FALSE) !=
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index ccb215c398c..d27bd85b7ce 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -618,7 +618,11 @@ StreamLog(void)
stream.timeline);
stream.stream_stop = stop_streaming;
+#ifndef WIN32
stream.stop_socket = PGINVALID_SOCKET;
+#else
+ stream.stop_event = NULL;
+#endif
stream.standby_message_timeout = standby_message_timeout;
stream.synchronous = synchronous;
stream.do_sync = do_sync;
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index d39e4b11a1a..cc46de0252e 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -37,8 +37,8 @@ static bool still_sending = true; /* feedback still needs to be sent? */
static PGresult *HandleCopyStream(PGconn *conn, StreamCtl *stream,
XLogRecPtr *stoppos);
-static int CopyStreamPoll(PGconn *conn, long timeout_ms, pgsocket stop_socket);
-static int CopyStreamReceive(PGconn *conn, long timeout, pgsocket stop_socket,
+static int CopyStreamPoll(PGconn *conn, long timeout_ms, StreamCtl *stream);
+static int CopyStreamReceive(PGconn *conn, long timeout, StreamCtl *stream,
char **buffer);
static bool ProcessKeepaliveMsg(PGconn *conn, StreamCtl *stream, char *copybuf,
int len, XLogRecPtr blockpos, TimestampTz *last_status);
@@ -414,6 +414,27 @@ CheckServerVersionForStreaming(PGconn
Re: Windows crash / abort handling
Hi, On 2022-01-09 16:57:04 -0800, Andres Freund wrote: > I've attached a patch implementing these changes. Unless somebody is planning to look at this soon, I'm planning to push it to master. It's too annoying to have these hangs and not see backtraces. We're going to have to do this in all binaries at some point, but that's a considerably larger patch... Greetings, Andres Freund
Re: archive modules
Here is a new revision. I've moved basic_archive to contrib, hardened it
as suggested, and added shutdown support for archive modules.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From f62fea53b93ba7181dfe084b4100eba59eb82aaa Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 19 Nov 2021 01:04:41 +
Subject: [PATCH v14 1/3] Introduce archive modules infrastructure.
---
src/backend/access/transam/xlog.c | 2 +-
src/backend/postmaster/pgarch.c | 93 +--
src/backend/postmaster/shell_archive.c| 24 -
src/backend/utils/init/miscinit.c | 1 +
src/backend/utils/misc/guc.c | 12 ++-
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/access/xlog.h | 1 -
src/include/postmaster/pgarch.h | 52 ++-
8 files changed, 172 insertions(+), 14 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index dfe2a0bcce..958220c495 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8831,7 +8831,7 @@ ShutdownXLOG(int code, Datum arg)
* process one more time at the end of shutdown). The checkpoint
* record will go to the next XLOG file and won't be archived (yet).
*/
- if (XLogArchivingActive() && XLogArchiveCommandSet())
+ if (XLogArchivingActive())
RequestXLogSwitch(false);
CreateCheckPoint(CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_IMMEDIATE);
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 6e3fcedc97..d4a7ca97ca 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -89,6 +89,8 @@ typedef struct PgArchData
slock_t arch_lck;
} PgArchData;
+char *XLogArchiveLibrary = "";
+
/* --
* Local data
@@ -96,6 +98,8 @@ typedef struct PgArchData
*/
static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
+static ArchiveModuleCallbacks ArchiveContext;
+
/*
* Stuff for tracking multiple files to archive from each scan of
@@ -140,6 +144,7 @@ static void pgarch_archiveDone(char *xlog);
static void pgarch_die(int code, Datum arg);
static void HandlePgArchInterrupts(void);
static int ready_file_comparator(Datum a, Datum b, void *arg);
+static void LoadArchiveLibrary(void);
/* Report shared memory space needed by PgArchShmemInit */
Size
@@ -236,6 +241,11 @@ PgArchiverMain(void)
*/
PgArch->pgprocno = MyProc->pgprocno;
+ /*
+ * Load the archive_library.
+ */
+ LoadArchiveLibrary();
+
/* Create workspace for pgarch_readyXlog() */
arch_files = palloc(sizeof(struct arch_files_state));
arch_files->arch_files_size = 0;
@@ -407,11 +417,12 @@ pgarch_ArchiverCopyLoop(void)
*/
HandlePgArchInterrupts();
- /* can't do anything if no command ... */
- if (!XLogArchiveCommandSet())
+ /* can't do anything if not configured ... */
+ if (ArchiveContext.check_configured_cb != NULL &&
+!ArchiveContext.check_configured_cb())
{
ereport(WARNING,
- (errmsg("archive_mode enabled, yet archive_command is not set")));
+ (errmsg("archive_mode enabled, yet archiving is not configured")));
return;
}
@@ -492,7 +503,7 @@ pgarch_ArchiverCopyLoop(void)
/*
* pgarch_archiveXlog
*
- * Invokes system(3) to copy one archive file to wherever it should go
+ * Invokes archive_file_cb to copy one archive file to wherever it should go
*
* Returns true if successful
*/
@@ -509,7 +520,7 @@ pgarch_archiveXlog(char *xlog)
snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
set_ps_display(activitymsg);
- ret = shell_archive_file(xlog, pathname);
+ ret = ArchiveContext.archive_file_cb(xlog, pathname);
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
@@ -759,13 +770,79 @@ HandlePgArchInterrupts(void)
if (ProcSignalBarrierPending)
ProcessProcSignalBarrier();
+ /* Perform logging of memory contexts of this process */
+ if (LogMemoryContextPending)
+ ProcessLogMemoryContextInterrupt();
+
if (ConfigReloadPending)
{
+ char *archiveLib = pstrdup(XLogArchiveLibrary);
+ bool archiveLibChanged;
+
ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
+
+ archiveLibChanged = strcmp(XLogArchiveLibrary, archiveLib) != 0;
+ pfree(archiveLib);
+
+ if (archiveLibChanged)
+ {
+ /*
+ * Call the currently loaded archive module's shutdown callback, if
+ * one is defined.
+ */
+ if (ArchiveContext.shutdown_cb != NULL)
+ArchiveContext.shutdown_cb();
+
+ /*
+ * Ideally, we would simply unload the previous archive module and
+ * load the new one, but there is presently no mechanism for
+ * unloading a library (see the comment above
+ * internal_unload_library()). To deal with this, we simply restart
+ * the archiver. The new archive module will be loaded when the new
+ * archiver process starts up.
+ */
+ ereport(LOG,
+
Re: pg_basebackup WAL streamer shutdown is bogus - leading to slow tests
On 2022-01-17 10:06:56 -0800, Andres Freund wrote: > Yes, that's what I was suggesting. I wasn't thinking of using a static var, > but putting it in StreamCtl. Note that what pgwin32_waitforsinglesocket() > is doing doesn't protect against the problem referenced above, because it > still is reset by WSAEventSelect. Do we are about breaking StreamCtl ABI? I don't think so?
Re: row filtering for logical replication
On 2022-Jan-28, Andres Freund wrote:
> > + foreach(lc, data->publications)
> > + {
> > + Publication *pub = lfirst(lc);
...
> Isn't this basically O(schemas * publications)?
Yeah, there are various places in the logical replication code that seem
pretty careless about this kind of thing -- most of it seems to assume
that there are going to be few publications, so it just looks things up
over and over with abandon, and I saw at least one place where it looped
up an inheritance hierarchy for partitioning doing indexscans at each
level(*). I think a lot more thought is going to be required to fix
these things in a thorough manner -- a log.repl.-specific caching
mechanism, I imagine.
(*) Before 025b920a3d45, psql was forced to seqscan pg_publication_rel
for one of the describe.c queries, and nobody seems to have noticed.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Y una voz del caos me habló y me dijo
"Sonríe y sé feliz, podría ser peor".
Y sonreí. Y fui feliz.
Y fue peor.
Re: Add last commit LSN to pg_last_committed_xact()
On Fri, Jan 28, 2022 at 7:47 PM Andres Freund wrote:
>
> On 2022-01-28 16:36:32 -0800, Andres Freund wrote:
> > On 2022-01-28 18:43:57 -0500, James Coleman wrote:
> > > Alternatively I see pg_attribute_aligned, but that's not defined
> > > (AFAICT) on clang, for example, so I'm not sure that'd be acceptable?
> >
> > clang should have it (it defines __GNUC__). The problem would be msvc, I
> > think. Not sure if there's a way to get to a common way of defining it
> > between
> > gcc-like compilers and msvc (the rest is niche enough that we don't need to
> > care about the efficiency I think).
>
> Seems like it's doable:
>
> https://godbolt.org/z/3c5573bTW
Oh, thanks. I'd seen some discussion previously on the list about
clang not supporting it, but that seems to have been incorrect. Also I
didn't know about that compiler site -- that's really neat.
Here's an updated patch series using that approach; the first patch
can (and probably should be) committed separately/regardless to update
the pg_attribute_aligned to be used in MSVC.
Thanks,
James Coleman
From c104bec6ed93cea06f4d5ff0f9150490f159dfaa Mon Sep 17 00:00:00 2001
From: jcoleman
Date: Sat, 29 Jan 2022 11:28:45 -0500
Subject: [PATCH v4 1/2] Support pg_attribute_aligned in MSVC
---
configure | 2 ++
src/include/c.h | 4
2 files changed, 6 insertions(+)
diff --git a/configure b/configure
index 3f2aea0d7d..e9ab674f38 100755
--- a/configure
+++ b/configure
@@ -17862,6 +17862,8 @@ else
/* This must match the corresponding code in c.h: */
#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__)
#define pg_attribute_aligned(a) __attribute__((aligned(a)))
+#elif defined(_MSC_VER)
+#define pg_attribute_aligned(a) __declspec(align(a))
#endif
typedef __int128 int128a
#if defined(pg_attribute_aligned)
diff --git a/src/include/c.h b/src/include/c.h
index 4f16e589b3..0441b031b4 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -179,6 +179,10 @@
#define pg_attribute_noreturn()
#endif
+#if defined(_MSC_VER)
+#define pg_attribute_aligned(a) __declspec(align(a))
+#endif
+
/*
* Use "pg_attribute_always_inline" in place of "inline" for functions that
* we wish to force inlining of, even when the compiler's heuristics would
--
2.17.1
From 1f456c9f3976e49cc9be31878c5ad5d7eba5c1b6 Mon Sep 17 00:00:00 2001
From: jcoleman
Date: Sat, 29 Jan 2022 12:18:45 -0500
Subject: [PATCH v4 2/2] Expose LSN of last commit via pg_last_committed_xact
---
src/backend/access/transam/xact.c | 6 +++-
src/backend/access/transam/xlogfuncs.c| 17 ++
src/backend/storage/ipc/procarray.c | 34 +++
src/include/access/transam.h | 2 ++
src/include/catalog/pg_proc.dat | 6
src/include/storage/proc.h| 10 ++
src/include/storage/procarray.h | 2 ++
.../test_misc/t/002_last_commit_lsn.pl| 31 +
8 files changed, 107 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/test_misc/t/002_last_commit_lsn.pl
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index c9516e03fa..f2ae4b0667 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1355,6 +1355,7 @@ RecordTransactionCommit(void)
else
{
bool replorigin;
+ XLogRecPtr commit_lsn;
/*
* Are we using the replication origins feature? Or, in other words,
@@ -1391,7 +1392,7 @@ RecordTransactionCommit(void)
SetCurrentTransactionStopTimestamp();
- XactLogCommitRecord(xactStopTimestamp,
+ commit_lsn = XactLogCommitRecord(xactStopTimestamp,
nchildren, children, nrels, rels,
nmsgs, invalMessages,
RelcacheInitFileInval,
@@ -1419,6 +1420,7 @@ RecordTransactionCommit(void)
TransactionTreeSetCommitTsData(xid, nchildren, children,
replorigin_session_origin_timestamp,
replorigin_session_origin);
+ MyProc->lastCommitLSN = commit_lsn;
}
/*
@@ -5883,6 +5885,8 @@ xact_redo_commit(xl_xact_parsed_commit *parsed,
TransactionTreeSetCommitTsData(xid, parsed->nsubxacts, parsed->subxacts,
commit_time, origin_id);
+ MyProc->lastCommitLSN = lsn;
+
if (standbyState == STANDBY_DISABLED)
{
/*
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index d8af5aad58..05ee661fc2 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -29,6 +29,7 @@
#include "replication/walreceiver.h"
#include "storage/fd.h"
#include "storage/ipc.h"
+#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/builtins.h"
#include "utils/guc.h"
@@ -425,6 +426,22 @@ pg_last_wal_replay_lsn(PG_FUNCTION_ARGS)
PG_RETURN_LSN(recptr);
}
+/*
+ * pg_last_commit_lsn
+ *
+ * SQL-callable wrapper to obtain the lsn of the last commit.
+ */
+Datum
+pg_last_commit_lsn(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn = GetLastCommitLSN();
+
Re: Remove extra includes of "access/xloginsert.h" when "access/xlog.h" is included
On 2022-Jan-29, Bharath Rupireddy wrote: > Removing the xloginsert.h in xlog.h would need us to add xloginsert.h > in more areas. Sure. > And also, it might break any non-core extensions that > includes just xlog.h and gets xloginsert.h. That's a pretty easy fix anyway -- it's not even version-specific, since the fix would work with the older versions. It's not something that would break on a minor version, either. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "The problem with the facetime model is not just that it's demoralizing, but that the people pretending to work interrupt the ones actually working." (Paul Graham)
Re: Multiple Query IDs for a rewritten parse tree
> On Sun, Jan 30, 2022 at 01:48:20AM +0800, Julien Rouhaud wrote: > Hi, > > On Sat, Jan 29, 2022 at 06:12:05PM +0100, Dmitry Dolgov wrote: > > > On Sat, Jan 29, 2022 at 03:51:33PM +0800, Julien Rouhaud wrote: > > > > > > I'm also unsure of how are extensions supposed to cooperate in general, as > > > I feel that queryids should be implemented for some "intent" (like > > > monitoring, > > > planning optimization...). That being said I'm not sure if e.g. AQO > > > heuristics > > > are too specific for its need or if it could be shared with other > > > extension > > > that might be dealing with similar concerns. > > > > Assuming there are multiple providers and consumers of queryIds, every > > such consumer extension needs to know which type of queryId it wants to > > use. E.g. in case of pg_stat_statements, it needs to be somehow > > configured to know which of those kinds to take, to preserve > > extensibility you're talking about. Does the answer make sense, or did > > you mean something else? > > I guess, but I don't think that the proposed approach does that. Yes, it doesn't, I'm just channeling my understanding of the problem.
Re: Multiple Query IDs for a rewritten parse tree
Hi, On Sat, Jan 29, 2022 at 06:12:05PM +0100, Dmitry Dolgov wrote: > > On Sat, Jan 29, 2022 at 03:51:33PM +0800, Julien Rouhaud wrote: > > > > I'm also unsure of how are extensions supposed to cooperate in general, as > > I feel that queryids should be implemented for some "intent" (like > > monitoring, > > planning optimization...). That being said I'm not sure if e.g. AQO > > heuristics > > are too specific for its need or if it could be shared with other extension > > that might be dealing with similar concerns. > > Assuming there are multiple providers and consumers of queryIds, every > such consumer extension needs to know which type of queryId it wants to > use. E.g. in case of pg_stat_statements, it needs to be somehow > configured to know which of those kinds to take, to preserve > extensibility you're talking about. Does the answer make sense, or did > you mean something else? I guess, but I don't think that the proposed approach does that. The DBA should be able to configure a monitoring queryid provider, a planning queryid provider... and the extensions should have a way to know which is which. And also I don't think that the DBA should be allowed to setup multiple monitoring queryid providers, nor change them dynamically. > Btw, the approach in this thread still doesn't give a clue what to do > when an extension needs to reuse some parts of core queryId generator, > as in case with pg_stat_statements and "IN" condition merging. Indeed, as the query text normalization is not extensible.
Re: Multiple Query IDs for a rewritten parse tree
> On Sat, Jan 29, 2022 at 03:51:33PM +0800, Julien Rouhaud wrote: > Hi, > > On Fri, Jan 28, 2022 at 05:51:56PM +0100, Dmitry Dolgov wrote: > > > On Fri, Jan 21, 2022 at 11:33:22AM +0500, Andrey V. Lepikhov wrote: > > > On 1/9/22 5:49 AM, Tom Lane wrote: > > > > The idea I'd been vaguely thinking about is to allow attaching a list > > > > of query-hash nodes to a Query, where each node would contain a "tag" > > > > identifying the specific hash calculation method, and also the value > > > > of the query's hash calculated according to that method. We could > > > > probably get away with saying that all such hash values must be uint64. > > > > The main difference from your function-OID idea, I think, is that > > > > I'm envisioning the tags as being small integers with well-known > > > > values, similarly to the way we manage stakind values in pg_statistic. > > > > In this way, an extension that wants a hash that the core knows how > > > > to calculate doesn't need its own copy of the code, and similarly > > > > one extension could publish a calculation method for use by other > > > > extensions. > > > > > > To move forward, I have made a patch that implements this idea (see > > > attachment). It is a POC, but passes all regression tests. > > [...] > > > 4. We should reserve position of default in-core generator > > > > From the discussion above I was under the impression that the core > > generator should be distinguished by a predefined kind. > > I don't really like this approach. IIUC, this patch as-is is meant to break > pg_stat_statements extensibility. If kind == 0 is reserved for in-core > queryid > then you can't use pg_stat_statement with a different queryid generator > anymore. Unless you meant that kind == 0 is reserved for monitoring / > pg_stat_statement purpose and custom extension should register that specific > kind too if that's what they are supposed to implement? > > [...] > > I'm also unsure of how are extensions supposed to cooperate in general, as > I feel that queryids should be implemented for some "intent" (like monitoring, > planning optimization...). That being said I'm not sure if e.g. AQO > heuristics > are too specific for its need or if it could be shared with other extension > that might be dealing with similar concerns. Assuming there are multiple providers and consumers of queryIds, every such consumer extension needs to know which type of queryId it wants to use. E.g. in case of pg_stat_statements, it needs to be somehow configured to know which of those kinds to take, to preserve extensibility you're talking about. Does the answer make sense, or did you mean something else? Btw, the approach in this thread still doesn't give a clue what to do when an extension needs to reuse some parts of core queryId generator, as in case with pg_stat_statements and "IN" condition merging.
Re: Allow users to choose what happens when recovery target is not reached
On Sat, Nov 13, 2021 at 6:45 PM Bharath Rupireddy wrote: > > Firstly, the proposed patch adds no new behaviour as such, it just > gives the ability that is existing today on v12 and below (prior to > commit dc78866 which went into v13 and later). > > I think performing PITR is the user's wish - whether the primary is > available or not, it is completely the user's choice. The user might > start the PITR, when the primary is available, thinking that it sends > all the WAL files required for achieving recovery target. But imagine > a disaster happens and the primary server crashes, say the recovery > has replayed a huge bunch of WAL records (a TB may be), and the > primary failed without sending the last one or few WAL files, should > the PITR target server be failing this case after replaying a huge > bunch of WAL records? The user might want the target server to be > available instead of FATALly shutting down. This is the exact problem > the proposed patch is trying to solve. > > With the GUC proposed, the user can choose what to do in these > scenarios. The user will be fully aware what she needs when she choose > to set the new GUC recovery_end_before_target_action to 'promote' > instead of default 'shutdown'. Hi Hackers, with a recent bug report [1] in pgsql-bugs, I'm checking if the proposal here in this thread interests anyone. [1] https://www.postgresql.org/message-id/CALj2ACVnCsNyJTG_75%2B5Us2evfsLYz5CEhmCV4qH%3DVPa0kWOvw%40mail.gmail.com Regards, Bharath Rupireddy.
Re: psql - add SHOW_ALL_RESULTS option
Hello Peter, It would be better to do without. Also, it makes one wonder how others are supposed to use this multiple-results API properly, if even psql can't do it without extending libpq. Needs more thought. Fine with me! Obviously I'm okay if libpq is repaired instead of writing strange code on the client to deal with strange behavior. I have a new thought on this, as long as we are looking into libpq. Why can't libpq provide a variant of PQexec() that returns all results, instead of just the last one. It has all the information, all it has to do is return the results instead of throwing them away. Then the changes in psql would be very limited, and we don't have to re-invent PQexec() from its pieces in psql. And this would also make it easier for other clients and user code to make use of this functionality more easily. Attached is a rough draft of what this could look like. It basically works. Thoughts? My 0.02€: With this approach results are not available till the last one has been returned? If so, it loses the nice asynchronous property of getting results as they come when they come? This might or might not be desirable depending on the use case. For "psql", ISTM that we should want immediate and asynchronous anyway?? I'm unclear about what happens wrt to client-side data buffering if several large results are returned? COPY?? Also, I guess the user must free the returned array on top of closing all results? -- Fabien.
Re: Is it correct to update db state in control file as "shutting down" during end-of-recovery checkpoint?
On Sat, Jan 29, 2022 at 7:43 AM Michael Paquier wrote:
>
> On Fri, Jan 28, 2022 at 06:32:27PM +0900, Kyotaro Horiguchi wrote:
> > End-of-recovery checkpoint is requested as CHECKPOINT_WAIT, which
> > seems to me to mean the state is always DB_IN_ARCHIVE_RECOVERY while
> > the checkpoint is running?
>
> With the patch, yes, we would keep the control file under
> DB_IN_{ARCHIVE,CRASH}_RECOVERY during the whole period of the
> end-of-recovery checkpoint. On HEAD, we would have DB_SHUTDOWNING
> until the end-of-recovery checkpoint completes, after which we switch
> to DB_SHUTDOWNED for a short time before DB_IN_PRODUCTION.
>
> > If correct, if server is killed druing the
> > end-of-recovery checkpoint, the state stays DB_IN_ARCHIVE_RECOVERY
> > instead of DB_SHUTDOWNING or DB_SHUTDOWNED. AFAICS there's no
> > differece between the first two at next startup.
>
> One difference is the hint given to the user at the follow-up startup.
> Crash and archive recovery states can freak people easily as they
> mention the risk of corruption. Using DB_SHUTDOWNING reduces this
> window.
If the server crashes in end-of-recovery, in the follow-up startup,
the server has to start all the recovery right? In that case,
DB_IN_{ARCHIVE, CRASH}_RECOVERY would represent the correct state to
the user, not the DB_SHUTDOWNING/DB_SHUTDOWNED IMO.
There's another option to have a new state
DB_IN_END_OF_RECOVERY_CHECKPOINT, if the DB_IN_{ARCHIVE,
CRASH}_RECOVERY really scares users of end-of-recovery crash?
Regards,
Bharath Rupireddy.
Replace pg_controldata output fields with macros for better code manageability
Hi, While working on another pg_control patch, I observed that the pg_controldata output fields such as "Latest checkpoint's TimeLineID:", "Latest checkpoint's NextOID:'' and so on, are being used in pg_resetwal.c, pg_controldata.c and pg_upgrade/controldata.c. Direct usage of these fields in many places doesn't look good and can be error prone if we try to change the text in one place and forget in another place. I'm thinking of having those fields as macros in pg_control.h, something like [1] and use it all the places. This will be a good idea for better code manageability IMO. Thoughts? [1] #define PG_CONTROL_FIELD_VERSION_NUMBER "pg_control version number:" #define PG_CONTROL_FIELD_CATALOG_NUMBER "Catalog version number:" #define PG_CONTROL_FIELD_CHECKPOINT_TLI "Latest checkpoint's TimeLineID:" #define PG_CONTROL_FIELD_CHECKPOINT_PREV_TLI "Latest checkpoint's PrevTimeLineID:" #define PG_CONTROL_FIELD_CHECKPOINT_OLDESTXID "Latest checkpoint's oldestXID:" #define PG_CONTROL_FIELD_CHECKPOINT_OLDESTXID_DB "Latest checkpoint's oldestXID's DB:" and so on. Regards, Bharath Rupireddy.
Re: Remove extra includes of "access/xloginsert.h" when "access/xlog.h" is included
On Fri, Jan 28, 2022 at 9:25 PM Alvaro Herrera wrote: > > On 2022-Jan-28, Bharath Rupireddy wrote: > > > Hi, > > > > It seems like there are some instances where xloginsert.h is included > > right after xlog.h but xlog.h has already included xloginsert.h. > > Unless I'm missing something badly, we can safely remove including > > xloginsert.h after xlog.h. Attempting to post a patch to remove the > > extra xloginsert.h includes. > > Why isn't it better to remove the line that includes xloginsert.h in > xlog.h instead? When xloginsert.h was introduced (commit 2076db2aea76), > XLogRecData was put there so xloginsert.h was necessary for xlog.h; but > now we have a forward declaration (per commit 2c03216d8311) so it > doesn't seem needed anymore. Removing the xloginsert.h in xlog.h would need us to add xloginsert.h in more areas. And also, it might break any non-core extensions that includes just xlog.h and gets xloginsert.h. Instead I prefer removing xloginsert.h if there's xlog.h included. Attaching v2 patch removing xloginsert.h in a few more places. Regards, Bharath Rupireddy. From 00e6b9a3b24811c63568a176dc871e89f1356d03 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Sat, 29 Jan 2022 13:19:21 + Subject: [PATCH v2] remove extra includes of xloginsert.h when xlog.h is included --- src/backend/access/gin/ginfast.c| 1 - src/backend/access/heap/heapam.c| 1 - src/backend/access/nbtree/nbtpage.c | 1 - src/backend/access/nbtree/nbtsort.c | 1 - src/backend/access/spgist/spginsert.c | 1 - src/backend/access/transam/clog.c | 1 - src/backend/access/transam/twophase.c | 1 - src/backend/access/transam/xact.c | 1 - src/backend/access/transam/xloginsert.c | 1 - src/backend/catalog/storage.c | 1 - src/backend/commands/sequence.c | 1 - src/backend/utils/cache/relmapper.c | 1 - src/include/access/generic_xlog.h | 1 - 13 files changed, 13 deletions(-) diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c index 7409fdc165..6f07b7b7d3 100644 --- a/src/backend/access/gin/ginfast.c +++ b/src/backend/access/gin/ginfast.c @@ -21,7 +21,6 @@ #include "access/gin_private.h" #include "access/ginxlog.h" #include "access/xlog.h" -#include "access/xloginsert.h" #include "catalog/pg_am.h" #include "commands/vacuum.h" #include "miscadmin.h" diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 98230aac49..b602627100 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -49,7 +49,6 @@ #include "access/visibilitymap.h" #include "access/xact.h" #include "access/xlog.h" -#include "access/xloginsert.h" #include "access/xlogutils.h" #include "catalog/catalog.h" #include "miscadmin.h" diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c index 6b5f01e1d0..9048c9509e 100644 --- a/src/backend/access/nbtree/nbtpage.c +++ b/src/backend/access/nbtree/nbtpage.c @@ -27,7 +27,6 @@ #include "access/tableam.h" #include "access/transam.h" #include "access/xlog.h" -#include "access/xloginsert.h" #include "miscadmin.h" #include "storage/indexfsm.h" #include "storage/lmgr.h" diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c index dc220146fd..9ee28db14d 100644 --- a/src/backend/access/nbtree/nbtsort.c +++ b/src/backend/access/nbtree/nbtsort.c @@ -51,7 +51,6 @@ #include "access/table.h" #include "access/xact.h" #include "access/xlog.h" -#include "access/xloginsert.h" #include "catalog/index.h" #include "commands/progress.h" #include "executor/instrument.h" diff --git a/src/backend/access/spgist/spginsert.c b/src/backend/access/spgist/spginsert.c index bfb74049d0..e5e311c3f6 100644 --- a/src/backend/access/spgist/spginsert.c +++ b/src/backend/access/spgist/spginsert.c @@ -21,7 +21,6 @@ #include "access/spgxlog.h" #include "access/tableam.h" #include "access/xlog.h" -#include "access/xloginsert.h" #include "catalog/index.h" #include "miscadmin.h" #include "storage/bufmgr.h" diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c index de787c3d37..9fc2cbd6ba 100644 --- a/src/backend/access/transam/clog.c +++ b/src/backend/access/transam/clog.c @@ -36,7 +36,6 @@ #include "access/slru.h" #include "access/transam.h" #include "access/xlog.h" -#include "access/xloginsert.h" #include "access/xlogutils.h" #include "miscadmin.h" #include "pg_trace.h" diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index 271a3146db..2b2eb1b94e 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -84,7 +84,6 @@ #include "access/twophase_rmgr.h" #include "access/xact.h" #include "access/xlog.h" -#include "access/xloginsert.h" #include "access/xlogreader.h" #include "access/xlogutils.h" #include "catalog/pg_type.h" diff --git a/src/backend/access/transam/xact.c
Re: [PATCH] New default role allowing to change per-role/database settings
Hi,
Am Mittwoch, dem 08.09.2021 um 07:38 + schrieb
shinya11.k...@nttdata.com:
> > Thanks for letting me know, I've attached a rebased v4 of this
> > patch, no other changes.
Please find attached another rebase, sorry it took so long.
I tried it, but when I used set command, tab completion did not work >
properly and an error occurred.
It's been a while, but I believe the patch only changes the ALTER ROLE
command, not the SET/SHOW commands. I thought that was more generally
useful, can you explain the SET use-case?
> ---
> postgres=> \conninfo
> You are connected to database "postgres" as user "aaa" via socket in
> "/tmp" at port "5432".
> postgres=> \du
> List of roles
> Role name |
> Attributes | Member of
> ---+-
> ---+---
> aaa
> | |
> {pg_change_role_settings}
> penguin | Superuser, Create role, Create DB, Replication, Bypass
> RLS | {}
> postgres=> show log
> log_autovacuum_min_duration log_executor_stats
> log_min_error_statement log_replication_commands
> log_timezone
> log_checkpoints log_file_mode
> log_min_messages log_rotation_age
> log_transaction_sample_rate
> log_connections log_hostname
> log_parameter_max_length log_rotation_size
> log_truncate_on_rotation
> log_destination log_line_prefix
> log_parameter_max_length_on_error log_statement
> logging_collector
> log_disconnections log_lock_waits
> log_parser_stats log_statement_sample_rate
> logical_decoding_work_mem
> log_duration log_min_duration_sample
> log_planner_stats log_statement_stats
> log_error_verbosity log_min_duration_statement
> log_recovery_conflict_waits log_temp_files
> postgres=> show log_duration ;
> log_duration
> --
> off
> (1 row)
>
> postgres=> set log
> log_parameter_max_length_on_error logical_decoding_work_mem
> postgres=> set log_duration to on;
> 2021-09-08 16:23:39.216 JST [533860] ERROR: permission denied to set
> parameter "log_duration"
> 2021-09-08 16:23:39.216 JST [533860] STATEMENT: set log_duration to
> on;
> ERROR: permission denied to set parameter "log_duration"
So this would work:
postgres=> SHOW ROLE;
role
--
rolesetadmin
(1 row)
postgres=> \du
List of roles
Role name | Attributes |
Member of
--++---
postgres | Superuser, Create role, Create DB, Replication, Bypass RLS | {}
rolesetadmin | Cannot login |
{pg_change_role_settings}
test | Cannot login | {}
postgres=> ALTER ROLE test SET log_statement='all';
ALTER ROLE
postgres=> \drds
List of settings
Role | Database | Settings
--+--+---
test | | log_statement=all
(1 row)
I am not sure if there is anything to be done about tab completion, can
you clarify here?
Michael
--
Michael Banck
Teamleiter PostgreSQL-Team
Projektleiter
Tel.: +49 2166 9901-171
E-Mail: michael.ba...@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Geoff Richardson, Peter Lilley
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
From 4144cd5ccaa074b04dc4cbec3689f91e19f7f138 Mon Sep 17 00:00:00 2001
From: Michael Banck
Date: Sat, 29 Jan 2022 11:44:57 +0100
Subject: [PATCH v5] Add new PGC_ADMINSET guc context and
pg_change_role_settings predefined role.
This commit introduces `administrator' as a middle ground between `superuser'
and `user' for guc contexts.
Those settings initially include all previously `superuser'-context settings
from the 'Reporting and Logging' ('What/When to Log' but not 'Where to Log')
and both 'Statistics' categories. Further settings could be added in follow-up
commits.
Also, a new predefined role pg_change_role_settings is introduced. This allows
administrators (that are not superusers) that have been granted this role to
change the above PGC_ADMINSET settings on a per-role basis like "ALTER ROLE
[...] SET log_statement".
The rationale is to allow certain
Re: [BUG]Update Toast data failure in logical replication
On Fri, Jan 28, 2022 at 12:16 PM Dilip Kumar wrote:
>
> I think the best way is to do some refactoring and renaming of the
> function, because as part of HeapDetermineModifiedColumns we are
> already processing the tuple so we can not put extra overhead of
> reprocessing it again. In short I like the idea of renaming the
> HeapDetermineModifiedColumns and moving part of heap_tuple_attr_equals
> code into the caller. Here is the patch set for the same. I have
> divided it into two patches which can eventually be merged, 0001- for
> refactoring 0002- does the actual work.
>
+ /*
+ * If it's a whole-tuple reference, say "not equal". It's not really
+ * worth supporting this case, since it could only succeed after a
+ * no-op update, which is hardly a case worth optimizing for.
+ */
+ if (attrnum == 0)
+ continue;
+
+ /*
+ * Likewise, automatically say "not equal" for any system attribute
+ * other than tableOID; we cannot expect these to be consistent in a
+ * HOT chain, or even to be set correctly yet in the new tuple.
+ */
+ if (attrnum < 0)
+ {
+ if (attrnum != TableOidAttributeNumber)
+ continue;
+ }
These two cases need to be considered as the corresponding attribute
is modified, so the attnum needs to be added in the bitmapset of
modified attrs.
I have changed this and various other comments in the patch. I have
modified the docs as well to reflect the changes. I thought of adding
a test but I think the current test in toast.sql seems sufficient.
Kindly let me know what you think of the attached? I think we should
backpatch this till v10. What do you think?
Does anyone else have better ideas to fix this?
--
With Regards,
Amit Kapila.
v6-0001-WAL-log-unchanged-toasted-replica-identity-key-at.patch
Description: Binary data
