Re: macos ventura SDK spews warnings
Hi, On 2022-10-15 21:00:00 -0400, Tom Lane wrote: > I wrote: > > snprintf is required by POSIX going back to SUSv2, so it's pretty darn > > hard to imagine any currently-used platform that hasn't got it. Even > > my now-extinct dinosaur gaur had it (per digging in backup files). > > I think we could certainly assume its presence in the branches that > > require C99. > > After further thought, I think the best compromise is just that: > > (1) apply s/sprintf/snprintf/ patch in branches back to v12, where > we began to require C99. > > (2) in v11 and back to 9.2, enable -Wno-deprecated if available. > > One thing motivating this choice is that we're just a couple > weeks away from the final release of v10. So I'm hesitant to do > anything that might turn out to be moving the portability goalposts > in v10. But we're already assuming we can detect -Wno-foo options > correctly in v10 and older (e.g. 4c5a29c0e), so point (2) seems > pretty low-risk. Makes sense to me. - Andres
Re: Use -fvisibility=hidden for shared libraries
Hi, On 2022-10-15 22:50:07 -0400, Tom Lane wrote: > Andres Freund writes: > > I guess we should report the CFLAGS_SL_MODULE etc via pg_config? > > Dunno ... where do you stop? I'm not really convinced that extensions > that don't want to use PGXS have any claim on our buildsystem to provide > platform-specific configuration details for them. That works for me. I just didn't want to decide on that "by fiat" after I noticed it... Greetings, Andres Freund
Re: Suppressing useless wakeups in walreceiver
On Thu, Oct 13, 2022 at 12:09:54PM -0700, Nathan Bossart wrote:
> On Thu, Oct 13, 2022 at 12:37:39PM +0200, Alvaro Herrera wrote:
>> The main reason is that it seems odd to have startpointTLI in the struct
>> used in some places together with a file-global recvFileTLI which isn't.
>> The way one is passed as argument and the other as part of a struct
>> seemed too distracting. This should reduce the number of moving parts,
>> ISTM.
>
> Makes sense. Do you think the struct should be file-global so that it
> doesn't need to be provided as an argument to most of the static functions
> in this file?
Here's a different take. Instead of creating structs and altering function
signatures, we can just make the wake-up times file-global, and we can skip
the changes related to reducing the number of calls to
GetCurrentTimestamp() for now. This results in a far less invasive patch.
(I still think reducing the number of calls to GetCurrentTimestamp() is
worthwhile, but I'm beginning to agree with Bharath that it should be
handled separately.)
Thoughts?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 389a398412c51ee18cb37e04548350d661c78d4f Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Thu, 27 Jan 2022 21:43:17 +1300
Subject: [PATCH v7 1/1] Suppress useless wakeups in walreceiver.
Instead of waking up 10 times per second to check for various timeout
conditions, keep track of when we next have periodic work to do.
Reviewed-by: Kyotaro Horiguchi, Bharath Rupireddy, Alvaro Herrera
Discussion: https://postgr.es/m/CA%2BhUKGJGhX4r2LPUE3Oy9BX71Eum6PBcS8L3sJpScR9oKaTVaA%40mail.gmail.com
---
src/backend/replication/walreceiver.c | 163 +-
1 file changed, 106 insertions(+), 57 deletions(-)
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index 6cbb67c92a..a8740c84d2 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -95,8 +95,6 @@ bool hot_standby_feedback;
static WalReceiverConn *wrconn = NULL;
WalReceiverFunctionsType *WalReceiverFunctions = NULL;
-#define NAPTIME_PER_CYCLE 100 /* max sleep time between cycles (100ms) */
-
/*
* These variables are used similarly to openLogFile/SegNo,
* but for walreceiver to write the XLOG. recvFileTLI is the TimeLineID
@@ -116,6 +114,23 @@ static struct
XLogRecPtr Flush; /* last byte + 1 flushed in the standby */
} LogstreamResult;
+/*
+ * Reasons to wake up and perform periodic tasks.
+ */
+typedef enum WalRcvWakeupReason
+{
+ WALRCV_WAKEUP_TERMINATE,
+ WALRCV_WAKEUP_PING,
+ WALRCV_WAKEUP_REPLY,
+ WALRCV_WAKEUP_HSFEEDBACK,
+ NUM_WALRCV_WAKEUPS
+} WalRcvWakeupReason;
+
+/*
+ * Wake up times for periodic tasks.
+ */
+TimestampTz wakeup[NUM_WALRCV_WAKEUPS];
+
static StringInfoData reply_message;
static StringInfoData incoming_message;
@@ -132,6 +147,7 @@ static void XLogWalRcvClose(XLogRecPtr recptr, TimeLineID tli);
static void XLogWalRcvSendReply(bool force, bool requestReply);
static void XLogWalRcvSendHSFeedback(bool immed);
static void ProcessWalSndrMessage(XLogRecPtr walEnd, TimestampTz sendTime);
+static void WalRcvComputeNextWakeup(WalRcvWakeupReason reason, TimestampTz now);
/*
* Process any interrupts the walreceiver process may have received.
@@ -179,9 +195,7 @@ WalReceiverMain(void)
TimeLineID primaryTLI;
bool first_stream;
WalRcvData *walrcv = WalRcv;
- TimestampTz last_recv_timestamp;
- TimestampTz starttime;
- bool ping_sent;
+ TimestampTz now;
char *err;
char *sender_host = NULL;
int sender_port = 0;
@@ -192,7 +206,7 @@ WalReceiverMain(void)
*/
Assert(walrcv != NULL);
- starttime = GetCurrentTimestamp();
+ now = GetCurrentTimestamp();
/*
* Mark walreceiver as running in shared memory.
@@ -248,7 +262,7 @@ WalReceiverMain(void)
/* Initialise to a sanish value */
walrcv->lastMsgSendTime =
- walrcv->lastMsgReceiptTime = walrcv->latestWalEndTime = starttime;
+ walrcv->lastMsgReceiptTime = walrcv->latestWalEndTime = now;
/* Report the latch to use to awaken this process */
walrcv->latch = &MyProc->procLatch;
@@ -414,9 +428,10 @@ WalReceiverMain(void)
initStringInfo(&reply_message);
initStringInfo(&incoming_message);
- /* Initialize the last recv timestamp */
- last_recv_timestamp = GetCurrentTimestamp();
- ping_sent = false;
+ /* Initialize nap wakeup times. */
+ now = GetCurrentTimestamp();
+ for (int i = 0; i < NUM_WALRCV_WAKEUPS; ++i)
+WalRcvComputeNextWakeup(i, now);
/* Loop until end-of-streaming or error */
for (;;)
@@ -426,6 +441,8 @@ WalReceiverMain(void)
bool endofwal = false;
pgsocket wait_fd = PGINVALID_SOCKET;
int rc;
+TimestampTz nextWakeup;
+int64 nap;
/*
* Exit walreceiver if we're not in recovery. This should not
@@ -443,11 +460,15 @@ WalReceiverMain(void)
{
ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
+ now = GetCurrentTimestamp();
Re: Use -fvisibility=hidden for shared libraries
Andres Freund writes: > I guess we should report the CFLAGS_SL_MODULE etc via pg_config? Dunno ... where do you stop? I'm not really convinced that extensions that don't want to use PGXS have any claim on our buildsystem to provide platform-specific configuration details for them. regards, tom lane
Re: Use -fvisibility=hidden for shared libraries
Hi, On 2022-09-01 14:19:35 -0700, Andres Freund wrote: > Here's an updated patch for this (also shared recently on another thread). I've since then committed this. I was reminded of this when thinking about https://postgr.es/m/1595488.1665869988%40sss.pgh.pa.us Thinking about this made me realize that we currently don't expose -fvisibility=hidden via pg_config. It's added to extensions built via PGXS, via Makefile.global, but that doesn't help extensions that don't want to use PGXS. I guess we should report the CFLAGS_SL_MODULE etc via pg_config? Greetings, Andres Freund
Re: PATCH: Using BRIN indexes for sorted output
On Sat, Oct 15, 2022 at 8:23 AM Tomas Vondra wrote: > On 10/15/22 15:46, Zhihong Yu wrote: > >... > > 8) Parallel version is not supported, but I think it shouldn't be > > possible. Just make the leader build the range info, and then let the > > workers to acquire/sort ranges and merge them by Gather Merge. > > ... > > Hi, > > I am still going over the patch. > > > > Minor: for #8, I guess you meant `it should be possible` . > > > > Yes, I meant to say it should be possible. Sorry for the confusion. > > > > regards > > -- > Tomas Vondra > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > Hi, For brin_minmax_ranges, looking at the assignment to gottuple and reading gottuple, it seems variable gottuple can be omitted - we can check tup directly. + /* Maybe mark the range as processed. */ + range->processed |= mark_processed; `Maybe` can be dropped. For brinsort_load_tuples(), do we need to check for interrupts inside the loop ? Similar question for subsequent methods involving loops, such as brinsort_load_unsummarized_ranges. Cheers
Re: New docs chapter on Transaction Management and related changes
On Fri, Oct 14, 2022 at 05:46:55PM -0400, Robert Treat wrote: > On Fri, Oct 14, 2022 at 3:51 PM Bruce Momjian wrote: > > Attached is the merged patch from all the great comments I received. I > > have also rebuilt the docs with the updated patch: > > > > https://momjian.us/tmp/pgsql/ > > > > + RELEASE SAVEPOINT also subcommits and destroys > + all savepoints that were established after the named savepoint was > + established. This means that any subtransactions of the named savepoint > + will also be subcommitted and destroyed. > > Wonder if we should be more explicit that data changes are preserved, > not destroyed... something like: > "This means that any changes within subtransactions of the named > savepoint will be subcommitted and those subtransactions will be > destroyed." Good point. I reread the section and there was just too much confusion over subtransactions, partly because the behavior just doesn't map easily to subtransaction. I therefore merged all three paragraphs into one and tried to make the text saner; release_savepoint.sgml diff attached, URL content updated. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson diff --git a/doc/src/sgml/ref/release_savepoint.sgml b/doc/src/sgml/ref/release_savepoint.sgml index daf8eb9a43..ae603ad102 100644 --- a/doc/src/sgml/ref/release_savepoint.sgml +++ b/doc/src/sgml/ref/release_savepoint.sgml @@ -34,23 +34,14 @@ RELEASE [ SAVEPOINT ] savepoint_name Description - RELEASE SAVEPOINT destroys a savepoint previously defined - in the current transaction. - - - - Destroying a savepoint makes it unavailable as a rollback point, - but it has no other user visible behavior. It does not undo the - effects of commands executed after the savepoint was established. - (To do that, see .) - Destroying a savepoint when - it is no longer needed allows the system to reclaim some resources - earlier than transaction end. - - - - RELEASE SAVEPOINT also destroys all savepoints that were - established after the named savepoint was established. + RELEASE SAVEPOINT destroys the named savepoint and + all active savepoints that were created after the named savepoint. + All changes made since the creation of the savepoint, excluding rolled + back savepoints changes, are treated as part of the transaction + or savepoint that was active when the named savepoint was created. + Changes made after RELEASE SAVEPOINT will be in + the same transaction, and have the same transaction id, as changes + made before the named savepoint was created. @@ -78,7 +69,7 @@ RELEASE [ SAVEPOINT ] savepoint_name It is not possible to release a savepoint when the transaction is in - an aborted state. + an aborted state, to do that use . @@ -104,6 +95,36 @@ COMMIT; The above transaction will insert both 3 and 4. + + + A more complex example with multiple nested subtransactions: + +BEGIN; +INSERT INTO table1 VALUES (1); +SAVEPOINT sp1; +INSERT INTO table1 VALUES (2); +SAVEPOINT sp2; +INSERT INTO table1 VALUES (3); +RELEASE SAVEPOINT sp2; +INSERT INTO table1 VALUES (4))); -- generates an error + + In this example, the application requests the release of the savepoint + sp2, which inserted 3. This changes the insert's + transaction context to sp1. When the statement + attempting to insert value 4 generates an error, the insertion of 2 and + 4 are lost because they are in the same, now-rolled back savepoint, + and value 3 is in the same transaction context. The application can + now only choose one of these two commands, since all other commands + will be ignored with a warning: + + ROLLBACK; + ROLLBACK TO SAVEPOINT sp1; + + Choosing ROLLBACK will abort everything, including + value 1, whereas ROLLBACK TO SAVEPOINT sp1 will retain + value 1 and allow the transaction to continue. + +
Re: macos ventura SDK spews warnings
I wrote: > snprintf is required by POSIX going back to SUSv2, so it's pretty darn > hard to imagine any currently-used platform that hasn't got it. Even > my now-extinct dinosaur gaur had it (per digging in backup files). > I think we could certainly assume its presence in the branches that > require C99. After further thought, I think the best compromise is just that: (1) apply s/sprintf/snprintf/ patch in branches back to v12, where we began to require C99. (2) in v11 and back to 9.2, enable -Wno-deprecated if available. One thing motivating this choice is that we're just a couple weeks away from the final release of v10. So I'm hesitant to do anything that might turn out to be moving the portability goalposts in v10. But we're already assuming we can detect -Wno-foo options correctly in v10 and older (e.g. 4c5a29c0e), so point (2) seems pretty low-risk. regards, tom lane
Re: macos ventura SDK spews warnings
On 2022-10-15 14:19:55 -0700, Andres Freund wrote: > The meson specific warning is > [972/1027] Linking target > src/backend/replication/libpqwalreceiver/libpqwalreceiver.dylib > ld: warning: -undefined dynamic_lookup may not work with chained fixups > > Which is caused by meson defaulting to -Wl,-undefined,dynamic_lookup for > modules. But we don't need that because we use -bund-loader. Adding > -Wl,-undefined,error as in the attached fixes it. Pushed the patch for that.
Re: macos ventura SDK spews warnings
Andres Freund writes: > On 2022-10-15 18:47:16 -0400, Tom Lane wrote: >>> Originally we used the platform's sprintf there because we couldn't >>> rely on platforms having functional snprintf. That's no longer the case, >>> I imagine, so we could just switch these calls over to snprintf. > Is there a platform still supported in older branches that we need to worry > about? snprintf is required by POSIX going back to SUSv2, so it's pretty darn hard to imagine any currently-used platform that hasn't got it. Even my now-extinct dinosaur gaur had it (per digging in backup files). I think we could certainly assume its presence in the branches that require C99. Even before that, is anybody really still building on nineties-vintage platforms? > I wonder if we ought to add -Wno-deprecated to out-of-support branches to deal > with this kind of thing... Yeah, that might be a better answer than playing whack-a-mole with these sorts of warnings. regards, tom lane
Re: macos ventura SDK spews warnings
Hi, On 2022-10-15 18:47:16 -0400, Tom Lane wrote: > I wrote: > > Andres Freund writes: > >> [24/2258] Compiling C object src/port/libpgport_srv.a.p/snprintf.c.o > >> ../../../src/postgres/src/port/snprintf.c:1002:11: warning: 'sprintf' is > >> deprecated: This function is provided for compatibility reasons only. Due > >> to security concerns inherent in the design of sprintf(3), it is highly > >> recommended that you use snprintf(3) instead. [-Wdeprecated-declarations] > > > Originally we used the platform's sprintf there because we couldn't > > rely on platforms having functional snprintf. That's no longer the case, > > I imagine, so we could just switch these calls over to snprintf. I'm > > kind of surprised that we haven't already been getting the likes of > > this warning from, eg, OpenBSD. Is there a platform still supported in older branches that we need to worry about? > The attached seems enough to silence it for me. > > Should we back-patch this? Probably, but not sure either. We could just let it stew in HEAD for a while. > I suppose, but how far? It seems to fall under the rules we established for > back-patching into out-of-support branches, ie it silences compiler warnings > but shouldn't change any behavior. But it feels like a bigger change than > most of the other things we've done that with. I wonder if we ought to add -Wno-deprecated to out-of-support branches to deal with this kind of thing... Greetings, Andres Freund
Re: macos ventura SDK spews warnings
I wrote:
> Andres Freund writes:
>> [24/2258] Compiling C object src/port/libpgport_srv.a.p/snprintf.c.o
>> ../../../src/postgres/src/port/snprintf.c:1002:11: warning: 'sprintf' is
>> deprecated: This function is provided for compatibility reasons only. Due
>> to security concerns inherent in the design of sprintf(3), it is highly
>> recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]
> Originally we used the platform's sprintf there because we couldn't
> rely on platforms having functional snprintf. That's no longer the case,
> I imagine, so we could just switch these calls over to snprintf. I'm
> kind of surprised that we haven't already been getting the likes of
> this warning from, eg, OpenBSD.
The attached seems enough to silence it for me.
Should we back-patch this? I suppose, but how far? It seems to fall
under the rules we established for back-patching into out-of-support
branches, ie it silences compiler warnings but shouldn't change any
behavior. But it feels like a bigger change than most of the other
things we've done that with.
regards, tom lane
diff --git a/src/port/snprintf.c b/src/port/snprintf.c
index e037cf0a88..81d9c8c274 100644
--- a/src/port/snprintf.c
+++ b/src/port/snprintf.c
@@ -998,8 +998,8 @@ fmtptr(const void *value, PrintfTarget *target)
int vallen;
char convert[64];
- /* we rely on regular C library's sprintf to do the basic conversion */
- vallen = sprintf(convert, "%p", value);
+ /* we rely on regular C library's snprintf to do the basic conversion */
+ vallen = snprintf(convert, sizeof(convert), "%p", value);
if (vallen < 0)
target->failed = true;
else
@@ -1149,11 +1149,11 @@ fmtfloat(double value, char type, int forcesign, int leftjust,
int padlen; /* amount to pad with spaces */
/*
- * We rely on the regular C library's sprintf to do the basic conversion,
+ * We rely on the regular C library's snprintf to do the basic conversion,
* then handle padding considerations here.
*
* The dynamic range of "double" is about 1E+-308 for IEEE math, and not
- * too wildly more than that with other hardware. In "f" format, sprintf
+ * too wildly more than that with other hardware. In "f" format, snprintf
* could therefore generate at most 308 characters to the left of the
* decimal point; while we need to allow the precision to get as high as
* 308+17 to ensure that we don't truncate significant digits from very
@@ -1205,14 +1205,14 @@ fmtfloat(double value, char type, int forcesign, int leftjust,
fmt[2] = '*';
fmt[3] = type;
fmt[4] = '\0';
- vallen = sprintf(convert, fmt, prec, value);
+ vallen = snprintf(convert, sizeof(convert), fmt, prec, value);
}
else
{
fmt[0] = '%';
fmt[1] = type;
fmt[2] = '\0';
- vallen = sprintf(convert, fmt, value);
+ vallen = snprintf(convert, sizeof(convert), fmt, value);
}
if (vallen < 0)
goto fail;
@@ -1341,7 +1341,7 @@ pg_strfromd(char *str, size_t count, int precision, double value)
fmt[2] = '*';
fmt[3] = 'g';
fmt[4] = '\0';
- vallen = sprintf(convert, fmt, precision, value);
+ vallen = snprintf(convert, sizeof(convert), fmt, precision, value);
if (vallen < 0)
{
target.failed = true;
fix archive module shutdown callback
Hi hackers,
Presently, when an archive module sets up a shutdown callback, it will be
called upon ERROR/FATAL (via PG_ENSURE_ERROR_CLEANUP), when the archive
library changes (via HandlePgArchInterrupts()), and upon normal shutdown.
There are a couple of problems with this:
* HandlePgArchInterrupts() calls the shutdown callback directly before
proc_exit(). However, the PG_ENSURE_ERROR_CLEANUP surrounding the call to
pgarch_MainLoop() sets up a before_shmem_exit callback that also calls the
shutdown callback. This means that the shutdown callback will be called
twice whenever archive_library is changed via SIGHUP.
* PG_ENSURE_ERROR_CLEANUP is intended for both ERROR and FATAL. However,
the archiver operates at the bottom of the exception stack, so ERRORs are
treated as FATALs. This means that PG_ENSURE_ERROR_CLEANUP is excessive.
We only need to set up the before_shmem_exit callback.
To fix, the attached patch removes the use of PG_ENSURE_ERROR_CLEANUP and
the call to the shutdown callback in HandlePgArchInterrupts() in favor of
just setting up a before_shmem_exit callback in LoadArchiveLibrary().
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 3868cd7bd3..7b6d48f7c5 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -252,13 +252,7 @@ PgArchiverMain(void)
/* 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);
+ pgarch_MainLoop();
proc_exit(0);
}
@@ -803,12 +797,6 @@ HandlePgArchInterrupts(void)
if (archiveLibChanged)
{
- /*
- * Call the currently loaded archive module's shutdown callback,
- * if one is defined.
- */
- call_archive_module_shutdown_callback(0, 0);
-
/*
* Ideally, we would simply unload the previous archive module and
* load the new one, but there is presently no mechanism for
@@ -858,6 +846,8 @@ LoadArchiveLibrary(void)
if (ArchiveContext.archive_file_cb == NULL)
ereport(ERROR,
(errmsg("archive modules must register an archive callback")));
+
+ before_shmem_exit(call_archive_module_shutdown_callback, 0);
}
/*
Re: macos ventura SDK spews warnings
Andres Freund writes: > One class of warnings is specific to meson (see further down), but the other > is common between autoconf and meson: > [24/2258] Compiling C object src/port/libpgport_srv.a.p/snprintf.c.o > ../../../src/postgres/src/port/snprintf.c:1002:11: warning: 'sprintf' is > deprecated: This function is provided for compatibility reasons only. Due to > security concerns inherent in the design of sprintf(3), it is highly > recommended that you use snprintf(3) instead. [-Wdeprecated-declarations] > vallen = sprintf(convert, "%p", value); > ^ Originally we used the platform's sprintf there because we couldn't rely on platforms having functional snprintf. That's no longer the case, I imagine, so we could just switch these calls over to snprintf. I'm kind of surprised that we haven't already been getting the likes of this warning from, eg, OpenBSD. Note that the hundreds of other sprintf calls in our code are actually calling pg_sprintf, which hasn't got a deprecation label. But the ones in snprintf.c are really trying to call the platform's version. (I wonder how much we ought to worry about bugs in the pg_sprintf usages? But that's a matter for another day and another thread.) regards, tom lane
Re: [RFC] building postgres with meson - v13
Andres Freund writes: > Seems like we should have a different pg_config flag for meson options than > for configure, and perhaps separately an option to show the buildsystem? Yeah, probably a good idea, given that shoving the options for one buildsystem into the other isn't likely to work. regards, tom lane
macos ventura SDK spews warnings
Hi
I had recently updated the M1 mini that I use to test macOS stuff on. Just
tried to test a change on it and was greeted with a lot of
warnings. Apparently the update brought in a newer SDK (MacOSX13.0.sdk), even
though the OS is still Monterey.
One class of warnings is specific to meson (see further down), but the other
is common between autoconf and meson:
[24/2258] Compiling C object src/port/libpgport_srv.a.p/snprintf.c.o
../../../src/postgres/src/port/snprintf.c:1002:11: warning: 'sprintf' is
deprecated: This function is provided for compatibility reasons only. Due to
security concerns inherent in the design of sprintf(3), it is highly
recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]
vallen = sprintf(convert, "%p", value);
^
/Library/Developer/CommandLineTools/SDKs/MacOSX13.0.sdk/usr/include/stdio.h:188:1:
note: 'sprintf' has been explicitly marked deprecated here
__deprecated_msg("This function is provided for compatibility reasons only.
Due to security concerns inherent in the design of sprintf(3), it is highly
recommended that you use snprintf(3) instead.")
^
/Library/Developer/CommandLineTools/SDKs/MacOSX13.0.sdk/usr/include/sys/cdefs.h:215:48:
note: expanded from macro '__deprecated_msg'
#define __deprecated_msg(_msg) __attribute__((__deprecated__(_msg)))
^
the same warning is repeated for a bunch of different lines in the same file,
and then over the three versions of libpgport that we build.
This is pretty noisy.
The meson specific warning is
[972/1027] Linking target
src/backend/replication/libpqwalreceiver/libpqwalreceiver.dylib
ld: warning: -undefined dynamic_lookup may not work with chained fixups
Which is caused by meson defaulting to -Wl,-undefined,dynamic_lookup for
modules. But we don't need that because we use -bund-loader. Adding
-Wl,-undefined,error as in the attached fixes it.
Greetings,
Andres Freund
>From 11411bd2dce8d13bb74371d850b1f707b1dd8a01 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Sat, 15 Oct 2022 14:02:35 -0700
Subject: [PATCH v1] meson: macos: Use -Wl,-undefined,error for modules
meson defaults to -Wl,-undefined,dynamic_lookup for modules, which we don't
want because a) it's different from what we do for autoconf, b) it causes
warnings starting in macOS Ventura when combined with -bundle_loader.
ci-os-only: macos
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
meson.build | 5 +
1 file changed, 5 insertions(+)
diff --git a/meson.build b/meson.build
index fdf8ec8ad9c..f288f0a5fc7 100644
--- a/meson.build
+++ b/meson.build
@@ -228,6 +228,11 @@ elif host_system == 'darwin'
message('darwin sysroot: @0@'.format(pg_sysroot))
cflags += ['-isysroot', pg_sysroot]
ldflags += ['-isysroot', pg_sysroot]
+ # meson defaults to -Wl,-undefined,dynamic_lookup for modules, which we
+ # don't want because a) it's different from what we do for autoconf, b) it
+ # causes warnings starting in macOS Ventura when combined with
+ # -bundle_loader
+ ldflags_mod += ['-Wl,-undefined,error']
elif host_system == 'freebsd'
sema_kind = 'unnamed_posix'
--
2.38.0
Re: thinko in basic_archive.c
On Sat, Oct 15, 2022 at 10:19:05AM +0530, Bharath Rupireddy wrote: > Can you please help me understand how name collisions can happen with > temp file names including WAL file name, timestamp to millisecond > scale, and PID? Having the timestamp is enough to provide a non-unique > temp file name when PID wraparound occurs, right? Am I missing > something here? Outside of contrived cases involving multiple servers, inaccurate clocks, PID reuse, etc., it seems unlikely. > Hm, we cannot remove the temp file for all sorts of crashes, but > having on_shmem_exit() or before_shmem_exit() or atexit() or any such > callback removing it would help us cover some crash scenarios (that > exit with proc_exit() or exit()) at least. I think the basic_archive > module currently leaves temp files around even when the server is > restarted legitimately while copying to or renaming the temp file, no? I think the right way to do this would be to add handling for leftover files in the sigsetjmp() block and a shutdown callback (which just sets up a before_shmem_exit callback). While this should ensure those files are cleaned up after an ERROR or FATAL, crashes and unlink() failures could still leave files behind. We'd probably also need to avoid cleaning up the temp file if copy_file() fails because it already exists, as we won't know if it's actually ours. Overall, I suspect it'd be more trouble than it's worth. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: [meson] add missing pg_attribute_aligned for MSVC in meson build
Hi, On 2022-10-15 12:02:14 -0700, Andres Freund wrote: > See the attached patch fixing those omissions. I'll push it to HEAD once it > has the CI stamp of approval. Done. Thanks for the report!
Re: [RFC] building postgres with meson - v13
Hi, On 2022-10-13 23:35:14 -0400, Tom Lane wrote: > "shiy.f...@fujitsu.com" writes: > > On Fri, Oct 14, 2022 12:40 AM Andres Freund wrote: > >> It'd be a fair amount of work, both initially and to maintain it, to > >> generate > >> something compatible. I can see some benefit in showing some feature > >> influencing output in --configure, but compatible output doesn't seem > >> worth it > >> to me. > > > And it's ok for me that the output is not exactly the same as before. > > Yeah, the output doesn't have to be exactly the same. Seems like we should have a different pg_config flag for meson options than for configure, and perhaps separately an option to show the buildsystem? Maybe --buildsystem -> autoconf|meson, --configure -> existing CONFIGURE_ARGS for autoconf, empty otherwise, --meson-options -> meson options? Greetings, Andres Freund
Re: [meson] add missing pg_attribute_aligned for MSVC in meson build
Hi,
On 2022-10-14 10:59:28 +0800, Junwang Zhao wrote:
> Commit ec3c9cc add pg_attribute_aligned in MSVC[1],
> which was pushed one day before the meson commits,
> so meson build missed this feature.
Good catch. It shouldn't have practical consequences for the moment, given
that msvc doesn't support 128bit integers, but of course we should still be
correct.
Looked through other recent changes to configure and found a few additional
omissions.
See the attached patch fixing those omissions. I'll push it to HEAD once it
has the CI stamp of approval.
Greetings,
Andres Freund
>From 57bab33302d4cdd8d48f91382aac912d7f07de59 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Sat, 15 Oct 2022 12:00:16 -0700
Subject: [PATCH v1] meson: catch up to a few configure changes
I (Andres) missed a few recent changes to configure when merging
e6927270cd1 "meson: Add initial version of meson based build system". Mirror
the changes in
- ec3c9cc202f "Add definition pg_attribute_aligned() for MSVC"
- b086a47a270 "Bump minimum version of Bison to 2.3"
- 8b878bffa8d "Bump minimum version of Flex to 2.5.35"
As MSVC does not implement 128 bit integers, the oversight of not using
pg_attribute_aligned() should not have current practical consequences. But of
course the code from c.h should still be correctly mirrored.
I (Andres) also hadn't implemented the minimum perl version check. Added that
now.
Reported-by: Junwang Zhao
Author: Junwang Zhao
Author: Andres Freund
Discussion: https://postgr.es/m/caeg8a3k9c87ewawmdogms0li1j6p_7r-uc0-zn6cjtrmr7v...@mail.gmail.com
---
meson.build | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/meson.build b/meson.build
index 925db70c9d5..fdf8ec8ad9c 100644
--- a/meson.build
+++ b/meson.build
@@ -316,8 +316,8 @@ endif
# External programs
perl = find_program(get_option('PERL'), required: true, native: true)
python = find_program(get_option('PYTHON'), required: true, native: true)
-flex = find_program(get_option('FLEX'), native: true, version: '>= 2.5.31')
-bison = find_program(get_option('BISON'), native: true, version: '>= 1.875')
+flex = find_program(get_option('FLEX'), native: true, version: '>= 2.5.35')
+bison = find_program(get_option('BISON'), native: true, version: '>= 2.3')
sed = find_program(get_option('SED'), 'sed', native: true)
prove = find_program(get_option('PROVE'), native: true)
tar = find_program(get_option('TAR'), native: true)
@@ -864,7 +864,10 @@ if not perlopt.disabled()
perl_inc_dir = '@0@/CORE'.format(archlibexp)
-if useshrplib != 'true'
+if perlversion.version_compare('< 5.14')
+ perl_may_work = false
+ perl_msg = 'Perl version 5.14 or later is required, but this is @0@'.format(perlversion)
+elif useshrplib != 'true'
perl_may_work = false
perl_msg = 'need a shared perl'
endif
@@ -1458,6 +1461,8 @@ if cc.links('''
/* 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)
--
2.38.0
Re: PATCH: Using BRIN indexes for sorted output
On 10/15/22 15:46, Zhihong Yu wrote: >... > 8) Parallel version is not supported, but I think it shouldn't be > possible. Just make the leader build the range info, and then let the > workers to acquire/sort ranges and merge them by Gather Merge. > ... > Hi, > I am still going over the patch. > > Minor: for #8, I guess you meant `it should be possible` . > Yes, I meant to say it should be possible. Sorry for the confusion. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: PATCH: Using BRIN indexes for sorted output
On Sat, Oct 15, 2022 at 5:34 AM Tomas Vondra wrote: > Hi, > > There have been a couple discussions about using BRIN indexes for > sorting - in fact this was mentioned even in the "Improving Indexing > Performance" unconference session this year (don't remember by whom). > But I haven't seen any patches, so here's one. > > The idea is that we can use information about ranges to split the table > into smaller parts that can be sorted in smaller chunks. For example if > you have a tiny 2MB table with two ranges, with values in [0,100] and > [101,200] intervals, then it's clear we can sort the first range, output > tuples, and then sort/output the second range. > > The attached patch builds "BRIN Sort" paths/plans, closely resembling > index scans, only for BRIN indexes. And this special type of index scan > does what was mentioned above - incrementally sorts the data. It's a bit > more complicated because of overlapping ranges, ASC/DESC, NULL etc. > > This is disabled by default, using a GUC enable_brinsort (you may need > to tweak other GUCs to disable parallel plans etc.). > > A trivial example, demonstrating the benefits: > > create table t (a int) with (fillfactor = 10); > insert into t select i from generate_series(1,1000) s(i); > > > First, a simple LIMIT query: > > explain (analyze, costs off) select * from t order by a limit 10; > > QUERY PLAN > > Limit (actual time=1879.768..1879.770 rows=10 loops=1) >-> Sort (actual time=1879.767..1879.768 rows=10 loops=1) > Sort Key: a > Sort Method: top-N heapsort Memory: 25kB > -> Seq Scan on t > (actual time=0.007..1353.110 rows=1000 loops=1) > Planning Time: 0.083 ms > Execution Time: 1879.786 ms > (7 rows) > > QUERY PLAN > > Limit (actual time=1.217..1.219 rows=10 loops=1) >-> BRIN Sort using t_a_idx on t >(actual time=1.216..1.217 rows=10 loops=1) > Sort Key: a > Planning Time: 0.084 ms > Execution Time: 1.234 ms > (5 rows) > > That's a pretty nice improvement - of course, this is thanks to having a > perfectly sequential, and the difference can be almost arbitrary by > making the table smaller/larger. Similarly, if the table gets less > sequential (making ranges to overlap), the BRIN plan will be more > expensive. Feel free to experiment with other data sets. > > However, not only the LIMIT queries can improve - consider a sort of the > whole table: > > test=# explain (analyze, costs off) select * from t order by a; > >QUERY PLAN > - > Sort (actual time=2806.468..3487.213 rows=1000 loops=1) >Sort Key: a >Sort Method: external merge Disk: 117528kB >-> Seq Scan on t (actual time=0.018..1498.754 rows=1000 loops=1) > Planning Time: 0.110 ms > Execution Time: 3766.825 ms > (6 rows) > > test=# explain (analyze, costs off) select * from t order by a; > QUERY PLAN > > > -- > BRIN Sort using t_a_idx on t (actual time=1.210..2670.875 rows=1000 > loops=1) >Sort Key: a > Planning Time: 0.073 ms > Execution Time: 2939.324 ms > (4 rows) > > Right - not a huge difference, but still a nice 25% speedup, mostly due > to not having to spill data to disk and sorting smaller amounts of data. > > There's a bunch of issues with this initial version of the patch, > usually described in XXX comments in the relevant places.6) > > 1) The paths are created in build_index_paths() because that's what > creates index scans (which the new path resembles). But that is expected > to produce IndexPath, not BrinSortPath, so it's not quite correct. > Should be somewhere "higher" I guess. > > 2) BRIN indexes don't have internal ordering, i.e. ASC/DESC and NULLS > FIRST/LAST does not really matter for them. The patch just generates > paths for all 4 combinations (or tries to). Maybe there's a better way. > > 3) I'm not quite sure the separation of responsibilities between > opfamily and opclass is optimal. I added a new amproc, but maybe this > should be split differently. At the moment only minmax indexes have > this, but adding this to minmax-multi should be trivial. > > 4) The state changes in nodeBrinSort is a bit confusing. Works, but may > need cleanup and refactoring. Ideas welcome. > > 5) The costing is essentially just plain cost_index. I have some ideas > about BRIN costing in general, which I'll post in a separate thread (as > it's not specific to this patch). > > 6) At the moment this only picks one of the index keys, specified in the > ORDER BY clause. I think we can generalize this to multiple keys, but > thinking about multi-key ranges was a
Re: create subscription - improved warning message
On Fri, Oct 14, 2022 at 8:22 AM Peter Smith wrote: > > On Thu, Oct 13, 2022 at 9:07 AM Peter Smith wrote: > > > > On Thu, Oct 13, 2022 at 2:01 AM Tom Lane wrote: > > > > > > Alvaro Herrera writes: > > > > On 2022-Oct-12, Amit Kapila wrote: > > > >> Okay, then I think we can commit the last error message patch of Peter > > > >> [1] as we have an agreement on the same, and then work on this as a > > > >> separate patch. What do you think? > > > > > > > No objection. > > > > > > Yeah, the v3-0001 patch is fine. I agree that the docs change needs > > > more work. > > > > Thanks to everybody for the feedback/suggestions. I will work on > > improving the documentation for this and post something in a day or > > so. > > > PSA a patch for adding examples of how to activate a subscription that > was originally created in a disconnected mode. > > The new docs are added as part of the "Logical Replication - > Subscription" section 31.2. > > The CREATE SUBSCRIPTION reference page was also updated to include > links to the new docs. > You have used 'pgoutput' as plugin name in the examples. Shall we mention in some way that this is a default plugin used for built-in logical replication and it is required to use only this one to enable logical replication? -- With Regards, Amit Kapila.
Re: Improve description of XLOG_RUNNING_XACTS
On Fri, Oct 14, 2022 at 3:38 PM Bharath Rupireddy wrote: > > On Mon, Oct 3, 2022 at 1:46 PM Michael Paquier wrote: > > > > On Fri, Sep 16, 2022 at 10:55:53AM +0900, Kyotaro Horiguchi wrote: > > > Putting an arbitrary upper-bound on the number of subxids to print > > > might work? I'm not sure how we can determine the upper-bound, though. > > > > You could hardcode it so as it does not blow up the whole view, say > > 20~30. Anyway, I agree with the concern raised upthread about the > > amount of extra data this would add to the output, so having at least > > the number of subxids would be better than the existing state of > > things telling only if the list of overflowed. So let's stick to > > that. > > I spent some time today reading this. As others said upthread, the > output can be more verbose if all the backends are running max > subtransactions or subtransactions overflow occurred in all the > backends. > As far as I can understand, this contains subtransactions only when they didn't overflow. The latest information provided by Sawada-San for similar records (XLOG_STANDBY_LOCK and XLOG_INVALIDATIONS) made me think that maybe we are just over-worried about the worst case. > This can blow-up the output. > If we get some reports like that, then we can probably use Michael's idea of displaying additional information with a separate flag. > Hard-limiting the number of subxids isn't a good idea because the > whole purpose of it is gone. > Agreed. -- With Regards, Amit Kapila.
