Re: Refactoring postmaster's code to cleanup after child exit

[Alexander Lakhin]

Hello Heikki,

10.08.2024 00:13, Heikki Linnakangas wrote:


Committed the patches up to and including this one, with tiny comment changes.


I've noticed that on the current HEAD server.log contains binary data
(an invalid process name) after a child crash. For example, while playing
with -ftapv, I've got:
SELECT to_date('2024 613566758 1', 'IYYY IW ID');
server closed the connection unexpectedly

grep -a 'was terminated' server.log
2024-08-18 07:07:06.482 UTC|||66c19d96.3482f6|LOG:  `�!x� (PID 3441407) was 
terminated by signal 6: Aborted

It looks like this was introduced by commit 28a520c0b (IIUC, namebuf in
CleanupBackend() may stay uninitialized in some code paths).

Best regards,
Alexander

RE: Conflict detection and logging in logical replication

[Zhijie Hou (Fujitsu)]

On Friday, August 16, 2024 2:31 PM Amit Kapila  wrote:
> 
> On Fri, Aug 16, 2024 at 10:46 AM shveta malik 
> wrote:
> >
> > On Thu, Aug 15, 2024 at 12:47 PM Zhijie Hou (Fujitsu)
> >  wrote:
> > >
> > > Thanks. I have checked and merged the changes. Here is the V15 patch
> > > which addressed above comments.
> >
> > Thanks for the patch. Please find few comments and queries:
> >
> > 1)
> > For various conflicts , we have these in Logs:
> > Replica identity (val1)=(30).(for RI on 1 column)
> > Replica identity (pk, val1)=(200, 20).  (for RI on  2 columns)
> > Replica identity (40, 40, 11).(for RI full)
> >
> > Shall we have have column list in last case as well, or can simply
> > have *full* keyword i.e. Replica identity full (40, 40, 11)
> >
> 
> I would prefer 'full' instead of the entire column list as the complete 
> column list
> could be long and may not much sense.

+1 and will change in V16 patch.

Best Regards,
Hou zj

RE: Conflict detection and logging in logical replication

[Zhijie Hou (Fujitsu)]

On Friday, August 16, 2024 2:49 PM Amit Kapila  wrote:
> 
> 
> > 
> >
> > One more comment:
> >
> > 5)
> > For insert/update_exists, the sequence is:
> > Key .. ; existing local tuple .. ; remote tuple ...
> >
> > For rest of the conflicts, sequence is:
> >  Existing local tuple .. ; remote tuple .. ; replica identity ..
> >
> > Is it intentional? Shall the 'Key' or 'Replica Identity' be the first
> > one to come in all conflicts?
> >
> 
> This is worth considering but Replica Identity signifies the old tuple values,
> that is why it is probably kept at the end. 

Right. I personally think the current position is ok.

Best Regards,
Hou zj 

RE: Conflict detection and logging in logical replication

[Zhijie Hou (Fujitsu)]

On Friday, August 16, 2024 5:25 PM shveta malik  wrote:
> 
> On Fri, Aug 16, 2024 at 12:19 PM Amit Kapila 
> wrote:
> >
> > On Fri, Aug 16, 2024 at 11:48 AM shveta malik 
> wrote:
> > >
> > > On Fri, Aug 16, 2024 at 10:46 AM shveta malik 
> wrote:
> > > >
> > > > 3)
> > > > For update_exists(), we dump:
> > > > Key (a, b)=(2, 1)
> > > >
> > > > For delete_missing, update_missing, update_differ, we dump:
> > > > Replica identity (a, b)=(2, 1).
> > > >
> > > > For update_exists as well, shouldn't we dump 'Replica identity'?
> > > > Only for insert case, it should be referred as 'Key'.
> > > >
> > >
> > > On rethinking, is it because for update_exists case 'Key' dumped is
> > > not the one used to search the row to be updated? Instead it is the
> > > one used to search the conflicting row. Unlike update_differ, the
> > > row to be updated and the row currently conflicting will be
> > > different for update_exists case. I earlier thought that 'KEY' and
> > > 'Existing local tuple' dumped always belong to the row currently
> > > being updated/deleted/inserted. But for 'update_eixsts', that is not
> > > the case. We are dumping 'Existing local tuple' and 'Key' for the
> > > row which is conflicting and not the one being updated.  Example:
> > >
> > > ERROR:  conflict detected on relation "public.tab_1":
> > > conflict=update_exists Key (a, b)=(2, 1); existing local tuple (2, 1); 
> > > remote
> tuple (2, 1).
> > >
> > > Operations performed were:
> > > Pub: insert into tab values (1,1);
> > > Sub: insert into tab values (2,1);
> > > Pub: update tab set a=2 where a=1;
> > >
> > > Here Key and local tuple are both 2,1 instead of 1,1. While replica
> > > identity value (used to search original row) will be 1,1 only.
> > >
> > > It may be slightly confusing or say tricky to understand when
> > > compared to other conflicts' LOGs. But not sure what better we can do
> here.
> > >
> >
> > The update_exists behaves more like insert_exists as we detect that
> > only while inserting into index. It is also not clear to me if we can
> > do better than to clarify this in docs.
> >
> 
> Instead of 'existing local tuple', will it be slightly better to have 
> 'conflicting local
> tuple'?

I am slightly not sure about adding one more variety to describe the "existing
local tuple". I think we’d better use a consistent word. But if others feel 
otherwise,
I can change it in next version.

> 
> Few trivial comments:
> 
> 1)
> errdetail_apply_conflict() header says:
> 
>  * 2. Display of conflicting key, existing local tuple, remote new tuple, and
>  *replica identity columns,  if any.
> 
> We may mention that existing *conflicting* local tuple.

Like above, I think that would duplicate the "existing local tuple" word.

> 
> Looking at build_tuple_value_details(), the cases where we display 'KEY 'and
> the ones where we display 'replica identity' are mutually exclusives (we have
> ASSERTs like that).  Shall we add this info in
> header that either Key or   'replica identity' is displayed. Or if we
> don't want to make it mutually exclusive then update_exists is one such casw
> where we can have both Key and 'Replica Identity cols'.

I think it’s fine to display replica identity for update_exists, so added.

> 
> 
> 2)
> BuildIndexValueDescription() header comment says:
> 
>  * This is currently used
>  * for building unique-constraint, exclusion-constraint and logical 
> replication
>  * tuple missing conflict error messages
> 
> Is it being used only for 'tuple missing conflict' flow? I thought, it will 
> be hit for
> other flows as well.

Removed the "tuple missing".

Attach the V16 patch which addressed the comments we agreed on.
I will add a doc patch to explain the log format after the 0001 is RFC.


Best Regards,
Hou zj



v16-0001-Detect-and-log-conflicts-in-logical-replication.patch
Description:  v16-0001-Detect-and-log-conflicts-in-logical-replication.patch

Re: Remove dependence on integer wrapping

[Alexander Lakhin]

18.08.2024 00:52, Joseph Koshakow wrote:

The largest possible (theoretical) value for `nbuckets` is
`1073741824`, the largest power of 2 that fits into an `int`. So, the
largest possible value for `nbuckets << 1` is `2147483648`. This can
fully fit in a `uint32`, so the simple fix for this case is to cast
`nbuckets` to a `uint32` before shifting. I've attached this fix,
Alexander if you have time I would appreciate if you were able to test
it.



Yes, I've tested v25-0002-*.patch and can confirm that this fix works
as well.

Best regards,
Alexander

CI cpluspluscheck failures

[Thomas Munro]

Hi,

The CompilerWarnings cspluspluscheck started failing recently.

1.  LLVM library version issue.  See commit message for details.
2.  pg_verify_backup.h now uses simplehash.h, which references
pg_fatal(), which nobody declared.

I'm not sure why the first one started happening at the commit
(aa2d6b15) that caused the second one, I feel like I'm missing
something...

https://github.com/postgres/postgres/commits/master/

Anyway, these patches turn it green for me.  Please see attached.

https://cirrus-ci.com/task/5014011601747968
From 0a0f45ce79907a0e967563ce7dc529429b604f4e Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Sun, 18 Aug 2024 23:28:10 +1200
Subject: [PATCH 1/2] ci: Fix cpluspluscheck failure due to LLVM warnings.

The virtual machine image used to run CompilerWarnings task now has two
versions of LLVM installed.  While configure is running with
LLVM_CONFIG="llvm-config-16", cpluspluscheck was not smart enough to add
it to the include search path, so it was finding LLVM 14's headers in
/usr/include.  Fix that.

The warnings come from LLVM 14 headers, and are suppressed in normal
building (see commit a56e7b66), but that didn't affect cpluspluscheck.
An alternative fix might be to add the warning suppression, but then it
doesn't really make sense for cplusplus check to be seeing different
headers than the other build tasks.

(Note that a proposed change would remove the warning-generating code,
and we could then remove the warning suppression from commit a56e7b66
too, see commitfest submission #4920.)
---
 src/tools/pginclude/headerscheck | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/tools/pginclude/headerscheck b/src/tools/pginclude/headerscheck
index 436e2b92a3..5953e9d6bf 100755
--- a/src/tools/pginclude/headerscheck
+++ b/src/tools/pginclude/headerscheck
@@ -49,6 +49,7 @@ CXX=`sed -n 's/^CXX[ 	]*=[ 	]*//p' "$MGLOB"`
 PG_SYSROOT=`sed -n 's/^PG_SYSROOT[ 	]*=[ 	]*//p' "$MGLOB"`
 perl_includespec=`sed -n 's/^perl_includespec[ 	]*=[ 	]*//p' "$MGLOB"`
 python_includespec=`sed -n 's/^python_includespec[ 	]*=[ 	]*//p' "$MGLOB"`
+llvm_includespec=`sed -n 's/^LLVM_CPPFLAGS[ 	]*=[ 	]*//p' "$MGLOB"`
 
 # needed on Darwin
 CPPFLAGS=`echo "$CPPFLAGS" | sed "s|\\\$(PG_SYSROOT)|$PG_SYSROOT|g"`
@@ -235,6 +236,8 @@ do
 		EXTRAINCLUDES="-I $builddir/src/backend/parser/" ;;
 	src/backend/utils/adt/*)
 		EXTRAINCLUDES="-I $builddir/src/backend/utils/adt/" ;;
+	src/include/jit/*)
+		EXTRAINCLUDES="$llvm_includespec" ;;
 	*)
 		EXTRAINCLUDES="" ;;
 	esac
-- 
2.39.2

From 359461690fd89e79e67e649f1dac7e8898fd3dd2 Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Sun, 18 Aug 2024 23:56:11 +1200
Subject: [PATCH 2/2] ci: Placate cpluspluscheck about pg_verifybackup.h.

simplehash.h references pg_fatal(), which cpluspluscheck says is
undeclared.
---
 src/bin/pg_verifybackup/pg_verifybackup.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/bin/pg_verifybackup/pg_verifybackup.h b/src/bin/pg_verifybackup/pg_verifybackup.h
index c395217788..d8c566ed58 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.h
+++ b/src/bin/pg_verifybackup/pg_verifybackup.h
@@ -16,6 +16,7 @@
 
 #include "common/controldata_utils.h"
 #include "common/hashfn_unstable.h"
+#include "common/logging.h"
 #include "common/parse_manifest.h"
 #include "fe_utils/simple_list.h"
 
-- 
2.39.2

Re: macOS prefetching support

[Peter Eisentraut]

On 17.08.24 00:01, Thomas Munro wrote:

On Sat, Aug 17, 2024 at 6:58 AM Peter Eisentraut  wrote:

What to do about the order of the symbols and include files.  I threw
something into src/include/port/darwin.h, but I'm not sure if that's
good.  Alternatively, we could not use __darwin__ but instead the more
standard and predefined defined(__APPLE__) && defined(__MACH__).


Hmm.  fd.h and fd.c test for F_NOCACHE, which is pretty closely
related.  Now I'm wondering why we actually need this in
pg_config_manual.h at all.  Who would turn it off at compile time, and
why would they not be satisfied with setting relevant GUCs to 0?  Can
we just teach fd.h to define USE_PREFETCH if
defined(POSIX_FADV_WILLNEED) || defined(F_RDADVISE)?


I thought USE_PREFETCH existed so that we don't have the run-time 
overhead for all the bookkeeping code if we don't have any OS-level 
prefetch support at the end.  But it looks like most of that bookkeeping 
code is skipped anyway if the *_io_concurrency settings are at 0.  So 
yes, getting rid of USE_PREFETCH globally would be useful.



(I have also thought multiple times about removing the configure
probes for F_FULLFSYNC, and just doing #ifdef.  Oh, that's in my patch
for CF #4453.)


Understandable, but we should be careful here that we don't create 
setups that can cause bugs like 
.



I think that's fine.  I don't really like the word "prefetch", could
mean many different things.  What about "requires OS support for
issuing read-ahead advice", which uses a word that appears in both of
those interfaces?


I like that term.

Cirrus CI for macOS branches 16 and 15 broken

[Peter Eisentraut]

The Cirrus CI for REL_16_STABLE and REL_15_STABLE for macOS is 
apparently broken right now.  Here is a log example:


[13:57:11.305] sh src/tools/ci/ci_macports_packages.sh \
[13:57:11.305]   ccache \
[13:57:11.305]   icu \
[13:57:11.305]   kerberos5 \
[13:57:11.305]   lz4 \
[13:57:11.305]   meson \
[13:57:11.305]   openldap \
[13:57:11.305]   openssl \
[13:57:11.305]   p5.34-io-tty \
[13:57:11.305]   p5.34-ipc-run \
[13:57:11.305]   python312 \
[13:57:11.305]   tcl \
[13:57:11.305]   zstd
[13:57:11.325] macOS major version: 14
[13:57:12.554] MacPorts package URL: 
https://github.com/macports/macports-base/releases/download/v2.9.3/MacPorts-2.9.3-14-Sonoma.pkg

[14:01:37.252] installer: Package name is MacPorts
[14:01:37.252] installer: Installing at base path /
[14:01:37.252] installer: The install was successful.
[14:01:37.257]
[14:01:37.257] real 4m23.837s
[14:01:37.257] user 0m0.385s
[14:01:37.257] sys  0m0.339s
[14:01:37.282] macportsuser root
[14:01:37.431] Error: /opt/local/bin/port: Failed to initialize 
MacPorts, sqlite error: attempt to write a readonly database (8) while 
executing query: CREATE INDEX registry.snapshot_file_id ON 
snapshot_files(id)

[14:01:37.599] Error: No ports matched the given expression


REL_17_STABLE and up are working.

I know there have been some changes recently to manage the OS version 
change.  Are these older branches expected to work?

Re: Meson far from ready on Windows

[walther]

Andres Freund:

That's not necessarily true. The nix package manager and thus NixOS track all 
dependencies for a piece of software. If any of the dependencies are updated, all 
dependents are rebuilt, too. So the security concern doesn't apply here. There is a 
"static overlay", which builds everything linked fully statically.


Right. There's definitely some scenario where it's ok, I was simplifying a bit.


Unfortunately, PostgreSQL doesn't build in that, so far.


I've built mostly statically linked pg without much of a problem, what trouble 
did you encounter? Think there were some issues with linking Kerberos and 
openldap statically, but not on postgres' side.


Mostly the "can't disable shared libraries / backend builds" part 
mentioned below.



Building the postgres backend without support for dynamic linking doesn't make 
sense though. Extensions are just stop ingrained part of pg.


I think there might be some limited use-cases for a fully-static 
postgres backend without the ability to load extensions: Even if we get 
libpq to build fine in the fully-static overlay mentioned above, a lot 
of reverse dependencies have to disable tests, because they need a 
running postgres server to run their tests against.


Providing a really simple postgres backend, with only minimal 
functionality, would allow some basic sanity tests, even in this purely 
static environment.



Lately, I have been looking into building at least libpq in that static 
overlay, via Meson. There are two related config options:
-Ddefault_library=shared|static|both
-Dprefer_static

The first controls which libraries (libpq, ...) to build ourselves. The second 
controls linking, IIUC also against external dependencies.


Pg by default builds a static libpq on nearly all platforms (not aix I think 
and maybe not Windows when building with autoconf, not sure about the old msvc 
system) today?


Yes, PG builds a static libpq today. But it's hard-to-impossible to 
*disable building the shared library*. In the fully static overlay, this 
causes the build to fail, because shared libraries can't be build.



Maybe it would be a first step to support -Dprefer_static?


That should work for nearly all dependencies today. Except for libintl, I 
think.  I found that there are a lot of buglets in static link dependencies of 
various libraries though.


To support prefer_static, we'd also have to look at our internal 
linking, i.e. whether for example psql is linked against libpq 
statically or dynamically. Once prefer_static controls that, that's 
already a step forward to be able to build more of the code-base without 
shared libraries available.


Best,

Wolfgang

Re: The xversion-upgrade test fails to stop server

[Alexander Lakhin]

Hello Andrew,

04.06.2024 14:56, Andrew Dunstan wrote:




but I can't see ../snapshot_too_old/output_iso/regression.diff and
.../snapshot_too_old/output_iso/log/postmaster.log in the log.




will do.



I've discovered a couple of other failures where the interesting log files
are not saved.

[1] has only inst/logfile saved and that's not enough for TAP tests in
src/test/modules.

I've emulated the failure (not trying to guess the real cause) with:
--- a/src/test/modules/test_custom_rmgrs/Makefile
+++ b/src/test/modules/test_custom_rmgrs/Makefile
@@ -14,0 +15,4 @@ TAP_TESTS = 1
+remove:
+    make uninstall
+install: remove

and saw mostly the same with the buildfarm client REL_17.

I've tried the following addition to run_build.pl:
@@ -2194,6 +2194,8 @@ sub make_testmodules_install_check
    my @logs = glob("$pgsql/src/test/modules/*/regression.diffs");
    push(@logs, "inst/logfile");
    $log->add_log($_) foreach (@logs);
+   @logs = glob("$pgsql/src/test/modules/*/tmp_check/log/*");
+   $log->add_log($_) foreach (@logs);
and it works as expected for me.

The output of another failure ([2]) contains only:
\342\226\266 1/1 + partition_prune  3736 ms FAIL
but no regression.diffs
(Fortunately, in this particular case I found "FATAL:  incorrect checksum
in control file" in inst/logfile, so that can explain the failure.)

Probably, run_build.pl needs something like:
@@ -1848,7 +1848,7 @@ sub make_install_check
    @checklog = run_log("cd $pgsql/src/test/regress && $make 
$chktarget");
    }
    my $status = $? >> 8;
-   my @logfiles = ("$pgsql/src/test/regress/regression.diffs", 
"inst/logfile");
+   my @logfiles = ("$pgsql/src/test/regress/regression.diffs", 
"$pgsql/testrun/regress-running/regress/regression.diffs", "inst/logfile");



A similar failure have occurred today:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=opaleye&dt=2024-06-08%2001%3A41%3A41

So maybe it would make sense to increase default PGCTLTIMEOUT for
buildfarm clients, say, to 180 seconds?



Sure. For now I have added it to the config on crake, but we can make it a 
default.


By the way, opaleye failed once again (with 120 seconds timeout):
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=opaleye&dt=2024-08-13%2002%3A04%3A07

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gull&dt=2024-08-06%2014%3A56%3A39
[2] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2024-08-17%2001%3A12%3A50

Best regards,
Alexander

Improving the notation for ecpg.addons rules

[Tom Lane]

I've gotten annoyed by the notation used for ecpg.addons rules,
in which all the tokens of the gram.y rule to be modified
are just concatenated.  This is unreadable and potentially
ambiguous:

ECPG: fetch_argsABSOLUTE_PSignedIconstopt_from_incursor_name addon

The attached draft patch changes things so that we can write

ECPG: addon fetch_args ABSOLUTE_P SignedIconst opt_from_in cursor_name

which is a whole lot closer to what actually appears in gram.y:

fetch_args:cursor_name
...
| ABSOLUTE_P SignedIconst opt_from_in cursor_name

(Note that I've also moved the rule type "addon" to the front.
This isn't strictly necessary, but it seems less mistake-prone.)

While I've not done it in the attached, perhaps it would be
an improvement to allow a colon after the target nonterminal:

ECPG: addon fetch_args: ABSOLUTE_P SignedIconst opt_from_in cursor_name

to bring it even closer to what is written in gram.y.  You could
imagine going further and writing this case as something like

ECPG: addon fetch_args | ABSOLUTE_P SignedIconst opt_from_in cursor_name

but I think that might be a step too far.  IMO it's not adding much
readability, and it seems like introducing an unnecessary dependency
on exactly how the gram.y alternatives are laid out.

BTW, the attached patch won't apply to HEAD, it's meant to apply
after the patch series being discussed at [1].  So I won't stick
this in the CF yet.

Thoughts?

regards, tom lane

[1] https://www.postgresql.org/message-id/flat/2011420.1713493...@sss.pgh.pa.us

diff --git a/src/interfaces/ecpg/preproc/README.parser b/src/interfaces/ecpg/preproc/README.parser
index d6bb872165..11b6e102b6 100644
--- a/src/interfaces/ecpg/preproc/README.parser
+++ b/src/interfaces/ecpg/preproc/README.parser
@@ -40,19 +40,19 @@ continue to use the normal Bison notations.)
 
 
 ecpg.addons contains entries that begin with a line like
-   ECPG: concattokens ruletype
+   ECPG: ruletype tokenlist
 and typically have one or more following lines that are the code
 for a grammar action.  Any line not starting with "ECPG:" is taken
 to be part of the code block for the preceding "ECPG:" line.
 
-"concattokens" identifies which gram.y production this entry affects.
-It is simply the target nonterminal and the tokens from the gram.y rule
-concatenated together.  For example, to modify the action for a gram.y
-rule like this:
+"tokenlist" identifies which gram.y production this entry affects.
+It is simply a list of the target nonterminal and the input tokens
+from the gram.y rule.  For example, to modify the action for a
+gram.y rule like this:
   target: tokenA tokenB tokenC {...}
-"concattokens" would be "targettokenAtokenBtokenC".  If we want to
+"tokenlist" would be "target tokenA tokenB tokenC".  If we want to
 modify a non-first alternative for a nonterminal, we still write the
-nonterminal.  For example, "concattokens" should be "targettokenDtokenE"
+nonterminal.  For example, "tokenlist" should be "target tokenD tokenE"
 to affect the second alternative in:
   target: tokenA tokenB tokenC {...}
   | tokenD tokenE {...}
@@ -72,7 +72,7 @@ c) "rule" - the automatic action is emitted, but then the entry's
 code block is added verbatim afterwards.  This typically is
 used to add new alternatives to a nonterminal of the core grammar.
 For example, given the entry:
-  ECPG: targettokenAtokenBtokenC rule
+  ECPG: rule target tokenA tokenB tokenC
   | tokenD tokenE { custom_action; }
 what will be emitted is
   target: tokenA tokenB tokenC { automatic_action; }
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index 05de4ff1f1..71f7ad26ad 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -1,5 +1,5 @@
 /* src/interfaces/ecpg/preproc/ecpg.addons */
-ECPG: stmtClosePortalStmt block
+ECPG: block stmt ClosePortalStmt
 	{
 		if (INFORMIX_MODE)
 		{
@@ -16,23 +16,23 @@ ECPG: stmtClosePortalStmt block
 
 		output_statement(@1, 0, ECPGst_normal);
 	}
-ECPG: stmtDeallocateStmt block
+ECPG: block stmt DeallocateStmt
 	{
 		output_deallocate_prepare_statement(@1);
 	}
-ECPG: stmtDeclareCursorStmt block
+ECPG: block stmt DeclareCursorStmt
 	{
 		output_simple_statement(@1, (strncmp(@1, "ECPGset_var", strlen("ECPGset_var")) == 0) ? 4 : 0);
 	}
-ECPG: stmtDiscardStmt block
-ECPG: stmtFetchStmt block
+ECPG: block stmt DiscardStmt
+ECPG: block stmt FetchStmt
 	{ output_statement(@1, 1, ECPGst_normal); }
-ECPG: stmtDeleteStmt block
-ECPG: stmtInsertStmt block
-ECPG: stmtSelectStmt block
-ECPG: stmtUpdateStmt block
+ECPG: block stmt DeleteStmt
+ECPG: block stmt InsertStmt
+ECPG: block stmt SelectStmt
+ECPG: block stmt UpdateStmt
 	{ output_statement(@1, 1, ECPGst_prepnormal); }
-ECPG: stmtExecuteStmt block
+ECPG: block stmt ExecuteStmt
 	{
 		check_declared_list($1.name);
 		if ($1.type == NULL || strlen($1.type) ==

Re: Cirrus CI for macOS branches 16 and 15 broken

[Tomas Vondra]

On 8/18/24 16:07, Peter Eisentraut wrote:
> The Cirrus CI for REL_16_STABLE and REL_15_STABLE for macOS is
> apparently broken right now.  Here is a log example:
> 
> [13:57:11.305] sh src/tools/ci/ci_macports_packages.sh \
> [13:57:11.305]   ccache \
> [13:57:11.305]   icu \
> [13:57:11.305]   kerberos5 \
> [13:57:11.305]   lz4 \
> [13:57:11.305]   meson \
> [13:57:11.305]   openldap \
> [13:57:11.305]   openssl \
> [13:57:11.305]   p5.34-io-tty \
> [13:57:11.305]   p5.34-ipc-run \
> [13:57:11.305]   python312 \
> [13:57:11.305]   tcl \
> [13:57:11.305]   zstd
> [13:57:11.325] macOS major version: 14
> [13:57:12.554] MacPorts package URL:
> https://github.com/macports/macports-base/releases/download/v2.9.3/MacPorts-2.9.3-14-Sonoma.pkg
> [14:01:37.252] installer: Package name is MacPorts
> [14:01:37.252] installer: Installing at base path /
> [14:01:37.252] installer: The install was successful.
> [14:01:37.257]
> [14:01:37.257] real    4m23.837s
> [14:01:37.257] user    0m0.385s
> [14:01:37.257] sys    0m0.339s
> [14:01:37.282] macportsuser root
> [14:01:37.431] Error: /opt/local/bin/port: Failed to initialize
> MacPorts, sqlite error: attempt to write a readonly database (8) while
> executing query: CREATE INDEX registry.snapshot_file_id ON
> snapshot_files(id)
> [14:01:37.599] Error: No ports matched the given expression
> 
> 
> REL_17_STABLE and up are working.
> 

Are they? I see exactly the same failure on all branches, including 17
and master. For here this is on master:

https://cirrus-ci.com/task/5918517050998784

regards

-- 
Tomas Vondra

Re: Cirrus CI for macOS branches 16 and 15 broken

[Thomas Munro]

On Mon, Aug 19, 2024 at 2:07 AM Peter Eisentraut  wrote:
> [14:01:37.431] Error: /opt/local/bin/port: Failed to initialize
> MacPorts, sqlite error: attempt to write a readonly database (8) while
> executing query: CREATE INDEX registry.snapshot_file_id ON
> snapshot_files(id)

Hmmm.  Basically there is a loop-back disk device that get cached
between runs (same technique as ccache), on which macports is
installed.  This makes it ready to test stuff fast, with all the
dependencies ready and being updated only when they need to be
upgraded.  It is both clever and scary due to the path dependency...
(Cf other OSes, where we have a base image with all the right packages
installed already, no "memory" between runs like that.)

The macOS major version and hash of the MacPorts package install
script are in the cache key for that (see 64c39bd5), so a change to
that script would make a totally fresh installation, and hopefully
work.  I will look into that, but it would also be nice to understand
how it go itself into that state so we can avoid it...

> I know there have been some changes recently to manage the OS version
> change.  Are these older branches expected to work?

Yes.

Re: CI cpluspluscheck failures

[Thomas Munro]

On Mon, Aug 19, 2024 at 12:10 AM Thomas Munro  wrote:
> I'm not sure why the first one started happening at the commit
> (aa2d6b15) that caused the second one, I feel like I'm missing
> something...

What I was missing is that the LLVM warnings were already present, but
not causing failures because they are "system" headers.  So I'll go
and push the fix for aa2d6b15, and wait longer for comment on the LLVM
one which has been wrong, but green, for a while.

Re: Cirrus CI for macOS branches 16 and 15 broken

[Thomas Munro]

On Mon, Aug 19, 2024 at 7:52 AM Thomas Munro  wrote:
> The macOS major version and hash of the MacPorts package install
> script are in the cache key for that (see 64c39bd5), so a change to
> that script would make a totally fresh installation, and hopefully
> work.  I will look into that, but it would also be nice to understand
> how it go itself into that state so we can avoid it...

Oh, it already is a cache miss and thus a fresh installation, in
Tomas's example.  I can reproduce that in my own Github account by
making a trivial change to ci_macports_packages.sh to I get a cache
miss too.  It appears to install macports just fine, and then a later
command fails in MacPort's sqlite package registry database, "attempt
to write a readonly database".  At a wild guess, what has changed here
to trigger this new condition is that MacPorts has noticed a new
stable release of itself available and taken some new code path
related to upgrading.  No idea why it thinks its package database is
read-only, though... looking...

Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

[Alexander Korotkov]

On Sat, Aug 10, 2024 at 6:57 PM Dmitry Koval  wrote:
> > Probably
> > QueryCompletion struct fits this purpose best from the existing
> > parameters.  Attached draft patch implements returning oid of newly
> > created relation as part of QueryCompletion.  Thoughts?
>
> I agree, returning the oid of the newly created relation is the best way
> to solve the problem.
> (Excuse me, I won't have access to a laptop for the next week - and
> won't be able to look at the source code).

Thank you for your feedback.  Although, I decided QueryCompletion is
not a good place for this new field.  It looks more appropriate to
place it to TableLikeClause, which already contains one relation oid
inside.  The revised patch is attached.

--
Regards,
Alexander Korotkov
Supabase


v2-0001-Avoid-repeated-table-name-lookups-in-createPartit.patch
Description: Binary data

Re: Drop database command will raise "wrong tuple length" if pg_database tuple contains toast attribute.

[Tomas Vondra]

On 8/16/24 13:26, Tomas Vondra wrote:
> Hi Ayush,
> 
> ...
> 
> So this fix seems reasonable.
> 

I've pushed this to all affected branches, except for 11 which is EOL.

I thought about adding a test, but I couldn't think of a TAP test where
this would really fit, and it didn't seem very practical to have a test
creating hundreds of roles. So I abandoned the idea.


Thanks for the report and the fix!

-- 
Tomas Vondra

Re: Cirrus CI for macOS branches 16 and 15 broken

[Tom Lane]

Thomas Munro  writes:
> Oh, it already is a cache miss and thus a fresh installation, in
> Tomas's example.  I can reproduce that in my own Github account by
> making a trivial change to ci_macports_packages.sh to I get a cache
> miss too.  It appears to install macports just fine, and then a later
> command fails in MacPort's sqlite package registry database, "attempt
> to write a readonly database".  At a wild guess, what has changed here
> to trigger this new condition is that MacPorts has noticed a new
> stable release of itself available and taken some new code path
> related to upgrading.  No idea why it thinks its package database is
> read-only, though... looking...

Indeed, MacPorts seems to have recently put out a 2.10.1 release.
This is not specific to the CI installation though.  What I saw on
my laptop, following my usual process for a MacPorts update, was:

$ sudo port -v selfupdate
... reported installing 2.10.1 ...
$ port outdated  # to see what will be upgraded
... failed with "write a readonly database" error!
$ sudo port upgrade outdated
... it's busily rebuilding a pile o' stuff ...

I didn't think to try it, but I bet "sudo port outdated" would
have worked.  I'm also betting that something in the CI update
recipe is taking the same shortcut of omitting "sudo".  That
works in the normal case, but seemingly not after a MacPorts base
update.

regards, tom lane

Re: Cirrus CI for macOS branches 16 and 15 broken

[Thomas Munro]

On Mon, Aug 19, 2024 at 10:01 AM Thomas Munro  wrote:
> Oh, it already is a cache miss and thus a fresh installation, in
> Tomas's example.  I can reproduce that in my own Github account by
> making a trivial change to ci_macports_packages.sh to I get a cache
> miss too.  It appears to install macports just fine, and then a later
> command fails in MacPort's sqlite package registry database, "attempt
> to write a readonly database".  At a wild guess, what has changed here
> to trigger this new condition is that MacPorts has noticed a new
> stable release of itself available and taken some new code path
> related to upgrading.  No idea why it thinks its package database is
> read-only, though... looking...

I still don't know what's happening.  In case it helps someone else
see it, the error comes from "sudo port unsetrequested installed".
But in any case, switching to 2.10.1 seems to do the trick.  See
attached.
From 9659863fcb1942413a9e6ae66f687c9b2a77ba9e Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Mon, 19 Aug 2024 09:32:50 +1200
Subject: [PATCH] ci: Upgrade MacPorts version to 2.10.1.

MacPorts version 2.9.3 started failing in our ci_macports_packages.sh
script, for reasons not yet determined (at a guess, possibly linked to
the release of 2.10.1).  2.10.1 seems to work, so let's switch to it.

Note that commit 64c39bd5 abandoned an earlier attempt to select
MacPorts versions automatically, after it choked on a broken beta
version.

Back-patch to 15, where CI began.

Discussion: https://postgr.es/m/81f104e8-f0a9-43c0-85bd-2bbbf590a5b8%40eisentraut.org
---
 src/tools/ci/ci_macports_packages.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/tools/ci/ci_macports_packages.sh b/src/tools/ci/ci_macports_packages.sh
index 71248bd22b..b3df6d36a4 100755
--- a/src/tools/ci/ci_macports_packages.sh
+++ b/src/tools/ci/ci_macports_packages.sh
@@ -19,7 +19,7 @@ echo "macOS major version: $macos_major_version"
 # Scan the available MacPorts releases to find one that matches the running
 # macOS release.
 macports_release_list_url="https://api.github.com/repos/macports/macports-base/releases";
-macports_version_pattern="2\.9\.3"
+macports_version_pattern="2\.10\.1"
 macports_url="$( curl -s $macports_release_list_url | grep "\"https://github.com/macports/macports-base/releases/download/v$macports_version_pattern/MacPorts-$macports_version_pattern-$macos_major_version-[A-Za-z]*\.pkg\""; | sed 's/.*: "//;s/".*//' | head -1 )"
 echo "MacPorts package URL: $macports_url"
 
-- 
2.39.2

Re: Cirrus CI for macOS branches 16 and 15 broken

[Tom Lane]

Thomas Munro  writes:
> I still don't know what's happening.  In case it helps someone else
> see it, the error comes from "sudo port unsetrequested installed".
> But in any case, switching to 2.10.1 seems to do the trick.  See
> attached.

Interesting.  Now that I've finished "sudo port upgrade outdated",
my laptop is back to a state where unprivileged "port outdated"
is successful.

What this smells like is that MacPorts has to do some kind of database
update as a result of its major version change, and there are code
paths that are not expecting that to get invoked.  It makes sense
that unprivileged "port outdated" would fail to perform the database
update, but not quite as much for "sudo port unsetrequested installed"
to fail.  That case seems like a MacPorts bug; maybe worth filing?

regards, tom lane

possible issue in postgres_fdw batching

[Tomas Vondra]

Hi,

There's a pgsql-bugs report [1] about a behavior change with batching
enabled in postgres_fdw. While I ultimately came to the conclusion the
behavior change is not a bug, I find it annoying. But I'm not 100% sure
about the "not a bug" conclusion, and and the same time I can't think of
a way to handle it better ...

The short story is that given a foreign table "t" and a function "f"
that queries the data, the batching can change the behavior. The bug
report uses DEFAULT expressions and constraints, but that's not quite
necessary.

Consider this simplified example:

  CREATE TABLE t (a INT);

  CREATE FOREIGN TABLE f (a INT) SERVER loopback
 OPTIONS (table_name 't');


  CREATE FUNCTION counter() RETURNS int LANGUAGE sql AS
  $$ SELECT COUNT(*) FROM f $$;

And now do

  INSERT INTO f SELECT counter() FROM generate_series(1,100);

With batch_size=1 this produces a nice sequence, with batching we start
to get duplicate values - which is not surprising, as the function is
oblivious to the data still in the buffer.

The first question is if this is a bug. At first I thought yes - this is
a bug, but I'm no longer convinced of that. The problem is this function
is inherently unsafe under concurrency - even without batching, it only
takes two concurrent sessions to get the same misbehavior.

Ofc, the concurrency can be prevented by locking the table in some way,
but the batching can be disabled with the same effect.

Anyway, I was thinking about ways to improve / fix this, and I don't
have much. The first thought was that we could inspect the expressions
and disable batching if any of the expressions is volatile.

Unfortunately, the expressions can come from anywhere (it's not just
default values as in the original report), and it's not even easily
accessible from nodeModifyTable. Which is where we decide whether to do
batching, etc. It might be a subquery, values, ...

The other option that just occurred to me is that maybe it would be
possible to flush the batches if we happen to query the same foreign
table from other parts of the plan. In this case it'd mean that every
time we call counter(), we'd flush data. But that doesn't seem very
simple either, because we need to find the nodeModifyTable nodes for the
same foreign table, and we'd need to trigger the flush.


Opinions? Does this qualify as a bug, of just a manifestation of a
function that doesn't work under concurrency?


[1]
https://www.postgresql.org/message-id/20240809030755.jubqv6f6vpxkf...@jasonk.me

-- 
Tomas Vondra

Re: possible issue in postgres_fdw batching

[Tom Lane]

Tomas Vondra  writes:
> Consider this simplified example:

>   CREATE TABLE t (a INT);
>   CREATE FOREIGN TABLE f (a INT) SERVER loopback
>  OPTIONS (table_name 't');
>   CREATE FUNCTION counter() RETURNS int LANGUAGE sql AS
>   $$ SELECT COUNT(*) FROM f $$;

> And now do

>   INSERT INTO f SELECT counter() FROM generate_series(1,100);

> With batch_size=1 this produces a nice sequence, with batching we start
> to get duplicate values - which is not surprising, as the function is
> oblivious to the data still in the buffer.

> The first question is if this is a bug.

I'd say no; this query is unduly chummy with implementation details.
Even with no foreign table in the picture at all, we would be fully
within our rights to execute the SELECT to completion before any
of the insertions became visible.  (Arguably, it's the current
behavior that is surprising, not that one.)

regards, tom lane

Re: Cirrus CI for macOS branches 16 and 15 broken

[Thomas Munro]

On Mon, Aug 19, 2024 at 10:55 AM Tom Lane  wrote:
> Thomas Munro  writes:
> > I still don't know what's happening.  In case it helps someone else
> > see it, the error comes from "sudo port unsetrequested installed".
> > But in any case, switching to 2.10.1 seems to do the trick.  See
> > attached.
>
> Interesting.  Now that I've finished "sudo port upgrade outdated",
> my laptop is back to a state where unprivileged "port outdated"
> is successful.
>
> What this smells like is that MacPorts has to do some kind of database
> update as a result of its major version change, and there are code
> paths that are not expecting that to get invoked.  It makes sense
> that unprivileged "port outdated" would fail to perform the database
> update, but not quite as much for "sudo port unsetrequested installed"
> to fail.  That case seems like a MacPorts bug; maybe worth filing?

Huh.  Right, interesting theory.  OK, I'll push that patch to use
2.10.1 anyway, and report what we observed to see what they say.

It's funny that when I had an automatic "pick latest" thing, it broke
on their beta release, but when I pinned it to 2.9.3, it broke when
they made a new stable release anyway.  A middle way would be to use a
pattern that skips alpha/beta/etc...

Re: Cirrus CI for macOS branches 16 and 15 broken

[Tom Lane]

Thomas Munro  writes:
> On Mon, Aug 19, 2024 at 10:55 AM Tom Lane  wrote:
>> What this smells like is that MacPorts has to do some kind of database
>> update as a result of its major version change, and there are code
>> paths that are not expecting that to get invoked.  It makes sense
>> that unprivileged "port outdated" would fail to perform the database
>> update, but not quite as much for "sudo port unsetrequested installed"
>> to fail.  That case seems like a MacPorts bug; maybe worth filing?

> Huh.  Right, interesting theory.  OK, I'll push that patch to use
> 2.10.1 anyway, and report what we observed to see what they say.

Actually, it's a bug that it's trying to force an upgrade on us, isn't
it?  Or does the CI recipe include something that asks for that?

regards, tom lane

Re: Emitting JSON to file using COPY TO

[jian he]

On Mon, Apr 1, 2024 at 8:00 PM jian he  wrote:
>
rebased.
minor cosmetic error message change.

I think all the issues in this thread have been addressed.
From b96dfe41f0935b08b1190f399e29ee2450169529 Mon Sep 17 00:00:00 2001
From: jian he 
Date: Sat, 17 Aug 2024 11:08:25 +0800
Subject: [PATCH v11 1/2] introduce json format for COPY TO

 json format is only allowed in COPY TO operation.
 also cannot be used with header option.

 discussion: https://postgr.es/m/CALvfUkBxTYy5uWPFVwpk_7ii2zgT07t3d-yR_cy4sfrrLU%3Dkcg%40mail.gmail.com
 discussion: https://postgr.es/m/6a04628d-0d53-41d9-9e35-5a8dc302c...@joeconway.com
---
 doc/src/sgml/ref/copy.sgml |  5 +++
 src/backend/commands/copy.c| 13 +++
 src/backend/commands/copyto.c  | 51 +---
 src/backend/parser/gram.y  |  8 +
 src/backend/utils/adt/json.c   |  5 ++-
 src/include/commands/copy.h|  1 +
 src/include/utils/json.h   |  2 ++
 src/test/regress/expected/copy.out | 54 ++
 src/test/regress/sql/copy.sql  | 38 +
 9 files changed, 169 insertions(+), 8 deletions(-)

diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 1518af8a04..616abf508e 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -218,9 +218,14 @@ COPY { table_name [ ( text,
   csv (Comma Separated Values),
+  json (JavaScript Object Notation),
   or binary.
   The default is text.
  
+ 
+  The json option is allowed only in
+  COPY TO.
+ 
 

 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 3bb579a3a4..a3ee65d000 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -494,6 +494,8 @@ ProcessCopyOptions(ParseState *pstate,
  /* default format */ ;
 			else if (strcmp(fmt, "csv") == 0)
 opts_out->csv_mode = true;
+			else if (strcmp(fmt, "json") == 0)
+opts_out->json_mode = true;
 			else if (strcmp(fmt, "binary") == 0)
 opts_out->binary = true;
 			else
@@ -739,6 +741,11 @@ ProcessCopyOptions(ParseState *pstate,
 		/*- translator: %s is the name of a COPY option, e.g. ON_ERROR */
  errmsg("cannot specify %s in BINARY mode", "HEADER")));
 
+	if (opts_out->json_mode && opts_out->header_line)
+		ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot specify %s in JSON mode", "HEADER")));
+
 	/* Check quote */
 	if (!opts_out->csv_mode && opts_out->quote != NULL)
 		ereport(ERROR,
@@ -837,6 +844,12 @@ ProcessCopyOptions(ParseState *pstate,
  errmsg("COPY %s cannot be used with %s", "FREEZE",
 		"COPY TO")));
 
+	/* Check json format  */
+	if (opts_out->json_mode && is_from)
+		ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("COPY json mode cannot be used with %s", "COPY FROM")));
+
 	if (opts_out->default_print)
 	{
 		if (!is_from)
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 91de442f43..14ba9fde50 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -24,6 +24,7 @@
 #include "executor/execdesc.h"
 #include "executor/executor.h"
 #include "executor/tuptable.h"
+#include "funcapi.h"
 #include "libpq/libpq.h"
 #include "libpq/pqformat.h"
 #include "mb/pg_wchar.h"
@@ -31,6 +32,7 @@
 #include "pgstat.h"
 #include "storage/fd.h"
 #include "tcop/tcopprot.h"
+#include "utils/json.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
@@ -139,9 +141,20 @@ SendCopyBegin(CopyToState cstate)
 
 	pq_beginmessage(&buf, PqMsg_CopyOutResponse);
 	pq_sendbyte(&buf, format);	/* overall format */
-	pq_sendint16(&buf, natts);
-	for (i = 0; i < natts; i++)
-		pq_sendint16(&buf, format); /* per-column formats */
+	if (!cstate->opts.json_mode)
+	{
+		pq_sendint16(&buf, natts);
+		for (i = 0; i < natts; i++)
+			pq_sendint16(&buf, format); /* per-column formats */
+	}
+	else
+	{
+		/*
+		 * JSON mode is always one non-binary column
+		 */
+		pq_sendint16(&buf, 1);
+		pq_sendint16(&buf, 0);
+	}
 	pq_endmessage(&buf);
 	cstate->copy_dest = COPY_FRONTEND;
 }
@@ -917,7 +930,7 @@ CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
 	/* Make sure the tuple is fully deconstructed */
 	slot_getallattrs(slot);
 
-	if (!cstate->opts.binary)
+	if (!cstate->opts.binary && !cstate->opts.json_mode)
 	{
 		bool		need_delim = false;
 
@@ -945,7 +958,7 @@ CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
 			}
 		}
 	}
-	else
+	else if (!cstate->opts.json_mode)
 	{
 		foreach_int(attnum, cstate->attnumlist)
 		{
@@ -965,6 +978,34 @@ CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
 			}
 		}
 	}
+	else
+	{
+		Datum		rowdata;
+		StringInfo	result;
+
+		/*
+		 * if COPY TO source data is from a query, not a table, then we need
+		 * copy CopyToState->TupleDesc->attrs to
+		 * slot->tts_tupleDescriptor->attrs because the slot's TupleDesc->attrs
+		 * may change during query execution, but comp

Re: Injection Points remaining stats

[Michael Paquier]

On Mon, Aug 12, 2024 at 10:25:13AM -0400, Yogesh Sharma wrote:
> Attaching a patch to add remaining cached and loaded stats as mentioned in
> commit f68cd847fa40ead44a786b9c34aff9ccc048004b message. Existing TAP tests
> were updated to handle new stats. This patch has been tested on HEAD using
> "make check-world" after enabling injection points via
> "--enable-injection-points".

Thanks a lot for the patch.  I should have tackled that in
f68cd847fa40 but I've just lacked a combination of time and energy
while the original commit was already enough.

The code indentation was a bit incorrect, and I think that we should
also have tests to stress that the insertion of the new stats is
correct.  I have fixed the indentation, added some tests and improved
a couple of surrounding descriptions while on it.

I'm tempted to propose a separate improvement for the template of the
fixed-numbered stats.  We could do like pgstatfuncs.c where we use a
macro to define the routines of the counters, and have one function
for each counter incremented.  That's a separate refactoring, so I'll
propose that on a different thread.
--
Michael


signature.asc
Description: PGP signature

Some refactoring for fixed-numbered stats template in injection_points

[Michael Paquier]

Hi all,

While reading again the code of injection_stats_fixed.c that holds the
template for fixed-numbered pgstats, I got an idea to make the code a
bit more elegant.  Rather than using a single routine to increment the
counters, we could use a series of routines with its internals hidden
in a macro like in pgstatfuncs.c.

The point of a template is to be as elegant as possible, and this
is arguably a better style, still this comes down to personal taste
perhaps.  Please feel free to comment about the attached.

Thanks,
--
Michael
From caa28aecf2726ffd0b2161e56ac832a3c1590469 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Mon, 19 Aug 2024 09:23:28 +0900
Subject: [PATCH] injection_points: Make template for fixed-numbered stats more
 elegant

There is no functional change here, but the point of a template is to be
be as clear as possible as it should be a point of reference, and this
is arguably more elegant long-term.
---
 .../injection_points/injection_points.c   | 10 ++--
 .../injection_points/injection_stats.h| 10 ++--
 .../injection_points/injection_stats_fixed.c  | 55 ++-
 3 files changed, 40 insertions(+), 35 deletions(-)

diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 4e775c7ec6..d8cfde2bf5 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -297,7 +297,7 @@ injection_points_attach(PG_FUNCTION_ARGS)
 		condition.pid = MyProcPid;
 	}
 
-	pgstat_report_inj_fixed(1, 0, 0, 0, 0);
+	pgstat_report_inj_fixed_numattach(1);
 	InjectionPointAttach(name, "injection_points", function, &condition,
 		 sizeof(InjectionPointCondition));
 
@@ -329,7 +329,7 @@ injection_points_load(PG_FUNCTION_ARGS)
 	if (inj_state == NULL)
 		injection_init_shmem();
 
-	pgstat_report_inj_fixed(0, 0, 0, 0, 1);
+	pgstat_report_inj_fixed_numloaded(1);
 	INJECTION_POINT_LOAD(name);
 
 	PG_RETURN_VOID();
@@ -344,7 +344,7 @@ injection_points_run(PG_FUNCTION_ARGS)
 {
 	char	   *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
 
-	pgstat_report_inj_fixed(0, 0, 1, 0, 0);
+	pgstat_report_inj_fixed_numrun(1);
 	INJECTION_POINT(name);
 
 	PG_RETURN_VOID();
@@ -359,7 +359,7 @@ injection_points_cached(PG_FUNCTION_ARGS)
 {
 	char	   *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
 
-	pgstat_report_inj_fixed(0, 0, 0, 1, 0);
+	pgstat_report_inj_fixed_numcached(1);
 	INJECTION_POINT_CACHED(name);
 
 	PG_RETURN_VOID();
@@ -436,7 +436,7 @@ injection_points_detach(PG_FUNCTION_ARGS)
 {
 	char	   *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
 
-	pgstat_report_inj_fixed(0, 1, 0, 0, 0);
+	pgstat_report_inj_fixed_numdetach(1);
 	if (!InjectionPointDetach(name))
 		elog(ERROR, "could not detach injection point \"%s\"", name);
 
diff --git a/src/test/modules/injection_points/injection_stats.h b/src/test/modules/injection_points/injection_stats.h
index 126c110169..ff9fcde0e3 100644
--- a/src/test/modules/injection_points/injection_stats.h
+++ b/src/test/modules/injection_points/injection_stats.h
@@ -23,10 +23,10 @@ extern void pgstat_report_inj(const char *name);
 
 /* injection_stats_fixed.c */
 extern void pgstat_register_inj_fixed(void);
-extern void pgstat_report_inj_fixed(uint32 numattach,
-	uint32 numdetach,
-	uint32 numrun,
-	uint32 numcached,
-	uint32 numloaded);
+extern void pgstat_report_inj_fixed_numattach(uint32 numattach);
+extern void pgstat_report_inj_fixed_numdetach(uint32 numdetach);
+extern void pgstat_report_inj_fixed_numrun(uint32 numrun);
+extern void pgstat_report_inj_fixed_numcached(uint32 numcached);
+extern void pgstat_report_inj_fixed_numloaded(uint32 numloaded);
 
 #endif
diff --git a/src/test/modules/injection_points/injection_stats_fixed.c b/src/test/modules/injection_points/injection_stats_fixed.c
index 82b07e5332..269dcc428b 100644
--- a/src/test/modules/injection_points/injection_stats_fixed.c
+++ b/src/test/modules/injection_points/injection_stats_fixed.c
@@ -133,33 +133,38 @@ pgstat_register_inj_fixed(void)
 	inj_fixed_loaded = true;
 }
 
-/*
- * Report fixed number of statistics for an injection point.
- */
-void
-pgstat_report_inj_fixed(uint32 numattach,
-		uint32 numdetach,
-		uint32 numrun,
-		uint32 numcached,
-		uint32 numloaded)
-{
-	PgStatShared_InjectionPointFixed *stats_shmem;
-
-	/* leave if disabled */
-	if (!inj_fixed_loaded)
-		return;
-
-	stats_shmem = pgstat_get_custom_shmem_data(PGSTAT_KIND_INJECTION_FIXED);
-
-	pgstat_begin_changecount_write(&stats_shmem->changecount);
-	stats_shmem->stats.numattach += numattach;
-	stats_shmem->stats.numdetach += numdetach;
-	stats_shmem->stats.numrun += numrun;
-	stats_shmem->stats.numcached += numcached;
-	stats_shmem->stats.numloaded += numloaded;
-	pgstat_end_changecount_write(&stats_shmem->changecount);
+#define PGSTAT_REPORT_INJ_FIXED_COUNTER(numstat)\
+void			\
+CppConcat(pgstat_report_inj_fixed_,numstat)(uint32 numst

Re: Cirrus CI for macOS branches 16 and 15 broken

[Tom Lane]

I wrote:
> Interesting.  Now that I've finished "sudo port upgrade outdated",
> my laptop is back to a state where unprivileged "port outdated"
> is successful.

I confirmed on another machine that, immediately after "sudo port
selfupdate" from 2.9.3 to 2.10.1, I get

$ port outdated
sqlite error: attempt to write a readonly database (8) while executing query: 
CREATE INDEX registry.snapshot_file_id ON snapshot_files(id)

but if I do "sudo port outdated", I get the right thing:

$ sudo port outdated
The following installed ports are outdated:
bash   5.2.26_0 < 5.2.32_0   
bind9  9.18.27_0 < 9.20.0_3 
... etc etc ...

and then once I've done that, unprivileged "port outdated" works
again:

$ port outdated
The following installed ports are outdated:
bash   5.2.26_0 < 5.2.32_0   
bind9  9.18.27_0 < 9.20.0_3  
... yadda yadda ...

So there's definitely some behind-the-back updating going on there.
I'm not sure why the CI script should trigger that though.  It
does do a couple of "port" calls without "sudo", but not in places
where the state should be only partially upgraded.

regards, tom lane

Re: Cirrus CI for macOS branches 16 and 15 broken

[Thomas Munro]

On Mon, Aug 19, 2024 at 12:51 PM Tom Lane  wrote:
> I'm not sure why the CI script should trigger that though.  It
> does do a couple of "port" calls without "sudo", but not in places
> where the state should be only partially upgraded.

Oooh, I think I see where we missed a sudo:

if [ -n "$(port -q installed installed)" ] ; then

Re: Feature Request: Extending PostgreSQL's Identifier Length Limit

[Bruce Momjian]

On Thu, Jul 18, 2024 at 11:45:45AM +0200, Álvaro Herrera wrote:
> On 2024-Jul-18, David HJ wrote:
> 
> > As a long-time PostgreSQL user, I've increasingly run into issues with the
> > 63-byte limit for identifiers, particularly table names. This limit, while
> > historically sufficient, is becoming a significant pain point in modern
> > database design and usage.
> 
> This has been discussed before.  I think the latest discussion, and some
> preliminary proof-of-concept patches, were around here:
> 
> https://postgr.es/m/CAFBsxsF2V8n9w0SGK56bre3Mk9fzZS=9aaa8gfs_n+woa3d...@mail.gmail.com

FYI, the COMMENT ON command can help to document identifiers.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Cirrus CI for macOS branches 16 and 15 broken

[Tom Lane]

Thomas Munro  writes:
> On Mon, Aug 19, 2024 at 12:51 PM Tom Lane  wrote:
>> I'm not sure why the CI script should trigger that though.  It
>> does do a couple of "port" calls without "sudo", but not in places
>> where the state should be only partially upgraded.

> Oooh, I think I see where we missed a sudo:

> if [ -n "$(port -q installed installed)" ] ; then

I wondered about that too, but you should still have a plain 2.9.3
installation at that point.  AFAICT you'd only be at risk between

sudo port selfupdate
sudo port upgrade outdated

and there's nothing but a comment there.

regards, tom lane

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Michael Paquier]

On Sat, Aug 10, 2024 at 10:50:55AM -0700, Noah Misch wrote:
> On Sat, Jul 27, 2024 at 07:24:33AM +0900, Michael Paquier wrote:
>> I've just applied now the remaining pieces down to 17.
> 
> Comparing commit c9e2457 to the patch in zqgvzsbw5tgkq...@paquier.xyz, the
> commit lacks the slru.c portion.

And a portion of multixact.c as well, thanks!  I am pretty sure that I
have messed up with a `git add` while doing a rebase on this dev
branch.  I'll take care of it.
--
Michael


signature.asc
Description: PGP signature

Re: Logical Replication of sequences

[Peter Smith]

Here are my review comments for the latest patchset

v20240817-0001. No changes. No comments.
v20240817-0002. No changes. No comments.
v20240817-0003. See below.
v20240817-0004. See below.
v20240817-0005. No changes. No comments.

//

v20240817-0003 and 0004.

(This is a repeat of the same comment as in previous reviews, but lots
more functions seem affected now)

IIUC, the LR code tries to follow function naming conventions (e.g.
CamelCase/snake_case for exposed/static functions respectively),
intended to make the code more readable. But, this only works if the
conventions are followed.

Now, patches 0003 and 0004 are shuffling more and more functions
between modules while changing them from static to non-static (or vice
versa). So, the function name conventions are being violated many
times. IMO these functions ought to be renamed according to their new
modifiers to avoid the confusion caused by ignoring the name
conventions.

==
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Conflict detection and logging in logical replication

[shveta malik]

On Sun, Aug 18, 2024 at 2:27 PM Zhijie Hou (Fujitsu)
 wrote:
>
> Attach the V16 patch which addressed the comments we agreed on.
> I will add a doc patch to explain the log format after the 0001 is RFC.
>

Thank You for addressing comments. Please see this scenario:

create table tab1(pk int primary key, val1 int unique, val2 int);

pub: insert into tab1 values(1,1,1);
sub: insert into tab1 values(2,2,3);
pub: update tab1 set val1=2 where pk=1;

Wrong 'replica identity' column logged? shouldn't it be pk?

ERROR:  conflict detected on relation "public.tab1": conflict=update_exists
DETAIL:  Key already exists in unique index "tab1_val1_key", modified
locally in transaction 801 at 2024-08-19 08:50:47.974815+05:30.
Key (val1)=(2); existing local tuple (2, 2, 3); remote tuple (1, 2,
1); replica identity (val1)=(1).

thanks
Shveta

RE: Conflict detection and logging in logical replication

[Zhijie Hou (Fujitsu)]

On Friday, August 16, 2024 7:47 PM Michail Nikolaev 
  wrote:
> > I think you might misunderstand the behavior of CheckAndReportConflict(),
> > even if it found a conflict, it still inserts the tuple into the index which
> > means the change is anyway applied.
> 
> > In the above conditions where a concurrent tuple insertion is removed or
> > rolled back before CheckAndReportConflict, the tuple inserted by apply will
> > remain. There is no need to report anything in such cases as apply was
> > successful.
> 
> Yes, thank you for explanation, I was thinking UNIQUE_CHECK_PARTIAL works
> differently.
> 
> But now I think DirtySnapshot-related bug is a blocker for this feature then,
> I'll reply into original after rechecking it.

Based on your response in the original thread[1], where you confirmed that the
dirty snapshot bug does not impact the detection of insert_exists conflicts, I
assume we are in agreement that this bug is not a blocker for the detection
feature. If you think otherwise, please feel free to let me know.

[1] 
https://www.postgresql.org/message-id/CANtu0oh69b%2BVCiASX86dF_eY%3D9%3DA2RmMQ_%2B0%2BuxZ_Zir%2BoNhhw%40mail.gmail.com

Best Regards,
Hou zj

Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

[Robert Haas]

On Sat, Aug 17, 2024 at 5:32 AM Jelte Fennema-Nio  wrote:
> Not introducing new APIs definitely is useful to clients and the
> community. Before users can use a new API, their libpq wrapper needs
> to expose this new function that calls it through FFI. First of all
> this requires work from client authors.

Sure, just like they do every other new libpq function.

> But the **key point being**:
> The new function would not be present in old libpq versions. So for
> some clients, the FFI wrapper would also not exist for those old libpq
> versions, or at least would fail. So now users before actually being
> able to check for a minor protocol version, they first need an up to
> date libpq wrapper library for their language that exposes the new
> function, and then they'd still have to check their actual libpq
> version, before they could finally check for the minor protocol
> version...

I feel like what you're really complaining about here is that libpq is
not properly versioned. We've just been calling it libpq.so.5 forever
instead of bumping the version number when we change stuff. Maybe we
should start doing that, because that's exactly what version numbers
are for. Alternatively or in addition, maybe we should have a function
in libpq that returns its own PostgreSQL version, because that would
solve this problem for all cases, whereas what you're proposing here
only solves it for this particular case (and at the risk of breaking
things for somebody).

I just don't see why this particular change is special. We add new
libpq interfaces all the time and we don't do anything to make that
easy for libpq clients to discover. If we implemented longer cancel
keys or protocol parameters or transparent column encryption without a
protocol version bump, clients would still need to figure out that
those features were present in the libpq they are linked against, just
like they presumably already need to worry about whether they're
linked a new-enough libpq to have any other feature that's been added
since forever ago. Sure, that's not great, but it doesn't seem any
more not great in this case than any other, and I'd rather see us come
up with a nice general solution to that problem than hack this
specific case by redefining an existing function.

Also, I kind of wish you had brought this argument up earlier. Maybe
you did and I missed it, but I was under the impression that you were
just arguing that "nobody will notice or care," which is a quite
different argument than what you make here.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Document that PG_TRY block cannot have a return statement

[Xiaoran Wang]

LGTM!

Serpent  于2024年8月15日周四 15:01写道：

> Hi,
>
> What about this wording:
>
> The code that might throw ereport(ERROR) cannot contain any non local
> control flow other than ereport(ERROR) e.g.: return, goto, break, continue.
> In other words once PG_TRY() is executed, either PG_CATCH() or
> PG_FINALLY() must be executed as well.
>
> I used 'code that might throw ereport(ERROR)' for XXX since this is what's
> used earlier in the comment.
>
> On Tue, 12 Sept 2023 at 17:22, Tom Lane  wrote:
>
>> Serpent  writes:
>> > I'm talking about this part:
>>
>> > PG_TRY();
>> > {
>> >   ... code that might throw ereport(ERROR) ...
>> > }
>>
>> Ah.  Your phrasing needs work for clarity then.  Also, "return"
>> is hardly the only way to break it; break, continue, or goto
>> leading out of the PG_TRY are other possibilities.  Maybe more
>> like "The XXX code must exit normally (by control reaching
>> the end) if it does not throw ereport(ERROR)."  Not quite sure
>> what to use for XXX.
>>
>> regards, tom lane
>>
>

-- 
Best regards !
Xiaoran Wang

Re: Conflict detection and logging in logical replication

[shveta malik]

On Mon, Aug 19, 2024 at 9:07 AM shveta malik  wrote:
>
> On Sun, Aug 18, 2024 at 2:27 PM Zhijie Hou (Fujitsu)
>  wrote:
> >
> > Attach the V16 patch which addressed the comments we agreed on.
> > I will add a doc patch to explain the log format after the 0001 is RFC.
> >
>
> Thank You for addressing comments. Please see this scenario:
>
> create table tab1(pk int primary key, val1 int unique, val2 int);
>
> pub: insert into tab1 values(1,1,1);
> sub: insert into tab1 values(2,2,3);
> pub: update tab1 set val1=2 where pk=1;
>
> Wrong 'replica identity' column logged? shouldn't it be pk?
>
> ERROR:  conflict detected on relation "public.tab1": conflict=update_exists
> DETAIL:  Key already exists in unique index "tab1_val1_key", modified
> locally in transaction 801 at 2024-08-19 08:50:47.974815+05:30.
> Key (val1)=(2); existing local tuple (2, 2, 3); remote tuple (1, 2,
> 1); replica identity (val1)=(1).

Apart from this one, I have no further comments on v16.

thanks
Shveta

Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

[Tom Lane]

Robert Haas  writes:
> I feel like what you're really complaining about here is that libpq is
> not properly versioned. We've just been calling it libpq.so.5 forever
> instead of bumping the version number when we change stuff. Maybe we
> should start doing that, because that's exactly what version numbers
> are for. Alternatively or in addition, maybe we should have a function
> in libpq that returns its own PostgreSQL version, because that would
> solve this problem for all cases, whereas what you're proposing here
> only solves it for this particular case (and at the risk of breaking
> things for somebody).

Not really.  *No* runtime test is adequate for discovery of a new
library API, because if you try to call a function that doesn't exist
in the version you have, you will get a compile or link failure long
before you can call any inquiry function.

Bumping the .so's major version just creates another way to fail
at link time, so I'm not seeing how that would make this better.

> I just don't see why this particular change is special. We add new
> libpq interfaces all the time and we don't do anything to make that
> easy for libpq clients to discover.

Indeed.  But we have actually paid a little bit of attention to that,
in the form of inventing #define symbols that can be tested at compile
time.  (There's an open item for 17 concerning failure to do that for
some new-in-17 APIs.)  Yeah, it's grotty, but runtime checks aren't
especially relevant here.

In any case, please let us not abuse the wire protocol version number
as an indicator of the libpq-to-application API version.  They are
fundamentally different things.

regards, tom lane

Re: Improving the notation for ecpg.addons rules

[Michael Paquier]

On Sun, Aug 18, 2024 at 01:13:36PM -0400, Tom Lane wrote:
> While I've not done it in the attached, perhaps it would be
> but I think that might be a step too far.  IMO it's not adding much
> readability, and it seems like introducing an unnecessary dependency
> on exactly how the gram.y alternatives are laid out.

Not being too aggressive with the changes sounds like a good thing
here.

> BTW, the attached patch won't apply to HEAD, it's meant to apply
> after the patch series being discussed at [1].  So I won't stick
> this in the CF yet.
> 
> Thoughts?

Seeing changes like "stmtClosePortalStmt" changing to "stmt
ClosePortalStmt" is clearly an improvement in readability.
SignedIconstIconst was also fun.  Your change is a good idea.

It looks like %replace_line expects all its elements to have one space
between each token, still this is not enforced with a check across its
hardcoded elements?
--
Michael


signature.asc
Description: PGP signature

Re: Improving the notation for ecpg.addons rules

[Tom Lane]

Michael Paquier  writes:
> It looks like %replace_line expects all its elements to have one space
> between each token, still this is not enforced with a check across its
> hardcoded elements?

Yeah, I was wondering about that.  I wouldn't do it exactly like
that, but with a check that the entry gets matched somewhere.
The other patchset adds cross-checks that each ecpg.addons entry is
used exactly once, but there's no such check for these arrays that
are internal to parse.pl.  Can't quite decide if it's worth adding.

I debugged the patch in this thread by checking that the emitted
preproc.y didn't change, but that might not be helpful for changes
that are actually intended to change the grammar.

regards, tom lane

Re: Pgoutput not capturing the generated columns

[Peter Smith]

Hi, Here are my review comments for v27-0001.

==
contrib/test_decoding/expected/generated_columns.out
contrib/test_decoding/sql/generated_columns.sql

+-- By default, 'include-generated-columns' is enabled, so the values
for the generated column 'b' will be replicated even if it is not
explicitly specified.

nit - The "default" is only like this for "test_decoding" (e.g., the
CREATE SUBSCRIPTION option is the opposite), so let's make the comment
clearer about that.
nit - Use sentence case in the comments.

==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/contrib/test_decoding/expected/generated_columns.out 
b/contrib/test_decoding/expected/generated_columns.out
index 48f900f..9b03f6d 100644
--- a/contrib/test_decoding/expected/generated_columns.out
+++ b/contrib/test_decoding/expected/generated_columns.out
@@ -1,13 +1,14 @@
--- test decoding of generated columns
+-- Test decoding of generated columns.
 SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 
'test_decoding');
  ?column? 
 --
  init
 (1 row)
 
--- column b' is a generated column
+-- Column b' is a generated column.
 CREATE TABLE gencoltable (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
--- By default, 'include-generated-columns' is enabled, so the values for the 
generated column 'b' will be replicated even if it is not explicitly specified.
+-- For 'test_decoding' the parameter 'include-generated-columns' is enabled by 
default,
+-- so the values for the generated column 'b' will be replicated even if it is 
not explicitly specified.
 INSERT INTO gencoltable (a) VALUES (1);
 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 
'include-xids', '0', 'skip-empty-xacts', '1');
 data 
@@ -17,7 +18,7 @@ SELECT data FROM 
pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
  COMMIT
 (3 rows)
 
--- when 'include-generated-columns' is enabled, the values of the generated 
column 'b' will be replicated.
+-- When 'include-generated-columns' is enabled, the values of the generated 
column 'b' will be replicated.
 INSERT INTO gencoltable (a) VALUES (2);
 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 
'include-xids', '0', 'skip-empty-xacts', '1', 'include-generated-columns', '1');
 data 
@@ -27,7 +28,7 @@ SELECT data FROM 
pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
  COMMIT
 (3 rows)
 
--- when 'include-generated-columns' is disabled, the values of the generated 
column 'b' will not be replicated.
+-- When 'include-generated-columns' is disabled, the values of the generated 
column 'b' will not be replicated.
 INSERT INTO gencoltable (a) VALUES (3);
 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 
'include-xids', '0', 'skip-empty-xacts', '1', 'include-generated-columns', '0');
   data  
@@ -37,7 +38,7 @@ SELECT data FROM 
pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
  COMMIT
 (3 rows)
 
--- with REPLICA IDENTITY = FULL, to show old-key data includes generated 
columns data for updates.
+-- When REPLICA IDENTITY = FULL, show old-key data includes generated columns 
data for updates.
 ALTER TABLE gencoltable REPLICA IDENTITY FULL;
 UPDATE gencoltable SET a = 10;
 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 
'include-xids', '0', 'skip-empty-xacts', '1', 'include-generated-columns', '1');
diff --git a/contrib/test_decoding/sql/generated_columns.sql 
b/contrib/test_decoding/sql/generated_columns.sql
index fb156c2..7b455a1 100644
--- a/contrib/test_decoding/sql/generated_columns.sql
+++ b/contrib/test_decoding/sql/generated_columns.sql
@@ -1,23 +1,24 @@
--- test decoding of generated columns
+-- Test decoding of generated columns.
 
 SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 
'test_decoding');
 
--- column b' is a generated column
+-- Column b' is a generated column.
 CREATE TABLE gencoltable (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
 
--- By default, 'include-generated-columns' is enabled, so the values for the 
generated column 'b' will be replicated even if it is not explicitly specified.
+-- For 'test_decoding' the parameter 'include-generated-columns' is enabled by 
default,
+-- so the values for the generated column 'b' will be replicated even if it is 
not explicitly specified.
 INSERT INTO gencoltable (a) VALUES (1);
 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 
'include-xids', '0', 'skip-empty-xacts', '1');
 
--- when 'include-generated-columns' is enabled, the values of the generated 
column 'b' will be replicated.
+-- When 'include-generated-columns' is enabled, the values of the generated 
column 'b' will be replicated.
 INSERT INTO gencoltable (a) VALUES (2);
 SELECT data FROM pg_logical_slot_get_changes('regress

Re: Conflict detection and logging in logical replication

[Amit Kapila]

On Mon, Aug 19, 2024 at 9:08 AM shveta malik  wrote:
>
> On Sun, Aug 18, 2024 at 2:27 PM Zhijie Hou (Fujitsu)
>  wrote:
> >
> > Attach the V16 patch which addressed the comments we agreed on.
> > I will add a doc patch to explain the log format after the 0001 is RFC.
> >
>
> Thank You for addressing comments. Please see this scenario:
>
> create table tab1(pk int primary key, val1 int unique, val2 int);
>
> pub: insert into tab1 values(1,1,1);
> sub: insert into tab1 values(2,2,3);
> pub: update tab1 set val1=2 where pk=1;
>
> Wrong 'replica identity' column logged? shouldn't it be pk?
>
> ERROR:  conflict detected on relation "public.tab1": conflict=update_exists
> DETAIL:  Key already exists in unique index "tab1_val1_key", modified
> locally in transaction 801 at 2024-08-19 08:50:47.974815+05:30.
> Key (val1)=(2); existing local tuple (2, 2, 3); remote tuple (1, 2,
> 1); replica identity (val1)=(1).
>

The docs say that by default replica identity is primary_key [1] (see
REPLICA IDENTITY), [2] (see pg_class.relreplident). So, using the same
format to display PK seems reasonable. I don't think adding additional
code to distinguish these two cases in the LOG message is worth it. We
can always change such things later if that is what users and or
others prefer.

[1] - https://www.postgresql.org/docs/devel/sql-altertable.html
[2] - https://www.postgresql.org/docs/devel/catalog-pg-class.html

-- 
With Regards,
Amit Kapila.

Improve pg_re_throw: check if sigjmp_buf is valid and report error

[Xiaoran Wang]

https://www.postgresql.org/message-id/canncwrjtse6wkkus_y8wj2phvrvaqpxmk_qtepsf_+nvpyx...@mail.gmail.com

As the problem discussed in the above thread, I also run into that. Besides
updating the doc, I would like to report a error for it.

If the code in PG_TRY contains any non local control flow other than
ereport(ERROR) like goto, break etc., the PG_CATCH or PG_END_TRY cannot
be called, then the PG_exception_stack will point to the memory whose
stack frame has been released. So after that, when the pg_re_throw
called, __longjmp() will crash and report Segmentation fault error.

 In that case, to help developers to figure out the root cause easily, it is
 better to report that 'the sigjmp_buf is invalid' rather than letting
 the __longjmp report any error.

 Addition to sigjmp_buf, add another field 'int magic' which is next to
 the sigjum_buf in the local stack frame memory. The magic's value is always
 'PG_exception_magic 0x12345678'. And in 'pg_re_throw' routine, check if
 the magic's value is still '0x12345678', if not, that means the memory
 where the 'PG_exception_stack' points to has been released, and the
'sigbuf'
 must be invalid.

  The related code is in patch 0001

 --
 I'm not sure if it is necessary to add a regress test for it. In patch
0002,  to test the
 patch can work correctly, I have added a function 'pg_re_throw_crash' in
regress.c

 create function pg_re_throw_crash()
 RETURNS void
 AS :'regresslib', 'pg_re_throw_crash'
  LANGUAGE C STRICT STABLE PARALLEL SAFE;
 create above function and run 'select pg_re_throw_crash()', then will get
the error
'FATAL:  Invalid sigjum_buf, code in PG_TRY cannot contain any non local
control flow other than ereport'

-- 
Best regards !
Xiaoran Wang


0001-Imporve-pg_re_throw-check-if-sigjmp_buf-is-valid-and.patch
Description: Binary data


0002-Test-pg_re_throw-checking-invalid-sigjmp_buf.patch
Description: Binary data

Re: Apply PGDLLIMPORT markings to some GUC variables

[Michael Paquier]

On Tue, Aug 13, 2024 at 03:00:00PM -0400, Robert Haas wrote:
> This seems correct to me.

It is not the first time that this happens in recent history.  Would
it be worth automating that?  I would guess a TAP test that takes a
copy of the headers, applies the changes while filtering the few
exceptions, then compares it to the origin in the tree?
--
Michael


signature.asc
Description: PGP signature

Re: Create syscaches for pg_extension

[Michael Paquier]

On Tue, Aug 13, 2024 at 05:38:55PM +0300, Alexander Korotkov wrote:
> +1 from me too

I won't hide that I've wanted that in the past..
--
Michael


signature.asc
Description: PGP signature

Re: Conflict detection and logging in logical replication

[shveta malik]

On Mon, Aug 19, 2024 at 11:37 AM Amit Kapila  wrote:
>
> On Mon, Aug 19, 2024 at 9:08 AM shveta malik  wrote:
> >
> > On Sun, Aug 18, 2024 at 2:27 PM Zhijie Hou (Fujitsu)
> >  wrote:
> > >
> > > Attach the V16 patch which addressed the comments we agreed on.
> > > I will add a doc patch to explain the log format after the 0001 is RFC.
> > >
> >
> > Thank You for addressing comments. Please see this scenario:
> >
> > create table tab1(pk int primary key, val1 int unique, val2 int);
> >
> > pub: insert into tab1 values(1,1,1);
> > sub: insert into tab1 values(2,2,3);
> > pub: update tab1 set val1=2 where pk=1;
> >
> > Wrong 'replica identity' column logged? shouldn't it be pk?
> >
> > ERROR:  conflict detected on relation "public.tab1": conflict=update_exists
> > DETAIL:  Key already exists in unique index "tab1_val1_key", modified
> > locally in transaction 801 at 2024-08-19 08:50:47.974815+05:30.
> > Key (val1)=(2); existing local tuple (2, 2, 3); remote tuple (1, 2,
> > 1); replica identity (val1)=(1).
> >
>
> The docs say that by default replica identity is primary_key [1] (see
> REPLICA IDENTITY),

yes, I agree. But here the importance of dumping it was to know the
value of RI as well which is being used as an identification of row
being updated rather than row being conflicted. Value is logged
correctly.

>[2] (see pg_class.relreplident). So, using the same
> format to display PK seems reasonable. I don't think adding additional
> code to distinguish these two cases in the LOG message is worth it.

I don't see any additional code added for this case except getting an
existing logic being used for update_exists.

>We
> can always change such things later if that is what users and or
> others prefer.
>

Sure, if fixing this issue (where we are reporting the wrong col name)
needs additional logic, then I am okay to skip it for the time being.
We can address later if/when needed.

thanks
Shveta

Re: Normalize queries starting with SET for pg_stat_statements

[Michael Paquier]

On Tue, Aug 13, 2024 at 10:54:34AM -0400, Greg Sabino Mullane wrote:
> Now that I've spent some time away from this, I'm reconsidering why we are
> going through all the trouble of semi-jumbling SET statements. Maybe we
> just keep it simple and everything becomes "SET myvar = $1" or even "SET
> myvar" full stop?

Showing a dollar-character to show the fact that we have a value
behind makes the post sense to me.

> I'm having a hard time finding a real-world situation in
> which we need to distinguish different SET/RESET items within
> pg_stat_statements.

I'm -1 on keeping the distinction, and AFAIK it's not really different
with the underlying problems that we need to solve for SET TRANSACTION
and the kind, no? 

FWIW, I'm OK with hiding the value when it comes to a SET clause in a
CREATE FUNCTION.  We already hide the contents of SQL queries inside
the SQL functions when these are queries that can be normalized, so
there is a kind of thin argument for consistency, or something close
to that.
--
Michael


signature.asc
Description: PGP signature

Re: Thread-safe nl_langinfo() and localeconv()

[Thomas Munro]

Here is a slightly better version of patch 0003.  I removed some
unnecessary refactoring, making the patch smaller.

FTR I wrote a small program[1] for CI to test the assumptions about
Windows in 0001.  I printed out the addresses of the objects, to
confirm that different threads were looking at different objects once
the thread local mode was activated, and also assert that the struct
contents were as expected while 8 threads switched locales in a tight
loop, and the output[2] looked OK to me.

[1] 
https://github.com/macdice/hello-windows/blob/793eb2fe3e6738c200781f681a22a7e6358f39e5/test.c
[2] https://cirrus-ci.com/task/4650412253380608
From 1517f2a7372496c6e91d0369fb42ebed30a88c29 Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Tue, 13 Aug 2024 14:15:54 +1200
Subject: [PATCH v5 1/3] Provide thread-safe pg_localeconv_r().

This involves four different implementation strategies:

1.  For Windows, we now require _configthreadlocale() to be available
and work, and the documentation says that the object returned by
localeconv() is in thread-local memory.

2.  For glibc, we translate to nl_langinfo_l() calls, because it offers
the same information that way as an extension, and that API is
thread-safe.

3.  For macOS/*BSD, use localeconv_l(), which is thread-safe.

4.  For everything else, use uselocale() to set the locale for the
thread, and use a big ugly lock to defend against the returned object
being concurrently clobbered.  In practice this currently means only
Solaris.

The new call is used in pg_locale.c, replacing calls to setlocale() and
localeconv().

This patch adds a hard requirement on Windows' _configthreadlocale().
In the past there were said to be MinGW systems that didn't have it, or
had it but it didn't work.  As far as I can tell, CI (optional MinGW
task + mingw cross build warning task) and build farm (fairywren msys)
should be happy with this.  Fingers crossed.  (There are places that use
configure checks for that in ECPG; other proposed patches would remove
those later.)

Reviewed-by: Heikki Linnakangas 
Discussion: https://postgr.es/m/CA%2BhUKGJqVe0%2BPv9dvC9dSums_PXxGo9SWcxYAMBguWJUGbWz-A%40mail.gmail.com
---
 configure |   2 +-
 configure.ac  |   1 +
 meson.build   |   1 +
 src/backend/utils/adt/pg_locale.c | 128 ++-
 src/include/pg_config.h.in|   3 +
 src/include/port.h|   6 +
 src/port/Makefile |   1 +
 src/port/meson.build  |   1 +
 src/port/pg_localeconv_r.c| 367 ++
 9 files changed, 402 insertions(+), 108 deletions(-)
 create mode 100644 src/port/pg_localeconv_r.c

diff --git a/configure b/configure
index 2abbeb27944..60dcf1e436e 100755
--- a/configure
+++ b/configure
@@ -15232,7 +15232,7 @@ fi
 LIBS_including_readline="$LIBS"
 LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
 
-for ac_func in backtrace_symbols copyfile copy_file_range getifaddrs getpeerucred inet_pton kqueue mbstowcs_l memset_s posix_fallocate ppoll pthread_is_threaded_np setproctitle setproctitle_fast strchrnul strsignal syncfs sync_file_range uselocale wcstombs_l
+for ac_func in backtrace_symbols copyfile copy_file_range getifaddrs getpeerucred inet_pton localeconv_l kqueue mbstowcs_l memset_s posix_fallocate ppoll pthread_is_threaded_np setproctitle setproctitle_fast strchrnul strsignal syncfs sync_file_range uselocale wcstombs_l
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/configure.ac b/configure.ac
index c46ed2c591a..59e51a74629 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1735,6 +1735,7 @@ AC_CHECK_FUNCS(m4_normalize([
 	getifaddrs
 	getpeerucred
 	inet_pton
+	localeconv_l
 	kqueue
 	mbstowcs_l
 	memset_s
diff --git a/meson.build b/meson.build
index cd711c6d018..028a14547aa 100644
--- a/meson.build
+++ b/meson.build
@@ -2675,6 +2675,7 @@ func_checks = [
   ['inet_aton'],
   ['inet_pton'],
   ['kqueue'],
+  ['localeconv_l'],
   ['mbstowcs_l'],
   ['memset_s'],
   ['mkdtemp'],
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index cd3661e7279..dd4ba9e0e89 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -543,12 +543,8 @@ PGLC_localeconv(void)
 	static struct lconv CurrentLocaleConv;
 	static bool CurrentLocaleConvAllocated = false;
 	struct lconv *extlconv;
-	struct lconv worklconv;
-	char	   *save_lc_monetary;
-	char	   *save_lc_numeric;
-#ifdef WIN32
-	char	   *save_lc_ctype;
-#endif
+	struct lconv tmp;
+	struct lconv worklconv = {0};
 
 	/* Did we do it already? */
 	if (CurrentLocaleConvValid)
@@ -562,77 +558,21 @@ PGLC_localeconv(void)
 	}
 
 	/*
-	 * This is tricky because we really don't want to risk throwing error
-	 * while the locale is set to other than our usual settings.  Therefore,
-	 * the process is: collect the usual settings, set locale to special
-	 *

Re: Use read streams in pg_visibility

[Michael Paquier]

On Tue, Aug 13, 2024 at 03:22:27PM +0300, Nazir Bilal Yavuz wrote:
> Hi,
> 
> I am working on using the read stream in pg_visibility. There are two
> places to use it:
> 
> 1- collect_visibility_data()
> 
> This one is straightforward. I created a read stream object if
> 'include_pd' is true because read I/O is done when 'include_pd' is
> true. There is ~4% timing improvement with this change. I started the
> server with the default settings and created a 6 GB table. Then run
> 100 times pg_visibility() by clearing the OS cache between each run.
> --

Mind sharing a script for reproducibility?  Except for the drop_caches
part, of course..  
--
Michael


signature.asc
Description: PGP signature

Re: Fsync (flush) all inserted WAL records

[Michael Paquier]

On Wed, Aug 07, 2024 at 06:00:45PM +0300, Aleksander Alekseev wrote:
> Assuming the function has value, as you claim, I see no reason not to
> expose it similarly to pg_current_wal_*(). On top of that you will
> have to test-cover it anyway. The easiest way to do it will be to have
> an SQL-wrapper.

I cannot be absolutely without seeing a patch, but adding SQL
functions in this area is usually very useful for monitoring purposes
of external solutions.
--
Michael


signature.asc
Description: PGP signature

Re: Conflict detection and logging in logical replication

[Amit Kapila]

On Mon, Aug 19, 2024 at 11:54 AM shveta malik  wrote:
>
> On Mon, Aug 19, 2024 at 11:37 AM Amit Kapila  wrote:
> >
> > On Mon, Aug 19, 2024 at 9:08 AM shveta malik  wrote:
> > >
> > > On Sun, Aug 18, 2024 at 2:27 PM Zhijie Hou (Fujitsu)
> > >  wrote:
> > > >
> > > > Attach the V16 patch which addressed the comments we agreed on.
> > > > I will add a doc patch to explain the log format after the 0001 is RFC.
> > > >
> > >
> > > Thank You for addressing comments. Please see this scenario:
> > >
> > > create table tab1(pk int primary key, val1 int unique, val2 int);
> > >
> > > pub: insert into tab1 values(1,1,1);
> > > sub: insert into tab1 values(2,2,3);
> > > pub: update tab1 set val1=2 where pk=1;
> > >
> > > Wrong 'replica identity' column logged? shouldn't it be pk?
> > >
> > > ERROR:  conflict detected on relation "public.tab1": 
> > > conflict=update_exists
> > > DETAIL:  Key already exists in unique index "tab1_val1_key", modified
> > > locally in transaction 801 at 2024-08-19 08:50:47.974815+05:30.
> > > Key (val1)=(2); existing local tuple (2, 2, 3); remote tuple (1, 2,
> > > 1); replica identity (val1)=(1).
> > >
> >
> > The docs say that by default replica identity is primary_key [1] (see
> > REPLICA IDENTITY),
>
> yes, I agree. But here the importance of dumping it was to know the
> value of RI as well which is being used as an identification of row
> being updated rather than row being conflicted. Value is logged
> correctly.
>

Agreed, sorry, I misunderstood the problem reported. I thought the
suggestion was to use 'primary key' instead of 'replica identity' but
you pointed out that the column used in 'replica identity' was wrong.
We should fix this one.

-- 
With Regards,
Amit Kapila.

Re: Speed up Hash Join by teaching ExprState about hashing

[David Rowley]

Thanks for having a look.

On Sat, 17 Aug 2024 at 23:21, Tels  wrote:
> Is it nec. to rotate existing_hash here before checking for isnull?
> Because in case of isnull, isn't the result of the rotate thrown away?

Yeah, I think that it's worthwhile moving that to after the isnull
check so as not to waste the effort.  Unfortunately doing the same in
the JIT code meant copying and pasting the code that emits the
rotation code.

The attached v5 patch includes this change.

David


v5-0001-Speed-up-Hash-Join-by-making-ExprStates-support-h.patch
Description: Binary data

Re: Refactoring postmaster's code to cleanup after child exit

[Heikki Linnakangas]

On 18/08/2024 11:00, Alexander Lakhin wrote:

10.08.2024 00:13, Heikki Linnakangas wrote:


Committed the patches up to and including this one, with tiny comment changes.


I've noticed that on the current HEAD server.log contains binary data
(an invalid process name) after a child crash. For example, while playing
with -ftapv, I've got:
SELECT to_date('2024 613566758 1', 'IYYY IW ID');
server closed the connection unexpectedly

grep -a 'was terminated' server.log
2024-08-18 07:07:06.482 UTC|||66c19d96.3482f6|LOG:  `�!x� (PID 3441407) was 
terminated by signal 6: Aborted

It looks like this was introduced by commit 28a520c0b (IIUC, namebuf in
CleanupBackend() may stay uninitialized in some code paths).


Fixed, thanks!

--
Heikki Linnakangas
Neon (https://neon.tech)