Re: [PoC] pg_upgrade: allow to upgrade publisher node

[Amit Kapila]

On Fri, Aug 11, 2023 at 10:43 AM Julien Rouhaud  wrote:
>
> On Thu, Aug 10, 2023 at 04:30:40PM +0900, Masahiko Sawada wrote:
> > On Thu, Aug 10, 2023 at 2:27 PM Amit Kapila  wrote:
> > >
> > > Sawada-San, Julien, and others, do you have any thoughts on the above 
> > > point?
> >
> > IIUC during the old cluster running in the middle of pg_upgrade it
> > doesn't accept TCP connections. I'm not sure we need to worry about
> > the case where someone in the same server attempts to create
> > replication slots during the upgrade.
>
> AFAICS this is only true for non-Windows platform, so we would still need some
> extra safeguards on Windows.  Having those on all platforms will probably be
> simpler and won't hurt otherwise.
>
> > The same is true for other objects, as Amit mentioned.
>
> I disagree.  As I mentioned before any module registered in
> shared_preload_libraries can spawn background workers which can perform any
> activity.  There were previous reports of corruption because of multi-xact
> being generated by such bgworkers during pg_upgrade, I'm pretty sure that 
> there
> are some modules that create objects (automatic partitioning tools for
> instance).  It's also unclear to me what would happen if some writes are
> performed by such module at various points of the pg_upgrade process.  
> Couldn't
> that lead to either data loss or broken slot (as it couldn't stream changes
> from older major version)?
>

It won't be any bad than what can happen to tables. If we know that
such bgworkers can cause corruption if they do writes during the
upgrade, I don't think it is the job of this patch to prevent the
related scenarios. We can probably disallow the creation of new slots
during the binary upgrade but that also I am not sure. I guess it
would be better to document such hazards as a first step and then
probably write a patch to prevent WAL writes or something along those
lines.

-- 
With Regards,
Amit Kapila.

Re: Inconsistent results with libc sorting on Windows

[Noah Misch]

On Wed, Jun 14, 2023 at 12:50:28PM +0200, Juan José Santamaría Flecha wrote:
> On Wed, Jun 14, 2023 at 4:13 AM Peter Geoghegan  wrote:
> 
> > On Tue, Jun 13, 2023 at 5:59 PM Thomas Munro 
> > wrote:
> > > Trying to follow along here... you're doing the moral equivalent of
> > > strxfrm(), so sort keys have the transitive property but direct string
> > > comparisons don't?  Or is this because LCIDs reach a different
> > > algorithm somehow (or otherwise why do you need to use LCIDs for this,
> > > when there is a non-LCID version of that function, with a warning not
> > > to use the older LCID version[1]?)
> >
> > I'm reminded of the fact that the abbreviated keys strxfrm() debacle
> > (back when 9.5 was released) was caused by a bug in strcoll() -- not a
> > bug in strxfrm() itself. From our point of view the problem was that
> > strxfrm() failed to be bug compatible with strcoll() due to a buggy
> > strcoll() optimization.
> >
> > I believe that strxfrm() is generally less likely to have bugs than
> > strcoll(). There are far fewer opportunities to dodge unnecessary work
> > in the case of strxfrm()-like algorithms (offering something like
> > ICU's pg_strnxfrm_prefix_icu() prefix optimization is the only one).
> > On the other hand, collation library implementers are likely to
> > heavily optimize strcoll() for typical use-cases such as sorting and
> > binary search. Using strxfrm() for everything is discouraged [1].
> >
> 
> Yes, I think the situation is quite similar to what you describe, with its
> WIN32 peculiarities. Take for example the attached program, it'll output:
> 
> s1 = s2
> s2 = s3
> s1 > s3
> c1 > c2
> c2 > c3
> c1 > c3
> 
> As you can see the test for CompareStringEx() is broken, but we get a sane
> answer with LCMapStringEx().

The LCMapStringEx() solution is elegant.  I do see
https://learn.microsoft.com/en-us/windows/win32/intl/handling-sorting-in-your-applications
says, "If an application calls the function to create a sort key for a string
containing an Arabic kashida, the function creates no sort key value."  That's
aggravating.

Re: [PoC] pg_upgrade: allow to upgrade publisher node

[Amit Kapila]

On Thu, Aug 10, 2023 at 7:07 PM Masahiko Sawada  wrote:
>
> On Thu, Aug 10, 2023 at 12:52 PM Amit Kapila  wrote:
> >
> >
> > Are you suggesting doing this before we start the old cluster or after
> > we stop the old cluster? I was thinking about the pros and cons of
> > doing this check when the server is 'on' (along with other upgrade
> > checks something like the patch is doing now) versus when the server
> > is 'off'. I think the advantage of doing it when the server is 'off'
> > (after check_and_dump_old_cluster()) is that it will be ensured that
> > there is no extra WAL that could be generated during the upgrade and
> > has not been verified against confirmed_flush_lsn location. But OTOH,
> > to retrieve slot information when the server is 'off', we need a
> > separate utility or probably a functionality for the same in
> > pg_upgrade and also some WAL reading stuff which sounds to me like a
> > larger change that may not be warranted here. I think anyway the extra
> > WAL (if any got generated during the upgrade) won't be required after
> > the upgrade so not convinced to make such a check while the server is
> > 'off'. Are there reasons which make it better to do this while the old
> > cluster is 'off'?
>
> What I imagined is that we do this check before
> check_and_dump_old_cluster() while the server is 'off'. Reading the
> slot state file would be simple and I guess we would not need a tool
> or cli program for that. We need to expose RepliactionSlotOnDisk,
> though.
>

Won't that require a lot of version-specific checks as across versions
the file format could be different? For the case of the control file,
we use version-specific pg_controldata (for the old cluster, the
corresponding version's pg_controldata) utility to read the old
version control file. I thought we need something similar here if we
want to do what you are suggesting.

>
> After reading the control file and the slots' state files we
> check if slot's confirmed_flush_lsn matches the latest checkpoint LSN
> in the control file (BTW maybe we can get slot name and plugin name
> here instead of using pg_dump?).

But isn't the advantage of doing via pg_dump (in binary_mode) that we
allow some outside core in-place upgrade tool to also use it if
required? If we don't think that would be required then we can
probably use the info we retrieve it in pg_upgrade.

>
> the first commit. Or another idea would be to allow users to mark
> replication slots "upgradable" so that pg_upgrade skips the
> confirmed_flush_lsn check.
>

I guess for that we need to ask users to ensure that confirm_flush_lsn
is up-to-date and then provide some slot-level API to mark the slots
with the required status. If so, that sounds a bit complicated for
users.

-- 
With Regards,
Amit Kapila.

Re: [PoC] pg_upgrade: allow to upgrade publisher node

[Julien Rouhaud]

Hi,

On Thu, Aug 10, 2023 at 04:30:40PM +0900, Masahiko Sawada wrote:
> On Thu, Aug 10, 2023 at 2:27 PM Amit Kapila  wrote:
> >
> > Sawada-San, Julien, and others, do you have any thoughts on the above point?
>
> IIUC during the old cluster running in the middle of pg_upgrade it
> doesn't accept TCP connections. I'm not sure we need to worry about
> the case where someone in the same server attempts to create
> replication slots during the upgrade.

AFAICS this is only true for non-Windows platform, so we would still need some
extra safeguards on Windows.  Having those on all platforms will probably be
simpler and won't hurt otherwise.

> The same is true for other objects, as Amit mentioned.

I disagree.  As I mentioned before any module registered in
shared_preload_libraries can spawn background workers which can perform any
activity.  There were previous reports of corruption because of multi-xact
being generated by such bgworkers during pg_upgrade, I'm pretty sure that there
are some modules that create objects (automatic partitioning tools for
instance).  It's also unclear to me what would happen if some writes are
performed by such module at various points of the pg_upgrade process.  Couldn't
that lead to either data loss or broken slot (as it couldn't stream changes
from older major version)?

Re: Report planning memory in EXPLAIN ANALYZE

[Andrey Lepikhov]

On 10/8/2023 15:33, Ashutosh Bapat wrote:

On Wed, Aug 9, 2023 at 8:56 PM David Rowley  wrote:


On Thu, 10 Aug 2023 at 03:12, Ashutosh Bapat
 wrote:
I guess it depends on the problem you're trying to solve. I had
thought you were trying to do some work to reduce the memory used by
the planner, so I imagined you wanted this so you could track your
progress and also to help ensure we don't make too many mistakes in
the future which makes memory consumption worse again.
Another way we might go about reducing planner memory is to make
changes to the allocators themselves.  For example, aset rounds up to
the next power of 2.  If we decided to do something like add more
freelists to double the number so we could add a mid-way point
freelist between each power of 2, then we might find it reduces the
planner memory by, say 12.5%.  If we just tracked what was consumed by
palloc() that would appear to save us nothing, but it might actually
save us several malloced blocks.



This won't just affect planner but every subsystem of PostgreSQL, not
just planner, unless we device a new context type for planner.

My point is what's relevant here is how much net memory planner asked
for. Internally how much memory PostgreSQL allocated seems relevant
for the memory context infra as such but not so much for planner
alone.

If you think that memory allocated is important, I will add both used
and (net) allocated memory to EXPLAIN output.
As a developer, I like having as much internal info in my EXPLAIN as 
possible. But this command is designed mainly for users, not core 
developers. The size of memory allocated usually doesn't make sense for 
users. On the other hand, developers can watch it easily in different 
ways, if needed. So, I vote for the 'memory used' metric only.


The patch looks good, passes the tests.

--
regards,
Andrey Lepikhov
Postgres Professional

Re: Cirrus-ci is lowering free CI cycles - what to do with cfbot, etc?

[Noah Misch]

On Tue, Aug 08, 2023 at 07:59:55PM -0700, Andres Freund wrote:
> On 2023-08-08 22:29:50 -0400, Robert Treat wrote:

> 3) using ~350 USD / mo in GCP costs for windows, linux, freebsd (*)

> > The other likely option would be to seek out cloud credits

> I tried to start that progress within microsoft, fwiw.  Perhaps Joe and
> Jonathan know how to start within AWS?  And perhaps Noah inside GCP?
> 
> It'd be the least work to get it up and running in GCP, as it's already
> running there

I'm looking at this.  Thanks for bringing it to my attention.

Re: [PATCH] Add loongarch native checksum implementation.

[Amit Kapila]

On Thu, Aug 10, 2023 at 5:07 PM John Naylor
 wrote:
>
> On Thu, Aug 10, 2023 at 5:54 PM Michael Paquier  wrote:
> >
> > On Thu, Aug 10, 2023 at 03:56:37PM +0530, Amit Kapila wrote:
> > > In MSVC build, on doing: perl mkvcbuild.pl after this commit, I am
> > > facing the below error:
> > > Generating configuration headers...
> > > undefined symbol: USE_LOONGARCH_CRC32C at src/include/pg_config.h line
> > > 718 at ../postgresql/src/tools/msvc/Mkvcbuild.pm line 872.
> > >
> > > Am I missing something or did the commit miss something?
> >
> > Yes, the commit has missed the addition of USE_LOONGARCH_CRC32C in
> > Solution.pm.  If you want to be consistent with pg_config.h.in, you
> > could add it just after USE_LLVM, for instance.
>
> Oops, fixing now...
>

It is fixed now. Thanks!

-- 
With Regards,
Amit Kapila.

Simplify create_merge_append_path a bit for clarity

[Richard Guo]

As explained in the comments for generate_orderedappend_paths, we don't
currently support parameterized MergeAppend paths, and it doesn't seem
like going to change anytime soon.  Based on that,  we could simplify
create_merge_append_path a bit, such as set param_info to NULL directly
rather than call get_appendrel_parampathinfo() for it.  We already have
an Assert on that in create_merge_append_plan.

I understand that the change would not make any difference for
performance, it's just for clarity's sake.

Thanks
Richard


v1-0001-Simplify-create_merge_append_path-a-bit-for-clarity.patch
Description: Binary data

Re: 2023-08-10 release announcement draft

[Donghang Lin]

Hi,

Sorry, join late after the release note is out.

>  * An out-of-memory error from JIT will now cause a PostgreSQL `FATAL`
error instead of a C++ exception.

I assume this statement is related to this fix[1].
I think an OOM error from JIT causing a PostgreSQL `FATAL` error is the
actual behavior before the fix.
What the fix does is to bring exceptions back to the c++ world when OOM
happens outside of LLVM.

Thanks,
Donghang Lin

[1]
https://github.com/postgres/postgres/blame/accf4f84887eb8b53978a0dbf9cb5656e1779fcb/doc/src/sgml/release-15.sgml#L488-L509



On Wed, Aug 9, 2023 at 7:10 PM Jonathan S. Katz 
wrote:

> On 8/8/23 11:13 PM, Robert Treat wrote:
>
> > "Users who have skipped one or more update releases may need to run
> > additional, post-update steps; "
> >
> > The comma should be removed.
> >
> > "please see the release notes for earlier versions for details."
> >
> > Use of 'for' twice is grammatically incorrect; I am partial to "please
> > see the release notes from earlier versions for details." but could
> > also see "please see the release notes for earlier versions to get
> > details."
>
> Interestingly, I think this language has been unmodified for awhile.
> Upon reading it, I agree, and took your suggestions.
>
> Thanks,
>
> Jonathan
>
>

Re: [PoC] pg_upgrade: allow to upgrade publisher node

[Bruce Momjian]

On Thu, Aug 10, 2023 at 10:37:04PM +0900, Masahiko Sawada wrote:
> On Thu, Aug 10, 2023 at 12:52 PM Amit Kapila  wrote:
> > Are you suggesting doing this before we start the old cluster or after
> > we stop the old cluster? I was thinking about the pros and cons of
> > doing this check when the server is 'on' (along with other upgrade
> > checks something like the patch is doing now) versus when the server
> > is 'off'. I think the advantage of doing it when the server is 'off'
> > (after check_and_dump_old_cluster()) is that it will be ensured that
> > there is no extra WAL that could be generated during the upgrade and
> > has not been verified against confirmed_flush_lsn location. But OTOH,
> > to retrieve slot information when the server is 'off', we need a
> > separate utility or probably a functionality for the same in
> > pg_upgrade and also some WAL reading stuff which sounds to me like a
> > larger change that may not be warranted here. I think anyway the extra
> > WAL (if any got generated during the upgrade) won't be required after
> > the upgrade so not convinced to make such a check while the server is
> > 'off'. Are there reasons which make it better to do this while the old
> > cluster is 'off'?
> 
> What I imagined is that we do this check before
> check_and_dump_old_cluster() while the server is 'off'. Reading the
> slot state file would be simple and I guess we would not need a tool
> or cli program for that.

Agreed.

> BTW this check would not be able to support live-check but I think
> it's not a problem as this check with a running server will never be
> able to pass.

Agreed.

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

  Only you can decide what is important to you.

Re: Add PG CI to older PG releases

[Andres Freund]

Hi,

On 2023-08-10 18:09:03 -0400, Tom Lane wrote:
> Nazir Bilal Yavuz  writes:
> > PG CI is added starting from PG 15, adding PG CI to PG 14 and below
> > could be beneficial. So, firstly I tried adding it to the
> > REL_14_STABLE branch. If that makes sense, I will try to add PG CI to
> > other old PG releases.
> 
> I'm not actually sure this is worth spending time on.  We don't
> do new development in three-release-back branches, nor do we have
> so many spare CI cycles that we can routinely run CI testing in
> those branches [1].

I find it much less stressful to backpatch if I can test the patches via CI
first.  I don't think I'm alone in that - Alvaro for example has previously
done this in a more limited fashion (no windows):
https://postgr.es/m/20220928192226.4c6zeenujaoqq4bq%40alvherre.pgsql

Greetings,

Andres Freund

Re: Add PG CI to older PG releases

[Andres Freund]

Hi,

On 2023-08-10 19:55:15 +0300, Nazir Bilal Yavuz wrote:
> v2-0001-windows-Only-consider-us-to-be-running-as-service.patch is an
> older commit (59751ae47fd43add30350a4258773537e98d4063). A couple of
> tests were failing without this because the log file was empty and the
> tests were comparing strings with the log files (like in the first
> mail).

Hm. I'm a bit worried about backpatching this one, it's not a small
behavioural change. But the prior behaviour is pretty awful and could very
justifiably be viewed as a bug.

At the very least this would need to be combined with

commit 950e64fa46b164df87b5eb7c6e15213ab9880f87
Author: Andres Freund 
Date:   2022-07-18 17:06:34 -0700

Use STDOUT/STDERR_FILENO in most of syslogger.

Greetings,

Andres Freund

Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication

[Peter Smith]

On Fri, Aug 11, 2023 at 12:54 AM Melih Mutlu  wrote:
>
> Hi Peter and Vignesh,
>
> Peter Smith , 7 Ağu 2023 Pzt, 09:25 tarihinde şunu 
> yazdı:
>>
>> Hi Melih.
>>
>> Now that the design#1 ERRORs have been fixed, we returned to doing
>> performance measuring of the design#1 patch versus HEAD.
>
>
> Thanks a lot for taking the time to benchmark the patch. It's really helpful.
>
>> Publisher "busy" table does commit every 1000 inserts:
>> 2w 4w 8w 16w
>> HEAD 11898 5855 1868 1631
>> HEAD+v24-0002 21905 8254 3531 1626
>> %improvement -84% -41% -89% 0%
>>
>>
>> ^ Note - design#1 was slower than HEAD here
>>
>>
>> ~
>>
>>
>> Publisher "busy" table does commit every 2000 inserts:
>> 2w 4w 8w 16w
>> HEAD 21740 7109 3454 1703
>> HEAD+v24-0002 21585 10877 4779 2293
>> %improvement 1% -53% -38% -35%
>
>
> I assume you meant HEAD+v26-0002 and not v24. I wanted to quickly reproduce 
> these two cases where the patch was significantly worse. Interestingly my 
> results are a bit different than yours.
>

No, I meant what I wrote there. When I ran the tests the HEAD included
the v25-0001 refactoring patch, but v26 did not yet exist.

For now, we are only performance testing the first
"Reuse-Tablesyc-Workers" patch, but not yet including the second patch
("Reuse connection when...").

Note that those "Reuse-Tablesyc-Workers" patches v24-0002 and v26-0001
are equivalent because there are only cosmetic log message differences
between them.
So, my testing was with HEAD+v24-0002 (but not including v24-0003).
Your same testing should be with HEAD+v26-0001 (but not including v26-0002).

--
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: obtaining proc oid given a oper id

[CK Tan]

Found it.

/*
 * get_opcode
 *
 *  Returns the regproc id of the routine used to implement an
 *  operator given the operator oid.
 */
RegProcedure
get_opcode(Oid opno)

On Thu, Aug 10, 2023 at 1:17 PM CK Tan  wrote:
>
> Hi Hackers, is there a function that would lookup the proc that
> implements an operator?
>
> Thanks,
> -cktan

Re: Add PG CI to older PG releases

[Tom Lane]

Nazir Bilal Yavuz  writes:
> PG CI is added starting from PG 15, adding PG CI to PG 14 and below
> could be beneficial. So, firstly I tried adding it to the
> REL_14_STABLE branch. If that makes sense, I will try to add PG CI to
> other old PG releases.

I'm not actually sure this is worth spending time on.  We don't
do new development in three-release-back branches, nor do we have
so many spare CI cycles that we can routinely run CI testing in
those branches [1].

regards, tom lane

[1] 
https://www.postgresql.org/message-id/flat/20230808021541.7lbzdefvma7qmn3w%40awork3.anarazel.de

obtaining proc oid given a oper id

[CK Tan]

Hi Hackers, is there a function that would lookup the proc that
implements an operator?

Thanks,
-cktan

[PATCH] Support static linking against LLVM

[Marcelo Juchem]

By default, PostgreSQL doesn't explicitly choose whether to link
statically or dynamically against LLVM when LLVM JIT is enabled (e.g.:
`./configure --with-llvm`).

`llvm-config` will choose to dynamically link by default.

In order to statically link, one must pass `--link-static` to
`llvm-config` when listing linker flags (`--ldflags`) and libraries
(`--libs`).

This patch enables custom flags to be passed to `llvm-config` linker
related invocations through the environment variable
`LLVM_CONFIG_LINK_ARGS`.

To statically link against LLVM it suffices, then, to call `configure`
with environment variable `LLVM_CONFIG_LINK_ARGS=--link-static`.
---
 config/llvm.m4 | 5 +++--
 configure  | 6 --
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/config/llvm.m4 b/config/llvm.m4
index 3a75cd8b4d..712bd3de6c 100644
--- a/config/llvm.m4
+++ b/config/llvm.m4
@@ -13,6 +13,7 @@ AC_DEFUN([PGAC_LLVM_SUPPORT],
   AC_REQUIRE([AC_PROG_AWK])
 
   AC_ARG_VAR(LLVM_CONFIG, [path to llvm-config command])
+  AC_ARG_VAR(LLVM_CONFIG_LINK_ARGS, [extra arguments for llvm-config linker 
related flags])
   PGAC_PATH_PROGS(LLVM_CONFIG, llvm-config llvm-config-7 llvm-config-6.0 
llvm-config-5.0 llvm-config-4.0 llvm-config-3.9)
 
   # no point continuing if llvm wasn't found
@@ -52,7 +53,7 @@ AC_DEFUN([PGAC_LLVM_SUPPORT],
 esac
   done
 
-  for pgac_option in `$LLVM_CONFIG --ldflags`; do
+  for pgac_option in `$LLVM_CONFIG --ldflags $LLVM_CONFIG_LINK_ARGS`; do
 case $pgac_option in
   -L*) LDFLAGS="$LDFLAGS $pgac_option";;
 esac
@@ -84,7 +85,7 @@ AC_DEFUN([PGAC_LLVM_SUPPORT],
   # And then get the libraries that need to be linked in for the
   # selected components.  They're large libraries, we only want to
   # link them into the LLVM using shared library.
-  for pgac_option in `$LLVM_CONFIG --libs --system-libs $pgac_components`; do
+  for pgac_option in `$LLVM_CONFIG --libs --system-libs $LLVM_CONFIG_LINK_ARGS 
$pgac_components`; do
 case $pgac_option in
   -l*) LLVM_LIBS="$LLVM_LIBS $pgac_option";;
 esac
diff --git a/configure b/configure
index 86ffccb1ee..974b7f2d4e 100755
--- a/configure
+++ b/configure
@@ -1595,6 +1595,8 @@ Some influential environment variables:
   CXX C++ compiler command
   CXXFLAGSC++ compiler flags
   LLVM_CONFIG path to llvm-config command
+  LLVM_CONFIG_LINK_ARGS
+  extra arguments for llvm-config linker related flags
   CLANG   path to clang compiler to generate bitcode
   CPP C preprocessor
   PKG_CONFIG  path to pkg-config utility
@@ -5200,7 +5202,7 @@ fi
 esac
   done
 
-  for pgac_option in `$LLVM_CONFIG --ldflags`; do
+  for pgac_option in `$LLVM_CONFIG --ldflags $LLVM_CONFIG_LINK_ARGS`; do
 case $pgac_option in
   -L*) LDFLAGS="$LDFLAGS $pgac_option";;
 esac
@@ -5232,7 +5234,7 @@ fi
   # And then get the libraries that need to be linked in for the
   # selected components.  They're large libraries, we only want to
   # link them into the LLVM using shared library.
-  for pgac_option in `$LLVM_CONFIG --libs --system-libs $pgac_components`; do
+  for pgac_option in `$LLVM_CONFIG --libs --system-libs $LLVM_CONFIG_LINK_ARGS 
$pgac_components`; do
 case $pgac_option in
   -l*) LLVM_LIBS="$LLVM_LIBS $pgac_option";;
 esac
-- 
2.40.1

Re: WIP: new system catalog pg_wait_event

[Drouvot, Bertrand]

Hi,

On 8/9/23 9:56 AM, Michael Paquier wrote:

On Tue, Aug 08, 2023 at 10:16:37AM +0200, Drouvot, Bertrand wrote:

Please find attached v3 adding the wait event types.


+-- There will surely be at least 9 wait event types, 240 wait events and at
+-- least 27 related to WAL
+select count(distinct(wait_event_type)) > 8 as ok_type,
+   count(*) > 239 as ok,
+   count(*) FILTER (WHERE description like '%WAL%') > 26 AS ok_wal_desc
+from pg_wait_event;
The point is to check the execution of this function, so this could be
simpler, like that or a GROUP BY clause with the event type:
SELECT count(*) > 0 FROM pg_wait_event;
SELECT wait_event_type, count(*) > 0 AS has_data FROM pg_wait_event
   GROUP BY wait_event_type ORDER BY wait_event_type;



Thanks for looking at it!
Right, so v4 attached is just testing "SELECT count(*) > 0 FROM pg_wait_event;",
that does look enough to test.


+   printf $ic "\tmemset(values, 0, sizeof(values));\n";
+   printf $ic "\tmemset(nulls, 0, sizeof(nulls));\n\n";
+   printf $ic "\tvalues[0] = CStringGetTextDatum(\"%s\");\n", $last;
+   printf $ic "\tvalues[1] = CStringGetTextDatum(\"%s\");\n", 
$wev->[1];
+   printf $ic "\tvalues[2] = CStringGetTextDatum(\"%s\");\n\n", 
$new_desc;

That's overcomplicated for some code generated.  Wouldn't it be
simpler to generate a list of elements, with the code inserting the
tuples materialized looping over it?



Yeah, agree thanks!
In v4, the perl script now appends the wait events in a List that way:

"
printf $ic "\telement = (wait_event_element *) 
palloc(sizeof(wait_event_element));\n";

printf $ic "\telement->wait_event_type = \"%s\";\n", $last;
printf $ic "\telement->wait_event_name = \"%s\";\n", $wev->[1];
printf $ic "\telement->wait_event_description = \"%s\";\n\n", 
$new_desc;

printf $ic "\twait_event = lappend(wait_event, element);\n\n";
"

And the C function pg_get_wait_events() now iterates over this List.


+   my $new_desc = substr $wev->[2], 1, -2;
+   $new_desc =~ s/'/\\'/g;
+   $new_desc =~ s/<.*>(.*?)<.*>/$1/g;
+   $new_desc =~ s//$1/g;
+   $new_desc =~ s/; see.*$//;
Better to document what this does,


good idea...

I had to turn them "on" one by one to recall why they are there...;-)
Done in v4.


the contents produced look good.


yeap



+   rename($ictmp, "$output_path/pg_wait_event_insert.c")
+ || die "rename: $ictmp to $output_path/pg_wait_event_insert.c: $!";

  # seems nicer to not add that as an include path for the whole backend.
  waitevent_sources = files(
'wait_event.c',
+  'pg_wait_event.c',
  )

This could use a name referring to SQL functions, say
wait_event_funcs.c, with a wait_event_data.c or a
wait_event_funcs_data.c?


That sounds better indeed, thanks! v4 is using wait_event_funcs.c and
wait_event_funcs_data.c.



+   # Don't generate .c (except pg_wait_event_insert.c) and .h files for
+   # Extension, LWLock and Lock, these are handled independently.
+   my $is_exception = $waitclass eq 'WaitEventExtension' ||
+  $waitclass eq 'WaitEventLWLock' ||
+  $waitclass eq 'WaitEventLock';
Perhaps it would be cleaner to use a separate loop?


Agree that's worth it given the fact that iterating one more time is not that
costly here.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comFrom aa3e81810daaba7b1b2e05219ef662f2fd0607f8 Mon Sep 17 00:00:00 2001
From: bdrouvotAWS 
Date: Sat, 5 Aug 2023 12:39:42 +
Subject: [PATCH v4] pg_wait_event

Adding a new system view, namely pg_wait_event, that describes the wait events.
---
 doc/src/sgml/system-views.sgml| 64 +
 src/backend/catalog/system_views.sql  |  3 +
 src/backend/utils/activity/.gitignore |  1 +
 src/backend/utils/activity/Makefile   |  6 +-
 .../activity/generate-wait_event_types.pl | 46 -
 src/backend/utils/activity/meson.build|  1 +
 src/backend/utils/activity/wait_event_funcs.c | 69 +++
 src/include/catalog/pg_proc.dat   |  6 ++
 src/include/utils/meson.build |  4 +-
 src/test/regress/expected/rules.out   |  4 ++
 src/test/regress/expected/sysviews.out|  7 ++
 src/test/regress/sql/sysviews.sql |  3 +
 src/tools/msvc/clean.bat  |  1 +
 13 files changed, 209 insertions(+), 6 deletions(-)
  24.4% doc/src/sgml/
  58.5% src/backend/utils/activity/
   5.9% src/include/catalog/
   4.1% src/test/regress/expected/
   6.9% src/

diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 57b228076e..ed26c8326f 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -221,6 +221,11 @@
   views
  
 
+ 
+  pg_wait_event
+  wait events
+ 

Re: Add PG CI to older PG releases

[Nazir Bilal Yavuz]

Hi,

On Thu, 10 Aug 2023 at 18:05, Peter Eisentraut  wrote:
> I see only one attachment, so it's not clear what these commit hashes
> refer to.

I split the commit into 3 parts.

v2-0001-windows-Only-consider-us-to-be-running-as-service.patch is an
older commit (59751ae47fd43add30350a4258773537e98d4063). A couple of
tests were failing without this because the log file was empty and the
tests were comparing strings with the log files (like in the first
mail).

v2-0002-Make-PG_TEST_USE_UNIX_SOCKETS-work-for-tap-tests-.patch is
slightly modified version of 45f52709d86ceaaf282a440f6311c51fc526340b
to backpatch it to PG 14. Without this, Windows tests were failing
when they tried to create sockets with 'unix_socket_directories' path.

v2-0003-ci-Add-PG-CI-to-PG-14.patch adds files that are needed for CI
to work. I copied these files from the REL_15_STABLE branch. Then I
removed '--with-zstd' from .cirrus.yml since it was not supported yet.

Regards,
Nazir Bilal Yavuz
Microsoft
From 382121670bb96c941d4ad0110b5d3479ecfb54d8 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz 
Date: Thu, 10 Aug 2023 18:40:35 +0300
Subject: [PATCH v2 2/3] Make PG_TEST_USE_UNIX_SOCKETS work for tap tests on
 windows.

This commit is backpatched version of
45f52709d86ceaaf282a440f6311c51fc526340b for PG 14.
---
 src/bin/pg_ctl/t/001_start_stop.pl |  1 +
 src/test/perl/PostgresNode.pm  | 13 -
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_ctl/t/001_start_stop.pl b/src/bin/pg_ctl/t/001_start_stop.pl
index d656a7fe183..110926e40e3 100644
--- a/src/bin/pg_ctl/t/001_start_stop.pl
+++ b/src/bin/pg_ctl/t/001_start_stop.pl
@@ -35,6 +35,7 @@ print $conf TestLib::slurp_file($ENV{TEMP_CONFIG})
 if ($use_unix_sockets)
 {
 	print $conf "listen_addresses = ''\n";
+	$tempdir_short =~ s!\\!/!g if $TestLib::windows_os;
 	print $conf "unix_socket_directories = '$tempdir_short'\n";
 }
 else
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index aae02b18d69..ca2902e6430 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -126,7 +126,18 @@ INIT
 	$use_tcp= !$TestLib::use_unix_sockets;
 	$test_localhost = "127.0.0.1";
 	$last_host_assigned = 1;
-	$test_pghost= $use_tcp ? $test_localhost : TestLib::tempdir_short;
+	if ($use_tcp)
+	{
+		$test_pghost = $test_localhost;
+	}
+	else
+	{
+		# On windows, replace windows-style \ path separators with / when
+		# putting socket directories either in postgresql.conf or libpq
+		# connection strings, otherwise they are interpreted as escapes.
+		$test_pghost = TestLib::tempdir_short;
+		$test_pghost =~ s!\\!/!g if $TestLib::windows_os;
+	}
 	$ENV{PGHOST}= $test_pghost;
 	$ENV{PGDATABASE}= 'postgres';
 
-- 
2.40.1

From 6889e05d785254078e5099287326b48b9e85 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz 
Date: Thu, 10 Aug 2023 18:56:06 +0300
Subject: [PATCH v2 3/3] ci: Add PG CI to PG 14

PG CI is added starting from PG 15. Adding PG CI to PG 14 and below
could be beneficial. So, add PG CI to PG 14.

For the initial CI commit see: 93d97349461347d952e8cebdf62f5aa84b4bd20a
---
 .cirrus.yml | 616 
 src/tools/ci/README |  67 +++
 src/tools/ci/cores_backtrace.sh |  50 ++
 src/tools/ci/gcp_freebsd_repartition.sh |  28 ++
 src/tools/ci/pg_ci_base.conf|  14 +
 src/tools/ci/windows_build_config.pl|  13 +
 6 files changed, 788 insertions(+)
 create mode 100644 .cirrus.yml
 create mode 100644 src/tools/ci/README
 create mode 100755 src/tools/ci/cores_backtrace.sh
 create mode 100755 src/tools/ci/gcp_freebsd_repartition.sh
 create mode 100644 src/tools/ci/pg_ci_base.conf
 create mode 100644 src/tools/ci/windows_build_config.pl

diff --git a/.cirrus.yml b/.cirrus.yml
new file mode 100644
index 000..08cb737f81d
--- /dev/null
+++ b/.cirrus.yml
@@ -0,0 +1,616 @@
+# CI configuration file for CI utilizing cirrus-ci.org
+#
+# For instructions on how to enable the CI integration in a repository and
+# further details, see src/tools/ci/README
+
+
+env:
+  # Source of images / containers
+  GCP_PROJECT: pg-ci-images
+  IMAGE_PROJECT: $GCP_PROJECT
+  CONTAINER_REPO: us-docker.pkg.dev/${GCP_PROJECT}/ci
+
+  # The lower depth accelerates git clone. Use a bit of depth so that
+  # concurrent tasks and retrying older jobs have a chance of working.
+  CIRRUS_CLONE_DEPTH: 500
+  # Useful to be able to analyse what in a script takes long
+  CIRRUS_LOG_TIMESTAMP: true
+
+  CCACHE_MAXSIZE: "250M"
+
+  # target to test, for all but windows
+  CHECK: check-world PROVE_FLAGS=$PROVE_FLAGS
+  CHECKFLAGS: -Otarget
+  PROVE_FLAGS: --timer
+  PGCTLTIMEOUT: 120 # avoids spurious failures during parallel tests
+  TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
+  PG_TEST_EXTRA: kerberos ldap ssl
+
+
+# What files to preserve in case tests fail
+on_failure: _failure
+  log_artifacts:
+paths:
+  - 

Re: Allow parallel plan for referential integrity checks?

[Juan José Santamaría Flecha]

On Tue, Jul 4, 2023 at 9:45 AM Daniel Gustafsson  wrote:

>
> As there is no new patch submitted I will go ahead and do that, please feel
> free to resubmit when there is renewed interest in working on this.
>
>
Recently I restored a database from a directory format backup and having
this feature would have been quite useful. So, I would like to resume the
work on this patch, unless OP or someone else is already on it.

Regards,

Juan José Santamaría Flecha

Re: Add PG CI to older PG releases

[Peter Eisentraut]

On 10.08.23 16:43, Nazir Bilal Yavuz wrote:

1_ 76e38b37a5f179d4c9d2865ff31b79130407530b is added for debugging
Windows. Also a couple of SSL tests were failing without this because
the log file is empty. Example failures on 001_ssltests.pl:


I see only one attachment, so it's not clear what these commit hashes 
refer to.

Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails

[Jehan-Guillaume de Rorthais]

On Thu, 3 Aug 2023 11:02:43 +0200
Alvaro Herrera  wrote:

> On 2023-Aug-03, tender wang wrote:
> 
> > I think  old "sub-FK" should not be dropped, that will be violates foreign
> > key constraint.  
> 
> Yeah, I've been playing more with the patch and it is definitely not
> doing the right things.  Just eyeballing the contents of pg_trigger and
> pg_constraint for partitions added by ALTER...ATTACH shows that the
> catalog contents are inconsistent with those added by CREATE TABLE
> PARTITION OF.

Well, as stated in my orignal message, at the patch helps understanding the
problem and sketch a possible solution. It definitely is not complete.

After DETACHing the table, we surely needs to check everything again and
recreating what is needed to keep the FK consistent.

But should we keep the FK after DETACH? Did you check the two other discussions
related to FK, self-FK & partition? Unfortunately, as Tender experienced, the
more we dig the more we find bugs. Moreover, the second one might seems
unsolvable and deserve a closer look. See:

* FK broken after DETACHing referencing part
  https://www.postgresql.org/message-id/20230420144344.40744130%40karst
* Issue attaching a table to a partitioned table with an  auto-referenced
  foreign key
  https://www.postgresql.org/message-id/20230707175859.17c91538%40karst

RE: [PoC] pg_upgrade: allow to upgrade publisher node

[Hayato Kuroda (Fujitsu)]

Dear hackers,

Based on recent discussions, I updated the patch set. I did not reply one by one
because there are many posts, but thank you for giving many suggestion!

Followings shows what I changed.

1.
This feature is now enabled by default. Instead 
"--exclude-logical-replication-slots"
was added. (Per suggestions like [1])

2.
Pg_upgrade raises ERROR when some slots are 'WALAVAIL_REMOVED'. (Per 
discussion[2])

3.
Slots which are 'WALAVAIL_UNRESERVED' are dumped and restored. (Per 
consideration[3])

4.
Combination --logical-replication-slots-only and other --only options was
prohibit again. (Per suggestion[4]) Currently --data-only and --schema-only
could not be used together, so I followed the same style. Additionally, it's not
easy for user to predict the behavior if specifying many --only command.

5. 
Fixed some bugs related with combinations of options. E.g., v18 did not allow to
use "--create", but now it could use same time. This was because information
of role did not get from node while doing slot dump.

6.
The ordering of patches was changed. The patch "Always persist to disk..."
became 0001. (Per suggestion [4])

7.
Functions for checking were changed (per [5]). Currently WALs between
confirmed_lsn and current location is scanned and confirmed. The requirements
are little hacky:

* The first record after the confirmed_lsn must be SHUTDOWN_CHECKPOINT
* Other records till current position must be either RUNNING_XACT,
  CHECKPOINT_ONLINE or XLOG_FPI_FOR_HINT.

In the checking function (validate_wal_record_types_after), WALs are read
repeatedly and confirmed its type. v18 required to change the version number
for pg_walinspect, it is not needed anymore.


[1]: 
https://www.postgresql.org/message-id/ad83b9f2-ced3-c51c-342a-cc281ff562fc%40postgresql.org
[2]: 
https://www.postgresql.org/message-id/CAA4eK1%2B8btsYhNQvw6QJ4iTw1wFhkFXXABT%3DED1eHFvtekRanQ%40mail.gmail.com
[3]: 
https://www.postgresql.org/message-id/TYAPR01MB5866FD3F7992A46D0457F0E6F50BA%40TYAPR01MB5866.jpnprd01.prod.outlook.com
[4]: 
https://www.postgresql.org/message-id/CAA4eK1%2BCD82Kssy%2BiqpETPKYUh9AmNORF%2B3iGfNXgxKxqL3T6g%40mail.gmail.com
[5]: 
https://www.postgresql.org/message-id/CAD21AoC4D4wYTcLM8T-rAv%3DpO5kS6ffcVD1e7h4eFERT4%2BfwQQ%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED



v19-0001-Always-persist-to-disk-logical-slots-during-a-sh.patch
Description:  v19-0001-Always-persist-to-disk-logical-slots-during-a-sh.patch


v19-0002-pg_upgrade-Allow-to-replicate-logical-replicatio.patch
Description:  v19-0002-pg_upgrade-Allow-to-replicate-logical-replicatio.patch


v19-0003-pg_upgrade-Add-check-function-for-logical-replic.patch
Description:  v19-0003-pg_upgrade-Add-check-function-for-logical-replic.patch

Re: Fix last unitialized memory warning

[Peter Eisentraut]

On 09.08.23 17:29, Tristan Partin wrote:

On Wed Aug 9, 2023 at 10:02 AM CDT, Peter Eisentraut wrote:

On 09.08.23 10:07, Peter Eisentraut wrote:
> On 08.08.23 17:14, Tristan Partin wrote:
>>> I was able to reproduce the warning now on Fedora.  I agree with 
the >>> patch

>>>
>>> -   PgBenchValue vargs[MAX_FARGS];
>>> +   PgBenchValue vargs[MAX_FARGS] = { 0 };
>>>
>>> I suggest to also do
>>>
>>>   typedef enum
>>>   {
>>> -   PGBT_NO_VALUE,
>>> +   PGBT_NO_VALUE = 0,
>>>
>>> to make clear that the initialization value is meant to be invalid.
>>>
>>> I also got the plpgsql warning that you showed earlier, but I >>> 
couldn't think of a reasonable way to fix that.

>>
>> Applied in v2.
> > committed

This patch has apparently upset one buildfarm member with a very old 
compiler: 
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=lapwing=HEAD


Any thoughts?


Best I could find is SO question[0] which links out to[1]. Try this 
patch.


committed this fix

Re: LLVM 16 (opaque pointers)

[Ronan Dunklau]

Le dimanche 21 mai 2023, 05:01:41 CEST Thomas Munro a écrit :
> Hi,
> 
> Here is a draft version of the long awaited patch to support LLVM 16.
> It's mostly mechanical donkeywork, but it took some time: this donkey
> found it quite hard to understand the mighty getelementptr
> instruction[1] and the code generation well enough to figure out all
> the right types, and small mistakes took some debugging effort*.  I
> now finally have a patch that passes all tests.
> 
> Though it's not quite ready yet, I thought I should give this status
> update to report that the main task is more or less complete, since
> we're starting to get quite a few emails about it (mostly from Fedora
> users) and there is an entry for it on the Open Items for 16 wiki
> page.  Comments/review/testing welcome.

Hello Thomas,

Thank you for this effort !

I've tested it against llvm 15 and 16, and found no problem with it.

> 6. I need to go through the types again with a fine tooth comb, and
> check the test coverage to look out for eg GEP array arithmetic with
> the wrong type/size that isn't being exercised.

I haven't gone through the test coverage myself, but I exercised the following 
things: 

 - running make installcheck with jit_above_cost = 0
 - letting sqlsmith hammer random queries at it for a few hours.

This didn't show obvious issues.

> *For anyone working with this type of IR generation code and
> questioning their sanity, I can pass on some excellent advice I got
> from Andres: build LLVM yourself with assertions enabled, as they
> catch some classes of silly mistake that otherwise just segfault
> > inscrutably on execution.

I tried my hand at backporting it to previous versions, and not knowing 
anything about it made me indeed question my sanity.  It's quite easy for PG 
15, 14, 13. PG 12 is nothing insurmontable either, but PG 11 is a bit hairier 
most notably due to to the change in fcinfo args representation. But I guess 
that's also a topic for another day :-)

Best regards,

--
Ronan Dunklau

Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication

[Melih Mutlu]

Hi Peter and Vignesh,

Peter Smith , 7 Ağu 2023 Pzt, 09:25 tarihinde şunu
yazdı:

> Hi Melih.
>
> Now that the design#1 ERRORs have been fixed, we returned to doing
> performance measuring of the design#1 patch versus HEAD.


Thanks a lot for taking the time to benchmark the patch. It's really
helpful.

Publisher "busy" table does commit every 1000 inserts:
> 2w 4w 8w 16w
> HEAD 11898 5855 1868 1631
> HEAD+v24-0002 21905 8254 3531 1626
> %improvement -84% -41% -89% 0%


> ^ Note - design#1 was slower than HEAD here


> ~


> Publisher "busy" table does commit every 2000 inserts:
> 2w 4w 8w 16w
> HEAD 21740 7109 3454 1703
> HEAD+v24-0002 21585 10877 4779 2293
> %improvement 1% -53% -38% -35%


I assume you meant HEAD+v26-0002 and not v24. I wanted to quickly reproduce
these two cases where the patch was significantly worse. Interestingly my
results are a bit different than yours.

Publisher "busy" table does commit every 1000 inserts:
2w 4w 8w 16w
HEAD 22405 10335 5008 3304
HEAD+v26  19954  8037 4068 2761
%improvement 1% 2% 2% 1%

Publisher "busy" table does commit every 2000 inserts:
2w 4w 8w 16w
HEAD 33122 14220 7251 4279
HEAD+v26 34248 16213 7356 3914
%improvement 0% -1% 0% 1%

If I'm not doing something wrong in testing (or maybe the patch doesn't
perform reliable yet for some reason), I don't see a drastic change in
performance. But I guess the patch is supposed to perform better than HEAD
in these both cases anyway. right?. I would expect the performance of the
patch to converge to HEAD's performance with large tables. But I'm not sure
what to expect when apply worker is busy with large transactions.

However, I need to investigate a bit more what Vignesh shared earlier [1].
It makes sense that those issues can cause this problem here.

It just takes a bit of time for me to figure out these things, but I'm
working on it.

[1]
https://www.postgresql.org/message-id/CALDaNm1TA068E2niJFUR9ig%2BYz3-ank%3Dj5%3Dj-2UocbzaDnQPrA%40mail.gmail.com



Thanks,
-- 
Melih Mutlu
Microsoft

Re: libpq compression (part 2)

[Jonah H. Harris]

Pinging to see if anyone has continued to work on this behind-the-scenes or
whether this is the latest patch set there is.

-- 
Jonah H. Harris

Add PG CI to older PG releases

[Nazir Bilal Yavuz]

Hi,

PG CI is added starting from PG 15, adding PG CI to PG 14 and below
could be beneficial. So, firstly I tried adding it to the
REL_14_STABLE branch. If that makes sense, I will try to add PG CI to
other old PG releases.

'Add PG CI to PG 14' patch is attached. I merged both CI commits and
the least amount of commits to pass the CI on all OSes.

Addition to CI commits, other changes are:

1_ 76e38b37a5f179d4c9d2865ff31b79130407530b is added for debugging
Windows. Also a couple of SSL tests were failing without this because
the log file is empty. Example failures on 001_ssltests.pl:

#   Failed test 'certificate authorization succeeds with DN mapping:
log matches'
#   at c:/cirrus/src/test/perl/PostgresNode.pm line 2195.
#   ''
# doesn't match '(?^:connection authenticated:
identity="CN=ssltestuser-dn,OU=Testing,OU=Engineering,O=PGDG"
method=cert)'

#   Failed test 'certificate authorization succeeds with CN mapping:
log matches'
#   at c:/cirrus/src/test/perl/PostgresNode.pm line 2195.
#   ''
# doesn't match '(?^:connection authenticated:
identity="CN=ssltestuser-dn,OU=Testing,OU=Engineering,O=PGDG"
method=cert)'

#   Failed test 'certificate authorization fails with client cert
belonging to another user: log matches'
#   at c:/cirrus/src/test/perl/PostgresNode.pm line 2243.
#   ''
# doesn't match '(?^:connection authenticated:
identity="CN=ssltestuser" method=cert)'
# Looks like you failed 3 tests of 110.

2_ 45f52709d86ceaaf282a440f6311c51fc526340b is added for fixing socket
directories on Windows.

3_ I removed '--with-zstd' since it was not supported yet.

Any kind of feedback would be appreciated.

Regards,
Nazir Bilal Yavuz
Microsoft
From 34986d591eaa5bf66569674857832cc508570b3b Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz 
Date: Thu, 10 Aug 2023 17:18:38 +0300
Subject: [PATCH v1] ci: Add PG CI to PG 14

PG CI is added starting from PG 15. Adding PG CI to PG 14 and below
could be beneficial. So, add PG CI to PG 14.

76e38b37a5f179d4c9d2865ff31b79130407530b is added for debugging Windows.
Also, a couple of SSL tests were failing without this because the log
file is empty.

45f52709d86ceaaf282a440f6311c51fc526340b is added for fixing socket
directories on Windows.

For the initial CI commit see: 93d97349461347d952e8cebdf62f5aa84b4bd20a
---
 .cirrus.yml | 616 
 src/bin/pg_ctl/pg_ctl.c |  33 ++
 src/bin/pg_ctl/t/001_start_stop.pl  |   1 +
 src/port/win32security.c|  18 +-
 src/test/perl/PostgresNode.pm   |   4 +
 src/tools/ci/README |  67 +++
 src/tools/ci/cores_backtrace.sh |  50 ++
 src/tools/ci/gcp_freebsd_repartition.sh |  28 ++
 src/tools/ci/pg_ci_base.conf|  14 +
 src/tools/ci/windows_build_config.pl|  13 +
 10 files changed, 841 insertions(+), 3 deletions(-)
 create mode 100644 .cirrus.yml
 create mode 100644 src/tools/ci/README
 create mode 100755 src/tools/ci/cores_backtrace.sh
 create mode 100755 src/tools/ci/gcp_freebsd_repartition.sh
 create mode 100644 src/tools/ci/pg_ci_base.conf
 create mode 100644 src/tools/ci/windows_build_config.pl

diff --git a/.cirrus.yml b/.cirrus.yml
new file mode 100644
index 000..08cb737f81d
--- /dev/null
+++ b/.cirrus.yml
@@ -0,0 +1,616 @@
+# CI configuration file for CI utilizing cirrus-ci.org
+#
+# For instructions on how to enable the CI integration in a repository and
+# further details, see src/tools/ci/README
+
+
+env:
+  # Source of images / containers
+  GCP_PROJECT: pg-ci-images
+  IMAGE_PROJECT: $GCP_PROJECT
+  CONTAINER_REPO: us-docker.pkg.dev/${GCP_PROJECT}/ci
+
+  # The lower depth accelerates git clone. Use a bit of depth so that
+  # concurrent tasks and retrying older jobs have a chance of working.
+  CIRRUS_CLONE_DEPTH: 500
+  # Useful to be able to analyse what in a script takes long
+  CIRRUS_LOG_TIMESTAMP: true
+
+  CCACHE_MAXSIZE: "250M"
+
+  # target to test, for all but windows
+  CHECK: check-world PROVE_FLAGS=$PROVE_FLAGS
+  CHECKFLAGS: -Otarget
+  PROVE_FLAGS: --timer
+  PGCTLTIMEOUT: 120 # avoids spurious failures during parallel tests
+  TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
+  PG_TEST_EXTRA: kerberos ldap ssl
+
+
+# What files to preserve in case tests fail
+on_failure: _failure
+  log_artifacts:
+paths:
+  - "**/*.log"
+  - "**/*.diffs"
+  - "**/regress_log_*"
+type: text/plain
+
+task:
+  name: FreeBSD - 13
+
+  env:
+# FreeBSD on GCP is slow when running with larger number of CPUS /
+# jobs. Using one more job than cpus seems to work best.
+CPUS: 2
+BUILD_JOBS: 3
+TEST_JOBS: 3
+
+CCACHE_DIR: /tmp/ccache_dir
+
+  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*freebsd.*'
+
+  compute_engine_instance:
+image_project: $IMAGE_PROJECT
+image: family/pg-ci-freebsd-13
+platform: freebsd
+ 

Re: proposal: psql: show current user in prompt

[Jelte Fennema]

On Thu, 10 Aug 2023 at 14:44, Pavel Stehule  wrote:
> čt 10. 8. 2023 v 14:05 odesílatel Jelte Fennema  napsal:
>> That it is not rolled-back
>> in a case like this?
>>
>> BEGIN;
>> \set PROMPT '%N'
>> ROLLBACK;
>
>
> surely not.
>
> \set is client side setting, and it is not transactional. Attention - "\set" 
> and "set" commands are absolutely different creatures.

To clarify: I agree it's the desired behavior that \set is not rolled back.

> It Would be strange (can be very messy) if I had a message like "cannot set a 
> prompt, because you should do ROLLBACK first"

This was a very helpful sentence for my understanding. To double check
that I'm understanding you correctly. This is the kind of case that
you're talking about.

postgres=# BEGIN;
postgres=# SELECT some syntax error;
ERROR:  42601: syntax error at or near "some"
postgres=# \set PROMPT '%N'
ERROR:  25P02: current transaction is aborted, commands ignored until
end of transaction block

I agree that it should not throw an error like that. So indeed a
dedicated message type is needed for psql too. Because any query will
cause that error.

But afaict there's no problem with using pqParseInput3() and
PQexecFinish() even if the message isn't handled as part of the
transaction. Some other messages that pqParseInput3 handles which are
not part of the transaction are 'N' (Notice) and 'K' (secret key).

how to ensure parallel restore consistency ?

[熊艳辉]

PG version 15.4
i read code about pg_dump  pg_restore, there is an option -j/--jobs 
related with parallel,
i have questions about parallel dump  restore
1)In pg_dump when running parallel dump, it can ensure dump data 
consistency using synchronized snaphot,

why should it start a trasaction in every parallel jobs, also set isolation 
level ,read only, and no need commit trasaction
2)In pg_restore there are no actions same with pg_dump under parallel 
mode, how to ensure restore data consistency?

no trasaction and no commit action
3)when we do dump or restore under parallel mode, should we set whole 
database read-only(write behavior forbidden??)



发自我的企业微信

回复：Re: inconsistency between the VM page visibility status and the visibility status of the page

[熊艳辉]

Sure, that's correct. There are indeed some code bugs in my logic, thank you 
very much for your response.




发自我的企业微信





 --回复的邮件信息--
   Tomas Vondra

Re: [PoC] pg_upgrade: allow to upgrade publisher node

[Masahiko Sawada]

On Thu, Aug 10, 2023 at 12:52 PM Amit Kapila  wrote:
>
> On Thu, Aug 10, 2023 at 6:46 AM Masahiko Sawada  wrote:
> >
> > On Wed, Aug 9, 2023 at 1:15 PM Amit Kapila  wrote:
> > >
> > > On Wed, Aug 9, 2023 at 8:01 AM Masahiko Sawada  
> > > wrote:
> > >
> > > I feel it would be a good idea to provide such a tool for users to
> > > avoid getting errors during upgrade but I think the upgrade code still
> > > needs to ensure that there are no WAL records between
> > > confirm_flush_lsn and SHUTDOWN_CHECKPOINT than required. Or, do you
> > > want to say that we don't do any verification check during the upgrade
> > > and let the data loss happens if the user didn't ensure that by
> > > running such a tool?
> >
> > I meant that if we can check the slot state file while the old cluster
> > stops, we can ensure there are no WAL records between slot's
> > confirmed_fluhs_lsn (in the state file) and the latest checkpoint (in
> > the control file).
> >
>
> Are you suggesting doing this before we start the old cluster or after
> we stop the old cluster? I was thinking about the pros and cons of
> doing this check when the server is 'on' (along with other upgrade
> checks something like the patch is doing now) versus when the server
> is 'off'. I think the advantage of doing it when the server is 'off'
> (after check_and_dump_old_cluster()) is that it will be ensured that
> there is no extra WAL that could be generated during the upgrade and
> has not been verified against confirmed_flush_lsn location. But OTOH,
> to retrieve slot information when the server is 'off', we need a
> separate utility or probably a functionality for the same in
> pg_upgrade and also some WAL reading stuff which sounds to me like a
> larger change that may not be warranted here. I think anyway the extra
> WAL (if any got generated during the upgrade) won't be required after
> the upgrade so not convinced to make such a check while the server is
> 'off'. Are there reasons which make it better to do this while the old
> cluster is 'off'?

What I imagined is that we do this check before
check_and_dump_old_cluster() while the server is 'off'. Reading the
slot state file would be simple and I guess we would not need a tool
or cli program for that. We need to expose RepliactionSlotOnDisk,
though. After reading the control file and the slots' state files we
check if slot's confirmed_flush_lsn matches the latest checkpoint LSN
in the control file (BTW maybe we can get slot name and plugin name
here instead of using pg_dump?). Extra WAL records could be generated
only after this check, so we wouldn't need to worry about that for
slots for logical replication. As for non-logical replication slots,
we would need some WAL reading stuff, but I'm not sure we need it for
the first commit. Or another idea would be to allow users to mark
replication slots "upgradable" so that pg_upgrade skips the
confirmed_flush_lsn check.

BTW this check would not be able to support live-check but I think
it's not a problem as this check with a running server will never be
able to pass.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: Adding a pg_servername() function

[Jimmy Angelakos]

Hi all,

FWIW, I too believe this is a feature that has been sorely missing for
years, leading us to use awful hacks like running "hostname" as PROGRAM and
retrieving its output.
Ideally that's what this function (or GUC) should return, what the system
believes its hostname to be. It doesn't even have to return any additional
hostnames it may have.

2 more points:
1. I too don't see it as relevant to the cluster's name
2. I agree that this may not be desirable for non-monitoring roles but
would not go so far as to require superuser. If a cloud provider doesn't
want this hostname exposed for whatever reason, they can hack Postgres to
block this. They don't seem to have trouble hacking it for other reasons.

Best regards,
Jimmy

Jimmy Angelakos
Senior Solutions Architect
EDB - https://www.enterprisedb.com/


On Thu, 10 Aug 2023 at 08:44, Laetitia Avrot 
wrote:

> Dear Tom,
>
> Thank you for your interest in that patch and for taking the time to point
> out several things that need to be better. Please find below my answers.
>
> Le mer. 9 août 2023 à 16:04, Tom Lane  a écrit :
>
>> I actually do object to this, because I think the concept of "server
>> name" is extremely ill-defined and if we try to support it, we will
>> forever be chasing requests for alternative behaviors.
>
>
> Yes, that's on me with choosing a poor name. I will go with
> pg_gethostname().
>
>
>> Just to start
>> with, is a prospective user expecting a fully-qualified domain name
>> or just the base name?  If the machine has several names (perhaps
>> associated with different IP addresses), what do you do about that?
>> I wouldn't be too surprised if users would expect to get the name
>> associated with the IP address that the current connection came
>> through.  Or at least they might tell you they want that, until
>> they discover they're getting "localhost.localdomain" on loopback
>> connections and come right back to bitch about that.
>>
>
> If there is a gap between what the function does and the user
> expectations, it is my job to write the documentation in a more clear way
> to set expectations to the right level and explain precisely what this
> function is doing. Again, using a better name as pg_gethostname() will also
> help to remove the confusion.
>
>
>>
>> Windows likely adds a whole 'nother set of issues to "what's the
>> machine name", but I don't know enough about it to say what.
>>
>
> Windows does have a similar gethostname() function (see here:
> https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-gethostname
> ).
>
>
>>
>> I think the upthread suggestion to use cluster_name is going to
>> be a superior solution for most people, not least because they
>> can use it today and it will work the same regardless of platform.
>>
>
> I don't think cluster_name is the same as hostname. True, people could use
> that parameter for that usage, but it does not feel right to entertain
> confusion between the cluster_name (which, in my humble opinion, should be
> different depending on the Postgres cluster) and the answer to "on which
> host is this Postgres cluster running?".
>
>
>> > (*) But we should think about access control for this.  If you're in a
>> > DBaaS environment, providers might not like that you can read out their
>> > internal host names.
>>
>> There's that, too.
>
>
> Of course, this function will need special access and DBaaS providers will
> be able to not allow their users to use that function, as they already do
> for other features. As I said, the patch is only at the stage of POC, at
> the moment.
>
> Le mer. 9 août 2023 à 18:31, Tom Lane  a écrit :
>
>>
>>
>> One concrete reason why I am doubtful about this is the case of
>> multiple PG servers running on the same machine.  gethostname()
>> will be unable to distinguish them.
>>
>>
> And that's where my bad name for this function brings confusion. If this
> function returns the hostname, then it does seem totally legit and normal
> to get the same if 3 Postgres clusters are running on the same host. If
> people want to identify their cluster, they should use cluster_name. I
> totally agree with that.
>
> Why do I think this is useful?
>
> 1- In most companies I've worked for, people responsible for the OS
> settings, Network settings, and database settings are different persons.
> Also, for most companies I've worked with, maintaining their inventory and
> finding out which Postgres cluster is running on which host is still a
> difficult thing and error-prone to do. I thought that it could be nice and
> useful to display easily for the DBAs on which host the cluster they are
> connected to is running so that when they are called at 2 AM, their life
> could be a little easier.
>
> 2- In addition, as already pointed out, I know that pg_staviz (a
> monitoring tool) needs that information and uses this very dirty hack to
> get it (see
> 

Re: [question] multil-column range partition prune

[Christoph Moench-Tegeder]

## tender wang (tndrw...@gmail.com):

> But I want to know why we don't prune when just have latter partition key
> in whereClause.

Start with the high level documentation
https://www.postgresql.org/docs/current/sql-createtable.html#SQL-CREATETABLE-PARTITION
where the 5th paragraph points you to
https://www.postgresql.org/docs/current/functions-comparisons.html#ROW-WISE-COMPARISON
which has a detailed explanation of row comparison.

Regards,
Christoph

-- 
Spare Space.

Re: proposal: psql: show current user in prompt

[Pavel Stehule]

čt 10. 8. 2023 v 14:05 odesílatel Jelte Fennema  napsal:

> On Tue, 8 Aug 2023 at 07:20, Pavel Stehule 
> wrote:
> > The reason why I implemented separate flow is usage from psql and
> independence of transaction state.  It is used for the \set command, that
> is non-transactional, not SQL. If I inject this message to some other flow,
> I lose this independence.
>
> I still don't understand the issue that you're trying to solve by
> introducing a new flow for handling the response. What do you mean
> with independence of the transaction state? That it is not rolled-back
> in a case like this?
>
> BEGIN;
> \set PROMPT '%N'
> ROLLBACK;
>

surely not.

\set is client side setting, and it is not transactional. Attention -
"\set" and "set" commands are absolutely different creatures.


> That seems more like a Postgres server concern, i.e. don't revert the
> change back on ROLLBACK. (I think your current server-side
> implementation already does that)
>

Postgres does it, but not on the client side. What I know, almost all
environments don't support transactions on the client side. Postgres is not
special in this direction.


>
> I guess one reason that I don't understand what you mean is that libpq
> doesn't really care about "transaction state" at all. It's really a
> wrapper around a socket with easy functions to send messages in the
> postgres protocol over it. So it cares about the "connection state",
> but not really about a "transaction state". (it does track the current
> connection state, but it doesn't actually use the value except when
> reporting the value when PQtransactionStatus is called by the user of
> libpq)
>
> > Without independence on transaction state and SQL, I can just implement
> some SQL function that sets reporting for any GUC, and it is more simple
> than extending protocol.
>
> I don't understand why this is not possible. As far as I can tell this
> should work fine for the usecase of psql. I still prefer the protocol
> message approach though, because that makes it possible for connection
> poolers to intercept the message and handle it accordingly. And I see
> some use cases for this reporting feature for PgBouncer as well.
>

Maybe we are talking about different features. Now, I have no idea how the
proposed feature can be useful for pgbouncer?

Sure If I implement a new flow, then pgbouncer cannot forward. But it is
not too hard to implement redirection of new flow to pgbouncer.


> However, I think this is probably the key thing that I don't
> understand about the problem you're describing: So, could you explain
> in some more detail why implementing a SQL function would not work for
> psql?
>

I try to get some consistency. psql setting and some features like
formatting doesn't depend on transactional state. It depends just on
connection.  This is the reason why I don't want to inject dependency on
transaction state. Without this dependency, I don't need to check
transaction state, and I can execute prompt settings immediately. If I
implement this feature as transactional, then I need to wait to idle or to
make a new transaction (and this I have not under my control). I try to be
consistent with current psql behaviour. It Would be strange (can be very
messy) if I had a message like "cannot set a prompt, because you should do
ROLLBACK first"

When this feature should be implemented as an injected message, then I have
another problem. Which SQL command I have to send to the server, when the
user wants to set a prompt? And then I don't need to implement a new
message, and I can just implement the SQL function
pg_catalog.report_config(...).

Re: proposal: psql: show current user in prompt

[Jelte Fennema]

On Tue, 8 Aug 2023 at 07:20, Pavel Stehule  wrote:
> The reason why I implemented separate flow is usage from psql and 
> independence of transaction state.  It is used for the \set command, that is 
> non-transactional, not SQL. If I inject this message to some other flow, I 
> lose this independence.

I still don't understand the issue that you're trying to solve by
introducing a new flow for handling the response. What do you mean
with independence of the transaction state? That it is not rolled-back
in a case like this?

BEGIN;
\set PROMPT '%N'
ROLLBACK;

That seems more like a Postgres server concern, i.e. don't revert the
change back on ROLLBACK. (I think your current server-side
implementation already does that)

I guess one reason that I don't understand what you mean is that libpq
doesn't really care about "transaction state" at all. It's really a
wrapper around a socket with easy functions to send messages in the
postgres protocol over it. So it cares about the "connection state",
but not really about a "transaction state". (it does track the current
connection state, but it doesn't actually use the value except when
reporting the value when PQtransactionStatus is called by the user of
libpq)

> Without independence on transaction state and SQL, I can just implement some 
> SQL function that sets reporting for any GUC, and it is more simple than 
> extending protocol.

I don't understand why this is not possible. As far as I can tell this
should work fine for the usecase of psql. I still prefer the protocol
message approach though, because that makes it possible for connection
poolers to intercept the message and handle it accordingly. And I see
some use cases for this reporting feature for PgBouncer as well.
However, I think this is probably the key thing that I don't
understand about the problem you're describing: So, could you explain
in some more detail why implementing a SQL function would not work for
psql?

Re: [PATCH] Add loongarch native checksum implementation.

[John Naylor]

On Thu, Aug 10, 2023 at 5:54 PM Michael Paquier  wrote:
>
> On Thu, Aug 10, 2023 at 03:56:37PM +0530, Amit Kapila wrote:
> > In MSVC build, on doing: perl mkvcbuild.pl after this commit, I am
> > facing the below error:
> > Generating configuration headers...
> > undefined symbol: USE_LOONGARCH_CRC32C at src/include/pg_config.h line
> > 718 at ../postgresql/src/tools/msvc/Mkvcbuild.pm line 872.
> >
> > Am I missing something or did the commit miss something?
>
> Yes, the commit has missed the addition of USE_LOONGARCH_CRC32C in
> Solution.pm.  If you want to be consistent with pg_config.h.in, you
> could add it just after USE_LLVM, for instance.

Oops, fixing now...

--
John Naylor
EDB: http://www.enterprisedb.com

Re: A failure in 031_recovery_conflict.pl on Debian/s390x

[Thomas Munro]

On Thu, Aug 10, 2023 at 9:15 PM Christoph Berg  wrote:
> No XXX lines this time either, but I've seen then im logfiles that
> went through successfully.

Hmm.  Well, I think this looks like a different kind of bug then.
That patch of mine is about fixing some unsafe coding on the receiving
side of a signal.  In this case it's apparently not being sent.  So
either the Heap2/PRUNE record was able to proceed (indicating that
that CURSOR was not holding a pin as expected), or VACUUM decided not
to actually do anything to that block (conditional cleanup lock vs
transient pin changing behaviour?), or there's a bug somewhere in/near
LockBufferForCleanup(), which should have emitted that XXX message
before even calling ResolveRecoveryConflictWithBufferPin().

Do you still have the data directories around from that run, so we can
see if the expected Heap2/PRUNE was actually logged?  For example
(using meson layout here, in the build directory) that'd be something
like:

$ ./tmp_install/home/tmunro/install/bin/pg_waldump
testrun/recovery/031_recovery_conflict/data/t_031_recovery_conflict_standby_data/pgdata/pg_wal/00010003

In there I see this:

rmgr: Heap2   len (rec/tot): 57/57, tx:  0, lsn:
0/0344BB90, prev 0/0344BB68, desc: PRUNE snapshotConflictHorizon: 0,
nredirected: 0, ndead: 1, nunused: 0, redirected: [], dead: [21],
unused: [], blkref #0: rel 1663/16385/16386 blk 0

That's the WAL record that's supposed to be causing
031_recovery_conflict_standby.log to talk about a conflict, starting
with this:

2023-08-10 22:47:04.564 NZST [57145] LOG:  recovery still waiting
after 10.035 ms: recovery conflict on buffer pin
2023-08-10 22:47:04.564 NZST [57145] CONTEXT:  WAL redo at 0/344BB90
for Heap2/PRUNE: snapshotConflictHorizon: 0, nredirected: 0, ndead: 1,
 nunused: 0, redirected: [], dead: [21], unused: []; blkref #0: rel
1663/16385/16386, blk 0

Re: [PATCH] Add loongarch native checksum implementation.

[Michael Paquier]

On Thu, Aug 10, 2023 at 03:56:37PM +0530, Amit Kapila wrote:
> In MSVC build, on doing: perl mkvcbuild.pl after this commit, I am
> facing the below error:
> Generating configuration headers...
> undefined symbol: USE_LOONGARCH_CRC32C at src/include/pg_config.h line
> 718 at ../postgresql/src/tools/msvc/Mkvcbuild.pm line 872.
> 
> Am I missing something or did the commit miss something?

Yes, the commit has missed the addition of USE_LOONGARCH_CRC32C in
Solution.pm.  If you want to be consistent with pg_config.h.in, you
could add it just after USE_LLVM, for instance.
--
Michael


signature.asc
Description: PGP signature

Re: Add assertion on held AddinShmemInitLock in GetNamedLWLockTranche()

[Michael Paquier]

On Fri, Jul 28, 2023 at 11:07:49AM +0530, Bharath Rupireddy wrote:
> Why to block multiple readers (if at all there exists any), with
> LWLockHeldByMeInMode(..., LW_EXCLUSIVE)? I think
> Assert(LWLockHeldByMe(AddinShmemInitLock)); suffices in
> GetNamedLWLockTranche.

I am not sure to follow this argument.  Why would it make sense for
anybody to use that in shared mode?  We document that the exclusive
mode is required because we retrieve the lock position in shmem that
an extension needs to know about when it initializes its own shmem
state, and that cannot be done safely without LW_EXCLUSIVE.  Any
extension I can find in https://codesearch.debian.net/ does that as
well.
--
Michael


signature.asc
Description: PGP signature

Re: [question] multil-column range partition prune

[Matthias van de Meent]

On Thu, 10 Aug 2023 at 12:16, tender wang  wrote:
>
> I have an range partition and query below:
> create table p_range(a int, b int) partition by range (a,b); create table 
> p_range1 partition of p_range for values from (1,1) to (3,3); create table 
> p_range2 partition of p_range for values from (4,4) to (6,6); explain select 
> * from p_range where b =2;
> QUERY PLAN
> --
>  Append  (cost=0.00..76.61 rows=22 width=8)
>->  Seq Scan on p_range1 p_range_1  (cost=0.00..38.25 rows=11 width=8)
>  Filter: (b = 2)
>->  Seq Scan on p_range2 p_range_2  (cost=0.00..38.25 rows=11 width=8)
>  Filter: (b = 2)
> (5 rows)
>
> The result of EXPLAIN shows that no partition prune happened.
> And gen_prune_steps_from_opexps() has comments that can answer the result.
> /*
> * For range partitioning, if we have no clauses for the current key,
> * we can't consider any later keys either, so we can stop here.
> */
> if (part_scheme->strategy == PARTITION_STRATEGY_RANGE &&
> clauselist == NIL)
> break;
>
> But I want to know why we don't prune when just have latter partition key in 
> whereClause.
> Thanks.

Multi-column range partitioning uses row compares for range
partitions. For single columns that doesn't matter much, but for
multiple columns it is slightly less intuitive. But because they are
row compares, that means for the given partitions, the values
contained would be:

p_range1 contains rows with
- A = 1, B >= 1
- A > 1 and  A < 3, B: any value
- A = 3, B < 3

p_range2 contains rows with:
- A = 4, B >= 4
- A > 4 and A < 6, B: any value
- A = 6, B < 6

As you can see, each partition contains a set of rows that may have
any value for B, and thus these partitions cannot be pruned based on
the predicate.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

Re: [PATCH] Add loongarch native checksum implementation.

[Amit Kapila]

On Thu, Aug 10, 2023 at 10:35 AM John Naylor
 wrote:
>
> On Tue, Aug 8, 2023 at 2:22 PM YANG Xudong  wrote:
> >
> > On 2023/8/8 14:38, John Naylor wrote:
> >
> > > v4 0001 is the same as v3, but with a draft commit message. I will
> > > squash and commit this week, unless there is additional feedback.
> >
> > Looks good to me. Thanks for the additional patch.
>
> I pushed this with another small comment change. Unfortunately, I didn't 
> glance at the buildfarm beforehand -- it seems many members are failing an 
> isolation check added by commit fa2e87494, including both loongarch64 
> members. I'll check back periodically.
>
> https://buildfarm.postgresql.org/cgi-bin/show_status.pl
>

In MSVC build, on doing: perl mkvcbuild.pl after this commit, I am
facing the below error:
Generating configuration headers...
undefined symbol: USE_LOONGARCH_CRC32C at src/include/pg_config.h line
718 at ../postgresql/src/tools/msvc/Mkvcbuild.pm line 872.

Am I missing something or did the commit miss something?

--
With Regards,
Amit Kapila.

[question] multil-column range partition prune

[tender wang]

I have an range partition and query below:
create table p_range(a int, b int) partition by range (a,b); create table
p_range1 partition of p_range for values from (1,1) to (3,3); create table
p_range2 partition of p_range for values from (4,4) to (6,6); explain
select * from p_range where b =2;
QUERY PLAN
--
 Append  (cost=0.00..76.61 rows=22 width=8)
   ->  Seq Scan on p_range1 p_range_1  (cost=0.00..38.25 rows=11 width=8)
 Filter: (b = 2)
   ->  Seq Scan on p_range2 p_range_2  (cost=0.00..38.25 rows=11 width=8)
 Filter: (b = 2)
(5 rows)

The result of EXPLAIN shows that no partition prune happened.
And gen_prune_steps_from_opexps() has comments that can answer the result.
/*
* For range partitioning, if we have no clauses for the current key,
* we can't consider any later keys either, so we can stop here.
*/
if (part_scheme->strategy == PARTITION_STRATEGY_RANGE &&
clauselist == NIL)
break;

But I want to know why we don't prune when just have latter partition key
in whereClause.
Thanks.

Re: pgsql: Ignore BRIN indexes when checking for HOT udpates

[Alvaro Herrera]

On 2023-Aug-09, Tomas Vondra wrote:

> On 8/9/23 11:11, Alvaro Herrera wrote:
> > I was trying to use RelationGetIndexAttrBitmap for something and
> > realized that its header comment does not really explain things very
> > well.  That was already the case before this commit, but it (this
> > commit) did add new possible values without mentioning them.  I propose
> > the attached comment-only patch.
> 
> +1

Thanks for looking!  Pushed now.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Re: [PoC] Reducing planning time when tables have many partitions

[Yuya Watari]

Hello David,

I really appreciate your quick reply.

On Wed, Aug 9, 2023 at 7:28 PM David Rowley  wrote:
> If 0004 is adding an em_index to mark the index into
> PlannerInfo->eq_members, can't you use that in
> setup_eclass_member[_strict]_iterator to loop to verify that the two
> methods yield the same result?
>
> i.e:
>
> + Bitmapset *matching_ems = NULL;
> + memcpy(_iter, iter, sizeof(EquivalenceMemberIterator));
> + memcpy(_iter, iter, sizeof(EquivalenceMemberIterator));
> +
> + idx_iter.use_index = true;
> + noidx_iter.use_index = false;
> +
> + while ((em = eclass_member_iterator_strict_next(_iter)) != NULL)
> + matching_ems = bms_add_member(matching_ems, em->em_index);
> +
> + Assert(bms_equal(matching_ems, iter->matching_ems));
>
> That should void the complaint that the Assert checking is too slow.
> You can also delete the list_ptr_cmp function too (also noticed a
> complaint about that).

Thanks for sharing your idea regarding this verification. It looks
good to solve the degradation problem in assert-enabled builds. I will
try it.

> For the 0003 patch.  Can you explain why you think these fields should
> be in RangeTblEntry rather than RelOptInfo? I can only guess you might
> have done this for memory usage so that we don't have to carry those
> fields for join rels?  I think RelOptInfo is the correct place to
> store fields that are only used in the planner.  If you put them in
> RangeTblEntry they'll end up in pg_rewrite and be stored for all
> views.  Seems very space inefficient and scary as it limits the scope
> for fixing bugs in back branches due to RangeTblEntries being
> serialized into the catalogues in various places.

This change was not made for performance reasons but to avoid null
reference exceptions. The details are explained in my email [1]. In
brief, the earlier patch did not work because simple_rel_array[i]
could be NULL for some 'i', and we referenced such a RelOptInfo. For
example, the following code snippet in add_eq_member() does not work.
I inserted "Assert(rel != NULL)" into this code, and then the
assertion failed. So, I moved the indexes to RangeTblEntry to address
this issue, but I don't know if this solution is good. We may have to
solve this in a different way.

=
@@ -572,9 +662,31 @@ add_eq_member(EquivalenceClass *ec, Expr *expr,
Relids relids,
+i = -1;
+while ((i = bms_next_member(expr_relids, i)) >= 0)
+{
+RelOptInfo *rel = root->simple_rel_array[i];
+
+rel->eclass_member_indexes =
bms_add_member(rel->eclass_member_indexes, em_index);
+}
=

On Wed, Aug 9, 2023 at 8:54 PM David Rowley  wrote:
> So, I have three concerns with this patch.

> I think the best way to move this forward is to explore not putting
> partitioned table partitions in EMs and instead see if we can
> translate to top-level parent before lookups.  This might just be too
> complex to translate the Exprs all the time and it may add overhead
> unless we can quickly determine somehow that we don't need to attempt
> to translate the Expr when the given Expr is already from the
> top-level parent. If that can't be made to work, then maybe that shows
> the current patch has merit.

I really appreciate your detailed advice. I am sorry that I will not
be able to respond for a week or two due to my vacation, but I will
explore and work on these issues. Thanks again for your kind reply.

[1] 
https://www.postgresql.org/message-id/CAJ2pMkYR_X-%3Dpq%2B39-W5kc0OG7q9u5YUwDBCHnkPur17DXnxuQ%40mail.gmail.com

-- 
Best regards,
Yuya Watari

Re: A failure in 031_recovery_conflict.pl on Debian/s390x

[Christoph Berg]

Re: To Thomas Munro
> 603 iterations later it hit again, but didn't log anything. (I believe
> I did run "make" in the right directory.)

This time it took 3086 iterations to hit the problem.

Running c27f8621eedf7 + Debian patches + v8 +
pgstat-report-conflicts-immediately.patch + the XXX logging.

> [22:20:24.714](3.145s) # issuing query via background psql:
> # BEGIN;
> # DECLARE test_recovery_conflict_cursor CURSOR FOR SELECT b FROM 
> test_recovery_conflict_table1;
> # FETCH FORWARD FROM test_recovery_conflict_cursor;
> [22:20:24.745](0.031s) ok 1 - buffer pin conflict: cursor with conflicting 
> pin established
> Waiting for replication conn standby's replay_lsn to pass 0/343 on primary
> done
> timed out waiting for match: (?^:User was holding shared buffer pin for too 
> long) at t/031_recovery_conflict.pl line 318.

No XXX lines this time either, but I've seen then im logfiles that
went through successfully.

> Perhaps this can simply be attributed to the machine being too busy.

With the patches, the problem of dying so often that builds targeting
several distributions in parallel will usually fail is gone.

Christoph
2023-08-10 01:00:48.765 UTC [892297] LOG:  starting PostgreSQL 17devel (Debian 
17~~devel-1) on s390x-ibm-linux-gnu, compiled by gcc (Debian 13.2.0-1) 13.2.0, 
64-bit
2023-08-10 01:00:48.765 UTC [892297] LOG:  listening on Unix socket 
"/tmp/5M7Y6uSm5n/.s.PGSQL.63013"
2023-08-10 01:00:48.769 UTC [892300] LOG:  database system was shut down at 
2023-08-10 01:00:48 UTC
2023-08-10 01:00:48.772 UTC [892297] LOG:  database system is ready to accept 
connections
2023-08-10 01:00:48.867 UTC [892305] 031_recovery_conflict.pl LOG:  statement: 
CREATE TABLESPACE test_recovery_conflict_tblspc LOCATION ''
2023-08-10 01:00:48.889 UTC [892307] 031_recovery_conflict.pl LOG:  received 
replication command: SHOW data_directory_mode
2023-08-10 01:00:48.889 UTC [892307] 031_recovery_conflict.pl STATEMENT:  SHOW 
data_directory_mode
2023-08-10 01:00:48.896 UTC [892307] 031_recovery_conflict.pl LOG:  received 
replication command: SHOW wal_segment_size
2023-08-10 01:00:48.896 UTC [892307] 031_recovery_conflict.pl STATEMENT:  SHOW 
wal_segment_size
2023-08-10 01:00:48.901 UTC [892307] 031_recovery_conflict.pl LOG:  received 
replication command: IDENTIFY_SYSTEM
2023-08-10 01:00:48.901 UTC [892307] 031_recovery_conflict.pl STATEMENT:  
IDENTIFY_SYSTEM
2023-08-10 01:00:48.907 UTC [892307] 031_recovery_conflict.pl LOG:  received 
replication command: BASE_BACKUP ( LABEL 'pg_basebackup base backup',  
PROGRESS,  CHECKPOINT 'fast',  WAIT 0,  MANIFEST 'yes',  TARGET 'client')
2023-08-10 01:00:48.907 UTC [892307] 031_recovery_conflict.pl STATEMENT:  
BASE_BACKUP ( LABEL 'pg_basebackup base backup',  PROGRESS,  CHECKPOINT 'fast', 
 WAIT 0,  MANIFEST 'yes',  TARGET 'client')
2023-08-10 01:00:48.913 UTC [892298] LOG:  checkpoint starting: immediate force 
wait
2023-08-10 01:00:48.921 UTC [892298] LOG:  checkpoint complete: wrote 7 buffers 
(5.5%); 0 WAL file(s) added, 0 removed, 1 recycled; write=0.001 s, sync=0.001 
s, total=0.009 s; sync files=0, longest=0.000 s, average=0.000 s; 
distance=11350 kB, estimate=11350 kB; lsn=0/260, redo lsn=0/228
2023-08-10 01:00:48.939 UTC [892308] 031_recovery_conflict.pl LOG:  received 
replication command: SHOW data_directory_mode
2023-08-10 01:00:48.939 UTC [892308] 031_recovery_conflict.pl STATEMENT:  SHOW 
data_directory_mode
2023-08-10 01:00:48.944 UTC [892308] 031_recovery_conflict.pl LOG:  received 
replication command: CREATE_REPLICATION_SLOT "pg_basebackup_892308" TEMPORARY 
PHYSICAL ( RESERVE_WAL)
2023-08-10 01:00:48.944 UTC [892308] 031_recovery_conflict.pl STATEMENT:  
CREATE_REPLICATION_SLOT "pg_basebackup_892308" TEMPORARY PHYSICAL ( RESERVE_WAL)
2023-08-10 01:00:48.950 UTC [892308] 031_recovery_conflict.pl LOG:  received 
replication command: IDENTIFY_SYSTEM
2023-08-10 01:00:48.950 UTC [892308] 031_recovery_conflict.pl STATEMENT:  
IDENTIFY_SYSTEM
2023-08-10 01:00:48.952 UTC [892308] 031_recovery_conflict.pl LOG:  received 
replication command: START_REPLICATION SLOT "pg_basebackup_892308" 0/200 
TIMELINE 1
2023-08-10 01:00:48.952 UTC [892308] 031_recovery_conflict.pl STATEMENT:  
START_REPLICATION SLOT "pg_basebackup_892308" 0/200 TIMELINE 1
2023-08-10 01:00:49.188 UTC [892307] 031_recovery_conflict.pl LOG:  temporary 
file: path "base/pgsql_tmp/pgsql_tmp892307.0", size 137324
2023-08-10 01:00:49.188 UTC [892307] 031_recovery_conflict.pl STATEMENT:  
BASE_BACKUP ( LABEL 'pg_basebackup base backup',  PROGRESS,  CHECKPOINT 'fast', 
 WAIT 0,  MANIFEST 'yes',  TARGET 'client')
2023-08-10 01:00:49.487 UTC [892319] standby LOG:  received replication 
command: IDENTIFY_SYSTEM
2023-08-10 01:00:49.487 UTC [892319] standby STATEMENT:  IDENTIFY_SYSTEM
2023-08-10 01:00:49.493 UTC [892319] standby LOG:  received replication 
command: START_REPLICATION 0/300 TIMELINE 1
2023-08-10 01:00:49.493 UTC [892319] standby STATEMENT:  

Re: Fix pg_stat_reset_single_table_counters function

[Masahiko Sawada]

On Thu, Aug 10, 2023 at 2:10 PM Masahiro Ikeda  wrote:
>
> On 2023-08-01 15:23, Masahiro Ikeda wrote:
> > Hi,
> >
> > My colleague, Mitsuru Hinata (in CC), found the following issue.
> >
> > The documentation of pg_stat_reset_single_table_counters() says
> >> pg_stat_reset_single_table_counters ( oid ) → void
> >> Resets statistics for a single table or index in the current database
> >> or shared across all databases in the cluster to zero.
> >> This function is restricted to superusers by default, but other users
> >> can be granted EXECUTE to run the function.
> > https://www.postgresql.org/docs/devel/monitoring-stats.html
> >
> > But, the stats will not be reset if the table shared across all
> > databases is specified. IIUC, 5891c7a8e seemed to have mistakenly
> > removed the feature implemented in e04267844. What do you think?

Good catch! I've confirmed that the issue has been fixed by your patch.

However, I'm not sure the added regression tests are stable since
autovacuum workers may scan the pg_database and increment the
statistics after resetting the stats. Also, the patch bumps the
CATALOG_VERSION_NO but I don't think it's necessary.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: Support to define custom wait events for extensions

[Michael Paquier]

On Thu, Aug 10, 2023 at 01:08:39PM +0900, Masahiro Ikeda wrote:
> In addition, I change the followings:
> * update about custom wait events in sgml. we don't need to use
>   shmem_startup_hook.
> * change the hash names for readability.
>  (ex. WaitEventExtensionNameHash -> WaitEventExtensionHashById)
> * fix a bug to fail to get already defined events by names
>   because HASH_BLOBS was specified. HASH_STRINGS is right since
>   the key is C strings.

That's what I get based on what ShmemInitHash() relies on.

I got a few more comments about v3.  Overall this looks much better.

This time the ordering of the operations in WaitEventExtensionNew()
looks much better.

+* The entry must be stored because it's registered in
+* WaitEventExtensionNew().
Not sure of the value added by this comment, I would remove it.

+   if (!entry)
+   elog(ERROR, "could not find the name for custom wait event ID %u",
+   eventId);

Or a simpler "could not find custom wait event name for ID %u"?

+static HTAB *WaitEventExtensionHashById;   /* find names from ids */
+static HTAB *WaitEventExtensionHashByName; /* find ids from names */

These names are OK here.

+/* Local variables */
+static uint32 worker_spi_wait_event = 0;
That's a cached value, used as a placeholder for the custom wait event
ID found from the table.

+HASH_ELEM | HASH_STRINGS);   /* key is Null-terminated C strings */
Looks obvious to me based on the code, I would remove this note.

+/* hash table entres */
s/entres/entries/

+   /*
+* Allocate and register a new wait event. But, we need to recheck because
+* other processes could already do so while releasing the lock.
+*/

Could be reworked for the second sentence, like "Recheck if the event
exists, as it could be possible that a concurrent process has inserted
one with the same name while the lock was previously released."

+   /* Recheck */
Duplicate.

/* OK to make connection */
-   conn = libpqsrv_connect(connstr, WAIT_EVENT_EXTENSION);
+   if (wait_event_info == 0)
+   wait_event_info = WaitEventExtensionNew("dblink_get_con");
+   conn = libpqsrv_connect(connstr, wait_event_info);

It is going to be difficult to make that simpler ;)

This looks correct, but perhaps we need to think harder about the
custom event names and define a convention when more of this stuff is
added to the core modules.
--
Michael


signature.asc
Description: PGP signature

[question] difference between vm_extend and fsm_extend

[Junwang Zhao]

Hey hacks,

I'm looking the code of free space map and visibility map, both the module
call  `ExtendBufferedRelTo` to extend its file when necessary, what confused
me is that `vm_extend` has an extra step that send a shared message to force
other backends to close other smgr references.

My question is why does `fsm_extend` not need the invalidation step?

-- 
Regards
Junwang Zhao

Re: Report planning memory in EXPLAIN ANALYZE

[Ashutosh Bapat]

Hi David,

On Wed, Aug 9, 2023 at 8:09 PM Ashutosh Bapat
 wrote:
>
> I need to just make sure that the Planning Memory is reported with SUMMARY ON.
>
The patch reports planning memory in EXPLAIN without ANALYZE when SUMMARY = ON.

#explain (summary on) select * from a, b where a.c1 = b.c1 and a.c1 < b.c2;
  QUERY PLAN
---
 Append  (cost=55.90..245.70 rows=1360 width=24)
   ->  Hash Join  (cost=55.90..119.45 rows=680 width=24)
 Hash Cond: (a_1.c1 = b_1.c1)
 Join Filter: (a_1.c1 < b_1.c2)
 ->  Seq Scan on a_p1 a_1  (cost=0.00..30.40 rows=2040 width=12)
 ->  Hash  (cost=30.40..30.40 rows=2040 width=12)
   ->  Seq Scan on b_p1 b_1  (cost=0.00..30.40 rows=2040 width=12)
   ->  Hash Join  (cost=55.90..119.45 rows=680 width=24)
 Hash Cond: (a_2.c1 = b_2.c1)
 Join Filter: (a_2.c1 < b_2.c2)
 ->  Seq Scan on a_p2 a_2  (cost=0.00..30.40 rows=2040 width=12)
 ->  Hash  (cost=30.40..30.40 rows=2040 width=12)
   ->  Seq Scan on b_p2 b_2  (cost=0.00..30.40 rows=2040 width=12)
 Planning Time: 2.220 ms
 Planning Memory: 124816 bytes
(15 rows)

We are good there.

-- 
Best Wishes,
Ashutosh Bapat

Re: Report planning memory in EXPLAIN ANALYZE

[Ashutosh Bapat]

On Wed, Aug 9, 2023 at 8:56 PM David Rowley  wrote:
>
> On Thu, 10 Aug 2023 at 03:12, Ashutosh Bapat
>  wrote:
> > Thinking more about it, I think memory used is the only right metrics.
> > It's an optimization in MemoryContext implementation that malloc'ed
> > memory is not freed when it is returned using free().
>
> I guess it depends on the problem you're trying to solve. I had
> thought you were trying to do some work to reduce the memory used by
> the planner, so I imagined you wanted this so you could track your
> progress and also to help ensure we don't make too many mistakes in
> the future which makes memory consumption worse again.

Right.

>  For that, I
> imagined you'd want to know how much memory is held to ransom by the
> context with malloc(), not palloc(). Is it really useful to reduce the
> palloc'd memory by the end of planning if it does not reduce the
> malloc'd memory?

AFAIU, the memory freed by the end of planning can be used by later
stages of query processing. The memory malloc'ed during planning can
also be used at the time of execution if it was not palloc'ed or
palloc'ed but pfree'd. So I think it's useful to reduce palloc'ed
memory by the end of planning.



>
> Another way we might go about reducing planner memory is to make
> changes to the allocators themselves.  For example, aset rounds up to
> the next power of 2.  If we decided to do something like add more
> freelists to double the number so we could add a mid-way point
> freelist between each power of 2, then we might find it reduces the
> planner memory by, say 12.5%.  If we just tracked what was consumed by
> palloc() that would appear to save us nothing, but it might actually
> save us several malloced blocks.
>

This won't just affect planner but every subsystem of PostgreSQL, not
just planner, unless we device a new context type for planner.

My point is what's relevant here is how much net memory planner asked
for. Internally how much memory PostgreSQL allocated seems relevant
for the memory context infra as such but not so much for planner
alone.

If you think that memory allocated is important, I will add both used
and (net) allocated memory to EXPLAIN output.

-- 
Best Wishes,
Ashutosh Bapat

Re: [BackendXidGetPid] only access allProcs when xid matches

[Junwang Zhao]

On Thu, Aug 10, 2023 at 4:11 PM Ashutosh Bapat
 wrote:
>
> Please add this to commitfest so that it's not forgotten.
>

Added [1], thanks

[1]: https://commitfest.postgresql.org/44/4495/

> On Wed, Aug 9, 2023 at 8:37 PM Junwang Zhao  wrote:
> >
> > On Wed, Aug 9, 2023 at 10:46 PM Ashutosh Bapat
> >  wrote:
> > >
> > > On Wed, Aug 9, 2023 at 9:30 AM Junwang Zhao  wrote:
> > > >
> > > > In function `BackendXidGetPid`, when looping every proc's
> > > > TransactionId, there is no need to access its PGPROC since there
> > > > is shared memory access: `arrayP->pgprocnos[index]`.
> > > >
> > > > Though the compiler can optimize this kind of inefficiency, I
> > > > believe we should ship with better code.
> > > >
> > >
> > > Looks good to me. However, I would just move the variable declaration
> > > with their assignments inside the if () rather than combing the
> > > expressions. It more readable that way.
> >
> > yeah, make sense, also checked elsewhere using the original style,
> > attachment file
> > keep that style, thanks ;)
> >
> > >
> > > --
> > > Best Wishes,
> > > Ashutosh Bapat
> >
> >
> >
> > --
> > Regards
> > Junwang Zhao
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat



-- 
Regards
Junwang Zhao

Re: Adding a pg_servername() function

[Laetitia Avrot]

Dear Christoph,

Please find my answers below.

Le mer. 9 août 2023 à 22:05, Christoph Moench-Tegeder 
a écrit :

> ## GF (phab...@gmail.com):
>
> And now that I checked it: I do have systems with gethostname()
> returning an FQDN, and other systems return the (short) hostname
> only.


The return of gethostname() depends on what has been configured. So, yes
some people will prefer a FQDN while others will prefer a short hostname.
Also, it's a POSIX standard function (see
https://pubs.opengroup.org/onlinepubs/9699919799/), so I don't get why
getting a FQDN or a short name depending on what people set would be a
problem for Postgres while it's not for Linux.


> And it gets worse when you're talking "container" and
> "automatic image deployment". So I believe it's a good thing when
> a database does not expose too much of the OS below it...
>
>
I respectfully disagree. Containers still have a hostname: for example, by
default, docker uses their container id. (see
https://docs.docker.com/network/). In fact, the Open Container Initiative
defines the hostname as a runtime configuration parameter (see
https://github.com/opencontainers/runtime-spec/blob/main/config.md):

> Hostname
>
>- *hostname* (string, OPTIONAL) specifies the container's hostname as
>seen by processes running inside the container. On Linux, for example, this
>will change the hostname in the container
>
> 
>  UTS
>namespace .
>Depending on your namespace configuration
>
> ,
>the container UTS namespace may be the runtime
>
> 
>  UTS
>namespace .
>
> Example
>
> "hostname": "mrsdalloway"
>
>
Have a nice day,

Lætitia

Re: [BackendXidGetPid] only access allProcs when xid matches

[Ashutosh Bapat]

Please add this to commitfest so that it's not forgotten.

On Wed, Aug 9, 2023 at 8:37 PM Junwang Zhao  wrote:
>
> On Wed, Aug 9, 2023 at 10:46 PM Ashutosh Bapat
>  wrote:
> >
> > On Wed, Aug 9, 2023 at 9:30 AM Junwang Zhao  wrote:
> > >
> > > In function `BackendXidGetPid`, when looping every proc's
> > > TransactionId, there is no need to access its PGPROC since there
> > > is shared memory access: `arrayP->pgprocnos[index]`.
> > >
> > > Though the compiler can optimize this kind of inefficiency, I
> > > believe we should ship with better code.
> > >
> >
> > Looks good to me. However, I would just move the variable declaration
> > with their assignments inside the if () rather than combing the
> > expressions. It more readable that way.
>
> yeah, make sense, also checked elsewhere using the original style,
> attachment file
> keep that style, thanks ;)
>
> >
> > --
> > Best Wishes,
> > Ashutosh Bapat
>
>
>
> --
> Regards
> Junwang Zhao



-- 
Best Wishes,
Ashutosh Bapat

Re: Fix last unitialized memory warning

[Richard Guo]

On Thu, Aug 10, 2023 at 8:57 AM Julien Rouhaud  wrote:

> On Wed, Aug 09, 2023 at 10:29:56AM -0500, Tristan Partin wrote:
> > On Wed Aug 9, 2023 at 10:02 AM CDT, Peter Eisentraut wrote:
> > >
> > > This patch has apparently upset one buildfarm member with a very old
> > > compiler:
> > >
> https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=lapwing=HEAD
> > >
> > > Any thoughts?
> >
> > Best I could find is SO question[0] which links out to[1]. Try this
> patch.
> > Otherwise, a memset() would probably do too.
>
> Yes, it's a buggy warning that came up in the past a few times as I
> recall, for
> which we previously used the {{...}} approach to silence it.


I came across this warning too on one of my VMs, with gcc 4.8.5.  +1 to
silence it with {{...}}.  We did that in d937904 and 6392f2a (and maybe
more).

In case it helps, here is the GCC I'm on.

$ gcc --version
gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44)

Thanks
Richard

Re: Extract numeric [field] in JSONB more effectively

[Andy Fan]

Hi Chap:

  Thanks for the review.


> The minor spelling point, the word 'field' has been spelled
> 'filed' throughout this comment (just as in the email subject):
>
> +   /*
> +* Simplify cast(jsonb_object_filed(jsonb, filedName) as
> type)
> +* to jsonb_object_field_type(jsonb, filedName,
> targetTypeOid);
> +*/
>
>
Thanks for catching this, fixed in v5.


> The question: the simplification is currently being applied
> when the underlying operation uses F_JSONB_OBJECT_FIELD.
> Are there opportunities for a similar benefit if applied
> over F_JSONB_ARRAY_ELEMENT and/or F_JSONB_EXTRACT_PATH?
>

Yes, we do have similar opportunities for both functions.  v5 attached for
this.

-- 
Best Regards
Andy Fan


v5-0001-optimize-casting-jsonb-to-a-given-type.patch
Description: Binary data

Re: Adding a pg_servername() function

[Laetitia Avrot]

Dear Tom,

Thank you for your interest in that patch and for taking the time to point
out several things that need to be better. Please find below my answers.

Le mer. 9 août 2023 à 16:04, Tom Lane  a écrit :

> I actually do object to this, because I think the concept of "server
> name" is extremely ill-defined and if we try to support it, we will
> forever be chasing requests for alternative behaviors.


Yes, that's on me with choosing a poor name. I will go with
pg_gethostname().


> Just to start
> with, is a prospective user expecting a fully-qualified domain name
> or just the base name?  If the machine has several names (perhaps
> associated with different IP addresses), what do you do about that?
> I wouldn't be too surprised if users would expect to get the name
> associated with the IP address that the current connection came
> through.  Or at least they might tell you they want that, until
> they discover they're getting "localhost.localdomain" on loopback
> connections and come right back to bitch about that.
>

If there is a gap between what the function does and the user expectations,
it is my job to write the documentation in a more clear way to set
expectations to the right level and explain precisely what this function is
doing. Again, using a better name as pg_gethostname() will also help to
remove the confusion.


>
> Windows likely adds a whole 'nother set of issues to "what's the
> machine name", but I don't know enough about it to say what.
>

Windows does have a similar gethostname() function (see here:
https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-gethostname
).


>
> I think the upthread suggestion to use cluster_name is going to
> be a superior solution for most people, not least because they
> can use it today and it will work the same regardless of platform.
>

I don't think cluster_name is the same as hostname. True, people could use
that parameter for that usage, but it does not feel right to entertain
confusion between the cluster_name (which, in my humble opinion, should be
different depending on the Postgres cluster) and the answer to "on which
host is this Postgres cluster running?".


> > (*) But we should think about access control for this.  If you're in a
> > DBaaS environment, providers might not like that you can read out their
> > internal host names.
>
> There's that, too.


Of course, this function will need special access and DBaaS providers will
be able to not allow their users to use that function, as they already do
for other features. As I said, the patch is only at the stage of POC, at
the moment.

Le mer. 9 août 2023 à 18:31, Tom Lane  a écrit :

>
>
> One concrete reason why I am doubtful about this is the case of
> multiple PG servers running on the same machine.  gethostname()
> will be unable to distinguish them.
>
>
And that's where my bad name for this function brings confusion. If this
function returns the hostname, then it does seem totally legit and normal
to get the same if 3 Postgres clusters are running on the same host. If
people want to identify their cluster, they should use cluster_name. I
totally agree with that.

Why do I think this is useful?

1- In most companies I've worked for, people responsible for the OS
settings, Network settings, and database settings are different persons.
Also, for most companies I've worked with, maintaining their inventory and
finding out which Postgres cluster is running on which host is still a
difficult thing and error-prone to do. I thought that it could be nice and
useful to display easily for the DBAs on which host the cluster they are
connected to is running so that when they are called at 2 AM, their life
could be a little easier.

2- In addition, as already pointed out, I know that pg_staviz (a monitoring
tool) needs that information and uses this very dirty hack to get it (see
https://github.com/vyruss/pg_statviz/blob/7cd0c694cea40f780fb8b76275c6097b5d210de6/src/pg_statviz/libs/info.py#L30
)

CREATE TEMP TABLE _info(hostname text);
COPY _info FROM PROGRAM 'hostname';

3- Last but not least, as David E. Wheeler had created an extension to do
so for Postgres 9.0+ and I found out a customer who asked me for this
feature, I thought there might be out there more Postgres users who could
find this feature helpful.

I'm sorry if I'm not making myself clear enough about the use cases of that
feature. If you still object to my points, I will simply drop this patch.

Have a nice day,

Lætitia

Re: [PoC] pg_upgrade: allow to upgrade publisher node

[Masahiko Sawada]

On Thu, Aug 10, 2023 at 2:27 PM Amit Kapila  wrote:
>
> On Mon, Aug 7, 2023 at 3:46 PM Amit Kapila  wrote:
> >
> > On Mon, Aug 7, 2023 at 1:06 PM Julien Rouhaud  wrote:
> > >
> > > On Mon, Aug 07, 2023 at 12:42:33PM +0530, Amit Kapila wrote:
> > > > On Mon, Aug 7, 2023 at 11:29 AM Julien Rouhaud  
> > > > wrote:
> > > > >
> > > > > Unless I'm missing something I don't see what prevents something to 
> > > > > connect
> > > > > using the replication protocol and issue any query or even create new
> > > > > replication slots?
> > > > >
> > > >
> > > > I think the point is that if we have any slots where we have not
> > > > consumed the pending WAL (other than the expected like
> > > > SHUTDOWN_CHECKPOINT) or if there are invalid slots then the upgrade
> > > > won't proceed and we will request user to remove such slots or ensure
> > > > that WAL is consumed by slots. So, I think in the case you mentioned,
> > > > the upgrade won't succeed.
> > >
> > > What if new slots are added while the old instance is started in the 
> > > middle of
> > > pg_upgrade, *after* the various checks are done?
> > >
> >
> > They won't be copied but I think that won't be any different than
> > other objects like tables. Anyway, I have another idea which is to not
> > allow creating slots during binary upgrade unless one specifically
> > requests it by having an API like binary_upgrade_allow_slot_create()
> > similar to existing APIs binary_upgrade_*.
> >
>
> Sawada-San, Julien, and others, do you have any thoughts on the above point?

IIUC during the old cluster running in the middle of pg_upgrade it
doesn't accept TCP connections. I'm not sure we need to worry about
the case where someone in the same server attempts to create
replication slots during the upgrade. The same is true for other
objects, as Amit mentioned.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: Support "Right Semi Join" plan shapes

[Richard Guo]

On Tue, Apr 18, 2023 at 5:07 PM Richard Guo  wrote:

> In thread [1] which discussed 'Right Anti Join', Tom once mentioned
> 'Right Semi Join'.  After a preliminary investigation I think it is
> beneficial and can be implemented with very short change.  With 'Right
> Semi Join', what we want to do is to just have the first match for each
> inner tuple.  For HashJoin, after scanning the hash bucket for matches
> to current outer, we just need to check whether the inner tuple has been
> set match and skip it if so.  For MergeJoin, we can do it by avoiding
> restoring inner scan to the marked tuple in EXEC_MJ_TESTOUTER, in the
> case when new outer tuple == marked tuple.
>
> As that thread is already too long, fork a new thread and attach a patch
> used for discussion.  The patch implements 'Right Semi Join' for
> HashJoin.
>

The cfbot reminds that this patch does not apply any more, so rebase it
to v2.

Thanks
Richard


v2-0001-Support-Right-Semi-Join-plan-shapes.patch
Description: Binary data

Re: Adding a pg_servername() function

[Laetitia Avrot]

Le mer. 9 août 2023 à 17:32, GF  a écrit :

>
> Would it be less problematic if the function were called pg_gethostname()?
> @Laetitia: why did you propose that name? maybe to avoid clashes with some
> extension out there?
>
>
I used that name to be kind of coherent with the inet_server_addr(), but I
see now how it is a bad decision and can add confusion and wrong
expectations. I think pg_gethostname() is better.