Re: static libpq (and other libraries) overwritten on aix
On Thu, Aug 18, 2022 at 09:03:57AM -0700, Andres Freund wrote: > My point is that we currently, across most of the other platforms, support > building a "proper" static library, and install it too. But on AIX (and I > think mingw), we don't, but without an explicit comment about not doing so. In > fact, the all-static-lib target on those platforms will build a non-static > library, which seems not great. Yep. If someone had just pushed a correct patch to make AIX match our GNU/Linux static linking assistance, I wouldn't be arguing to revert that patch. At the same time, if someone asks me to choose high-value projects for 20 people, doing more for static linking on AIX won't be on the list. > On 2022-08-17 21:59:29 -0700, Noah Misch wrote: > > Along the lines of Robert's comment, it could be a nice code beautification > > to > > use a different suffix for the short-lived .a file. Perhaps _so_inputs.a. > > Agreed, it'd be an improvement. > > Afaict we could just stop building the intermediary static lib. Afaict the > MKLDEXPORT path isn't needed for libraries without an exports.txt because the > linker defaults to exporting "most" symbols If that works, great.
Re: Cleaning up historical portability baggage
On Sun, Aug 14, 2022 at 6:07 AM Tom Lane wrote: > Thomas Munro writes: > > I tried to figure out how to get rid of > > PGAC_STRUCT_SOCKADDR_STORAGE_MEMBERS, but there we're into genuine > > non-standard cross-platform differences. > > Right. I don't think it's worth sweating over. I managed to get rid of four of these probes. Some were unused, and one could be consolidated into another leaving just one probe of this ilk. 1. src/common/ip.c already made a leap by assuming that if you have ss_len then you must have sun_len. We might as well change that to be driven by the presence of sa_len instead. That leap is fine: if you have one, you have them all, and sa_len has the advantage of a stable name across systems that have it (unlike ss_len, which AIX calls __ss_len, requiring more configure gloop). 2. src/backend/libpq/ifaddr.c only needs to know if you have sa_len. This code is only used on AIX, so we could hard-wire it in theory, but it's good to keep it general so you can still compile and test it on systems without sa_len (mostly Linux). From a4cc14b0de234102bfe6312e793d515797fa572e Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Fri, 19 Aug 2022 16:16:34 +1200 Subject: [PATCH] Remove configure probes for sockaddr_storage members. Previously we had to deal with implementation details of struct sockaddr_storage in various replacement code, but now that we're able to rely on newer standard facilities we don't need to do that. We still need to keep a probe for sockaddr's sa_len, to use in a couple of places. In one place we previously assumed that the presence of ss_len implied the presence sun_len, but it's no less justifiable to test for sa_len there, since they all go together. --- config/c-library.m4| 18 +- configure | 48 -- configure.ac | 2 +- src/common/ip.c| 2 +- src/include/libpq/pqcomm.h | 13 --- src/include/pg_config.h.in | 12 -- src/tools/msvc/Solution.pm | 4 7 files changed, 8 insertions(+), 91 deletions(-) diff --git a/config/c-library.m4 b/config/c-library.m4 index 58453c4f76..c1dd804679 100644 --- a/config/c-library.m4 +++ b/config/c-library.m4 @@ -73,20 +73,14 @@ AC_DEFUN([PGAC_UNION_SEMUN], ])])# PGAC_UNION_SEMUN -# PGAC_STRUCT_SOCKADDR_STORAGE_MEMBERS -# -- -# Check the members of `struct sockaddr_storage'. We need to know about -# ss_family and ss_len. (Some platforms follow RFC 2553 and call them -# __ss_family and __ss_len.) We also check struct sockaddr's sa_len. -AC_DEFUN([PGAC_STRUCT_SOCKADDR_STORAGE_MEMBERS], -[AC_CHECK_MEMBERS([struct sockaddr_storage.ss_family, - struct sockaddr_storage.__ss_family, - struct sockaddr_storage.ss_len, - struct sockaddr_storage.__ss_len, - struct sockaddr.sa_len], [], [], +# PGAC_STRUCT_SOCKADDR_MEMBERS +# +# Check if struct sockaddr and subtypes have 4.4BSD-style length. +AC_DEFUN([PGAC_STRUCT_SOCKADDR_SA_LEN], +[AC_CHECK_MEMBERS([struct sockaddr.sa_len], [], [], [#include #include -])])# PGAC_STRUCT_SOCKADDR_STORAGE_MEMBERS +])])# PGAC_STRUCT_SOCKADDR_MEMBERS # PGAC_TYPE_LOCALE_T diff --git a/configure b/configure index b28fccbc47..4bd050008b 100755 --- a/configure +++ b/configure @@ -14990,54 +14990,6 @@ _ACEOF fi -ac_fn_c_check_member "$LINENO" "struct sockaddr_storage" "ss_family" "ac_cv_member_struct_sockaddr_storage_ss_family" "#include -#include - -" -if test "x$ac_cv_member_struct_sockaddr_storage_ss_family" = xyes; then : - -cat >>confdefs.h <<_ACEOF -#define HAVE_STRUCT_SOCKADDR_STORAGE_SS_FAMILY 1 -_ACEOF - - -fi -ac_fn_c_check_member "$LINENO" "struct sockaddr_storage" "__ss_family" "ac_cv_member_struct_sockaddr_storage___ss_family" "#include -#include - -" -if test "x$ac_cv_member_struct_sockaddr_storage___ss_family" = xyes; then : - -cat >>confdefs.h <<_ACEOF -#define HAVE_STRUCT_SOCKADDR_STORAGE___SS_FAMILY 1 -_ACEOF - - -fi -ac_fn_c_check_member "$LINENO" "struct sockaddr_storage" "ss_len" "ac_cv_member_struct_sockaddr_storage_ss_len" "#include -#include - -" -if test "x$ac_cv_member_struct_sockaddr_storage_ss_len" = xyes; then : - -cat >>confdefs.h <<_ACEOF -#define HAVE_STRUCT_SOCKADDR_STORAGE_SS_LEN 1 -_ACEOF - - -fi -ac_fn_c_check_member "$LINENO" "struct sockaddr_storage" "__ss_len" "ac_cv_member_struct_sockaddr_storage___ss_len" "#include -#include - -" -if test "x$ac_cv_member_struct_sockaddr_storage___ss_len" = xyes; then : - -cat >>confdefs.h <<_ACEOF -#define HAVE_STRUCT_SOCKADDR_STORAGE___SS_LEN 1 -_ACEOF - - -fi ac_fn_c_check_member "$LINENO" "struct sockaddr" "sa_len" "ac_cv_member_struct_sockaddr_sa_len" "#include #include diff --git a/configure.ac b/configure.ac index dd368290a6..6ff294d405 100644 --- a/configure.ac +++ b/configure.ac @@ -1614,7 +1614,7 @@ PGAC_C_COMPUTED_GOTO PGAC_STRUCT_TIMEZONE PGAC_UNION_SEMUN AC_CHECK_TYPES(socklen_t, [], [],
Re: Perform streaming logical transactions by background workers and parallel apply
Here are my review comments for the patch v23-0003:
==
3.1. src/backend/replication/logical/applybgworker.c -
apply_bgworker_relation_check
+ * Although the commit order is maintained by only allowing one process to
+ * commit at a time, the access order to the relation has changed. This could
+ * cause unexpected problems if the unique column on the replicated table is
+ * inconsistent with the publisher-side or contains non-immutable functions
+ * when applying transactions using an apply background worker.
+ */
+void
+apply_bgworker_relation_check(LogicalRepRelMapEntry *rel)
I’m not sure, but should that second sentence be rearranged as follows?
SUGGESTION
This could cause unexpected problems when applying transactions using
an apply background worker if the unique column on the replicated
table is inconsistent with the publisher-side, or if the relation
contains non-immutable functions.
~~~
3.2.
+ if (!am_apply_bgworker() &&
+ (list_length(ApplyBgworkersFreeList) == list_length(ApplyBgworkersList)))
+ return;
Previously I posted I was struggling to understand the above
condition, and then it was explained (see [1] comment #4) that:
> We need to check this for apply bgworker. (Both lists are "NIL" in apply
> bgworker.)
I think that information should be included in the code comment.
==
3.3. src/include/replication/logicalrelation.h
+/*
+ * States to determine if changes on one relation can be applied using an
+ * apply background worker.
+ */
+typedef enum ParallelApplySafety
+{
+ PARALLEL_APPLY_UNKNOWN = 0,
+ PARALLEL_APPLY_SAFE,
+ PARALLEL_APPLY_UNSAFE
+} ParallelApplySafety;
+
3.3a.
The enum value PARALLEL_APPLY_UNKNOWN doesn't really mean anything.
Maybe naming it PARALLEL_APPLY_SAFETY_UNKNOWN gives it the intended
meaning.
3.3b.
+ PARALLEL_APPLY_UNKNOWN = 0,
I didn't see any reason to explicitly assign this to 0.
~~~
3.4. src/include/replication/logicalrelation.h
@@ -31,6 +42,8 @@ typedef struct LogicalRepRelMapEntry
Relation localrel; /* relcache entry (NULL when closed) */
AttrMap*attrmap; /* map of local attributes to remote ones */
bool updatable; /* Can apply updates/deletes? */
+ ParallelApplySafety parallel_apply; /* Can apply changes in an apply
+
(Similar to above comment #3.3a)
The member name 'parallel_apply' doesn't really mean anything. Perhaps
renaming this to 'parallel_apply_safe' or 'parallel_safe' etc will
give it the intended meaning.
--
[1]
https://www.postgresql.org/message-id/OS3PR01MB6275739E73E8BEC5D13FB6739E6B9%40OS3PR01MB6275.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: shadow variables - pg15 edition
On Fri, Aug 19, 2022 at 03:37:52PM +1200, David Rowley wrote: > I'm happy for you to take that. I'd just rather not be batting such trivial > patches over the fence at each other for days or weeks. Yes, thanks for that. I read through your patch, which looks fine. Let me know what I can do when it's time for round two. -- Justin
Re: shadow variables - pg15 edition
On Fri, 19 Aug 2022 at 11:21, Justin Pryzby wrote: > > On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote: > > Any objections to pushing this to master only? > > I won't object, but some of your changes are what makes backpatching this less > reasonable (foreach_current_index and newtype). I had made these v15 patches > first to simplify backpatching, since having the same code in v15 means that > there's no backpatch hazard for this new-in-v15 code. I spent a bit more time on this and I see that make check-world does fail if I change either of the foreach_current_index() changes to be incorrect. e.g change the condition from "> 0" to be "== 0", "> 1" or "> -1". As for the newtype change, I was inclined to give the variable name with the most meaning to the one that's in scope for longer. I'm starting to feel like it would be ok to backpatch these new-to-pg-15 changes back into PG15. The reason I think this is that they all seem low enough risk that it's probably more risky to not backpatch and risk bugs being introduced due to mistakes being made in conflict resolution when future patches don't apply. It was the failing tests I mentioned above that swayed me on this. > I am opened to presenting the patches differently, but we need to come up with > a better process than one person writing patches and someone else rewriting > it. It wasn't my intention to purposefully rewrite everything. It's just that in order to get the work into something I was willing to commit, that's how it ended up. As for why I did that rather than ask you to was the fact that doing it myself required fewer keystrokes, mental effort and time than asking you to. It's not my intention to do that for any personal credit. I'm happy for you to take that. I'd just rather not be batting such trivial patches over the fence at each other for days or weeks. The effort-to-reward ratio for that is probably going to drop below my threshold after a few rounds. David
Re: Cleaning up historical portability baggage
On Tue, Aug 16, 2022 at 4:14 PM Thomas Munro wrote:
> On Tue, Aug 16, 2022 at 1:16 PM Andres Freund wrote:
> > > But let's suppose we want to play by a timid interpretation of that page's
> > > "do not issue low-level or STDIO.H I/O routines". It also says that
> > > SIGINT
> > > is special and runs the handler in a new thread (in a big warning box
> > > because that has other hazards that would break other kinds of code).
> > > Well,
> > > we *know* it's safe to unlink files in another thread... so... how cheesy
> > > would it be if we just did raise(SIGINT) in the real handlers?
> >
> > Not quite sure I understand. You're proposing to raise(SIGINT) for all other
> > handlers, so that signal_remove_temp() gets called in another thread,
> > because
> > we assume that'd be safe because doing file IO in other threads is safe?
> > That
> > assumes the signal handler invocation infrastructure isn't the problem...
>
> That's what I was thinking about, yeah. But after some more reading,
> now I'm wondering if we'd even need to do that, or what I'm missing.
> The 6 listed signals in the manual are SIGABRT, SIGFPE, SIGILL,
> SIGINT, SIGSEGV and SIGTERM (the 6 required by C). We want to run
> signal_remove_temp() on SIGHUP (doesn't exist, we made it up), SIGINT,
> SIGPIPE (doesn't exist, we made it up), and SIGTERM (exists for C spec
> compliance but will never be raised by the system according to the
> manual, and we don't raise it ourselves IIUC). So the only case we
> actually have to consider is SIGINT, and SIGINT handlers run in a
> thread, so if we assume it is therefore exempt from those
> very-hard-to-comply-with rules, aren't we good already? What am I
> missing?
I converted that analysis into a WIP patch, and tried to make the
Windows test setup as similar to Unix as possible. I put in the
explanation and an assertion that it's running in another thread.
This is blind coded as I don't have Windows, but it passes CI. I'd
probably need some help from a Windows-enabled hacker to go further
with this, though. Does the assertion hold if you control-C the
regression test, and is there any other way to get it to fail?
The next thing is that the security infrastructure added by commit
f6dc6dd5 for CVE-2014-0067 is ripped out (because unreachable) by the
attached, but the security infrastructure added by commit be76a6d3
probably doesn't work on Windows yet. Where src/port/mkdtemp.c does
mkdir(name, 0700), I believe Windows throws away the mode and makes a
default ACL directory, probably due to the mismatch between the
permissions models. I haven't studied the Windows security model, but
reading tells me that AF_UNIX will obey filesystem ACLs, so I think we
should be able to make it exactly as secure as Unix if we use native
APIs. Perhaps we just need to replace the mkdir() call in mkdtemp.c
with CreateDirectory(), passing in a locked-down owner-only
SECURITY_DESCRIPTOR, or something like that?
From 2bbaa4b37b4044f03d99b1effa137b06abfe0b21 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Fri, 19 Aug 2022 11:28:38 +1200
Subject: [PATCH 1/2] WIP: Always use Unix-domain sockets in pg_regress on
Windows.
Since we can now rely on Unix-domain sockets working on supported
Windows releases, we can remove a difference between Unix and Windows in
pg_regress.
Previously, we thought the socket cleanup code was unsafe, so we made
Unix-domain sockets an option with a "use-at-your-own-risk" note. On
closer inspection, the concerns about signal handlers don't seem to
apply here. (initdb.c has similar concerns but needs separate
investigation.)
XXX This removes the code added to secure the temporary cluster by
commit f6dc6dd5, since that's based on TCP sockets.
XXX In order to secure the temporary cluster as done for Unix by commit
be76a6d3, we want to make the socket directory non-accessible to other
users. That's done by POSIX.1-2008 mkdtemp() or our replacement code,
on Unix, which ultimately does mkdir(some_name, 0700). The
Windows/MinGW mkdir(name, mode) macro just throws away the mode, so it
makes a default-accessible directory. To do the equivalent thing in
Windows you'd probably need to use CreateDirectory() with an ACL that
means approximately 0700. Research needed.
---
src/port/mkdtemp.c| 4 +
src/test/regress/pg_regress.c | 274 --
2 files changed, 36 insertions(+), 242 deletions(-)
diff --git a/src/port/mkdtemp.c b/src/port/mkdtemp.c
index 8809957dcd..bf90f91848 100644
--- a/src/port/mkdtemp.c
+++ b/src/port/mkdtemp.c
@@ -187,6 +187,10 @@ GETTEMP(char *path, int *doopen, int domkdir)
}
else if (domkdir)
{
+ /*
+ * XXX Here we probably need to make a Windows ACL that allows
+ * only the current user, and use CreateDirectory().
+ */
if (mkdir(path, 0700) >= 0)
return 1;
if (errno != EEXIST)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 9ca1a8d906..87bd030214 100644
---
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Aug 19, 2022 at 4:36 AM Peter Smith wrote: > > Hi Wang-san, > > Here is some more information about my v21-0001 review [2] posted yesterday. > > ~~ > > If the streaming=parallel will be disallowed for publishers not using > protocol 4 (see Amit's post [1]), then please ignore all my previous > review comments about the protocol descriptions (see [2] comments #4b, > #7b, #47a, #47b). > > ~~ > > Also, I was having second thoughts about the name replacement for the > 'main_worker_pid' member (see [2] comments #1b, #49). Previously I > suggested 'apply_leader_pid', but now I think something like > 'apply_bgworker_leader_pid' would be better. (It's a bit verbose, but > now it gives the proper understanding that only an apply bgworker can > have a valid value for this member). > I find your previous suggestion to name it 'apply_leader_pid' better. According to me, it conveys the meaning. -- With Regards, Amit Kapila.
Re: Mingw task for Cirrus CI
Inline notes about changes since the last version.
On Thu, Jul 28, 2022 at 05:44:28PM -0500, Justin Pryzby wrote:
> I think the "only_if" should allow separately running one but not both of the
> windows instances, like:
>
> + only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' ||
> $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*mingw64'
>
> I'm not sure, but maybe this task should only run "by request", and omit the
> first condition:
>
> + only_if: $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*mingw64'
The patch shouldn't say this during development, or else cfbot doesn't run it..
Oops.
> I think it should include something like
>
> + setup_additional_packages_script: |
> +REM C:\msys64\usr\bin\pacman.exe -S --noconfirm ...
>
> Let's see what others think about those.
>
> Do you know if this handles logging of crash dumps ?
It does now, although I hardcoded "postgres.exe" ...
> + setup_additional_packages_script: |
> +REM C:\msys64\usr\bin\pacman.exe -S --noconfirm busybox
This should include choco, too.
> -CXXFLAGS='-Og -ggdb'"
> +CXXFLAGS='-Og -ggdb' && break;
> +rm -v ${CCACHE_DIR}/configure.cache;
> +done
I noticed that this doesn't seem to do the right thing with the exit status -
configure can fail without cirrusci noticing, and then the build fails at the
next step.
> for item in `find "$sourcetree" -name Makefile -print -o -name GNUmakefile
> -print | grep -v "$sourcetree/doc/src/sgml/images/"`; do
> -filename=`expr "$item" : "$sourcetree\(.*\)"`
> -if test ! -f "${item}.in"; then
> -if cmp "$item" "$buildtree/$filename" >/dev/null 2>&1; then : ; else
> -ln -fs "$item" "$buildtree/$filename" || exit 1
> -fi
> -fi
> +filename=${item#$sourcetree}
> +[ -e "$buildtree/$filename" ] && continue
I fixed this to check for ".in" files as intended.
It'd be a lot better if the image didn't take so long to start. :(
--
Justin
>From 32646786299672d333cbf1f49bafac9e90e0d3be Mon Sep 17 00:00:00 2001
From: Melih Mutlu
Date: Mon, 21 Feb 2022 14:46:05 +0300
Subject: [PATCH 1/2] Added Windows with MinGW environment in Cirrus CI
---
.cirrus.yml | 78 -
1 file changed, 65 insertions(+), 13 deletions(-)
diff --git a/.cirrus.yml b/.cirrus.yml
index a9193c2c34f..472661c0936 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -339,13 +339,29 @@ task:
cores_script: src/tools/ci/cores_backtrace.sh macos "${HOME}/cores"
+WINDOWS_ENVIRONMENT_BASE: _ENVIRONMENT_BASE
+env:
+ # Half the allowed per-user CPU cores
+ CPUS: 4
+ # The default working dir is in a directory msbuild complains about
+ CIRRUS_WORKING_DIR: "c:/cirrus"
+
+ # Avoids port conflicts between concurrent tap test runs
+ PG_TEST_USE_UNIX_SOCKETS: 1
+
+only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*windows.*'
+
+sysinfo_script: |
+ chcp
+ systeminfo
+ powershell -Command get-psdrive -psprovider filesystem
+ set
+
task:
+ << : *WINDOWS_ENVIRONMENT_BASE
name: Windows - Server 2019, VS 2019
env:
-# Half the allowed per-user CPU cores
-CPUS: 4
-
# Our windows infrastructure doesn't have test concurrency above the level
# of a single vcregress test target. Due to that, it's useful to run prove
# with multiple jobs. For the other tasks it isn't, because two sources
@@ -355,15 +371,11 @@ task:
# likely can be improved upon further.
PROVE_FLAGS: -j10 --timer
-# The default cirrus working dir is in a directory msbuild complains about
-CIRRUS_WORKING_DIR: "c:/cirrus"
# Avoid re-installing over and over
NO_TEMP_INSTALL: 1
# git's tar doesn't deal with drive letters, see
# https://postgr.es/m/b6782dc3-a7b0-ed56-175f-f8f54cb08d67%40dunslane.net
TAR: "c:/windows/system32/tar.exe"
-# Avoids port conflicts between concurrent tap test runs
-PG_TEST_USE_UNIX_SOCKETS: 1
PG_REGRESS_SOCK_DIR: "c:/cirrus/"
# -m enables parallelism
# verbosity:minimal + Summary reduce verbosity, while keeping a summary of
@@ -398,12 +410,6 @@ task:
cpu: $CPUS
memory: 4G
- sysinfo_script: |
-chcp
-systeminfo
-powershell -Command get-psdrive -psprovider filesystem
-set
-
setup_additional_packages_script: |
REM 3min
REM choco install -y --no-progress --version=1.0.0 visualstudio2022-workload-vctools --install-args="--add Microsoft.VisualStudio.Component.VC.CLI.Support"
@@ -475,6 +481,52 @@ task:
path: "crashlog-*.txt"
type: text/plain
+task:
+ << : *WINDOWS_ENVIRONMENT_BASE
+ name: Windows - Server 2019, MinGW64
+ windows_container:
+image: $CONTAINER_REPO/windows_ci_mingw64:latest
+cpu: $CPUS
+memory: 4G
+ env:
+CCACHE_DIR: C:/msys64/ccache
+BUILD_DIR: "%CIRRUS_WORKING_DIR%/build"
+
+ ccache_cache:
+folder: ${CCACHE_DIR}
+
Re: Cirrus CI (Windows help wanted)
Hi, On 2022-08-18 20:17:09 -0500, Justin Pryzby wrote: > On Thu, Aug 18, 2022 at 06:09:39PM -0700, Andres Freund wrote: > > > But look: https://cirrus-ci.com/task/4953593575899136 > > > > Why is the build phase so slow in that image? 12min? That's way slower than > > the windows builds normally. > > Because I'd just rebased it, and it's using ccache, which I mentioned is > several times slower for cache misses, and only ~50% faster for cache hits :( > 20220701191841.gh13...@telsasoft.com That makes it basically unusable - builds being that slow even just occasionally is prohibitive for something like cfbot I think. I'm a bit surprised though - I'm fairly certain that that wasn't the case when I tried it locally (with ninja at least). > > > => I installed most of the goodies, but deferred running the installers > > > until > > > the image is run, and it starts just as quickly. It has to run the > > > installer > > > each time, rather than once when building the image. That's crummy, but > > > it's > > > still 1-2 minutes faster than now. Maybe for some of those, it's not > > > needed to > > > run an installer at all. (Like if the installer can be extracted into > > > c:\programfiles). > > > > I am strongly against that. For one, the amount of traffic that causes with > > the software providers is substantial. For another, the failure rates of > > downloading stuff every time are quite high. > > I think you misunderstood. The installers are *retrieved* when the image is > built, and zipfiles are extracted. But for .exes, the installation is > deferred > until the image is run. Ah, that's better. I still think it's better to not install things every time and reduce the install footprint though. Greetings, Andres Freund
Re: Cirrus CI (Windows help wanted)
On Thu, Aug 18, 2022 at 06:09:39PM -0700, Andres Freund wrote: > > But look: https://cirrus-ci.com/task/4953593575899136 > > Why is the build phase so slow in that image? 12min? That's way slower than > the windows builds normally. Because I'd just rebased it, and it's using ccache, which I mentioned is several times slower for cache misses, and only ~50% faster for cache hits :( 20220701191841.gh13...@telsasoft.com There's a patch to use depend mode, which would make cache misses less expensive. > > => I installed most of the goodies, but deferred running the installers > > until > > the image is run, and it starts just as quickly. It has to run the > > installer > > each time, rather than once when building the image. That's crummy, but > > it's > > still 1-2 minutes faster than now. Maybe for some of those, it's not > > needed to > > run an installer at all. (Like if the installer can be extracted into > > c:\programfiles). > > I am strongly against that. For one, the amount of traffic that causes with > the software providers is substantial. For another, the failure rates of > downloading stuff every time are quite high. I think you misunderstood. The installers are *retrieved* when the image is built, and zipfiles are extracted. But for .exes, the installation is deferred until the image is run. -- Justin
Re: Cirrus CI (Windows help wanted)
Hi, On 2022-08-18 19:56:56 -0500, Justin Pryzby wrote: > Note that there's now a cirrusci image with visual studio 2022, which allows > building postgres. I tried it here. In the past there were repeated issues with the cirrus install of visual studio missing some things, and modifying the install to install them was very slow, that's why I had switched to installing VS ourselves. I suspect that the other issue right now is that they updated the host to a newer version of windows, and when container and host version don't match, the windows container stuff gets slower starting up. > But look: https://cirrus-ci.com/task/4953593575899136 Why is the build phase so slow in that image? 12min? That's way slower than the windows builds normally. > => I installed most of the goodies, but deferred running the installers until > the image is run, and it starts just as quickly. It has to run the installer > each time, rather than once when building the image. That's crummy, but it's > still 1-2 minutes faster than now. Maybe for some of those, it's not needed > to > run an installer at all. (Like if the installer can be extracted into > c:\programfiles). I am strongly against that. For one, the amount of traffic that causes with the software providers is substantial. For another, the failure rates of downloading stuff every time are quite high. I think pruning the container from unnecessary content, and trying to base it on the vs 2022 image (which is prel-loaded onto the host) is a better plan. Greetings, Andres Freund
Re: Cirrus CI (Windows help wanted)
On Sun, Jul 31, 2022 at 01:31:58PM -0700, Andres Freund wrote: > > But it failed like: > > > > https://cirrus-ci.com/task/5622728425209856?logs=push#L16 > > [00:09:53.675] unauthorized: You don't have the needed permissions to > > perform this operation, and you may have invalid credentials. To > > authenticate your request, follow the steps in: > > https://cloud.google.com/container-registry/docs/advanced-authentication > > > > Does this require tying my github account to a google account ? > > Or paying cirrusci ? Or ?? > > Not sure what the problem here is - it worked for me in the past. I reported the problem to Fedor at cirrusci who had to fix something. It works now. On Wed, Aug 03, 2022 at 09:06:05PM -0700, Andres Freund wrote: > On 2022-07-28 17:23:19 -0500, Justin Pryzby wrote: > > I think it could be a lot faster to start, since cirrus caches the generated > > docker image locally. Rather than (I gather) pulling the image every time. > > I'm quite certain that is not true. All the docker images built are just > uploaded to the google container registry and then downloaded onto a > *separate* windows host. The dockerfile: stuff generates a separate task > running on a separate machine... I think you're right. When I used an image built with with Andrew's original recipe using "dockerfile-as-a-ci-environment" , it still took ~4+ minutes to start. Note that there's now a cirrusci image with visual studio 2022, which allows building postgres. I tried it here. https://cirrus-ci.com/task/4939320325832704 Scheduled in 01:19 It makes sense that that's faster since it has none of the goodies or postgres-specific stuff in your image: debugging tools, perl, python, flex, bison, ssl, zlib, ICU... But look: https://cirrus-ci.com/task/4953593575899136 => I installed most of the goodies, but deferred running the installers until the image is run, and it starts just as quickly. It has to run the installer each time, rather than once when building the image. That's crummy, but it's still 1-2 minutes faster than now. Maybe for some of those, it's not needed to run an installer at all. (Like if the installer can be extracted into c:\programfiles). -- Justin
Re: pg15b3: crash in paralell vacuum
On Thu, Aug 18, 2022 at 7:25 AM Masahiko Sawada wrote: > Justin, if it's reproducible in your environment, could you please try > it again with the attached patch? Pushed, thanks. I wonder how this issue could have been caught earlier, or even avoided in the first place. Would the bug have been caught if Valgrind had known to mark dynamic shared memory VALGRIND_MAKE_MEM_UNDEFINED() when it is first allocated? ISTM that we should do something that is analogous to aset.c's Valgrind handling for palloc() requests. Similar work on buffers in shared memory led to us catching a tricky bug involving unsafe access to a buffer, a little while ago -- see bugfix commit 7b7ed046. The bug in question would probably have taken much longer to catch without the instrumentation. In fact, it seems like a good idea to use Valgrind for *anything* where it *might* catch bugs, just in case. Valgrind can work well for shared memory without any extra work. The backend's own idea of the memory (the memory mapping used by the process) is all that Valgrind cares about. You don't have to worry about Valgrind instrumentation in one backend causing confusion in another backend. It's very practical, and very general purpose. I think that most of the protection comes from a basic understanding of "this memory is unsafe to access, this memory contains uninitialized data that cannot be assumed to have any particular value, this memory is initialized and safe". -- Peter Geoghegan
Re: shadow variables - pg15 edition
On Fri, Aug 19, 2022 at 9:21 AM Justin Pryzby wrote: > > On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote: > > 0001. I'd also rather see these 4 renamed: > .. > > 0002. I don't really like the "my" name. I also see you've added the > .. > > How about just "tinfo"? > .. > > 0005. How about just "tslot". I'm not a fan of "this". > .. > > Since I'm not a fan of "this", how about the outer one gets renamed > .. > > 0007. Meh, more "this". How about just "col". > .. > > 0008. Sorry, I had to change this one too. > > I agree that ii_oid and newtype are better names (although it's a bit > unfortunate to rename the outer "ttype" var of wider scope). > > > 0003. The following is used for the exact same purpose as its shadowed > > counterpart. I suggest just using the variable from the outer scope. > > And that's what my original patch did, before people insisted that the patches > shouldn't change variable scope. Now it's back to where I stared. > > > There's a discussion about reverting this entire patch. Not sure if > > patching master and not backpatching to pg15 would be useful to the > > people who may be doing that revert. > > I think if it were reverted, it'd be in both branches. > > > I've attached a patch which does things more along the lines of how I > > would have done it. I don't think we should be back patching this > > stuff. > > > > Any objections to pushing this to master only? > > I won't object, but some of your changes are what makes backpatching this less > reasonable (foreach_current_index and newtype). I had made these v15 patches > first to simplify backpatching, since having the same code in v15 means that > there's no backpatch hazard for this new-in-v15 code. > > I am opened to presenting the patches differently, but we need to come up with > a better process than one person writing patches and someone else rewriting > it. > I also don't see the value of debating which order to write the patches in. > Grouping by variable name or doing other statistical analysis doesn't change > the fact that there are 50+ issues to address to allow -Wshadow to be usable. > > Maybe these would be helpful ? > - if I publish the patches on github; > - if I send the patches with more context; > - if you have an suggestion/objection/complaint with a patch, I can address > it >and/or re-arrange the patchset so this is later, and all the polished >patches are presented first. > Starting off with patches might come to grief, and it won't be much fun rearranging patches over and over. Because there are so many changes, I think it would be better to attack this task methodically: STEP 1 - Capture every shadow warning and categorise exactly what kind is it. e.g maybe do this as some XLS which can be shared. The last time I looked there were hundreds of instances, but I expect there will be less than a couple of dozen different *categories* of them. e.g. shadow of a global var e.g. shadow of a function param e.g. shadow of a function var in a code block for the exact same usage e.g. shadow of a function var in a code block for some 'tmp' var e.g. shadow of a function var in a code block due to a mistake e.g. shadow of a function var by some loop index e.g. shadow of a function var for some loop 'first' handling e.g. bug etc... STEP 2 - Define your rules for how intend to address each of these kinds of shadows (e.g. just simple rename of the var, use 'foreach_current_index', ...). Hopefully, it will be easy to reach an agreement now since all instances of the same kind will look pretty much the same. STEP 3 - Fix all of the same kinds of shadows per single patch (using the already agreed fix approach from step 2). REPEAT STEPS 2,3 until done. -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Remaining case where reltuples can become distorted across multiple VACUUM operations
On Thu, Aug 11, 2022 at 1:48 AM Matthias van de Meent
wrote:
> I think I understand your reasoning, but I don't agree with the
> conclusion. The attached patch 0002 does fix that skew too, at what I
> consider negligible cost. 0001 is your patch with a new version
> number.
Your patch added allowSystemTableMods to one of the tests. I guess
that this was an oversight?
> I'm fine with your patch as is, but would appreciate it if known
> estimate mistakes would also be fixed.
Why do you think that this particular scenario/example deserves
special attention? As I've acknowledged already, it is true that your
scenario is one in which we provably give a less accurate estimate,
based on already-available information. But other than that, I don't
see any underlying principle that would be violated by my original
patch (any kind of principle, held by anybody). reltuples is just an
estimate.
I was thinking of going your way on this, purely because it didn't
seem like there'd be much harm in it (why not just handle your case
and be done with it?). But I don't think that it's a good idea now.
reltuples is usually derived by ANALYZE using a random sample, so the
idea that tuple density can be derived accurately enough from a random
sample is pretty baked in. You're talking about a case where ignoring
just one page ("sampling" all but one of the pages) *isn't* good
enough. It just doesn't seem like something that needs to be addressed
-- it's quite awkward to do so.
Barring any further objections, I plan on committing the original
version tomorrow.
> An alternative solution could be doing double-vetting, where we ignore
> tuples_scanned if <2% of pages AND <2% of previous estimated tuples
> was scanned.
I'm not sure that I've understood you, but I think that you're talking
about remembering more information (in pg_class), which is surely out
of scope for a bug fix.
--
Peter Geoghegan
Re: shadow variables - pg15 edition
On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote: > 0001. I'd also rather see these 4 renamed: .. > 0002. I don't really like the "my" name. I also see you've added the .. > How about just "tinfo"? .. > 0005. How about just "tslot". I'm not a fan of "this". .. > Since I'm not a fan of "this", how about the outer one gets renamed .. > 0007. Meh, more "this". How about just "col". .. > 0008. Sorry, I had to change this one too. I agree that ii_oid and newtype are better names (although it's a bit unfortunate to rename the outer "ttype" var of wider scope). > 0003. The following is used for the exact same purpose as its shadowed > counterpart. I suggest just using the variable from the outer scope. And that's what my original patch did, before people insisted that the patches shouldn't change variable scope. Now it's back to where I stared. > There's a discussion about reverting this entire patch. Not sure if > patching master and not backpatching to pg15 would be useful to the > people who may be doing that revert. I think if it were reverted, it'd be in both branches. > I've attached a patch which does things more along the lines of how I > would have done it. I don't think we should be back patching this > stuff. > > Any objections to pushing this to master only? I won't object, but some of your changes are what makes backpatching this less reasonable (foreach_current_index and newtype). I had made these v15 patches first to simplify backpatching, since having the same code in v15 means that there's no backpatch hazard for this new-in-v15 code. I am opened to presenting the patches differently, but we need to come up with a better process than one person writing patches and someone else rewriting it. I also don't see the value of debating which order to write the patches in. Grouping by variable name or doing other statistical analysis doesn't change the fact that there are 50+ issues to address to allow -Wshadow to be usable. Maybe these would be helpful ? - if I publish the patches on github; - if I send the patches with more context; - if you have an suggestion/objection/complaint with a patch, I can address it and/or re-arrange the patchset so this is later, and all the polished patches are presented first. -- Justin
Re: Perform streaming logical transactions by background workers and parallel apply
Hi Wang-san, Here is some more information about my v21-0001 review [2] posted yesterday. ~~ If the streaming=parallel will be disallowed for publishers not using protocol 4 (see Amit's post [1]), then please ignore all my previous review comments about the protocol descriptions (see [2] comments #4b, #7b, #47a, #47b). ~~ Also, I was having second thoughts about the name replacement for the 'main_worker_pid' member (see [2] comments #1b, #49). Previously I suggested 'apply_leader_pid', but now I think something like 'apply_bgworker_leader_pid' would be better. (It's a bit verbose, but now it gives the proper understanding that only an apply bgworker can have a valid value for this member). -- [1] https://www.postgresql.org/message-id/CAA4eK1JR2GR9jjaz9T1ZxzgLVS0h089EE8ZB%3DF2EsVHbM_5sfA%40mail.gmail.com [2] https://www.postgresql.org/message-id/CAHut%2BPuxEQ88PDhFcBftnNY1BAjdj_9G8FYhTvPHKjP8yfacaQ%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia
Re: pg15b3: crash in parallel vacuum
On Thu, Aug 18, 2022 at 09:52:36AM -0500, Justin Pryzby wrote: > On Thu, Aug 18, 2022 at 11:24:22PM +0900, Masahiko Sawada wrote: > > The status = 11 is invalid value. Probably because indstats was not > > initialized to 0 as I mentioned. > > > > Justin, if it's reproducible in your environment, could you please try > > it again with the attached patch? > > Yes, this seems to resolve the problem. It seems a bit crazy that this escaped detection until now. Are these allocations especially vulnerable to uninitialized data ? -- Justin
Re: s390x builds on buildfarm
On Fri, Aug 19, 2022 at 8:12 AM Vivian Kong wrote: > From: Andres Freund >> Does IBM provide any AIX instances to open source projects? We have access to >> some via the gcc compile farm, but they're a bit outdated, often very >> overloaded, and seem to have some other issues (system perl segfaulting etc). It looks like the way IBM supports open source projects doing POWER development and testing is via the Oregon State U Open Source Lab[1]. It's pretty Linux-focused and I don't see AIX in the OS drop-down list for OpenStack managed virtual machines, but it has "other", and we can see from the GCC build farm machine list[2] that their AIX boxes are hosted there, and a quick search tells me that OpenStack understands AIX[3], so maybe that works or maybe it's a special order. I wonder if we could find an advocate for PostgreSQL on AIX at IBM, for that box on the request form. (More generally, an advocate anywhere would be a nice thing to have for each port...) [1] https://osuosl.org/services/powerdev/ [2] https://cfarm.tetaneutral.net/machines/list/ [3] https://wiki.openstack.org/wiki/PowerVM
RE: s390x builds on buildfarm
Hi Andres, Sorry I don’t have any connections in AIX. I couldn’t find info related to this. Sorry I couldn’t help. Regards, Vivian Kong Linux on IBM Z Open Source Ecosystem IBM Canada Toronto Lab From: Andres Freund Date: Wednesday, August 17, 2022 at 7:19 PM To: Vivian Kong Cc: pgsql-hackers@lists.postgresql.org Subject: [EXTERNAL] Re: s390x builds on buildfarm Hi, On 2022-08-10 13:04:40 +, Vivian Kong wrote: > Are builds being paused on s390x as it looks like the s390x builds were last > run 15 days ago. If so, wondering what is the reason for the pause and what > is required to resume the builds? The OS the builds were running on seems > to have reached end of life. Please let me know if we can help with getting > them updated and resume the builds. I realize the question below is likely not your department, but perhaps you could refer us to the right people? Does IBM provide any AIX instances to open source projects? We have access to some via the gcc compile farm, but they're a bit outdated, often very overloaded, and seem to have some other issues (system perl segfaulting etc). Greetings, Andres Freund
Re: shared-memory based stats collector - v70
Hi, On 2022-08-18 15:26:31 -0400, Greg Stark wrote: > And indexes of course. It's a bit frustrating since without the > catalog you won't know what table the index actually is for... But > they're pretty important stats. FWIW, I think we should split relation stats into table and index stats. Historically it'd have added a lot of complexity to separate the two, but I don't think that's the case anymore. And we waste space for index stats by having lots of table specific fields. > On that note though... What do you think about having the capability > to add other stats kinds to the stats infrastructure? Getting closer to that was one of my goals working on the shared memory stats stuff. > It would make a lot of sense for pg_stat_statements to add its entries here > instead of having to reimplement a lot of the same magic. Yes, we should move pg_stat_statements over. It's pretty easy to get massive contention on stats entries with pg_stat_statements, because it doesn't have support for "batching" updates to shared stats. And reimplementing the same logic in pg_stat_statements.c doesn't make sense. And the set of normalized queries could probably stored in DSA as well - the file based thing we have right now is problematic. > To do that I guess more of the code needs to be moved to be table > driven from the kind structs either with callbacks or with other meta > data. Pretty much all of it already is. The only substantial missing bit is reading/writing of stats files, but that should be pretty easy. And of course making the callback array extensible. > So the kind record could contain tupledesc and the code to construct the > returned tuple so that these functions could return any custom entry as well > as the standard entries. I don't see how this would work well - we don't have functions returning variable kinds of tuples. And what would convert a struct to a tuple? Nor do I think it's needed - if you have an extension providing a new stats kind it can also provide accessors. Greetings, Andres Freund
Re: shared-memory based stats collector - v70
On Thu, 18 Aug 2022 at 02:27, Drouvot, Bertrand wrote: > > What I had in mind is to provide an API to retrieve stats for those that > would need to connect to each database individually otherwise. > > That's why I focused on PGSTAT_KIND_RELATION that has > PgStat_KindInfo.accessed_across_databases set to false. > > I think that another candidate could also be PGSTAT_KIND_FUNCTION. And indexes of course. It's a bit frustrating since without the catalog you won't know what table the index actually is for... But they're pretty important stats. On that note though... What do you think about having the capability to add other stats kinds to the stats infrastructure? It would make a lot of sense for pg_stat_statements to add its entries here instead of having to reimplement a lot of the same magic. And I have in mind an extension that allows adding other stats and it would be nice to avoid having to reimplement any of this. To do that I guess more of the code needs to be moved to be table driven from the kind structs either with callbacks or with other meta data. So the kind record could contain tupledesc and the code to construct the returned tuple so that these functions could return any custom entry as well as the standard entries. -- greg
Re: Another dead configure test
Andres Freund writes: > On 2022-08-18 13:00:28 -0400, Tom Lane wrote: >> I wondered about that, but we do need TCL_SHARED_BUILD in configure >> itself, and the PGAC_EVAL_TCLCONFIGSH macro is going to AC_SUBST it. >> We could remove the line in Makefile.global but I don't think that >> buys much, and it might be more confusing not less so. >> From the meson-generates-Makefile.global angle I like fewer symbols that have > to be considered in Makefile.global.in :). But even leaving that aside, I > think it's clearer to not have things in Makefile.global if they're not used. > But it's obviously not important. Yeah, I'm not excited about it either way --- feel free to change if you'd rather. regards, tom lane
Re: Skipping schema changes in publication
On Mon, Aug 8, 2022 at 2:53 PM vignesh C wrote:
>
> On Mon, Aug 8, 2022 at 12:46 PM vignesh C wrote:
> >
> > On Fri, Jun 3, 2022 at 3:36 PM vignesh C wrote:
> > >
> > > On Thu, May 26, 2022 at 7:04 PM osumi.takami...@fujitsu.com
> > > wrote:
> > > >
> > > > On Monday, May 23, 2022 2:13 PM vignesh C wrote:
> > > > > Attached v7 patch which fixes the buildfarm warning for an unused
> > > > > warning in
> > > > > release mode as in [1].
> > > > Hi, thank you for the patches.
> > > >
> > > >
> > > > I'll share several review comments.
> > > >
> > > > For v7-0001.
> > > >
> > > > (1) I'll suggest some minor rewording.
> > > >
> > > > +
> > > > + The RESET clause will reset the publication to
> > > > the
> > > > + default state which includes resetting the publication options,
> > > > setting
> > > > + ALL TABLES flag to false and
> > > > + dropping all relations and schemas that are associated with the
> > > > publication.
> > > >
> > > > My suggestion is
> > > > "The RESET clause will reset the publication to the
> > > > default state. It resets the publication operations,
> > > > sets ALL TABLES flag to false and drops all relations
> > > > and schemas associated with the publication."
> > >
> > > I felt the existing looks better. I would prefer to keep it that way.
> > >
> > > > (2) typo and rewording
> > > >
> > > > +/*
> > > > + * Reset the publication.
> > > > + *
> > > > + * Reset the publication options, setting ALL TABLES flag to false and
> > > > drop
> > > > + * all relations and schemas that are associated with the publication.
> > > > + */
> > > >
> > > > The "setting" in this sentence should be "set".
> > > >
> > > > How about changing like below ?
> > > > FROM:
> > > > "Reset the publication options, setting ALL TABLES flag to false and
> > > > drop
> > > > all relations and schemas that are associated with the publication."
> > > > TO:
> > > > "Reset the publication operations, set ALL TABLES flag to false and drop
> > > > all relations and schemas associated with the publication."
> > >
> > > I felt the existing looks better. I would prefer to keep it that way.
> > >
> > > > (3) AlterPublicationReset
> > > >
> > > > Do we need to call CacheInvalidateRelcacheAll() or
> > > > InvalidatePublicationRels() at the end of
> > > > AlterPublicationReset() like AlterPublicationOptions() ?
> > >
> > > CacheInvalidateRelcacheAll should be called if we change all tables
> > > from true to false, else the cache will not be invalidated. Modified
> > >
> > > >
> > > > For v7-0002.
> > > >
> > > > (4)
> > > >
> > > > + if (stmt->for_all_tables)
> > > > + {
> > > > + boolisdefault =
> > > > CheckPublicationDefValues(tup);
> > > > +
> > > > + if (!isdefault)
> > > > + ereport(ERROR,
> > > > +
> > > > errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > > > + errmsg("adding ALL TABLES
> > > > requires the publication to have default publication options, no
> > > > tables/
> > > > + errhint("Use ALTER PUBLICATION
> > > > ... RESET to reset the publication"));
> > > >
> > > >
> > > > The errmsg string has three messages for user and is a bit long
> > > > (we have two sentences there connected by 'and').
> > > > Can't we make it concise and split it into a couple of lines for code
> > > > readability ?
> > > >
> > > > I'll suggest a change below.
> > > > FROM:
> > > > "adding ALL TABLES requires the publication to have default publication
> > > > options, no tables/schemas associated and ALL TABLES flag should not be
> > > > set"
> > > > TO:
> > > > "adding ALL TABLES requires the publication defined not for ALL TABLES"
> > > > "to have default publish actions without any associated tables/schemas"
> > >
> > > Added errdetail and split it
> > >
> > > > (5) typo
> > > >
> > > >
> > > > +EXCEPT TABLE
> > > > +
> > > > +
> > > > + This clause specifies a list of tables to exclude from the
> > > > publication.
> > > > + It can only be used with FOR ALL TABLES.
> > > > +
> > > > +
> > > > +
> > > > +
> > > >
> > > > Kindly change
> > > > FROM:
> > > > This clause specifies a list of tables to exclude from the publication.
> > > > TO:
> > > > This clause specifies a list of tables to be excluded from the
> > > > publication.
> > > > or
> > > > This clause specifies a list of tables excluded from the publication.
> > >
> > > Modified
> > >
> > > > (6) Minor suggestion for an expression change
> > > >
> > > >Marks the publication as one that replicates changes for all
> > > > tables in
> > > > - the database, including tables created in the future.
> > > > + the database, including tables created in the future. If
> > > > + EXCEPT TABLE is specified, then exclude
> > > > replicating
> > > > + the changes for the specified tables.
> > > >
>
Re: pg_auth_members.grantor is bunk
On Wed, Aug 10, 2022 at 4:28 PM Robert Haas wrote: > Well, CI isn't happy with this, and for good reason: CI is happier with this version, so I've committed 0001. If no major problems emerge, I'll proceed with 0002 as well. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Another dead configure test
Hi, On 2022-08-18 13:00:28 -0400, Tom Lane wrote: > Andres Freund writes: > > Looks good, except that it perhaps could go a tad further: TCL_SHARED_BUILD > > isn't used either afaics? > > I wondered about that, but we do need TCL_SHARED_BUILD in configure > itself, and the PGAC_EVAL_TCLCONFIGSH macro is going to AC_SUBST it. > We could remove the line in Makefile.global but I don't think that > buys much, and it might be more confusing not less so. >From the meson-generates-Makefile.global angle I like fewer symbols that have to be considered in Makefile.global.in :). But even leaving that aside, I think it's clearer to not have things in Makefile.global if they're not used. But it's obviously not important. Greetings, Andres Freund
Re: Another dead configure test
Andres Freund writes: > Looks good, except that it perhaps could go a tad further: TCL_SHARED_BUILD > isn't used either afaics? I wondered about that, but we do need TCL_SHARED_BUILD in configure itself, and the PGAC_EVAL_TCLCONFIGSH macro is going to AC_SUBST it. We could remove the line in Makefile.global but I don't think that buys much, and it might be more confusing not less so. regards, tom lane
Re: Another dead configure test
Hi, On 2022-08-18 11:04:03 -0400, Tom Lane wrote: > I happened to notice that configure extracts TCL_SHLIB_LD_LIBS > from tclConfig.sh, and puts the value into Makefile.global, > but then we never use it anywhere. AFAICT the only use went > away in cd75f94da, in 2003. I propose the attached. Looks good, except that it perhaps could go a tad further: TCL_SHARED_BUILD isn't used either afaics? Greetings, Andres Freund
Re: XLogBeginRead's header comment lies
On Wed, Aug 17, 2022 at 10:57 AM Robert Haas wrote: > Yeah, that looks right to me. I'm inclined to commit your patch with > some changes to wording of the comments. I'm also inclined not to > back-patch, since we don't know that this breaks anything for existing > users of the xlogreader facility. Done. -- Robert Haas EDB: http://www.enterprisedb.com
Re: static libpq (and other libraries) overwritten on aix
Hi, On 2022-08-17 21:59:29 -0700, Noah Misch wrote: > The AIX sections of Makefile.shlib misuse the terms "static" and "shared". > > Imagine s/static library/library ending in .a/ and s/shared library/library > ending in .so/. That yields an accurate description of the makefile rules. I realize that aspect. My point is that we currently, across most of the other platforms, support building a "proper" static library, and install it too. But on AIX (and I think mingw), we don't, but without an explicit comment about not doing so. In fact, the all-static-lib target on those platforms will build a non-static library, which seems not great. > >There could also be > >a race in the buildrules leading to sometimes static libs sometimes > > shared > >libs winning, I think. > > Not since commit e8564ef, to my knowledge. I'd missed that the $(stlib): ... bit is not defined due to haslibarule being defined... > Along the lines of Robert's comment, it could be a nice code beautification to > use a different suffix for the short-lived .a file. Perhaps _so_inputs.a. Agreed, it'd be an improvement. Afaict we could just stop building the intermediary static lib. Afaict the MKLDEXPORT path isn't needed for libraries without an exports.txt because the linker defaults to exporting "most" symbols, and for symbols with an exports.txt we don't need it either. The only path that really needs MKLDEXPORT is postgres. Not really for the export side either, but for the import side. Greetings, Andres Freund
Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints
On Wed, Aug 17, 2022 at 12:02 AM Dilip Kumar wrote: > > Although 0002 is formally a performance optimization, I'm inclined to > > think that if we're going to commit it, it should also be back-patched > > into v15, because letting the code diverge when we're not even out of > > beta yet seems painful. > > Yeah that makes sense to me. OK, done. -- Robert Haas EDB: http://www.enterprisedb.com
Another dead configure test
I happened to notice that configure extracts TCL_SHLIB_LD_LIBS from tclConfig.sh, and puts the value into Makefile.global, but then we never use it anywhere. AFAICT the only use went away in cd75f94da, in 2003. I propose the attached. regards, tom lane diff --git a/configure b/configure index b7fd6c5f4e..9e8ac510ac 100755 --- a/configure +++ b/configure @@ -634,7 +634,6 @@ DBTOEPUB FOP XSLTPROC XMLLINT -TCL_SHLIB_LD_LIBS TCL_SHARED_BUILD TCL_LIB_SPEC TCL_LIBS @@ -18818,7 +18817,7 @@ eval TCL_LIBS=\"$TCL_LIBS\" eval TCL_LIB_SPEC=\"$TCL_LIB_SPEC\" eval TCL_SHARED_BUILD=\"$TCL_SHARED_BUILD\" -if test "$TCL_SHARED_BUILD" != 1; then +if test "$TCL_SHARED_BUILD" != 1; then as_fn_error $? "cannot build PL/Tcl because Tcl is not a shared library Use --without-tcl to disable building PL/Tcl." "$LINENO" 5 fi diff --git a/configure.ac b/configure.ac index e5740f4fb5..67cf317c3b 100644 --- a/configure.ac +++ b/configure.ac @@ -2252,7 +2252,6 @@ if test "$with_tcl" = yes; then PGAC_PATH_TCLCONFIGSH([$with_tclconfig]) PGAC_EVAL_TCLCONFIGSH([$TCL_CONFIG_SH], [TCL_INCLUDE_SPEC,TCL_LIBS,TCL_LIB_SPEC,TCL_SHARED_BUILD]) -AC_SUBST(TCL_SHLIB_LD_LIBS)dnl don't want to double-evaluate that one if test "$TCL_SHARED_BUILD" != 1; then AC_MSG_ERROR([cannot build PL/Tcl because Tcl is not a shared library Use --without-tcl to disable building PL/Tcl.]) diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 0625b60c43..5664c645f8 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -221,7 +221,6 @@ TCL_LIBS = @TCL_LIBS@ TCL_LIB_SPEC = @TCL_LIB_SPEC@ TCL_INCLUDE_SPEC = @TCL_INCLUDE_SPEC@ TCL_SHARED_BUILD = @TCL_SHARED_BUILD@ -TCL_SHLIB_LD_LIBS = @TCL_SHLIB_LD_LIBS@ PTHREAD_CFLAGS = @PTHREAD_CFLAGS@ PTHREAD_LIBS = @PTHREAD_LIBS@
Re: Add support for DEFAULT specification in COPY FROM
On 2022-08-18 Th 05:55, Dagfinn Ilmari Mannsåker wrote: > Andrew Dunstan writes: > >> On 2022-08-16 Tu 14:12, Israel Barth Rubio wrote: >>> Hello all, >>> >>> With the current implementation of COPY FROM in PostgreSQL we are >>> able to load the DEFAULT value/expression of a column if the column >>> is absent in the list of specified columns. We are not able to >>> explicitly ask that PostgreSQL uses the DEFAULT value/expression in a >>> column that is being fetched from the input file, though. >>> >>> This patch adds support for handling DEFAULT values in COPY FROM. It >>> works similarly to NULL in COPY FROM: whenever the marker that was >>> set for DEFAULT value/expression is read from the input stream, it >>> will evaluate the DEFAULT value/expression of the corresponding >>> column. > […] >> Interesting, and probably useful. I've only had a brief look, but it's >> important that the default marker not be quoted in CSV mode (c.f. NULL) >> -f it is it should be taken as a literal rather than a special value. > For the NULL marker that can be overridden for individual columns with > the FORCE(_NOT)_NULL option. This feature should have a similar > FORCE(_NOT)_DEFAULT option to allow the DEFAULT marker to be ignored, or > recognised even when quoted, respectively. > That seems to be over-egging the pudding somewhat. FORCE_NOT_DEFAULT should not be necessary at all, since here if there's no default specified nothing will be taken as the default. I suppose a quoted default is just faintly possible, but I'd like a concrete example of a producer that emitted it. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: cataloguing NOT NULL constraints
On Thu, 2022-08-18 at 11:04 +0200, Alvaro Herrera wrote: > On 2022-Aug-18, Laurenz Albe wrote: > > On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote: > > > Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull' > > > bit is lost when the last one such constraint goes away. > > > > Wouldn't it be the correct solution to set "attnotnumm" to FALSE only > > when the last NOT NULL constraint is dropped? > > ... when the last NOT NULL or PRIMARY KEY constraint is dropped. We > have to keep attnotnull set when a PK exists even if there's no specific > NOT NULL constraint. Of course, I forgot that. I hope that is not too hard to implement. > > > 2. If a table has a primary key, and a table is created that inherits > > > from it, then the child has its column(s) marked attnotnull but there > > > is no pg_constraint row for that. This is not okay. But what should > > > happen? > > > > > > 1. a CHECK(col IS NOT NULL) constraint is created for each column > > > 2. a PRIMARY KEY () constraint is created > > > > I think it would be best to create a primary key constraint on the > > partition. > > Sorry, I wasn't specific enough. This applies to legacy inheritance > only; partitioning has its own solution (as you say: the PK constraint > exists), but legacy inheritance works differently. Creating a PK in > children tables is not feasible (because unicity cannot be maintained), > but creating a CHECK (NOT NULL) constraint is possible. > > I think a PRIMARY KEY should not be allowed to exist in an inheritance > parent, precisely because of this problem, but it seems too late to add > that restriction now. This behavior is absurd, but longstanding: My mistake; you clearly said "inherits". Since such an inheritance child currently does not have a primary key, you can insert duplicates. So automatically adding a NUT NULL constraint on the inheritance child seems the only solution that does not break backwards compatibility. pg_upgrade would have to be able to cope with that. Forcing a primary key constraint on the inheritance child could present an upgrade problem. Even if that is probably a rare and strange case, I don't think we should risk that. Moreover, if we force a primary key on the inheritance child, using ALTER TABLE ... INHERIT might have to create a unique index on the table, which can be cumbersome if the table is large. So I think a NOT NULL constraint is the least evil. Yours, Laurenz Albe
Re: pg15b3: crash in paralell vacuum
On Thu, Aug 18, 2022 at 11:24:22PM +0900, Masahiko Sawada wrote: > The status = 11 is invalid value. Probably because indstats was not > initialized to 0 as I mentioned. > > Justin, if it's reproducible in your environment, could you please try > it again with the attached patch? Yes, this seems to resolve the problem. Thanks, -- Justin
Re: TAP output format in pg_regress
On 2022-08-18 Th 07:24, Daniel Gustafsson wrote: > > One thing I haven't researched yet is if the Buildfarm or CFBot parses the > current output in any way or just check the exit status of the testrun. > > Buildfarm: just the status. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Add support for DEFAULT specification in COPY FROM
Hello Ilmari, Thanks for checking it, too. I can study to implement these changes to include a way of overriding the behavior for the given columns. Regards, Israel. Em qui., 18 de ago. de 2022 às 06:56, Dagfinn Ilmari Mannsåker < ilm...@ilmari.org> escreveu: > Andrew Dunstan writes: > > > On 2022-08-16 Tu 14:12, Israel Barth Rubio wrote: > >> Hello all, > >> > >> With the current implementation of COPY FROM in PostgreSQL we are > >> able to load the DEFAULT value/expression of a column if the column > >> is absent in the list of specified columns. We are not able to > >> explicitly ask that PostgreSQL uses the DEFAULT value/expression in a > >> column that is being fetched from the input file, though. > >> > >> This patch adds support for handling DEFAULT values in COPY FROM. It > >> works similarly to NULL in COPY FROM: whenever the marker that was > >> set for DEFAULT value/expression is read from the input stream, it > >> will evaluate the DEFAULT value/expression of the corresponding > >> column. > […] > > Interesting, and probably useful. I've only had a brief look, but it's > > important that the default marker not be quoted in CSV mode (c.f. NULL) > > -f it is it should be taken as a literal rather than a special value. > > For the NULL marker that can be overridden for individual columns with > the FORCE(_NOT)_NULL option. This feature should have a similar > FORCE(_NOT)_DEFAULT option to allow the DEFAULT marker to be ignored, or > recognised even when quoted, respectively. > > - ilmari >
Re: s390x builds on buildfarm
Hi everyone, On Wed, Aug 10, 2022 at 6:56 AM Andrew Dunstan wrote: > > On 2022-08-10 We 09:04, Vivian Kong wrote: > > > > Hi, > > > > > > > > Are builds being paused on s390x as it looks like the s390x builds > > were last run 15 days ago. If so, wondering what is the reason for > > the pause and what is required to resume the builds? > > The OS the builds were running on seems to have reached end of life. > > Please let me know if we can help with getting them updated and resume > > the builds. > > > > > > > > > > Mark, I think you run most or all of these. Yeah, IBM moved me to new hardware and I haven't set them up yet. I will try to do that soon. Regards, Mark
Re: Add support for DEFAULT specification in COPY FROM
Hello, Thanks for your review. I submitted the patch to the next commit fest (https://commitfest.postgresql.org/39/3822/). Regards, Israel. Em qua., 17 de ago. de 2022 às 18:56, Andrew Dunstan escreveu: > > On 2022-08-17 We 17:12, Israel Barth Rubio wrote: > > > > > > Does that address your concerns? > > > > I am attaching the new patch, containing the above test in the regress > > suite. > > > Thanks, yes, that all looks sane. > > > Please add this to the next CommitFest if you haven't already done so. > > > cheers > > > andrew > > > -- > Andrew Dunstan > EDB: https://www.enterprisedb.com > >
Re: static libpq (and other libraries) overwritten on aix
On 2022-08-18 Th 10:10, Robert Haas wrote: > On Thu, Aug 18, 2022 at 12:59 AM Noah Misch wrote: >> Along the lines of Robert's comment, it could be a nice code beautification >> to >> use a different suffix for the short-lived .a file. Perhaps _so_inputs.a. > Yeah, this is the kind of thing I was thinking about. +1 for that and clarifying Makefile.shlib. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Skipping schema changes in publication
On Thu, Aug 18, 2022 at 12:33 PM Nitin Jadhav wrote: > > I spent some time on understanding the proposal and the patch. Here > are a few comments wrt the test cases. > > > +ALTER PUBLICATION testpub_reset ADD TABLE pub_sch1.tbl1; > > + > > +-- Verify that tables associated with the publication are dropped after > > RESET > > +\dRp+ testpub_reset > > +ALTER PUBLICATION testpub_reset RESET; > > +\dRp+ testpub_reset > > > > +ALTER PUBLICATION testpub_reset ADD ALL TABLES IN SCHEMA public; > > + > > +-- Verify that schemas associated with the publication are dropped after > > RESET > > +\dRp+ testpub_reset > > +ALTER PUBLICATION testpub_reset RESET; > > +\dRp+ testpub_reset > > The results for the above two cases are the same before and after the > reset. Is there any way to verify that? If you see the expected, first \dRp+ command includes: +Tables: +"pub_sch1.tbl1" The second \dRp+ does not include the Tables. We are trying to verify that after reset, the tables will be removed from the publication. The second test is similar to the first, the only difference here is that we test schema instead of tables. i.e we verify that the schemas will be removed from the publication. > --- > > > +-- Can't add EXCEPT TABLE to 'FOR ALL TABLES' publication > > +ALTER PUBLICATION testpub_reset ADD ALL TABLES EXCEPT TABLE pub_sch1.tbl1; > > + > > > > +-- Can't add EXCEPT TABLE to 'FOR TABLE' publication > > +ALTER PUBLICATION testpub_reset ADD ALL TABLES EXCEPT TABLE pub_sch1.tbl1; > > + > > > > +-- Can't add EXCEPT TABLE to 'FOR ALL TABLES IN SCHEMA' publication > > +ALTER PUBLICATION testpub_reset ADD ALL TABLES EXCEPT TABLE pub_sch1.tbl1; > > + > > I did not understand the objective of these tests. I think we need to > improve the comments. There are different publications like "ALL TABLES", "TABLE", "ALL TABLES IN SCHEMA" publications. Here we are trying to verify that except tables cannot be added to "ALL TABLES", "TABLE", "ALL TABLES IN SCHEMA" publications. If you see the expected file, you will see the following error: +-- Can't add EXCEPT TABLE to 'FOR ALL TABLES' publication +ALTER PUBLICATION testpub_reset ADD ALL TABLES EXCEPT TABLE pub_sch1.tbl1; +ERROR: adding ALL TABLES requires the publication to have default publication parameter values +DETAIL: ALL TABLES flag should not be set and no tables/schemas should be associated. +HINT: Use ALTER PUBLICATION ... RESET to reset the publication I felt the existing comment is ok. Let me know if you still feel any change is required. Regards, Vignesh
Re: pg15b3: crash in paralell vacuum
On Thu, Aug 18, 2022 at 11:04 PM Justin Pryzby wrote:
>
> On Thu, Aug 18, 2022 at 08:34:06AM -0500, Justin Pryzby wrote:
> > Unfortunately, it looks like the RPM packages are compiled with -O2, so
> > this is
> > of limited use. So I'll be back shortly with more...
>
> #3 0x006874f1 in parallel_vacuum_process_all_indexes (pvs=0x25bdce0,
> num_index_scans=0, vacuum=vacuum@entry=false) at vacuumparallel.c:611
> 611 Assert(indstats->status ==
> PARALLEL_INDVAC_STATUS_INITIAL);
>
> (gdb) p *pvs
> $1 = {pcxt = 0x25bc1e0, indrels = 0x25bbf70, nindexes = 8, shared =
> 0x7fc5184393a0, indstats = 0x7fc5184393e0, dead_items = 0x7fc5144393a0,
> buffer_usage = 0x7fc514439280, wal_usage = 0x7fc514439240,
> will_parallel_vacuum = 0x266d818, nindexes_parallel_bulkdel = 5,
> nindexes_parallel_cleanup = 0, nindexes_parallel_condcleanup = 5, bstrategy =
> 0x264f120, relnamespace = 0x0, relname = 0x0, indname = 0x0,
> status = PARALLEL_INDVAC_STATUS_INITIAL}
>
> (gdb) p *indstats
> $2 = {status = 11, parallel_workers_can_process = false, istat_updated =
> false, istat = {num_pages = 0, estimated_count = false, num_index_tuples = 0,
> tuples_removed = 0, pages_newly_deleted = 0, pages_deleted = 1,
> pages_free = 0}}
The status = 11 is invalid value. Probably because indstats was not
initialized to 0 as I mentioned.
Justin, if it's reproducible in your environment, could you please try
it again with the attached patch?
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
initialize_indstats.patch
Description: Binary data
Re: pg15b3: crash in paralell vacuum
On Thu, Aug 18, 2022 at 11:06 PM Masahiko Sawada wrote:
>
> Hi,
>
> On Thu, Aug 18, 2022 at 10:34 PM Justin Pryzby wrote:
> >
> > Immediately after upgrading an internal instance, a loop around "vacuum" did
> > this:
>
> Thank you for the report!
>
> >
> > TRAP: FailedAssertion("indstats->status == PARALLEL_INDVAC_STATUS_INITIAL",
> > File: "vacuumparallel.c", Line: 611, PID: 27635)
> > postgres: postgres pryzbyj [local]
> > VACUUM(ExceptionalCondition+0x8d)[0x99d9fd]
> > postgres: postgres pryzbyj [local] VACUUM[0x6915db]
> > postgres: postgres pryzbyj [local] VACUUM(heap_vacuum_rel+0x12b6)[0x5083e6]
> > postgres: postgres pryzbyj [local] VACUUM[0x68e97a]
> > postgres: postgres pryzbyj [local] VACUUM(vacuum+0x48e)[0x68fe9e]
> > postgres: postgres pryzbyj [local] VACUUM(ExecVacuum+0x2ae)[0x69065e]
> > postgres: postgres pryzbyj [local]
> > VACUUM(standard_ProcessUtility+0x530)[0x8567b0]
> > /usr/pgsql-15/lib/pg_stat_statements.so(+0x5450)[0x7f52b891c450]
> > postgres: postgres pryzbyj [local] VACUUM[0x85490a]
> > postgres: postgres pryzbyj [local] VACUUM[0x854a53]
> > postgres: postgres pryzbyj [local] VACUUM(PortalRun+0x179)[0x855029]
> > postgres: postgres pryzbyj [local] VACUUM[0x85099b]
> > postgres: postgres pryzbyj [local] VACUUM(PostgresMain+0x199a)[0x85268a]
> > postgres: postgres pryzbyj [local] VACUUM[0x496a21]
> > postgres: postgres pryzbyj [local] VACUUM(PostmasterMain+0x11c0)[0x7b3980]
> > postgres: postgres pryzbyj [local] VACUUM(main+0x1c6)[0x4986a6]
> > /lib64/libc.so.6(__libc_start_main+0xf5)[0x7f52c4b893d5]
> > postgres: postgres pryzbyj [local] VACUUM[0x498c59]
> > < 2022-08-18 07:56:51.963 CDT >LOG: server process (PID 27635) was
> > terminated by signal 6: Aborted
> > < 2022-08-18 07:56:51.963 CDT >DETAIL: Failed process was running: VACUUM
> > ANALYZE alarms
> >
> > Unfortunately, it looks like the RPM packages are compiled with -O2, so
> > this is
> > of limited use.
> >
> > Core was generated by `postgres: postgres pryzbyj [local] VACUUM
> > '.
> > Program terminated with signal 6, Aborted.
> > #0 0x7f52c4b9d207 in raise () from /lib64/libc.so.6
> > Missing separate debuginfos, use: debuginfo-install
> > audit-libs-2.8.4-4.el7.x86_64 bzip2-libs-1.0.6-13.el7.x86_64
> > cyrus-sasl-lib-2.1.26-23.el7.x86_64 elfutils-libelf-0.176-5.el7.x86_64
> > elfutils-libs-0.176-5.el7.x86_64 glibc-2.17-260.el7_6.3.x86_64
> > keyutils-libs-1.5.8-3.el7.x86_64 krb5-libs-1.15.1-51.el7_9.x86_64
> > libattr-2.4.46-13.el7.x86_64 libcap-2.22-9.el7.x86_64
> > libcap-ng-0.7.5-4.el7.x86_64 libcom_err-1.42.9-19.el7.x86_64
> > libgcc-4.8.5-39.el7.x86_64 libgcrypt-1.5.3-14.el7.x86_64
> > libgpg-error-1.12-3.el7.x86_64 libicu-50.1.2-17.el7.x86_64
> > libselinux-2.5-15.el7.x86_64 libstdc++-4.8.5-39.el7.x86_64
> > libxml2-2.9.1-6.el7_9.6.x86_64 libzstd-1.5.2-1.el7.x86_64
> > lz4-1.7.5-2.el7.x86_64 nspr-4.19.0-1.el7_5.x86_64
> > nss-3.36.0-7.1.el7_6.x86_64 nss-softokn-freebl-3.36.0-5.el7_5.x86_64
> > nss-util-3.36.0-1.1.el7_6.x86_64 openldap-2.4.44-21.el7_6.x86_64
> > openssl-libs-1.0.2k-22.el7_9.x86_64 pam-1.1.8-22.el7.x86_64
> > pcre-8.32-17.el7.x86_64 systemd-libs-219-62.el7_6.5.x86_64
> > xz-libs-5.2.2-1.el7.x86_64 zlib-1.2.7-18.el7.x86_64
> > (gdb) bt
> > #0 0x7f52c4b9d207 in raise () from /lib64/libc.so.6
> > #1 0x7f52c4b9e8f8 in abort () from /lib64/libc.so.6
> > #2 0x0099da1e in ExceptionalCondition
> > (conditionName=conditionName@entry=0xafae40 "indstats->status ==
> > PARALLEL_INDVAC_STATUS_INITIAL", errorType=errorType@entry=0x9fb4b7
> > "FailedAssertion",
> > fileName=fileName@entry=0xafb0c0 "vacuumparallel.c",
> > lineNumber=lineNumber@entry=611) at assert.c:69
> > #3 0x006915db in parallel_vacuum_process_all_indexes
> > (pvs=0x2e85f80, num_index_scans=, vacuum=) at
> > vacuumparallel.c:611
> > #4 0x005083e6 in heap_vacuum_rel (rel=,
> > params=, bstrategy=) at vacuumlazy.c:2679
>
> It seems that parallel_vacuum_cleanup_all_indexes() got called[1],
> which means this was the first time to perform parallel vacuum (i.e.,
> index cleanup).
Sorry, this explanation is wrong. But according to the recent
information from Justin it was the first time to perform parallel
vacuum:
#3 0x006874f1 in parallel_vacuum_process_all_indexes
(pvs=0x25bdce0, num_index_scans=0, vacuum=vacuum@entry=false) at
vacuumparallel.c:611
611 Assert(indstats->status ==
PARALLEL_INDVAC_STATUS_INITIAL);
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Re: static libpq (and other libraries) overwritten on aix
On Thu, Aug 18, 2022 at 12:59 AM Noah Misch wrote: > Along the lines of Robert's comment, it could be a nice code beautification to > use a different suffix for the short-lived .a file. Perhaps _so_inputs.a. Yeah, this is the kind of thing I was thinking about. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg15b3: crash in paralell vacuum
Hi,
On Thu, Aug 18, 2022 at 10:34 PM Justin Pryzby wrote:
>
> Immediately after upgrading an internal instance, a loop around "vacuum" did
> this:
Thank you for the report!
>
> TRAP: FailedAssertion("indstats->status == PARALLEL_INDVAC_STATUS_INITIAL",
> File: "vacuumparallel.c", Line: 611, PID: 27635)
> postgres: postgres pryzbyj [local] VACUUM(ExceptionalCondition+0x8d)[0x99d9fd]
> postgres: postgres pryzbyj [local] VACUUM[0x6915db]
> postgres: postgres pryzbyj [local] VACUUM(heap_vacuum_rel+0x12b6)[0x5083e6]
> postgres: postgres pryzbyj [local] VACUUM[0x68e97a]
> postgres: postgres pryzbyj [local] VACUUM(vacuum+0x48e)[0x68fe9e]
> postgres: postgres pryzbyj [local] VACUUM(ExecVacuum+0x2ae)[0x69065e]
> postgres: postgres pryzbyj [local]
> VACUUM(standard_ProcessUtility+0x530)[0x8567b0]
> /usr/pgsql-15/lib/pg_stat_statements.so(+0x5450)[0x7f52b891c450]
> postgres: postgres pryzbyj [local] VACUUM[0x85490a]
> postgres: postgres pryzbyj [local] VACUUM[0x854a53]
> postgres: postgres pryzbyj [local] VACUUM(PortalRun+0x179)[0x855029]
> postgres: postgres pryzbyj [local] VACUUM[0x85099b]
> postgres: postgres pryzbyj [local] VACUUM(PostgresMain+0x199a)[0x85268a]
> postgres: postgres pryzbyj [local] VACUUM[0x496a21]
> postgres: postgres pryzbyj [local] VACUUM(PostmasterMain+0x11c0)[0x7b3980]
> postgres: postgres pryzbyj [local] VACUUM(main+0x1c6)[0x4986a6]
> /lib64/libc.so.6(__libc_start_main+0xf5)[0x7f52c4b893d5]
> postgres: postgres pryzbyj [local] VACUUM[0x498c59]
> < 2022-08-18 07:56:51.963 CDT >LOG: server process (PID 27635) was
> terminated by signal 6: Aborted
> < 2022-08-18 07:56:51.963 CDT >DETAIL: Failed process was running: VACUUM
> ANALYZE alarms
>
> Unfortunately, it looks like the RPM packages are compiled with -O2, so this
> is
> of limited use.
>
> Core was generated by `postgres: postgres pryzbyj [local] VACUUM
> '.
> Program terminated with signal 6, Aborted.
> #0 0x7f52c4b9d207 in raise () from /lib64/libc.so.6
> Missing separate debuginfos, use: debuginfo-install
> audit-libs-2.8.4-4.el7.x86_64 bzip2-libs-1.0.6-13.el7.x86_64
> cyrus-sasl-lib-2.1.26-23.el7.x86_64 elfutils-libelf-0.176-5.el7.x86_64
> elfutils-libs-0.176-5.el7.x86_64 glibc-2.17-260.el7_6.3.x86_64
> keyutils-libs-1.5.8-3.el7.x86_64 krb5-libs-1.15.1-51.el7_9.x86_64
> libattr-2.4.46-13.el7.x86_64 libcap-2.22-9.el7.x86_64
> libcap-ng-0.7.5-4.el7.x86_64 libcom_err-1.42.9-19.el7.x86_64
> libgcc-4.8.5-39.el7.x86_64 libgcrypt-1.5.3-14.el7.x86_64
> libgpg-error-1.12-3.el7.x86_64 libicu-50.1.2-17.el7.x86_64
> libselinux-2.5-15.el7.x86_64 libstdc++-4.8.5-39.el7.x86_64
> libxml2-2.9.1-6.el7_9.6.x86_64 libzstd-1.5.2-1.el7.x86_64
> lz4-1.7.5-2.el7.x86_64 nspr-4.19.0-1.el7_5.x86_64 nss-3.36.0-7.1.el7_6.x86_64
> nss-softokn-freebl-3.36.0-5.el7_5.x86_64 nss-util-3.36.0-1.1.el7_6.x86_64
> openldap-2.4.44-21.el7_6.x86_64 openssl-libs-1.0.2k-22.el7_9.x86_64
> pam-1.1.8-22.el7.x86_64 pcre-8.32-17.el7.x86_64
> systemd-libs-219-62.el7_6.5.x86_64 xz-libs-5.2.2-1.el7.x86_64
> zlib-1.2.7-18.el7.x86_64
> (gdb) bt
> #0 0x7f52c4b9d207 in raise () from /lib64/libc.so.6
> #1 0x7f52c4b9e8f8 in abort () from /lib64/libc.so.6
> #2 0x0099da1e in ExceptionalCondition
> (conditionName=conditionName@entry=0xafae40 "indstats->status ==
> PARALLEL_INDVAC_STATUS_INITIAL", errorType=errorType@entry=0x9fb4b7
> "FailedAssertion",
> fileName=fileName@entry=0xafb0c0 "vacuumparallel.c",
> lineNumber=lineNumber@entry=611) at assert.c:69
> #3 0x006915db in parallel_vacuum_process_all_indexes (pvs=0x2e85f80,
> num_index_scans=, vacuum=) at
> vacuumparallel.c:611
> #4 0x005083e6 in heap_vacuum_rel (rel=,
> params=, bstrategy=) at vacuumlazy.c:2679
It seems that parallel_vacuum_cleanup_all_indexes() got called[1],
which means this was the first time to perform parallel vacuum (i.e.,
index cleanup).
I'm not convinced yet but it could be a culprit that we missed doing
memset(0) for the shared array of PVIndStats in
parallel_vacuum_init(). This shared array was introduced in PG15.
[1]
https://github.com/postgres/postgres/blob/REL_15_STABLE/src/backend/access/heap/vacuumlazy.c#L2679
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Re: pg15b3: crash in paralell vacuum
On Thu, Aug 18, 2022 at 08:34:06AM -0500, Justin Pryzby wrote:
> Unfortunately, it looks like the RPM packages are compiled with -O2, so this
> is
> of limited use. So I'll be back shortly with more...
#3 0x006874f1 in parallel_vacuum_process_all_indexes (pvs=0x25bdce0,
num_index_scans=0, vacuum=vacuum@entry=false) at vacuumparallel.c:611
611 Assert(indstats->status ==
PARALLEL_INDVAC_STATUS_INITIAL);
(gdb) p *pvs
$1 = {pcxt = 0x25bc1e0, indrels = 0x25bbf70, nindexes = 8, shared =
0x7fc5184393a0, indstats = 0x7fc5184393e0, dead_items = 0x7fc5144393a0,
buffer_usage = 0x7fc514439280, wal_usage = 0x7fc514439240,
will_parallel_vacuum = 0x266d818, nindexes_parallel_bulkdel = 5,
nindexes_parallel_cleanup = 0, nindexes_parallel_condcleanup = 5, bstrategy =
0x264f120, relnamespace = 0x0, relname = 0x0, indname = 0x0,
status = PARALLEL_INDVAC_STATUS_INITIAL}
(gdb) p *indstats
$2 = {status = 11, parallel_workers_can_process = false, istat_updated = false,
istat = {num_pages = 0, estimated_count = false, num_index_tuples = 0,
tuples_removed = 0, pages_newly_deleted = 0, pages_deleted = 1,
pages_free = 0}}
(gdb) bt f
...
#3 0x006874f1 in parallel_vacuum_process_all_indexes (pvs=0x25bdce0,
num_index_scans=0, vacuum=vacuum@entry=false) at vacuumparallel.c:611
indstats = 0x7fc5184393e0
i = 0
nworkers = 2
new_status = PARALLEL_INDVAC_STATUS_NEED_CLEANUP
__func__ = "parallel_vacuum_process_all_indexes"
#4 0x00687ef0 in parallel_vacuum_cleanup_all_indexes (pvs=, num_table_tuples=num_table_tuples@entry=409149,
num_index_scans=, estimated_count=estimated_count@entry=true)
at vacuumparallel.c:486
No locals.
#5 0x004f80b8 in lazy_cleanup_all_indexes
(vacrel=vacrel@entry=0x25bc510) at vacuumlazy.c:2679
reltuples = 409149
estimated_count = true
#6 0x004f884a in lazy_scan_heap (vacrel=vacrel@entry=0x25bc510) at
vacuumlazy.c:1278
rel_pages = 67334
blkno = 67334
next_unskippable_block = 67334
next_failsafe_block = 0
next_fsm_block_to_vacuum = 0
dead_items = 0x7fc5144393a0
vmbuffer = 1300
next_unskippable_allvis = true
skipping_current_range = false
initprog_index = {0, 1, 5}
initprog_val = {1, 67334, 11184809}
__func__ = "lazy_scan_heap"
#7 0x004f925f in heap_vacuum_rel (rel=0x7fc52df6b820,
params=0x7ffd74f74620, bstrategy=0x264f120) at vacuumlazy.c:534
vacrel = 0x25bc510
verbose = true
instrument =
aggressive = false
skipwithvm = true
frozenxid_updated = false
minmulti_updated = false
OldestXmin = 32759288
FreezeLimit = 4277726584
OldestMxact = 157411
MultiXactCutoff = 4290124707
orig_rel_pages = 67334
new_rel_pages =
new_rel_allvisible = 4
ru0 = {tv = {tv_sec = 1660830451, tv_usec = 473980}, ru = {ru_utime =
{tv_sec = 0, tv_usec = 317891}, ru_stime = {tv_sec = 1, tv_usec = 212372},
{ru_maxrss = 74524, __ru_maxrss_word = 74524}, {ru_ixrss = 0,
__ru_ixrss_word = 0}, {ru_idrss = 0, __ru_idrss_word = 0},
{ru_isrss = 0, __ru_isrss_word = 0}, {ru_minflt = 18870, __ru_minflt_word =
18870}, {ru_majflt = 0, __ru_majflt_word = 0}, {ru_nswap = 0,
__ru_nswap_word = 0}, {ru_inblock = 1124750, __ru_inblock_word =
1124750}, {ru_oublock = 0, __ru_oublock_word = 0}, {ru_msgsnd = 0,
__ru_msgsnd_word = 0}, {ru_msgrcv = 0, __ru_msgrcv_word = 0}, {ru_nsignals = 0,
__ru_nsignals_word = 0}, {ru_nvcsw = 42, __ru_nvcsw_word = 42},
{ru_nivcsw = 35, __ru_nivcsw_word = 35}}}
starttime = 714145651473980
startreadtime = 0
startwritetime = 0
startwalusage = {wal_records = 2, wal_fpi = 0, wal_bytes = 421}
StartPageHit = 50
StartPageMiss = 0
StartPageDirty = 0
errcallback = {previous = 0x0, callback = 0x4f5f41
, arg = 0x25bc510}
indnames = 0x266d838
__func__ = "heap_vacuum_rel"
This is a qemu VM which (full disclosure) has crashed a few times recently due
to OOM. This is probably a postgres bug, but conceivably it's being tickled by
bad data (although the vm crashing shouldn't cause that, either, following
recovery). This is also an instance that was pg_upgraded from v14 (and earlier
versions) to v15b1 and then b2, so it's conceivably possible there's weird data
pages that wouldn't be written by beta3. But that doesn't seem to be the issue
here anyway.
--
Justin
Re: Data caching
On Thu, Aug 18, 2022 at 04:12:45PM +0530, Anant ngo wrote: > Hello > > Is there a postgres extension or project related to application-level/ > foreign-table data caching ? The postgres_fdw extension fetches data from > foreign table for each command. > > I have seen previous messages in archive about caching in form of global temp > tables, query cache etc. There are good discussions about whether support > should be built-in but did not find any implementation. > > I have seen the 44 postgres extensions that come pre-installed with ubuntu > 16.04 but none of them do this. You can do foreign-table data caching via materialized views: https://momjian.us/main/blogs/pgblog/2017.html#September_1_2017 Also, this is more of a question for pgsql-gene...@postgresql.org. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
Summary function for pg_buffercache
Hi hackers, Added a pg_buffercache_summary() function to retrieve an aggregated summary information with less cost. It's often useful to know only how many buffers are used, how many of them are dirty etc. for monitoring purposes. This info can already be retrieved by pg_buffercache. The extension currently creates a row with many details for each buffer, then summary info can be aggregated from that returned table. But it is quite expensive to run regularly for monitoring. The attached patch adds a pg_buffercache_summary() function to get this summary info faster. New function only collects following info and returns them in a single row: - used_buffers = number of buffers with a valid relfilenode (both dirty and not) - unused_buffers = number of buffers with invalid relfilenode - dirty_buffers = number of dirty buffers. - pinned_buffers = number of buffers that have at least one pinning backend (i.e. refcount > 0) - average usagecount of used buffers One other difference between pg_buffercache_summary and pg_buffercache_pages is that pg_buffercache_summary does not get locks on buffer headers as opposed to pg_buffercache_pages. Since the purpose of pg_buffercache_summary is just to give us an overall idea about shared buffers and to be a cheaper function, locks are not strictly needed. To compare pg_buffercache_summary() and pg_buffercache_pages(), I used a simple query to aggregate the summary information above by calling pg_buffercache_pages(). Here is the result: postgres=# show shared_buffers; shared_buffers 16GB (1 row) Time: 0.756 ms postgres=# SELECT relfilenode <> 0 AS is_valid, isdirty, count(*) FROM pg_buffercache GROUP BY relfilenode <> 0, isdirty; is_valid | isdirty | count --+-+- t| f | 209 | | 2096904 t| t | 39 (3 rows) Time: 1434.870 ms (00:01.435) postgres=# select * from pg_buffercache_summary(); used_buffers | unused_buffers | dirty_buffers | pinned_buffers | avg_usagecount --++---++ 248 |2096904 |39 | 0 | 3.141129 (1 row) Time: 9.712 ms There is a significant difference between timings of those two functions, even though they return similar results. I would appreciate any feedback/comment on this change. Thanks, Melih 0001-Added-pg_buffercache_summary-function.patch Description: Binary data
Re: Strip -mmacosx-version-min options from plperl build
Peter Eisentraut writes: > This is because the link command uses the option > -mmacosx-version-min=11.3, which comes in from perl_embed_ldflags (perl > -MExtUtils::Embed -e ldopts), but the compile commands don't use that > option, which creates a situation that ld considers inconsistent. > I think an appropriate fix is to strip out the undesired option from > perl_embed_ldflags. We already do that for other options. Proposed > patch attached. Agreed on rejecting -mmacosx-version-min, but I wonder if we should think about adopting a whitelist-instead-of-blacklist approach to adopting stuff from perl_embed_ldflags. ISTR that in pltcl we already use the approach of accepting only -L and -l, and perhaps similar strictness would serve us well here. As an example, on a not-too-new MacPorts install, I see $ /opt/local/bin/perl -MExtUtils::Embed -e ldopts -L/opt/local/lib -Wl,-headerpad_max_install_names -fstack-protector-strong -L/opt/local/lib/perl5/5.28/darwin-thread-multi-2level/CORE -lperl I can't see any really good reason why we should allow perl to be injecting that sort of -f option into the plperl build, and I'm pretty dubious about the -headerpad_max_install_names bit too. I think also that this would allow us to drop the weird dance of trying to subtract ccdlflags. regards, tom lane
Re: RFC: Moving specific attributes from pg_attribute column into attoptions
Hi hackers! Tom, thank you for your feedback! We thought about this because it already seems that custom Toasters could have a bunch of options, so we already thinking how to store them. I'll check if we can implement storing Toaster options in PG_ATTRDEF. Andres Freund complained that 'atttoaster' column extends already the largest catalog table. It is a reasonable complain because atttoaster option only makes sense for columns and datatypes only, and the Default Toaster is accessible by global constant DEFAULT_TOASTER_OID and does not require accessing the PG_ATTRDEF table. Also, we thought about making Toaster responsible for column compression and thus moving 'attcompression' out from PG_ATTRIBUTE column to Toaster options. What do you think about this? Using JSON - accepted, we won't do it. On Wed, Aug 17, 2022 at 11:51 PM Tom Lane wrote: > Nikita Malakhov writes: > > We already had some thoughts on storing, let's call them "optional" > > attributes into 'attoptions' instead of extending the PG_ATTRIBUTE > > table, and here came feedback from Andres Freund with a remark that > > we're increasing the largest catalog table. So we decided to propose > > moving these "optional" attributes from being the PG_ATTRIBUTE column to > > be the part of 'attoptions' column of this table. > > This smells very much like what was done eons ago to create the > pg_attrdef catalog. I don't have any concrete comments to make, > only to suggest that that's an instructive parallel case. One > thing that comes to mind immediately is whether this stuff could > be unified with pg_attrdef instead of creating Yet Another catalog > that has to be consulted on the way to getting any real work done. > > I think that pg_attrdef was originally separated to keep large > default expressions from overrunning the maximum tuple size, > a motivation that disappeared once we could TOAST system tables. > However, nowadays it's still useful for it to be separate because > it simplifies representation of dependencies of default expressions > (pg_depend refers to OIDs of pg_attrdef entries for that). > If we're thinking of moving anything that would need dependency > management then it might need its own catalog, maybe? > > On the whole I'm not convinced that what you suggest will be a > net win. pg_attrdef wins to the extent that there are a lot of > columns with no non-null default and hence no need for any pg_attrdef > entry. But the minute you move something that most tables need, like > attcompression, you'll just have another bloated catalog to deal with. > > > Also, we suggest that options stored in 'attoptions' column could be > packed > > as JSON values. > > Please, no. Use of JSON in a SQL database pretty much always > represents a failure to think hard enough about what you need > to store. Sometimes it's not worth thinking all that hard; > but I strenuously oppose applying that sort of standard in > the system catalogs. > > regards, tom lane > -- Regards, Nikita Malakhov https://postgrespro.ru/
pg15b3: crash in paralell vacuum
Immediately after upgrading an internal instance, a loop around "vacuum" did
this:
TRAP: FailedAssertion("indstats->status == PARALLEL_INDVAC_STATUS_INITIAL",
File: "vacuumparallel.c", Line: 611, PID: 27635)
postgres: postgres pryzbyj [local] VACUUM(ExceptionalCondition+0x8d)[0x99d9fd]
postgres: postgres pryzbyj [local] VACUUM[0x6915db]
postgres: postgres pryzbyj [local] VACUUM(heap_vacuum_rel+0x12b6)[0x5083e6]
postgres: postgres pryzbyj [local] VACUUM[0x68e97a]
postgres: postgres pryzbyj [local] VACUUM(vacuum+0x48e)[0x68fe9e]
postgres: postgres pryzbyj [local] VACUUM(ExecVacuum+0x2ae)[0x69065e]
postgres: postgres pryzbyj [local]
VACUUM(standard_ProcessUtility+0x530)[0x8567b0]
/usr/pgsql-15/lib/pg_stat_statements.so(+0x5450)[0x7f52b891c450]
postgres: postgres pryzbyj [local] VACUUM[0x85490a]
postgres: postgres pryzbyj [local] VACUUM[0x854a53]
postgres: postgres pryzbyj [local] VACUUM(PortalRun+0x179)[0x855029]
postgres: postgres pryzbyj [local] VACUUM[0x85099b]
postgres: postgres pryzbyj [local] VACUUM(PostgresMain+0x199a)[0x85268a]
postgres: postgres pryzbyj [local] VACUUM[0x496a21]
postgres: postgres pryzbyj [local] VACUUM(PostmasterMain+0x11c0)[0x7b3980]
postgres: postgres pryzbyj [local] VACUUM(main+0x1c6)[0x4986a6]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7f52c4b893d5]
postgres: postgres pryzbyj [local] VACUUM[0x498c59]
< 2022-08-18 07:56:51.963 CDT >LOG: server process (PID 27635) was terminated
by signal 6: Aborted
< 2022-08-18 07:56:51.963 CDT >DETAIL: Failed process was running: VACUUM
ANALYZE alarms
Unfortunately, it looks like the RPM packages are compiled with -O2, so this is
of limited use.
Core was generated by `postgres: postgres pryzbyj [local] VACUUM '.
Program terminated with signal 6, Aborted.
#0 0x7f52c4b9d207 in raise () from /lib64/libc.so.6
Missing separate debuginfos, use: debuginfo-install
audit-libs-2.8.4-4.el7.x86_64 bzip2-libs-1.0.6-13.el7.x86_64
cyrus-sasl-lib-2.1.26-23.el7.x86_64 elfutils-libelf-0.176-5.el7.x86_64
elfutils-libs-0.176-5.el7.x86_64 glibc-2.17-260.el7_6.3.x86_64
keyutils-libs-1.5.8-3.el7.x86_64 krb5-libs-1.15.1-51.el7_9.x86_64
libattr-2.4.46-13.el7.x86_64 libcap-2.22-9.el7.x86_64
libcap-ng-0.7.5-4.el7.x86_64 libcom_err-1.42.9-19.el7.x86_64
libgcc-4.8.5-39.el7.x86_64 libgcrypt-1.5.3-14.el7.x86_64
libgpg-error-1.12-3.el7.x86_64 libicu-50.1.2-17.el7.x86_64
libselinux-2.5-15.el7.x86_64 libstdc++-4.8.5-39.el7.x86_64
libxml2-2.9.1-6.el7_9.6.x86_64 libzstd-1.5.2-1.el7.x86_64
lz4-1.7.5-2.el7.x86_64 nspr-4.19.0-1.el7_5.x86_64 nss-3.36.0-7.1.el7_6.x86_64
nss-softokn-freebl-3.36.0-5.el7_5.x86_64 nss-util-3.36.0-1.1.el7_6.x86_64
openldap-2.4.44-21.el7_6.x86_64 openssl-libs-1.0.2k-22.el7_9.x86_64
pam-1.1.8-22.el7.x86_64 pcre-8.32-17.el7.x86_64
systemd-libs-219-62.el7_6.5.x86_64 xz-libs-5.2.2-1.el7.x86_64
zlib-1.2.7-18.el7.x86_64
(gdb) bt
#0 0x7f52c4b9d207 in raise () from /lib64/libc.so.6
#1 0x7f52c4b9e8f8 in abort () from /lib64/libc.so.6
#2 0x0099da1e in ExceptionalCondition
(conditionName=conditionName@entry=0xafae40 "indstats->status ==
PARALLEL_INDVAC_STATUS_INITIAL", errorType=errorType@entry=0x9fb4b7
"FailedAssertion",
fileName=fileName@entry=0xafb0c0 "vacuumparallel.c",
lineNumber=lineNumber@entry=611) at assert.c:69
#3 0x006915db in parallel_vacuum_process_all_indexes (pvs=0x2e85f80,
num_index_scans=, vacuum=) at vacuumparallel.c:611
#4 0x005083e6 in heap_vacuum_rel (rel=,
params=, bstrategy=) at vacuumlazy.c:2679
#5 0x0068e97a in table_relation_vacuum (bstrategy=,
params=0x7fff46de9a80, rel=0x7f52c7bc2c10) at
../../../src/include/access/tableam.h:1680
#6 vacuum_rel (relid=52187497, relation=,
params=0x7fff46de9a80) at vacuum.c:2092
#7 0x0068fe9e in vacuum (relations=0x2dbeee8,
params=params@entry=0x7fff46de9a80, bstrategy=,
bstrategy@entry=0x0, isTopLevel=isTopLevel@entry=true) at vacuum.c:475
#8 0x0069065e in ExecVacuum (pstate=pstate@entry=0x2dc38d0,
vacstmt=vacstmt@entry=0x2d9f3a0, isTopLevel=isTopLevel@entry=true) at
vacuum.c:275
#9 0x008567b0 in standard_ProcessUtility (pstmt=pstmt@entry=0x2d9f7a0,
queryString=queryString@entry=0x2d9e8a0 "VACUUM ANALYZE alarms",
readOnlyTree=, context=context@entry=PROCESS_UTILITY_TOPLEVEL,
params=params@entry=0x0, queryEnv=queryEnv@entry=0x0,
dest=dest@entry=0x2d9f890, qc=qc@entry=0x7fff46dea0c0) at utility.c:866
#10 0x7f52b891c450 in pgss_ProcessUtility (pstmt=0x2d9f7a0,
queryString=0x2d9e8a0 "VACUUM ANALYZE alarms", readOnlyTree=,
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x2d9f890,
qc=0x7fff46dea0c0) at pg_stat_statements.c:1143
#11 0x0085490a in PortalRunUtility (portal=portal@entry=0x2e20fc0,
pstmt=0x2d9f7a0, isTopLevel=isTopLevel@entry=true,
setHoldSnapshot=setHoldSnapshot@entry=false, dest=0x2d9f890, qc=0x7fff46dea0c0)
at pquery.c:1158
#12 0x00854a53 in PortalRunMulti (portal=portal@entry=0x2e20fc0,
Re: Perform streaming logical transactions by background workers and parallel apply
On Wed, Aug 17, 2022 at 11:58 AM wangw.f...@fujitsu.com
wrote:
>
> Attach the new patches.
>
Few comments on v23-0001
===
1.
+ /*
+ * Attach to the dynamic shared memory segment for the parallel query, and
+ * find its table of contents.
+ *
+ * Note: at this point, we have not created any ResourceOwner in this
+ * process. This will result in our DSM mapping surviving until process
+ * exit, which is fine. If there were a ResourceOwner, it would acquire
+ * ownership of the mapping, but we have no need for that.
+ */
In the first sentence, instead of a parallel query, you need to use
parallel apply. I think we don't need to repeat the entire note as we
have in ParallelWorkerMain. You can say something like: "Like parallel
query, we don't need resource owner by this time. See
ParallelWorkerMain"
2.
+/*
+ * There are three fields in message: start_lsn, end_lsn and send_time. Because
+ * we have updated these statistics in apply worker, we could ignore these
+ * fields in apply background worker. (see function LogicalRepApplyLoop).
+ */
+#define SIZE_STATS_MESSAGE (2*sizeof(XLogRecPtr)+sizeof(TimestampTz))
The first sentence in the above comment isn't clear about which
message it talks about. I think it is about any message received by
this apply bgworker, if so, can we change it to: "There are three
fields in each message received by apply worker: start_lsn, end_lsn,
and send_time."
3.
+/*
+ * Return the apply background worker that will be used for the specified xid.
+ *
+ * If an apply background worker is found in the free list then re-use it,
+ * otherwise start a fresh one. Cache the worker ApplyBgworkersHash keyed by
+ * the specified xid.
+ */
+ApplyBgworkerInfo *
+apply_bgworker_start(TransactionId xid)
The first sentence should say apply background worker info. Can we
change the cache-related sentence in the above comment to "Cache the
worker info in ApplyBgworkersHash keyed by the specified xid."?
4.
/*
+ * We use first byte of message for additional communication between
+ * main Logical replication worker and apply background workers, so if
+ * it differs from 'w', then process it first.
+ */
+ c = pq_getmsgbyte();
+ switch (c)
+ {
+ /* End message of streaming chunk */
+ case LOGICAL_REP_MSG_STREAM_STOP:
+ elog(DEBUG1, "[Apply BGW #%u] ended processing streaming chunk, "
+ "waiting on shm_mq_receive", shared->worker_id);
+
Why do we need special handling of LOGICAL_REP_MSG_STREAM_STOP message
here? Instead, why not let it get handled via apply_dispatch path? You
will require special handling for apply_bg_worker but I see other
messages do have similar handling.
5.
+ /*
+ * Now, we have initialized DSM. Attach to slot.
+ */
+ logicalrep_worker_attach(worker_slot);
Can we change this comment to something like: "Primary initialization
is complete. Now, we can attach to our slot.". IIRC, we have done it
after initialization to avoid some race conditions among leader apply
worker and this parallel apply worker. If so, can we explain the same
in the comments?
6.
+/*
+ * Set up a dynamic shared memory segment.
+ *
+ * We set up a control region that contains a ApplyBgworkerShared,
+ * plus one region per message queue. There are as many message queues as
+ * the number of workers.
+ */
+static bool
+apply_bgworker_setup_dsm(ApplyBgworkerInfo *wstate)
I think the part of the comment: "There are as many message queues as
the number of workers." doesn't seem to fit atop this function as this
has nothing to do with the number of workers. It would be a good idea
to write something about what all is communicated via DSM in the
description you have written about apply bg workers in worker.c.
7.
+ /* Check if there are free worker slot(s). */
+ LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
+ napplyworkers = logicalrep_apply_bgworker_count(MyLogicalRepWorker->subid);
+ LWLockRelease(LogicalRepWorkerLock);
+
+ if (napplyworkers >= max_apply_bgworkers_per_subscription)
+ return NULL;
Won't it be better to check this restriction in
logicalrep_worker_launch() as we do for tablesync workers? That way
all similar restrictions will be in one place.
8.
+ if (rel->state != SUBREL_STATE_READY)
+ ereport(ERROR,
+ (errmsg("logical replication apply workers for subscription \"%s\"
will restart",
+ MySubscription->name),
+ errdetail("Cannot handle streamed replication transaction by apply "
+"background workers until all tables are synchronized")));
errdetail messages always end with a full stop.
--
With Regards,
Amit Kapila.
Data caching
Hello Is there a postgres extension or project related to application-level/foreign-table data caching ? The postgres_fdw extension fetches data from foreign table for each command. I have seen previous messages in archive about caching in form of global temp tables, query cache etc. There are good discussions about whether support should be built-in but did not find any implementation. I have seen the 44 postgres extensions that come pre-installed with ubuntu 16.04 but none of them do this. Thanks. Anant.
Re: Assertion failure on PG15 with modified test_shm_mq test
Hi, On Thu, Aug 18, 2022 at 5:38 AM Andres Freund wrote: > I don't think we have the infrastructure for a nice solution to this at the > moment - we need a fairly large overhaul of process initialization / > shutdown > to handle these interdependencies nicely. > > Ok, understood. > We can't move pgstat shutdown into on_dsm callback because that's too late > to > allocate *new* dsm segments, which we might need to do while flushing > out pending stats. > > See > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fa91d4c91f28f4819dc54f93adbd413a685e366a > for a way to avoid the problem. > > Thanks for the hint. I will try that approach. I wonder though if there is something more we can do. For example, would it make sense to throw a WARNING and avoid segfault if pgstat machinery is already shutdown? Just worried if the code can be reached from multiple paths and testing all of those would be difficult for extension developers, especially given this may happen in error recovery path. Thanks, Pavan -- Pavan Deolasee EnterpriseDB: https://www.enterprisedb..com
Re: TAP output format in pg_regress
> On 18 Aug 2022, at 00:49, Andres Freund wrote: >> while still be TAP compliant enough that running it with prove (with a tiny >> Perl wrapper) works. > > Wonder if we could utilize that for making failures of 002_pg_upgrade.pl and > 027_stream_regress.pl easier to understand, by reporting pg_regress' steps as > part of the outer test. But that'd probably be at least somewhat hard, due to > the embedded test count etc. I have a feeling it might require some quite bespoke logic to tie it together (which may not be worth it in the end) but I'll have a look. >> A normal "make check" with this patch applied now looks like this: >> >> ... > > I'm happy with that compared to our current output. Great! Once this thread has settled on a format, maybe it should go to a separate -hackers thread to get more visibility and (hopefully) buy-in for such a change. One thing I haven't researched yet is if the Buildfarm or CFBot parses the current output in any way or just check the exit status of the testrun. >> The ending comment isn't currently shown when executed via prove as it has to >> go to STDERR for that to work, and I'm not sure that's something we want to >> do >> in the general case. I don't expect running the pg_regress tests via prove >> is >> going to be the common case. How does the meson TAP ingestion handle >> stdout/stderr? > > It'll parse stdout for tap output and log stderr output separately. Then I think this patch will be compatible with that. >> There is a slight reduction in information, for example around tests run >> serially vs in a parallel group. This could perhaps be handled by marking >> the >> test name in some way like "tablespace (serial) or prefixing with a symbol or >> somerthing. I can take a stab at that in case we think that level of detail >> is >> important to preserve. > > I think we could just indent the test "description" for tests in parallel > groups: > > I.e. instead of > > ok 6 - varchar 68 ms > ... > ok 6 - varchar 68 ms > > that'd make it sufficiently clear, and is a bit more similar to the current > format? Thats a better option, done that way. > I wonder if we should indent the test name so that the number doesn't cause > wrapping? The tricky part there is that we don't know beforehands how many tests will be run. We could add a first pass over the schedule, which seems excessive for this, or align with a fixed max such that number of tests is the maximum number of tests which will print neatly aligned. > And perhaps we can skip the - in that case? The dash is listed as "Recommended" but not required in the TAP spec (including up to v14) so we can skip it. With these changes, the "worst case" output in terms of testnames alignment would be something like this (assuming at most tests): ok 211 stats 852 ms # parallel group (2 tests): event_trigger oidjoins ok 212 event_trigger 149 ms not ok 213 oidjoins 194 ms ok 214 fast_default 178 ms 1..214 I think that's fairly readable, and not that much different from today. -- Daniel Gustafsson https://vmware.com/ v7-0001-Make-pg_regress-output-format-TAP-compliant.patch Description: Binary data
Re: Perform streaming logical transactions by background workers and parallel apply
On Thu, Aug 18, 2022 at 3:40 PM Peter Smith wrote: > > On Thu, Aug 18, 2022 at 6:57 PM Amit Kapila wrote: > > > > > 47. src/include/replication/logicalproto.h > > > > > > @@ -32,12 +32,17 @@ > > > * > > > * LOGICALREP_PROTO_TWOPHASE_VERSION_NUM is the minimum protocol version > > > with > > > * support for two-phase commit decoding (at prepare time). Introduced > > > in PG15. > > > + * > > > + * LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM is the minimum protocol > > > version > > > + * with support for streaming large transactions using apply background > > > + * workers. Introduced in PG16. > > > */ > > > #define LOGICALREP_PROTO_MIN_VERSION_NUM 1 > > > #define LOGICALREP_PROTO_VERSION_NUM 1 > > > #define LOGICALREP_PROTO_STREAM_VERSION_NUM 2 > > > #define LOGICALREP_PROTO_TWOPHASE_VERSION_NUM 3 > > > -#define LOGICALREP_PROTO_MAX_VERSION_NUM > > > LOGICALREP_PROTO_TWOPHASE_VERSION_NUM > > > +#define LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM 4 > > > +#define LOGICALREP_PROTO_MAX_VERSION_NUM > > > LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM > > > > > > 47a. > > > I don't think that comment is strictly true. IIUC the new protocol > > > version 4 is currently only affecting the *extra* STREAM_ABORT members > > > – but in fact streaming=parallel is still functional without using > > > those extra members, isn't it? So maybe this description needed to be > > > modified a bit to be more accurate? > > > > > > > The reason for sending this extra abort members is to ensure that > > after aborting the transaction, if the subscriber/apply worker > > restarts, it doesn't need to request the transaction again. Do you > > have suggestions for improving this comment? > > > > I gave three review comments for v21-0001 that were all related to > this same point: > i- #4b (commit message) > ii- #7 (protocol pgdocs) > iii- #47a (code comment) > > The point was: > AFAIK protocol 4 is only to let the parallel streaming logic behave > *better* in how it can handle restarts after aborts. But that does not > mean that protocol 4 is a *pre-requisite* for "allowing" > streaming=parallel to work in the first place. I thought that a PG15 > publisher and PG16 subscriber can still work using streaming=parallel > even with protocol 3, but it just won't be quite as clever for > handling restarts after abort as protocol 4 (PG16 -> PG16) would be. > It is not only that it makes it better but one can say that it is a break of a replication protocol that after the client (subscriber) has applied some transaction, it requests the same transaction again. So, I think it is better to make the parallelism work only when the server version is also 16. -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Thu, Aug 18, 2022 at 6:57 PM Amit Kapila wrote:
>
> On Thu, Aug 18, 2022 at 11:59 AM Peter Smith wrote:
> >
> > Here are my review comments for patch v21-0001:
> >
> > 4. Commit message
> >
> > In addition, the patch extends the logical replication STREAM_ABORT message
> > so
> > that abort_time and abort_lsn can also be sent which can be used to update
> > the
> > replication origin in apply background worker when the streaming
> > transaction is
> > aborted.
> >
> > 4a.
> > Should this para also mention something about the introduction of
> > protocol version 4?
> >
> > 4b.
> > Should this para also mention that these extensions are not strictly
> > mandatory for the parallel streaming to still work?
> >
>
> Without parallel streaming/apply, we don't need to send this extra
> message. So, I don't think it will be correct to say that.
See my reply to 47a below.
>
> >
> > 46. src/backend/replication/logical/worker.c - apply_error_callback
> >
> > + if (errarg->remote_attnum < 0)
> > + {
> > + if (XLogRecPtrIsInvalid(errarg->finish_lsn))
> > + errcontext("processing remote data for replication origin \"%s\"
> > during \"%s\" for replication target relation \"%s.%s\" in transaction
> > %u",
> > +errarg->origin_name,
> > +logicalrep_message_type(errarg->command),
> > +errarg->rel->remoterel.nspname,
> > +errarg->rel->remoterel.relname,
> > +errarg->remote_xid);
> > + else
> > + errcontext("processing remote data for replication origin \"%s\"
> > during \"%s\" for replication target relation \"%s.%s\" in transaction
> > %u finished at %X/%X",
> > +errarg->origin_name,
> > +logicalrep_message_type(errarg->command),
> > +errarg->rel->remoterel.nspname,
> > +errarg->rel->remoterel.relname,
> > +errarg->remote_xid,
> > +LSN_FORMAT_ARGS(errarg->finish_lsn));
> > + }
> > + else
> > + {
> > + if (XLogRecPtrIsInvalid(errarg->finish_lsn))
> > + errcontext("processing remote data for replication origin \"%s\"
> > during \"%s\" for replication target relation \"%s.%s\" column \"%s\"
> > in transaction %u",
> > +errarg->origin_name,
> > +logicalrep_message_type(errarg->command),
> > +errarg->rel->remoterel.nspname,
> > +errarg->rel->remoterel.relname,
> > +errarg->rel->remoterel.attnames[errarg->remote_attnum],
> > +errarg->remote_xid);
> > + else
> > + errcontext("processing remote data for replication origin \"%s\"
> > during \"%s\" for replication target relation \"%s.%s\" column \"%s\"
> > in transaction %u finished at %X/%X",
> > +errarg->origin_name,
> > +logicalrep_message_type(errarg->command),
> > +errarg->rel->remoterel.nspname,
> > +errarg->rel->remoterel.relname,
> > +errarg->rel->remoterel.attnames[errarg->remote_attnum],
> > +errarg->remote_xid,
> > +LSN_FORMAT_ARGS(errarg->finish_lsn));
> > + }
> > + }
> >
> > Hou-san had asked [3](comment #14) me how the above code can be
> > shortened. Below is one idea, but maybe you won't like it ;-)
> >
> > #define MSG_O_T_S_R "processing remote data for replication origin
> > \"%s\" during \"%s\" for replication target relation \"%s.%s\" "
> > #define O_T_S_R\
> > errarg->origin_name,\
> > logicalrep_message_type(errarg->command),\
> > errarg->rel->remoterel.nspname,\
> > errarg->rel->remoterel.relname
> >
> > if (errarg->remote_attnum < 0)
> > {
> > if (XLogRecPtrIsInvalid(errarg->finish_lsn))
> > errcontext(MSG_O_T_S_R "in transaction %u",
> >O_T_S_R,
> >errarg->remote_xid);
> > else
> > errcontext(MSG_O_T_S_R "in transaction %u finished at %X/%X",
> >O_T_S_R,
> >errarg->remote_xid,
> >LSN_FORMAT_ARGS(errarg->finish_lsn));
> > }
> > else
> > {
> > if (XLogRecPtrIsInvalid(errarg->finish_lsn))
> > errcontext(MSG_O_T_S_R "column \"%s\" in transaction %u",
> >O_T_S_R,
> >errarg->rel->remoterel.attnames[errarg->remote_attnum],
> >errarg->remote_xid);
> > else
> > errcontext(MSG_O_T_S_R "column \"%s\" in transaction %u finished at %X/%X",
> >O_T_S_R,
> >errarg->rel->remoterel.attnames[errarg->remote_attnum],
> >errarg->remote_xid,
> >LSN_FORMAT_ARGS(errarg->finish_lsn));
> > }
> > #undef O_T_S_R
> > #undef MSG_O_T_S_R
> >
> > ==
> >
>
> I don't like this much. I think this reduces readability.
I agree. That wasn't a very serious suggestion :-)
>
> > 47. src/include/replication/logicalproto.h
> >
> > @@ -32,12 +32,17 @@
> > *
> > * LOGICALREP_PROTO_TWOPHASE_VERSION_NUM is the minimum protocol version
> > with
> > * support for two-phase commit decoding (at prepare time). Introduced in
> > PG15.
> > + *
> > + * LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM is the minimum protocol
> > version
> > + * with support for streaming large transactions using apply background
> > + * workers. Introduced in PG16.
> > */
> > #define LOGICALREP_PROTO_MIN_VERSION_NUM 1
> > #define LOGICALREP_PROTO_VERSION_NUM 1
> > #define LOGICALREP_PROTO_STREAM_VERSION_NUM 2
> > #define LOGICALREP_PROTO_TWOPHASE_VERSION_NUM 3
> >
Re: Add support for DEFAULT specification in COPY FROM
Andrew Dunstan writes: > On 2022-08-16 Tu 14:12, Israel Barth Rubio wrote: >> Hello all, >> >> With the current implementation of COPY FROM in PostgreSQL we are >> able to load the DEFAULT value/expression of a column if the column >> is absent in the list of specified columns. We are not able to >> explicitly ask that PostgreSQL uses the DEFAULT value/expression in a >> column that is being fetched from the input file, though. >> >> This patch adds support for handling DEFAULT values in COPY FROM. It >> works similarly to NULL in COPY FROM: whenever the marker that was >> set for DEFAULT value/expression is read from the input stream, it >> will evaluate the DEFAULT value/expression of the corresponding >> column. […] > Interesting, and probably useful. I've only had a brief look, but it's > important that the default marker not be quoted in CSV mode (c.f. NULL) > -f it is it should be taken as a literal rather than a special value. For the NULL marker that can be overridden for individual columns with the FORCE(_NOT)_NULL option. This feature should have a similar FORCE(_NOT)_DEFAULT option to allow the DEFAULT marker to be ignored, or recognised even when quoted, respectively. - ilmari
Re: hash_xlog_split_allocate_page: failed to acquire cleanup lock
On Wed, Aug 17, 2022 at 5:55 PM Robert Haas wrote: > > Regarding the question of whether we need a cleanup lock on the new > bucket I am not really seeing the advantage of going down that path. > Simply fixing this code to take a cleanup lock instead of hoping that > it always gets one by accident is low risk and should fix the observed > problem. Getting rid of the cleanup lock will be more invasive and I'd > like to see some evidence that it's a necessary step before we take > the risk of breaking things. > The patch proposed by you is sufficient to fix the observed issue. BTW, we are able to reproduce the issue and your patch fixed it. The idea is to ensure that checkpointer only tries to sync the buffer for the new bucket, otherwise, it will block while acquiring the lock on the old bucket buffer in SyncOneBuffer because the replay process would already have it and we won't be able to hit required condition. To simulate it, we need to stop the replay before we acquire the lock for the old bucket. Then, let checkpointer advance the buf_id beyond the buffer which we will get for the old bucket (in the place where it loops over all buffers, and mark the ones that need to be written with BM_CHECKPOINT_NEEDED.). After that let the replay process proceed till the point where it checks for the clean-up lock on the new bucket. Next, let the checkpointer advance to sync the buffer corresponding to the new bucket buffer. This will reproduce the required condition. We have tried many other combinations but couldn't able to hit it. For example, we were not able to generate it via bgwriter because it expects the buffer to have zero usage and ref count which is not possible during the replay in hash_xlog_split_allocate_page() as we already have increased the usage count for the new bucket buffer before checking the cleanup lock on it. I agree with you that getting rid of the clean-up lock on the new bucket is a more invasive patch and should be done separately if required. Yesterday, I have done a brief analysis and I think that is possible but it doesn't seem to be a good idea to backpatch it. -- With Regards, Amit Kapila.
Re: cataloguing NOT NULL constraints
On 2022-Aug-18, Laurenz Albe wrote: > On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote: > > 1. In my implementation, you can have more than one NOT NULL > > pg_constraint row for a column. What should happen if the user does > > ALTER TABLE .. ALTER COLUMN .. DROP NOT NULL; > > ? Currently it throws an error about the ambiguity (ie. which > > constraint to drop). > > I'd say that is a good solution, particularly if there is a hint to drop > the constraint instead, similar to when you try to drop an index that > implements a constraint. Ah, I didn't think about the hint. I'll add that, thanks. > > Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull' > > bit is lost when the last one such constraint goes away. > > Wouldn't it be the correct solution to set "attnotnumm" to FALSE only > when the last NOT NULL constraint is dropped? ... when the last NOT NULL or PRIMARY KEY constraint is dropped. We have to keep attnotnull set when a PK exists even if there's no specific NOT NULL constraint. > > 2. If a table has a primary key, and a table is created that inherits > > from it, then the child has its column(s) marked attnotnull but there > > is no pg_constraint row for that. This is not okay. But what should > > happen? > > > > 1. a CHECK(col IS NOT NULL) constraint is created for each column > > 2. a PRIMARY KEY () constraint is created > > I think it would be best to create a primary key constraint on the > partition. Sorry, I wasn't specific enough. This applies to legacy inheritance only; partitioning has its own solution (as you say: the PK constraint exists), but legacy inheritance works differently. Creating a PK in children tables is not feasible (because unicity cannot be maintained), but creating a CHECK (NOT NULL) constraint is possible. I think a PRIMARY KEY should not be allowed to exist in an inheritance parent, precisely because of this problem, but it seems too late to add that restriction now. This behavior is absurd, but longstanding: 55432 16devel 1787364=# create table parent (a int primary key); CREATE TABLE 55432 16devel 1787364=# create table child () inherits (parent); CREATE TABLE 55432 16devel 1787364=# insert into parent values (1); INSERT 0 1 55432 16devel 1787364=# insert into child values (1); INSERT 0 1 55432 16devel 1787364=# select * from parent; a ─── 1 1 (2 filas) -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "But static content is just dynamic content that isn't moving!" http://smylers.hates-software.com/2007/08/15/fe244d0c.html
Re: Perform streaming logical transactions by background workers and parallel apply
On Thu, Aug 18, 2022 at 11:59 AM Peter Smith wrote:
>
> Here are my review comments for patch v21-0001:
>
> 4. Commit message
>
> In addition, the patch extends the logical replication STREAM_ABORT message so
> that abort_time and abort_lsn can also be sent which can be used to update the
> replication origin in apply background worker when the streaming transaction
> is
> aborted.
>
> 4a.
> Should this para also mention something about the introduction of
> protocol version 4?
>
> 4b.
> Should this para also mention that these extensions are not strictly
> mandatory for the parallel streaming to still work?
>
Without parallel streaming/apply, we don't need to send this extra
message. So, I don't think it will be correct to say that.
>
> 46. src/backend/replication/logical/worker.c - apply_error_callback
>
> + if (errarg->remote_attnum < 0)
> + {
> + if (XLogRecPtrIsInvalid(errarg->finish_lsn))
> + errcontext("processing remote data for replication origin \"%s\"
> during \"%s\" for replication target relation \"%s.%s\" in transaction
> %u",
> +errarg->origin_name,
> +logicalrep_message_type(errarg->command),
> +errarg->rel->remoterel.nspname,
> +errarg->rel->remoterel.relname,
> +errarg->remote_xid);
> + else
> + errcontext("processing remote data for replication origin \"%s\"
> during \"%s\" for replication target relation \"%s.%s\" in transaction
> %u finished at %X/%X",
> +errarg->origin_name,
> +logicalrep_message_type(errarg->command),
> +errarg->rel->remoterel.nspname,
> +errarg->rel->remoterel.relname,
> +errarg->remote_xid,
> +LSN_FORMAT_ARGS(errarg->finish_lsn));
> + }
> + else
> + {
> + if (XLogRecPtrIsInvalid(errarg->finish_lsn))
> + errcontext("processing remote data for replication origin \"%s\"
> during \"%s\" for replication target relation \"%s.%s\" column \"%s\"
> in transaction %u",
> +errarg->origin_name,
> +logicalrep_message_type(errarg->command),
> +errarg->rel->remoterel.nspname,
> +errarg->rel->remoterel.relname,
> +errarg->rel->remoterel.attnames[errarg->remote_attnum],
> +errarg->remote_xid);
> + else
> + errcontext("processing remote data for replication origin \"%s\"
> during \"%s\" for replication target relation \"%s.%s\" column \"%s\"
> in transaction %u finished at %X/%X",
> +errarg->origin_name,
> +logicalrep_message_type(errarg->command),
> +errarg->rel->remoterel.nspname,
> +errarg->rel->remoterel.relname,
> +errarg->rel->remoterel.attnames[errarg->remote_attnum],
> +errarg->remote_xid,
> +LSN_FORMAT_ARGS(errarg->finish_lsn));
> + }
> + }
>
> Hou-san had asked [3](comment #14) me how the above code can be
> shortened. Below is one idea, but maybe you won't like it ;-)
>
> #define MSG_O_T_S_R "processing remote data for replication origin
> \"%s\" during \"%s\" for replication target relation \"%s.%s\" "
> #define O_T_S_R\
> errarg->origin_name,\
> logicalrep_message_type(errarg->command),\
> errarg->rel->remoterel.nspname,\
> errarg->rel->remoterel.relname
>
> if (errarg->remote_attnum < 0)
> {
> if (XLogRecPtrIsInvalid(errarg->finish_lsn))
> errcontext(MSG_O_T_S_R "in transaction %u",
>O_T_S_R,
>errarg->remote_xid);
> else
> errcontext(MSG_O_T_S_R "in transaction %u finished at %X/%X",
>O_T_S_R,
>errarg->remote_xid,
>LSN_FORMAT_ARGS(errarg->finish_lsn));
> }
> else
> {
> if (XLogRecPtrIsInvalid(errarg->finish_lsn))
> errcontext(MSG_O_T_S_R "column \"%s\" in transaction %u",
>O_T_S_R,
>errarg->rel->remoterel.attnames[errarg->remote_attnum],
>errarg->remote_xid);
> else
> errcontext(MSG_O_T_S_R "column \"%s\" in transaction %u finished at %X/%X",
>O_T_S_R,
>errarg->rel->remoterel.attnames[errarg->remote_attnum],
>errarg->remote_xid,
>LSN_FORMAT_ARGS(errarg->finish_lsn));
> }
> #undef O_T_S_R
> #undef MSG_O_T_S_R
>
> ==
>
I don't like this much. I think this reduces readability.
> 47. src/include/replication/logicalproto.h
>
> @@ -32,12 +32,17 @@
> *
> * LOGICALREP_PROTO_TWOPHASE_VERSION_NUM is the minimum protocol version with
> * support for two-phase commit decoding (at prepare time). Introduced in
> PG15.
> + *
> + * LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM is the minimum protocol
> version
> + * with support for streaming large transactions using apply background
> + * workers. Introduced in PG16.
> */
> #define LOGICALREP_PROTO_MIN_VERSION_NUM 1
> #define LOGICALREP_PROTO_VERSION_NUM 1
> #define LOGICALREP_PROTO_STREAM_VERSION_NUM 2
> #define LOGICALREP_PROTO_TWOPHASE_VERSION_NUM 3
> -#define LOGICALREP_PROTO_MAX_VERSION_NUM
> LOGICALREP_PROTO_TWOPHASE_VERSION_NUM
> +#define LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM 4
> +#define LOGICALREP_PROTO_MAX_VERSION_NUM
> LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM
>
> 47a.
> I don't think that comment is strictly true. IIUC the new protocol
> version 4 is currently only affecting the *extra* STREAM_ABORT members
> – but in fact streaming=parallel is
Re: shadow variables - pg15 edition
On Thu, Aug 18, 2022 at 5:27 PM David Rowley wrote: > > On Thu, 18 Aug 2022 at 17:16, Justin Pryzby wrote: > > > > On Thu, Aug 18, 2022 at 03:17:33PM +1200, David Rowley wrote: > > > I'm probably not the only committer to want to run a mile when they > > > see someone posting 17 or 26 patches in an email. So maybe "bang for > > > buck" is a better method for getting the ball rolling here. As you > > > know, I was recently bitten by local shadows in af7d270dd, so I do > > > believe in the cause. > > > > > > What do you think? > > > > You already fixed the shadow var introduced in master/pg16, and I sent > > patches > > for the shadow vars added in pg15 (marked as such and presented as > > 001-008), so > > perhaps it's okay to start with that ? > > Alright, I made a pass over the 0001-0008 patches. > ... > > 0005. How about just "tslot". I'm not a fan of "this". > (I'm sure there are others like this; I just picked this one as an example) AFAICT the offending 'slot' really should have never been declared at all at the local scope in the first place - e.g. the other code in this function seems happy enough with the pattern of just re-using the function scoped 'slot'. I understand that for this shadow patch changing the var-name is considered the saner/safer way than tampering with the scope, but perhaps it is still useful to include a comment when changing ones like this? e.g. + TupleTableSlot *tslot; /* TODO - Why declare this at all? Shouldn't it just re-use the 'slot' at function scope? */ Otherwise, such knowledge will be lost, and nobody will ever know to revisit them, which feels a bit more like *hiding* the mistake than fixing it. -- Kind Regards, Peter Smith. Fujitsu Australia
Re: cataloguing NOT NULL constraints
On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote: > I've been working on having NOT NULL constraints have pg_constraint > rows. > > Everything is working now. Some things are a bit weird, and I would > like opinions on them: > > 1. In my implementation, you can have more than one NOT NULL > pg_constraint row for a column. What should happen if the user does > ALTER TABLE .. ALTER COLUMN .. DROP NOT NULL; > ? Currently it throws an error about the ambiguity (ie. which > constraint to drop). I'd say that is a good solution, particularly if there is a hint to drop the constraint instead, similar to when you try to drop an index that implements a constraint. > Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull' > bit is lost when the last one such constraint goes away. Wouldn't it be the correct solution to set "attnotnumm" to FALSE only when the last NOT NULL constraint is dropped? > 2. If a table has a primary key, and a table is created that inherits > from it, then the child has its column(s) marked attnotnull but there > is no pg_constraint row for that. This is not okay. But what should > happen? > > 1. a CHECK(col IS NOT NULL) constraint is created for each column > 2. a PRIMARY KEY () constraint is created I think it would be best to create a primary key constraint on the partition. Yours, Laurenz Albe
Re: build remaining Flex files standalone
I wrote > [v4] This piece is a leftover from the last version, and forgot to remove it, will fix: diff --git a/contrib/cube/cubeparse.y b/contrib/cube/cubeparse.y index 7577c4515c..e3b750b695 100644 --- a/contrib/cube/cubeparse.y +++ b/contrib/cube/cubeparse.y @@ -7,6 +7,7 @@ #include "postgres.h" #include "cubedata.h" +#include "cube_internal.h" #include "utils/float.h" -- John Naylor EDB: http://www.enterprisedb.com
Re: build remaining Flex files standalone
> > > > > index dbe7d4f742..0b373048b5 100644 > > > > > --- a/contrib/cube/cubedata.h > > > > > +++ b/contrib/cube/cubedata.h > > > > > @@ -67,3 +67,7 @@ extern void cube_scanner_finish(void); > > > > > > > > > > /* in cubeparse.y */ > > > > > extern int cube_yyparse(NDBOX **result); > > > > > + > > > > > +/* All grammar constructs return strings */ > > > > > +#define YYSTYPE char * > > > > > > > > Why does this need to be defined in a semi-public header? If we do this > > > > in > > > > multiple files we'll end up with the danger of macro redefinition > > > > warnings. > > For v4, I #defined YYSTYPE Sorry for the misfire. Continuing on, I #defined YYSTYPE in cubescan.l before #including cubeparse.h. I also added scanbuflen to the %parse-param to prevent resorting to a global variable. The rest of the patches are unchanged. -- John Naylor EDB: http://www.enterprisedb.com From 2b95401d925bed67b2cb1eb9e8cdb1f1dd3bcc8e Mon Sep 17 00:00:00 2001 From: John Naylor Date: Tue, 16 Aug 2022 12:01:41 +0700 Subject: [PATCH v4 02/11] Move private declarations shared between guc.c and guc-file.l to new header FIXME: fails headerscheck --- src/backend/utils/misc/guc-file.l | 1 + src/backend/utils/misc/guc.c | 1 + src/include/utils/guc.h | 10 -- src/include/utils/guc_internal.h | 24 4 files changed, 26 insertions(+), 10 deletions(-) create mode 100644 src/include/utils/guc_internal.h diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l index b4fa09749b..843838b1df 100644 --- a/src/backend/utils/misc/guc-file.l +++ b/src/backend/utils/misc/guc-file.l @@ -18,6 +18,7 @@ #include "miscadmin.h" #include "storage/fd.h" #include "utils/guc.h" +#include "utils/guc_internal.h" /* diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 66ab3912a0..293834fc13 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -100,6 +100,7 @@ #include "utils/builtins.h" #include "utils/bytea.h" #include "utils/float.h" +#include "utils/guc_internal.h" #include "utils/guc_tables.h" #include "utils/memutils.h" #include "utils/pg_locale.h" diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index aae071cd82..45ae1b537f 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -442,16 +442,6 @@ extern void GUC_check_errcode(int sqlerrcode); pre_format_elog_string(errno, TEXTDOMAIN), \ GUC_check_errhint_string = format_elog_string -/* functions shared between guc.c and guc-file.l */ -extern int guc_name_compare(const char *namea, const char *nameb); -extern ConfigVariable *ProcessConfigFileInternal(GucContext context, - bool applySettings, int elevel); -extern void record_config_file_error(const char *errmsg, - const char *config_file, - int lineno, - ConfigVariable **head_p, - ConfigVariable **tail_p); - /* * The following functions are not in guc.c, but are declared here to avoid * having to include guc.h in some widely used headers that it really doesn't diff --git a/src/include/utils/guc_internal.h b/src/include/utils/guc_internal.h new file mode 100644 index 00..5d5db6bdce --- /dev/null +++ b/src/include/utils/guc_internal.h @@ -0,0 +1,24 @@ +/* + * guc_internals.h + * + * Declarations shared between backend/utils/misc/guc.c and + * backend/utils/misc/guc-file.l + * + * Copyright (c) 2000-2022, PostgreSQL Global Development Group + * + * src/include/utils/guc_internals.h + * + */ +#ifndef GUC_INTERNALS_H +#define GUC_INTERNALS_H + +extern int guc_name_compare(const char *namea, const char *nameb); +extern ConfigVariable *ProcessConfigFileInternal(GucContext context, + bool applySettings, int elevel); +extern void record_config_file_error(const char *errmsg, + const char *config_file, + int lineno, + ConfigVariable **head_p, + ConfigVariable **tail_p); + +#endif /* GUC_INTERNALS_H */ -- 2.36.1 From c066efea2193be8e7b21bb44c067383f34f37ec8 Mon Sep 17 00:00:00 2001 From: John Naylor Date: Tue, 16 Aug 2022 10:42:19 +0700 Subject: [PATCH v4 01/11] Preparatory refactoring for compiling guc-file.c standalone Mostly this involves moving ProcessConfigFileInternal() to guc.c and fixing the shared API to match. --- src/backend/utils/misc/guc-file.l | 360 +- src/backend/utils/misc/guc.c | 360 +- src/include/utils/guc.h | 9 + 3 files changed, 364 insertions(+), 365 deletions(-) diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l index ce5633844c..b4fa09749b 100644 --- a/src/backend/utils/misc/guc-file.l +++ b/src/backend/utils/misc/guc-file.l @@ -48,12 +48,6 @@ static sigjmp_buf *GUC_flex_fatal_jmp; static
Re: build remaining Flex files standalone
On Wed, Aug 17, 2022 at 8:14 AM Andres Freund wrote: > > Hi, > > On 2022-08-16 17:41:43 +0700, John Naylor wrote: > > For v3, I addressed some comments and added .h files to the > > headerscheck exceptions. > > Thanks! > > > > /* > > * NB: include bootparse.h only AFTER including bootstrap.h, because > > bootstrap.h > > * includes node definitions needed for YYSTYPE. > > */ > > > > Future cleanup: I see this in headerscheck: > > > > # We can't make these Bison output files compilable standalone > > # without using "%code require", which old Bison versions lack. > > # parser/gram.h will be included by parser/gramparse.h anyway. > > > > That directive has been supported in Bison since 2.4.2. > > 2.4.2 is from 2010. So I think we could just start relying on it? > > > > > > +/* functions shared between guc.c and guc-file.l */ > > > > [...] > > > I think I prefer your suggestion of a guc_internal.h upthread. > > > > Started in 0002, but left open the headerscheck failure. > > > > Also, if such a thing is meant to be #include'd only by two generated > > files, maybe it should just live in the directory where they live, and > > not in the src/include dir? > > It's not something we've done for the backend afaics, but I don't see a reason > not to start at some point. > > > > > > From 7d4ecfcb3e91f3b45e94b9e64c7c40f1bbd22aa8 Mon Sep 17 00:00:00 2001 > > > > From: John Naylor > > > > Date: Fri, 12 Aug 2022 15:45:24 +0700 > > > > Subject: [PATCH v201 2/9] Build booscanner.c standalone > > > > > > > -# bootscanner is compiled as part of bootparse > > > > -bootparse.o: bootscanner.c > > > > +# See notes in src/backend/parser/Makefile about the following two > > > > rules > > > > +bootparse.h: bootparse.c > > > > + touch $@ > > > > + > > > > +bootparse.c: BISONFLAGS += -d > > > > + > > > > +# Force these dependencies to be known even without dependency info > > > > built: > > > > +bootparse.o bootscan.o: bootparse.h > > > > > > Wonder if we could / should wrap this is something common. It's somewhat > > > annoying to repeat this stuff everywhere. > > > > I haven't looked at the Meson effort recently, but if the build rule > > is less annoying there, I'm inclined to leave this as a wart until > > autotools are retired. > > The only complicating thing in the rules there is the dependencies from one .c > file to another .c file. > > > > > > diff --git a/contrib/cube/cubedata.h b/contrib/cube/cubedata.h > > > > index dbe7d4f742..0b373048b5 100644 > > > > --- a/contrib/cube/cubedata.h > > > > +++ b/contrib/cube/cubedata.h > > > > @@ -67,3 +67,7 @@ extern void cube_scanner_finish(void); > > > > > > > > /* in cubeparse.y */ > > > > extern int cube_yyparse(NDBOX **result); > > > > + > > > > +/* All grammar constructs return strings */ > > > > +#define YYSTYPE char * > > > > > > Why does this need to be defined in a semi-public header? If we do this in > > > multiple files we'll end up with the danger of macro redefinition > > > warnings. For v4, I #defined YYSTYPE -- John Naylor EDB: http://www.enterprisedb.com
Re: POC: GROUP BY optimization
On 8/18/22 03:32, David Rowley wrote: > On Thu, 18 Aug 2022 at 02:46, Tomas Vondra > wrote: >> So I don't think the current costing is wrong, but it certainly is more >> complex. But the test does not test what it intended - I have two ideas >> how to make it work: >> >> 1) increase the number of rows in the table >> >> 2) increase cpu_operator_cost (for that one test?) >> >> 3) tweak the costing somehow, to increase the cost a bit > > Why not, 4) SET parallel_setup_cost = 0; there are plenty of other > places we do just that so we get a parallel plan without having to > generate enough cost to drown out the parallel worker startup cost. > > Here are a couple of patches to demo the idea. > Yeah, that's an option too. I should have mentioned it along with the cpu_operator_cost. BTW would you mind taking a look at the costing? I think it's fine, but it would be good if someone not involved in the patch takes a look. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: shadow variables - pg15 edition
On Thu, 18 Aug 2022 at 17:16, Justin Pryzby wrote:
>
> On Thu, Aug 18, 2022 at 03:17:33PM +1200, David Rowley wrote:
> > I'm probably not the only committer to want to run a mile when they
> > see someone posting 17 or 26 patches in an email. So maybe "bang for
> > buck" is a better method for getting the ball rolling here. As you
> > know, I was recently bitten by local shadows in af7d270dd, so I do
> > believe in the cause.
> >
> > What do you think?
>
> You already fixed the shadow var introduced in master/pg16, and I sent patches
> for the shadow vars added in pg15 (marked as such and presented as 001-008),
> so
> perhaps it's okay to start with that ?
Alright, I made a pass over the 0001-0008 patches.
0001. I'd also rather see these 4 renamed:
+++ b/src/bin/pg_dump/pg_dump.c
@@ -3144,7 +3144,6 @@ dumpDatabase(Archive *fout)
PQExpBuffer loHorizonQry = createPQExpBuffer();
int i_relfrozenxid,
i_relfilenode,
- i_oid,
i_relminmxid;
Adding an extra 'i' (for inner) on the front seems fine to me.
0002. I don't really like the "my" name. I also see you've added the
word "this" to many other variables that are shadowing. It feels kinda
like you're missing a "self" and a "me" in there somewhere! :)
@@ -7080,21 +7080,21 @@ getConstraints(Archive *fout, TableInfo
tblinfo[], int numTables)
appendPQExpBufferChar(tbloids, '{');
for (int i = 0; i < numTables; i++)
{
- TableInfo *tbinfo = [i];
+ TableInfo *mytbinfo = [i];
How about just "tinfo"?
0003. The following is used for the exact same purpose as its shadowed
counterpart. I suggest just using the variable from the outer scope.
@@ -16799,21 +16799,21 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo)
*/
if (OidIsValid(tbinfo->owning_tab) && !tbinfo->is_identity_sequence)
{
- TableInfo *owning_tab = findTableByOid(tbinfo->owning_tab);
+ TableInfo *this_owning_tab = findTableByOid(tbinfo->owning_tab);
0004. I would rather people used foreach_current_index(lc) > 0 to
determine when we're not doing the first iteration of a foreach loop.
I understand there are more complex cases with filtering that this
cannot be done, but these are highly simple and using
foreach_current_index() removes multiple lines of code and makes it
look nicer.
@@ -762,8 +762,8 @@ fetch_remote_table_info(char *nspname, char *relname,
TupleTableSlot *slot;
Oid attrsRow[] = {INT2VECTOROID};
StringInfoData pub_names;
- bool first = true;
+ first = true;
initStringInfo(_names);
foreach(lc, MySubscription->publications)
0005. How about just "tslot". I'm not a fan of "this".
+++ b/src/backend/replication/logical/tablesync.c
@@ -759,7 +759,7 @@ fetch_remote_table_info(char *nspname, char *relname,
if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 15)
{
WalRcvExecResult *pubres;
- TupleTableSlot *slot;
+ TupleTableSlot *thisslot;
0006. A see the outer shadowed counterpart is used to add a new backup
type. Since I'm not a fan of "this", how about the outer one gets
renamed to "newtype"?
+++ b/src/backend/backup/basebackup_target.c
@@ -73,9 +73,9 @@ BaseBackupAddTarget(char *name,
/* Search the target type list for an existing entry with this name. */
foreach(lc, BaseBackupTargetTypeList)
{
- BaseBackupTargetType *ttype = lfirst(lc);
+ BaseBackupTargetType *this_ttype = lfirst(lc);
0007. Meh, more "this". How about just "col".
+++ b/src/backend/parser/parse_jsontable.c
@@ -341,13 +341,13 @@ transformJsonTableChildPlan(JsonTableContext
*cxt, JsonTablePlan *plan,
/* transform all nested columns into cross/union join */
foreach(lc, columns)
{
- JsonTableColumn *jtc = castNode(JsonTableColumn, lfirst(lc));
+ JsonTableColumn *thisjtc = castNode(JsonTableColumn, lfirst(lc));
There's a discussion about reverting this entire patch. Not sure if
patching master and not backpatching to pg15 would be useful to the
people who may be doing that revert.
0008. Sorry, I had to change this one too. I just have an aversion to
variables named "temp" or "tmp".
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -3109,10 +3109,10 @@ JsonItemFromDatum(Datum val, Oid typid, int32
typmod, JsonbValue *res)
if (JsonContainerIsScalar(>root))
{
- bool res PG_USED_FOR_ASSERTS_ONLY;
+ bool tmp PG_USED_FOR_ASSERTS_ONLY;
- res = JsonbExtractScalar(>root, jbv);
- Assert(res);
+ tmp = JsonbExtractScalar(>root, jbv);
+ Assert(tmp);
I've attached a patch which does things more along the lines of how I
would have done it. I don't think we should be back patching this
stuff.
Any objections to pushing this to master only?
David
diff --git a/src/backend/backup/basebackup_target.c
b/src/backend/backup/basebackup_target.c
index 83928e3205..f280660a03 100644
--- a/src/backend/backup/basebackup_target.c
+++ b/src/backend/backup/basebackup_target.c
@@ -62,7 +62,7 @@ BaseBackupAddTarget(char *name,
void *(*check_detail) (char *, char *),
bbsink *(*get_sink) (bbsink *, void *))
Re: Skipping schema changes in publication
I spent some time on understanding the proposal and the patch. Here
are a few comments wrt the test cases.
> +ALTER PUBLICATION testpub_reset ADD TABLE pub_sch1.tbl1;
> +
> +-- Verify that tables associated with the publication are dropped after RESET
> +\dRp+ testpub_reset
> +ALTER PUBLICATION testpub_reset RESET;
> +\dRp+ testpub_reset
>
> +ALTER PUBLICATION testpub_reset ADD ALL TABLES IN SCHEMA public;
> +
> +-- Verify that schemas associated with the publication are dropped after
> RESET
> +\dRp+ testpub_reset
> +ALTER PUBLICATION testpub_reset RESET;
> +\dRp+ testpub_reset
The results for the above two cases are the same before and after the
reset. Is there any way to verify that?
---
> +-- Can't add EXCEPT TABLE to 'FOR ALL TABLES' publication
> +ALTER PUBLICATION testpub_reset ADD ALL TABLES EXCEPT TABLE pub_sch1.tbl1;
> +
>
> +-- Can't add EXCEPT TABLE to 'FOR TABLE' publication
> +ALTER PUBLICATION testpub_reset ADD ALL TABLES EXCEPT TABLE pub_sch1.tbl1;
> +
>
> +-- Can't add EXCEPT TABLE to 'FOR ALL TABLES IN SCHEMA' publication
> +ALTER PUBLICATION testpub_reset ADD ALL TABLES EXCEPT TABLE pub_sch1.tbl1;
> +
I did not understand the objective of these tests. I think we need to
improve the comments.
Thanks & Regards,
On Mon, Aug 8, 2022 at 2:53 PM vignesh C wrote:
>
> On Mon, Aug 8, 2022 at 12:46 PM vignesh C wrote:
> >
> > On Fri, Jun 3, 2022 at 3:36 PM vignesh C wrote:
> > >
> > > On Thu, May 26, 2022 at 7:04 PM osumi.takami...@fujitsu.com
> > > wrote:
> > > >
> > > > On Monday, May 23, 2022 2:13 PM vignesh C wrote:
> > > > > Attached v7 patch which fixes the buildfarm warning for an unused
> > > > > warning in
> > > > > release mode as in [1].
> > > > Hi, thank you for the patches.
> > > >
> > > >
> > > > I'll share several review comments.
> > > >
> > > > For v7-0001.
> > > >
> > > > (1) I'll suggest some minor rewording.
> > > >
> > > > +
> > > > + The RESET clause will reset the publication to
> > > > the
> > > > + default state which includes resetting the publication options,
> > > > setting
> > > > + ALL TABLES flag to false and
> > > > + dropping all relations and schemas that are associated with the
> > > > publication.
> > > >
> > > > My suggestion is
> > > > "The RESET clause will reset the publication to the
> > > > default state. It resets the publication operations,
> > > > sets ALL TABLES flag to false and drops all relations
> > > > and schemas associated with the publication."
> > >
> > > I felt the existing looks better. I would prefer to keep it that way.
> > >
> > > > (2) typo and rewording
> > > >
> > > > +/*
> > > > + * Reset the publication.
> > > > + *
> > > > + * Reset the publication options, setting ALL TABLES flag to false and
> > > > drop
> > > > + * all relations and schemas that are associated with the publication.
> > > > + */
> > > >
> > > > The "setting" in this sentence should be "set".
> > > >
> > > > How about changing like below ?
> > > > FROM:
> > > > "Reset the publication options, setting ALL TABLES flag to false and
> > > > drop
> > > > all relations and schemas that are associated with the publication."
> > > > TO:
> > > > "Reset the publication operations, set ALL TABLES flag to false and drop
> > > > all relations and schemas associated with the publication."
> > >
> > > I felt the existing looks better. I would prefer to keep it that way.
> > >
> > > > (3) AlterPublicationReset
> > > >
> > > > Do we need to call CacheInvalidateRelcacheAll() or
> > > > InvalidatePublicationRels() at the end of
> > > > AlterPublicationReset() like AlterPublicationOptions() ?
> > >
> > > CacheInvalidateRelcacheAll should be called if we change all tables
> > > from true to false, else the cache will not be invalidated. Modified
> > >
> > > >
> > > > For v7-0002.
> > > >
> > > > (4)
> > > >
> > > > + if (stmt->for_all_tables)
> > > > + {
> > > > + boolisdefault =
> > > > CheckPublicationDefValues(tup);
> > > > +
> > > > + if (!isdefault)
> > > > + ereport(ERROR,
> > > > +
> > > > errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > > > + errmsg("adding ALL TABLES
> > > > requires the publication to have default publication options, no
> > > > tables/
> > > > + errhint("Use ALTER PUBLICATION
> > > > ... RESET to reset the publication"));
> > > >
> > > >
> > > > The errmsg string has three messages for user and is a bit long
> > > > (we have two sentences there connected by 'and').
> > > > Can't we make it concise and split it into a couple of lines for code
> > > > readability ?
> > > >
> > > > I'll suggest a change below.
> > > > FROM:
> > > > "adding ALL TABLES requires the publication to have default publication
> > > > options, no tables/schemas associated and ALL TABLES flag should not be
> > > > set"
> > > > TO:
> > > > "adding ALL TABLES
Strip -mmacosx-version-min options from plperl build
When building on macOS against a Homebrew-provided Perl installation, I
get these warnings during the build:
ld: warning: object file (SPI.o) was built for newer macOS version
(12.4) than being linked (11.3)
ld: warning: object file (plperl.o) was built for newer macOS version
(12.4) than being linked (11.3)
...
This is because the link command uses the option
-mmacosx-version-min=11.3, which comes in from perl_embed_ldflags (perl
-MExtUtils::Embed -e ldopts), but the compile commands don't use that
option, which creates a situation that ld considers inconsistent.
I think an appropriate fix is to strip out the undesired option from
perl_embed_ldflags. We already do that for other options. Proposed
patch attached.From e50c439f2fced8d1b3af9b3bc142f8d43a781c5a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 18 Aug 2022 08:24:03 +0200
Subject: [PATCH] Strip -mmacosx-version-min options from plperl build
---
config/perl.m4 | 2 +-
configure | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/config/perl.m4 b/config/perl.m4
index c823fc8cf0..65f338bda7 100644
--- a/config/perl.m4
+++ b/config/perl.m4
@@ -100,7 +100,7 @@ if test "$PORTNAME" = "win32" ; then
else
pgac_tmp1=`$PERL -MExtUtils::Embed -e ldopts`
pgac_tmp2=`$PERL -MConfig -e 'print $Config{ccdlflags}'`
- perl_embed_ldflags=`echo X"$pgac_tmp1" | sed -e "s/^X//" -e
"s%$pgac_tmp2%%" -e ["s/ -arch [-a-zA-Z0-9_]*//g"]`
+ perl_embed_ldflags=`echo X"$pgac_tmp1" | sed -e "s/^X//" -e
"s%$pgac_tmp2%%" -e ["s/ -arch [-a-zA-Z0-9_]*//g"] -e ["s/
-mmacosx-version-min=[0-9.]*//g"]`
fi
AC_SUBST(perl_embed_ldflags)dnl
if test -z "$perl_embed_ldflags" ; then
diff --git a/configure b/configure
index 176e0f9b00..8e0f648702 100755
--- a/configure
+++ b/configure
@@ -10479,7 +10479,7 @@ if test "$PORTNAME" = "win32" ; then
else
pgac_tmp1=`$PERL -MExtUtils::Embed -e ldopts`
pgac_tmp2=`$PERL -MConfig -e 'print $Config{ccdlflags}'`
- perl_embed_ldflags=`echo X"$pgac_tmp1" | sed -e "s/^X//" -e
"s%$pgac_tmp2%%" -e "s/ -arch [-a-zA-Z0-9_]*//g"`
+ perl_embed_ldflags=`echo X"$pgac_tmp1" | sed -e "s/^X//" -e
"s%$pgac_tmp2%%" -e "s/ -arch [-a-zA-Z0-9_]*//g" -e "s/
-mmacosx-version-min=[0-9.]*//g"`
fi
if test -z "$perl_embed_ldflags" ; then
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
--
2.37.1
Re: Perform streaming logical transactions by background workers and parallel apply
Hi Wang-san, FYI, I also checked the latest patch v23-0001 but found that the v21-0001/v23-0001 differences are minimal, so all my v21* review comments are still applicable for the patch v23-0001. -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Compressed pluggable storage experiments
Hi all, This is a continuation of the above thread... >> > 4. In order to use WAL-logging each page must start with a standard 24 >> > byte PageHeaderData even if it is needless for storage itself. Not a >> > big deal though. Another (acutally documented) WAL-related limitation >> > is that only generic WAL can be used within extension. So unless >> > inserts are made in bulks it's going to require a lot of disk space to >> > accomodate logs and wide bandwith for replication. >> >> Not sure what to suggest. Either you should ignore this problem, or >> you should fix it. I am working on an environment similar to the above extension(pg_cryogen which experiments pluggable storage api's) but don't have much knowledge on pg's logical replication.. Please suggest some approaches to support pg's logical replication for a table with a custom access method, which writes generic wal record. On Wed, 17 Aug 2022 at 19:04, Tomas Vondra wrote: > On Fri, Oct 18, 2019 at 03:25:05AM -0700, Andres Freund wrote: > >Hi, > > > >On 2019-10-17 12:47:47 -0300, Alvaro Herrera wrote: > >> On 2019-Oct-10, Ildar Musin wrote: > >> > >> > 1. Unlike FDW API, in pluggable storage API there are no routines like > >> > "begin modify table" and "end modify table" and there is no shared > >> > state between insert/update/delete calls. > >> > >> Hmm. I think adding a begin/end to modifytable is a reasonable thing to > >> do (it'd be a no-op for heap and zheap I guess). > > > >I'm fairly strongly against that. Adding two additional "virtual" > >function calls for something that's rarely going to be used, seems like > >adding too much overhead to me. > > > > That seems a bit strange to me. Sure - if there's an alternative way to > achieve the desired behavior (clear way to finalize writes etc.), then > cool, let's do that. But forcing people to use invonvenient workarounds > seems like a bad thing to me - having a convenient and clear API is > quite valueable, IMHO. > > Let's see if this actually has a measuerable overhead first. > > > > >> > 2. It looks like I cannot implement custom storage options. E.g. for > >> > compressed storage it makes sense to implement different compression > >> > methods (lz4, zstd etc.) and corresponding options (like compression > >> > level). But as i can see storage options (like fillfactor etc) are > >> > hardcoded and are not extensible. Possible solution is to use GUCs > >> > which would work but is not extremely convinient. > >> > >> Yeah, the reloptions module is undergoing some changes. I expect that > >> there will be a way to extend reloptions from an extension, at the end > >> of that set of patches. > > > >Cool. > > > > Yep. > > > > >> > 3. A bit surprising limitation that in order to use bitmap scan the > >> > maximum number of tuples per page must not exceed 291 due to > >> > MAX_TUPLES_PER_PAGE macro in tidbitmap.c which is calculated based on > >> > 8kb page size. In case of 1mb page this restriction feels really > >> > limiting. > >> > >> I suppose this is a hardcoded limit that needs to be fixed by patching > >> core as we make table AM more pervasive. > > > >That's not unproblematic - a dynamic limit would make a number of > >computations more expensive, and we already spend plenty CPU cycles > >building the tid bitmap. And we'd waste plenty of memory just having all > >that space for the worst case. ISTM that we "just" need to replace the > >TID bitmap with some tree like structure. > > > > I think the zedstore has roughly the same problem, and Heikki mentioned > some possible solutions to dealing with it in his pgconfeu talk (and it > was discussed in the zedstore thread, I think). > > > > >> > 4. In order to use WAL-logging each page must start with a standard 24 > >> > byte PageHeaderData even if it is needless for storage itself. Not a > >> > big deal though. Another (acutally documented) WAL-related limitation > >> > is that only generic WAL can be used within extension. So unless > >> > inserts are made in bulks it's going to require a lot of disk space to > >> > accomodate logs and wide bandwith for replication. > >> > >> Not sure what to suggest. Either you should ignore this problem, or > >> you should fix it. > > > >I think if it becomes a problem you should ask for an rmgr ID to use for > >your extension, which we encode and then then allow to set the relevant > >rmgr callbacks for that rmgr id at startup. But you should obviously > >first develop the WAL logging etc, and make sure it's beneficial over > >generic wal logging for your case. > > > > AFAIK compressed/columnar engines generally implement two types of > storage - write-optimized store (WOS) and read-optimized store (ROS), > where the WOS is mostly just an uncompressed append-only buffer, and ROS > is compressed etc. ISTM the WOS would benefit from a more elaborate WAL > logging, but ROS should be mostly fine with the generic WAL logging. > > But yeah, we should test and measure how beneficial that actually is.
Re: Perform streaming logical transactions by background workers and parallel apply
Here are my review comments for patch v21-0001:
Note - There are some "general" comments which will result in lots of
smaller changes. The subsequent "detailed" review comments have some
overlap with these general comments but I expect some will be missed
so please search/replace to fix all code related to those general
comments.
==
1. GENERAL - main_worker_pid and replorigin_session_setup
Quite a few of my subsequent review comments below are related to the
somewhat tricky (IMO) change to the code for this area. Here is a
summary of some things that can be done to clean/simplify this logic.
1a.
Make the existing replorigin_session_setup function just be a wrapper
that delegates to the other function passing the acquired_by as 0.
This is because in every case but one (in the apply bg worker main) we
are always passing 0, and IMO there is no need to spread the messy
extra param to places that do not use it.
1b.
'main_worker_pid' is a confusing member name given the way it gets
used - e.g. not even set when you actually *are* the main apply
worker? You can still keep all the same logic, but just change the
name to something more like 'apply_leader_pid' - then the code can
make sense because the main apply workers have no "apply leader" but
the apply background workers do.
1c.
IMO it will be much better to use pid_t and InvalidPid for the type
and the unset values of this member.
1d.
The checks/Asserts for main_worker_pid are confusing to read. (e.g.
Assert(worker->main_worker_pid != 0) means the worker is a apply
background worker. IMO there should be convenient macros for these -
then code can be readable again.
e.g.
#define isApplyMainWorker(worker) (worker->apply_leader_pid == InvalidPid)
#define isApplyBgWorker(worker) (worker->apply_leader_pid != InvalidPid)
==
2. GENERAL - ApplyBgworkerInfo
I like that the struct ApplyBgworkerState was renamed to the more
appropriate name ApplyBgworkerInfo. But now all the old variable names
(e.g. 'wstate') and parameters must be updated as well. Please
search/replace them all in code and comments.
e.g.
ApplyBgworkerInfo *wstate
should now be something like:
ApplyBgworkerInfo *winfo;
==
3. GENERAL - ApplyBgWorkerStatus --> ApplyBgworkerState
IMO the enum should be changed to ApplyBgWorkerState because the
values all represent the discrete state that the bgworker is at. See
the top StackOverflow answer here [1] which is the same as the point I
am trying to make with this comment.
This is a simple mechanical exercise rename to fix the reliability
but it will impact lots of variables, parameters, function names, and
comments. Please search/replace to get them all.
==
4. Commit message
In addition, the patch extends the logical replication STREAM_ABORT message so
that abort_time and abort_lsn can also be sent which can be used to update the
replication origin in apply background worker when the streaming transaction is
aborted.
4a.
Should this para also mention something about the introduction of
protocol version 4?
4b.
Should this para also mention that these extensions are not strictly
mandatory for the parallel streaming to still work?
==
5. doc/src/sgml/catalogs.sgml
- If true, the subscription will allow streaming of in-progress
- transactions
+ Controls how to handle the streaming of in-progress transactions:
+ f = disallow streaming of in-progress transactions,
+ t = spill the changes of in-progress transactions to
+ disk and apply at once after the transaction is committed on the
+ publisher,
+ p = apply changes directly using a background
+ worker if available(same as 't' if no worker is available)
Missing whitespace before '('
==
6. doc/src/sgml/logical-replication.sgml
@@ -1334,7 +1344,8 @@ CONTEXT: processing remote data for replication
origin "pg_16395" during "INSER
subscription. A disabled subscription or a crashed subscription will have
zero rows in this view. If the initial data synchronization of any
table is in progress, there will be additional workers for the tables
- being synchronized.
+ being synchronized. Moreover, if the streaming transaction is applied
+ parallelly, there will be additional workers.
"applied parallelly" sounds a bit strange.
SUGGESTION-1
Moreover, if the streaming transaction is applied in parallel, there
will be additional workers.
SUGGESTION-2
Moreover, if the streaming transaction is applied using 'parallel'
mode, there will be additional workers.
==
7. doc/src/sgml/protocol.sgml
@@ -3106,6 +3106,11 @@ psql "dbname=postgres replication=database" -c
"IDENTIFY_SYSTEM;"
Version 3 is supported only for server version 15
and above, and it allows streaming of two-phase commits.
+
+ Version 4 is supported only for server version 16
+ and above, and it allows applying stream of large in-progress
+ transactions in parallel.
+
Re: Cleaning up historical portability baggage
On Mon, Aug 15, 2022 at 5:53 PM Thomas Munro wrote:
> Remove configure probe for IPv6.
> Remove dead ifaddrs.c fallback code.
> Remove configure probe for net/if.h.
> Fix macro problem with gai_strerror on Windows.
> Remove configure probe for netinet/tcp.h.
> mstcpip.h is not missing on MinGW.
I pushed these except one, plus one more about which
turned out to be not needed after a bit of archeology.
Here's a slightly better AF_INET6 one. I'm planning to push it
tomorrow if there are no objections. It does something a little more
aggressive than the preceding stuff, because SUSv3 says that IPv6 is
an "option". I don't see that as an issue: it also says that various
other ubiquitous stuff we're using is optional. Of course, it would
be absurd for a new socket implementation to appear today that can't
talk to a decent chunk of the internet, and all we require here is the
headers. That optionality was relevant for the transition period a
couple of decades ago.
From f162a15a6d723f8c94d9daa6236149e1f39b0d9a Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Thu, 18 Aug 2022 11:55:10 +1200
Subject: [PATCH] Remove configure probe for sockaddr_in6 and require AF_INET6.
SUSv3 defines struct sockaddr_in6, and all targeted Unix
systems have it. Windows has it in . Remove the configure
probe, the macro and a small amount of dead code.
Also remove a mention of IPv6-less builds from the documentation, since
there aren't any.
This is similar to commits f5580882 and 077bf2f2 for Unix sockets.
Even though AF_INET6 is an "optional" component of SUSv3, there are no
known modern operating system without it, and it seems even less likely
to be omitted from future systems than AF_UNIX.
Discussion: https://postgr.es/m/ca+hukgkernfhmvb_h0upremp4lpzgn06yr2_0tyikjzb-2e...@mail.gmail.com
---
configure | 10 --
configure.ac| 6 --
doc/src/sgml/client-auth.sgml | 2 --
src/backend/libpq/auth.c| 21 -
src/backend/libpq/hba.c | 5 -
src/backend/libpq/ifaddr.c | 18 +-
src/backend/libpq/pqcomm.c | 2 --
src/backend/utils/adt/network.c | 10 --
src/backend/utils/adt/pgstatfuncs.c | 11 ++-
src/bin/initdb/initdb.c | 10 --
src/include/pg_config.h.in | 3 ---
src/include/utils/inet.h| 9 -
src/interfaces/libpq/fe-connect.c | 2 --
src/port/inet_net_ntop.c| 5 ++---
src/tools/ifaddrs/test_ifaddrs.c| 2 --
src/tools/msvc/Solution.pm | 1 -
16 files changed, 9 insertions(+), 108 deletions(-)
diff --git a/configure b/configure
index b7fd6c5f4e..c275330114 100755
--- a/configure
+++ b/configure
@@ -16279,16 +16279,6 @@ cat >>confdefs.h <<_ACEOF
_ACEOF
-ac_fn_c_check_type "$LINENO" "struct sockaddr_in6" "ac_cv_type_struct_sockaddr_in6" "$ac_includes_default
-#include
-"
-if test "x$ac_cv_type_struct_sockaddr_in6" = xyes; then :
-
-$as_echo "#define HAVE_IPV6 1" >>confdefs.h
-
-fi
-
-
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for PS_STRINGS" >&5
$as_echo_n "checking for PS_STRINGS... " >&6; }
if ${pgac_cv_var_PS_STRINGS+:} false; then :
diff --git a/configure.ac b/configure.ac
index e5740f4fb5..a97a48a508 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1801,12 +1801,6 @@ AC_CHECK_DECLS([pwritev], [], [AC_LIBOBJ(pwritev)], [#include ])
# This is probably only present on macOS, but may as well check always
AC_CHECK_DECLS(F_FULLFSYNC, [], [], [#include ])
-AC_CHECK_TYPE([struct sockaddr_in6],
-[AC_DEFINE(HAVE_IPV6, 1, [Define to 1 if you have support for IPv6.])],
-[],
-[$ac_includes_default
-#include ])
-
AC_CACHE_CHECK([for PS_STRINGS], [pgac_cv_var_PS_STRINGS],
[AC_LINK_IFELSE([AC_LANG_PROGRAM(
[#include
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 433759928b..c6f1b70fd3 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -305,8 +305,6 @@ hostnogssenc database user
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 1545ff9f16..71677b69d8 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -3014,13 +3014,8 @@ PerformRadiusTransaction(const char *server, const char *secret, const char *por
int packetlength;
pgsocket sock;
-#ifdef HAVE_IPV6
struct sockaddr_in6 localaddr;
struct sockaddr_in6 remoteaddr;
-#else
- struct sockaddr_in localaddr;
- struct sockaddr_in remoteaddr;
-#endif
struct addrinfo hint;
struct addrinfo *serveraddrs;
int port;
@@ -3130,18 +3125,12 @@ PerformRadiusTransaction(const char *server, const char *secret, const char *por
}
memset(, 0, sizeof(localaddr));
-#ifdef HAVE_IPV6
localaddr.sin6_family = serveraddrs[0].ai_family;
localaddr.sin6_addr = in6addr_any;
if (localaddr.sin6_family == AF_INET6)
addrsize = sizeof(struct sockaddr_in6);
else
addrsize =
